RE: [PATCH RFC V3 17/29] arm/virt: Release objects for *disabled* possible vCPUs after init

[Salil Mehta]

>  From: Gavin Shan <gshan@redhat.com>
>  Sent: Tuesday, August 20, 2024 1:06 AM
>  To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org;
>  qemu-arm@nongnu.org; mst@redhat.com
>  
>  Hi Salil,
>  
>  On 8/19/24 10:21 PM, Salil Mehta wrote:
>  >>   From: Gavin Shan <gshan@redhat.com>
>  >>   Sent: Tuesday, August 13, 2024 2:17 AM
>  >>   To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org;
>  >>   qemu-arm@nongnu.org; mst@redhat.com
>  >>
>  >>   On 6/14/24 9:36 AM, Salil Mehta wrote:
>  >>   > During `machvirt_init()`, QOM ARMCPU objects are pre-created along
>  >>   > with the corresponding KVM vCPUs in the host for all possible vCPUs.
>  >>   > This is necessary due to the architectural constraint that KVM
>  >>   > restricts the deferred creation of KVM vCPUs and VGIC
>  >>   > initialization/sizing after VM initialization. Hence, VGIC is 
> pre-sized
>  with
>  >>   possible vCPUs.
>  >>   >
>  >>   > After the initialization of the machine is complete, the disabled
>  >>   > possible KVM vCPUs are parked in the per-virt-machine list
>  >>   > "kvm_parked_vcpus," and we release the QOM ARMCPU objects for
>  the
>  >>   > disabled vCPUs. These will be re-created when the vCPU is
>  hotplugged
>  >>   > again. The QOM ARMCPU object is then re-attached to the
>  corresponding
>  >>   parked KVM vCPU.
>  >>   >
>  >>   > Alternatively, we could have chosen not to release the QOM CPU
>  objects
>  >>   > and kept reusing them. This approach might require some
>  modifications
>  >>   > to the `qdevice_add()` interface to retrieve the old ARMCPU object
>  >>   > instead of creating a new one for the hotplug request.
>  >>   >
>  >>   > Each of these approaches has its own pros and cons. This prototype
>  >>   > uses the first approach (suggestions are welcome!).
>  >>   >
>  >>   > 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
>  >>   > 9d33f30a6a..a72cd3b20d 100644
>  >>   > --- a/hw/arm/virt.c
>  >>   > +++ b/hw/arm/virt.c
>  >>   > @@ -2050,6 +2050,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;
>  >>   > @@ -2111,6 +2112,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));
>  >>   > +            }
>  >>   > +        }
>  >>   > +    }
>  >>   >   }
>  >>
>  >>   It's probably hard to keep those ARMCPU objects forever. First of all,
>  one
>  >>   vCPU can be hot-added first and then hot-removed afterwards. With
>  those
>  >>   ARMCPU objects kept forever, the syntax of 'device_add' and
>  'device_del'
>  >>   become broken at least.
>  >
>  > I had prototyped both approaches 4 years back. Yes, interface problem
>  > with device_add was solved by a trick of keeping the old vCPU object
>  > and on device_add instead of creating a new vCPU object we could use
>  > the old vCPU object and then call qdev_realize() on it.
>  >
>  > But bigger problem with this approach is that of migration. Only
>  > realized objects have state to be migrated. So it might look cleaner
>  > on one aspect but had its own issues.
>  >
>  > I think I did share a prototype of this with Igor which he was not in
>  > agreement with and wanted vCPU objects to be destroyed like in x86.
>  > Hence, we stuck with the current approach.
>  >
>  
>  Migration needn't to be the blocker necessarily. For those states of vCPUs,
>  which have been instantiated and not realized yet, those states can be
>  moved around to be managed under another migratable object (e.g.
>  MachineState). In this way, those vCPU states can be migrated together
>  with MachineState. 


Migration was a blocker., I'll have to muddle my head again to gain back every
context of that which at this juncture seems unnecessary.


However, it sounds strange to me to have vCPU objects
>  instantiated, but unrealized until hot-add is triggered.
>  It's not what QOM supposes to support.


Yes, but for user it does not matter as it is an internal implementation and
we don’t have to expose that to the external user either. Just a representation.


>  
>  Ok, I don't realize x86 also follows this model: instantiate hotpluggable
>  vCPUs and destroy them on bootup.


Correct, so we have now kept it consistent with x86 but of course we have
fake states for ARM arch when vCPUs don’t exist and this state Is not migrated.
ACPI persistent state gets initialized at the start by the architecture 
specific code.
Hence, what vCPU state we expose to kernel during Qemu init and hot(un)plug
operations varies.


>  
>  >
>  >>   The ideal mechanism would be to avoid instanciating those ARMCPU objects
>  >>   and destroying them soon. I don't know if ms->possible_cpus->cpus[] can
>  >>   fit and how much efforts needed.
>  >
>  > This is what we are doing now in the current approach. Please read the
>  > KVMForum slides of 2023 for more details and the cover letter of RFC V3
>  for more details.
>  >
>  
>  My question has been parsed in a wrong way. 

Oh. 😊

My question was if it's doable
>  to avoid instantiating vCPU#1 on bootup when we have command lines '-
>  cpu host -smp cpus=1,max_cpus=2'
>  and release it in machvirt_init(). Instead, ms->possible_cpus->cpus[] are
>  reused to create
>  GICv3 and vCPU file descriptors.


We create QOM vCPU objects to initialize KVM vCPU objects in host kernel.
and only permanently populate the 'possible_cpus' list for the realized vCPUs.
We release vCPU objects for other vCPUs which will be hot-plugged in future. 
and only make then part of 'possible_cpus' list after they get plugged and
realized.


 It looks like a clean way at least. There may
>  be significant efforts to reuse mc->possible_cpus->cpus[], but vCPU#1
>  object has ephemeral life cycle.
>  It looks unnecessary to create the ephemeral vCPU#1 object.

I don’t understand this clearly. Are  you suggesting to reuse only single
vCPU object to initialize all KVM vCPUs not yet plugged? If yes, then
I'm not sure what do we gain here by adding this complexity? It does
not consume time or resources because we are not realizing any of these
vCPU object in any case. 

Thanks
Salil.


>  
>  Thanks,
>  Gavin
>