[PATCH for-7.1 0/4] use dc->vmsd with spapr devices vmstate

[PATCH for-7.1 0/4] use dc->vmsd with spapr devices vmstate

[Daniel Henrique Barboza]

Hi,

This short series converts some spapr devices to use the dc->vmsd
interface to register the vmstate. For most of them it was needed
to use qdev_set_legacy_instance_id() to keep compatibility with the
instance_id being used for awhile.

Although no functional changes were made the resulting code is a bit
shorter and maintainable. After these patches there are only 3 places
where vmstate_register() APIs are being used.

No behavior changes were detected when testing migration scenarios with
hotplug/unplug of devices.

Daniel Henrique Barboza (4):
  hw/ppc: use qdev to register logical DRC vmstates
  hw/ppc: use qdev to register physical DRC vmstates
  hw/ppc: use qdev to register spapr_iommu tcet vmstate
  hw/ppc: use qdev to register spapr_nvdimm vmsd

 hw/ppc/spapr_drc.c    | 13 ++++++-------
 hw/ppc/spapr_iommu.c  |  7 +++----
 hw/ppc/spapr_nvdimm.c | 11 ++---------
 3 files changed, 11 insertions(+), 20 deletions(-)

-- 
2.35.1

[PATCH for-7.1 1/4] hw/ppc: use qdev to register logical DRC vmstates

[Daniel Henrique Barboza]

Today we use vmstate_register() to register the vmstate of logical DRCs
during drc_realize(), following it up with a similar
vmstate_unregister() during drc_unrealize().

We can instead use qdev to init the vmstate of the device via the
dc->vmsd interface. This will spare us of both vmstate calls and make
the code a little more maintainable.

Since we're using spapr_drc_index() as instance_id in
vmstate_register(), we'll need to use qdev_set_legacy_instance_id() to set
the same instance_id the DRC vmstates are now used to have.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr_drc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 76bc5d42a0..a5ef64d2a2 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -536,8 +536,7 @@ static void drc_realize(DeviceState *d, Error **errp)
     trace_spapr_drc_realize_child(spapr_drc_index(drc), child_name);
     object_property_add_alias(root_container, link_name,
                               drc->owner, child_name);
-    vmstate_register(VMSTATE_IF(drc), spapr_drc_index(drc), &vmstate_spapr_drc,
-                     drc);
+    qdev_set_legacy_instance_id(d, spapr_drc_index(drc), 1);
     trace_spapr_drc_realize_complete(spapr_drc_index(drc));
 }
 
@@ -548,7 +547,6 @@ static void drc_unrealize(DeviceState *d)
     Object *root_container;
 
     trace_spapr_drc_unrealize(spapr_drc_index(drc));
-    vmstate_unregister(VMSTATE_IF(drc), &vmstate_spapr_drc, drc);
     root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
     object_property_del(root_container, name);
 }
@@ -673,8 +671,11 @@ static void spapr_drc_physical_class_init(ObjectClass *k, void *data)
 
 static void spapr_drc_logical_class_init(ObjectClass *k, void *data)
 {
+    DeviceClass *dk = DEVICE_CLASS(k);
     SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
 
+    dk->vmsd = &vmstate_spapr_drc;
+
     drck->dr_entity_sense = logical_entity_sense;
     drck->isolate = drc_isolate_logical;
     drck->unisolate = drc_unisolate_logical;
-- 
2.35.1

[PATCH for-7.1 2/4] hw/ppc: use qdev to register physical DRC vmstates

[Daniel Henrique Barboza]

Similar to logical DRCs, let's convert physical DRCs to register their
vmstates using dc->vmsd.

The same constraints with instance_id being set to spapr_drc_index()
also applies in this case. However, since realize_physical() calls
drc_realize(), qdev_set_legacy_instance_id() is already being set.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr_drc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index a5ef64d2a2..5a60885876 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -640,9 +640,6 @@ static void realize_physical(DeviceState *d, Error **errp)
         return;
     }
 
-    vmstate_register(VMSTATE_IF(drcp),
-                     spapr_drc_index(SPAPR_DR_CONNECTOR(drcp)),
-                     &vmstate_spapr_drc_physical, drcp);
     qemu_register_reset(drc_physical_reset, drcp);
 }
 
