Re: [Qemu-devel] [PATCH 0/4] VGA optimization

Re: [Qemu-devel] [PATCH 0/4] VGA optimization

[Glauber Costa]

On Mon, Nov 10, 2008 at 4:37 PM, Glauber Costa <glommer@redhat.com> wrote:
> hey guys,
>
> I hope this is the last version (Of course, once this is merged,
> the optimizations of the optimization can start ;-) )
>
> I split it in 4 patches. The first two ones are just moving
> things out of the way, and then #3 and #4 do the real thing.
> #3 kvm-side, #4 overall qemu.
>
> They merge most of the suggestion Anthony and Stefano's sent
> on last iteration.
>
> Hope you like it.
I'm so smart, that I forgot the -n option to git _again_

move vga_io_address to VGA State is first
de-register mem region for MMIO is second
Introduce KVM logging interface, third
and vga optimization is fourth

sorry
>
>
>
>
>



-- 
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] Introduce kvm logging interface.

[Anthony Liguori]

Glauber Costa wrote:
> 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 |  140 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  kvm.h     |    5 ++
>  2 files changed, 145 insertions(+), 0 deletions(-)
>
> diff --git a/kvm-all.c b/kvm-all.c
> index 6d50609..c2c253f 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -31,8 +31,15 @@
>      do { } while (0)
>  #endif
>  
> +#define warning(fmt, ...) \
> +    do { fprintf(stderr, "%s:%d" fmt, __func__, __LINE__, ## __VA_ARGS__); } while (0)
>   

I'd rather this be a dprintf().  Error paths should not be chatty for 
normal users because then a malicious can potentially start a crap-flood.

>  typedef struct kvm_userspace_memory_region KVMSlot;
>  
> +#define kvm_uaddr(addr) ((addr) + (ram_addr_t)phys_ram_base)
>   

Should always be a static inline when possible, but I'm suspicious that 
this function should even exist.

> +typedef struct kvm_dirty_log KVMDirty;
>   

KVMDirtyLog?

>  int kvm_allowed = 0;
>  
>  struct KVMState
> @@ -71,6 +78,24 @@ static KVMSlot *kvm_lookup_slot(KVMState *s, target_phys_addr_t start_addr)
>      return NULL;
>  }
>  
> +/* find the slot correspondence using userspace_addr as a key */
> +static KVMSlot *kvm_lookup_slot_uaddr(KVMState *s, ram_addr_t addr)
> +{
> +    int i;
> +    
> +    uint64_t uaddr = (uint64_t)kvm_uaddr(addr);
>   

Since this is the only use of kvm_uaddr, you could just fold the #define 
into here.

> +void kvm_physical_sync_dirty_bitmap(ram_addr_t start_addr, ram_addr_t end_addr)
> +{
> +    KVMState *s = kvm_state;
> +    KVMSlot *mem = kvm_lookup_slot_uaddr(s, start_addr);
> +    KVMDirty d;
> +    unsigned long alloc_size = mem->memory_size >> TARGET_PAGE_BITS / sizeof(d.dirty_bitmap);
> +    ram_addr_t addr;
> +
> +    dprintf("sync addr: %lx %llx %llx\n", start_addr, mem->guest_phys_addr, kvm_uaddr(start_addr));
> +    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;
> +    }
> +    
> +    for (addr = start_addr; addr < end_addr; addr += TARGET_PAGE_SIZE){
> +        unsigned nr;
> +        nr = (uint32_t)((uint64_t)kvm_uaddr(addr) - mem->userspace_addr) >> TARGET_PAGE_BITS;
> +        phys_ram_dirty[addr >> TARGET_PAGE_BITS] |= lookup_bitmap_phys((unsigned long *)d.dirty_bitmap, nr);
>   

This is only setting the VGA_DIRTY_FLAG and really only by happy 
coincidence.  If that was on purpose, then shame on you ;-)  You need to 
set all bits in order for live migration to work.

You should use cpu_physical_memory_set_dirty().  I also think you could 
simplify things by folding lookup_bitmap_phys() into this function but 
that's not a requirement for merging.

> +    }
> +out:
> +    qemu_free(d.dirty_bitmap);
> +}
> +
>  int kvm_init(int smp_cpus)
>  {
>      KVMState *s;
> diff --git a/kvm.h b/kvm.h
> index 37102b4..39e9048 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);
>  
> +int kvm_physical_memory_get_dirty(ram_addr_t addr);
>   

