[PATCH v3 0/5] WIP: ramfb: migration support

[PATCH v3 0/5] 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.

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 (5):
  hw/core: remove needless includes
  hw/pc: remove needless includes
  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             | 15 ++++---------
 hw/display/ramfb-standalone.c | 27 +++++++++++++++++++++++
 hw/display/ramfb.c            | 19 ++++++++++++++++
 hw/i386/pc.c                  | 41 -----------------------------------
 hw/vfio/display.c             | 23 ++++++++++++++++++++
 hw/vfio/pci.c                 | 32 +++++++++++++++++++++++++++
 8 files changed, 112 insertions(+), 52 deletions(-)

-- 
2.41.0

[PATCH v3 1/5] hw/core: remove needless includes

[marcandre.lureau]

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

The include list is large, make it smaller.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Acked-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/core/machine.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index a232ae0bcd..df40f10dfa 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -11,32 +11,22 @@
  */
 
 #include "qemu/osdep.h"
-#include "qemu/option.h"
 #include "qemu/accel.h"
 #include "sysemu/replay.h"
-#include "qemu/units.h"
 #include "hw/boards.h"
 #include "hw/loader.h"
 #include "qapi/error.h"
-#include "qapi/qapi-visit-common.h"
 #include "qapi/qapi-visit-machine.h"
-#include "qapi/visitor.h"
 #include "qom/object_interfaces.h"
-#include "hw/sysbus.h"
 #include "sysemu/cpus.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/reset.h"
 #include "sysemu/runstate.h"
-#include "sysemu/numa.h"
 #include "sysemu/xen.h"
-#include "qemu/error-report.h"
 #include "sysemu/qtest.h"
-#include "hw/pci/pci.h"
 #include "hw/mem/nvdimm.h"
 #include "migration/global_state.h"
-#include "migration/vmstate.h"
 #include "exec/confidential-guest-support.h"
-#include "hw/virtio/virtio.h"
 #include "hw/virtio/virtio-pci.h"
 #include "hw/virtio/virtio-net.h"
 
-- 
2.41.0

[PATCH v3 2/5] hw/pc: remove needless includes

[marcandre.lureau]

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

The include list is gigantic, make it smaller.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Acked-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/i386/pc.c | 41 -----------------------------------------
 1 file changed, 41 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 5d399b6247..c376c5032d 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -24,79 +24,38 @@
 
 #include "qemu/osdep.h"
 #include "qemu/units.h"
-#include "hw/i386/x86.h"
 #include "hw/i386/pc.h"
 #include "hw/char/serial.h"
 #include "hw/char/parallel.h"
-#include "hw/i386/topology.h"
 #include "hw/i386/fw_cfg.h"
 #include "hw/i386/vmport.h"
 #include "sysemu/cpus.h"
-#include "hw/block/fdc.h"
 #include "hw/ide/internal.h"
-#include "hw/ide/isa.h"
-#include "hw/pci/pci.h"
-#include "hw/pci/pci_bus.h"
-#include "hw/pci-bridge/pci_expander_bridge.h"
-#include "hw/nvram/fw_cfg.h"
 #include "hw/timer/hpet.h"
-#include "hw/firmware/smbios.h"
 #include "hw/loader.h"
-#include "elf.h"
-#include "migration/vmstate.h"
-#include "multiboot.h"
 #include "hw/rtc/mc146818rtc.h"
 #include "hw/intc/i8259.h"
-#include "hw/intc/ioapic.h"
 #include "hw/timer/i8254.h"
 #include "hw/input/i8042.h"
-#include "hw/irq.h"
 #include "hw/audio/pcspk.h"
-#include "hw/pci/msi.h"
-#include "hw/sysbus.h"
 #include "sysemu/sysemu.h"
-#include "sysemu/tcg.h"
-#include "sysemu/numa.h"
-#include "sysemu/kvm.h"
 #include "sysemu/xen.h"
 #include "sysemu/reset.h"
-#include "sysemu/runstate.h"
 #include "kvm/kvm_i386.h"
-#include "hw/xen/xen.h"
-#include "hw/xen/start_info.h"
-#include "ui/qemu-spice.h"
-#include "exec/memory.h"
-#include "qemu/bitmap.h"
-#include "qemu/config-file.h"
-#include "qemu/error-report.h"
-#include "qemu/option.h"
-#include "qemu/cutils.h"
-#include "hw/acpi/acpi.h"
 #include "hw/acpi/cpu_hotplug.h"
 #include "acpi-build.h"
