Re: [Qemu-devel] [PATCH 3/3] ramfb enhancement

[Marcel Apfelbaum]

Hi Gerd,

On 5/10/19 1:39 PM, Gerd Hoffmann wrote:

On Fri, May 10, 2019 at 12:20:56PM +0300, Marcel Apfelbaum wrote:

Hi Gerd,

On 5/10/19 11:59 AM, Gerd Hoffmann wrote:

Hi,


Got it, thanks. Is a pity ramfb is not a PCI device :), it was worth the
question anyway.

If you look for a simple pci display device check out bochs-display.
It's simliar to stdvga (so ovmf and bochs-drm.ko can drive it just
fine), but without legacy vga emulation, only a linear framebuffer is
supported.  And code size is a fraction of stdvga ...

I actually need the ramfb display in conjunction with kvmgt.

I want to be able to save the VM state to disk, which is actually a kind
of 'live migration' as far as I understand, but live-migration can't work
since
we use device assignment  (vfio-pci-nohotplug device).

vfio live migration is being worked on btw.


I was hoping to be able to hot-unplug/hot-plug the vfio device,
but as the name suggests, can't do so since
the ramfb display uses fw-config to pass the configuration to firmware.

Yes, fw_cfg files can't be hotplugged, that is where this restriction
comes from.


How hard/possible is to make ramfb display a PCI device and move the
configuration from fw-config to PCI configuration space?

Well, the whole point of using ramfb is that it is *not* a pci device,
but something you can attach to other devices as boot display.  Right
now we have that for vfio only, in theory it can likewise be done for
virtio (so you can use virtio-ramfb instead of virtio-vga for bios
display support).  Prototype exists.  Given that OVMF has a full
virtio-gpu driver there isn't much need for that though ...

Piggyback on the pci config space of the device you are attaching ramfb
to isn't going to work very well for unknown devices (i.e. vfio case).
For virtio it would have worked without too much trouble probably, using
a vendor capability to grab some register space.

For a separate pci device you can just use bochs-display.  Maybe add
some logic for the automagic display switching (i.e. if vfio has a valid
framebuffer use that and bochs-display otherwise).


Thanks for the explanation and the pointers!
I'll try to come up with something.

Thanks,
Marcel


cheers,
   Gerd

Re: [Qemu-devel] [PATCH 3/3] ramfb enhancement

[Gerd Hoffmann]

On Fri, May 10, 2019 at 12:20:56PM +0300, Marcel Apfelbaum wrote:
> Hi Gerd,
> 
> On 5/10/19 11:59 AM, Gerd Hoffmann wrote:
> >Hi,
> > 
> > > Got it, thanks. Is a pity ramfb is not a PCI device :), it was worth the
> > > question anyway.
> > If you look for a simple pci display device check out bochs-display.
> > It's simliar to stdvga (so ovmf and bochs-drm.ko can drive it just
> > fine), but without legacy vga emulation, only a linear framebuffer is
> > supported.  And code size is a fraction of stdvga ...
> 
> I actually need the ramfb display in conjunction with kvmgt.
> 
> I want to be able to save the VM state to disk, which is actually a kind
> of 'live migration' as far as I understand, but live-migration can't work
> since
> we use device assignment  (vfio-pci-nohotplug device).

vfio live migration is being worked on btw.

> I was hoping to be able to hot-unplug/hot-plug the vfio device,
> but as the name suggests, can't do so since
> the ramfb display uses fw-config to pass the configuration to firmware.

Yes, fw_cfg files can't be hotplugged, that is where this restriction
comes from.

> How hard/possible is to make ramfb display a PCI device and move the
> configuration from fw-config to PCI configuration space?

Well, the whole point of using ramfb is that it is *not* a pci device,
but something you can attach to other devices as boot display.  Right
now we have that for vfio only, in theory it can likewise be done for
virtio (so you can use virtio-ramfb instead of virtio-vga for bios
display support).  Prototype exists.  Given that OVMF has a full
virtio-gpu driver there isn't much need for that though ...

Piggyback on the pci config space of the device you are attaching ramfb
to isn't going to work very well for unknown devices (i.e. vfio case).
For virtio it would have worked without too much trouble probably, using
a vendor capability to grab some register space.

