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

[Cédric Le Goater]

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)
> >   {
> >   }