Re: [PATCH RFC 01/22] arm/cpuhp: Add QMP vcpu params validation support

[Andrew Jones]

On Sat, Jun 13, 2020 at 10:36:08PM +0100, Salil Mehta wrote:
> For now, vcpu hotplug is only supported with single socket single thread,
> single die. NUMA is not supported either and everthing falls into single
> node. Work to properly support these could be taken later once community
> agrees with the base framework changes being presented to support ARM vcpu
> hotplug in QEMU. Hence, these checks.
> 
> Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> ---
>  hw/arm/virt.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 37462a6f78..5d1afdd031 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2201,6 +2201,46 @@ static HotplugHandler 
> *virt_machine_get_hotplug_handler(MachineState *machine,
>      return NULL;
>  }
>  
> +static void virt_smp_parse(MachineState *ms, QemuOpts *opts)
> +{
> +    if (opts) {
> +        unsigned cpus    = qemu_opt_get_number(opts, "cpus", 1);
> +        unsigned sockets = qemu_opt_get_number(opts, "sockets", 1);
> +        unsigned cores   = qemu_opt_get_number(opts, "cores", cpus);
> +        unsigned threads = qemu_opt_get_number(opts, "threads", 1);
> +        unsigned int max_cpus;
> +
> +        if (sockets > 1 || threads > 1) {
> +            error_report("does not support more than one socket or thread");
> +            exit(1);
> +        }
> +
> +        if (cores != cpus) {
> +            error_report("cpu topology: "
> +                         "sockets (%u) * cores (%u) * threads (%u) < "
> +                         "smp_cpus (%u)",
> +                         sockets, cores, threads, cpus);
> +            exit(1);
> +        }
> +
> +        max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
> +        if (sockets * cores * threads > max_cpus) {
> +            error_report("cpu topology: "
> +                         "sockets (%u) * cores (%u) * threads (%u) > "
> +                         "maxcpus (%u)",
> +                         sockets, cores, threads,
> +                         max_cpus);
> +            exit(1);
> +        }
> +
> +        ms->smp.max_cpus = max_cpus;
> +        ms->smp.sockets = sockets;
> +        ms->smp.cpus = cpus;
> +        ms->smp.cores = cores;
> +        ms->smp.threads = threads;
> +    }
> +}
> +
>  /*
>   * for arm64 kvm_type [7-0] encodes the requested number of bits
>   * in the IPA address space
> @@ -2266,6 +2306,7 @@ static void virt_machine_class_init(ObjectClass *oc, 
> void *data)
>      mc->nvdimm_supported = true;
>      mc->auto_enable_numa_with_memhp = true;
>      mc->default_ram_id = "mach-virt.ram";
> +    mc->smp_parse = virt_smp_parse;
>  
>      object_class_property_add(oc, "acpi", "OnOffAuto",
>          virt_get_acpi, virt_set_acpi,
> -- 
> 2.17.1
> 
> 
>

Hi Salil,

This patch and the whole series has inspired me to pick up the vcpu
topology work again. In fact, I think it may be necessary in order
to properly describe a cpu when hot[un]plugging. I'll try to pull
together an RFC soon, at least for TCG. For KVM, we may need to
change KVM in order to allow user-controlled MPIDR. Although I'm
not sure about that anymore, because, as you stated somewhere else,
we already have user-controlled MPIDR to some degree, since KVM simply
transforms the cpu index.

Regarding this patch specifically, I would change this to allow
sockets, but prefer cores (i.e. when only '-smp N' is given, then
N is the number of cores, not sockets). Also I would allow threads,
but only for !kvm_enabled(). Then the function would be similar to
something I think I once posted long ago, or at least wrote and maybe
never posted...

Thanks,
drew