[Qemu-devel] vga optmization

[Qemu-devel] vga optmization

[Glauber Costa]

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

Hi guys,

this is a port of current kvm vga memory optimization to our new
infrastructure proposed by anthony. It's goal is to use as few
kvm specific hooks as possible. In fact, the only one I'm relying
on is enabling/disabling of logging. The rest, is pretty much general.

We map the linear frame buffer area as RAM, and then use dirty tracking
to decide whether or not to update it. To be consistent with qemu,
this version, differently from upstream kvm, tracks memory based on its
physical address, represented by vram_offset, instead of vram_ptr, or
any other construct.

Let me know what you think


[-- Attachment #2: vga-optimization.patch --]
[-- Type: text/plain, Size: 17681 bytes --]

diff --git a/cpu-all.h b/cpu-all.h
index cdd79bc..9118f4d 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -46,6 +46,8 @@
 
 #ifdef BSWAP_NEEDED
 
+#include "kvm.h"
+
 static inline uint16_t tswap16(uint16_t s)
 {
     return bswap16(s);
@@ -909,17 +911,10 @@ int cpu_memory_rw_debug(CPUState *env, target_ulong addr,
 #define KQEMU_DIRTY_FLAG     0x04
 #define MIGRATION_DIRTY_FLAG 0x08
 
-/* read dirty bit (return 0 or 1) */
-static inline int cpu_physical_memory_is_dirty(ram_addr_t addr)
-{
-    return phys_ram_dirty[addr >> TARGET_PAGE_BITS] == 0xff;
-}
+int cpu_physical_memory_get_dirty(ram_addr_t addr, int dirty_flags);
+int cpu_physical_memory_is_dirty(ram_addr_t addr);
 
-static inline int cpu_physical_memory_get_dirty(ram_addr_t addr,
-                                                int dirty_flags)
-{
-    return phys_ram_dirty[addr >> TARGET_PAGE_BITS] & dirty_flags;
-}
+void qemu_physical_sync_dirty_bitmap(ram_addr_t start_addr);
 
 static inline void cpu_physical_memory_set_dirty(ram_addr_t addr)
 {
diff --git a/exec.c b/exec.c
index ef1072b..4ea145e 100644
--- a/exec.c
+++ b/exec.c
@@ -1818,11 +1818,40 @@ int cpu_physical_memory_set_dirty_tracking(int enable)
     return 0;
 }
 
+int cpu_physical_memory_get_dirty(ram_addr_t addr,
+                                                int dirty_flags)
+{
+    int is_dirty = 0;
+    is_dirty = phys_ram_dirty[addr >> TARGET_PAGE_BITS] & dirty_flags;
+    if (is_dirty)
+        goto out;
+#ifdef CONFIG_KVM
+    if (kvm_enabled())
+        is_dirty = kvm_physical_memory_get_dirty(addr);
+        /* to make it usable below */
+        is_dirty = !!is_dirty * 0xff;
+#endif
+out:
+    return is_dirty;
+}
+
+/* read dirty bit (return 0 or 1) */
+int cpu_physical_memory_is_dirty(ram_addr_t addr)
+{
+    return cpu_physical_memory_get_dirty(addr, 0xff) == 0xff;
+}
+
 int cpu_physical_memory_get_dirty_tracking(void)
 {
     return in_migration;
 }
 
+void qemu_physical_sync_dirty_bitmap(ram_addr_t start_addr)
+{
+    if (kvm_enabled())
+        kvm_physical_sync_dirty_bitmap(start_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 af9c9e6..27be4a7 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:
@@ -248,6 +249,9 @@ typedef struct CirrusVGAState {
     int cirrus_linear_io_addr;
     int cirrus_linear_bitblt_io_addr;
     int cirrus_mmio_io_addr;
+    uint32_t cirrus_lfb_addr;
+    uint32_t cirrus_lfb_end;
+    uint32_t cirrus_lfb_mapped;
     uint32_t cirrus_addr_mask;
     uint32_t linear_mmio_mask;
     uint8_t cirrus_shadow_gr0;
@@ -2618,6 +2622,69 @@ static CPUWriteMemoryFunc *cirrus_linear_bitblt_write[3] = {
     cirrus_linear_bitblt_writel,
 };
 
+void set_vram_mapping(target_phys_addr_t begin, target_phys_addr_t end, ram_addr_t target)
+{
+    /* align begin and end address */
+    begin = begin & TARGET_PAGE_MASK;
+    end = begin + VGA_RAM_SIZE;
+    end = (end + TARGET_PAGE_SIZE -1 ) & TARGET_PAGE_MASK;
+
+    cpu_register_physical_memory(begin, end - begin, target);
+
+    if (kvm_enabled()) {
+        /* XXX: per-slot dirty tracking in qemu may get rid of it */
+        kvm_log_start(begin, end - begin);
+    }
+}
+
+void unset_vram_mapping(target_phys_addr_t begin, target_phys_addr_t end)
+{
+    /* align begin and end address */
+    end = begin + VGA_RAM_SIZE;
+    begin = begin & TARGET_PAGE_MASK;
+    end = (end + TARGET_PAGE_SIZE -1 ) & TARGET_PAGE_MASK;
+
+    if (kvm_enabled()) {
+        /* XXX: per-slot dirty tracking in qemu may get rid of it */
+        kvm_log_stop(begin, end - begin);
+    }
+    cpu_register_physical_memory(begin, end - begin, IO_MEM_UNASSIGNED);
+
+}
+
+static void map_linear_vram(CirrusVGAState *s)
+{
+
+    if (!s->cirrus_lfb_mapped && s->cirrus_lfb_addr && s->cirrus_lfb_end) {
+        set_vram_mapping(s->cirrus_lfb_addr, s->cirrus_lfb_end, s->vram_offset);
+        s->cirrus_lfb_mapped = 1;
+    }
+    
+    if(!(s->cirrus_srcptr != s->cirrus_srcptr_end)
+        && !((s->sr[0x07] & 0x01) == 0)
+        && !((s->gr[0x0B] & 0x14) == 0x14)
+        && !(s->gr[0x0B] & 0x02)) {
+        cpu_register_physical_memory(isa_mem_base + 0xa0000, 0x20000, s->vram_offset | IO_MEM_RAM);
+    }
+    else {
+        cpu_register_physical_memory(isa_mem_base + 0xa0000, 0x20000, s->vga_io_memory);
+    }
+}
+
+void unmap_linear_vram(CirrusVGAState *s)
+{
+
+    if (s->cirrus_lfb_mapped && s->cirrus_lfb_addr && s->cirrus_lfb_end) {
+        unset_vram_mapping(s->cirrus_lfb_addr,
+                           s->cirrus_lfb_end);
+        s->cirrus_lfb_mapped = 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)
 {
@@ -2636,11 +2703,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;
@@ -3117,7 +3186,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) {
@@ -3156,10 +3225,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) {
@@ -3258,9 +3327,13 @@ static void cirrus_pci_lfb_map(PCIDevice *d, int region_num,
 
     /* XXX: add byte swapping apertures */
     cpu_register_physical_memory(addr, s->vram_size,
-				 s->cirrus_linear_io_addr);
+				 s->cirrus_linear_io_addr | s->vram_offset);
     cpu_register_physical_memory(addr + 0x1000000, 0x400000,
 				 s->cirrus_linear_bitblt_io_addr);
+
+    s->cirrus_lfb_mapped = 0;
+    s->cirrus_lfb_addr = addr;
+    s->cirrus_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..275ef0a 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
@@ -1568,6 +1569,8 @@ static void vga_draw_graphic(VGAState *s, int full_update)
     uint32_t v, addr1, addr;
     vga_draw_line_func *vga_draw_line;
 
+    qemu_physical_sync_dirty_bitmap(s->vram_offset);
+
     full_update |= update_basic_params(s);
 
     s->get_resolution(s, &width, &height);
@@ -2102,6 +2105,9 @@ static void vga_map(PCIDevice *pci_dev, int region_num,
     } else {
         cpu_register_physical_memory(addr, s->vram_size, s->vram_offset);
     }
+
+    if (kvm_enabled())
+        kvm_log_start(addr, VGA_RAM_SIZE);
 }
 
 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 82a755e..7d3c011 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;                                             \
@@ -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;                                                \
+    uint32_t vga_io_memory;                                             \
     int (*get_bpp)(struct VGAState *s);                                 \
     void (*get_offsets)(struct VGAState *s,                             \
                         uint32_t *pline_offset,                         \
diff --git a/kvm-all.c b/kvm-all.c
index 4379071..4916865 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -31,7 +31,18 @@
     do { } while (0)
 #endif
 
-typedef struct kvm_userspace_memory_region KVMSlot;
+#define warning(fmt, ...) \
+    do { fprintf(stderr, "%s:%d" fmt, __func__, __LINE__, ## __VA_ARGS__); } while (0)
+
+typedef struct KVMSlot {
+        struct kvm_userspace_memory_region region;
+        int logging_count;
+        uint64_t *dirty_bitmap;
+} KVMSlot;
+
+#define kvm_uaddr(addr) ((addr) + (ram_addr_t)phys_ram_base)
+
+typedef struct kvm_dirty_log KVMDirty;
 
 int kvm_allowed = 0;
 
@@ -49,7 +60,7 @@ static KVMSlot *kvm_alloc_slot(KVMState *s)
     int i;
 
     for (i = 0; i < ARRAY_SIZE(s->slots); i++) {
-        if (s->slots[i].memory_size == 0)
+        if (s->slots[i].region.memory_size == 0)
             return &s->slots[i];
     }
 
@@ -63,8 +74,26 @@ static KVMSlot *kvm_lookup_slot(KVMState *s, target_phys_addr_t start_addr)
     for (i = 0; i < ARRAY_SIZE(s->slots); i++) {
         KVMSlot *mem = &s->slots[i];
 
-        if (start_addr >= mem->guest_phys_addr &&
-            start_addr < (mem->guest_phys_addr + mem->memory_size))
+        if (start_addr >= mem->region.guest_phys_addr &&
+            start_addr < (mem->region.guest_phys_addr + mem->region.memory_size))
+            return mem;
+    }
+
+    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->region.userspace_addr &&
+            uaddr < (mem->region.userspace_addr + mem->region.memory_size))
             return mem;
     }
 
@@ -109,6 +138,133 @@ 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->region.flags & ~mask) | flags;
+    if (flags == mem->region.flags)
+            return 0;
+
+    mem->region.flags = flags;
+
+    r = kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem->region);
+    if (r == -1)
+        fprintf(stderr, "%s: %m\n", __FUNCTION__);
+
+    return r;
+}
+
+#define BMAP_SIZE(mem) ((mem->region.memory_size >> TARGET_PAGE_BITS)/ sizeof(*mem->dirty_bitmap))
+
+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);
+
+        dprintf("try log start %llx len %llx\n", phys_addr, len);
+
+        if (mem == NULL)  {
+                fprintf(stderr, "BUG: %s: invalid parameters\n", __func__);
+                return -EINVAL;
+        }
+
+        if (mem->logging_count++)
+                return 0;
+
+        mem->dirty_bitmap = malloc(BMAP_SIZE(mem));  
+
+        if (mem->dirty_bitmap == NULL)
+            return -ENOMEM;
+
+        dprintf("slot %d: enable logging (phys %llx, uaddr %llx)\n",
+                 mem->region.slot, mem->region.guest_phys_addr, mem->region.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;
+        }
+        dprintf("slot %d: disabling logging\n", mem->region.slot);
+
+        if (mem->logging_count--)
+                return 0;
+
+        return kvm_dirty_pages_log_change(mem,
+                                          0,
+                                          KVM_MEM_LOG_DIRTY_PAGES);
+}
+
+#define BITMAP_WORD(mem) (sizeof(*(mem)->dirty_bitmap) * 8)
+static inline int lookup_bitmap_phys(KVMSlot *mem, ram_addr_t addr)
+{
+    unsigned nr = (uint32_t)((uint64_t)kvm_uaddr(addr) - mem->region.userspace_addr) >> TARGET_PAGE_BITS;
+    unsigned word = nr / BITMAP_WORD(mem);
+    unsigned bit = nr % BITMAP_WORD(mem);
+    int ret;
+
+    ret = (mem->dirty_bitmap[word] >> bit) & 1;
+    return ret;
+}
+
+int kvm_physical_memory_get_dirty(ram_addr_t addr)
+{
+    KVMState *s = kvm_state;
+    KVMSlot *mem = kvm_lookup_slot_uaddr(s, addr);
+
+    if (mem == NULL) {
+            fprintf(stderr, "BUG: %s: invalid parameters\n", __func__);
+            return -EINVAL;
+    }
+
+    if (mem->dirty_bitmap == NULL) {
+        return 0;
+    }
+    return lookup_bitmap_phys(mem, addr);
+}
+
+void kvm_physical_sync_dirty_bitmap(ram_addr_t start_addr)
+{
+    KVMState *s = kvm_state;
+    KVMSlot *mem = kvm_lookup_slot_uaddr(s, start_addr);
+    
+    KVMDirty d;
+
+    dprintf("sync addr: %lx %llx %llx\n", start_addr, mem->region.guest_phys_addr, kvm_uaddr(start_addr));
+    if (mem == NULL) {
+            fprintf(stderr, "BUG: %s: invalid parameters\n", __func__);
+            return;
+    }
+
+    if (mem->dirty_bitmap == NULL)
+        warning("Asked to sync dirty bitmap for region with logging disabled\n");
+
+    d.slot = mem->region.slot;
+    d.dirty_bitmap = mem->dirty_bitmap;
+    dprintf("slot %d, phys_addr %llx, uaddr: %llx\n",
+            d.slot, mem->region.guest_phys_addr, mem->region.userspace_addr);
+
+    if (kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d) == -1)
+        warning("ioctl failed %d\n", errno);
+    
+}
+
 int kvm_init(int smp_cpus)
 {
     KVMState *s;
@@ -123,7 +279,7 @@ int kvm_init(int smp_cpus)
         return -ENOMEM;
 
     for (i = 0; i < ARRAY_SIZE(s->slots); i++)
-        s->slots[i].slot = i;
+        s->slots[i].region.slot = i;
 
     s->vmfd = -1;
     s->fd = open("/dev/kvm", O_RDWR);
@@ -317,15 +473,18 @@ 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) {
-            mem->memory_size = 0;
-            mem->guest_phys_addr = start_addr;
-            mem->userspace_addr = 0;
-            mem->flags = 0;
-
-            kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, mem);
-        } else if (start_addr >= mem->guest_phys_addr &&
-                   (start_addr + size) <= (mem->guest_phys_addr + mem->memory_size))
+        if ((flags == IO_MEM_UNASSIGNED) || (flags >= TLB_MMIO)) {
+            if (mem->region.flags == KVM_MEM_LOG_DIRTY_PAGES)
+                return;
+            dprintf("deleting memory %llx with flags %d\n", mem->region.guest_phys_addr, mem->region.flags);
+            mem->region.memory_size = 0;
+            mem->region.guest_phys_addr = start_addr;
+            mem->region.userspace_addr = 0;
+            mem->region.flags = 0;
+
+            kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem->region);
+        } else if (start_addr >= mem->region.guest_phys_addr &&
+                   (start_addr + size) <= (mem->region.guest_phys_addr + mem->region.memory_size))
             return;
     }
 
