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(-)
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.1On 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), \
* 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
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), \
> >
* 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
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), \
> > > >
> >
* 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
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
© 2016 - 2024 Red Hat, Inc.