[Qemu-devel] [PATCH 0/5] VGA Optimization Episode IV - A new hope

[Qemu-devel] [PATCH 0/5] VGA Optimization Episode IV - A new hope

[Glauber Costa]

                          Hey guys,
                    I'm  back  in this new
	       episode of  the  vga  optmization
           patches. After Anthony Skywalker comments,
        the  rebels  figured  out that using  userspace_addr 
     as an offset could trigger the death star. There is a new 
   proposal of informing kvm through its interface about the address
 (ram_addr_t) of the region we want to update. We can have the choice of
updating or not the region...

[Qemu-devel] [PATCH 1/5] better type checking for vga

[Glauber Costa]

unsigned long is too bad of a type. Use ram_addr_t instead.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 hw/cirrus_vga.c |    4 ++--
 hw/vga.c        |    2 +-
 hw/vga_int.h    |    2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 55f3ced..c23b6a1 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -3232,7 +3232,7 @@ static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci)
  ***************************************/
 
 void isa_cirrus_vga_init(DisplayState *ds, uint8_t *vga_ram_base,
-                         unsigned long vga_ram_offset, int vga_ram_size)
+                         ram_addr_t vga_ram_offset, int vga_ram_size)
 {
     CirrusVGAState *s;
 
@@ -3274,7 +3274,7 @@ static void cirrus_pci_mmio_map(PCIDevice *d, int region_num,
 }
 
 void pci_cirrus_vga_init(PCIBus *bus, DisplayState *ds, uint8_t *vga_ram_base,
-                         unsigned long vga_ram_offset, int vga_ram_size)
+                         ram_addr_t vga_ram_offset, int vga_ram_size)
 {
     PCICirrusVGAState *d;
     uint8_t *pci_conf;
diff --git a/hw/vga.c b/hw/vga.c
index 9540db0..0b38fe5 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -2105,7 +2105,7 @@ static void vga_map(PCIDevice *pci_dev, int region_num,
 }
 
 void vga_common_init(VGAState *s, DisplayState *ds, uint8_t *vga_ram_base,
-                     unsigned long vga_ram_offset, int vga_ram_size)
+                     ram_addr_t vga_ram_offset, int vga_ram_size)
 {
     int i, j, v, b;
 
diff --git a/hw/vga_int.h b/hw/vga_int.h
index 82a755e..d559fcf 100644
--- a/hw/vga_int.h
+++ b/hw/vga_int.h
@@ -100,7 +100,7 @@ typedef void (* vga_update_retrace_info_fn)(struct VGAState *s);
 
 #define VGA_STATE_COMMON                                                \
     uint8_t *vram_ptr;                                                  \
-    unsigned long vram_offset;                                          \
+    ram_addr_t vram_offset;                                             \
     unsigned int vram_size;                                             \
     unsigned long bios_offset;                                          \
     unsigned int bios_size;                                             \
-- 
1.5.6.5

[Qemu-devel] [PATCH 2/5] move vga_io_address to VGA State

[Glauber Costa]

It'll be reused later by the vga optimization.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 hw/cirrus_vga.c |    6 +++---
 hw/vga_int.h    |    1 +
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index c23b6a1..777bfbb 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -3118,7 +3118,7 @@ static int cirrus_vga_load(QEMUFile *f, void *opaque, int version_id)
 
 static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci)
 {
-    int vga_io_memory, i;
+    int i;
     static int inited;
 
     if (!inited) {
@@ -3157,10 +3157,10 @@ static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci)
     register_ioport_read(0x3ba, 1, 1, vga_ioport_read, s);
     register_ioport_read(0x3da, 1, 1, vga_ioport_read, s);
 
-    vga_io_memory = cpu_register_io_memory(0, cirrus_vga_mem_read,
+    s->vga_io_memory = cpu_register_io_memory(0, cirrus_vga_mem_read,
                                            cirrus_vga_mem_write, s);
     cpu_register_physical_memory(isa_mem_base + 0x000a0000, 0x20000,
-                                 vga_io_memory);
+                                 s->vga_io_memory);
 
     s->sr[0x06] = 0x0f;
     if (device_id == CIRRUS_ID_CLGD5446) {
diff --git a/hw/vga_int.h b/hw/vga_int.h
index d559fcf..91a8d77 100644
--- a/hw/vga_int.h
+++ b/hw/vga_int.h
@@ -129,6 +129,7 @@ typedef void (* vga_update_retrace_info_fn)(struct VGAState *s);
     int dac_8bit;                                                       \
     uint8_t palette[768];                                               \
     int32_t bank_offset;                                                \
+    int vga_io_memory;                                             \
     int (*get_bpp)(struct VGAState *s);                                 \
     void (*get_offsets)(struct VGAState *s,                             \
                         uint32_t *pline_offset,                         \
-- 
1.5.6.5

[Qemu-devel] [PATCH 3/5] de-register mem region for MMIO.

[Glauber Costa]

Besides unassigned memory, we also don't care about MMIO.
So if we're giving an MMIO area that is already registered,
wipe it out.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 kvm-all.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 4379071..6d50609 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -317,7 +317,8 @@ void kvm_set_phys_mem(target_phys_addr_t start_addr,
 
     mem = kvm_lookup_slot(s, start_addr);
     if (mem) {
-        if (flags == IO_MEM_UNASSIGNED) {
+        if ((flags == IO_MEM_UNASSIGNED) || (flags >= TLB_MMIO)) {
+            dprintf("deleting memory %llx with flags %d\n", mem->guest_phys_addr, mem->flags);
             mem->memory_size = 0;
             mem->guest_phys_addr = start_addr;
             mem->userspace_addr = 0;
-- 
1.5.6.5

[Qemu-devel] [PATCH 4/5] Introduce kvm logging interface.

[Glauber Costa]

Introduce functions to stop and start logging of memory regions.
We select region based on its start address.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 kvm-all.c |  139 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 kvm.h     |    5 ++
 2 files changed, 144 insertions(+), 0 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 6d50609..cb7bf6a 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -31,8 +31,13 @@
     do { } while (0)
 #endif
 
+#define warning(fmt, ...) \
+    do { dprintf("%s:%d" fmt, __func__, __LINE__, ## __VA_ARGS__); } while (0)
+
 typedef struct kvm_userspace_memory_region KVMSlot;
 
+typedef struct kvm_dirty_log KVMDirtyLog;
+
 int kvm_allowed = 0;
 
 struct KVMState
@@ -109,6 +114,140 @@ err:
     return ret;
 }
 
+/*
+ * dirty pages logging control
+ */
+static int kvm_dirty_pages_log_change(KVMSlot *mem,
+                                      unsigned flags,
+                                      unsigned mask)
+{
+    int r = -1;
+    KVMState *s = kvm_state;
+
+    flags = (mem->flags & ~mask) | flags;
+    if (flags == mem->flags)
+            return 0;
+
+    mem->flags = flags;
+
+    r = kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, mem);
+    if (r == -1)
+        fprintf(stderr, "%s: %m\n", __FUNCTION__);
+
+    return r;
+}
+
+int kvm_log_start(target_phys_addr_t phys_addr, target_phys_addr_t len)
+{
+        KVMState *s = kvm_state;
+        KVMSlot *mem = kvm_lookup_slot(s, phys_addr);
+
+        if (mem == NULL)  {
+                fprintf(stderr, "BUG: %s: invalid parameters\n", __func__);
+                return -EINVAL;
+        }
+
+        /* Already logging, no need to issue ioctl */
+        if (mem->flags & KVM_MEM_LOG_DIRTY_PAGES)
+            return 0;
+
+        dprintf("slot %d: enable logging (phys %llx, uaddr %llx)\n",
+                 mem->slot, mem->guest_phys_addr, mem->userspace_addr);
+
+        return kvm_dirty_pages_log_change(mem,
+                                          KVM_MEM_LOG_DIRTY_PAGES,
+                                          KVM_MEM_LOG_DIRTY_PAGES);
+}
+
+int kvm_log_stop(target_phys_addr_t phys_addr, target_phys_addr_t len)
+{
+
+        KVMState *s = kvm_state;
+        KVMSlot *mem = kvm_lookup_slot(s, phys_addr);
+
+        if (mem == NULL) {
+                fprintf(stderr, "BUG: %s: invalid parameters\n", __func__);
+                return -EINVAL;
+        }
+
+        /* Not logging, no need to issue ioctl */
+        if (!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES))
+            return 0;
+
+        dprintf("slot %d: disabling logging\n", mem->slot);
+        return kvm_dirty_pages_log_change(mem,
+                                          0,
+                                          KVM_MEM_LOG_DIRTY_PAGES);
+}
+
+static inline int lookup_bitmap_phys(unsigned long *dirty, unsigned nr)
+{
+    unsigned word = nr / (sizeof(*dirty) * 8);
+    unsigned bit = nr % (sizeof(*dirty) * 8);
+    int ret;
+
+    ret = (dirty[word] >> bit) & 1;
+    return ret;
+}
+
+/**
+ * kvm_physical_sync_dirty_bitmap - Grab dirty bitmap from kernel space
+ * If a phys_offset parameter is given, this function updates qemu's dirty
+ * bitmap using cpu_physical_memory_set_dirty(). This means all bits are set
+ * to dirty.
+ *
+ * @start_add: start of logged region. This is what we use to search the memslot
+ * @end_addr: end of logged region. Only matters if we are updating qemu dirty bitmap.
+ * @phys_offset: the phys_offset we want to use for qemu dirty bitmap update. Passing
+ *               NULL makes the update not happen. In this case, we only grab the bitmap
+ *               from kernel.
+ */
+void kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr, target_phys_addr_t end_addr,
+                                    ram_addr_t phys_offset)
+{
+    KVMState *s = kvm_state;
+    KVMDirtyLog d;
+    KVMSlot *mem = kvm_lookup_slot(s, start_addr);
+    unsigned long alloc_size = mem->memory_size >> TARGET_PAGE_BITS / sizeof(d.dirty_bitmap);
+    ram_addr_t addr;
+    target_phys_addr_t phys_addr;
+
+    printf("sync addr: %llx into %lx\n", start_addr, phys_offset);
+    if (mem == NULL) {
+            fprintf(stderr, "BUG: %s: invalid parameters\n", __func__);
+            return;
+    }
+
+    d.dirty_bitmap = qemu_mallocz(alloc_size);
+
+    if (d.dirty_bitmap == NULL) {
+        warning("Could not allocate dirty bitmap\n");
+        return;
+    }
+
+    d.slot = mem->slot;
+    dprintf("slot %d, phys_addr %llx, uaddr: %llx\n",
+            d.slot, mem->guest_phys_addr, mem->userspace_addr);
+
+    if (kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d) == -1) {
+        warning("ioctl failed %d\n", errno);
+        goto out;
+    }
+
+    /* Caller don't want to update dirty bitmap */
+    if (!phys_offset)
+        goto out;
+
+    phys_addr = start_addr;
+    for (addr = phys_offset; phys_addr < end_addr; phys_addr+= TARGET_PAGE_SIZE, addr += TARGET_PAGE_SIZE) {
+        unsigned long *bitmap = (unsigned long *)d.dirty_bitmap;
+        if (lookup_bitmap_phys(bitmap, (phys_addr - start_addr) >> TARGET_PAGE_BITS))
+            cpu_physical_memory_set_dirty(addr);
+    }
+out:
+    qemu_free(d.dirty_bitmap);
+}
+
 int kvm_init(int smp_cpus)
 {
     KVMState *s;
diff --git a/kvm.h b/kvm.h
index 37102b4..90503e8 100644
--- a/kvm.h
+++ b/kvm.h
@@ -38,6 +38,11 @@ void kvm_set_phys_mem(target_phys_addr_t start_addr,
                       ram_addr_t size,
                       ram_addr_t phys_offset);
 
+void kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr, target_phys_addr_t end_addr,
+                                    ram_addr_t phys_offset);
+
+int kvm_log_start(target_phys_addr_t phys_addr, target_phys_addr_t len);
+int kvm_log_stop(target_phys_addr_t phys_addr, target_phys_addr_t len);
 /* internal API */
 
 struct KVMState;
-- 
1.5.6.5

[Qemu-devel] [PATCH 5/5] vga optimization.

[Glauber Costa]

Hypervisors like KVM perform badly while doing mmio on
a loop, because it'll generate an exit on each access.
This is the case with VGA, which results in very bad
performance.

In this patch, we map the linear frame buffer as RAM,
make sure it has dirty region tracking enabled, and then
just let the region to be written.

Cleanups suggestions by:
  Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 cpu-all.h       |    2 ++
 exec.c          |    7 +++++++
 hw/cirrus_vga.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 hw/vga.c        |   25 +++++++++++++++++++++++++
 hw/vga_int.h    |    8 ++++++++
 5 files changed, 85 insertions(+), 0 deletions(-)

diff --git a/cpu-all.h b/cpu-all.h
index cdd79bc..6ce2bde 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -934,6 +934,8 @@ int cpu_physical_memory_set_dirty_tracking(int enable);
 
 int cpu_physical_memory_get_dirty_tracking(void);
 
+void cpu_physical_sync_dirty_bitmap(target_phys_addr_t start_addr, target_phys_addr_t end_addr, ram_addr_t phys_offset);
+
 void dump_exec_info(FILE *f,
                     int (*cpu_fprintf)(FILE *f, const char *fmt, ...));
 
diff --git a/exec.c b/exec.c
index ef1072b..c09d370 100644
--- a/exec.c
+++ b/exec.c
@@ -1823,6 +1823,13 @@ int cpu_physical_memory_get_dirty_tracking(void)
     return in_migration;
 }
 
+void cpu_physical_sync_dirty_bitmap(target_phys_addr_t start_addr, target_phys_addr_t end_addr,
+                                    ram_addr_t phys_offset)
+{
+    if (kvm_enabled())
+        kvm_physical_sync_dirty_bitmap(start_addr, end_addr, phys_offset);
+}
+
 static inline void tlb_update_dirty(CPUTLBEntry *tlb_entry)
 {
     ram_addr_t ram_addr;
diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 777bfbb..4b0b204 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -31,6 +31,7 @@
 #include "pci.h"
 #include "console.h"
 #include "vga_int.h"
+#include "kvm.h"
 
 /*
  * TODO:
@@ -2619,6 +2620,39 @@ static CPUWriteMemoryFunc *cirrus_linear_bitblt_write[3] = {
     cirrus_linear_bitblt_writel,
 };
 
+static void map_linear_vram(CirrusVGAState *s)
+{
+    int phys_offset = s->vga_io_memory;
+
+    if (!s->map_addr && s->lfb_addr && s->lfb_end) {
+        s->map_addr = s->lfb_addr;
+        s->map_end = s->lfb_end;
+        cpu_register_physical_memory(s->map_addr, s->map_end - s->map_addr, s->vram_offset);
+        vga_dirty_log_start((VGAState *)s);
+    }
+
+    if(!(s->cirrus_srcptr != s->cirrus_srcptr_end)
+        && !((s->sr[0x07] & 0x01) == 0)
+        && !((s->gr[0x0B] & 0x14) == 0x14)
+        && !(s->gr[0x0B] & 0x02)) {
+        phys_offset = s->vram_offset | IO_MEM_RAM;
+    }
+    cpu_register_physical_memory(isa_mem_base + 0xa0000, 0x20000, phys_offset);
+}
+
+static void unmap_linear_vram(CirrusVGAState *s)
+{
+
+    if (s->map_addr && s->lfb_addr && s->lfb_end) {
+        vga_dirty_log_stop((VGAState *)s);
+        s->map_addr = s->map_end = 0;
+    }
+
+    cpu_register_physical_memory(isa_mem_base + 0x000a0000, 0x20000,
+                                 s->vga_io_memory);
+
+}
+
 /* Compute the memory access functions */
 static void cirrus_update_memory_access(CirrusVGAState *s)
 {
@@ -2637,11 +2671,13 @@ static void cirrus_update_memory_access(CirrusVGAState *s)
 
 	mode = s->gr[0x05] & 0x7;
 	if (mode < 4 || mode > 5 || ((s->gr[0x0B] & 0x4) == 0)) {
+            map_linear_vram(s);
             s->cirrus_linear_write[0] = cirrus_linear_mem_writeb;
             s->cirrus_linear_write[1] = cirrus_linear_mem_writew;
             s->cirrus_linear_write[2] = cirrus_linear_mem_writel;
         } else {
         generic_io:
+            unmap_linear_vram(s);
             s->cirrus_linear_write[0] = cirrus_linear_writeb;
             s->cirrus_linear_write[1] = cirrus_linear_writew;
             s->cirrus_linear_write[2] = cirrus_linear_writel;
@@ -3262,6 +3298,13 @@ static void cirrus_pci_lfb_map(PCIDevice *d, int region_num,
 				 s->cirrus_linear_io_addr);
     cpu_register_physical_memory(addr + 0x1000000, 0x400000,
 				 s->cirrus_linear_bitblt_io_addr);
+
+    s->map_addr = s->map_end = 0;
+    s->lfb_addr = addr & TARGET_PAGE_MASK;
+    s->lfb_end = ((addr + VGA_RAM_SIZE) + TARGET_PAGE_SIZE - 1) & TARGET_PAGE_MASK;
+    /* account for overflow */
+    if (s->lfb_end < addr + VGA_RAM_SIZE)
+        s->lfb_end = addr + VGA_RAM_SIZE;
 }
 
 static void cirrus_pci_mmio_map(PCIDevice *d, int region_num,
diff --git a/hw/vga.c b/hw/vga.c
index 0b38fe5..eaaf8f9 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -28,6 +28,7 @@
 #include "vga_int.h"
 #include "pixel_ops.h"
 #include "qemu-timer.h"
+#include "kvm.h"
 
 //#define DEBUG_VGA
 //#define DEBUG_VGA_MEM
@@ -1243,6 +1244,8 @@ static void vga_draw_text(VGAState *s, int full_update)
     vga_draw_glyph8_func *vga_draw_glyph8;
     vga_draw_glyph9_func *vga_draw_glyph9;
 
+    vga_dirty_log_stop(s);
+
     full_update |= update_palette16(s);
     palette = s->last_palette;
 
@@ -1568,6 +1571,9 @@ static void vga_draw_graphic(VGAState *s, int full_update)
     uint32_t v, addr1, addr;
     vga_draw_line_func *vga_draw_line;
 
+    cpu_physical_sync_dirty_bitmap(s->map_addr, s->map_end, s->vram_offset);
+    vga_dirty_log_start(s);
+
     full_update |= update_basic_params(s);
 
     s->get_resolution(s, &width, &height);
@@ -1743,6 +1749,8 @@ static void vga_draw_blank(VGAState *s, int full_update)
         return;
     if (s->last_scr_width <= 0 || s->last_scr_height <= 0)
         return;
+    vga_dirty_log_stop(s);
+
     if (s->ds->depth == 8)
         val = s->rgb_to_pixel(0, 0, 0);
     else
@@ -2092,6 +2100,18 @@ typedef struct PCIVGAState {
     VGAState vga_state;
 } PCIVGAState;
 
+void vga_dirty_log_start(VGAState *s)
+{
+    if (kvm_enabled() && s->map_addr)
+        kvm_log_start(s->map_addr, s->map_end - s->map_addr);
+}
+
+void vga_dirty_log_stop(VGAState *s)
+{
+    if (kvm_enabled() && s->map_addr)
+        kvm_log_stop(s->map_addr, s->map_end - s->map_addr);
+}
+
 static void vga_map(PCIDevice *pci_dev, int region_num,
                     uint32_t addr, uint32_t size, int type)
 {
@@ -2102,6 +2122,11 @@ static void vga_map(PCIDevice *pci_dev, int region_num,
     } else {
         cpu_register_physical_memory(addr, s->vram_size, s->vram_offset);
     }
+
+    s->map_addr = addr;
+    s->map_end = addr + VGA_RAM_SIZE;
+
+    vga_dirty_log_start(s);
 }
 
 void vga_common_init(VGAState *s, DisplayState *ds, uint8_t *vga_ram_base,
diff --git a/hw/vga_int.h b/hw/vga_int.h
index 91a8d77..d1d35f9 100644
--- a/hw/vga_int.h
+++ b/hw/vga_int.h
@@ -102,6 +102,10 @@ typedef void (* vga_update_retrace_info_fn)(struct VGAState *s);
     uint8_t *vram_ptr;                                                  \
     ram_addr_t vram_offset;                                             \
     unsigned int vram_size;                                             \
+    uint32_t lfb_addr;                                                  \
+    uint32_t lfb_end;                                                   \
+    uint32_t map_addr;                                                  \
+    uint32_t map_end;                                                   \
     unsigned long bios_offset;                                          \
     unsigned int bios_size;                                             \
     target_phys_addr_t base_ctrl;                                       \
@@ -189,6 +193,10 @@ static inline int c6_to_8(int v)
 void vga_common_init(VGAState *s, DisplayState *ds, uint8_t *vga_ram_base,
                      unsigned long vga_ram_offset, int vga_ram_size);
 void vga_init(VGAState *s);
+
+void vga_dirty_log_start(VGAState *s);
+void vga_dirty_log_stop(VGAState *s);
+
 uint32_t vga_mem_readb(void *opaque, target_phys_addr_t addr);
 void vga_mem_writeb(void *opaque, target_phys_addr_t addr, uint32_t val);
 void vga_invalidate_scanlines(VGAState *s, int y1, int y2);
-- 
1.5.6.5

Re: [Qemu-devel] [PATCH 5/5] vga optimization.

[Anthony Liguori]

Glauber Costa wrote:
> +static void map_linear_vram(CirrusVGAState *s)
> +{
> +    int phys_offset = s->vga_io_memory;
> +
> +    if (!s->map_addr && s->lfb_addr && s->lfb_end) {
> +        s->map_addr = s->lfb_addr;
> +        s->map_end = s->lfb_end;
> +        cpu_register_physical_memory(s->map_addr, s->map_end - s->map_addr, s->vram_offset);
> +        vga_dirty_log_start((VGAState *)s);
>   

So you register this region and enable dirty tracking.

> +    }
> +
> +    if(!(s->cirrus_srcptr != s->cirrus_srcptr_end)
> +        && !((s->sr[0x07] & 0x01) == 0)
> +        && !((s->gr[0x0B] & 0x14) == 0x14)
> +        && !(s->gr[0x0B] & 0x02)) {
> +        phys_offset = s->vram_offset | IO_MEM_RAM;
> +    }
> +    cpu_register_physical_memory(isa_mem_base + 0xa0000, 0x20000, phys_offset);
>   

But also potentially change 0xa0000..0xc0000 to RAM, but you don't 
enable dirty tracking on this region.  I think you either have to leave 
this region as MMIO or enable dirty tracking on it too.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 5/5] vga optimization.

[Stefano Stabellini]

Anthony Liguori wrote:
>Glauber Costa wrote:
>> +static void map_linear_vram(CirrusVGAState *s)
>> +{
>> +    int phys_offset = s->vga_io_memory;
>> +
>> +    if (!s->map_addr && s->lfb_addr && s->lfb_end) {
>> +        s->map_addr = s->lfb_addr;
>> +        s->map_end = s->lfb_end;
>> +        cpu_register_physical_memory(s->map_addr, s->map_end - s->map_addr, s->vram_offset);
>> +        vga_dirty_log_start((VGAState *)s);
>>   
>
>So you register this region and enable dirty tracking.

I don't think it should be enabled here.

>> +    }
>> +
>> +    if(!(s->cirrus_srcptr != s->cirrus_srcptr_end)
>> +        && !((s->sr[0x07] & 0x01) == 0)
>> +        && !((s->gr[0x0B] & 0x14) == 0x14)
>> +        && !(s->gr[0x0B] & 0x02)) {
>> +        phys_offset = s->vram_offset | IO_MEM_RAM;
>> +    }
>> +    cpu_register_physical_memory(isa_mem_base + 0xa0000, 0x20000, phys_offset);
>>   
>
>But also potentially change 0xa0000..0xc0000 to RAM, but you don't 
>enable dirty tracking on this region.  I think you either have to leave 
>this region as MMIO or enable dirty tracking on it too.
>

To be honest I don't understand why this is part is needed.

@@ -2102,6 +2122,11 @@ static void vga_map(PCIDevice *pci_dev, int region_num,
     } else {
         cpu_register_physical_memory(addr, s->vram_size, s->vram_offset);
     }
+
+    s->map_addr = addr;
+    s->map_end = addr + VGA_RAM_SIZE;
+
+    vga_dirty_log_start(s);
 }

I think you should set lfb_addr and lfb_end here instead of map_addr and
map_end.

Re: [Qemu-devel] [PATCH 5/5] vga optimization.

[Glauber Costa]

On Tue, Nov 11, 2008 at 1:33 PM, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> Anthony Liguori wrote:
>>Glauber Costa wrote:
>>> +static void map_linear_vram(CirrusVGAState *s)
>>> +{
>>> +    int phys_offset = s->vga_io_memory;
>>> +
>>> +    if (!s->map_addr && s->lfb_addr && s->lfb_end) {
>>> +        s->map_addr = s->lfb_addr;
>>> +        s->map_end = s->lfb_end;
>>> +        cpu_register_physical_memory(s->map_addr, s->map_end - s->map_addr, s->vram_offset);
>>> +        vga_dirty_log_start((VGAState *)s);
>>>
>>
>>So you register this region and enable dirty tracking.
>
> I don't think it should be enabled here.
>
>>> +    }
>>> +
>>> +    if(!(s->cirrus_srcptr != s->cirrus_srcptr_end)
>>> +        && !((s->sr[0x07] & 0x01) == 0)
>>> +        && !((s->gr[0x0B] & 0x14) == 0x14)
>>> +        && !(s->gr[0x0B] & 0x02)) {
>>> +        phys_offset = s->vram_offset | IO_MEM_RAM;
>>> +    }
>>> +    cpu_register_physical_memory(isa_mem_base + 0xa0000, 0x20000, phys_offset);
>>>
>>
>>But also potentially change 0xa0000..0xc0000 to RAM, but you don't
>>enable dirty tracking on this region.  I think you either have to leave
>>this region as MMIO or enable dirty tracking on it too.
>>
>
> To be honest I don't understand why this is part is needed.
>
> @@ -2102,6 +2122,11 @@ static void vga_map(PCIDevice *pci_dev, int region_num,
>     } else {
>         cpu_register_physical_memory(addr, s->vram_size, s->vram_offset);
>     }
> +
> +    s->map_addr = addr;
> +    s->map_end = addr + VGA_RAM_SIZE;
> +
> +    vga_dirty_log_start(s);
>  }
>
> I think you should set lfb_addr and lfb_end here instead of map_addr and
> map_end.
>
It is possible for the lfb to be set, but not mapped.
So map_addr is what really indicates we're interested in tracking this region.
Or I am totally wrong?



-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [Qemu-devel] [PATCH 5/5] vga optimization.

[Stefano Stabellini]

Glauber Costa wrote:

>> @@ -2102,6 +2122,11 @@ static void vga_map(PCIDevice *pci_dev, int region_num,
>>     } else {
>>         cpu_register_physical_memory(addr, s->vram_size, s->vram_offset);
>>     }
>> +
>> +    s->map_addr = addr;
>> +    s->map_end = addr + VGA_RAM_SIZE;
>> +
>> +    vga_dirty_log_start(s);
>>  }
>>
>> I think you should set lfb_addr and lfb_end here instead of map_addr and
>> map_end.
>>
> It is possible for the lfb to be set, but not mapped.
> So map_addr is what really indicates we're interested in tracking this region.
> Or I am totally wrong?
> 

Yes, you are right.
However these changes are in vga_map that is the pci_register_io_region
function, much like cirrus_pci_lfb_map for cirrus.
They are supposed to set the lfb_addr before we remap it.

Besides I just noticed that you should set lfb_addr only in case that
region_num != PCI_ROM_SLOT.