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

[Igor Mammedov]

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)?

> >  
> >  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

CCing Peter

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