[PATCH v2 0/6] ramfb: a bunch of reverts and fixes

[PATCH v2 0/6] ramfb: a bunch of reverts and fixes

[Gerd Hoffmann]

Even though these are bugfixes this is probably 5.1 material
at this point ...

v2: reorder patches, add patch #6, pick up acks & reviews.

Gerd Hoffmann (6):
  Revert "hw/display/ramfb: initialize fw-config space with xres/ yres"
  Revert "hw/display/ramfb: lock guest resolution after it's set"
  ramfb: drop leftover debug message
  ramfb: don't update RAMFBState on errors
  ramfb: add sanity checks to ramfb_create_display_surface
  ramfb: fix size calculation

 include/hw/display/ramfb.h    |  2 +-
 hw/display/ramfb-standalone.c | 12 +-----
 hw/display/ramfb.c            | 78 +++++++++++++----------------------
 hw/vfio/display.c             |  4 +-
 stubs/ramfb.c                 |  2 +-
 5 files changed, 34 insertions(+), 64 deletions(-)

-- 
2.18.2

[PATCH v2 1/6] Revert "hw/display/ramfb: initialize fw-config space with xres/ yres"

[Gerd Hoffmann]

This reverts commit f79081b4b71b72640bedd40a7cd76f864c8287f1.

Patch has broken byteorder handling: RAMFBCfg fields are in bigendian
byteorder, the reset function doesn't care so native byteorder is used
instead.  Given this went unnoticed so far the feature is obviously
unused, so just revert the patch.

Cc: Hou Qiming <hqm03ster@gmail.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Acked-by: Laszlo Ersek <lersek@redhat.com>
---
 include/hw/display/ramfb.h    |  2 +-
 hw/display/ramfb-standalone.c | 12 +-----------
 hw/display/ramfb.c            | 16 +---------------
 hw/vfio/display.c             |  4 ++--
 stubs/ramfb.c                 |  2 +-
 5 files changed, 6 insertions(+), 30 deletions(-)

diff --git a/include/hw/display/ramfb.h b/include/hw/display/ramfb.h
index f6c2de93b222..b33a2c467b28 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(DeviceState *dev, Error **errp);
+RAMFBState *ramfb_setup(Error **errp);
 
 /* ramfb-standalone.c */
 #define TYPE_RAMFB_DEVICE "ramfb"
diff --git a/hw/display/ramfb-standalone.c b/hw/display/ramfb-standalone.c
index d76a9d0fe2c9..b18db97eeb1b 100644
--- a/hw/display/ramfb-standalone.c
+++ b/hw/display/ramfb-standalone.c
@@ -3,7 +3,6 @@
 #include "qemu/module.h"
 #include "hw/loader.h"
 #include "hw/qdev-properties.h"
-#include "hw/isa/isa.h"
 #include "hw/display/ramfb.h"
 #include "ui/console.h"
 
@@ -13,8 +12,6 @@ typedef struct RAMFBStandaloneState {
     SysBusDevice parent_obj;
     QemuConsole *con;
     RAMFBState *state;
-    uint32_t xres;
-    uint32_t yres;
 } RAMFBStandaloneState;
 
 static void display_update_wrapper(void *dev)
@@ -37,22 +34,15 @@ 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(dev, errp);
+    ramfb->state = ramfb_setup(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;
-    device_class_set_props(dc, 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 7ba07c80f6e1..bd4746dc1768 100644
--- a/hw/display/ramfb.c
+++ b/hw/display/ramfb.c
@@ -13,7 +13,6 @@
 
 #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"
@@ -31,7 +30,6 @@ struct QEMU_PACKED RAMFBCfg {
 struct RAMFBState {
     DisplaySurface *ds;
     uint32_t width, height;
-    uint32_t starting_width, starting_height;
     struct RAMFBCfg cfg;
     bool locked;
 };
@@ -117,11 +115,9 @@ 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)
+RAMFBState *ramfb_setup(Error **errp)
 {
     FWCfgState *fw_cfg = fw_cfg_find();
     RAMFBState *s;
@@ -133,16 +129,6 @@ RAMFBState *ramfb_setup(DeviceState* dev, 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 f4977c66e1b5..a57a22674d62 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -353,7 +353,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(DEVICE(vdev), errp);
+        vdev->dpy->ramfb = ramfb_setup(errp);
     }
     vfio_display_edid_init(vdev);
     return 0;
@@ -479,7 +479,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(DEVICE(vdev), errp);
+        vdev->dpy->ramfb = ramfb_setup(errp);
     }
     return 0;
 }
diff --git a/stubs/ramfb.c b/stubs/ramfb.c
index 0799093a5d6e..48143f33542f 100644
--- a/stubs/ramfb.c
+++ b/stubs/ramfb.c
@@ -6,7 +6,7 @@ void ramfb_display_update(QemuConsole *con, RAMFBState *s)
 {
 }
 
-RAMFBState *ramfb_setup(DeviceState* dev, Error **errp)
+RAMFBState *ramfb_setup(Error **errp)
 {
     error_setg(errp, "ramfb support not available");
     return NULL;
-- 
2.18.2

[PATCH v2 2/6] Revert "hw/display/ramfb: lock guest resolution after it's set"

[Gerd Hoffmann]

This reverts commit a9e0cb67b7f4c485755659f9b764c38b5f970de4.

This breaks OVMF.  Reproducer: Just hit 'ESC' at early boot to enter
firmware setup.  OVMF wants switch from (default) 800x600 to 640x480 for
that, and this patch blocks it.

Cc: Hou Qiming <hqm03ster@gmail.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/display/ramfb.c | 26 ++++----------------------
 1 file changed, 4 insertions(+), 22 deletions(-)

diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
index bd4746dc1768..9d41c2ad2868 100644
--- a/hw/display/ramfb.c
+++ b/hw/display/ramfb.c
@@ -31,7 +31,6 @@ struct RAMFBState {
     DisplaySurface *ds;
     uint32_t width, height;
     struct RAMFBCfg cfg;
-    bool locked;
 };
 
 static void ramfb_unmap_display_surface(pixman_image_t *image, void *unused)
@@ -72,25 +71,18 @@ static DisplaySurface *ramfb_create_display_surface(int width, int height,
 static void ramfb_fw_cfg_write(void *dev, off_t offset, size_t len)
 {
     RAMFBState *s = dev;
-    uint32_t fourcc, format, width, height;
+    uint32_t fourcc, format;
     hwaddr stride, addr;
 
-    width     = be32_to_cpu(s->cfg.width);
-    height    = be32_to_cpu(s->cfg.height);
+    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);
     format    = qemu_drm_format_to_pixman(fourcc);
 
     fprintf(stderr, "%s: %dx%d @ 0x%" PRIx64 "\n", __func__,
-            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->width, s->height, addr);
     s->ds = ramfb_create_display_surface(s->width, s->height,
                                          format, stride, addr);
 }
@@ -110,13 +102,6 @@ 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();
@@ -129,12 +114,9 @@ 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.18.2

[PATCH v2 3/6] ramfb: drop leftover debug message

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/display/ramfb.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
index 9d41c2ad2868..228defee5683 100644
--- a/hw/display/ramfb.c
+++ b/hw/display/ramfb.c
@@ -81,8 +81,6 @@ static void ramfb_fw_cfg_write(void *dev, off_t offset, size_t len)
     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);
     s->ds = ramfb_create_display_surface(s->width, s->height,
                                          format, stride, addr);
 }
-- 
2.18.2

[PATCH v2 4/6] ramfb: don't update RAMFBState on errors

[Gerd Hoffmann]

Store width & height & surface in local variables.  Update RAMFBState
with the new values only in case the ramfb_create_display_surface() call
succeeds.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/display/ramfb.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
index 228defee5683..eb8b4bc49a2f 100644
--- a/hw/display/ramfb.c
+++ b/hw/display/ramfb.c
@@ -71,18 +71,25 @@ static DisplaySurface *ramfb_create_display_surface(int width, int height,
 static void ramfb_fw_cfg_write(void *dev, off_t offset, size_t len)
 {
     RAMFBState *s = dev;
-    uint32_t fourcc, format;
+    DisplaySurface *surface;
+    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);
-    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);
+    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);
 
-    s->ds = ramfb_create_display_surface(s->width, s->height,
-                                         format, stride, addr);
+    surface = ramfb_create_display_surface(width, height,
+                                           format, stride, addr);
+    if (!surface)
+        return;
+
+    s->width = width;
+    s->height = height;
+    s->ds = surface;
 }
 
 void ramfb_display_update(QemuConsole *con, RAMFBState *s)
-- 
2.18.2

[PATCH v2 5/6] ramfb: add sanity checks to ramfb_create_display_surface

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/ramfb.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
index eb8b4bc49a2f..be884c9ea837 100644
--- a/hw/display/ramfb.c
+++ b/hw/display/ramfb.c
@@ -15,6 +15,7 @@
 #include "qapi/error.h"
 #include "hw/loader.h"
 #include "hw/display/ramfb.h"
+#include "hw/display/bochs-vbe.h" /* for limits */
 #include "ui/console.h"
 #include "sysemu/reset.h"
 
