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

Commit Message

Salil Mehta Sept. 26, 2023, 10:04 a.m. UTC

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 <[e-mail]>
Signed-off-by: Salil Mehta <[e-mail]>
Co-developed-by: Keqian Zhu <[e-mail]>
Signed-off-by: Keqian Zhu <[e-mail]>
Signed-off-by: Salil Mehta <[e-mail]>
---
 hw/arm/virt.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

Comments

Gavin Shan Sept. 28, 2023, 11:57 p.m. UTC | #1

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 <[e-mail]>
> Signed-off-by: Salil Mehta <[e-mail]>
> Co-developed-by: Keqian Zhu <[e-mail]>
> Signed-off-by: Keqian Zhu <[e-mail]>
> Signed-off-by: Salil Mehta <[e-mail]>
> ---
>   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

Salil Mehta Oct. 16, 2023, 11:28 p.m. UTC | #2

Hi Gavin,

> From: Gavin Shan <[e-mail]>
> Sent: Friday, September 29, 2023 12:58 AM
> To: Salil Mehta <[e-mail]>; [e-mail]; [e-mail]
> Cc: [e-mail]; [e-mail]; Jonathan Cameron
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> zhukeqian <[e-mail]>; wangxiongfeng (C)
> <[e-mail]>; wangyanan (Y) <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail]
> Subject: Re: [PATCH RFC V2 23/37] arm/virt: Release objects for *disabled*
> possible vCPUs after init
> 
> 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 <[e-mail]>
> > Signed-off-by: Salil Mehta <[e-mail]>
> > Co-developed-by: Keqian Zhu <[e-mail]>
> > Signed-off-by: Keqian Zhu <[e-mail]>
> > Signed-off-by: Salil Mehta <[e-mail]>
> > ---
> >   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 don’t 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?

For now, this code leg won't hit for TCG, HVE or qtest. These are not supported
in this release. I might enable support of TCG in coming months. Some fixing and
testing is required.

I had created 2 working prototypes earlier in the year 2020. One did not release
the CPU objects. Hence, the ACPI CPU Hotplug state was always in sync with QOM
CPUState. But it required some changes in the qdev creation leg for the reuse of
the existing CPU object - which Igor was not in favor of. Plus, it had some issues
with Live migration (which were left unresolved at that time).  Hence, used the
current approach as the primary one.

Thanks
Salil.

Patch

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));
+            }
+        }
+    }
 }
 
 static void virt_cpu_set_properties(Object *cpuobj, const CPUArchId *cpu_slot,