Re: [PATCH RFC V2 23/37] arm/virt: Release objects for *disabled* possible vCPUs after init

[Gavin Shan <gshan@redhat.com>]

Hi Salil,

On 9/26/23 20:04, Salil Mehta wrote:
> During machvirt_init(), QOM ARMCPU objects are also pre-created along with the
> corresponding KVM vCPUs in the host for all possible vCPUs. This necessary
> because of the architectural constraint, KVM restricts the deferred creation of
> the KVM vCPUs and VGIC initialization/sizing after VM init. Hence, VGIC is
> pre-sized with possible vCPUs.
> 
> After initialization of the machine is complete disabled possible KVM vCPUs are
> then parked at the per-virt-machine list "kvm_parked_vcpus" and we release the
> QOM ARMCPU objects for the disabled vCPUs. These shall be re-created at the time
> when vCPU is hotplugged again. QOM ARMCPU object is then re-attached with
> corresponding parked KVM vCPU.
> 
> Alternatively, we could've never released the QOM CPU objects and kept on
> reusing. This approach might require some modifications of qdevice_add()
> interface to get old ARMCPU object instead of creating a new one for the hotplug
> request.
> 
> Each of the above approaches come with their own pros and cons. This prototype
> uses the 1st approach.(suggestions are welcome!)
> 
> Co-developed-by: Salil Mehta <salil.mehta@huawei.com>
> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> ---
>   hw/arm/virt.c | 32 ++++++++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index f1bee569d5..3b068534a8 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1965,6 +1965,7 @@ static void virt_cpu_post_init(VirtMachineState *vms, MemoryRegion *sysmem)
>   {
>       CPUArchIdList *possible_cpus = vms->parent.possible_cpus;
>       int max_cpus = MACHINE(vms)->smp.max_cpus;
> +    MachineState *ms = MACHINE(vms);
>       bool aarch64, steal_time;
>       CPUState *cpu;
>       int n;
> @@ -2025,6 +2026,37 @@ static void virt_cpu_post_init(VirtMachineState *vms, MemoryRegion *sysmem)
>               }
>           }
>       }
> +
> +    if (kvm_enabled() || tcg_enabled()) {
> +        for (n = 0; n < possible_cpus->len; n++) {
> +            cpu = qemu_get_possible_cpu(n);
> +
> +            /*
> +             * Now, GIC has been sized with possible CPUs and we dont require
> +             * disabled vCPU objects to be represented in the QOM. Release the
> +             * disabled ARMCPU objects earlier used during init for pre-sizing.
> +             *
> +             * We fake to the guest through ACPI about the presence(_STA.PRES=1)
> +             * of these non-existent vCPUs at VMM/qemu and present these as
> +             * disabled vCPUs(_STA.ENA=0) so that they cant be used. These vCPUs
> +             * can be later added to the guest through hotplug exchanges when
> +             * ARMCPU objects are created back again using 'device_add' QMP
> +             * command.
> +             */
> +            /*
> +             * RFC: Question: Other approach could've been to keep them forever
> +             * and release it only once when qemu exits as part of finalize or
> +             * when new vCPU is hotplugged. In the later old could be released
> +             * for the newly created object for the same vCPU?
> +             */
> +            if (!qemu_enabled_cpu(cpu)) {
> +                CPUArchId *cpu_slot;
> +                cpu_slot = virt_find_cpu_slot(ms, cpu->cpu_index);
> +                cpu_slot->cpu = NULL;
> +                object_unref(OBJECT(cpu));
> +            }
> +        }
> +    }
>   }
>   

Needn't we release those CPU instances for hve and qtest? Besides, I think it's
hard for reuse those objects because they're managed by QOM, which is almost
transparent to us, correct?

>   static void virt_cpu_set_properties(Object *cpuobj, const CPUArchId *cpu_slot,

Thanks,
Gavin