Hi Salil,
On 9/26/23 20:04, 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.
Note that we don't have 'vcpu-id' property. It's actually the index to the array
ms->possible_cpus->cpus[] and cpu->cpu_index. Please improve the commit log if
it makes sense.
Co-developed-by: Salil Mehta <salil.me...@huawei.com>
Signed-off-by: Salil Mehta <salil.me...@huawei.com>
Co-developed-by: Keqian Zhu <zhukeqi...@huawei.com>
Signed-off-by: Keqian Zhu <zhukeqi...@huawei.com>
Signed-off-by: Salil Mehta <salil.me...@huawei.com>
---
hw/arm/virt.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++
target/arm/cpu.c | 4 +++
target/arm/cpu.h | 4 +++
3 files changed, 71 insertions(+)
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 7d9dbc2663..57fe97c242 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -221,6 +221,11 @@ static const char *valid_cpus[] = {
ARM_CPU_TYPE_NAME("max"),
};
+static int virt_get_socket_id(const MachineState *ms, int cpu_index);
+static int virt_get_cluster_id(const MachineState *ms, int cpu_index);
+static int virt_get_core_id(const MachineState *ms, int cpu_index);
+static int virt_get_thread_id(const MachineState *ms, int cpu_index);
+
static bool cpu_type_valid(const char *cpu)
{
int i;
@@ -2168,6 +2173,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);
@@ -2652,10 +2665,59 @@ static int64_t virt_get_default_cpu_node_id(const
MachineState *ms, int idx)
return socket_id % ms->numa_state->num_nodes;
}
It seems it's not unnecessary to keep virt_get_{socket, cluster, core,
thread}_id()
because they're called for once. I would suggest to figure out the socket,
cluster,
core and thread ID through @possible_cpus in machvirt_init(), like below.
Besides, we can't always expose property "cluster-id" since cluster in the CPU
topology isn't always supported, seeing MachineClass::smp_props. Some users may
want to hide cluster for unknown reasons. 'cluster-id' shouldn't be exposed in
this case. Otherwise, users may be confused by 'cluster-id' property while it
has been disabled. For example, a VM is started with the following command lines
and 'cluster-id' shouldn't be supported in vCPU hot-add.
-cpu host -smp=maxcpus=2,cpus=1,sockets=2,cores=1,threads=1
(qemu) device_add
host,id=cpu1,socket-id=1,cluster-id=0,core-id=0,thread-id=0
object_property_set_int(cpuobj, "socket-id",
possible_cpus->cpus[i].props.socket_id, NULL);
if (mc->smp_props.cluster_supported && mc->smp_props.has_clusters) {
object_property_set_int(cpuobj, "cluster-id",
possible_cpus->cpus[i].props.cluster_id, NULL);
}
object_property_set_int(cpuobj, "core-id",
possible_cpus->cpus[i].props.core_id, NULL);
object_property_set_int(cpuobj, "thread-id",
possible_cpus->cpus[i].props.thread_id, NULL);
+static 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 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 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 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;
+}
+
+static int
+virt_get_cpu_id_from_cpu_topo(const MachineState *ms, DeviceState *dev)
+{
+ int cpu_id, sock_vcpu_num, clus_vcpu_num, core_vcpu_num;
+ ARMCPU *cpu = ARM_CPU(dev);
+
+ /* calculate total logical cpus across socket/cluster/core */
+ sock_vcpu_num = cpu->socket_id * (ms->smp.threads * ms->smp.cores *
+ ms->smp.clusters);
+ clus_vcpu_num = cpu->cluster_id * (ms->smp.threads * ms->smp.cores);
+ core_vcpu_num = cpu->core_id * ms->smp.threads;
+
+ /* get vcpu-id(logical cpu index) for this vcpu from this topology */
+ cpu_id = (sock_vcpu_num + clus_vcpu_num + core_vcpu_num) + cpu->thread_id;
+
+ assert(cpu_id >= 0 && cpu_id < ms->possible_cpus->len);
+
+ return cpu_id;
+}
+
This function is called for once in PATCH[04/37]. I think it needs to be moved
around to PATCH[04/37].
[PATCH RFC V2 04/37] arm/virt,target/arm: Machine init time change common to
vCPU {cold|hot}-plug
The function name can be shortened because I don't see the suffix
"_from_cpu_topo"
is too much helpful. I think virt_get_cpu_index() would be good enough since
it's
called for once to return the index in array MachineState::possible_cpus::cpus[]
and the return value is stored to CPUState::cpu_index.
static int virt_get_cpu_index(const MachineState *ms, ARMCPU *cpu)
{
int index, cpus_in_socket, cpus_in_cluster, cpus_in_core;
/*
* It's fine to take cluster into account even it's not supported. In this
* case, ms->smp.clusters is always one.
*/
}
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);
@@ -2669,6 +2731,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);
This initialization seems to accomodate HMP command "info hotpluggable-cpus".
It would be nice if it can be mentioned in the commit log.
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 93c28d50e5..1376350416 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2277,6 +2277,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),
DEFINE_PROP_END_OF_LIST()
};
All those 4 properties are used for vCPU hot-add, meaning they're not needed
when vCPU hotplug isn't supported on the specific board. Even for hw/virt board,
cluster isn't always supported and 'cluster-id' shouldn't always be exposed,
as explained above. How about to register the properties dynamically only when
they're needed by vCPU hotplug?
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 88e5accda6..d51d39f621 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1094,6 +1094,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;
It would be fine to keep those fields even the corresponding properties are
dynamically registered, but a little bit memory overhead incurred :)
/* Used to synchronize KVM and QEMU in-kernel device levels */
uint8_t device_irq_level;
Thanks,
Gavin
