[Qemu-devel] [PATCH 0/4] display: add new bochs-display device

[Qemu-devel] [PATCH 0/4] display: add new bochs-display device

[Gerd Hoffmann]

Gerd Hoffmann (4):
  vga: move bochs vbe defines to header file
  display: add new bochs-display device
  bochs-display: add dirty tracking support
  bochs-display: add pcie support

 hw/display/vga_int.h           |  35 +---
 include/hw/display/bochs-vbe.h |  64 ++++++++
 hw/display/bochs-display.c     | 362 +++++++++++++++++++++++++++++++++++++++++
 hw/display/vga-pci.c           |  13 --
 hw/display/Makefile.objs       |   1 +
 5 files changed, 429 insertions(+), 46 deletions(-)
 create mode 100644 include/hw/display/bochs-vbe.h
 create mode 100644 hw/display/bochs-display.c

-- 
2.9.3

[Qemu-devel] [PATCH 1/4] vga: move bochs vbe defines to header file

[Gerd Hoffmann]

Create a new header file, move the bochs vbe dispi interface
defines to it, so they can be used outside vga code.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/vga_int.h           | 35 ++---------------------
 include/hw/display/bochs-vbe.h | 64 ++++++++++++++++++++++++++++++++++++++++++
 hw/display/vga-pci.c           | 13 ---------
 3 files changed, 66 insertions(+), 46 deletions(-)
 create mode 100644 include/hw/display/bochs-vbe.h

diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
index fe23b81442..313cff84fc 100644
--- a/hw/display/vga_int.h
+++ b/hw/display/vga_int.h
@@ -29,42 +29,11 @@
 #include "exec/memory.h"
 #include "ui/console.h"
 
+#include "hw/display/bochs-vbe.h"
+
 #define ST01_V_RETRACE      0x08
 #define ST01_DISP_ENABLE    0x01
 
-#define VBE_DISPI_MAX_XRES              16000
-#define VBE_DISPI_MAX_YRES              12000
-#define VBE_DISPI_MAX_BPP               32
-
-#define VBE_DISPI_INDEX_ID              0x0
-#define VBE_DISPI_INDEX_XRES            0x1
-#define VBE_DISPI_INDEX_YRES            0x2
-#define VBE_DISPI_INDEX_BPP             0x3
-#define VBE_DISPI_INDEX_ENABLE          0x4
-#define VBE_DISPI_INDEX_BANK            0x5
-#define VBE_DISPI_INDEX_VIRT_WIDTH      0x6
-#define VBE_DISPI_INDEX_VIRT_HEIGHT     0x7
-#define VBE_DISPI_INDEX_X_OFFSET        0x8
-#define VBE_DISPI_INDEX_Y_OFFSET        0x9
-#define VBE_DISPI_INDEX_NB              0xa /* size of vbe_regs[] */
-#define VBE_DISPI_INDEX_VIDEO_MEMORY_64K 0xa /* read-only, not in vbe_regs */
-
-#define VBE_DISPI_ID0                   0xB0C0
-#define VBE_DISPI_ID1                   0xB0C1
-#define VBE_DISPI_ID2                   0xB0C2
-#define VBE_DISPI_ID3                   0xB0C3
-#define VBE_DISPI_ID4                   0xB0C4
-#define VBE_DISPI_ID5                   0xB0C5
-
-#define VBE_DISPI_DISABLED              0x00
-#define VBE_DISPI_ENABLED               0x01
-#define VBE_DISPI_GETCAPS               0x02
-#define VBE_DISPI_8BIT_DAC              0x20
-#define VBE_DISPI_LFB_ENABLED           0x40
-#define VBE_DISPI_NOCLEARMEM            0x80
-
-#define VBE_DISPI_LFB_PHYSICAL_ADDRESS  0xE0000000
-
 #define CH_ATTR_SIZE (160 * 100)
 #define VGA_MAX_HEIGHT 2048
 
diff --git a/include/hw/display/bochs-vbe.h b/include/hw/display/bochs-vbe.h
new file mode 100644
index 0000000000..6f27ed4a91
--- /dev/null
+++ b/include/hw/display/bochs-vbe.h
@@ -0,0 +1,64 @@
+/*
+ * bochs vesa bios extension interface
+ */
+
+#define VBE_DISPI_MAX_XRES              16000
+#define VBE_DISPI_MAX_YRES              12000
+#define VBE_DISPI_MAX_BPP               32
+
+#define VBE_DISPI_INDEX_ID              0x0
+#define VBE_DISPI_INDEX_XRES            0x1
+#define VBE_DISPI_INDEX_YRES            0x2
+#define VBE_DISPI_INDEX_BPP             0x3
+#define VBE_DISPI_INDEX_ENABLE          0x4
+#define VBE_DISPI_INDEX_BANK            0x5
+#define VBE_DISPI_INDEX_VIRT_WIDTH      0x6
+#define VBE_DISPI_INDEX_VIRT_HEIGHT     0x7
+#define VBE_DISPI_INDEX_X_OFFSET        0x8
+#define VBE_DISPI_INDEX_Y_OFFSET        0x9
+#define VBE_DISPI_INDEX_NB              0xa /* size of vbe_regs[] */
+#define VBE_DISPI_INDEX_VIDEO_MEMORY_64K 0xa /* read-only, not in vbe_regs */
+
+/* VBE_DISPI_INDEX_ID */
+#define VBE_DISPI_ID0                   0xB0C0
+#define VBE_DISPI_ID1                   0xB0C1
+#define VBE_DISPI_ID2                   0xB0C2
+#define VBE_DISPI_ID3                   0xB0C3
+#define VBE_DISPI_ID4                   0xB0C4
+#define VBE_DISPI_ID5                   0xB0C5
+
+/* VBE_DISPI_INDEX_ENABLE */
+#define VBE_DISPI_DISABLED              0x00
+#define VBE_DISPI_ENABLED               0x01
+#define VBE_DISPI_GETCAPS               0x02
+#define VBE_DISPI_8BIT_DAC              0x20
+#define VBE_DISPI_LFB_ENABLED           0x40
+#define VBE_DISPI_NOCLEARMEM            0x80
+
+/* only used by isa-vga, pci vga devices use a memory bar */
+#define VBE_DISPI_LFB_PHYSICAL_ADDRESS  0xE0000000
+
+
+/*
+ * qemu extension: mmio bar (region 2)
+ */
+
+#define PCI_VGA_MMIO_SIZE     0x1000
+
+/* vga register region */
+#define PCI_VGA_IOPORT_OFFSET 0x400
+#define PCI_VGA_IOPORT_SIZE   (0x3e0 - 0x3c0)
+
+/* bochs vbe register region */
+#define PCI_VGA_BOCHS_OFFSET  0x500
+#define PCI_VGA_BOCHS_SIZE    (0x0b * 2)
+
+/* qemu extension register region */
+#define PCI_VGA_QEXT_OFFSET   0x600
+#define PCI_VGA_QEXT_SIZE     (2 * 4)
+
+/* qemu extension registers */
+#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
diff --git a/hw/display/vga-pci.c b/hw/display/vga-pci.c
index f312930664..fb3e4cd400 100644
--- a/hw/display/vga-pci.c
+++ b/hw/display/vga-pci.c
@@ -31,19 +31,6 @@
 #include "qemu/timer.h"
 #include "hw/loader.h"
 
-#define PCI_VGA_IOPORT_OFFSET 0x400
-#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,
-- 
2.9.3

[Qemu-devel] [PATCH 2/4] display: add new bochs-display device

[Gerd Hoffmann]

