[v5] ramfb: migration support

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

Posted by marcandre.lureau@redhat.com 2 years, 4 months ago

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

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

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

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

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

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

Posted by Laszlo Ersek 2 years, 4 months ago

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

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

My R-b stands.

Thanks
Laszlo

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

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

Posted by Cédric Le Goater 2 years, 4 months ago

On 10/9/23 12:19, Laszlo Ersek wrote:
> On 10/9/23 08:32, marcandre.lureau@redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> Add a "VFIODisplay" subsection whenever "x-ramfb-migrate" is turned on.
>>
>> Turn it off by default on machines <= 8.1 for compatibility reasons.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>   hw/vfio/pci.h     |  3 +++
>>   hw/core/machine.c |  1 +
>>   hw/vfio/display.c | 21 +++++++++++++++++++++
>>   hw/vfio/pci.c     | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>   stubs/ramfb.c     |  2 ++
>>   5 files changed, 71 insertions(+)
>>
>> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
>> index 2d836093a8..fd06695542 100644
>> --- a/hw/vfio/pci.h
>> +++ b/hw/vfio/pci.h
>> @@ -173,6 +173,7 @@ struct VFIOPCIDevice {
>>       bool no_kvm_ioeventfd;
>>       bool no_vfio_ioeventfd;
>>       bool enable_ramfb;
>> +    OnOffAuto ramfb_migrate;
>>       bool defer_kvm_irq_routing;
>>       bool clear_parent_atomics_on_exit;
>>       VFIODisplay *dpy;
>> @@ -226,4 +227,6 @@ void vfio_display_reset(VFIOPCIDevice *vdev);
>>   int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp);
>>   void vfio_display_finalize(VFIOPCIDevice *vdev);
>>   
>> +extern const VMStateDescription vfio_display_vmstate;
>> +
>>   #endif /* HW_VFIO_VFIO_PCI_H */
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 6305f2d7a4..05aef2cf9f 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -35,6 +35,7 @@
>>   GlobalProperty hw_compat_8_1[] = {
>>       { TYPE_PCI_BRIDGE, "x-pci-express-writeable-slt-bug", "true" },
>>       { "ramfb", "x-migrate", "off" },
>> +    { "vfio-pci-nohotplug", "x-ramfb-migrate", "off" }
>>   };
>>   const size_t hw_compat_8_1_len = G_N_ELEMENTS(hw_compat_8_1);
>>   
>> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
>> index bec864f482..2739ba56ec 100644
>> --- a/hw/vfio/display.c
>> +++ b/hw/vfio/display.c
>> @@ -542,3 +542,24 @@ void vfio_display_finalize(VFIOPCIDevice *vdev)
>>       vfio_display_edid_exit(vdev->dpy);
>>       g_free(vdev->dpy);
>>   }
>> +
>> +static bool migrate_needed(void *opaque)
>> +{
>> +    VFIODisplay *dpy = opaque;
>> +    bool ramfb_exists = dpy->ramfb != NULL;
>> +
>> +    /* see vfio_display_migration_needed() */
>> +    assert(ramfb_exists);
>> +    return ramfb_exists;
>> +}
>> +
>> +const VMStateDescription vfio_display_vmstate = {
>> +    .name = "VFIODisplay",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .needed = migrate_needed,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_STRUCT_POINTER(ramfb, VFIODisplay, ramfb_vmstate, RAMFBState),
>> +        VMSTATE_END_OF_LIST(),
>> +    }
>> +};
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 3b2ca3c24c..e44ed21180 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2608,6 +2608,32 @@ static bool vfio_msix_present(void *opaque, int version_id)
>>       return msix_present(pdev);
>>   }
>>   
>> +static bool vfio_display_migration_needed(void *opaque)
>> +{
>> +    VFIOPCIDevice *vdev = opaque;
>> +
>> +    /*
>> +     * We need to migrate the VFIODisplay object if ramfb *migration* was
>> +     * explicitly requested (in which case we enforced both ramfb=on and
>> +     * display=on), or ramfb migration was left at the default "auto"
>> +     * setting, and *ramfb* was explicitly requested (in which case we
>> +     * enforced display=on).
>> +     */
>> +    return vdev->ramfb_migrate == ON_OFF_AUTO_ON ||
>> +        (vdev->ramfb_migrate == ON_OFF_AUTO_AUTO && vdev->enable_ramfb);
>> +}
>> +
>> +const VMStateDescription vmstate_vfio_display = {
>> +    .name = "VFIOPCIDevice/VFIODisplay",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .needed = vfio_display_migration_needed,
>> +    .fields = (VMStateField[]){
>> +        VMSTATE_STRUCT_POINTER(dpy, VFIOPCIDevice, vfio_display_vmstate, VFIODisplay),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>>   const VMStateDescription vmstate_vfio_pci_config = {
>>       .name = "VFIOPCIDevice",
>>       .version_id = 1,
>> @@ -2616,6 +2642,10 @@ const VMStateDescription vmstate_vfio_pci_config = {
>>           VMSTATE_PCI_DEVICE(pdev, VFIOPCIDevice),
>>           VMSTATE_MSIX_TEST(pdev, VFIOPCIDevice, vfio_msix_present),
>>           VMSTATE_END_OF_LIST()
>> +    },
>> +    .subsections = (const VMStateDescription*[]) {
>> +        &vmstate_vfio_display,
>> +        NULL
>>       }
>>   };
>>   
>> @@ -3271,6 +3301,19 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>>           }
>>       }
>>   
>> +    if (vdev->ramfb_migrate == ON_OFF_AUTO_ON && !vdev->enable_ramfb) {
>> +        warn_report("x-ramfb-migrate=on but ramfb=off");
>> +        vdev->ramfb_migrate = ON_OFF_AUTO_OFF;
> 
> the warning could give a hint about the resultant action taken (i.e.,
> forcing off x-ramfb-migrate), but don't repost just for that; it's a nit.

yes.

How about :

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

I can amend the patch if you agree.

Thanks,

C.

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

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

Posted by Marc-André Lureau 2 years, 4 months ago

Hi

On Tue, Oct 10, 2023 at 11:03 AM Cédric Le Goater <clg@redhat.com> wrote:
>
> On 10/9/23 12:19, Laszlo Ersek wrote:
> > On 10/9/23 08:32, marcandre.lureau@redhat.com wrote:
> >> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>
> >> Add a "VFIODisplay" subsection whenever "x-ramfb-migrate" is turned on.
> >>
> >> Turn it off by default on machines <= 8.1 for compatibility reasons.
> >>
> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> >> ---
> >>   hw/vfio/pci.h     |  3 +++
> >>   hw/core/machine.c |  1 +
> >>   hw/vfio/display.c | 21 +++++++++++++++++++++
> >>   hw/vfio/pci.c     | 44 ++++++++++++++++++++++++++++++++++++++++++++
> >>   stubs/ramfb.c     |  2 ++
> >>   5 files changed, 71 insertions(+)
> >>
> >> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> >> index 2d836093a8..fd06695542 100644
> >> --- a/hw/vfio/pci.h
> >> +++ b/hw/vfio/pci.h
> >> @@ -173,6 +173,7 @@ struct VFIOPCIDevice {
> >>       bool no_kvm_ioeventfd;
> >>       bool no_vfio_ioeventfd;
> >>       bool enable_ramfb;
> >> +    OnOffAuto ramfb_migrate;
> >>       bool defer_kvm_irq_routing;
> >>       bool clear_parent_atomics_on_exit;
> >>       VFIODisplay *dpy;
> >> @@ -226,4 +227,6 @@ void vfio_display_reset(VFIOPCIDevice *vdev);
> >>   int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp);
> >>   void vfio_display_finalize(VFIOPCIDevice *vdev);
> >>
> >> +extern const VMStateDescription vfio_display_vmstate;
> >> +
> >>   #endif /* HW_VFIO_VFIO_PCI_H */
> >> diff --git a/hw/core/machine.c b/hw/core/machine.c
> >> index 6305f2d7a4..05aef2cf9f 100644
> >> --- a/hw/core/machine.c
> >> +++ b/hw/core/machine.c
> >> @@ -35,6 +35,7 @@
> >>   GlobalProperty hw_compat_8_1[] = {
> >>       { TYPE_PCI_BRIDGE, "x-pci-express-writeable-slt-bug", "true" },
> >>       { "ramfb", "x-migrate", "off" },
> >> +    { "vfio-pci-nohotplug", "x-ramfb-migrate", "off" }
> >>   };
> >>   const size_t hw_compat_8_1_len = G_N_ELEMENTS(hw_compat_8_1);
> >>
> >> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> >> index bec864f482..2739ba56ec 100644
> >> --- a/hw/vfio/display.c
> >> +++ b/hw/vfio/display.c
> >> @@ -542,3 +542,24 @@ void vfio_display_finalize(VFIOPCIDevice *vdev)
> >>       vfio_display_edid_exit(vdev->dpy);
> >>       g_free(vdev->dpy);
> >>   }
> >> +
> >> +static bool migrate_needed(void *opaque)
> >> +{
> >> +    VFIODisplay *dpy = opaque;
> >> +    bool ramfb_exists = dpy->ramfb != NULL;
> >> +
> >> +    /* see vfio_display_migration_needed() */
> >> +    assert(ramfb_exists);
> >> +    return ramfb_exists;
> >> +}
> >> +
> >> +const VMStateDescription vfio_display_vmstate = {
> >> +    .name = "VFIODisplay",
> >> +    .version_id = 1,
> >> +    .minimum_version_id = 1,
> >> +    .needed = migrate_needed,
> >> +    .fields = (VMStateField[]) {
> >> +        VMSTATE_STRUCT_POINTER(ramfb, VFIODisplay, ramfb_vmstate, RAMFBState),
> >> +        VMSTATE_END_OF_LIST(),
> >> +    }
> >> +};
> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> >> index 3b2ca3c24c..e44ed21180 100644
> >> --- a/hw/vfio/pci.c
> >> +++ b/hw/vfio/pci.c
> >> @@ -2608,6 +2608,32 @@ static bool vfio_msix_present(void *opaque, int version_id)
> >>       return msix_present(pdev);
> >>   }
> >>
> >> +static bool vfio_display_migration_needed(void *opaque)
> >> +{
> >> +    VFIOPCIDevice *vdev = opaque;
> >> +
> >> +    /*
> >> +     * We need to migrate the VFIODisplay object if ramfb *migration* was
> >> +     * explicitly requested (in which case we enforced both ramfb=on and
> >> +     * display=on), or ramfb migration was left at the default "auto"
> >> +     * setting, and *ramfb* was explicitly requested (in which case we
> >> +     * enforced display=on).
> >> +     */
> >> +    return vdev->ramfb_migrate == ON_OFF_AUTO_ON ||
> >> +        (vdev->ramfb_migrate == ON_OFF_AUTO_AUTO && vdev->enable_ramfb);
> >> +}
> >> +
> >> +const VMStateDescription vmstate_vfio_display = {
> >> +    .name = "VFIOPCIDevice/VFIODisplay",
> >> +    .version_id = 1,
> >> +    .minimum_version_id = 1,
> >> +    .needed = vfio_display_migration_needed,
> >> +    .fields = (VMStateField[]){
> >> +        VMSTATE_STRUCT_POINTER(dpy, VFIOPCIDevice, vfio_display_vmstate, VFIODisplay),
> >> +        VMSTATE_END_OF_LIST()
> >> +    }
> >> +};
> >> +
> >>   const VMStateDescription vmstate_vfio_pci_config = {
> >>       .name = "VFIOPCIDevice",
> >>       .version_id = 1,
> >> @@ -2616,6 +2642,10 @@ const VMStateDescription vmstate_vfio_pci_config = {
> >>           VMSTATE_PCI_DEVICE(pdev, VFIOPCIDevice),
> >>           VMSTATE_MSIX_TEST(pdev, VFIOPCIDevice, vfio_msix_present),
> >>           VMSTATE_END_OF_LIST()
> >> +    },
> >> +    .subsections = (const VMStateDescription*[]) {
> >> +        &vmstate_vfio_display,
> >> +        NULL
> >>       }
> >>   };
> >>
> >> @@ -3271,6 +3301,19 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
> >>           }
> >>       }
> >>
> >> +    if (vdev->ramfb_migrate == ON_OFF_AUTO_ON && !vdev->enable_ramfb) {
> >> +        warn_report("x-ramfb-migrate=on but ramfb=off");
> >> +        vdev->ramfb_migrate = ON_OFF_AUTO_OFF;
> >
> > the warning could give a hint about the resultant action taken (i.e.,
> > forcing off x-ramfb-migrate), but don't repost just for that; it's a nit.
>
> yes.
>
> How about :
>
>          warn_report("x-ramfb-migrate=on but ramfb=off. Forcing x-ramfb-migrate to off.");

Sure, that's better.

>
> I can amend the patch if you agree.

 thanks!

[PATCH v5 1/3] ramfb: add migration support
[PATCH v5 2/3] ramfb-standalone: add migration support
[PATCH v5 3/3] hw/vfio: add ramfb migration support