[Qemu-devel] [PATCH 0/2] hw/vfio/display: add ramfb support

[Qemu-devel] [PATCH 0/2] hw/vfio/display: add ramfb support

[Gerd Hoffmann]

So we have a boot display when using a vgpu as primary display.

Gerd Hoffmann (2):
  stubs: add ramfb
  hw/vfio/display: add ramfb support

 include/hw/vfio/vfio-common.h |  2 ++
 hw/vfio/display.c             | 12 ++++++++++++
 hw/vfio/pci.c                 | 15 +++++++++++++++
 stubs/ramfb.c                 | 13 +++++++++++++
 stubs/Makefile.objs           |  1 +
 5 files changed, 43 insertions(+)
 create mode 100644 stubs/ramfb.c

-- 
2.9.3

[Qemu-devel] [PATCH 1/2] stubs: add ramfb

[Gerd Hoffmann]

Needed to make sure code using ramfb (vfio) compiles properly even on
platforms without fw_cfg (and therefore no ramfb) support.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 stubs/ramfb.c       | 13 +++++++++++++
 stubs/Makefile.objs |  1 +
 2 files changed, 14 insertions(+)
 create mode 100644 stubs/ramfb.c

diff --git a/stubs/ramfb.c b/stubs/ramfb.c
new file mode 100644
index 0000000000..48143f3354
--- /dev/null
+++ b/stubs/ramfb.c
@@ -0,0 +1,13 @@
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/display/ramfb.h"
+
+void ramfb_display_update(QemuConsole *con, RAMFBState *s)
+{
+}
+
+RAMFBState *ramfb_setup(Error **errp)
+{
+    error_setg(errp, "ramfb support not available");
+    return NULL;
+}
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 53d3f32cb2..5dd0aeeec6 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -43,3 +43,4 @@ stub-obj-y += xen-common.o
 stub-obj-y += xen-hvm.o
 stub-obj-y += pci-host-piix.o
 stub-obj-y += ram-block.o
+stub-obj-y += ramfb.o
-- 
2.9.3

[Qemu-devel] [PATCH 2/2] hw/vfio/display: add ramfb support

[Gerd Hoffmann]

So we have a boot display when using a vgpu as primary display.

Use vfio-pci-ramfb instead of vfio-pci to enable it.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/hw/vfio/vfio-common.h |  2 ++
 hw/vfio/display.c             | 12 ++++++++++++
 hw/vfio/pci.c                 | 15 +++++++++++++++
 3 files changed, 29 insertions(+)

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 821def0565..0d85a0a6f8 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -26,6 +26,7 @@
 #include "qemu/queue.h"
 #include "qemu/notify.h"
 #include "ui/console.h"
+#include "hw/display/ramfb.h"
 #ifdef CONFIG_LINUX
 #include <linux/vfio.h>
 #endif
@@ -146,6 +147,7 @@ typedef struct VFIODMABuf {
 
 typedef struct VFIODisplay {
     QemuConsole *con;
+    RAMFBState *ramfb;
     struct {
         VFIORegion buffer;
         DisplaySurface *surface;
diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index 59c0e5d1d7..3901f580c6 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -124,6 +124,9 @@ static void vfio_display_dmabuf_update(void *opaque)
 
     primary = vfio_display_get_dmabuf(vdev, DRM_PLANE_TYPE_PRIMARY);
     if (primary == NULL) {
+        if (dpy->ramfb) {
+            ramfb_display_update(dpy->con, dpy->ramfb);
+        }
         return;
     }
 
@@ -181,6 +184,9 @@ static int vfio_display_dmabuf_init(VFIOPCIDevice *vdev, Error **errp)
     vdev->dpy->con = graphic_console_init(DEVICE(vdev), 0,
                                           &vfio_display_dmabuf_ops,
                                           vdev);
+    if (strcmp(object_get_typename(OBJECT(vdev)), "vfio-pci-ramfb") == 0) {
+        vdev->dpy->ramfb = ramfb_setup(errp);
+    }
     return 0;
 }
 
@@ -228,6 +234,9 @@ static void vfio_display_region_update(void *opaque)
         return;
     }
     if (!plane.drm_format || !plane.size) {
+        if (dpy->ramfb) {
+            ramfb_display_update(dpy->con, dpy->ramfb);
+        }
         return;
     }
     format = qemu_drm_format_to_pixman(plane.drm_format);
@@ -300,6 +309,9 @@ static int vfio_display_region_init(VFIOPCIDevice *vdev, Error **errp)
     vdev->dpy->con = graphic_console_init(DEVICE(vdev), 0,
                                           &vfio_display_region_ops,
                                           vdev);
+    if (strcmp(object_get_typename(OBJECT(vdev)), "vfio-pci-ramfb") == 0) {
+        vdev->dpy->ramfb = ramfb_setup(errp);
+    }
     return 0;
 }
 
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 866f0deeb7..7c0628756e 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3258,9 +3258,24 @@ static const TypeInfo vfio_pci_dev_info = {
     },
 };
 
