Re: [Qemu-devel] [PATCHv2 5/5] spapr: Fix 2.7<->2.8 migration of PCI host bridge

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

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

On Tue, Nov 22, 2016 at 07:17:27PM +1100, Alexey Kardashevskiy wrote:
> On 21/11/16 16:31, 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.
> > 
> > Along with the actual field that caused the breakage, there are
> > several other ill-advised VMSTATE_EQUAL()s.  To fix forwards
> > migration, we read the values in the stream into scratch variables and
> > ignore them, instead of comparing for equality.  To fix backwards
> > migration, we populate those scratch variables in pre_save() with
> > adjusted values to match the old behaviour.
> > 
> > To permit the eventual possibility of removing this cruft from the
> > stream, we only include these compatibility fields if a new
> > 'pre-2.8-migration' property is set.  We clear it on the pseries-2.8
> > machine type, which obviously can't be migrated backwards, but set it
> > on earlier machine type versions.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c              |  5 +++++
> >  hw/ppc/spapr_pci.c          | 33 ++++++++++++++++++++++++++++-----
> >  include/hw/pci-host/spapr.h |  6 ++++++
> >  3 files changed, 39 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 775ad2e..c3269c7 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2772,6 +2772,11 @@ DEFINE_SPAPR_MACHINE(2_8, "2.8", true);
> >          .driver = TYPE_POWERPC_CPU,                 \
> >          .property = "pre-2.8-migration",            \
> >          .value    = "on",                           \
> > +    },                                              \
> > +    {                                               \
> > +        .driver = TYPE_SPAPR_PCI_HOST_BRIDGE,       \
> > +        .property = "pre-2.8-migration",            \
> > +        .value    = "on",                           \
> >      },
> >  
> >  static void phb_placement_2_7(sPAPRMachineState *spapr, uint32_t index,
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index e429c94..c62c1cb 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1590,6 +1590,8 @@ static Property spapr_phb_properties[] = {
> >      DEFINE_PROP_UINT64("pgsz", sPAPRPHBState, page_size_mask,
> >                         (1ULL << 12) | (1ULL << 16)),
> >      DEFINE_PROP_UINT32("numa_node", sPAPRPHBState, numa_node, -1),
> > +    DEFINE_PROP_BOOL("pre-2.8-migration", sPAPRPHBState,
> > +                     pre_2_8_migration, false),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >  
> > @@ -1636,6 +1638,20 @@ static void spapr_pci_pre_save(void *opaque)
> >          sphb->msi_devs[i].key = *(uint32_t *) key;
> >          sphb->msi_devs[i].value = *(spapr_pci_msi *) value;
> >      }
> > +
> > +    if (sphb->pre_2_8_migration) {
> 
> 
> You check for pre_2_8_migration here but you do not in 3/5, what is the
> difference?

Uh.. where don't I in 3/5?

> Also this chunk did not apply on top of dwg/master or qemu.org/master (some
> whitespace issue? not sure) so I tested dwg/ppc-for-2.8, it works.

Yes, the series was based on ppc-for-2.8

> 
> 
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> 
> 
> 
> > +        sphb->mig_liobn = sphb->dma_liobn[0];
> > +        sphb->mig_mem_win_addr = sphb->mem_win_addr;
> > +        sphb->mig_mem_win_size = sphb->mem_win_size;
> > +        sphb->mig_io_win_addr = sphb->io_win_addr;
> > +        sphb->mig_io_win_size = sphb->io_win_size;
> > +
> > +        if ((sphb->mem64_win_size != 0)
> > +            && (sphb->mem64_win_addr
> > +                == (sphb->mem_win_addr + sphb->mem_win_size))) {
> > +            sphb->mig_mem_win_size += sphb->mem64_win_size;
> > +        }
> > +    }
> >  }
> >  
> >  /*
> > @@ -1680,6 +1696,13 @@ static int spapr_pci_post_load(void *opaque, int version_id)
> >      return 0;
> >  }
> >  
> > +static bool pre_2_8_migration(void *opaque, int version_id)
> > +{
> > +    sPAPRPHBState *sphb = opaque;
> > +
> > +    return sphb->pre_2_8_migration;
> > +}
> > +
> >  static const VMStateDescription vmstate_spapr_pci = {
> >      .name = "spapr_pci",
> >      .version_id = 2,
> > @@ -1688,11 +1711,11 @@ static const VMStateDescription vmstate_spapr_pci = {
> >      .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_UINT32_TEST(mig_liobn, sPAPRPHBState, pre_2_8_migration),
> > +        VMSTATE_UINT64_TEST(mig_mem_win_addr, sPAPRPHBState, pre_2_8_migration),
> > +        VMSTATE_UINT64_TEST(mig_mem_win_size, sPAPRPHBState, pre_2_8_migration),
> > +        VMSTATE_UINT64_TEST(mig_io_win_addr, sPAPRPHBState, pre_2_8_migration),
> > +        VMSTATE_UINT64_TEST(mig_io_win_size, sPAPRPHBState, pre_2_8_migration),
> >          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/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> > index b92c1b5..092294e 100644
> > --- a/include/hw/pci-host/spapr.h
> > +++ b/include/hw/pci-host/spapr.h
> > @@ -79,6 +79,12 @@ struct sPAPRPHBState {
> >      uint64_t dma64_win_addr;
> >  
> >      uint32_t numa_node;
> > +
> > +    /* Fields for migration compatibility hacks */
> > +    bool pre_2_8_migration;
> > +    uint32_t mig_liobn;
> > +    hwaddr mig_mem_win_addr, mig_mem_win_size;
> > +    hwaddr mig_io_win_addr, mig_io_win_size;
> >  };
> >  
> >  #define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL
> > 
> 
> 

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