After writing up the virtual mdev device emulating a display supporting
the bochs vbe dispi interface (mbochs.ko) and seeing how simple it
actually is I've figured that would be useful for qemu too.

So, here it is, -device bochs-display.  It is basically -device VGA
without legacy vga emulation.  PCI bar 0 is the framebuffer, PCI bar 2
is mmio with the registers.  The vga registers are simply not there
though, neither in the legacy ioport location nor in the mmio bar.
Consequently it is PCI class DISPLAY_OTHER not DISPLAY_VGA.

So there is no text mode emulation, no weird video modes (planar,
256color palette), no memory window at 0xa0000.  Just a linear
framebuffer in the pci memory bar.  And the amount of code to emulate
this (and therefore the attack surface) is an order of magnitude smaller
when compared to vga emulation.

Compatibility wise it almost works with OVMF (little tweak needed).
The bochs-drm.ko linux kernel module can handle it just fine too.
So once the OVMF fix is merged UEFI guests should not see any
functional difference to VGA.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/bochs-display.c | 323 +++++++++++++++++++++++++++++++++++++++++++++
 hw/display/Makefile.objs   |   1 +
 2 files changed, 324 insertions(+)
 create mode 100644 hw/display/bochs-display.c

diff --git a/hw/display/bochs-display.c b/hw/display/bochs-display.c
new file mode 100644
index 0000000000..beeda58475
--- /dev/null
+++ b/hw/display/bochs-display.c
@@ -0,0 +1,323 @@
+/*
+ * QEMU PCI bochs display adapter.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include "qemu/osdep.h"
+#include "hw/hw.h"
+#include "hw/pci/pci.h"
+#include "hw/display/bochs-vbe.h"
+
+#include "qapi/error.h"
+
+#include "ui/console.h"
+#include "ui/qemu-pixman.h"
+
+typedef struct BochsDisplayMode {
+    pixman_format_code_t format;
+    uint32_t             bytepp;
+    uint32_t             width;
+    uint32_t             height;
+    uint32_t             stride;
+    uint32_t             __pad;
+    uint64_t             offset;
+    uint64_t             size;
+} BochsDisplayMode;
+
+typedef struct BochsDisplayState {
+    /* parent */
+    PCIDevice        pci;
+
+    /* device elements */
+    QemuConsole      *con;
+    MemoryRegion     vram;
+    MemoryRegion     mmio;
+    MemoryRegion     vbe;
+    MemoryRegion     qext;
+
+    /* device config */
+    uint64_t         vgamem;
+
+    /* device registers */
+    uint16_t         vbe_regs[VBE_DISPI_INDEX_NB];
+    bool             big_endian_fb;
+
+    /* device state */
+    BochsDisplayMode mode;
+} BochsDisplayState;
+
+#define TYPE_BOCHS_DISPLAY "bochs-display"
+#define BOCHS_DISPLAY(obj) OBJECT_CHECK(BochsDisplayState, (obj), \
+                                        TYPE_BOCHS_DISPLAY)
+
+static const VMStateDescription vmstate_bochs_display = {
+    .name = "bochs-display",
+    .fields = (VMStateField[]) {
+        VMSTATE_PCI_DEVICE(pci, BochsDisplayState),
+        VMSTATE_UINT16_ARRAY(vbe_regs, BochsDisplayState, VBE_DISPI_INDEX_NB),
+        VMSTATE_BOOL(big_endian_fb, BochsDisplayState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static uint64_t bochs_display_vbe_read(void *ptr, hwaddr addr,
+                                       unsigned size)
+{
+    BochsDisplayState *s = ptr;
+    unsigned int index = addr >> 1;
+
+    switch (index) {
+    case VBE_DISPI_INDEX_ID:
+        return VBE_DISPI_ID5;
+    case VBE_DISPI_INDEX_VIDEO_MEMORY_64K:
+        return s->vgamem / (64 * 1024);
+    }
+
+    if (index >= ARRAY_SIZE(s->vbe_regs)) {
+        return -1;
+    }
+    return s->vbe_regs[index];
+}
+
+static void bochs_display_vbe_write(void *ptr, hwaddr addr,
+                                    uint64_t val, unsigned size)
+{
+    BochsDisplayState *s = ptr;
+    unsigned int index = addr >> 1;
+
+    if (index >= ARRAY_SIZE(s->vbe_regs)) {
+        return;
+    }
+    s->vbe_regs[index] = val;
+}
+
+static const MemoryRegionOps bochs_display_vbe_ops = {
+    .read = bochs_display_vbe_read,
+    .write = bochs_display_vbe_write,
+    .valid.min_access_size = 1,
+    .valid.max_access_size = 4,
+    .impl.min_access_size = 2,
+    .impl.max_access_size = 2,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static uint64_t bochs_display_qext_read(void *ptr, hwaddr addr,
+                                        unsigned size)
+{
+    BochsDisplayState *s = ptr;
+
+    switch (addr) {
+    case PCI_VGA_QEXT_REG_SIZE:
+        return PCI_VGA_QEXT_SIZE;
+    case PCI_VGA_QEXT_REG_BYTEORDER:
+        return s->big_endian_fb ?
+            PCI_VGA_QEXT_BIG_ENDIAN : PCI_VGA_QEXT_LITTLE_ENDIAN;
+    default:
+        return 0;
+    }
+}
+
+static void bochs_display_qext_write(void *ptr, hwaddr addr,
+                                     uint64_t val, unsigned size)
+{
+    BochsDisplayState *s = ptr;
+
+    switch (addr) {
+    case PCI_VGA_QEXT_REG_BYTEORDER:
+        if (val == PCI_VGA_QEXT_BIG_ENDIAN) {
+            s->big_endian_fb = true;
+        }
+        if (val == PCI_VGA_QEXT_LITTLE_ENDIAN) {
+            s->big_endian_fb = false;
+        }
+        break;
+    }
+}
+
+static const MemoryRegionOps bochs_display_qext_ops = {
+    .read = bochs_display_qext_read,
+    .write = bochs_display_qext_write,
+    .valid.min_access_size = 4,
+    .valid.max_access_size = 4,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static void bochs_display_get_mode(BochsDisplayState *s,
+                                   BochsDisplayMode *mode)
+{
+    uint16_t *vbe = s->vbe_regs;
+    uint32_t virt_width;
+
+    if (!(vbe[VBE_DISPI_INDEX_ENABLE] & VBE_DISPI_ENABLED))
+        goto nofb;
+
+    memset(mode, 0, sizeof(*mode));
+    switch (vbe[VBE_DISPI_INDEX_BPP]) {
+    case 16:
+        /* best effort: support native endianess only */
+        mode->format = PIXMAN_r5g6b5;
+        mode->bytepp = 2;
+    case 32:
+        mode->format = s->big_endian_fb ? PIXMAN_BE_x8r8g8b8 : PIXMAN_LE_x8r8g8b8;
+        mode->bytepp = 4;
+        break;
+    default:
+        goto nofb;
+    }
+
+    mode->width  = vbe[VBE_DISPI_INDEX_XRES];
+    mode->height = vbe[VBE_DISPI_INDEX_YRES];
+    virt_width  = vbe[VBE_DISPI_INDEX_VIRT_WIDTH];
+    if (virt_width < mode->width)
+        virt_width = mode->width;
+    mode->stride = virt_width * mode->bytepp;
+    mode->size   = (uint64_t)mode->stride * mode->height;
+    mode->offset = ((uint64_t)vbe[VBE_DISPI_INDEX_X_OFFSET] * mode->bytepp +
+                    (uint64_t)vbe[VBE_DISPI_INDEX_Y_OFFSET] * mode->stride);
+
+    if (mode->width < 64 || mode->height < 64) {
+        goto nofb;
+    }
+    if (mode->offset + mode->size > s->vgamem) {
+        goto nofb;
+    }
+    return;
+
+nofb:
+    memset(mode, 0, sizeof(*mode));
+    return;
+}
+
+static void bochs_display_update(void *opaque)
+{
+    BochsDisplayState *s = opaque;
+    BochsDisplayMode mode;
+    DisplaySurface *ds;
+    uint8_t *ptr;
+
+    bochs_display_get_mode(s, &mode);
+    if (!mode.size) {
+        /* no (valid) video mode */
+        return;
+    }
+
+    if (memcmp(&s->mode, &mode, sizeof(mode)) != 0) {
+        /* video mode switch */
+        s->mode = mode;
+        ptr = memory_region_get_ram_ptr(&s->vram);
+        ds = qemu_create_displaysurface_from(mode.width,
+                                             mode.height,
+                                             mode.format,
+                                             mode.stride,
+                                             ptr + mode.offset);
+        dpy_gfx_replace_surface(s->con, ds);
+    }
+
+    dpy_gfx_update_full(s->con);
+}
+
+static const GraphicHwOps bochs_display_gfx_ops = {
+    .gfx_update = bochs_display_update,
+};
+
+static void bochs_display_realize(PCIDevice *dev, Error **errp)
+{
+    BochsDisplayState *s = BOCHS_DISPLAY(dev);
+    Object *obj = OBJECT(dev);
+
+    s->con = graphic_console_init(DEVICE(dev), 0, &bochs_display_gfx_ops, s);
+
+    if (s->vgamem < (4 * 1024 * 1024)) {
+        error_setg(errp, "bochs-display: video memory too small");
+    }
+    if (s->vgamem > (256 * 1024 * 1024)) {
+        error_setg(errp, "bochs-display: video memory too big");
+    }
+    s->vgamem = pow2ceil(s->vgamem);
+
+    memory_region_init_ram(&s->vram, obj, "bochs-display-vram", s->vgamem,
+                           &error_fatal);
+    memory_region_init_io(&s->vbe, obj, &bochs_display_vbe_ops, s,
+                          "bochs dispi interface", PCI_VGA_BOCHS_SIZE);
+    memory_region_init_io(&s->qext, obj, &bochs_display_qext_ops, s,
+                          "qemu extended regs", PCI_VGA_QEXT_SIZE);
+
+    memory_region_init(&s->mmio, obj, "bochs-display-mmio", 4096);
+    memory_region_add_subregion(&s->mmio, PCI_VGA_BOCHS_OFFSET, &s->vbe);
+    memory_region_add_subregion(&s->mmio, PCI_VGA_QEXT_OFFSET, &s->qext);
+
+    pci_set_byte(&s->pci.config[PCI_REVISION_ID], 2);
+    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);
+}
+
+static bool bochs_display_get_big_endian_fb(Object *obj, Error **errp)
+{
+    BochsDisplayState *s = BOCHS_DISPLAY(obj);
+
+    return s->big_endian_fb;
+}
+
+static void bochs_display_set_big_endian_fb(Object *obj, bool value, Error **errp)
+{
+    BochsDisplayState *s = BOCHS_DISPLAY(obj);
+
+    s->big_endian_fb = value;
+}
+
+static void bochs_display_init(Object *obj)
+{
+    /* Expose framebuffer byteorder via QOM */
+    object_property_add_bool(obj, "big-endian-framebuffer",
+                             bochs_display_get_big_endian_fb,
+                             bochs_display_set_big_endian_fb,
+                             NULL);
+}
+
+static void bochs_display_exit(PCIDevice *dev)
+{
+    BochsDisplayState *s = BOCHS_DISPLAY(dev);
+
+    graphic_console_close(s->con);
+}
+
+static Property bochs_display_properties[] = {
+    DEFINE_PROP_SIZE("vgamem", BochsDisplayState, vgamem, 16 * 1024 * 1024),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void bochs_display_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->class_id  = PCI_CLASS_DISPLAY_OTHER;
+    k->vendor_id = PCI_VENDOR_ID_QEMU;
+    k->device_id = PCI_DEVICE_ID_QEMU_VGA;
+
+    k->realize   = bochs_display_realize;
+    k->exit      = bochs_display_exit;
+    dc->vmsd     = &vmstate_bochs_display;
+    dc->props    = bochs_display_properties;
+    set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
+}
+
+static const TypeInfo bochs_display_type_info = {
+    .name           = TYPE_BOCHS_DISPLAY,
+    .parent         = TYPE_PCI_DEVICE,
+    .instance_size  = sizeof(BochsDisplayState),
+    .instance_init  = bochs_display_init,
+    .class_init     = bochs_display_class_init,
+    .interfaces     = (InterfaceInfo[]) {
+        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
+        { },
+    },
+};
+
+static void bochs_display_register_types(void)
+{
+    type_register_static(&bochs_display_type_info);
+}
+
+type_init(bochs_display_register_types)
diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
index 3c7c75b94d..1181c4a72b 100644
--- a/hw/display/Makefile.objs
+++ b/hw/display/Makefile.objs
@@ -9,6 +9,7 @@ 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_ISA) += vga-isa.o
 common-obj-$(CONFIG_VGA_ISA_MM) += vga-isa-mm.o
 common-obj-$(CONFIG_VMWARE_VGA) += vmware_vga.o
-- 
2.9.3

[Qemu-devel] [PATCH 3/4] bochs-display: add dirty tracking support

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/bochs-display.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/hw/display/bochs-display.c b/hw/display/bochs-display.c
index beeda58475..9ea6f798f4 100644
--- a/hw/display/bochs-display.c
+++ b/hw/display/bochs-display.c
@@ -192,9 +192,13 @@ nofb:
 static void bochs_display_update(void *opaque)
 {
     BochsDisplayState *s = opaque;
+    DirtyBitmapSnapshot *snap = NULL;
+    bool full_update = false;
     BochsDisplayMode mode;
     DisplaySurface *ds;
     uint8_t *ptr;
+    bool dirty;
+    int y, ys;
 
     bochs_display_get_mode(s, &mode);
     if (!mode.size) {
@@ -212,9 +216,34 @@ static void bochs_display_update(void *opaque)
                                              mode.stride,
                                              ptr + mode.offset);
         dpy_gfx_replace_surface(s->con, ds);
+        full_update = true;
     }
 
-    dpy_gfx_update_full(s->con);
+    if (full_update) {
+        dpy_gfx_update_full(s->con);
+    } else {
+        snap = memory_region_snapshot_and_clear_dirty(&s->vram,
+                                                      mode.offset, mode.size,
+                                                      DIRTY_MEMORY_VGA);
+        ys = -1;
+        for (y = 0; y < mode.height; y++) {
+            dirty = memory_region_snapshot_get_dirty(&s->vram, snap,
+                                                     mode.offset + mode.stride * y,
+                                                     mode.stride);
+            if (dirty && ys < 0) {
+                ys = y;
+            }
+            if (!dirty && ys >= 0) {
+                dpy_gfx_update(s->con, 0, ys,
+                               mode.width, y - ys);
+                ys = -1;
+            }
+        }
+        if (ys >= 0) {
+            dpy_gfx_update(s->con, 0, ys,
+                           mode.width, y - ys);
+        }
+    }
 }
 
 static const GraphicHwOps bochs_display_gfx_ops = {
@@ -250,6 +279,8 @@ static void bochs_display_realize(PCIDevice *dev, Error **errp)
     pci_set_byte(&s->pci.config[PCI_REVISION_ID], 2);
     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);
+
+    memory_region_set_log(&s->vram, true, DIRTY_MEMORY_VGA);
 }
 
 static bool bochs_display_get_big_endian_fb(Object *obj, Error **errp)
-- 
2.9.3

[Qemu-devel] [PATCH 4/4] bochs-display: add pcie support

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/bochs-display.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/display/bochs-display.c b/hw/display/bochs-display.c
index 9ea6f798f4..23dbf57b8f 100644
--- a/hw/display/bochs-display.c
+++ b/hw/display/bochs-display.c
@@ -254,6 +254,7 @@ static void bochs_display_realize(PCIDevice *dev, Error **errp)
 {
     BochsDisplayState *s = BOCHS_DISPLAY(dev);
     Object *obj = OBJECT(dev);
+    int ret;
 
     s->con = graphic_console_init(DEVICE(dev), 0, &bochs_display_gfx_ops, s);
 
@@ -280,6 +281,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 (pci_bus_is_express(pci_get_bus(dev))) {
+        dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
+        ret = pcie_endpoint_cap_init(dev, 0x80);
+        assert(ret > 0);
+    }
+
     memory_region_set_log(&s->vram, true, DIRTY_MEMORY_VGA);
 }
 
@@ -341,6 +348,7 @@ static const TypeInfo bochs_display_type_info = {
     .instance_init  = bochs_display_init,
     .class_init     = bochs_display_class_init,
     .interfaces     = (InterfaceInfo[]) {
+        { INTERFACE_PCIE_DEVICE },
         { INTERFACE_CONVENTIONAL_PCI_DEVICE },
         { },
     },
-- 
2.9.3

Re: [Qemu-devel] [PATCH 0/4] display: add new bochs-display device

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180517092513.735-1-kraxel@redhat.com
Subject: [Qemu-devel] [PATCH 0/4] display: add new bochs-display device

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/20180517092513.735-1-kraxel@redhat.com -> patchew/20180517092513.735-1-kraxel@redhat.com
Switched to a new branch 'test'
110b775813 bochs-display: add pcie support
1e7d025590 bochs-display: add dirty tracking support
ab5e478d06 display: add new bochs-display device
149c738fb9 vga: move bochs vbe defines to header file

=== OUTPUT BEGIN ===
Checking PATCH 1/4: vga: move bochs vbe defines to header file...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#86: 
new file mode 100644

total: 0 errors, 1 warnings, 127 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 2/4: display: add new bochs-display device...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#43: 
new file mode 100644

ERROR: braces {} are necessary for all arms of this statement
#199: FILE: hw/display/bochs-display.c:152:
+    if (!(vbe[VBE_DISPI_INDEX_ENABLE] & VBE_DISPI_ENABLED))
[...]

WARNING: line over 80 characters
#209: FILE: hw/display/bochs-display.c:162:
+        mode->format = s->big_endian_fb ? PIXMAN_BE_x8r8g8b8 : PIXMAN_LE_x8r8g8b8;

ERROR: braces {} are necessary for all arms of this statement
#219: FILE: hw/display/bochs-display.c:172:
+    if (virt_width < mode->width)
[...]

WARNING: line over 80 characters
#309: FILE: hw/display/bochs-display.c:262:
+static void bochs_display_set_big_endian_fb(Object *obj, bool value, Error **errp)

total: 2 errors, 3 warnings, 330 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 3/4: bochs-display: add dirty tracking support...
WARNING: line over 80 characters
#44: FILE: hw/display/bochs-display.c:231:
+                                                     mode.offset + mode.stride * y,

total: 0 errors, 1 warnings, 56 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 4/4: bochs-display: add pcie support...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH 1/4] vga: move bochs vbe defines to header file

[Philippe Mathieu-Daudé]

On 05/17/2018 06:25 AM, Gerd Hoffmann wrote:
> Create a new header file, move the bochs vbe dispi interface
> defines to it, so they can be used outside vga code.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/display/vga_int.h           | 35 ++---------------------
>  include/hw/display/bochs-vbe.h | 64 ++++++++++++++++++++++++++++++++++++++++++
>  hw/display/vga-pci.c           | 13 ---------

Adding those file in your Graphics MAINTAINERS entry:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>  3 files changed, 66 insertions(+), 46 deletions(-)
>  create mode 100644 include/hw/display/bochs-vbe.h
> 
> diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
> index fe23b81442..313cff84fc 100644
> --- a/hw/display/vga_int.h
> +++ b/hw/display/vga_int.h
> @@ -29,42 +29,11 @@
>  #include "exec/memory.h"
>  #include "ui/console.h"
>  
> +#include "hw/display/bochs-vbe.h"
> +
>  #define ST01_V_RETRACE      0x08
>  #define ST01_DISP_ENABLE    0x01
>  
> -#define VBE_DISPI_MAX_XRES              16000
> -#define VBE_DISPI_MAX_YRES              12000
> -#define VBE_DISPI_MAX_BPP               32
> -
> -#define VBE_DISPI_INDEX_ID              0x0
> -#define VBE_DISPI_INDEX_XRES            0x1
> -#define VBE_DISPI_INDEX_YRES            0x2
> -#define VBE_DISPI_INDEX_BPP             0x3
> -#define VBE_DISPI_INDEX_ENABLE          0x4
> -#define VBE_DISPI_INDEX_BANK            0x5
> -#define VBE_DISPI_INDEX_VIRT_WIDTH      0x6
> -#define VBE_DISPI_INDEX_VIRT_HEIGHT     0x7
> -#define VBE_DISPI_INDEX_X_OFFSET        0x8
> -#define VBE_DISPI_INDEX_Y_OFFSET        0x9
> -#define VBE_DISPI_INDEX_NB              0xa /* size of vbe_regs[] */
> -#define VBE_DISPI_INDEX_VIDEO_MEMORY_64K 0xa /* read-only, not in vbe_regs */
> -
> -#define VBE_DISPI_ID0                   0xB0C0
> -#define VBE_DISPI_ID1                   0xB0C1
> -#define VBE_DISPI_ID2                   0xB0C2
> -#define VBE_DISPI_ID3                   0xB0C3
> -#define VBE_DISPI_ID4                   0xB0C4
> -#define VBE_DISPI_ID5                   0xB0C5
> -
> -#define VBE_DISPI_DISABLED              0x00
> -#define VBE_DISPI_ENABLED               0x01
> -#define VBE_DISPI_GETCAPS               0x02
> -#define VBE_DISPI_8BIT_DAC              0x20
> -#define VBE_DISPI_LFB_ENABLED           0x40
> -#define VBE_DISPI_NOCLEARMEM            0x80
> -
> -#define VBE_DISPI_LFB_PHYSICAL_ADDRESS  0xE0000000
> -
>  #define CH_ATTR_SIZE (160 * 100)
>  #define VGA_MAX_HEIGHT 2048
>  
> diff --git a/include/hw/display/bochs-vbe.h b/include/hw/display/bochs-vbe.h
> new file mode 100644
> index 0000000000..6f27ed4a91
> --- /dev/null
> +++ b/include/hw/display/bochs-vbe.h
> @@ -0,0 +1,64 @@
> +/*
> + * bochs vesa bios extension interface
> + */
> +
> +#define VBE_DISPI_MAX_XRES              16000
> +#define VBE_DISPI_MAX_YRES              12000
> +#define VBE_DISPI_MAX_BPP               32
> +
> +#define VBE_DISPI_INDEX_ID              0x0
> +#define VBE_DISPI_INDEX_XRES            0x1
> +#define VBE_DISPI_INDEX_YRES            0x2
> +#define VBE_DISPI_INDEX_BPP             0x3
> +#define VBE_DISPI_INDEX_ENABLE          0x4
> +#define VBE_DISPI_INDEX_BANK            0x5
> +#define VBE_DISPI_INDEX_VIRT_WIDTH      0x6
> +#define VBE_DISPI_INDEX_VIRT_HEIGHT     0x7
> +#define VBE_DISPI_INDEX_X_OFFSET        0x8
> +#define VBE_DISPI_INDEX_Y_OFFSET        0x9
> +#define VBE_DISPI_INDEX_NB              0xa /* size of vbe_regs[] */
> +#define VBE_DISPI_INDEX_VIDEO_MEMORY_64K 0xa /* read-only, not in vbe_regs */
> +
> +/* VBE_DISPI_INDEX_ID */
> +#define VBE_DISPI_ID0                   0xB0C0
> +#define VBE_DISPI_ID1                   0xB0C1
> +#define VBE_DISPI_ID2                   0xB0C2
> +#define VBE_DISPI_ID3                   0xB0C3
> +#define VBE_DISPI_ID4                   0xB0C4
> +#define VBE_DISPI_ID5                   0xB0C5
> +
> +/* VBE_DISPI_INDEX_ENABLE */
> +#define VBE_DISPI_DISABLED              0x00
> +#define VBE_DISPI_ENABLED               0x01
> +#define VBE_DISPI_GETCAPS               0x02
> +#define VBE_DISPI_8BIT_DAC              0x20
> +#define VBE_DISPI_LFB_ENABLED           0x40
> +#define VBE_DISPI_NOCLEARMEM            0x80
> +
> +/* only used by isa-vga, pci vga devices use a memory bar */
> +#define VBE_DISPI_LFB_PHYSICAL_ADDRESS  0xE0000000
> +
> +
> +/*
> + * qemu extension: mmio bar (region 2)
> + */
> +
> +#define PCI_VGA_MMIO_SIZE     0x1000
> +
> +/* vga register region */
> +#define PCI_VGA_IOPORT_OFFSET 0x400
> +#define PCI_VGA_IOPORT_SIZE   (0x3e0 - 0x3c0)
> +
> +/* bochs vbe register region */
> +#define PCI_VGA_BOCHS_OFFSET  0x500
> +#define PCI_VGA_BOCHS_SIZE    (0x0b * 2)
> +
> +/* qemu extension register region */
> +#define PCI_VGA_QEXT_OFFSET   0x600
> +#define PCI_VGA_QEXT_SIZE     (2 * 4)
> +
> +/* qemu extension registers */
> +#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
> diff --git a/hw/display/vga-pci.c b/hw/display/vga-pci.c
> index f312930664..fb3e4cd400 100644
> --- a/hw/display/vga-pci.c
> +++ b/hw/display/vga-pci.c
> @@ -31,19 +31,6 @@
>  #include "qemu/timer.h"
>  #include "hw/loader.h"
>  
> -#define PCI_VGA_IOPORT_OFFSET 0x400
> -#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,
> 

Re: [Qemu-devel] [PATCH 1/4] vga: move bochs vbe defines to header file

[Gerd Hoffmann]

On Thu, May 17, 2018 at 12:20:58PM -0300, Philippe Mathieu-Daudé wrote:
> On 05/17/2018 06:25 AM, Gerd Hoffmann wrote:
> > Create a new header file, move the bochs vbe dispi interface
> > defines to it, so they can be used outside vga code.
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  hw/display/vga_int.h           | 35 ++---------------------
> >  include/hw/display/bochs-vbe.h | 64 ++++++++++++++++++++++++++++++++++++++++++
> >  hw/display/vga-pci.c           | 13 ---------
> 
> Adding those file in your Graphics MAINTAINERS entry:

"Graphics" is for ui/.

There isn't one for hw/display/*vga* in the devices section though,
guess I should add one ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/4] vga: move bochs vbe defines to header file