+static void vfio_pci_ramfb_dev_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->hotpluggable = false;
+}
+
+static const TypeInfo vfio_pci_ramfb_dev_info = {
+    .name = "vfio-pci-ramfb",
+    .parent = "vfio-pci",
+    .instance_size = sizeof(VFIOPCIDevice),
+    .class_init = vfio_pci_ramfb_dev_class_init,
+};
+
 static void register_vfio_pci_dev_type(void)
 {
     type_register_static(&vfio_pci_dev_info);
+    type_register_static(&vfio_pci_ramfb_dev_info);
 }
 
 type_init(register_vfio_pci_dev_type)
-- 
2.9.3

Re: [Qemu-devel] [PATCH 2/2] hw/vfio/display: add ramfb support

[Alex Williamson]

On Mon, 10 Sep 2018 08:43:40 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

> So we have a boot display when using a vgpu as primary display.
> 
> Use vfio-pci-ramfb instead of vfio-pci to enable it.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/hw/vfio/vfio-common.h |  2 ++
>  hw/vfio/display.c             | 12 ++++++++++++
>  hw/vfio/pci.c                 | 15 +++++++++++++++
>  3 files changed, 29 insertions(+)
> 
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 821def0565..0d85a0a6f8 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -26,6 +26,7 @@
>  #include "qemu/queue.h"
>  #include "qemu/notify.h"
>  #include "ui/console.h"
> +#include "hw/display/ramfb.h"
>  #ifdef CONFIG_LINUX
>  #include <linux/vfio.h>
>  #endif
> @@ -146,6 +147,7 @@ typedef struct VFIODMABuf {
>  
>  typedef struct VFIODisplay {
>      QemuConsole *con;
> +    RAMFBState *ramfb;
>      struct {
>          VFIORegion buffer;
>          DisplaySurface *surface;
> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> index 59c0e5d1d7..3901f580c6 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -124,6 +124,9 @@ static void vfio_display_dmabuf_update(void *opaque)
>  
>      primary = vfio_display_get_dmabuf(vdev, DRM_PLANE_TYPE_PRIMARY);
>      if (primary == NULL) {
> +        if (dpy->ramfb) {
> +            ramfb_display_update(dpy->con, dpy->ramfb);
> +        }
>          return;
>      }
>  
> @@ -181,6 +184,9 @@ static int vfio_display_dmabuf_init(VFIOPCIDevice *vdev, Error **errp)
>      vdev->dpy->con = graphic_console_init(DEVICE(vdev), 0,
>                                            &vfio_display_dmabuf_ops,
>                                            vdev);
> +    if (strcmp(object_get_typename(OBJECT(vdev)), "vfio-pci-ramfb") == 0) {
> +        vdev->dpy->ramfb = ramfb_setup(errp);
> +    }
>      return 0;
>  }
>  
> @@ -228,6 +234,9 @@ static void vfio_display_region_update(void *opaque)
>          return;
>      }
>      if (!plane.drm_format || !plane.size) {
> +        if (dpy->ramfb) {
> +            ramfb_display_update(dpy->con, dpy->ramfb);
> +        }
>          return;
>      }
>      format = qemu_drm_format_to_pixman(plane.drm_format);
> @@ -300,6 +309,9 @@ static int vfio_display_region_init(VFIOPCIDevice *vdev, Error **errp)
>      vdev->dpy->con = graphic_console_init(DEVICE(vdev), 0,
>                                            &vfio_display_region_ops,
>                                            vdev);
> +    if (strcmp(object_get_typename(OBJECT(vdev)), "vfio-pci-ramfb") == 0) {
> +        vdev->dpy->ramfb = ramfb_setup(errp);
> +    }
>      return 0;
>  }
>  
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 866f0deeb7..7c0628756e 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3258,9 +3258,24 @@ static const TypeInfo vfio_pci_dev_info = {
>      },
>  };
>  
> +static void vfio_pci_ramfb_dev_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->hotpluggable = false;
> +}
> +
> +static const TypeInfo vfio_pci_ramfb_dev_info = {
> +    .name = "vfio-pci-ramfb",
> +    .parent = "vfio-pci",
> +    .instance_size = sizeof(VFIOPCIDevice),
> +    .class_init = vfio_pci_ramfb_dev_class_init,
> +};
> +
>  static void register_vfio_pci_dev_type(void)
>  {
>      type_register_static(&vfio_pci_dev_info);
> +    type_register_static(&vfio_pci_ramfb_dev_info);
>  }
>  
>  type_init(register_vfio_pci_dev_type)


