| Message ID | 1478663122-29357-1-git-send-email-david@gibson.dropbear.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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), >
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), > > > >
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), >>> >> >> >
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),
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(-)