From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33457) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c8r3D-0000me-0y for qemu-devel@nongnu.org; Mon, 21 Nov 2016 11:02:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c8r38-000449-2c for qemu-devel@nongnu.org; Mon, 21 Nov 2016 11:02:35 -0500 Received: from 8.mo53.mail-out.ovh.net ([87.98.185.57]:44199) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c8r37-00043q-T0 for qemu-devel@nongnu.org; Mon, 21 Nov 2016 11:02:30 -0500 Received: from player158.ha.ovh.net (b7.ovh.net [213.186.33.57]) by mo53.mail-out.ovh.net (Postfix) with ESMTP id 7496E434C5 for ; Mon, 21 Nov 2016 17:02:28 +0100 (CET) Date: Mon, 21 Nov 2016 17:02:20 +0100 From: Greg Kurz Message-ID: <20161121170220.44a764ce@bahia> In-Reply-To: <5f3ab979-741a-28a1-4feb-4b8696fe598e@redhat.com> References: <1479706302-2251-1-git-send-email-david@gibson.dropbear.id.au> <1479706302-2251-6-git-send-email-david@gibson.dropbear.id.au> <5f3ab979-741a-28a1-4feb-4b8696fe598e@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCHv2 5/5] spapr: Fix 2.7<->2.8 migration of PCI host bridge List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Thomas Huth Cc: David Gibson , mdroth@linux.vnet.ibm.com, dgilbert@redhat.com, lvivier@redhat.com, qemu-ppc@nongnu.org, qemu-devel@nongnu.org On Mon, 21 Nov 2016 13:02:53 +0100 Thomas Huth wrote: > 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 > > --- > > 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? > Then shouldn't we forbid pre_2_8_migration to be set when we have a non-contiguous window ? > > + } > > } > > With or without warning message, patch looks fine to me, so: > > Reviewed-by: Thomas Huth > >