@@ -334,12 +493,12 @@ void kvm_set_phys_mem(target_phys_addr_t start_addr,
         return;
 
     mem = kvm_alloc_slot(s);
-    mem->memory_size = size;
-    mem->guest_phys_addr = start_addr;
-    mem->userspace_addr = (unsigned long)(phys_ram_base + phys_offset);
-    mem->flags = 0;
+    mem->region.memory_size = size;
+    mem->region.guest_phys_addr = start_addr;
+    mem->region.userspace_addr = (unsigned long)(phys_ram_base + phys_offset);
+    mem->region.flags = 0;
 
-    kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, mem);
+    kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem->region);
     /* FIXME deal with errors */
 }
 
diff --git a/kvm.h b/kvm.h
index 37102b4..6e2d9d2 100644
--- a/kvm.h
+++ b/kvm.h
@@ -38,6 +38,9 @@ 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);
+
 /* internal API */
 
 struct KVMState;

Re: [Qemu-devel] vga optmization

[Stefano Stabellini]

Can we try to avoid any if (kvm_enabled()) in the vga code and use some
kind of more generic hook instead?
Xen has some very similar changes to the vga code, it would be nice to
merge them as well.

Glauber Costa wrote:

> Hi guys,
> 
> this is a port of current kvm vga memory optimization to our new
> infrastructure proposed by anthony. It's goal is to use as few
> kvm specific hooks as possible. In fact, the only one I'm relying
> on is enabling/disabling of logging. The rest, is pretty much general.
> 
> We map the linear frame buffer area as RAM, and then use dirty tracking
> to decide whether or not to update it. To be consistent with qemu,
> this version, differently from upstream kvm, tracks memory based on its
> physical address, represented by vram_offset, instead of vram_ptr, or
> any other construct.
> 
> Let me know what you think
> 
> 

Re: [Qemu-devel] vga optmization

[Glauber Costa]

On Mon, Nov 3, 2008 at 3:43 PM, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> Can we try to avoid any if (kvm_enabled()) in the vga code and use some
> kind of more generic hook instead?
> Xen has some very similar changes to the vga code, it would be nice to
> merge them as well.

Yes, we could. But then we'd need to have qemu to view memory as slots.
It's worthy, but long term.

By the way, posting those xen changes would be awesome.
If we both can avoid using hv-specific hooks (which I did: there's
only one left), and
so do you, I'm sure we can come up with something very nice in the end
of the day.
>
> Glauber Costa wrote:
>
>> Hi guys,
>>
>> this is a port of current kvm vga memory optimization to our new
>> infrastructure proposed by anthony. It's goal is to use as few
>> kvm specific hooks as possible. In fact, the only one I'm relying
>> on is enabling/disabling of logging. The rest, is pretty much general.
>>
>> We map the linear frame buffer area as RAM, and then use dirty tracking
>> to decide whether or not to update it. To be consistent with qemu,
>> this version, differently from upstream kvm, tracks memory based on its
>> physical address, represented by vram_offset, instead of vram_ptr, or
>> any other construct.
>>
>> Let me know what you think
>>
>>
>
>
>
>
>



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

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

Re: [Qemu-devel] vga optmization

[Blue Swirl]

On 11/3/08, Glauber Costa <glommer@redhat.com> wrote:
> Hi guys,
>
>  this is a port of current kvm vga memory optimization to our new
>  infrastructure proposed by anthony. It's goal is to use as few
>  kvm specific hooks as possible. In fact, the only one I'm relying
>  on is enabling/disabling of logging. The rest, is pretty much general.
>
>  We map the linear frame buffer area as RAM, and then use dirty tracking
>  to decide whether or not to update it. To be consistent with qemu,
>  this version, differently from upstream kvm, tracks memory based on its
>  physical address, represented by vram_offset, instead of vram_ptr, or
>  any other construct.
>
>  Let me know what you think

The patch does not apply, because of the kvm files. What patches do I
need to apply first? I'd like to see how does the optimization apply
to TCX.

This also may mean that some of my comments below can be invalid.

>  +void set_vram_mapping(target_phys_addr_t begin, target_phys_addr_t end, ram_addr_t target)

No "static"?

