[PATCH] acpi: Bodge acpi_index migration

[PATCH] acpi: Bodge acpi_index migration

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

The 'acpi_index' field is a statically configured field, which for
some reason is migrated; this never makes much sense because it's
command line static.

However, on piix4 it's conditional, and the condition/test function
ends up having the wrong pointer passed to it (it gets a PIIX4PMState
not the AcpiPciHpState it was expecting, because VMSTATE_PCI_HOTPLUG
is a macro and not another struct).  This means the field is randomly
loaded/saved based on a random pointer.  In 6.x this random pointer
randomly seems to get 0 for everyone (!); in 7.0rc it's getting junk
and trying to load a field that the source didn't send.  The migration
stream gets out of line and hits the section footer.

The bodge is on piix4 never to load the field:
  a) Most 6.x builds never send it, so most of the time the migration
    will work.
  b) We can backport this fix to 6.x to remove the boobytrap.
  c) It should never have made a difference anyway since the acpi-index
    is command line configured and should be correct on the destination
    anyway
  d) ich9 is still sending/receiving this (unconditionally all the time)
    but due to (c) should never notice.  We could follow up to make it
    skip.

It worries me just when (a) actually happens.

Fixes: b32bd76 ("pci: introduce acpi-index property for PCI device")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/932

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/acpi/acpi-pci-hotplug-stub.c |  4 ----
 hw/acpi/pcihp.c                 |  6 ------
 hw/acpi/piix4.c                 | 11 ++++++++++-
 include/hw/acpi/pcihp.h         |  2 --
 4 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/hw/acpi/acpi-pci-hotplug-stub.c b/hw/acpi/acpi-pci-hotplug-stub.c
index 734e4c5986..a43f6dafc9 100644
--- a/hw/acpi/acpi-pci-hotplug-stub.c
+++ b/hw/acpi/acpi-pci-hotplug-stub.c
@@ -41,7 +41,3 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
     return;
 }
 
-bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
-{
-    return false;
-}
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 6351bd3424..bf65bbea49 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -554,12 +554,6 @@ void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
                                    OBJ_PROP_FLAG_READ);
 }
 
-bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
-{
-     AcpiPciHpState *s = opaque;
-     return s->acpi_index;
-}
-
 const VMStateDescription vmstate_acpi_pcihp_pci_status = {
     .name = "acpi_pcihp_pci_status",
     .version_id = 1,
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index cc37fa3416..48aeedd5f0 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -267,6 +267,15 @@ static bool piix4_vmstate_need_smbus(void *opaque, int version_id)
     return pm_smbus_vmstate_needed();
 }
 
