Re: [Qemu-devel] [PATCH v3 15/19] apic: use apic_id as apic's migration instance_id

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Igor Mammedov (imammedo@redhat.com) wrote:
> instance_id is generated by last_used_id + 1 for a given device type
> so for QEMU with 3 CPUs instance_id for APICs is a seti of [0, 1, 2]
> When CPU in the middle is hot-removed and migration started
> APICs with instance_ids 0 and 2 are transferred in migration stream.
> However target starts with 2 CPUs and APICs' instance_ids are
> generated from scratch [0, 1] hence migration fails with error
>   Unknown savevm section or instance 'apic' 2
> 
> Fix issue by manually registering APIC's vmsd with apic_id as
> instance_id, in this case instance_id on target will always
> match instance_id on source as apic_id is the same for a given
> cpu instance.
> 
> Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

OK, I think this works from the migration side of things.
The old version used the qdev registration but since you didn't
have a bus it was still the simple "apic" naming, so I don't think
that changes.
Since you're tieing it to machine type we never have to worry
about clashes between the old and new numbering.

So, from the migration side of things I think this is about
as simple a fix as we're going to see, although I'm
expecting you're running into similar problems in other devices.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Dave

> ---
>  hw/intc/apic_common.c           | 12 +++++++++++-
>  include/hw/i386/apic_internal.h |  1 +
>  include/hw/i386/pc.h            |  5 +++++
>  3 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index 0bb9ad5..14ac43c 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -294,11 +294,14 @@ static int apic_load_old(QEMUFile *f, void *opaque, int version_id)
>      return 0;
>  }
>  
> +static const VMStateDescription vmstate_apic_common;
> +
>  static void apic_common_realize(DeviceState *dev, Error **errp)
>  {
>      APICCommonState *s = APIC_COMMON(dev);
>      APICCommonClass *info;
>      static DeviceState *vapic;
> +    int instance_id = s->id;
>  
>      info = APIC_COMMON_GET_CLASS(s);
>      info->realize(dev, errp);
> @@ -313,6 +316,11 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
>          info->enable_tpr_reporting(s, true);
>      }
>  
> +    if (s->legacy_instance_id) {
> +        instance_id = -1;
> +    }
> +    vmstate_register_with_alias_id(NULL, instance_id, &vmstate_apic_common,
> +                                   s, -1, 0);
>  }
>  
>  static void apic_common_unrealize(DeviceState *dev, Error **errp)
> @@ -320,6 +328,7 @@ static void apic_common_unrealize(DeviceState *dev, Error **errp)
>      APICCommonState *s = APIC_COMMON(dev);
>      APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
>  
> +    vmstate_unregister(NULL, &vmstate_apic_common, s);
>      info->unrealize(dev, errp);
>  
>      if (apic_report_tpr_access && info->enable_tpr_reporting) {
> @@ -422,6 +431,8 @@ static Property apic_properties_common[] = {
>      DEFINE_PROP_UINT8("version", APICCommonState, version, 0x14),
>      DEFINE_PROP_BIT("vapic", APICCommonState, vapic_control, VAPIC_ENABLE_BIT,
>                      true),
> +    DEFINE_PROP_BOOL("legacy-instance-id", APICCommonState, legacy_instance_id,
> +                     false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -429,7 +440,6 @@ static void apic_common_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
> -    dc->vmsd = &vmstate_apic_common;
>      dc->reset = apic_reset_common;
>      dc->props = apic_properties_common;
>      dc->realize = apic_common_realize;
> diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
> index 52ca45d..a694322 100644
> --- a/include/hw/i386/apic_internal.h
> +++ b/include/hw/i386/apic_internal.h
> @@ -182,6 +182,7 @@ struct APICCommonState {
>      uint32_t vapic_control;
>      DeviceState *vapic;
>      hwaddr vapic_paddr; /* note: persistence via kvmvapic */
> +    bool legacy_instance_id;
>  };
>  
>  typedef struct VAPICState {
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 7e43b20..e2f0426 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -374,6 +374,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>          .driver   = TYPE_X86_CPU,\
>          .property = "cpuid-0xb",\
>          .value    = "off",\
> +    },\
> +    {\
> +        .driver   = "apic",\
> +        .property = "legacy-instance-id",\
> +        .value    = "on",\
>      },
>  
>  #define PC_COMPAT_2_5 \
> -- 
> 2.7.0
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK