[PATCH v4 0/3] WIP: ramfb: migration support

[PATCH v4 0/3] WIP: ramfb: migration support

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Hi,

Implement RAMFB migration, and add properties to enable it only on >= 8.2
machines, + a few related cleanups.

Cedric, did you get the chance to test the VFIO display/ramfb code?

thanks

v4: (Laszlo review and suggestions)
- change migrate_needed() to assert(ramfb_exists)
- rename vfio_display_needed() to vfio_display_migration_needed(),
  update the condition and associated comment
- move the ramfb-migrate option check and add a check for ramfb=on
- add a stub to fix compilation on some architectures

v3:
- add a "x-" prefix to properties, as they are not meant for users.
- RAMFB now exports a ramfb_vmstate for actual devices to include
- VFIOPCIDevice now has a VFIODisplay optional subsection whenever ramfb
  migration is required (untested)

Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1859424

Marc-André Lureau (3):
  ramfb: add migration support
  ramfb-standalone: add migration support
  hw/vfio: add ramfb migration support

 hw/vfio/pci.h                 |  3 +++
 include/hw/display/ramfb.h    |  4 ++++
 hw/core/machine.c             |  5 +++-
 hw/display/ramfb-standalone.c | 27 +++++++++++++++++++++
 hw/display/ramfb.c            | 19 +++++++++++++++
 hw/vfio/display.c             | 20 ++++++++++++++++
 hw/vfio/pci.c                 | 44 +++++++++++++++++++++++++++++++++++
 stubs/ramfb.c                 |  2 ++
 8 files changed, 123 insertions(+), 1 deletion(-)

-- 
2.41.0

[PATCH v4 1/3] ramfb: add migration support

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Implementing RAMFB migration is quite straightforward. One caveat is to
treat the whole RAMFBCfg as a blob, since that's what is exposed to the
guest directly. This avoid having to fiddle with endianness issues if we
were to migrate fields individually as integers.

The devices using RAMFB will have to include ramfb_vmstate in their
migration description.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/hw/display/ramfb.h |  4 ++++
 hw/display/ramfb.c         | 19 +++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/include/hw/display/ramfb.h b/include/hw/display/ramfb.h
index b33a2c467b..a7e0019144 100644
--- a/include/hw/display/ramfb.h
+++ b/include/hw/display/ramfb.h
@@ -1,11 +1,15 @@
 #ifndef RAMFB_H
 #define RAMFB_H
 
+#include "migration/vmstate.h"
+
 /* ramfb.c */
 typedef struct RAMFBState RAMFBState;
 void ramfb_display_update(QemuConsole *con, RAMFBState *s);
 RAMFBState *ramfb_setup(Error **errp);
 
+extern const VMStateDescription ramfb_vmstate;
+
 /* ramfb-standalone.c */
 #define TYPE_RAMFB_DEVICE "ramfb"
 
diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
index c2b002d534..477ef7272a 100644
--- a/hw/display/ramfb.c
+++ b/hw/display/ramfb.c
@@ -28,6 +28,8 @@ struct QEMU_PACKED RAMFBCfg {
     uint32_t stride;
 };
 
+typedef struct RAMFBCfg RAMFBCfg;
+
 struct RAMFBState {
     DisplaySurface *ds;
     uint32_t width, height;
@@ -116,6 +118,23 @@ void ramfb_display_update(QemuConsole *con, RAMFBState *s)
     dpy_gfx_update_full(con);
 }
 
