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

[Philippe Mathieu-Daudé]

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.

Maybe the timer should get a link of the hart array it belongs to,
and offset to this array base hartid?

I'm going to
NACK
this patch until further review / clarifications.

Regards,

Phil.