>  +{
>  +    /* align begin and end address */
>  +    begin = begin & TARGET_PAGE_MASK;
>  +    end = begin + VGA_RAM_SIZE;
>  +    end = (end + TARGET_PAGE_SIZE -1 ) & TARGET_PAGE_MASK;

This will fail if "end" is at the last page of the memory.

>  +void unset_vram_mapping(target_phys_addr_t begin, target_phys_addr_t end)

No "static"?

>  +{
>  +    /* align begin and end address */
>  +    end = begin + VGA_RAM_SIZE;
>  +    begin = begin & TARGET_PAGE_MASK;
>  +    end = (end + TARGET_PAGE_SIZE -1 ) & TARGET_PAGE_MASK;

This will fail if "end" is at the last page of the memory.

>  +void unmap_linear_vram(CirrusVGAState *s)

No "static"?

>  +    uint32_t vga_io_memory;                                             \

cpu_register_io_memory() returns an "int".

Re: [Qemu-devel] vga optmization

[Stefano Stabellini]

Glauber Costa wrote:

> On Mon, Nov 3, 2008 at 3:43 PM, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
>> Can we try to avoid any if (kvm_enabled()) in the vga code and use some
>> kind of more generic hook instead?
>> Xen has some very similar changes to the vga code, it would be nice to
>> merge them as well.
> 
> Yes, we could. But then we'd need to have qemu to view memory as slots.
> It's worthy, but long term.
> 
> By the way, posting those xen changes would be awesome.
> If we both can avoid using hv-specific hooks (which I did: there's
> only one left), and
> so do you, I'm sure we can come up with something very nice in the end
> of the day.

I totally agree.
Posting those patches has been on my TODO list for quite some time, I
think that now is the time to increase the priority :)
I'll keep you posted.

Re: [Qemu-devel] vga optmization

[Fabrice Bellard]

Glauber Costa wrote:
> Hi guys,
> 
> this is a port of current kvm vga memory optimization to our new
> infrastructure proposed by anthony. It's goal is to use as few
> kvm specific hooks as possible. In fact, the only one I'm relying
> on is enabling/disabling of logging. The rest, is pretty much general.
> 
> We map the linear frame buffer area as RAM, and then use dirty tracking
> to decide whether or not to update it. To be consistent with qemu,
> this version, differently from upstream kvm, tracks memory based on its
> physical address, represented by vram_offset, instead of vram_ptr, or
> any other construct.
> 
> Let me know what you think

Why don't you modify the lower level QEMU dirty bits handling functions
to be consistent with the KVM dirty bits ? By doing that you can avoid
patching the device drivers and have smaller code. The fact that KVM use
physical memory addresses is not a problem if you can convert the ram
addresses to physical memory addresses (in most cases there is only one
physical address corresponding to one RAM address).

And if KVM does not use the dynamic translator during large amount of
time it might be worth bypassing most of the QEMU dirty bits handling
system to use only the KVM system in order to get better performance.

Regards,

Fabrice.

Re: [Qemu-devel] vga optmization

[Glauber Costa]

On Mon, Nov 3, 2008 at 4:03 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On 11/3/08, Glauber Costa <glommer@redhat.com> wrote:
>> Hi guys,
>>
>>  this is a port of current kvm vga memory optimization to our new
>>  infrastructure proposed by anthony. It's goal is to use as few
>>  kvm specific hooks as possible. In fact, the only one I'm relying
>>  on is enabling/disabling of logging. The rest, is pretty much general.
>>
>>  We map the linear frame buffer area as RAM, and then use dirty tracking
>>  to decide whether or not to update it. To be consistent with qemu,
>>  this version, differently from upstream kvm, tracks memory based on its
>>  physical address, represented by vram_offset, instead of vram_ptr, or
>>  any other construct.
>>
>>  Let me know what you think
>
> The patch does not apply, because of the kvm files. What patches do I
> need to apply first? I'd like to see how does the optimization apply
> to TCX.

Yes, you are missing kvm patches some time ago, which he plans to merge.

> This also may mean that some of my comments below can be invalid.
>
>>  +void set_vram_mapping(target_phys_addr_t begin, target_phys_addr_t end, ram_addr_t target)
>
> No "static"?
thanks, will update.

>
>>  +{
>>  +    /* align begin and end address */
>>  +    begin = begin & TARGET_PAGE_MASK;
>>  +    end = begin + VGA_RAM_SIZE;
>>  +    end = (end + TARGET_PAGE_SIZE -1 ) & TARGET_PAGE_MASK;
>
> This will fail if "end" is at the last page of the memory.

How so? I don't think it's possible for the addr 0xsomething0001 to be
valid, but 0x(something+1)000 to be not.

>
>>  +    uint32_t vga_io_memory;                                             \
>
> cpu_register_io_memory() returns an "int".

can it possibly be negative?
that said, of course I can change it for consistency, but I'd like to
understand it more

thanks


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

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

Re: [Qemu-devel] vga optmization

[Glauber Costa]

On Mon, Nov 3, 2008 at 4:13 PM, Fabrice Bellard <fabrice@bellard.org> wrote:
> Glauber Costa wrote:
>> Hi guys,
>>
>> this is a port of current kvm vga memory optimization to our new
>> infrastructure proposed by anthony. It's goal is to use as few
>> kvm specific hooks as possible. In fact, the only one I'm relying
>> on is enabling/disabling of logging. The rest, is pretty much general.
>>
>> We map the linear frame buffer area as RAM, and then use dirty tracking
>> to decide whether or not to update it. To be consistent with qemu,
>> this version, differently from upstream kvm, tracks memory based on its
>> physical address, represented by vram_offset, instead of vram_ptr, or
>> any other construct.
>>
>> Let me know what you think
>
> Why don't you modify the lower level QEMU dirty bits handling functions
> to be consistent with the KVM dirty bits ? By doing that you can avoid
> patching the device drivers and have smaller code. The fact that KVM use
> physical memory addresses is not a problem if you can convert the ram
> addresses to physical memory addresses (in most cases there is only one
> physical address corresponding to one RAM address).

Not here. We have to map vga linear frame buffer as RAM. So the option
is to always do that. That's pretty much the only device specific
thing we're doing here
About physical memory, exactly: code in kvm userspace does it a little
bit differently,
and it is _this_ code that does it the qemu way. So it seems we're in agreement.

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

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

Re: [Qemu-devel] vga optmization

[Blue Swirl]

On 11/3/08, Glauber Costa <glommer@gmail.com> wrote:
> On Mon, Nov 3, 2008 at 4:03 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
>  > On 11/3/08, Glauber Costa <glommer@redhat.com> wrote:
>  >> Hi guys,
>  >>
>  >>  this is a port of current kvm vga memory optimization to our new
>  >>  infrastructure proposed by anthony. It's goal is to use as few
>  >>  kvm specific hooks as possible. In fact, the only one I'm relying
>  >>  on is enabling/disabling of logging. The rest, is pretty much general.
>  >>
>  >>  We map the linear frame buffer area as RAM, and then use dirty tracking
>  >>  to decide whether or not to update it. To be consistent with qemu,
>  >>  this version, differently from upstream kvm, tracks memory based on its
>  >>  physical address, represented by vram_offset, instead of vram_ptr, or
>  >>  any other construct.
>  >>
>  >>  Let me know what you think
>  >
>  > The patch does not apply, because of the kvm files. What patches do I
>  > need to apply first? I'd like to see how does the optimization apply
>  > to TCX.
>
>
> Yes, you are missing kvm patches some time ago, which he plans to merge.
>
>
>  > This also may mean that some of my comments below can be invalid.
>  >
>  >>  +void set_vram_mapping(target_phys_addr_t begin, target_phys_addr_t end, ram_addr_t target)
>  >
>  > No "static"?
>
> thanks, will update.
>
>
>  >
>  >>  +{
>  >>  +    /* align begin and end address */
>  >>  +    begin = begin & TARGET_PAGE_MASK;
>  >>  +    end = begin + VGA_RAM_SIZE;
>  >>  +    end = (end + TARGET_PAGE_SIZE -1 ) & TARGET_PAGE_MASK;
>  >
>  > This will fail if "end" is at the last page of the memory.
>
>
> How so? I don't think it's possible for the addr 0xsomething0001 to be
>  valid, but 0x(something+1)000 to be not.

0xfffff001 + 0xfff = 0 (wrapped), then masking still gives 0.

>  >
>  >>  +    uint32_t vga_io_memory;                                             \
>  >
>  > cpu_register_io_memory() returns an "int".
>
>
> can it possibly be negative?
>  that said, of course I can change it for consistency, but I'd like to
>  understand it more

In case of error it could be negative but I'm only thinking about consistency.

Re: [Qemu-devel] vga optmization

[Glauber Costa]

On Mon, Nov 3, 2008 at 4:41 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On 11/3/08, Glauber Costa <glommer@gmail.com> wrote:
>> On Mon, Nov 3, 2008 at 4:03 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
>>  > On 11/3/08, Glauber Costa <glommer@redhat.com> wrote:
>>  >> Hi guys,
>>  >>
>>  >>  this is a port of current kvm vga memory optimization to our new
>>  >>  infrastructure proposed by anthony. It's goal is to use as few
>>  >>  kvm specific hooks as possible. In fact, the only one I'm relying
>>  >>  on is enabling/disabling of logging. The rest, is pretty much general.
>>  >>
>>  >>  We map the linear frame buffer area as RAM, and then use dirty tracking
>>  >>  to decide whether or not to update it. To be consistent with qemu,
>>  >>  this version, differently from upstream kvm, tracks memory based on its
>>  >>  physical address, represented by vram_offset, instead of vram_ptr, or
>>  >>  any other construct.
>>  >>
>>  >>  Let me know what you think
>>  >
>>  > The patch does not apply, because of the kvm files. What patches do I
>>  > need to apply first? I'd like to see how does the optimization apply
>>  > to TCX.
>>
>>
>> Yes, you are missing kvm patches some time ago, which he plans to merge.
>>
>>
>>  > This also may mean that some of my comments below can be invalid.
>>  >
>>  >>  +void set_vram_mapping(target_phys_addr_t begin, target_phys_addr_t end, ram_addr_t target)
>>  >
>>  > No "static"?
>>
>> thanks, will update.
>>
>>
>>  >
>>  >>  +{
>>  >>  +    /* align begin and end address */
>>  >>  +    begin = begin & TARGET_PAGE_MASK;
>>  >>  +    end = begin + VGA_RAM_SIZE;
>>  >>  +    end = (end + TARGET_PAGE_SIZE -1 ) & TARGET_PAGE_MASK;
>>  >
>>  > This will fail if "end" is at the last page of the memory.
>>
>>
>> How so? I don't think it's possible for the addr 0xsomething0001 to be
>>  valid, but 0x(something+1)000 to be not.
>
> 0xfffff001 + 0xfff = 0 (wrapped), then masking still gives 0.

oh, you're talking about the last page in the possible address space.
In that case I agree, but I believe we're not expecting vga to ever be there.

>
>>  >
>>  >>  +    uint32_t vga_io_memory;                                             \
>>  >
>>  > cpu_register_io_memory() returns an "int".
>>
>>
>> can it possibly be negative?
>>  that said, of course I can change it for consistency, but I'd like to
>>  understand it more
>
> In case of error it could be negative but I'm only thinking about consistency.
fair. I'll update it.
>
>



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

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

Re: [Qemu-devel] vga optmization

[Avi Kivity]

Glauber Costa wrote:
> this is a port of current kvm vga memory optimization to our new
> infrastructure proposed by anthony. It's goal is to use as few
> kvm specific hooks as possible. In fact, the only one I'm relying
> on is enabling/disabling of logging. The rest, is pretty much general.
>
> We map the linear frame buffer area as RAM, and then use dirty tracking
> to decide whether or not to update it. To be consistent with qemu,
> this version, differently from upstream kvm, tracks memory based on its
> physical address, represented by vram_offset, instead of vram_ptr, or
> any other construct.
>
> Let me know what you think
>
>   
> +int cpu_physical_memory_get_dirty(ram_addr_t addr,
> +                                                int dirty_flags)
> +{
> +    int is_dirty = 0;
> +    is_dirty = phys_ram_dirty[addr >> TARGET_PAGE_BITS] & dirty_flags;
> +    if (is_dirty)
> +        goto out;
> +#ifdef CONFIG_KVM
> +    if (kvm_enabled())
> +        is_dirty = kvm_physical_memory_get_dirty(addr);
> +        /* to make it usable below */
> +        is_dirty = !!is_dirty * 0xff;
> +#endif
> +out:
> +    return is_dirty;
> +}
> +
>   

The kvm dirty bitmap and qemu dirty bitmap are different.  'qemu dirty' 
means 'written to since hte last time the dirty bit was cleared', while 
'kvm dirty' means 'written to since the last time the bitmap was 
synchronized'.  So the qemu bitmap is stickier than the kvm bitmap.

The current code accounts for that by merging the kvm bitmap into the 
qemu bitmap, but you're losing some information here.  It doesn't matter 
for vga, since you're clearing the dirty bit immediately anyway, but it 
will matter for other uses (example, live migration with the vga 
optimization enabled).

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

Re: [Qemu-devel] vga optmization

[andrzej zaborowski]

Hi,

2008/11/3 Glauber Costa <glommer@redhat.com>:
[...]
> diff --git a/cpu-all.h b/cpu-all.h
> index cdd79bc..9118f4d 100644
> --- a/cpu-all.h
> +++ b/cpu-all.h
> @@ -46,6 +46,8 @@
>
>  #ifdef BSWAP_NEEDED
>
> +#include "kvm.h"
> +
>  static inline uint16_t tswap16(uint16_t s)
>  {
>     return bswap16(s);
> @@ -909,17 +911,10 @@ int cpu_memory_rw_debug(CPUState *env, target_ulong addr,
>  #define KQEMU_DIRTY_FLAG     0x04
>  #define MIGRATION_DIRTY_FLAG 0x08
>
> -/* read dirty bit (return 0 or 1) */
> -static inline int cpu_physical_memory_is_dirty(ram_addr_t addr)
> -{
> -    return phys_ram_dirty[addr >> TARGET_PAGE_BITS] == 0xff;
> -}
> +int cpu_physical_memory_get_dirty(ram_addr_t addr, int dirty_flags);
> +int cpu_physical_memory_is_dirty(ram_addr_t addr);
>
> -static inline int cpu_physical_memory_get_dirty(ram_addr_t addr,
> -                                                int dirty_flags)
> -{
> -    return phys_ram_dirty[addr >> TARGET_PAGE_BITS] & dirty_flags;
> -}
> +void qemu_physical_sync_dirty_bitmap(ram_addr_t start_addr);

This will prevent the functions from being inlined even if KVM is
disabled (e.g. on other archs) and I think it could be easily
retained.

Cheers

Re: [Qemu-devel] vga optmization

[Stefano Stabellini]

andrzej zaborowski wrote:

> Hi,
> 
> 2008/11/3 Glauber Costa <glommer@redhat.com>:
> [...]
>> diff --git a/cpu-all.h b/cpu-all.h
>> index cdd79bc..9118f4d 100644
>> --- a/cpu-all.h
>> +++ b/cpu-all.h
>> @@ -46,6 +46,8 @@
>>
>>  #ifdef BSWAP_NEEDED
>>
>> +#include "kvm.h"
>> +
>>  static inline uint16_t tswap16(uint16_t s)
>>  {
>>     return bswap16(s);
>> @@ -909,17 +911,10 @@ int cpu_memory_rw_debug(CPUState *env, target_ulong addr,
>>  #define KQEMU_DIRTY_FLAG     0x04
>>  #define MIGRATION_DIRTY_FLAG 0x08
>>
>> -/* read dirty bit (return 0 or 1) */
>> -static inline int cpu_physical_memory_is_dirty(ram_addr_t addr)
>> -{
>> -    return phys_ram_dirty[addr >> TARGET_PAGE_BITS] == 0xff;
>> -}
>> +int cpu_physical_memory_get_dirty(ram_addr_t addr, int dirty_flags);
>> +int cpu_physical_memory_is_dirty(ram_addr_t addr);
>>
>> -static inline int cpu_physical_memory_get_dirty(ram_addr_t addr,
>> -                                                int dirty_flags)
>> -{
>> -    return phys_ram_dirty[addr >> TARGET_PAGE_BITS] & dirty_flags;
>> -}
>> +void qemu_physical_sync_dirty_bitmap(ram_addr_t start_addr);
> 
> This will prevent the functions from being inlined even if KVM is
> disabled (e.g. on other archs) and I think it could be easily
> retained.
> 


I agree on this.

> void qemu_physical_sync_dirty_bitmap(ram_addr_t start_addr)
> {
>     if (kvm_enabled())
>         kvm_physical_sync_dirty_bitmap(start_addr);
> }
> 


Why don't you make qemu_physical_sync_dirty_bitmap take also and end
address, and you merge the two bitmaps in this address range in this
function, so you don't have to change cpu_physical_memory_get_dirty at
all?

I am saying to do something like:

void qemu_physical_sync_dirty_bitmap(ram_addr_t start_addr, ram_addr_t end_addr)
{
	/* sync and merge the two bitmaps between start_addr and end_addr */
}

then leave cpu_physical_memory_get_dirty untouched.
I would prefer this approch, it also leaves more space to other
optimizations.

Re: [Qemu-devel] vga optmization

[Glauber Costa]

On Tue, Nov 4, 2008 at 9:40 AM, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> andrzej zaborowski wrote:
>
>> Hi,
>>
>> 2008/11/3 Glauber Costa <glommer@redhat.com>:
>> [...]
>>> diff --git a/cpu-all.h b/cpu-all.h
>>> index cdd79bc..9118f4d 100644
>>> --- a/cpu-all.h
>>> +++ b/cpu-all.h
>>> @@ -46,6 +46,8 @@
>>>
>>>  #ifdef BSWAP_NEEDED
>>>
>>> +#include "kvm.h"
>>> +
>>>  static inline uint16_t tswap16(uint16_t s)
>>>  {
>>>     return bswap16(s);
>>> @@ -909,17 +911,10 @@ int cpu_memory_rw_debug(CPUState *env, target_ulong addr,
>>>  #define KQEMU_DIRTY_FLAG     0x04
>>>  #define MIGRATION_DIRTY_FLAG 0x08
>>>
>>> -/* read dirty bit (return 0 or 1) */
>>> -static inline int cpu_physical_memory_is_dirty(ram_addr_t addr)
>>> -{
>>> -    return phys_ram_dirty[addr >> TARGET_PAGE_BITS] == 0xff;
>>> -}
>>> +int cpu_physical_memory_get_dirty(ram_addr_t addr, int dirty_flags);
>>> +int cpu_physical_memory_is_dirty(ram_addr_t addr);
>>>
>>> -static inline int cpu_physical_memory_get_dirty(ram_addr_t addr,
>>> -                                                int dirty_flags)
>>> -{
>>> -    return phys_ram_dirty[addr >> TARGET_PAGE_BITS] & dirty_flags;
>>> -}
>>> +void qemu_physical_sync_dirty_bitmap(ram_addr_t start_addr);
>>
>> This will prevent the functions from being inlined even if KVM is
>> disabled (e.g. on other archs) and I think it could be easily
>> retained.
>>
>
>
> I agree on this.
>
>> void qemu_physical_sync_dirty_bitmap(ram_addr_t start_addr)
>> {
>>     if (kvm_enabled())
>>         kvm_physical_sync_dirty_bitmap(start_addr);
>> }
>>
>
>
> Why don't you make qemu_physical_sync_dirty_bitmap take also and end
> address, and you merge the two bitmaps in this address range in this
> function, so you don't have to change cpu_physical_memory_get_dirty at
> all?
>
> I am saying to do something like:
>
> void qemu_physical_sync_dirty_bitmap(ram_addr_t start_addr, ram_addr_t end_addr)
> {
>        /* sync and merge the two bitmaps between start_addr and end_addr */
> }
>
> then leave cpu_physical_memory_get_dirty untouched.
> I would prefer this approch, it also leaves more space to other
> optimizations.

Either way works for me. I'm fine with your suggestion.
>
>
>



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

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

Re: [Qemu-devel] vga optmization

[Avi Kivity]

Stefano Stabellini wrote:
> Why don't you make qemu_physical_sync_dirty_bitmap take also and end
> address, and you merge the two bitmaps in this address range in this
> function, so you don't have to change cpu_physical_memory_get_dirty at
> all?
>
> I am saying to do something like:
>
> void qemu_physical_sync_dirty_bitmap(ram_addr_t start_addr, ram_addr_t end_addr)
> {
> 	/* sync and merge the two bitmaps between start_addr and end_addr */
> }
>
> then leave cpu_physical_memory_get_dirty untouched.
> I would prefer this approch, it also leaves more space to other
> optimizations.
>   

That's how current kvm userspace works.  It's also more correct, since 
the kvm bitmap feeds both the vga dirty bits and the live migration 
dirty bits.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] vga optmization

[Anthony Liguori]

Avi Kivity wrote:
> Stefano Stabellini wrote:
>> Why don't you make qemu_physical_sync_dirty_bitmap take also and end
>> address, and you merge the two bitmaps in this address range in this
>> function, so you don't have to change cpu_physical_memory_get_dirty at
>> all?
>>
>> I am saying to do something like:
>>
>> void qemu_physical_sync_dirty_bitmap(ram_addr_t start_addr, 
>> ram_addr_t end_addr)
>> {
>>     /* sync and merge the two bitmaps between start_addr and end_addr */
>> }
>>
>> then leave cpu_physical_memory_get_dirty untouched.
>> I would prefer this approch, it also leaves more space to other
>> optimizations.
>>   
>
> That's how current kvm userspace works.  It's also more correct, since 
> the kvm bitmap feeds both the vga dirty bits and the live migration 
> dirty bits.

Yes, I was thinking the same thing myself.

Regards,

Anthony Liguori

Re: [Qemu-devel] vga optmization

[Glauber Costa]

On Tue, Nov 4, 2008 at 12:51 PM, Avi Kivity <avi@redhat.com> wrote:
> Stefano Stabellini wrote:
>>
>> Why don't you make qemu_physical_sync_dirty_bitmap take also and end
>> address, and you merge the two bitmaps in this address range in this
>> function, so you don't have to change cpu_physical_memory_get_dirty at
>> all?
>>
>> I am saying to do something like:
>>
>> void qemu_physical_sync_dirty_bitmap(ram_addr_t start_addr, ram_addr_t
>> end_addr)
>> {
>>        /* sync and merge the two bitmaps between start_addr and end_addr
>> */
>> }
>>
>> then leave cpu_physical_memory_get_dirty untouched.
>> I would prefer this approch, it also leaves more space to other
>> optimizations.
>>
>
> That's how current kvm userspace works.  It's also more correct, since the
> kvm bitmap feeds both the vga dirty bits and the live migration dirty bits.

My reason to do this way, was to be lazy about updating the dirty bitmap.
But if the common case is to check for all pages in the region, then
it won't matter much.

>
> --
> error compiling committee.c: too many arguments to function
>
>
>
>



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

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

Re: [Qemu-devel] vga optmization

[Stefano Stabellini]

Avi Kivity wrote:

> Stefano Stabellini wrote:
>> Why don't you make qemu_physical_sync_dirty_bitmap take also and end
>> address, and you merge the two bitmaps in this address range in this
>> function, so you don't have to change cpu_physical_memory_get_dirty at
>> all?
>>
>> I am saying to do something like:
>>
>> void qemu_physical_sync_dirty_bitmap(ram_addr_t start_addr, ram_addr_t
>> end_addr)
>> {
>>     /* sync and merge the two bitmaps between start_addr and end_addr */
>> }
>>
>> then leave cpu_physical_memory_get_dirty untouched.
>> I would prefer this approch, it also leaves more space to other
>> optimizations.
>>   
> 
> That's how current kvm userspace works.  It's also more correct, since
> the kvm bitmap feeds both the vga dirty bits and the live migration
> dirty bits.
> 


That's also how currently qemu-xen works.
I am glad that we agree :)

Re: [Qemu-devel] vga optmization

[Stefano Stabellini]

Glauber Costa wrote:

> On Tue, Nov 4, 2008 at 12:51 PM, Avi Kivity <avi@redhat.com> wrote:
>> Stefano Stabellini wrote:
>>> Why don't you make qemu_physical_sync_dirty_bitmap take also and end
>>> address, and you merge the two bitmaps in this address range in this
>>> function, so you don't have to change cpu_physical_memory_get_dirty at
>>> all?
>>>
>>> I am saying to do something like:
>>>
>>> void qemu_physical_sync_dirty_bitmap(ram_addr_t start_addr, ram_addr_t
>>> end_addr)
>>> {
>>>        /* sync and merge the two bitmaps between start_addr and end_addr
>>> */
>>> }
>>>
>>> then leave cpu_physical_memory_get_dirty untouched.
>>> I would prefer this approch, it also leaves more space to other
>>> optimizations.
>>>
>> That's how current kvm userspace works.  It's also more correct, since the
>> kvm bitmap feeds both the vga dirty bits and the live migration dirty bits.
> 
> My reason to do this way, was to be lazy about updating the dirty bitmap.
> But if the common case is to check for all pages in the region, then
> it won't matter much.

We have an optimization not to always check for all pages in the region,
 it is not really good looking, but it is working fine.
I could send a patch with this optimization after yours, it only affects
vga.c.

Re: [Qemu-devel] vga optmization

[Glauber Costa]

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

On Tue, Nov 04, 2008 at 03:01:21PM +0000, Stefano Stabellini wrote:
> Avi Kivity wrote:
> 
> > Stefano Stabellini wrote:
> >> Why don't you make qemu_physical_sync_dirty_bitmap take also and end
> >> address, and you merge the two bitmaps in this address range in this
> >> function, so you don't have to change cpu_physical_memory_get_dirty at
> >> all?
> >>
> >> I am saying to do something like:
> >>
> >> void qemu_physical_sync_dirty_bitmap(ram_addr_t start_addr, ram_addr_t
> >> end_addr)
> >> {
> >>     /* sync and merge the two bitmaps between start_addr and end_addr */
> >> }
> >>
> >> then leave cpu_physical_memory_get_dirty untouched.
> >> I would prefer this approch, it also leaves more space to other
> >> optimizations.
> >>   
> > 
> > That's how current kvm userspace works.  It's also more correct, since
> > the kvm bitmap feeds both the vga dirty bits and the live migration
> > dirty bits.
> > 
> 
> 
> That's also how currently qemu-xen works.
> I am glad that we agree :)

I'm attaching a new version. Let me know if it's better this way.

[-- Attachment #2: vga2.patch --]
[-- Type: text/plain, Size: 16233 bytes --]

diff --git a/exec.c b/exec.c
index ef1072b..2ed5e2b 100644
--- a/exec.c
+++ b/exec.c
@@ -1823,6 +1823,17 @@ int cpu_physical_memory_get_dirty_tracking(void)
     return in_migration;
 }
 
+void qemu_physical_sync_dirty_bitmap(ram_addr_t start_addr, ram_addr_t end_addr)
+{
+    if (kvm_enabled()) {
+        ram_addr_t addr;
+        kvm_physical_sync_dirty_bitmap(start_addr);
+        for (addr = start_addr; addr < end_addr; addr += TARGET_PAGE_SIZE)
+            phys_ram_dirty[addr >> TARGET_PAGE_BITS] |= kvm_physical_memory_get_dirty(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 af9c9e6..bc1a3af 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:
@@ -248,6 +249,9 @@ typedef struct CirrusVGAState {
     int cirrus_linear_io_addr;
     int cirrus_linear_bitblt_io_addr;
     int cirrus_mmio_io_addr;
+    uint32_t cirrus_lfb_addr;
+    uint32_t cirrus_lfb_end;
+    uint32_t cirrus_lfb_mapped;
     uint32_t cirrus_addr_mask;
     uint32_t linear_mmio_mask;
     uint8_t cirrus_shadow_gr0;
@@ -2618,6 +2622,69 @@ static CPUWriteMemoryFunc *cirrus_linear_bitblt_write[3] = {
     cirrus_linear_bitblt_writel,
 };
 
+static void set_vram_mapping(target_phys_addr_t begin, target_phys_addr_t end, ram_addr_t target)
+{
+    /* align begin and end address */
+    begin = begin & TARGET_PAGE_MASK;
+    end = begin + VGA_RAM_SIZE;
+    end = (end + TARGET_PAGE_SIZE -1 ) & TARGET_PAGE_MASK;
+
+    cpu_register_physical_memory(begin, end - begin, target);
+
+    if (kvm_enabled()) {
+        /* XXX: per-slot dirty tracking in qemu may get rid of it */
+        kvm_log_start(begin, end - begin);
+    }
+}
+
+static void unset_vram_mapping(target_phys_addr_t begin, target_phys_addr_t end)
+{
+    /* align begin and end address */
+    end = begin + VGA_RAM_SIZE;
+    begin = begin & TARGET_PAGE_MASK;
+    end = (end + TARGET_PAGE_SIZE -1 ) & TARGET_PAGE_MASK;
+
+    if (kvm_enabled()) {
+        /* XXX: per-slot dirty tracking in qemu may get rid of it */
+        kvm_log_stop(begin, end - begin);
+    }
+    cpu_register_physical_memory(begin, end - begin, IO_MEM_UNASSIGNED);
+
+}
+
+static void map_linear_vram(CirrusVGAState *s)
+{
+
+    if (!s->cirrus_lfb_mapped && s->cirrus_lfb_addr && s->cirrus_lfb_end) {
+        set_vram_mapping(s->cirrus_lfb_addr, s->cirrus_lfb_end, s->vram_offset);
+        s->cirrus_lfb_mapped = 1;
+    }
+    
+    if(!(s->cirrus_srcptr != s->cirrus_srcptr_end)
+        && !((s->sr[0x07] & 0x01) == 0)
+        && !((s->gr[0x0B] & 0x14) == 0x14)
+        && !(s->gr[0x0B] & 0x02)) {
+        cpu_register_physical_memory(isa_mem_base + 0xa0000, 0x20000, s->vram_offset | IO_MEM_RAM);
+    }
+    else {
+        cpu_register_physical_memory(isa_mem_base + 0xa0000, 0x20000, s->vga_io_memory);
+    }
+}
+
+static void unmap_linear_vram(CirrusVGAState *s)
+{
+
+    if (s->cirrus_lfb_mapped && s->cirrus_lfb_addr && s->cirrus_lfb_end) {
+        unset_vram_mapping(s->cirrus_lfb_addr,
+                           s->cirrus_lfb_end);
+        s->cirrus_lfb_mapped = 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)
 {
@@ -2636,11 +2703,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;
@@ -3117,7 +3186,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) {
@@ -3156,10 +3225,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) {
@@ -3258,9 +3327,13 @@ static void cirrus_pci_lfb_map(PCIDevice *d, int region_num,
 
     /* XXX: add byte swapping apertures */
     cpu_register_physical_memory(addr, s->vram_size,
-				 s->cirrus_linear_io_addr);
+				 s->cirrus_linear_io_addr | s->vram_offset);
     cpu_register_physical_memory(addr + 0x1000000, 0x400000,
 				 s->cirrus_linear_bitblt_io_addr);
+
+    s->cirrus_lfb_mapped = 0;
+    s->cirrus_lfb_addr = addr;
+    s->cirrus_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..1568be4 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
@@ -1568,6 +1569,8 @@ static void vga_draw_graphic(VGAState *s, int full_update)
     uint32_t v, addr1, addr;
     vga_draw_line_func *vga_draw_line;
 
+    qemu_physical_sync_dirty_bitmap(s->vram_offset, s->vram_offset + VGA_RAM_SIZE);
+
     full_update |= update_basic_params(s);
 
     s->get_resolution(s, &width, &height);
@@ -2102,6 +2105,9 @@ static void vga_map(PCIDevice *pci_dev, int region_num,
     } else {
         cpu_register_physical_memory(addr, s->vram_size, s->vram_offset);
     }
+
+    if (kvm_enabled())
+        kvm_log_start(addr, VGA_RAM_SIZE);
 }
 
 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 82a755e..91a8d77 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;                                             \
@@ -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,                         \
diff --git a/kvm-all.c b/kvm-all.c
index 4379071..4916865 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -31,7 +31,18 @@
     do { } while (0)
 #endif
 
-typedef struct kvm_userspace_memory_region KVMSlot;
+#define warning(fmt, ...) \
+    do { fprintf(stderr, "%s:%d" fmt, __func__, __LINE__, ## __VA_ARGS__); } while (0)
+
+typedef struct KVMSlot {
+        struct kvm_userspace_memory_region region;
+        int logging_count;
+        uint64_t *dirty_bitmap;
+} KVMSlot;
+
+#define kvm_uaddr(addr) ((addr) + (ram_addr_t)phys_ram_base)
+
+typedef struct kvm_dirty_log KVMDirty;
 
 int kvm_allowed = 0;
 
@@ -49,7 +60,7 @@ static KVMSlot *kvm_alloc_slot(KVMState *s)
     int i;
 
     for (i = 0; i < ARRAY_SIZE(s->slots); i++) {
-        if (s->slots[i].memory_size == 0)
+        if (s->slots[i].region.memory_size == 0)
             return &s->slots[i];
     }
 
@@ -63,8 +74,26 @@ static KVMSlot *kvm_lookup_slot(KVMState *s, target_phys_addr_t start_addr)
     for (i = 0; i < ARRAY_SIZE(s->slots); i++) {
         KVMSlot *mem = &s->slots[i];
 
-        if (start_addr >= mem->guest_phys_addr &&
-            start_addr < (mem->guest_phys_addr + mem->memory_size))
+        if (start_addr >= mem->region.guest_phys_addr &&
+            start_addr < (mem->region.guest_phys_addr + mem->region.memory_size))
+            return mem;
+    }
+
+    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->region.userspace_addr &&
+            uaddr < (mem->region.userspace_addr + mem->region.memory_size))
             return mem;
     }
 
@@ -109,6 +138,133 @@ 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->region.flags & ~mask) | flags;
+    if (flags == mem->region.flags)
+            return 0;
+
+    mem->region.flags = flags;
+
+    r = kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem->region);
+    if (r == -1)
+        fprintf(stderr, "%s: %m\n", __FUNCTION__);
+
+    return r;
+}
+
+#define BMAP_SIZE(mem) ((mem->region.memory_size >> TARGET_PAGE_BITS)/ sizeof(*mem->dirty_bitmap))
+
+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);
+
+        dprintf("try log start %llx len %llx\n", phys_addr, len);
+
+        if (mem == NULL)  {
+                fprintf(stderr, "BUG: %s: invalid parameters\n", __func__);
+                return -EINVAL;
+        }
+
+        if (mem->logging_count++)
+                return 0;
+
+        mem->dirty_bitmap = malloc(BMAP_SIZE(mem));  
+
+        if (mem->dirty_bitmap == NULL)
+            return -ENOMEM;
+
+        dprintf("slot %d: enable logging (phys %llx, uaddr %llx)\n",
+                 mem->region.slot, mem->region.guest_phys_addr, mem->region.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;
+        }
+        dprintf("slot %d: disabling logging\n", mem->region.slot);
+
+        if (mem->logging_count--)
+                return 0;
+
+        return kvm_dirty_pages_log_change(mem,
+                                          0,
+                                          KVM_MEM_LOG_DIRTY_PAGES);
+}
+
+#define BITMAP_WORD(mem) (sizeof(*(mem)->dirty_bitmap) * 8)
+static inline int lookup_bitmap_phys(KVMSlot *mem, ram_addr_t addr)
+{
+    unsigned nr = (uint32_t)((uint64_t)kvm_uaddr(addr) - mem->region.userspace_addr) >> TARGET_PAGE_BITS;
+    unsigned word = nr / BITMAP_WORD(mem);
+    unsigned bit = nr % BITMAP_WORD(mem);
+    int ret;
+
+    ret = (mem->dirty_bitmap[word] >> bit) & 1;
+    return ret;
+}
+
+int kvm_physical_memory_get_dirty(ram_addr_t addr)
+{
+    KVMState *s = kvm_state;
+    KVMSlot *mem = kvm_lookup_slot_uaddr(s, addr);
+
+    if (mem == NULL) {
+            fprintf(stderr, "BUG: %s: invalid parameters\n", __func__);
+            return -EINVAL;
+    }
+
+    if (mem->dirty_bitmap == NULL) {
+        return 0;
+    }
+    return lookup_bitmap_phys(mem, addr);
+}
+
+void kvm_physical_sync_dirty_bitmap(ram_addr_t start_addr)
+{
+    KVMState *s = kvm_state;
+    KVMSlot *mem = kvm_lookup_slot_uaddr(s, start_addr);
+    
+    KVMDirty d;
+
+    dprintf("sync addr: %lx %llx %llx\n", start_addr, mem->region.guest_phys_addr, kvm_uaddr(start_addr));
+    if (mem == NULL) {
+            fprintf(stderr, "BUG: %s: invalid parameters\n", __func__);
+            return;
+    }
+
+    if (mem->dirty_bitmap == NULL)
+        warning("Asked to sync dirty bitmap for region with logging disabled\n");
+
+    d.slot = mem->region.slot;
+    d.dirty_bitmap = mem->dirty_bitmap;
+    dprintf("slot %d, phys_addr %llx, uaddr: %llx\n",
+            d.slot, mem->region.guest_phys_addr, mem->region.userspace_addr);
+
+    if (kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d) == -1)
+        warning("ioctl failed %d\n", errno);
+    
+}
+
 int kvm_init(int smp_cpus)
 {
     KVMState *s;
@@ -123,7 +279,7 @@ int kvm_init(int smp_cpus)
         return -ENOMEM;
 
     for (i = 0; i < ARRAY_SIZE(s->slots); i++)
-        s->slots[i].slot = i;
+        s->slots[i].region.slot = i;
 
     s->vmfd = -1;
     s->fd = open("/dev/kvm", O_RDWR);
@@ -317,15 +473,18 @@ 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) {
-            mem->memory_size = 0;
-            mem->guest_phys_addr = start_addr;
-            mem->userspace_addr = 0;
-            mem->flags = 0;
-
-            kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, mem);
-        } else if (start_addr >= mem->guest_phys_addr &&
-                   (start_addr + size) <= (mem->guest_phys_addr + mem->memory_size))
+        if ((flags == IO_MEM_UNASSIGNED) || (flags >= TLB_MMIO)) {
+            if (mem->region.flags == KVM_MEM_LOG_DIRTY_PAGES)
+                return;
+            dprintf("deleting memory %llx with flags %d\n", mem->region.guest_phys_addr, mem->region.flags);
+            mem->region.memory_size = 0;
+            mem->region.guest_phys_addr = start_addr;
+            mem->region.userspace_addr = 0;
+            mem->region.flags = 0;
+
+            kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem->region);
+        } else if (start_addr >= mem->region.guest_phys_addr &&
+                   (start_addr + size) <= (mem->region.guest_phys_addr + mem->region.memory_size))
             return;
     }
 
@@ -334,12 +493,12 @@ void kvm_set_phys_mem(target_phys_addr_t start_addr,
         return;
 
     mem = kvm_alloc_slot(s);
-    mem->memory_size = size;
-    mem->guest_phys_addr = start_addr;
-    mem->userspace_addr = (unsigned long)(phys_ram_base + phys_offset);
-    mem->flags = 0;
+    mem->region.memory_size = size;
+    mem->region.guest_phys_addr = start_addr;
+    mem->region.userspace_addr = (unsigned long)(phys_ram_base + phys_offset);
+    mem->region.flags = 0;
 
-    kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, mem);
+    kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem->region);
     /* FIXME deal with errors */
 }
 
diff --git a/kvm.h b/kvm.h
index 37102b4..6e2d9d2 100644
--- a/kvm.h
+++ b/kvm.h
@@ -38,6 +38,9 @@ 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);
+
 /* internal API */
 
 struct KVMState;

