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

[Salil Mehta]

>  From: Igor Mammedov <imammedo@redhat.com>
>  Sent: Monday, August 12, 2024 9:16 AM
>  To: Gavin Shan <gshan@redhat.com>
>  
>  On Mon, 12 Aug 2024 14:35:56 +1000
>  Gavin Shan <gshan@redhat.com> wrote:
>  
>  > On 6/14/24 9:36 AM, Salil Mehta wrote:
>  > > This shall be used to store user specified
>  > > topology{socket,cluster,core,thread}
>  > > and shall be converted to a unique 'vcpu-id' which is used as
>  > > slot-index during hot(un)plug of vCPU.
>  > >
>  > > Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
>  > > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>  > > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
>  > > ---
>  > >   hw/arm/virt.c         | 10 ++++++++++
>  > >   include/hw/arm/virt.h | 28 ++++++++++++++++++++++++++++
>  > >   target/arm/cpu.c      |  4 ++++
>  > >   target/arm/cpu.h      |  4 ++++
>  > >   4 files changed, 46 insertions(+)
>  > >
>  >
>  > Those 4 properties are introduced to determine the vCPU's slot, which
>  > is the index to MachineState::possible_cpus::cpus[]. From there, the
>  > CPU object or instance is referenced and then the CPU's state can be
>  > further determined. It sounds reasonable to use the CPU's topology to
>  > determine the index. However, I'm wandering if this can be simplified to
>  use 'cpu-index' or 'index' for a couple of facts: (1) 'cpu-index'
>  
>  Please, don't. We've spent a bunch of time to get rid of cpu-index in user
>  visible interface (well, old NUMA CLI is still there along with 'new' 
> topology
>  based one, but that's the last one).


Agreed. We shouldn't expose CPU index to user.

>  
>  > or 'index' is simplified. Users have to provide 4 parameters in order
>  > to determine its index in the extreme case, for example "device_add
>  > host-arm-cpu, id=cpu7,socket-id=1,
>  > cluster-id=1,core-id=1,thread-id=1". With 'cpu-index' or 'index', it
>  > can be simplified to 'index=7'. (2) The cold-booted and hotpluggable
>  > CPUs are determined by their index instead of their topology. For
>  > example, CPU0/1/2/3 are cold-booted CPUs while CPU4/5/6/7 are
>  hotpluggable CPUs with command lines '-smp maxcpus=8,cpus=4'. So 'index'
>  makes more sense to identify a vCPU's slot.
>  cpu-index have been used for hotplug with x86 machines as a starting point
>  to implement hotplug as it was easy to hack and it has already existed in
>  QEMU.
>  
>  But that didn't scale as was desired and had its own issues.
>  Hence the current interface that majority agreed upon.
>  I don't remember exact arguments anymore (they could be found qemu-
>  devel if needed) Here is a link to the talk that tried to explain why topo
>  based was introduced.
>  
>  http://events17.linuxfoundation.org/sites/events/files/slides/CPU%20Hot-
>  plug%20support%20in%20QEMU.pdf


I think you are referring to slide-19 of above presentation?

Thanks
Salil.