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

[Gavin Shan]

On 8/12/24 6:15 PM, Igor Mammedov wrote:
> 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).

Ok, thanks for the hints. It's a question I had from the beginning. I didn't 
dig into
the historic background. From the vCPU hotplug document (cpu-hotplug.rst), the 
CPU
topology is used to identify hot-added vCPU on x86 and it's reasonable for ARM 
to
follow this mechanism.

> > 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

Right, I overlooked the migration case where the source and destination vCPU 
have
to be strictly correlation. This strict correlation can become broken with 
'index'
or 'cpu-index', but it's ensured with the CPU topology as stated on page-19.

Thanks,
Gavin