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

[Alistair Francis]

On Mon, Nov 20, 2023 at 10:17 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> On 13/11/23 05:25, LeoHou 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.

I also can't find the original patch

> >>
> >> 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

This doesn't sound right

cpu_by_arch_id() calls riscv_get_arch_id() to compare against the id.

riscv_get_arch_id() just returns the mhartid.

>From the RISC-V priv spec on mhartid:

Hart IDs might not necessarily be numbered contiguously in a
multiprocessor system, but at least one hart must have a hart ID of
zero. Hart IDs must be unique within the execution environment.

So no two harts should have the same ID. So only a single aclint
should have a hartid-base of 0.

>From a quick look I couldn't see where we set mhartid though, so it's
possible we just don't set it or set it correctly. In which case that
is the bug to be fixed.

Alistair

> >>>> 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.
> >
> > 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.
>
> Right. However since there is no heterogeneous support in QEMU
> at present, we don't need this patch in the next release.
>
> So I'd rather wait and work on a correct fix. Up to the maintainer.
>
> Regards,
>
> Phil.
>
> >> I'm going to
> >> NACK
> >> this patch until further review / clarifications.
> >
> > Regards,
> >
> > Leo Hou.
>
>