[PATCH v5 0/3] ramfb: migration support

[PATCH v5 0/3] 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.

thanks

v5:
- add missing VMSTATE_END_OF_LIST
- changed ramfb=off & x-mig=on user config error to a warning
- add r-b tags

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             |  2 ++
 hw/display/ramfb-standalone.c | 27 +++++++++++++++++++++
 hw/display/ramfb.c            | 19 +++++++++++++++
 hw/vfio/display.c             | 21 +++++++++++++++++
 hw/vfio/pci.c                 | 44 +++++++++++++++++++++++++++++++++++
 stubs/ramfb.c                 |  2 ++
 8 files changed, 122 insertions(+)

-- 
2.41.0

[PATCH v5 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>
Reviewed-by: Laszlo Ersek <lersek@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 v5 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>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/core/machine.c             |  1 +
 hw/display/ramfb-standalone.c | 27 +++++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index cfd1edfe20..6305f2d7a4 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -34,6 +34,7 @@
 
 GlobalProperty hw_compat_8_1[] = {
     { TYPE_PCI_BRIDGE, "x-pci-express-writeable-slt-bug", "true" },
+    { "ramfb", "x-migrate", "off" },
 };
 const size_t hw_compat_8_1_len = G_N_ELEMENTS(hw_compat_8_1);
 
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 v5 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>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/vfio/pci.h     |  3 +++
 hw/core/machine.c |  1 +
 hw/vfio/display.c | 21 +++++++++++++++++++++
 hw/vfio/pci.c     | 44 ++++++++++++++++++++++++++++++++++++++++++++
 stubs/ramfb.c     |  2 ++
 5 files changed, 71 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 6305f2d7a4..05aef2cf9f 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -35,6 +35,7 @@
 GlobalProperty hw_compat_8_1[] = {
     { TYPE_PCI_BRIDGE, "x-pci-express-writeable-slt-bug", "true" },
     { "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..2739ba56ec 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -542,3 +542,24 @@ 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),
+        VMSTATE_END_OF_LIST(),
+    }
+};
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 3b2ca3c24c..e44ed21180 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) {
+        warn_report("x-ramfb-migrate=on but ramfb=off");
+        vdev->ramfb_migrate = ON_OFF_AUTO_OFF;
+    }
+    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 v5 3/3] hw/vfio: add ramfb migration support

[Laszlo Ersek]

On 10/9/23 08:32, 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>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  hw/vfio/pci.h     |  3 +++
>  hw/core/machine.c |  1 +
>  hw/vfio/display.c | 21 +++++++++++++++++++++
>  hw/vfio/pci.c     | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  stubs/ramfb.c     |  2 ++
>  5 files changed, 71 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 6305f2d7a4..05aef2cf9f 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -35,6 +35,7 @@
>  GlobalProperty hw_compat_8_1[] = {
>      { TYPE_PCI_BRIDGE, "x-pci-express-writeable-slt-bug", "true" },
>      { "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..2739ba56ec 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -542,3 +542,24 @@ 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),
> +        VMSTATE_END_OF_LIST(),
> +    }
> +};
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 3b2ca3c24c..e44ed21180 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) {
> +        warn_report("x-ramfb-migrate=on but ramfb=off");
> +        vdev->ramfb_migrate = ON_OFF_AUTO_OFF;

the warning could give a hint about the resultant action taken (i.e.,
forcing off x-ramfb-migrate), but don't repost just for that; it's a nit.

My R-b stands.

Thanks
Laszlo

> +    }
> +    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 v5 3/3] hw/vfio: add ramfb migration support

[Cédric Le Goater]

On 10/9/23 12:19, Laszlo Ersek wrote:
> On 10/9/23 08:32, 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>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>   hw/vfio/pci.h     |  3 +++
>>   hw/core/machine.c |  1 +
>>   hw/vfio/display.c | 21 +++++++++++++++++++++
>>   hw/vfio/pci.c     | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>   stubs/ramfb.c     |  2 ++
>>   5 files changed, 71 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 6305f2d7a4..05aef2cf9f 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -35,6 +35,7 @@
>>   GlobalProperty hw_compat_8_1[] = {
>>       { TYPE_PCI_BRIDGE, "x-pci-express-writeable-slt-bug", "true" },
>>       { "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..2739ba56ec 100644
>> --- a/hw/vfio/display.c
>> +++ b/hw/vfio/display.c
>> @@ -542,3 +542,24 @@ 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),
>> +        VMSTATE_END_OF_LIST(),
>> +    }
>> +};
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 3b2ca3c24c..e44ed21180 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) {
>> +        warn_report("x-ramfb-migrate=on but ramfb=off");
>> +        vdev->ramfb_migrate = ON_OFF_AUTO_OFF;
> 
> the warning could give a hint about the resultant action taken (i.e.,
> forcing off x-ramfb-migrate), but don't repost just for that; it's a nit.

yes.

How about :

         warn_report("x-ramfb-migrate=on but ramfb=off. Forcing x-ramfb-migrate to off.");

I can amend the patch if you agree.

Thanks,

C.

> My R-b stands.
> 
> Thanks
> Laszlo
> 
>> +    }
>> +    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 v5 3/3] hw/vfio: add ramfb migration support