Re: [Qemu-devel] vga optmization

[Anthony Liguori]

Glauber Costa wrote:
> diff --git a/exec.c b/exec.c
> index ef1072b..2ed5e2b 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1823,6 +1823,17 @@ int cpu_physical_memory_get_dirty_tracking(void)
>      return in_migration;
>  }
>
> +void qemu_physical_sync_dirty_bitmap(ram_addr_t start_addr, ram_addr_t end_addr)
> +{
> +    if (kvm_enabled()) {
> +        ram_addr_t addr;
> +        kvm_physical_sync_dirty_bitmap(start_addr);
> +        for (addr = start_addr; addr < end_addr; addr += TARGET_PAGE_SIZE)
> +            phys_ram_dirty[addr >> TARGET_PAGE_BITS] |= kvm_physical_memory_get_dirty(addr);

Might as well push this into kvm-all.c so that this becomes:

if (kvm_enabled())
    kvm_physical_sync_dirty_bitmap(start_addr, end_addr);

And this very likely will become a hook one day.

And I think the function should be renamed to 
cpu_physical_memory_update_dirty() to be more in line with the rest of 
the functions.

>         
> +    }
> +}
> +
>  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 af9c9e6..bc1a3af 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:
> @@ -248,6 +249,9 @@ typedef struct CirrusVGAState {
>      int cirrus_linear_io_addr;
>      int cirrus_linear_bitblt_io_addr;
>      int cirrus_mmio_io_addr;
> +    uint32_t cirrus_lfb_addr;
> +    uint32_t cirrus_lfb_end;
> +    uint32_t cirrus_lfb_mapped;
>      uint32_t cirrus_addr_mask;
>      uint32_t linear_mmio_mask;
>      uint8_t cirrus_shadow_gr0;
> @@ -2618,6 +2622,69 @@ static CPUWriteMemoryFunc *cirrus_linear_bitblt_write[3] = {
>      cirrus_linear_bitblt_writel,
>  };
>
> +static void set_vram_mapping(target_phys_addr_t begin, target_phys_addr_t end, ram_addr_t target)
> +{
> +    /* align begin and end address */
> +    begin = begin & TARGET_PAGE_MASK;
> +    end = begin + VGA_RAM_SIZE;
> +    end = (end + TARGET_PAGE_SIZE -1 ) & TARGET_PAGE_MASK;
> +
> +    cpu_register_physical_memory(begin, end - begin, target);
> +
> +    if (kvm_enabled()) {
> +        /* XXX: per-slot dirty tracking in qemu may get rid of it */
> +        kvm_log_start(begin, end - begin);
>   

I feel confident that we can get rid of this.  We just need a good 
suggestion.

> +    }
> +}
> +
> +static void unset_vram_mapping(target_phys_addr_t begin, target_phys_addr_t end)
> +{
> +    /* align begin and end address */
> +    end = begin + VGA_RAM_SIZE;
> +    begin = begin & TARGET_PAGE_MASK;
> +    end = (end + TARGET_PAGE_SIZE -1 ) & TARGET_PAGE_MASK;
> +
> +    if (kvm_enabled()) {
> +        /* XXX: per-slot dirty tracking in qemu may get rid of it */
> +        kvm_log_stop(begin, end - begin);
> +    }
> +    cpu_register_physical_memory(begin, end - begin, IO_MEM_UNASSIGNED);
>   

This is a little odd.  We only ever toggle the vram mapping from normal 
IO_RAM to MMIO.  We should never switch it to IO_MEM_UNASSIGNED.

> +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;
> +        }
> +        dprintf("slot %d: disabling logging\n", mem->region.slot);
> +
> +        if (mem->logging_count--)
> +                return 0;
> +
> +        return kvm_dirty_pages_log_change(mem,
> +                                          0,
> +                                          KVM_MEM_LOG_DIRTY_PAGES);
> +}
> +
> +#define BITMAP_WORD(mem) (sizeof(*(mem)->dirty_bitmap) * 8)
>   