[Marc-André Lureau]

Hi

On Thu, May 17, 2018 at 11:25 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> Create a new header file, move the bochs vbe dispi interface
> defines to it, so they can be used outside vga code.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/display/vga_int.h           | 35 ++---------------------
>  include/hw/display/bochs-vbe.h | 64 ++++++++++++++++++++++++++++++++++++++++++
>  hw/display/vga-pci.c           | 13 ---------
>  3 files changed, 66 insertions(+), 46 deletions(-)
>  create mode 100644 include/hw/display/bochs-vbe.h
>
> diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
> index fe23b81442..313cff84fc 100644
> --- a/hw/display/vga_int.h
> +++ b/hw/display/vga_int.h
> @@ -29,42 +29,11 @@
>  #include "exec/memory.h"
>  #include "ui/console.h"
>
> +#include "hw/display/bochs-vbe.h"
> +
>  #define ST01_V_RETRACE      0x08
>  #define ST01_DISP_ENABLE    0x01
>
> -#define VBE_DISPI_MAX_XRES              16000
> -#define VBE_DISPI_MAX_YRES              12000
> -#define VBE_DISPI_MAX_BPP               32
> -
> -#define VBE_DISPI_INDEX_ID              0x0
> -#define VBE_DISPI_INDEX_XRES            0x1
> -#define VBE_DISPI_INDEX_YRES            0x2
> -#define VBE_DISPI_INDEX_BPP             0x3
> -#define VBE_DISPI_INDEX_ENABLE          0x4
> -#define VBE_DISPI_INDEX_BANK            0x5
> -#define VBE_DISPI_INDEX_VIRT_WIDTH      0x6
> -#define VBE_DISPI_INDEX_VIRT_HEIGHT     0x7
> -#define VBE_DISPI_INDEX_X_OFFSET        0x8
> -#define VBE_DISPI_INDEX_Y_OFFSET        0x9
> -#define VBE_DISPI_INDEX_NB              0xa /* size of vbe_regs[] */
> -#define VBE_DISPI_INDEX_VIDEO_MEMORY_64K 0xa /* read-only, not in vbe_regs */
> -
> -#define VBE_DISPI_ID0                   0xB0C0
> -#define VBE_DISPI_ID1                   0xB0C1
> -#define VBE_DISPI_ID2                   0xB0C2
> -#define VBE_DISPI_ID3                   0xB0C3
> -#define VBE_DISPI_ID4                   0xB0C4
> -#define VBE_DISPI_ID5                   0xB0C5
> -
> -#define VBE_DISPI_DISABLED              0x00
> -#define VBE_DISPI_ENABLED               0x01
> -#define VBE_DISPI_GETCAPS               0x02
> -#define VBE_DISPI_8BIT_DAC              0x20
> -#define VBE_DISPI_LFB_ENABLED           0x40
> -#define VBE_DISPI_NOCLEARMEM            0x80
> -
> -#define VBE_DISPI_LFB_PHYSICAL_ADDRESS  0xE0000000
> -
>  #define CH_ATTR_SIZE (160 * 100)
>  #define VGA_MAX_HEIGHT 2048
>
> diff --git a/include/hw/display/bochs-vbe.h b/include/hw/display/bochs-vbe.h
> new file mode 100644
> index 0000000000..6f27ed4a91
> --- /dev/null
> +++ b/include/hw/display/bochs-vbe.h
> @@ -0,0 +1,64 @@