This function no longer exists.

If you fix these issues, then I think it's ready for merging.

Regards,

Anthony Liguori

> +void kvm_physical_sync_dirty_bitmap(ram_addr_t start_addr, ram_addr_t end_addr);
> +
> +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;
>   

Re: [Qemu-devel] [PATCH] Introduce kvm logging interface.

[Anthony Liguori]

Glauber Costa wrote:
> 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>
> ---
>
> +/* find the slot correspondence using userspace_addr as a key */
> +static KVMSlot *kvm_lookup_slot_uaddr(KVMState *s, ram_addr_t addr)
> +{
> +    int i;
> +    
> +    uint64_t uaddr = (uint64_t)kvm_uaddr(addr);
>   

warning: cast from pointer to integer of different size

(64-bit host, pointer is unsigned long, uint64_t is unsigned long long).

> +        nr = (uint32_t)((uint64_t)kvm_uaddr(addr) - mem->userspace_addr) >> TARGET_PAGE_BITS;
>   

warning: cast from pointer to integer of different size

Regards,

Anthony Liguori

[Qemu-devel] [PATCH 0/4] VGA optimization

[Glauber Costa]

hey guys,

I hope this is the last version (Of course, once this is merged,
the optimizations of the optimization can start ;-) )

I split it in 4 patches. The first two ones are just moving
things out of the way, and then #3 and #4 do the real thing.
#3 kvm-side, #4 overall qemu.

They merge most of the suggestion Anthony and Stefano's sent
on last iteration.

Hope you like it.
 

[Qemu-devel] [PATCH] 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 55f3ced..6b407ef 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 82a755e..fbe5d64 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] 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] 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 |  140 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 kvm.h     |    5 ++
 2 files changed, 145 insertions(+), 0 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 6d50609..c2c253f 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -31,8 +31,15 @@
     do { } while (0)
 #endif
 
+#define warning(fmt, ...) \
+    do { fprintf(stderr, "%s:%d" fmt, __func__, __LINE__, ## __VA_ARGS__); } while (0)
+
 typedef struct kvm_userspace_memory_region KVMSlot;
 
+#define kvm_uaddr(addr) ((addr) + (ram_addr_t)phys_ram_base)
+
+typedef struct kvm_dirty_log KVMDirty;
+
 int kvm_allowed = 0;
 
 struct KVMState
@@ -71,6 +78,24 @@ static KVMSlot *kvm_lookup_slot(KVMState *s, target_phys_addr_t start_addr)
     return NULL;
 }
 
+/* find the slot correspondence using userspace_addr as a key */
+static KVMSlot *kvm_lookup_slot_uaddr(KVMState *s, ram_addr_t addr)
+{
+    int i;
+    
+    uint64_t uaddr = (uint64_t)kvm_uaddr(addr);
+
+    for (i = 0; i < ARRAY_SIZE(s->slots); i++) {
+        KVMSlot *mem = &s->slots[i];
+
+        if (uaddr >= mem->userspace_addr &&
+            uaddr < (mem->userspace_addr + mem->memory_size))
+            return mem;
+    }
+
+    return NULL;
+}
+
 int kvm_init_vcpu(CPUState *env)
 {
     KVMState *s = kvm_state;
@@ -109,6 +134,121 @@ 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;
+}
+
+void kvm_physical_sync_dirty_bitmap(ram_addr_t start_addr, ram_addr_t end_addr)
+{
+    KVMState *s = kvm_state;
+    KVMSlot *mem = kvm_lookup_slot_uaddr(s, start_addr);
+    KVMDirty d;
+    unsigned long alloc_size = mem->memory_size >> TARGET_PAGE_BITS / sizeof(d.dirty_bitmap);
+    ram_addr_t addr;
+
+    dprintf("sync addr: %lx %llx %llx\n", start_addr, mem->guest_phys_addr, kvm_uaddr(start_addr));
+    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;
+    }
+    
+    for (addr = start_addr; addr < end_addr; addr += TARGET_PAGE_SIZE){
+        unsigned nr;
+        nr = (uint32_t)((uint64_t)kvm_uaddr(addr) - mem->userspace_addr) >> TARGET_PAGE_BITS;
+        phys_ram_dirty[addr >> TARGET_PAGE_BITS] |= lookup_bitmap_phys((unsigned long *)d.dirty_bitmap, nr);
+    }
+out:
+    qemu_free(d.dirty_bitmap);
+}
+
 int kvm_init(int smp_cpus)
 {
     KVMState *s;
diff --git a/kvm.h b/kvm.h
index 37102b4..39e9048 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);
 
