[PATCH v2 0/5] ramfb: migration support

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

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

Marc-André Lureau (5):
  hw: remove needless includes
  pc: remove needless includes
  ramfb: implement migration support
  ramfb: make migration conditional
  hw: turn off ramfb migration for machines <= 8.1

 hw/vfio/pci.h                 |  1 +
 include/hw/display/ramfb.h    |  2 +-
 hw/core/machine.c             | 15 ++++---------
 hw/display/ramfb-standalone.c |  8 ++++++-
 hw/display/ramfb.c            | 25 ++++++++++++++++++++-
 hw/i386/pc.c                  | 41 -----------------------------------
 hw/vfio/display.c             |  4 ++--
 hw/vfio/pci.c                 |  1 +
 stubs/ramfb.c                 |  2 +-
 9 files changed, 41 insertions(+), 58 deletions(-)

-- 
2.41.0

[PATCH v2 1/5] hw: 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>
---
 hw/core/machine.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index cb38b8cf4c..68cb556197 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

Re: [PATCH v2 1/5] hw: remove needless includes

[Laszlo Ersek]

On 10/2/23 13:11, marcandre.lureau@redhat.com wrote:
> 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>
> ---
>  hw/core/machine.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index cb38b8cf4c..68cb556197 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"
>  

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

[PATCH v2 2/5] 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>
---
 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

Re: [PATCH v2 2/5] pc: remove needless includes

[Laszlo Ersek]

On 10/2/23 13:11, marcandre.lureau@redhat.com wrote:
> 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>
> ---
>  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
>  
Acked-by: Laszlo Ersek <lersek@redhat.com>

[PATCH v2 3/5] ramfb: implement 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 following patches turns the migration only on machine >= 8.2.

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

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/display/ramfb.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
index 79b9754a58..4aaaa7d653 100644
--- a/hw/display/ramfb.c
+++ b/hw/display/ramfb.c
@@ -12,6 +12,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "migration/vmstate.h"
 #include "qapi/error.h"
 #include "hw/loader.h"
 #include "hw/display/ramfb.h"
@@ -28,6 +29,8 @@ struct QEMU_PACKED RAMFBCfg {
     uint32_t stride;
 };
 
