Re: [PATCH RFC V3 01/29] arm/virt,target/arm: Add new ARMCPU {socket,cluster,core,thread}-id property

[Jonathan Cameron]

On Tue, 10 Sep 2024 12:01:05 +0100
Salil Mehta <salil.mehta@huawei.com> wrote:

> HI Zhao,

A few trivial comments inline.

> 
> >  From: Zhao Liu <zhao1.liu@intel.com>
> >  Sent: Monday, September 9, 2024 4:28 PM
> >  To: Salil Mehta <salil.mehta@huawei.com>
> >  
> >  On Wed, Sep 04, 2024 at 05:37:21PM +0000, Salil Mehta wrote:  
> >  > Date: Wed, 4 Sep 2024 17:37:21 +0000
> >  > From: Salil Mehta <salil.mehta@huawei.com>
> >  > Subject: RE: [PATCH RFC V3 01/29] arm/virt,target/arm: Add new ARMCPU
> >  > {socket,cluster,core,thread}-id property
> >  >
> >  > Hi Zhao,
> >  >  
> >  > >  From: zhao1.liu@intel.com <zhao1.liu@intel.com>
> >  > >  Sent: Wednesday, September 4, 2024 3:43 PM
> >  > >  To: Salil Mehta <salil.mehta@huawei.com>
> >  > >
> >  > >  Hi Salil,
> >  > >
> >  > >  On Mon, Aug 19, 2024 at 11:53:52AM +0000, Salil Mehta wrote:  
> >  > >  > Date: Mon, 19 Aug 2024 11:53:52 +0000  > From: Salil Mehta  
> >  > > <salil.mehta@huawei.com>  > Subject: RE: [PATCH RFC V3 01/29]
> >  > > arm/virt,target/arm: Add new ARMCPU  >
> >  > > {socket,cluster,core,thread}-id property
> >  > >
> >  > >  [snip]
> >  > >  
> >  > >  > >  > NULL); @@ -2708,6 +2716,7 @@ static const CPUArchIdList  > >  
> >  > > *virt_possible_cpu_arch_ids(MachineState *ms)  
> >  > >  > >  >   {
> >  > >  > >  >       int n;
> >  > >  > >  >       unsigned int max_cpus = ms->smp.max_cpus;
> >  > >  > >  > +    unsigned int smp_threads = ms->smp.threads;
> >  > >  > >  >       VirtMachineState *vms = VIRT_MACHINE(ms);
> >  > >  > >  >       MachineClass *mc = MACHINE_GET_CLASS(vms);
> >  > >  > >  >
> >  > >  > >  > @@ -2721,6 +2730,7 @@ static const CPUArchIdList  > >  
> >  > > *virt_possible_cpu_arch_ids(MachineState *ms)  
> >  > >  > >  >       ms->possible_cpus->len = max_cpus;
> >  > >  > >  >       for (n = 0; n < ms->possible_cpus->len; n++) {
> >  > >  > >  >           ms->possible_cpus->cpus[n].type = ms->cpu_type;
> >  > >  > >  > +        ms->possible_cpus->cpus[n].vcpus_count = smp_threads;
> >  > >  > >  >           ms->possible_cpus->cpus[n].arch_id =
> >  > >  > >  >               virt_cpu_mp_affinity(vms, n);
> >  > >  > >  >  
> >  > >  > >
> >  > >  > >  Why @vcpus_count is initialized to @smp_threads? it needs to  
> >  > > be  > > documented in the commit log.  
> >  > >  >
> >  > >  >
> >  > >  > Because every thread internally amounts to a vCPU in QOM and  
> >  > > which is  > in 1:1 relationship with KVM vCPU. AFAIK, QOM does not
> >  > > strictly  > follows any architecture. Once you start to get into
> >  > > details of  > threads there are many aspects of shared resources one
> >  > > will have to  > consider and these can vary across different
> >  > > implementations of  architecture.
> >  > >
> >  > >  For SPAPR CPU, the granularity of >possible_cpus->cpus[] is "core",
> >  > > and for  x86, it's "thread" granularity.  
> >  >
> >  >
> >  > We have threads per-core at microarchitecture level in ARM as well.
> >  > But each thread appears like a vCPU to OS and AFAICS there are no
> >  > special attributes attached to it. SMT can be enabled/disabled at
> >  > firmware and should get reflected in the configuration accordingly
> >  > i.e. value of *threads-per-core* changes between 1 and 'N'.  This
> >  > means 'vcpus_count' has to reflect the correct configuration. But I
> >  > think threads lack proper representation in Qemu QOM.  
> >  
> >  In topology related part, SMT (of x86) usually represents the logical
> >  processor level. And thread has the same meaning.  
> 
> 
> Agreed. It is same in ARM as well. The difference could be in how hardware
> threads are implemented at microarchitecture level.  Nevertheless, we do
> have such virtual configurations, and the meaning of *threads* as-in QOM
> topology (socket,cluster,core,thread) is virtualized similar to the hardware
> threads in host. And One should be able to configure threads support in the
> virtual environment,  regardless whether or not underlying hardware
> supports threads. That's my take.
> 
> Other aspect is how we then expose these threads to the guest. The guest
> kernel (just like host kernel) should gather topology information using
> ACPI PPTT Table (This is ARM specific?). 

Not ARM specific, but not used by x86 in practice (I believe some risc-v boards
use it).
https://lore.kernel.org/linux-riscv/20240617131425.7526-3-cuiyunhui@bytedance.com/

> Later is populated by the Qemu
> (just like by firmware for the host kernel) by making use of the virtual
> topology. ARM guest kernel, in absence of PPTT support can detect
> presence of hardware threads by reading MT Bit within the MPIDR_EL1
> register.

Sadly no it can't.  Lots of CPUs cores that are single thread set that
bit anyway (so it's garbage and PPTT / DT is the only source of truth)
https://lore.kernel.org/all/CAFfO_h7vUEhqV15epf+_zVrbDhc3JrejkkOVsHzHgCXNk+nDdg@mail.gmail.com/T/

Jonathan