[Qemu-devel] [PATCH v2 0/6] vga: dirty log cleanup, more linear mapping

[Qemu-devel] [PATCH v2 0/6] vga: dirty log cleanup, more linear mapping

[Jan Kiszka]

This is a rebased, memory region-aware version of the original series I
sent a few weeks back. It no longer eliminates log_start/stop
CPUPhysMemoryClient callbacks, but it still
 - accelerates chain 4 vga mode under KVM
 - fixes reset of vmware-vga
 - cleans up vmware-vga a bit

Patch 1 is still a candidate for stable.

CC: Andrzej Zaborowski <balrogg@gmail.com>
CC: Avi Kivity <avi@redhat.com>
CC: Gerd Hoffmann <kraxel@redhat.com>

Jan Kiszka (6):
  vmware-vga: Register reset service
  vmware-vga: Disable verbose mode
  vmware-vga: Remove dead DIRECT_VRAM mode
  vmware-vga: Eliminate vga_dirty_log_restart
  vga: Use linear mapping + dirty logging in chain 4 memory access mode
  vga: Drop some unused fields

 hw/vga.c        |   52 ++++++++++++++--
 hw/vga_int.h    |    4 +-
 hw/vmware_vga.c |  185 +++++++++++--------------------------------------------
 3 files changed, 84 insertions(+), 157 deletions(-)

-- 
1.7.3.4

[Qemu-devel] [PATCH v2 1/6] vmware-vga: Register reset service

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Fixes cold reset in vmware graphic modes.

Reviewed-by: Andrzej Zaborowski <balrogg@gmail.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/vmware_vga.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index d5cfa70..3626bb0 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -998,8 +998,12 @@ static void vmsvga_update_display(void *opaque)
     }
 }
 
