Re: [Qemu-devel] [PATCH] spapr: Fix migration of PCI host bridges from qemu-2.7

[David Gibson <david@gibson.dropbear.id.au>]

[-- 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 --]