For a separate pci device you can just use bochs-display.  Maybe add
some logic for the automagic display switching (i.e. if vfio has a valid
framebuffer use that and bochs-display otherwise).

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 3/3] ramfb enhancement

[Marcel Apfelbaum]

Hi Gerd,

On 5/10/19 11:59 AM, Gerd Hoffmann wrote:

   Hi,


Got it, thanks. Is a pity ramfb is not a PCI device :), it was worth the
question anyway.

If you look for a simple pci display device check out bochs-display.
It's simliar to stdvga (so ovmf and bochs-drm.ko can drive it just
fine), but without legacy vga emulation, only a linear framebuffer is
supported.  And code size is a fraction of stdvga ...


I actually need the ramfb display in conjunction with kvmgt.

I want to be able to save the VM state to disk, which is actually a kind
of 'live migration' as far as I understand, but live-migration can't 
work since

we use device assignment  (vfio-pci-nohotplug device).

I was hoping to be able to hot-unplug/hot-plug the vfio device,
but as the name suggests, can't do so since
the ramfb display uses fw-config to pass the configuration to firmware.

How hard/possible is to make ramfb display a PCI device and move the
configuration from fw-config to PCI configuration space?

Thanks,
Marcel


cheers,
   Gerd

Re: [Qemu-devel] [PATCH 3/3] ramfb enhancement

[Gerd Hoffmann]

  Hi,

> Got it, thanks. Is a pity ramfb is not a PCI device :), it was worth the
> question anyway.

If you look for a simple pci display device check out bochs-display.
It's simliar to stdvga (so ovmf and bochs-drm.ko can drive it just
fine), but without legacy vga emulation, only a linear framebuffer is
supported.  And code size is a fraction of stdvga ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 3/3] ramfb enhancement

[Marcel Apfelbaum]

On 5/10/19 5:20 AM, Hou Qiming wrote:

> Please format the commit subject with a prefix and do not use the same
> subject for all the pacthes
> in the series, for this patch it can be something like:

I'll resend the patches with improved title lines after other issues 
are cleared. Thanks for the advice.


> Will this result in a silent failure? So we need to add something to 
the

> log?

Based on my experience with OVMF, the "silent failure" only happens 
when the firmware is malicious. In the current workflow, the only 
failure modes are:
- The firmware does not support ramfb, in which case the patch is 
never reached
- The firmware fails to allocate a big framebuffer, in which case it 
writes log to the serial and hangs / reboots, likely before reaching 
the patch


If you insist, I can add a log. I need to ask though, what is the 
standard way to change something in [PATCH 1/3]? I've never done it 
before and there doesn't seem to be an easy git command for that.


No need for now, I think. Thanks for the explanations.



> It is actually a cool feature, changing the resolution following a
> display window
> resize, but sadly is not stable enough. Let's hope it will be fixed 
someday.


I agree. It's kind of hard to validate everything properly...

Then again, ramfb is not exactly efficient (requiring a full screen 
glTexSubImage2D every frame). After the boot screen, I feel it's 
better to leave such fancy features to GVT / virtio-gl / ...


> Write an initial resolution to the configuration space on guest
reset,
> which a later BIOS / OVMF patch can take advantage of.
>

I like the idea of moving the ramfb configuration to the PCI
configuration space,
do you think is possible to move all the ramfb configuration there
so we
will
not need the fw-config file?
()


I need to clarify that when I say "configuration space", I mean the 
fw-config file. What I did is to initialize that fw-config content to 
the resolution specified on the command line instead of all-zeros.


ramfb is not a PCI device and I don't think it's possible to move its 
configuration there. Even when it's attached to vfio-pci, it's 
technically a separate thing from the guest's POV.




Got it, thanks. Is a pity ramfb is not a PCI device :), it was worth the 
question anyway.

Thanks for info,
Marcel



Is this chunk related to this patch? If not, you may want to split it.


Yes. That last chunk lets the user specify an initial resolution 
without an EDID when ramfb is created as `-device vfio-pci,ramfb=on`. 
It can be useful when debugging GPU passthrough in EFI shell or the 
Windows Recovery thing (which shows up in ramfb).

Qiming

Re: [Qemu-devel] [PATCH 3/3] ramfb enhancement

[Gerd Hoffmann]

> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3080,8 +3080,10 @@ static void vfio_realize(PCIDevice *pdev, Error
> **errp)
>  error_setg(errp, "xres and yres properties require
> display=on");
>  goto out_teardown;
>  }
> -if (vdev->dpy->edid_regs == NULL) {
> -error_setg(errp, "xres and yres properties need edid support");
> +if (vdev->dpy->edid_regs == NULL && !vdev->enable_ramfb) {
> +error_setg(errp,
> +   "xres and yres properties need edid support"
> +   " or ramfb=on");
>  goto out_teardown;
>  }
>  }

I don't think this is useful.  We should continue to allow xres and yres
only in case the vfio device actually has edid support.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 3/3] ramfb enhancement

[Hou Qiming]

> Please format the commit subject with a prefix and do not use the same
> subject for all the pacthes
> in the series, for this patch it can be something like:

I'll resend the patches with improved title lines after other issues are
cleared. Thanks for the advice.

> Will this result in a silent failure? So we need to add something to the
> log?

Based on my experience with OVMF, the "silent failure" only happens when
the firmware is malicious. In the current workflow, the only failure modes
are:
- The firmware does not support ramfb, in which case the patch is never
reached
- The firmware fails to allocate a big framebuffer, in which case it writes
log to the serial and hangs / reboots, likely before reaching the patch

If you insist, I can add a log. I need to ask though, what is the standard
way to change something in [PATCH 1/3]? I've never done it before and there
doesn't seem to be an easy git command for that.

> It is actually a cool feature, changing the resolution following a
> display window
> resize, but sadly is not stable enough. Let's hope it will be fixed
someday.

I agree. It's kind of hard to validate everything properly...

Then again, ramfb is not exactly efficient (requiring a full screen
glTexSubImage2D every frame). After the boot screen, I feel it's better to
leave such fancy features to GVT / virtio-gl / ...

> Write an initial resolution to the configuration space on guest reset,
> > which a later BIOS / OVMF patch can take advantage of.
> >
>
I like the idea of moving the ramfb configuration to the PCI
> configuration space,
> do you think is possible to move all the ramfb configuration there so we
> will
> not need the fw-config file?
> ()
>

I need to clarify that when I say "configuration space", I mean the
fw-config file. What I did is to initialize that fw-config content to the
resolution specified on the command line instead of all-zeros.

ramfb is not a PCI device and I don't think it's possible to move its
configuration there. Even when it's attached to vfio-pci, it's technically
a separate thing from the guest's POV.

Is this chunk related to this patch? If not, you may want to split it.
>

Yes. That last chunk lets the user specify an initial resolution without an
EDID when ramfb is created as `-device vfio-pci,ramfb=on`. It can be useful
when debugging GPU passthrough in EFI shell or the Windows Recovery thing
(which shows up in ramfb).

Qiming

Re: [Qemu-devel] [PATCH 3/3] ramfb enhancement

[Marcel Apfelbaum]

On 5/9/19 10:58 AM, Hou Qiming wrote:


Write an initial resolution to the configuration space on guest reset,
which a later BIOS / OVMF patch can take advantage of.



I like the idea of moving the ramfb configuration to the PCI 
configuration space,
do you think is possible to move all the ramfb configuration there so we 
will

not need the fw-config file?
()

Signed-off-by: HOU Qiming >

---
 hw/display/ramfb-standalone.c | 12 +++-
 hw/display/ramfb.c    | 16 +++-
 hw/vfio/display.c |  4 ++--
 hw/vfio/pci.c |  6 --
 include/hw/display/ramfb.h    |  2 +-
 stubs/ramfb.c |  2 +-
 6 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/hw/display/ramfb-standalone.c b/hw/display/ramfb-standalone.c
index da3229a..6441449 100644
--- a/hw/display/ramfb-standalone.c
+++ b/hw/display/ramfb-standalone.c
@@ -1,6 +1,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "hw/loader.h"
+#include "hw/isa/isa.h"
 #include "hw/display/ramfb.h"
 #include "ui/console.h"
 #include "sysemu/sysemu.h"
