[Qemu-devel] Re: [PATCH 05/14] Change cpu_phys mem callback to use [offset, size) bounds instead of [start, end).

[Jan Kiszka <jan.kiszka@siemens.com>]

Zachary Amsden wrote:
> Signed-off-by: Zachary Amsden <zamsden@redhat.com>
> ---
>  cpu-all.h        |    2 +-
>  exec.c           |    4 ++--
>  hw/cirrus_vga.c  |   11 ++++++-----
>  hw/framebuffer.c |    2 +-
>  hw/vga.c         |   12 ++++++------
>  hw/vga_int.h     |    2 +-
>  kvm-all.c        |    4 ++--
>  7 files changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/cpu-all.h b/cpu-all.h
> index 97a224d..43bbf18 100644
> --- a/cpu-all.h
> +++ b/cpu-all.h
> @@ -907,7 +907,7 @@ int cpu_physical_memory_set_dirty_tracking(int enable);
>  int cpu_physical_memory_get_dirty_tracking(void);
>  
>  int cpu_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
> -                                   target_phys_addr_t end_addr);
> +                                   ram_addr_t size);
>  
>  void dump_exec_info(FILE *f,
>                      int (*cpu_fprintf)(FILE *f, const char *fmt, ...));
> diff --git a/exec.c b/exec.c
> index 688f603..f8bfcd4 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1921,12 +1921,12 @@ int cpu_physical_memory_get_dirty_tracking(void)
>  }
>  
>  int cpu_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
> -                                   target_phys_addr_t end_addr)
> +                                   ram_addr_t size)
>  {
>      int ret = 0;
>  
>      if (kvm_enabled())
> -        ret = kvm_physical_sync_dirty_bitmap(start_addr, end_addr);
> +        ret = kvm_physical_sync_dirty_bitmap(start_addr, size);
>      return ret;
>  }
>  
> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> index 0a12782..2287e94 100644
> --- a/hw/cirrus_vga.c
> +++ b/hw/cirrus_vga.c
> @@ -1220,7 +1220,7 @@ static void cirrus_update_bank_ptr(CirrusVGAState * s, unsigned bank_index)
>           * on the current location, otherwise we lose this pointer forever */
>          if (s->vga.lfb_vram_mapped) {
>              target_phys_addr_t base_addr = isa_mem_base + 0xa0000 + bank_index * 0x8000;
> -            cpu_physical_sync_dirty_bitmap(base_addr, base_addr + 0x8000);
> +            cpu_physical_sync_dirty_bitmap(base_addr, 0x8000);
>          }
>  	s->cirrus_bank_base[bank_index] = offset;
>  	s->cirrus_bank_limit[bank_index] = limit;
> @@ -2588,8 +2588,9 @@ static void map_linear_vram(CirrusVGAState *s)
>  {
>      if (!s->vga.map_addr && s->vga.lfb_addr && s->vga.lfb_end) {
>          s->vga.map_addr = s->vga.lfb_addr;
> -        s->vga.map_end = s->vga.lfb_end;
> -        cpu_register_physical_memory(s->vga.map_addr, s->vga.map_end - s->vga.map_addr, s->vga.vram_offset);
> +        s->vga.map_size = s->vga.lfb_end - s->vga.lfb_addr;
> +        cpu_register_physical_memory(s->vga.map_addr, s->vga.map_size,
> +                                     s->vga.vram_offset);
>      }
>  
>      if (!s->vga.map_addr)
> @@ -2620,7 +2621,7 @@ static void map_linear_vram(CirrusVGAState *s)
>  static void unmap_linear_vram(CirrusVGAState *s)
>  {
>      if (s->vga.map_addr && s->vga.lfb_addr && s->vga.lfb_end)
> -        s->vga.map_addr = s->vga.map_end = 0;
> +        s->vga.map_addr = s->vga.map_size = 0;
>  
>      cpu_register_physical_memory(isa_mem_base + 0xa0000, 0x20000,
>                                   s->vga.vga_io_memory);
> @@ -3270,7 +3271,7 @@ static void cirrus_pci_lfb_map(PCIDevice *d, int region_num,
>      cpu_register_physical_memory(addr + 0x1000000, 0x400000,
>  				 s->cirrus_linear_bitblt_io_addr);
>  
> -    s->vga.map_addr = s->vga.map_end = 0;
> +    s->vga.map_addr = s->vga.map_size = 0;
>      s->vga.lfb_addr = addr & TARGET_PAGE_MASK;
>      s->vga.lfb_end = ((addr + VGA_RAM_SIZE) + TARGET_PAGE_SIZE - 1) & TARGET_PAGE_MASK;
>      /* account for overflow */
> diff --git a/hw/framebuffer.c b/hw/framebuffer.c
> index 24cdf25..f8efe5b 100644
> --- a/hw/framebuffer.c
> +++ b/hw/framebuffer.c
> @@ -49,7 +49,7 @@ void framebuffer_update_display(
>      *first_row = -1;
>      src_len = src_width * rows;
>  
> -    cpu_physical_sync_dirty_bitmap(base, base + src_len);
> +    cpu_physical_sync_dirty_bitmap(base, src_len);
>      pd = cpu_get_physical_page_desc(base);
>      pd2 = cpu_get_physical_page_desc(base + src_len - 1);
>      /* We should reall check that this is a continuous ram region.
> diff --git a/hw/vga.c b/hw/vga.c
> index 134ad16..ca94a68 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -1571,11 +1571,11 @@ void vga_invalidate_scanlines(VGAState *s, int y1, int y2)
>  static void vga_sync_dirty_bitmap(VGAState *s)
>  {
>      if (s->map_addr)
> -        cpu_physical_sync_dirty_bitmap(s->map_addr, s->map_end);
> +        cpu_physical_sync_dirty_bitmap(s->map_addr, s->map_size);
>  
>      if (s->lfb_vram_mapped) {
> -        cpu_physical_sync_dirty_bitmap(isa_mem_base + 0xa0000, 0xa8000);
> -        cpu_physical_sync_dirty_bitmap(isa_mem_base + 0xa8000, 0xb0000);
> +        cpu_physical_sync_dirty_bitmap(isa_mem_base + 0xa0000, 0x8000);
> +        cpu_physical_sync_dirty_bitmap(isa_mem_base + 0xa8000, 0x8000);
>      }
>  }
>  
> @@ -1878,7 +1878,7 @@ void vga_reset(void *opaque)
>      s->lfb_addr = 0;
>      s->lfb_end = 0;
>      s->map_addr = 0;
> -    s->map_end = 0;
> +    s->map_size = 0;
>      s->lfb_vram_mapped = 0;
>      s->bios_offset = 0;
>      s->bios_size = 0;
> @@ -2227,7 +2227,7 @@ typedef struct 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);
> +        kvm_log_start(s->map_addr, s->map_size);
>  
>      if (kvm_enabled() && s->lfb_vram_mapped) {
>          kvm_log_start(isa_mem_base + 0xa0000, 0x8000);
> @@ -2245,7 +2245,7 @@ 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 + s->vram_size;
> +        s->map_size = s->vram_size;
>          vga_dirty_log_start(s);
>      }
>  }
> diff --git a/hw/vga_int.h b/hw/vga_int.h
> index 631b1b0..83b69ef 100644
> --- a/hw/vga_int.h
> +++ b/hw/vga_int.h
> @@ -105,7 +105,7 @@ typedef struct VGACommonState {
>      uint32_t lfb_addr;
>      uint32_t lfb_end;
>      uint32_t map_addr;
> -    uint32_t map_end;
> +    uint32_t map_size;
>      uint32_t lfb_vram_mapped; /* whether 0xa0000 is mapped as ram */
>      unsigned long bios_offset;
>      unsigned int bios_size;
> diff --git a/kvm-all.c b/kvm-all.c
> index d843338..2a48b63 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -294,11 +294,11 @@ int kvm_set_migration_log(int enable)
>   * @end_addr: end of logged region.
>   */
>  int kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
> -                                   target_phys_addr_t end_addr)
> +                                   ram_addr_t dirty_size)
>  {
>      KVMState *s = kvm_state;
>      unsigned long size, allocated_size = 0;
> -    target_phys_addr_t phys_addr;
> +    target_phys_addr_t phys_addr, end_addr = start_addr + dirty_size;

I personally prefer adding a separate line for this. But that's a matter
of taste I guess.

>      ram_addr_t addr;
>      KVMDirtyLog d;
>      KVMSlot *mem;

Otherwise, no concerns.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux