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

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

[Gerd Hoffmann]

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

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

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

-- 
2.18.2

[PATCH 1/5] 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>
---
 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 2/5] 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>
---
 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 3/5] 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>
---
 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 9d41c2ad2868..fbe959147dc9 100644
--- a/hw/display/ramfb.c
+++ b/hw/display/ramfb.c
@@ -71,20 +71,27 @@ 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);
 
     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);
+    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 4/5] 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 fbe959147dc9..d1b1cb9bb294 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 5/5] ramfb: drop leftover debug message

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@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 d1b1cb9bb294..be884c9ea837 100644
--- a/hw/display/ramfb.c
+++ b/hw/display/ramfb.c
@@ -88,8 +88,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);
     surface = ramfb_create_display_surface(width, height,
                                            format, stride, addr);
     if (!surface)
-- 
2.18.2

Re: [PATCH 3/5] ramfb: don't update RAMFBState on errors

[Philippe Mathieu-Daudé]

On 4/22/20 12:02 PM, Gerd Hoffmann wrote:
> 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>
> ---
>   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 9d41c2ad2868..fbe959147dc9 100644
> --- a/hw/display/ramfb.c
> +++ b/hw/display/ramfb.c
> @@ -71,20 +71,27 @@ 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);
>   
>       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);
> +    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)
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 5/5] ramfb: drop leftover debug message

[Philippe Mathieu-Daudé]

On 4/22/20 12:02 PM, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@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 d1b1cb9bb294..be884c9ea837 100644
> --- a/hw/display/ramfb.c
> +++ b/hw/display/ramfb.c
> @@ -88,8 +88,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);
>       surface = ramfb_create_display_surface(width, height,
>                                              format, stride, addr);
>       if (!surface)
> 

I'd move this before patch #3/5 "ramfb: don't update RAMFBState on 
errors". Anyway,

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

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

[no-reply]

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



Hi,

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

Subject: [PATCH 0/5] ramfb: a bunch of reverts and fixes
Message-id: 20200422100211.30614-1-kraxel@redhat.com
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 ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20200422011722.13287-1-richard.henderson@linaro.org -> patchew/20200422011722.13287-1-richard.henderson@linaro.org
 - [tag update]      patchew/20200422100211.30614-1-kraxel@redhat.com -> patchew/20200422100211.30614-1-kraxel@redhat.com
Switched to a new branch 'test'
9cc0b6a ramfb: drop leftover debug message
b28e041 ramfb: add sanity checks to ramfb_create_display_surface
29a55a8 ramfb: don't update RAMFBState on errors
0ff0dd8 Revert "hw/display/ramfb: lock guest resolution after it's set"
db523e5 Revert "hw/display/ramfb: initialize fw-config space with xres/ yres"

=== OUTPUT BEGIN ===
1/5 Checking commit db523e55cde0 (Revert "hw/display/ramfb: initialize fw-config space with xres/ yres")
2/5 Checking commit 0ff0dd8955d3 (Revert "hw/display/ramfb: lock guest resolution after it's set")
3/5 Checking commit 29a55a84e498 (ramfb: don't update RAMFBState on errors)
ERROR: braces {} are necessary for all arms of this statement
#46: FILE: hw/display/ramfb.c:89:
+    if (!surface)
[...]

total: 1 errors, 0 warnings, 36 lines checked

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

4/5 Checking commit b28e041cc318 (ramfb: add sanity checks to ramfb_create_display_surface)
5/5 Checking commit 9cc0b6a2ea69 (ramfb: drop leftover debug message)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200422100211.30614-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 1/5] Revert "hw/display/ramfb: initialize fw-config space with xres/ yres"

[Laszlo Ersek]

On 04/22/20 12:02, Gerd Hoffmann wrote:
> 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>
> ---
>  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(-)

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


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

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

[Laszlo Ersek]

On 04/22/20 12:02, Gerd Hoffmann wrote:
> 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>
> ---
>  hw/display/ramfb.c | 26 ++++----------------------
>  1 file changed, 4 insertions(+), 22 deletions(-)

(1) I've verified that, after applying patches #1 and #2 in this series,
the resultant state of "hw/display/ramfb.c" matches the one at
a9e0cb67b7f4^ (note the caret), modulo:

- commit 71e8a9158558 ("Include sysemu/reset.h a lot less", 2019-08-16), and

- commit 85eb7c18ee39 ("Let cpu_[physical]_memory() calls pass a boolean
'is_write' argument", 2020-02-20).

So:

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

(2) I've also tested this patch (i.e., built QEMU with patches #1 and #2
applied -- patch #1 is needed for context --, and entered the OVMF Setup
TUI while using ramfb).

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

Thanks!
Laszlo

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

Re: [PATCH 3/5] ramfb: don't update RAMFBState on errors

[Laszlo Ersek]

On 04/22/20 12:02, Gerd Hoffmann wrote:
> 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>
> ---
>  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 9d41c2ad2868..fbe959147dc9 100644
> --- a/hw/display/ramfb.c
> +++ b/hw/display/ramfb.c
> @@ -71,20 +71,27 @@ 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);
>  
>      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);
> +    surface = ramfb_create_display_surface(width, height,
> +                                           format, stride, addr);
> +    if (!surface)
> +        return;

