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 <da...@gibson.dropbear.id.au>
>>> ---
>>> 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 <a...@ozlabs.ru> >> >> >> >> >>> - 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