this doesn't need to be a macro.

Can you split this patch up a bit?  At least one patch containing the 
KVM slot changes.  Also split out the cirrus verse standard VGA 
changes.  I think if you switch things around to get rid of 
kvm_physical_get_dirty(), you'll find you can malloc the memory for the 
dirty bitmap at the time of sync() which should significantly reduce 
your overall complexity.

Regards,

Anthony Liguori

Re: [Qemu-devel] vga optmization

[Avi Kivity]

Glauber Costa wrote:
> My reason to do this way, was to be lazy about updating the dirty bitmap.
> But if the common case is to check for all pages in the region, then
> it won't matter much.
>   

In the common case we only check for the displayable area.  We should 
split the region into two, one for the displayable area and one for the 
offscreen framebuffer (the boundary changes dynamically with the 
resolution).  If qemu can tell us nobody is looking at the screen, we 
can dirty tracking altogether.

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

Re: [Qemu-devel] vga optmization

[Anthony Liguori]

Avi Kivity wrote:
> Glauber Costa wrote:
>> My reason to do this way, was to be lazy about updating the dirty 
>> bitmap.
>> But if the common case is to check for all pages in the region, then
>> it won't matter much.
>>   
>
> In the common case we only check for the displayable area.  We should 
> split the region into two, one for the displayable area and one for 
> the offscreen framebuffer (the boundary changes dynamically with the 
> resolution).  If qemu can tell us nobody is looking at the screen, we 
> can dirty tracking altogether.

