From 48d1f092a7960d711fb2c77ab8d3f9d0a0ca0d5c Mon Sep 17 00:00:00 2001
From: HQM <hqm03ster@gmail.com>
Date: Mon, 6 May 2019 15:37:59 +0800
Subject: [PATCH] Precautionary glBindTexture and surface->texture validation
 in surface_gl_update_texture

In a GVT-g setup with dmabuf and GTK GUI, the current 2D texture at
surface_gl_update_texture is not necessarily
surface->texture. Adding a glBindTexture fixes related crashes and
artifacts, and is generally more secure.

Signed-off-by: HQM <hqm03ster@gmail.com>
---
 ui/console-gl.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/ui/console-gl.c b/ui/console-gl.c
index a56e1cd..c1cb3bd 100644
--- a/ui/console-gl.c
+++ b/ui/console-gl.c
@@ -92,13 +92,17 @@ void surface_gl_update_texture(QemuGLShader *gls,

     assert(gls);

-    glPixelStorei(GL_UNPACK_ROW_LENGTH_EXT,
-                  surface_stride(surface) /
surface_bytes_per_pixel(surface));
-    glTexSubImage2D(GL_TEXTURE_2D, 0,
-                    x, y, w, h,
-                    surface->glformat, surface->gltype,
-                    data + surface_stride(surface) * y
-                    + surface_bytes_per_pixel(surface) * x);
+    if (surface->texture) {
+        glBindTexture(GL_TEXTURE_2D, surface->texture);
+        glPixelStorei(GL_UNPACK_ROW_LENGTH_EXT,
+                      surface_stride(surface)
+                      / surface_bytes_per_pixel(surface));
+        glTexSubImage2D(GL_TEXTURE_2D, 0,
+                        x, y, w, h,
+                        surface->glformat, surface->gltype,
+                        data + surface_stride(surface) * y
+                        + surface_bytes_per_pixel(surface) * x);
+    }
 }

 void surface_gl_render_texture(QemuGLShader *gls,
-- 
2.17.1

Hi Qiming,
Thanks for submitting the patch!

On 5/6/19 10:50 AM, Hou Qiming wrote:
> From 48d1f092a7960d711fb2c77ab8d3f9d0a0ca0d5c Mon Sep 17 00:00:00 2001
> From: HQM <hqm03ster@gmail.com <mailto:hqm03ster@gmail.com>>
> Date: Mon, 6 May 2019 15:37:59 +0800
> Subject: [PATCH] Precautionary glBindTexture and surface->texture 
> validation
>  in surface_gl_update_texture
>

The lines above should not go into the patch comment, while the mail 
subject should
start with [PATCH].

I suggest preparing the patch with git format-patch and sending it with 
git send-email.
You can also prepare it "manually" as long the format is correct.

> In a GVT-g setup with dmabuf and GTK GUI, the current 2D texture at 
> surface_gl_update_texture is not necessarily
> surface->texture. Adding a glBindTexture fixes related crashes and 
> artifacts, and is generally more secure.
>
> Signed-off-by: HQM <hqm03ster@gmail.com <mailto:hqm03ster@gmail.com>>
> ---
>  ui/console-gl.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/ui/console-gl.c b/ui/console-gl.c
> index a56e1cd..c1cb3bd 100644
> --- a/ui/console-gl.c
> +++ b/ui/console-gl.c
> @@ -92,13 +92,17 @@ void surface_gl_update_texture(QemuGLShader *gls,
>
>      assert(gls);
>
> -    glPixelStorei(GL_UNPACK_ROW_LENGTH_EXT,
> -                  surface_stride(surface) / 
> surface_bytes_per_pixel(surface));
> -    glTexSubImage2D(GL_TEXTURE_2D, 0,
> -                    x, y, w, h,
> -                    surface->glformat, surface->gltype,
> -                    data + surface_stride(surface) * y
> -                    + surface_bytes_per_pixel(surface) * x);
> +    if (surface->texture) {

I confirm it fixes a boot QEMU crash when the Windows guest i915 driver 
loads.

> +        glBindTexture(GL_TEXTURE_2D, surface->texture);

I confirm it fixes strange artifacts seen on screen (some huge mouse 
icon on the upper left side)
when guest monitor "turns off" or the GTK window gets resized and the 
guest desktop resolution changes.

> +        glPixelStorei(GL_UNPACK_ROW_LENGTH_EXT,
> +                      surface_stride(surface)
> +                      / surface_bytes_per_pixel(surface));
> +        glTexSubImage2D(GL_TEXTURE_2D, 0,
> +                        x, y, w, h,
> +                        surface->glformat, surface->gltype,
> +                        data + surface_stride(surface) * y
> +                        + surface_bytes_per_pixel(surface) * x);
> +    }
>  }
>
>  void surface_gl_render_texture(QemuGLShader *gls,
> -- 
> 2.17.1
>
>

I have no OpenGL background to understand the consequences, but the patch
does solve 2 gvt issues, so:

Tested-by: Marcel Apfelbaum<marcel.apfelbaum@gmail.com>


Thanks,
Marcel

Pulled back the `qemu_create_displaysurface_guestmem` function to create
the display surface so that the guest memory gets properly unmaped.

Only allow one resolution change per guest boot, which prevents a
crash when the guest writes garbage to the configuration space (e.g.
when rebooting).

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

Signed-off-by: HOU Qiming <hqm03ster@gmail.com>
---
 hw/display/ramfb-standalone.c | 12 ++++-
 hw/display/ramfb.c            | 91 +++++++++++++++++++++++++++++------
 hw/vfio/display.c             |  4 +-
 hw/vfio/pci.c                 |  6 ++-
 include/hw/display/ramfb.h    |  2 +-
 stubs/ramfb.c                 |  2 +-
 6 files changed, 96 insertions(+), 21 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, &wrapper_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 25c8ad7..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,18 +30,57 @@ 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;
 };

+static void qemu_unmap_displaysurface_guestmem(pixman_image_t *image,
+                                               void *unused)
+{
+    void *data = pixman_image_get_data(image);
+    uint32_t size = pixman_image_get_stride(image) *
+        pixman_image_get_height(image);
+    cpu_physical_memory_unmap(data, size, 0, 0);
+}
+
+static DisplaySurface *qemu_create_displaysurface_guestmem(
+    int width, int height,
+    pixman_format_code_t format,
+    int linesize, uint64_t addr)
+{
+    DisplaySurface *surface;
+    hwaddr size;
+    void *data;
+
+    if (linesize == 0) {
+        linesize = width * PIXMAN_FORMAT_BPP(format) / 8;
+    }
+
+    size = (hwaddr)linesize * height;
+    data = cpu_physical_memory_map(addr, &size, 0);
+    if (size != (hwaddr)linesize * height) {
+        cpu_physical_memory_unmap(data, size, 0, 0);
+        return NULL;
+    }
+
+    surface = qemu_create_displaysurface_from
+        (width, height, format, linesize, data);
+    pixman_image_set_destroy_function
+        (surface->image, qemu_unmap_displaysurface_guestmem, NULL);
+
+    return surface;
+}
+
 static void ramfb_fw_cfg_write(void *dev, off_t offset, size_t len)
 {
     RAMFBState *s = dev;
-    void *framebuffer;
-    uint32_t fourcc, format;
+    uint32_t fourcc, format, width, height;
     hwaddr stride, addr, length;

-    s->width  = be32_to_cpu(s->cfg.width);
-    s->height = be32_to_cpu(s->cfg.height);
+    width     = be32_to_cpu(s->cfg.width);
+    height    = be32_to_cpu(s->cfg.height);
     stride    = be32_to_cpu(s->cfg.stride);
     fourcc    = be32_to_cpu(s->cfg.fourcc);
     addr      = be64_to_cpu(s->cfg.addr);
@@ -48,17 +88,18 @@ static void ramfb_fw_cfg_write(void *dev, off_t offset,
size_t len)
     format    = qemu_drm_format_to_pixman(fourcc);

     fprintf(stderr, "%s: %dx%d @ 0x%" PRIx64 "\n", __func__,
-            s->width, s->height, addr);
-    framebuffer = address_space_map(&address_space_memory,
-                                    addr, &length, false,
-                                    MEMTXATTRS_UNSPECIFIED);
-    if (!framebuffer || length < stride * s->height) {
-        s->width = 0;
-        s->height = 0;
+            width, height, addr);
+    if (s->locked) {
+        fprintf(stderr, "%s: resolution locked, change rejected\n",
__func__);
         return;
     }
-    s->ds = qemu_create_displaysurface_from(s->width, s->height,
-                                            format, stride, framebuffer);
+    s->locked = true;
+    s->addr = addr;
+    s->length = length;
+    s->width = width;
+    s->height = height;
+    s->ds = qemu_create_displaysurface_guestmem(s->width, s->height,
+                                                format, stride, s->addr);
 }

 void ramfb_display_update(QemuConsole *con, RAMFBState *s)
@@ -76,7 +117,16 @@ void ramfb_display_update(QemuConsole *con, RAMFBState
*s)
     dpy_gfx_update_full(con);
 }

