Re: [PATCH RFC V3 03/29] hw/arm/virt: Limit number of possible vCPUs for unsupported Accel or GIC Type

[Gavin Shan]

On 6/14/24 9:36 AM, Salil Mehta wrote:
> If Virtual CPU Hotplug support does not exist on a particular Accel platform or
> ARM GIC version, we should limit the possible vCPUs to those available during
> boot time (i.e SMP CPUs) and explicitly disable Virtual CPU Hotplug support.
>
> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> ---
>   hw/arm/virt.c | 66 +++++++++++++++++++++++++++++----------------------
>   1 file changed, 38 insertions(+), 28 deletions(-)

Most of the code changes are moving the check between @max_cpus and 
@max_supported_cpus.
It would make the review easier if the code movement can be put into a 
preparatory patch
if you agree.

Thanks,
Gavin

> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 11fc7fc318..3e1c4d2d2f 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2082,8 +2082,6 @@ static void machvirt_init(MachineState *machine)
>       unsigned int smp_cpus = machine->smp.cpus;
>       unsigned int max_cpus = machine->smp.max_cpus;
>   
> -    possible_cpus = mc->possible_cpu_arch_ids(machine);
> -
>       /*
>        * In accelerated mode, the memory map is computed earlier in kvm_type()
>        * to create a VM with the right number of IPA bits.
> @@ -2098,7 +2096,7 @@ static void machvirt_init(MachineState *machine)
>            * we are about to deal with. Once this is done, get rid of
>            * the object.
>            */
> -        cpuobj = object_new(possible_cpus->cpus[0].type);
> +        cpuobj = object_new(machine->cpu_type);
>           armcpu = ARM_CPU(cpuobj);
>   
>           pa_bits = arm_pamax(armcpu);
> @@ -2113,6 +2111,43 @@ static void machvirt_init(MachineState *machine)
>        */
>       finalize_gic_version(vms);
>   
> +    /*
> +     * The maximum number of CPUs depends on the GIC version, or on how
> +     * many redistributors we can fit into the memory map (which in turn
> +     * depends on whether this is a GICv3 or v4).
> +     */
> +    if (vms->gic_version == VIRT_GIC_VERSION_2) {
> +        virt_max_cpus = GIC_NCPU;
> +    } else {
> +        virt_max_cpus = virt_redist_capacity(vms, VIRT_GIC_REDIST);
> +        if (vms->highmem_redists) {
> +            virt_max_cpus += virt_redist_capacity(vms, VIRT_HIGH_GIC_REDIST2);
> +        }
> +    }
> +
> +    if (tcg_enabled() || hvf_enabled() || qtest_enabled() ||
> +        (vms->gic_version < VIRT_GIC_VERSION_3)) {
> +        max_cpus = machine->smp.max_cpus = smp_cpus;
> +        mc->has_hotpluggable_cpus = false;
> +        if (vms->gic_version >= VIRT_GIC_VERSION_3) {
> +            warn_report("cpu hotplug feature has been disabled");
> +        }
> +    }
> +
> +    if (max_cpus > virt_max_cpus) {
> +        error_report("Number of SMP CPUs requested (%d) exceeds max CPUs "
> +                     "supported by machine 'mach-virt' (%d)",
> +                     max_cpus, virt_max_cpus);
> +        if (vms->gic_version != VIRT_GIC_VERSION_2 && !vms->highmem_redists) {
> +            error_printf("Try 'highmem-redists=on' for more CPUs\n");
> +        }
> +
> +        exit(1);
> +    }
> +
> +    /* uses smp.max_cpus to initialize all possible vCPUs */
> +    possible_cpus = mc->possible_cpu_arch_ids(machine);
> +
>       if (vms->secure) {
>           /*
>            * The Secure view of the world is the same as the NonSecure,
> @@ -2147,31 +2182,6 @@ static void machvirt_init(MachineState *machine)
>           vms->psci_conduit = QEMU_PSCI_CONDUIT_HVC;
>       }
>   
> -    /*
> -     * The maximum number of CPUs depends on the GIC version, or on how
> -     * many redistributors we can fit into the memory map (which in turn
> -     * depends on whether this is a GICv3 or v4).
> -     */
> -    if (vms->gic_version == VIRT_GIC_VERSION_2) {
> -        virt_max_cpus = GIC_NCPU;
> -    } else {
> -        virt_max_cpus = virt_redist_capacity(vms, VIRT_GIC_REDIST);
> -        if (vms->highmem_redists) {
> -            virt_max_cpus += virt_redist_capacity(vms, VIRT_HIGH_GIC_REDIST2);
> -        }
> -    }
> -
> -    if (max_cpus > virt_max_cpus) {
> -        error_report("Number of SMP CPUs requested (%d) exceeds max CPUs "
> -                     "supported by machine 'mach-virt' (%d)",
> -                     max_cpus, virt_max_cpus);
> -        if (vms->gic_version != VIRT_GIC_VERSION_2 && !vms->highmem_redists) {
> -            error_printf("Try 'highmem-redists=on' for more CPUs\n");
> -        }
> -
> -        exit(1);
> -    }
> -
>       if (vms->secure && (kvm_enabled() || hvf_enabled())) {
>           error_report("mach-virt: %s does not support providing "
>                        "Security extensions (TrustZone) to the guest CPU",