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

[Alexey Kardashevskiy]

On 09/11/16 23:19, David Gibson wrote:
> 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 <address@hidden>
>>> ---
>>>  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.

afaict to fix matching properly,  TYPE_SYS_BUS_DEVICE needs get_dev_path()
hook, just like spapr_vio_get_dev_name; otherwise SPAPR PHB in migration
always named as "spapr_pci" but yes, this would be much outside of the
scope of this patch :-/


> 
>>
>> Nevertheless, this works,
>>
>> Reviewed-by: Alexey Kardashevskiy <address@hidden>
>>
>>
>>
>>
>>> -        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),
>>>
>>
>>
> 


-- 
Alexey

signature.asc
Description: OpenPGP digital signature