[Qemu-devel] [PATCH 0/3] vga: add endianness switching support

[Qemu-devel] [PATCH 0/3] vga: add endianness switching support

[Gerd Hoffmann]

  Hi,

Series applies on top of the vga patches posted yesterday.
It adds a mmio register to the pci stdvga cards allowing
to switch framebuffer endiannness.  Ben's patches 1+2 prepare
for runtime-swicthable endianness and patch #3 actually
implements the new register.  It has been placed in a new
range to avoid conflicts in case the bochs dispi interface
will be extended in the future.  The separate range also
makes it easier to turn off on old machine types.

please review,
  Gerd

Benjamin Herrenschmidt (2):
  vga: Make fb endian a common state variable
  vga: Add endian control register

Gerd Hoffmann (1):
  vga-pci: add qext region to mmio

 docs/specs/standard-vga.txt |  9 ++++++
 hw/display/vga-pci.c        | 68 +++++++++++++++++++++++++++++++++++++++++++++
 hw/display/vga.c            | 67 ++++++++++++++++++++++++++++++++++----------
 hw/display/vga_int.h        |  3 ++
 include/hw/i386/pc.h        |  8 ++++++
 5 files changed, 141 insertions(+), 14 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/3] vga: Make fb endian a common state variable

[Gerd Hoffmann]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

And initialize it based on target endian

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 hw/display/vga.c     | 32 +++++++++++++++++++-------------
 hw/display/vga_int.h |  1 +
 2 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 9132e92..3f02984 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -1461,16 +1461,11 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
     int disp_width, multi_scan, multi_run;
     uint8_t *d;
     uint32_t v, addr1, addr;
-    vga_draw_line_func *vga_draw_line;
-#if defined(TARGET_WORDS_BIGENDIAN)
-    static const bool big_endian_fb = true;
-#else
-    static const bool big_endian_fb = false;
-#endif
-#if defined(HOST_WORDS_BIGENDIAN)
-    static const bool byteswap = !big_endian_fb;
+    vga_draw_line_func *vga_draw_line = NULL;
+#ifdef HOST_WORDS_BIGENDIAN
+    bool byteswap = !s->big_endian_fb;
 #else
-    static const bool byteswap = big_endian_fb;
+    bool byteswap = s->big_endian_fb;
 #endif
 
     full_update |= update_basic_params(s);
@@ -1573,19 +1568,19 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
             bits = 8;
             break;
         case 15:
-            v = big_endian_fb ? VGA_DRAW_LINE15_BE : VGA_DRAW_LINE15_LE;
+            v = s->big_endian_fb ? VGA_DRAW_LINE15_BE : VGA_DRAW_LINE15_LE;
             bits = 15;
             break;
         case 16:
-            v = big_endian_fb ? VGA_DRAW_LINE16_BE : VGA_DRAW_LINE16_LE;
+            v = s->big_endian_fb ? VGA_DRAW_LINE16_BE : VGA_DRAW_LINE16_LE;
             bits = 15;
             break;
         case 24:
-            v = big_endian_fb ? VGA_DRAW_LINE24_BE : VGA_DRAW_LINE24_LE;
+            v = s->big_endian_fb ? VGA_DRAW_LINE24_BE : VGA_DRAW_LINE24_LE;
             bits = 24;
             break;
         case 32:
-            v = big_endian_fb ? VGA_DRAW_LINE32_BE : VGA_DRAW_LINE32_LE;
+            v = s->big_endian_fb ? VGA_DRAW_LINE32_BE : VGA_DRAW_LINE32_LE;
             bits = 32;
             break;
         }
@@ -2129,6 +2124,17 @@ void vga_common_init(VGACommonState *s, Object *obj, bool global_vmstate)
         s->update_retrace_info = vga_precise_update_retrace_info;
         break;
     }
+
+    /*
+     * Set default fb endian based on target, should probably be turned
+     * into a device attribute set by the machine/platform to remove
+     * all target endian dependencies from this file.
+     */
+#ifdef TARGET_WORDS_BIGENDIAN
+    s->big_endian_fb = true;
+#else
+    s->big_endian_fb = false;
+#endif
     vga_dirty_log_start(s);
 }
 
diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
index 9073370..28d67cf 100644
--- a/hw/display/vga_int.h
+++ b/hw/display/vga_int.h
@@ -157,6 +157,7 @@ typedef struct VGACommonState {
     const GraphicHwOps *hw_ops;
     bool full_update_text;
     bool full_update_gfx;
+    bool big_endian_fb;
     /* hardware mouse cursor support */
     uint32_t invalidated_y_table[VGA_MAX_HEIGHT / 32];
     void (*cursor_invalidate)(struct VGACommonState *s);
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/3] vga: Add endian control register

[Gerd Hoffmann]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Include the endian state in the migration stream as an optional
subsection which we only include when the endian isn't the default,
thus enabling backward compatibility of the common case.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Changes by kraxel:
 * Remove bochs dispi interface changes.  We'll do that in
   a different way to make sure we don't conflict with
   possible future bochs dispi interface changes.
 * keep live migration bits.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/vga.c     | 41 +++++++++++++++++++++++++++++++++++++----
 hw/display/vga_int.h |  4 +++-
 2 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 3f02984..fd16806 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -1508,7 +1508,8 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
     if (s->line_offset != s->last_line_offset ||
         disp_width != s->last_width ||
         height != s->last_height ||
-        s->last_depth != depth) {
+        s->last_depth != depth ||
+        s->last_byteswap != byteswap) {
         if (depth == 32 || (depth == 16 && !byteswap)) {
             pixman_format_code_t format =
                 qemu_default_pixman_format(depth, !byteswap);
@@ -1526,6 +1527,7 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
         s->last_height = height;
         s->last_line_offset = s->line_offset;
         s->last_depth = depth;
+        s->last_byteswap = byteswap;
         full_update = 1;
     } else if (is_buffer_shared(surface) &&
                (full_update || surface_data(surface) != s->vram_ptr
@@ -1789,6 +1791,7 @@ void vga_common_reset(VGACommonState *s)
     s->cursor_start = 0;
     s->cursor_end = 0;
     s->cursor_offset = 0;
+    s->big_endian_fb = s->default_endian_fb;
     memset(s->invalidated_y_table, '\0', sizeof(s->invalidated_y_table));
     memset(s->last_palette, '\0', sizeof(s->last_palette));
     memset(s->last_ch_attr, '\0', sizeof(s->last_ch_attr));
@@ -2020,6 +2023,28 @@ static int vga_common_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static bool vga_endian_state_needed(void *opaque)
+{
+    VGACommonState *s = opaque;
+
+    /*
+     * Only send the endian state if it's different from the
+     * default one, thus ensuring backward compatibility for
+     * migration of the common case
+     */
+    return s->default_endian_fb != s->big_endian_fb;
+}
+
+const VMStateDescription vmstate_vga_endian = {
+    .name = "vga.endian",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8_EQUAL(big_endian_fb, VGACommonState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 const VMStateDescription vmstate_vga_common = {
     .name = "vga",
     .version_id = 2,
@@ -2056,6 +2081,14 @@ const VMStateDescription vmstate_vga_common = {
         VMSTATE_UINT32(vbe_line_offset, VGACommonState),
         VMSTATE_UINT32(vbe_bank_mask, VGACommonState),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (VMStateSubsection []) {
+        {
+            .vmsd = &vmstate_vga_endian,
+            .needed = vga_endian_state_needed,
+        }, {
+            /* empty */
+        }
     }
 };
 
@@ -2126,14 +2159,14 @@ void vga_common_init(VGACommonState *s, Object *obj, bool global_vmstate)
     }
 
     /*
-     * Set default fb endian based on target, should probably be turned
+     * Set default fb endian based on target, could probably be turned
      * into a device attribute set by the machine/platform to remove
      * all target endian dependencies from this file.
      */
 #ifdef TARGET_WORDS_BIGENDIAN
-    s->big_endian_fb = true;
+    s->default_endian_fb = true;
 #else
-    s->big_endian_fb = false;
+    s->default_endian_fb = false;
 #endif
     vga_dirty_log_start(s);
 }
diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
index 28d67cf..e44b4a7 100644
--- a/hw/display/vga_int.h
+++ b/hw/display/vga_int.h
@@ -150,6 +150,7 @@ typedef struct VGACommonState {
     uint32_t last_width, last_height; /* in chars or pixels */
     uint32_t last_scr_width, last_scr_height; /* in pixels */
     uint32_t last_depth; /* in bits */
+    bool last_byteswap;
     uint8_t cursor_start, cursor_end;
     bool cursor_visible_phase;
     int64_t cursor_blink_time;
@@ -157,7 +158,8 @@ typedef struct VGACommonState {
     const GraphicHwOps *hw_ops;
     bool full_update_text;
     bool full_update_gfx;
-    bool big_endian_fb;
+    uint8_t big_endian_fb;
+    uint8_t default_endian_fb;
     /* hardware mouse cursor support */
     uint32_t invalidated_y_table[VGA_MAX_HEIGHT / 32];
     void (*cursor_invalidate)(struct VGACommonState *s);
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/3] vga-pci: add qext region to mmio

[Gerd Hoffmann]

Add a qemu extented register range to the standard vga mmio bar.
Right nowe there are two registers:  One readonly register returning the
size of the region (so we can easily add more registers there if needed)
and one endian control register, so guests (especially ppc) can flip
the framebuffer endianness as they need it.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 docs/specs/standard-vga.txt |  9 ++++++
 hw/display/vga-pci.c        | 68 +++++++++++++++++++++++++++++++++++++++++++++
 include/hw/i386/pc.h        |  8 ++++++
 3 files changed, 85 insertions(+)

diff --git a/docs/specs/standard-vga.txt b/docs/specs/standard-vga.txt
index f82773e..19d2a74 100644
--- a/docs/specs/standard-vga.txt
+++ b/docs/specs/standard-vga.txt
@@ -70,3 +70,12 @@ Likewise applies to the pci variant only for obvious reasons.
 0500 - 0515 : bochs dispi interface registers, mapped flat
               without index/data ports.  Use (index << 1)
               as offset for (16bit) register access.
+
+0600 - 0607 : qemu extended registers.  qemu 2.2+ only.
+              The pci revision is 2 (or greater) when
+              these registers are present.  The registers
+              are 32bit.
+  0600      : qemu extended register region size, in bytes.
+  0604      : framebuffer endianness register.
+              - 0xbebebebe indicates big endian.
+              - 0x1e1e1e1e indicates little endian.
diff --git a/hw/display/vga-pci.c b/hw/display/vga-pci.c
index 0351d94..3394ec2 100644
--- a/hw/display/vga-pci.c
+++ b/hw/display/vga-pci.c
@@ -35,10 +35,18 @@
 #define PCI_VGA_IOPORT_SIZE   (0x3e0 - 0x3c0)
 #define PCI_VGA_BOCHS_OFFSET  0x500
 #define PCI_VGA_BOCHS_SIZE    (0x0b * 2)
+#define PCI_VGA_QEXT_OFFSET   0x600
+#define PCI_VGA_QEXT_SIZE     (2 * 4)
 #define PCI_VGA_MMIO_SIZE     0x1000
 
+#define PCI_VGA_QEXT_REG_SIZE         (0 * 4)
+#define PCI_VGA_QEXT_REG_BYTEORDER    (1 * 4)
+#define  PCI_VGA_QEXT_LITTLE_ENDIAN   0x1e1e1e1e
+#define  PCI_VGA_QEXT_BIG_ENDIAN      0xbebebebe
+
 enum vga_pci_flags {
     PCI_VGA_FLAG_ENABLE_MMIO = 1,
+    PCI_VGA_FLAG_ENABLE_QEXT = 2,
 };
 
 typedef struct PCIVGAState {
@@ -48,6 +56,7 @@ typedef struct PCIVGAState {
     MemoryRegion mmio;
     MemoryRegion ioport;
     MemoryRegion bochs;
+    MemoryRegion qext;
 } PCIVGAState;
 
 static const VMStateDescription vmstate_vga_pci = {
@@ -140,6 +149,46 @@ static const MemoryRegionOps pci_vga_bochs_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
+static uint64_t pci_vga_qext_read(void *ptr, hwaddr addr, unsigned size)
+{
+    PCIVGAState *d = ptr;
+
+    switch (addr) {
+    case PCI_VGA_QEXT_REG_SIZE:
+        return PCI_VGA_QEXT_SIZE;
+    case PCI_VGA_QEXT_REG_BYTEORDER:
+        return d->vga.big_endian_fb ?
+            PCI_VGA_QEXT_BIG_ENDIAN : PCI_VGA_QEXT_LITTLE_ENDIAN;
+    default:
+        return 0;
+    }
+}
+
+static void pci_vga_qext_write(void *ptr, hwaddr addr,
+                               uint64_t val, unsigned size)
+{
+    PCIVGAState *d = ptr;
+
+    switch (addr) {
+    case PCI_VGA_QEXT_REG_BYTEORDER:
+        if (val == PCI_VGA_QEXT_BIG_ENDIAN) {
+            d->vga.big_endian_fb = true;
+        }
+        if (val == PCI_VGA_QEXT_LITTLE_ENDIAN) {
+            d->vga.big_endian_fb = false;
+        }
+        break;
+    }
+}
+
+static const MemoryRegionOps pci_vga_qext_ops = {
+    .read = pci_vga_qext_read,
+    .write = pci_vga_qext_write,
+    .valid.min_access_size = 4,
+    .valid.max_access_size = 4,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
 static int pci_std_vga_initfn(PCIDevice *dev)
 {
     PCIVGAState *d = DO_UPCAST(PCIVGAState, dev, dev);
@@ -167,6 +216,15 @@ static int pci_std_vga_initfn(PCIDevice *dev)
                                     &d->ioport);
         memory_region_add_subregion(&d->mmio, PCI_VGA_BOCHS_OFFSET,
                                     &d->bochs);
+
+        if (d->flags & (1 << PCI_VGA_FLAG_ENABLE_QEXT)) {
+            memory_region_init_io(&d->qext, NULL, &pci_vga_qext_ops, d,
+                                  "qemu extended regs", PCI_VGA_QEXT_SIZE);
+            memory_region_add_subregion(&d->mmio, PCI_VGA_QEXT_OFFSET,
+                                        &d->qext);
+            pci_set_byte(&d->dev.config[PCI_REVISION_ID], 2);
+        }
+
         pci_register_bar(&d->dev, 2, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
     }
 
@@ -199,6 +257,14 @@ static int pci_secondary_vga_initfn(PCIDevice *dev)
     memory_region_add_subregion(&d->mmio, PCI_VGA_BOCHS_OFFSET,
                                 &d->bochs);
 
+    if (d->flags & (1 << PCI_VGA_FLAG_ENABLE_QEXT)) {
+        memory_region_init_io(&d->qext, NULL, &pci_vga_qext_ops, d,
+                              "qemu extended regs", PCI_VGA_QEXT_SIZE);
+        memory_region_add_subregion(&d->mmio, PCI_VGA_QEXT_OFFSET,
+                                    &d->qext);
+        pci_set_byte(&d->dev.config[PCI_REVISION_ID], 2);
+    }
+
     pci_register_bar(&d->dev, 0, PCI_BASE_ADDRESS_MEM_PREFETCH, &s->vram);
     pci_register_bar(&d->dev, 2, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
 
@@ -215,11 +281,13 @@ static void pci_secondary_vga_reset(DeviceState *dev)
 static Property vga_pci_properties[] = {
     DEFINE_PROP_UINT32("vgamem_mb", PCIVGAState, vga.vram_size_mb, 16),
     DEFINE_PROP_BIT("mmio", PCIVGAState, flags, PCI_VGA_FLAG_ENABLE_MMIO, true),
+    DEFINE_PROP_BIT("qext", PCIVGAState, flags, PCI_VGA_FLAG_ENABLE_QEXT, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
 static Property secondary_pci_properties[] = {
     DEFINE_PROP_UINT32("vgamem_mb", PCIVGAState, vga.vram_size_mb, 16),
+    DEFINE_PROP_BIT("qext", PCIVGAState, flags, PCI_VGA_FLAG_ENABLE_QEXT, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 77316d5..23acc1f 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -306,6 +306,14 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
             .driver   = "intel-hda",\
             .property = "old_msi_addr",\
             .value    = "on",\
+        },{\
+            .driver   = "VGA",\
+            .property = "qext",\
+            .value    = "off",\
+        },{\
+            .driver   = "secondary-vga",\
+            .property = "qext",\
+            .value    = "off",\
         }
 
 #define PC_COMPAT_2_0 \
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 0/3] vga: add endianness switching support

[Benjamin Herrenschmidt]

On Tue, 2014-09-23 at 14:30 +0200, Gerd Hoffmann wrote:
>   Hi,

(Adding Mike)

> Series applies on top of the vga patches posted yesterday.
> It adds a mmio register to the pci stdvga cards allowing
> to switch framebuffer endiannness.  Ben's patches 1+2 prepare
> for runtime-swicthable endianness and patch #3 actually
> implements the new register.  It has been placed in a new
> range to avoid conflicts in case the bochs dispi interface
> will be extended in the future.  The separate range also
> makes it easier to turn off on old machine types.

Thanks. I'll try to produce patches for offb and bochsdrmfb to
adjust endianness & test some time this week or next week.

Now the remaining point of contention is the "auto switch"
functionality which, sadly, is needed for existing LE guests to work
properly.

I understand the reticence, on the other hand, our "pseries" machine is
really a paravirtualized environment where qemu implements the
hypervisor hcall interface, so in that context, it does make some sense
that the "hypervisor" affects some HW configuration/state on endian
switch (which is such an hcall).

The problem is how to do that in a reasonably clean way from an
implementation perspective.

The patch at
https://github.com/ozbenh/qemu/commit/778c92cebcd12fdafd2050dc4d7ca2521c462e46 is one attempt to do it, for whatever it's worth, but I'm open to alternatives.

Cheers,
Ben.

Re: [Qemu-devel] [PATCH 0/3] vga: add endianness switching support

[Alexander Graf]

On 23.09.14 22:24, Benjamin Herrenschmidt wrote:
> On Tue, 2014-09-23 at 14:30 +0200, Gerd Hoffmann wrote:
>>   Hi,
> 
> (Adding Mike)
> 
>> Series applies on top of the vga patches posted yesterday.
>> It adds a mmio register to the pci stdvga cards allowing
>> to switch framebuffer endiannness.  Ben's patches 1+2 prepare
>> for runtime-swicthable endianness and patch #3 actually
>> implements the new register.  It has been placed in a new
>> range to avoid conflicts in case the bochs dispi interface
>> will be extended in the future.  The separate range also
>> makes it easier to turn off on old machine types.
> 
> Thanks. I'll try to produce patches for offb and bochsdrmfb to
> adjust endianness & test some time this week or next week.
> 
> Now the remaining point of contention is the "auto switch"
> functionality which, sadly, is needed for existing LE guests to work
> properly.
> 
> I understand the reticence, on the other hand, our "pseries" machine is
> really a paravirtualized environment where qemu implements the
> hypervisor hcall interface, so in that context, it does make some sense
> that the "hypervisor" affects some HW configuration/state on endian
> switch (which is such an hcall).
> 
> The problem is how to do that in a reasonably clean way from an
> implementation perspective.
> 
> The patch at
> https://github.com/ozbenh/qemu/commit/778c92cebcd12fdafd2050dc4d7ca2521c462e46 is one attempt to do it, for whatever it's worth, but I'm open to alternatives.

Apart from stylistic problems I think the approach is reasonable. The
only alternative I could think of would be to simply loop through every
device in the device tree we can find and search for VGA objects.


Alex

Re: [Qemu-devel] [PATCH 1/3] vga: Make fb endian a common state variable

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 498 bytes --]

On Tue, Sep 23, 2014 at 02:30:55PM +0200, Gerd Hoffmann wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> And initialize it based on target endian
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 2/3] vga: Add endian control register

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3908 bytes --]

On Tue, Sep 23, 2014 at 02:30:56PM +0200, Gerd Hoffmann wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> Include the endian state in the migration stream as an optional
> subsection which we only include when the endian isn't the default,
> thus enabling backward compatibility of the common case.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> Changes by kraxel:
>  * Remove bochs dispi interface changes.  We'll do that in
>    a different way to make sure we don't conflict with
>    possible future bochs dispi interface changes.
>  * keep live migration bits.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/display/vga.c     | 41 +++++++++++++++++++++++++++++++++++++----
>  hw/display/vga_int.h |  4 +++-
>  2 files changed, 40 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/display/vga.c b/hw/display/vga.c
> index 3f02984..fd16806 100644
> --- a/hw/display/vga.c
> +++ b/hw/display/vga.c
> @@ -1508,7 +1508,8 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
>      if (s->line_offset != s->last_line_offset ||
>          disp_width != s->last_width ||
>          height != s->last_height ||
> -        s->last_depth != depth) {
> +        s->last_depth != depth ||
> +        s->last_byteswap != byteswap) {
>          if (depth == 32 || (depth == 16 && !byteswap)) {
>              pixman_format_code_t format =
>                  qemu_default_pixman_format(depth, !byteswap);
> @@ -1526,6 +1527,7 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
>          s->last_height = height;
>          s->last_line_offset = s->line_offset;
>          s->last_depth = depth;
> +        s->last_byteswap = byteswap;
>          full_update = 1;
>      } else if (is_buffer_shared(surface) &&
>                 (full_update || surface_data(surface) != s->vram_ptr
> @@ -1789,6 +1791,7 @@ void vga_common_reset(VGACommonState *s)
>      s->cursor_start = 0;
>      s->cursor_end = 0;
>      s->cursor_offset = 0;
> +    s->big_endian_fb = s->default_endian_fb;
>      memset(s->invalidated_y_table, '\0', sizeof(s->invalidated_y_table));
>      memset(s->last_palette, '\0', sizeof(s->last_palette));
>      memset(s->last_ch_attr, '\0', sizeof(s->last_ch_attr));
> @@ -2020,6 +2023,28 @@ static int vga_common_post_load(void *opaque, int version_id)
>      return 0;
>  }
>  
> +static bool vga_endian_state_needed(void *opaque)
> +{
> +    VGACommonState *s = opaque;
> +
> +    /*
> +     * Only send the endian state if it's different from the
> +     * default one, thus ensuring backward compatibility for
> +     * migration of the common case
> +     */
> +    return s->default_endian_fb != s->big_endian_fb;
> +}
> +
> +const VMStateDescription vmstate_vga_endian = {
> +    .name = "vga.endian",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8_EQUAL(big_endian_fb, VGACommonState),

I don't think this is right.  If I'm remembering how the vmstate
macros work, this will abort if big_endian_fb isn't equal on source
and destination.  Which means as soon as it's possible to actually
change the big_endian_fb value, migrations will start failing/

I think this should instead be VMSTATE_BOOL()

[snip]
> -    bool big_endian_fb;
> +    uint8_t big_endian_fb;
> +    uint8_t default_endian_fb;
>      /* hardware mouse cursor support */
>      uint32_t invalidated_y_table[VGA_MAX_HEIGHT / 32];
>      void (*cursor_invalidate)(struct VGACommonState *s);

In which case you also don't need to change the type of the
big_endian_fb field.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 3/3] vga-pci: add qext region to mmio

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 714 bytes --]

On Tue, Sep 23, 2014 at 02:30:57PM +0200, Gerd Hoffmann wrote:
> Add a qemu extented register range to the standard vga mmio bar.
> Right nowe there are two registers:  One readonly register returning the
> size of the region (so we can easily add more registers there if needed)
> and one endian control register, so guests (especially ppc) can flip
> the framebuffer endianness as they need it.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 3/3] vga-pci: add qext region to mmio

[Michael S. Tsirkin]

On Tue, Sep 23, 2014 at 02:30:57PM +0200, Gerd Hoffmann wrote:
> Add a qemu extented register range to the standard vga mmio bar.
> Right nowe there are two registers:  One readonly register returning the
> size of the region (so we can easily add more registers there if needed)
> and one endian control register, so guests (especially ppc) can flip
> the framebuffer endianness as they need it.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  docs/specs/standard-vga.txt |  9 ++++++
>  hw/display/vga-pci.c        | 68 +++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/i386/pc.h        |  8 ++++++
>  3 files changed, 85 insertions(+)
> 
> diff --git a/docs/specs/standard-vga.txt b/docs/specs/standard-vga.txt
> index f82773e..19d2a74 100644
> --- a/docs/specs/standard-vga.txt
> +++ b/docs/specs/standard-vga.txt
> @@ -70,3 +70,12 @@ Likewise applies to the pci variant only for obvious reasons.
>  0500 - 0515 : bochs dispi interface registers, mapped flat
>                without index/data ports.  Use (index << 1)
>                as offset for (16bit) register access.
> +
> +0600 - 0607 : qemu extended registers.  qemu 2.2+ only.
> +              The pci revision is 2 (or greater) when
> +              these registers are present.  The registers
> +              are 32bit.
> +  0600      : qemu extended register region size, in bytes.
> +  0604      : framebuffer endianness register.
> +              - 0xbebebebe indicates big endian.
> +              - 0x1e1e1e1e indicates little endian.
> diff --git a/hw/display/vga-pci.c b/hw/display/vga-pci.c
> index 0351d94..3394ec2 100644
> --- a/hw/display/vga-pci.c
> +++ b/hw/display/vga-pci.c
> @@ -35,10 +35,18 @@
>  #define PCI_VGA_IOPORT_SIZE   (0x3e0 - 0x3c0)
>  #define PCI_VGA_BOCHS_OFFSET  0x500
>  #define PCI_VGA_BOCHS_SIZE    (0x0b * 2)
> +#define PCI_VGA_QEXT_OFFSET   0x600
> +#define PCI_VGA_QEXT_SIZE     (2 * 4)
>  #define PCI_VGA_MMIO_SIZE     0x1000
>  
> +#define PCI_VGA_QEXT_REG_SIZE         (0 * 4)
> +#define PCI_VGA_QEXT_REG_BYTEORDER    (1 * 4)
> +#define  PCI_VGA_QEXT_LITTLE_ENDIAN   0x1e1e1e1e
> +#define  PCI_VGA_QEXT_BIG_ENDIAN      0xbebebebe
> +
>  enum vga_pci_flags {
>      PCI_VGA_FLAG_ENABLE_MMIO = 1,
> +    PCI_VGA_FLAG_ENABLE_QEXT = 2,
>  };
>  
>  typedef struct PCIVGAState {
> @@ -48,6 +56,7 @@ typedef struct PCIVGAState {
>      MemoryRegion mmio;
>      MemoryRegion ioport;
>      MemoryRegion bochs;
> +    MemoryRegion qext;
>  } PCIVGAState;
>  
>  static const VMStateDescription vmstate_vga_pci = {
> @@ -140,6 +149,46 @@ static const MemoryRegionOps pci_vga_bochs_ops = {
>      .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>  
> +static uint64_t pci_vga_qext_read(void *ptr, hwaddr addr, unsigned size)
> +{
> +    PCIVGAState *d = ptr;
> +
> +    switch (addr) {
> +    case PCI_VGA_QEXT_REG_SIZE:
> +        return PCI_VGA_QEXT_SIZE;
> +    case PCI_VGA_QEXT_REG_BYTEORDER:
> +        return d->vga.big_endian_fb ?
> +            PCI_VGA_QEXT_BIG_ENDIAN : PCI_VGA_QEXT_LITTLE_ENDIAN;
> +    default:
> +        return 0;
> +    }
> +}
> +
> +static void pci_vga_qext_write(void *ptr, hwaddr addr,
> +                               uint64_t val, unsigned size)
> +{
> +    PCIVGAState *d = ptr;
> +
> +    switch (addr) {
> +    case PCI_VGA_QEXT_REG_BYTEORDER:
> +        if (val == PCI_VGA_QEXT_BIG_ENDIAN) {
> +            d->vga.big_endian_fb = true;
> +        }
> +        if (val == PCI_VGA_QEXT_LITTLE_ENDIAN) {
> +            d->vga.big_endian_fb = false;
> +        }
> +        break;
> +    }
> +}
> +
> +static const MemoryRegionOps pci_vga_qext_ops = {
> +    .read = pci_vga_qext_read,
> +    .write = pci_vga_qext_write,
> +    .valid.min_access_size = 4,
> +    .valid.max_access_size = 4,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
>  static int pci_std_vga_initfn(PCIDevice *dev)
>  {
>      PCIVGAState *d = DO_UPCAST(PCIVGAState, dev, dev);
> @@ -167,6 +216,15 @@ static int pci_std_vga_initfn(PCIDevice *dev)
>                                      &d->ioport);
>          memory_region_add_subregion(&d->mmio, PCI_VGA_BOCHS_OFFSET,
>                                      &d->bochs);
> +
> +        if (d->flags & (1 << PCI_VGA_FLAG_ENABLE_QEXT)) {
> +            memory_region_init_io(&d->qext, NULL, &pci_vga_qext_ops, d,
> +                                  "qemu extended regs", PCI_VGA_QEXT_SIZE);
> +            memory_region_add_subregion(&d->mmio, PCI_VGA_QEXT_OFFSET,
> +                                        &d->qext);
> +            pci_set_byte(&d->dev.config[PCI_REVISION_ID], 2);
> +        }
> +
>          pci_register_bar(&d->dev, 2, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
>      }
>  
> @@ -199,6 +257,14 @@ static int pci_secondary_vga_initfn(PCIDevice *dev)
>      memory_region_add_subregion(&d->mmio, PCI_VGA_BOCHS_OFFSET,
>                                  &d->bochs);
>  
> +    if (d->flags & (1 << PCI_VGA_FLAG_ENABLE_QEXT)) {
> +        memory_region_init_io(&d->qext, NULL, &pci_vga_qext_ops, d,
> +                              "qemu extended regs", PCI_VGA_QEXT_SIZE);
> +        memory_region_add_subregion(&d->mmio, PCI_VGA_QEXT_OFFSET,
> +                                    &d->qext);
> +        pci_set_byte(&d->dev.config[PCI_REVISION_ID], 2);
> +    }
> +
>      pci_register_bar(&d->dev, 0, PCI_BASE_ADDRESS_MEM_PREFETCH, &s->vram);
>      pci_register_bar(&d->dev, 2, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
>  
> @@ -215,11 +281,13 @@ static void pci_secondary_vga_reset(DeviceState *dev)
>  static Property vga_pci_properties[] = {
>      DEFINE_PROP_UINT32("vgamem_mb", PCIVGAState, vga.vram_size_mb, 16),
>      DEFINE_PROP_BIT("mmio", PCIVGAState, flags, PCI_VGA_FLAG_ENABLE_MMIO, true),
> +    DEFINE_PROP_BIT("qext", PCIVGAState, flags, PCI_VGA_FLAG_ENABLE_QEXT, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
>  static Property secondary_pci_properties[] = {
>      DEFINE_PROP_UINT32("vgamem_mb", PCIVGAState, vga.vram_size_mb, 16),
> +    DEFINE_PROP_BIT("qext", PCIVGAState, flags, PCI_VGA_FLAG_ENABLE_QEXT, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 77316d5..23acc1f 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -306,6 +306,14 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>              .driver   = "intel-hda",\
>              .property = "old_msi_addr",\
>              .value    = "on",\
> +        },{\
> +            .driver   = "VGA",\
> +            .property = "qext",\
> +            .value    = "off",\

I'd prefer a more friendly name like "qemu-extended-registers".
There's very little documentation for properties besides
the name, so the name has to be explicit.


> +        },{\
> +            .driver   = "secondary-vga",\
> +            .property = "qext",\
> +            .value    = "off",\
>          }
>  
>  #define PC_COMPAT_2_0 \
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH 3/3] vga-pci: add qext region to mmio

[Gerd Hoffmann]

On Mo, 2014-09-29 at 19:18 +0300, Michael S. Tsirkin wrote:
> > +            .driver   = "VGA",\
> > +            .property = "qext",\
> > +            .value    = "off",\
> 
> I'd prefer a more friendly name like "qemu-extended-registers".
> There's very little documentation for properties besides
> the name, so the name has to be explicit.

Done.

cheers,
  Gerd