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

[Salil Mehta]

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 Qemu, each vCPU reflects an execution context (which gets uniquely mapped
to KVM vCPU). AFAICS, we only have *CPUState* (Struct ArchCPU) as a placeholder
for this execution context and there is no *ThreadState* (derived out of
Struct CPUState). Hence, we've  to map all the threads as QOM vCPUs. This means
the array of present or possible CPUs represented by 'struct CPUArchIdList' 
contains
all execution contexts which actually might be vCPU or a thread. Hence, usage of
*vcpus_count* seems quite superficial to me frankly.

Also, AFAICS, KVM does not have the concept of the threads and only has
KVM vCPUs, but you are still allowed to specify the topology with sockets, dies,
clusters, cores, threads in most architectures.  


>  
>  And smp.threads means how many threads in one core, so for x86, the
>  vcpus_count of a "thread" is 1, and for spapr, the vcpus_count of a "core"
>  equals to smp.threads.


Sure, but does the KVM specifies this? and how does these threads map to the QOM
vCPU objects or execution context? AFAICS there is nothing but 'CPUState'
which will be made part of the  possible vCPU list 'struct CPUArchIdList'.



>  
>  IIUC, your granularity is still "thread", so that this filed should be 1.


Well, again we need more discussion on this. I've stated my concerns against
doing this. User should be allowed to create virtual topology which will
include 'threads' as one of the parameter.


>  
>  -Zhao
>  
>  > It is a bigger problem than you think, which I've touched at very
>  > nascent stages while doing POC of vCPU hotplug but tried to avoid till now.
>  >
>  >
>  > But I would like to hear other community members views on this.
>  >
>  > Hi Igor/Peter,
>  >
>  > What is your take on this?
>  >
>  > Thanks
>  > Salil.