Re: [PATCH V15 0/7] Add architecture agnostic code to support vCPU Hotplug

[Salil Mehta]

Hi Igor,

On 15/07/2024 15:11, Igor Mammedov wrote:
> On Mon, 15 Jul 2024 14:19:12 +0000
> Salil Mehta <salil.mehta@huawei.com> wrote:
>
> > >   From: qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org <qemu-
> > >   arm-bounces+salil.mehta=huawei.com@nongnu.org> On Behalf Of Salil
> > >   Mehta via
> > >   Sent: Monday, July 15, 2024 3:14 PM
> > >   To: Igor Mammedov <imammedo@redhat.com>
> > >   
> > >   Hi Igor,
> > >     
> > >   >  From: Igor Mammedov <imammedo@redhat.com>
> > >   >  Sent: Monday, July 15, 2024 2:55 PM
> > >   >  To: Salil Mehta <salil.mehta@huawei.com>
> > >   >
> > >   >  On Sat, 13 Jul 2024 19:25:09 +0100
> > >   >  Salil Mehta <salil.mehta@huawei.com> wrote:
> > >   >
> > >   >  > [Note: References are present at the last after the revision
> > >   > history]  >  > Virtual CPU hotplug support is being added across
> > >   > various architectures  [1][3].
> > >   >  > This series adds various code bits common across all architectures:
> > >   >  >
> > >   >  > 1. vCPU creation and Parking code refactor [Patch 1] 2. Update ACPI
> > >   > > GED framework to support vCPU Hotplug [Patch 2,3] 3. ACPI CPUs AML
> > >   > > code change [Patch 4,5] 4. Helper functions to support unrealization
> > >   > > of CPU objects [Patch 6,7]
> > >   >
> > >   >  with patch 1 and 3 fixed should be good to go.
> > >   >
> > >   >  Salil,
> > >   >  Can you remind me what happened to migration part of this?
> > >   >  Ideally it should be a part of of this series as it should be common
> > >   > for  everything that uses GED and should be a conditional part of
> > >   > GED's  VMSTATE.
> > >   >
> > >   >  If this series is just a common base and no actual hotplug on top of
> > >   > it is  merged in this release (provided patch 13 is fixed), I'm fine
> > >   > with migration  bits being a separate series on top.
> > >   >
> > >   >  However if some machine would be introducing cpu hotplug in the same
> > >   > release, then the migration part should be merged before it or be a
> > >   > part  that cpu hotplug series.
> > >   
> > >   We have tested Live/Pseudo Migration and it seem to work with the
> > >   changes part of the architecture specific patch-set.
>
> have you tested, migration from new QEMU to an older one (that doesn't have 
> cpuhotplug builtin)?


Just curious, how can we detect at source Qemu what version of the Qemu
destination is running. We require some sort of compatibility check but
then this is a problem not specific to CPU Hotplug?

We  are not initializing CPU Hotplug VMSD in this patch-set. I was
wondering then how can a new machine attempt to migrate VMSD state from 
new Qemu to older Qemu.

ARM vCPU Hotplug patches will be on top of this later in next Qemu cycle.


> > >   
> > >   Ampere: https://lore.kernel.org/all/e17e28ac-28c7-496f-b212-
> > >   2c9b552dbf63@amperemail.onmicrosoft.com/
> > >   Oracle: https://lore.kernel.org/all/46D74D30-EE54-4AD2-8F0E-
> > >   BA5627FAA63E@oracle.com/
> > >   
> > >   
> > >   For ARM, please check below patch part of RFC V3 for changes related to
> > >   migration:
> > >   https://lore.kernel.org/qemu-devel/20240613233639.202896-15-
> > >   salil.mehta@huawei.com/
> >
> >
> > Do you wish to move below change into this path-set and make it common
> > to all instead?
>
> it would be the best to include this with here.
>
> > diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> > index 63226b0040..e92ce07955 100644
> > --- a/hw/acpi/generic_event_device.c
> > +++ b/hw/acpi/generic_event_device.c
> > @@ -333,6 +333,16 @@ static const VMStateDescription vmstate_memhp_state = {
> >       }
> >   };
> >   
> > +static const VMStateDescription vmstate_cpuhp_state = {
> > +    .name = "acpi-ged/cpuhp",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .fields      = (VMStateField[]) {
> > +        VMSTATE_CPU_HOTPLUG(cpuhp_state, AcpiGedState),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> >   static const VMStateDescription vmstate_ged_state = {
> >       .name = "acpi-ged-state",
> >       .version_id = 1,
> > @@ -381,6 +391,7 @@ static const VMStateDescription vmstate_acpi_ged = {
> >       },
> >       .subsections = (const VMStateDescription * const []) {
> >           &vmstate_memhp_state,
> > +        &vmstate_cpuhp_state,
>
> I'm not migration guru but I believe this should be conditional
> to avoid breaking cross-version migration.
> See 679dd1a957d '.needed = vmstate_test_use_cpuhp. part


Sure, thanks for this. As I can see, the needed() function is used at
the source to decide if the state corresponding to a particular device
can be forwarded to the destination QEMU/VM. But how can this be used
to check for cross-version migration?

BTW, I've prepared V16. May I request a quick peek at:

https://github.com/salil-mehta/qemu/commits/virt-cpuhp-armv8/rfc-v3.arch.agnostic.v16/


Above does not have the suggested migration change yet. I can add it as
a separate path


Best regards,
Salil

> CCing Peter
>
> >           &vmstate_ghes_state,
> >           NULL
> >       }
> >
> > Maybe I can add a separate patch for this in the end? Please confirm.
> >
> > Thanks
> > Salil.