@@ -49,6 +50,11 @@ static DisplaySurface *ramfb_create_display_surface(int width, int height,
     hwaddr size;
     void *data;
 
+    if (width < 16 || width > VBE_DISPI_MAX_XRES ||
+        height < 16 || height > VBE_DISPI_MAX_YRES ||
+        format == 0 /* unknown format */)
+        return NULL;
+
     if (linesize == 0) {
         linesize = width * PIXMAN_FORMAT_BPP(format) / 8;
     }
-- 
2.18.2

[PATCH v2 6/6] ramfb: fix size calculation

[Gerd Hoffmann]

size calculation isn't correct with guest-supplied stride, the last
display line isn't accounted for correctly.

For the typical case of stride > linesize (add padding) we error on the
safe side (calculated size is larger than actual size).

With stride < linesize (scanlines overlap) the calculated size is
smaller than the actual size though so our guest memory mapping might
end up being too small.

While being at it also fix ramfb_create_display_surface to use hwaddr
for the parameters.  That way all calculation are done with hwaddr type
and we can't get funny effects from type castings.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Acked-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/display/ramfb.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
index be884c9ea837..928d74d10bc7 100644
--- a/hw/display/ramfb.c
+++ b/hw/display/ramfb.c
@@ -44,10 +44,10 @@ static void ramfb_unmap_display_surface(pixman_image_t *image, void *unused)
 
 static DisplaySurface *ramfb_create_display_surface(int width, int height,
                                                     pixman_format_code_t format,
-                                                    int linesize, uint64_t addr)
+                                                    hwaddr stride, hwaddr addr)
 {
     DisplaySurface *surface;
-    hwaddr size;
+    hwaddr size, mapsize, linesize;
     void *data;
 
     if (width < 16 || width > VBE_DISPI_MAX_XRES ||
@@ -55,19 +55,20 @@ static DisplaySurface *ramfb_create_display_surface(int width, int height,
         format == 0 /* unknown format */)
         return NULL;
 
-    if (linesize == 0) {
-        linesize = width * PIXMAN_FORMAT_BPP(format) / 8;
+    linesize = width * PIXMAN_FORMAT_BPP(format) / 8;
+    if (stride == 0) {
+        stride = linesize;
     }
 
-    size = (hwaddr)linesize * height;
-    data = cpu_physical_memory_map(addr, &size, false);
-    if (size != (hwaddr)linesize * height) {
-        cpu_physical_memory_unmap(data, size, 0, 0);
+    mapsize = size = stride * (height - 1) + linesize;
+    data = cpu_physical_memory_map(addr, &mapsize, false);
+    if (size != mapsize) {
+        cpu_physical_memory_unmap(data, mapsize, 0, 0);
         return NULL;
     }
 
     surface = qemu_create_displaysurface_from(width, height,
-                                              format, linesize, data);
+                                              format, stride, data);
     pixman_image_set_destroy_function(surface->image,
                                       ramfb_unmap_display_surface, NULL);
 
-- 
2.18.2

Re: [PATCH v2 0/6] ramfb: a bunch of reverts and fixes

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200429115236.28709-1-kraxel@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20200429115236.28709-1-kraxel@redhat.com
Subject: [PATCH v2 0/6] ramfb: a bunch of reverts and fixes
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Switched to a new branch 'test'
606207c ramfb: fix size calculation
3503c7b ramfb: add sanity checks to ramfb_create_display_surface
78b6e20 ramfb: don't update RAMFBState on errors
96648223 ramfb: drop leftover debug message
329fe4e Revert "hw/display/ramfb: lock guest resolution after it's set"
9fed95d Revert "hw/display/ramfb: initialize fw-config space with xres/ yres"

=== OUTPUT BEGIN ===
1/6 Checking commit 9fed95ded1c1 (Revert "hw/display/ramfb: initialize fw-config space with xres/ yres")
2/6 Checking commit 329fe4e6f68d (Revert "hw/display/ramfb: lock guest resolution after it's set")
3/6 Checking commit 96648223c33a (ramfb: drop leftover debug message)
4/6 Checking commit 78b6e20dc09f (ramfb: don't update RAMFBState on errors)
ERROR: braces {} are necessary for all arms of this statement
#46: FILE: hw/display/ramfb.c:87:
+    if (!surface)
[...]

total: 1 errors, 0 warnings, 34 lines checked

Patch 4/6 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

5/6 Checking commit 3503c7b9d778 (ramfb: add sanity checks to ramfb_create_display_surface)
6/6 Checking commit 606207c60d25 (ramfb: fix size calculation)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200429115236.28709-1-kraxel@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v2 5/6] ramfb: add sanity checks to ramfb_create_display_surface

[Laszlo Ersek]

On 04/29/20 13:52, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/display/ramfb.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
> index eb8b4bc49a2f..be884c9ea837 100644
> --- a/hw/display/ramfb.c
> +++ b/hw/display/ramfb.c
> @@ -15,6 +15,7 @@
>  #include "qapi/error.h"
>  #include "hw/loader.h"
>  #include "hw/display/ramfb.h"
> +#include "hw/display/bochs-vbe.h" /* for limits */
>  #include "ui/console.h"
>  #include "sysemu/reset.h"
>  
> @@ -49,6 +50,11 @@ static DisplaySurface *ramfb_create_display_surface(int width, int height,
>      hwaddr size;
>      void *data;
>  
> +    if (width < 16 || width > VBE_DISPI_MAX_XRES ||
> +        height < 16 || height > VBE_DISPI_MAX_YRES ||
> +        format == 0 /* unknown format */)
> +        return NULL;
> +
>      if (linesize == 0) {
>          linesize = width * PIXMAN_FORMAT_BPP(format) / 8;
>      }
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>