[Qemu-devel] [PATCH 0/8] arm/raspi: Make fb handle virtual viewport

[Qemu-devel] [PATCH 0/8] arm/raspi: Make fb handle virtual viewport

[Peter Maydell]

The raspi framebuffer supports a virtual viewport which can
be set up so that the virtual framebuffer size is larger than
the physical screen size, and the displayed area is at some
offset within this virtual framebuffer area. This patchset
implements that support.

To do that I had to do a fair amount of refactoring, because
getting the viewport code to work correctly and prevent the
guest from making it fall over by specifying silly offsets
or sizes requires that we properly validate and clip the
config that the guest sends us.

Note that the documentation in places like
https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface
has various errors in it. The behaviour implemented here
corresponds to how a hardware raspi2 I tested seems to behave,
using test programs based on the code from
http://www.valvers.com/open-software/raspberry-pi/step05-bare-metal-programming-in-c-pt5/

(I was a little bit hampered because I couldn't get the rpi
to actually *display* anything to the HDMI port, so mostly
I was testing the edge case behaviour of attempting to set
and read back various config values.)

This series fixes this bug:
https://bugs.launchpad.net/qemu/+bug/1777672

thanks
-- PMM

Peter Maydell (8):
  hw/misc/bcm2835_fb: Move config fields to their own struct
  hw/misc/bcm2835_property: Track fb settings using BCM2835FBConfig
  hw/display/bcm2835_fb: Drop unused size and pitch fields
  hw/display/bcm2835_fb: Reset resolution, etc correctly
  hw/display/bcm2835_fb: Abstract out calculation of pitch, size
  hw/display/bcm2835_fb: Fix handling of virtual framebuffer
  hw/display/bcm2835_fb: Validate config settings
  hw/display/bcm2835_fb: Validate bcm2835_fb_mbox_push() config

 include/hw/display/bcm2835_fb.h |  59 +++++++--
 hw/display/bcm2835_fb.c         | 218 +++++++++++++++++++-------------
 hw/misc/bcm2835_property.c      | 123 +++++++++---------
 3 files changed, 240 insertions(+), 160 deletions(-)

-- 
2.18.0

[Qemu-devel] [PATCH 1/8] hw/misc/bcm2835_fb: Move config fields to their own struct

[Peter Maydell]

The handling of framebuffer properties in the bcm2835_property code
is a bit clumsy, because for each of the many fb related properties
we try to track the value we're about to set and whether we're going
to be setting a value, and then we hand all the new values off
to the framebuffer via a function which takes them all as separate
arguments. It would be simpler if the property code could easily
copy all the framebuffer's current settings, update them with
the new specified values and then ask the framebuffer to switch
to the new set.

As the first part of this refactoring, pull all the fb config
settings fields in BCM2835FBState out into their own struct.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/display/bcm2835_fb.h |  26 ++++++--
 hw/display/bcm2835_fb.c         | 114 +++++++++++++++++---------------
 hw/misc/bcm2835_property.c      |  28 ++++----
 3 files changed, 94 insertions(+), 74 deletions(-)

diff --git a/include/hw/display/bcm2835_fb.h b/include/hw/display/bcm2835_fb.h
index ae0a3807f2c..8485825ba5f 100644
--- a/include/hw/display/bcm2835_fb.h
+++ b/include/hw/display/bcm2835_fb.h
@@ -17,6 +17,20 @@
 #define TYPE_BCM2835_FB "bcm2835-fb"
 #define BCM2835_FB(obj) OBJECT_CHECK(BCM2835FBState, (obj), TYPE_BCM2835_FB)
 