My concern here is still all of the extra tooling that needs to be
added to management layers above QEMU for this device that exists only
because we can't hotplug the primary display in QEMU.  What happens when
we can hotplug the primary display?  Aren't disabling hotplug of a
vfio-pci device and supporting ramfb two separate things?  I think
we're leaking current implementation issues out to the device options
when really we'd rather have a "ramfb" (or perhaps "console") option on
the vfio-pci device and the hotplug capability determined automatically
and available through introspection of the device.  Thanks,

Alex

Re: [Qemu-devel] [PATCH 2/2] hw/vfio/display: add ramfb support

[Gerd Hoffmann]

  Hi,

> >      type_register_static(&vfio_pci_dev_info);
> > +    type_register_static(&vfio_pci_ramfb_dev_info);

> My concern here is still all of the extra tooling that needs to be
> added to management layers above QEMU for this device that exists only
> because we can't hotplug the primary display in QEMU.  What happens when
> we can hotplug the primary display?

Ramfb uses fw_cfg, and fw_cfg files can't be added or removed at
runtime, the interface simply isn't designed for that.

> Aren't disabling hotplug of a
> vfio-pci device and supporting ramfb two separate things?  I think
> we're leaking current implementation issues out to the device options
> when really we'd rather have a "ramfb" (or perhaps "console") option on
> the vfio-pci device and the hotplug capability determined automatically
> and available through introspection of the device.

Well, I don't think libvirt will have too much trouble handling this.
We have two variants (with and without vga compatibility) of other
devices: qxl-vga and qxl, virtio-vga and virtio-gpu-pci.  libvirt copes
just fine and picks the right one (I think depending on video model
'primary' property).

Also libvirt manages hotpluggability per device *class*, not per device
*instance*.  So a device being hotpluggable or not depending on some
device property is a problem for libvirt ...

I'm open to suggestions how to handle this better, as long as the
libvirt people are on board with the approach.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 2/2] hw/vfio/display: add ramfb support

[Alex Williamson]