+typedef struct RAMFBCfg RAMFBCfg;
+
 struct RAMFBState {
     DisplaySurface *ds;
     uint32_t width, height;
@@ -115,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;
+}
+
+static const VMStateDescription vmstate_ramfb = {
+    .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();
@@ -127,6 +147,7 @@ RAMFBState *ramfb_setup(Error **errp)
 
     s = g_new0(RAMFBState, 1);
 
+    vmstate_register(NULL, 0, &vmstate_ramfb, s);
     rom_add_vga("vgabios-ramfb.bin");
     fw_cfg_add_file_callback(fw_cfg, "etc/ramfb",
                              NULL, ramfb_fw_cfg_write, s,
-- 
2.41.0

Re: [PATCH v2 3/5] ramfb: implement migration support

[Marc-André Lureau]

Hi

On Mon, Oct 2, 2023 at 3:12 PM <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 following patches turns the migration only on machine >= 8.2.
>
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1859424
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/display/ramfb.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
> index 79b9754a58..4aaaa7d653 100644
> --- a/hw/display/ramfb.c
> +++ b/hw/display/ramfb.c
> @@ -12,6 +12,7 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include "migration/vmstate.h"
>  #include "qapi/error.h"
>  #include "hw/loader.h"
>  #include "hw/display/ramfb.h"
> @@ -28,6 +29,8 @@ struct QEMU_PACKED RAMFBCfg {
>      uint32_t stride;
>  };
>
> +typedef struct RAMFBCfg RAMFBCfg;
> +
>  struct RAMFBState {
>      DisplaySurface *ds;
>      uint32_t width, height;
> @@ -115,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;
> +}
> +
> +static const VMStateDescription vmstate_ramfb = {
> +    .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();
> @@ -127,6 +147,7 @@ RAMFBState *ramfb_setup(Error **errp)
>
>      s = g_new0(RAMFBState, 1);
>
> +    vmstate_register(NULL, 0, &vmstate_ramfb, s);

wip:
I am going to make it attached to the actual device.




-- 
Marc-André Lureau

Re: [PATCH v2 3/5] ramfb: implement migration support

[Laszlo Ersek]

On 10/2/23 14:01, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Oct 2, 2023 at 3:12 PM <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 following patches turns the migration only on machine >= 8.2.
>>
>> Fixes:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1859424
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  hw/display/ramfb.c | 21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>
>> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
>> index 79b9754a58..4aaaa7d653 100644
>> --- a/hw/display/ramfb.c
>> +++ b/hw/display/ramfb.c
>> @@ -12,6 +12,7 @@
>>   */
>>
>>  #include "qemu/osdep.h"
>> +#include "migration/vmstate.h"
>>  #include "qapi/error.h"
>>  #include "hw/loader.h"
>>  #include "hw/display/ramfb.h"
>> @@ -28,6 +29,8 @@ struct QEMU_PACKED RAMFBCfg {
>>      uint32_t stride;
>>  };
>>
>> +typedef struct RAMFBCfg RAMFBCfg;
>> +
>>  struct RAMFBState {
>>      DisplaySurface *ds;
>>      uint32_t width, height;
>> @@ -115,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;
>> +}
>> +
>> +static const VMStateDescription vmstate_ramfb = {
>> +    .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();
>> @@ -127,6 +147,7 @@ RAMFBState *ramfb_setup(Error **errp)
>>
>>      s = g_new0(RAMFBState, 1);
>>
>> +    vmstate_register(NULL, 0, &vmstate_ramfb, s);
> 
> wip:
> I am going to make it attached to the actual device.

I'm really curious about that -- I think it's going to be better, and
it'll teach me stuff about migration!

Laszlo

Re: [PATCH v2 3/5] ramfb: implement migration support

[Laszlo Ersek]

On 10/2/23 13:11, 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 following patches turns the migration only on machine >= 8.2.
> 
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1859424
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/display/ramfb.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
> index 79b9754a58..4aaaa7d653 100644
> --- a/hw/display/ramfb.c
> +++ b/hw/display/ramfb.c
> @@ -12,6 +12,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "migration/vmstate.h"
>  #include "qapi/error.h"
>  #include "hw/loader.h"
>  #include "hw/display/ramfb.h"
> @@ -28,6 +29,8 @@ struct QEMU_PACKED RAMFBCfg {
>      uint32_t stride;
>  };
>  
> +typedef struct RAMFBCfg RAMFBCfg;
> +
>  struct RAMFBState {
>      DisplaySurface *ds;
>      uint32_t width, height;
> @@ -115,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;
> +}
> +
> +static const VMStateDescription vmstate_ramfb = {
> +    .name = "ramfb",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .post_load = ramfb_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_BUFFER_UNSAFE(cfg, RAMFBState, 0, sizeof(RAMFBCfg)),

I just couldn't figure out, from code review, why VMSTATE_BUFFER would
not work here. So I applied your patches, changed this like follows:

diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
index 077fd2fa2c31..04bf01059994 100644
--- a/hw/display/ramfb.c
+++ b/hw/display/ramfb.c
@@ -131,7 +131,7 @@ static const VMStateDescription vmstate_ramfb = {
     .minimum_version_id = 1,
     .post_load = ramfb_post_load,
     .fields = (VMStateField[]) {
-        VMSTATE_BUFFER_UNSAFE(cfg, RAMFBState, 0, sizeof(RAMFBCfg)),
+        VMSTATE_BUFFER(cfg, RAMFBState),
         VMSTATE_END_OF_LIST()
     }
 };

and tried to build it.

I got a wall of error messages about cryptic macro nesting.

I'm quite annoyed that nearly none of the VMSTATE_ macros are
documented; even git-blame tends to be unhelpful. Ultimately though,
there was one useful bit in the wall of error messages: the error was
related to "type_check_array".

Upon reviewing type_check_array, my impression is that VMSTATE_BUFFER is
suitable only for *array fields*. I randomly picked an existent example,
namely

static const VMStateDescription bulk_in_vmstate = {
    .name = "CCID BulkIn state",
    .version_id = 1,
    .minimum_version_id = 1,
    .fields = (VMStateField[]) {
        VMSTATE_BUFFER(data, BulkIn),
        VMSTATE_UINT32(len, BulkIn),
        VMSTATE_UINT32(pos, BulkIn),
        VMSTATE_END_OF_LIST()
    }
};

from "hw/usb/dev-smartcard-reader.c", and sure enough, "data" is an
array field in BulkIn:

typedef struct BulkIn {
    uint8_t  data[BULK_IN_BUF_SIZE];
    uint32_t len;
    uint32_t pos;
} BulkIn;

So that's the reason.

Again, annoying lack of documentation, but I agree that your
VMSTATE_BUFFER_UNSAFE application is judicious.

Thanks!
Laszlo



> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  RAMFBState *ramfb_setup(Error **errp)
>  {
>      FWCfgState *fw_cfg = fw_cfg_find();
> @@ -127,6 +147,7 @@ RAMFBState *ramfb_setup(Error **errp)
>  
>      s = g_new0(RAMFBState, 1);
>  
> +    vmstate_register(NULL, 0, &vmstate_ramfb, s);
>      rom_add_vga("vgabios-ramfb.bin");
>      fw_cfg_add_file_callback(fw_cfg, "etc/ramfb",
>                               NULL, ramfb_fw_cfg_write, s,

[PATCH v2 4/5] ramfb: make migration conditional

[marcandre.lureau]

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

RAMFB migration was unsupported until now, let's make it conditional.
The following patch will prevent machines <= 8.1 to migrate it.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/vfio/pci.h                 | 1 +
 include/hw/display/ramfb.h    | 2 +-
 hw/display/ramfb-standalone.c | 8 +++++++-
 hw/display/ramfb.c            | 6 ++++--
 hw/vfio/display.c             | 4 ++--
 hw/vfio/pci.c                 | 1 +
 stubs/ramfb.c                 | 2 +-
 7 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index 2d836093a8..671cc78912 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -156,6 +156,7 @@ struct VFIOPCIDevice {
     OnOffAuto display;
     uint32_t display_xres;
     uint32_t display_yres;
+    bool ramfb_migrate;
     int32_t bootindex;
     uint32_t igd_gms;
     OffAutoPCIBAR msix_relo;
diff --git a/include/hw/display/ramfb.h b/include/hw/display/ramfb.h
index b33a2c467b..40063b62bd 100644
--- a/include/hw/display/ramfb.h
+++ b/include/hw/display/ramfb.h
@@ -4,7 +4,7 @@
 /* ramfb.c */
 typedef struct RAMFBState RAMFBState;
 void ramfb_display_update(QemuConsole *con, RAMFBState *s);
-RAMFBState *ramfb_setup(Error **errp);
+RAMFBState *ramfb_setup(bool migrate, Error **errp);
 
 /* ramfb-standalone.c */
 #define TYPE_RAMFB_DEVICE "ramfb"
diff --git a/hw/display/ramfb-standalone.c b/hw/display/ramfb-standalone.c
index 8c0094397f..6bbd69ccdf 100644
--- a/hw/display/ramfb-standalone.c
+++ b/hw/display/ramfb-standalone.c
@@ -15,6 +15,7 @@ struct RAMFBStandaloneState {
     SysBusDevice parent_obj;
     QemuConsole *con;
     RAMFBState *state;
+    bool migrate;
 };
 
 static void display_update_wrapper(void *dev)
@@ -37,9 +38,13 @@ static void ramfb_realizefn(DeviceState *dev, Error **errp)
     RAMFBStandaloneState *ramfb = RAMFB(dev);
 
     ramfb->con = graphic_console_init(dev, 0, &wrapper_ops, dev);
-    ramfb->state = ramfb_setup(errp);
+    ramfb->state = ramfb_setup(ramfb->migrate, errp);
 }
 
+static Property ramfb_properties[] = {
+    DEFINE_PROP_BOOL("migrate", RAMFBStandaloneState, migrate,  true),
+    DEFINE_PROP_END_OF_LIST(),
+};
 static void ramfb_class_initfn(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -48,6 +53,7 @@ static void ramfb_class_initfn(ObjectClass *klass, void *data)
     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 = {
diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
index 4aaaa7d653..73e08d605f 100644
--- a/hw/display/ramfb.c
+++ b/hw/display/ramfb.c
@@ -135,7 +135,7 @@ static const VMStateDescription vmstate_ramfb = {
     }
 };
 
-RAMFBState *ramfb_setup(Error **errp)
+RAMFBState *ramfb_setup(bool migrate, Error **errp)
 {
     FWCfgState *fw_cfg = fw_cfg_find();
     RAMFBState *s;
@@ -147,7 +147,9 @@ RAMFBState *ramfb_setup(Error **errp)
 
     s = g_new0(RAMFBState, 1);
 
-    vmstate_register(NULL, 0, &vmstate_ramfb, s);
+    if (migrate) {
+        vmstate_register(NULL, 0, &vmstate_ramfb, s);
+    }
     rom_add_vga("vgabios-ramfb.bin");
     fw_cfg_add_file_callback(fw_cfg, "etc/ramfb",
                              NULL, ramfb_fw_cfg_write, s,
diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index bec864f482..3f6b251ccd 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -356,7 +356,7 @@ static int vfio_display_dmabuf_init(VFIOPCIDevice *vdev, Error **errp)
                                           &vfio_display_dmabuf_ops,
                                           vdev);
     if (vdev->enable_ramfb) {
-        vdev->dpy->ramfb = ramfb_setup(errp);
+        vdev->dpy->ramfb = ramfb_setup(vdev->ramfb_migrate, errp);
     }
     vfio_display_edid_init(vdev);
     return 0;
@@ -483,7 +483,7 @@ static int vfio_display_region_init(VFIOPCIDevice *vdev, Error **errp)
                                           &vfio_display_region_ops,
                                           vdev);
     if (vdev->enable_ramfb) {
-        vdev->dpy->ramfb = ramfb_setup(errp);
+        vdev->dpy->ramfb = ramfb_setup(vdev->ramfb_migrate, errp);
     }
     return 0;
 }
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 3b2ca3c24c..6575b8f32d 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3484,6 +3484,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_BOOL("ramfb-migrate", VFIOPCIDevice, ramfb_migrate,  true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/stubs/ramfb.c b/stubs/ramfb.c
index 48143f3354..8869a5db09 100644
--- a/stubs/ramfb.c
+++ b/stubs/ramfb.c
@@ -6,7 +6,7 @@ void ramfb_display_update(QemuConsole *con, RAMFBState *s)
 {
 }
 
-RAMFBState *ramfb_setup(Error **errp)
+RAMFBState *ramfb_setup(bool migrate, Error **errp)
 {
     error_setg(errp, "ramfb support not available");
     return NULL;
-- 
2.41.0

Re: [PATCH v2 4/5] ramfb: make migration conditional

[Cédric Le Goater]

On 10/2/23 13:11, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> RAMFB migration was unsupported until now, let's make it conditional.
> The following patch will prevent machines <= 8.1 to migrate it.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Maybe localize the new 'ramfb_migrate' attribute close to 'enable_ramfb'
in VFIOPCIDevice. Anyhow,

Acked-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/vfio/pci.h                 | 1 +
>   include/hw/display/ramfb.h    | 2 +-
>   hw/display/ramfb-standalone.c | 8 +++++++-
>   hw/display/ramfb.c            | 6 ++++--
>   hw/vfio/display.c             | 4 ++--
>   hw/vfio/pci.c                 | 1 +
>   stubs/ramfb.c                 | 2 +-
>   7 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index 2d836093a8..671cc78912 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -156,6 +156,7 @@ struct VFIOPCIDevice {
>       OnOffAuto display;
>       uint32_t display_xres;
>       uint32_t display_yres;
> +    bool ramfb_migrate;
>       int32_t bootindex;
>       uint32_t igd_gms;
>       OffAutoPCIBAR msix_relo;
> diff --git a/include/hw/display/ramfb.h b/include/hw/display/ramfb.h
> index b33a2c467b..40063b62bd 100644
> --- a/include/hw/display/ramfb.h
> +++ b/include/hw/display/ramfb.h
> @@ -4,7 +4,7 @@
>   /* ramfb.c */
>   typedef struct RAMFBState RAMFBState;
>   void ramfb_display_update(QemuConsole *con, RAMFBState *s);
> -RAMFBState *ramfb_setup(Error **errp);
> +RAMFBState *ramfb_setup(bool migrate, Error **errp);
>   
>   /* ramfb-standalone.c */
>   #define TYPE_RAMFB_DEVICE "ramfb"
> diff --git a/hw/display/ramfb-standalone.c b/hw/display/ramfb-standalone.c
> index 8c0094397f..6bbd69ccdf 100644
> --- a/hw/display/ramfb-standalone.c
> +++ b/hw/display/ramfb-standalone.c
> @@ -15,6 +15,7 @@ struct RAMFBStandaloneState {
>       SysBusDevice parent_obj;
>       QemuConsole *con;
>       RAMFBState *state;
> +    bool migrate;
>   };
>   
>   static void display_update_wrapper(void *dev)
> @@ -37,9 +38,13 @@ static void ramfb_realizefn(DeviceState *dev, Error **errp)
>       RAMFBStandaloneState *ramfb = RAMFB(dev);
>   
>       ramfb->con = graphic_console_init(dev, 0, &wrapper_ops, dev);
> -    ramfb->state = ramfb_setup(errp);
> +    ramfb->state = ramfb_setup(ramfb->migrate, errp);
>   }
>   
> +static Property ramfb_properties[] = {
> +    DEFINE_PROP_BOOL("migrate", RAMFBStandaloneState, migrate,  true),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
>   static void ramfb_class_initfn(ObjectClass *klass, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -48,6 +53,7 @@ static void ramfb_class_initfn(ObjectClass *klass, void *data)
>       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 = {
> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
> index 4aaaa7d653..73e08d605f 100644
> --- a/hw/display/ramfb.c
> +++ b/hw/display/ramfb.c
> @@ -135,7 +135,7 @@ static const VMStateDescription vmstate_ramfb = {
>       }
>   };
>   
> -RAMFBState *ramfb_setup(Error **errp)
> +RAMFBState *ramfb_setup(bool migrate, Error **errp)
>   {
>       FWCfgState *fw_cfg = fw_cfg_find();
>       RAMFBState *s;
> @@ -147,7 +147,9 @@ RAMFBState *ramfb_setup(Error **errp)
>   
>       s = g_new0(RAMFBState, 1);
>   
> -    vmstate_register(NULL, 0, &vmstate_ramfb, s);
> +    if (migrate) {
> +        vmstate_register(NULL, 0, &vmstate_ramfb, s);
> +    }
>       rom_add_vga("vgabios-ramfb.bin");
>       fw_cfg_add_file_callback(fw_cfg, "etc/ramfb",
>                                NULL, ramfb_fw_cfg_write, s,
> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> index bec864f482..3f6b251ccd 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -356,7 +356,7 @@ static int vfio_display_dmabuf_init(VFIOPCIDevice *vdev, Error **errp)
>                                             &vfio_display_dmabuf_ops,
>                                             vdev);
>       if (vdev->enable_ramfb) {
> -        vdev->dpy->ramfb = ramfb_setup(errp);
> +        vdev->dpy->ramfb = ramfb_setup(vdev->ramfb_migrate, errp);
>       }
>       vfio_display_edid_init(vdev);
>       return 0;
> @@ -483,7 +483,7 @@ static int vfio_display_region_init(VFIOPCIDevice *vdev, Error **errp)
>                                             &vfio_display_region_ops,
>                                             vdev);
>       if (vdev->enable_ramfb) {
> -        vdev->dpy->ramfb = ramfb_setup(errp);
> +        vdev->dpy->ramfb = ramfb_setup(vdev->ramfb_migrate, errp);
>       }
>       return 0;
>   }
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 3b2ca3c24c..6575b8f32d 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3484,6 +3484,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_BOOL("ramfb-migrate", VFIOPCIDevice, ramfb_migrate,  true),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   
> diff --git a/stubs/ramfb.c b/stubs/ramfb.c
> index 48143f3354..8869a5db09 100644
> --- a/stubs/ramfb.c
> +++ b/stubs/ramfb.c
> @@ -6,7 +6,7 @@ void ramfb_display_update(QemuConsole *con, RAMFBState *s)
>   {
>   }
>   
> -RAMFBState *ramfb_setup(Error **errp)
> +RAMFBState *ramfb_setup(bool migrate, Error **errp)
>   {
>       error_setg(errp, "ramfb support not available");
>       return NULL;

Re: [PATCH v2 4/5] ramfb: make migration conditional

[Alex Williamson]

On Mon, 2 Oct 2023 15:38:10 +0200
Cédric Le Goater <clg@redhat.com> wrote:

> On 10/2/23 13:11, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > RAMFB migration was unsupported until now, let's make it conditional.
> > The following patch will prevent machines <= 8.1 to migrate it.
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>  
> Maybe localize the new 'ramfb_migrate' attribute close to 'enable_ramfb'
> in VFIOPCIDevice. Anyhow,

Shouldn't this actually be tied to whether the device is migratable
(which for GVT-g - the only ramfb user afaik - it's not)?  What does it
mean to have a ramfb-migrate=true property on a device that doesn't
support migration, or false on a device that does support migration.  I
don't understand why this is a user controllable property.  Thanks,

Alex

> > ---
> >   hw/vfio/pci.h                 | 1 +
> >   include/hw/display/ramfb.h    | 2 +-
> >   hw/display/ramfb-standalone.c | 8 +++++++-
> >   hw/display/ramfb.c            | 6 ++++--
> >   hw/vfio/display.c             | 4 ++--
> >   hw/vfio/pci.c                 | 1 +
> >   stubs/ramfb.c                 | 2 +-
> >   7 files changed, 17 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> > index 2d836093a8..671cc78912 100644
> > --- a/hw/vfio/pci.h
> > +++ b/hw/vfio/pci.h
> > @@ -156,6 +156,7 @@ struct VFIOPCIDevice {
> >       OnOffAuto display;
> >       uint32_t display_xres;
> >       uint32_t display_yres;
> > +    bool ramfb_migrate;
> >       int32_t bootindex;
> >       uint32_t igd_gms;
> >       OffAutoPCIBAR msix_relo;
> > diff --git a/include/hw/display/ramfb.h b/include/hw/display/ramfb.h
> > index b33a2c467b..40063b62bd 100644
> > --- a/include/hw/display/ramfb.h
> > +++ b/include/hw/display/ramfb.h
> > @@ -4,7 +4,7 @@
> >   /* ramfb.c */
> >   typedef struct RAMFBState RAMFBState;
> >   void ramfb_display_update(QemuConsole *con, RAMFBState *s);
> > -RAMFBState *ramfb_setup(Error **errp);
> > +RAMFBState *ramfb_setup(bool migrate, Error **errp);
> >   
> >   /* ramfb-standalone.c */
> >   #define TYPE_RAMFB_DEVICE "ramfb"
> > diff --git a/hw/display/ramfb-standalone.c b/hw/display/ramfb-standalone.c
> > index 8c0094397f..6bbd69ccdf 100644
> > --- a/hw/display/ramfb-standalone.c
> > +++ b/hw/display/ramfb-standalone.c
> > @@ -15,6 +15,7 @@ struct RAMFBStandaloneState {
> >       SysBusDevice parent_obj;
> >       QemuConsole *con;
> >       RAMFBState *state;
> > +    bool migrate;
> >   };
> >   
> >   static void display_update_wrapper(void *dev)
> > @@ -37,9 +38,13 @@ static void ramfb_realizefn(DeviceState *dev, Error **errp)
> >       RAMFBStandaloneState *ramfb = RAMFB(dev);
> >   
> >       ramfb->con = graphic_console_init(dev, 0, &wrapper_ops, dev);
> > -    ramfb->state = ramfb_setup(errp);
> > +    ramfb->state = ramfb_setup(ramfb->migrate, errp);
> >   }
> >   
> > +static Property ramfb_properties[] = {
> > +    DEFINE_PROP_BOOL("migrate", RAMFBStandaloneState, migrate,  true),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> >   static void ramfb_class_initfn(ObjectClass *klass, void *data)
> >   {
> >       DeviceClass *dc = DEVICE_CLASS(klass);
> > @@ -48,6 +53,7 @@ static void ramfb_class_initfn(ObjectClass *klass, void *data)
> >       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 = {
> > diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
> > index 4aaaa7d653..73e08d605f 100644
> > --- a/hw/display/ramfb.c
> > +++ b/hw/display/ramfb.c
> > @@ -135,7 +135,7 @@ static const VMStateDescription vmstate_ramfb = {
> >       }
> >   };
> >   
> > -RAMFBState *ramfb_setup(Error **errp)
> > +RAMFBState *ramfb_setup(bool migrate, Error **errp)
> >   {
> >       FWCfgState *fw_cfg = fw_cfg_find();
> >       RAMFBState *s;
> > @@ -147,7 +147,9 @@ RAMFBState *ramfb_setup(Error **errp)
> >   
> >       s = g_new0(RAMFBState, 1);
> >   
> > -    vmstate_register(NULL, 0, &vmstate_ramfb, s);
> > +    if (migrate) {
> > +        vmstate_register(NULL, 0, &vmstate_ramfb, s);
> > +    }
> >       rom_add_vga("vgabios-ramfb.bin");
> >       fw_cfg_add_file_callback(fw_cfg, "etc/ramfb",
> >                                NULL, ramfb_fw_cfg_write, s,
> > diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> > index bec864f482..3f6b251ccd 100644
> > --- a/hw/vfio/display.c
> > +++ b/hw/vfio/display.c
> > @@ -356,7 +356,7 @@ static int vfio_display_dmabuf_init(VFIOPCIDevice *vdev, Error **errp)
> >                                             &vfio_display_dmabuf_ops,
> >                                             vdev);
> >       if (vdev->enable_ramfb) {
> > -        vdev->dpy->ramfb = ramfb_setup(errp);
> > +        vdev->dpy->ramfb = ramfb_setup(vdev->ramfb_migrate, errp);
> >       }
> >       vfio_display_edid_init(vdev);
> >       return 0;
> > @@ -483,7 +483,7 @@ static int vfio_display_region_init(VFIOPCIDevice *vdev, Error **errp)
> >                                             &vfio_display_region_ops,
> >                                             vdev);
> >       if (vdev->enable_ramfb) {
> > -        vdev->dpy->ramfb = ramfb_setup(errp);
> > +        vdev->dpy->ramfb = ramfb_setup(vdev->ramfb_migrate, errp);
> >       }
> >       return 0;
> >   }
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index 3b2ca3c24c..6575b8f32d 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -3484,6 +3484,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_BOOL("ramfb-migrate", VFIOPCIDevice, ramfb_migrate,  true),
> >       DEFINE_PROP_END_OF_LIST(),
> >   };
> >   
> > diff --git a/stubs/ramfb.c b/stubs/ramfb.c
> > index 48143f3354..8869a5db09 100644
> > --- a/stubs/ramfb.c
> > +++ b/stubs/ramfb.c
> > @@ -6,7 +6,7 @@ void ramfb_display_update(QemuConsole *con, RAMFBState *s)
> >   {
> >   }
> >   
> > -RAMFBState *ramfb_setup(Error **errp)
> > +RAMFBState *ramfb_setup(bool migrate, Error **errp)
> >   {
> >       error_setg(errp, "ramfb support not available");
> >       return NULL;  
> 

Re: [PATCH v2 4/5] ramfb: make migration conditional

[Laszlo Ersek]

On 10/2/23 16:41, Alex Williamson wrote:
> On Mon, 2 Oct 2023 15:38:10 +0200
> Cédric Le Goater <clg@redhat.com> wrote:
> 
>> On 10/2/23 13:11, marcandre.lureau@redhat.com wrote:
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> RAMFB migration was unsupported until now, let's make it conditional.
>>> The following patch will prevent machines <= 8.1 to migrate it.
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>  
>> Maybe localize the new 'ramfb_migrate' attribute close to 'enable_ramfb'
>> in VFIOPCIDevice. Anyhow,
> 
> Shouldn't this actually be tied to whether the device is migratable
> (which for GVT-g - the only ramfb user afaik - it's not)?  What does it
> mean to have a ramfb-migrate=true property on a device that doesn't
> support migration, or false on a device that does support migration.  I
> don't understand why this is a user controllable property.  Thanks,

The comments in <https://bugzilla.redhat.com/show_bug.cgi?id=1859424> (which are unfortunately not public :/ ) suggest that ramfb migration was simply forgotten when vGPU migration was implemented. So, "now that vGPU migration is done", this should be added.

Comment 8 suggests that the following domain XML snippet

    <hostdev mode='subsystem' type='mdev' managed='no' model='vfio-pci' display='on' ramfb='on'>
      <source>
        <address uuid='b155147a-663a-4009-ae7f-e9a96805b3ce'/>
      </source>
      <alias name='ua-b155147a-663a-4009-ae7f-e9a96805b3ce'/>
      <address type='pci' domain='0x0000' bus='0x07' slot='0x00' function='0x0'/>
    </hostdev>

is migratable, but the ramfb device malfunctions on the destination host.

There's also a huge QEMU cmdline in comment#0 of the bug; I've not tried to read that.

AIUI BTW the property is not for the user to control, it's just a compat knob for versioned machine types. AIUI those are usually implemented with such (user-visible / -tweakable) device properties.

Laszlo

> 
> Alex
> 
>>> ---
>>>   hw/vfio/pci.h                 | 1 +
>>>   include/hw/display/ramfb.h    | 2 +-
>>>   hw/display/ramfb-standalone.c | 8 +++++++-
>>>   hw/display/ramfb.c            | 6 ++++--
>>>   hw/vfio/display.c             | 4 ++--
>>>   hw/vfio/pci.c                 | 1 +
>>>   stubs/ramfb.c                 | 2 +-
>>>   7 files changed, 17 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
>>> index 2d836093a8..671cc78912 100644
>>> --- a/hw/vfio/pci.h
>>> +++ b/hw/vfio/pci.h
>>> @@ -156,6 +156,7 @@ struct VFIOPCIDevice {
>>>       OnOffAuto display;
>>>       uint32_t display_xres;
>>>       uint32_t display_yres;
>>> +    bool ramfb_migrate;
>>>       int32_t bootindex;
>>>       uint32_t igd_gms;
>>>       OffAutoPCIBAR msix_relo;
>>> diff --git a/include/hw/display/ramfb.h b/include/hw/display/ramfb.h
>>> index b33a2c467b..40063b62bd 100644
>>> --- a/include/hw/display/ramfb.h
>>> +++ b/include/hw/display/ramfb.h
>>> @@ -4,7 +4,7 @@
>>>   /* ramfb.c */
>>>   typedef struct RAMFBState RAMFBState;
>>>   void ramfb_display_update(QemuConsole *con, RAMFBState *s);
>>> -RAMFBState *ramfb_setup(Error **errp);
>>> +RAMFBState *ramfb_setup(bool migrate, Error **errp);
>>>   
>>>   /* ramfb-standalone.c */
>>>   #define TYPE_RAMFB_DEVICE "ramfb"
>>> diff --git a/hw/display/ramfb-standalone.c b/hw/display/ramfb-standalone.c
>>> index 8c0094397f..6bbd69ccdf 100644
>>> --- a/hw/display/ramfb-standalone.c
>>> +++ b/hw/display/ramfb-standalone.c
>>> @@ -15,6 +15,7 @@ struct RAMFBStandaloneState {
>>>       SysBusDevice parent_obj;
>>>       QemuConsole *con;
>>>       RAMFBState *state;
>>> +    bool migrate;
>>>   };
>>>   
>>>   static void display_update_wrapper(void *dev)
>>> @@ -37,9 +38,13 @@ static void ramfb_realizefn(DeviceState *dev, Error **errp)
>>>       RAMFBStandaloneState *ramfb = RAMFB(dev);
>>>   
>>>       ramfb->con = graphic_console_init(dev, 0, &wrapper_ops, dev);
>>> -    ramfb->state = ramfb_setup(errp);
>>> +    ramfb->state = ramfb_setup(ramfb->migrate, errp);
>>>   }
>>>   
>>> +static Property ramfb_properties[] = {
>>> +    DEFINE_PROP_BOOL("migrate", RAMFBStandaloneState, migrate,  true),
>>> +    DEFINE_PROP_END_OF_LIST(),
>>> +};
>>>   static void ramfb_class_initfn(ObjectClass *klass, void *data)
>>>   {
>>>       DeviceClass *dc = DEVICE_CLASS(klass);
>>> @@ -48,6 +53,7 @@ static void ramfb_class_initfn(ObjectClass *klass, void *data)
>>>       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 = {
>>> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
>>> index 4aaaa7d653..73e08d605f 100644
>>> --- a/hw/display/ramfb.c
>>> +++ b/hw/display/ramfb.c
>>> @@ -135,7 +135,7 @@ static const VMStateDescription vmstate_ramfb = {
>>>       }
>>>   };
>>>   
>>> -RAMFBState *ramfb_setup(Error **errp)
>>> +RAMFBState *ramfb_setup(bool migrate, Error **errp)
>>>   {
>>>       FWCfgState *fw_cfg = fw_cfg_find();
>>>       RAMFBState *s;
>>> @@ -147,7 +147,9 @@ RAMFBState *ramfb_setup(Error **errp)
>>>   
>>>       s = g_new0(RAMFBState, 1);
>>>   
>>> -    vmstate_register(NULL, 0, &vmstate_ramfb, s);
>>> +    if (migrate) {
>>> +        vmstate_register(NULL, 0, &vmstate_ramfb, s);
>>> +    }
>>>       rom_add_vga("vgabios-ramfb.bin");
>>>       fw_cfg_add_file_callback(fw_cfg, "etc/ramfb",
>>>                                NULL, ramfb_fw_cfg_write, s,
>>> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
>>> index bec864f482..3f6b251ccd 100644
>>> --- a/hw/vfio/display.c
>>> +++ b/hw/vfio/display.c
>>> @@ -356,7 +356,7 @@ static int vfio_display_dmabuf_init(VFIOPCIDevice *vdev, Error **errp)
>>>                                             &vfio_display_dmabuf_ops,
>>>                                             vdev);
>>>       if (vdev->enable_ramfb) {
>>> -        vdev->dpy->ramfb = ramfb_setup(errp);
>>> +        vdev->dpy->ramfb = ramfb_setup(vdev->ramfb_migrate, errp);
>>>       }
>>>       vfio_display_edid_init(vdev);
>>>       return 0;
>>> @@ -483,7 +483,7 @@ static int vfio_display_region_init(VFIOPCIDevice *vdev, Error **errp)
>>>                                             &vfio_display_region_ops,
>>>                                             vdev);
>>>       if (vdev->enable_ramfb) {
>>> -        vdev->dpy->ramfb = ramfb_setup(errp);
>>> +        vdev->dpy->ramfb = ramfb_setup(vdev->ramfb_migrate, errp);
>>>       }
>>>       return 0;
>>>   }
>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>> index 3b2ca3c24c..6575b8f32d 100644
>>> --- a/hw/vfio/pci.c
>>> +++ b/hw/vfio/pci.c
>>> @@ -3484,6 +3484,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_BOOL("ramfb-migrate", VFIOPCIDevice, ramfb_migrate,  true),
>>>       DEFINE_PROP_END_OF_LIST(),
>>>   };
>>>   
>>> diff --git a/stubs/ramfb.c b/stubs/ramfb.c
>>> index 48143f3354..8869a5db09 100644
>>> --- a/stubs/ramfb.c
>>> +++ b/stubs/ramfb.c
>>> @@ -6,7 +6,7 @@ void ramfb_display_update(QemuConsole *con, RAMFBState *s)
>>>   {
>>>   }
>>>   
>>> -RAMFBState *ramfb_setup(Error **errp)
>>> +RAMFBState *ramfb_setup(bool migrate, Error **errp)
>>>   {
>>>       error_setg(errp, "ramfb support not available");
>>>       return NULL;  
>>
> 

Re: [PATCH v2 4/5] ramfb: make migration conditional

[Alex Williamson]

On Mon, 2 Oct 2023 20:24:11 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 10/2/23 16:41, Alex Williamson wrote:
> > On Mon, 2 Oct 2023 15:38:10 +0200
> > Cédric Le Goater <clg@redhat.com> wrote:
> >   
> >> On 10/2/23 13:11, marcandre.lureau@redhat.com wrote:  
> >>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>>
> >>> RAMFB migration was unsupported until now, let's make it conditional.
> >>> The following patch will prevent machines <= 8.1 to migrate it.
> >>>
> >>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>    
> >> Maybe localize the new 'ramfb_migrate' attribute close to 'enable_ramfb'
> >> in VFIOPCIDevice. Anyhow,  
> > 
> > Shouldn't this actually be tied to whether the device is migratable
> > (which for GVT-g - the only ramfb user afaik - it's not)?  What does it
> > mean to have a ramfb-migrate=true property on a device that doesn't
> > support migration, or false on a device that does support migration.  I
> > don't understand why this is a user controllable property.  Thanks,  
> 
> The comments in <https://bugzilla.redhat.com/show_bug.cgi?id=1859424>
> (which are unfortunately not public :/ ) suggest that ramfb migration
> was simply forgotten when vGPU migration was implemented. So, "now
> that vGPU migration is done", this should be added.
> 
> Comment 8 suggests that the following domain XML snippet
> 
>     <hostdev mode='subsystem' type='mdev' managed='no'
> model='vfio-pci' display='on' ramfb='on'> <source>
>         <address uuid='b155147a-663a-4009-ae7f-e9a96805b3ce'/>
>       </source>
>       <alias name='ua-b155147a-663a-4009-ae7f-e9a96805b3ce'/>
>       <address type='pci' domain='0x0000' bus='0x07' slot='0x00'
> function='0x0'/> </hostdev>
> 
> is migratable, but the ramfb device malfunctions on the destination
> host.
> 
> There's also a huge QEMU cmdline in comment#0 of the bug; I've not
> tried to read that.
> 
> AIUI BTW the property is not for the user to control, it's just a
> compat knob for versioned machine types. AIUI those are usually
> implemented with such (user-visible / -tweakable) device properties.

If it's not for user control it's unfortunate that we expose it to the
user at all, but should it at least use the "x-" prefix to indicate that
it's not intended to be an API?  It's still odd to think that we can
have scenarios of a non-migratable vfio device registering a migratable
ramfb, and vice versa, but I suppose in the end it doesn't matter.
Thanks,

Alex

> >>> ---
> >>>   hw/vfio/pci.h                 | 1 +
> >>>   include/hw/display/ramfb.h    | 2 +-
> >>>   hw/display/ramfb-standalone.c | 8 +++++++-
> >>>   hw/display/ramfb.c            | 6 ++++--
> >>>   hw/vfio/display.c             | 4 ++--
> >>>   hw/vfio/pci.c                 | 1 +
> >>>   stubs/ramfb.c                 | 2 +-
> >>>   7 files changed, 17 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> >>> index 2d836093a8..671cc78912 100644
> >>> --- a/hw/vfio/pci.h
> >>> +++ b/hw/vfio/pci.h
> >>> @@ -156,6 +156,7 @@ struct VFIOPCIDevice {
> >>>       OnOffAuto display;
> >>>       uint32_t display_xres;
> >>>       uint32_t display_yres;
> >>> +    bool ramfb_migrate;
> >>>       int32_t bootindex;
> >>>       uint32_t igd_gms;
> >>>       OffAutoPCIBAR msix_relo;
> >>> diff --git a/include/hw/display/ramfb.h
> >>> b/include/hw/display/ramfb.h index b33a2c467b..40063b62bd 100644
> >>> --- a/include/hw/display/ramfb.h
> >>> +++ b/include/hw/display/ramfb.h
> >>> @@ -4,7 +4,7 @@
> >>>   /* ramfb.c */
> >>>   typedef struct RAMFBState RAMFBState;
> >>>   void ramfb_display_update(QemuConsole *con, RAMFBState *s);
> >>> -RAMFBState *ramfb_setup(Error **errp);
> >>> +RAMFBState *ramfb_setup(bool migrate, Error **errp);
> >>>   
> >>>   /* ramfb-standalone.c */
> >>>   #define TYPE_RAMFB_DEVICE "ramfb"
> >>> diff --git a/hw/display/ramfb-standalone.c
> >>> b/hw/display/ramfb-standalone.c index 8c0094397f..6bbd69ccdf
> >>> 100644 --- a/hw/display/ramfb-standalone.c
> >>> +++ b/hw/display/ramfb-standalone.c
> >>> @@ -15,6 +15,7 @@ struct RAMFBStandaloneState {
> >>>       SysBusDevice parent_obj;
> >>>       QemuConsole *con;
> >>>       RAMFBState *state;
> >>> +    bool migrate;
> >>>   };
> >>>   
> >>>   static void display_update_wrapper(void *dev)
> >>> @@ -37,9 +38,13 @@ static void ramfb_realizefn(DeviceState *dev,
> >>> Error **errp) RAMFBStandaloneState *ramfb = RAMFB(dev);
> >>>   
> >>>       ramfb->con = graphic_console_init(dev, 0, &wrapper_ops,
> >>> dev);
> >>> -    ramfb->state = ramfb_setup(errp);
> >>> +    ramfb->state = ramfb_setup(ramfb->migrate, errp);
> >>>   }
> >>>   
> >>> +static Property ramfb_properties[] = {
> >>> +    DEFINE_PROP_BOOL("migrate", RAMFBStandaloneState, migrate,
> >>> true),
> >>> +    DEFINE_PROP_END_OF_LIST(),
> >>> +};
> >>>   static void ramfb_class_initfn(ObjectClass *klass, void *data)
> >>>   {
> >>>       DeviceClass *dc = DEVICE_CLASS(klass);
> >>> @@ -48,6 +53,7 @@ static void ramfb_class_initfn(ObjectClass
> >>> *klass, void *data) 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 = {
> >>> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
> >>> index 4aaaa7d653..73e08d605f 100644
> >>> --- a/hw/display/ramfb.c
> >>> +++ b/hw/display/ramfb.c
> >>> @@ -135,7 +135,7 @@ static const VMStateDescription vmstate_ramfb
> >>> = { }
> >>>   };
> >>>   
> >>> -RAMFBState *ramfb_setup(Error **errp)
> >>> +RAMFBState *ramfb_setup(bool migrate, Error **errp)
> >>>   {
> >>>       FWCfgState *fw_cfg = fw_cfg_find();
> >>>       RAMFBState *s;
> >>> @@ -147,7 +147,9 @@ RAMFBState *ramfb_setup(Error **errp)
> >>>   
> >>>       s = g_new0(RAMFBState, 1);
> >>>   
> >>> -    vmstate_register(NULL, 0, &vmstate_ramfb, s);
> >>> +    if (migrate) {
> >>> +        vmstate_register(NULL, 0, &vmstate_ramfb, s);
> >>> +    }
> >>>       rom_add_vga("vgabios-ramfb.bin");
> >>>       fw_cfg_add_file_callback(fw_cfg, "etc/ramfb",
> >>>                                NULL, ramfb_fw_cfg_write, s,
> >>> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> >>> index bec864f482..3f6b251ccd 100644
> >>> --- a/hw/vfio/display.c
> >>> +++ b/hw/vfio/display.c
> >>> @@ -356,7 +356,7 @@ static int
> >>> vfio_display_dmabuf_init(VFIOPCIDevice *vdev, Error **errp)
> >>> &vfio_display_dmabuf_ops, vdev);
> >>>       if (vdev->enable_ramfb) {
> >>> -        vdev->dpy->ramfb = ramfb_setup(errp);
> >>> +        vdev->dpy->ramfb = ramfb_setup(vdev->ramfb_migrate,
> >>> errp); }
> >>>       vfio_display_edid_init(vdev);
> >>>       return 0;
> >>> @@ -483,7 +483,7 @@ static int
> >>> vfio_display_region_init(VFIOPCIDevice *vdev, Error **errp)
> >>> &vfio_display_region_ops, vdev);
> >>>       if (vdev->enable_ramfb) {
> >>> -        vdev->dpy->ramfb = ramfb_setup(errp);
> >>> +        vdev->dpy->ramfb = ramfb_setup(vdev->ramfb_migrate,
> >>> errp); }
> >>>       return 0;
> >>>   }
> >>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> >>> index 3b2ca3c24c..6575b8f32d 100644
> >>> --- a/hw/vfio/pci.c
> >>> +++ b/hw/vfio/pci.c
> >>> @@ -3484,6 +3484,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_BOOL("ramfb-migrate", VFIOPCIDevice,
> >>> ramfb_migrate,  true), DEFINE_PROP_END_OF_LIST(),
> >>>   };
> >>>   
> >>> diff --git a/stubs/ramfb.c b/stubs/ramfb.c
> >>> index 48143f3354..8869a5db09 100644
> >>> --- a/stubs/ramfb.c
> >>> +++ b/stubs/ramfb.c
> >>> @@ -6,7 +6,7 @@ void ramfb_display_update(QemuConsole *con,
> >>> RAMFBState *s) {
> >>>   }
> >>>   
> >>> -RAMFBState *ramfb_setup(Error **errp)
> >>> +RAMFBState *ramfb_setup(bool migrate, Error **errp)
> >>>   {
> >>>       error_setg(errp, "ramfb support not available");
> >>>       return NULL;    
> >>  
> >   
> 

Re: [PATCH v2 4/5] ramfb: make migration conditional

[Laszlo Ersek]

On 10/2/23 21:26, Alex Williamson wrote:
> On Mon, 2 Oct 2023 20:24:11 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 10/2/23 16:41, Alex Williamson wrote:
>>> On Mon, 2 Oct 2023 15:38:10 +0200
>>> Cédric Le Goater <clg@redhat.com> wrote:
>>>   
>>>> On 10/2/23 13:11, marcandre.lureau@redhat.com wrote:  
>>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>
>>>>> RAMFB migration was unsupported until now, let's make it conditional.
>>>>> The following patch will prevent machines <= 8.1 to migrate it.
>>>>>
>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>    
>>>> Maybe localize the new 'ramfb_migrate' attribute close to 'enable_ramfb'
>>>> in VFIOPCIDevice. Anyhow,  
>>>
>>> Shouldn't this actually be tied to whether the device is migratable
>>> (which for GVT-g - the only ramfb user afaik - it's not)?  What does it
>>> mean to have a ramfb-migrate=true property on a device that doesn't
>>> support migration, or false on a device that does support migration.  I
>>> don't understand why this is a user controllable property.  Thanks,  
>>
>> The comments in <https://bugzilla.redhat.com/show_bug.cgi?id=1859424>
>> (which are unfortunately not public :/ ) suggest that ramfb migration
>> was simply forgotten when vGPU migration was implemented. So, "now
>> that vGPU migration is done", this should be added.
>>
>> Comment 8 suggests that the following domain XML snippet
>>
>>     <hostdev mode='subsystem' type='mdev' managed='no'
>> model='vfio-pci' display='on' ramfb='on'> <source>
>>         <address uuid='b155147a-663a-4009-ae7f-e9a96805b3ce'/>
>>       </source>
>>       <alias name='ua-b155147a-663a-4009-ae7f-e9a96805b3ce'/>
>>       <address type='pci' domain='0x0000' bus='0x07' slot='0x00'
>> function='0x0'/> </hostdev>
>>
>> is migratable, but the ramfb device malfunctions on the destination
>> host.
>>
>> There's also a huge QEMU cmdline in comment#0 of the bug; I've not
>> tried to read that.
>>
>> AIUI BTW the property is not for the user to control, it's just a
>> compat knob for versioned machine types. AIUI those are usually
>> implemented with such (user-visible / -tweakable) device properties.
> 
> If it's not for user control it's unfortunate that we expose it to the
> user at all, but should it at least use the "x-" prefix to indicate that
> it's not intended to be an API?

I *think* it was your commit db32d0f43839 ("vfio/pci: Add option to
disable GeForce quirks", 2018-02-06) that hda introduced me to the "x-"
prefixed properties!

For some reason though, machine type compat knobs are never named like
that, AFAIR.

> It's still odd to think that we can
> have scenarios of a non-migratable vfio device registering a migratable
> ramfb, and vice versa, but I suppose in the end it doesn't matter.

I do think it matters! For one, if migration is not possible with
vfio-pci-nohotplug, then how can QE (or anyone else) *test* the patch
(i.e. that it makes a difference)? In that case, the ramfb_setup() call
from vfio-pci-nohotplug should just open-code "false" for the
"migratable" parameter.

But, more importantly, I think either we're missing something about RHBZ
1859424, or that use case is just plain wrong. Gerd, any comments perhaps?

Migration certainly makes sense for ramfb-standalone though.

Laszlo

Re: [PATCH v2 4/5] ramfb: make migration conditional

[Alex Williamson]

On Mon, 2 Oct 2023 21:41:55 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 10/2/23 21:26, Alex Williamson wrote:
> > On Mon, 2 Oct 2023 20:24:11 +0200
> > Laszlo Ersek <lersek@redhat.com> wrote:
> >   
> >> On 10/2/23 16:41, Alex Williamson wrote:  
> >>> On Mon, 2 Oct 2023 15:38:10 +0200
> >>> Cédric Le Goater <clg@redhat.com> wrote:
> >>>     
> >>>> On 10/2/23 13:11, marcandre.lureau@redhat.com wrote:    
> >>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>>>>
> >>>>> RAMFB migration was unsupported until now, let's make it conditional.
> >>>>> The following patch will prevent machines <= 8.1 to migrate it.
> >>>>>
> >>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>      
> >>>> Maybe localize the new 'ramfb_migrate' attribute close to 'enable_ramfb'
> >>>> in VFIOPCIDevice. Anyhow,    
> >>>
> >>> Shouldn't this actually be tied to whether the device is migratable
> >>> (which for GVT-g - the only ramfb user afaik - it's not)?  What does it
> >>> mean to have a ramfb-migrate=true property on a device that doesn't
> >>> support migration, or false on a device that does support migration.  I
> >>> don't understand why this is a user controllable property.  Thanks,    
> >>
> >> The comments in <https://bugzilla.redhat.com/show_bug.cgi?id=1859424>
> >> (which are unfortunately not public :/ ) suggest that ramfb migration
> >> was simply forgotten when vGPU migration was implemented. So, "now
> >> that vGPU migration is done", this should be added.
> >>
> >> Comment 8 suggests that the following domain XML snippet
> >>
> >>     <hostdev mode='subsystem' type='mdev' managed='no'
> >> model='vfio-pci' display='on' ramfb='on'> <source>
> >>         <address uuid='b155147a-663a-4009-ae7f-e9a96805b3ce'/>
> >>       </source>
> >>       <alias name='ua-b155147a-663a-4009-ae7f-e9a96805b3ce'/>
> >>       <address type='pci' domain='0x0000' bus='0x07' slot='0x00'  
> >> function='0x0'/> </hostdev>  
> >>
> >> is migratable, but the ramfb device malfunctions on the destination
> >> host.
> >>
> >> There's also a huge QEMU cmdline in comment#0 of the bug; I've not
> >> tried to read that.
> >>
> >> AIUI BTW the property is not for the user to control, it's just a
> >> compat knob for versioned machine types. AIUI those are usually
> >> implemented with such (user-visible / -tweakable) device properties.  
> > 
> > If it's not for user control it's unfortunate that we expose it to the
> > user at all, but should it at least use the "x-" prefix to indicate that
> > it's not intended to be an API?  
> 
> I *think* it was your commit db32d0f43839 ("vfio/pci: Add option to
> disable GeForce quirks", 2018-02-06) that hda introduced me to the "x-"
> prefixed properties!
> 
> For some reason though, machine type compat knobs are never named like
> that, AFAIR.

Maybe I'm misunderstanding your comment, but it appears quite common to
use "x-" prefix things in the compat tables...

GlobalProperty hw_compat_8_0[] = {
    { "migration", "multifd-flush-after-each-section", "on"},
    { TYPE_PCI_DEVICE, "x-pcie-ari-nextfn-1", "on" },
    { TYPE_VIRTIO_NET, "host_uso", "off"},
    { TYPE_VIRTIO_NET, "guest_uso4", "off"},
    { TYPE_VIRTIO_NET, "guest_uso6", "off"},
};
const size_t hw_compat_8_0_len = G_N_ELEMENTS(hw_compat_8_0);

GlobalProperty hw_compat_7_2[] = {
    { "e1000e", "migrate-timadj", "off" },
    { "virtio-mem", "x-early-migration", "false" },
    { "migration", "x-preempt-pre-7-2", "true" },
    { TYPE_PCI_DEVICE, "x-pcie-err-unc-mask", "off" },
};
const size_t hw_compat_7_2_len = G_N_ELEMENTS(hw_compat_7_2);
[etc]

> > It's still odd to think that we can
> > have scenarios of a non-migratable vfio device registering a migratable
> > ramfb, and vice versa, but I suppose in the end it doesn't matter.  
> 
> I do think it matters! For one, if migration is not possible with
> vfio-pci-nohotplug, then how can QE (or anyone else) *test* the patch
> (i.e. that it makes a difference)? In that case, the ramfb_setup() call
> from vfio-pci-nohotplug should just open-code "false" for the
> "migratable" parameter.

Some vfio devices support migration, most don't.  I was thinking
ramfb_setup might be called with something like:

	(vdev->ramfb_migrate && vdev->enable_migration)

so that at least the ramfb migration state matches the device, but I
think ultimately it only saves a little bit of overhead in registering
the vmstate, either one not supporting migration should block migration.

Hmm, since enable_migration is auto/on/off, it seems like device
realize should fail if set to 'on' and ramfb_migrate is false.  I think
that's the only way the device options don't become self contradictory.
Thanks,

Alex

Re: [PATCH v2 4/5] ramfb: make migration conditional

[Laszlo Ersek]

On 10/2/23 22:38, Alex Williamson wrote:
> On Mon, 2 Oct 2023 21:41:55 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 10/2/23 21:26, Alex Williamson wrote:
>>> On Mon, 2 Oct 2023 20:24:11 +0200
>>> Laszlo Ersek <lersek@redhat.com> wrote:
>>>   
>>>> On 10/2/23 16:41, Alex Williamson wrote:  
>>>>> On Mon, 2 Oct 2023 15:38:10 +0200
>>>>> Cédric Le Goater <clg@redhat.com> wrote:
>>>>>     
>>>>>> On 10/2/23 13:11, marcandre.lureau@redhat.com wrote:    
>>>>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>>>
>>>>>>> RAMFB migration was unsupported until now, let's make it conditional.
>>>>>>> The following patch will prevent machines <= 8.1 to migrate it.
>>>>>>>
>>>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>      
>>>>>> Maybe localize the new 'ramfb_migrate' attribute close to 'enable_ramfb'
>>>>>> in VFIOPCIDevice. Anyhow,    
>>>>>
>>>>> Shouldn't this actually be tied to whether the device is migratable
>>>>> (which for GVT-g - the only ramfb user afaik - it's not)?  What does it
>>>>> mean to have a ramfb-migrate=true property on a device that doesn't
>>>>> support migration, or false on a device that does support migration.  I
>>>>> don't understand why this is a user controllable property.  Thanks,    
>>>>
>>>> The comments in <https://bugzilla.redhat.com/show_bug.cgi?id=1859424>
>>>> (which are unfortunately not public :/ ) suggest that ramfb migration
>>>> was simply forgotten when vGPU migration was implemented. So, "now
>>>> that vGPU migration is done", this should be added.
>>>>
>>>> Comment 8 suggests that the following domain XML snippet
>>>>
>>>>     <hostdev mode='subsystem' type='mdev' managed='no'
>>>> model='vfio-pci' display='on' ramfb='on'> <source>
>>>>         <address uuid='b155147a-663a-4009-ae7f-e9a96805b3ce'/>
>>>>       </source>
>>>>       <alias name='ua-b155147a-663a-4009-ae7f-e9a96805b3ce'/>
>>>>       <address type='pci' domain='0x0000' bus='0x07' slot='0x00'  
>>>> function='0x0'/> </hostdev>  
>>>>
>>>> is migratable, but the ramfb device malfunctions on the destination
>>>> host.
>>>>
>>>> There's also a huge QEMU cmdline in comment#0 of the bug; I've not
>>>> tried to read that.
>>>>
>>>> AIUI BTW the property is not for the user to control, it's just a
>>>> compat knob for versioned machine types. AIUI those are usually
>>>> implemented with such (user-visible / -tweakable) device properties.  
>>>
>>> If it's not for user control it's unfortunate that we expose it to the
>>> user at all, but should it at least use the "x-" prefix to indicate that
>>> it's not intended to be an API?  
>>
>> I *think* it was your commit db32d0f43839 ("vfio/pci: Add option to
>> disable GeForce quirks", 2018-02-06) that hda introduced me to the "x-"
>> prefixed properties!
>>
>> For some reason though, machine type compat knobs are never named like
>> that, AFAIR.
> 
> Maybe I'm misunderstanding your comment, but it appears quite common to
> use "x-" prefix things in the compat tables...

You didn't misunderstand; I was wrong. I judged this off the compat prop
backports to RHEL that I remembered. Your examples from the tree are
good evidence.

> 
> GlobalProperty hw_compat_8_0[] = {
>     { "migration", "multifd-flush-after-each-section", "on"},
>     { TYPE_PCI_DEVICE, "x-pcie-ari-nextfn-1", "on" },
>     { TYPE_VIRTIO_NET, "host_uso", "off"},
>     { TYPE_VIRTIO_NET, "guest_uso4", "off"},
>     { TYPE_VIRTIO_NET, "guest_uso6", "off"},
> };
> const size_t hw_compat_8_0_len = G_N_ELEMENTS(hw_compat_8_0);
> 
> GlobalProperty hw_compat_7_2[] = {
>     { "e1000e", "migrate-timadj", "off" },
>     { "virtio-mem", "x-early-migration", "false" },
>     { "migration", "x-preempt-pre-7-2", "true" },
>     { TYPE_PCI_DEVICE, "x-pcie-err-unc-mask", "off" },
> };
> const size_t hw_compat_7_2_len = G_N_ELEMENTS(hw_compat_7_2);
> [etc]
> 
>>> It's still odd to think that we can
>>> have scenarios of a non-migratable vfio device registering a migratable
>>> ramfb, and vice versa, but I suppose in the end it doesn't matter.  
>>
>> I do think it matters! For one, if migration is not possible with
>> vfio-pci-nohotplug, then how can QE (or anyone else) *test* the patch
>> (i.e. that it makes a difference)? In that case, the ramfb_setup() call
>> from vfio-pci-nohotplug should just open-code "false" for the
>> "migratable" parameter.
> 
> Some vfio devices support migration, most don't.  I was thinking
> ramfb_setup might be called with something like:
> 
> 	(vdev->ramfb_migrate && vdev->enable_migration)
> 
> so that at least the ramfb migration state matches the device, but I
> think ultimately it only saves a little bit of overhead in registering
> the vmstate, either one not supporting migration should block migration.
> 
> Hmm, since enable_migration is auto/on/off, it seems like device
> realize should fail if set to 'on' and ramfb_migrate is false.  I think
> that's the only way the device options don't become self contradictory.
> Thanks,

... easy-looking migration patchset becomes quite complex; isn't that
the story with almost all QEMU work? :)

Thanks!
Laszlo

Re: [PATCH v2 4/5] ramfb: make migration conditional

[Cédric Le Goater]

On 10/2/23 22:38, Alex Williamson wrote:
> On Mon, 2 Oct 2023 21:41:55 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 10/2/23 21:26, Alex Williamson wrote:
>>> On Mon, 2 Oct 2023 20:24:11 +0200
>>> Laszlo Ersek <lersek@redhat.com> wrote:
>>>    
>>>> On 10/2/23 16:41, Alex Williamson wrote:
>>>>> On Mon, 2 Oct 2023 15:38:10 +0200
>>>>> Cédric Le Goater <clg@redhat.com> wrote:
>>>>>      
>>>>>> On 10/2/23 13:11, marcandre.lureau@redhat.com wrote:
>>>>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>>>
>>>>>>> RAMFB migration was unsupported until now, let's make it conditional.
>>>>>>> The following patch will prevent machines <= 8.1 to migrate it.
>>>>>>>
>>>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>> Maybe localize the new 'ramfb_migrate' attribute close to 'enable_ramfb'
>>>>>> in VFIOPCIDevice. Anyhow,
>>>>>
>>>>> Shouldn't this actually be tied to whether the device is migratable
>>>>> (which for GVT-g - the only ramfb user afaik - it's not)?  What does it
>>>>> mean to have a ramfb-migrate=true property on a device that doesn't
>>>>> support migration, or false on a device that does support migration.  I
>>>>> don't understand why this is a user controllable property.  Thanks,
>>>>
>>>> The comments in <https://bugzilla.redhat.com/show_bug.cgi?id=1859424>
>>>> (which are unfortunately not public :/ ) suggest that ramfb migration
>>>> was simply forgotten when vGPU migration was implemented. So, "now
>>>> that vGPU migration is done", this should be added.
>>>>
>>>> Comment 8 suggests that the following domain XML snippet
>>>>
>>>>      <hostdev mode='subsystem' type='mdev' managed='no'
>>>> model='vfio-pci' display='on' ramfb='on'> <source>
>>>>          <address uuid='b155147a-663a-4009-ae7f-e9a96805b3ce'/>
>>>>        </source>
>>>>        <alias name='ua-b155147a-663a-4009-ae7f-e9a96805b3ce'/>
>>>>        <address type='pci' domain='0x0000' bus='0x07' slot='0x00'
>>>> function='0x0'/> </hostdev>
>>>>
>>>> is migratable, but the ramfb device malfunctions on the destination
>>>> host.
>>>>
>>>> There's also a huge QEMU cmdline in comment#0 of the bug; I've not
>>>> tried to read that.
>>>>
>>>> AIUI BTW the property is not for the user to control, it's just a
>>>> compat knob for versioned machine types. AIUI those are usually
>>>> implemented with such (user-visible / -tweakable) device properties.
>>>
>>> If it's not for user control it's unfortunate that we expose it to the
>>> user at all, but should it at least use the "x-" prefix to indicate that
>>> it's not intended to be an API?
>>
>> I *think* it was your commit db32d0f43839 ("vfio/pci: Add option to
>> disable GeForce quirks", 2018-02-06) that hda introduced me to the "x-"
>> prefixed properties!
>>
>> For some reason though, machine type compat knobs are never named like
>> that, AFAIR.
> 
> Maybe I'm misunderstanding your comment, but it appears quite common to
> use "x-" prefix things in the compat tables...
> 
> GlobalProperty hw_compat_8_0[] = {
>      { "migration", "multifd-flush-after-each-section", "on"},
>      { TYPE_PCI_DEVICE, "x-pcie-ari-nextfn-1", "on" },
>      { TYPE_VIRTIO_NET, "host_uso", "off"},
>      { TYPE_VIRTIO_NET, "guest_uso4", "off"},
>      { TYPE_VIRTIO_NET, "guest_uso6", "off"},
> };
> const size_t hw_compat_8_0_len = G_N_ELEMENTS(hw_compat_8_0);
> 
> GlobalProperty hw_compat_7_2[] = {
>      { "e1000e", "migrate-timadj", "off" },
>      { "virtio-mem", "x-early-migration", "false" },
>      { "migration", "x-preempt-pre-7-2", "true" },
>      { TYPE_PCI_DEVICE, "x-pcie-err-unc-mask", "off" },
> };
> const size_t hw_compat_7_2_len = G_N_ELEMENTS(hw_compat_7_2);
> [etc]
> 
>>> It's still odd to think that we can
>>> have scenarios of a non-migratable vfio device registering a migratable
>>> ramfb, and vice versa, but I suppose in the end it doesn't matter.
>>
>> I do think it matters! For one, if migration is not possible with
>> vfio-pci-nohotplug, then how can QE (or anyone else) *test* the patch
>> (i.e. that it makes a difference)? In that case, the ramfb_setup() call
>> from vfio-pci-nohotplug should just open-code "false" for the
>> "migratable" parameter.
> 
> Some vfio devices support migration, most don't.  I was thinking
> ramfb_setup might be called with something like:
> 
> 	(vdev->ramfb_migrate && vdev->enable_migration)
> 
> so that at least the ramfb migration state matches the device, but I
> think ultimately it only saves a little bit of overhead in registering
> the vmstate, either one not supporting migration should block migration.
> 
> Hmm, since enable_migration is auto/on/off, it seems like device
> realize should fail if set to 'on' and ramfb_migrate is false.  I think
> that's the only way the device options don't become self contradictory.

Why isn't VFIODisplay a QOM object ? vfio_display_probe() is more or
less a realize routine, and we have a reset and finalize handlers for it.

(thinking aloud) the "ramfb-migrate" property could then be moved
down VFIODisplay, along with the other specific display properties.
Compatibility could be handled with property aliases. "enable_migration"
could set "ramfb-migrate".This looks like it would be nice model cleanup.

May be not the right time ?

Thanks,

C.

Re: [PATCH v2 4/5] ramfb: make migration conditional

[Marc-André Lureau]

Hi

On Tue, Oct 3, 2023 at 11:43 AM Cédric Le Goater <clg@redhat.com> wrote:
>
> On 10/2/23 22:38, Alex Williamson wrote:
> > On Mon, 2 Oct 2023 21:41:55 +0200
> > Laszlo Ersek <lersek@redhat.com> wrote:
> >
> >> On 10/2/23 21:26, Alex Williamson wrote:
> >>> On Mon, 2 Oct 2023 20:24:11 +0200
> >>> Laszlo Ersek <lersek@redhat.com> wrote:
> >>>
> >>>> On 10/2/23 16:41, Alex Williamson wrote:
> >>>>> On Mon, 2 Oct 2023 15:38:10 +0200
> >>>>> Cédric Le Goater <clg@redhat.com> wrote:
> >>>>>
> >>>>>> On 10/2/23 13:11, marcandre.lureau@redhat.com wrote:
> >>>>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>>>>>>
> >>>>>>> RAMFB migration was unsupported until now, let's make it conditional.
> >>>>>>> The following patch will prevent machines <= 8.1 to migrate it.
> >>>>>>>
> >>>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>>>>> Maybe localize the new 'ramfb_migrate' attribute close to 'enable_ramfb'
> >>>>>> in VFIOPCIDevice. Anyhow,
> >>>>>
> >>>>> Shouldn't this actually be tied to whether the device is migratable
> >>>>> (which for GVT-g - the only ramfb user afaik - it's not)?  What does it
> >>>>> mean to have a ramfb-migrate=true property on a device that doesn't
> >>>>> support migration, or false on a device that does support migration.  I
> >>>>> don't understand why this is a user controllable property.  Thanks,
> >>>>
> >>>> The comments in <https://bugzilla.redhat.com/show_bug.cgi?id=1859424>
> >>>> (which are unfortunately not public :/ ) suggest that ramfb migration
> >>>> was simply forgotten when vGPU migration was implemented. So, "now
> >>>> that vGPU migration is done", this should be added.
> >>>>
> >>>> Comment 8 suggests that the following domain XML snippet
> >>>>
> >>>>      <hostdev mode='subsystem' type='mdev' managed='no'
> >>>> model='vfio-pci' display='on' ramfb='on'> <source>
> >>>>          <address uuid='b155147a-663a-4009-ae7f-e9a96805b3ce'/>
> >>>>        </source>
> >>>>        <alias name='ua-b155147a-663a-4009-ae7f-e9a96805b3ce'/>
> >>>>        <address type='pci' domain='0x0000' bus='0x07' slot='0x00'
> >>>> function='0x0'/> </hostdev>
> >>>>
> >>>> is migratable, but the ramfb device malfunctions on the destination
> >>>> host.
> >>>>
> >>>> There's also a huge QEMU cmdline in comment#0 of the bug; I've not
> >>>> tried to read that.
> >>>>
> >>>> AIUI BTW the property is not for the user to control, it's just a
> >>>> compat knob for versioned machine types. AIUI those are usually
> >>>> implemented with such (user-visible / -tweakable) device properties.
> >>>
> >>> If it's not for user control it's unfortunate that we expose it to the
> >>> user at all, but should it at least use the "x-" prefix to indicate that
> >>> it's not intended to be an API?
> >>
> >> I *think* it was your commit db32d0f43839 ("vfio/pci: Add option to
> >> disable GeForce quirks", 2018-02-06) that hda introduced me to the "x-"
> >> prefixed properties!
> >>
> >> For some reason though, machine type compat knobs are never named like
> >> that, AFAIR.
> >
> > Maybe I'm misunderstanding your comment, but it appears quite common to
> > use "x-" prefix things in the compat tables...
> >
> > GlobalProperty hw_compat_8_0[] = {
> >      { "migration", "multifd-flush-after-each-section", "on"},
> >      { TYPE_PCI_DEVICE, "x-pcie-ari-nextfn-1", "on" },
> >      { TYPE_VIRTIO_NET, "host_uso", "off"},
> >      { TYPE_VIRTIO_NET, "guest_uso4", "off"},
> >      { TYPE_VIRTIO_NET, "guest_uso6", "off"},
> > };
> > const size_t hw_compat_8_0_len = G_N_ELEMENTS(hw_compat_8_0);
> >
> > GlobalProperty hw_compat_7_2[] = {
> >      { "e1000e", "migrate-timadj", "off" },
> >      { "virtio-mem", "x-early-migration", "false" },
> >      { "migration", "x-preempt-pre-7-2", "true" },
> >      { TYPE_PCI_DEVICE, "x-pcie-err-unc-mask", "off" },
> > };
> > const size_t hw_compat_7_2_len = G_N_ELEMENTS(hw_compat_7_2);
> > [etc]
> >
> >>> It's still odd to think that we can
> >>> have scenarios of a non-migratable vfio device registering a migratable
> >>> ramfb, and vice versa, but I suppose in the end it doesn't matter.
> >>
> >> I do think it matters! For one, if migration is not possible with
> >> vfio-pci-nohotplug, then how can QE (or anyone else) *test* the patch
> >> (i.e. that it makes a difference)? In that case, the ramfb_setup() call
> >> from vfio-pci-nohotplug should just open-code "false" for the
> >> "migratable" parameter.
> >
> > Some vfio devices support migration, most don't.  I was thinking
> > ramfb_setup might be called with something like:
> >
> >       (vdev->ramfb_migrate && vdev->enable_migration)
> >
> > so that at least the ramfb migration state matches the device, but I
> > think ultimately it only saves a little bit of overhead in registering
> > the vmstate, either one not supporting migration should block migration.
> >
> > Hmm, since enable_migration is auto/on/off, it seems like device
> > realize should fail if set to 'on' and ramfb_migrate is false.  I think
> > that's the only way the device options don't become self contradictory.
>
> Why isn't VFIODisplay a QOM object ? vfio_display_probe() is more or
> less a realize routine, and we have a reset and finalize handlers for it.
>
> (thinking aloud) the "ramfb-migrate" property could then be moved
> down VFIODisplay, along with the other specific display properties.
> Compatibility could be handled with property aliases. "enable_migration"
> could set "ramfb-migrate".This looks like it would be nice model cleanup.
>
> May be not the right time ?

Yes, I thought about some similar changes (though I am not sure QOM is
necessary).

Now I am trying to test my changes that add a VFIODisplay migration
subsection, but I don't think I have a GVT-g GPU (TGL GT1). When I try
with a random PCI device, I get "VFIO migration is not supported in
kernel". I can try to comment out some code, but that seems hazardous.





--
Marc-André Lureau

Re: [PATCH v2 4/5] ramfb: make migration conditional

[Cédric Le Goater]

On 10/3/23 10:23, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Oct 3, 2023 at 11:43 AM Cédric Le Goater <clg@redhat.com> wrote:
>>
>> On 10/2/23 22:38, Alex Williamson wrote:
>>> On Mon, 2 Oct 2023 21:41:55 +0200
>>> Laszlo Ersek <lersek@redhat.com> wrote:
>>>
>>>> On 10/2/23 21:26, Alex Williamson wrote:
>>>>> On Mon, 2 Oct 2023 20:24:11 +0200
>>>>> Laszlo Ersek <lersek@redhat.com> wrote:
>>>>>
>>>>>> On 10/2/23 16:41, Alex Williamson wrote:
>>>>>>> On Mon, 2 Oct 2023 15:38:10 +0200
>>>>>>> Cédric Le Goater <clg@redhat.com> wrote:
>>>>>>>
>>>>>>>> On 10/2/23 13:11, marcandre.lureau@redhat.com wrote:
>>>>>>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>>>>>
>>>>>>>>> RAMFB migration was unsupported until now, let's make it conditional.
>>>>>>>>> The following patch will prevent machines <= 8.1 to migrate it.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>>>> Maybe localize the new 'ramfb_migrate' attribute close to 'enable_ramfb'
>>>>>>>> in VFIOPCIDevice. Anyhow,
>>>>>>>
>>>>>>> Shouldn't this actually be tied to whether the device is migratable
>>>>>>> (which for GVT-g - the only ramfb user afaik - it's not)?  What does it
>>>>>>> mean to have a ramfb-migrate=true property on a device that doesn't
>>>>>>> support migration, or false on a device that does support migration.  I
>>>>>>> don't understand why this is a user controllable property.  Thanks,
>>>>>>
>>>>>> The comments in <https://bugzilla.redhat.com/show_bug.cgi?id=1859424>
>>>>>> (which are unfortunately not public :/ ) suggest that ramfb migration
>>>>>> was simply forgotten when vGPU migration was implemented. So, "now
>>>>>> that vGPU migration is done", this should be added.
>>>>>>
>>>>>> Comment 8 suggests that the following domain XML snippet
>>>>>>
>>>>>>       <hostdev mode='subsystem' type='mdev' managed='no'
>>>>>> model='vfio-pci' display='on' ramfb='on'> <source>
>>>>>>           <address uuid='b155147a-663a-4009-ae7f-e9a96805b3ce'/>
>>>>>>         </source>
>>>>>>         <alias name='ua-b155147a-663a-4009-ae7f-e9a96805b3ce'/>
>>>>>>         <address type='pci' domain='0x0000' bus='0x07' slot='0x00'
>>>>>> function='0x0'/> </hostdev>
>>>>>>
>>>>>> is migratable, but the ramfb device malfunctions on the destination
>>>>>> host.
>>>>>>
>>>>>> There's also a huge QEMU cmdline in comment#0 of the bug; I've not
>>>>>> tried to read that.
>>>>>>
>>>>>> AIUI BTW the property is not for the user to control, it's just a
>>>>>> compat knob for versioned machine types. AIUI those are usually
>>>>>> implemented with such (user-visible / -tweakable) device properties.
>>>>>
>>>>> If it's not for user control it's unfortunate that we expose it to the
>>>>> user at all, but should it at least use the "x-" prefix to indicate that
>>>>> it's not intended to be an API?
>>>>
>>>> I *think* it was your commit db32d0f43839 ("vfio/pci: Add option to
>>>> disable GeForce quirks", 2018-02-06) that hda introduced me to the "x-"
>>>> prefixed properties!
>>>>
>>>> For some reason though, machine type compat knobs are never named like
>>>> that, AFAIR.
>>>
>>> Maybe I'm misunderstanding your comment, but it appears quite common to
>>> use "x-" prefix things in the compat tables...
>>>
>>> GlobalProperty hw_compat_8_0[] = {
>>>       { "migration", "multifd-flush-after-each-section", "on"},
>>>       { TYPE_PCI_DEVICE, "x-pcie-ari-nextfn-1", "on" },
>>>       { TYPE_VIRTIO_NET, "host_uso", "off"},
>>>       { TYPE_VIRTIO_NET, "guest_uso4", "off"},
>>>       { TYPE_VIRTIO_NET, "guest_uso6", "off"},
>>> };
>>> const size_t hw_compat_8_0_len = G_N_ELEMENTS(hw_compat_8_0);
>>>
>>> GlobalProperty hw_compat_7_2[] = {
>>>       { "e1000e", "migrate-timadj", "off" },
>>>       { "virtio-mem", "x-early-migration", "false" },
>>>       { "migration", "x-preempt-pre-7-2", "true" },
>>>       { TYPE_PCI_DEVICE, "x-pcie-err-unc-mask", "off" },
>>> };
>>> const size_t hw_compat_7_2_len = G_N_ELEMENTS(hw_compat_7_2);
>>> [etc]
>>>
>>>>> It's still odd to think that we can
>>>>> have scenarios of a non-migratable vfio device registering a migratable
>>>>> ramfb, and vice versa, but I suppose in the end it doesn't matter.
>>>>
>>>> I do think it matters! For one, if migration is not possible with
>>>> vfio-pci-nohotplug, then how can QE (or anyone else) *test* the patch
>>>> (i.e. that it makes a difference)? In that case, the ramfb_setup() call
>>>> from vfio-pci-nohotplug should just open-code "false" for the
>>>> "migratable" parameter.
>>>
>>> Some vfio devices support migration, most don't.  I was thinking
>>> ramfb_setup might be called with something like:
>>>
>>>        (vdev->ramfb_migrate && vdev->enable_migration)
>>>
>>> so that at least the ramfb migration state matches the device, but I
>>> think ultimately it only saves a little bit of overhead in registering
>>> the vmstate, either one not supporting migration should block migration.
>>>
>>> Hmm, since enable_migration is auto/on/off, it seems like device
>>> realize should fail if set to 'on' and ramfb_migrate is false.  I think
>>> that's the only way the device options don't become self contradictory.
>>
>> Why isn't VFIODisplay a QOM object ? vfio_display_probe() is more or
>> less a realize routine, and we have a reset and finalize handlers for it.
>>
>> (thinking aloud) the "ramfb-migrate" property could then be moved
>> down VFIODisplay, along with the other specific display properties.
>> Compatibility could be handled with property aliases. "enable_migration"
>> could set "ramfb-migrate".This looks like it would be nice model cleanup.
>>
>> May be not the right time ?
> 
> Yes, I thought about some similar changes (though I am not sure QOM is
> necessary).
> 
> Now I am trying to test my changes that add a VFIODisplay migration
> subsection, but I don't think I have a GVT-g GPU (TGL GT1). When I try
> with a random PCI device, I get "VFIO migration is not supported in
> kernel". I can try to comment out some code, but that seems hazardous.

I have recently setup a T480 with GVT-g and a windows guest. I can give
your changes a try.

C.

Re: [PATCH v2 4/5] ramfb: make migration conditional

[Laszlo Ersek]

On 10/2/23 13:11, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> RAMFB migration was unsupported until now, let's make it conditional.
> The following patch will prevent machines <= 8.1 to migrate it.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/vfio/pci.h                 | 1 +
>  include/hw/display/ramfb.h    | 2 +-
>  hw/display/ramfb-standalone.c | 8 +++++++-
>  hw/display/ramfb.c            | 6 ++++--
>  hw/vfio/display.c             | 4 ++--
>  hw/vfio/pci.c                 | 1 +
>  stubs/ramfb.c                 | 2 +-
>  7 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index 2d836093a8..671cc78912 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -156,6 +156,7 @@ struct VFIOPCIDevice {
>      OnOffAuto display;
>      uint32_t display_xres;
>      uint32_t display_yres;
> +    bool ramfb_migrate;
>      int32_t bootindex;
>      uint32_t igd_gms;
>      OffAutoPCIBAR msix_relo;
> diff --git a/include/hw/display/ramfb.h b/include/hw/display/ramfb.h
> index b33a2c467b..40063b62bd 100644
> --- a/include/hw/display/ramfb.h
> +++ b/include/hw/display/ramfb.h
> @@ -4,7 +4,7 @@
>  /* ramfb.c */
>  typedef struct RAMFBState RAMFBState;
>  void ramfb_display_update(QemuConsole *con, RAMFBState *s);
> -RAMFBState *ramfb_setup(Error **errp);
> +RAMFBState *ramfb_setup(bool migrate, Error **errp);
>  
>  /* ramfb-standalone.c */
>  #define TYPE_RAMFB_DEVICE "ramfb"
> diff --git a/hw/display/ramfb-standalone.c b/hw/display/ramfb-standalone.c
> index 8c0094397f..6bbd69ccdf 100644
> --- a/hw/display/ramfb-standalone.c
> +++ b/hw/display/ramfb-standalone.c
> @@ -15,6 +15,7 @@ struct RAMFBStandaloneState {
>      SysBusDevice parent_obj;
>      QemuConsole *con;
>      RAMFBState *state;
> +    bool migrate;
>  };
>  
>  static void display_update_wrapper(void *dev)
> @@ -37,9 +38,13 @@ static void ramfb_realizefn(DeviceState *dev, Error **errp)
>      RAMFBStandaloneState *ramfb = RAMFB(dev);
>  
>      ramfb->con = graphic_console_init(dev, 0, &wrapper_ops, dev);
> -    ramfb->state = ramfb_setup(errp);
> +    ramfb->state = ramfb_setup(ramfb->migrate, errp);
>  }
>  
> +static Property ramfb_properties[] = {
> +    DEFINE_PROP_BOOL("migrate", RAMFBStandaloneState, migrate,  true),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
>  static void ramfb_class_initfn(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -48,6 +53,7 @@ static void ramfb_class_initfn(ObjectClass *klass, void *data)
>      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 = {
> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
> index 4aaaa7d653..73e08d605f 100644
> --- a/hw/display/ramfb.c
> +++ b/hw/display/ramfb.c
> @@ -135,7 +135,7 @@ static const VMStateDescription vmstate_ramfb = {
>      }
>  };
>  
> -RAMFBState *ramfb_setup(Error **errp)
> +RAMFBState *ramfb_setup(bool migrate, Error **errp)
>  {
>      FWCfgState *fw_cfg = fw_cfg_find();
>      RAMFBState *s;
> @@ -147,7 +147,9 @@ RAMFBState *ramfb_setup(Error **errp)
>  
>      s = g_new0(RAMFBState, 1);
>  
> -    vmstate_register(NULL, 0, &vmstate_ramfb, s);
> +    if (migrate) {
> +        vmstate_register(NULL, 0, &vmstate_ramfb, s);
> +    }
>      rom_add_vga("vgabios-ramfb.bin");
>      fw_cfg_add_file_callback(fw_cfg, "etc/ramfb",
>                               NULL, ramfb_fw_cfg_write, s,
> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> index bec864f482..3f6b251ccd 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -356,7 +356,7 @@ static int vfio_display_dmabuf_init(VFIOPCIDevice *vdev, Error **errp)
>                                            &vfio_display_dmabuf_ops,
>                                            vdev);
>      if (vdev->enable_ramfb) {
> -        vdev->dpy->ramfb = ramfb_setup(errp);
> +        vdev->dpy->ramfb = ramfb_setup(vdev->ramfb_migrate, errp);
>      }
>      vfio_display_edid_init(vdev);
>      return 0;
> @@ -483,7 +483,7 @@ static int vfio_display_region_init(VFIOPCIDevice *vdev, Error **errp)
>                                            &vfio_display_region_ops,
>                                            vdev);
>      if (vdev->enable_ramfb) {
> -        vdev->dpy->ramfb = ramfb_setup(errp);
> +        vdev->dpy->ramfb = ramfb_setup(vdev->ramfb_migrate, errp);
>      }
>      return 0;
>  }
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 3b2ca3c24c..6575b8f32d 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3484,6 +3484,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_BOOL("ramfb-migrate", VFIOPCIDevice, ramfb_migrate,  true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/stubs/ramfb.c b/stubs/ramfb.c
> index 48143f3354..8869a5db09 100644
> --- a/stubs/ramfb.c
> +++ b/stubs/ramfb.c
> @@ -6,7 +6,7 @@ void ramfb_display_update(QemuConsole *con, RAMFBState *s)
>  {
>  }
>  
> -RAMFBState *ramfb_setup(Error **errp)
> +RAMFBState *ramfb_setup(bool migrate, Error **errp)
>  {
>      error_setg(errp, "ramfb support not available");
>      return NULL;

I'm not a seasoned migration reviewer, so -- apologies for the churn --
I'd prefer if this patch were split in three:

- First patch: ramfb_setup() update. All callers pass in constant
"false". Stub updated.

- Second patch: new property for vfio-pci-nohotplug, hooked up to
ramfb_setup() for real.

- Third patch: new property for ramfb-standalone, hooked up to
ramfb_setup() for real.

Without this separation, the hunks are just heaped together in the
patch, and I'm having a hard time understanding the data flow.

Again, apologies that I'm requesting this for a very small patch.

Thanks,
Laszlo

[PATCH v2 5/5] hw: turn off ramfb migration for machines <= 8.1

[marcandre.lureau]

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

For compatibility reasons.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/core/machine.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 68cb556197..2fa7647422 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -30,7 +30,10 @@
 #include "hw/virtio/virtio-pci.h"
 #include "hw/virtio/virtio-net.h"
 
-GlobalProperty hw_compat_8_1[] = {};
+GlobalProperty hw_compat_8_1[] = {
+    { "ramfb", "migrate", "off" },
+    { "vfio-pci-nohotplug", "ramfb-migrate", "off" }
+};
 const size_t hw_compat_8_1_len = G_N_ELEMENTS(hw_compat_8_1);
 
 GlobalProperty hw_compat_8_0[] = {
-- 
2.41.0

Re: [PATCH v2 5/5] hw: turn off ramfb migration for machines <= 8.1

[Laszlo Ersek]

On 10/2/23 13:11, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> For compatibility reasons.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/core/machine.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 68cb556197..2fa7647422 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -30,7 +30,10 @@
>  #include "hw/virtio/virtio-pci.h"
>  #include "hw/virtio/virtio-net.h"
>  
> -GlobalProperty hw_compat_8_1[] = {};
> +GlobalProperty hw_compat_8_1[] = {
> +    { "ramfb", "migrate", "off" },
> +    { "vfio-pci-nohotplug", "ramfb-migrate", "off" }
> +};
>  const size_t hw_compat_8_1_len = G_N_ELEMENTS(hw_compat_8_1);
>  
>  GlobalProperty hw_compat_8_0[] = {

In the other discussion, you mentioned the concrete reason for this -- I
think if we don't do this, then the ramfb vmstate blocks backward
migration? Can you document the reason here explicitly (commit message,
I mean, doesn't have to be a code comment)?

Thanks!
Laszlo

Re: [PATCH v2 5/5] hw: turn off ramfb migration for machines <= 8.1

[Marc-André Lureau]

Hi

On Mon, Oct 2, 2023 at 6:42 PM Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 10/2/23 13:11, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > For compatibility reasons.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  hw/core/machine.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 68cb556197..2fa7647422 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -30,7 +30,10 @@
> >  #include "hw/virtio/virtio-pci.h"
> >  #include "hw/virtio/virtio-net.h"
> >
> > -GlobalProperty hw_compat_8_1[] = {};
> > +GlobalProperty hw_compat_8_1[] = {
> > +    { "ramfb", "migrate", "off" },
> > +    { "vfio-pci-nohotplug", "ramfb-migrate", "off" }
> > +};
> >  const size_t hw_compat_8_1_len = G_N_ELEMENTS(hw_compat_8_1);
> >
> >  GlobalProperty hw_compat_8_0[] = {
>
> In the other discussion, you mentioned the concrete reason for this -- I
> think if we don't do this, then the ramfb vmstate blocks backward
> migration? Can you document the reason here explicitly (commit message,
> I mean, doesn't have to be a code comment)?

By using device section/subsection in v3, I changed a little bit the
reasons for the properties. Now it falls under the common issue
documented in migration.rst "Changing migration data structure".

From v3, let me know if you still think we should document that better
in the commit message.

thanks

-- 
Marc-André Lureau