Yeah, there already is an ds->idle in the DisplayState code.  This is 
set by VNC and by SDL.

Of course, dirty updating is driven by vga_hw_update() which is driven 
by a timer who's interval is determined by ds->idle so we already do 
this in practice.

Regards,

Anthony Liguori

Re: [Qemu-devel] vga optmization

[Stefano Stabellini]

Glauber Costa wrote:

>> That's also how currently qemu-xen works.
>> I am glad that we agree :)
> 
> I'm attaching a new version. Let me know if it's better this way.
> 



Yes, this is much better thanks.

I still have comments about possible improvements, so I wrote patch
(against your patch).
This patch is not meant to be applied, is only meant to be read (I believe
that C code is more meaningful than English :).

Some of the changes include:

- instead of adding cirrus_lfb_addr, add a more generic lfb_addr to
VGAState, so that can be reused in the future for possible stdvga only
mappings;

- instead of using cirrus_lfb_mapped as a boolean, use it as the
mapping address, it is more useful that way;

- instead of keeping the kvm dirty map always enabled, enable it only
when the framebuffer is linear and in graphical mode;

- look at the changes to vga.c, there is a simple check to reduce the
dirty area to sync.

It would be nice to check if the last two changes are actually a
performance improvement.


diff -r a5bcebe9e2bc hw/cirrus_vga.c
--- a/hw/cirrus_vga.c	Wed Nov 05 12:09:13 2008 +0000
+++ b/hw/cirrus_vga.c	Wed Nov 05 14:29:54 2008 +0000
@@ -249,9 +249,6 @@
     int cirrus_linear_io_addr;
     int cirrus_linear_bitblt_io_addr;
     int cirrus_mmio_io_addr;