+static int ramfb_post_load(void *opaque, int version_id)
+{
+    ramfb_fw_cfg_write(opaque, 0, 0);
+    return 0;
+}
+
+const VMStateDescription ramfb_vmstate = {
+    .name = "ramfb",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .post_load = ramfb_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_BUFFER_UNSAFE(cfg, RAMFBState, 0, sizeof(RAMFBCfg)),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 RAMFBState *ramfb_setup(Error **errp)
 {
     FWCfgState *fw_cfg = fw_cfg_find();
-- 
2.41.0

[PATCH v4 2/3] ramfb-standalone: add migration support

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Add a "ramfb-dev" section whenever "x-migrate" is turned on. Turn it off
by default on machines <= 8.1 for compatibility reasons.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/core/machine.c             |  4 +++-
 hw/display/ramfb-standalone.c | 27 +++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 281ef0dccd..e4361e3d48 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -31,7 +31,9 @@
 #include "hw/virtio/virtio-net.h"
 #include "audio/audio.h"
 
-GlobalProperty hw_compat_8_1[] = {};
+GlobalProperty hw_compat_8_1[] = {
+    { "ramfb", "x-migrate", "off" },
+};
 const size_t hw_compat_8_1_len = G_N_ELEMENTS(hw_compat_8_1);
 
 GlobalProperty hw_compat_8_0[] = {
diff --git a/hw/display/ramfb-standalone.c b/hw/display/ramfb-standalone.c
index 8c0094397f..a96e7ebcd9 100644
--- a/hw/display/ramfb-standalone.c
+++ b/hw/display/ramfb-standalone.c
@@ -1,4 +1,5 @@
 #include "qemu/osdep.h"
+#include "migration/vmstate.h"
 #include "qapi/error.h"
 #include "qemu/module.h"
 #include "hw/loader.h"
@@ -15,6 +16,7 @@ struct RAMFBStandaloneState {
     SysBusDevice parent_obj;
     QemuConsole *con;
     RAMFBState *state;
+    bool migrate;
 };
 
 static void display_update_wrapper(void *dev)
@@ -40,14 +42,39 @@ static void ramfb_realizefn(DeviceState *dev, Error **errp)
     ramfb->state = ramfb_setup(errp);
 }
 
+static bool migrate_needed(void *opaque)
+{
+    RAMFBStandaloneState *ramfb = RAMFB(opaque);
+
+    return ramfb->migrate;
+}
+
+static const VMStateDescription ramfb_dev_vmstate = {
+    .name = "ramfb-dev",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = migrate_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_STRUCT_POINTER(state, RAMFBStandaloneState, ramfb_vmstate, RAMFBState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static Property ramfb_properties[] = {
+    DEFINE_PROP_BOOL("x-migrate", RAMFBStandaloneState, migrate,  true),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void ramfb_class_initfn(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
+    dc->vmsd = &ramfb_dev_vmstate;
     dc->realize = ramfb_realizefn;
     dc->desc = "ram framebuffer standalone device";
     dc->user_creatable = true;
+    device_class_set_props(dc, ramfb_properties);
 }
 
 static const TypeInfo ramfb_info = {
-- 
2.41.0

[PATCH v4 3/3] hw/vfio: add ramfb migration support

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Add a "VFIODisplay" subsection whenever "x-ramfb-migrate" is turned on.

Turn it off by default on machines <= 8.1 for compatibility reasons.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/vfio/pci.h     |  3 +++
 hw/core/machine.c |  1 +
 hw/vfio/display.c | 20 ++++++++++++++++++++
 hw/vfio/pci.c     | 44 ++++++++++++++++++++++++++++++++++++++++++++
 stubs/ramfb.c     |  2 ++
 5 files changed, 70 insertions(+)

diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index 2d836093a8..fd06695542 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -173,6 +173,7 @@ struct VFIOPCIDevice {
     bool no_kvm_ioeventfd;
     bool no_vfio_ioeventfd;
     bool enable_ramfb;
+    OnOffAuto ramfb_migrate;
     bool defer_kvm_irq_routing;
     bool clear_parent_atomics_on_exit;
     VFIODisplay *dpy;
@@ -226,4 +227,6 @@ void vfio_display_reset(VFIOPCIDevice *vdev);
 int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp);
 void vfio_display_finalize(VFIOPCIDevice *vdev);
 
+extern const VMStateDescription vfio_display_vmstate;
+
 #endif /* HW_VFIO_VFIO_PCI_H */
diff --git a/hw/core/machine.c b/hw/core/machine.c
index e4361e3d48..f2c59a293c 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -33,6 +33,7 @@
 
 GlobalProperty hw_compat_8_1[] = {
     { "ramfb", "x-migrate", "off" },
+    { "vfio-pci-nohotplug", "x-ramfb-migrate", "off" }
 };
 const size_t hw_compat_8_1_len = G_N_ELEMENTS(hw_compat_8_1);
 
diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index bec864f482..0bdb807642 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -542,3 +542,23 @@ void vfio_display_finalize(VFIOPCIDevice *vdev)
     vfio_display_edid_exit(vdev->dpy);
     g_free(vdev->dpy);
 }
+
+static bool migrate_needed(void *opaque)
+{
+    VFIODisplay *dpy = opaque;
+    bool ramfb_exists = dpy->ramfb != NULL;
+
+    /* see vfio_display_migration_needed() */
+    assert(ramfb_exists);
+    return ramfb_exists;
+}
+
+const VMStateDescription vfio_display_vmstate = {
+    .name = "VFIODisplay",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = migrate_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_STRUCT_POINTER(ramfb, VFIODisplay, ramfb_vmstate, RAMFBState),
+    }
+};
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 3b2ca3c24c..d2ede2f1a2 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2608,6 +2608,32 @@ static bool vfio_msix_present(void *opaque, int version_id)
     return msix_present(pdev);
 }
 
+static bool vfio_display_migration_needed(void *opaque)
+{
+    VFIOPCIDevice *vdev = opaque;
+
+    /*
+     * We need to migrate the VFIODisplay object if ramfb *migration* was
+     * explicitly requested (in which case we enforced both ramfb=on and
+     * display=on), or ramfb migration was left at the default "auto"
+     * setting, and *ramfb* was explicitly requested (in which case we
+     * enforced display=on).
+     */
+    return vdev->ramfb_migrate == ON_OFF_AUTO_ON ||
+        (vdev->ramfb_migrate == ON_OFF_AUTO_AUTO && vdev->enable_ramfb);
+}
+
+const VMStateDescription vmstate_vfio_display = {
+    .name = "VFIOPCIDevice/VFIODisplay",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = vfio_display_migration_needed,
+    .fields = (VMStateField[]){
+        VMSTATE_STRUCT_POINTER(dpy, VFIOPCIDevice, vfio_display_vmstate, VFIODisplay),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 const VMStateDescription vmstate_vfio_pci_config = {
     .name = "VFIOPCIDevice",
     .version_id = 1,
@@ -2616,6 +2642,10 @@ const VMStateDescription vmstate_vfio_pci_config = {
         VMSTATE_PCI_DEVICE(pdev, VFIOPCIDevice),
         VMSTATE_MSIX_TEST(pdev, VFIOPCIDevice, vfio_msix_present),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (const VMStateDescription*[]) {
+        &vmstate_vfio_display,
+        NULL
     }
 };
 
@@ -3271,6 +3301,19 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
         }
     }
 
+    if (vdev->ramfb_migrate == ON_OFF_AUTO_ON && !vdev->enable_ramfb) {
+        error_setg(errp, "x-ramfb-migrate requires ramfb=on");
+        goto out_deregister;
+    }
+    if (vbasedev->enable_migration == ON_OFF_AUTO_OFF) {
+        if (vdev->ramfb_migrate == ON_OFF_AUTO_AUTO) {
+            vdev->ramfb_migrate = ON_OFF_AUTO_OFF;
+        } else if (vdev->ramfb_migrate == ON_OFF_AUTO_ON) {
+            error_setg(errp, "x-ramfb-migrate requires enable-migration");
+            goto out_deregister;
+        }
+    }
+
     if (!pdev->failover_pair_id) {
         if (!vfio_migration_realize(vbasedev, errp)) {
             goto out_deregister;
@@ -3484,6 +3527,7 @@ static const TypeInfo vfio_pci_dev_info = {
 
 static Property vfio_pci_dev_nohotplug_properties[] = {
     DEFINE_PROP_BOOL("ramfb", VFIOPCIDevice, enable_ramfb, false),
+    DEFINE_PROP_ON_OFF_AUTO("x-ramfb-migrate", VFIOPCIDevice, ramfb_migrate, ON_OFF_AUTO_AUTO),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/stubs/ramfb.c b/stubs/ramfb.c
index 48143f3354..cf64733b10 100644
--- a/stubs/ramfb.c
+++ b/stubs/ramfb.c
@@ -2,6 +2,8 @@
 #include "qapi/error.h"
 #include "hw/display/ramfb.h"
 
+const VMStateDescription ramfb_vmstate = {};
+
 void ramfb_display_update(QemuConsole *con, RAMFBState *s)
 {
 }
-- 
2.41.0

Re: [PATCH v4 0/3] WIP: ramfb: migration support

[Cédric Le Goater]

On 10/5/23 13:30, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Hi,
> 
> Implement RAMFB migration, and add properties to enable it only on >= 8.2
> machines, + a few related cleanups.
> 
> Cedric, did you get the chance to test the VFIO display/ramfb code?

Nope. I was busy with VFIO stuff. I haven't even read Laszlo's
email yet. I will try this or next week.

That said, could we avoid adding another migration property in
VFIOPCIDevice and use the available "enable-migration" ?

C.


> thanks
> 
> v4: (Laszlo review and suggestions)
> - change migrate_needed() to assert(ramfb_exists)
> - rename vfio_display_needed() to vfio_display_migration_needed(),
>    update the condition and associated comment
> - move the ramfb-migrate option check and add a check for ramfb=on
> - add a stub to fix compilation on some architectures
> 
> v3:
> - add a "x-" prefix to properties, as they are not meant for users.
> - RAMFB now exports a ramfb_vmstate for actual devices to include
> - VFIOPCIDevice now has a VFIODisplay optional subsection whenever ramfb
>    migration is required (untested)
> 
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1859424
> 
> Marc-André Lureau (3):
>    ramfb: add migration support
>    ramfb-standalone: add migration support
>    hw/vfio: add ramfb migration support
> 
>   hw/vfio/pci.h                 |  3 +++
>   include/hw/display/ramfb.h    |  4 ++++
>   hw/core/machine.c             |  5 +++-
>   hw/display/ramfb-standalone.c | 27 +++++++++++++++++++++
>   hw/display/ramfb.c            | 19 +++++++++++++++
>   hw/vfio/display.c             | 20 ++++++++++++++++
>   hw/vfio/pci.c                 | 44 +++++++++++++++++++++++++++++++++++
>   stubs/ramfb.c                 |  2 ++
>   8 files changed, 123 insertions(+), 1 deletion(-)
> 

Re: [PATCH v4 0/3] WIP: ramfb: migration support

[Laszlo Ersek]

On 10/5/23 14:01, Cédric Le Goater wrote:
> On 10/5/23 13:30, marcandre.lureau@redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> Hi,
>>
>> Implement RAMFB migration, and add properties to enable it only on >= 8.2
>> machines, + a few related cleanups.
>>
>> Cedric, did you get the chance to test the VFIO display/ramfb code?
> 
> Nope. I was busy with VFIO stuff. I haven't even read Laszlo's
> email yet. I will try this or next week.
> 
> That said, could we avoid adding another migration property in
> VFIOPCIDevice and use the available "enable-migration" ?

I'm not entirely sure, but I suspect we can't / shouldn't do that.
"x-ramfb-migrate" is effectively a machine type compat prop, so if it
doesn't *precisely* line up with enable-migration (i.e., if they aren't
equivalent), then we shouldn't merge them. AFAICT, a 8.1 machine type
may have "enable-migration" set, but it should still have
"x-ramfb-migrate" clear.

Laszlo

Re: [PATCH v4 0/3] WIP: ramfb: migration support

[Laszlo Ersek]

On 10/5/23 16:16, Laszlo Ersek wrote:
> On 10/5/23 14:01, Cédric Le Goater wrote:
>> On 10/5/23 13:30, marcandre.lureau@redhat.com wrote:
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> Hi,
>>>
>>> Implement RAMFB migration, and add properties to enable it only on >= 8.2
>>> machines, + a few related cleanups.
>>>
>>> Cedric, did you get the chance to test the VFIO display/ramfb code?
>>
>> Nope. I was busy with VFIO stuff. I haven't even read Laszlo's
>> email yet. I will try this or next week.
>>
>> That said, could we avoid adding another migration property in
>> VFIOPCIDevice and use the available "enable-migration" ?
> 
> I'm not entirely sure, but I suspect we can't / shouldn't do that.
> "x-ramfb-migrate" is effectively a machine type compat prop, so if it
> doesn't *precisely* line up with enable-migration (i.e., if they aren't
> equivalent), then we shouldn't merge them. AFAICT, a 8.1 machine type
> may have "enable-migration" set, but it should still have
> "x-ramfb-migrate" clear.

or more precisely, not clear, but "auto".

Laszlo

Re: [PATCH v4 1/3] ramfb: add migration support

[Laszlo Ersek]

On 10/5/23 13:30, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Implementing RAMFB migration is quite straightforward. One caveat is to
> treat the whole RAMFBCfg as a blob, since that's what is exposed to the
> guest directly. This avoid having to fiddle with endianness issues if we
> were to migrate fields individually as integers.
> 
> The devices using RAMFB will have to include ramfb_vmstate in their
> migration description.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/hw/display/ramfb.h |  4 ++++
>  hw/display/ramfb.c         | 19 +++++++++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/include/hw/display/ramfb.h b/include/hw/display/ramfb.h
> index b33a2c467b..a7e0019144 100644
> --- a/include/hw/display/ramfb.h
> +++ b/include/hw/display/ramfb.h
> @@ -1,11 +1,15 @@
>  #ifndef RAMFB_H
>  #define RAMFB_H
>  
> +#include "migration/vmstate.h"
> +
>  /* ramfb.c */
>  typedef struct RAMFBState RAMFBState;
>  void ramfb_display_update(QemuConsole *con, RAMFBState *s);
>  RAMFBState *ramfb_setup(Error **errp);
>  
> +extern const VMStateDescription ramfb_vmstate;
> +
>  /* ramfb-standalone.c */
>  #define TYPE_RAMFB_DEVICE "ramfb"
>  
> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
> index c2b002d534..477ef7272a 100644
> --- a/hw/display/ramfb.c
> +++ b/hw/display/ramfb.c
> @@ -28,6 +28,8 @@ struct QEMU_PACKED RAMFBCfg {
>      uint32_t stride;
>  };
>  
> +typedef struct RAMFBCfg RAMFBCfg;
> +
>  struct RAMFBState {
>      DisplaySurface *ds;
>      uint32_t width, height;
> @@ -116,6 +118,23 @@ void ramfb_display_update(QemuConsole *con, RAMFBState *s)
>      dpy_gfx_update_full(con);
>  }
>  
> +static int ramfb_post_load(void *opaque, int version_id)
> +{
> +    ramfb_fw_cfg_write(opaque, 0, 0);
> +    return 0;
> +}
> +
> +const VMStateDescription ramfb_vmstate = {
> +    .name = "ramfb",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .post_load = ramfb_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_BUFFER_UNSAFE(cfg, RAMFBState, 0, sizeof(RAMFBCfg)),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  RAMFBState *ramfb_setup(Error **errp)
>  {
>      FWCfgState *fw_cfg = fw_cfg_find();

Identical to v3, which I reviewed, so:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [PATCH v4 2/3] ramfb-standalone: add migration support

[Laszlo Ersek]

On 10/5/23 13:30, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Add a "ramfb-dev" section whenever "x-migrate" is turned on. Turn it off
> by default on machines <= 8.1 for compatibility reasons.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/core/machine.c             |  4 +++-
>  hw/display/ramfb-standalone.c | 27 +++++++++++++++++++++++++++
>  2 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 281ef0dccd..e4361e3d48 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -31,7 +31,9 @@
>  #include "hw/virtio/virtio-net.h"
>  #include "audio/audio.h"
>  
> -GlobalProperty hw_compat_8_1[] = {};
> +GlobalProperty hw_compat_8_1[] = {
> +    { "ramfb", "x-migrate", "off" },
> +};
>  const size_t hw_compat_8_1_len = G_N_ELEMENTS(hw_compat_8_1);
>  
>  GlobalProperty hw_compat_8_0[] = {
> diff --git a/hw/display/ramfb-standalone.c b/hw/display/ramfb-standalone.c
> index 8c0094397f..a96e7ebcd9 100644
> --- a/hw/display/ramfb-standalone.c
> +++ b/hw/display/ramfb-standalone.c
> @@ -1,4 +1,5 @@
>  #include "qemu/osdep.h"
> +#include "migration/vmstate.h"
>  #include "qapi/error.h"
>  #include "qemu/module.h"
>  #include "hw/loader.h"
> @@ -15,6 +16,7 @@ struct RAMFBStandaloneState {
>      SysBusDevice parent_obj;
>      QemuConsole *con;
>      RAMFBState *state;
> +    bool migrate;
>  };
>  
>  static void display_update_wrapper(void *dev)
> @@ -40,14 +42,39 @@ static void ramfb_realizefn(DeviceState *dev, Error **errp)
>      ramfb->state = ramfb_setup(errp);
>  }
>  
> +static bool migrate_needed(void *opaque)
> +{
> +    RAMFBStandaloneState *ramfb = RAMFB(opaque);
> +
> +    return ramfb->migrate;
> +}
> +
> +static const VMStateDescription ramfb_dev_vmstate = {
> +    .name = "ramfb-dev",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = migrate_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_STRUCT_POINTER(state, RAMFBStandaloneState, ramfb_vmstate, RAMFBState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static Property ramfb_properties[] = {
> +    DEFINE_PROP_BOOL("x-migrate", RAMFBStandaloneState, migrate,  true),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void ramfb_class_initfn(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
> +    dc->vmsd = &ramfb_dev_vmstate;
>      dc->realize = ramfb_realizefn;
>      dc->desc = "ram framebuffer standalone device";
>      dc->user_creatable = true;
> +    device_class_set_props(dc, ramfb_properties);
>  }
>  
>  static const TypeInfo ramfb_info = {

Identical to v3, which I reviewed, so:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [PATCH v4 3/3] hw/vfio: add ramfb migration support

[Cédric Le Goater]

On 10/5/23 13:30, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Add a "VFIODisplay" subsection whenever "x-ramfb-migrate" is turned on.
> 
> Turn it off by default on machines <= 8.1 for compatibility reasons.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   hw/vfio/pci.h     |  3 +++
>   hw/core/machine.c |  1 +
>   hw/vfio/display.c | 20 ++++++++++++++++++++
>   hw/vfio/pci.c     | 44 ++++++++++++++++++++++++++++++++++++++++++++
>   stubs/ramfb.c     |  2 ++
>   5 files changed, 70 insertions(+)
> 
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index 2d836093a8..fd06695542 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -173,6 +173,7 @@ struct VFIOPCIDevice {
>       bool no_kvm_ioeventfd;
>       bool no_vfio_ioeventfd;
>       bool enable_ramfb;
> +    OnOffAuto ramfb_migrate;
>       bool defer_kvm_irq_routing;
>       bool clear_parent_atomics_on_exit;
>       VFIODisplay *dpy;
> @@ -226,4 +227,6 @@ void vfio_display_reset(VFIOPCIDevice *vdev);
>   int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp);
>   void vfio_display_finalize(VFIOPCIDevice *vdev);
>   
> +extern const VMStateDescription vfio_display_vmstate;
> +
>   #endif /* HW_VFIO_VFIO_PCI_H */
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index e4361e3d48..f2c59a293c 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -33,6 +33,7 @@
>   
>   GlobalProperty hw_compat_8_1[] = {
>       { "ramfb", "x-migrate", "off" },
> +    { "vfio-pci-nohotplug", "x-ramfb-migrate", "off" }
>   };
>   const size_t hw_compat_8_1_len = G_N_ELEMENTS(hw_compat_8_1);
>   
> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> index bec864f482..0bdb807642 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -542,3 +542,23 @@ void vfio_display_finalize(VFIOPCIDevice *vdev)
>       vfio_display_edid_exit(vdev->dpy);
>       g_free(vdev->dpy);
>   }
> +
> +static bool migrate_needed(void *opaque)
> +{
> +    VFIODisplay *dpy = opaque;
> +    bool ramfb_exists = dpy->ramfb != NULL;
> +
> +    /* see vfio_display_migration_needed() */
> +    assert(ramfb_exists);
> +    return ramfb_exists;
> +}
> +
> +const VMStateDescription vfio_display_vmstate = {
> +    .name = "VFIODisplay",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = migrate_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_STRUCT_POINTER(ramfb, VFIODisplay, ramfb_vmstate, RAMFBState),

This vfio_display_vmstate lacks :

  VMSTATE_END_OF_LIST()

Thanks,

C.

> +    }
> +};
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 3b2ca3c24c..d2ede2f1a2 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2608,6 +2608,32 @@ static bool vfio_msix_present(void *opaque, int version_id)
>       return msix_present(pdev);
>   }
>   
> +static bool vfio_display_migration_needed(void *opaque)
> +{
> +    VFIOPCIDevice *vdev = opaque;
> +
> +    /*
> +     * We need to migrate the VFIODisplay object if ramfb *migration* was
> +     * explicitly requested (in which case we enforced both ramfb=on and
> +     * display=on), or ramfb migration was left at the default "auto"
> +     * setting, and *ramfb* was explicitly requested (in which case we
> +     * enforced display=on).
> +     */
> +    return vdev->ramfb_migrate == ON_OFF_AUTO_ON ||
> +        (vdev->ramfb_migrate == ON_OFF_AUTO_AUTO && vdev->enable_ramfb);
> +}
> +
> +const VMStateDescription vmstate_vfio_display = {
> +    .name = "VFIOPCIDevice/VFIODisplay",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = vfio_display_migration_needed,
> +    .fields = (VMStateField[]){
> +        VMSTATE_STRUCT_POINTER(dpy, VFIOPCIDevice, vfio_display_vmstate, VFIODisplay),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>   const VMStateDescription vmstate_vfio_pci_config = {
>       .name = "VFIOPCIDevice",
>       .version_id = 1,
> @@ -2616,6 +2642,10 @@ const VMStateDescription vmstate_vfio_pci_config = {
>           VMSTATE_PCI_DEVICE(pdev, VFIOPCIDevice),
>           VMSTATE_MSIX_TEST(pdev, VFIOPCIDevice, vfio_msix_present),
>           VMSTATE_END_OF_LIST()
> +    },
> +    .subsections = (const VMStateDescription*[]) {
> +        &vmstate_vfio_display,
> +        NULL
>       }
>   };
>   
> @@ -3271,6 +3301,19 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>           }
>       }
>   
> +    if (vdev->ramfb_migrate == ON_OFF_AUTO_ON && !vdev->enable_ramfb) {
> +        error_setg(errp, "x-ramfb-migrate requires ramfb=on");
> +        goto out_deregister;
> +    }
> +    if (vbasedev->enable_migration == ON_OFF_AUTO_OFF) {
> +        if (vdev->ramfb_migrate == ON_OFF_AUTO_AUTO) {
> +            vdev->ramfb_migrate = ON_OFF_AUTO_OFF;
> +        } else if (vdev->ramfb_migrate == ON_OFF_AUTO_ON) {
> +            error_setg(errp, "x-ramfb-migrate requires enable-migration");
> +            goto out_deregister;
> +        }
> +    }
> +
>       if (!pdev->failover_pair_id) {
>           if (!vfio_migration_realize(vbasedev, errp)) {
>               goto out_deregister;
> @@ -3484,6 +3527,7 @@ static const TypeInfo vfio_pci_dev_info = {
>   
>   static Property vfio_pci_dev_nohotplug_properties[] = {
>       DEFINE_PROP_BOOL("ramfb", VFIOPCIDevice, enable_ramfb, false),
> +    DEFINE_PROP_ON_OFF_AUTO("x-ramfb-migrate", VFIOPCIDevice, ramfb_migrate, ON_OFF_AUTO_AUTO),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   
> diff --git a/stubs/ramfb.c b/stubs/ramfb.c
> index 48143f3354..cf64733b10 100644
> --- a/stubs/ramfb.c
> +++ b/stubs/ramfb.c
> @@ -2,6 +2,8 @@
>   #include "qapi/error.h"
>   #include "hw/display/ramfb.h"
>   
> +const VMStateDescription ramfb_vmstate = {};
> +
>   void ramfb_display_update(QemuConsole *con, RAMFBState *s)
>   {
>   }

Re: [PATCH v4 3/3] hw/vfio: add ramfb migration support

[Laszlo Ersek]

On 10/5/23 13:30, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Add a "VFIODisplay" subsection whenever "x-ramfb-migrate" is turned on.
> 
> Turn it off by default on machines <= 8.1 for compatibility reasons.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/vfio/pci.h     |  3 +++
>  hw/core/machine.c |  1 +
>  hw/vfio/display.c | 20 ++++++++++++++++++++
>  hw/vfio/pci.c     | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  stubs/ramfb.c     |  2 ++
>  5 files changed, 70 insertions(+)
> 
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index 2d836093a8..fd06695542 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -173,6 +173,7 @@ struct VFIOPCIDevice {
>      bool no_kvm_ioeventfd;
>      bool no_vfio_ioeventfd;
>      bool enable_ramfb;
> +    OnOffAuto ramfb_migrate;
>      bool defer_kvm_irq_routing;
>      bool clear_parent_atomics_on_exit;
>      VFIODisplay *dpy;
> @@ -226,4 +227,6 @@ void vfio_display_reset(VFIOPCIDevice *vdev);
>  int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp);
>  void vfio_display_finalize(VFIOPCIDevice *vdev);
>  
> +extern const VMStateDescription vfio_display_vmstate;
> +
>  #endif /* HW_VFIO_VFIO_PCI_H */
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index e4361e3d48..f2c59a293c 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -33,6 +33,7 @@
>  
>  GlobalProperty hw_compat_8_1[] = {
>      { "ramfb", "x-migrate", "off" },
> +    { "vfio-pci-nohotplug", "x-ramfb-migrate", "off" }
>  };
>  const size_t hw_compat_8_1_len = G_N_ELEMENTS(hw_compat_8_1);
>  
> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> index bec864f482..0bdb807642 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -542,3 +542,23 @@ void vfio_display_finalize(VFIOPCIDevice *vdev)
>      vfio_display_edid_exit(vdev->dpy);
>      g_free(vdev->dpy);
>  }
> +
> +static bool migrate_needed(void *opaque)
> +{
> +    VFIODisplay *dpy = opaque;
> +    bool ramfb_exists = dpy->ramfb != NULL;
> +
> +    /* see vfio_display_migration_needed() */
> +    assert(ramfb_exists);
> +    return ramfb_exists;
> +}
> +
> +const VMStateDescription vfio_display_vmstate = {
> +    .name = "VFIODisplay",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = migrate_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_STRUCT_POINTER(ramfb, VFIODisplay, ramfb_vmstate, RAMFBState),
> +    }
> +};
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 3b2ca3c24c..d2ede2f1a2 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2608,6 +2608,32 @@ static bool vfio_msix_present(void *opaque, int version_id)
>      return msix_present(pdev);
>  }
>  
> +static bool vfio_display_migration_needed(void *opaque)
> +{
> +    VFIOPCIDevice *vdev = opaque;
> +
> +    /*
> +     * We need to migrate the VFIODisplay object if ramfb *migration* was
> +     * explicitly requested (in which case we enforced both ramfb=on and
> +     * display=on), or ramfb migration was left at the default "auto"
> +     * setting, and *ramfb* was explicitly requested (in which case we
> +     * enforced display=on).
> +     */
> +    return vdev->ramfb_migrate == ON_OFF_AUTO_ON ||
> +        (vdev->ramfb_migrate == ON_OFF_AUTO_AUTO && vdev->enable_ramfb);
> +}
> +
> +const VMStateDescription vmstate_vfio_display = {
> +    .name = "VFIOPCIDevice/VFIODisplay",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = vfio_display_migration_needed,
> +    .fields = (VMStateField[]){
> +        VMSTATE_STRUCT_POINTER(dpy, VFIOPCIDevice, vfio_display_vmstate, VFIODisplay),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  const VMStateDescription vmstate_vfio_pci_config = {
>      .name = "VFIOPCIDevice",
>      .version_id = 1,
> @@ -2616,6 +2642,10 @@ const VMStateDescription vmstate_vfio_pci_config = {
>          VMSTATE_PCI_DEVICE(pdev, VFIOPCIDevice),
>          VMSTATE_MSIX_TEST(pdev, VFIOPCIDevice, vfio_msix_present),
>          VMSTATE_END_OF_LIST()
> +    },
> +    .subsections = (const VMStateDescription*[]) {
> +        &vmstate_vfio_display,
> +        NULL
>      }
>  };
>  
> @@ -3271,6 +3301,19 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>          }
>      }
>  
> +    if (vdev->ramfb_migrate == ON_OFF_AUTO_ON && !vdev->enable_ramfb) {
> +        error_setg(errp, "x-ramfb-migrate requires ramfb=on");
> +        goto out_deregister;
> +    }
> +    if (vbasedev->enable_migration == ON_OFF_AUTO_OFF) {
> +        if (vdev->ramfb_migrate == ON_OFF_AUTO_AUTO) {
> +            vdev->ramfb_migrate = ON_OFF_AUTO_OFF;
> +        } else if (vdev->ramfb_migrate == ON_OFF_AUTO_ON) {
> +            error_setg(errp, "x-ramfb-migrate requires enable-migration");
> +            goto out_deregister;
> +        }
> +    }
> +
>      if (!pdev->failover_pair_id) {
>          if (!vfio_migration_realize(vbasedev, errp)) {
>              goto out_deregister;
> @@ -3484,6 +3527,7 @@ static const TypeInfo vfio_pci_dev_info = {
>  
>  static Property vfio_pci_dev_nohotplug_properties[] = {
>      DEFINE_PROP_BOOL("ramfb", VFIOPCIDevice, enable_ramfb, false),
> +    DEFINE_PROP_ON_OFF_AUTO("x-ramfb-migrate", VFIOPCIDevice, ramfb_migrate, ON_OFF_AUTO_AUTO),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/stubs/ramfb.c b/stubs/ramfb.c
> index 48143f3354..cf64733b10 100644
> --- a/stubs/ramfb.c
> +++ b/stubs/ramfb.c
> @@ -2,6 +2,8 @@
>  #include "qapi/error.h"
>  #include "hw/display/ramfb.h"
>  
> +const VMStateDescription ramfb_vmstate = {};
> +
>  void ramfb_display_update(QemuConsole *con, RAMFBState *s)
>  {
>  }

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [PATCH v4 0/3] WIP: ramfb: migration support

[Cédric Le Goater]

On 10/5/23 13:30, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Hi,
> 
> Implement RAMFB migration, and add properties to enable it only on >= 8.2
> machines, + a few related cleanups.
> 
> Cedric, did you get the chance to test the VFIO display/ramfb code?

Done with a :

GPU 00000000:00:03.0
     Product Name                          : GRID RTX6000-1Q
     Product Brand                         : NVIDIA RTX Virtual Workstation


rhle9+gdm was running and it looks pretty good after migration.

I would like to go through the models now (I really don't like the
VFIODisplay attributes being in VFIOPCIDevice, it's ugly)

Thanks,

C.


> 
> thanks
> 
> v4: (Laszlo review and suggestions)
> - change migrate_needed() to assert(ramfb_exists)
> - rename vfio_display_needed() to vfio_display_migration_needed(),
>    update the condition and associated comment
> - move the ramfb-migrate option check and add a check for ramfb=on
> - add a stub to fix compilation on some architectures
> 
> v3:
> - add a "x-" prefix to properties, as they are not meant for users.
> - RAMFB now exports a ramfb_vmstate for actual devices to include
> - VFIOPCIDevice now has a VFIODisplay optional subsection whenever ramfb
>    migration is required (untested)
> 
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1859424
> 
> Marc-André Lureau (3):
>    ramfb: add migration support
>    ramfb-standalone: add migration support
>    hw/vfio: add ramfb migration support
> 
>   hw/vfio/pci.h                 |  3 +++
>   include/hw/display/ramfb.h    |  4 ++++
>   hw/core/machine.c             |  5 +++-
>   hw/display/ramfb-standalone.c | 27 +++++++++++++++++++++
>   hw/display/ramfb.c            | 19 +++++++++++++++
>   hw/vfio/display.c             | 20 ++++++++++++++++
>   hw/vfio/pci.c                 | 44 +++++++++++++++++++++++++++++++++++
>   stubs/ramfb.c                 |  2 ++
>   8 files changed, 123 insertions(+), 1 deletion(-)
> 

Re: [PATCH v4 3/3] hw/vfio: add ramfb migration support

[Cédric Le Goater]

On 10/5/23 13:30, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Add a "VFIODisplay" subsection whenever "x-ramfb-migrate" is turned on.
> 
> Turn it off by default on machines <= 8.1 for compatibility reasons.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   hw/vfio/pci.h     |  3 +++
>   hw/core/machine.c |  1 +
>   hw/vfio/display.c | 20 ++++++++++++++++++++
>   hw/vfio/pci.c     | 44 ++++++++++++++++++++++++++++++++++++++++++++
>   stubs/ramfb.c     |  2 ++
>   5 files changed, 70 insertions(+)
> 
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index 2d836093a8..fd06695542 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -173,6 +173,7 @@ struct VFIOPCIDevice {
>       bool no_kvm_ioeventfd;
>       bool no_vfio_ioeventfd;
>       bool enable_ramfb;
> +    OnOffAuto ramfb_migrate;
>       bool defer_kvm_irq_routing;
>       bool clear_parent_atomics_on_exit;
>       VFIODisplay *dpy;
> @@ -226,4 +227,6 @@ void vfio_display_reset(VFIOPCIDevice *vdev);
>   int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp);
>   void vfio_display_finalize(VFIOPCIDevice *vdev);
>   
> +extern const VMStateDescription vfio_display_vmstate;
> +
>   #endif /* HW_VFIO_VFIO_PCI_H */
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index e4361e3d48..f2c59a293c 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -33,6 +33,7 @@
>   
>   GlobalProperty hw_compat_8_1[] = {
>       { "ramfb", "x-migrate", "off" },
> +    { "vfio-pci-nohotplug", "x-ramfb-migrate", "off" }
>   };
>   const size_t hw_compat_8_1_len = G_N_ELEMENTS(hw_compat_8_1);
>   
> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> index bec864f482..0bdb807642 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -542,3 +542,23 @@ void vfio_display_finalize(VFIOPCIDevice *vdev)
>       vfio_display_edid_exit(vdev->dpy);
>       g_free(vdev->dpy);
>   }
> +
> +static bool migrate_needed(void *opaque)
> +{
> +    VFIODisplay *dpy = opaque;
> +    bool ramfb_exists = dpy->ramfb != NULL;
> +
> +    /* see vfio_display_migration_needed() */
> +    assert(ramfb_exists);
> +    return ramfb_exists;
> +}
> +
> +const VMStateDescription vfio_display_vmstate = {
> +    .name = "VFIODisplay",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = migrate_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_STRUCT_POINTER(ramfb, VFIODisplay, ramfb_vmstate, RAMFBState),
> +    }
> +};
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 3b2ca3c24c..d2ede2f1a2 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2608,6 +2608,32 @@ static bool vfio_msix_present(void *opaque, int version_id)
>       return msix_present(pdev);
>   }
>   
> +static bool vfio_display_migration_needed(void *opaque)
> +{
> +    VFIOPCIDevice *vdev = opaque;
> +
> +    /*
> +     * We need to migrate the VFIODisplay object if ramfb *migration* was
> +     * explicitly requested (in which case we enforced both ramfb=on and
> +     * display=on), or ramfb migration was left at the default "auto"
> +     * setting, and *ramfb* was explicitly requested (in which case we
> +     * enforced display=on).
> +     */
> +    return vdev->ramfb_migrate == ON_OFF_AUTO_ON ||
> +        (vdev->ramfb_migrate == ON_OFF_AUTO_AUTO && vdev->enable_ramfb);> +}
> +
> +const VMStateDescription vmstate_vfio_display = {
> +    .name = "VFIOPCIDevice/VFIODisplay",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = vfio_display_migration_needed,
> +    .fields = (VMStateField[]){
> +        VMSTATE_STRUCT_POINTER(dpy, VFIOPCIDevice, vfio_display_vmstate, VFIODisplay),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>   const VMStateDescription vmstate_vfio_pci_config = {
>       .name = "VFIOPCIDevice",
>       .version_id = 1,
> @@ -2616,6 +2642,10 @@ const VMStateDescription vmstate_vfio_pci_config = {
>           VMSTATE_PCI_DEVICE(pdev, VFIOPCIDevice),
>           VMSTATE_MSIX_TEST(pdev, VFIOPCIDevice, vfio_msix_present),
>           VMSTATE_END_OF_LIST()
> +    },
> +    .subsections = (const VMStateDescription*[]) {
> +        &vmstate_vfio_display,
> +        NULL
>       }
>   };
>   
> @@ -3271,6 +3301,19 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>           }
>       }
>   
> +    if (vdev->ramfb_migrate == ON_OFF_AUTO_ON && !vdev->enable_ramfb) {
> +        error_setg(errp, "x-ramfb-migrate requires ramfb=on");

This is a case where QEMU would be run from the command line. Could we
ouput a warning and force "ramfb_migrate" to OFF in that case ? since
the machine would still run.

Thanks,

C.


> +        goto out_deregister;
> +    }
> +    if (vbasedev->enable_migration == ON_OFF_AUTO_OFF) {
> +        if (vdev->ramfb_migrate == ON_OFF_AUTO_AUTO) {
> +            vdev->ramfb_migrate = ON_OFF_AUTO_OFF;
> +        } else if (vdev->ramfb_migrate == ON_OFF_AUTO_ON) {
> +            error_setg(errp, "x-ramfb-migrate requires enable-migration");
> +            goto out_deregister;
> +        }
> +    }
> +
>       if (!pdev->failover_pair_id) {
>           if (!vfio_migration_realize(vbasedev, errp)) {
>               goto out_deregister;
> @@ -3484,6 +3527,7 @@ static const TypeInfo vfio_pci_dev_info = {
>   
>   static Property vfio_pci_dev_nohotplug_properties[] = {
>       DEFINE_PROP_BOOL("ramfb", VFIOPCIDevice, enable_ramfb, false),
> +    DEFINE_PROP_ON_OFF_AUTO("x-ramfb-migrate", VFIOPCIDevice, ramfb_migrate, ON_OFF_AUTO_AUTO),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   
> diff --git a/stubs/ramfb.c b/stubs/ramfb.c
> index 48143f3354..cf64733b10 100644
> --- a/stubs/ramfb.c
> +++ b/stubs/ramfb.c
> @@ -2,6 +2,8 @@
>   #include "qapi/error.h"
>   #include "hw/display/ramfb.h"
>   
> +const VMStateDescription ramfb_vmstate = {};
> +
>   void ramfb_display_update(QemuConsole *con, RAMFBState *s)
>   {
>   }

Re: [PATCH v4 3/3] hw/vfio: add ramfb migration support

[Laszlo Ersek]

On 10/5/23 18:34, Cédric Le Goater wrote:
> On 10/5/23 13:30, marcandre.lureau@redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> Add a "VFIODisplay" subsection whenever "x-ramfb-migrate" is turned on.
>>
>> Turn it off by default on machines <= 8.1 for compatibility reasons.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>   hw/vfio/pci.h     |  3 +++
>>   hw/core/machine.c |  1 +
>>   hw/vfio/display.c | 20 ++++++++++++++++++++
>>   hw/vfio/pci.c     | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>   stubs/ramfb.c     |  2 ++
>>   5 files changed, 70 insertions(+)
>>
>> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
>> index 2d836093a8..fd06695542 100644
>> --- a/hw/vfio/pci.h
>> +++ b/hw/vfio/pci.h
>> @@ -173,6 +173,7 @@ struct VFIOPCIDevice {
>>       bool no_kvm_ioeventfd;
>>       bool no_vfio_ioeventfd;
>>       bool enable_ramfb;
>> +    OnOffAuto ramfb_migrate;
>>       bool defer_kvm_irq_routing;
>>       bool clear_parent_atomics_on_exit;
>>       VFIODisplay *dpy;
>> @@ -226,4 +227,6 @@ void vfio_display_reset(VFIOPCIDevice *vdev);
>>   int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp);
>>   void vfio_display_finalize(VFIOPCIDevice *vdev);
>>   +extern const VMStateDescription vfio_display_vmstate;
>> +
>>   #endif /* HW_VFIO_VFIO_PCI_H */
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index e4361e3d48..f2c59a293c 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -33,6 +33,7 @@
>>     GlobalProperty hw_compat_8_1[] = {
>>       { "ramfb", "x-migrate", "off" },
>> +    { "vfio-pci-nohotplug", "x-ramfb-migrate", "off" }
>>   };
>>   const size_t hw_compat_8_1_len = G_N_ELEMENTS(hw_compat_8_1);
>>   diff --git a/hw/vfio/display.c b/hw/vfio/display.c
>> index bec864f482..0bdb807642 100644
>> --- a/hw/vfio/display.c
>> +++ b/hw/vfio/display.c
>> @@ -542,3 +542,23 @@ void vfio_display_finalize(VFIOPCIDevice *vdev)
>>       vfio_display_edid_exit(vdev->dpy);
>>       g_free(vdev->dpy);
>>   }
>> +
>> +static bool migrate_needed(void *opaque)
>> +{
>> +    VFIODisplay *dpy = opaque;
>> +    bool ramfb_exists = dpy->ramfb != NULL;
>> +
>> +    /* see vfio_display_migration_needed() */
>> +    assert(ramfb_exists);
>> +    return ramfb_exists;
>> +}
>> +
>> +const VMStateDescription vfio_display_vmstate = {
>> +    .name = "VFIODisplay",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .needed = migrate_needed,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_STRUCT_POINTER(ramfb, VFIODisplay, ramfb_vmstate,
>> RAMFBState),
>> +    }
>> +};
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 3b2ca3c24c..d2ede2f1a2 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2608,6 +2608,32 @@ static bool vfio_msix_present(void *opaque, int
>> version_id)
>>       return msix_present(pdev);
>>   }
>>   +static bool vfio_display_migration_needed(void *opaque)
>> +{
>> +    VFIOPCIDevice *vdev = opaque;
>> +
>> +    /*
>> +     * We need to migrate the VFIODisplay object if ramfb *migration*
>> was
>> +     * explicitly requested (in which case we enforced both ramfb=on and
>> +     * display=on), or ramfb migration was left at the default "auto"
>> +     * setting, and *ramfb* was explicitly requested (in which case we
>> +     * enforced display=on).
>> +     */
>> +    return vdev->ramfb_migrate == ON_OFF_AUTO_ON ||
>> +        (vdev->ramfb_migrate == ON_OFF_AUTO_AUTO &&
>> vdev->enable_ramfb);> +}
>> +
>> +const VMStateDescription vmstate_vfio_display = {
>> +    .name = "VFIOPCIDevice/VFIODisplay",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .needed = vfio_display_migration_needed,
>> +    .fields = (VMStateField[]){
>> +        VMSTATE_STRUCT_POINTER(dpy, VFIOPCIDevice,
>> vfio_display_vmstate, VFIODisplay),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>>   const VMStateDescription vmstate_vfio_pci_config = {
>>       .name = "VFIOPCIDevice",
>>       .version_id = 1,
>> @@ -2616,6 +2642,10 @@ const VMStateDescription
>> vmstate_vfio_pci_config = {
>>           VMSTATE_PCI_DEVICE(pdev, VFIOPCIDevice),
>>           VMSTATE_MSIX_TEST(pdev, VFIOPCIDevice, vfio_msix_present),
>>           VMSTATE_END_OF_LIST()
>> +    },
>> +    .subsections = (const VMStateDescription*[]) {
>> +        &vmstate_vfio_display,
>> +        NULL
>>       }
>>   };
>>   @@ -3271,6 +3301,19 @@ static void vfio_realize(PCIDevice *pdev,
>> Error **errp)
>>           }
>>       }
>>   +    if (vdev->ramfb_migrate == ON_OFF_AUTO_ON &&
>> !vdev->enable_ramfb) {
>> +        error_setg(errp, "x-ramfb-migrate requires ramfb=on");
> 
> This is a case where QEMU would be run from the command line. Could we
> ouput a warning and force "ramfb_migrate" to OFF in that case ? since
> the machine would still run.

Sounds like a valid idea to me:

- consistency between x-ramfb-migrate and ramfb would be maintained

- x-* properties are not meant as user interface, so QEMU doesn't
guarantee anything about them, AIUI

Laszlo

> 
> Thanks,
> 
> C.
> 
> 
>> +        goto out_deregister;
>> +    }
>> +    if (vbasedev->enable_migration == ON_OFF_AUTO_OFF) {
>> +        if (vdev->ramfb_migrate == ON_OFF_AUTO_AUTO) {
>> +            vdev->ramfb_migrate = ON_OFF_AUTO_OFF;
>> +        } else if (vdev->ramfb_migrate == ON_OFF_AUTO_ON) {
>> +            error_setg(errp, "x-ramfb-migrate requires
>> enable-migration");
>> +            goto out_deregister;
>> +        }
>> +    }
>> +
>>       if (!pdev->failover_pair_id) {
>>           if (!vfio_migration_realize(vbasedev, errp)) {
>>               goto out_deregister;
>> @@ -3484,6 +3527,7 @@ static const TypeInfo vfio_pci_dev_info = {
>>     static Property vfio_pci_dev_nohotplug_properties[] = {
>>       DEFINE_PROP_BOOL("ramfb", VFIOPCIDevice, enable_ramfb, false),
>> +    DEFINE_PROP_ON_OFF_AUTO("x-ramfb-migrate", VFIOPCIDevice,
>> ramfb_migrate, ON_OFF_AUTO_AUTO),
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   diff --git a/stubs/ramfb.c b/stubs/ramfb.c
>> index 48143f3354..cf64733b10 100644
>> --- a/stubs/ramfb.c
>> +++ b/stubs/ramfb.c
>> @@ -2,6 +2,8 @@
>>   #include "qapi/error.h"
>>   #include "hw/display/ramfb.h"
>>   +const VMStateDescription ramfb_vmstate = {};
>> +
>>   void ramfb_display_update(QemuConsole *con, RAMFBState *s)
>>   {
>>   }
>