+/*
+ * Configuration information about the fb which the guest can program
+ * via the mailbox property interface.
+ */
+typedef struct {
+    uint32_t xres, yres;
+    uint32_t xres_virtual, yres_virtual;
+    uint32_t xoffset, yoffset;
+    uint32_t bpp;
+    uint32_t base;
+    uint32_t pixo;
+    uint32_t alpha;
+} BCM2835FBConfig;
+
 typedef struct {
     /*< private >*/
     SysBusDevice busdev;
@@ -31,12 +45,12 @@ typedef struct {
     qemu_irq mbox_irq;
 
     bool lock, invalidate, pending;
-    uint32_t xres, yres;
-    uint32_t xres_virtual, yres_virtual;
-    uint32_t xoffset, yoffset;
-    uint32_t bpp;
-    uint32_t base, pitch, size;
-    uint32_t pixo, alpha;
+
+    BCM2835FBConfig config;
+
+    /* These are just cached values calculated from the config settings */
+    uint32_t size;
+    uint32_t pitch;
 } BCM2835FBState;
 
 void bcm2835_fb_reconfigure(BCM2835FBState *s, uint32_t *xres, uint32_t *yres,
diff --git a/hw/display/bcm2835_fb.c b/hw/display/bcm2835_fb.c
index 3355f4c131b..0ffad49ab8f 100644
--- a/hw/display/bcm2835_fb.c
+++ b/hw/display/bcm2835_fb.c
@@ -52,7 +52,7 @@ static void draw_line_src16(void *opaque, uint8_t *dst, const uint8_t *src,
     int bpp = surface_bits_per_pixel(surface);
 
     while (width--) {
-        switch (s->bpp) {
+        switch (s->config.bpp) {
         case 8:
             /* lookup palette starting at video ram base
              * TODO: cache translation, rather than doing this each time!
@@ -91,7 +91,7 @@ static void draw_line_src16(void *opaque, uint8_t *dst, const uint8_t *src,
             break;
         }
 
-        if (s->pixo == 0) {
+        if (s->config.pixo == 0) {
             /* swap to BGR pixel format */
             uint8_t tmp = r;
             r = b;
@@ -135,12 +135,12 @@ static void fb_update_display(void *opaque)
     int src_width = 0;
     int dest_width = 0;
 
-    if (s->lock || !s->xres) {
+    if (s->lock || !s->config.xres) {
         return;
     }
 
-    src_width = s->xres * (s->bpp >> 3);
-    dest_width = s->xres;
+    src_width = s->config.xres * (s->config.bpp >> 3);
+    dest_width = s->config.xres;
 
     switch (surface_bits_per_pixel(surface)) {
     case 0:
@@ -165,16 +165,18 @@ static void fb_update_display(void *opaque)
     }
 
     if (s->invalidate) {
-        framebuffer_update_memory_section(&s->fbsection, s->dma_mr, s->base,
-                                          s->yres, src_width);
+        framebuffer_update_memory_section(&s->fbsection, s->dma_mr,
+                                          s->config.base,
+                                          s->config.yres, src_width);
     }
 
-    framebuffer_update_display(surface, &s->fbsection, s->xres, s->yres,
+    framebuffer_update_display(surface, &s->fbsection,
+                               s->config.xres, s->config.yres,
                                src_width, dest_width, 0, s->invalidate,
                                draw_line_src16, s, &first, &last);
 
     if (first >= 0) {
-        dpy_gfx_update(s->con, 0, first, s->xres, last - first + 1);
+        dpy_gfx_update(s->con, 0, first, s->config.xres, last - first + 1);
     }
 
     s->invalidate = false;
@@ -186,28 +188,28 @@ static void bcm2835_fb_mbox_push(BCM2835FBState *s, uint32_t value)
 
     s->lock = true;
 
-    s->xres = ldl_le_phys(&s->dma_as, value);
-    s->yres = ldl_le_phys(&s->dma_as, value + 4);
-    s->xres_virtual = ldl_le_phys(&s->dma_as, value + 8);
-    s->yres_virtual = ldl_le_phys(&s->dma_as, value + 12);
-    s->bpp = ldl_le_phys(&s->dma_as, value + 20);
-    s->xoffset = ldl_le_phys(&s->dma_as, value + 24);
-    s->yoffset = ldl_le_phys(&s->dma_as, value + 28);
+    s->config.xres = ldl_le_phys(&s->dma_as, value);
+    s->config.yres = ldl_le_phys(&s->dma_as, value + 4);
+    s->config.xres_virtual = ldl_le_phys(&s->dma_as, value + 8);
+    s->config.yres_virtual = ldl_le_phys(&s->dma_as, value + 12);
+    s->config.bpp = ldl_le_phys(&s->dma_as, value + 20);
+    s->config.xoffset = ldl_le_phys(&s->dma_as, value + 24);
+    s->config.yoffset = ldl_le_phys(&s->dma_as, value + 28);
 
-    s->base = s->vcram_base | (value & 0xc0000000);
-    s->base += BCM2835_FB_OFFSET;
+    s->config.base = s->vcram_base | (value & 0xc0000000);
+    s->config.base += BCM2835_FB_OFFSET;
 
     /* TODO - Manage properly virtual resolution */
 
-    s->pitch = s->xres * (s->bpp >> 3);
-    s->size = s->yres * s->pitch;
+    s->pitch = s->config.xres * (s->config.bpp >> 3);
+    s->size = s->config.yres * s->pitch;
 
     stl_le_phys(&s->dma_as, value + 16, s->pitch);
-    stl_le_phys(&s->dma_as, value + 32, s->base);
+    stl_le_phys(&s->dma_as, value + 32, s->config.base);
     stl_le_phys(&s->dma_as, value + 36, s->size);
 
     s->invalidate = true;
-    qemu_console_resize(s->con, s->xres, s->yres);
+    qemu_console_resize(s->con, s->config.xres, s->config.yres);
     s->lock = false;
 }
 
@@ -219,34 +221,34 @@ void bcm2835_fb_reconfigure(BCM2835FBState *s, uint32_t *xres, uint32_t *yres,
 
     /* TODO: input validation! */
     if (xres) {
-        s->xres = *xres;
+        s->config.xres = *xres;
     }
     if (yres) {
-        s->yres = *yres;
+        s->config.yres = *yres;
     }
     if (xoffset) {
-        s->xoffset = *xoffset;
+        s->config.xoffset = *xoffset;
     }
     if (yoffset) {
-        s->yoffset = *yoffset;
+        s->config.yoffset = *yoffset;
     }
     if (bpp) {
-        s->bpp = *bpp;
+        s->config.bpp = *bpp;
     }
     if (pixo) {
-        s->pixo = *pixo;
+        s->config.pixo = *pixo;
     }
     if (alpha) {
-        s->alpha = *alpha;
+        s->config.alpha = *alpha;
     }
 
     /* TODO - Manage properly virtual resolution */
 
-    s->pitch = s->xres * (s->bpp >> 3);
-    s->size = s->yres * s->pitch;
+    s->pitch = s->config.xres * (s->config.bpp >> 3);
+    s->size = s->config.yres * s->pitch;
 
     s->invalidate = true;
-    qemu_console_resize(s->con, s->xres, s->yres);
+    qemu_console_resize(s->con, s->config.xres, s->config.yres);
     s->lock = false;
 }
 
@@ -312,18 +314,18 @@ static const VMStateDescription vmstate_bcm2835_fb = {
         VMSTATE_BOOL(lock, BCM2835FBState),
         VMSTATE_BOOL(invalidate, BCM2835FBState),
         VMSTATE_BOOL(pending, BCM2835FBState),
-        VMSTATE_UINT32(xres, BCM2835FBState),
-        VMSTATE_UINT32(yres, BCM2835FBState),
-        VMSTATE_UINT32(xres_virtual, BCM2835FBState),
-        VMSTATE_UINT32(yres_virtual, BCM2835FBState),
-        VMSTATE_UINT32(xoffset, BCM2835FBState),
-        VMSTATE_UINT32(yoffset, BCM2835FBState),
-        VMSTATE_UINT32(bpp, BCM2835FBState),
-        VMSTATE_UINT32(base, BCM2835FBState),
+        VMSTATE_UINT32(config.xres, BCM2835FBState),
+        VMSTATE_UINT32(config.yres, BCM2835FBState),
+        VMSTATE_UINT32(config.xres_virtual, BCM2835FBState),
+        VMSTATE_UINT32(config.yres_virtual, BCM2835FBState),
+        VMSTATE_UINT32(config.xoffset, BCM2835FBState),
+        VMSTATE_UINT32(config.yoffset, BCM2835FBState),
+        VMSTATE_UINT32(config.bpp, BCM2835FBState),
+        VMSTATE_UINT32(config.base, BCM2835FBState),
         VMSTATE_UINT32(pitch, BCM2835FBState),
         VMSTATE_UINT32(size, BCM2835FBState),
-        VMSTATE_UINT32(pixo, BCM2835FBState),
-        VMSTATE_UINT32(alpha, BCM2835FBState),
+        VMSTATE_UINT32(config.pixo, BCM2835FBState),
+        VMSTATE_UINT32(config.alpha, BCM2835FBState),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -349,13 +351,13 @@ static void bcm2835_fb_reset(DeviceState *dev)
 
     s->pending = false;
 
-    s->xres_virtual = s->xres;
-    s->yres_virtual = s->yres;
-    s->xoffset = 0;
-    s->yoffset = 0;
-    s->base = s->vcram_base + BCM2835_FB_OFFSET;
-    s->pitch = s->xres * (s->bpp >> 3);
-    s->size = s->yres * s->pitch;
+    s->config.xres_virtual = s->config.xres;
+    s->config.yres_virtual = s->config.yres;
+    s->config.xoffset = 0;
+    s->config.yoffset = 0;
+    s->config.base = s->vcram_base + BCM2835_FB_OFFSET;
+    s->pitch = s->config.xres * (s->config.bpp >> 3);
+    s->size = s->config.yres * s->pitch;
 
     s->invalidate = true;
     s->lock = false;
@@ -385,18 +387,20 @@ static void bcm2835_fb_realize(DeviceState *dev, Error **errp)
     bcm2835_fb_reset(dev);
 
     s->con = graphic_console_init(dev, 0, &vgafb_ops, s);
-    qemu_console_resize(s->con, s->xres, s->yres);
+    qemu_console_resize(s->con, s->config.xres, s->config.yres);
 }
 
 static Property bcm2835_fb_props[] = {
     DEFINE_PROP_UINT32("vcram-base", BCM2835FBState, vcram_base, 0),/*required*/
     DEFINE_PROP_UINT32("vcram-size", BCM2835FBState, vcram_size,
                        DEFAULT_VCRAM_SIZE),
-    DEFINE_PROP_UINT32("xres", BCM2835FBState, xres, 640),
-    DEFINE_PROP_UINT32("yres", BCM2835FBState, yres, 480),
-    DEFINE_PROP_UINT32("bpp", BCM2835FBState, bpp, 16),
-    DEFINE_PROP_UINT32("pixo", BCM2835FBState, pixo, 1), /* 1=RGB, 0=BGR */
-    DEFINE_PROP_UINT32("alpha", BCM2835FBState, alpha, 2), /* alpha ignored */
+    DEFINE_PROP_UINT32("xres", BCM2835FBState, config.xres, 640),
+    DEFINE_PROP_UINT32("yres", BCM2835FBState, config.yres, 480),
+    DEFINE_PROP_UINT32("bpp", BCM2835FBState, config.bpp, 16),
+    DEFINE_PROP_UINT32("pixo",
+                       BCM2835FBState, config.pixo, 1), /* 1=RGB, 0=BGR */
+    DEFINE_PROP_UINT32("alpha",
+                       BCM2835FBState, config.alpha, 2), /* alpha ignored */
     DEFINE_PROP_END_OF_LIST()
 };
 
diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c
index 70eaafd325f..c79f358702d 100644
--- a/hw/misc/bcm2835_property.c
+++ b/hw/misc/bcm2835_property.c
@@ -141,10 +141,10 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
         /* Frame buffer */
 
         case 0x00040001: /* Allocate buffer */
-            stl_le_phys(&s->dma_as, value + 12, s->fbdev->base);
-            tmp_xres = newxres != NULL ? *newxres : s->fbdev->xres;
-            tmp_yres = newyres != NULL ? *newyres : s->fbdev->yres;
-            tmp_bpp = newbpp != NULL ? *newbpp : s->fbdev->bpp;
+            stl_le_phys(&s->dma_as, value + 12, s->fbdev->config.base);
+            tmp_xres = newxres != NULL ? *newxres : s->fbdev->config.xres;
+            tmp_yres = newyres != NULL ? *newyres : s->fbdev->config.yres;
+            tmp_bpp = newbpp != NULL ? *newbpp : s->fbdev->config.bpp;
             stl_le_phys(&s->dma_as, value + 16,
                         tmp_xres * tmp_yres * tmp_bpp / 8);
             resplen = 8;
@@ -157,8 +157,8 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
             break;
         case 0x00040003: /* Get display width/height */
         case 0x00040004:
-            tmp_xres = newxres != NULL ? *newxres : s->fbdev->xres;
-            tmp_yres = newyres != NULL ? *newyres : s->fbdev->yres;
+            tmp_xres = newxres != NULL ? *newxres : s->fbdev->config.xres;
+            tmp_yres = newyres != NULL ? *newyres : s->fbdev->config.yres;
             stl_le_phys(&s->dma_as, value + 12, tmp_xres);
             stl_le_phys(&s->dma_as, value + 16, tmp_yres);
             resplen = 8;
@@ -176,7 +176,7 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
             resplen = 8;
             break;
         case 0x00040005: /* Get depth */
-            tmp_bpp = newbpp != NULL ? *newbpp : s->fbdev->bpp;
+            tmp_bpp = newbpp != NULL ? *newbpp : s->fbdev->config.bpp;
             stl_le_phys(&s->dma_as, value + 12, tmp_bpp);
             resplen = 4;
             break;
@@ -189,7 +189,7 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
             resplen = 4;
             break;
         case 0x00040006: /* Get pixel order */
-            tmp_pixo = newpixo != NULL ? *newpixo : s->fbdev->pixo;
+            tmp_pixo = newpixo != NULL ? *newpixo : s->fbdev->config.pixo;
             stl_le_phys(&s->dma_as, value + 12, tmp_pixo);
             resplen = 4;
             break;
@@ -202,7 +202,7 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
             resplen = 4;
             break;
         case 0x00040007: /* Get alpha */
-            tmp_alpha = newalpha != NULL ? *newalpha : s->fbdev->alpha;
+            tmp_alpha = newalpha != NULL ? *newalpha : s->fbdev->config.alpha;
             stl_le_phys(&s->dma_as, value + 12, tmp_alpha);
             resplen = 4;
             break;
@@ -215,14 +215,16 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
             resplen = 4;
             break;
         case 0x00040008: /* Get pitch */
-            tmp_xres = newxres != NULL ? *newxres : s->fbdev->xres;
-            tmp_bpp = newbpp != NULL ? *newbpp : s->fbdev->bpp;
+            tmp_xres = newxres != NULL ? *newxres : s->fbdev->config.xres;
+            tmp_bpp = newbpp != NULL ? *newbpp : s->fbdev->config.bpp;
             stl_le_phys(&s->dma_as, value + 12, tmp_xres * tmp_bpp / 8);
             resplen = 4;
             break;
         case 0x00040009: /* Get virtual offset */
-            tmp_xoffset = newxoffset != NULL ? *newxoffset : s->fbdev->xoffset;
-            tmp_yoffset = newyoffset != NULL ? *newyoffset : s->fbdev->yoffset;
+            tmp_xoffset = newxoffset != NULL ?
+                *newxoffset : s->fbdev->config.xoffset;
+            tmp_yoffset = newyoffset != NULL ?
+                *newyoffset : s->fbdev->config.yoffset;
             stl_le_phys(&s->dma_as, value + 12, tmp_xoffset);
             stl_le_phys(&s->dma_as, value + 16, tmp_yoffset);
             resplen = 8;
-- 
2.18.0

[Qemu-devel] [PATCH 2/8] hw/misc/bcm2835_property: Track fb settings using BCM2835FBConfig

[Peter Maydell]

Refactor the fb property setting code so that rather than
using a set of pointers to local variables to track
whether a config value has been updated in the current
mbox and if so what its new value is, we just copy
all the current settings of the fb at the start, and
then update that copy as we go along, before asking
the fb to switch to it at the end.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/display/bcm2835_fb.h |  4 +-
 hw/display/bcm2835_fb.c         | 27 ++---------
 hw/misc/bcm2835_property.c      | 80 ++++++++++++++-------------------
 3 files changed, 37 insertions(+), 74 deletions(-)

diff --git a/include/hw/display/bcm2835_fb.h b/include/hw/display/bcm2835_fb.h
index 8485825ba5f..b965698d28a 100644
--- a/include/hw/display/bcm2835_fb.h
+++ b/include/hw/display/bcm2835_fb.h
@@ -53,8 +53,6 @@ typedef struct {
     uint32_t pitch;
 } BCM2835FBState;
 
-void bcm2835_fb_reconfigure(BCM2835FBState *s, uint32_t *xres, uint32_t *yres,
-                            uint32_t *xoffset, uint32_t *yoffset, uint32_t *bpp,
-                            uint32_t *pixo, uint32_t *alpha);
+void bcm2835_fb_reconfigure(BCM2835FBState *s, BCM2835FBConfig *newconfig);
 
 #endif
diff --git a/hw/display/bcm2835_fb.c b/hw/display/bcm2835_fb.c
index 0ffad49ab8f..8155de5d0b1 100644
--- a/hw/display/bcm2835_fb.c
+++ b/hw/display/bcm2835_fb.c
@@ -213,34 +213,13 @@ static void bcm2835_fb_mbox_push(BCM2835FBState *s, uint32_t value)
     s->lock = false;
 }
 
-void bcm2835_fb_reconfigure(BCM2835FBState *s, uint32_t *xres, uint32_t *yres,
-                            uint32_t *xoffset, uint32_t *yoffset, uint32_t *bpp,
-                            uint32_t *pixo, uint32_t *alpha)
+void bcm2835_fb_reconfigure(BCM2835FBState *s, BCM2835FBConfig *newconfig)
 {
     s->lock = true;
 
     /* TODO: input validation! */
-    if (xres) {
-        s->config.xres = *xres;
-    }
-    if (yres) {
-        s->config.yres = *yres;
-    }
-    if (xoffset) {
-        s->config.xoffset = *xoffset;
-    }
-    if (yoffset) {
-        s->config.yoffset = *yoffset;
-    }
-    if (bpp) {
-        s->config.bpp = *bpp;
-    }
-    if (pixo) {
-        s->config.pixo = *pixo;
-    }
-    if (alpha) {
-        s->config.alpha = *alpha;
-    }
+
+    s->config = *newconfig;
 
     /* TODO - Manage properly virtual resolution */
 
diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c
index c79f358702d..df0645d1b84 100644
--- a/hw/misc/bcm2835_property.c
+++ b/hw/misc/bcm2835_property.c
@@ -21,11 +21,14 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
     uint32_t tmp;
     int n;
     uint32_t offset, length, color;
-    uint32_t xres, yres, xoffset, yoffset, bpp, pixo, alpha;
-    uint32_t tmp_xres, tmp_yres, tmp_xoffset, tmp_yoffset;
-    uint32_t tmp_bpp, tmp_pixo, tmp_alpha;
-    uint32_t *newxres = NULL, *newyres = NULL, *newxoffset = NULL,
-        *newyoffset = NULL, *newbpp = NULL, *newpixo = NULL, *newalpha = NULL;
+
+    /*
+     * Copy the current state of the framebuffer config; we will update
+     * this copy as we process tags and then ask the framebuffer to use
+     * it at the end.
+     */
+    BCM2835FBConfig fbconfig = s->fbdev->config;
+    bool fbconfig_updated = false;
 
     value &= ~0xf;
 
@@ -141,12 +144,9 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
         /* Frame buffer */
 
         case 0x00040001: /* Allocate buffer */
-            stl_le_phys(&s->dma_as, value + 12, s->fbdev->config.base);
-            tmp_xres = newxres != NULL ? *newxres : s->fbdev->config.xres;
-            tmp_yres = newyres != NULL ? *newyres : s->fbdev->config.yres;
-            tmp_bpp = newbpp != NULL ? *newbpp : s->fbdev->config.bpp;
+            stl_le_phys(&s->dma_as, value + 12, fbconfig.base);
             stl_le_phys(&s->dma_as, value + 16,
-                        tmp_xres * tmp_yres * tmp_bpp / 8);
+                        fbconfig.xres * fbconfig.yres * fbconfig.bpp / 8);
             resplen = 8;
             break;
         case 0x00048001: /* Release buffer */
@@ -157,10 +157,8 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
             break;
         case 0x00040003: /* Get display width/height */
         case 0x00040004:
-            tmp_xres = newxres != NULL ? *newxres : s->fbdev->config.xres;
-            tmp_yres = newyres != NULL ? *newyres : s->fbdev->config.yres;
-            stl_le_phys(&s->dma_as, value + 12, tmp_xres);
-            stl_le_phys(&s->dma_as, value + 16, tmp_yres);
+            stl_le_phys(&s->dma_as, value + 12, fbconfig.xres);
+            stl_le_phys(&s->dma_as, value + 16, fbconfig.yres);
             resplen = 8;
             break;
         case 0x00044003: /* Test display width/height */
@@ -169,74 +167,64 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
             break;
         case 0x00048003: /* Set display width/height */
         case 0x00048004:
-            xres = ldl_le_phys(&s->dma_as, value + 12);
-            newxres = &xres;
-            yres = ldl_le_phys(&s->dma_as, value + 16);
-            newyres = &yres;
+            fbconfig.xres = ldl_le_phys(&s->dma_as, value + 12);
+            fbconfig.yres = ldl_le_phys(&s->dma_as, value + 16);
+            fbconfig_updated = true;
             resplen = 8;
             break;
         case 0x00040005: /* Get depth */
-            tmp_bpp = newbpp != NULL ? *newbpp : s->fbdev->config.bpp;
-            stl_le_phys(&s->dma_as, value + 12, tmp_bpp);
+            stl_le_phys(&s->dma_as, value + 12, fbconfig.bpp);
             resplen = 4;
             break;
         case 0x00044005: /* Test depth */
             resplen = 4;
             break;
         case 0x00048005: /* Set depth */
-            bpp = ldl_le_phys(&s->dma_as, value + 12);
-            newbpp = &bpp;
+            fbconfig.bpp = ldl_le_phys(&s->dma_as, value + 12);
+            fbconfig_updated = true;
             resplen = 4;
             break;
         case 0x00040006: /* Get pixel order */
-            tmp_pixo = newpixo != NULL ? *newpixo : s->fbdev->config.pixo;
-            stl_le_phys(&s->dma_as, value + 12, tmp_pixo);
+            stl_le_phys(&s->dma_as, value + 12, fbconfig.pixo);
             resplen = 4;
             break;
         case 0x00044006: /* Test pixel order */
             resplen = 4;
             break;
         case 0x00048006: /* Set pixel order */
-            pixo = ldl_le_phys(&s->dma_as, value + 12);
-            newpixo = &pixo;
+            fbconfig.pixo = ldl_le_phys(&s->dma_as, value + 12);
+            fbconfig_updated = true;
             resplen = 4;
             break;
         case 0x00040007: /* Get alpha */
-            tmp_alpha = newalpha != NULL ? *newalpha : s->fbdev->config.alpha;
-            stl_le_phys(&s->dma_as, value + 12, tmp_alpha);
+            stl_le_phys(&s->dma_as, value + 12, fbconfig.alpha);
             resplen = 4;
             break;
         case 0x00044007: /* Test pixel alpha */
             resplen = 4;
             break;
         case 0x00048007: /* Set alpha */
-            alpha = ldl_le_phys(&s->dma_as, value + 12);
-            newalpha = &alpha;
+            fbconfig.alpha = ldl_le_phys(&s->dma_as, value + 12);
+            fbconfig_updated = true;
             resplen = 4;
             break;
         case 0x00040008: /* Get pitch */
-            tmp_xres = newxres != NULL ? *newxres : s->fbdev->config.xres;
-            tmp_bpp = newbpp != NULL ? *newbpp : s->fbdev->config.bpp;
-            stl_le_phys(&s->dma_as, value + 12, tmp_xres * tmp_bpp / 8);
+            stl_le_phys(&s->dma_as, value + 12,
+                        fbconfig.xres * fbconfig.bpp / 8);
             resplen = 4;
             break;
         case 0x00040009: /* Get virtual offset */
-            tmp_xoffset = newxoffset != NULL ?
-                *newxoffset : s->fbdev->config.xoffset;
-            tmp_yoffset = newyoffset != NULL ?
-                *newyoffset : s->fbdev->config.yoffset;
-            stl_le_phys(&s->dma_as, value + 12, tmp_xoffset);
-            stl_le_phys(&s->dma_as, value + 16, tmp_yoffset);
+            stl_le_phys(&s->dma_as, value + 12, fbconfig.xoffset);
+            stl_le_phys(&s->dma_as, value + 16, fbconfig.yoffset);
             resplen = 8;
             break;
         case 0x00044009: /* Test virtual offset */
             resplen = 8;
             break;
         case 0x00048009: /* Set virtual offset */
-            xoffset = ldl_le_phys(&s->dma_as, value + 12);
-            newxoffset = &xoffset;
-            yoffset = ldl_le_phys(&s->dma_as, value + 16);
-            newyoffset = &yoffset;
+            fbconfig.xoffset = ldl_le_phys(&s->dma_as, value + 12);
+            fbconfig.yoffset = ldl_le_phys(&s->dma_as, value + 16);
+            fbconfig_updated = true;
             resplen = 8;
             break;
         case 0x0004000a: /* Get/Test/Set overscan */
@@ -287,10 +275,8 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
     }
 
     /* Reconfigure framebuffer if required */
-    if (newxres || newyres || newxoffset || newyoffset || newbpp || newpixo
-        || newalpha) {
-        bcm2835_fb_reconfigure(s->fbdev, newxres, newyres, newxoffset,
-                               newyoffset, newbpp, newpixo, newalpha);
+    if (fbconfig_updated) {
+        bcm2835_fb_reconfigure(s->fbdev, &fbconfig);
     }
 
     /* Buffer response code */
-- 
2.18.0

[Qemu-devel] [PATCH 3/8] hw/display/bcm2835_fb: Drop unused size and pitch fields

[Peter Maydell]

The BCM2835FBState struct has a 'pitch' field which is a
cached copy of xres * (bpp >> 3), and a 'size' field which is
a cached copy of pitch * yres. However we don't actually do
anything with these fields; delete them. We retain the
now-unused slots in the VMState struct for migration
compatibility.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/display/bcm2835_fb.h |  4 ----
 hw/display/bcm2835_fb.c         | 19 ++++++++-----------
 2 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/include/hw/display/bcm2835_fb.h b/include/hw/display/bcm2835_fb.h
index b965698d28a..69cbf2d1fd9 100644
--- a/include/hw/display/bcm2835_fb.h
+++ b/include/hw/display/bcm2835_fb.h
@@ -47,10 +47,6 @@ typedef struct {
     bool lock, invalidate, pending;
 
     BCM2835FBConfig config;
-
-    /* These are just cached values calculated from the config settings */
-    uint32_t size;
-    uint32_t pitch;
 } BCM2835FBState;
 
 void bcm2835_fb_reconfigure(BCM2835FBState *s, BCM2835FBConfig *newconfig);
diff --git a/hw/display/bcm2835_fb.c b/hw/display/bcm2835_fb.c
index 8155de5d0b1..9faabf0d0b4 100644
--- a/hw/display/bcm2835_fb.c
+++ b/hw/display/bcm2835_fb.c
@@ -184,6 +184,9 @@ static void fb_update_display(void *opaque)
 
 static void bcm2835_fb_mbox_push(BCM2835FBState *s, uint32_t value)
 {
+    uint32_t pitch;
+    uint32_t size;
+
     value &= ~0xf;
 
     s->lock = true;
@@ -201,12 +204,12 @@ static void bcm2835_fb_mbox_push(BCM2835FBState *s, uint32_t value)
 
     /* TODO - Manage properly virtual resolution */
 
-    s->pitch = s->config.xres * (s->config.bpp >> 3);
-    s->size = s->config.yres * s->pitch;
+    pitch = s->config.xres * (s->config.bpp >> 3);
+    size = s->config.yres * pitch;
 
-    stl_le_phys(&s->dma_as, value + 16, s->pitch);
+    stl_le_phys(&s->dma_as, value + 16, pitch);
     stl_le_phys(&s->dma_as, value + 32, s->config.base);
-    stl_le_phys(&s->dma_as, value + 36, s->size);
+    stl_le_phys(&s->dma_as, value + 36, size);
 
     s->invalidate = true;
     qemu_console_resize(s->con, s->config.xres, s->config.yres);
@@ -223,9 +226,6 @@ void bcm2835_fb_reconfigure(BCM2835FBState *s, BCM2835FBConfig *newconfig)
 
     /* TODO - Manage properly virtual resolution */
 
-    s->pitch = s->config.xres * (s->config.bpp >> 3);
-    s->size = s->config.yres * s->pitch;
-
     s->invalidate = true;
     qemu_console_resize(s->con, s->config.xres, s->config.yres);
     s->lock = false;
@@ -301,8 +301,7 @@ static const VMStateDescription vmstate_bcm2835_fb = {
         VMSTATE_UINT32(config.yoffset, BCM2835FBState),
         VMSTATE_UINT32(config.bpp, BCM2835FBState),
         VMSTATE_UINT32(config.base, BCM2835FBState),
-        VMSTATE_UINT32(pitch, BCM2835FBState),
-        VMSTATE_UINT32(size, BCM2835FBState),
+        VMSTATE_UNUSED(8), /* Was pitch and size */
         VMSTATE_UINT32(config.pixo, BCM2835FBState),
         VMSTATE_UINT32(config.alpha, BCM2835FBState),
         VMSTATE_END_OF_LIST()
@@ -335,8 +334,6 @@ static void bcm2835_fb_reset(DeviceState *dev)
     s->config.xoffset = 0;
     s->config.yoffset = 0;
     s->config.base = s->vcram_base + BCM2835_FB_OFFSET;
-    s->pitch = s->config.xres * (s->config.bpp >> 3);
-    s->size = s->config.yres * s->pitch;
 
     s->invalidate = true;
     s->lock = false;
-- 
2.18.0

[Qemu-devel] [PATCH 4/8] hw/display/bcm2835_fb: Reset resolution, etc correctly

[Peter Maydell]

The bcm2835_fb's initial resolution and other parameters are set
via QOM properties. We should reset to those initial values on
device reset, which means we need to save the QOM property
values somewhere that they are not overwritten by guest
changes to the framebuffer configuration.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/display/bcm2835_fb.h |  1 +
 hw/display/bcm2835_fb.c         | 27 +++++++++++++++------------
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/include/hw/display/bcm2835_fb.h b/include/hw/display/bcm2835_fb.h
index 69cbf2d1fd9..374de546121 100644
--- a/include/hw/display/bcm2835_fb.h
+++ b/include/hw/display/bcm2835_fb.h
@@ -47,6 +47,7 @@ typedef struct {
     bool lock, invalidate, pending;
 
     BCM2835FBConfig config;
+    BCM2835FBConfig initial_config;
 } BCM2835FBState;
 
 void bcm2835_fb_reconfigure(BCM2835FBState *s, BCM2835FBConfig *newconfig);
diff --git a/hw/display/bcm2835_fb.c b/hw/display/bcm2835_fb.c
index 9faabf0d0b4..d95686c74c8 100644
--- a/hw/display/bcm2835_fb.c
+++ b/hw/display/bcm2835_fb.c
@@ -329,11 +329,7 @@ static void bcm2835_fb_reset(DeviceState *dev)
 
     s->pending = false;
 
-    s->config.xres_virtual = s->config.xres;
-    s->config.yres_virtual = s->config.yres;
-    s->config.xoffset = 0;
-    s->config.yoffset = 0;
-    s->config.base = s->vcram_base + BCM2835_FB_OFFSET;
+    s->config = s->initial_config;
 
     s->invalidate = true;
     s->lock = false;
@@ -357,6 +353,13 @@ static void bcm2835_fb_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    /* Fill in the parts of initial_config that are not set by QOM properties */
+    s->initial_config.xres_virtual = s->initial_config.xres;
+    s->initial_config.yres_virtual = s->initial_config.yres;
+    s->initial_config.xoffset = 0;
+    s->initial_config.yoffset = 0;
+    s->initial_config.base = s->vcram_base + BCM2835_FB_OFFSET;
+
     s->dma_mr = MEMORY_REGION(obj);
     address_space_init(&s->dma_as, s->dma_mr, NULL);
 
@@ -370,13 +373,13 @@ static Property bcm2835_fb_props[] = {
     DEFINE_PROP_UINT32("vcram-base", BCM2835FBState, vcram_base, 0),/*required*/
     DEFINE_PROP_UINT32("vcram-size", BCM2835FBState, vcram_size,
                        DEFAULT_VCRAM_SIZE),
-    DEFINE_PROP_UINT32("xres", BCM2835FBState, config.xres, 640),
-    DEFINE_PROP_UINT32("yres", BCM2835FBState, config.yres, 480),
-    DEFINE_PROP_UINT32("bpp", BCM2835FBState, config.bpp, 16),
-    DEFINE_PROP_UINT32("pixo",
-                       BCM2835FBState, config.pixo, 1), /* 1=RGB, 0=BGR */
-    DEFINE_PROP_UINT32("alpha",
-                       BCM2835FBState, config.alpha, 2), /* alpha ignored */
+    DEFINE_PROP_UINT32("xres", BCM2835FBState, initial_config.xres, 640),
+    DEFINE_PROP_UINT32("yres", BCM2835FBState, initial_config.yres, 480),
+    DEFINE_PROP_UINT32("bpp", BCM2835FBState, initial_config.bpp, 16),
+    DEFINE_PROP_UINT32("pixo", BCM2835FBState,
+                       initial_config.pixo, 1), /* 1=RGB, 0=BGR */
+    DEFINE_PROP_UINT32("alpha", BCM2835FBState,
+                       initial_config.alpha, 2), /* alpha ignored */
     DEFINE_PROP_END_OF_LIST()
 };
 
-- 
2.18.0

[Qemu-devel] [PATCH 5/8] hw/display/bcm2835_fb: Abstract out calculation of pitch, size

[Peter Maydell]

Abstract out the calculation of the pitch and size of the
framebuffer into functions that operate on the BCM2835FBConfig
struct -- these are about to get a little more complicated
when we add support for virtual and physical sizes differing.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/display/bcm2835_fb.h | 22 ++++++++++++++++++++++
 hw/display/bcm2835_fb.c         |  6 +++---
 hw/misc/bcm2835_property.c      |  4 ++--
 3 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/include/hw/display/bcm2835_fb.h b/include/hw/display/bcm2835_fb.h
index 374de546121..95bcec7fe33 100644
--- a/include/hw/display/bcm2835_fb.h
+++ b/include/hw/display/bcm2835_fb.h
@@ -52,4 +52,26 @@ typedef struct {
 
 void bcm2835_fb_reconfigure(BCM2835FBState *s, BCM2835FBConfig *newconfig);
 
+/**
+ * bcm2835_fb_get_pitch: return number of bytes per line of the framebuffer
+ * @config: configuration info for the framebuffer
+ *
+ * Return the number of bytes per line of the framebuffer, ie the number
+ * that must be added to a pixel address to get the address of the pixel
+ * directly below it on screen.
+ */
+static inline uint32_t bcm2835_fb_get_pitch(BCM2835FBConfig *config)
+{
+    return config->xres * (config->bpp >> 3);
+}
+
+/**
+ * bcm2835_fb_get_size: return total size of framebuffer in bytes
+ * @config: configuration info for the framebuffer
+ */
+static inline uint32_t bcm2835_fb_get_size(BCM2835FBConfig *config)
+{
+    return config->yres * bcm2835_fb_get_pitch(config);
+}
+
 #endif
diff --git a/hw/display/bcm2835_fb.c b/hw/display/bcm2835_fb.c
index d95686c74c8..a6c0a0cc942 100644
--- a/hw/display/bcm2835_fb.c
+++ b/hw/display/bcm2835_fb.c
@@ -139,7 +139,7 @@ static void fb_update_display(void *opaque)
         return;
     }
 
-    src_width = s->config.xres * (s->config.bpp >> 3);
+    src_width = bcm2835_fb_get_pitch(&s->config);
     dest_width = s->config.xres;
 
     switch (surface_bits_per_pixel(surface)) {
@@ -204,8 +204,8 @@ static void bcm2835_fb_mbox_push(BCM2835FBState *s, uint32_t value)
 
     /* TODO - Manage properly virtual resolution */
 
-    pitch = s->config.xres * (s->config.bpp >> 3);
-    size = s->config.yres * pitch;
+    pitch = bcm2835_fb_get_pitch(&s->config);
+    size = bcm2835_fb_get_size(&s->config);
 
     stl_le_phys(&s->dma_as, value + 16, pitch);
     stl_le_phys(&s->dma_as, value + 32, s->config.base);
diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c
index df0645d1b84..c8c4979bd26 100644
--- a/hw/misc/bcm2835_property.c
+++ b/hw/misc/bcm2835_property.c
@@ -146,7 +146,7 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
         case 0x00040001: /* Allocate buffer */
             stl_le_phys(&s->dma_as, value + 12, fbconfig.base);
             stl_le_phys(&s->dma_as, value + 16,
-                        fbconfig.xres * fbconfig.yres * fbconfig.bpp / 8);
+                        bcm2835_fb_get_size(&fbconfig));
             resplen = 8;
             break;
         case 0x00048001: /* Release buffer */
@@ -210,7 +210,7 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
             break;
         case 0x00040008: /* Get pitch */
             stl_le_phys(&s->dma_as, value + 12,
-                        fbconfig.xres * fbconfig.bpp / 8);
+                        bcm2835_fb_get_pitch(&fbconfig));
             resplen = 4;
             break;
         case 0x00040009: /* Get virtual offset */
-- 
2.18.0

[Qemu-devel] [PATCH 6/8] hw/display/bcm2835_fb: Fix handling of virtual framebuffer

[Peter Maydell]

The raspi framebuffir in bcm2835_fb supports the definition
of a virtual "viewport", which is smaller than the full
physical framebuffer size and at an adjustable offset within
it. Only the viewport area is sent to the screen. This allows
the guest to do things like double buffering, or scrolling
by adjusting the viewport origin. Currently QEMU doesn't
implement this at all.

Add support for this feature:
 * the property mailbox code needs to distinguish the
   virtual width/height from the physical width/height
 * the framebuffer code needs to do something with the
   virtual width/height/origin information

Note that the wiki documentation on the semantics of the
virtual and physical height and width has it the wrong way
around -- the virtual size is the size of the allocated
buffer, and the physical size is the size of the display,
so the virtual size is always the same as or larger than
the physical.

If the viewport size is set smaller than the physical
screen size, we ignore the viewport settings completely
and just display the physical screen area.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/display/bcm2835_fb.h |  6 ++++--
 hw/display/bcm2835_fb.c         | 28 ++++++++++++++++++++++------
 hw/misc/bcm2835_property.c      | 21 +++++++++++++++------
 3 files changed, 41 insertions(+), 14 deletions(-)

diff --git a/include/hw/display/bcm2835_fb.h b/include/hw/display/bcm2835_fb.h
index 95bcec7fe33..d992c60c120 100644
--- a/include/hw/display/bcm2835_fb.h
+++ b/include/hw/display/bcm2835_fb.h
@@ -62,7 +62,8 @@ void bcm2835_fb_reconfigure(BCM2835FBState *s, BCM2835FBConfig *newconfig);
  */
 static inline uint32_t bcm2835_fb_get_pitch(BCM2835FBConfig *config)
 {
-    return config->xres * (config->bpp >> 3);
+    uint32_t xres = MAX(config->xres, config->xres_virtual);
+    return xres * (config->bpp >> 3);
 }
 
 /**
@@ -71,7 +72,8 @@ static inline uint32_t bcm2835_fb_get_pitch(BCM2835FBConfig *config)
  */
 static inline uint32_t bcm2835_fb_get_size(BCM2835FBConfig *config)
 {
-    return config->yres * bcm2835_fb_get_pitch(config);
+    uint32_t yres = MAX(config->yres, config->yres_virtual);
+    return yres * bcm2835_fb_get_pitch(config);
 }
 
 #endif
diff --git a/hw/display/bcm2835_fb.c b/hw/display/bcm2835_fb.c
index a6c0a0cc942..76a10072b46 100644
--- a/hw/display/bcm2835_fb.c
+++ b/hw/display/bcm2835_fb.c
@@ -126,6 +126,18 @@ static void draw_line_src16(void *opaque, uint8_t *dst, const uint8_t *src,
     }
 }
 
+static bool fb_use_offsets(BCM2835FBConfig *config)
+{
+    /*
+     * Return true if we should use the viewport offsets.
+     * Experimentally, the hardware seems to do this only if the
+     * viewport size is larger than the physical screen. (It doesn't
+     * prevent the guest setting this silly viewport setting, though...)
+     */
+    return config->xres_virtual > config->xres &&
+        config->yres_virtual > config->yres;
+}
+
 static void fb_update_display(void *opaque)
 {
     BCM2835FBState *s = opaque;
@@ -134,12 +146,18 @@ static void fb_update_display(void *opaque)
     int last = 0;
     int src_width = 0;
     int dest_width = 0;
+    uint32_t xoff = 0, yoff = 0;
 
     if (s->lock || !s->config.xres) {
         return;
     }
 
     src_width = bcm2835_fb_get_pitch(&s->config);
+    if (fb_use_offsets(&s->config)) {
+        xoff = s->config.xoffset;
+        yoff = s->config.yoffset;
+    }
+
     dest_width = s->config.xres;
 
     switch (surface_bits_per_pixel(surface)) {
@@ -165,8 +183,9 @@ static void fb_update_display(void *opaque)
     }
 
     if (s->invalidate) {
+        hwaddr base = s->config.base + xoff + yoff * src_width;
         framebuffer_update_memory_section(&s->fbsection, s->dma_mr,
-                                          s->config.base,
+                                          base,
                                           s->config.yres, src_width);
     }
 
@@ -176,7 +195,8 @@ static void fb_update_display(void *opaque)
                                draw_line_src16, s, &first, &last);
 
     if (first >= 0) {
-        dpy_gfx_update(s->con, 0, first, s->config.xres, last - first + 1);
+        dpy_gfx_update(s->con, 0, first, s->config.xres,
+                       last - first + 1);
     }
 
     s->invalidate = false;
@@ -202,8 +222,6 @@ static void bcm2835_fb_mbox_push(BCM2835FBState *s, uint32_t value)
     s->config.base = s->vcram_base | (value & 0xc0000000);
     s->config.base += BCM2835_FB_OFFSET;
 
-    /* TODO - Manage properly virtual resolution */
-
     pitch = bcm2835_fb_get_pitch(&s->config);
     size = bcm2835_fb_get_size(&s->config);
 
@@ -224,8 +242,6 @@ void bcm2835_fb_reconfigure(BCM2835FBState *s, BCM2835FBConfig *newconfig)
 
     s->config = *newconfig;
 
-    /* TODO - Manage properly virtual resolution */
-
     s->invalidate = true;
     qemu_console_resize(s->con, s->config.xres, s->config.yres);
     s->lock = false;
diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c
index c8c4979bd26..e3ab677891b 100644
--- a/hw/misc/bcm2835_property.c
+++ b/hw/misc/bcm2835_property.c
@@ -155,23 +155,32 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
         case 0x00040002: /* Blank screen */
             resplen = 4;
             break;
-        case 0x00040003: /* Get display width/height */
-        case 0x00040004:
+        case 0x00040003: /* Get physical display width/height */
             stl_le_phys(&s->dma_as, value + 12, fbconfig.xres);
             stl_le_phys(&s->dma_as, value + 16, fbconfig.yres);
             resplen = 8;
             break;
-        case 0x00044003: /* Test display width/height */
-        case 0x00044004:
+        case 0x00040004: /* Get virtual display width/height */
+            stl_le_phys(&s->dma_as, value + 12, fbconfig.xres_virtual);
+            stl_le_phys(&s->dma_as, value + 16, fbconfig.yres_virtual);
             resplen = 8;
             break;
-        case 0x00048003: /* Set display width/height */
-        case 0x00048004:
+        case 0x00044003: /* Test physical display width/height */
+        case 0x00044004: /* Test virtual display width/height */
+            resplen = 8;
+            break;
+        case 0x00048003: /* Set physical display width/height */
             fbconfig.xres = ldl_le_phys(&s->dma_as, value + 12);
             fbconfig.yres = ldl_le_phys(&s->dma_as, value + 16);
             fbconfig_updated = true;
             resplen = 8;
             break;
+        case 0x00048004: /* Set virtual display width/height */
+            fbconfig.xres_virtual = ldl_le_phys(&s->dma_as, value + 12);
+            fbconfig.yres_virtual = ldl_le_phys(&s->dma_as, value + 16);
+            fbconfig_updated = true;
+            resplen = 8;
+            break;
         case 0x00040005: /* Get depth */
             stl_le_phys(&s->dma_as, value + 12, fbconfig.bpp);
             resplen = 4;
-- 
2.18.0

[Qemu-devel] [PATCH 7/8] hw/display/bcm2835_fb: Validate config settings

[Peter Maydell]

Validate the config settings that the guest tries to set.

The wiki page documentation is not really accurate here:
generally rather than failing requests to set bad parameters,
the hardware will just clip them to something sensible.

Validate the most important parameters: sizes and
the viewport offsets. This prevents the framebuffer
code from trying to read out-of-range memory.

In the property handling code, we validate the new parameters every
time we encounter a tag that sets them. This means we validate the
config multiple times if the request includes multiple config-setting
tags, but the code would require significant restructuring to do a
validation only once but still return the clipped settings for
get-parameter tags and the buffer allocation tag.

Validation of settings made via the older bcm2835_fb_mbox_push()
function will be done in the next commit.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/display/bcm2835_fb.h |  8 +++++
 hw/display/bcm2835_fb.c         | 48 +++++++++++++++++++++++++++--
 hw/misc/bcm2835_property.c      | 54 ++++++++++++++++-----------------
 3 files changed, 81 insertions(+), 29 deletions(-)

diff --git a/include/hw/display/bcm2835_fb.h b/include/hw/display/bcm2835_fb.h
index d992c60c120..228988ba056 100644
--- a/include/hw/display/bcm2835_fb.h
+++ b/include/hw/display/bcm2835_fb.h
@@ -76,4 +76,12 @@ static inline uint32_t bcm2835_fb_get_size(BCM2835FBConfig *config)
     return yres * bcm2835_fb_get_pitch(config);
 }
 
+/**
+ * bcm2835_fb_validate_config: check provided config
+ *
+ * Validates the configuration information provided by the guest and
+ * adjusts it if necessary.
+ */
+void bcm2835_fb_validate_config(BCM2835FBConfig *config);
+
 #endif
diff --git a/hw/display/bcm2835_fb.c b/hw/display/bcm2835_fb.c
index 76a10072b46..3edb8b5cfcb 100644
--- a/hw/display/bcm2835_fb.c
+++ b/hw/display/bcm2835_fb.c
@@ -34,6 +34,13 @@
 #define DEFAULT_VCRAM_SIZE 0x4000000
 #define BCM2835_FB_OFFSET  0x00100000
 
+/* Maximum permitted framebuffer size; experimentally determined on an rpi2 */
+#define XRES_MAX 3840
+#define YRES_MAX 2560
+/* Framebuffer size used if guest requests zero size */
+#define XRES_SMALL 592
+#define YRES_SMALL 488
+
 static void fb_invalidate_display(void *opaque)
 {
     BCM2835FBState *s = BCM2835_FB(opaque);
@@ -202,6 +209,45 @@ static void fb_update_display(void *opaque)
     s->invalidate = false;
 }
 
+void bcm2835_fb_validate_config(BCM2835FBConfig *config)
+{
+    /*
+     * Validate the config, and clip any bogus values into range,
+     * as the hardware does. Note that fb_update_display() relies on
+     * this happening to prevent it from performing out-of-range
+     * accesses on redraw.
+     */
+    config->xres = MIN(config->xres, XRES_MAX);
+    config->xres_virtual = MIN(config->xres_virtual, XRES_MAX);
+    config->yres = MIN(config->yres, YRES_MAX);
+    config->yres_virtual = MIN(config->yres_virtual, YRES_MAX);
+
+    /*
+     * These are not minima: a 40x40 framebuffer will be accepted.
+     * They're only used as defaults if the guest asks for zero size.
+     */
+    if (config->xres == 0) {
+        config->xres = XRES_SMALL;
+    }
+    if (config->yres == 0) {
+        config->yres = YRES_SMALL;
+    }
+    if (config->xres_virtual == 0) {
+        config->xres_virtual = config->xres;
+    }
+    if (config->yres_virtual == 0) {
+        config->yres_virtual = config->yres;
+    }
+
+    if (fb_use_offsets(config)) {
+        /* Clip the offsets so the viewport is within the physical screen */
+        config->xoffset = MIN(config->xoffset,
+                              config->xres_virtual - config->xres);
+        config->yoffset = MIN(config->yoffset,
+                              config->yres_virtual - config->yres);
+    }
+}
+
 static void bcm2835_fb_mbox_push(BCM2835FBState *s, uint32_t value)
 {
     uint32_t pitch;
@@ -238,8 +284,6 @@ void bcm2835_fb_reconfigure(BCM2835FBState *s, BCM2835FBConfig *newconfig)
 {
     s->lock = true;
 
-    /* TODO: input validation! */
-
     s->config = *newconfig;
 
     s->invalidate = true;
diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c
index e3ab677891b..145427ae0f8 100644
--- a/hw/misc/bcm2835_property.c
+++ b/hw/misc/bcm2835_property.c
@@ -155,16 +155,6 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
         case 0x00040002: /* Blank screen */
             resplen = 4;
             break;
-        case 0x00040003: /* Get physical display width/height */
-            stl_le_phys(&s->dma_as, value + 12, fbconfig.xres);
-            stl_le_phys(&s->dma_as, value + 16, fbconfig.yres);
-            resplen = 8;
-            break;
-        case 0x00040004: /* Get virtual display width/height */
-            stl_le_phys(&s->dma_as, value + 12, fbconfig.xres_virtual);
-            stl_le_phys(&s->dma_as, value + 16, fbconfig.yres_virtual);
-            resplen = 8;
-            break;
         case 0x00044003: /* Test physical display width/height */
         case 0x00044004: /* Test virtual display width/height */
             resplen = 8;
@@ -172,29 +162,35 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
         case 0x00048003: /* Set physical display width/height */
             fbconfig.xres = ldl_le_phys(&s->dma_as, value + 12);
             fbconfig.yres = ldl_le_phys(&s->dma_as, value + 16);
+            bcm2835_fb_validate_config(&fbconfig);
             fbconfig_updated = true;
+            /* fall through */
+        case 0x00040003: /* Get physical display width/height */
+            stl_le_phys(&s->dma_as, value + 12, fbconfig.xres);
+            stl_le_phys(&s->dma_as, value + 16, fbconfig.yres);
             resplen = 8;
             break;
         case 0x00048004: /* Set virtual display width/height */
             fbconfig.xres_virtual = ldl_le_phys(&s->dma_as, value + 12);
             fbconfig.yres_virtual = ldl_le_phys(&s->dma_as, value + 16);
+            bcm2835_fb_validate_config(&fbconfig);
             fbconfig_updated = true;
+            /* fall through */
+        case 0x00040004: /* Get virtual display width/height */
+            stl_le_phys(&s->dma_as, value + 12, fbconfig.xres_virtual);
+            stl_le_phys(&s->dma_as, value + 16, fbconfig.yres_virtual);
             resplen = 8;
             break;
-        case 0x00040005: /* Get depth */
-            stl_le_phys(&s->dma_as, value + 12, fbconfig.bpp);
-            resplen = 4;
-            break;
         case 0x00044005: /* Test depth */
             resplen = 4;
             break;
         case 0x00048005: /* Set depth */
             fbconfig.bpp = ldl_le_phys(&s->dma_as, value + 12);
+            bcm2835_fb_validate_config(&fbconfig);
             fbconfig_updated = true;
-            resplen = 4;
-            break;
-        case 0x00040006: /* Get pixel order */
-            stl_le_phys(&s->dma_as, value + 12, fbconfig.pixo);
+            /* fall through */
+        case 0x00040005: /* Get depth */
+            stl_le_phys(&s->dma_as, value + 12, fbconfig.bpp);
             resplen = 4;
             break;
         case 0x00044006: /* Test pixel order */
@@ -202,11 +198,11 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
             break;
         case 0x00048006: /* Set pixel order */
             fbconfig.pixo = ldl_le_phys(&s->dma_as, value + 12);
+            bcm2835_fb_validate_config(&fbconfig);
             fbconfig_updated = true;
-            resplen = 4;
-            break;
-        case 0x00040007: /* Get alpha */
-            stl_le_phys(&s->dma_as, value + 12, fbconfig.alpha);
+            /* fall through */
+        case 0x00040006: /* Get pixel order */
+            stl_le_phys(&s->dma_as, value + 12, fbconfig.pixo);
             resplen = 4;
             break;
         case 0x00044007: /* Test pixel alpha */
@@ -214,7 +210,11 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
             break;
         case 0x00048007: /* Set alpha */
             fbconfig.alpha = ldl_le_phys(&s->dma_as, value + 12);
+            bcm2835_fb_validate_config(&fbconfig);
             fbconfig_updated = true;
+            /* fall through */
+        case 0x00040007: /* Get alpha */
+            stl_le_phys(&s->dma_as, value + 12, fbconfig.alpha);
             resplen = 4;
             break;
         case 0x00040008: /* Get pitch */
@@ -222,18 +222,18 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
                         bcm2835_fb_get_pitch(&fbconfig));
             resplen = 4;
             break;
-        case 0x00040009: /* Get virtual offset */
-            stl_le_phys(&s->dma_as, value + 12, fbconfig.xoffset);
-            stl_le_phys(&s->dma_as, value + 16, fbconfig.yoffset);
-            resplen = 8;
-            break;
         case 0x00044009: /* Test virtual offset */
             resplen = 8;
             break;
         case 0x00048009: /* Set virtual offset */
             fbconfig.xoffset = ldl_le_phys(&s->dma_as, value + 12);
             fbconfig.yoffset = ldl_le_phys(&s->dma_as, value + 16);
+            bcm2835_fb_validate_config(&fbconfig);
             fbconfig_updated = true;
+            /* fall through */
+        case 0x00040009: /* Get virtual offset */
+            stl_le_phys(&s->dma_as, value + 12, fbconfig.xoffset);
+            stl_le_phys(&s->dma_as, value + 16, fbconfig.yoffset);
             resplen = 8;
             break;
         case 0x0004000a: /* Get/Test/Set overscan */
-- 
2.18.0

[Qemu-devel] [PATCH 8/8] hw/display/bcm2835_fb: Validate bcm2835_fb_mbox_push() config

[Peter Maydell]

Refactor bcm2835_fb_mbox_push() to work by calling
bcm2835_fb_validate_config() and bcm2835_fb_reconfigure(),
so that config set this way is also validated.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/display/bcm2835_fb.c | 63 ++++++++++++++++++++---------------------
 1 file changed, 31 insertions(+), 32 deletions(-)

diff --git a/hw/display/bcm2835_fb.c b/hw/display/bcm2835_fb.c
index 3edb8b5cfcb..d534d00a65f 100644
--- a/hw/display/bcm2835_fb.c
+++ b/hw/display/bcm2835_fb.c
@@ -248,38 +248,6 @@ void bcm2835_fb_validate_config(BCM2835FBConfig *config)
     }
 }
 
-static void bcm2835_fb_mbox_push(BCM2835FBState *s, uint32_t value)
-{
-    uint32_t pitch;
-    uint32_t size;
-
-    value &= ~0xf;
-
-    s->lock = true;
-
-    s->config.xres = ldl_le_phys(&s->dma_as, value);
-    s->config.yres = ldl_le_phys(&s->dma_as, value + 4);
-    s->config.xres_virtual = ldl_le_phys(&s->dma_as, value + 8);
-    s->config.yres_virtual = ldl_le_phys(&s->dma_as, value + 12);
-    s->config.bpp = ldl_le_phys(&s->dma_as, value + 20);
-    s->config.xoffset = ldl_le_phys(&s->dma_as, value + 24);
-    s->config.yoffset = ldl_le_phys(&s->dma_as, value + 28);
-
-    s->config.base = s->vcram_base | (value & 0xc0000000);
-    s->config.base += BCM2835_FB_OFFSET;
-
-    pitch = bcm2835_fb_get_pitch(&s->config);
-    size = bcm2835_fb_get_size(&s->config);
-
-    stl_le_phys(&s->dma_as, value + 16, pitch);
-    stl_le_phys(&s->dma_as, value + 32, s->config.base);
-    stl_le_phys(&s->dma_as, value + 36, size);
-
-    s->invalidate = true;
-    qemu_console_resize(s->con, s->config.xres, s->config.yres);
-    s->lock = false;
-}
-
 void bcm2835_fb_reconfigure(BCM2835FBState *s, BCM2835FBConfig *newconfig)
 {
     s->lock = true;
@@ -291,6 +259,37 @@ void bcm2835_fb_reconfigure(BCM2835FBState *s, BCM2835FBConfig *newconfig)
     s->lock = false;
 }
 
+static void bcm2835_fb_mbox_push(BCM2835FBState *s, uint32_t value)
+{
+    uint32_t pitch;
+    uint32_t size;
+    BCM2835FBConfig newconf;
+
+    value &= ~0xf;
+
+    newconf.xres = ldl_le_phys(&s->dma_as, value);
+    newconf.yres = ldl_le_phys(&s->dma_as, value + 4);
+    newconf.xres_virtual = ldl_le_phys(&s->dma_as, value + 8);
+    newconf.yres_virtual = ldl_le_phys(&s->dma_as, value + 12);
+    newconf.bpp = ldl_le_phys(&s->dma_as, value + 20);
+    newconf.xoffset = ldl_le_phys(&s->dma_as, value + 24);
+    newconf.yoffset = ldl_le_phys(&s->dma_as, value + 28);
+
+    newconf.base = s->vcram_base | (value & 0xc0000000);
+    newconf.base += BCM2835_FB_OFFSET;
+
+    bcm2835_fb_validate_config(&newconf);
+
+    pitch = bcm2835_fb_get_pitch(&newconf);
+    size = bcm2835_fb_get_size(&newconf);
+
+    stl_le_phys(&s->dma_as, value + 16, pitch);
+    stl_le_phys(&s->dma_as, value + 32, newconf.base);
+    stl_le_phys(&s->dma_as, value + 36, size);
+
+    bcm2835_fb_reconfigure(s, &newconf);
+}
+
 static uint64_t bcm2835_fb_read(void *opaque, hwaddr offset, unsigned size)
 {
     BCM2835FBState *s = opaque;
-- 
2.18.0

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/8] arm/raspi: Make fb handle virtual viewport

[Peter Maydell]

Ping for code review?

thanks
-- PMM

On 14 August 2018 at 15:44, Peter Maydell <peter.maydell@linaro.org> wrote:
> The raspi framebuffer supports a virtual viewport which can
> be set up so that the virtual framebuffer size is larger than
> the physical screen size, and the displayed area is at some
> offset within this virtual framebuffer area. This patchset
> implements that support.
>
> To do that I had to do a fair amount of refactoring, because
> getting the viewport code to work correctly and prevent the
> guest from making it fall over by specifying silly offsets
> or sizes requires that we properly validate and clip the
> config that the guest sends us.
>
> Note that the documentation in places like
> https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface
> has various errors in it. The behaviour implemented here
> corresponds to how a hardware raspi2 I tested seems to behave,
> using test programs based on the code from
> http://www.valvers.com/open-software/raspberry-pi/step05-bare-metal-programming-in-c-pt5/
>
> (I was a little bit hampered because I couldn't get the rpi
> to actually *display* anything to the HDMI port, so mostly
> I was testing the edge case behaviour of attempting to set
> and read back various config values.)
>
> This series fixes this bug:
> https://bugs.launchpad.net/qemu/+bug/1777672
>
> thanks
> -- PMM
>
> Peter Maydell (8):
>   hw/misc/bcm2835_fb: Move config fields to their own struct
>   hw/misc/bcm2835_property: Track fb settings using BCM2835FBConfig
>   hw/display/bcm2835_fb: Drop unused size and pitch fields
>   hw/display/bcm2835_fb: Reset resolution, etc correctly
>   hw/display/bcm2835_fb: Abstract out calculation of pitch, size
>   hw/display/bcm2835_fb: Fix handling of virtual framebuffer
>   hw/display/bcm2835_fb: Validate config settings
>   hw/display/bcm2835_fb: Validate bcm2835_fb_mbox_push() config
>
>  include/hw/display/bcm2835_fb.h |  59 +++++++--
>  hw/display/bcm2835_fb.c         | 218 +++++++++++++++++++-------------
>  hw/misc/bcm2835_property.c      | 123 +++++++++---------
>  3 files changed, 240 insertions(+), 160 deletions(-)

Re: [Qemu-devel] [PATCH 1/8] hw/misc/bcm2835_fb: Move config fields to their own struct

[Richard Henderson]

On 08/14/2018 07:44 AM, Peter Maydell wrote:
> The handling of framebuffer properties in the bcm2835_property code
> is a bit clumsy, because for each of the many fb related properties
> we try to track the value we're about to set and whether we're going
> to be setting a value, and then we hand all the new values off
> to the framebuffer via a function which takes them all as separate
> arguments. It would be simpler if the property code could easily
> copy all the framebuffer's current settings, update them with
> the new specified values and then ask the framebuffer to switch
> to the new set.
> 
> As the first part of this refactoring, pull all the fb config
> settings fields in BCM2835FBState out into their own struct.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/display/bcm2835_fb.h |  26 ++++++--
>  hw/display/bcm2835_fb.c         | 114 +++++++++++++++++---------------
>  hw/misc/bcm2835_property.c      |  28 ++++----
>  3 files changed, 94 insertions(+), 74 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 2/8] hw/misc/bcm2835_property: Track fb settings using BCM2835FBConfig

[Richard Henderson]

On 08/14/2018 07:44 AM, Peter Maydell wrote:
> Refactor the fb property setting code so that rather than
> using a set of pointers to local variables to track
> whether a config value has been updated in the current
> mbox and if so what its new value is, we just copy
> all the current settings of the fb at the start, and
> then update that copy as we go along, before asking
> the fb to switch to it at the end.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/display/bcm2835_fb.h |  4 +-
>  hw/display/bcm2835_fb.c         | 27 ++---------
>  hw/misc/bcm2835_property.c      | 80 ++++++++++++++-------------------
>  3 files changed, 37 insertions(+), 74 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 3/8] hw/display/bcm2835_fb: Drop unused size and pitch fields

[Richard Henderson]

On 08/14/2018 07:44 AM, Peter Maydell wrote:
> The BCM2835FBState struct has a 'pitch' field which is a
> cached copy of xres * (bpp >> 3), and a 'size' field which is
> a cached copy of pitch * yres. However we don't actually do
> anything with these fields; delete them. We retain the
> now-unused slots in the VMState struct for migration
> compatibility.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/display/bcm2835_fb.h |  4 ----
>  hw/display/bcm2835_fb.c         | 19 ++++++++-----------
>  2 files changed, 8 insertions(+), 15 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 4/8] hw/display/bcm2835_fb: Reset resolution, etc correctly

[Richard Henderson]

On 08/14/2018 07:44 AM, Peter Maydell wrote:
> The bcm2835_fb's initial resolution and other parameters are set
> via QOM properties. We should reset to those initial values on
> device reset, which means we need to save the QOM property
> values somewhere that they are not overwritten by guest
> changes to the framebuffer configuration.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/display/bcm2835_fb.h |  1 +
>  hw/display/bcm2835_fb.c         | 27 +++++++++++++++------------
>  2 files changed, 16 insertions(+), 12 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 5/8] hw/display/bcm2835_fb: Abstract out calculation of pitch, size