-#include "hw/mem/pc-dimm.h"
 #include "hw/mem/nvdimm.h"
-#include "hw/cxl/cxl.h"
 #include "hw/cxl/cxl_host.h"
-#include "qapi/error.h"
-#include "qapi/qapi-visit-common.h"
-#include "qapi/qapi-visit-machine.h"
-#include "qapi/visitor.h"
-#include "hw/core/cpu.h"
 #include "hw/usb.h"
 #include "hw/i386/intel_iommu.h"
 #include "hw/net/ne2000-isa.h"
-#include "standard-headers/asm-x86/bootparam.h"
 #include "hw/virtio/virtio-iommu.h"
 #include "hw/virtio/virtio-md-pci.h"
 #include "hw/i386/kvm/xen_overlay.h"
 #include "hw/i386/kvm/xen_evtchn.h"
 #include "hw/i386/kvm/xen_gnttab.h"
 #include "hw/i386/kvm/xen_xenstore.h"
-#include "sysemu/replay.h"
-#include "target/i386/cpu.h"
 #include "e820_memory_layout.h"
-#include "fw_cfg.h"
 #include "trace.h"
 #include CONFIG_DEVICES
 
-- 
2.41.0

[PATCH v3 3/5] 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 79b9754a58..32ccb7053b 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;
@@ -115,6 +117,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 v3 4/5] 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 df40f10dfa..47a07d1d9b 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -30,7 +30,9 @@
 #include "hw/virtio/virtio-pci.h"
 #include "hw/virtio/virtio-net.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 v3 5/5] 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 | 23 +++++++++++++++++++++++
 hw/vfio/pci.c     | 32 ++++++++++++++++++++++++++++++++
 4 files changed, 59 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 47a07d1d9b..f2f8940a85 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -32,6 +32,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..de5bf71dd1 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -542,3 +542,26 @@ void vfio_display_finalize(VFIOPCIDevice *vdev)
     vfio_display_edid_exit(vdev->dpy);
     g_free(vdev->dpy);
 }
+
+static bool migrate_needed(void *opaque)
+{
+    /*
+     * If we are here, it's because vfio_display_needed(), which is only true
+     * when dpy->ramfb_migrate atm.
+     *
+     * If the migration condition is changed, we should check here if
+     * ramfb_migrate is true. (this will need a way to lookup the associated
+     * VFIOPCIDevice somehow, or fields to be moved, ..)
+     */
+    return true;
+}
+
+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..4689f2e5c1 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2608,6 +2608,25 @@ static bool vfio_msix_present(void *opaque, int version_id)
     return msix_present(pdev);
 }
 
+static bool vfio_display_needed(void *opaque)
+{
+    VFIOPCIDevice *vdev = opaque;
+
+    /* the only thing that justifies the VFIODisplay sub-section atm */
+    return vdev->ramfb_migrate != ON_OFF_AUTO_OFF;
+}
+
+const VMStateDescription vmstate_vfio_display = {
+    .name = "VFIOPCIDevice/VFIODisplay",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = vfio_display_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 +2635,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
     }
 };
 
@@ -3275,6 +3298,14 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
         if (!vfio_migration_realize(vbasedev, errp)) {
             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 migration");
+                goto out_deregister;
+            }
+        }
     }
 
     vfio_register_err_notifier(vdev);
@@ -3484,6 +3515,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(),
 };
 
-- 
2.41.0

Re: [PATCH v3 4/5] ramfb-standalone: add migration support

[Laszlo Ersek]

On 10/3/23 10:56, 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 df40f10dfa..47a07d1d9b 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -30,7 +30,9 @@
>  #include "hw/virtio/virtio-pci.h"
>  #include "hw/virtio/virtio-net.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 = {

This patch (and patch #3) is *exactly* what I had in mind, when I was
racking my brain about RHBZ 1859424 a week (or a few weeks?) ago.
Specifically the VMSTATE_STRUCT_POINTER + VMSTATE_BUFFER_UNSAFE chain.

That's not to say I could have *completed* the patch myself, of course.
:) (From the many things, I didn't know that it was
VMSTATE_BUFFER_UNSAFE that we'd need -- but I certainly thought of
VMSTATE_STRUCT_POINTER for the higher-level devices.)

It's interesting that we don't have to do this in a "subsection" here --
this device used to have no VMStateDescription at all!