No header guards & copyright notice?

> +/*
> + * bochs vesa bios extension interface
> + */
> +
> +#define VBE_DISPI_MAX_XRES              16000
> +#define VBE_DISPI_MAX_YRES              12000
> +#define VBE_DISPI_MAX_BPP               32
> +
> +#define VBE_DISPI_INDEX_ID              0x0
> +#define VBE_DISPI_INDEX_XRES            0x1
> +#define VBE_DISPI_INDEX_YRES            0x2
> +#define VBE_DISPI_INDEX_BPP             0x3
> +#define VBE_DISPI_INDEX_ENABLE          0x4
> +#define VBE_DISPI_INDEX_BANK            0x5
> +#define VBE_DISPI_INDEX_VIRT_WIDTH      0x6
> +#define VBE_DISPI_INDEX_VIRT_HEIGHT     0x7
> +#define VBE_DISPI_INDEX_X_OFFSET        0x8
> +#define VBE_DISPI_INDEX_Y_OFFSET        0x9
> +#define VBE_DISPI_INDEX_NB              0xa /* size of vbe_regs[] */
> +#define VBE_DISPI_INDEX_VIDEO_MEMORY_64K 0xa /* read-only, not in vbe_regs */
> +
> +/* VBE_DISPI_INDEX_ID */
> +#define VBE_DISPI_ID0                   0xB0C0
> +#define VBE_DISPI_ID1                   0xB0C1
> +#define VBE_DISPI_ID2                   0xB0C2
> +#define VBE_DISPI_ID3                   0xB0C3
> +#define VBE_DISPI_ID4                   0xB0C4
> +#define VBE_DISPI_ID5                   0xB0C5
> +
> +/* VBE_DISPI_INDEX_ENABLE */
> +#define VBE_DISPI_DISABLED              0x00
> +#define VBE_DISPI_ENABLED               0x01
> +#define VBE_DISPI_GETCAPS               0x02
> +#define VBE_DISPI_8BIT_DAC              0x20
> +#define VBE_DISPI_LFB_ENABLED           0x40
> +#define VBE_DISPI_NOCLEARMEM            0x80
> +
> +/* only used by isa-vga, pci vga devices use a memory bar */
> +#define VBE_DISPI_LFB_PHYSICAL_ADDRESS  0xE0000000
> +
> +
> +/*
> + * qemu extension: mmio bar (region 2)
> + */
> +
> +#define PCI_VGA_MMIO_SIZE     0x1000
> +