-RAMFBState *ramfb_setup(Error **errp)
+static void ramfb_reset(void *opaque)
+{
+    RAMFBState *s = (RAMFBState *)opaque;
+    s->locked = false;
+    memset(&s->cfg, 0, sizeof(s->cfg));
+    s->cfg.width = s->starting_width;
+    s->cfg.height = s->starting_height;
+}
+
+RAMFBState *ramfb_setup(DeviceState* dev, Error **errp)
 {
     FWCfgState *fw_cfg = fw_cfg_find();
     RAMFBState *s;
@@ -88,9 +138,22 @@ 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");
     fw_cfg_add_file_callback(fw_cfg, "etc/ramfb",
                              NULL, ramfb_fw_cfg_write, s,
                              &s->cfg, sizeof(s->cfg), false);
+    qemu_register_reset(ramfb_reset, s);
     return s;
 }
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)
                                           &vfio_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)
                                           &vfio_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) {
+            error_setg(errp,
+                       "xres and yres properties need edid support"
+                       " or ramfb=on");
             goto out_teardown;
         }
     }
diff --git a/include/hw/display/ramfb.h b/include/hw/display/ramfb.h
index b33a2c4..f6c2de9 100644
--- a/include/hw/display/ramfb.h
+++ b/include/hw/display/ramfb.h
@@ -4,7 +4,7 @@
 /* ramfb.c */
 typedef struct RAMFBState RAMFBState;
 void ramfb_display_update(QemuConsole *con, RAMFBState *s);
-RAMFBState *ramfb_setup(Error **errp);
+RAMFBState *ramfb_setup(DeviceState *dev, Error **errp);

 /* ramfb-standalone.c */
 #define TYPE_RAMFB_DEVICE "ramfb"
diff --git a/stubs/ramfb.c b/stubs/ramfb.c
index 48143f3..0799093 100644
--- a/stubs/ramfb.c
+++ b/stubs/ramfb.c
@@ -6,7 +6,7 @@ void ramfb_display_update(QemuConsole *con, RAMFBState *s)
 {
 }

