[PATCH 1/2] vfio: Don't be a iterable and legacy device at the same time

[PATCH 1/2] vfio: Don't be a iterable and legacy device at the same time

[Lukas Straub]

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

Legacy savevm devices only implement save_state() and load_state().
Iterable devices shouldn't implement save_state() or else they are
handled both as an iterable and legacy device in the savevm code.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---

Note: this patch is completely untested.

 hw/vfio/migration.c | 38 +++++++++++---------------------------
 1 file changed, 11 insertions(+), 27 deletions(-)

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 6b58dddb88..8d7f22dbd3 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -166,23 +166,6 @@ static int vfio_load_buffer(QEMUFile *f, VFIODevice *vbasedev,
     return ret;
 }
 
-static int vfio_save_device_config_state(QEMUFile *f, void *opaque)
-{
-    VFIODevice *vbasedev = opaque;
-
-    qemu_put_be64(f, VFIO_MIG_FLAG_DEV_CONFIG_STATE);
-
-    if (vbasedev->ops && vbasedev->ops->vfio_save_config) {
-        vbasedev->ops->vfio_save_config(vbasedev, f);
-    }
-
-    qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
-
-    trace_vfio_save_device_config_state(vbasedev->name);
-
-    return qemu_file_get_error(f);
-}
-
 static int vfio_load_device_config_state(QEMUFile *f, void *opaque)
 {
     VFIODevice *vbasedev = opaque;
@@ -351,7 +334,6 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
         }
     } while (!ret);
 
-    qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
     ret = qemu_file_get_error(f);
     if (ret) {
         return ret;
@@ -365,20 +347,23 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
                                    VFIO_DEVICE_STATE_ERROR);
     trace_vfio_save_complete_precopy(vbasedev->name, ret);
 
-    return ret;
-}
+    qemu_put_be64(f, VFIO_MIG_FLAG_DEV_CONFIG_STATE);
 
-static void vfio_save_state(QEMUFile *f, void *opaque)
-{
-    VFIODevice *vbasedev = opaque;
-    int ret;
+    if (vbasedev->ops && vbasedev->ops->vfio_save_config) {
+        vbasedev->ops->vfio_save_config(vbasedev, f);
+    }
+
+    qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
+
+    trace_vfio_save_device_config_state(vbasedev->name);
 
-    ret = vfio_save_device_config_state(f, opaque);
+    ret = qemu_file_get_error(f);
     if (ret) {
         error_report("%s: Failed to save device config space",
                      vbasedev->name);
-        qemu_file_set_error(f, ret);
     }
+
+    return ret;
 }
 
 static int vfio_load_setup(QEMUFile *f, void *opaque)
@@ -458,7 +443,6 @@ static const SaveVMHandlers savevm_vfio_handlers = {
     .save_cleanup = vfio_save_cleanup,
     .state_pending_exact = vfio_state_pending_exact,
     .save_live_complete_precopy = vfio_save_complete_precopy,
-    .save_state = vfio_save_state,
     .load_setup = vfio_load_setup,
     .load_cleanup = vfio_load_cleanup,
     .load_state = vfio_load_state,
-- 
2.39.2


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

[PATCH 2/2] savevm: Disallow devices being legacy and iterable at the same time

[Lukas Straub]

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

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 migration/savevm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/migration/savevm.c b/migration/savevm.c
index 5986f852b2..8d3cf7d675 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -774,6 +774,8 @@ int register_savevm_live(const char *idstr,
         se->is_ram = 1;
     }
 
+    assert(!(se->ops->save_setup && se->ops->save_state));
+
     pstrcat(se->idstr, sizeof(se->idstr), idstr);
 
     if (instance_id == VMSTATE_INSTANCE_ID_ANY) {
-- 
2.39.2

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

Re: [PATCH 1/2] vfio: Don't be a iterable and legacy device at the same time

[Peter Xu]

On Thu, Jun 22, 2023 at 01:22:26PM +0200, Lukas Straub wrote:
> Legacy savevm devices only implement save_state() and load_state().
> Iterable devices shouldn't implement save_state() or else they are
> handled both as an iterable and legacy device in the savevm code.
> 
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> ---
> 
> Note: this patch is completely untested.

PS: if you're not confident on the change will always work, better mark as
rfc to show a proposal of such change.

Comparing to the "legacy" vs "modern" migration, IIUC it was about whether
to use vmsd, so it's "save_state()" vs "vmsd" in that regard.

Personally, I don't immediately see a direct conflict / issue with device
providing both save_state() and save_setup().  It means the device declares
both (1) iterable data, and (2) non-iterable data (which can be either vmsd
or save_state()).

I do think vmsd is still preferred here for (2), e.g., I quickly looked at
vmstate_vfio_pci_config which seems fine to be implemented as a vmsd, with
a post_load() perhaps.  But that's another story.  It just all looks still
fine.

Do we get any benefit from having that restriction?

-- 
Peter Xu