[Qemu-devel] [PULL 0/5] Vga 20181012 patches

[Qemu-devel] [PULL 0/5] Vga 20181012 patches

[Gerd Hoffmann]

The following changes since commit 75e50c80e051423a6f55a34ee4a1eec842444a5b:

  Merge remote-tracking branch 'remotes/armbru/tags/pull-misc-2018-10-10' into staging (2018-10-11 10:43:37 +0100)

are available in the git repository at:

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

for you to fetch changes up to b398f03c60bb5739ae68c161c315c7cce8ef2fde:

  hw/display/cirrus_vga: Move "isa-cirrus-vga" device into a separate file (2018-10-12 13:50:38 +0200)

----------------------------------------------------------------
vga: config tweaks, edid updates, qxl bugfix.

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

Gerd Hoffmann (4):
  display: add separate config option for bochs-display
  bochs-display: wire up edid support
  qxl: check qxl_phys2virt return value
  i2c: switch ddc to use the new edid generator

Thomas Huth (1):
  hw/display/cirrus_vga: Move "isa-cirrus-vga" device into a separate
    file

 hw/display/cirrus_vga_internal.h | 103 ++++++++++++++++++++
 include/hw/i2c/i2c-ddc.h         |   4 +-
 hw/display/bochs-display.c       |  13 +++
 hw/display/cirrus_vga.c          | 143 +---------------------------
 hw/display/cirrus_vga_isa.c      |  98 +++++++++++++++++++
 hw/display/qxl.c                 |   2 +-
 hw/i2c/i2c-ddc.c                 | 200 ++-------------------------------------
 default-configs/pci.mak          |   1 +
 hw/display/Makefile.objs         |   4 +-
 9 files changed, 235 insertions(+), 333 deletions(-)
 create mode 100644 hw/display/cirrus_vga_internal.h
 create mode 100644 hw/display/cirrus_vga_isa.c

-- 
2.9.3

[Qemu-devel] [PULL 1/5] display: add separate config option for bochs-display

[Gerd Hoffmann]

This allows modern architectures which don't care about vga
compatibility (risc-v for example) build bochs-display without
including all vga emulation too.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Message-id: 20181005160147.892-2-kraxel@redhat.com
---
 default-configs/pci.mak  | 1 +
 hw/display/Makefile.objs | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/default-configs/pci.mak b/default-configs/pci.mak
index de53d20ac6..6c7be12779 100644
--- a/default-configs/pci.mak
+++ b/default-configs/pci.mak
@@ -44,5 +44,6 @@ CONFIG_SDHCI=y
 CONFIG_EDU=y
 CONFIG_VGA=y
 CONFIG_VGA_PCI=y
+CONFIG_BOCHS_DISPLAY=y
 CONFIG_IVSHMEM_DEVICE=$(CONFIG_IVSHMEM)
 CONFIG_ROCKER=y
diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
index 780a76b9f0..2a0de16a55 100644
--- a/hw/display/Makefile.objs
+++ b/hw/display/Makefile.objs
@@ -14,11 +14,11 @@ common-obj-$(CONFIG_SSD0323) += ssd0323.o
 common-obj-$(CONFIG_XEN) += xenfb.o
 
 common-obj-$(CONFIG_VGA_PCI) += vga-pci.o
-common-obj-$(CONFIG_VGA_PCI) += bochs-display.o
 common-obj-$(CONFIG_VGA_PCI) += edid-region.o
 common-obj-$(CONFIG_VGA_ISA) += vga-isa.o
 common-obj-$(CONFIG_VGA_ISA_MM) += vga-isa-mm.o
 common-obj-$(CONFIG_VMWARE_VGA) += vmware_vga.o
+common-obj-$(CONFIG_BOCHS_DISPLAY) += bochs-display.o
 
 common-obj-$(CONFIG_BLIZZARD) += blizzard.o
 common-obj-$(CONFIG_EXYNOS4) += exynos4210_fimd.o
-- 
2.9.3

[Qemu-devel] [PULL 2/5] bochs-display: wire up edid support

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-id: 20181005160147.892-3-kraxel@redhat.com
---
 hw/display/bochs-display.c | 13 +++++++++++++
 hw/display/Makefile.objs   |  1 +
 2 files changed, 14 insertions(+)

diff --git a/hw/display/bochs-display.c b/hw/display/bochs-display.c
index 09d8944a1b..3d439eb240 100644
--- a/hw/display/bochs-display.c
+++ b/hw/display/bochs-display.c
@@ -9,6 +9,7 @@
 #include "hw/hw.h"
 #include "hw/pci/pci.h"
 #include "hw/display/bochs-vbe.h"
+#include "hw/display/edid.h"
 
 #include "qapi/error.h"
 
@@ -35,9 +36,13 @@ typedef struct BochsDisplayState {
     MemoryRegion     mmio;
     MemoryRegion     vbe;
     MemoryRegion     qext;
+    MemoryRegion     edid;
 
     /* device config */
     uint64_t         vgamem;
+    bool             enable_edid;
+    qemu_edid_info   edid_info;
+    uint8_t          edid_blob[256];
 
     /* device registers */
     uint16_t         vbe_regs[VBE_DISPI_INDEX_NB];
@@ -283,6 +288,12 @@ static void bochs_display_realize(PCIDevice *dev, Error **errp)
     pci_register_bar(&s->pci, 0, PCI_BASE_ADDRESS_MEM_PREFETCH, &s->vram);
     pci_register_bar(&s->pci, 2, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->mmio);
 
+    if (s->enable_edid) {
+        qemu_edid_generate(s->edid_blob, sizeof(s->edid_blob), &s->edid_info);
+        qemu_edid_region_io(&s->edid, obj, s->edid_blob, sizeof(s->edid_blob));
+        memory_region_add_subregion(&s->mmio, 0, &s->edid);
+    }
+
     if (pci_bus_is_express(pci_get_bus(dev))) {
         dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
         ret = pcie_endpoint_cap_init(dev, 0x80);
@@ -325,6 +336,8 @@ static void bochs_display_exit(PCIDevice *dev)
 
 static Property bochs_display_properties[] = {
     DEFINE_PROP_SIZE("vgamem", BochsDisplayState, vgamem, 16 * MiB),
+    DEFINE_PROP_BOOL("edid", BochsDisplayState, enable_edid, false),
+    DEFINE_EDID_PROPERTIES(BochsDisplayState, edid_info),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
index 2a0de16a55..2e1b1bcd22 100644
--- a/hw/display/Makefile.objs
+++ b/hw/display/Makefile.objs
@@ -19,6 +19,7 @@ common-obj-$(CONFIG_VGA_ISA) += vga-isa.o
 common-obj-$(CONFIG_VGA_ISA_MM) += vga-isa-mm.o
 common-obj-$(CONFIG_VMWARE_VGA) += vmware_vga.o
 common-obj-$(CONFIG_BOCHS_DISPLAY) += bochs-display.o
+common-obj-$(CONFIG_BOCHS_DISPLAY) += edid-region.o
 
 common-obj-$(CONFIG_BLIZZARD) += blizzard.o
 common-obj-$(CONFIG_EXYNOS4) += exynos4210_fimd.o
-- 
2.9.3

[Qemu-devel] [PULL 3/5] qxl: check qxl_phys2virt return value

[Gerd Hoffmann]

Fixes: CID 1395986
Fixes: 979f7ef8966bc4495a710ed9e4af42098f92ee79
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-id: 20181005134608.1251-1-kraxel@redhat.com
---
 hw/display/qxl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index e628cf1286..f608abc769 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -290,7 +290,7 @@ static void qxl_spice_monitors_config_async(PCIQXLDevice *qxl, int replay)
     }
 
     cfg = qxl_phys2virt(qxl, qxl->guest_monitors_config, MEMSLOT_GROUP_GUEST);
-    if (cfg->count == 1) {
+    if (cfg != NULL && cfg->count == 1) {
         qxl->guest_primary.resized = 1;
         qxl->guest_head0_width  = cfg->heads[0].width;
         qxl->guest_head0_height = cfg->heads[0].height;
-- 
2.9.3

[Qemu-devel] [PULL 4/5] i2c: switch ddc to use the new edid generator

[Gerd Hoffmann]

This also makes the default display resolution configurable,
via xres and yres properties.  The default is 1024x768.

The old code had a hard-coded resolution of 1600x1200.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: BALATON Zoltan <balaton@eik.bme.hu>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-id: 20181005110837.28209-1-kraxel@redhat.com
---
 include/hw/i2c/i2c-ddc.h |   4 +-
 hw/i2c/i2c-ddc.c         | 200 ++---------------------------------------------
 2 files changed, 11 insertions(+), 193 deletions(-)

diff --git a/include/hw/i2c/i2c-ddc.h b/include/hw/i2c/i2c-ddc.h
index d9b5f33f58..c29443c5af 100644
--- a/include/hw/i2c/i2c-ddc.h
+++ b/include/hw/i2c/i2c-ddc.h
@@ -19,14 +19,16 @@
 #ifndef I2C_DDC_H
 #define I2C_DDC_H
 
+#include "hw/display/edid.h"
+
 /* A simple I2C slave which just returns the contents of its EDID blob. */
-
 struct I2CDDCState {
     /*< private >*/
     I2CSlave i2c;
     /*< public >*/
     bool firstbyte;
     uint8_t reg;
+    qemu_edid_info edid_info;
     uint8_t edid_blob[128];
 };
 
diff --git a/hw/i2c/i2c-ddc.c b/hw/i2c/i2c-ddc.c
index bec0c91e2d..be34fe072c 100644
--- a/hw/i2c/i2c-ddc.c
+++ b/hw/i2c/i2c-ddc.c
@@ -32,197 +32,6 @@
     }                                                                          \
 } while (0)
 
-/* Structure defining a monitor's characteristics in a
- * readable format: this should be passed to build_edid_blob()
- * to convert it into the 128 byte binary EDID blob.
- * Not all bits of the EDID are customisable here.
- */
-struct EDIDData {
-    char manuf_id[3]; /* three upper case letters */
-    uint16_t product_id;
-    uint32_t serial_no;
-    uint8_t manuf_week;
-    int manuf_year;
-    uint8_t h_cm;
-    uint8_t v_cm;
-    uint8_t gamma;
-    char monitor_name[14];
-    char serial_no_string[14];
-    /* Range limits */
-    uint8_t vmin; /* Hz */
-    uint8_t vmax; /* Hz */
-    uint8_t hmin; /* kHz */
-    uint8_t hmax; /* kHz */
-    uint8_t pixclock; /* MHz / 10 */
-    uint8_t timing_data[18];
-};
-
-typedef struct EDIDData EDIDData;
-
-/* EDID data for a simple LCD monitor */
-static const EDIDData lcd_edid = {
-    /* The manuf_id ought really to be an assigned EISA ID */
-    .manuf_id = "QMU",
-    .product_id = 0,
-    .serial_no = 1,
-    .manuf_week = 1,
-    .manuf_year = 2011,
-    .h_cm = 40,
-    .v_cm = 30,
-    .gamma = 0x78,
-    .monitor_name = "QEMU monitor",
-    .serial_no_string = "1",
-    .vmin = 40,
-    .vmax = 120,
-    .hmin = 30,
-    .hmax = 100,
-    .pixclock = 18,
-    .timing_data = {
-        /* Borrowed from a 21" LCD */
-        0x48, 0x3f, 0x40, 0x30, 0x62, 0xb0, 0x32, 0x40, 0x40,
-        0xc0, 0x13, 0x00, 0x98, 0x32, 0x11, 0x00, 0x00, 0x1e
-    }
-};
-
-static uint8_t manuf_char_to_int(char c)
-{
-    return (c - 'A') & 0x1f;
-}
-
-static void write_ascii_descriptor_block(uint8_t *descblob, uint8_t blocktype,
-                                         const char *string)
-{
-    /* Write an EDID Descriptor Block of the "ascii string" type */
-    int i;
-    descblob[0] = descblob[1] = descblob[2] = descblob[4] = 0;
-    descblob[3] = blocktype;
-    /* The rest is 13 bytes of ASCII; if less then the rest must
-     * be filled with newline then spaces
-     */
-    for (i = 5; i < 19; i++) {
-        descblob[i] = string[i - 5];
-        if (!descblob[i]) {
-            break;
-        }
-    }
-    if (i < 19) {
-        descblob[i++] = '\n';
-    }
-    for ( ; i < 19; i++) {
-        descblob[i] = ' ';
-    }
-}
-
-static void write_range_limits_descriptor(const EDIDData *edid,
-                                          uint8_t *descblob)
-{
-    int i;
-    descblob[0] = descblob[1] = descblob[2] = descblob[4] = 0;
-    descblob[3] = 0xfd;
-    descblob[5] = edid->vmin;
-    descblob[6] = edid->vmax;
-    descblob[7] = edid->hmin;
-    descblob[8] = edid->hmax;
-    descblob[9] = edid->pixclock;
-    descblob[10] = 0;
-    descblob[11] = 0xa;
-    for (i = 12; i < 19; i++) {
-        descblob[i] = 0x20;
-    }
-}
-
-static void build_edid_blob(const EDIDData *edid, uint8_t *blob)
-{
-    /* Write an EDID 1.3 format blob (128 bytes) based
-     * on the EDIDData structure.
-     */
-    int i;
-    uint8_t cksum;
-
-    /* 00-07 : header */
-    blob[0] = blob[7] = 0;
-    for (i = 1 ; i < 7; i++) {
-        blob[i] = 0xff;
-    }
-    /* 08-09 : manufacturer ID */
-    blob[8] = (manuf_char_to_int(edid->manuf_id[0]) << 2)
-        | (manuf_char_to_int(edid->manuf_id[1]) >> 3);
-    blob[9] = (manuf_char_to_int(edid->manuf_id[1]) << 5)
-        | manuf_char_to_int(edid->manuf_id[2]);
-    /* 10-11 : product ID code */
-    blob[10] = edid->product_id;
-    blob[11] = edid->product_id >> 8;
-    blob[12] = edid->serial_no;
-    blob[13] = edid->serial_no >> 8;
-    blob[14] = edid->serial_no >> 16;
-    blob[15] = edid->serial_no >> 24;
-    /* 16 : week of manufacture */
-    blob[16] = edid->manuf_week;
-    /* 17 : year of manufacture - 1990 */
-    blob[17] = edid->manuf_year - 1990;
-    /* 18, 19 : EDID version and revision */
-    blob[18] = 1;
-    blob[19] = 3;
-    /* 20 - 24 : basic display parameters */
-    /* We are always a digital display */
-    blob[20] = 0x80;
-    /* 21, 22 : max h/v size in cm */
-    blob[21] = edid->h_cm;
-    blob[22] = edid->v_cm;
-    /* 23 : gamma (divide by 100 then add 1 for actual value) */
-    blob[23] = edid->gamma;
-    /* 24 feature support: no power management, RGB, preferred timing mode,
-     * standard colour space
-     */
-    blob[24] = 0x0e;
-    /* 25 - 34 : chromaticity coordinates. These are the
-     * standard sRGB chromaticity values
-     */
-    blob[25] = 0xee;
-    blob[26] = 0x91;
-    blob[27] = 0xa3;
-    blob[28] = 0x54;
-    blob[29] = 0x4c;
-    blob[30] = 0x99;
-    blob[31] = 0x26;
-    blob[32] = 0x0f;
-    blob[33] = 0x50;
-    blob[34] = 0x54;
-    /* 35, 36 : Established timings: claim to support everything */
-    blob[35] = blob[36] = 0xff;
-    /* 37 : manufacturer's reserved timing: none */
-    blob[37] = 0;
-    /* 38 - 53 : standard timing identification
-     * don't claim anything beyond what the 'established timings'
-     * already provide. Unused slots must be (0x1, 0x1)
-     */
-    for (i = 38; i < 54; i++) {
-        blob[i] = 0x1;
-    }
-    /* 54 - 71 : descriptor block 1 : must be preferred timing data */
-    memcpy(blob + 54, edid->timing_data, 18);
-    /* 72 - 89, 90 - 107, 108 - 125 : descriptor block 2, 3, 4
-     * Order not important, but we must have a monitor name and a
-     * range limits descriptor.
-     */
-    write_range_limits_descriptor(edid, blob + 72);
-    write_ascii_descriptor_block(blob + 90, 0xfc, edid->monitor_name);
-    write_ascii_descriptor_block(blob + 108, 0xff, edid->serial_no_string);
-
-    /* 126 : extension flag */
-    blob[126] = 0;
-
-    cksum = 0;
-    for (i = 0; i < 127; i++) {
-        cksum += blob[i];
-    }
-    /* 127 : checksum */
-    blob[127] = -cksum;
-    if (DEBUG_I2CDDC) {
-        qemu_hexdump((char *)blob, stdout, "", 128);
-    }
-}
-
 static void i2c_ddc_reset(DeviceState *ds)
 {
     I2CDDCState *s = I2CDDC(ds);
@@ -270,7 +79,8 @@ static int i2c_ddc_tx(I2CSlave *i2c, uint8_t data)
 static void i2c_ddc_init(Object *obj)
 {
     I2CDDCState *s = I2CDDC(obj);
-    build_edid_blob(&lcd_edid, s->edid_blob);
+
+    qemu_edid_generate(s->edid_blob, sizeof(s->edid_blob), &s->edid_info);
 }
 
 static const VMStateDescription vmstate_i2c_ddc = {
@@ -283,6 +93,11 @@ static const VMStateDescription vmstate_i2c_ddc = {
     }
 };
 
+static Property i2c_ddc_properties[] = {
+    DEFINE_EDID_PROPERTIES(I2CDDCState, edid_info),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void i2c_ddc_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
@@ -290,6 +105,7 @@ static void i2c_ddc_class_init(ObjectClass *oc, void *data)
 
     dc->reset = i2c_ddc_reset;
     dc->vmsd = &vmstate_i2c_ddc;
+    dc->props = i2c_ddc_properties;
     isc->event = i2c_ddc_event;
     isc->recv = i2c_ddc_rx;
     isc->send = i2c_ddc_tx;
-- 
2.9.3

[Qemu-devel] [PULL 5/5] hw/display/cirrus_vga: Move "isa-cirrus-vga" device into a separate file

[Gerd Hoffmann]

From: Thomas Huth <thuth@redhat.com>

In downstream distros like RHEL we'd like to disable some of the "legacy"
devices of QEMU. The ISA version of the Cirrus VGA device is one of these
legacy devices. So let's make the build process a little bit more flexible
here by putting the Cirrus ISA code into a separate file which is only
included if both, CONFIG_VGA_CIRRUS and CONFIG_VGA_ISA are set.

Note that this disables "isa-cirrus-vga" for the ppc-softmmu and the
alpha-softmmu target since CONFIG_VGA_ISA is not set there. But I think
this is OK since these targets are only interested in the PCI variant
anyway.

Signed-off-by: Thomas Huth <thuth@redhat.com>
Message-id: 1539339106-32427-1-git-send-email-thuth@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/cirrus_vga_internal.h | 103 ++++++++++++++++++++++++++++
 hw/display/cirrus_vga.c          | 143 ++-------------------------------------
 hw/display/cirrus_vga_isa.c      |  98 +++++++++++++++++++++++++++
 hw/display/Makefile.objs         |   1 +
 4 files changed, 207 insertions(+), 138 deletions(-)
 create mode 100644 hw/display/cirrus_vga_internal.h
 create mode 100644 hw/display/cirrus_vga_isa.c

diff --git a/hw/display/cirrus_vga_internal.h b/hw/display/cirrus_vga_internal.h
new file mode 100644
index 0000000000..a78ebbd920
--- /dev/null
+++ b/hw/display/cirrus_vga_internal.h
@@ -0,0 +1,103 @@
+/*
+ * QEMU Cirrus CLGD 54xx VGA Emulator, ISA bus support
+ *
+ * Copyright (c) 2004 Fabrice Bellard
+ * Copyright (c) 2004 Makoto Suzuki (suzu)
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef CIRRUS_VGA_INTERNAL_H
+#define CIRRUS_VGA_INTERNAL_H
+
+#include "vga_int.h"
+
+/* IDs */
+#define CIRRUS_ID_CLGD5422  (0x23 << 2)
+#define CIRRUS_ID_CLGD5426  (0x24 << 2)
+#define CIRRUS_ID_CLGD5424  (0x25 << 2)
+#define CIRRUS_ID_CLGD5428  (0x26 << 2)
+#define CIRRUS_ID_CLGD5430  (0x28 << 2)
+#define CIRRUS_ID_CLGD5434  (0x2A << 2)
+#define CIRRUS_ID_CLGD5436  (0x2B << 2)
+#define CIRRUS_ID_CLGD5446  (0x2E << 2)
+
+extern const VMStateDescription vmstate_cirrus_vga;
+
+struct CirrusVGAState;
+typedef void (*cirrus_bitblt_rop_t)(struct CirrusVGAState *s,
+                                    uint32_t dstaddr, uint32_t srcaddr,
+                                    int dstpitch, int srcpitch,
+                                    int bltwidth, int bltheight);
+
+typedef struct CirrusVGAState {
+    VGACommonState vga;
+
+    MemoryRegion cirrus_vga_io;
+    MemoryRegion cirrus_linear_io;
+    MemoryRegion cirrus_linear_bitblt_io;
+    MemoryRegion cirrus_mmio_io;
+    MemoryRegion pci_bar;
+    bool linear_vram;  /* vga.vram mapped over cirrus_linear_io */
+    MemoryRegion low_mem_container; /* container for 0xa0000-0xc0000 */
+    MemoryRegion low_mem;           /* always mapped, overridden by: */
+    MemoryRegion cirrus_bank[2];    /*   aliases at 0xa0000-0xb0000  */
+    uint32_t cirrus_addr_mask;
+    uint32_t linear_mmio_mask;
+    uint8_t cirrus_shadow_gr0;
+    uint8_t cirrus_shadow_gr1;
+    uint8_t cirrus_hidden_dac_lockindex;
+    uint8_t cirrus_hidden_dac_data;
+    uint32_t cirrus_bank_base[2];
+    uint32_t cirrus_bank_limit[2];
+    uint8_t cirrus_hidden_palette[48];
+    bool enable_blitter;
+    int cirrus_blt_pixelwidth;
+    int cirrus_blt_width;
+    int cirrus_blt_height;
+    int cirrus_blt_dstpitch;
+    int cirrus_blt_srcpitch;
+    uint32_t cirrus_blt_fgcol;
+    uint32_t cirrus_blt_bgcol;
+    uint32_t cirrus_blt_dstaddr;
+    uint32_t cirrus_blt_srcaddr;
+    uint8_t cirrus_blt_mode;
+    uint8_t cirrus_blt_modeext;
+    cirrus_bitblt_rop_t cirrus_rop;
+#define CIRRUS_BLTBUFSIZE (2048 * 4) /* one line width */
+    uint8_t cirrus_bltbuf[CIRRUS_BLTBUFSIZE];
+    uint8_t *cirrus_srcptr;
+    uint8_t *cirrus_srcptr_end;
+    uint32_t cirrus_srccounter;
+    /* hwcursor display state */
+    int last_hw_cursor_size;
+    int last_hw_cursor_x;
+    int last_hw_cursor_y;
+    int last_hw_cursor_y_start;
+    int last_hw_cursor_y_end;
+    int real_vram_size; /* XXX: suppress that */
+    int device_id;
+    int bustype;
+} CirrusVGAState;
+
+void cirrus_init_common(CirrusVGAState *s, Object *owner,
+                        int device_id, int is_pci,
+                        MemoryRegion *system_memory, MemoryRegion *system_io);
+
+#endif
diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 04c87c8e8d..d9b854d74d 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -33,8 +33,8 @@
 #include "hw/hw.h"
 #include "hw/pci/pci.h"
 #include "ui/pixel_ops.h"
-#include "vga_int.h"
 #include "hw/loader.h"
+#include "cirrus_vga_internal.h"
 
 /*
  * TODO:
@@ -52,16 +52,6 @@
  *
  ***************************************/
 
-// ID
-#define CIRRUS_ID_CLGD5422  (0x23<<2)
-#define CIRRUS_ID_CLGD5426  (0x24<<2)
-#define CIRRUS_ID_CLGD5424  (0x25<<2)
-#define CIRRUS_ID_CLGD5428  (0x26<<2)
-#define CIRRUS_ID_CLGD5430  (0x28<<2)
-#define CIRRUS_ID_CLGD5434  (0x2A<<2)
-#define CIRRUS_ID_CLGD5436  (0x2B<<2)
-#define CIRRUS_ID_CLGD5446  (0x2E<<2)
-
 // sequencer 0x07
 #define CIRRUS_SR7_BPP_VGA            0x00
 #define CIRRUS_SR7_BPP_SVGA           0x01
@@ -176,65 +166,10 @@
 
 #define CIRRUS_PNPMMIO_SIZE         0x1000
 
-struct CirrusVGAState;
-typedef void (*cirrus_bitblt_rop_t) (struct CirrusVGAState *s,
-                                     uint32_t dstaddr, uint32_t srcaddr,
-				     int dstpitch, int srcpitch,
-				     int bltwidth, int bltheight);
 typedef void (*cirrus_fill_t)(struct CirrusVGAState *s,
                               uint32_t dstaddr, int dst_pitch,
                               int width, int height);
 
-typedef struct CirrusVGAState {
-    VGACommonState vga;
-
-    MemoryRegion cirrus_vga_io;
-    MemoryRegion cirrus_linear_io;
-    MemoryRegion cirrus_linear_bitblt_io;
-    MemoryRegion cirrus_mmio_io;
-    MemoryRegion pci_bar;
-    bool linear_vram;  /* vga.vram mapped over cirrus_linear_io */
-    MemoryRegion low_mem_container; /* container for 0xa0000-0xc0000 */
-    MemoryRegion low_mem;           /* always mapped, overridden by: */
-    MemoryRegion cirrus_bank[2];    /*   aliases at 0xa0000-0xb0000  */
-    uint32_t cirrus_addr_mask;
-    uint32_t linear_mmio_mask;
-    uint8_t cirrus_shadow_gr0;
-    uint8_t cirrus_shadow_gr1;
-    uint8_t cirrus_hidden_dac_lockindex;
-    uint8_t cirrus_hidden_dac_data;
-    uint32_t cirrus_bank_base[2];
-    uint32_t cirrus_bank_limit[2];
-    uint8_t cirrus_hidden_palette[48];
-    bool enable_blitter;
-    int cirrus_blt_pixelwidth;
-    int cirrus_blt_width;
-    int cirrus_blt_height;
-    int cirrus_blt_dstpitch;
-    int cirrus_blt_srcpitch;
-    uint32_t cirrus_blt_fgcol;
-    uint32_t cirrus_blt_bgcol;
-    uint32_t cirrus_blt_dstaddr;
-    uint32_t cirrus_blt_srcaddr;
-    uint8_t cirrus_blt_mode;
-    uint8_t cirrus_blt_modeext;
-    cirrus_bitblt_rop_t cirrus_rop;
-#define CIRRUS_BLTBUFSIZE (2048 * 4) /* one line width */
-    uint8_t cirrus_bltbuf[CIRRUS_BLTBUFSIZE];
-    uint8_t *cirrus_srcptr;
-    uint8_t *cirrus_srcptr_end;
-    uint32_t cirrus_srccounter;
-    /* hwcursor display state */
-    int last_hw_cursor_size;
-    int last_hw_cursor_x;
-    int last_hw_cursor_y;
-    int last_hw_cursor_y_start;
-    int last_hw_cursor_y_end;
-    int real_vram_size; /* XXX: suppress that */
-    int device_id;
-    int bustype;
-} CirrusVGAState;
-
 typedef struct PCICirrusVGAState {
     PCIDevice dev;
     CirrusVGAState cirrus_vga;
@@ -244,16 +179,6 @@ typedef struct PCICirrusVGAState {
 #define PCI_CIRRUS_VGA(obj) \
     OBJECT_CHECK(PCICirrusVGAState, (obj), TYPE_PCI_CIRRUS_VGA)
 
-#define TYPE_ISA_CIRRUS_VGA "isa-cirrus-vga"
-#define ISA_CIRRUS_VGA(obj) \
-    OBJECT_CHECK(ISACirrusVGAState, (obj), TYPE_ISA_CIRRUS_VGA)
-
-typedef struct ISACirrusVGAState {
-    ISADevice parent_obj;
-
-    CirrusVGAState cirrus_vga;
-} ISACirrusVGAState;
-
 static uint8_t rop_to_index[256];
 
 /***************************************
@@ -2829,7 +2754,7 @@ static int cirrus_post_load(void *opaque, int version_id)
     return 0;
 }
 
-static const VMStateDescription vmstate_cirrus_vga = {
+const VMStateDescription vmstate_cirrus_vga = {
     .name = "cirrus_vga",
     .version_id = 2,
     .minimum_version_id = 1,
@@ -2932,10 +2857,9 @@ static const MemoryRegionOps cirrus_vga_io_ops = {
     },
 };
 
-static void cirrus_init_common(CirrusVGAState *s, Object *owner,
-                               int device_id, int is_pci,
-                               MemoryRegion *system_memory,
-                               MemoryRegion *system_io)
+void cirrus_init_common(CirrusVGAState *s, Object *owner,
+                        int device_id, int is_pci,
+                        MemoryRegion *system_memory, MemoryRegion *system_io)
 {
     int i;
     static int inited;
@@ -3031,62 +2955,6 @@ static void cirrus_init_common(CirrusVGAState *s, Object *owner,
 
 /***************************************
  *
- *  ISA bus support
- *
- ***************************************/
-
-static void isa_cirrus_vga_realizefn(DeviceState *dev, Error **errp)
-{
-    ISADevice *isadev = ISA_DEVICE(dev);
-    ISACirrusVGAState *d = ISA_CIRRUS_VGA(dev);
-    VGACommonState *s = &d->cirrus_vga.vga;
-
-    /* follow real hardware, cirrus card emulated has 4 MB video memory.
-       Also accept 8 MB/16 MB for backward compatibility. */
-    if (s->vram_size_mb != 4 && s->vram_size_mb != 8 &&
-        s->vram_size_mb != 16) {
-        error_setg(errp, "Invalid cirrus_vga ram size '%u'",
-                   s->vram_size_mb);
-        return;
-    }
-    s->global_vmstate = true;
-    vga_common_init(s, OBJECT(dev));
-    cirrus_init_common(&d->cirrus_vga, OBJECT(dev), CIRRUS_ID_CLGD5430, 0,
-                       isa_address_space(isadev),
-                       isa_address_space_io(isadev));
-    s->con = graphic_console_init(dev, 0, s->hw_ops, s);
-    rom_add_vga(VGABIOS_CIRRUS_FILENAME);
-    /* XXX ISA-LFB support */
-    /* FIXME not qdev yet */
-}
-
-static Property isa_cirrus_vga_properties[] = {
-    DEFINE_PROP_UINT32("vgamem_mb", struct ISACirrusVGAState,
-                       cirrus_vga.vga.vram_size_mb, 4),
-    DEFINE_PROP_BOOL("blitter", struct ISACirrusVGAState,
-                     cirrus_vga.enable_blitter, true),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
-static void isa_cirrus_vga_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-
-    dc->vmsd  = &vmstate_cirrus_vga;
-    dc->realize = isa_cirrus_vga_realizefn;
-    dc->props = isa_cirrus_vga_properties;
-    set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
-}
-
-static const TypeInfo isa_cirrus_vga_info = {
-    .name          = TYPE_ISA_CIRRUS_VGA,
-    .parent        = TYPE_ISA_DEVICE,
-    .instance_size = sizeof(ISACirrusVGAState),
-    .class_init = isa_cirrus_vga_class_init,
-};
-
-/***************************************
- *
  *  PCI bus support
  *
  ***************************************/
@@ -3171,7 +3039,6 @@ static const TypeInfo cirrus_vga_info = {
 
 static void cirrus_vga_register_types(void)
 {
-    type_register_static(&isa_cirrus_vga_info);
     type_register_static(&cirrus_vga_info);
 }
 
diff --git a/hw/display/cirrus_vga_isa.c b/hw/display/cirrus_vga_isa.c
new file mode 100644
index 0000000000..fa10b74230
--- /dev/null
+++ b/hw/display/cirrus_vga_isa.c
@@ -0,0 +1,98 @@
+/*
+ * QEMU Cirrus CLGD 54xx VGA Emulator, ISA bus support
+ *
+ * Copyright (c) 2004 Fabrice Bellard
+ * Copyright (c) 2004 Makoto Suzuki (suzu)
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/hw.h"
+#include "hw/loader.h"
+#include "hw/isa/isa.h"
+#include "cirrus_vga_internal.h"
+
+#define TYPE_ISA_CIRRUS_VGA "isa-cirrus-vga"
+#define ISA_CIRRUS_VGA(obj) \
+    OBJECT_CHECK(ISACirrusVGAState, (obj), TYPE_ISA_CIRRUS_VGA)
+
+typedef struct ISACirrusVGAState {
+    ISADevice parent_obj;
+
+    CirrusVGAState cirrus_vga;
+} ISACirrusVGAState;
+
+static void isa_cirrus_vga_realizefn(DeviceState *dev, Error **errp)
+{
+    ISADevice *isadev = ISA_DEVICE(dev);
+    ISACirrusVGAState *d = ISA_CIRRUS_VGA(dev);
+    VGACommonState *s = &d->cirrus_vga.vga;
+
+    /* follow real hardware, cirrus card emulated has 4 MB video memory.
+       Also accept 8 MB/16 MB for backward compatibility. */
+    if (s->vram_size_mb != 4 && s->vram_size_mb != 8 &&
+        s->vram_size_mb != 16) {
+        error_setg(errp, "Invalid cirrus_vga ram size '%u'",
+                   s->vram_size_mb);
+        return;
+    }
+    s->global_vmstate = true;
+    vga_common_init(s, OBJECT(dev));
+    cirrus_init_common(&d->cirrus_vga, OBJECT(dev), CIRRUS_ID_CLGD5430, 0,
+                       isa_address_space(isadev),
+                       isa_address_space_io(isadev));
+    s->con = graphic_console_init(dev, 0, s->hw_ops, s);
+    rom_add_vga(VGABIOS_CIRRUS_FILENAME);
+    /* XXX ISA-LFB support */
+    /* FIXME not qdev yet */
+}
+
+static Property isa_cirrus_vga_properties[] = {
+    DEFINE_PROP_UINT32("vgamem_mb", struct ISACirrusVGAState,
+                       cirrus_vga.vga.vram_size_mb, 4),
+    DEFINE_PROP_BOOL("blitter", struct ISACirrusVGAState,
+                     cirrus_vga.enable_blitter, true),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void isa_cirrus_vga_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->vmsd  = &vmstate_cirrus_vga;
+    dc->realize = isa_cirrus_vga_realizefn;
+    dc->props = isa_cirrus_vga_properties;
+    set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
+}
+
+static const TypeInfo isa_cirrus_vga_info = {
+    .name          = TYPE_ISA_CIRRUS_VGA,
+    .parent        = TYPE_ISA_DEVICE,
+    .instance_size = sizeof(ISACirrusVGAState),
+    .class_init = isa_cirrus_vga_class_init,
+};
+
+static void cirrus_vga_isa_register_types(void)
+{
+    type_register_static(&isa_cirrus_vga_info);
+}
+
+type_init(cirrus_vga_isa_register_types)
diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
index 2e1b1bcd22..97acd5b6cb 100644
--- a/hw/display/Makefile.objs
+++ b/hw/display/Makefile.objs
@@ -5,6 +5,7 @@ common-obj-$(CONFIG_FW_CFG_DMA) += ramfb-standalone.o
 
 common-obj-$(CONFIG_ADS7846) += ads7846.o
 common-obj-$(CONFIG_VGA_CIRRUS) += cirrus_vga.o
+common-obj-$(call land,$(CONFIG_VGA_CIRRUS),$(CONFIG_VGA_ISA))+=cirrus_vga_isa.o
 common-obj-$(CONFIG_G364FB) += g364fb.o
 common-obj-$(CONFIG_JAZZ_LED) += jazz_led.o
 common-obj-$(CONFIG_PL110) += pl110.o
-- 
2.9.3

Re: [Qemu-devel] [PULL 0/5] Vga 20181012 patches

[Peter Maydell]

On 12 October 2018 at 13:38, Gerd Hoffmann <kraxel@redhat.com> wrote:
> The following changes since commit 75e50c80e051423a6f55a34ee4a1eec842444a5b:
>
>   Merge remote-tracking branch 'remotes/armbru/tags/pull-misc-2018-10-10' into staging (2018-10-11 10:43:37 +0100)
>
> are available in the git repository at:
>
>   git://git.kraxel.org/qemu tags/vga-20181012-pull-request
>
> for you to fetch changes up to b398f03c60bb5739ae68c161c315c7cce8ef2fde:
>
>   hw/display/cirrus_vga: Move "isa-cirrus-vga" device into a separate file (2018-10-12 13:50:38 +0200)
>
> ----------------------------------------------------------------
> vga: config tweaks, edid updates, qxl bugfix.
>
> ----------------------------------------------------------------
>
> Gerd Hoffmann (4):
>   display: add separate config option for bochs-display
>   bochs-display: wire up edid support
>   qxl: check qxl_phys2virt return value
>   i2c: switch ddc to use the new edid generator
>
> Thomas Huth (1):
>   hw/display/cirrus_vga: Move "isa-cirrus-vga" device into a separate
>     file

Hi. This generates new clang runtime sanitizer errors:

QTEST_QEMU_BINARY=sh4-softmmu/qemu-system-sh4 QTEST_QEMU_IMG=qemu-img
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
gtester -k --verbose -m=quick tests/endianness-test tests/qmp-test
tests/qmp-cmd-test tests/device-introspect-test tests/cdrom-test
tests/machine-none-test tests/qom-test tests/test-hmp
TEST: tests/endianness-test... (pid=19412)
  /sh4/endianness/r2d:
/home/petmay01/linaro/qemu-for-merges/hw/display/edid-generate.c:226:5:
runtime error: store to misaligned address 0x55bc89d7cffe for type
'uint32_t' (aka 'unsigned int'), which requires 4 byte alignment
0x55bc89d7cffe: note: pointer points here
 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00  00 00
             ^

which are caused by this line in edid_desc_timing():

    *(uint32_t *)(desc) = cpu_to_le32(clock / 10000);

Casting to some pointer type like this is almost never right;
you want instead to just use one of our existing utility
functions for "store to arbitrary memory with correct
endianness and handling alignment". In this case that's
    stl_le_p(desc, clock / 10000);

This also failed to pass 'make check' on SPARC with lots of
bus errors, which are almost certainly the same misaligned
access problem.

There seem to be other instances of the problem in the
same file, eg
    *(uint16_t *)(edid +  8) = cpu_to_be16(vendor_id);
    *(uint16_t *)(edid + 10) = cpu_to_le16(model_nr);
    *(uint32_t *)(edid + 12) = cpu_to_le32(serial_nr);


Could you fix all these and resubmit, please?

thanks
-- PMM