-    uint32_t cirrus_lfb_addr;
-    uint32_t cirrus_lfb_end;
-    uint32_t cirrus_lfb_mapped;
     uint32_t cirrus_addr_mask;
     uint32_t linear_mmio_mask;
     uint8_t cirrus_shadow_gr0;
@@ -2655,9 +2652,10 @@
 static void map_linear_vram(CirrusVGAState *s)
 {
 
-    if (!s->cirrus_lfb_mapped && s->cirrus_lfb_addr && s->cirrus_lfb_end) {
-        set_vram_mapping(s->cirrus_lfb_addr, s->cirrus_lfb_end, s->vram_offset);
-        s->cirrus_lfb_mapped = 1;
+    if (!s->map_addr && s->lfb_addr && s->lfb_end) {
+        set_vram_mapping(s->lfb_addr, s->lfb_end, s->vram_offset);
+        s->map_addr = s->lfb_addr;
+        s->map_end = s->lfb_end;
     }
     
     if(!(s->cirrus_srcptr != s->cirrus_srcptr_end)
@@ -2674,10 +2672,10 @@
 static void unmap_linear_vram(CirrusVGAState *s)
 {
 
-    if (s->cirrus_lfb_mapped && s->cirrus_lfb_addr && s->cirrus_lfb_end) {
-        unset_vram_mapping(s->cirrus_lfb_addr,
-                           s->cirrus_lfb_end);
-        s->cirrus_lfb_mapped = 0;
+    if (s->map_addr && s->lfb_addr && s->lfb_end) {
+        unset_vram_mapping(s->lfb_addr,
+                           s->lfb_end);
+        s->map_addr = s->map_end = 0;
     }
 
     cpu_register_physical_memory(isa_mem_base + 0x000a0000, 0x20000,
@@ -3331,9 +3329,9 @@
     cpu_register_physical_memory(addr + 0x1000000, 0x400000,
 				 s->cirrus_linear_bitblt_io_addr);
 
-    s->cirrus_lfb_mapped = 0;
-    s->cirrus_lfb_addr = addr;
-    s->cirrus_lfb_end = addr + VGA_RAM_SIZE;
+    s->map_addr = 0;
+    s->lfb_addr = addr;
+    s->lfb_end = addr + VGA_RAM_SIZE;
 }
 
 static void cirrus_pci_mmio_map(PCIDevice *d, int region_num,
diff -r a5bcebe9e2bc hw/vga.c
--- a/hw/vga.c	Wed Nov 05 12:09:13 2008 +0000
+++ b/hw/vga.c	Wed Nov 05 14:29:54 2008 +0000
@@ -1244,6 +1244,9 @@
     vga_draw_glyph8_func *vga_draw_glyph8;
     vga_draw_glyph9_func *vga_draw_glyph9;
 
+    /* disable dirty bit tracking */
+    // kvm_log_stop();
+
     full_update |= update_palette16(s);
     palette = s->last_palette;
 
@@ -1569,7 +1572,8 @@
     uint32_t v, addr1, addr;
     vga_draw_line_func *vga_draw_line;
 
-    qemu_physical_sync_dirty_bitmap(s->vram_offset, s->vram_offset + VGA_RAM_SIZE);
+    /* Enable dirty bit tracking */
+    // kvm_log_start
 
     full_update |= update_basic_params(s);
 
@@ -1657,6 +1661,31 @@
         s->cursor_invalidate(s);
 
     line_offset = s->line_offset;
+
+    if (s->lfb_addr) {
+        if (height - 1 > s->line_compare || multi_run || (s->cr[0x17] & 3) != 3) {
+            /* Tricky things happen, just track all video memory */
+            start = 0;
+            end = s->vram_size;
+        } else {
+            /* Tricky things won't have any effect, i.e. we are in the very simple
+             * (and very usual) case of a linear buffer. */
+            /* use page table dirty bit tracking for the LFB plus border */
+            start = (s->start_addr * 4) & TARGET_PAGE_MASK;
+            end = ((s->start_addr * 4 + height * line_offset) + TARGET_PAGE_SIZE - 1) & TARGET_PAGE_MASK;
+        }
+
+        for (y = 0 ; y < start; y += TARGET_PAGE_SIZE)
+            /* We will not read that anyway. */
+            cpu_physical_memory_set_dirty(s->vram_offset + y);
+
+        qemu_physical_sync_dirty_bitmap(s->lfb_addr + y, end);
+
+        for ( ; y < s->vram_size; y += TARGET_PAGE_SIZE)
+            /* We will not read that anyway. */
+            cpu_physical_memory_set_dirty(s->vram_offset + y);
+    }
+
 #if 0
     printf("w=%d h=%d v=%d line_offset=%d cr[0x09]=0x%02x cr[0x17]=0x%02x linecmp=%d sr[0x01]=0x%02x\n",
            width, height, v, line_offset, s->cr[9], s->cr[0x17], s->line_compare, s->sr[0x01]);
@@ -1746,6 +1775,10 @@
         return;
     if (s->last_scr_width <= 0 || s->last_scr_height <= 0)
         return;
+
+    /* disable dirty bit tracking */
+    // kvm_log_stop();
+
     if (s->ds->depth == 8)
         val = s->rgb_to_pixel(0, 0, 0);
     else
diff -r a5bcebe9e2bc hw/vga_int.h
--- a/hw/vga_int.h	Wed Nov 05 12:09:13 2008 +0000
+++ b/hw/vga_int.h	Wed Nov 05 14:29:54 2008 +0000
@@ -106,6 +106,10 @@
     unsigned int bios_size;                                             \
     target_phys_addr_t base_ctrl;                                       \
     int it_shift;                                                       \
+    unsigned long lfb_addr;                                             \
+    unsigned long lfb_end;                                              \
+    uint32_t map_addr;                                                  \
+    uint32_t map_end;                                                   \
     PCIDevice *pci_dev;                                                 \
     uint32_t latch;                                                     \
     uint8_t sr_index;                                                   \

Re: [Qemu-devel] vga optmization

[Glauber Costa]

On Wed, Nov 05, 2008 at 02:42:22PM +0000, Stefano Stabellini wrote:
> Glauber Costa wrote:
> 
> >> That's also how currently qemu-xen works.
> >> I am glad that we agree :)
> > 
> > I'm attaching a new version. Let me know if it's better this way.
> > 
> 
> 
> 
> Yes, this is much better thanks.
> 
> I still have comments about possible improvements, so I wrote patch
> (against your patch).
> This patch is not meant to be applied, is only meant to be read (I believe
> that C code is more meaningful than English :).
> 
> Some of the changes include:
> 
> - instead of adding cirrus_lfb_addr, add a more generic lfb_addr to
> VGAState, so that can be reused in the future for possible stdvga only
> mappings;
> 
> - instead of using cirrus_lfb_mapped as a boolean, use it as the
> mapping address, it is more useful that way;
> 
> - instead of keeping the kvm dirty map always enabled, enable it only
> when the framebuffer is linear and in graphical mode;
> 
> - look at the changes to vga.c, there is a simple check to reduce the
> dirty area to sync.
> 
> It would be nice to check if the last two changes are actually a
> performance improvement.
I'm not opposed to the changes. I'll redo a patch and resend it today.
However, I'm not planning to include the last optimization you sent.
It is better that we do it in a separate commit. If you can get numbers,
even better. I'll try it myself too.


> 
> 
> diff -r a5bcebe9e2bc hw/cirrus_vga.c
> --- a/hw/cirrus_vga.c	Wed Nov 05 12:09:13 2008 +0000
> +++ b/hw/cirrus_vga.c	Wed Nov 05 14:29:54 2008 +0000
> @@ -249,9 +249,6 @@
>      int cirrus_linear_io_addr;
>      int cirrus_linear_bitblt_io_addr;
>      int cirrus_mmio_io_addr;
> -    uint32_t cirrus_lfb_addr;
> -    uint32_t cirrus_lfb_end;
> -    uint32_t cirrus_lfb_mapped;
>      uint32_t cirrus_addr_mask;
>      uint32_t linear_mmio_mask;
>      uint8_t cirrus_shadow_gr0;
> @@ -2655,9 +2652,10 @@
>  static void map_linear_vram(CirrusVGAState *s)
>  {
>  
> -    if (!s->cirrus_lfb_mapped && s->cirrus_lfb_addr && s->cirrus_lfb_end) {
> -        set_vram_mapping(s->cirrus_lfb_addr, s->cirrus_lfb_end, s->vram_offset);
> -        s->cirrus_lfb_mapped = 1;
> +    if (!s->map_addr && s->lfb_addr && s->lfb_end) {
> +        set_vram_mapping(s->lfb_addr, s->lfb_end, s->vram_offset);
> +        s->map_addr = s->lfb_addr;
> +        s->map_end = s->lfb_end;
>      }
>      
>      if(!(s->cirrus_srcptr != s->cirrus_srcptr_end)
> @@ -2674,10 +2672,10 @@
>  static void unmap_linear_vram(CirrusVGAState *s)
>  {
>  
> -    if (s->cirrus_lfb_mapped && s->cirrus_lfb_addr && s->cirrus_lfb_end) {
> -        unset_vram_mapping(s->cirrus_lfb_addr,
> -                           s->cirrus_lfb_end);
> -        s->cirrus_lfb_mapped = 0;
> +    if (s->map_addr && s->lfb_addr && s->lfb_end) {
> +        unset_vram_mapping(s->lfb_addr,
> +                           s->lfb_end);
> +        s->map_addr = s->map_end = 0;
>      }
>  
>      cpu_register_physical_memory(isa_mem_base + 0x000a0000, 0x20000,
> @@ -3331,9 +3329,9 @@
>      cpu_register_physical_memory(addr + 0x1000000, 0x400000,
>  				 s->cirrus_linear_bitblt_io_addr);
>  
> -    s->cirrus_lfb_mapped = 0;
> -    s->cirrus_lfb_addr = addr;
> -    s->cirrus_lfb_end = addr + VGA_RAM_SIZE;
> +    s->map_addr = 0;
> +    s->lfb_addr = addr;
> +    s->lfb_end = addr + VGA_RAM_SIZE;
>  }
>  
>  static void cirrus_pci_mmio_map(PCIDevice *d, int region_num,
> diff -r a5bcebe9e2bc hw/vga.c
> --- a/hw/vga.c	Wed Nov 05 12:09:13 2008 +0000
> +++ b/hw/vga.c	Wed Nov 05 14:29:54 2008 +0000
> @@ -1244,6 +1244,9 @@
>      vga_draw_glyph8_func *vga_draw_glyph8;
>      vga_draw_glyph9_func *vga_draw_glyph9;
>  
> +    /* disable dirty bit tracking */
> +    // kvm_log_stop();
> +
>      full_update |= update_palette16(s);
>      palette = s->last_palette;
>  
> @@ -1569,7 +1572,8 @@
>      uint32_t v, addr1, addr;
>      vga_draw_line_func *vga_draw_line;
>  
> -    qemu_physical_sync_dirty_bitmap(s->vram_offset, s->vram_offset + VGA_RAM_SIZE);
> +    /* Enable dirty bit tracking */
> +    // kvm_log_start
>  
>      full_update |= update_basic_params(s);
>  
> @@ -1657,6 +1661,31 @@
>          s->cursor_invalidate(s);
>  
>      line_offset = s->line_offset;
> +
> +    if (s->lfb_addr) {
> +        if (height - 1 > s->line_compare || multi_run || (s->cr[0x17] & 3) != 3) {
> +            /* Tricky things happen, just track all video memory */
> +            start = 0;
> +            end = s->vram_size;
> +        } else {
> +            /* Tricky things won't have any effect, i.e. we are in the very simple
> +             * (and very usual) case of a linear buffer. */
> +            /* use page table dirty bit tracking for the LFB plus border */
> +            start = (s->start_addr * 4) & TARGET_PAGE_MASK;
> +            end = ((s->start_addr * 4 + height * line_offset) + TARGET_PAGE_SIZE - 1) & TARGET_PAGE_MASK;
> +        }
> +
> +        for (y = 0 ; y < start; y += TARGET_PAGE_SIZE)
> +            /* We will not read that anyway. */
> +            cpu_physical_memory_set_dirty(s->vram_offset + y);
> +
> +        qemu_physical_sync_dirty_bitmap(s->lfb_addr + y, end);
> +
> +        for ( ; y < s->vram_size; y += TARGET_PAGE_SIZE)
> +            /* We will not read that anyway. */
> +            cpu_physical_memory_set_dirty(s->vram_offset + y);
> +    }
> +
>  #if 0
>      printf("w=%d h=%d v=%d line_offset=%d cr[0x09]=0x%02x cr[0x17]=0x%02x linecmp=%d sr[0x01]=0x%02x\n",
>             width, height, v, line_offset, s->cr[9], s->cr[0x17], s->line_compare, s->sr[0x01]);
> @@ -1746,6 +1775,10 @@
>          return;
>      if (s->last_scr_width <= 0 || s->last_scr_height <= 0)
>          return;
> +
> +    /* disable dirty bit tracking */
> +    // kvm_log_stop();
> +
>      if (s->ds->depth == 8)
>          val = s->rgb_to_pixel(0, 0, 0);
>      else
> diff -r a5bcebe9e2bc hw/vga_int.h
> --- a/hw/vga_int.h	Wed Nov 05 12:09:13 2008 +0000
> +++ b/hw/vga_int.h	Wed Nov 05 14:29:54 2008 +0000
> @@ -106,6 +106,10 @@
>      unsigned int bios_size;                                             \
>      target_phys_addr_t base_ctrl;                                       \
>      int it_shift;                                                       \
> +    unsigned long lfb_addr;                                             \
> +    unsigned long lfb_end;                                              \
> +    uint32_t map_addr;                                                  \
> +    uint32_t map_end;                                                   \
>      PCIDevice *pci_dev;                                                 \
>      uint32_t latch;                                                     \
>      uint8_t sr_index;                                                   \

Re: [Qemu-devel] vga optmization

[Stefano Stabellini]

Glauber Costa wrote:

> On Wed, Nov 05, 2008 at 02:42:22PM +0000, Stefano Stabellini wrote:
>> Glauber Costa wrote:
>>
>>>> That's also how currently qemu-xen works.
>>>> I am glad that we agree :)
>>> I'm attaching a new version. Let me know if it's better this way.
>>>
>>
>>
>> Yes, this is much better thanks.
>>
>> I still have comments about possible improvements, so I wrote patch
>> (against your patch).
>> This patch is not meant to be applied, is only meant to be read (I believe
>> that C code is more meaningful than English :).
>>
>> Some of the changes include:
>>
>> - instead of adding cirrus_lfb_addr, add a more generic lfb_addr to
>> VGAState, so that can be reused in the future for possible stdvga only
>> mappings;
>>
>> - instead of using cirrus_lfb_mapped as a boolean, use it as the
>> mapping address, it is more useful that way;
>>
>> - instead of keeping the kvm dirty map always enabled, enable it only
>> when the framebuffer is linear and in graphical mode;
>>
>> - look at the changes to vga.c, there is a simple check to reduce the
>> dirty area to sync.
>>
>> It would be nice to check if the last two changes are actually a
>> performance improvement.
> I'm not opposed to the changes. I'll redo a patch and resend it today.
> However, I'm not planning to include the last optimization you sent.
> It is better that we do it in a separate commit. If you can get numbers,
> even better. I'll try it myself too.

Fine with me.