diff mbox series

[RFC,v3,9/9] hw/arm/virt: Add separate -smp parsing function for ARM machines

Message ID 20210516102900.28036-10-wangyanan55@huawei.com (mailing list archive)
State New, archived
Headers show
Series hw/arm/virt: Introduce cpu topology support | expand

Commit Message

Yanan Wang May 16, 2021, 10:29 a.m. UTC 
The cpu hierarchy topology information parsed out from QEMU -smp
command line will be exposed to guest kernel through ACPI and DT
since machine type 6.1, so we will expect more detailed topology
descriptions and will be more strict with the -smp cmdlines when
parsing them.

Compared with the default smp_parse() where all missing values
are automatically calculated in turn, there is some difference
in ARM specific virt_smp_parse(). The parsing logic is like:
At least one of cpus or maxcpus must be provided. Threads will
default to 1 if not provided (assume not support SMT). Sockets
and cores must be either both provided or both not.

Note, if neither sockets nor cores are provided, we calculate
all the missing values like smp_parse() did before, but will
disable support of exposing these auto-populated descriptions
to guest. Then guest will populate its topology by default.

Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/arm/virt.c   | 95 +++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-options.hx |  4 +++
 2 files changed, 99 insertions(+)

Comments

Andrew Jones May 17, 2021, 8:24 a.m. UTC | #1 
On Sun, May 16, 2021 at 06:29:00PM +0800, Yanan Wang wrote:
> The cpu hierarchy topology information parsed out from QEMU -smp
> command line will be exposed to guest kernel through ACPI and DT
> since machine type 6.1, so we will expect more detailed topology
> descriptions and will be more strict with the -smp cmdlines when
> parsing them.
> 
> Compared with the default smp_parse() where all missing values
> are automatically calculated in turn, there is some difference
> in ARM specific virt_smp_parse(). The parsing logic is like:
> At least one of cpus or maxcpus must be provided. Threads will
> default to 1 if not provided (assume not support SMT). Sockets
> and cores must be either both provided or both not.
> 
> Note, if neither sockets nor cores are provided, we calculate
> all the missing values like smp_parse() did before, but will
> disable support of exposing these auto-populated descriptions
> to guest. Then guest will populate its topology by default.
> 
> Suggested-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/arm/virt.c   | 95 +++++++++++++++++++++++++++++++++++++++++++++++++
>  qemu-options.hx |  4 +++
>  2 files changed, 99 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 50e324975f..44e990e3be 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -76,6 +76,8 @@
>  #include "hw/virtio/virtio-iommu.h"
>  #include "hw/char/pl011.h"
>  #include "qemu/guest-random.h"
> +#include "qapi/qmp/qerror.h"
> +#include "sysemu/replay.h"
>  
>  #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
>      static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
> @@ -2627,6 +2629,98 @@ static int virt_kvm_type(MachineState *ms, const char *type_str)
>      return fixed_ipa ? 0 : requested_pa_size;
>  }
>  
> +/*
> + * virt_smp_parse - Used to parse -smp command line for ARM machines.
> + *
> + * Compared with the default smp_parse where all the missing values
> + * are automatically calculated in turn, in this function, we expect
> + * more detailed topology information provided and are more strict
> + * with the -smp cmdlines when parsing them.
> + *
> + * We require that at least one of cpus or maxcpus must be provided.
> + * Threads will default to 1 if not provided. Sockets and cores must
> + * be either both provided or both not.
> + *
> + * Note, if neither sockets nor cores are specified, we will calculate
> + * all the missing values just like smp_parse() does, but will disable
> + * exposure of cpu topology descriptions to guest.
> + */
> +static void virt_smp_parse(MachineState *ms, QemuOpts *opts)
> +{
> +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(ms);
> +
> +    if (opts) {
> +        unsigned cpus = qemu_opt_get_number(opts, "cpus", 0);
> +        unsigned maxcpus = qemu_opt_get_number(opts, "maxcpus", 0);
> +        unsigned sockets = qemu_opt_get_number(opts, "sockets", 0);
> +        unsigned cores = qemu_opt_get_number(opts, "cores", 0);
> +        unsigned threads = qemu_opt_get_number(opts, "threads", 0);
> +
> +        /* Default threads to 1 if not provided */
> +        threads = threads > 0 ? threads : 1;

Can't do this yet, need to do it later, because...

> +
> +        if (cpus == 0 && maxcpus == 0) {
> +            error_report("at least one of cpus or maxcpus must be provided");
> +            exit(1);
> +        }
> +
> +        if (sockets == 0 && cores == 0) {
> +            /* Disable exposure of topology descriptions to guest */
> +            vmc->no_cpu_topology = true;

...we should do ensure threads == 0 here, otherwise provide another error
message.

> +
> +            /* Compute missing values, prefer sockets over cores */
> +            cores = 1;

Now threads = 1 is good here.

> +            if (cpus == 0) {
> +                sockets = 1;
> +                cpus = sockets * cores * threads;

This should be

  cpus = maxcpus;
  sockets = cpus;

> +            } else {
> +                maxcpus = maxcpus > 0 ? maxcpus : cpus;
> +                sockets = maxcpus / (cores * threads);

We know cores and threads should both be 1 here, so just do

 sockets = maxcpus;

> +            }
> +        } else if (sockets > 0 && cores > 0) {

Now 
        threads = threads > 0 ? threads : 1;

is good here.

> +            cpus = cpus > 0 ? cpus : sockets * cores * threads;
> +            maxcpus = maxcpus > 0 ? maxcpus : cpus;

We should calculate maxcpus first based on the topology,

  maxcpus = maxcpus > 0 ? maxcpus : sockets * cores * threads;
  cpus = cpus > 0 ? cpus : maxcpus;

Please do comprehensive testing to ensure everything works as it
should. You can drop this function into a standalone executable
and run it for all possible inputs, maxcpus=0, maxcpus < cpus, maxcpus ==
cpus, maxcpus > cpus, sockets = 0, sockets < cpus, sockets == cpus, etc.

> +        } else {
> +            error_report("sockets and cores must be both provided "
> +                         "or both not");
> +            exit(1);
> +        }
> +
> +        if (maxcpus < cpus) {
> +            error_report("maxcpus must be equal to or greater than smp");
> +            exit(1);
> +        }
> +
> +        if (sockets * cores * threads < cpus) {
> +            error_report("cpu topology: "
> +                         "sockets (%u) * cores (%u) * threads (%u) < "
> +                         "smp_cpus (%u)",
> +                         sockets, cores, threads, cpus);
> +            exit(1);
> +        }
> +
> +        if (sockets * cores * threads != maxcpus) {
> +            error_report("cpu topology: "
> +                         "sockets (%u) * cores (%u) * threads (%u) "
> +                         "!= maxcpus (%u)",
> +                         sockets, cores, threads, maxcpus);
> +            exit(1);
> +        }
> +
> +        ms->smp.cpus = cpus;
> +        ms->smp.max_cpus = maxcpus;
> +        ms->smp.sockets = sockets;
> +        ms->smp.cores = cores;
> +        ms->smp.threads = threads;
> +    }
> +
> +    if (ms->smp.cpus > 1) {
> +        Error *blocker = NULL;
> +        error_setg(&blocker, QERR_REPLAY_NOT_SUPPORTED, "smp");
> +        replay_add_blocker(blocker);
> +    }
> +}
> +
>  static void virt_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> @@ -2652,6 +2746,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>      mc->cpu_index_to_instance_props = virt_cpu_index_to_props;
>      mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a15");
>      mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
> +    mc->smp_parse = virt_smp_parse;
>      mc->kvm_type = virt_kvm_type;
>      assert(!mc->get_hotplug_handler);
>      mc->get_hotplug_handler = virt_machine_get_hotplug_handler;
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 635dc8a624..bd97086c21 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -203,6 +203,10 @@ SRST
>      computed. If any on the three values is given, the total number of
>      CPUs n can be omitted. maxcpus specifies the maximum number of
>      hotpluggable CPUs.
> +
> +    For the ARM target, at least one of cpus or maxcpus must be provided.
> +    Threads will default to 1 if not provided. Sockets and cores must be
> +    either both provided or both not.
>  ERST
>  
>  DEF("numa", HAS_ARG, QEMU_OPTION_numa,
> -- 
> 2.19.1
>
Yanan Wang May 18, 2021, 2:16 a.m. UTC | #2 
Hi Drew,

On 2021/5/17 16:24, Andrew Jones wrote:
> On Sun, May 16, 2021 at 06:29:00PM +0800, Yanan Wang wrote:
>> The cpu hierarchy topology information parsed out from QEMU -smp
>> command line will be exposed to guest kernel through ACPI and DT
>> since machine type 6.1, so we will expect more detailed topology
>> descriptions and will be more strict with the -smp cmdlines when
>> parsing them.
>>
>> Compared with the default smp_parse() where all missing values
>> are automatically calculated in turn, there is some difference
>> in ARM specific virt_smp_parse(). The parsing logic is like:
>> At least one of cpus or maxcpus must be provided. Threads will
>> default to 1 if not provided (assume not support SMT). Sockets
>> and cores must be either both provided or both not.
>>
>> Note, if neither sockets nor cores are provided, we calculate
>> all the missing values like smp_parse() did before, but will
>> disable support of exposing these auto-populated descriptions
>> to guest. Then guest will populate its topology by default.
>>
>> Suggested-by: Andrew Jones <drjones@redhat.com>
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> ---
>>   hw/arm/virt.c   | 95 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   qemu-options.hx |  4 +++
>>   2 files changed, 99 insertions(+)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 50e324975f..44e990e3be 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -76,6 +76,8 @@
>>   #include "hw/virtio/virtio-iommu.h"
>>   #include "hw/char/pl011.h"
>>   #include "qemu/guest-random.h"
>> +#include "qapi/qmp/qerror.h"
>> +#include "sysemu/replay.h"
>>   
>>   #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
>>       static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
>> @@ -2627,6 +2629,98 @@ static int virt_kvm_type(MachineState *ms, const char *type_str)
>>       return fixed_ipa ? 0 : requested_pa_size;
>>   }
>>   
>> +/*
>> + * virt_smp_parse - Used to parse -smp command line for ARM machines.
>> + *
>> + * Compared with the default smp_parse where all the missing values
>> + * are automatically calculated in turn, in this function, we expect
>> + * more detailed topology information provided and are more strict
>> + * with the -smp cmdlines when parsing them.
>> + *
>> + * We require that at least one of cpus or maxcpus must be provided.
>> + * Threads will default to 1 if not provided. Sockets and cores must
>> + * be either both provided or both not.
>> + *
>> + * Note, if neither sockets nor cores are specified, we will calculate
>> + * all the missing values just like smp_parse() does, but will disable
>> + * exposure of cpu topology descriptions to guest.
>> + */
>> +static void virt_smp_parse(MachineState *ms, QemuOpts *opts)
>> +{
>> +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(ms);
>> +
>> +    if (opts) {
>> +        unsigned cpus = qemu_opt_get_number(opts, "cpus", 0);
>> +        unsigned maxcpus = qemu_opt_get_number(opts, "maxcpus", 0);
>> +        unsigned sockets = qemu_opt_get_number(opts, "sockets", 0);
>> +        unsigned cores = qemu_opt_get_number(opts, "cores", 0);
>> +        unsigned threads = qemu_opt_get_number(opts, "threads", 0);
>> +
>> +        /* Default threads to 1 if not provided */
>> +        threads = threads > 0 ? threads : 1;
> Can't do this yet, need to do it later, because...
>
>> +
>> +        if (cpus == 0 && maxcpus == 0) {
>> +            error_report("at least one of cpus or maxcpus must be provided");
>> +            exit(1);
>> +        }
>> +
>> +        if (sockets == 0 && cores == 0) {
>> +            /* Disable exposure of topology descriptions to guest */
>> +            vmc->no_cpu_topology = true;
> ...we should do ensure threads == 0 here, otherwise provide another error
> message.
>
>> +
>> +            /* Compute missing values, prefer sockets over cores */
>> +            cores = 1;
> Now threads = 1 is good here.
>
>> +            if (cpus == 0) {
>> +                sockets = 1;
>> +                cpus = sockets * cores * threads;
> This should be
>
>    cpus = maxcpus;
>    sockets = cpus;
>
>> +            } else {
>> +                maxcpus = maxcpus > 0 ? maxcpus : cpus;
>> +                sockets = maxcpus / (cores * threads);
> We know cores and threads should both be 1 here, so just do
>
>   sockets = maxcpus;
>
>> +            }
>> +        } else if (sockets > 0 && cores > 0) {
> Now
>          threads = threads > 0 ? threads : 1;
>
> is good here.
>
>> +            cpus = cpus > 0 ? cpus : sockets * cores * threads;
>> +            maxcpus = maxcpus > 0 ? maxcpus : cpus;
> We should calculate maxcpus first based on the topology,
>
>    maxcpus = maxcpus > 0 ? maxcpus : sockets * cores * threads;
>    cpus = cpus > 0 ? cpus : maxcpus;
 From comments above, now I see something was missed by me in
previous discussion in v3. We won't allow that threads is provided
but sockets and cores are not. And cpus should default to maxcpus
if it is not explicitly specified.

I will fix as above suggests.
> Please do comprehensive testing to ensure everything works as it
> should. You can drop this function into a standalone executable
> and run it for all possible inputs, maxcpus=0, maxcpus < cpus, maxcpus ==
> cpus, maxcpus > cpus, sockets = 0, sockets < cpus, sockets == cpus, etc.
Of course, these tests are definitely necessary, will do it after the 
rework.

Thnaks,
Yanan
>> +        } else {
>> +            error_report("sockets and cores must be both provided "
>> +                         "or both not");
>> +            exit(1);
>> +        }
>> +
>> +        if (maxcpus < cpus) {
>> +            error_report("maxcpus must be equal to or greater than smp");
>> +            exit(1);
>> +        }
>> +
>> +        if (sockets * cores * threads < cpus) {
>> +            error_report("cpu topology: "
>> +                         "sockets (%u) * cores (%u) * threads (%u) < "
>> +                         "smp_cpus (%u)",
>> +                         sockets, cores, threads, cpus);
>> +            exit(1);
>> +        }
>> +
>> +        if (sockets * cores * threads != maxcpus) {
>> +            error_report("cpu topology: "
>> +                         "sockets (%u) * cores (%u) * threads (%u) "
>> +                         "!= maxcpus (%u)",
>> +                         sockets, cores, threads, maxcpus);
>> +            exit(1);
>> +        }
>> +
>> +        ms->smp.cpus = cpus;
>> +        ms->smp.max_cpus = maxcpus;
>> +        ms->smp.sockets = sockets;
>> +        ms->smp.cores = cores;
>> +        ms->smp.threads = threads;
>> +    }
>> +
>> +    if (ms->smp.cpus > 1) {
>> +        Error *blocker = NULL;
>> +        error_setg(&blocker, QERR_REPLAY_NOT_SUPPORTED, "smp");
>> +        replay_add_blocker(blocker);
>> +    }
>> +}
>> +
>>   static void virt_machine_class_init(ObjectClass *oc, void *data)
>>   {
>>       MachineClass *mc = MACHINE_CLASS(oc);
>> @@ -2652,6 +2746,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>>       mc->cpu_index_to_instance_props = virt_cpu_index_to_props;
>>       mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a15");
>>       mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
>> +    mc->smp_parse = virt_smp_parse;
>>       mc->kvm_type = virt_kvm_type;
>>       assert(!mc->get_hotplug_handler);
>>       mc->get_hotplug_handler = virt_machine_get_hotplug_handler;
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 635dc8a624..bd97086c21 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -203,6 +203,10 @@ SRST
>>       computed. If any on the three values is given, the total number of
>>       CPUs n can be omitted. maxcpus specifies the maximum number of
>>       hotpluggable CPUs.
>> +
>> +    For the ARM target, at least one of cpus or maxcpus must be provided.
>> +    Threads will default to 1 if not provided. Sockets and cores must be
>> +    either both provided or both not.
>>   ERST
>>   
>>   DEF("numa", HAS_ARG, QEMU_OPTION_numa,
>> -- 
>> 2.19.1
>>
> .
diff mbox series

Patch

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 50e324975f..44e990e3be 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -76,6 +76,8 @@ 
 #include "hw/virtio/virtio-iommu.h"
 #include "hw/char/pl011.h"
 #include "qemu/guest-random.h"
+#include "qapi/qmp/qerror.h"
+#include "sysemu/replay.h"
 
 #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
     static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
@@ -2627,6 +2629,98 @@  static int virt_kvm_type(MachineState *ms, const char *type_str)
     return fixed_ipa ? 0 : requested_pa_size;
 }
 
+/*
+ * virt_smp_parse - Used to parse -smp command line for ARM machines.
+ *
+ * Compared with the default smp_parse where all the missing values
+ * are automatically calculated in turn, in this function, we expect
+ * more detailed topology information provided and are more strict
+ * with the -smp cmdlines when parsing them.
+ *
+ * We require that at least one of cpus or maxcpus must be provided.
+ * Threads will default to 1 if not provided. Sockets and cores must
+ * be either both provided or both not.
+ *
+ * Note, if neither sockets nor cores are specified, we will calculate
+ * all the missing values just like smp_parse() does, but will disable
+ * exposure of cpu topology descriptions to guest.
+ */
+static void virt_smp_parse(MachineState *ms, QemuOpts *opts)
+{
+    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(ms);
+
+    if (opts) {
+        unsigned cpus = qemu_opt_get_number(opts, "cpus", 0);
+        unsigned maxcpus = qemu_opt_get_number(opts, "maxcpus", 0);
+        unsigned sockets = qemu_opt_get_number(opts, "sockets", 0);
+        unsigned cores = qemu_opt_get_number(opts, "cores", 0);
+        unsigned threads = qemu_opt_get_number(opts, "threads", 0);
+
+        /* Default threads to 1 if not provided */
+        threads = threads > 0 ? threads : 1;
+
+        if (cpus == 0 && maxcpus == 0) {
+            error_report("at least one of cpus or maxcpus must be provided");
+            exit(1);
+        }
+
+        if (sockets == 0 && cores == 0) {
+            /* Disable exposure of topology descriptions to guest */
+            vmc->no_cpu_topology = true;
+
+            /* Compute missing values, prefer sockets over cores */
+            cores = 1;
+            if (cpus == 0) {
+                sockets = 1;
+                cpus = sockets * cores * threads;
+            } else {
+                maxcpus = maxcpus > 0 ? maxcpus : cpus;
+                sockets = maxcpus / (cores * threads);
+            }
+        } else if (sockets > 0 && cores > 0) {
+            cpus = cpus > 0 ? cpus : sockets * cores * threads;
+            maxcpus = maxcpus > 0 ? maxcpus : cpus;
+        } else {
+            error_report("sockets and cores must be both provided "
+                         "or both not");
+            exit(1);
+        }
+
+        if (maxcpus < cpus) {
+            error_report("maxcpus must be equal to or greater than smp");
+            exit(1);
+        }
+
+        if (sockets * cores * threads < cpus) {
+            error_report("cpu topology: "
+                         "sockets (%u) * cores (%u) * threads (%u) < "
+                         "smp_cpus (%u)",
+                         sockets, cores, threads, cpus);
+            exit(1);
+        }
+
+        if (sockets * cores * threads != maxcpus) {
+            error_report("cpu topology: "
+                         "sockets (%u) * cores (%u) * threads (%u) "
+                         "!= maxcpus (%u)",
+                         sockets, cores, threads, maxcpus);
+            exit(1);
+        }
+
+        ms->smp.cpus = cpus;
+        ms->smp.max_cpus = maxcpus;
+        ms->smp.sockets = sockets;
+        ms->smp.cores = cores;
+        ms->smp.threads = threads;
+    }
+
+    if (ms->smp.cpus > 1) {
+        Error *blocker = NULL;
+        error_setg(&blocker, QERR_REPLAY_NOT_SUPPORTED, "smp");
+        replay_add_blocker(blocker);
+    }
+}
+
 static void virt_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -2652,6 +2746,7 @@  static void virt_machine_class_init(ObjectClass *oc, void *data)
     mc->cpu_index_to_instance_props = virt_cpu_index_to_props;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a15");
     mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
+    mc->smp_parse = virt_smp_parse;
     mc->kvm_type = virt_kvm_type;
     assert(!mc->get_hotplug_handler);
     mc->get_hotplug_handler = virt_machine_get_hotplug_handler;
diff --git a/qemu-options.hx b/qemu-options.hx
index 635dc8a624..bd97086c21 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -203,6 +203,10 @@  SRST
     computed. If any on the three values is given, the total number of
     CPUs n can be omitted. maxcpus specifies the maximum number of
     hotpluggable CPUs.
+
+    For the ARM target, at least one of cpus or maxcpus must be provided.
+    Threads will default to 1 if not provided. Sockets and cores must be
+    either both provided or both not.
 ERST
 
 DEF("numa", HAS_ARG, QEMU_OPTION_numa,