@@ -651,7 +648,6 @@ static void unrealize_physical(DeviceState *d)
     SpaprDrcPhysical *drcp = SPAPR_DRC_PHYSICAL(d);
 
     drc_unrealize(d);
-    vmstate_unregister(VMSTATE_IF(drcp), &vmstate_spapr_drc_physical, drcp);
     qemu_unregister_reset(drc_physical_reset, drcp);
 }
 
@@ -662,6 +658,8 @@ static void spapr_drc_physical_class_init(ObjectClass *k, void *data)
 
     dk->realize = realize_physical;
     dk->unrealize = unrealize_physical;
+    dk->vmsd = &vmstate_spapr_drc_physical;
+
     drck->dr_entity_sense = physical_entity_sense;
     drck->isolate = drc_isolate_physical;
     drck->unisolate = drc_unisolate_physical;
-- 
2.35.1

[PATCH for-7.1 3/4] hw/ppc: use qdev to register spapr_iommu tcet vmstate

[Daniel Henrique Barboza]

Use the dc->vmsd interface to register the vmstate of the tcet. The
instance_id is being set to tcet->liobn by calling
qdev_set_legacy_instance_id() during spapr_tce_table_realize().

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr_iommu.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
index 81e5a1aea3..ac3389f0b7 100644
--- a/hw/ppc/spapr_iommu.c
+++ b/hw/ppc/spapr_iommu.c
@@ -321,8 +321,8 @@ static void spapr_tce_table_realize(DeviceState *dev, Error **errp)
 
     QLIST_INSERT_HEAD(&spapr_tce_tables, tcet, list);
 
-    vmstate_register(VMSTATE_IF(tcet), tcet->liobn, &vmstate_spapr_tce_table,
-                     tcet);
+    qdev_set_legacy_instance_id(dev, tcet->liobn,
+                                vmstate_spapr_tce_table.minimum_version_id);
 }
 
 void spapr_tce_set_need_vfio(SpaprTceTable *tcet, bool need_vfio)