A warning message here, or inside ramfb_create_display_surface(), could
be nice; but I agree it's not required at all.

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

Thanks
Laszlo

> +
> +    s->width = width;
> +    s->height = height;
> +    s->ds = surface;
>  }
>  
>  void ramfb_display_update(QemuConsole *con, RAMFBState *s)
> 

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

[Laszlo Ersek]

On 04/22/20 12:02, 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 fbe959147dc9..d1b1cb9bb294 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 ||

Seems to make sense (I've checked the constants).

> +        format == 0 /* unknown format */)

OK, this is from qemu_drm_format_to_pixman().

> +        return NULL;
> +
>      if (linesize == 0) {
>          linesize = width * PIXMAN_FORMAT_BPP(format) / 8;
>      }
> 

I would suggest four more sanity checks:

- if "linesize" is nonzero, make sure it is a whole multiple of the
required word size (?)

- if "linesize" is nonzero, make sure it is not bogus with relation to
"width". I'm thinking something like:

    if (linesize > 0) {
        min_linesize = width * PIXMAN_FORMAT_BPP(format) / 8;
        if (linesize < min_linesize) {
            return NULL;
        }
    }

May not be the best way to put it, but you get the idea.

- We might want to put an upper bound on "linesize" too. I realize
"(hwaddr)linesize * height" should be safe in this function, as the
multiplication is done in uint64_t. But we also pass "linesize" to
qemu_create_displaysurface_from(), and who knows how it is used for
multiplication there.

- in the ramfb_create_display_surface() function, we should change the
type of the parameters "width", "height", and "linesize", from "int" to
"uint32_t". In ramfb_fw_cfg_write(), we do take them as uint32_t from
the guest, but then pass them as "int"s. And so the current state can
produce negative values for any of "width", "height", and "linesize" --
and I'd rather not investigate where those lead. (The new checks catch a
negative "width" and "height" already, but not "linesize".) Casting a
negative "linesize" to (hwaddr) produces a big, ugly value, FWIW.

Thanks
Laszlo

Re: [PATCH 5/5] ramfb: drop leftover debug message

[Laszlo Ersek]

On 04/22/20 12:02, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@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 d1b1cb9bb294..be884c9ea837 100644
> --- a/hw/display/ramfb.c
> +++ b/hw/display/ramfb.c
> @@ -88,8 +88,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);
>      surface = ramfb_create_display_surface(width, height,
>                                             format, stride, addr);
>      if (!surface)
> 

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

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

[Gerd Hoffmann]

  Hi,

> - if "linesize" is nonzero, make sure it is a whole multiple of the
> required word size (?)
> 
> - if "linesize" is nonzero, make sure it is not bogus with relation to
> "width". I'm thinking something like:

Yep, the whole linesize is a bit bogus, noticed after sending out this
series, I have a followup patch for that (see below).

take care,
  Gerd

From 154dcff73dc533fc95c88bd960ed65108af6c734 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Wed, 22 Apr 2020 12:07:59 +0200
Subject: [PATCH] ramfb: fix size calculation

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>
---
 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 4/5] ramfb: add sanity checks to ramfb_create_display_surface

[Laszlo Ersek]

On 04/23/20 13:41, Gerd Hoffmann wrote:
>   Hi,
> 
>> - if "linesize" is nonzero, make sure it is a whole multiple of the
>> required word size (?)
>>
>> - if "linesize" is nonzero, make sure it is not bogus with relation to
>> "width". I'm thinking something like:
> 
> Yep, the whole linesize is a bit bogus, noticed after sending out this
> series, I have a followup patch for that (see below).
> 
> take care,
>   Gerd
> 
> From 154dcff73dc533fc95c88bd960ed65108af6c734 Mon Sep 17 00:00:00 2001
> From: Gerd Hoffmann <kraxel@redhat.com>
> Date: Wed, 22 Apr 2020 12:07:59 +0200
> Subject: [PATCH] ramfb: fix size calculation
> 
> 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>
> ---
>  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);
>  
> 

I don't understand two things about this patch:

- "stride" can still be smaller than "linesize" (scanlines can still
overlap). Why is that not a problem?

- assuming "stride > linesize" (i.e., strictly larger), we don't seem to
map the last complete stride, and that seems to be intentional. Is that
safe with regard to qemu_create_displaysurface_from()? Ultimately the
stride is passed to pixman_image_create_bits(), and the underlying
"data" may not be large enough for that. What am I missing?

Hm... bonus question: qemu_create_displaysurface_from() still accepts
"linesize" as a signed int. (Not sure about pixman_image_create_bits().)
Should we do something specific to prevent that value from being
negative? The guest gives us a uint32_t.

Thanks
Laszlo

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

[Gerd Hoffmann]

> > -    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);
> >  
> > 
> 
> I don't understand two things about this patch:
> 
> - "stride" can still be smaller than "linesize" (scanlines can still
> overlap). Why is that not a problem?

Why it should be?  It is the guests choice.  Not a very useful one, but
hey, if the guest prefers it that way we are at your service ...

