Re: [PATCH 1/1] hw/intc/riscv_aclint:Change the way to get CPUState from hard-base to pu_index

[Igor Mammedov]

On Mon, 13 Nov 2023 04:25:43 +0000
LeoHou <LeoHou@canaan-creative.com> wrote:

> On 9/11/23 23:26, Philippe Mathieu-Daudé  wrote:
>  
> >Hi Leo,
> >
> >First, I can't find your patch in my mailbox, so I'm replying to
> >Dongxue's review.
> >
> >On 9/11/23 03:41, Dongxue Zhang wrote:  
> >> Reviewed-by: Dongxue Zhang <zhangdongxue@canaan-creative.com>
> >> 
> >>   
> >>> On Thu, Nov 9, 2023 at 10:22 AM Leo Hou <LeoHou@canaan-creative.com> 
> >>> wrote:
> >>>
> >>> From: Leo Hou <houyingle@canaan-creative.com>
> >>>
> >>> cpu_by_arch_id() uses hartid-base as the index to obtain the 
> >>> corresponding CPUState structure variable.
> >>> qemu_get_cpu() uses cpu_index as the index to obtain the corresponding 
> >>> CPUState structure variable.
> >>>
> >>> In heterogeneous CPU or multi-socket scenarios, multiple aclint needs to 
> >>> be instantiated,
> >>> and the hartid-base of each cpu bound by aclint can start from 0. If 
> >>> cpu_by_arch_id() is still used
> >>> in this case, all aclint will bind to the earliest initialized hart with 
> >>> hartid-base 0 and cause conflicts.
> >>>
> >>> So with cpu_index as the index, use qemu_get_cpu() to get the CPUState 
> >>> struct variable,
> >>> and connect the aclint interrupt line to the hart of the CPU indexed with 
> >>> cpu_index
> >>> (the corresponding hartid-base can start at 0). It's more reasonable.
> >>>
> >>> Signed-off-by: Leo Hou <houyingle@canaan-creative.com>
> >>> ---
> >>>   hw/intc/riscv_aclint.c | 16 ++++++++--------
> >>>   1 file changed, 8 insertions(+), 8 deletions(-)
> >>>
> >>>diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
> >>> index ab1a0b4b3a..be8f539fcb 100644
> >>> --- a/hw/intc/riscv_aclint.c
> >>> +++ b/hw/intc/riscv_aclint.c
> >>> @@ -130,7 +130,7 @@ static uint64_t riscv_aclint_mtimer_read(void 
> >>> *opaque, hwaddr addr,
> >>>           addr < (mtimer->timecmp_base + (mtimer->num_harts << 3))) {
> >>>           size_t hartid = mtimer->hartid_base +
> >>>                           ((addr - mtimer->timecmp_base) >> 3);
> >>> -        CPUState *cpu = cpu_by_arch_id(hartid);
> >>> +        CPUState *cpu = qemu_get_cpu(hartid);  
> >
> >There is some code smell here. qemu_get_cpu() shouldn't be called by
> >device models, but only by accelerators.  

cpu_index might match arch_id in simple cases but might be not the same
when architecture/topo gets more complex, and just breaks in case of
sparse or out of order created CPUs/intc (hotplug case).
So using it is like a ticking bomb.


> Yes, qemu_get_cpu() is designed to be called by accelerators.
> But there is currently no new API to support multi-socket and
> heterogeneous processor architectures，and sifive_plic has been
> designed with qemu_get_cpu().
> Please refer to:
> [1] 
> https://lore.kernel.org/qemu-devel/1519683480-33201-16-git-send-email-mjc@sifive.com/
> [2] 
> https://lore.kernel.org/qemu-devel/20200825184836.1282371-3-alistair.francis@wdc.com/
> 
> 
> >Maybe the timer should get a link of the hart array it belongs to,
> >and offset to this array base hartid?  
> 
> The same problem exists not only with timer, but also with aclint.
> There needs to be a general approach to this problem.
> 
> 
> >I'm going to
> >NACK
> >this patch until further review / clarifications.  
> 
> Regards,
> 
> Leo Hou.