From b0588cb51da6986715294bfec4b52f55612a666e Mon Sep 17 00:00:00 2001 From: BALATON Zoltan Date: Sat, 16 May 2020 15:13:03 +0200 Subject: [PATCH 1/8] ati-vga: Do not allow unaligned access via index register 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 Tested-by: Alexander Bulekov Acked-by: Alexander Bulekov Message-id: 20200516132352.39E9374594E@zero.eik.bme.hu Signed-off-by: Gerd Hoffmann --- 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 58ec8291d4..065f197678 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 */ From 63dc3465d6e2c1c31769b0d099991ee978e6e311 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Mon, 4 May 2020 10:20:02 +0200 Subject: [PATCH 2/8] hw/display: Include local 'framebuffer.h' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "framebuffer.h" header is not an exported include. Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Thomas Huth Reviewed-by: Richard Henderson Message-id: 20200504082003.16298-2-f4bug@amsat.org Signed-off-by: Gerd Hoffmann --- 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 7e2a4556bd..6261bfe65b 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 2b726a10f8..b0513a8fba 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" From 2fc979cb9d717161c1dfde69fa5fe495c1ab03e9 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Wed, 29 Apr 2020 13:52:31 +0200 Subject: [PATCH 3/8] Revert "hw/display/ramfb: initialize fw-config space with xres/ yres" 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 Signed-off-by: Gerd Hoffmann Acked-by: Laszlo Ersek Message-id: 20200429115236.28709-2-kraxel@redhat.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, 6 insertions(+), 30 deletions(-) diff --git a/hw/display/ramfb-standalone.c b/hw/display/ramfb-standalone.c index d76a9d0fe2..b18db97eeb 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 7ba07c80f6..bd4746dc17 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 f4977c66e1..a57a22674d 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/include/hw/display/ramfb.h b/include/hw/display/ramfb.h index f6c2de93b2..b33a2c467b 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/stubs/ramfb.c b/stubs/ramfb.c index 0799093a5d..48143f3354 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; From c326eedc7584b94f6f9f3b8ba61a6e9ff04ad681 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Wed, 29 Apr 2020 13:52:32 +0200 Subject: [PATCH 4/8] Revert "hw/display/ramfb: lock guest resolution after it's set" 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 Signed-off-by: Gerd Hoffmann Reviewed-by: Laszlo Ersek 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 bd4746dc17..9d41c2ad28 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; } From 46a298d60271f03d4f85031827426fca67af2a20 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Wed, 29 Apr 2020 13:52:33 +0200 Subject: [PATCH 5/8] ramfb: drop leftover debug message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Gerd Hoffmann Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Laszlo Ersek 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 9d41c2ad28..228defee56 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); } From 19aaee2a65a26693fe0a30b5a0746f967774e278 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Wed, 29 Apr 2020 13:52:34 +0200 Subject: [PATCH 6/8] ramfb: don't update RAMFBState on errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Laszlo Ersek 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 228defee56..1a20bdec41 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) From 819c83e27895472befbfee67d3d7d089c61d7fbd Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Wed, 29 Apr 2020 13:52:35 +0200 Subject: [PATCH 7/8] ramfb: add sanity checks to ramfb_create_display_surface Signed-off-by: Gerd Hoffmann Reviewed-by: Laszlo Ersek 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 1a20bdec41..52dae78db4 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; } From 3fcf15df0073a76d37e2816597771d4c9763e413 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Wed, 29 Apr 2020 13:52:36 +0200 Subject: [PATCH 8/8] 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 Acked-by: Laszlo Ersek 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 52dae78db4..79b9754a58 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);