On Tue, 11 Sep 2018 06:38:43 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > >      type_register_static(&vfio_pci_dev_info);
> > > +    type_register_static(&vfio_pci_ramfb_dev_info);  
> 
> > My concern here is still all of the extra tooling that needs to be
> > added to management layers above QEMU for this device that exists only
> > because we can't hotplug the primary display in QEMU.  What happens when
> > we can hotplug the primary display?  
> 
> Ramfb uses fw_cfg, and fw_cfg files can't be added or removed at
> runtime, the interface simply isn't designed for that.
> 
> > Aren't disabling hotplug of a
> > vfio-pci device and supporting ramfb two separate things?  I think
> > we're leaking current implementation issues out to the device options
> > when really we'd rather have a "ramfb" (or perhaps "console") option on
> > the vfio-pci device and the hotplug capability determined automatically
> > and available through introspection of the device.  
> 
> Well, I don't think libvirt will have too much trouble handling this.
> We have two variants (with and without vga compatibility) of other
> devices: qxl-vga and qxl, virtio-vga and virtio-gpu-pci.  libvirt copes
> just fine and picks the right one (I think depending on video model
> 'primary' property).
> 
> Also libvirt manages hotpluggability per device *class*, not per device
> *instance*.  So a device being hotpluggable or not depending on some
> device property is a problem for libvirt ...
> 
> I'm open to suggestions how to handle this better, as long as the
> libvirt people are on board with the approach.

Ok, so we need a new class to handle making a device non-hotpluggable,
but I'm still not sure whether we should make:

 -device vfio-pci-ramfb

or

 -device vfio-pci-nohotplug,ramfb=on

Where ramfb would be a property only available on the nohotplug class
variant.  The latter seems to provide a lot more flexibility, but which
is more practical for libvirt?  Thanks,

Alex

Re: [Qemu-devel] [PATCH 2/2] hw/vfio/display: add ramfb support

[Gerd Hoffmann]

  Hi,

> > Also libvirt manages hotpluggability per device *class*, not per device
> > *instance*.  So a device being hotpluggable or not depending on some
> > device property is a problem for libvirt ...
> > 
> > I'm open to suggestions how to handle this better, as long as the
> > libvirt people are on board with the approach.
> 
> Ok, so we need a new class to handle making a device non-hotpluggable,
> but I'm still not sure whether we should make:
> 
>  -device vfio-pci-ramfb
> 
> or
> 
>  -device vfio-pci-nohotplug,ramfb=on
> 
> Where ramfb would be a property only available on the nohotplug class
> variant.

I'm fine with the latter.

> The latter seems to provide a lot more flexibility, but which
> is more practical for libvirt?

Any comment from the libvirt camp?

thanks,
  Gerd

Re: [Qemu-devel] [libvirt] [PATCH 2/2] hw/vfio/display: add ramfb support

[Erik Skultety]

On Fri, Sep 14, 2018 at 12:50:09PM +0200, Gerd Hoffmann wrote:
>   Hi,
>
> > > Also libvirt manages hotpluggability per device *class*, not per device
> > > *instance*.  So a device being hotpluggable or not depending on some
> > > device property is a problem for libvirt ...
> > >
> > > I'm open to suggestions how to handle this better, as long as the
> > > libvirt people are on board with the approach.
> >
> > Ok, so we need a new class to handle making a device non-hotpluggable,
> > but I'm still not sure whether we should make:
> >
> >  -device vfio-pci-ramfb
> >
> > or
> >
> >  -device vfio-pci-nohotplug,ramfb=on
> >
> > Where ramfb would be a property only available on the nohotplug class
> > variant.
>
> I'm fine with the latter.
>
> > The latter seems to provide a lot more flexibility, but which
> > is more practical for libvirt?
>
> Any comment from the libvirt camp?

We had a discussion about this a few months ago [1] where we spoke about
-device vfio-pci-ramfb. However, as Alex has pointed out, the latter proposal
gives us more flexibility in terms of introduction of other device properties
which are unrelated to ramfb but still might require non-hotpluggable device.
Either way, libvirt needs a capability to test whether we should favour this
new device over plain vfio-pci if an mdev with display='on' is required.
What about new device properties (specifically mdev)? In the discussion below,
Gerd noted that apart from the ramfb stuff and the fact that one can be
hotplugged while the latter can not, these are identical (option-wise), is that
to stay, IOW are we going to keep these two device classes in sync when
introducing new vfio-pci device options or are these going to divert more? Is
it even possible? What I mean by that is that I'd like to avoid is a situation
where there are 2 disjunct sets of options which could potentially lead to
problems in decision making in libvirt and we don't like making decisions.

