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

[LeoHou]

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.

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.