@@ -424,8 +424,6 @@ static void spapr_tce_table_unrealize(DeviceState *dev)
 {
     SpaprTceTable *tcet = SPAPR_TCE_TABLE(dev);
 
-    vmstate_unregister(VMSTATE_IF(tcet), &vmstate_spapr_tce_table, tcet);
-
     QLIST_REMOVE(tcet, list);
 
     spapr_tce_table_disable(tcet);
@@ -673,6 +671,7 @@ static void spapr_tce_table_class_init(ObjectClass *klass, void *data)
     dc->realize = spapr_tce_table_realize;
     dc->reset = spapr_tce_reset;
     dc->unrealize = spapr_tce_table_unrealize;
+    dc->vmsd = &vmstate_spapr_tce_table;
     /* Reason: This is just an internal device for handling the hypercalls */
     dc->user_creatable = false;
 
-- 
2.35.1

[PATCH for-7.1 4/4] hw/ppc: use qdev to register spapr_nvdimm vmsd

[Daniel Henrique Barboza]

Make the code a little more maintainable by using dc->vmsd to register
the vmstate instead of using vmstate_(un)register calls.

'instance_id' was being set to VMSTATE_INSTANCE_ID_ANY so there is no need
for qdev_set_legacy_instance_id() calls.

spapr_nvdimm_unrealize() was removed since it was only being used to
call vmstate_unregister().

Cc: Shivaprasad G Bhat <sbhat@linux.ibm.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr_nvdimm.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
index c4c97da5de..973e9d0fbe 100644
--- a/hw/ppc/spapr_nvdimm.c
+++ b/hw/ppc/spapr_nvdimm.c
@@ -866,14 +866,6 @@ static void spapr_nvdimm_realize(NVDIMMDevice *dimm, Error **errp)
     if (!is_pmem || pmem_override) {
         s_nvdimm->hcall_flush_required = true;
     }
-
-    vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY,
-                     &vmstate_spapr_nvdimm_states, dimm);
-}
-
-static void spapr_nvdimm_unrealize(NVDIMMDevice *dimm)
-{
-    vmstate_unregister(NULL, &vmstate_spapr_nvdimm_states, dimm);
 }
 
 static Property spapr_nvdimm_properties[] = {
@@ -888,8 +880,9 @@ static void spapr_nvdimm_class_init(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
     NVDIMMClass *nvc = NVDIMM_CLASS(oc);
 
+    dc->vmsd = &vmstate_spapr_nvdimm_states;
+
     nvc->realize = spapr_nvdimm_realize;
-    nvc->unrealize = spapr_nvdimm_unrealize;
 
     device_class_set_props(dc, spapr_nvdimm_properties);
 }
-- 
2.35.1

Re: [PATCH for-7.1 2/4] hw/ppc: use qdev to register physical DRC vmstates

[David Gibson]

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

On Tue, Mar 22, 2022 at 03:38:52PM -0300, Daniel Henrique Barboza wrote:
> Similar to logical DRCs, let's convert physical DRCs to register their
> vmstates using dc->vmsd.
> 
> The same constraints with instance_id being set to spapr_drc_index()
> also applies in this case. However, since realize_physical() calls
> drc_realize(), qdev_set_legacy_instance_id() is already being set.

Ok, and you've verified that you don't need to set the legacy ID on
both "layers"?  That is, have you tested that you can migrate from
before this change to after?

> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  hw/ppc/spapr_drc.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index a5ef64d2a2..5a60885876 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -640,9 +640,6 @@ static void realize_physical(DeviceState *d, Error **errp)
>          return;
>      }
>  
> -    vmstate_register(VMSTATE_IF(drcp),
> -                     spapr_drc_index(SPAPR_DR_CONNECTOR(drcp)),
> -                     &vmstate_spapr_drc_physical, drcp);
>      qemu_register_reset(drc_physical_reset, drcp);
>  }
>  
> @@ -651,7 +648,6 @@ static void unrealize_physical(DeviceState *d)
>      SpaprDrcPhysical *drcp = SPAPR_DRC_PHYSICAL(d);
>  
>      drc_unrealize(d);
> -    vmstate_unregister(VMSTATE_IF(drcp), &vmstate_spapr_drc_physical, drcp);
>      qemu_unregister_reset(drc_physical_reset, drcp);
>  }
>  
> @@ -662,6 +658,8 @@ static void spapr_drc_physical_class_init(ObjectClass *k, void *data)
>  
>      dk->realize = realize_physical;
>      dk->unrealize = unrealize_physical;
> +    dk->vmsd = &vmstate_spapr_drc_physical;
> +
>      drck->dr_entity_sense = physical_entity_sense;
>      drck->isolate = drc_isolate_physical;
>      drck->unisolate = drc_unisolate_physical;

-- 
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: 833 bytes --]

Re: [PATCH for-7.1 0/4] use dc->vmsd with spapr devices vmstate

[David Gibson]

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

On Tue, Mar 22, 2022 at 03:38:50PM -0300, Daniel Henrique Barboza wrote:
> Hi,
> 
> This short series converts some spapr devices to use the dc->vmsd
> interface to register the vmstate. For most of them it was needed
> to use qdev_set_legacy_instance_id() to keep compatibility with the
> instance_id being used for awhile.
> 
> Although no functional changes were made the resulting code is a bit
> shorter and maintainable. After these patches there are only 3 places
> where vmstate_register() APIs are being used.
> 
> No behavior changes were detected when testing migration scenarios with
> hotplug/unplug of devices.

Looks good tome.

-- 
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: 833 bytes --]

Re: [PATCH for-7.1 2/4] hw/ppc: use qdev to register physical DRC vmstates

[Daniel Henrique Barboza]

On 3/22/22 22:48, David Gibson wrote:
> On Tue, Mar 22, 2022 at 03:38:52PM -0300, Daniel Henrique Barboza wrote:
>> Similar to logical DRCs, let's convert physical DRCs to register their
>> vmstates using dc->vmsd.
>>
>> The same constraints with instance_id being set to spapr_drc_index()
>> also applies in this case. However, since realize_physical() calls
>> drc_realize(), qdev_set_legacy_instance_id() is already being set.
> 
> Ok, and you've verified that you don't need to set the legacy ID on
> both "layers"?  That is, have you tested that you can migrate from
> before this change to after?

