Re: [Qemu-devel] [PATCH 0/6] New shot at VGA optimization

Re: [Qemu-devel] [PATCH 0/6] New shot at VGA optimization

[Avi Kivity]

Glauber Costa wrote:
> Hi,
>
> The last version of the vga patch brought hope of a clean interface
> for the vga optimization. However, it failed in some guests due to
> the explicit need for alias, contrary to what we used to think.
>
> This needs arises from a KVM host bug (http://lkml.org/lkml/2008/11/17/496)
>
> However, even if it gets eventually fixed, any qemu running on an
> old enough kernel will fail. Because of that, this series introduces
> implicit aliasing support for kvm first, to just then implement the vga
> optimization.
>
> The main idea is that qemu should never need to know about our 
> dirty trick.
>
> Hope I didn't miss anything.
>
>   

Looks good.

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

Re: [Qemu-devel] [PATCH 0/6] New shot at VGA optimization

[Glauber Costa]

On Wed, Nov 19, 2008 at 12:23 PM, Avi Kivity <avi@redhat.com> wrote:
> Glauber Costa wrote:
>>
>> Hi,
>>
>> The last version of the vga patch brought hope of a clean interface
>> for the vga optimization. However, it failed in some guests due to
>> the explicit need for alias, contrary to what we used to think.
>>
>> This needs arises from a KVM host bug
>> (http://lkml.org/lkml/2008/11/17/496)
>>
>> However, even if it gets eventually fixed, any qemu running on an
>> old enough kernel will fail. Because of that, this series introduces
>> implicit aliasing support for kvm first, to just then implement the vga
>> optimization.
>>
>> The main idea is that qemu should never need to know about our dirty
>> trick.
>>
>> Hope I didn't miss anything.
>>
>>
>
> Looks good.

Thanks, so do you.



-- 
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 1/6] kvm: memory aliasing support for kvm

[Anthony Liguori]

Glauber Costa wrote:
> We use information encoded in phys_offset to support memory
> alias for kvm-all.c. Recall that for any given memory slot,
> userspace_addr = phys_ram_base + phys_offset (without any flags).
>
> This way, we can detect whether or not the registered slot is
> effectively pointing to the same area, regardless of the fact
> that it has a different guest_phys_addr.
>   

I think we've missed the abstraction here.  If you added an arch hook 
for kvm_set_phys_mem(), then you could have hidden the alias stuff 
entirely within target-i386/x86.c

But we can fix that up later.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 2/6] better type checking for vga

[Anthony Liguori]

Glauber Costa wrote:
> unsigned long is too bad of a type. Use ram_addr_t instead.
>
> Signed-off-by: Glauber Costa <glommer@redhat.com>
> ---
>  hw/cirrus_vga.c |    4 ++--
>  hw/vga.c        |    2 +-
>  hw/vga_int.h    |    2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> index af9c9e6..3cdc8e6 100644
> --- a/hw/cirrus_vga.c
> +++ b/hw/cirrus_vga.c
> @@ -3231,7 +3231,7 @@ static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci)
>   ***************************************/
>  
>  void isa_cirrus_vga_init(DisplayState *ds, uint8_t *vga_ram_base,
> -                         unsigned long vga_ram_offset, int vga_ram_size)
> +                         ram_addr_t vga_ram_offset, int vga_ram_size)
>  {
>      CirrusVGAState *s;
>  
> @@ -3273,7 +3273,7 @@ static void cirrus_pci_mmio_map(PCIDevice *d, int region_num,
>  }
>  
>  void pci_cirrus_vga_init(PCIBus *bus, DisplayState *ds, uint8_t *vga_ram_base,
> -                         unsigned long vga_ram_offset, int vga_ram_size)
> +                         ram_addr_t vga_ram_offset, int vga_ram_size)
>  {
>      PCICirrusVGAState *d;
>      uint8_t *pci_conf;
> diff --git a/hw/vga.c b/hw/vga.c
> index bd59aae..b44b77d 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -2105,7 +2105,7 @@ static void vga_map(PCIDevice *pci_dev, int region_num,
>  }
>  
>  void vga_common_init(VGAState *s, DisplayState *ds, uint8_t *vga_ram_base,
> -                     unsigned long vga_ram_offset, int vga_ram_size)
> +                     ram_addr_t vga_ram_offset, int vga_ram_size)
>   

You've changed the signature of this function (and others), without 
updating the definition in vga_int.h.  Compile fails.

Regards,

Antony Liguori

Re: [Qemu-devel] [PATCH 4/6] kvm: de-register mem region for MMIO.

[Anthony Liguori]

Glauber Costa wrote:
> 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.
>   

Applied.  Thanks.

Regards,

Anthony Liguori