FWIW (patches #3 and #4):

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

Laszlo

Re: [PATCH v3 5/5] hw/vfio: add ramfb migration support

[Cédric Le Goater]

On 10/3/23 10:56, 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.


This change breaks linking on various platforms with :

/usr/bin/ld: libqemu-xtensa-softmmu.fa.p/hw_vfio_display.c.o:(.data.rel+0x50): undefined reference to `ramfb_vmstate'

Some stubs updates are missing it seems..

Thanks,

C.

> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   hw/vfio/pci.h     |  3 +++
>   hw/core/machine.c |  1 +
>   hw/vfio/display.c | 23 +++++++++++++++++++++++
>   hw/vfio/pci.c     | 32 ++++++++++++++++++++++++++++++++
>   4 files changed, 59 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 47a07d1d9b..f2f8940a85 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -32,6 +32,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..de5bf71dd1 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -542,3 +542,26 @@ void vfio_display_finalize(VFIOPCIDevice *vdev)
>       vfio_display_edid_exit(vdev->dpy);
>       g_free(vdev->dpy);
>   }
> +
> +static bool migrate_needed(void *opaque)
> +{
> +    /*
> +     * If we are here, it's because vfio_display_needed(), which is only true
> +     * when dpy->ramfb_migrate atm.
> +     *
> +     * If the migration condition is changed, we should check here if
> +     * ramfb_migrate is true. (this will need a way to lookup the associated
> +     * VFIOPCIDevice somehow, or fields to be moved, ..)
> +     */
> +    return true;
> +}
> +
> +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..4689f2e5c1 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2608,6 +2608,25 @@ static bool vfio_msix_present(void *opaque, int version_id)
>       return msix_present(pdev);
>   }
>   
> +static bool vfio_display_needed(void *opaque)
> +{
> +    VFIOPCIDevice *vdev = opaque;
> +
> +    /* the only thing that justifies the VFIODisplay sub-section atm */
> +    return vdev->ramfb_migrate != ON_OFF_AUTO_OFF;
> +}
> +
> +const VMStateDescription vmstate_vfio_display = {
> +    .name = "VFIOPCIDevice/VFIODisplay",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = vfio_display_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 +2635,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
>       }
>   };
>   
> @@ -3275,6 +3298,14 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>           if (!vfio_migration_realize(vbasedev, errp)) {
>               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 migration");
> +                goto out_deregister;
> +            }
> +        }
>       }
>   
>       vfio_register_err_notifier(vdev);
> @@ -3484,6 +3515,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(),
>   };
>   

Re: [PATCH v3 5/5] hw/vfio: add ramfb migration support

[Marc-André Lureau]

Hi

On Tue, Oct 3, 2023 at 2:17 PM Cédric Le Goater <clg@redhat.com> wrote:
>
> On 10/3/23 10:56, 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.
>
>
> This change breaks linking on various platforms with :
>
> /usr/bin/ld: libqemu-xtensa-softmmu.fa.p/hw_vfio_display.c.o:(.data.rel+0x50): undefined reference to `ramfb_vmstate'
>
> Some stubs updates are missing it seems..
>

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 = {};
+


And I think we should also change the "needed" condition to:

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 4689f2e5c1..b327844764 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2613,7 +2613,7 @@ static bool vfio_display_needed(void *opaque)
     VFIOPCIDevice *vdev = opaque;

     /* the only thing that justifies the VFIODisplay sub-section atm */
-    return vdev->ramfb_migrate != ON_OFF_AUTO_OFF;
+    return vdev->enable_ramfb && vdev->ramfb_migrate != ON_OFF_AUTO_OFF;
 }