+/*
+ * This is a fudge to turn off the acpi_index field, whose
+ * test was always broken on piix4.
+ */
+static bool vmstate_test_never(void *opaque, int version_id)
+{
+    return false;
+}
+
 /* qemu-kvm 1.2 uses version 3 but advertised as 2
  * To support incoming qemu-kvm 1.2 migration, change version_id
  * and minimum_version_id to 2 below (which breaks migration from
@@ -297,7 +306,7 @@ static const VMStateDescription vmstate_acpi = {
             struct AcpiPciHpPciStatus),
         VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState,
                             vmstate_test_use_acpi_hotplug_bridge,
-                            vmstate_acpi_pcihp_use_acpi_index),
+                            vmstate_test_never),
         VMSTATE_END_OF_LIST()
     },
     .subsections = (const VMStateDescription*[]) {
diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
index af1a169fc3..7e268c2c9c 100644
--- a/include/hw/acpi/pcihp.h
+++ b/include/hw/acpi/pcihp.h
@@ -73,8 +73,6 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off);
 
 extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
 
-bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id);
-
 #define VMSTATE_PCI_HOTPLUG(pcihp, state, test_pcihp, test_acpi_index) \
         VMSTATE_UINT32_TEST(pcihp.hotplug_select, state, \
                             test_pcihp), \
-- 
2.35.1

Re: [PATCH] acpi: Bodge acpi_index migration

[Igor Mammedov]

On Wed,  6 Apr 2022 09:35:31 +0100
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> The 'acpi_index' field is a statically configured field, which for
> some reason is migrated; this never makes much sense because it's
> command line static.

that's true only for the field that's part of PCIDEvice,
however AcpiPciHpState::acpi_index is runtime state and _must_
be migrated if set, otherwise guest might get wrong index
if it's in process of querying it

 
> However, on piix4 it's conditional, and the condition/test function
> ends up having the wrong pointer passed to it (it gets a PIIX4PMState
> not the AcpiPciHpState it was expecting, because VMSTATE_PCI_HOTPLUG
> is a macro and not another struct).  This means the field is randomly
> loaded/saved based on a random pointer.  In 6.x this random pointer
> randomly seems to get 0 for everyone (!); in 7.0rc it's getting junk
> and trying to load a field that the source didn't send.  The migration
> stream gets out of line and hits the section footer.

I'm a bit confused by description,
do you have a reproducer for me to try?

> The bodge is on piix4 never to load the field:
>   a) Most 6.x builds never send it, so most of the time the migration
>     will work.
>   b) We can backport this fix to 6.x to remove the boobytrap.
>   c) It should never have made a difference anyway since the acpi-index
>     is command line configured and should be correct on the destination
>     anyway
>   d) ich9 is still sending/receiving this (unconditionally all the time)
>     but due to (c) should never notice.  We could follow up to make it
>     skip.
> 
> It worries me just when (a) actually happens.
> 
> Fixes: b32bd76 ("pci: introduce acpi-index property for PCI device")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/932
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  hw/acpi/acpi-pci-hotplug-stub.c |  4 ----
>  hw/acpi/pcihp.c                 |  6 ------
>  hw/acpi/piix4.c                 | 11 ++++++++++-
>  include/hw/acpi/pcihp.h         |  2 --
>  4 files changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/acpi/acpi-pci-hotplug-stub.c b/hw/acpi/acpi-pci-hotplug-stub.c
> index 734e4c5986..a43f6dafc9 100644
> --- a/hw/acpi/acpi-pci-hotplug-stub.c
> +++ b/hw/acpi/acpi-pci-hotplug-stub.c
> @@ -41,7 +41,3 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
>      return;
>  }
>  
> -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
> -{
> -    return false;
> -}
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 6351bd3424..bf65bbea49 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -554,12 +554,6 @@ void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
>                                     OBJ_PROP_FLAG_READ);
>  }
>  
> -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
> -{
> -     AcpiPciHpState *s = opaque;
> -     return s->acpi_index;
> -}
> -
>  const VMStateDescription vmstate_acpi_pcihp_pci_status = {
>      .name = "acpi_pcihp_pci_status",
>      .version_id = 1,
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index cc37fa3416..48aeedd5f0 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -267,6 +267,15 @@ static bool piix4_vmstate_need_smbus(void *opaque, int version_id)
>      return pm_smbus_vmstate_needed();
>  }
>  
> +/*
> + * This is a fudge to turn off the acpi_index field, whose
> + * test was always broken on piix4.
> + */
> +static bool vmstate_test_never(void *opaque, int version_id)
> +{
> +    return false;
> +}
> +
>  /* qemu-kvm 1.2 uses version 3 but advertised as 2
>   * To support incoming qemu-kvm 1.2 migration, change version_id
>   * and minimum_version_id to 2 below (which breaks migration from
> @@ -297,7 +306,7 @@ static const VMStateDescription vmstate_acpi = {
>              struct AcpiPciHpPciStatus),
>          VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState,
>                              vmstate_test_use_acpi_hotplug_bridge,
> -                            vmstate_acpi_pcihp_use_acpi_index),
> +                            vmstate_test_never),
>          VMSTATE_END_OF_LIST()
>      },
>      .subsections = (const VMStateDescription*[]) {
> diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> index af1a169fc3..7e268c2c9c 100644
> --- a/include/hw/acpi/pcihp.h
> +++ b/include/hw/acpi/pcihp.h
> @@ -73,8 +73,6 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off);
>  
>  extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
>  
> -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id);
> -
>  #define VMSTATE_PCI_HOTPLUG(pcihp, state, test_pcihp, test_acpi_index) \
>          VMSTATE_UINT32_TEST(pcihp.hotplug_select, state, \
>                              test_pcihp), \

Re: [PATCH] acpi: Bodge acpi_index migration

[Dr. David Alan Gilbert]

* Igor Mammedov (imammedo@redhat.com) wrote:
> On Wed,  6 Apr 2022 09:35:31 +0100
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > The 'acpi_index' field is a statically configured field, which for
> > some reason is migrated; this never makes much sense because it's
> > command line static.
> 
> that's true only for the field that's part of PCIDEvice,
> however AcpiPciHpState::acpi_index is runtime state and _must_
> be migrated if set, otherwise guest might get wrong index
> if it's in process of querying it

So this patch only changes the piix4.c version; I'm confused, is there
a AcpiPciHpState::acpi_index that's runtime setable in there?

>  
> > However, on piix4 it's conditional, and the condition/test function
> > ends up having the wrong pointer passed to it (it gets a PIIX4PMState
> > not the AcpiPciHpState it was expecting, because VMSTATE_PCI_HOTPLUG
> > is a macro and not another struct).  This means the field is randomly
> > loaded/saved based on a random pointer.  In 6.x this random pointer
> > randomly seems to get 0 for everyone (!); in 7.0rc it's getting junk
> > and trying to load a field that the source didn't send.  The migration
> > stream gets out of line and hits the section footer.
> 
> I'm a bit confused by description,
> do you have a reproducer for me to try?

Yeh, see the linked gitlab case command line:
  https://gitlab.com/qemu-project/qemu/-/issues/932

./x86_64-softmmu/qemu-system-x86_64 -M pc-q35-6.2 -m 512 -device virtio-scsi-pci,id=scsihw0,bus=pcie.0,addr=0x5,acpi-index=3 -drive if=none,my.qcow2,format=qcow2,id=drive-scsi0,node-name=scsi0 -device 'scsi-hd,bus=scsihw0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0,id=scsi0'  -nographic

just migrating from a 6.2 to a head triggers this.

Dave

> > The bodge is on piix4 never to load the field:
> >   a) Most 6.x builds never send it, so most of the time the migration
> >     will work.
> >   b) We can backport this fix to 6.x to remove the boobytrap.
> >   c) It should never have made a difference anyway since the acpi-index
> >     is command line configured and should be correct on the destination
> >     anyway
> >   d) ich9 is still sending/receiving this (unconditionally all the time)
> >     but due to (c) should never notice.  We could follow up to make it
> >     skip.
> > 
> > It worries me just when (a) actually happens.
> > 
> > Fixes: b32bd76 ("pci: introduce acpi-index property for PCI device")
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/932
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  hw/acpi/acpi-pci-hotplug-stub.c |  4 ----
> >  hw/acpi/pcihp.c                 |  6 ------
> >  hw/acpi/piix4.c                 | 11 ++++++++++-
> >  include/hw/acpi/pcihp.h         |  2 --
> >  4 files changed, 10 insertions(+), 13 deletions(-)
> > 
> > diff --git a/hw/acpi/acpi-pci-hotplug-stub.c b/hw/acpi/acpi-pci-hotplug-stub.c
> > index 734e4c5986..a43f6dafc9 100644
> > --- a/hw/acpi/acpi-pci-hotplug-stub.c
> > +++ b/hw/acpi/acpi-pci-hotplug-stub.c
> > @@ -41,7 +41,3 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
> >      return;
> >  }
> >  
> > -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
> > -{
> > -    return false;
> > -}
> > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > index 6351bd3424..bf65bbea49 100644
> > --- a/hw/acpi/pcihp.c
> > +++ b/hw/acpi/pcihp.c
> > @@ -554,12 +554,6 @@ void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
> >                                     OBJ_PROP_FLAG_READ);
> >  }
> >  
> > -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
> > -{
> > -     AcpiPciHpState *s = opaque;
> > -     return s->acpi_index;
> > -}
> > -
> >  const VMStateDescription vmstate_acpi_pcihp_pci_status = {
> >      .name = "acpi_pcihp_pci_status",
> >      .version_id = 1,
> > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > index cc37fa3416..48aeedd5f0 100644
> > --- a/hw/acpi/piix4.c
> > +++ b/hw/acpi/piix4.c
> > @@ -267,6 +267,15 @@ static bool piix4_vmstate_need_smbus(void *opaque, int version_id)
> >      return pm_smbus_vmstate_needed();
> >  }
> >  
> > +/*
> > + * This is a fudge to turn off the acpi_index field, whose
> > + * test was always broken on piix4.
> > + */
> > +static bool vmstate_test_never(void *opaque, int version_id)
> > +{
> > +    return false;
> > +}
> > +
> >  /* qemu-kvm 1.2 uses version 3 but advertised as 2
> >   * To support incoming qemu-kvm 1.2 migration, change version_id
> >   * and minimum_version_id to 2 below (which breaks migration from
> > @@ -297,7 +306,7 @@ static const VMStateDescription vmstate_acpi = {
> >              struct AcpiPciHpPciStatus),
> >          VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState,
> >                              vmstate_test_use_acpi_hotplug_bridge,
> > -                            vmstate_acpi_pcihp_use_acpi_index),
> > +                            vmstate_test_never),
> >          VMSTATE_END_OF_LIST()
> >      },
> >      .subsections = (const VMStateDescription*[]) {
> > diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> > index af1a169fc3..7e268c2c9c 100644
> > --- a/include/hw/acpi/pcihp.h
> > +++ b/include/hw/acpi/pcihp.h
> > @@ -73,8 +73,6 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off);
> >  
> >  extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
> >  
> > -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id);
> > -
> >  #define VMSTATE_PCI_HOTPLUG(pcihp, state, test_pcihp, test_acpi_index) \
> >          VMSTATE_UINT32_TEST(pcihp.hotplug_select, state, \
> >                              test_pcihp), \
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH] acpi: Bodge acpi_index migration

[Dr. David Alan Gilbert]

* Dr. David Alan Gilbert (dgilbert@redhat.com) wrote:
> * Igor Mammedov (imammedo@redhat.com) wrote:
> > On Wed,  6 Apr 2022 09:35:31 +0100
> > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> > 
> > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > 
> > > The 'acpi_index' field is a statically configured field, which for
> > > some reason is migrated; this never makes much sense because it's
> > > command line static.
> > 
> > that's true only for the field that's part of PCIDEvice,
> > however AcpiPciHpState::acpi_index is runtime state and _must_
> > be migrated if set, otherwise guest might get wrong index
> > if it's in process of querying it
> 
> So this patch only changes the piix4.c version; I'm confused, is there
> a AcpiPciHpState::acpi_index that's runtime setable in there?
> 
> >  
> > > However, on piix4 it's conditional, and the condition/test function
> > > ends up having the wrong pointer passed to it (it gets a PIIX4PMState
> > > not the AcpiPciHpState it was expecting, because VMSTATE_PCI_HOTPLUG
> > > is a macro and not another struct).  This means the field is randomly
> > > loaded/saved based on a random pointer.  In 6.x this random pointer
> > > randomly seems to get 0 for everyone (!); in 7.0rc it's getting junk
> > > and trying to load a field that the source didn't send.  The migration
> > > stream gets out of line and hits the section footer.
> > 
> > I'm a bit confused by description,
> > do you have a reproducer for me to try?
> 
> Yeh, see the linked gitlab case command line:
>   https://gitlab.com/qemu-project/qemu/-/issues/932
> 
> ./x86_64-softmmu/qemu-system-x86_64 -M pc-q35-6.2 -m 512 -device virtio-scsi-pci,id=scsihw0,bus=pcie.0,addr=0x5,acpi-index=3 -drive if=none,my.qcow2,format=qcow2,id=drive-scsi0,node-name=scsi0 -device 'scsi-hd,bus=scsihw0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0,id=scsi0'  -nographic

Oops no, wrong line; -M pc-i440fx-6.2  triggers this; q35 doesn't.

Dave

> just migrating from a 6.2 to a head triggers this.
> 
> Dave
> 
> > > The bodge is on piix4 never to load the field:
> > >   a) Most 6.x builds never send it, so most of the time the migration
> > >     will work.
> > >   b) We can backport this fix to 6.x to remove the boobytrap.
> > >   c) It should never have made a difference anyway since the acpi-index
> > >     is command line configured and should be correct on the destination
> > >     anyway
> > >   d) ich9 is still sending/receiving this (unconditionally all the time)
> > >     but due to (c) should never notice.  We could follow up to make it
> > >     skip.
> > > 
> > > It worries me just when (a) actually happens.
> > > 
> > > Fixes: b32bd76 ("pci: introduce acpi-index property for PCI device")
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/932
> > > 
> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > ---
> > >  hw/acpi/acpi-pci-hotplug-stub.c |  4 ----
> > >  hw/acpi/pcihp.c                 |  6 ------
> > >  hw/acpi/piix4.c                 | 11 ++++++++++-
> > >  include/hw/acpi/pcihp.h         |  2 --
> > >  4 files changed, 10 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/hw/acpi/acpi-pci-hotplug-stub.c b/hw/acpi/acpi-pci-hotplug-stub.c
> > > index 734e4c5986..a43f6dafc9 100644
> > > --- a/hw/acpi/acpi-pci-hotplug-stub.c
> > > +++ b/hw/acpi/acpi-pci-hotplug-stub.c
> > > @@ -41,7 +41,3 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
> > >      return;
> > >  }
> > >  
> > > -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
> > > -{
> > > -    return false;
> > > -}
> > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > index 6351bd3424..bf65bbea49 100644
> > > --- a/hw/acpi/pcihp.c
> > > +++ b/hw/acpi/pcihp.c
> > > @@ -554,12 +554,6 @@ void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
> > >                                     OBJ_PROP_FLAG_READ);
> > >  }
> > >  
> > > -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
> > > -{
> > > -     AcpiPciHpState *s = opaque;
> > > -     return s->acpi_index;
> > > -}
> > > -
> > >  const VMStateDescription vmstate_acpi_pcihp_pci_status = {
> > >      .name = "acpi_pcihp_pci_status",
> > >      .version_id = 1,
> > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > > index cc37fa3416..48aeedd5f0 100644
> > > --- a/hw/acpi/piix4.c
> > > +++ b/hw/acpi/piix4.c
> > > @@ -267,6 +267,15 @@ static bool piix4_vmstate_need_smbus(void *opaque, int version_id)
> > >      return pm_smbus_vmstate_needed();
> > >  }
> > >  
> > > +/*
> > > + * This is a fudge to turn off the acpi_index field, whose
> > > + * test was always broken on piix4.
> > > + */
> > > +static bool vmstate_test_never(void *opaque, int version_id)
> > > +{
> > > +    return false;
> > > +}
> > > +
> > >  /* qemu-kvm 1.2 uses version 3 but advertised as 2
> > >   * To support incoming qemu-kvm 1.2 migration, change version_id
> > >   * and minimum_version_id to 2 below (which breaks migration from
> > > @@ -297,7 +306,7 @@ static const VMStateDescription vmstate_acpi = {
> > >              struct AcpiPciHpPciStatus),
> > >          VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState,
> > >                              vmstate_test_use_acpi_hotplug_bridge,
> > > -                            vmstate_acpi_pcihp_use_acpi_index),
> > > +                            vmstate_test_never),
> > >          VMSTATE_END_OF_LIST()
> > >      },
> > >      .subsections = (const VMStateDescription*[]) {
> > > diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> > > index af1a169fc3..7e268c2c9c 100644
> > > --- a/include/hw/acpi/pcihp.h
> > > +++ b/include/hw/acpi/pcihp.h
> > > @@ -73,8 +73,6 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off);
> > >  
> > >  extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
> > >  
> > > -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id);
> > > -
> > >  #define VMSTATE_PCI_HOTPLUG(pcihp, state, test_pcihp, test_acpi_index) \
> > >          VMSTATE_UINT32_TEST(pcihp.hotplug_select, state, \
> > >                              test_pcihp), \
> > 
> -- 
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH] acpi: Bodge acpi_index migration

[Michael S. Tsirkin]

On Wed, Apr 06, 2022 at 10:44:18AM +0100, Dr. David Alan Gilbert wrote:
> * Dr. David Alan Gilbert (dgilbert@redhat.com) wrote:
> > * Igor Mammedov (imammedo@redhat.com) wrote:
> > > On Wed,  6 Apr 2022 09:35:31 +0100
> > > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> > > 
> > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > > 
> > > > The 'acpi_index' field is a statically configured field, which for
> > > > some reason is migrated; this never makes much sense because it's
> > > > command line static.
> > > 
> > > that's true only for the field that's part of PCIDEvice,
> > > however AcpiPciHpState::acpi_index is runtime state and _must_
> > > be migrated if set, otherwise guest might get wrong index
> > > if it's in process of querying it
> > 
> > So this patch only changes the piix4.c version; I'm confused, is there
> > a AcpiPciHpState::acpi_index that's runtime setable in there?
> > 
> > >  
> > > > However, on piix4 it's conditional, and the condition/test function
> > > > ends up having the wrong pointer passed to it (it gets a PIIX4PMState
> > > > not the AcpiPciHpState it was expecting, because VMSTATE_PCI_HOTPLUG
> > > > is a macro and not another struct).  This means the field is randomly
> > > > loaded/saved based on a random pointer.  In 6.x this random pointer
> > > > randomly seems to get 0 for everyone (!); in 7.0rc it's getting junk
> > > > and trying to load a field that the source didn't send.  The migration
> > > > stream gets out of line and hits the section footer.
> > > 
> > > I'm a bit confused by description,
> > > do you have a reproducer for me to try?
> > 
> > Yeh, see the linked gitlab case command line:
> >   https://gitlab.com/qemu-project/qemu/-/issues/932
> > 
> > ./x86_64-softmmu/qemu-system-x86_64 -M pc-q35-6.2 -m 512 -device virtio-scsi-pci,id=scsihw0,bus=pcie.0,addr=0x5,acpi-index=3 -drive if=none,my.qcow2,format=qcow2,id=drive-scsi0,node-name=scsi0 -device 'scsi-hd,bus=scsihw0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0,id=scsi0'  -nographic
> 
> Oops no, wrong line; -M pc-i440fx-6.2  triggers this; q35 doesn't.
> 
> Dave


Igor were you able to reproduce?

> > just migrating from a 6.2 to a head triggers this.
> > 
> > Dave
> > 
> > > > The bodge is on piix4 never to load the field:
> > > >   a) Most 6.x builds never send it, so most of the time the migration
> > > >     will work.
> > > >   b) We can backport this fix to 6.x to remove the boobytrap.
> > > >   c) It should never have made a difference anyway since the acpi-index
> > > >     is command line configured and should be correct on the destination
> > > >     anyway
> > > >   d) ich9 is still sending/receiving this (unconditionally all the time)
> > > >     but due to (c) should never notice.  We could follow up to make it
> > > >     skip.
> > > > 
> > > > It worries me just when (a) actually happens.
> > > > 
> > > > Fixes: b32bd76 ("pci: introduce acpi-index property for PCI device")
> > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/932
> > > > 
> > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > > ---
> > > >  hw/acpi/acpi-pci-hotplug-stub.c |  4 ----
> > > >  hw/acpi/pcihp.c                 |  6 ------
> > > >  hw/acpi/piix4.c                 | 11 ++++++++++-
> > > >  include/hw/acpi/pcihp.h         |  2 --
> > > >  4 files changed, 10 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/hw/acpi/acpi-pci-hotplug-stub.c b/hw/acpi/acpi-pci-hotplug-stub.c
> > > > index 734e4c5986..a43f6dafc9 100644
> > > > --- a/hw/acpi/acpi-pci-hotplug-stub.c
> > > > +++ b/hw/acpi/acpi-pci-hotplug-stub.c
> > > > @@ -41,7 +41,3 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
> > > >      return;
> > > >  }
> > > >  
> > > > -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
> > > > -{
> > > > -    return false;
> > > > -}
> > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > > index 6351bd3424..bf65bbea49 100644
> > > > --- a/hw/acpi/pcihp.c
> > > > +++ b/hw/acpi/pcihp.c
> > > > @@ -554,12 +554,6 @@ void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
> > > >                                     OBJ_PROP_FLAG_READ);
> > > >  }
> > > >  
> > > > -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
> > > > -{
> > > > -     AcpiPciHpState *s = opaque;
> > > > -     return s->acpi_index;
> > > > -}
> > > > -
> > > >  const VMStateDescription vmstate_acpi_pcihp_pci_status = {
> > > >      .name = "acpi_pcihp_pci_status",
> > > >      .version_id = 1,
> > > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > > > index cc37fa3416..48aeedd5f0 100644
> > > > --- a/hw/acpi/piix4.c
> > > > +++ b/hw/acpi/piix4.c
> > > > @@ -267,6 +267,15 @@ static bool piix4_vmstate_need_smbus(void *opaque, int version_id)
> > > >      return pm_smbus_vmstate_needed();
> > > >  }
> > > >  
> > > > +/*
> > > > + * This is a fudge to turn off the acpi_index field, whose
> > > > + * test was always broken on piix4.
> > > > + */
> > > > +static bool vmstate_test_never(void *opaque, int version_id)
> > > > +{
> > > > +    return false;
> > > > +}
> > > > +
> > > >  /* qemu-kvm 1.2 uses version 3 but advertised as 2
> > > >   * To support incoming qemu-kvm 1.2 migration, change version_id
> > > >   * and minimum_version_id to 2 below (which breaks migration from
> > > > @@ -297,7 +306,7 @@ static const VMStateDescription vmstate_acpi = {
> > > >              struct AcpiPciHpPciStatus),
> > > >          VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState,
> > > >                              vmstate_test_use_acpi_hotplug_bridge,
> > > > -                            vmstate_acpi_pcihp_use_acpi_index),
> > > > +                            vmstate_test_never),
> > > >          VMSTATE_END_OF_LIST()
> > > >      },
> > > >      .subsections = (const VMStateDescription*[]) {
> > > > diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> > > > index af1a169fc3..7e268c2c9c 100644
> > > > --- a/include/hw/acpi/pcihp.h
> > > > +++ b/include/hw/acpi/pcihp.h
> > > > @@ -73,8 +73,6 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off);
> > > >  
> > > >  extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
> > > >  
> > > > -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id);
> > > > -
> > > >  #define VMSTATE_PCI_HOTPLUG(pcihp, state, test_pcihp, test_acpi_index) \
> > > >          VMSTATE_UINT32_TEST(pcihp.hotplug_select, state, \
> > > >                              test_pcihp), \
> > > 
> > -- 
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> -- 
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH] acpi: Bodge acpi_index migration

[Igor Mammedov]

On Wed, 6 Apr 2022 10:38:51 +0100
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

> * Igor Mammedov (imammedo@redhat.com) wrote:
> > On Wed,  6 Apr 2022 09:35:31 +0100
> > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> >   
> > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > 
> > > The 'acpi_index' field is a statically configured field, which for
> > > some reason is migrated; this never makes much sense because it's
> > > command line static.  
> > 
> > that's true only for the field that's part of PCIDEvice,
> > however AcpiPciHpState::acpi_index is runtime state and _must_
> > be migrated if set, otherwise guest might get wrong index
> > if it's in process of querying it  
> 
> So this patch only changes the piix4.c version; I'm confused, is there
> a AcpiPciHpState::acpi_index that's runtime setable in there?

> > >          VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState,
                                    ^^^ AcpiPciHpState
> > >                              vmstate_test_use_acpi_hotplug_bridge,
> > > -                            vmstate_acpi_pcihp_use_acpi_index),

hw/acpi/pcihp.c:pci_write():
   s->acpi_index = object_property_get_uint(o, "acpi-index", NULL);

s->acpi_index is runtime value that is supposed to be migrated if it's set
to something other then 0

I may have botched VMSTATE_PCI_HOTPLUG, intent was to migrate
AcpiPciHpState::acpi_index if necessary. But I'm not sure how
if I used correct approach for to migrate an optional value
i.e.  maybe instead of VMSTATE_UINT32_TEST(pcihp.acpi_index, state, test_acpi_index)
I should've used subsection, because destination has no clue if
acpi_index would be transmitted over wire or not?
    
> > > However, on piix4 it's conditional, and the condition/test function
> > > ends up having the wrong pointer passed to it (it gets a PIIX4PMState
> > > not the AcpiPciHpState it was expecting, because VMSTATE_PCI_HOTPLUG
> > > is a macro and not another struct).  This means the field is randomly
> > > loaded/saved based on a random pointer.  In 6.x this random pointer
> > > randomly seems to get 0 for everyone (!); in 7.0rc it's getting junk
> > > and trying to load a field that the source didn't send.  The migration
> > > stream gets out of line and hits the section footer.  
> > 
> > I'm a bit confused by description,
> > do you have a reproducer for me to try?  
> 
> Yeh, see the linked gitlab case command line:
>   https://gitlab.com/qemu-project/qemu/-/issues/932
> 
> ./x86_64-softmmu/qemu-system-x86_64 -M pc-q35-6.2 -m 512 -device virtio-scsi-pci,id=scsihw0,bus=pcie.0,addr=0x5,acpi-index=3 -drive if=none,my.qcow2,format=qcow2,id=drive-scsi0,node-name=scsi0 -device 'scsi-hd,bus=scsihw0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0,id=scsi0'  -nographic
> 
> just migrating from a 6.2 to a head triggers this.

Over here any migration from qemu-6.2 to HEAD at 3d31fe4d662f13c7
fails even without acpi-index, as simple as this:

qemu-system-x86_64-6.2 -M pc-i440fx-6.2  -m 512 -vnc :0 -monitor stdio
(qemu) stop
(qemu) migrate "exec:gzip -c > STATEFILE.gz"

qemu-system-x86_64-7.0 -M pc-i440fx-6.2  -m 512 -vnc :0 -monitor stdio -incoming "exec: gzip -c -d STATEFILE.gz"

(qemu) qemu-system-x86_64-7.0: Missing section footer for 0000:00:01.3/piix4_pm
qemu-system-x86_64-7.0: load of migration failed: Invalid argument


Like you pointed out in gitlab issue, vmstate_acpi_pcihp_use_acpi_index
is broken. Following applied to HEAD should fix immediate issue on destination
reading random value:

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index f0b5fac44a..c97db491c8 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -269,6 +269,11 @@ static bool piix4_vmstate_need_smbus(void *opaque, int version_id)
     return pm_smbus_vmstate_needed();
 }
 
+static bool vmstate_piix4_need_acpi_index(void *opaque, int version_id)
+{
+    PIIX4PMState *s = PIIX4_PM(opaque);
+    return vmstate_acpi_pcihp_use_acpi_index(&(s->acpi_pci_hotplug), version_id);
+}
 /* qemu-kvm 1.2 uses version 3 but advertised as 2
  * To support incoming qemu-kvm 1.2 migration, change version_id
  * and minimum_version_id to 2 below (which breaks migration from
@@ -299,7 +304,7 @@ static const VMStateDescription vmstate_acpi = {
             struct AcpiPciHpPciStatus),
         VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState,
                             vmstate_test_use_acpi_hotplug_bridge,
-                            vmstate_acpi_pcihp_use_acpi_index),
+                            vmstate_piix4_need_acpi_index),
         VMSTATE_END_OF_LIST()
     },
     .subsections = (const VMStateDescription*[]) {


> Dave
> 
> > > The bodge is on piix4 never to load the field:
> > >   a) Most 6.x builds never send it, so most of the time the migration
> > >     will work.
> > >   b) We can backport this fix to 6.x to remove the boobytrap.
> > >   c) It should never have made a difference anyway since the acpi-index
> > >     is command line configured and should be correct on the destination
> > >     anyway
> > >   d) ich9 is still sending/receiving this (unconditionally all the time)
> > >     but due to (c) should never notice.  We could follow up to make it
> > >     skip.
> > > 
> > > It worries me just when (a) actually happens.
> > > 
> > > Fixes: b32bd76 ("pci: introduce acpi-index property for PCI device")
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/932
> > > 
> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > ---
> > >  hw/acpi/acpi-pci-hotplug-stub.c |  4 ----
> > >  hw/acpi/pcihp.c                 |  6 ------
> > >  hw/acpi/piix4.c                 | 11 ++++++++++-
> > >  include/hw/acpi/pcihp.h         |  2 --
> > >  4 files changed, 10 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/hw/acpi/acpi-pci-hotplug-stub.c b/hw/acpi/acpi-pci-hotplug-stub.c
> > > index 734e4c5986..a43f6dafc9 100644
> > > --- a/hw/acpi/acpi-pci-hotplug-stub.c
> > > +++ b/hw/acpi/acpi-pci-hotplug-stub.c
> > > @@ -41,7 +41,3 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
> > >      return;
> > >  }
> > >  
> > > -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
> > > -{
> > > -    return false;
> > > -}
> > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > index 6351bd3424..bf65bbea49 100644
> > > --- a/hw/acpi/pcihp.c
> > > +++ b/hw/acpi/pcihp.c
> > > @@ -554,12 +554,6 @@ void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
> > >                                     OBJ_PROP_FLAG_READ);
> > >  }
> > >  
> > > -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
> > > -{
> > > -     AcpiPciHpState *s = opaque;
> > > -     return s->acpi_index;
> > > -}
> > > -
> > >  const VMStateDescription vmstate_acpi_pcihp_pci_status = {
> > >      .name = "acpi_pcihp_pci_status",
> > >      .version_id = 1,
> > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > > index cc37fa3416..48aeedd5f0 100644
> > > --- a/hw/acpi/piix4.c
> > > +++ b/hw/acpi/piix4.c
> > > @@ -267,6 +267,15 @@ static bool piix4_vmstate_need_smbus(void *opaque, int version_id)
> > >      return pm_smbus_vmstate_needed();
> > >  }
> > >  
> > > +/*
> > > + * This is a fudge to turn off the acpi_index field, whose
> > > + * test was always broken on piix4.
> > > + */
> > > +static bool vmstate_test_never(void *opaque, int version_id)
> > > +{
> > > +    return false;
> > > +}
> > > +
> > >  /* qemu-kvm 1.2 uses version 3 but advertised as 2
> > >   * To support incoming qemu-kvm 1.2 migration, change version_id
> > >   * and minimum_version_id to 2 below (which breaks migration from
> > > @@ -297,7 +306,7 @@ static const VMStateDescription vmstate_acpi = {
> > >              struct AcpiPciHpPciStatus),
> > >          VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState,
> > >                              vmstate_test_use_acpi_hotplug_bridge,
> > > -                            vmstate_acpi_pcihp_use_acpi_index),
> > > +                            vmstate_test_never),
> > >          VMSTATE_END_OF_LIST()
> > >      },
> > >      .subsections = (const VMStateDescription*[]) {
> > > diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> > > index af1a169fc3..7e268c2c9c 100644
> > > --- a/include/hw/acpi/pcihp.h
> > > +++ b/include/hw/acpi/pcihp.h
> > > @@ -73,8 +73,6 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off);
> > >  
> > >  extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
> > >  
> > > -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id);
> > > -
> > >  #define VMSTATE_PCI_HOTPLUG(pcihp, state, test_pcihp, test_acpi_index) \
> > >          VMSTATE_UINT32_TEST(pcihp.hotplug_select, state, \
> > >                              test_pcihp), \  
> >   

Re: [PATCH] acpi: Bodge acpi_index migration

[Dr. David Alan Gilbert]

* Igor Mammedov (imammedo@redhat.com) wrote:
> On Wed, 6 Apr 2022 10:38:51 +0100
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> 
> > * Igor Mammedov (imammedo@redhat.com) wrote:
> > > On Wed,  6 Apr 2022 09:35:31 +0100
> > > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> > >   
> > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > > 
> > > > The 'acpi_index' field is a statically configured field, which for
> > > > some reason is migrated; this never makes much sense because it's
> > > > command line static.  
> > > 
> > > that's true only for the field that's part of PCIDEvice,
> > > however AcpiPciHpState::acpi_index is runtime state and _must_
> > > be migrated if set, otherwise guest might get wrong index
> > > if it's in process of querying it  
> > 
> > So this patch only changes the piix4.c version; I'm confused, is there
> > a AcpiPciHpState::acpi_index that's runtime setable in there?
> 
> > > >          VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState,
>                                     ^^^ AcpiPciHpState
> > > >                              vmstate_test_use_acpi_hotplug_bridge,
> > > > -                            vmstate_acpi_pcihp_use_acpi_index),
> 
> hw/acpi/pcihp.c:pci_write():
>    s->acpi_index = object_property_get_uint(o, "acpi-index", NULL);
> 
> s->acpi_index is runtime value that is supposed to be migrated if it's set
> to something other then 0
> 
> I may have botched VMSTATE_PCI_HOTPLUG, intent was to migrate
> AcpiPciHpState::acpi_index if necessary. But I'm not sure how
> if I used correct approach for to migrate an optional value
> i.e.  maybe instead of VMSTATE_UINT32_TEST(pcihp.acpi_index, state, test_acpi_index)
> I should've used subsection, because destination has no clue if
> acpi_index would be transmitted over wire or not?
>     
> > > > However, on piix4 it's conditional, and the condition/test function
> > > > ends up having the wrong pointer passed to it (it gets a PIIX4PMState
> > > > not the AcpiPciHpState it was expecting, because VMSTATE_PCI_HOTPLUG
> > > > is a macro and not another struct).  This means the field is randomly
> > > > loaded/saved based on a random pointer.  In 6.x this random pointer
> > > > randomly seems to get 0 for everyone (!); in 7.0rc it's getting junk
> > > > and trying to load a field that the source didn't send.  The migration
> > > > stream gets out of line and hits the section footer.  
> > > 
> > > I'm a bit confused by description,
> > > do you have a reproducer for me to try?  
> > 
> > Yeh, see the linked gitlab case command line:
> >   https://gitlab.com/qemu-project/qemu/-/issues/932
> > 
> > ./x86_64-softmmu/qemu-system-x86_64 -M pc-q35-6.2 -m 512 -device virtio-scsi-pci,id=scsihw0,bus=pcie.0,addr=0x5,acpi-index=3 -drive if=none,my.qcow2,format=qcow2,id=drive-scsi0,node-name=scsi0 -device 'scsi-hd,bus=scsihw0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0,id=scsi0'  -nographic
> > 
> > just migrating from a 6.2 to a head triggers this.
> 
> Over here any migration from qemu-6.2 to HEAD at 3d31fe4d662f13c7
> fails even without acpi-index, as simple as this:
> 
> qemu-system-x86_64-6.2 -M pc-i440fx-6.2  -m 512 -vnc :0 -monitor stdio
> (qemu) stop
> (qemu) migrate "exec:gzip -c > STATEFILE.gz"
> 
> qemu-system-x86_64-7.0 -M pc-i440fx-6.2  -m 512 -vnc :0 -monitor stdio -incoming "exec: gzip -c -d STATEFILE.gz"
> 
> (qemu) qemu-system-x86_64-7.0: Missing section footer for 0000:00:01.3/piix4_pm
> qemu-system-x86_64-7.0: load of migration failed: Invalid argument
> 
> 
> Like you pointed out in gitlab issue, vmstate_acpi_pcihp_use_acpi_index
> is broken. Following applied to HEAD should fix immediate issue on destination
> reading random value:
> 
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index f0b5fac44a..c97db491c8 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -269,6 +269,11 @@ static bool piix4_vmstate_need_smbus(void *opaque, int version_id)
>      return pm_smbus_vmstate_needed();
>  }
>  
> +static bool vmstate_piix4_need_acpi_index(void *opaque, int version_id)
> +{
> +    PIIX4PMState *s = PIIX4_PM(opaque);
> +    return vmstate_acpi_pcihp_use_acpi_index(&(s->acpi_pci_hotplug), version_id);
> +}

But if acpi_index was set on the source 6.2 host, it won't send the
index, but the 7.0 would expect it, and it would fail in the same way
wouldn't it?

Dave

>  /* qemu-kvm 1.2 uses version 3 but advertised as 2
>   * To support incoming qemu-kvm 1.2 migration, change version_id
>   * and minimum_version_id to 2 below (which breaks migration from
> @@ -299,7 +304,7 @@ static const VMStateDescription vmstate_acpi = {
>              struct AcpiPciHpPciStatus),
>          VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState,
>                              vmstate_test_use_acpi_hotplug_bridge,
> -                            vmstate_acpi_pcihp_use_acpi_index),
> +                            vmstate_piix4_need_acpi_index),
>          VMSTATE_END_OF_LIST()
>      },
>      .subsections = (const VMStateDescription*[]) {
> 
> 
> > Dave
> > 
> > > > The bodge is on piix4 never to load the field:
> > > >   a) Most 6.x builds never send it, so most of the time the migration
> > > >     will work.
> > > >   b) We can backport this fix to 6.x to remove the boobytrap.
> > > >   c) It should never have made a difference anyway since the acpi-index
> > > >     is command line configured and should be correct on the destination
> > > >     anyway
> > > >   d) ich9 is still sending/receiving this (unconditionally all the time)
> > > >     but due to (c) should never notice.  We could follow up to make it
> > > >     skip.
> > > > 
> > > > It worries me just when (a) actually happens.
> > > > 
> > > > Fixes: b32bd76 ("pci: introduce acpi-index property for PCI device")
> > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/932
> > > > 
> > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > > ---
> > > >  hw/acpi/acpi-pci-hotplug-stub.c |  4 ----
> > > >  hw/acpi/pcihp.c                 |  6 ------
> > > >  hw/acpi/piix4.c                 | 11 ++++++++++-
> > > >  include/hw/acpi/pcihp.h         |  2 --
> > > >  4 files changed, 10 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/hw/acpi/acpi-pci-hotplug-stub.c b/hw/acpi/acpi-pci-hotplug-stub.c
> > > > index 734e4c5986..a43f6dafc9 100644
> > > > --- a/hw/acpi/acpi-pci-hotplug-stub.c
> > > > +++ b/hw/acpi/acpi-pci-hotplug-stub.c
> > > > @@ -41,7 +41,3 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
> > > >      return;
> > > >  }
> > > >  
> > > > -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
> > > > -{
> > > > -    return false;
> > > > -}
> > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > > index 6351bd3424..bf65bbea49 100644
> > > > --- a/hw/acpi/pcihp.c
> > > > +++ b/hw/acpi/pcihp.c
> > > > @@ -554,12 +554,6 @@ void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
> > > >                                     OBJ_PROP_FLAG_READ);
> > > >  }
> > > >  
> > > > -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
> > > > -{
> > > > -     AcpiPciHpState *s = opaque;
> > > > -     return s->acpi_index;
> > > > -}
> > > > -
> > > >  const VMStateDescription vmstate_acpi_pcihp_pci_status = {
> > > >      .name = "acpi_pcihp_pci_status",
> > > >      .version_id = 1,
> > > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > > > index cc37fa3416..48aeedd5f0 100644
> > > > --- a/hw/acpi/piix4.c
> > > > +++ b/hw/acpi/piix4.c
> > > > @@ -267,6 +267,15 @@ static bool piix4_vmstate_need_smbus(void *opaque, int version_id)
> > > >      return pm_smbus_vmstate_needed();
> > > >  }
> > > >  
> > > > +/*
> > > > + * This is a fudge to turn off the acpi_index field, whose
> > > > + * test was always broken on piix4.
> > > > + */
> > > > +static bool vmstate_test_never(void *opaque, int version_id)
> > > > +{
> > > > +    return false;
> > > > +}
> > > > +
> > > >  /* qemu-kvm 1.2 uses version 3 but advertised as 2
> > > >   * To support incoming qemu-kvm 1.2 migration, change version_id
> > > >   * and minimum_version_id to 2 below (which breaks migration from
> > > > @@ -297,7 +306,7 @@ static const VMStateDescription vmstate_acpi = {
> > > >              struct AcpiPciHpPciStatus),
> > > >          VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState,
> > > >                              vmstate_test_use_acpi_hotplug_bridge,
> > > > -                            vmstate_acpi_pcihp_use_acpi_index),
> > > > +                            vmstate_test_never),
> > > >          VMSTATE_END_OF_LIST()
> > > >      },
> > > >      .subsections = (const VMStateDescription*[]) {
> > > > diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> > > > index af1a169fc3..7e268c2c9c 100644
> > > > --- a/include/hw/acpi/pcihp.h
> > > > +++ b/include/hw/acpi/pcihp.h
> > > > @@ -73,8 +73,6 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off);
> > > >  
> > > >  extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
> > > >  
> > > > -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id);
> > > > -
> > > >  #define VMSTATE_PCI_HOTPLUG(pcihp, state, test_pcihp, test_acpi_index) \
> > > >          VMSTATE_UINT32_TEST(pcihp.hotplug_select, state, \
> > > >                              test_pcihp), \  
> > >   
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH] acpi: Bodge acpi_index migration

[Igor Mammedov]

On Wed, 6 Apr 2022 17:11:09 +0100
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

> * Igor Mammedov (imammedo@redhat.com) wrote:
> > On Wed, 6 Apr 2022 10:38:51 +0100
> > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> >   
> > > * Igor Mammedov (imammedo@redhat.com) wrote:  
> > > > On Wed,  6 Apr 2022 09:35:31 +0100
> > > > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> > > >     
> > > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > > > 
> > > > > The 'acpi_index' field is a statically configured field, which for
> > > > > some reason is migrated; this never makes much sense because it's
> > > > > command line static.    
> > > > 
> > > > that's true only for the field that's part of PCIDEvice,
> > > > however AcpiPciHpState::acpi_index is runtime state and _must_
> > > > be migrated if set, otherwise guest might get wrong index
> > > > if it's in process of querying it    
> > > 
> > > So this patch only changes the piix4.c version; I'm confused, is there
> > > a AcpiPciHpState::acpi_index that's runtime setable in there?  
> >   
> > > > >          VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState,  
> >                                     ^^^ AcpiPciHpState  
> > > > >                              vmstate_test_use_acpi_hotplug_bridge,
> > > > > -                            vmstate_acpi_pcihp_use_acpi_index),  
> > 
> > hw/acpi/pcihp.c:pci_write():
> >    s->acpi_index = object_property_get_uint(o, "acpi-index", NULL);
> > 
> > s->acpi_index is runtime value that is supposed to be migrated if it's set
> > to something other then 0
> > 
> > I may have botched VMSTATE_PCI_HOTPLUG, intent was to migrate
> > AcpiPciHpState::acpi_index if necessary. But I'm not sure how
> > if I used correct approach for to migrate an optional value
> > i.e.  maybe instead of VMSTATE_UINT32_TEST(pcihp.acpi_index, state, test_acpi_index)
> > I should've used subsection, because destination has no clue if
> > acpi_index would be transmitted over wire or not?
> >       
> > > > > However, on piix4 it's conditional, and the condition/test function
> > > > > ends up having the wrong pointer passed to it (it gets a PIIX4PMState
> > > > > not the AcpiPciHpState it was expecting, because VMSTATE_PCI_HOTPLUG
> > > > > is a macro and not another struct).  This means the field is randomly
> > > > > loaded/saved based on a random pointer.  In 6.x this random pointer
> > > > > randomly seems to get 0 for everyone (!); in 7.0rc it's getting junk
> > > > > and trying to load a field that the source didn't send.  The migration
> > > > > stream gets out of line and hits the section footer.    
> > > > 
> > > > I'm a bit confused by description,
> > > > do you have a reproducer for me to try?    
> > > 
> > > Yeh, see the linked gitlab case command line:
> > >   https://gitlab.com/qemu-project/qemu/-/issues/932
> > > 
> > > ./x86_64-softmmu/qemu-system-x86_64 -M pc-q35-6.2 -m 512 -device virtio-scsi-pci,id=scsihw0,bus=pcie.0,addr=0x5,acpi-index=3 -drive if=none,my.qcow2,format=qcow2,id=drive-scsi0,node-name=scsi0 -device 'scsi-hd,bus=scsihw0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0,id=scsi0'  -nographic
> > > 
> > > just migrating from a 6.2 to a head triggers this.  
> > 
> > Over here any migration from qemu-6.2 to HEAD at 3d31fe4d662f13c7
> > fails even without acpi-index, as simple as this:
> > 
> > qemu-system-x86_64-6.2 -M pc-i440fx-6.2  -m 512 -vnc :0 -monitor stdio
> > (qemu) stop
> > (qemu) migrate "exec:gzip -c > STATEFILE.gz"
> > 
> > qemu-system-x86_64-7.0 -M pc-i440fx-6.2  -m 512 -vnc :0 -monitor stdio -incoming "exec: gzip -c -d STATEFILE.gz"
> > 
> > (qemu) qemu-system-x86_64-7.0: Missing section footer for 0000:00:01.3/piix4_pm
> > qemu-system-x86_64-7.0: load of migration failed: Invalid argument
> > 
> > 
> > Like you pointed out in gitlab issue, vmstate_acpi_pcihp_use_acpi_index
> > is broken. Following applied to HEAD should fix immediate issue on destination
> > reading random value:
> > 
> > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > index f0b5fac44a..c97db491c8 100644
> > --- a/hw/acpi/piix4.c
> > +++ b/hw/acpi/piix4.c
> > @@ -269,6 +269,11 @@ static bool piix4_vmstate_need_smbus(void *opaque, int version_id)
> >      return pm_smbus_vmstate_needed();
> >  }
> >  
> > +static bool vmstate_piix4_need_acpi_index(void *opaque, int version_id)
> > +{
> > +    PIIX4PMState *s = PIIX4_PM(opaque);
> > +    return vmstate_acpi_pcihp_use_acpi_index(&(s->acpi_pci_hotplug), version_id);
> > +}  
> 
> But if acpi_index was set on the source 6.2 host, it won't send the
> index, but the 7.0 would expect it, and it would fail in the same way
> wouldn't it?

With piix4 fixed up 7.0 won't expect field as s->acpi_index initialized to 0
so check will always return 0 and the field won't be expected.
( testing confirms it).
If test on 6.2 host somehow manages to return 1, destination won't
be able to accept it, because it has no idea about it (that is not fixable, I'm afraid).

For Q35 we set check  to NULL
        VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug,
                            ICH9LPCPMRegs,
                            NULL, NULL),

which if I read vmstate_load_state() correctly will always expect
the field and will always store fields since field->version_id == 0
for VMSTATE_UINT32_TEST.

So we can't remove field without breaking Q35.

Net effect:
  * not send the field for PC machine (ever)
  * send field always for Q35 (always)

So your patch is good with fixed commit message
and a comment close to the field that it's not really used with piix4

And to make migration of acpi_index on PC machine working,
we need add an extra subsection that should be able to
handle conditional value.

> 
> Dave
> 
> >  /* qemu-kvm 1.2 uses version 3 but advertised as 2
> >   * To support incoming qemu-kvm 1.2 migration, change version_id
> >   * and minimum_version_id to 2 below (which breaks migration from
> > @@ -299,7 +304,7 @@ static const VMStateDescription vmstate_acpi = {
> >              struct AcpiPciHpPciStatus),
> >          VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState,
> >                              vmstate_test_use_acpi_hotplug_bridge,
> > -                            vmstate_acpi_pcihp_use_acpi_index),
> > +                            vmstate_piix4_need_acpi_index),
> >          VMSTATE_END_OF_LIST()
> >      },
> >      .subsections = (const VMStateDescription*[]) {
> > 
> >   
> > > Dave
> > >   
> > > > > The bodge is on piix4 never to load the field:
> > > > >   a) Most 6.x builds never send it, so most of the time the migration
> > > > >     will work.
> > > > >   b) We can backport this fix to 6.x to remove the boobytrap.
> > > > >   c) It should never have made a difference anyway since the acpi-index
> > > > >     is command line configured and should be correct on the destination
> > > > >     anyway
> > > > >   d) ich9 is still sending/receiving this (unconditionally all the time)
> > > > >     but due to (c) should never notice.  We could follow up to make it
> > > > >     skip.
> > > > > 
> > > > > It worries me just when (a) actually happens.
> > > > > 
> > > > > Fixes: b32bd76 ("pci: introduce acpi-index property for PCI device")
> > > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/932
> > > > > 
> > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > > > ---
> > > > >  hw/acpi/acpi-pci-hotplug-stub.c |  4 ----
> > > > >  hw/acpi/pcihp.c                 |  6 ------
> > > > >  hw/acpi/piix4.c                 | 11 ++++++++++-
> > > > >  include/hw/acpi/pcihp.h         |  2 --
> > > > >  4 files changed, 10 insertions(+), 13 deletions(-)
> > > > > 
> > > > > diff --git a/hw/acpi/acpi-pci-hotplug-stub.c b/hw/acpi/acpi-pci-hotplug-stub.c
> > > > > index 734e4c5986..a43f6dafc9 100644
> > > > > --- a/hw/acpi/acpi-pci-hotplug-stub.c
> > > > > +++ b/hw/acpi/acpi-pci-hotplug-stub.c
> > > > > @@ -41,7 +41,3 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
> > > > >      return;
> > > > >  }
> > > > >  
> > > > > -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
> > > > > -{
> > > > > -    return false;
> > > > > -}
> > > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > > > index 6351bd3424..bf65bbea49 100644
> > > > > --- a/hw/acpi/pcihp.c
> > > > > +++ b/hw/acpi/pcihp.c
> > > > > @@ -554,12 +554,6 @@ void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
> > > > >                                     OBJ_PROP_FLAG_READ);
> > > > >  }
> > > > >  
> > > > > -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
> > > > > -{
> > > > > -     AcpiPciHpState *s = opaque;
> > > > > -     return s->acpi_index;
> > > > > -}
> > > > > -
> > > > >  const VMStateDescription vmstate_acpi_pcihp_pci_status = {
> > > > >      .name = "acpi_pcihp_pci_status",
> > > > >      .version_id = 1,
> > > > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > > > > index cc37fa3416..48aeedd5f0 100644
> > > > > --- a/hw/acpi/piix4.c
> > > > > +++ b/hw/acpi/piix4.c
> > > > > @@ -267,6 +267,15 @@ static bool piix4_vmstate_need_smbus(void *opaque, int version_id)
> > > > >      return pm_smbus_vmstate_needed();
> > > > >  }
> > > > >  
> > > > > +/*
> > > > > + * This is a fudge to turn off the acpi_index field, whose
> > > > > + * test was always broken on piix4.
> > > > > + */
> > > > > +static bool vmstate_test_never(void *opaque, int version_id)
> > > > > +{
> > > > > +    return false;
> > > > > +}
> > > > > +
> > > > >  /* qemu-kvm 1.2 uses version 3 but advertised as 2
> > > > >   * To support incoming qemu-kvm 1.2 migration, change version_id
> > > > >   * and minimum_version_id to 2 below (which breaks migration from
> > > > > @@ -297,7 +306,7 @@ static const VMStateDescription vmstate_acpi = {
> > > > >              struct AcpiPciHpPciStatus),
> > > > >          VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState,
> > > > >                              vmstate_test_use_acpi_hotplug_bridge,
> > > > > -                            vmstate_acpi_pcihp_use_acpi_index),
> > > > > +                            vmstate_test_never),
> > > > >          VMSTATE_END_OF_LIST()
> > > > >      },
> > > > >      .subsections = (const VMStateDescription*[]) {
> > > > > diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> > > > > index af1a169fc3..7e268c2c9c 100644
> > > > > --- a/include/hw/acpi/pcihp.h
> > > > > +++ b/include/hw/acpi/pcihp.h
> > > > > @@ -73,8 +73,6 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off);
> > > > >  
> > > > >  extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
> > > > >  
> > > > > -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id);
> > > > > -
> > > > >  #define VMSTATE_PCI_HOTPLUG(pcihp, state, test_pcihp, test_acpi_index) \
> > > > >          VMSTATE_UINT32_TEST(pcihp.hotplug_select, state, \
> > > > >                              test_pcihp), \    
> > > >     
> >   

Re: [PATCH] acpi: Bodge acpi_index migration

[Dr. David Alan Gilbert]

* Igor Mammedov (imammedo@redhat.com) wrote:
> On Wed, 6 Apr 2022 17:11:09 +0100
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> 
> > * Igor Mammedov (imammedo@redhat.com) wrote:
> > > On Wed, 6 Apr 2022 10:38:51 +0100
> > > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > >   
> > > > * Igor Mammedov (imammedo@redhat.com) wrote:  
> > > > > On Wed,  6 Apr 2022 09:35:31 +0100
> > > > > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> > > > >     
> > > > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > > > > 
> > > > > > The 'acpi_index' field is a statically configured field, which for
> > > > > > some reason is migrated; this never makes much sense because it's
> > > > > > command line static.    
> > > > > 
> > > > > that's true only for the field that's part of PCIDEvice,
> > > > > however AcpiPciHpState::acpi_index is runtime state and _must_
> > > > > be migrated if set, otherwise guest might get wrong index
> > > > > if it's in process of querying it    
> > > > 
> > > > So this patch only changes the piix4.c version; I'm confused, is there
> > > > a AcpiPciHpState::acpi_index that's runtime setable in there?  
> > >   
> > > > > >          VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState,  
> > >                                     ^^^ AcpiPciHpState  
> > > > > >                              vmstate_test_use_acpi_hotplug_bridge,
> > > > > > -                            vmstate_acpi_pcihp_use_acpi_index),  
> > > 
> > > hw/acpi/pcihp.c:pci_write():
> > >    s->acpi_index = object_property_get_uint(o, "acpi-index", NULL);
> > > 
> > > s->acpi_index is runtime value that is supposed to be migrated if it's set
> > > to something other then 0
> > > 
> > > I may have botched VMSTATE_PCI_HOTPLUG, intent was to migrate
> > > AcpiPciHpState::acpi_index if necessary. But I'm not sure how
> > > if I used correct approach for to migrate an optional value
> > > i.e.  maybe instead of VMSTATE_UINT32_TEST(pcihp.acpi_index, state, test_acpi_index)
> > > I should've used subsection, because destination has no clue if
> > > acpi_index would be transmitted over wire or not?
> > >       
> > > > > > However, on piix4 it's conditional, and the condition/test function
> > > > > > ends up having the wrong pointer passed to it (it gets a PIIX4PMState
> > > > > > not the AcpiPciHpState it was expecting, because VMSTATE_PCI_HOTPLUG
> > > > > > is a macro and not another struct).  This means the field is randomly
> > > > > > loaded/saved based on a random pointer.  In 6.x this random pointer
> > > > > > randomly seems to get 0 for everyone (!); in 7.0rc it's getting junk
> > > > > > and trying to load a field that the source didn't send.  The migration
> > > > > > stream gets out of line and hits the section footer.    
> > > > > 
> > > > > I'm a bit confused by description,
> > > > > do you have a reproducer for me to try?    
> > > > 
> > > > Yeh, see the linked gitlab case command line:
> > > >   https://gitlab.com/qemu-project/qemu/-/issues/932
> > > > 
> > > > ./x86_64-softmmu/qemu-system-x86_64 -M pc-q35-6.2 -m 512 -device virtio-scsi-pci,id=scsihw0,bus=pcie.0,addr=0x5,acpi-index=3 -drive if=none,my.qcow2,format=qcow2,id=drive-scsi0,node-name=scsi0 -device 'scsi-hd,bus=scsihw0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0,id=scsi0'  -nographic
> > > > 
> > > > just migrating from a 6.2 to a head triggers this.  
> > > 
> > > Over here any migration from qemu-6.2 to HEAD at 3d31fe4d662f13c7
> > > fails even without acpi-index, as simple as this:
> > > 
> > > qemu-system-x86_64-6.2 -M pc-i440fx-6.2  -m 512 -vnc :0 -monitor stdio
> > > (qemu) stop
> > > (qemu) migrate "exec:gzip -c > STATEFILE.gz"
> > > 
> > > qemu-system-x86_64-7.0 -M pc-i440fx-6.2  -m 512 -vnc :0 -monitor stdio -incoming "exec: gzip -c -d STATEFILE.gz"
> > > 
> > > (qemu) qemu-system-x86_64-7.0: Missing section footer for 0000:00:01.3/piix4_pm
> > > qemu-system-x86_64-7.0: load of migration failed: Invalid argument
> > > 
> > > 
> > > Like you pointed out in gitlab issue, vmstate_acpi_pcihp_use_acpi_index
> > > is broken. Following applied to HEAD should fix immediate issue on destination
> > > reading random value:
> > > 
> > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > > index f0b5fac44a..c97db491c8 100644
> > > --- a/hw/acpi/piix4.c
> > > +++ b/hw/acpi/piix4.c
> > > @@ -269,6 +269,11 @@ static bool piix4_vmstate_need_smbus(void *opaque, int version_id)
> > >      return pm_smbus_vmstate_needed();
> > >  }
> > >  
> > > +static bool vmstate_piix4_need_acpi_index(void *opaque, int version_id)
> > > +{
> > > +    PIIX4PMState *s = PIIX4_PM(opaque);
> > > +    return vmstate_acpi_pcihp_use_acpi_index(&(s->acpi_pci_hotplug), version_id);
> > > +}  
> > 
> > But if acpi_index was set on the source 6.2 host, it won't send the
> > index, but the 7.0 would expect it, and it would fail in the same way
> > wouldn't it?
> 
> With piix4 fixed up 7.0 won't expect field as s->acpi_index initialized to 0
> so check will always return 0 and the field won't be expected.
> ( testing confirms it).
> If test on 6.2 host somehow manages to return 1, destination won't
> be able to accept it, because it has no idea about it (that is not fixable, I'm afraid).
> 
> For Q35 we set check  to NULL
>         VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug,
>                             ICH9LPCPMRegs,
>                             NULL, NULL),
> 
> which if I read vmstate_load_state() correctly will always expect
> the field and will always store fields since field->version_id == 0
> for VMSTATE_UINT32_TEST.
> 
> So we can't remove field without breaking Q35.

Yes.

> Net effect:
>   * not send the field for PC machine (ever)
>   * send field always for Q35 (always)
> 
> So your patch is good with fixed commit message
> and a comment close to the field that it's not really used with piix4

Could you write a new commit message based on mine?

> And to make migration of acpi_index on PC machine working,
> we need add an extra subsection that should be able to
> handle conditional value.

Yes; I hadn't realised acpi_index was actually writeable.

Dave

> 
> > 
> > Dave
> > 
> > >  /* qemu-kvm 1.2 uses version 3 but advertised as 2
> > >   * To support incoming qemu-kvm 1.2 migration, change version_id
> > >   * and minimum_version_id to 2 below (which breaks migration from
> > > @@ -299,7 +304,7 @@ static const VMStateDescription vmstate_acpi = {
> > >              struct AcpiPciHpPciStatus),
> > >          VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState,
> > >                              vmstate_test_use_acpi_hotplug_bridge,
> > > -                            vmstate_acpi_pcihp_use_acpi_index),
> > > +                            vmstate_piix4_need_acpi_index),
> > >          VMSTATE_END_OF_LIST()
> > >      },
> > >      .subsections = (const VMStateDescription*[]) {
> > > 
> > >   
> > > > Dave
> > > >   
> > > > > > The bodge is on piix4 never to load the field:
> > > > > >   a) Most 6.x builds never send it, so most of the time the migration
> > > > > >     will work.
> > > > > >   b) We can backport this fix to 6.x to remove the boobytrap.
> > > > > >   c) It should never have made a difference anyway since the acpi-index
> > > > > >     is command line configured and should be correct on the destination
> > > > > >     anyway
> > > > > >   d) ich9 is still sending/receiving this (unconditionally all the time)
> > > > > >     but due to (c) should never notice.  We could follow up to make it
> > > > > >     skip.
> > > > > > 
> > > > > > It worries me just when (a) actually happens.
> > > > > > 
> > > > > > Fixes: b32bd76 ("pci: introduce acpi-index property for PCI device")
> > > > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/932
> > > > > > 
> > > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > > > > ---
> > > > > >  hw/acpi/acpi-pci-hotplug-stub.c |  4 ----
> > > > > >  hw/acpi/pcihp.c                 |  6 ------
> > > > > >  hw/acpi/piix4.c                 | 11 ++++++++++-
> > > > > >  include/hw/acpi/pcihp.h         |  2 --
> > > > > >  4 files changed, 10 insertions(+), 13 deletions(-)
> > > > > > 
> > > > > > diff --git a/hw/acpi/acpi-pci-hotplug-stub.c b/hw/acpi/acpi-pci-hotplug-stub.c
> > > > > > index 734e4c5986..a43f6dafc9 100644
> > > > > > --- a/hw/acpi/acpi-pci-hotplug-stub.c
> > > > > > +++ b/hw/acpi/acpi-pci-hotplug-stub.c
> > > > > > @@ -41,7 +41,3 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
> > > > > >      return;
> > > > > >  }
> > > > > >  
> > > > > > -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
> > > > > > -{
> > > > > > -    return false;
> > > > > > -}
> > > > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > > > > index 6351bd3424..bf65bbea49 100644
> > > > > > --- a/hw/acpi/pcihp.c
> > > > > > +++ b/hw/acpi/pcihp.c
> > > > > > @@ -554,12 +554,6 @@ void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
> > > > > >                                     OBJ_PROP_FLAG_READ);
> > > > > >  }
> > > > > >  
> > > > > > -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
> > > > > > -{
> > > > > > -     AcpiPciHpState *s = opaque;
> > > > > > -     return s->acpi_index;
> > > > > > -}
> > > > > > -
> > > > > >  const VMStateDescription vmstate_acpi_pcihp_pci_status = {
> > > > > >      .name = "acpi_pcihp_pci_status",
> > > > > >      .version_id = 1,
> > > > > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > > > > > index cc37fa3416..48aeedd5f0 100644
> > > > > > --- a/hw/acpi/piix4.c
> > > > > > +++ b/hw/acpi/piix4.c
> > > > > > @@ -267,6 +267,15 @@ static bool piix4_vmstate_need_smbus(void *opaque, int version_id)
> > > > > >      return pm_smbus_vmstate_needed();
> > > > > >  }
> > > > > >  
> > > > > > +/*
> > > > > > + * This is a fudge to turn off the acpi_index field, whose
> > > > > > + * test was always broken on piix4.
> > > > > > + */
> > > > > > +static bool vmstate_test_never(void *opaque, int version_id)
> > > > > > +{
> > > > > > +    return false;
> > > > > > +}
> > > > > > +
> > > > > >  /* qemu-kvm 1.2 uses version 3 but advertised as 2
> > > > > >   * To support incoming qemu-kvm 1.2 migration, change version_id
> > > > > >   * and minimum_version_id to 2 below (which breaks migration from
> > > > > > @@ -297,7 +306,7 @@ static const VMStateDescription vmstate_acpi = {
> > > > > >              struct AcpiPciHpPciStatus),
> > > > > >          VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState,
> > > > > >                              vmstate_test_use_acpi_hotplug_bridge,
> > > > > > -                            vmstate_acpi_pcihp_use_acpi_index),
> > > > > > +                            vmstate_test_never),
> > > > > >          VMSTATE_END_OF_LIST()
> > > > > >      },
> > > > > >      .subsections = (const VMStateDescription*[]) {
> > > > > > diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> > > > > > index af1a169fc3..7e268c2c9c 100644
> > > > > > --- a/include/hw/acpi/pcihp.h
> > > > > > +++ b/include/hw/acpi/pcihp.h
> > > > > > @@ -73,8 +73,6 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off);
> > > > > >  
> > > > > >  extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
> > > > > >  
> > > > > > -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id);
> > > > > > -
> > > > > >  #define VMSTATE_PCI_HOTPLUG(pcihp, state, test_pcihp, test_acpi_index) \
> > > > > >          VMSTATE_UINT32_TEST(pcihp.hotplug_select, state, \
> > > > > >                              test_pcihp), \    
> > > > >     
> > >   
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH] acpi: Bodge acpi_index migration

[Igor Mammedov]

On Wed, 6 Apr 2022 18:36:41 +0100
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

> * Igor Mammedov (imammedo@redhat.com) wrote:
> > On Wed, 6 Apr 2022 17:11:09 +0100
> > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> >   
> > > * Igor Mammedov (imammedo@redhat.com) wrote:  
> > > > On Wed, 6 Apr 2022 10:38:51 +0100
> > > > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > > >     
> > > > > * Igor Mammedov (imammedo@redhat.com) wrote:    
> > > > > > On Wed,  6 Apr 2022 09:35:31 +0100
> > > > > > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> > > > > >       
> > > > > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > > > > > 
> > > > > > > The 'acpi_index' field is a statically configured field, which for
> > > > > > > some reason is migrated; this never makes much sense because it's
> > > > > > > command line static.      
> > > > > > 
> > > > > > that's true only for the field that's part of PCIDEvice,
> > > > > > however AcpiPciHpState::acpi_index is runtime state and _must_
> > > > > > be migrated if set, otherwise guest might get wrong index
> > > > > > if it's in process of querying it      
> > > > > 
> > > > > So this patch only changes the piix4.c version; I'm confused, is there
> > > > > a AcpiPciHpState::acpi_index that's runtime setable in there?    
> > > >     
> > > > > > >          VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState,    
> > > >                                     ^^^ AcpiPciHpState    
> > > > > > >                              vmstate_test_use_acpi_hotplug_bridge,
> > > > > > > -                            vmstate_acpi_pcihp_use_acpi_index),    
> > > > 
> > > > hw/acpi/pcihp.c:pci_write():
> > > >    s->acpi_index = object_property_get_uint(o, "acpi-index", NULL);
> > > > 
> > > > s->acpi_index is runtime value that is supposed to be migrated if it's set
> > > > to something other then 0
> > > > 
> > > > I may have botched VMSTATE_PCI_HOTPLUG, intent was to migrate
> > > > AcpiPciHpState::acpi_index if necessary. But I'm not sure how
> > > > if I used correct approach for to migrate an optional value
> > > > i.e.  maybe instead of VMSTATE_UINT32_TEST(pcihp.acpi_index, state, test_acpi_index)
> > > > I should've used subsection, because destination has no clue if
> > > > acpi_index would be transmitted over wire or not?
> > > >         
> > > > > > > However, on piix4 it's conditional, and the condition/test function
> > > > > > > ends up having the wrong pointer passed to it (it gets a PIIX4PMState
> > > > > > > not the AcpiPciHpState it was expecting, because VMSTATE_PCI_HOTPLUG
> > > > > > > is a macro and not another struct).  This means the field is randomly
> > > > > > > loaded/saved based on a random pointer.  In 6.x this random pointer
> > > > > > > randomly seems to get 0 for everyone (!); in 7.0rc it's getting junk
> > > > > > > and trying to load a field that the source didn't send.  The migration
> > > > > > > stream gets out of line and hits the section footer.      
> > > > > > 
> > > > > > I'm a bit confused by description,
> > > > > > do you have a reproducer for me to try?      
> > > > > 
> > > > > Yeh, see the linked gitlab case command line:
> > > > >   https://gitlab.com/qemu-project/qemu/-/issues/932
> > > > > 
> > > > > ./x86_64-softmmu/qemu-system-x86_64 -M pc-q35-6.2 -m 512 -device virtio-scsi-pci,id=scsihw0,bus=pcie.0,addr=0x5,acpi-index=3 -drive if=none,my.qcow2,format=qcow2,id=drive-scsi0,node-name=scsi0 -device 'scsi-hd,bus=scsihw0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0,id=scsi0'  -nographic
> > > > > 
> > > > > just migrating from a 6.2 to a head triggers this.    
> > > > 
> > > > Over here any migration from qemu-6.2 to HEAD at 3d31fe4d662f13c7
> > > > fails even without acpi-index, as simple as this:
> > > > 
> > > > qemu-system-x86_64-6.2 -M pc-i440fx-6.2  -m 512 -vnc :0 -monitor stdio
> > > > (qemu) stop
> > > > (qemu) migrate "exec:gzip -c > STATEFILE.gz"
> > > > 
> > > > qemu-system-x86_64-7.0 -M pc-i440fx-6.2  -m 512 -vnc :0 -monitor stdio -incoming "exec: gzip -c -d STATEFILE.gz"
> > > > 
> > > > (qemu) qemu-system-x86_64-7.0: Missing section footer for 0000:00:01.3/piix4_pm
> > > > qemu-system-x86_64-7.0: load of migration failed: Invalid argument
> > > > 
> > > > 
> > > > Like you pointed out in gitlab issue, vmstate_acpi_pcihp_use_acpi_index
> > > > is broken. Following applied to HEAD should fix immediate issue on destination
> > > > reading random value:
> > > > 
> > > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > > > index f0b5fac44a..c97db491c8 100644
> > > > --- a/hw/acpi/piix4.c
> > > > +++ b/hw/acpi/piix4.c
> > > > @@ -269,6 +269,11 @@ static bool piix4_vmstate_need_smbus(void *opaque, int version_id)
> > > >      return pm_smbus_vmstate_needed();
> > > >  }
> > > >  
> > > > +static bool vmstate_piix4_need_acpi_index(void *opaque, int version_id)
> > > > +{
> > > > +    PIIX4PMState *s = PIIX4_PM(opaque);
> > > > +    return vmstate_acpi_pcihp_use_acpi_index(&(s->acpi_pci_hotplug), version_id);
> > > > +}    
> > > 
> > > But if acpi_index was set on the source 6.2 host, it won't send the
> > > index, but the 7.0 would expect it, and it would fail in the same way
> > > wouldn't it?  
> > 
> > With piix4 fixed up 7.0 won't expect field as s->acpi_index initialized to 0
> > so check will always return 0 and the field won't be expected.
> > ( testing confirms it).
> > If test on 6.2 host somehow manages to return 1, destination won't
> > be able to accept it, because it has no idea about it (that is not fixable, I'm afraid).
> > 
> > For Q35 we set check  to NULL
> >         VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug,
> >                             ICH9LPCPMRegs,
> >                             NULL, NULL),
> > 
> > which if I read vmstate_load_state() correctly will always expect
> > the field and will always store fields since field->version_id == 0
> > for VMSTATE_UINT32_TEST.
> > 
> > So we can't remove field without breaking Q35.  
> 
> Yes.
> 
> > Net effect:
> >   * not send the field for PC machine (ever)
> >   * send field always for Q35 (always)
> > 
> > So your patch is good with fixed commit message
> > and a comment close to the field that it's not really used with piix4  
> 
> Could you write a new commit message based on mine?

I ended up rewriting patch (kept removals), but replaced test_never
with compat knob 'send field always for 7.0 and don't end it ever for older'
to match q35 and so we wouldn't have to add subsection for piix4 only.

Will post it shortly.

> 
> > And to make migration of acpi_index on PC machine working,
> > we need add an extra subsection that should be able to
> > handle conditional value.  
> 
> Yes; I hadn't realised acpi_index was actually writeable.
> 
> Dave
> 
> >   
> > > 
> > > Dave
> > >   
> > > >  /* qemu-kvm 1.2 uses version 3 but advertised as 2
> > > >   * To support incoming qemu-kvm 1.2 migration, change version_id
> > > >   * and minimum_version_id to 2 below (which breaks migration from
> > > > @@ -299,7 +304,7 @@ static const VMStateDescription vmstate_acpi = {
> > > >              struct AcpiPciHpPciStatus),
> > > >          VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState,
> > > >                              vmstate_test_use_acpi_hotplug_bridge,
> > > > -                            vmstate_acpi_pcihp_use_acpi_index),
> > > > +                            vmstate_piix4_need_acpi_index),
> > > >          VMSTATE_END_OF_LIST()
> > > >      },
> > > >      .subsections = (const VMStateDescription*[]) {
> > > > 
> > > >     
> > > > > Dave
> > > > >     
> > > > > > > The bodge is on piix4 never to load the field:
> > > > > > >   a) Most 6.x builds never send it, so most of the time the migration
> > > > > > >     will work.
> > > > > > >   b) We can backport this fix to 6.x to remove the boobytrap.
> > > > > > >   c) It should never have made a difference anyway since the acpi-index
> > > > > > >     is command line configured and should be correct on the destination
> > > > > > >     anyway
> > > > > > >   d) ich9 is still sending/receiving this (unconditionally all the time)
> > > > > > >     but due to (c) should never notice.  We could follow up to make it
> > > > > > >     skip.
> > > > > > > 
> > > > > > > It worries me just when (a) actually happens.
> > > > > > > 
> > > > > > > Fixes: b32bd76 ("pci: introduce acpi-index property for PCI device")
> > > > > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/932
> > > > > > > 
> > > > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > > > > > ---
> > > > > > >  hw/acpi/acpi-pci-hotplug-stub.c |  4 ----
> > > > > > >  hw/acpi/pcihp.c                 |  6 ------
> > > > > > >  hw/acpi/piix4.c                 | 11 ++++++++++-
> > > > > > >  include/hw/acpi/pcihp.h         |  2 --
> > > > > > >  4 files changed, 10 insertions(+), 13 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/hw/acpi/acpi-pci-hotplug-stub.c b/hw/acpi/acpi-pci-hotplug-stub.c
> > > > > > > index 734e4c5986..a43f6dafc9 100644
> > > > > > > --- a/hw/acpi/acpi-pci-hotplug-stub.c
> > > > > > > +++ b/hw/acpi/acpi-pci-hotplug-stub.c
> > > > > > > @@ -41,7 +41,3 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
> > > > > > >      return;
> > > > > > >  }
> > > > > > >  
> > > > > > > -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
> > > > > > > -{
> > > > > > > -    return false;
> > > > > > > -}
> > > > > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > > > > > index 6351bd3424..bf65bbea49 100644
> > > > > > > --- a/hw/acpi/pcihp.c
> > > > > > > +++ b/hw/acpi/pcihp.c
> > > > > > > @@ -554,12 +554,6 @@ void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
> > > > > > >                                     OBJ_PROP_FLAG_READ);
> > > > > > >  }
> > > > > > >  
> > > > > > > -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
> > > > > > > -{
> > > > > > > -     AcpiPciHpState *s = opaque;
> > > > > > > -     return s->acpi_index;
> > > > > > > -}
> > > > > > > -
> > > > > > >  const VMStateDescription vmstate_acpi_pcihp_pci_status = {
> > > > > > >      .name = "acpi_pcihp_pci_status",
> > > > > > >      .version_id = 1,
> > > > > > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > > > > > > index cc37fa3416..48aeedd5f0 100644
> > > > > > > --- a/hw/acpi/piix4.c
> > > > > > > +++ b/hw/acpi/piix4.c
> > > > > > > @@ -267,6 +267,15 @@ static bool piix4_vmstate_need_smbus(void *opaque, int version_id)
> > > > > > >      return pm_smbus_vmstate_needed();
> > > > > > >  }
> > > > > > >  
> > > > > > > +/*
> > > > > > > + * This is a fudge to turn off the acpi_index field, whose
> > > > > > > + * test was always broken on piix4.
> > > > > > > + */
> > > > > > > +static bool vmstate_test_never(void *opaque, int version_id)
> > > > > > > +{
> > > > > > > +    return false;
> > > > > > > +}
> > > > > > > +
> > > > > > >  /* qemu-kvm 1.2 uses version 3 but advertised as 2
> > > > > > >   * To support incoming qemu-kvm 1.2 migration, change version_id
> > > > > > >   * and minimum_version_id to 2 below (which breaks migration from
> > > > > > > @@ -297,7 +306,7 @@ static const VMStateDescription vmstate_acpi = {
> > > > > > >              struct AcpiPciHpPciStatus),
> > > > > > >          VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState,
> > > > > > >                              vmstate_test_use_acpi_hotplug_bridge,
> > > > > > > -                            vmstate_acpi_pcihp_use_acpi_index),
> > > > > > > +                            vmstate_test_never),
> > > > > > >          VMSTATE_END_OF_LIST()
> > > > > > >      },
> > > > > > >      .subsections = (const VMStateDescription*[]) {
> > > > > > > diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> > > > > > > index af1a169fc3..7e268c2c9c 100644
> > > > > > > --- a/include/hw/acpi/pcihp.h
> > > > > > > +++ b/include/hw/acpi/pcihp.h
> > > > > > > @@ -73,8 +73,6 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off);
> > > > > > >  
> > > > > > >  extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
> > > > > > >  
> > > > > > > -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id);
> > > > > > > -
> > > > > > >  #define VMSTATE_PCI_HOTPLUG(pcihp, state, test_pcihp, test_acpi_index) \
> > > > > > >          VMSTATE_UINT32_TEST(pcihp.hotplug_select, state, \
> > > > > > >                              test_pcihp), \      
> > > > > >       
> > > >     
> >   

Re: [PATCH] acpi: Bodge acpi_index migration

[Dr. David Alan Gilbert]

* Alex Williamson (alex.williamson@redhat.com) wrote:
> On Tue,  5 Apr 2022 20:06:58 +0100
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > The 'acpi_index' field is a statically configured field, which for
> > some reason is migrated; this never makes much sense because it's
> > command line static.
> > 
> > However, on piix4 it's conditional, and the condition/test function
> > ends up having the wrong pointer passed to it (it gets a PIIX4PMState
> > not the AcpiPciHpState it was expecting, because VMSTATE_PCI_HOTPLUG
> > is a macro and not another struct).  This means the field is randomly
> > loaded/saved based on a random pointer.  In 6.x this random pointer
> > randomly seems to get 0 for everyone (!); in 7.0rc it's getting junk
> > and trying to load a field that the source didn't send.
> 
> FWIW, after some hunting and pecking, 6.2 (64bit):

Ah thanks for doing that.

> (gdb) p &((struct AcpiPciHpState *)0)->acpi_index
> $1 = (uint32_t *) 0xc04
> 
> (gdb) p &((struct PIIX4PMState *)0)->ar.tmr.io.addr
> $2 = (hwaddr *) 0xc00
> 
> f53faa70bb63:
> 
> (gdb) p &((struct AcpiPciHpState *)0)->acpi_index
> $1 = (uint32_t *) 0xc04
> 
> (gdb) p &((struct PIIX4PMState *)0)->io_gpe.coalesced.tqh_circ.tql_prev
> $2 = (struct QTailQLink **) 0xc00
> 
> So yeah, it seems 0xc04 will always be part of a pointer on current
> mainline.  I can't really speak to the ACPIPMTimer MemoryRegion in the
> PIIX4PMState, maybe if there's a hwaddr it's always 32bit and the upper
> dword is reliably zero?  Thanks,

I'm a bit confused by that:

    memory_region_init_io(&ar->tmr.io, memory_region_owner(parent),
                          &acpi_pm_tmr_ops, ar, "acpi-tmr", 4);
    memory_region_add_subregion(parent, 8, &ar->tmr.io);
    .write = acpi_pm_tmr_write,
static void acpi_pm_tmr_write(void *opaque, hwaddr addr, uint64_t val,
                              unsigned width)
{
    /* nothing */
}

