| Message ID | 20240613233639.202896-2-salil.mehta@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support of Virtual CPU Hotplug for ARMv8 Arch | expand |
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' 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. > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index 3c93c0c0a6..11fc7fc318 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -2215,6 +2215,14 @@ static void machvirt_init(MachineState *machine) > &error_fatal); > > aarch64 &= object_property_get_bool(cpuobj, "aarch64", NULL); > + object_property_set_int(cpuobj, "socket-id", > + virt_get_socket_id(machine, n), NULL); > + object_property_set_int(cpuobj, "cluster-id", > + virt_get_cluster_id(machine, n), NULL); > + object_property_set_int(cpuobj, "core-id", > + virt_get_core_id(machine, n), NULL); > + object_property_set_int(cpuobj, "thread-id", > + virt_get_thread_id(machine, n), NULL); > > if (!vms->secure) { > object_property_set_bool(cpuobj, "has_el3", false, 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. > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h > index bb486d36b1..6f9a7bb60b 100644 > --- a/include/hw/arm/virt.h > +++ b/include/hw/arm/virt.h > @@ -209,4 +209,32 @@ static inline int virt_gicv3_redist_region_count(VirtMachineState *vms) > vms->highmem_redists) ? 2 : 1; > } > > +static inline int virt_get_socket_id(const MachineState *ms, int cpu_index) > +{ > + assert(cpu_index >= 0 && cpu_index < ms->possible_cpus->len); > + > + return ms->possible_cpus->cpus[cpu_index].props.socket_id; > +} > + > +static inline int virt_get_cluster_id(const MachineState *ms, int cpu_index) > +{ > + assert(cpu_index >= 0 && cpu_index < ms->possible_cpus->len); > + > + return ms->possible_cpus->cpus[cpu_index].props.cluster_id; > +} > + > +static inline int virt_get_core_id(const MachineState *ms, int cpu_index) > +{ > + assert(cpu_index >= 0 && cpu_index < ms->possible_cpus->len); > + > + return ms->possible_cpus->cpus[cpu_index].props.core_id; > +} > + > +static inline int virt_get_thread_id(const MachineState *ms, int cpu_index) > +{ > + assert(cpu_index >= 0 && cpu_index < ms->possible_cpus->len); > + > + return ms->possible_cpus->cpus[cpu_index].props.thread_id; > +} > + > #endif /* QEMU_ARM_VIRT_H */ > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index 77f8c9c748..abc4ed0842 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -2582,6 +2582,10 @@ static Property arm_cpu_properties[] = { > DEFINE_PROP_UINT64("mp-affinity", ARMCPU, > mp_affinity, ARM64_AFFINITY_INVALID), > DEFINE_PROP_INT32("node-id", ARMCPU, node_id, CPU_UNSET_NUMA_NODE_ID), > + DEFINE_PROP_INT32("socket-id", ARMCPU, socket_id, 0), > + DEFINE_PROP_INT32("cluster-id", ARMCPU, cluster_id, 0), > + DEFINE_PROP_INT32("core-id", ARMCPU, core_id, 0), > + DEFINE_PROP_INT32("thread-id", ARMCPU, thread_id, 0), > DEFINE_PROP_INT32("core-count", ARMCPU, core_count, -1), > /* True to default to the backward-compat old CNTFRQ rather than 1Ghz */ > DEFINE_PROP_BOOL("backcompat-cntfrq", ARMCPU, backcompat_cntfrq, false), > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > index c17264c239..208c719db3 100644 > --- a/target/arm/cpu.h > +++ b/target/arm/cpu.h > @@ -1076,6 +1076,10 @@ struct ArchCPU { > QLIST_HEAD(, ARMELChangeHook) el_change_hooks; > > int32_t node_id; /* NUMA node this CPU belongs to */ > + int32_t socket_id; > + int32_t cluster_id; > + int32_t core_id; > + int32_t thread_id; > > /* Used to synchronize KVM and QEMU in-kernel device levels */ > uint8_t device_irq_level; Thanks, Gavin
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). > 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 > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > > index 3c93c0c0a6..11fc7fc318 100644 > > --- a/hw/arm/virt.c > > +++ b/hw/arm/virt.c > > @@ -2215,6 +2215,14 @@ static void machvirt_init(MachineState *machine) > > &error_fatal); > > > > aarch64 &= object_property_get_bool(cpuobj, "aarch64", NULL); > > + object_property_set_int(cpuobj, "socket-id", > > + virt_get_socket_id(machine, n), NULL); > > + object_property_set_int(cpuobj, "cluster-id", > > + virt_get_cluster_id(machine, n), NULL); > > + object_property_set_int(cpuobj, "core-id", > > + virt_get_core_id(machine, n), NULL); > > + object_property_set_int(cpuobj, "thread-id", > > + virt_get_thread_id(machine, n), NULL); > > > > if (!vms->secure) { > > object_property_set_bool(cpuobj, "has_el3", false, 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. > > > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h > > index bb486d36b1..6f9a7bb60b 100644 > > --- a/include/hw/arm/virt.h > > +++ b/include/hw/arm/virt.h > > @@ -209,4 +209,32 @@ static inline int virt_gicv3_redist_region_count(VirtMachineState *vms) > > vms->highmem_redists) ? 2 : 1; > > } > > > > +static inline int virt_get_socket_id(const MachineState *ms, int cpu_index) > > +{ > > + assert(cpu_index >= 0 && cpu_index < ms->possible_cpus->len); > > + > > + return ms->possible_cpus->cpus[cpu_index].props.socket_id; > > +} > > + > > +static inline int virt_get_cluster_id(const MachineState *ms, int cpu_index) > > +{ > > + assert(cpu_index >= 0 && cpu_index < ms->possible_cpus->len); > > + > > + return ms->possible_cpus->cpus[cpu_index].props.cluster_id; > > +} > > + > > +static inline int virt_get_core_id(const MachineState *ms, int cpu_index) > > +{ > > + assert(cpu_index >= 0 && cpu_index < ms->possible_cpus->len); > > + > > + return ms->possible_cpus->cpus[cpu_index].props.core_id; > > +} > > + > > +static inline int virt_get_thread_id(const MachineState *ms, int cpu_index) > > +{ > > + assert(cpu_index >= 0 && cpu_index < ms->possible_cpus->len); > > + > > + return ms->possible_cpus->cpus[cpu_index].props.thread_id; > > +} > > + > > #endif /* QEMU_ARM_VIRT_H */ > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > > index 77f8c9c748..abc4ed0842 100644 > > --- a/target/arm/cpu.c > > +++ b/target/arm/cpu.c > > @@ -2582,6 +2582,10 @@ static Property arm_cpu_properties[] = { > > DEFINE_PROP_UINT64("mp-affinity", ARMCPU, > > mp_affinity, ARM64_AFFINITY_INVALID), > > DEFINE_PROP_INT32("node-id", ARMCPU, node_id, CPU_UNSET_NUMA_NODE_ID), > > + DEFINE_PROP_INT32("socket-id", ARMCPU, socket_id, 0), > > + DEFINE_PROP_INT32("cluster-id", ARMCPU, cluster_id, 0), > > + DEFINE_PROP_INT32("core-id", ARMCPU, core_id, 0), > > + DEFINE_PROP_INT32("thread-id", ARMCPU, thread_id, 0), > > DEFINE_PROP_INT32("core-count", ARMCPU, core_count, -1), > > /* True to default to the backward-compat old CNTFRQ rather than 1Ghz */ > > DEFINE_PROP_BOOL("backcompat-cntfrq", ARMCPU, backcompat_cntfrq, false), > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > > index c17264c239..208c719db3 100644 > > --- a/target/arm/cpu.h > > +++ b/target/arm/cpu.h > > @@ -1076,6 +1076,10 @@ struct ArchCPU { > > QLIST_HEAD(, ARMELChangeHook) el_change_hooks; > > > > int32_t node_id; /* NUMA node this CPU belongs to */ > > + int32_t socket_id; > > + int32_t cluster_id; > > + int32_t core_id; > > + int32_t thread_id; > > > > /* Used to synchronize KVM and QEMU in-kernel device levels */ > > uint8_t device_irq_level; > > Thanks, > Gavin >
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
HI Gavin, Sorry, I was away for almost entire last week. Joined back today. Thanks for taking out time to review. > From: Gavin Shan <gshan@redhat.com> > Sent: Monday, August 12, 2024 5:36 AM > To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org; > qemu-arm@nongnu.org; mst@redhat.com > > 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[]. Correct. 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' Are you suggesting to use CPU index while specifying vCPUs through command line and I'm not even sure how will it simply CPU naming? CPU index is internal index to QOM. The closest thing which you can have is the 'slot-id' and later can have mapping to the CPU index internally but I'm not sure how much useful it is to introduce this slot abstraction. I did raise this in the original RFC I posted in 2020. for a couple of facts: (1) 'cpu-index' > 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. I'm not sure if anybody wants to use it this way. People want to specify topology i.e. where the vCPU fits. Internally it's up to QOM to translate that topology to some index. > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c index > > 3c93c0c0a6..11fc7fc318 100644 > > --- a/hw/arm/virt.c > > +++ b/hw/arm/virt.c > > @@ -2215,6 +2215,14 @@ static void machvirt_init(MachineState > *machine) > > &error_fatal); > > > > aarch64 &= object_property_get_bool(cpuobj, "aarch64", > > NULL); > > + object_property_set_int(cpuobj, "socket-id", > > + virt_get_socket_id(machine, n), NULL); > > + object_property_set_int(cpuobj, "cluster-id", > > + virt_get_cluster_id(machine, n), NULL); > > + object_property_set_int(cpuobj, "core-id", > > + virt_get_core_id(machine, n), NULL); > > + object_property_set_int(cpuobj, "thread-id", > > + virt_get_thread_id(machine, n), > > + NULL); > > > > if (!vms->secure) { > > object_property_set_bool(cpuobj, "has_el3", false, > > 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. 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. > > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h index > > bb486d36b1..6f9a7bb60b 100644 > > --- a/include/hw/arm/virt.h > > +++ b/include/hw/arm/virt.h > > @@ -209,4 +209,32 @@ static inline int > virt_gicv3_redist_region_count(VirtMachineState *vms) > > vms->highmem_redists) ? 2 : 1; > > } > > > > +static inline int virt_get_socket_id(const MachineState *ms, int > > +cpu_index) { > > + assert(cpu_index >= 0 && cpu_index < ms->possible_cpus->len); > > + > > + return ms->possible_cpus->cpus[cpu_index].props.socket_id; > > +} > > + > > +static inline int virt_get_cluster_id(const MachineState *ms, int > > +cpu_index) { > > + assert(cpu_index >= 0 && cpu_index < ms->possible_cpus->len); > > + > > + return ms->possible_cpus->cpus[cpu_index].props.cluster_id; > > +} > > + > > +static inline int virt_get_core_id(const MachineState *ms, int > > +cpu_index) { > > + assert(cpu_index >= 0 && cpu_index < ms->possible_cpus->len); > > + > > + return ms->possible_cpus->cpus[cpu_index].props.core_id; > > +} > > + > > +static inline int virt_get_thread_id(const MachineState *ms, int > > +cpu_index) { > > + assert(cpu_index >= 0 && cpu_index < ms->possible_cpus->len); > > + > > + return ms->possible_cpus->cpus[cpu_index].props.thread_id; > > +} > > + > > #endif /* QEMU_ARM_VIRT_H */ > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c index > > 77f8c9c748..abc4ed0842 100644 > > --- a/target/arm/cpu.c > > +++ b/target/arm/cpu.c > > @@ -2582,6 +2582,10 @@ static Property arm_cpu_properties[] = { > > DEFINE_PROP_UINT64("mp-affinity", ARMCPU, > > mp_affinity, ARM64_AFFINITY_INVALID), > > DEFINE_PROP_INT32("node-id", ARMCPU, node_id, > > CPU_UNSET_NUMA_NODE_ID), > > + DEFINE_PROP_INT32("socket-id", ARMCPU, socket_id, 0), > > + DEFINE_PROP_INT32("cluster-id", ARMCPU, cluster_id, 0), > > + DEFINE_PROP_INT32("core-id", ARMCPU, core_id, 0), > > + DEFINE_PROP_INT32("thread-id", ARMCPU, thread_id, 0), > > DEFINE_PROP_INT32("core-count", ARMCPU, core_count, -1), > > /* True to default to the backward-compat old CNTFRQ rather than > 1Ghz */ > > DEFINE_PROP_BOOL("backcompat-cntfrq", ARMCPU, > backcompat_cntfrq, > > false), diff --git a/target/arm/cpu.h b/target/arm/cpu.h index > > c17264c239..208c719db3 100644 > > --- a/target/arm/cpu.h > > +++ b/target/arm/cpu.h > > @@ -1076,6 +1076,10 @@ struct ArchCPU { > > QLIST_HEAD(, ARMELChangeHook) el_change_hooks; > > > > int32_t node_id; /* NUMA node this CPU belongs to */ > > + int32_t socket_id; > > + int32_t cluster_id; > > + int32_t core_id; > > + int32_t thread_id; > > > > /* Used to synchronize KVM and QEMU in-kernel device levels */ > > uint8_t device_irq_level; > > Thanks, > Gavin >
> 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.
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. 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. IIUC, your granularity is still "thread", so that this filed should be 1. -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.
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.
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. To change these meanings is also possible, but I think it should be based on the actual use case. we can consider the complexity of the implementation when there is a need. > 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. There are some uses for topology, such as it affects scheduling behavior, and it affects feature emulation, etc. > > 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? At least as you said, KVM (for x86) doesn't consider higher-level topologies at the moment, but that's not to say that it won't in the future, as certain registers do have topology dependencies. > and how does these threads map to the QOM vCPU objects or execution context? Each CPU object will create a (software) thread, you can refer the function "kvm_start_vcpu_thread(CPUState *cpu)", which will be called when CPU object realizes. > AFAICS there is nothing but 'CPUState' > which will be made part of the possible vCPU list 'struct CPUArchIdList'. As I said, an example is spapr ("spapr_possible_cpu_arch_ids()"), which maps possible_cpu to core object. However, this is a very specific example, and like Igor's slides said, I understand it's an architectural requirement. > > > > 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. > I don't seem to understand...There is a “threads” parameter in -smp, does this not satisfy your use case? Regards, Zhao
HI Zhao, > 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?). 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. Every property in 'ms->possible_cpus->cpus[n].props should be exactly same as finalized and part of the MachineState::CpuTopology. Hence, number of threads-per-core 'vcpus_count' should not be treated differently. But there is a catch! (I explained that earlier) To change these > meanings is also possible, but I think it should be based on the actual use > case. we can consider the complexity of the implementation when there is a > need. Agreed. There is no ambiguity in the meaning of hardware threads or the virtualized MachineState::CpuTopology. Properties of all the possible vCPUs should exactly be same as part of MachineState. This includes the number of threads-per-core. You mentioned 'vcpus_count' should be 1 but does that mean user can never specify threads > 1 in virtual configuration for x86? > > > 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. > > There are some uses for topology, such as it affects scheduling behavior, > and it affects feature emulation, etc. True. And we should be flexible at the VMM level. We should let Admin of the VMM control how he creates the virtual topology which best fits on the underlying hardware features of the host. This includes, NUMA, sub-NUMA, cores, hardware, threads, cache topology etc. > > > > 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? > > At least as you said, KVM (for x86) doesn't consider higher-level topologies > at the moment, but that's not to say that it won't in the future, as certain > registers do have topology dependencies. sure. so you mean for x86 virtual topology, smp.threads = 1 always? > > > and how does these threads map to the QOM vCPU objects or execution > context? > > Each CPU object will create a (software) thread, you can refer the function > "kvm_start_vcpu_thread(CPUState *cpu)", which will be called when CPU > object realizes. Yes, sure, and each such QOM vCPU thread and 'struct CPUState' is mapped to the lowest granularity of execution specified within the QOM virtual topology. It could be a 'thread' or a 'core'. And all these will run as a KVM vCPU scheduled on some hardware core and maybe hardware thread (if enabled). So there is no difference across architectures regarding this part. I was trying to point that in QOM, even the threads will have their own 'struct CPUState' and each one will be part of the "CPUArchIdList *possible_cpus" maintained at the MachineState. At this level we loose the relationship information of the cores and their corresponding threads (given by 'vcpus_count'). > > AFAICS there is nothing but 'CPUState' > > which will be made part of the possible vCPU list 'struct CPUArchIdList'. > > As I said, an example is spapr ("spapr_possible_cpu_arch_ids()"), which > maps possible_cpu to core object. However, this is a very specific example, > and like Igor's slides said, I understand it's an architectural requirement. I'm sure there must have been some. I'm trying to understand it. Can you share the slides? > > > > > > > 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. > > > > I don't seem to understand...There is a "threads" parameter in -smp, does > this not satisfy your use case? It certainly does. But this is what should get reflected in the 'vcpus_count' as well? Best regards Salil > > Regards, > Zhao >
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
On Wed, Sep 11, 2024 at 11:35 AM Jonathan Cameron < Jonathan.Cameron@huawei.com> wrote: > 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/ Yes, agreed, this last explanation was not completely correct. IICRC, Marc did point out in RFC V1 of June 2020 that value MT=0 is set by KVM to tell the guest kernel that vCPUs at the same affinity-1 fields are independent. Problem was with the interpretation of non-zero MT Bit and it was not consistent. The key thing is we should not rely on the value of the MT Bit in MPIDR to know if multithreading exists. So yes, to know the exact status of the multithreading on ARM systems parsing PPTT Table is the only way. I mentioned that because handling still exists in the kernel code but I think it exists for handling those other cases. Maybe a comment is required here (?): https://elixir.bootlin.com/linux/v6.11-rc7/source/arch/arm64/kernel/topology.c#L34 Thanks for pointing this out. Thanks Salil. > > > Jonathan > >
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 3c93c0c0a6..11fc7fc318 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -2215,6 +2215,14 @@ static void machvirt_init(MachineState *machine) &error_fatal); aarch64 &= object_property_get_bool(cpuobj, "aarch64", NULL); + object_property_set_int(cpuobj, "socket-id", + virt_get_socket_id(machine, n), NULL); + object_property_set_int(cpuobj, "cluster-id", + virt_get_cluster_id(machine, n), NULL); + object_property_set_int(cpuobj, "core-id", + virt_get_core_id(machine, n), NULL); + object_property_set_int(cpuobj, "thread-id", + virt_get_thread_id(machine, n), NULL); if (!vms->secure) { object_property_set_bool(cpuobj, "has_el3", false, 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); diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h index bb486d36b1..6f9a7bb60b 100644 --- a/include/hw/arm/virt.h +++ b/include/hw/arm/virt.h @@ -209,4 +209,32 @@ static inline int virt_gicv3_redist_region_count(VirtMachineState *vms) vms->highmem_redists) ? 2 : 1; } +static inline int virt_get_socket_id(const MachineState *ms, int cpu_index) +{ + assert(cpu_index >= 0 && cpu_index < ms->possible_cpus->len); + + return ms->possible_cpus->cpus[cpu_index].props.socket_id; +} + +static inline int virt_get_cluster_id(const MachineState *ms, int cpu_index) +{ + assert(cpu_index >= 0 && cpu_index < ms->possible_cpus->len); + + return ms->possible_cpus->cpus[cpu_index].props.cluster_id; +} + +static inline int virt_get_core_id(const MachineState *ms, int cpu_index) +{ + assert(cpu_index >= 0 && cpu_index < ms->possible_cpus->len); + + return ms->possible_cpus->cpus[cpu_index].props.core_id; +} + +static inline int virt_get_thread_id(const MachineState *ms, int cpu_index) +{ + assert(cpu_index >= 0 && cpu_index < ms->possible_cpus->len); + + return ms->possible_cpus->cpus[cpu_index].props.thread_id; +} + #endif /* QEMU_ARM_VIRT_H */ diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 77f8c9c748..abc4ed0842 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -2582,6 +2582,10 @@ static Property arm_cpu_properties[] = { DEFINE_PROP_UINT64("mp-affinity", ARMCPU, mp_affinity, ARM64_AFFINITY_INVALID), DEFINE_PROP_INT32("node-id", ARMCPU, node_id, CPU_UNSET_NUMA_NODE_ID), + DEFINE_PROP_INT32("socket-id", ARMCPU, socket_id, 0), + DEFINE_PROP_INT32("cluster-id", ARMCPU, cluster_id, 0), + DEFINE_PROP_INT32("core-id", ARMCPU, core_id, 0), + DEFINE_PROP_INT32("thread-id", ARMCPU, thread_id, 0), DEFINE_PROP_INT32("core-count", ARMCPU, core_count, -1), /* True to default to the backward-compat old CNTFRQ rather than 1Ghz */ DEFINE_PROP_BOOL("backcompat-cntfrq", ARMCPU, backcompat_cntfrq, false), diff --git a/target/arm/cpu.h b/target/arm/cpu.h index c17264c239..208c719db3 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -1076,6 +1076,10 @@ struct ArchCPU { QLIST_HEAD(, ARMELChangeHook) el_change_hooks; int32_t node_id; /* NUMA node this CPU belongs to */ + int32_t socket_id; + int32_t cluster_id; + int32_t core_id; + int32_t thread_id; /* Used to synchronize KVM and QEMU in-kernel device levels */ uint8_t device_irq_level;
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(+)