> Thanks,
>
> C.
>
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >   hw/vfio/pci.h     |  3 +++
> >   hw/core/machine.c |  1 +
> >   hw/vfio/display.c | 23 +++++++++++++++++++++++
> >   hw/vfio/pci.c     | 32 ++++++++++++++++++++++++++++++++
> >   4 files changed, 59 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 47a07d1d9b..f2f8940a85 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -32,6 +32,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..de5bf71dd1 100644
> > --- a/hw/vfio/display.c
> > +++ b/hw/vfio/display.c
> > @@ -542,3 +542,26 @@ void vfio_display_finalize(VFIOPCIDevice *vdev)
> >       vfio_display_edid_exit(vdev->dpy);
> >       g_free(vdev->dpy);
> >   }
> > +
> > +static bool migrate_needed(void *opaque)
> > +{
> > +    /*
> > +     * If we are here, it's because vfio_display_needed(), which is only true
> > +     * when dpy->ramfb_migrate atm.
> > +     *
> > +     * If the migration condition is changed, we should check here if
> > +     * ramfb_migrate is true. (this will need a way to lookup the associated
> > +     * VFIOPCIDevice somehow, or fields to be moved, ..)
> > +     */
> > +    return true;
> > +}
> > +
> > +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..4689f2e5c1 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -2608,6 +2608,25 @@ static bool vfio_msix_present(void *opaque, int version_id)
> >       return msix_present(pdev);
> >   }
> >
> > +static bool vfio_display_needed(void *opaque)
> > +{
> > +    VFIOPCIDevice *vdev = opaque;
> > +
> > +    /* the only thing that justifies the VFIODisplay sub-section atm */
> > +    return vdev->ramfb_migrate != ON_OFF_AUTO_OFF;
> > +}
> > +
> > +const VMStateDescription vmstate_vfio_display = {
> > +    .name = "VFIOPCIDevice/VFIODisplay",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .needed = vfio_display_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 +2635,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
> >       }
> >   };
> >
> > @@ -3275,6 +3298,14 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
> >           if (!vfio_migration_realize(vbasedev, errp)) {
> >               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 migration");
> > +                goto out_deregister;
> > +            }
> > +        }
> >       }
> >
> >       vfio_register_err_notifier(vdev);
> > @@ -3484,6 +3515,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(),
> >   };
> >
>
>


-- 
Marc-André Lureau

Re: [PATCH v3 5/5] hw/vfio: add ramfb migration support

[Laszlo Ersek]

On 10/3/23 10:56, 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 | 23 +++++++++++++++++++++++
>  hw/vfio/pci.c     | 32 ++++++++++++++++++++++++++++++++
>  4 files changed, 59 insertions(+)

Quoting the hunks somewhat out of order (so that I can better understand
the dependencies):

>
> 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;

So this is where the complications start. The structure that contains
the "RAMFBState *ramfb" field is VFIODisplay (i.e., the "dpy" field
here), not VFIOPCIDevice. In order to stay consistent with the previous
patch in the series, we'd have to add the flag to VFIODisplay.

But it seems that VFIODisplay cannot have its own properties. Because:

- properties are associated in a TypeInfo.class_init function, with
  device_class_set_props()

- there is no TypeInfo for VFIODisplay, only for VFIOPCIDevice.

To make things even more complicated, both TYPE_VFIO_PCI and
TYPE_VFIO_PCI_NOHOTPLUG use VFIOPCIDevice directly. The latter only
differs in the exposure of the property "ramfb". Commit b290659fc3dd
("hw/vfio/display: add ramfb support", 2018-10-15) introduced the
property with TYPE_VFIO_PCI_NOHOTPLUG, *but* it squeezed the boolean
field backing the property to VFIOPCIDevice, rather than creating a
separate structure type *containing* VFIOPCIDevice *plus* the new field
"enable_ramfb".

I'm not sure if that was optimal (it may have been "unavoidable" for all
I know), but either way, now we have no choice but to follow suit, and
add the property's "backing field" to VFIOPCIDevice *again*.

OK. I kinda convinced myself this is proper. Let's move on.

> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 3b2ca3c24c..4689f2e5c1 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3484,6 +3515,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(),
>  };
>

This is where we expose the new knob as a property, side to side with
the previously mentioned "ramfb" one. OK.

> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 47a07d1d9b..f2f8940a85 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -32,6 +32,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);
>

Setting the property to "off" for old machine types, OK.

> @@ -3275,6 +3298,14 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>          if (!vfio_migration_realize(vbasedev, errp)) {
>              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 migration");
> +                goto out_deregister;
> +            }
> +        }
>      }
>
>      vfio_register_err_notifier(vdev);

This looks good to me from two aspects, and not so good from two other
aspects.

- good: seems to ensure the knob consistency that Alex highlighted

- good: we enforce the "ramfb=on requires display=on" predicate too in
  the same function