+int kvm_physical_memory_get_dirty(ram_addr_t addr);
+void kvm_physical_sync_dirty_bitmap(ram_addr_t start_addr, ram_addr_t end_addr);
+
+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] 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          |    6 ++++++
 hw/cirrus_vga.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 hw/vga.c        |   25 +++++++++++++++++++++++++
 hw/vga_int.h    |   10 +++++++++-
 5 files changed, 85 insertions(+), 1 deletions(-)

diff --git a/cpu-all.h b/cpu-all.h
index cdd79bc..6e12847 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(ram_addr_t start_addr, ram_addr_t end_addr);
+
 void dump_exec_info(FILE *f,
                     int (*cpu_fprintf)(FILE *f, const char *fmt, ...));
 
diff --git a/exec.c b/exec.c
index ef1072b..975099a 100644
--- a/exec.c
+++ b/exec.c
@@ -1823,6 +1823,12 @@ int cpu_physical_memory_get_dirty_tracking(void)
     return in_migration;
 }
 
+void cpu_physical_sync_dirty_bitmap(ram_addr_t start_addr, ram_addr_t end_addr)
+{
+    if (kvm_enabled())
+        kvm_physical_sync_dirty_bitmap(start_addr, end_addr);
+}
+
 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 6b407ef..63983ba 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 9540db0..df6179b 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->vram_offset, s->vram_offset + VGA_RAM_SIZE);
+    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 fbe5d64..d1d35f9 100644
--- a/hw/vga_int.h
+++ b/hw/vga_int.h
@@ -100,8 +100,12 @@ 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;                                             \
+    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 0/4] VGA optimization

[Anthony Liguori]

Glauber Costa wrote:
> hey guys,
>   

I gave you some bad advice that I think is causing the breakage I'm 
seeing now.  I suggested that you simply do a lookup to find the slot 
given a target_phys_addr_t but that isn't correct.  Let me explain why.

ram_addr_t represents a guest physical address.  From a ram_addr_t you 
can get a target_phys_addr_t.  Sometimes these are the same but they 
aren't always.

You can have multiple ram_addr_t's pointing to the same 
target_phys_addr_t.  This is ram aliasing and it happens for a variety 
of reasons.  In general, it's pretty expensive to map a ram_addr_t to a 
target_phys_addr_t because, among other things, for a range of 
(ram_addr_t, size_t), you may have many (target_phys_addr_t, size) 
tuples that you have to deal with.