> Signed-off-by: Glauber Costa <glommer@redhat.com>
> ---
>  kvm-all.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/kvm-all.c b/kvm-all.c
> index b5bbdcb..9700e50 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -344,7 +344,7 @@ 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)) {
>              mem->memory_size = 0;
>              mem->guest_phys_addr = start_addr;
>              mem->userspace_addr = 0;
>   

Re: [Qemu-devel] [PATCH 5/6] 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 |  153 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  kvm.h     |    5 ++
>  2 files changed, 158 insertions(+), 0 deletions(-)
>
> diff --git a/kvm-all.c b/kvm-all.c
> index 9700e50..c522205 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -32,8 +32,13 @@
>      do { } while (0)
>  #endif
>  
> +#define warning(fmt, ...) \
> +    do { dprintf("%s:%d" fmt, __func__, __LINE__, ## __VA_ARGS__); } while (0)
>   

I don't think this macro really serves a purpose.  You can just add 
"%s:%d: " to the beginning of dprintf() if you want.

Also note that it gives goofy output right now because of the missing 
space after %d.

> +/*
> + * 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 end_addr)
> +{
> +        KVMState *s = kvm_state;
> +        KVMSlot *mem = kvm_lookup_slot(s, phys_addr);
> +
> +        /*
> +         * We don't need to do dirty tracking of alias slots. If we track the
> +         * memory the alias is pointing to, it should be enough
> +         */
> +        if (kvm_arch_is_alias_slot(phys_addr))
> +            return 0;
> +
> +        if (mem == NULL)  {
> +                warning("invalid parameters %llx-%llx\n", phys_addr, end_addr);
> +                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 end_addr)
> +{
> +
> +        KVMState *s = kvm_state;
> +        KVMSlot *mem = kvm_lookup_slot(s, phys_addr);
> +
> +        if (kvm_arch_is_alias_slot(phys_addr))
> +            return 0;
> +
> +        if (mem == NULL) {
> +                warning("invalid parameters %llx - %llx \n", phys_addr, end_addr);
> +                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);
> +}
>   

Note that start and stop are identical except for a different printf().  
The call a common helper function.  Why not fold everything into 
kvm_dirty_pages_log_change() and make that the public interface 
(kvm_set_log).

> +static inline int lookup_bitmap_phys(unsigned long *dirty, unsigned nr)
> +{
> +    unsigned word = nr / (sizeof(*dirty) * 8);
> +    unsigned bit = nr % (sizeof(*dirty) * 8);
> +    int ret;
> +
> +    ret = (dirty[word] >> bit) & 1;
> +    return ret;
> +}
> +
> +/**
> + * kvm_physical_sync_dirty_bitmap - Grab dirty bitmap from kernel space
> + * If a phys_offset parameter is given, this function updates qemu's dirty
> + * bitmap using cpu_physical_memory_set_dirty(). This means all bits are set
> + * to dirty.
> + *
> + * @start_add: start of logged region. This is what we use to search the memslot
> + * @end_addr: end of logged region. Only matters if we are updating qemu dirty bitmap.
> + * @phys_offset: the phys_offset we want to use for qemu dirty bitmap update. Passing
> + *               NULL makes the update not happen. In this case, we only grab the bitmap
> + *               from kernel.
> + */
> +void kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr, target_phys_addr_t end_addr,
> +                                    ram_addr_t phys_offset)
>   

This interface is weird and broken.  If we wanted to use this for live 
migration, we would end up passing phys_offset=0 but that has a special 
meaning here.

But why are we passing phys_offset at all?  Why can't we do the lookup here?

Regards,

Anthony Liguori

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

[Anthony Liguori]

Glauber Costa wrote:
> @@ -1383,6 +1400,7 @@ cirrus_hook_write_sr(CirrusVGAState * s, unsigned reg_index, int reg_value)
>  	       reg_index, reg_value);
>  #endif
>  	break;
> +
>      case 0x17:			// Configuration Readback and Extended Control
>   

Please don't introduce stray whitespace.

>  	s->sr[reg_index] = (s->sr[reg_index] & 0x38) | (reg_value & 0xc7);
>          cirrus_update_memory_access(s);
> @@ -1528,12 +1546,13 @@ cirrus_hook_write_gr(CirrusVGAState * s, unsigned reg_index, int reg_value)
>  	s->gr[reg_index] = reg_value;
>  	cirrus_update_bank_ptr(s, 0);
>  	cirrus_update_bank_ptr(s, 1);
> -        break;
> +    cirrus_update_memory_access(s);
> +    break;
>   

This formatting seems way off?

>      case 0x0B:
>  	s->gr[reg_index] = reg_value;
>  	cirrus_update_bank_ptr(s, 0);
>  	cirrus_update_bank_ptr(s, 1);
> -        cirrus_update_memory_access(s);
> +    cirrus_update_memory_access(s);
>  	break;
>   

And this is just a reformatting change.  Please avoid these.

>      case 0x10:			// BGCOLOR 0x0000ff00
>      case 0x11:			// FGCOLOR 0x0000ff00
> @@ -2618,6 +2637,48 @@ static CPUWriteMemoryFunc *cirrus_linear_bitblt_write[3] = {
>      cirrus_linear_bitblt_writel,
>  };
>  
> +static void map_linear_vram(CirrusVGAState *s)
> +{
> +
> +    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->map_addr)
> +        return;
> +
> +    if (cirrus_lfb_is_mapped(s)) {
> +        cpu_register_physical_memory(isa_mem_base + 0xa0000, 0x8000,
> +                                    (s->vram_offset + s->cirrus_bank_base[0]) | IO_MEM_RAM);
> +        cpu_register_physical_memory(isa_mem_base + 0xa8000, 0x8000,
> +                                    (s->vram_offset + s->cirrus_bank_base[1]) | IO_MEM_RAM);
>   

Isn't necessary to reregister 0xa0000 too?

> +        if (kvm_enabled()) {
> +            kvm_log_start(0xa0000, 0x8000);
> +            kvm_log_start(0xa8000, 0x8000);
> +        }
>   

Why would you enable logging on a different region from what you've 
registered?  Shouldn't you enable logging on both regions?  If we're 
going to enable logging based on target_phys_addr_t instead of 
ram_addr_t (and I think we should), then we should enable it on all 
target_phys_addr_ts.

> +    }
> +    else {
>   

This is formatted incorrectly.

>  
> +
>  /*
>   * graphic modes
>   */
>   

More extra whitespace.

Regards,

Anthony Liguori

[Qemu-devel] [PATCH 0/6] New shot at VGA optimization

[Glauber Costa]

Hi,

The last version of the vga patch brought hope of a clean interface
for the vga optimization. However, it failed in some guests due to
the explicit need for alias, contrary to what we used to think.

This needs arises from a KVM host bug (http://lkml.org/lkml/2008/11/17/496)

However, even if it gets eventually fixed, any qemu running on an
old enough kernel will fail. Because of that, this series introduces
implicit aliasing support for kvm first, to just then implement the vga
optimization.

The main idea is that qemu should never need to know about our 
dirty trick.

Hope I didn't miss anything.

[Qemu-devel] [PATCH 1/6] kvm: memory aliasing support for kvm

[Glauber Costa]

We use information encoded in phys_offset to support memory
alias for kvm-all.c. Recall that for any given memory slot,
userspace_addr = phys_ram_base + phys_offset (without any flags).

This way, we can detect whether or not the registered slot is
effectively pointing to the same area, regardless of the fact
that it has a different guest_phys_addr.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 kvm-all.c         |   36 +++++++++++++++++++++++++-
 kvm.h             |    6 ++++
 target-i386/kvm.c |   72 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 113 insertions(+), 1 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index d3fcf8b..b5bbdcb 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -75,6 +75,28 @@ static KVMSlot *kvm_lookup_slot(KVMState *s, target_phys_addr_t start_addr)
     return NULL;
 }
 
+/*
+ * If we are trying to register a slot that points to the same address
+ * (phys_offset) of an already existing slot, data should go to the same
+ * place regardless of to which guest_phys_addr we write to. When we are
+ * using the kvm_alias API, it means it should be an alias.
+ */
+static KVMSlot *kvm_should_alias(KVMState *s, ram_addr_t phys_offset)
+{
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(s->slots); i++) {
+        KVMSlot *mem = &s->slots[i];
+
+        ram_addr_t addr = mem->userspace_addr - (ram_addr_t)phys_ram_base;
+
+        if (phys_offset >= addr &&
+            phys_offset < (addr + mem->memory_size))
+            return mem;
+    }
+    return NULL;
+}
+
 int kvm_init_vcpu(CPUState *env)
 {
     KVMState *s = kvm_state;
@@ -315,7 +337,7 @@ void kvm_set_phys_mem(target_phys_addr_t start_addr,
 {
     KVMState *s = kvm_state;
     ram_addr_t flags = phys_offset & ~TARGET_PAGE_MASK;
-    KVMSlot *mem;
+    KVMSlot *mem, *as;
 
     /* KVM does not support read-only slots */
     phys_offset &= ~IO_MEM_ROM;
@@ -369,10 +391,22 @@ void kvm_set_phys_mem(target_phys_addr_t start_addr,
             abort();
         }
     }
+
+    if (((flags == IO_MEM_UNASSIGNED) || (flags >= TLB_MMIO))
+        && (kvm_arch_destroy_alias_slot(s, start_addr)))
+            return;
+
     /* KVM does not need to know about this memory */
     if (flags >= IO_MEM_UNASSIGNED)
         return;
 
+    if ((as = kvm_should_alias(s, phys_offset))) {
+        target_phys_addr_t target_addr = as->guest_phys_addr;
+        target_addr += phys_offset - (as->userspace_addr - (ram_addr_t)phys_ram_base);
+        kvm_arch_set_alias_slot(s, start_addr, size, target_addr); 
+        return;
+    }
+
     mem = kvm_alloc_slot(s);
     mem->memory_size = size;
     mem->guest_phys_addr = start_addr;
diff --git a/kvm.h b/kvm.h
index 304de27..7588601 100644
--- a/kvm.h
+++ b/kvm.h
@@ -51,6 +51,12 @@ int kvm_vcpu_ioctl(CPUState *env, int type, ...);
 
 /* Arch specific hooks */
 
+int kvm_arch_is_alias_slot(target_phys_addr_t start_addr);
+int kvm_arch_destroy_alias_slot(KVMState *s, target_phys_addr_t start_addr);
+void kvm_arch_set_alias_slot(KVMState *s, target_phys_addr_t start_addr,
+                             ram_addr_t size,
+                             target_phys_addr_t target_addr);
+
 int kvm_arch_post_run(CPUState *env, struct kvm_run *run);
 
 int kvm_arch_handle_exit(CPUState *env, struct kvm_run *run);
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 3f60654..4ede68a 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -33,6 +33,74 @@
     do { } while (0)
 #endif
 
+typedef struct kvm_memory_alias KVMAliasSlot;
+
+static KVMAliasSlot alias_slots[8];
+
+static KVMAliasSlot *kvm_lookup_alias_slot(target_phys_addr_t start_addr) 
+{ 
+    int i; 
+        
+    for (i = 0; i < ARRAY_SIZE(alias_slots); i++) { 
+        KVMAliasSlot *as = &alias_slots[i]; 
+ 
+        if (start_addr >= as->guest_phys_addr && 
+            start_addr < (as->guest_phys_addr + as->memory_size)) 
+            return as; 
+    } 
+ 
+    return NULL; 
+}
+
+int kvm_arch_is_alias_slot(target_phys_addr_t start_addr)
+{
+    return !!kvm_lookup_alias_slot(start_addr);
+}
+
+static KVMAliasSlot *kvm_alloc_alias_slot(void)
+{
+    int i; 
+            
+    for (i = 0; i < ARRAY_SIZE(alias_slots); i++) {
+        if (alias_slots[i].memory_size == 0)
+            return &alias_slots[i];
+    }
+
+    return NULL;
+}
+
+int kvm_arch_destroy_alias_slot(KVMState *s, target_phys_addr_t start_addr)
+{
+    
+    KVMAliasSlot *as = kvm_lookup_alias_slot(start_addr);
+    if (!as)
+        return 0;
+    as->memory_size = 0;
+    as->target_phys_addr = 0;
+
+    kvm_vm_ioctl(s, KVM_SET_MEMORY_ALIAS, as);
+    return 1;
+}
+
+void kvm_arch_set_alias_slot(KVMState *s, target_phys_addr_t start_addr,
+                      ram_addr_t size,
+                      target_phys_addr_t target_addr)
+{
+
+    int r;
+    KVMAliasSlot *as = kvm_lookup_alias_slot(start_addr);
+
+    if (!as)
+        as = kvm_alloc_alias_slot();
+
+    as->guest_phys_addr = (uint64_t)start_addr;
+    as->memory_size = (uint64_t)size;
+    as->target_phys_addr = (uint64_t)target_addr;
+    as->flags = 0;
+
+    r = kvm_vm_ioctl(s, KVM_SET_MEMORY_ALIAS, as);
+}
+
 int kvm_arch_init_vcpu(CPUState *env)
 {
     struct {
@@ -42,6 +110,10 @@ int kvm_arch_init_vcpu(CPUState *env)
     int limit, i, cpuid_i;
     uint32_t eax, ebx, ecx, edx;
 
+
+    for (i = 0; i < ARRAY_SIZE(alias_slots); i++)
+        alias_slots[i].slot = i;
+
     cpuid_i = 0;
 
     cpu_x86_cpuid(env, 0, &eax, &ebx, &ecx, &edx);
-- 
1.5.6.5

[Qemu-devel] [PATCH 2/6] better type checking for vga

[Glauber Costa]

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

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

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

[Qemu-devel] [PATCH 3/6] 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 3cdc8e6..85359a8 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -3117,7 +3117,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 +3156,10 @@ static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci)
     register_ioport_read(0x3ba, 1, 1, vga_ioport_read, s);
     register_ioport_read(0x3da, 1, 1, vga_ioport_read, s);
 
-    vga_io_memory = cpu_register_io_memory(0, cirrus_vga_mem_read,
+    s->vga_io_memory = cpu_register_io_memory(0, cirrus_vga_mem_read,
                                            cirrus_vga_mem_write, s);
     cpu_register_physical_memory(isa_mem_base + 0x000a0000, 0x20000,
-                                 vga_io_memory);
+                                 s->vga_io_memory);
 
     s->sr[0x06] = 0x0f;
     if (device_id == CIRRUS_ID_CLGD5446) {
diff --git a/hw/vga_int.h b/hw/vga_int.h
index d559fcf..91a8d77 100644
--- a/hw/vga_int.h
+++ b/hw/vga_int.h
@@ -129,6 +129,7 @@ typedef void (* vga_update_retrace_info_fn)(struct VGAState *s);
     int dac_8bit;                                                       \
     uint8_t palette[768];                                               \
     int32_t bank_offset;                                                \
+    int vga_io_memory;                                             \
     int (*get_bpp)(struct VGAState *s);                                 \
     void (*get_offsets)(struct VGAState *s,                             \
                         uint32_t *pline_offset,                         \
-- 
1.5.6.5

[Qemu-devel] [PATCH 4/6] kvm: 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 |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index b5bbdcb..9700e50 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -344,7 +344,7 @@ 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)) {
             mem->memory_size = 0;
             mem->guest_phys_addr = start_addr;
             mem->userspace_addr = 0;
-- 
1.5.6.5

[Qemu-devel] [PATCH 5/6] 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 |  153 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 kvm.h     |    5 ++
 2 files changed, 158 insertions(+), 0 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 9700e50..c522205 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -32,8 +32,13 @@
     do { } while (0)
 #endif
 
+#define warning(fmt, ...) \
+    do { dprintf("%s:%d" fmt, __func__, __LINE__, ## __VA_ARGS__); } while (0)
+
 typedef struct kvm_userspace_memory_region KVMSlot;
 
+typedef struct kvm_dirty_log KVMDirtyLog;
+
 int kvm_allowed = 0;
 
 struct KVMState
@@ -134,6 +139,154 @@ 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 end_addr)
+{
+        KVMState *s = kvm_state;
+        KVMSlot *mem = kvm_lookup_slot(s, phys_addr);
+
+        /*
+         * We don't need to do dirty tracking of alias slots. If we track the
+         * memory the alias is pointing to, it should be enough
+         */
+        if (kvm_arch_is_alias_slot(phys_addr))
+            return 0;
+
+        if (mem == NULL)  {
+                warning("invalid parameters %llx-%llx\n", phys_addr, end_addr);
+                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 end_addr)
+{
+
+        KVMState *s = kvm_state;
+        KVMSlot *mem = kvm_lookup_slot(s, phys_addr);
+
+        if (kvm_arch_is_alias_slot(phys_addr))
+            return 0;
+
+        if (mem == NULL) {
+                warning("invalid parameters %llx - %llx \n", phys_addr, end_addr);
+                return -EINVAL;
+        }
+
+        /* Not logging, no need to issue ioctl */
+        if (!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES))
+            return 0;
+
+        dprintf("slot %d: disabling logging\n", mem->slot);
+        return kvm_dirty_pages_log_change(mem,
+                                          0,
+                                          KVM_MEM_LOG_DIRTY_PAGES);
+}
+
+static inline int lookup_bitmap_phys(unsigned long *dirty, unsigned nr)
+{
+    unsigned word = nr / (sizeof(*dirty) * 8);
+    unsigned bit = nr % (sizeof(*dirty) * 8);
+    int ret;
+
+    ret = (dirty[word] >> bit) & 1;
+    return ret;
+}
+
+/**
+ * kvm_physical_sync_dirty_bitmap - Grab dirty bitmap from kernel space
+ * If a phys_offset parameter is given, this function updates qemu's dirty
+ * bitmap using cpu_physical_memory_set_dirty(). This means all bits are set
+ * to dirty.
+ *
+ * @start_add: start of logged region. This is what we use to search the memslot
+ * @end_addr: end of logged region. Only matters if we are updating qemu dirty bitmap.
+ * @phys_offset: the phys_offset we want to use for qemu dirty bitmap update. Passing
+ *               NULL makes the update not happen. In this case, we only grab the bitmap
+ *               from kernel.
+ */
+void kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr, target_phys_addr_t end_addr,
+                                    ram_addr_t phys_offset)
+{
+    KVMState *s = kvm_state;
+    KVMDirtyLog d;
+    KVMSlot *mem = kvm_lookup_slot(s, start_addr);
+    unsigned long alloc_size;
+    ram_addr_t addr;
+    target_phys_addr_t phys_addr = start_addr;
+
+    if (kvm_arch_is_alias_slot(phys_addr))
+         return;
+
+    dprintf("sync addr: %llx into %lx\n", start_addr, phys_offset);
+    if (mem == NULL) {
+            fprintf(stderr, "BUG: %s: invalid parameters\n", __func__);
+            return;
+    }
+
+    alloc_size = mem->memory_size >> TARGET_PAGE_BITS / sizeof(d.dirty_bitmap);
+    d.dirty_bitmap = qemu_mallocz(alloc_size);
+
+    if (d.dirty_bitmap == NULL) {
+        warning("Could not allocate dirty bitmap\n");
+        return;
+    }
+
+    d.slot = mem->slot;
+    dprintf("slot %d, phys_addr %llx, uaddr: %llx\n",
+            d.slot, mem->guest_phys_addr, mem->userspace_addr);
+
+    if (kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d) == -1) {
+        warning("ioctl failed %d\n", errno);
+        goto out;
+    }
+
+    /* Caller don't want to update dirty bitmap */
+    if (!phys_offset)
+        goto out;
+
+    phys_addr = start_addr;
+    for (addr = phys_offset; phys_addr < end_addr; phys_addr+= TARGET_PAGE_SIZE, addr += TARGET_PAGE_SIZE) {
+        unsigned long *bitmap = (unsigned long *)d.dirty_bitmap;
+        if (lookup_bitmap_phys(bitmap, (phys_addr - start_addr) >> TARGET_PAGE_BITS))
+            cpu_physical_memory_set_dirty(addr);
+    }
+out:
+    qemu_free(d.dirty_bitmap);
+}
+
 int kvm_init(int smp_cpus)
 {
     KVMState *s;
diff --git a/kvm.h b/kvm.h
index 7588601..cefda99 100644
--- a/kvm.h
+++ b/kvm.h
@@ -38,6 +38,11 @@ void kvm_set_phys_mem(target_phys_addr_t start_addr,
                       ram_addr_t size,
                       ram_addr_t phys_offset);
 
+void kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr, target_phys_addr_t end_addr,
+                                    ram_addr_t phys_offset);
+
+int kvm_log_start(target_phys_addr_t phys_addr, target_phys_addr_t len);
+int kvm_log_stop(target_phys_addr_t phys_addr, target_phys_addr_t len);
 /* internal API */
 
 struct KVMState;
-- 
1.5.6.5

[Qemu-devel] [PATCH 6/6] kvm: vga optimization.

[Glauber Costa]

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

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

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

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

diff --git a/cpu-all.h b/cpu-all.h
index cdd79bc..6ce2bde 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -934,6 +934,8 @@ int cpu_physical_memory_set_dirty_tracking(int enable);
 
 int cpu_physical_memory_get_dirty_tracking(void);
 
+void cpu_physical_sync_dirty_bitmap(target_phys_addr_t start_addr, target_phys_addr_t end_addr, ram_addr_t phys_offset);
+
 void dump_exec_info(FILE *f,
                     int (*cpu_fprintf)(FILE *f, const char *fmt, ...));
 
diff --git a/exec.c b/exec.c
index 1edc737..a971229 100644
--- a/exec.c
+++ b/exec.c
@@ -1822,6 +1822,13 @@ int cpu_physical_memory_get_dirty_tracking(void)
     return in_migration;
 }
 
+void cpu_physical_sync_dirty_bitmap(target_phys_addr_t start_addr, target_phys_addr_t end_addr,
+                                    ram_addr_t phys_offset)
+{
+    if (kvm_enabled())
+        kvm_physical_sync_dirty_bitmap(start_addr, end_addr, phys_offset);
+}
+
 static inline void tlb_update_dirty(CPUTLBEntry *tlb_entry)
 {
     ram_addr_t ram_addr;
diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 85359a8..7fa4c8a 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:
@@ -1192,6 +1193,14 @@ static void cirrus_get_resolution(VGAState *s, int *pwidth, int *pheight)
     *pheight = height;
 }
 
+static int cirrus_lfb_is_mapped(CirrusVGAState *s)
+{
+    return (!(s->cirrus_srcptr != s->cirrus_srcptr_end)
+        && !((s->sr[0x07] & 0x01) == 0)
+        && !((s->gr[0x0B] & 0x14) == 0x14)
+        && !(s->gr[0x0B] & 0x02));
+}
+
 /***************************************
  *
  * bank memory
@@ -1228,6 +1237,13 @@ static void cirrus_update_bank_ptr(CirrusVGAState * s, unsigned bank_index)
     }
 
     if (limit > 0) {
+        /* Thinking about changing bank base? First, drop the dirty bitmap information
+         * on the current location, otherwise we lose this pointer forever */
+        if (cirrus_lfb_is_mapped(s)) {
+            ram_addr_t phys_offset = s->vram_offset + s->cirrus_bank_base[bank_index];
+            target_phys_addr_t base_addr = isa_mem_base + 0xa0000 + bank_index * 0x8000;
+            cpu_physical_sync_dirty_bitmap(base_addr, base_addr + 0x8000, phys_offset);
+        }
 	s->cirrus_bank_base[bank_index] = offset;
 	s->cirrus_bank_limit[bank_index] = limit;
     } else {
@@ -1356,6 +1372,7 @@ cirrus_hook_write_sr(CirrusVGAState * s, unsigned reg_index, int reg_value)
 	s->hw_cursor_y = (reg_value << 3) | (reg_index >> 5);
 	break;
     case 0x07:			// Extended Sequencer Mode
+        cirrus_update_memory_access(s);
     case 0x08:			// EEPROM Control
     case 0x09:			// Scratch Register 0
     case 0x0a:			// Scratch Register 1
@@ -1383,6 +1400,7 @@ cirrus_hook_write_sr(CirrusVGAState * s, unsigned reg_index, int reg_value)
 	       reg_index, reg_value);
 #endif
 	break;
+
     case 0x17:			// Configuration Readback and Extended Control
 	s->sr[reg_index] = (s->sr[reg_index] & 0x38) | (reg_value & 0xc7);
         cirrus_update_memory_access(s);
@@ -1528,12 +1546,13 @@ cirrus_hook_write_gr(CirrusVGAState * s, unsigned reg_index, int reg_value)
 	s->gr[reg_index] = reg_value;
 	cirrus_update_bank_ptr(s, 0);
 	cirrus_update_bank_ptr(s, 1);
-        break;
+    cirrus_update_memory_access(s);
+    break;
     case 0x0B:
 	s->gr[reg_index] = reg_value;
 	cirrus_update_bank_ptr(s, 0);
 	cirrus_update_bank_ptr(s, 1);
-        cirrus_update_memory_access(s);
+    cirrus_update_memory_access(s);
 	break;
     case 0x10:			// BGCOLOR 0x0000ff00
     case 0x11:			// FGCOLOR 0x0000ff00
@@ -2618,6 +2637,48 @@ static CPUWriteMemoryFunc *cirrus_linear_bitblt_write[3] = {
     cirrus_linear_bitblt_writel,
 };
 
+static void map_linear_vram(CirrusVGAState *s)
+{
+
+    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->map_addr)
+        return;
+
+    if (cirrus_lfb_is_mapped(s)) {
+        cpu_register_physical_memory(isa_mem_base + 0xa0000, 0x8000,
+                                    (s->vram_offset + s->cirrus_bank_base[0]) | IO_MEM_RAM);
+        cpu_register_physical_memory(isa_mem_base + 0xa8000, 0x8000,
+                                    (s->vram_offset + s->cirrus_bank_base[1]) | IO_MEM_RAM);
+
+        if (kvm_enabled()) {
+            kvm_log_start(0xa0000, 0x8000);
+            kvm_log_start(0xa8000, 0x8000);
+        }
+    }
+    else {
+        cpu_register_physical_memory(isa_mem_base + 0xa0000, 0x8000, s->vga_io_memory);
+        cpu_register_physical_memory(isa_mem_base + 0xa8000, 0x8000, s->vga_io_memory);
+    }
+
+}
+
+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 + 0xa0000, 0x20000,
+                                 s->vga_io_memory);
+}
+
 /* Compute the memory access functions */
 static void cirrus_update_memory_access(CirrusVGAState *s)
 {
@@ -2636,11 +2697,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;
@@ -3102,6 +3165,7 @@ static int cirrus_vga_load(QEMUFile *f, void *opaque, int version_id)
     qemu_get_be32s(f, &s->hw_cursor_x);
     qemu_get_be32s(f, &s->hw_cursor_y);
 
+    cirrus_update_memory_access(s);
     /* force refresh */
     s->graphic_mode = -1;
     cirrus_update_bank_ptr(s, 0);
@@ -3261,6 +3325,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 b44b77d..a5927bd 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;
 
@@ -1556,6 +1559,7 @@ void vga_invalidate_scanlines(VGAState *s, int y1, int y2)
     }
 }
 