- (1) not so good: with regard to error handling, I think the placement
  of this new logic could be improved, IMO. If we fail here, we're still
  past a successful vfio_migration_realize() call, and I'm not sure if
  jumping to the "out_deregister" label can undo *that*. Right now,
  nothing after the vfio_migration_realize() call can fail inside
  vfio_realize(), so the error path may not be ready for rolling back
  vfio_migration_realize().

  Looking at the larger context:

    if (vdev->display_xres || vdev->display_yres) {
        if (vdev->dpy == NULL) {
            error_setg(errp, "xres and yres properties require display=on");
            goto out_deregister;
        }
        if (vdev->dpy->edid_regs == NULL) {
            error_setg(errp, "xres and yres properties need edid support");
            goto out_deregister;
        }
    }

    if (!pdev->failover_pair_id) {
        if (!vfio_migration_realize(vbasedev, errp)) {
            goto out_deregister;
        }
    }

  I suggest inserting the new consistency check *between* those two
  checks (and not after the second one). The reason is that both
  existent checks jump to "out_deregister" in case of failure, which
  means that you can insert further checks (and actions that don't
  allocate new resources) between them, and jump to "out_deregister" in
  case of failure.

- (2) not so good: we should reject an explicit "x-ramfb-migrate=on"
  setting if "enable_ramfb" is false. The valid (accepted) combinations
  are:

  #  display  ramfb  x-ramfb-migrate
  -  -------  -----  ---------------
  1  off      off    auto
  2  off      off    off
  3  on       off    auto
  4  on       off    off
  5  on       on     auto
  6  on       on     off
  7  on       on     on

  "ramfb ==> display" is alread enforced; we need to check
  "(x-ramfb-migrate==on) ==> ramfb" additionally.

Now the tricky part follows:

> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 3b2ca3c24c..4689f2e5c1 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2608,6 +2608,25 @@ static bool vfio_msix_present(void *opaque, int version_id)
>      return msix_present(pdev);
>  }
>
> +static bool vfio_display_needed(void *opaque)
> +{
> +    VFIOPCIDevice *vdev = opaque;
> +
> +    /* the only thing that justifies the VFIODisplay sub-section atm */
> +    return vdev->ramfb_migrate != ON_OFF_AUTO_OFF;
> +}

This doesn't look right to me. Consider again the table of valid cases
(as enforced by vfio_realize() per my suggestion above):

  #  display  ramfb  x-ramfb-migrate  result
  -  -------  -----  ---------------  -------------------------------------
  1  off      off    auto             no VFIODisplay object --> CRASH
  2  off      off    off              VFIODisplay migration unnecessary, OK
  3  on       off    auto             no RAMFBState object --> CRASH
  4  on       off    off              VFIODisplay migration unnecessary, OK
  5  on       on     auto             VFIODisplay migration needed, OK
  6  on       on     off              VFIODisplay migration unnecessary, OK
  7  on       on     on               VFIODisplay migration needed, OK

Suggestions:

- (3) name the function vfio_display_migration_needed()

- the condition should be:

    vdev->ramfb_migrate != ON_OFF_AUTO_OFF && vdev->enable_ramfb

  The condition (vdev->ramfb_migrate != ON_OFF_AUTO_OFF) restricts the
  table to the following rows:

    #  display  ramfb  x-ramfb-migrate  result
    -  -------  -----  ---------------  -------------------------------------
    1  off      off    auto             no VFIODisplay object --> CRASH
    3  on       off    auto             no RAMFBState object --> CRASH
    5  on       on     auto             VFIODisplay migration needed, OK
    7  on       on     on               VFIODisplay migration needed, OK

  From these, cases #5 and #7 are fine, and the additional restriction
  (vdev->enable_ramfb) leaves them unhurt.

  We need to exclude cases #1 and #3, and that's what the additional
  restriction (vdev->enable_ramfb) will do.

- (4) *Equivalently*, you could use the *easier-to-understand* condition

    vdev->ramfb_migrate == ON_OFF_AUTO_ON ||
    (vdev->ramfb_migrate == ON_OFF_AUTO_AUTO && vdev->enable_ramfb)

- (5) And then I'd suggest adding the comment (which matches the latter
  formulation of the condition):

  /*
   * 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).
   */

> +
> +const VMStateDescription vmstate_vfio_display = {
> +    .name = "VFIOPCIDevice/VFIODisplay",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = vfio_display_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 +2635,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
>      }
>  };
>

Seems OK.

> 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
> @@ -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 */

Seems right.

> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> index bec864f482..de5bf71dd1 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -542,3 +542,26 @@ void vfio_display_finalize(VFIOPCIDevice *vdev)
>      vfio_display_edid_exit(vdev->dpy);
>      g_free(vdev->dpy);
>  }
> +
> +static bool migrate_needed(void *opaque)
> +{
> +    /*
> +     * If we are here, it's because vfio_display_needed(), which is only true
> +     * when dpy->ramfb_migrate atm.
> +     *
> +     * If the migration condition is changed, we should check here if
> +     * ramfb_migrate is true. (this will need a way to lookup the associated
> +     * VFIOPCIDevice somehow, or fields to be moved, ..)
> +     */
> +    return true;
> +}

