Re: [PATCH] target/arm: fix MPIDR value for ARM CPUs with SMT

[Dorjoy Chowdhury]

On Sun, Apr 21, 2024 at 11:40 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 4/19/24 11:31, Dorjoy Chowdhury wrote:
> > +
> > +    /*
> > +     * Instantiate a temporary CPU object to build mp_affinity
> > +     * of the possible CPUs.
> > +     */
> > +    cpuobj = object_new(ms->cpu_type);
> > +    armcpu = ARM_CPU(cpuobj);
> > +
> >       for (n = 0; n < ms->possible_cpus->len; n++) {
> >           ms->possible_cpus->cpus[n].type = ms->cpu_type;
> >           ms->possible_cpus->cpus[n].arch_id =
> > -            sbsa_ref_cpu_mp_affinity(sms, n);
> > +            sbsa_ref_cpu_mp_affinity(armcpu, n);
> >           ms->possible_cpus->cpus[n].props.has_thread_id = true;
> >           ms->possible_cpus->cpus[n].props.thread_id = n;
> >       }
> > +
> > +    object_unref(cpuobj);
>
> Why is this instantiation necessary?
>

The instantiation is necessary (like the one in hw/arm/virt.c in
virt_possible_cpu_arch_ids) because the
"sbsa_ref_possible_cpu_arch_ids" function is called (via
possible_cpu_arch_ids) before the CPUs are created in the
"sbsa_ref_init" function. There was some related discussion here
(qemu-devel archive URL):
https://mail.gnu.org/archive/html/qemu-devel/2024-04/msg02074.html . I
picked this up from the same thing done in hw/arm/virt.c in the
"machvirt_init" function in the "if (!vms->memap) {" block.

> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -1314,8 +1314,18 @@ static void arm_cpu_dump_state(CPUState *cs, FILE 
> > *f, int flags)
> >       }
> >   }
> >
> > -uint64_t arm_build_mp_affinity(int idx, uint8_t clustersz)
> > +uint64_t arm_build_mp_affinity(ARMCPU *cpu, int idx, uint8_t clustersz)
> >   {
> > +    if (cpu->has_smt) {
> > +        /*
> > +         * Right now, the ARM CPUs with SMT supported by QEMU only have
> > +         * one thread per core. So Aff0 is always 0.
> > +         */
>
> Well, this isn't true.
>
>      -smp 
> [[cpus=]n][,maxcpus=maxcpus][,drawers=drawers][,books=books][,sockets=sockets]
>                     
> [,dies=dies][,clusters=clusters][,cores=cores][,threads=threads]
>
> I would expect all of Aff[0-3] to be settable with the proper topology 
> parameters.
>

Sorry, maybe I misunderstood. From the gitlab bug ticket (URL:
https://gitlab.com/qemu-project/qemu/-/issues/1608) and the discussion
in qemu-devel (URL:
https://mail.gnu.org/archive/html/qemu-devel/2024-04/msg01964.html) I
looked at the technical reference manuals of the MPIDR register of the
a-55, a-76, a-710, neoverse-v1, neoverse-n1, neoverse-n2 CPUs and all
of them had MT=1 and one thread per core so Aff0 is always 0.

I don't know what Aff3 should be set to. I see this comment in the
target/arm/cpu.c "arm_cpu_realizefn" function
   /* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will
override it.│
     * We don't support setting cluster ID ([16..23]) (known as Aff2
         │
     * in later ARM ARM versions), or any of the higher affinity level
fields,  │
     * so these bits always RAZ.
         │
     */
Right now we don't seem to set Aff3[39:32] at all in the mp_affinity
property. Let me know what should be the expected behavior here as I
don't have enough knowledge here. I will try to fix it. Thanks!

Regards,
Dorjoy