* [Qemu-devel] [PATCH] spapr: Fix migration of PCI host bridges from qemu-2.7
@ 2016-11-09 3:45 David Gibson
2016-11-09 5:14 ` Alexey Kardashevskiy
0 siblings, 1 reply; 4+ messages in thread
From: David Gibson @ 2016-11-09 3:45 UTC (permalink / raw)
To: aik, mdroth; +Cc: agraf, thuth, lvivier, qemu-ppc, qemu-devel, David Gibson
daa2369 "spapr_pci: Add a 64-bit MMIO window" subtly broke migration from
qemu-2.7 to the current version. It split the device's MMIO window into
two pieces for 32-bit and 64-bit MMIO.
The patch included backwards compatibility code to convert the old property
into the new format. However, the property value was also transferred in
the migration stream and compared with a (probably unwise) VMSTATE_EQUAL.
So, the "raw" value from 2.7 is compared to the new style converted value
from (pre-)2.8 giving a mismatch and migration failure.
Although it would be technically possible to fix this in a way allowing
backwards migration, that would leave an ugly legacy around indefinitely.
This patch takes the simpler approach of bumping the migration version,
dropping the unwise VMSTATE_EQUAL (and some equally unwise ones around it)
and ignoring them on an incoming migration.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
hw/ppc/spapr_pci.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 7cde30e..7f1cc29 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1658,19 +1658,24 @@ static int spapr_pci_post_load(void *opaque, int version_id)
return 0;
}
+static bool version_before_3(void *opaque, int version_id)
+{
+ return version_id < 3;
+}
+
static const VMStateDescription vmstate_spapr_pci = {
.name = "spapr_pci",
- .version_id = 2,
+ .version_id = 3,
.minimum_version_id = 2,
.pre_save = spapr_pci_pre_save,
.post_load = spapr_pci_post_load,
.fields = (VMStateField[]) {
VMSTATE_UINT64_EQUAL(buid, sPAPRPHBState),
- VMSTATE_UINT32_EQUAL(dma_liobn[0], sPAPRPHBState),
- VMSTATE_UINT64_EQUAL(mem_win_addr, sPAPRPHBState),
- VMSTATE_UINT64_EQUAL(mem_win_size, sPAPRPHBState),
- VMSTATE_UINT64_EQUAL(io_win_addr, sPAPRPHBState),
- VMSTATE_UINT64_EQUAL(io_win_size, sPAPRPHBState),
+ VMSTATE_UNUSED_TEST(version_before_3, sizeof(uint32_t) /* dma_liobn[0] */
+ + sizeof(uint64_t) /* mem_win_addr */
+ + sizeof(uint64_t) /* mem_win_size */
+ + sizeof(uint64_t) /* io_win_addr */
+ + sizeof(uint64_t) /* io_win_size */),
VMSTATE_STRUCT_ARRAY(lsi_table, sPAPRPHBState, PCI_NUM_PINS, 0,
vmstate_spapr_pci_lsi, struct spapr_pci_lsi),
VMSTATE_INT32(msi_devs_num, sPAPRPHBState),
--
2.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH] spapr: Fix migration of PCI host bridges from qemu-2.7
2016-11-09 3:45 [Qemu-devel] [PATCH] spapr: Fix migration of PCI host bridges from qemu-2.7 David Gibson
@ 2016-11-09 5:14 ` Alexey Kardashevskiy
2016-11-09 12:19 ` David Gibson
0 siblings, 1 reply; 4+ messages in thread
From: Alexey Kardashevskiy @ 2016-11-09 5:14 UTC (permalink / raw)
To: David Gibson, mdroth; +Cc: agraf, thuth, lvivier, qemu-ppc, qemu-devel
On 09/11/16 14:45, David Gibson wrote:
> daa2369 "spapr_pci: Add a 64-bit MMIO window" subtly broke migration from
> qemu-2.7 to the current version. It split the device's MMIO window into
> two pieces for 32-bit and 64-bit MMIO.
>
> The patch included backwards compatibility code to convert the old property
> into the new format. However, the property value was also transferred in
> the migration stream and compared with a (probably unwise) VMSTATE_EQUAL.
> So, the "raw" value from 2.7 is compared to the new style converted value
> from (pre-)2.8 giving a mismatch and migration failure.
>
> Although it would be technically possible to fix this in a way allowing
> backwards migration, that would leave an ugly legacy around indefinitely.
> This patch takes the simpler approach of bumping the migration version,
> dropping the unwise VMSTATE_EQUAL (and some equally unwise ones around it)
> and ignoring them on an incoming migration.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> hw/ppc/spapr_pci.c | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 7cde30e..7f1cc29 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1658,19 +1658,24 @@ static int spapr_pci_post_load(void *opaque, int version_id)
> return 0;
> }
>
> +static bool version_before_3(void *opaque, int version_id)
> +{
> + return version_id < 3;
> +}
> +
> static const VMStateDescription vmstate_spapr_pci = {
> .name = "spapr_pci",
> - .version_id = 2,
> + .version_id = 3,
> .minimum_version_id = 2,
> .pre_save = spapr_pci_pre_save,
> .post_load = spapr_pci_post_load,
> .fields = (VMStateField[]) {
> VMSTATE_UINT64_EQUAL(buid, sPAPRPHBState),
You could probably go one step further and get rid of @buid as well.
Nevertheless, this works,
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> - VMSTATE_UINT32_EQUAL(dma_liobn[0], sPAPRPHBState),
> - VMSTATE_UINT64_EQUAL(mem_win_addr, sPAPRPHBState),
> - VMSTATE_UINT64_EQUAL(mem_win_size, sPAPRPHBState),
> - VMSTATE_UINT64_EQUAL(io_win_addr, sPAPRPHBState),
> - VMSTATE_UINT64_EQUAL(io_win_size, sPAPRPHBState),
> + VMSTATE_UNUSED_TEST(version_before_3, sizeof(uint32_t) /* dma_liobn[0] */
> + + sizeof(uint64_t) /* mem_win_addr */
> + + sizeof(uint64_t) /* mem_win_size */
> + + sizeof(uint64_t) /* io_win_addr */
> + + sizeof(uint64_t) /* io_win_size */),
> VMSTATE_STRUCT_ARRAY(lsi_table, sPAPRPHBState, PCI_NUM_PINS, 0,
> vmstate_spapr_pci_lsi, struct spapr_pci_lsi),
> VMSTATE_INT32(msi_devs_num, sPAPRPHBState),
>
--
Alexey
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH] spapr: Fix migration of PCI host bridges from qemu-2.7
2016-11-09 5:14 ` Alexey Kardashevskiy
@ 2016-11-09 12:19 ` David Gibson
2016-11-09 12:39 ` Alexey Kardashevskiy
0 siblings, 1 reply; 4+ messages in thread
From: David Gibson @ 2016-11-09 12:19 UTC (permalink / raw)
To: Alexey Kardashevskiy; +Cc: mdroth, agraf, thuth, lvivier, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3593 bytes --]
On Wed, Nov 09, 2016 at 04:14:25PM +1100, Alexey Kardashevskiy wrote:
> On 09/11/16 14:45, David Gibson wrote:
> > daa2369 "spapr_pci: Add a 64-bit MMIO window" subtly broke migration from
> > qemu-2.7 to the current version. It split the device's MMIO window into
> > two pieces for 32-bit and 64-bit MMIO.
> >
> > The patch included backwards compatibility code to convert the old property
> > into the new format. However, the property value was also transferred in
> > the migration stream and compared with a (probably unwise) VMSTATE_EQUAL.
> > So, the "raw" value from 2.7 is compared to the new style converted value
> > from (pre-)2.8 giving a mismatch and migration failure.
> >
> > Although it would be technically possible to fix this in a way allowing
> > backwards migration, that would leave an ugly legacy around indefinitely.
> > This patch takes the simpler approach of bumping the migration version,
> > dropping the unwise VMSTATE_EQUAL (and some equally unwise ones around it)
> > and ignoring them on an incoming migration.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > hw/ppc/spapr_pci.c | 17 +++++++++++------
> > 1 file changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 7cde30e..7f1cc29 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1658,19 +1658,24 @@ static int spapr_pci_post_load(void *opaque, int version_id)
> > return 0;
> > }
> >
> > +static bool version_before_3(void *opaque, int version_id)
> > +{
> > + return version_id < 3;
> > +}
> > +
> > static const VMStateDescription vmstate_spapr_pci = {
> > .name = "spapr_pci",
> > - .version_id = 2,
> > + .version_id = 3,
> > .minimum_version_id = 2,
> > .pre_save = spapr_pci_pre_save,
> > .post_load = spapr_pci_post_load,
> > .fields = (VMStateField[]) {
> > VMSTATE_UINT64_EQUAL(buid, sPAPRPHBState),
>
>
> You could probably go one step further and get rid of @buid as well.
I thought about it. buid at least is specified state that's
vanishingly unlikely to change or disappear from the device. It also
does serve to make sure that QOM instance matching - which is always a
bit black magicy to me - is lining things up correctly.
>
> Nevertheless, this works,
>
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>
>
>
>
> > - VMSTATE_UINT32_EQUAL(dma_liobn[0], sPAPRPHBState),
> > - VMSTATE_UINT64_EQUAL(mem_win_addr, sPAPRPHBState),
> > - VMSTATE_UINT64_EQUAL(mem_win_size, sPAPRPHBState),
> > - VMSTATE_UINT64_EQUAL(io_win_addr, sPAPRPHBState),
> > - VMSTATE_UINT64_EQUAL(io_win_size, sPAPRPHBState),
> > + VMSTATE_UNUSED_TEST(version_before_3, sizeof(uint32_t) /* dma_liobn[0] */
> > + + sizeof(uint64_t) /* mem_win_addr */
> > + + sizeof(uint64_t) /* mem_win_size */
> > + + sizeof(uint64_t) /* io_win_addr */
> > + + sizeof(uint64_t) /* io_win_size */),
> > VMSTATE_STRUCT_ARRAY(lsi_table, sPAPRPHBState, PCI_NUM_PINS, 0,
> > vmstate_spapr_pci_lsi, struct spapr_pci_lsi),
> > VMSTATE_INT32(msi_devs_num, sPAPRPHBState),
> >
>
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH] spapr: Fix migration of PCI host bridges from qemu-2.7
2016-11-09 12:19 ` David Gibson
@ 2016-11-09 12:39 ` Alexey Kardashevskiy
0 siblings, 0 replies; 4+ messages in thread
From: Alexey Kardashevskiy @ 2016-11-09 12:39 UTC (permalink / raw)
To: David Gibson; +Cc: mdroth, agraf, thuth, lvivier, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3726 bytes --]
On 09/11/16 23:19, David Gibson wrote:
> On Wed, Nov 09, 2016 at 04:14:25PM +1100, Alexey Kardashevskiy wrote:
>> On 09/11/16 14:45, David Gibson wrote:
>>> daa2369 "spapr_pci: Add a 64-bit MMIO window" subtly broke migration from
>>> qemu-2.7 to the current version. It split the device's MMIO window into
>>> two pieces for 32-bit and 64-bit MMIO.
>>>
>>> The patch included backwards compatibility code to convert the old property
>>> into the new format. However, the property value was also transferred in
>>> the migration stream and compared with a (probably unwise) VMSTATE_EQUAL.
>>> So, the "raw" value from 2.7 is compared to the new style converted value
>>> from (pre-)2.8 giving a mismatch and migration failure.
>>>
>>> Although it would be technically possible to fix this in a way allowing
>>> backwards migration, that would leave an ugly legacy around indefinitely.
>>> This patch takes the simpler approach of bumping the migration version,
>>> dropping the unwise VMSTATE_EQUAL (and some equally unwise ones around it)
>>> and ignoring them on an incoming migration.
>>>
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>> ---
>>> hw/ppc/spapr_pci.c | 17 +++++++++++------
>>> 1 file changed, 11 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>>> index 7cde30e..7f1cc29 100644
>>> --- a/hw/ppc/spapr_pci.c
>>> +++ b/hw/ppc/spapr_pci.c
>>> @@ -1658,19 +1658,24 @@ static int spapr_pci_post_load(void *opaque, int version_id)
>>> return 0;
>>> }
>>>
>>> +static bool version_before_3(void *opaque, int version_id)
>>> +{
>>> + return version_id < 3;
>>> +}
>>> +
>>> static const VMStateDescription vmstate_spapr_pci = {
>>> .name = "spapr_pci",
>>> - .version_id = 2,
>>> + .version_id = 3,
>>> .minimum_version_id = 2,
>>> .pre_save = spapr_pci_pre_save,
>>> .post_load = spapr_pci_post_load,
>>> .fields = (VMStateField[]) {
>>> VMSTATE_UINT64_EQUAL(buid, sPAPRPHBState),
>>
>>
>> You could probably go one step further and get rid of @buid as well.
>
> I thought about it. buid at least is specified state that's
> vanishingly unlikely to change or disappear from the device. It also
> does serve to make sure that QOM instance matching - which is always a
> bit black magicy to me - is lining things up correctly.
afaict to fix matching properly, TYPE_SYS_BUS_DEVICE needs get_dev_path()
hook, just like spapr_vio_get_dev_name; otherwise SPAPR PHB in migration
always named as "spapr_pci" but yes, this would be much outside of the
scope of this patch :-/
>
>>
>> Nevertheless, this works,
>>
>> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>
>>
>>
>>
>>> - VMSTATE_UINT32_EQUAL(dma_liobn[0], sPAPRPHBState),
>>> - VMSTATE_UINT64_EQUAL(mem_win_addr, sPAPRPHBState),
>>> - VMSTATE_UINT64_EQUAL(mem_win_size, sPAPRPHBState),
>>> - VMSTATE_UINT64_EQUAL(io_win_addr, sPAPRPHBState),
>>> - VMSTATE_UINT64_EQUAL(io_win_size, sPAPRPHBState),
>>> + VMSTATE_UNUSED_TEST(version_before_3, sizeof(uint32_t) /* dma_liobn[0] */
>>> + + sizeof(uint64_t) /* mem_win_addr */
>>> + + sizeof(uint64_t) /* mem_win_size */
>>> + + sizeof(uint64_t) /* io_win_addr */
>>> + + sizeof(uint64_t) /* io_win_size */),
>>> VMSTATE_STRUCT_ARRAY(lsi_table, sPAPRPHBState, PCI_NUM_PINS, 0,
>>> vmstate_spapr_pci_lsi, struct spapr_pci_lsi),
>>> VMSTATE_INT32(msi_devs_num, sPAPRPHBState),
>>>
>>
>>
>
--
Alexey
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 839 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-11-09 12:39 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-09 3:45 [Qemu-devel] [PATCH] spapr: Fix migration of PCI host bridges from qemu-2.7 David Gibson
2016-11-09 5:14 ` Alexey Kardashevskiy
2016-11-09 12:19 ` David Gibson
2016-11-09 12:39 ` Alexey Kardashevskiy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).