This one seems unused, you could remove it

> +/* vga register region */
> +#define PCI_VGA_IOPORT_OFFSET 0x400
> +#define PCI_VGA_IOPORT_SIZE   (0x3e0 - 0x3c0)
> +
> +/* bochs vbe register region */
> +#define PCI_VGA_BOCHS_OFFSET  0x500
> +#define PCI_VGA_BOCHS_SIZE    (0x0b * 2)
> +
> +/* qemu extension register region */
> +#define PCI_VGA_QEXT_OFFSET   0x600
> +#define PCI_VGA_QEXT_SIZE     (2 * 4)
> +
> +/* qemu extension registers */
> +#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
> diff --git a/hw/display/vga-pci.c b/hw/display/vga-pci.c
> index f312930664..fb3e4cd400 100644
> --- a/hw/display/vga-pci.c
> +++ b/hw/display/vga-pci.c
> @@ -31,19 +31,6 @@
>  #include "qemu/timer.h"
>  #include "hw/loader.h"
>
> -#define PCI_VGA_IOPORT_OFFSET 0x400
> -#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,
> --
> 2.9.3
>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 2/4] display: add new bochs-display device

[Marc-André Lureau]

Hi

On Thu, May 17, 2018 at 11:25 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> After writing up the virtual mdev device emulating a display supporting
> the bochs vbe dispi interface (mbochs.ko) and seeing how simple it
> actually is I've figured that would be useful for qemu too.
>
> So, here it is, -device bochs-display.  It is basically -device VGA
> without legacy vga emulation.  PCI bar 0 is the framebuffer, PCI bar 2
> is mmio with the registers.  The vga registers are simply not there
> though, neither in the legacy ioport location nor in the mmio bar.
> Consequently it is PCI class DISPLAY_OTHER not DISPLAY_VGA.
>
> So there is no text mode emulation, no weird video modes (planar,
> 256color palette), no memory window at 0xa0000.  Just a linear
> framebuffer in the pci memory bar.  And the amount of code to emulate
> this (and therefore the attack surface) is an order of magnitude smaller
> when compared to vga emulation.
>
> Compatibility wise it almost works with OVMF (little tweak needed).
> The bochs-drm.ko linux kernel module can handle it just fine too.
> So once the OVMF fix is merged UEFI guests should not see any
> functional difference to VGA.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

