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