vga_common_init() takes a target_phys_addr_t (well, it really takes an 
unsigned long, but that's a bug).  It takes this as an optimization.  It 
avoids having to do the conversion and ensures that it's one big linear 
region.

For dirty tracking, we have a bitmap indexed by target_phys_addr_t in 
QEMU.  This means that we can happily set dirty bits based on 
target_phys_addr_t's.  We don't have to worry about what ram_addr_t it 
came from because they all map to the same bits.

Since KVM uses a slot API, and that API is indexed in ram_addr_t's, we 
need to enable dirty tracking on the ram_addr_t's.  We don't have a 
ram_addr_t in the VGA code.

The solution is pretty simple.  We need to keep track of the 
ram_addr_t's in the VGA code and enable dirty tracking on the 
appropriate ram_addr_ts.

Regards,

Anthony Liguori

> I hope this is the last version (Of course, once this is merged,
> the optimizations of the optimization can start ;-) )
>
> I split it in 4 patches. The first two ones are just moving
> things out of the way, and then #3 and #4 do the real thing.
> #3 kvm-side, #4 overall qemu.
>
> They merge most of the suggestion Anthony and Stefano's sent
> on last iteration.
>
> Hope you like it.
>  
>
>
>
>   

Re: [Qemu-devel] [PATCH 0/4] VGA optimization

[Anthony Liguori]

Anthony Liguori wrote:
> Glauber Costa wrote:
>> hey guys,
>>   
>
> I gave you some bad advice that I think is causing the breakage I'm 
> seeing now.  I suggested that you simply do a lookup to find the slot 
> given a target_phys_addr_t but that isn't correct.  Let me explain why.

Except I'm completely backwards but at least consistently backwards :-)

Regards,

Anthony Liguori

> ram_addr_t represents a guest physical address.  From a ram_addr_t you 
> can get a target_phys_addr_t.  Sometimes these are the same but they 
> aren't always.
>
> You can have multiple ram_addr_t's pointing to the same 
> target_phys_addr_t.  This is ram aliasing and it happens for a variety 
> of reasons.  In general, it's pretty expensive to map a ram_addr_t to 
> a target_phys_addr_t because, among other things, for a range of 
> (ram_addr_t, size_t), you may have many (target_phys_addr_t, size) 
> tuples that you have to deal with.
>
> vga_common_init() takes a target_phys_addr_t (well, it really takes an 
> unsigned long, but that's a bug).  It takes this as an optimization.  
> It avoids having to do the conversion and ensures that it's one big 
> linear region.
>
> For dirty tracking, we have a bitmap indexed by target_phys_addr_t in 
> QEMU.  This means that we can happily set dirty bits based on 
> target_phys_addr_t's.  We don't have to worry about what ram_addr_t it 
> came from because they all map to the same bits.
>
> Since KVM uses a slot API, and that API is indexed in ram_addr_t's, we 
> need to enable dirty tracking on the ram_addr_t's.  We don't have a 
> ram_addr_t in the VGA code.
>
> The solution is pretty simple.  We need to keep track of the 
> ram_addr_t's in the VGA code and enable dirty tracking on the 
> appropriate ram_addr_ts.
>
> Regards,
>
> Anthony Liguori
>
>> I hope this is the last version (Of course, once this is merged,
>> the optimizations of the optimization can start ;-) )
>>
>> I split it in 4 patches. The first two ones are just moving
>> things out of the way, and then #3 and #4 do the real thing.
>> #3 kvm-side, #4 overall qemu.
>>
>> They merge most of the suggestion Anthony and Stefano's sent
>> on last iteration.
>>
>> Hope you like it.
>>  
>>
>>
>>
>>   
>

Re: [Qemu-devel] [PATCH 0/4] VGA optimization

[Glauber Costa]

On Mon, Nov 10, 2008 at 4:47 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> Anthony Liguori wrote:
>>
>> Glauber Costa wrote:
>>>
>>> hey guys,
>>>
>>
>> I gave you some bad advice that I think is causing the breakage I'm seeing
>> now.  I suggested that you simply do a lookup to find the slot given a
>> target_phys_addr_t but that isn't correct.  Let me explain why.
>
> Except I'm completely backwards but at least consistently backwards :-)

Which is amazing ;-) Your text made perfect sense after a sed line ;-)




-- 
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 0/4] VGA optimization

[Glauber Costa]

