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 <drjo...@redhat.com>
> Signed-off-by: Yanan Wang <wangyana...@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
>