We only must make sure our size calculations are correct.  The patch
does that.  I think we can also outlaw stride < linesize if you are
happier with that alternative approach.  I doubt we have any guests
relying on this working.

> - assuming "stride > linesize" (i.e., strictly larger), we don't seem to
> map the last complete stride, and that seems to be intentional. Is that
> safe with regard to qemu_create_displaysurface_from()? Ultimately the
> stride is passed to pixman_image_create_bits(), and the underlying
> "data" may not be large enough for that. What am I missing?

Lets take a real-world example.  Wayland rounds up width and height to
multiples of 64 (probably for tiling on modern GPUs).  So with 800x600
you get an allocation of 832x640, like this:

     ###########**   <- y 0
     ###########**
     ###########**
     ###########**
     ###########**   <- y 600
     *************   <- y 640

     ^         ^ ^----- x 832
     |         +------- x 800
     +----------------- x 0

where "#" is image data and "*" is unused padding space.  Pixman will
access all "#", so we are mapping the region from the first "#" to the
last "#", including the unused padding on each scanline, except for the
last scanline.  Any unused scanlines at the bottom are excluded too
(ramfb doesn't even know whenever they exist).

The unused padding is only mapped because it is the easiest way to
handle things, not because we need it.  Also the padding is typically
*alot* smaller than PAGE_SIZE, so we couldn't exclude it from the
mapping even if we would like to ;)

> Hm... bonus question: qemu_create_displaysurface_from() still accepts
> "linesize" as a signed int. (Not sure about pixman_image_create_bits().)
> Should we do something specific to prevent that value from being
> negative? The guest gives us a uint32_t.

Not fully sure we can do that without breaking something, might be a
negative stride is used for upside down images (last scanline comes
first in memory).

take care,
  Gerd

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

[Laszlo Ersek]

On 04/27/20 13:11, Gerd Hoffmann wrote:
>>> -    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);
>>>  
>>>
>>
>> I don't understand two things about this patch:
>>
>> - "stride" can still be smaller than "linesize" (scanlines can still
>> overlap). Why is that not a problem?
> 
> Why it should be?  It is the guests choice.  Not a very useful one, but
> hey, if the guest prefers it that way we are at your service ...
> 
> We only must make sure our size calculations are correct.  The patch
> does that.  I think we can also outlaw stride < linesize if you are
> happier with that alternative approach.  I doubt we have any guests
> relying on this working.

OK, thanks. I agree -- if it doesn't break QEMU, then we can let guests
break themselves.

> 
>> - assuming "stride > linesize" (i.e., strictly larger), we don't seem to
>> map the last complete stride, and that seems to be intentional. Is that
>> safe with regard to qemu_create_displaysurface_from()? Ultimately the
>> stride is passed to pixman_image_create_bits(), and the underlying
>> "data" may not be large enough for that. What am I missing?
> 
> Lets take a real-world example.  Wayland rounds up width and height to
> multiples of 64 (probably for tiling on modern GPUs).  So with 800x600
> you get an allocation of 832x640, like this:
> 
>      ###########**   <- y 0
>      ###########**
>      ###########**
>      ###########**
>      ###########**   <- y 600
>      *************   <- y 640
> 
>      ^         ^ ^----- x 832
>      |         +------- x 800
>      +----------------- x 0
> 
> where "#" is image data and "*" is unused padding space.  Pixman will
> access all "#", so we are mapping the region from the first "#" to the
> last "#", including the unused padding on each scanline, except for the
> last scanline.  Any unused scanlines at the bottom are excluded too
> (ramfb doesn't even know whenever they exist).
> 
> The unused padding is only mapped because it is the easiest way to
> handle things, not because we need it.  Also the padding is typically
> *alot* smaller than PAGE_SIZE, so we couldn't exclude it from the
> mapping even if we would like to ;)

OK. If pixman only accesses the "#" marks, then it should be OK.

> 
>> Hm... bonus question: qemu_create_displaysurface_from() still accepts
>> "linesize" as a signed int. (Not sure about pixman_image_create_bits().)
>> Should we do something specific to prevent that value from being
>> negative? The guest gives us a uint32_t.
> 
> Not fully sure we can do that without breaking something, might be a
> negative stride is used for upside down images (last scanline comes
> first in memory).

Ugh... Upside down images???... Well, OK, I guess. :)

For the followup patch:

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

Laszlo

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

[Gerd Hoffmann]

  Hi,

> > Not fully sure we can do that without breaking something, might be a
> > negative stride is used for upside down images (last scanline comes
> > first in memory).
> 
> Ugh... Upside down images???... Well, OK, I guess. :)

Well, in the unix world (x11, wayland) x=0,y=0 is the upper left corner.
In the windows world x=0,y=0 is the lower left corner, in opengl too.
If you don't handle this correctly your guest display might show up
upside down ;)

qxl uses negative strides to signal that.  Looking at the code I see qxl
handles this locally (grep for qxl_stride in qxl-render.c), it doesn't
propagate into ui/console.c, so it should be safe to change the
qemu_create_displaysurface_from() arguments to unsigned from qxl point
of view.

take care,
  Gerd