(6) Adding this test function with constant "true" returned seems
helpful, yes; how about the following impl though:

  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),
> +    }
> +};

Thanks!
Laszlo

Re: [PATCH v3 5/5] hw/vfio: add ramfb migration support

[Laszlo Ersek]

On 10/3/23 12:47, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Oct 3, 2023 at 2:17 PM Cédric Le Goater <clg@redhat.com> wrote:
>>
>> On 10/3/23 10:56, 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.
>>
>>
>> This change breaks linking on various platforms with :
>>
>> /usr/bin/ld: libqemu-xtensa-softmmu.fa.p/hw_vfio_display.c.o:(.data.rel+0x50): undefined reference to `ramfb_vmstate'
>>
>> Some stubs updates are missing it seems..
>>
> 
> 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 = {};
> +
> 
> 
> And I think we should also change the "needed" condition to:
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 4689f2e5c1..b327844764 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2613,7 +2613,7 @@ static bool vfio_display_needed(void *opaque)
>      VFIOPCIDevice *vdev = opaque;
> 
>      /* the only thing that justifies the VFIODisplay sub-section atm */
> -    return vdev->ramfb_migrate != ON_OFF_AUTO_OFF;
> +    return vdev->enable_ramfb && vdev->ramfb_migrate != ON_OFF_AUTO_OFF;
>  }
> 

Exactly, this was one of my comments in review.

(But, see there -- I think the other formulation is easier to read and
understand.)

Laszlo

> 
> 
>> Thanks,
>>
>> C.
>>
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>   hw/vfio/pci.h     |  3 +++
>>>   hw/core/machine.c |  1 +
>>>   hw/vfio/display.c | 23 +++++++++++++++++++++++
>>>   hw/vfio/pci.c     | 32 ++++++++++++++++++++++++++++++++
>>>   4 files changed, 59 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 47a07d1d9b..f2f8940a85 100644
>>> --- a/hw/core/machine.c
>>> +++ b/hw/core/machine.c
>>> @@ -32,6 +32,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..de5bf71dd1 100644
>>> --- a/hw/vfio/display.c
>>> +++ b/hw/vfio/display.c
>>> @@ -542,3 +542,26 @@ void vfio_display_finalize(VFIOPCIDevice *vdev)
>>>       vfio_display_edid_exit(vdev->dpy);
>>>       g_free(vdev->dpy);
>>>   }
>>> +
>>> +static bool migrate_needed(void *opaque)
>>> +{
>>> +    /*
>>> +     * If we are here, it's because vfio_display_needed(), which is only true
>>> +     * when dpy->ramfb_migrate atm.
>>> +     *
>>> +     * If the migration condition is changed, we should check here if
>>> +     * ramfb_migrate is true. (this will need a way to lookup the associated
>>> +     * VFIOPCIDevice somehow, or fields to be moved, ..)
>>> +     */
>>> +    return true;
>>> +}
>>> +
>>> +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..4689f2e5c1 100644
>>> --- a/hw/vfio/pci.c
>>> +++ b/hw/vfio/pci.c
>>> @@ -2608,6 +2608,25 @@ static bool vfio_msix_present(void *opaque, int version_id)
>>>       return msix_present(pdev);
>>>   }
>>>
>>> +static bool vfio_display_needed(void *opaque)
>>> +{
>>> +    VFIOPCIDevice *vdev = opaque;
>>> +
>>> +    /* the only thing that justifies the VFIODisplay sub-section atm */
>>> +    return vdev->ramfb_migrate != ON_OFF_AUTO_OFF;
>>> +}
>>> +
>>> +const VMStateDescription vmstate_vfio_display = {
>>> +    .name = "VFIOPCIDevice/VFIODisplay",
>>> +    .version_id = 1,
>>> +    .minimum_version_id = 1,
>>> +    .needed = vfio_display_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 +2635,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
>>>       }
>>>   };
>>>
>>> @@ -3275,6 +3298,14 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>>>           if (!vfio_migration_realize(vbasedev, errp)) {
>>>               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 migration");
>>> +                goto out_deregister;
>>> +            }
>>> +        }
>>>       }
>>>
>>>       vfio_register_err_notifier(vdev);
>>> @@ -3484,6 +3515,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(),
>>>   };
>>>
>>
>>
> 
>