I did some basic testing with a linux guest, and migration, minor code
comments below

Tested-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  hw/display/bochs-display.c | 323 +++++++++++++++++++++++++++++++++++++++++++++
>  hw/display/Makefile.objs   |   1 +
>  2 files changed, 324 insertions(+)
>  create mode 100644 hw/display/bochs-display.c
>
> diff --git a/hw/display/bochs-display.c b/hw/display/bochs-display.c
> new file mode 100644
> index 0000000000..beeda58475
> --- /dev/null
> +++ b/hw/display/bochs-display.c
> @@ -0,0 +1,323 @@
> +/*
> + * QEMU PCI bochs display adapter.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#include "qemu/osdep.h"
> +#include "hw/hw.h"
> +#include "hw/pci/pci.h"
> +#include "hw/display/bochs-vbe.h"
> +
> +#include "qapi/error.h"
> +
> +#include "ui/console.h"
> +#include "ui/qemu-pixman.h"
> +
> +typedef struct BochsDisplayMode {
> +    pixman_format_code_t format;
> +    uint32_t             bytepp;
> +    uint32_t             width;
> +    uint32_t             height;
> +    uint32_t             stride;
> +    uint32_t             __pad;

out of curiosity, is the __pad necessary? add a comment?

> +    uint64_t             offset;
> +    uint64_t             size;
> +} BochsDisplayMode;
> +
> +typedef struct BochsDisplayState {
> +    /* parent */
> +    PCIDevice        pci;
> +
> +    /* device elements */
> +    QemuConsole      *con;
> +    MemoryRegion     vram;
> +    MemoryRegion     mmio;
> +    MemoryRegion     vbe;
> +    MemoryRegion     qext;
> +
> +    /* device config */
> +    uint64_t         vgamem;
> +
> +    /* device registers */
> +    uint16_t         vbe_regs[VBE_DISPI_INDEX_NB];
> +    bool             big_endian_fb;
> +
> +    /* device state */
> +    BochsDisplayMode mode;
> +} BochsDisplayState;
> +
> +#define TYPE_BOCHS_DISPLAY "bochs-display"
> +#define BOCHS_DISPLAY(obj) OBJECT_CHECK(BochsDisplayState, (obj), \
> +                                        TYPE_BOCHS_DISPLAY)
> +
> +static const VMStateDescription vmstate_bochs_display = {
> +    .name = "bochs-display",
> +    .fields = (VMStateField[]) {
> +        VMSTATE_PCI_DEVICE(pci, BochsDisplayState),
> +        VMSTATE_UINT16_ARRAY(vbe_regs, BochsDisplayState, VBE_DISPI_INDEX_NB),
> +        VMSTATE_BOOL(big_endian_fb, BochsDisplayState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static uint64_t bochs_display_vbe_read(void *ptr, hwaddr addr,
> +                                       unsigned size)
> +{
> +    BochsDisplayState *s = ptr;
> +    unsigned int index = addr >> 1;
> +
> +    switch (index) {
> +    case VBE_DISPI_INDEX_ID:
> +        return VBE_DISPI_ID5;
> +    case VBE_DISPI_INDEX_VIDEO_MEMORY_64K:
> +        return s->vgamem / (64 * 1024);
> +    }
> +
> +    if (index >= ARRAY_SIZE(s->vbe_regs)) {
> +        return -1;
> +    }
> +    return s->vbe_regs[index];
> +}
> +
> +static void bochs_display_vbe_write(void *ptr, hwaddr addr,
> +                                    uint64_t val, unsigned size)
> +{
> +    BochsDisplayState *s = ptr;
> +    unsigned int index = addr >> 1;
> +
> +    if (index >= ARRAY_SIZE(s->vbe_regs)) {
> +        return;
> +    }
> +    s->vbe_regs[index] = val;
> +}
> +
> +static const MemoryRegionOps bochs_display_vbe_ops = {
> +    .read = bochs_display_vbe_read,
> +    .write = bochs_display_vbe_write,
> +    .valid.min_access_size = 1,
> +    .valid.max_access_size = 4,
> +    .impl.min_access_size = 2,
> +    .impl.max_access_size = 2,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static uint64_t bochs_display_qext_read(void *ptr, hwaddr addr,
> +                                        unsigned size)
> +{
> +    BochsDisplayState *s = ptr;
> +
> +    switch (addr) {
> +    case PCI_VGA_QEXT_REG_SIZE:
> +        return PCI_VGA_QEXT_SIZE;
> +    case PCI_VGA_QEXT_REG_BYTEORDER:
> +        return s->big_endian_fb ?
> +            PCI_VGA_QEXT_BIG_ENDIAN : PCI_VGA_QEXT_LITTLE_ENDIAN;
> +    default:
> +        return 0;
> +    }
> +}
> +
> +static void bochs_display_qext_write(void *ptr, hwaddr addr,
> +                                     uint64_t val, unsigned size)
> +{
> +    BochsDisplayState *s = ptr;
> +
> +    switch (addr) {
> +    case PCI_VGA_QEXT_REG_BYTEORDER:
> +        if (val == PCI_VGA_QEXT_BIG_ENDIAN) {
> +            s->big_endian_fb = true;
> +        }
> +        if (val == PCI_VGA_QEXT_LITTLE_ENDIAN) {
> +            s->big_endian_fb = false;
> +        }
> +        break;
> +    }
> +}
> +
> +static const MemoryRegionOps bochs_display_qext_ops = {
> +    .read = bochs_display_qext_read,
> +    .write = bochs_display_qext_write,
> +    .valid.min_access_size = 4,
> +    .valid.max_access_size = 4,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static void bochs_display_get_mode(BochsDisplayState *s,
> +                                   BochsDisplayMode *mode)
> +{
> +    uint16_t *vbe = s->vbe_regs;
> +    uint32_t virt_width;
> +
> +    if (!(vbe[VBE_DISPI_INDEX_ENABLE] & VBE_DISPI_ENABLED))
> +        goto nofb;
> +
> +    memset(mode, 0, sizeof(*mode));
> +    switch (vbe[VBE_DISPI_INDEX_BPP]) {
> +    case 16:
> +        /* best effort: support native endianess only */
> +        mode->format = PIXMAN_r5g6b5;
> +        mode->bytepp = 2;
> +    case 32:
> +        mode->format = s->big_endian_fb ? PIXMAN_BE_x8r8g8b8 : PIXMAN_LE_x8r8g8b8;
> +        mode->bytepp = 4;
> +        break;
> +    default:
> +        goto nofb;
> +    }
> +
> +    mode->width  = vbe[VBE_DISPI_INDEX_XRES];
> +    mode->height = vbe[VBE_DISPI_INDEX_YRES];
> +    virt_width  = vbe[VBE_DISPI_INDEX_VIRT_WIDTH];
> +    if (virt_width < mode->width)
> +        virt_width = mode->width;
> +    mode->stride = virt_width * mode->bytepp;
> +    mode->size   = (uint64_t)mode->stride * mode->height;
> +    mode->offset = ((uint64_t)vbe[VBE_DISPI_INDEX_X_OFFSET] * mode->bytepp +
> +                    (uint64_t)vbe[VBE_DISPI_INDEX_Y_OFFSET] * mode->stride);
> +
> +    if (mode->width < 64 || mode->height < 64) {
> +        goto nofb;
> +    }
> +    if (mode->offset + mode->size > s->vgamem) {
> +        goto nofb;
> +    }
> +    return;
> +
> +nofb:
> +    memset(mode, 0, sizeof(*mode));
> +    return;

Or return an error, this would be a bit more explicit than checking
for mode.size? And you wouldn't have to memset() here.

> +}
> +
> +static void bochs_display_update(void *opaque)
> +{
> +    BochsDisplayState *s = opaque;
> +    BochsDisplayMode mode;
> +    DisplaySurface *ds;
> +    uint8_t *ptr;
> +
> +    bochs_display_get_mode(s, &mode);
> +    if (!mode.size) {
> +        /* no (valid) video mode */
> +        return;
> +    }
> +
> +    if (memcmp(&s->mode, &mode, sizeof(mode)) != 0) {
> +        /* video mode switch */
> +        s->mode = mode;
> +        ptr = memory_region_get_ram_ptr(&s->vram);
> +        ds = qemu_create_displaysurface_from(mode.width,
> +                                             mode.height,
> +                                             mode.format,
> +                                             mode.stride,
> +                                             ptr + mode.offset);
> +        dpy_gfx_replace_surface(s->con, ds);
> +    }
> +
> +    dpy_gfx_update_full(s->con);
> +}
> +
> +static const GraphicHwOps bochs_display_gfx_ops = {
> +    .gfx_update = bochs_display_update,
> +};
> +
> +static void bochs_display_realize(PCIDevice *dev, Error **errp)
> +{
> +    BochsDisplayState *s = BOCHS_DISPLAY(dev);
> +    Object *obj = OBJECT(dev);
> +
> +    s->con = graphic_console_init(DEVICE(dev), 0, &bochs_display_gfx_ops, s);
> +
> +    if (s->vgamem < (4 * 1024 * 1024)) {
> +        error_setg(errp, "bochs-display: video memory too small");
> +    }
> +    if (s->vgamem > (256 * 1024 * 1024)) {
> +        error_setg(errp, "bochs-display: video memory too big");
> +    }
> +    s->vgamem = pow2ceil(s->vgamem);
> +
> +    memory_region_init_ram(&s->vram, obj, "bochs-display-vram", s->vgamem,
> +                           &error_fatal);
> +    memory_region_init_io(&s->vbe, obj, &bochs_display_vbe_ops, s,
> +                          "bochs dispi interface", PCI_VGA_BOCHS_SIZE);
> +    memory_region_init_io(&s->qext, obj, &bochs_display_qext_ops, s,
> +                          "qemu extended regs", PCI_VGA_QEXT_SIZE);
> +
> +    memory_region_init(&s->mmio, obj, "bochs-display-mmio", 4096);
> +    memory_region_add_subregion(&s->mmio, PCI_VGA_BOCHS_OFFSET, &s->vbe);
> +    memory_region_add_subregion(&s->mmio, PCI_VGA_QEXT_OFFSET, &s->qext);
> +
> +    pci_set_byte(&s->pci.config[PCI_REVISION_ID], 2);
> +    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);
> +}
> +
> +static bool bochs_display_get_big_endian_fb(Object *obj, Error **errp)
> +{
> +    BochsDisplayState *s = BOCHS_DISPLAY(obj);
> +
> +    return s->big_endian_fb;
> +}
> +
> +static void bochs_display_set_big_endian_fb(Object *obj, bool value, Error **errp)
> +{
> +    BochsDisplayState *s = BOCHS_DISPLAY(obj);
> +
> +    s->big_endian_fb = value;
> +}
> +
> +static void bochs_display_init(Object *obj)
> +{
> +    /* Expose framebuffer byteorder via QOM */
> +    object_property_add_bool(obj, "big-endian-framebuffer",
> +                             bochs_display_get_big_endian_fb,
> +                             bochs_display_set_big_endian_fb,
> +                             NULL);
> +}
> +
> +static void bochs_display_exit(PCIDevice *dev)
> +{
> +    BochsDisplayState *s = BOCHS_DISPLAY(dev);
> +
> +    graphic_console_close(s->con);
> +}
> +
> +static Property bochs_display_properties[] = {
> +    DEFINE_PROP_SIZE("vgamem", BochsDisplayState, vgamem, 16 * 1024 * 1024),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void bochs_display_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +
> +    k->class_id  = PCI_CLASS_DISPLAY_OTHER;
> +    k->vendor_id = PCI_VENDOR_ID_QEMU;
> +    k->device_id = PCI_DEVICE_ID_QEMU_VGA;
> +
> +    k->realize   = bochs_display_realize;
> +    k->exit      = bochs_display_exit;
> +    dc->vmsd     = &vmstate_bochs_display;
> +    dc->props    = bochs_display_properties;
> +    set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
> +}
> +
> +static const TypeInfo bochs_display_type_info = {
> +    .name           = TYPE_BOCHS_DISPLAY,
> +    .parent         = TYPE_PCI_DEVICE,
> +    .instance_size  = sizeof(BochsDisplayState),
> +    .instance_init  = bochs_display_init,
> +    .class_init     = bochs_display_class_init,
> +    .interfaces     = (InterfaceInfo[]) {
> +        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> +        { },
> +    },
> +};
> +
> +static void bochs_display_register_types(void)
> +{
> +    type_register_static(&bochs_display_type_info);
> +}
> +
> +type_init(bochs_display_register_types)
> diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
> index 3c7c75b94d..1181c4a72b 100644
> --- a/hw/display/Makefile.objs
> +++ b/hw/display/Makefile.objs
> @@ -9,6 +9,7 @@ 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_ISA) += vga-isa.o
>  common-obj-$(CONFIG_VGA_ISA_MM) += vga-isa-mm.o
>  common-obj-$(CONFIG_VMWARE_VGA) += vmware_vga.o
> --
> 2.9.3
>
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 3/4] bochs-display: add dirty tracking support

[Marc-André Lureau]

Hi

On Thu, May 17, 2018 at 11:25 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  hw/display/bochs-display.c | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/hw/display/bochs-display.c b/hw/display/bochs-display.c
> index beeda58475..9ea6f798f4 100644
> --- a/hw/display/bochs-display.c
> +++ b/hw/display/bochs-display.c
> @@ -192,9 +192,13 @@ nofb:
>  static void bochs_display_update(void *opaque)
>  {
>      BochsDisplayState *s = opaque;
> +    DirtyBitmapSnapshot *snap = NULL;
> +    bool full_update = false;
>      BochsDisplayMode mode;
>      DisplaySurface *ds;
>      uint8_t *ptr;
> +    bool dirty;
> +    int y, ys;
>
>      bochs_display_get_mode(s, &mode);
>      if (!mode.size) {
> @@ -212,9 +216,34 @@ static void bochs_display_update(void *opaque)
>                                               mode.stride,
>                                               ptr + mode.offset);
>          dpy_gfx_replace_surface(s->con, ds);
> +        full_update = true;
>      }
>
> -    dpy_gfx_update_full(s->con);
> +    if (full_update) {
> +        dpy_gfx_update_full(s->con);
> +    } else {
> +        snap = memory_region_snapshot_and_clear_dirty(&s->vram,
> +                                                      mode.offset, mode.size,
> +                                                      DIRTY_MEMORY_VGA);
> +        ys = -1;
> +        for (y = 0; y < mode.height; y++) {
> +            dirty = memory_region_snapshot_get_dirty(&s->vram, snap,
> +                                                     mode.offset + mode.stride * y,
> +                                                     mode.stride);
> +            if (dirty && ys < 0) {
> +                ys = y;
> +            }
> +            if (!dirty && ys >= 0) {
> +                dpy_gfx_update(s->con, 0, ys,
> +                               mode.width, y - ys);
> +                ys = -1;
> +            }
> +        }
> +        if (ys >= 0) {
> +            dpy_gfx_update(s->con, 0, ys,
> +                           mode.width, y - ys);
> +        }
> +    }
>  }
>
>  static const GraphicHwOps bochs_display_gfx_ops = {
> @@ -250,6 +279,8 @@ static void bochs_display_realize(PCIDevice *dev, Error **errp)
>      pci_set_byte(&s->pci.config[PCI_REVISION_ID], 2);
>      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);
> +
> +    memory_region_set_log(&s->vram, true, DIRTY_MEMORY_VGA);
>  }
>
>  static bool bochs_display_get_big_endian_fb(Object *obj, Error **errp)
> --
> 2.9.3
>
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 4/4] bochs-display: add pcie support