-static void vmsvga_reset(struct vmsvga_state_s *s)
+static void vmsvga_reset(DeviceState *dev)
 {
+    struct pci_vmsvga_state_s *pci =
+        DO_UPCAST(struct pci_vmsvga_state_s, card.qdev, dev);
+    struct vmsvga_state_s *s = &pci->chip;
+
     s->index = 0;
     s->enable = 0;
     s->config = 0;
@@ -1225,8 +1229,6 @@ static void vmsvga_init(struct vmsvga_state_s *s, int vga_ram_size)
     vga_common_init(&s->vga, vga_ram_size);
     vga_init(&s->vga);
     vmstate_register(NULL, 0, &vmstate_vga_common, &s->vga);
-
-    vmsvga_reset(s);
 }
 
 static uint64_t vmsvga_io_read(void *opaque, target_phys_addr_t addr,
@@ -1311,6 +1313,7 @@ static PCIDeviceInfo vmsvga_info = {
     .qdev.name    = "vmware-svga",
     .qdev.size    = sizeof(struct pci_vmsvga_state_s),
     .qdev.vmsd    = &vmstate_vmware_vga,
+    .qdev.reset   = vmsvga_reset,
     .no_hotplug   = 1,
     .init         = pci_vmsvga_initfn,
     .romfile      = "vgabios-vmware.bin",
-- 
1.7.3.4

[Qemu-devel] [PATCH v2 2/6] vmware-vga: Disable verbose mode

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Elimiates 'vmsvga_value_write: guest runs Linux.' messages from the
console.

CC: Andrzej Zaborowski <balrogg@gmail.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/vmware_vga.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 3626bb0..7a22d6a 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -27,7 +27,7 @@
 #include "pci.h"
 #include "vmware_vga.h"
 
-#define VERBOSE
+#undef VERBOSE
 #undef DIRECT_VRAM
 #define HW_RECT_ACCEL
 #define HW_FILL_ACCEL
-- 
1.7.3.4

[Qemu-devel] [PATCH v2 3/6] vmware-vga: Remove dead DIRECT_VRAM mode

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

The code was disabled since day 1 of vmware-vga, and now it does not
even build anymore. Time for a cleanup.

CC: Andrzej Zaborowski <balrogg@gmail.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/vmware_vga.c |  172 ++++++++++---------------------------------------------
 1 files changed, 30 insertions(+), 142 deletions(-)

diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 7a22d6a..0c94ef1 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -28,7 +28,6 @@
 #include "vmware_vga.h"
 
 #undef VERBOSE
-#undef DIRECT_VRAM
 #define HW_RECT_ACCEL
 #define HW_FILL_ACCEL
 #define HW_MOUSE_ACCEL
@@ -292,7 +291,6 @@ enum {
 static inline void vmsvga_update_rect(struct vmsvga_state_s *s,
                 int x, int y, int w, int h)
 {
-#ifndef DIRECT_VRAM
     int line;
     int bypl;
     int width;
@@ -323,23 +321,17 @@ static inline void vmsvga_update_rect(struct vmsvga_state_s *s,
 
     for (; line > 0; line --, src += bypl, dst += bypl)
         memcpy(dst, src, width);
-#endif
 
     dpy_update(s->vga.ds, x, y, w, h);
 }
 
 static inline void vmsvga_update_screen(struct vmsvga_state_s *s)
 {
-#ifndef DIRECT_VRAM
-    memcpy(ds_get_data(s->vga.ds), s->vga.vram_ptr, s->bypp * s->width * s->height);
-#endif
-
+    memcpy(ds_get_data(s->vga.ds), s->vga.vram_ptr,
+           s->bypp * s->width * s->height);
     dpy_update(s->vga.ds, 0, 0, s->width, s->height);
 }
 
-#ifdef DIRECT_VRAM
-# define vmsvga_update_rect_delayed	vmsvga_update_rect
-#else
 static inline void vmsvga_update_rect_delayed(struct vmsvga_state_s *s,
                 int x, int y, int w, int h)
 {
@@ -350,7 +342,6 @@ static inline void vmsvga_update_rect_delayed(struct vmsvga_state_s *s,
     rect->w = w;
     rect->h = h;
 }
-#endif
 
 static inline void vmsvga_update_rect_flush(struct vmsvga_state_s *s)
 {
@@ -372,32 +363,23 @@ static inline void vmsvga_update_rect_flush(struct vmsvga_state_s *s)
 static inline void vmsvga_copy_rect(struct vmsvga_state_s *s,
                 int x0, int y0, int x1, int y1, int w, int h)
 {
-# ifdef DIRECT_VRAM
-    uint8_t *vram = ds_get_data(s->ds);
-# else
     uint8_t *vram = s->vga.vram_ptr;
-# endif
     int bypl = s->bypp * s->width;
     int width = s->bypp * w;
     int line = h;
     uint8_t *ptr[2];
 
-# ifdef DIRECT_VRAM
-    if (s->ds->dpy_copy)
-        qemu_console_copy(s->ds, x0, y0, x1, y1, w, h);
-    else
-# endif
-    {
-        if (y1 > y0) {
-            ptr[0] = vram + s->bypp * x0 + bypl * (y0 + h - 1);
-            ptr[1] = vram + s->bypp * x1 + bypl * (y1 + h - 1);
-            for (; line > 0; line --, ptr[0] -= bypl, ptr[1] -= bypl)
-                memmove(ptr[1], ptr[0], width);
-        } else {
-            ptr[0] = vram + s->bypp * x0 + bypl * y0;
-            ptr[1] = vram + s->bypp * x1 + bypl * y1;
-            for (; line > 0; line --, ptr[0] += bypl, ptr[1] += bypl)
-                memmove(ptr[1], ptr[0], width);
+    if (y1 > y0) {
+        ptr[0] = vram + s->bypp * x0 + bypl * (y0 + h - 1);
+        ptr[1] = vram + s->bypp * x1 + bypl * (y1 + h - 1);
+        for (; line > 0; line --, ptr[0] -= bypl, ptr[1] -= bypl) {
+            memmove(ptr[1], ptr[0], width);
+        }
+    } else {
+        ptr[0] = vram + s->bypp * x0 + bypl * y0;
+        ptr[1] = vram + s->bypp * x1 + bypl * y1;
+        for (; line > 0; line --, ptr[0] += bypl, ptr[1] += bypl) {
+            memmove(ptr[1], ptr[0], width);
         }
     }
 
@@ -409,11 +391,7 @@ static inline void vmsvga_copy_rect(struct vmsvga_state_s *s,
 static inline void vmsvga_fill_rect(struct vmsvga_state_s *s,
                 uint32_t c, int x, int y, int w, int h)
 {
-# ifdef DIRECT_VRAM
-    uint8_t *vram = ds_get_data(s->ds);
-# else
     uint8_t *vram = s->vga.vram_ptr;
-# endif
     int bypp = s->bypp;
     int bypl = bypp * s->width;
     int width = bypp * w;
@@ -424,31 +402,25 @@ static inline void vmsvga_fill_rect(struct vmsvga_state_s *s,
     uint8_t *src;
     uint8_t col[4];
 
-# ifdef DIRECT_VRAM
-    if (s->ds->dpy_fill)
-        s->ds->dpy_fill(s->ds, x, y, w, h, c);
-    else
-# endif
-    {
-        col[0] = c;
-        col[1] = c >> 8;
-        col[2] = c >> 16;
-        col[3] = c >> 24;
-
-        if (line --) {
-            dst = fst;
-            src = col;
-            for (column = width; column > 0; column --) {
-                *(dst ++) = *(src ++);
-                if (src - col == bypp)
-                    src = col;
-            }
-            dst = fst;
-            for (; line > 0; line --) {
-                dst += bypl;
-                memcpy(dst, fst, width);
+    col[0] = c;
+    col[1] = c >> 8;
+    col[2] = c >> 16;
+    col[3] = c >> 24;
+
+    if (line--) {
+        dst = fst;
+        src = col;
+        for (column = width; column > 0; column--) {
+            *(dst++) = *(src++);
+            if (src - col == bypp) {
+                src = col;
             }
         }
+        dst = fst;
+        for (; line > 0; line--) {
+            dst += bypl;
+            memcpy(dst, fst, width);
+        }
     }
 
     vmsvga_update_rect_delayed(s, x, y, w, h);
@@ -1084,82 +1056,6 @@ static void vmsvga_text_update(void *opaque, console_ch_t *chardata)
         s->vga.text_update(&s->vga, chardata);
 }
 
-#ifdef DIRECT_VRAM
-static uint32_t vmsvga_vram_readb(void *opaque, target_phys_addr_t addr)
-{
-    struct vmsvga_state_s *s = opaque;
-    if (addr < s->fb_size)
-        return *(uint8_t *) (ds_get_data(s->ds) + addr);
-    else
-        return *(uint8_t *) (s->vram_ptr + addr);
-}
-
-static uint32_t vmsvga_vram_readw(void *opaque, target_phys_addr_t addr)
-{
-    struct vmsvga_state_s *s = opaque;
-    if (addr < s->fb_size)
-        return *(uint16_t *) (ds_get_data(s->ds) + addr);
-    else
-        return *(uint16_t *) (s->vram_ptr + addr);
-}
-
-static uint32_t vmsvga_vram_readl(void *opaque, target_phys_addr_t addr)
-{
-    struct vmsvga_state_s *s = opaque;
-    if (addr < s->fb_size)
-        return *(uint32_t *) (ds_get_data(s->ds) + addr);
-    else
-        return *(uint32_t *) (s->vram_ptr + addr);
-}
-
-static void vmsvga_vram_writeb(void *opaque, target_phys_addr_t addr,
-                uint32_t value)
-{
-    struct vmsvga_state_s *s = opaque;
-    if (addr < s->fb_size)
-        *(uint8_t *) (ds_get_data(s->ds) + addr) = value;
-    else
-        *(uint8_t *) (s->vram_ptr + addr) = value;
-}
-
-static void vmsvga_vram_writew(void *opaque, target_phys_addr_t addr,
-                uint32_t value)
-{
-    struct vmsvga_state_s *s = opaque;
-    if (addr < s->fb_size)
-        *(uint16_t *) (ds_get_data(s->ds) + addr) = value;
-    else
-        *(uint16_t *) (s->vram_ptr + addr) = value;
-}
-
-static void vmsvga_vram_writel(void *opaque, target_phys_addr_t addr,
-                uint32_t value)
-{
-    struct vmsvga_state_s *s = opaque;
-    if (addr < s->fb_size)
-        *(uint32_t *) (ds_get_data(s->ds) + addr) = value;
-    else
-        *(uint32_t *) (s->vram_ptr + addr) = value;
-}
-
-static const MemoryRegionOps vmsvga_vram_io_ops = {
-    .old_mmio = {
-        .read = {
-            vmsvga_vram_readb,
-            vmsvga_vram_readw,
-            vmsvga_vram_readl,
-        },
-        .write = {
-            vmsvga_vram_writeb,
-            vmsvga_vram_writew,
-            vmsvga_vram_writel,
-        },
-    },
-    .endianness = DEVICE_NATIVE_ENDIAN,
-}
-
-#endif
-
 static int vmsvga_post_load(void *opaque, int version_id)
 {
     struct vmsvga_state_s *s = opaque;
@@ -1275,15 +1171,7 @@ static int pci_vmsvga_initfn(PCIDevice *dev)
         DO_UPCAST(struct pci_vmsvga_state_s, card, dev);
     MemoryRegion *iomem;
 
-#ifdef DIRECT_VRAM
-    DirectMem *directmem = qemu_malloc(sizeof(*directmem));
-
-    iomem = &directmem->mr;
-    memory_region_init_io(iomem, &vmsvga_vram_io_ops, &s->chip, "vmsvga",
-                          memory_region_size(&s->chip.vga.vram));
-#else
     iomem = &s->chip.vga.vram;
-#endif
 
     vga_dirty_log_restart(&s->chip.vga);
 
-- 
1.7.3.4

[Qemu-devel] [PATCH v2 4/6] vmware-vga: Eliminate vga_dirty_log_restart

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

After the conversion to the new Memory API, vga_dirty_log_restart became
seriously pointless. Remove it from vmware-vga and and then finally drop
the service.

CC: Andrzej Zaborowski <balrogg@gmail.com>
CC: Avi Kivity <avi@redhat.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/vga.c        |    6 ------
 hw/vga_int.h    |    1 -
 hw/vmware_vga.c |    2 --
 3 files changed, 0 insertions(+), 9 deletions(-)

diff --git a/hw/vga.c b/hw/vga.c
index 33dc478..fe481dd 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -1533,12 +1533,6 @@ void vga_dirty_log_stop(VGACommonState *s)
     memory_region_set_log(&s->vram, false, DIRTY_MEMORY_VGA);
 }
 
-void vga_dirty_log_restart(VGACommonState *s)
-{
-    vga_dirty_log_stop(s);
-    vga_dirty_log_start(s);
-}
-
 /*
  * graphic modes
  */
diff --git a/hw/vga_int.h b/hw/vga_int.h
index 100d98c..6fc23a6 100644
--- a/hw/vga_int.h
+++ b/hw/vga_int.h
@@ -193,7 +193,6 @@ void vga_common_reset(VGACommonState *s);
 
 void vga_dirty_log_start(VGACommonState *s);
 void vga_dirty_log_stop(VGACommonState *s);
-void vga_dirty_log_restart(VGACommonState *s);
 
 extern const VMStateDescription vmstate_vga_common;
 uint32_t vga_ioport_read(void *opaque, uint32_t addr);
diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 0c94ef1..475829e 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -1173,8 +1173,6 @@ static int pci_vmsvga_initfn(PCIDevice *dev)
 
     iomem = &s->chip.vga.vram;
 
-    vga_dirty_log_restart(&s->chip.vga);
-
     s->card.config[PCI_CACHE_LINE_SIZE]	= 0x08;		/* Cache line size */
     s->card.config[PCI_LATENCY_TIMER] = 0x40;		/* Latency timer */
     s->card.config[PCI_INTERRUPT_LINE] = 0xff;		/* End */
-- 
1.7.3.4

[Qemu-devel] [PATCH v2 5/6] vga: Use linear mapping + dirty logging in chain 4 memory access mode

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Most VGA memory access modes require MMIO handling as they demand weird
logic to get a byte from or into the video RAM. However, there is one
exception: chain 4 mode with all memory planes enabled for writing. This
mode actually allows lineary mapping, which can then be combined with
dirty logging to accelerate KVM.

This patch accelerates specifically VBE accesses like they are used by
grub in graphical mode. Not only the standard VGA adapter benefits from
this, also vmware and spice in VGA mode.

CC: Gerd Hoffmann <kraxel@redhat.com>
CC: Avi Kivity <avi@redhat.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/vga.c     |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 hw/vga_int.h |    1 +
 2 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/hw/vga.c b/hw/vga.c
index fe481dd..fa57766 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -153,6 +153,47 @@ static void vga_screen_dump(void *opaque, const char *filename);
 static char *screen_dump_filename;
 static DisplayChangeListener *screen_dump_dcl;
 
+static void vga_update_memory_access(VGACommonState *s)
+{
+    MemoryRegion *region, *old_region = s->chain4_alias;
+    target_phys_addr_t base, offset, size;
+
+    s->chain4_alias = NULL;
+
+    if ((s->sr[0x02] & 0xf) == 0xf && s->sr[0x04] & 0x08) {
+        offset = 0;
+        switch ((s->gr[6] >> 2) & 3) {
+        case 0:
+            base = 0xa0000;
+            size = 0x20000;
+            break;
+        case 1:
+            base = 0xa0000;
+            size = 0x10000;
+            offset = s->bank_offset;
+            break;
+        case 2:
+            base = 0xb0000;
+            size = 0x8000;
+            break;
+        case 3:
+            base = 0xb8000;
+            size = 0x8000;
+            break;
+        }
+        region = qemu_malloc(sizeof(*region));
+        memory_region_init_alias(region, "vga.chain4", &s->vram, offset, size);
+        memory_region_add_subregion_overlap(get_system_memory(), base,
+                                            region, 2);
+        s->chain4_alias = region;
+    }
+    if (old_region) {
+        memory_region_del_subregion(get_system_memory(), old_region);
+        memory_region_destroy(old_region);
+        qemu_free(old_region);
+    }
+}
+
 static void vga_dumb_update_retrace_info(VGACommonState *s)
 {
     (void) s;
@@ -446,6 +487,7 @@ void vga_ioport_write(void *opaque, uint32_t addr, uint32_t val)
 #endif
         s->sr[s->sr_index] = val & sr_mask[s->sr_index];
         if (s->sr_index == 1) s->update_retrace_info(s);
+        vga_update_memory_access(s);
         break;
     case 0x3c7:
         s->dac_read_index = val;
@@ -473,6 +515,7 @@ void vga_ioport_write(void *opaque, uint32_t addr, uint32_t val)
         printf("vga: write GR%x = 0x%02x\n", s->gr_index, val);
 #endif
         s->gr[s->gr_index] = val & gr_mask[s->gr_index];
+        vga_update_memory_access(s);
         break;
     case 0x3b4:
     case 0x3d4:
@@ -606,6 +649,7 @@ static void vbe_ioport_write_data(void *opaque, uint32_t addr, uint32_t val)
             }
             s->vbe_regs[s->vbe_index] = val;
             s->bank_offset = (val << 16);
+            vga_update_memory_access(s);
             break;
         case VBE_DISPI_INDEX_ENABLE:
             if ((val & VBE_DISPI_ENABLED) &&
@@ -665,6 +709,7 @@ static void vbe_ioport_write_data(void *opaque, uint32_t addr, uint32_t val)
             }
             s->dac_8bit = (val & VBE_DISPI_8BIT_DAC) > 0;
             s->vbe_regs[s->vbe_index] = val;
+            vga_update_memory_access(s);
             break;
         case VBE_DISPI_INDEX_VIRT_WIDTH:
             {
@@ -1886,6 +1931,7 @@ void vga_common_reset(VGACommonState *s)
         memset(&s->retrace_info, 0, sizeof (s->retrace_info));
         break;
     }
+    vga_update_memory_access(s);
 }
 
 static void vga_reset(void *opaque)
diff --git a/hw/vga_int.h b/hw/vga_int.h
index 6fc23a6..2b13b9d 100644
--- a/hw/vga_int.h
+++ b/hw/vga_int.h
@@ -110,6 +110,7 @@ typedef struct VGACommonState {
     uint32_t vram_size;
     uint32_t latch;
     uint32_t lfb_vram_mapped; /* whether 0xa0000 is mapped as ram */
+    MemoryRegion *chain4_alias;
     uint8_t sr_index;
     uint8_t sr[256];
     uint8_t gr_index;
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH v2 5/6] vga: Use linear mapping + dirty logging in chain 4 memory access mode

[Avi Kivity]

On 08/17/2011 03:41 PM, Jan Kiszka wrote:
> From: Jan Kiszka<jan.kiszka@siemens.com>
>
> Most VGA memory access modes require MMIO handling as they demand weird
> logic to get a byte from or into the video RAM. However, there is one
> exception: chain 4 mode with all memory planes enabled for writing. This
> mode actually allows lineary mapping, which can then be combined with
> dirty logging to accelerate KVM.
>
> This patch accelerates specifically VBE accesses like they are used by
> grub in graphical mode. Not only the standard VGA adapter benefits from
> this, also vmware and spice in VGA mode.

Excellent, this slowness is very annoying.

>
> +static void vga_update_memory_access(VGACommonState *s)
> +{
> +    MemoryRegion *region, *old_region = s->chain4_alias;
> +    target_phys_addr_t base, offset, size;
> +
> +    s->chain4_alias = NULL;
> +
> +    if ((s->sr[0x02]&  0xf) == 0xf&&  s->sr[0x04]&  0x08) {
> +        offset = 0;
> +        switch ((s->gr[6]>>  2)&  3) {
> +        case 0:
> +            base = 0xa0000;
> +            size = 0x20000;
> +            break;
> +        case 1:
> +            base = 0xa0000;
> +            size = 0x10000;
> +            offset = s->bank_offset;
> +            break;
> +        case 2:
> +            base = 0xb0000;
> +            size = 0x8000;
> +            break;
> +        case 3:
> +            base = 0xb8000;
> +            size = 0x8000;
> +            break;
> +        }
> +        region = qemu_malloc(sizeof(*region));
> +        memory_region_init_alias(region, "vga.chain4",&s->vram, offset, size);
> +        memory_region_add_subregion_overlap(get_system_memory(), base,
> +                                            region, 2);

get_system_memory() has been excised from vga in batch 4 of my patches, 
please adjust.

> +        s->chain4_alias = region;
> +    }
> +    if (old_region) {
> +        memory_region_del_subregion(get_system_memory(), old_region);
> +        memory_region_destroy(old_region);
> +        qemu_free(old_region);
> +    }
> +}
> +
>

The mmio code has

             s->plane_updated |= mask; /* only used to detect font change */

aren't we losing it?  we could easily recover it via dirty logging.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH v2 5/6] vga: Use linear mapping + dirty logging in chain 4 memory access mode

[Avi Kivity]

On 08/17/2011 04:38 PM, Avi Kivity wrote:
>
> The mmio code has
>
>             s->plane_updated |= mask; /* only used to detect font 
> change */
>
> aren't we losing it?  we could easily recover it via dirty logging.
>

We can't really recover it.  So I think we need to restrict the 
optimization to graphic mode.

Is grub using text mode or graphic mode?

If it's using text mode, it may be faster to compare the font plane to a 
snapshot from the last redraw than to take an exit.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH v2 5/6] vga: Use linear mapping + dirty logging in chain 4 memory access mode

[Jan Kiszka]

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

On 2011-08-17 16:48, Avi Kivity wrote:
> On 08/17/2011 04:38 PM, Avi Kivity wrote:
>>
>> The mmio code has
>>
>>             s->plane_updated |= mask; /* only used to detect font
>> change */
>>
>> aren't we losing it?  we could easily recover it via dirty logging.
>>

Yes, I forgot to forward-port plane_updated = 0xf from v1 of the patch.

> 
> We can't really recover it.

I don't see yet why we should not if we simply enforce a full update.
Can you elaborate?

>  So I think we need to restrict the
> optimization to graphic mode.
> 
> Is grub using text mode or graphic mode?
> 
> If it's using text mode, it may be faster to compare the font plane to a
> snapshot from the last redraw than to take an exit.

I wasn't optimizing for text mode here, it's just a side effect if it
happens to benefit from it as well. I could exclude it, but only if
really needed.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH v2 5/6] vga: Use linear mapping + dirty logging in chain 4 memory access mode

[Avi Kivity]

On 08/18/2011 03:26 PM, Jan Kiszka wrote:
> On 2011-08-17 16:48, Avi Kivity wrote:
> >  On 08/17/2011 04:38 PM, Avi Kivity wrote:
> >>
> >>  The mmio code has
> >>
> >>              s->plane_updated |= mask; /* only used to detect font
> >>  change */
> >>
> >>  aren't we losing it?  we could easily recover it via dirty logging.
> >>
>
> Yes, I forgot to forward-port plane_updated = 0xf from v1 of the patch.
>
> >
> >  We can't really recover it.
>
> I don't see yet why we should not if we simply enforce a full update.
> Can you elaborate?

If we go for a full update we might as well disable dirty logging entirely.

>
> >   So I think we need to restrict the
> >  optimization to graphic mode.
> >
> >  Is grub using text mode or graphic mode?
> >
> >  If it's using text mode, it may be faster to compare the font plane to a
> >  snapshot from the last redraw than to take an exit.
>
> I wasn't optimizing for text mode here, it's just a side effect if it
> happens to benefit from it as well. I could exclude it, but only if
> really needed.
>

Ok, so grub is using graphic mode?


We'll need to force full updates in text mode then.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH v2 5/6] vga: Use linear mapping + dirty logging in chain 4 memory access mode

[Jan Kiszka]

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

On 2011-08-19 08:19, Avi Kivity wrote:
> On 08/18/2011 03:26 PM, Jan Kiszka wrote:
>> On 2011-08-17 16:48, Avi Kivity wrote:
>> >  On 08/17/2011 04:38 PM, Avi Kivity wrote:
>> >>
>> >>  The mmio code has
>> >>
>> >>              s->plane_updated |= mask; /* only used to detect font
>> >>  change */
>> >>
>> >>  aren't we losing it?  we could easily recover it via dirty logging.
>> >>
>>
>> Yes, I forgot to forward-port plane_updated = 0xf from v1 of the patch.
>>
>> >
>> >  We can't really recover it.
>>
>> I don't see yet why we should not if we simply enforce a full update.
>> Can you elaborate?
> 
> If we go for a full update we might as well disable dirty logging entirely.

Full update only affects text screen refreshes. The guest access to the
VRAM will still benefit from the much faster mmio.

> 
>>
>> >   So I think we need to restrict the
>> >  optimization to graphic mode.
>> >
>> >  Is grub using text mode or graphic mode?
>> >
>> >  If it's using text mode, it may be faster to compare the font plane
>> to a
>> >  snapshot from the last redraw than to take an exit.
>>
>> I wasn't optimizing for text mode here, it's just a side effect if it
>> happens to benefit from it as well. I could exclude it, but only if
>> really needed.
>>
> 
> Ok, so grub is using graphic mode?

Yep.

> 
> 
> We'll need to force full updates in text mode then.

Right. I'll sent an update doing that. There are some other quirks of
the series remaining I'm addressing ATM.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[Qemu-devel] [PATCH v2 6/6] vga: Drop some unused fields

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Memory region refactorings obsoleted them.

CC: Avi Kivity <avi@redhat.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/vga_int.h |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/hw/vga_int.h b/hw/vga_int.h
index 2b13b9d..2b9fa7e 100644
--- a/hw/vga_int.h
+++ b/hw/vga_int.h
@@ -109,7 +109,6 @@ typedef struct VGACommonState {
     MemoryRegion vram;
     uint32_t vram_size;
     uint32_t latch;
-    uint32_t lfb_vram_mapped; /* whether 0xa0000 is mapped as ram */
     MemoryRegion *chain4_alias;
     uint8_t sr_index;
     uint8_t sr[256];
@@ -132,7 +131,6 @@ typedef struct VGACommonState {
     int dac_8bit;
     uint8_t palette[768];
     int32_t bank_offset;
-    MemoryRegion *vga_io_memory;
     int (*get_bpp)(struct VGACommonState *s);
     void (*get_offsets)(struct VGACommonState *s,
                         uint32_t *pline_offset,
-- 
1.7.3.4