[Richard Henderson]

On 08/14/2018 07:44 AM, Peter Maydell wrote:
> Abstract out the calculation of the pitch and size of the
> framebuffer into functions that operate on the BCM2835FBConfig
> struct -- these are about to get a little more complicated
> when we add support for virtual and physical sizes differing.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/display/bcm2835_fb.h | 22 ++++++++++++++++++++++
>  hw/display/bcm2835_fb.c         |  6 +++---
>  hw/misc/bcm2835_property.c      |  4 ++--
>  3 files changed, 27 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 6/8] hw/display/bcm2835_fb: Fix handling of virtual framebuffer

[Richard Henderson]

On 08/14/2018 07:44 AM, Peter Maydell wrote:
> The raspi framebuffir in bcm2835_fb supports the definition

framebuffer.

> of a virtual "viewport", which is smaller than the full
> physical framebuffer size and at an adjustable offset within
> it. Only the viewport area is sent to the screen. This allows
> the guest to do things like double buffering, or scrolling
> by adjusting the viewport origin. Currently QEMU doesn't
> implement this at all.
> 
> Add support for this feature:
>  * the property mailbox code needs to distinguish the
>    virtual width/height from the physical width/height
>  * the framebuffer code needs to do something with the
>    virtual width/height/origin information
> 
> Note that the wiki documentation on the semantics of the
> virtual and physical height and width has it the wrong way
> around -- the virtual size is the size of the allocated
> buffer, and the physical size is the size of the display,
> so the virtual size is always the same as or larger than
> the physical.
> 
> If the viewport size is set smaller than the physical
> screen size, we ignore the viewport settings completely
> and just display the physical screen area.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/display/bcm2835_fb.h |  6 ++++--
>  hw/display/bcm2835_fb.c         | 28 ++++++++++++++++++++++------
>  hw/misc/bcm2835_property.c      | 21 +++++++++++++++------
>  3 files changed, 41 insertions(+), 14 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 7/8] hw/display/bcm2835_fb: Validate config settings