[Marc-André Lureau]

Hi

On Thu, May 17, 2018 at 11:25 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---

Could you explain where the 0x80 offset comes from?


>  hw/display/bochs-display.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/hw/display/bochs-display.c b/hw/display/bochs-display.c
> index 9ea6f798f4..23dbf57b8f 100644
> --- a/hw/display/bochs-display.c
> +++ b/hw/display/bochs-display.c
> @@ -254,6 +254,7 @@ static void bochs_display_realize(PCIDevice *dev, Error **errp)
>  {
>      BochsDisplayState *s = BOCHS_DISPLAY(dev);
>      Object *obj = OBJECT(dev);
> +    int ret;
>
>      s->con = graphic_console_init(DEVICE(dev), 0, &bochs_display_gfx_ops, s);
>
> @@ -280,6 +281,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 (pci_bus_is_express(pci_get_bus(dev))) {
> +        dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
> +        ret = pcie_endpoint_cap_init(dev, 0x80);
> +        assert(ret > 0);
> +    }
> +
>      memory_region_set_log(&s->vram, true, DIRTY_MEMORY_VGA);
>  }
>
> @@ -341,6 +348,7 @@ static const TypeInfo bochs_display_type_info = {
>      .instance_init  = bochs_display_init,
>      .class_init     = bochs_display_class_init,
>      .interfaces     = (InterfaceInfo[]) {
> +        { INTERFACE_PCIE_DEVICE },
>          { INTERFACE_CONVENTIONAL_PCI_DEVICE },
>          { },
>      },
> --
> 2.9.3
>
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 1/4] vga: move bochs vbe defines to header file

[Gerd Hoffmann]

> > --- /dev/null
> > +++ b/include/hw/display/bochs-vbe.h
> > @@ -0,0 +1,64 @@
> 
> No header guards & copyright notice?

Header guards ok.

Copyright?
I don't think interface #defines like this are copyrightable ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 4/4] bochs-display: add pcie support

[Gerd Hoffmann]

On Fri, May 18, 2018 at 05:13:27PM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Thu, May 17, 2018 at 11:25 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> 
> Could you explain where the 0x80 offset comes from?

Pulled out of thin air.  Standard pci cfg space header size is 0x40, so
it must be between 0x40 and 0xff - sizeof(capability).  And it must not
overlap with other pci(e) capabilities (easy as this is the only one).
That are the only constrains I'm aware of.

cheers,
  Gerd