@@ -11,6 +12,8 @@ typedef struct RAMFBStandaloneState {
 SysBusDevice parent_obj;
 QemuConsole *con;
 RAMFBState *state;
+    uint32_t xres;
+    uint32_t yres;
 } RAMFBStandaloneState;

 static void display_update_wrapper(void *dev)
@@ -33,15 +36,22 @@ static void ramfb_realizefn(DeviceState *dev, 
Error **errp)

 RAMFBStandaloneState *ramfb = RAMFB(dev);

 ramfb->con = graphic_console_init(dev, 0, _ops, dev);
-    ramfb->state = ramfb_setup(errp);
+    ramfb->state = ramfb_setup(dev, errp);
 }

+static Property ramfb_properties[] = {
+    DEFINE_PROP_UINT32("xres", RAMFBStandaloneState, xres, 0),
+    DEFINE_PROP_UINT32("yres", RAMFBStandaloneState, yres, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void ramfb_class_initfn(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);

 set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
 dc->realize = ramfb_realizefn;
+    dc->props = ramfb_properties;
 dc->desc = "ram framebuffer standalone device";
 dc->user_creatable = true;
 }
diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
index fa6296b..0033ac8 100644
--- a/hw/display/ramfb.c
+++ b/hw/display/ramfb.c
@@ -12,6 +12,7 @@
  */
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qemu/option.h"
 #include "hw/loader.h"
 #include "hw/display/ramfb.h"
 #include "ui/console.h"
@@ -29,6 +30,7 @@ struct QEMU_PACKED RAMFBCfg {
 struct RAMFBState {
 DisplaySurface *ds;
 uint32_t width, height;
+    uint32_t starting_width, starting_height;
 hwaddr addr, length;
 struct RAMFBCfg cfg;
 bool locked;
@@ -120,9 +122,11 @@ static void ramfb_reset(void *opaque)
 RAMFBState *s = (RAMFBState *)opaque;
 s->locked = false;
 memset(>cfg, 0, sizeof(s->cfg));
+    s->cfg.width = s->starting_width;
+    s->cfg.height = s->starting_height;
 }

-RAMFBState *ramfb_setup(Error **errp)
+RAMFBState *ramfb_setup(DeviceState* dev, Error **errp)
 {
 FWCfgState *fw_cfg = fw_cfg_find();
 RAMFBState *s;
@@ -134,6 +138,16 @@ RAMFBState *ramfb_setup(Error **errp)

 s = g_new0(RAMFBState, 1);

+    const char *s_fb_width = qemu_opt_get(dev->opts, "xres");
+    const char *s_fb_height = qemu_opt_get(dev->opts, "yres");
+    if (s_fb_width) {
+    s->cfg.width = atoi(s_fb_width);
+    s->starting_width = s->cfg.width;
+    }
+    if (s_fb_height) {
+    s->cfg.height = atoi(s_fb_height);
+    s->starting_height = s->cfg.height;
+    }
 s->locked = false;

 rom_add_vga("vgabios-ramfb.bin");
diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index a3d9c8f..2c2d3e5 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -352,7 +352,7 @@ static int vfio_display_dmabuf_init(VFIOPCIDevice 
*vdev, Error **errp)

_display_dmabuf_ops,
   vdev);
 if (vdev->enable_ramfb) {
-    vdev->dpy->ramfb = ramfb_setup(errp);
+    vdev->dpy->ramfb = ramfb_setup(DEVICE(vdev), errp);
 }
 vfio_display_edid_init(vdev);
 return 0;
@@ -478,7 +478,7 @@ static int vfio_display_region_init(VFIOPCIDevice 
*vdev, Error **errp)

_display_region_ops,
   vdev);
 if (vdev->enable_ramfb) {
-    vdev->dpy->ramfb = ramfb_setup(errp);
+    vdev->dpy->ramfb = ramfb_setup(DEVICE(vdev), errp);
 }
 return 0;
 }
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 8cecb53..5d64daa 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3080,8 +3080,10 @@ static void vfio_realize(PCIDevice *pdev, Error 
**errp)
 error_setg(errp, "xres and yres properties require 
display=on");

 goto out_teardown;
 }
-    if (vdev->dpy->edid_regs == NULL) {
-    error_setg(errp, "xres and yres properties need edid 
support");

+    if (vdev->dpy->edid_regs == NULL && !vdev->enable_ramfb) {
+