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

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

[David Gibson]

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 <david@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),
-        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),
-- 
2.7.4

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

[Alexey Kardashevskiy]

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 <david@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.

Nevertheless, this works,

Reviewed-by: Alexey Kardashevskiy <aik@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

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3593 bytes --]

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 <david@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.

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

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Alexey Kardashevskiy]

[-- Attachment #1: Type: text/plain, Size: 3726 bytes --]

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 <david@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 <aik@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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 839 bytes --]