+
 /*
  * graphic modes
  */
@@ -1568,6 +1572,9 @@ static void vga_draw_graphic(VGAState *s, int full_update)
     uint32_t v, addr1, addr;
     vga_draw_line_func *vga_draw_line;
 
+    cpu_physical_sync_dirty_bitmap(s->map_addr, s->map_end, s->vram_offset);
+    vga_dirty_log_start(s);
+
     full_update |= update_basic_params(s);
 
     s->get_resolution(s, &width, &height);
@@ -1743,6 +1750,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 +2101,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 +2123,11 @@ static void vga_map(PCIDevice *pci_dev, int region_num,
     } else {
         cpu_register_physical_memory(addr, s->vram_size, s->vram_offset);
     }
+
+    s->map_addr = addr;
+    s->map_end = addr + VGA_RAM_SIZE;
+
+    vga_dirty_log_start(s);
 }
 
 void vga_common_init(VGAState *s, DisplayState *ds, uint8_t *vga_ram_base,
diff --git a/hw/vga_int.h b/hw/vga_int.h
index 91a8d77..d1d35f9 100644
--- a/hw/vga_int.h
+++ b/hw/vga_int.h
@@ -102,6 +102,10 @@ typedef void (* vga_update_retrace_info_fn)(struct VGAState *s);
     uint8_t *vram_ptr;                                                  \
     ram_addr_t vram_offset;                                             \
     unsigned int vram_size;                                             \
+    uint32_t lfb_addr;                                                  \
+    uint32_t lfb_end;                                                   \
+    uint32_t map_addr;                                                  \
+    uint32_t map_end;                                                   \
     unsigned long bios_offset;                                          \
     unsigned int bios_size;                                             \
     target_phys_addr_t base_ctrl;                                       \
@@ -189,6 +193,10 @@ static inline int c6_to_8(int v)
 void vga_common_init(VGAState *s, DisplayState *ds, uint8_t *vga_ram_base,
                      unsigned long vga_ram_offset, int vga_ram_size);
 void vga_init(VGAState *s);
+
+void vga_dirty_log_start(VGAState *s);
+void vga_dirty_log_stop(VGAState *s);
+
 uint32_t vga_mem_readb(void *opaque, target_phys_addr_t addr);
 void vga_mem_writeb(void *opaque, target_phys_addr_t addr, uint32_t val);
 void vga_invalidate_scanlines(VGAState *s, int y1, int y2);
-- 
1.5.6.5

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

[Glauber Costa]

>> +    if (!s->map_addr)
>> +        return;
>> +
>> +    if (cirrus_lfb_is_mapped(s)) {
>> +        cpu_register_physical_memory(isa_mem_base + 0xa0000, 0x8000,
>> +                                    (s->vram_offset +
>> s->cirrus_bank_base[0]) | IO_MEM_RAM);
>> +        cpu_register_physical_memory(isa_mem_base + 0xa8000, 0x8000,
>> +                                    (s->vram_offset +
>> s->cirrus_bank_base[1]) | IO_MEM_RAM);
>>
>
> Isn't necessary to reregister 0xa0000 too?

ENOFOLLOW. This is exactly what I'm doing.
>
>> +        if (kvm_enabled()) {
>> +            kvm_log_start(0xa0000, 0x8000);
>> +            kvm_log_start(0xa8000, 0x8000);
>> +        }
>>
>
> Why would you enable logging on a different region from what you've
> registered?  Shouldn't you enable logging on both regions?  If we're going
> to enable logging based on target_phys_addr_t instead of ram_addr_t (and I
> think we should), then we should enable it on all target_phys_addr_ts.

Again, I don't follow. We map 0xa0000 and 0xa8000 to some ram_addr_t,
and then enable logging in the very 0xa0000 and 0xa8000. What's the problem
with that? One late nitpick, it is that for consistency, I registered
0xa0000 + isa_mem_base,
(usually 0), and should use it in kvm_log_start.

>> +    }
>> +    else {
>>
>
> This is formatted incorrectly.
>
>>  +
>>  /*
>>  * graphic modes
>>  */
>>
>
> More extra whitespace.
>
> Regards,
>
> Anthony Liguori
>
>
>



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

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

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

[Glauber Costa]

On Wed, Nov 19, 2008 at 1:18 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> 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 |  153
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  kvm.h     |    5 ++
>>  2 files changed, 158 insertions(+), 0 deletions(-)
>>
>> diff --git a/kvm-all.c b/kvm-all.c
>> index 9700e50..c522205 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -32,8 +32,13 @@
>>     do { } while (0)
>>  #endif
>>  +#define warning(fmt, ...) \
>> +    do { dprintf("%s:%d" fmt, __func__, __LINE__, ## __VA_ARGS__); }
>> while (0)
>>
>
> I don't think this macro really serves a purpose.  You can just add "%s:%d:
> " to the beginning of dprintf() if you want.

Are you okay with all dprintfs having these info? This is very
valuable info, so I'd
like to have it. I  can send a separate patch.

>>
>
> Note that start and stop are identical except for a different printf().  The
> call a common helper function.  Why not fold everything into
> kvm_dirty_pages_log_change() and make that the public interface
> (kvm_set_log).

I personally believe kvm_set_log is a very bad interface. It's nicer
to read "start"
and "stop" instead, but I can definitely do this internally.

>
>> +static inline int lookup_bitmap_phys(unsigned long *dirty, unsigned nr)
>> +{
>> +    unsigned word = nr / (sizeof(*dirty) * 8);
>> +    unsigned bit = nr % (sizeof(*dirty) * 8);
>> +    int ret;
>> +
>> +    ret = (dirty[word] >> bit) & 1;
>> +    return ret;
>> +}
>> +
>> +/**
>> + * kvm_physical_sync_dirty_bitmap - Grab dirty bitmap from kernel space
>> + * If a phys_offset parameter is given, this function updates qemu's
>> dirty
>> + * bitmap using cpu_physical_memory_set_dirty(). This means all bits are
>> set
>> + * to dirty.
>> + *
>> + * @start_add: start of logged region. This is what we use to search the
>> memslot
>> + * @end_addr: end of logged region. Only matters if we are updating qemu
>> dirty bitmap.
>> + * @phys_offset: the phys_offset we want to use for qemu dirty bitmap
>> update. Passing
>> + *               NULL makes the update not happen. In this case, we only
>> grab the bitmap
>> + *               from kernel.
>> + */
>> +void kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
>> target_phys_addr_t end_addr,
>> +                                    ram_addr_t phys_offset)
>>
>
> This interface is weird and broken.  If we wanted to use this for live
> migration, we would end up passing phys_offset=0 but that has a special
> meaning here.
>
> But why are we passing phys_offset at all?  Why can't we do the lookup here?
Because, if you remember, the last time I sent a patch _without_ it,
you complained
that we can't really trust any translation based on userspace_addr.
Which is fine,
because it is fundamentally broken long term.

So, we need to be specific about what area are we writting the dirty bitmap too.
If the zero is the problem, we can have the interface to always write
to the given location,
and disregard 0 as a special meaning field.


-- 
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 6/6] kvm: vga optimization.

[Anthony Liguori]

Glauber Costa wrote:
>>> +    if (!s->map_addr)
>>> +        return;
>>> +
>>> +    if (cirrus_lfb_is_mapped(s)) {
>>> +        cpu_register_physical_memory(isa_mem_base + 0xa0000, 0x8000,
>>> +                                    (s->vram_offset +
>>> s->cirrus_bank_base[0]) | IO_MEM_RAM);
>>> +        cpu_register_physical_memory(isa_mem_base + 0xa8000, 0x8000,
>>> +                                    (s->vram_offset +
>>> s->cirrus_bank_base[1]) | IO_MEM_RAM);
>>>
>>>       
>> Isn't necessary to reregister 0xa0000 too?
>>     
>
> ENOFOLLOW. This is exactly what I'm doing.
>   

I was confused about isa_mem_base.  Just ignore me :-).

Regards,

Anthony Liguori

>>> +        if (kvm_enabled()) {
>>> +            kvm_log_start(0xa0000, 0x8000);
>>> +            kvm_log_start(0xa8000, 0x8000);
>>> +        }
>>>
>>>       
>> Why would you enable logging on a different region from what you've
>> registered?  Shouldn't you enable logging on both regions?  If we're going
>> to enable logging based on target_phys_addr_t instead of ram_addr_t (and I
>> think we should), then we should enable it on all target_phys_addr_ts.
>>     
>
> Again, I don't follow. We map 0xa0000 and 0xa8000 to some ram_addr_t,
> and then enable logging in the very 0xa0000 and 0xa8000. What's the problem
> with that? One late nitpick, it is that for consistency, I registered
> 0xa0000 + isa_mem_base,
> (usually 0), and should use it in kvm_log_start.
>
>   
>>> +    }
>>> +    else {
>>>
>>>       
>> This is formatted incorrectly.
>>
>>     
>>>  +
>>>  /*
>>>  * graphic modes
>>>  */
>>>
>>>       
>> More extra whitespace.
>>
>> Regards,
>>
>> Anthony Liguori
>>
>>
>>
>>     
>
>
>
>   

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

[Anthony Liguori]

Glauber Costa wrote:
> On Wed, Nov 19, 2008 at 1:18 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
>   
>
> Are you okay with all dprintfs having these info? This is very
> valuable info, so I'd
> like to have it. I  can send a separate patch.
>   

Yes.

>> Note that start and stop are identical except for a different printf().  The
>> call a common helper function.  Why not fold everything into
>> kvm_dirty_pages_log_change() and make that the public interface
>> (kvm_set_log).
>>     
>
> I personally believe kvm_set_log is a very bad interface. It's nicer
> to read "start"
> and "stop" instead, but I can definitely do this internally.
>   

It's not that important to me.  I'm more interested in removing the 
duplicated code.

>> This interface is weird and broken.  If we wanted to use this for live
>> migration, we would end up passing phys_offset=0 but that has a special
>> meaning here.
>>
>> But why are we passing phys_offset at all?  Why can't we do the lookup here?
>>     
> Because, if you remember, the last time I sent a patch _without_ it,
> you complained
> that we can't really trust any translation based on userspace_addr.
>   

But now that we have phys_offset, we know what the phys_offset is of a 
given slot, right?  So can't we just use that?

Regards,

Anthony Liguori