[Marc-André Lureau]

Hi

On Tue, Oct 10, 2023 at 11:03 AM Cédric Le Goater <clg@redhat.com> wrote:
>
> On 10/9/23 12:19, Laszlo Ersek wrote:
> > On 10/9/23 08:32, 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>
> >> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> >> ---
> >>   hw/vfio/pci.h     |  3 +++
> >>   hw/core/machine.c |  1 +
> >>   hw/vfio/display.c | 21 +++++++++++++++++++++
> >>   hw/vfio/pci.c     | 44 ++++++++++++++++++++++++++++++++++++++++++++
> >>   stubs/ramfb.c     |  2 ++
> >>   5 files changed, 71 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 6305f2d7a4..05aef2cf9f 100644
> >> --- a/hw/core/machine.c
> >> +++ b/hw/core/machine.c
> >> @@ -35,6 +35,7 @@
> >>   GlobalProperty hw_compat_8_1[] = {
> >>       { TYPE_PCI_BRIDGE, "x-pci-express-writeable-slt-bug", "true" },
> >>       { "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..2739ba56ec 100644
> >> --- a/hw/vfio/display.c
> >> +++ b/hw/vfio/display.c
> >> @@ -542,3 +542,24 @@ 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),
> >> +        VMSTATE_END_OF_LIST(),
> >> +    }
> >> +};
> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> >> index 3b2ca3c24c..e44ed21180 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) {
> >> +        warn_report("x-ramfb-migrate=on but ramfb=off");
> >> +        vdev->ramfb_migrate = ON_OFF_AUTO_OFF;
> >
> > the warning could give a hint about the resultant action taken (i.e.,
> > forcing off x-ramfb-migrate), but don't repost just for that; it's a nit.
>
> yes.
>
> How about :
>
>          warn_report("x-ramfb-migrate=on but ramfb=off. Forcing x-ramfb-migrate to off.");

Sure, that's better.

>
> I can amend the patch if you agree.

 thanks!

Re: [PATCH v5 0/3] ramfb: migration support

[Cédric Le Goater]

Hello,

On 10/9/23 08:32, 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.

Should this series go in vfio-next ?

Thanks,

C.

> 
> thanks
> 
> v5:
> - add missing VMSTATE_END_OF_LIST
> - changed ramfb=off & x-mig=on user config error to a warning
> - add r-b tags
> 
> 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             |  2 ++
>   hw/display/ramfb-standalone.c | 27 +++++++++++++++++++++
>   hw/display/ramfb.c            | 19 +++++++++++++++
>   hw/vfio/display.c             | 21 +++++++++++++++++
>   hw/vfio/pci.c                 | 44 +++++++++++++++++++++++++++++++++++
>   stubs/ramfb.c                 |  2 ++
>   8 files changed, 122 insertions(+)
> 

Re: [PATCH v5 0/3] ramfb: migration support

[Marc-André Lureau]

Hi

On Tue, Oct 10, 2023 at 4:49 PM Cédric Le Goater <clg@redhat.com> wrote:
>
> Hello,
>
> On 10/9/23 08:32, 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.
>
> Should this series go in vfio-next ?
>

That's a good option for me.

thanks

-- 
Marc-André Lureau

Re: [PATCH v5 0/3] ramfb: migration support

[Cédric Le Goater]

On 10/10/23 15:46, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Oct 10, 2023 at 4:49 PM Cédric Le Goater <clg@redhat.com> wrote:
>>
>> Hello,
>>
>> On 10/9/23 08:32, 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.
>>
>> Should this series go in vfio-next ?
>>
> 
> That's a good option for me.

Gerd, fine with you ?

Thanks,

C.

Re: [PATCH v5 0/3] ramfb: migration support

[Gerd Hoffmann]

On Wed, Oct 11, 2023 at 07:09:19PM +0200, Cédric Le Goater wrote:
> On 10/10/23 15:46, Marc-André Lureau wrote:
> > Hi
> > 
> > On Tue, Oct 10, 2023 at 4:49 PM Cédric Le Goater <clg@redhat.com> wrote:
> > > 
> > > Hello,
> > > 
> > > On 10/9/23 08:32, 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.
> > > 
> > > Should this series go in vfio-next ?
> > > 
> > 
> > That's a good option for me.
> 
> Gerd, fine with you ?

Yes.

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

take care,
  Gerd

Re: [PATCH v5 0/3] ramfb: migration support

[Cédric Le Goater]

On 10/12/23 11:18, Gerd Hoffmann wrote:
> On Wed, Oct 11, 2023 at 07:09:19PM +0200, Cédric Le Goater wrote:
>> On 10/10/23 15:46, Marc-André Lureau wrote:
>>> Hi
>>>
>>> On Tue, Oct 10, 2023 at 4:49 PM Cédric Le Goater <clg@redhat.com> wrote:
>>>>
>>>> Hello,
>>>>
>>>> On 10/9/23 08:32, 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.
>>>>
>>>> Should this series go in vfio-next ?
>>>>
>>>
>>> That's a good option for me.
>>
>> Gerd, fine with you ?
> 
> Yes.
> 
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>



Thanks,

Applied to vfio-next.


C.