-RAMFBState *ramfb_setup(Error **errp)
+RAMFBState *ramfb_setup(DeviceState* dev, Error **errp)
 {
     error_setg(errp, "ramfb support not available");
     return NULL;
-- 
2.17.1

On Thu, May 09, 2019 at 08:15:44AM +0800, Hou Qiming wrote:
> Pulled back the `qemu_create_displaysurface_guestmem` function to create
> the display surface so that the guest memory gets properly unmaped.
> 
> Only allow one resolution change per guest boot, which prevents a
> crash when the guest writes garbage to the configuration space (e.g.
> when rebooting).
> 
> Write an initial resolution to the configuration space on guest reset,
> which a later BIOS / OVMF patch can take advantage of.

Looks all sensible, but can you split this into one patch per change
please?

thanks,
  Gerd

Done. Will resend the split patches.

On Thu, May 9, 2019 at 2:48 PM Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Thu, May 09, 2019 at 08:15:44AM +0800, Hou Qiming wrote:
> > Pulled back the `qemu_create_displaysurface_guestmem` function to create
> > the display surface so that the guest memory gets properly unmaped.
> >
> > Only allow one resolution change per guest boot, which prevents a
> > crash when the guest writes garbage to the configuration space (e.g.
> > when rebooting).
> >
> > Write an initial resolution to the configuration space on guest reset,
> > which a later BIOS / OVMF patch can take advantage of.
>
> Looks all sensible, but can you split this into one patch per change
> please?
>
> thanks,
>   Gerd
>
>

Pulled back the `qemu_create_displaysurface_guestmem` function to create
the display surface so that the guest memory gets properly unmaped.

Signed-off-by: HOU Qiming <hqm03ster@gmail.com>
---
 hw/display/ramfb.c | 53 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 42 insertions(+), 11 deletions(-)

diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
index 25c8ad7..c27fcc7 100644
--- a/hw/display/ramfb.c
+++ b/hw/display/ramfb.c
@@ -29,13 +29,50 @@ struct QEMU_PACKED RAMFBCfg {
 struct RAMFBState {
     DisplaySurface *ds;
     uint32_t width, height;
+    hwaddr addr, length;
     struct RAMFBCfg cfg;
 };

+static void qemu_unmap_displaysurface_guestmem(pixman_image_t *image,
+                                               void *unused)
+{
+    void *data = pixman_image_get_data(image);
+    uint32_t size = pixman_image_get_stride(image) *
+        pixman_image_get_height(image);
+    cpu_physical_memory_unmap(data, size, 0, 0);
+}
+
+static DisplaySurface *qemu_create_displaysurface_guestmem(
+    int width, int height,
+    pixman_format_code_t format,
+    int linesize, uint64_t addr)
+{
+    DisplaySurface *surface;
+    hwaddr size;
+    void *data;
+
+    if (linesize == 0) {
+        linesize = width * PIXMAN_FORMAT_BPP(format) / 8;
+    }
+
+    size = (hwaddr)linesize * height;
+    data = cpu_physical_memory_map(addr, &size, 0);
+    if (size != (hwaddr)linesize * height) {
+        cpu_physical_memory_unmap(data, size, 0, 0);
+        return NULL;
+    }
+
+    surface = qemu_create_displaysurface_from
+        (width, height, format, linesize, data);
+    pixman_image_set_destroy_function
+        (surface->image, qemu_unmap_displaysurface_guestmem, NULL);
+
+    return surface;
+}
+
 static void ramfb_fw_cfg_write(void *dev, off_t offset, size_t len)
 {
     RAMFBState *s = dev;
-    void *framebuffer;
     uint32_t fourcc, format;
     hwaddr stride, addr, length;

@@ -49,16 +86,10 @@ static void ramfb_fw_cfg_write(void *dev, off_t offset,
size_t len)

     fprintf(stderr, "%s: %dx%d @ 0x%" PRIx64 "\n", __func__,
             s->width, s->height, addr);
-    framebuffer = address_space_map(&address_space_memory,
-                                    addr, &length, false,
-                                    MEMTXATTRS_UNSPECIFIED);
-    if (!framebuffer || length < stride * s->height) {
-        s->width = 0;
-        s->height = 0;
-        return;
-    }
-    s->ds = qemu_create_displaysurface_from(s->width, s->height,
-                                            format, stride, framebuffer);
+    s->addr = addr;
+    s->length = length;
+    s->ds = qemu_create_displaysurface_guestmem(s->width, s->height,
+                                                format, stride, s->addr);
 }

 void ramfb_display_update(QemuConsole *con, RAMFBState *s)
-- 
2.17.1

Hi Qiming,

On 5/9/19 10:57 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:

Next version should also have a V2 in the prefix, for this patch it can 
look something like:
    [PATCH v2 1/3] hw/display/ramfb: fix guest memory un-mapping


> Pulled back the `qemu_create_displaysurface_guestmem` function to create
> the display surface so that the guest memory gets properly unmaped.
>
> Signed-off-by: HOU Qiming <hqm03ster@gmail.com 
> <mailto:hqm03ster@gmail.com>>
> ---
>  hw/display/ramfb.c | 53 ++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 42 insertions(+), 11 deletions(-)
>
> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
> index 25c8ad7..c27fcc7 100644
> --- a/hw/display/ramfb.c
> +++ b/hw/display/ramfb.c
> @@ -29,13 +29,50 @@ struct QEMU_PACKED RAMFBCfg {
>  struct RAMFBState {
>      DisplaySurface *ds;
>      uint32_t width, height;
> +    hwaddr addr, length;
>      struct RAMFBCfg cfg;
>  };
>
> +static void qemu_unmap_displaysurface_guestmem(pixman_image_t *image,
> +                                               void *unused)
> +{
> +    void *data = pixman_image_get_data(image);
> +    uint32_t size = pixman_image_get_stride(image) *
> +        pixman_image_get_height(image);
> +    cpu_physical_memory_unmap(data, size, 0, 0);
> +}
> +
> +static DisplaySurface *qemu_create_displaysurface_guestmem(
> +    int width, int height,
> +    pixman_format_code_t format,
> +    int linesize, uint64_t addr)
> +{
> +    DisplaySurface *surface;
> +    hwaddr size;
> +    void *data;
> +
> +    if (linesize == 0) {
> +        linesize = width * PIXMAN_FORMAT_BPP(format) / 8;
> +    }
> +
> +    size = (hwaddr)linesize * height;
> +    data = cpu_physical_memory_map(addr, &size, 0);
> +    if (size != (hwaddr)linesize * height) {
> +        cpu_physical_memory_unmap(data, size, 0, 0);
> +        return NULL;

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

Thanks,
Marcel

> +    }
> +
> +    surface = qemu_create_displaysurface_from
> +        (width, height, format, linesize, data);
> +    pixman_image_set_destroy_function
> +        (surface->image, qemu_unmap_displaysurface_guestmem, NULL);
> +
> +    return surface;
> +}
> +
>  static void ramfb_fw_cfg_write(void *dev, off_t offset, size_t len)
>  {
>      RAMFBState *s = dev;
> -    void *framebuffer;
>      uint32_t fourcc, format;
>      hwaddr stride, addr, length;
>
> @@ -49,16 +86,10 @@ static void ramfb_fw_cfg_write(void *dev, off_t 
> offset, size_t len)
>
>      fprintf(stderr, "%s: %dx%d @ 0x%" PRIx64 "\n", __func__,
>              s->width, s->height, addr);
> -    framebuffer = address_space_map(&address_space_memory,
> -                                    addr, &length, false,
> -                                    MEMTXATTRS_UNSPECIFIED);
> -    if (!framebuffer || length < stride * s->height) {
> -        s->width = 0;
> -        s->height = 0;
> -        return;
> -    }
> -    s->ds = qemu_create_displaysurface_from(s->width, s->height,
> -                                            format, stride, framebuffer);
> +    s->addr = addr;
> +    s->length = length;
> +    s->ds = qemu_create_displaysurface_guestmem(s->width, s->height,
> +                                                format, stride, s->addr);
>  }
>
>  void ramfb_display_update(QemuConsole *con, RAMFBState *s)
> -- 
> 2.17.1
>
>

On Thu, May 09, 2019 at 03:57:24PM +0800, Hou Qiming wrote:
> Pulled back the `qemu_create_displaysurface_guestmem` function to create
> the display surface so that the guest memory gets properly unmaped.
> 
> Signed-off-by: HOU Qiming <hqm03ster@gmail.com>
> ---
>  hw/display/ramfb.c | 53 ++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 42 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
> index 25c8ad7..c27fcc7 100644
> --- a/hw/display/ramfb.c
> +++ b/hw/display/ramfb.c
> @@ -29,13 +29,50 @@ struct QEMU_PACKED RAMFBCfg {
>  struct RAMFBState {
>      DisplaySurface *ds;
>      uint32_t width, height;
> +    hwaddr addr, length;

Why do you add these?  Seem not to be used anywhere in the patch ...

Also a more descriptive subject line would be good.

cheers,
  Gerd

Only allow one resolution change per guest boot, which prevents a
crash when the guest writes garbage to the configuration space (e.g.
when rebooting).

Signed-off-by: HOU Qiming <hqm03ster@gmail.com>
---
 hw/display/ramfb.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
index c27fcc7..fa6296b 100644
--- a/hw/display/ramfb.c
+++ b/hw/display/ramfb.c
@@ -31,6 +31,7 @@ struct RAMFBState {
     uint32_t width, height;
     hwaddr addr, length;
     struct RAMFBCfg cfg;
+    bool locked;
 };

 static void qemu_unmap_displaysurface_guestmem(pixman_image_t *image,
@@ -73,11 +74,11 @@ static DisplaySurface
*qemu_create_displaysurface_guestmem(
 static void ramfb_fw_cfg_write(void *dev, off_t offset, size_t len)
 {
     RAMFBState *s = dev;
-    uint32_t fourcc, format;
+    uint32_t fourcc, format, width, height;
     hwaddr stride, addr, length;

-    s->width  = be32_to_cpu(s->cfg.width);
-    s->height = be32_to_cpu(s->cfg.height);
+    width     = be32_to_cpu(s->cfg.width);
+    height    = be32_to_cpu(s->cfg.height);
     stride    = be32_to_cpu(s->cfg.stride);
     fourcc    = be32_to_cpu(s->cfg.fourcc);
     addr      = be64_to_cpu(s->cfg.addr);
@@ -85,9 +86,16 @@ static void ramfb_fw_cfg_write(void *dev, off_t offset,
size_t len)
     format    = qemu_drm_format_to_pixman(fourcc);

     fprintf(stderr, "%s: %dx%d @ 0x%" PRIx64 "\n", __func__,
-            s->width, s->height, addr);
+            width, height, addr);
+    if (s->locked) {
+        fprintf(stderr, "%s: resolution locked, change rejected\n",
__func__);
+        return;
+    }
+    s->locked = true;
     s->addr = addr;
     s->length = length;
+    s->width = width;
+    s->height = height;
     s->ds = qemu_create_displaysurface_guestmem(s->width, s->height,
                                                 format, stride, s->addr);
 }
@@ -107,6 +115,13 @@ void ramfb_display_update(QemuConsole *con, RAMFBState
*s)
     dpy_gfx_update_full(con);
 }

+static void ramfb_reset(void *opaque)
+{
+    RAMFBState *s = (RAMFBState *)opaque;
+    s->locked = false;
+    memset(&s->cfg, 0, sizeof(s->cfg));
+}
+
 RAMFBState *ramfb_setup(Error **errp)
 {
     FWCfgState *fw_cfg = fw_cfg_find();
@@ -119,9 +134,12 @@ RAMFBState *ramfb_setup(Error **errp)

     s = g_new0(RAMFBState, 1);

+    s->locked = false;
+
     rom_add_vga("vgabios-ramfb.bin");
     fw_cfg_add_file_callback(fw_cfg, "etc/ramfb",
                              NULL, ramfb_fw_cfg_write, s,
                              &s->cfg, sizeof(s->cfg), false);
+    qemu_register_reset(ramfb_reset, s);
     return s;
 }
-- 
2.17.1

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

The subject for this patch may be:

     [PATCH V2 2/3] hw/display/ramfb: don't allow resolution change 
until vm reset

> Only allow one resolution change per guest boot, which prevents a
> crash when the guest writes garbage to the configuration space (e.g.
> when rebooting).
>

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.

> Signed-off-by: HOU Qiming <hqm03ster@gmail.com 
> <mailto:hqm03ster@gmail.com>>
> ---
>  hw/display/ramfb.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
> index c27fcc7..fa6296b 100644
> --- a/hw/display/ramfb.c
> +++ b/hw/display/ramfb.c
> @@ -31,6 +31,7 @@ struct RAMFBState {
>      uint32_t width, height;
>      hwaddr addr, length;
>      struct RAMFBCfg cfg;
> +    bool locked;
>  };
>
>  static void qemu_unmap_displaysurface_guestmem(pixman_image_t *image,
> @@ -73,11 +74,11 @@ static DisplaySurface 
> *qemu_create_displaysurface_guestmem(
>  static void ramfb_fw_cfg_write(void *dev, off_t offset, size_t len)
>  {
>      RAMFBState *s = dev;
> -    uint32_t fourcc, format;
> +    uint32_t fourcc, format, width, height;
>      hwaddr stride, addr, length;
>
> -    s->width  = be32_to_cpu(s->cfg.width);
> -    s->height = be32_to_cpu(s->cfg.height);
> +    width     = be32_to_cpu(s->cfg.width);
> +    height    = be32_to_cpu(s->cfg.height);
>      stride    = be32_to_cpu(s->cfg.stride);
>      fourcc    = be32_to_cpu(s->cfg.fourcc);
>      addr      = be64_to_cpu(s->cfg.addr);
> @@ -85,9 +86,16 @@ static void ramfb_fw_cfg_write(void *dev, off_t 
> offset, size_t len)
>      format    = qemu_drm_format_to_pixman(fourcc);
>
>      fprintf(stderr, "%s: %dx%d @ 0x%" PRIx64 "\n", __func__,
> -            s->width, s->height, addr);
> +            width, height, addr);
> +    if (s->locked) {
> +        fprintf(stderr, "%s: resolution locked, change rejected\n", 
> __func__);
> +        return;
> +    }
> +    s->locked = true;
>      s->addr = addr;
>      s->length = length;
> +    s->width = width;
> +    s->height = height;
>      s->ds = qemu_create_displaysurface_guestmem(s->width, s->height,
>                                                  format, stride, s->addr);
>  }
> @@ -107,6 +115,13 @@ void ramfb_display_update(QemuConsole *con, 
> RAMFBState *s)
>      dpy_gfx_update_full(con);
>  }
>
> +static void ramfb_reset(void *opaque)
> +{
> +    RAMFBState *s = (RAMFBState *)opaque;
> +    s->locked = false;
> +    memset(&s->cfg, 0, sizeof(s->cfg));
> +}
> +
>  RAMFBState *ramfb_setup(Error **errp)
>  {
>      FWCfgState *fw_cfg = fw_cfg_find();
> @@ -119,9 +134,12 @@ RAMFBState *ramfb_setup(Error **errp)
>
>      s = g_new0(RAMFBState, 1);
>
> +    s->locked = false;
> +
>      rom_add_vga("vgabios-ramfb.bin");
>      fw_cfg_add_file_callback(fw_cfg, "etc/ramfb",
>                               NULL, ramfb_fw_cfg_write, s,
>                               &s->cfg, sizeof(s->cfg), false);
> +    qemu_register_reset(ramfb_reset, s);
>      return s;
>  }
> -- 
> 2.17.1
>
>

On Thu, May 09, 2019 at 03:58:02PM +0800, Hou Qiming wrote:
> Only allow one resolution change per guest boot, which prevents a
> crash when the guest writes garbage to the configuration space (e.g.
> when rebooting).

Hmm?  Did you see that happen in practice?
It is not easy to write to fw_cfg by accident ...

> 
> Signed-off-by: HOU Qiming <hqm03ster@gmail.com>
> ---
>  hw/display/ramfb.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
> index c27fcc7..fa6296b 100644
> --- a/hw/display/ramfb.c
> +++ b/hw/display/ramfb.c
> @@ -31,6 +31,7 @@ struct RAMFBState {
>      uint32_t width, height;
>      hwaddr addr, length;
>      struct RAMFBCfg cfg;
> +    bool locked;
>  };
> 
>  static void qemu_unmap_displaysurface_guestmem(pixman_image_t *image,
> @@ -73,11 +74,11 @@ static DisplaySurface
> *qemu_create_displaysurface_guestmem(
>  static void ramfb_fw_cfg_write(void *dev, off_t offset, size_t len)
>  {
>      RAMFBState *s = dev;
> -    uint32_t fourcc, format;
> +    uint32_t fourcc, format, width, height;
>      hwaddr stride, addr, length;
> 
> -    s->width  = be32_to_cpu(s->cfg.width);
> -    s->height = be32_to_cpu(s->cfg.height);
> +    width     = be32_to_cpu(s->cfg.width);
> +    height    = be32_to_cpu(s->cfg.height);
>      stride    = be32_to_cpu(s->cfg.stride);
>      fourcc    = be32_to_cpu(s->cfg.fourcc);
>      addr      = be64_to_cpu(s->cfg.addr);
> @@ -85,9 +86,16 @@ static void ramfb_fw_cfg_write(void *dev, off_t offset,
> size_t len)
>      format    = qemu_drm_format_to_pixman(fourcc);
> 
>      fprintf(stderr, "%s: %dx%d @ 0x%" PRIx64 "\n", __func__,
> -            s->width, s->height, addr);
> +            width, height, addr);
> +    if (s->locked) {
> +        fprintf(stderr, "%s: resolution locked, change rejected\n",
> __func__);
> +        return;
> +    }
> +    s->locked = true;
>      s->addr = addr;
>      s->length = length;
> +    s->width = width;
> +    s->height = height;
>      s->ds = qemu_create_displaysurface_guestmem(s->width, s->height,
>                                                  format, stride, s->addr);
>  }
> @@ -107,6 +115,13 @@ void ramfb_display_update(QemuConsole *con, RAMFBState
> *s)
>      dpy_gfx_update_full(con);
>  }
> 
> +static void ramfb_reset(void *opaque)
> +{
> +    RAMFBState *s = (RAMFBState *)opaque;
> +    s->locked = false;
> +    memset(&s->cfg, 0, sizeof(s->cfg));
> +}
> +
>  RAMFBState *ramfb_setup(Error **errp)
>  {
>      FWCfgState *fw_cfg = fw_cfg_find();
> @@ -119,9 +134,12 @@ RAMFBState *ramfb_setup(Error **errp)
> 
>      s = g_new0(RAMFBState, 1);
> 
> +    s->locked = false;
> +
>      rom_add_vga("vgabios-ramfb.bin");
>      fw_cfg_add_file_callback(fw_cfg, "etc/ramfb",
>                               NULL, ramfb_fw_cfg_write, s,
>                               &s->cfg, sizeof(s->cfg), false);
> +    qemu_register_reset(ramfb_reset, s);
>      return s;
>  }
> -- 
> 2.17.1

> Only allow one resolution change per guest boot, which prevents a

> > crash when the guest writes garbage to the configuration space (e.g.
> > when rebooting).
>
> Hmm?  Did you see that happen in practice?
> It is not easy to write to fw_cfg by accident ...
>
>
Yes, this does happen in practice. It's observed in KVMGT setups by another
github user and me, when the guest Intel driver loads or when the guest
reboots. Link:
https://github.com/intel/gvt-linux/issues/23#issuecomment-483651476

Now that you mentioned it, I start to feel that it's not accidental. A
closer look at the "garbage" in that post shows that the overwriting
content are valid resolution values in the wrong endian. It could be a
misguided attempt to "resize ramfb" by the guest Intel driver.

-----

I'll fix the addr / length thing and remove the test part in vfio-pci in V2.

Qiming

On Fri, May 10, 2019 at 02:41:36PM +0800, Hou Qiming wrote:
> > Only allow one resolution change per guest boot, which prevents a
> 
> > > crash when the guest writes garbage to the configuration space (e.g.
> > > when rebooting).
> >
> > Hmm?  Did you see that happen in practice?
> > It is not easy to write to fw_cfg by accident ...
> >
> >
> Yes, this does happen in practice. It's observed in KVMGT setups by another
> github user and me, when the guest Intel driver loads or when the guest
> reboots. Link:
> https://github.com/intel/gvt-linux/issues/23#issuecomment-483651476
> 
> Now that you mentioned it, I start to feel that it's not accidental. A
> closer look at the "garbage" in that post shows that the overwriting
> content are valid resolution values in the wrong endian. It could be a
> misguided attempt to "resize ramfb" by the guest Intel driver.

Hmm.  The intel driver certainly isn't supposed to do that ...

So, allow writing only once might be a good idea, to make clear this
*really* is meant to be used by the firmware only, for a boot display.

cheers,
  Gerd

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

Signed-off-by: HOU Qiming <hqm03ster@gmail.com>
---
 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, &wrapper_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(&s->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)
                                           &vfio_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)
                                           &vfio_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) {
+            error_setg(errp,
+                       "xres and yres properties need edid support"
+                       " or ramfb=on");
             goto out_teardown;
         }
     }
diff --git a/include/hw/display/ramfb.h b/include/hw/display/ramfb.h
index b33a2c4..f6c2de9 100644
--- a/include/hw/display/ramfb.h
+++ b/include/hw/display/ramfb.h
@@ -4,7 +4,7 @@
 /* ramfb.c */
 typedef struct RAMFBState RAMFBState;
 void ramfb_display_update(QemuConsole *con, RAMFBState *s);
-RAMFBState *ramfb_setup(Error **errp);
+RAMFBState *ramfb_setup(DeviceState *dev, Error **errp);

 /* ramfb-standalone.c */
 #define TYPE_RAMFB_DEVICE "ramfb"
diff --git a/stubs/ramfb.c b/stubs/ramfb.c
index 48143f3..0799093 100644
--- a/stubs/ramfb.c
+++ b/stubs/ramfb.c
@@ -6,7 +6,7 @@ void ramfb_display_update(QemuConsole *con, RAMFBState *s)
 {
 }

-RAMFBState *ramfb_setup(Error **errp)
+RAMFBState *ramfb_setup(DeviceState* dev, Error **errp)
 {
     error_setg(errp, "ramfb support not available");
     return NULL;
-- 
2.17.1

> --- 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

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 <hqm03ster@gmail.com 
> <mailto:hqm03ster@gmail.com>>
> ---
>  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, &wrapper_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(&s->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)
> &vfio_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)
> &vfio_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) {
> +            error_setg(errp,
> +                       "xres and yres properties need edid support"
> +                       " or ramfb=on");

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

Thanks,
Marcel

>              goto out_teardown;
>          }
>      }
> diff --git a/include/hw/display/ramfb.h b/include/hw/display/ramfb.h
> index b33a2c4..f6c2de9 100644
> --- a/include/hw/display/ramfb.h
> +++ b/include/hw/display/ramfb.h
> @@ -4,7 +4,7 @@
>  /* ramfb.c */
>  typedef struct RAMFBState RAMFBState;
>  void ramfb_display_update(QemuConsole *con, RAMFBState *s);
> -RAMFBState *ramfb_setup(Error **errp);
> +RAMFBState *ramfb_setup(DeviceState *dev, Error **errp);
>
>  /* ramfb-standalone.c */
>  #define TYPE_RAMFB_DEVICE "ramfb"
> diff --git a/stubs/ramfb.c b/stubs/ramfb.c
> index 48143f3..0799093 100644
> --- a/stubs/ramfb.c
> +++ b/stubs/ramfb.c
> @@ -6,7 +6,7 @@ void ramfb_display_update(QemuConsole *con, RAMFBState *s)
>  {
>  }
>
> -RAMFBState *ramfb_setup(Error **errp)
> +RAMFBState *ramfb_setup(DeviceState* dev, Error **errp)
>  {
>      error_setg(errp, "ramfb support not available");
>      return NULL;
> -- 
> 2.17.1
>
>

> 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

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
>

  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

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
>

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

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
>

Pulled back the `qemu_create_displaysurface_guestmem` function to create
the display surface so that the guest memory gets properly unmapped.

Signed-off-by: HOU Qiming <hqm03ster@gmail.com>
---
 hw/display/ramfb.c | 53 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 40 insertions(+), 13 deletions(-)

diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
index 25c8ad7..98703f7 100644
--- a/hw/display/ramfb.c
+++ b/hw/display/ramfb.c
@@ -32,33 +32,60 @@ struct RAMFBState {
     struct RAMFBCfg cfg;
 };

+static void qemu_unmap_displaysurface_guestmem(pixman_image_t *image,
+                                               void *unused)
+{
+    void *data = pixman_image_get_data(image);
+    uint32_t size = pixman_image_get_stride(image) *
+        pixman_image_get_height(image);
+    cpu_physical_memory_unmap(data, size, 0, 0);
+}
+
+static DisplaySurface *qemu_create_displaysurface_guestmem(
+    int width, int height,
+    pixman_format_code_t format,
+    int linesize, uint64_t addr)
+{
+    DisplaySurface *surface;
+    hwaddr size;
+    void *data;
+
+    if (linesize == 0) {
+        linesize = width * PIXMAN_FORMAT_BPP(format) / 8;
+    }
+
+    size = (hwaddr)linesize * height;
+    data = cpu_physical_memory_map(addr, &size, 0);
+    if (size != (hwaddr)linesize * height) {
+        cpu_physical_memory_unmap(data, size, 0, 0);
+        return NULL;
+    }
+
+    surface = qemu_create_displaysurface_from
+        (width, height, format, linesize, data);
+    pixman_image_set_destroy_function
+        (surface->image, qemu_unmap_displaysurface_guestmem, NULL);
+
+    return surface;
+}
+
 static void ramfb_fw_cfg_write(void *dev, off_t offset, size_t len)
 {
     RAMFBState *s = dev;
-    void *framebuffer;
     uint32_t fourcc, format;
-    hwaddr stride, addr, length;
+    hwaddr stride, addr;

     s->width  = be32_to_cpu(s->cfg.width);
     s->height = be32_to_cpu(s->cfg.height);
     stride    = be32_to_cpu(s->cfg.stride);
     fourcc    = be32_to_cpu(s->cfg.fourcc);
     addr      = be64_to_cpu(s->cfg.addr);
-    length    = stride * s->height;
     format    = qemu_drm_format_to_pixman(fourcc);

     fprintf(stderr, "%s: %dx%d @ 0x%" PRIx64 "\n", __func__,
             s->width, s->height, addr);
-    framebuffer = address_space_map(&address_space_memory,
-                                    addr, &length, false,
-                                    MEMTXATTRS_UNSPECIFIED);
-    if (!framebuffer || length < stride * s->height) {
-        s->width = 0;
-        s->height = 0;
-        return;
-    }
-    s->ds = qemu_create_displaysurface_from(s->width, s->height,
-                                            format, stride, framebuffer);
+    s->ds = qemu_create_displaysurface_guestmem(s->width, s->height,
+                                                format, stride, addr);
 }

 void ramfb_display_update(QemuConsole *con, RAMFBState *s)
-- 
2.17.1

Only allow one resolution change per guest boot, which prevents a
crash when the guest writes garbage to the configuration space (e.g.
when rebooting).

Signed-off-by: HOU Qiming <hqm03ster@gmail.com>
---
 hw/display/ramfb.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
index 98703f7..d255ddc 100644
--- a/hw/display/ramfb.c
+++ b/hw/display/ramfb.c
@@ -30,6 +30,7 @@ struct RAMFBState {
     DisplaySurface *ds;
     uint32_t width, height;
     struct RAMFBCfg cfg;
+    bool locked;
 };

 static void qemu_unmap_displaysurface_guestmem(pixman_image_t *image,
@@ -72,18 +73,25 @@ static DisplaySurface
*qemu_create_displaysurface_guestmem(
 static void ramfb_fw_cfg_write(void *dev, off_t offset, size_t len)
 {
     RAMFBState *s = dev;
-    uint32_t fourcc, format;
+    uint32_t fourcc, format, width, height;
     hwaddr stride, addr;

-    s->width  = be32_to_cpu(s->cfg.width);
-    s->height = be32_to_cpu(s->cfg.height);
+    width     = be32_to_cpu(s->cfg.width);
+    height    = be32_to_cpu(s->cfg.height);
     stride    = be32_to_cpu(s->cfg.stride);
     fourcc    = be32_to_cpu(s->cfg.fourcc);
     addr      = be64_to_cpu(s->cfg.addr);
     format    = qemu_drm_format_to_pixman(fourcc);

     fprintf(stderr, "%s: %dx%d @ 0x%" PRIx64 "\n", __func__,
-            s->width, s->height, addr);
+            width, height, addr);
+    if (s->locked) {
+        fprintf(stderr, "%s: resolution locked, change rejected\n",
__func__);
+        return;
+    }
+    s->locked = true;
+    s->width = width;
+    s->height = height;
     s->ds = qemu_create_displaysurface_guestmem(s->width, s->height,
                                                 format, stride, addr);
 }
@@ -103,6 +111,13 @@ void ramfb_display_update(QemuConsole *con, RAMFBState
*s)
     dpy_gfx_update_full(con);
 }

+static void ramfb_reset(void *opaque)
+{
+    RAMFBState *s = (RAMFBState *)opaque;
+    s->locked = false;
+    memset(&s->cfg, 0, sizeof(s->cfg));
+}
+
 RAMFBState *ramfb_setup(Error **errp)
 {
     FWCfgState *fw_cfg = fw_cfg_find();
@@ -115,9 +130,12 @@ RAMFBState *ramfb_setup(Error **errp)

     s = g_new0(RAMFBState, 1);

+    s->locked = false;
+
     rom_add_vga("vgabios-ramfb.bin");
     fw_cfg_add_file_callback(fw_cfg, "etc/ramfb",
                              NULL, ramfb_fw_cfg_write, s,
                              &s->cfg, sizeof(s->cfg), false);
+    qemu_register_reset(ramfb_reset, s);
     return s;
 }
-- 
2.17.1

If xres / yres were specified in QEMU command line, write them as an initial
resolution to the fw-config space on guest reset, which a later BIOS / OVMF
patch can take advantage of.

Signed-off-by: HOU Qiming <hqm03ster@gmail.com>
---
 hw/display/ramfb-standalone.c | 12 +++++++++++-
 hw/display/ramfb.c            | 16 +++++++++++++++-
 hw/vfio/display.c             |  4 ++--
 include/hw/display/ramfb.h    |  2 +-
 stubs/ramfb.c                 |  2 +-
 5 files changed, 30 insertions(+), 6 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, &wrapper_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 d255ddc..9179d17 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;
     struct RAMFBCfg cfg;
     bool locked;
 };
