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

[Laszlo Ersek <lersek@redhat.com>]

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

Sounds like a valid idea to me:

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

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

Laszlo

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