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

[Laszlo Ersek]

On 10/3/23 12:47, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Oct 3, 2023 at 2:17 PM Cédric Le Goater <clg@redhat.com> wrote:
>>
>> On 10/3/23 10:56, marcandre.lureau@redhat.com wrote:
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> Add a "VFIODisplay" subsection whenever "x-ramfb-migrate" is turned on.
>>>
>>> Turn it off by default on machines <= 8.1 for compatibility reasons.
>>
>>
>> This change breaks linking on various platforms with :
>>
>> /usr/bin/ld: 
>> libqemu-xtensa-softmmu.fa.p/hw_vfio_display.c.o:(.data.rel+0x50): undefined 
>> reference to `ramfb_vmstate'
>>
>> Some stubs updates are missing it seems..
>>
> 
> diff --git a/stubs/ramfb.c b/stubs/ramfb.c
> index 48143f3354..cf64733b10 100644
> --- a/stubs/ramfb.c
> +++ b/stubs/ramfb.c
> @@ -2,6 +2,8 @@
>  #include "qapi/error.h"
>  #include "hw/display/ramfb.h"
> 
> +const VMStateDescription ramfb_vmstate = {};
> +
> 
> 
> And I think we should also change the "needed" condition to:
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 4689f2e5c1..b327844764 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2613,7 +2613,7 @@ static bool vfio_display_needed(void *opaque)
>      VFIOPCIDevice *vdev = opaque;
> 
>      /* the only thing that justifies the VFIODisplay sub-section atm */
> -    return vdev->ramfb_migrate != ON_OFF_AUTO_OFF;
> +    return vdev->enable_ramfb && vdev->ramfb_migrate != ON_OFF_AUTO_OFF;
>  }
> 

Exactly, this was one of my comments in review.

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

Laszlo

> 
> 
>> Thanks,
>>
>> C.
>>
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>   hw/vfio/pci.h     |  3 +++
>>>   hw/core/machine.c |  1 +
>>>   hw/vfio/display.c | 23 +++++++++++++++++++++++
>>>   hw/vfio/pci.c     | 32 ++++++++++++++++++++++++++++++++
>>>   4 files changed, 59 insertions(+)
>>>
>>> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
>>> index 2d836093a8..fd06695542 100644
>>> --- a/hw/vfio/pci.h
>>> +++ b/hw/vfio/pci.h
>>> @@ -173,6 +173,7 @@ struct VFIOPCIDevice {
>>>       bool no_kvm_ioeventfd;
>>>       bool no_vfio_ioeventfd;
>>>       bool enable_ramfb;
>>> +    OnOffAuto ramfb_migrate;
>>>       bool defer_kvm_irq_routing;
>>>       bool clear_parent_atomics_on_exit;
>>>       VFIODisplay *dpy;
>>> @@ -226,4 +227,6 @@ void vfio_display_reset(VFIOPCIDevice *vdev);
>>>   int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp);
>>>   void vfio_display_finalize(VFIOPCIDevice *vdev);
>>>
>>> +extern const VMStateDescription vfio_display_vmstate;
>>> +
>>>   #endif /* HW_VFIO_VFIO_PCI_H */
>>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>>> index 47a07d1d9b..f2f8940a85 100644
>>> --- a/hw/core/machine.c
>>> +++ b/hw/core/machine.c
>>> @@ -32,6 +32,7 @@
>>>
>>>   GlobalProperty hw_compat_8_1[] = {
>>>       { "ramfb", "x-migrate", "off" },
>>> +    { "vfio-pci-nohotplug", "x-ramfb-migrate", "off" }
>>>   };
>>>   const size_t hw_compat_8_1_len = G_N_ELEMENTS(hw_compat_8_1);
>>>
>>> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
>>> index bec864f482..de5bf71dd1 100644
>>> --- a/hw/vfio/display.c
>>> +++ b/hw/vfio/display.c
>>> @@ -542,3 +542,26 @@ void vfio_display_finalize(VFIOPCIDevice *vdev)
>>>       vfio_display_edid_exit(vdev->dpy);
>>>       g_free(vdev->dpy);
>>>   }
>>> +
>>> +static bool migrate_needed(void *opaque)
>>> +{
>>> +    /*
>>> +     * If we are here, it's because vfio_display_needed(), which is only 
>>> true
>>> +     * when dpy->ramfb_migrate atm.
>>> +     *
>>> +     * If the migration condition is changed, we should check here if
>>> +     * ramfb_migrate is true. (this will need a way to lookup the 
>>> associated
>>> +     * VFIOPCIDevice somehow, or fields to be moved, ..)
>>> +     */
>>> +    return true;
>>> +}
>>> +
>>> +const VMStateDescription vfio_display_vmstate = {
>>> +    .name = "VFIODisplay",
>>> +    .version_id = 1,
>>> +    .minimum_version_id = 1,
>>> +    .needed = migrate_needed,
>>> +    .fields = (VMStateField[]) {
>>> +        VMSTATE_STRUCT_POINTER(ramfb, VFIODisplay, ramfb_vmstate, 
>>> RAMFBState),
>>> +    }
>>> +};
>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>> index 3b2ca3c24c..4689f2e5c1 100644
>>> --- a/hw/vfio/pci.c
>>> +++ b/hw/vfio/pci.c
>>> @@ -2608,6 +2608,25 @@ static bool vfio_msix_present(void *opaque, int 
>>> version_id)
>>>       return msix_present(pdev);
>>>   }
>>>
>>> +static bool vfio_display_needed(void *opaque)
>>> +{
>>> +    VFIOPCIDevice *vdev = opaque;
>>> +
>>> +    /* the only thing that justifies the VFIODisplay sub-section atm */
>>> +    return vdev->ramfb_migrate != ON_OFF_AUTO_OFF;
>>> +}
>>> +
>>> +const VMStateDescription vmstate_vfio_display = {
>>> +    .name = "VFIOPCIDevice/VFIODisplay",
>>> +    .version_id = 1,
>>> +    .minimum_version_id = 1,
>>> +    .needed = vfio_display_needed,
>>> +    .fields = (VMStateField[]){
>>> +        VMSTATE_STRUCT_POINTER(dpy, VFIOPCIDevice, vfio_display_vmstate, 
>>> VFIODisplay),
>>> +        VMSTATE_END_OF_LIST()
>>> +    }
>>> +};
>>> +
>>>   const VMStateDescription vmstate_vfio_pci_config = {
>>>       .name = "VFIOPCIDevice",
>>>       .version_id = 1,
>>> @@ -2616,6 +2635,10 @@ const VMStateDescription vmstate_vfio_pci_config = {
>>>           VMSTATE_PCI_DEVICE(pdev, VFIOPCIDevice),
>>>           VMSTATE_MSIX_TEST(pdev, VFIOPCIDevice, vfio_msix_present),
>>>           VMSTATE_END_OF_LIST()
>>> +    },
>>> +    .subsections = (const VMStateDescription*[]) {
>>> +        &vmstate_vfio_display,
>>> +        NULL
>>>       }
>>>   };
>>>
>>> @@ -3275,6 +3298,14 @@ static void vfio_realize(PCIDevice *pdev, Error 
>>> **errp)
>>>           if (!vfio_migration_realize(vbasedev, errp)) {
>>>               goto out_deregister;
>>>           }
>>> +        if (vbasedev->enable_migration == ON_OFF_AUTO_OFF) {
>>> +            if (vdev->ramfb_migrate == ON_OFF_AUTO_AUTO) {
>>> +                vdev->ramfb_migrate = ON_OFF_AUTO_OFF;
>>> +            } else if (vdev->ramfb_migrate == ON_OFF_AUTO_ON) {
>>> +                error_setg(errp, "x-ramfb-migrate requires migration");
>>> +                goto out_deregister;
>>> +            }
>>> +        }
>>>       }
>>>
>>>       vfio_register_err_notifier(vdev);
>>> @@ -3484,6 +3515,7 @@ static const TypeInfo vfio_pci_dev_info = {
>>>
>>>   static Property vfio_pci_dev_nohotplug_properties[] = {
>>>       DEFINE_PROP_BOOL("ramfb", VFIOPCIDevice, enable_ramfb, false),
>>> +    DEFINE_PROP_ON_OFF_AUTO("x-ramfb-migrate", VFIOPCIDevice, 
>>> ramfb_migrate, ON_OFF_AUTO_AUTO),
>>>       DEFINE_PROP_END_OF_LIST(),
>>>   };
>>>
>>
>>
> 
>