On Mon, Nov 10, 2008 at 4:39 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> Glauber Costa wrote:
>>
>> hey guys,
>>
>
> I gave you some bad advice that I think is causing the breakage I'm seeing
> now.  I suggested that you simply do a lookup to find the slot given a
> target_phys_addr_t but that isn't correct.  Let me explain why.
>
> ram_addr_t represents a guest physical address.  From a ram_addr_t you can
> get a target_phys_addr_t.  Sometimes these are the same but they aren't
> always.
>
> You can have multiple ram_addr_t's pointing to the same target_phys_addr_t.
>  This is ram aliasing and it happens for a variety of reasons.  In general,
> it's pretty expensive to map a ram_addr_t to a target_phys_addr_t because,
> among other things, for a range of (ram_addr_t, size_t), you may have many
> (target_phys_addr_t, size) tuples that you have to deal with.
>
> vga_common_init() takes a target_phys_addr_t (well, it really takes an
> unsigned long, but that's a bug).  It takes this as an optimization.  It
> avoids having to do the conversion and ensures that it's one big linear
> region.
>
> For dirty tracking, we have a bitmap indexed by target_phys_addr_t in QEMU.
>  This means that we can happily set dirty bits based on
> target_phys_addr_t's.  We don't have to worry about what ram_addr_t it came
> from because they all map to the same bits.
>
> Since KVM uses a slot API, and that API is indexed in ram_addr_t's, we need
> to enable dirty tracking on the ram_addr_t's.  We don't have a ram_addr_t in
> the VGA code.
>
> The solution is pretty simple.  We need to keep track of the ram_addr_t's in
> the VGA code and enable dirty tracking on the appropriate ram_addr_ts.
>
> Regards,

My proposal is to let kvm interface to be:
void kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
target_phys_addr_t end_addr,
                                    ram_addr_t phys_offset)

to match, qemu then becomes:
void cpu_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
target_phys_addr_t end_addr,
                                    ram_addr_t phys_offset)

the caller does:

cpu_physical_sync_dirty_bitmap(s->map_addr, s->map_end, s->vram_offset);

This way, we tell kvm which particular area of the phys_ram_bitmap to
fill. I liked this
interface, because then we can have the behaviour of not synchronizing
back those bits
into qemu, by passing NULL as the third parameter.

I have a working version of it, will send shortly.




> Anthony Liguori
>
>> I hope this is the last version (Of course, once this is merged,
>> the optimizations of the optimization can start ;-) )
>>
>> I split it in 4 patches. The first two ones are just moving
>> things out of the way, and then #3 and #4 do the real thing.
>> #3 kvm-side, #4 overall qemu.
>>
>> They merge most of the suggestion Anthony and Stefano's sent
>> on last iteration.
>>
>> Hope you like it.
>>
>>
>>
>>
>
>
>
>



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

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

[Qemu-devel] [PATCH 0/4] VGA optimization

[Glauber Costa]

You know the deal: This is the vga optimization all over again!

Some patches were already commited, this version only carries 4 of them.
The first two are just cleanups to make it fit.

The third is for kvm, and the fourth, for general qemu.

Note that unfortunately, it won't work for everybody: kvm host
kernel has a bug that prevents it, so whoever wants to test it,
will need a patched kernel (until the patch gets upstream kvm
and you upgrade your kernel).

Re: [Qemu-devel] [PATCH 0/4] VGA optimization

[Anthony Liguori]

Glauber Costa wrote:
> You know the deal: This is the vga optimization all over again!
>
> Some patches were already commited, this version only carries 4 of them.
> The first two are just cleanups to make it fit.
>
> The third is for kvm, and the fourth, for general qemu.
>
> Note that unfortunately, it won't work for everybody: kvm host
> kernel has a bug that prevents it, so whoever wants to test it,
> will need a patched kernel (until the patch gets upstream kvm
> and you upgrade your kernel).
>   

Applied all.  Thanks.

Regards,

Anthony Liguori

>
>
>
>