It works for the wrong reasons. The way I tested this patch wasn't triggering
the migration of the DRCs.

Doing a hotplug and migrating afterwards was enough to migrate the DRC back in 2017,
but after all the work we did in DRC code over these years what happens now is that,
during machine reset, the drc is set to ready_state:

     if (drc->dev) {
         /* A device present at reset is ready to go, same as coldplugged */
         drc->state = drck->ready_state;

And spapr_drc_needed() is returning false:

     /*
      * We need to reset the DRC at CAS or to migrate the DRC state if it's
      * not equal to the expected long-term state, which is the same as the
      * coldplugged initial state, or if an unplug request is pending.
      */
     return drc->state != drck->ready_state || spapr_drc_unplug_requested(drc);


So the CPU DRCs aren't being migrated. I noticed that when turning on traces
of the migration code to debug a problem with patch 03.



Daniel

> 
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   hw/ppc/spapr_drc.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
>> index a5ef64d2a2..5a60885876 100644
>> --- a/hw/ppc/spapr_drc.c
>> +++ b/hw/ppc/spapr_drc.c
>> @@ -640,9 +640,6 @@ static void realize_physical(DeviceState *d, Error **errp)
>>           return;
>>       }
>>   
>> -    vmstate_register(VMSTATE_IF(drcp),
>> -                     spapr_drc_index(SPAPR_DR_CONNECTOR(drcp)),
>> -                     &vmstate_spapr_drc_physical, drcp);
>>       qemu_register_reset(drc_physical_reset, drcp);
>>   }
>>   
>> @@ -651,7 +648,6 @@ static void unrealize_physical(DeviceState *d)
>>       SpaprDrcPhysical *drcp = SPAPR_DRC_PHYSICAL(d);
>>   
>>       drc_unrealize(d);
>> -    vmstate_unregister(VMSTATE_IF(drcp), &vmstate_spapr_drc_physical, drcp);
>>       qemu_unregister_reset(drc_physical_reset, drcp);
>>   }
>>   
>> @@ -662,6 +658,8 @@ static void spapr_drc_physical_class_init(ObjectClass *k, void *data)
>>   
>>       dk->realize = realize_physical;
>>       dk->unrealize = unrealize_physical;
>> +    dk->vmsd = &vmstate_spapr_drc_physical;
>> +
>>       drck->dr_entity_sense = physical_entity_sense;
>>       drck->isolate = drc_isolate_physical;
>>       drck->unisolate = drc_unisolate_physical;
> 

Re: [PATCH for-7.1 0/4] use dc->vmsd with spapr devices vmstate

[Daniel Henrique Barboza]

On 3/22/22 22:49, David Gibson wrote:
> On Tue, Mar 22, 2022 at 03:38:50PM -0300, Daniel Henrique Barboza wrote:
>> Hi,
>>
>> This short series converts some spapr devices to use the dc->vmsd
>> interface to register the vmstate. For most of them it was needed
>> to use qdev_set_legacy_instance_id() to keep compatibility with the
>> instance_id being used for awhile.
>>
>> Although no functional changes were made the resulting code is a bit
>> shorter and maintainable. After these patches there are only 3 places
>> where vmstate_register() APIs are being used.
>>
>> No behavior changes were detected when testing migration scenarios with
>> hotplug/unplug of devices.
> 
> Looks good tome.

It looked good to me until, after further testing, I noticed that patch 03
breaks backward migration:

qemu_loadvm_state_section_startfull 560 (spapr_iommu) 0 2
qemu-system-ppc64: Unknown savevm section or instance 'spapr_iommu' 0. Make sure
that your current VM setup matches your saved VM setup, including any hotplugged devices

qemu-system-ppc64: load of migration failed: Invalid argument

I made a follow-up in the instance_id discussion [1] about it. For now patches 1-3 are
compromised. Only patch 04 is worth considering because the spapr_nvdimm device isn't
setting a custom instance_id.


[1] https://lists.gnu.org/archive/html/qemu-devel/2022-03/msg05942.html



Thanks,


Daniel

>