[PULL 0/8] Vga 20200518 patches

[PULL 0/8] Vga 20200518 patches

[Gerd Hoffmann]

The following changes since commit debe78ce14bf8f8940c2bdf3ef387505e9e035a9:

  Merge remote-tracking branch 'remotes/rth/tags/pull-fpu-20200515' into staging (2020-05-15 19:51:16 +0100)

are available in the Git repository at:

  git://git.kraxel.org/qemu tags/vga-20200518-pull-request

for you to fetch changes up to 3fcf15df0073a76d37e2816597771d4c9763e413:

  ramfb: fix size calculation (2020-05-18 15:43:51 +0200)

----------------------------------------------------------------
vga: ati-vga bugfix, ramfb cleanups and fixes.

----------------------------------------------------------------

BALATON Zoltan (1):
  ati-vga: Do not allow unaligned access via index register

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

Philippe Mathieu-Daudé (1):
  hw/display: Include local 'framebuffer.h'

 include/hw/display/ramfb.h    |  2 +-
 hw/display/artist.c           |  2 +-
 hw/display/ati.c              |  2 +-
 hw/display/next-fb.c          |  2 +-
 hw/display/ramfb-standalone.c | 12 +-----
 hw/display/ramfb.c            | 77 +++++++++++++----------------------
 hw/vfio/display.c             |  4 +-
 stubs/ramfb.c                 |  2 +-
 8 files changed, 37 insertions(+), 66 deletions(-)

-- 
2.18.4

[PULL 1/8] ati-vga: Do not allow unaligned access via index register

[Gerd Hoffmann]

From: BALATON Zoltan <balaton@eik.bme.hu>

According to docs bits 1 and 0 of MM_INDEX are hard coded to 0 so
unaligned access via this register should not be possible.
This also fixes problems reported in bug #1878134.

Buglink: https://bugs.launchpad.net/qemu/+bug/1878134
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Tested-by: Alexander Bulekov <alxndr@bu.edu>
Acked-by: Alexander Bulekov <alxndr@bu.edu>
Message-id: 20200516132352.39E9374594E@zero.eik.bme.hu
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/ati.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/ati.c b/hw/display/ati.c
index 58ec8291d460..065f197678e4 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -511,7 +511,7 @@ static void ati_mm_write(void *opaque, hwaddr addr,
     }
     switch (addr) {
     case MM_INDEX:
-        s->regs.mm_index = data;
+        s->regs.mm_index = data & ~3;
         break;
     case MM_DATA ... MM_DATA + 3:
         /* indexed access to regs or memory */
-- 
2.18.4

[PULL 2/8] hw/display: Include local 'framebuffer.h'

[Gerd Hoffmann]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

The "framebuffer.h" header is not an exported include.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-id: 20200504082003.16298-2-f4bug@amsat.org
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/artist.c  | 2 +-
 hw/display/next-fb.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/display/artist.c b/hw/display/artist.c
index 7e2a4556bd06..6261bfe65b72 100644
--- a/hw/display/artist.c
+++ b/hw/display/artist.c
@@ -21,7 +21,7 @@
 #include "migration/vmstate.h"
 #include "ui/console.h"
 #include "trace.h"
-#include "hw/display/framebuffer.h"
+#include "framebuffer.h"
 
 #define TYPE_ARTIST "artist"
 #define ARTIST(obj) OBJECT_CHECK(ARTISTState, (obj), TYPE_ARTIST)
diff --git a/hw/display/next-fb.c b/hw/display/next-fb.c
index 2b726a10f845..b0513a8fba62 100644
--- a/hw/display/next-fb.c
+++ b/hw/display/next-fb.c
@@ -27,7 +27,7 @@
 #include "hw/hw.h"
 #include "hw/boards.h"
 #include "hw/loader.h"
-#include "hw/display/framebuffer.h"
+#include "framebuffer.h"
 #include "ui/pixel_ops.h"
 #include "hw/m68k/next-cube.h"
 
-- 
2.18.4

[PULL 3/8] 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>
Message-id: 20200429115236.28709-2-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.4

[PULL 4/8] 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>
Message-id: 20200429115236.28709-3-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.4

[PULL 5/8] 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>
Message-id: 20200429115236.28709-4-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 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.4

[PULL 6/8] 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>
Message-id: 20200429115236.28709-5-kraxel@redhat.com
---
 hw/display/ramfb.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
index 228defee5683..1a20bdec416f 100644
--- a/hw/display/ramfb.c
+++ b/hw/display/ramfb.c
@@ -71,18 +71,26 @@ 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.4

[PULL 7/8] ramfb: add sanity checks to ramfb_create_display_surface

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Message-id: 20200429115236.28709-6-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 1a20bdec416f..52dae78db48f 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.4

[PULL 8/8] 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>
Message-id: 20200429115236.28709-7-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 52dae78db48f..79b9754a5820 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.4

Re: [PULL 0/8] Vga 20200518 patches

[Peter Maydell]

On Mon, 18 May 2020 at 16:18, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> The following changes since commit debe78ce14bf8f8940c2bdf3ef387505e9e035a9:
>
>   Merge remote-tracking branch 'remotes/rth/tags/pull-fpu-20200515' into staging (2020-05-15 19:51:16 +0100)
>
> are available in the Git repository at:
>
>   git://git.kraxel.org/qemu tags/vga-20200518-pull-request
>
> for you to fetch changes up to 3fcf15df0073a76d37e2816597771d4c9763e413:
>
>   ramfb: fix size calculation (2020-05-18 15:43:51 +0200)
>
> ----------------------------------------------------------------
> vga: ati-vga bugfix, ramfb cleanups and fixes.
>


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.1
for any user-visible changes.

-- PMM