Anyhow, I don't feel like any of the proposals has a strong
advantage/disadvantage in usage for libvirt, both will require a capability and
both would be special cased in our cmdline code depending on the 'display'
attribute. Luckily, we don't have mdev migration yet, so it's good we don't
have to worry about that at this point yet.

[1] https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg03430.html

Erik

Re: [Qemu-devel] [libvirt] [PATCH 2/2] hw/vfio/display: add ramfb support

[Alex Williamson]

On Fri, 14 Sep 2018 16:19:07 +0200
Erik Skultety <eskultet@redhat.com> wrote:

> On Fri, Sep 14, 2018 at 12:50:09PM +0200, Gerd Hoffmann wrote:
> >   Hi,
> >  
> > > > Also libvirt manages hotpluggability per device *class*, not per device
> > > > *instance*.  So a device being hotpluggable or not depending on some
> > > > device property is a problem for libvirt ...
> > > >
> > > > I'm open to suggestions how to handle this better, as long as the
> > > > libvirt people are on board with the approach.  
> > >
> > > Ok, so we need a new class to handle making a device non-hotpluggable,
> > > but I'm still not sure whether we should make:
> > >
> > >  -device vfio-pci-ramfb
> > >
> > > or
> > >
> > >  -device vfio-pci-nohotplug,ramfb=on
> > >
> > > Where ramfb would be a property only available on the nohotplug class
> > > variant.  
> >
> > I'm fine with the latter.
> >  
> > > The latter seems to provide a lot more flexibility, but which
> > > is more practical for libvirt?  
> >
> > Any comment from the libvirt camp?  
> 
> We had a discussion about this a few months ago [1] where we spoke about
> -device vfio-pci-ramfb.

Ah yes, probably my bad for not following up more thoroughly there.

> However, as Alex has pointed out, the latter proposal
> gives us more flexibility in terms of introduction of other device properties
> which are unrelated to ramfb but still might require non-hotpluggable device.
> Either way, libvirt needs a capability to test whether we should favour this
> new device over plain vfio-pci if an mdev with display='on' is required.
> What about new device properties (specifically mdev)? In the discussion below,
> Gerd noted that apart from the ramfb stuff and the fact that one can be
> hotplugged while the latter can not, these are identical (option-wise), is that
> to stay, IOW are we going to keep these two device classes in sync when
> introducing new vfio-pci device options or are these going to divert more? Is
> it even possible? What I mean by that is that I'd like to avoid is a situation
> where there are 2 disjunct sets of options which could potentially lead to
> problems in decision making in libvirt and we don't like making decisions.

The vfio-pci device is the parent of this new device, so it should
automatically inherit any new properties of vfio-pci, it only modifies
the device class for non-hotpluggability and adds properties dependent
on non-hotpluggability.  I'm not sure if libvirt would expose this as a
new model, ie. model="vfio-pci-nohotplug", or if it would be selected
via attribute, ie. nohotplug="on", or perhaps if enabling a property
only found on the nohotplug variant would select it, ie. ramfb="on".
The latter option alone makes it difficult for a user to select it for
any random device, for instance if they're trying to setup a kiosk VM
where they want to prevent even the guest OS admin from changing the VM
configuration.  In any case, it seems that libvirt would never be
enabling this automatically.

> Anyhow, I don't feel like any of the proposals has a strong
> advantage/disadvantage in usage for libvirt, both will require a capability and
> both would be special cased in our cmdline code depending on the 'display'
> attribute. Luckily, we don't have mdev migration yet, so it's good we don't
> have to worry about that at this point yet.

That's a good point that ramfb depends on display, it seems that
regardless of which route we take, using vfio-pci-ramfb or
vfio-pci-nohotplug,ramfb=on, it should fail without a display rather
than simply adding functionality if a display is present or in the
former case, being an obscure way to make a device non-hotpluggable.
Personally I prefer the non-hotplug variant of vfio-pci in hopes that
it provides more flexibility to users and we only need to tackle this
issue once rather than each device we invent with a non-hotplug
dependency.  Thanks,

Alex