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

[Richard Henderson <richard.henderson@linaro.org>]

On 4/22/24 04:26, Peter Maydell wrote:
> On Mon, 22 Apr 2024 at 11:46, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Sun, 21 Apr 2024 at 06:40, Richard Henderson
>> <richard.henderson@linaro.org> wrote:
>>>> --- 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.
>>
>> As I understand it the MPIDR value is more or less independent
>> of the topology information as presented to the guest OS.
>> The options to the -smp command set the firmware topology
>> information, which doesn't/shouldn't affect the reported
>> MPIDR values, and in particular shouldn't change whether
>> the CPU selected has the MT bit set or not.
>>
>> For Arm's CPUs they fall into two categories:
>>   * older ones don't set MT in their MPIDR, and the Aff0
>>     field is effectively the CPU number
>>   * newer ones do set MT in their MPIDR, but don't have
>>     SMT, so their Aff0 is always 0 and their Aff1
>>     is the CPU number
>>
>> Of all the CPUs we model, none of them are the
>> architecturally-permitted "MT is set, CPU implements
>> actual SMT, Aff0 indicates the thread in the CPU" type.
> 
> Looking at the TRM, Neoverse-E1 is "MT is set, actual SMT,
> Aff0 is the thread" (Aff0 can be 0 or 1). We just don't
> model that CPU type yet. But we should probably make
> sure we don't block ourselves into a corner where that
> would be awkward -- I'll have a think about this and
> look at what x86 does with the topology info.

I'm suggesting that we set things up per -smp, and if the user chooses a -cpu value for 
which that topology doesn't make sense, we do it anyway and let them keep both pieces.


r~