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

[Alexey Kardashevskiy]

On 23/11/16 11:17, David Gibson wrote:
> 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 <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) {
>>
>>
>> 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?


Never mind, I did not sleep well and while switching between patches, I
confused 1/5 (which does not have a check) with 3/5 (which does). My bad,
sorry for the noise. I run some more tests today, just to make sure I slept
well, all good :)



-- 
Alexey

signature.asc
Description: OpenPGP digital signature