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

[Thomas Huth]

On 21.11.2016 06: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 <address@hidden>
> ---
>  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) {
> +        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;
> +        }

Should we maybe print a warning/error message in case

 sphb->mem64_win_size != 0 &&
 sphb->mem64_win_addr != sphb->mem_win_addr + sphb->mem_win_size

... assuming that this means a configuration which can not be migrated
backwards?

> +    }
>  }

With or without warning message, patch looks fine to me, so:

Reviewed-by: Thomas Huth <address@hidden>