so, hmm I don't see it writing to that?

Dave

> Alex
> 
> >  The migration
> > stream gets out of line and hits the section footer.
> > 
> > The bodge is on piix4 never to load the field:
> >   a) Most 6.x builds never send it, so most of the time the migration
> >     will work.
> >   b) We can backport this fix to 6.x to remove the boobytrap.
> >   c) It should never have made a difference anyway since the acpi-index
> >     is command line configured and should be correct on the destination
> >     anyway
> >   d) ich9 is still sending/receiving this (unconditionally all the time)
> >     but due to (c) should never notice.  We could follow up to make it
> >     skip.
> > 
> > It worries me just when (a) actually happens.
> > 
> > Fixes: b32bd76 ("pci: introduce acpi-index property for PCI device")
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/932
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  hw/acpi/acpi-pci-hotplug-stub.c |  4 ----
> >  hw/acpi/pcihp.c                 |  6 ------
> >  hw/acpi/piix4.c                 | 11 ++++++++++-
> >  include/hw/acpi/pcihp.h         |  2 --
> >  4 files changed, 10 insertions(+), 13 deletions(-)
> > 
> > diff --git a/hw/acpi/acpi-pci-hotplug-stub.c b/hw/acpi/acpi-pci-hotplug-stub.c
> > index 734e4c5986..a43f6dafc9 100644
> > --- a/hw/acpi/acpi-pci-hotplug-stub.c
> > +++ b/hw/acpi/acpi-pci-hotplug-stub.c
> > @@ -41,7 +41,3 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
> >      return;
> >  }
> >  
> > -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
> > -{
> > -    return false;
> > -}
> > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > index 6351bd3424..bf65bbea49 100644
> > --- a/hw/acpi/pcihp.c
> > +++ b/hw/acpi/pcihp.c
> > @@ -554,12 +554,6 @@ void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
> >                                     OBJ_PROP_FLAG_READ);
> >  }
> >  
> > -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
> > -{
> > -     AcpiPciHpState *s = opaque;
> > -     return s->acpi_index;
> > -}
> > -
> >  const VMStateDescription vmstate_acpi_pcihp_pci_status = {
> >      .name = "acpi_pcihp_pci_status",
> >      .version_id = 1,
> > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > index cc37fa3416..48aeedd5f0 100644
> > --- a/hw/acpi/piix4.c
> > +++ b/hw/acpi/piix4.c
> > @@ -267,6 +267,15 @@ static bool piix4_vmstate_need_smbus(void *opaque, int version_id)
> >      return pm_smbus_vmstate_needed();
> >  }
> >  
> > +/*
> > + * This is a fudge to turn off the acpi_index field, whose
> > + * test was always broken on piix4.
> > + */
> > +static bool vmstate_test_never(void *opaque, int version_id)
> > +{
> > +    return false;
> > +}
> > +
> >  /* qemu-kvm 1.2 uses version 3 but advertised as 2
> >   * To support incoming qemu-kvm 1.2 migration, change version_id
> >   * and minimum_version_id to 2 below (which breaks migration from
> > @@ -297,7 +306,7 @@ static const VMStateDescription vmstate_acpi = {
> >              struct AcpiPciHpPciStatus),
> >          VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState,
> >                              vmstate_test_use_acpi_hotplug_bridge,
> > -                            vmstate_acpi_pcihp_use_acpi_index),
> > +                            vmstate_test_never),
> >          VMSTATE_END_OF_LIST()
> >      },
> >      .subsections = (const VMStateDescription*[]) {
> > diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> > index af1a169fc3..7e268c2c9c 100644
> > --- a/include/hw/acpi/pcihp.h
> > +++ b/include/hw/acpi/pcihp.h
> > @@ -73,8 +73,6 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off);
> >  
> >  extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
> >  
> > -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id);
> > -
> >  #define VMSTATE_PCI_HOTPLUG(pcihp, state, test_pcihp, test_acpi_index) \
> >          VMSTATE_UINT32_TEST(pcihp.hotplug_select, state, \
> >                              test_pcihp), \
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH] acpi: Bodge acpi_index migration

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> On Tue, Apr 05, 2022 at 08:06:58PM +0100, Dr. David Alan Gilbert (git) wrote:
> 
> The patch is fine but pls repost as text not as
> application/octet-stream.

I've just reposted it with a fudged header that should be fine.
( 20220406083531.10217-1-dgilbert@redhat.com )

Dave

> Thanks!
> 
> -- 
> MST
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH] acpi: Bodge acpi_index migration

[Dr. David Alan Gilbert]

* Dr. David Alan Gilbert (git) (dgilbert@redhat.com) wrote:

Hmm no, still bad mimetype
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH] acpi: Bodge acpi_index migration

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

The 'acpi_index' field is a statically configured field, which for
some reason is migrated; this never makes much sense because it's
command line static.

However, on piix4 it's conditional, and the condition/test function
ends up having the wrong pointer passed to it (it gets a PIIX4PMState
not the AcpiPciHpState it was expecting, because VMSTATE_PCI_HOTPLUG
is a macro and not another struct).  This means the field is randomly
loaded/saved based on a random pointer.  In 6.x this random pointer
randomly seems to get 0 for everyone (!); in 7.0rc it's getting junk
and trying to load a field that the source didn't send.  The migration
stream gets out of line and hits the section footer.

The bodge is on piix4 never to load the field:
  a) Most 6.x builds never send it, so most of the time the migration
    will work.
  b) We can backport this fix to 6.x to remove the boobytrap.
  c) It should never have made a difference anyway since the acpi-index
    is command line configured and should be correct on the destination
    anyway
  d) ich9 is still sending/receiving this (unconditionally all the time)
    but due to (c) should never notice.  We could follow up to make it
    skip.

It worries me just when (a) actually happens.

Fixes: b32bd76 ("pci: introduce acpi-index property for PCI device")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/932

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/acpi/acpi-pci-hotplug-stub.c |  4 ----
 hw/acpi/pcihp.c                 |  6 ------
 hw/acpi/piix4.c                 | 11 ++++++++++-
 include/hw/acpi/pcihp.h         |  2 --
 4 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/hw/acpi/acpi-pci-hotplug-stub.c b/hw/acpi/acpi-pci-hotplug-stub.c
index 734e4c5986..a43f6dafc9 100644
--- a/hw/acpi/acpi-pci-hotplug-stub.c
+++ b/hw/acpi/acpi-pci-hotplug-stub.c
@@ -41,7 +41,3 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
     return;
 }
 
-bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
-{
-    return false;
-}
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 6351bd3424..bf65bbea49 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -554,12 +554,6 @@ void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
                                    OBJ_PROP_FLAG_READ);
 }
 
-bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
-{
-     AcpiPciHpState *s = opaque;
-     return s->acpi_index;
-}
-
 const VMStateDescription vmstate_acpi_pcihp_pci_status = {
     .name = "acpi_pcihp_pci_status",
     .version_id = 1,
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index cc37fa3416..48aeedd5f0 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -267,6 +267,15 @@ static bool piix4_vmstate_need_smbus(void *opaque, int version_id)
     return pm_smbus_vmstate_needed();
 }
 
+/*
+ * This is a fudge to turn off the acpi_index field, whose
+ * test was always broken on piix4.
+ */
+static bool vmstate_test_never(void *opaque, int version_id)
+{
+    return false;
+}
+
 /* qemu-kvm 1.2 uses version 3 but advertised as 2
  * To support incoming qemu-kvm 1.2 migration, change version_id
  * and minimum_version_id to 2 below (which breaks migration from
@@ -297,7 +306,7 @@ static const VMStateDescription vmstate_acpi = {
             struct AcpiPciHpPciStatus),
         VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState,
                             vmstate_test_use_acpi_hotplug_bridge,
-                            vmstate_acpi_pcihp_use_acpi_index),
+                            vmstate_test_never),
         VMSTATE_END_OF_LIST()
     },
     .subsections = (const VMStateDescription*[]) {
diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
index af1a169fc3..7e268c2c9c 100644
--- a/include/hw/acpi/pcihp.h
+++ b/include/hw/acpi/pcihp.h
@@ -73,8 +73,6 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off);
 
 extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
 
-bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id);
-
 #define VMSTATE_PCI_HOTPLUG(pcihp, state, test_pcihp, test_acpi_index) \
         VMSTATE_UINT32_TEST(pcihp.hotplug_select, state, \
                             test_pcihp), \
-- 
2.35.1

Re: [PATCH] acpi: Bodge acpi_index migration

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> On Tue, Apr 05, 2022 at 08:06:58PM +0100, Dr. David Alan Gilbert (git) wrote:
> 
> The patch is fine but pls repost as text not as
> application/octet-stream.

Let me try and figure out how; this is the same git send-email
I've used for years; it's our corporate mail setup that's screwing the
header up.

Dave

> Thanks!
> 
> -- 
> MST
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH] acpi: Bodge acpi_index migration

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 207 bytes --]

Good work! :)

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

On Tue, Apr 5, 2022 at 11:07 PM Dr. David Alan Gilbert (git) <
dgilbert@redhat.com> wrote:

>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 640 bytes --]

Re: [PATCH] acpi: Bodge acpi_index migration

[Fabian Ebner]

Am 05.04.22 um 21:06 schrieb Dr. David Alan Gilbert (git):
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> The 'acpi_index' field is a statically configured field, which for
> some reason is migrated; this never makes much sense because it's
> command line static.
> 
> However, on piix4 it's conditional, and the condition/test function
> ends up having the wrong pointer passed to it (it gets a PIIX4PMState
> not the AcpiPciHpState it was expecting, because VMSTATE_PCI_HOTPLUG
> is a macro and not another struct).  This means the field is randomly
> loaded/saved based on a random pointer.  In 6.x this random pointer
> randomly seems to get 0 for everyone (!); in 7.0rc it's getting junk
> and trying to load a field that the source didn't send.  The migration
> stream gets out of line and hits the section footer.
> 
> The bodge is on piix4 never to load the field:
>   a) Most 6.x builds never send it, so most of the time the migration
>     will work.
>   b) We can backport this fix to 6.x to remove the boobytrap.
>   c) It should never have made a difference anyway since the acpi-index
>     is command line configured and should be correct on the destination
>     anyway
>   d) ich9 is still sending/receiving this (unconditionally all the time)
>     but due to (c) should never notice.  We could follow up to make it
>     skip.
> 
> It worries me just when (a) actually happens.
> 
> Fixes: b32bd76 ("pci: introduce acpi-index property for PCI device")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/932
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  hw/acpi/acpi-pci-hotplug-stub.c |  4 ----
>  hw/acpi/pcihp.c                 |  6 ------
>  hw/acpi/piix4.c                 | 11 ++++++++++-
>  include/hw/acpi/pcihp.h         |  2 --
>  4 files changed, 10 insertions(+), 13 deletions(-)
> 

Fixes the issue for me, thanks!

Tested-by: Fabian Ebner <f.ebner@proxmox.com>

Re: [PATCH] acpi: Bodge acpi_index migration

[Michael S. Tsirkin]

On Tue, Apr 05, 2022 at 08:06:58PM +0100, Dr. David Alan Gilbert (git) wrote:

The patch is fine but pls repost as text not as
application/octet-stream.

Thanks!

-- 
MST

Re: [PATCH] acpi: Bodge acpi_index migration

[Alex Williamson]

On Tue,  5 Apr 2022 20:06:58 +0100
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> The 'acpi_index' field is a statically configured field, which for
> some reason is migrated; this never makes much sense because it's
> command line static.
> 
> However, on piix4 it's conditional, and the condition/test function
> ends up having the wrong pointer passed to it (it gets a PIIX4PMState
> not the AcpiPciHpState it was expecting, because VMSTATE_PCI_HOTPLUG
> is a macro and not another struct).  This means the field is randomly
> loaded/saved based on a random pointer.  In 6.x this random pointer
> randomly seems to get 0 for everyone (!); in 7.0rc it's getting junk
> and trying to load a field that the source didn't send.

FWIW, after some hunting and pecking, 6.2 (64bit):

(gdb) p &((struct AcpiPciHpState *)0)->acpi_index
$1 = (uint32_t *) 0xc04

(gdb) p &((struct PIIX4PMState *)0)->ar.tmr.io.addr
$2 = (hwaddr *) 0xc00

f53faa70bb63:

(gdb) p &((struct AcpiPciHpState *)0)->acpi_index
$1 = (uint32_t *) 0xc04

(gdb) p &((struct PIIX4PMState *)0)->io_gpe.coalesced.tqh_circ.tql_prev
$2 = (struct QTailQLink **) 0xc00

So yeah, it seems 0xc04 will always be part of a pointer on current
mainline.  I can't really speak to the ACPIPMTimer MemoryRegion in the
PIIX4PMState, maybe if there's a hwaddr it's always 32bit and the upper
dword is reliably zero?  Thanks,

Alex

>  The migration
> stream gets out of line and hits the section footer.
> 
> The bodge is on piix4 never to load the field:
>   a) Most 6.x builds never send it, so most of the time the migration
>     will work.
>   b) We can backport this fix to 6.x to remove the boobytrap.
>   c) It should never have made a difference anyway since the acpi-index
>     is command line configured and should be correct on the destination
>     anyway
>   d) ich9 is still sending/receiving this (unconditionally all the time)
>     but due to (c) should never notice.  We could follow up to make it
>     skip.
> 
> It worries me just when (a) actually happens.
> 
> Fixes: b32bd76 ("pci: introduce acpi-index property for PCI device")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/932
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  hw/acpi/acpi-pci-hotplug-stub.c |  4 ----
>  hw/acpi/pcihp.c                 |  6 ------
>  hw/acpi/piix4.c                 | 11 ++++++++++-
>  include/hw/acpi/pcihp.h         |  2 --
>  4 files changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/acpi/acpi-pci-hotplug-stub.c b/hw/acpi/acpi-pci-hotplug-stub.c
> index 734e4c5986..a43f6dafc9 100644
> --- a/hw/acpi/acpi-pci-hotplug-stub.c
> +++ b/hw/acpi/acpi-pci-hotplug-stub.c
> @@ -41,7 +41,3 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
>      return;
>  }
>  
> -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
> -{
> -    return false;
> -}
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 6351bd3424..bf65bbea49 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -554,12 +554,6 @@ void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
>                                     OBJ_PROP_FLAG_READ);
>  }
>  
> -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
> -{
> -     AcpiPciHpState *s = opaque;
> -     return s->acpi_index;
> -}
> -
>  const VMStateDescription vmstate_acpi_pcihp_pci_status = {
>      .name = "acpi_pcihp_pci_status",
>      .version_id = 1,
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index cc37fa3416..48aeedd5f0 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -267,6 +267,15 @@ static bool piix4_vmstate_need_smbus(void *opaque, int version_id)
>      return pm_smbus_vmstate_needed();
>  }
>  
> +/*
> + * This is a fudge to turn off the acpi_index field, whose
> + * test was always broken on piix4.
> + */
> +static bool vmstate_test_never(void *opaque, int version_id)
> +{
> +    return false;
> +}
> +
>  /* qemu-kvm 1.2 uses version 3 but advertised as 2
>   * To support incoming qemu-kvm 1.2 migration, change version_id
>   * and minimum_version_id to 2 below (which breaks migration from
> @@ -297,7 +306,7 @@ static const VMStateDescription vmstate_acpi = {
>              struct AcpiPciHpPciStatus),
>          VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState,
>                              vmstate_test_use_acpi_hotplug_bridge,
> -                            vmstate_acpi_pcihp_use_acpi_index),
> +                            vmstate_test_never),
>          VMSTATE_END_OF_LIST()
>      },
>      .subsections = (const VMStateDescription*[]) {
> diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> index af1a169fc3..7e268c2c9c 100644
> --- a/include/hw/acpi/pcihp.h
> +++ b/include/hw/acpi/pcihp.h
> @@ -73,8 +73,6 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off);
>  
>  extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
>  
> -bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id);
> -
>  #define VMSTATE_PCI_HOTPLUG(pcihp, state, test_pcihp, test_acpi_index) \
>          VMSTATE_UINT32_TEST(pcihp.hotplug_select, state, \
>                              test_pcihp), \

[PATCH] acpi: Bodge acpi_index migration

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

The 'acpi_index' field is a statically configured field, which for
some reason is migrated; this never makes much sense because it's
command line static.

However, on piix4 it's conditional, and the condition/test function
ends up having the wrong pointer passed to it (it gets a PIIX4PMState
not the AcpiPciHpState it was expecting, because VMSTATE_PCI_HOTPLUG
is a macro and not another struct).  This means the field is randomly
loaded/saved based on a random pointer.  In 6.x this random pointer
randomly seems to get 0 for everyone (!); in 7.0rc it's getting junk
and trying to load a field that the source didn't send.  The migration
stream gets out of line and hits the section footer.

The bodge is on piix4 never to load the field:
  a) Most 6.x builds never send it, so most of the time the migration
    will work.
  b) We can backport this fix to 6.x to remove the boobytrap.
  c) It should never have made a difference anyway since the acpi-index
    is command line configured and should be correct on the destination
    anyway
  d) ich9 is still sending/receiving this (unconditionally all the time)
    but due to (c) should never notice.  We could follow up to make it
    skip.

It worries me just when (a) actually happens.

Fixes: b32bd76 ("pci: introduce acpi-index property for PCI device")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/932

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/acpi/acpi-pci-hotplug-stub.c |  4 ----
 hw/acpi/pcihp.c                 |  6 ------
 hw/acpi/piix4.c                 | 11 ++++++++++-
 include/hw/acpi/pcihp.h         |  2 --
 4 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/hw/acpi/acpi-pci-hotplug-stub.c b/hw/acpi/acpi-pci-hotplug-stub.c
index 734e4c5986..a43f6dafc9 100644
--- a/hw/acpi/acpi-pci-hotplug-stub.c
+++ b/hw/acpi/acpi-pci-hotplug-stub.c
@@ -41,7 +41,3 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
     return;
 }
 
-bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
-{
-    return false;
-}
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 6351bd3424..bf65bbea49 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -554,12 +554,6 @@ void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
                                    OBJ_PROP_FLAG_READ);
 }
 
-bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
-{
-     AcpiPciHpState *s = opaque;
-     return s->acpi_index;
-}
-
 const VMStateDescription vmstate_acpi_pcihp_pci_status = {
     .name = "acpi_pcihp_pci_status",
     .version_id = 1,
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index cc37fa3416..48aeedd5f0 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -267,6 +267,15 @@ static bool piix4_vmstate_need_smbus(void *opaque, int version_id)
     return pm_smbus_vmstate_needed();
 }
 
+/*
+ * This is a fudge to turn off the acpi_index field, whose
+ * test was always broken on piix4.
+ */
+static bool vmstate_test_never(void *opaque, int version_id)
+{
+    return false;
+}
+
 /* qemu-kvm 1.2 uses version 3 but advertised as 2
  * To support incoming qemu-kvm 1.2 migration, change version_id
  * and minimum_version_id to 2 below (which breaks migration from
@@ -297,7 +306,7 @@ static const VMStateDescription vmstate_acpi = {
             struct AcpiPciHpPciStatus),
         VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState,
                             vmstate_test_use_acpi_hotplug_bridge,
-                            vmstate_acpi_pcihp_use_acpi_index),
+                            vmstate_test_never),
         VMSTATE_END_OF_LIST()
     },
     .subsections = (const VMStateDescription*[]) {
diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
index af1a169fc3..7e268c2c9c 100644
--- a/include/hw/acpi/pcihp.h
+++ b/include/hw/acpi/pcihp.h
@@ -73,8 +73,6 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off);
 
 extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
 
-bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id);
-
 #define VMSTATE_PCI_HOTPLUG(pcihp, state, test_pcihp, test_acpi_index) \
         VMSTATE_UINT32_TEST(pcihp.hotplug_select, state, \
                             test_pcihp), \
-- 
2.35.1