@@ -116,9 +118,11 @@ static void ramfb_reset(void *opaque)
     RAMFBState *s = (RAMFBState *)opaque;
     s->locked = false;
     memset(&s->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;
@@ -130,6 +134,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)
                                           &vfio_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)
                                           &vfio_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/include/hw/display/ramfb.h b/include/hw/display/ramfb.h
index b33a2c4..f6c2de9 100644
--- a/include/hw/display/ramfb.h
+++ b/include/hw/display/ramfb.h
@@ -4,7 +4,7 @@
 /* ramfb.c */
 typedef struct RAMFBState RAMFBState;
 void ramfb_display_update(QemuConsole *con, RAMFBState *s);
-RAMFBState *ramfb_setup(Error **errp);
+RAMFBState *ramfb_setup(DeviceState *dev, Error **errp);

 /* ramfb-standalone.c */
 #define TYPE_RAMFB_DEVICE "ramfb"
diff --git a/stubs/ramfb.c b/stubs/ramfb.c
index 48143f3..0799093 100644
--- a/stubs/ramfb.c
+++ b/stubs/ramfb.c
@@ -6,7 +6,7 @@ void ramfb_display_update(QemuConsole *con, RAMFBState *s)
 {
 }

-RAMFBState *ramfb_setup(Error **errp)
+RAMFBState *ramfb_setup(DeviceState* dev, Error **errp)
 {
     error_setg(errp, "ramfb support not available");
     return NULL;
-- 
2.17.1