[Richard Henderson]

On 08/14/2018 07:44 AM, Peter Maydell wrote:
> Validate the config settings that the guest tries to set.
> 
> The wiki page documentation is not really accurate here:
> generally rather than failing requests to set bad parameters,
> the hardware will just clip them to something sensible.
> 
> Validate the most important parameters: sizes and
> the viewport offsets. This prevents the framebuffer
> code from trying to read out-of-range memory.
> 
> In the property handling code, we validate the new parameters every
> time we encounter a tag that sets them. This means we validate the
> config multiple times if the request includes multiple config-setting
> tags, but the code would require significant restructuring to do a
> validation only once but still return the clipped settings for
> get-parameter tags and the buffer allocation tag.
> 
> Validation of settings made via the older bcm2835_fb_mbox_push()
> function will be done in the next commit.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/display/bcm2835_fb.h |  8 +++++
>  hw/display/bcm2835_fb.c         | 48 +++++++++++++++++++++++++++--
>  hw/misc/bcm2835_property.c      | 54 ++++++++++++++++-----------------
>  3 files changed, 81 insertions(+), 29 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 8/8] hw/display/bcm2835_fb: Validate bcm2835_fb_mbox_push() config

[Richard Henderson]

On 08/14/2018 07:44 AM, Peter Maydell wrote:
> Refactor bcm2835_fb_mbox_push() to work by calling
> bcm2835_fb_validate_config() and bcm2835_fb_reconfigure(),
> so that config set this way is also validated.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/display/bcm2835_fb.c | 63 ++++++++++++++++++++---------------------
>  1 file changed, 31 insertions(+), 32 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~