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

[Andrew Jones]

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
>