Re: [PATCH v1 1/1] hw/riscv: fix PLIC hart topology configuration string when not getting CPUState correctly

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 15/4/25 13:14, Chao Liu wrote:
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
>> Hi,
>>
>> On 15/4/25 12:05, Chao Liu wrote:
>> > riscv_plic_hart_config_string() when getting CPUState via 
>> qemu_get_cpu()
>> > should be consistent with keeping sifive_plic_realize() by
>> > hartid_base + cpu_index.
>> > > For non-numa or single-cluster machines, hartid_base should be 0.
>> > > Also, to ensure that CPUState->cpu_index is set correctly, we need to
>> > update it with the value of mhartid during riscv_hart_realize().
>> > > Signed-off-by: Chao Liu <lc00631@tecorigin.com>
>> > Reviewed-by: zhaoqingze <zqz00548@tecorigin.com>
>> > ---
>> >   hw/riscv/boot.c            | 4 ++--
>> >   hw/riscv/microchip_pfsoc.c | 2 +-
>> >   hw/riscv/riscv_hart.c      | 1 +
>> >   hw/riscv/sifive_u.c        | 5 +++--
>> >   hw/riscv/virt.c            | 2 +-
>> >   include/hw/riscv/boot.h    | 2 +-
>> >   6 files changed, 9 insertions(+), 7 deletions(-)
>> > > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
>> > index 765b9e2b1a..d4c06e7530 100644
>> > --- a/hw/riscv/boot.c
>> > +++ b/hw/riscv/boot.c
>> > @@ -44,13 +44,13 @@ bool riscv_is_32bit(RISCVHartArrayState *harts)
>> >    * Return the per-socket PLIC hart topology configuration string
>> >    * (caller must free with g_free())
>> >    */
>> > -char *riscv_plic_hart_config_string(int hart_count)
>> > +char *riscv_plic_hart_config_string(int hart_base, int hart_count)
>> >   {
>> >       g_autofree const char **vals = g_new(const char *, hart_count 
>> + 1);
>> >       int i;
>> > >       for (i = 0; i < hart_count; i++) {
>> > -        CPUState *cs = qemu_get_cpu(i);
>> > +        CPUState *cs = qemu_get_cpu(hart_base + i);
>> >           CPURISCVState *env = &RISCV_CPU(cs)->env;
>> > >           if (kvm_enabled()) {
>> > diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
>> > index 9c846f9b5b..5269336346 100644
>> > --- a/hw/riscv/microchip_pfsoc.c
>> > +++ b/hw/riscv/microchip_pfsoc.c
>> > @@ -275,7 +275,7 @@ static void 
>> microchip_pfsoc_soc_realize(DeviceState *dev, Error **errp)
>> >                                   l2lim_mem);
>> > >       /* create PLIC hart topology configuration string */
>> > -    plic_hart_config = riscv_plic_hart_config_string(ms->smp.cpus);
>> > +    plic_hart_config = riscv_plic_hart_config_string(0, ms->smp.cpus);
>> > >       /* PLIC */
>> >       s->plic = sifive_plic_create(memmap[MICROCHIP_PFSOC_PLIC].base,
>> > diff --git a/hw/riscv/riscv_hart.c b/hw/riscv/riscv_hart.c
>> > index a55d156668..522e795033 100644
>> > --- a/hw/riscv/riscv_hart.c
>> > +++ b/hw/riscv/riscv_hart.c
>> > @@ -138,6 +138,7 @@ static bool 
>> riscv_hart_realize(RISCVHartArrayState *s, int idx,
>> >       }
>> > >       s->harts[idx].env.mhartid = s->hartid_base + idx;
>> > +    CPU(&s->harts[idx])->cpu_index = s->harts[idx].env.mhartid;
>>
>> Why do we need this particular change? CPUState::cpu_index isn't related
>> to RISC-V HART index, it is meant for the accelerators (KVM, TCG, ...),
>> and shouldn't be used by hw/ code.
>>
>> Otherwise the rest LGTM.
>>
>> Regards,
>>
>> Phil.
> 
> Thanks for the reply, here is an update to cpu_index, mainly for 
> consistency
> with the following:
> 
> static void sifive_plic_realize(DeviceState *dev, Error **errp)
> {
>     ...
> 
>     for (i = 0; i < s->num_harts; i++) {
>         /* this get cpu with hartid_base + i */        RISCVCPU *cpu = 
> RISCV_CPU(qemu_get_cpu(s->hartid_base + i));

OK I see. Pre-existing design issue, qemu_get_cpu() shouldn't exist.
Not your fault. Normally indexed cores belong to some array / cluster.

I'd assume PLIC to be wired to a fixed set of CPUs, not to some
unrelated random index. I'd expect the PLIC model to take either
an CPUCluster as link property, or a set of HART link properties,
then iterates over these internal references. Not to fish it out
elsewhere.

Again, pre-existing and not related to what you try to achieve.


Please add a /* Set CPU index for i.e. sifive_plic_realize() */
before the assignment in riscv_hart_realize(). With that:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Regards,

Phil.

>         if (riscv_cpu_claim_interrupts(cpu, MIP_SEIP) < 0) {
>             error_setg(errp, "SEIP already claimed");
>             return;
>         }
>     }
> 
>     msi_nonbroken = true;
> }
> 
> But when adding the cpu to the global chain table, the cpu_index starts 
> at 0.
> 
> Maybe there is a better way to handle this here?
> 
> For example:
> 
> 1. When updating the cpu_index in the upper level, we can set a base id, 
> and by
> setting this base id, we can update the cpu_index indirectly.
> 
> or
> 
> 2. Modify sifive_plic_realize() to iterate over cpu's to avoid getting them
> by id.
> 
> I haven't thought of a better way to handle this at the moment,
> so we can discuss it further
> 
> -- 
> Regards,
> Chao