Re: [Qemu-devel] [PATCH 2/6] memory: change dirty setting APIs to take a size

[Avi Kivity]

On 12/10/2011 06:44 PM, Blue Swirl wrote:
> Instead of each target knowing or guessing the guest page size,
> just pass the desired size of dirtied memory area. This should also
> improve performance due to memset() optimizations.
>
>
> -static inline void cpu_physical_memory_set_dirty(ram_addr_t addr)
> +static inline void cpu_physical_memory_range_set_dirty(ram_addr_t start,
> +                                                       ram_addr_t size)
>  {

Since you're changing all callers in one patch, might as well keep the
shorter name.

> -    ram_list.phys_dirty[addr >> TARGET_PAGE_BITS] = 0xff;
> +    start >>= TARGET_PAGE_BITS;
> +    size += TARGET_PAGE_SIZE - 1;
> +    size >>= TARGET_PAGE_BITS;
> +

This is wrong, consider start == 0xfff && size == 2; you only dirty one
page.

> +    memset(&ram_list.phys_dirty[start], 0xff, size);
>  }
>
>  static inline int cpu_physical_memory_set_dirty_flags(ram_addr_t addr,
> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> @@ -1918,8 +1915,8 @@ static void
> cirrus_mem_writeb_mode4and5_8bpp(CirrusVGAState * s,
>       val <<= 1;
>       dst++;
>      }
> -    memory_region_set_dirty(&s->vga.vram, offset);
> -    memory_region_set_dirty(&s->vga.vram, offset + 7);
> +    memory_region_set_dirty(&s->vga.vram, offset, 1);
> +    memory_region_set_dirty(&s->vga.vram, offset + 7, 1);
>  }


memory_region_set_dirty(..., offset, 8) matches the preceding code better

> -void memory_region_set_dirty(MemoryRegion *mr, target_phys_addr_t addr)
> +void memory_region_set_dirty(MemoryRegion *mr, target_phys_addr_t addr,
> +                             target_phys_addr_t size)
>  {
>      assert(mr->terminates);
> -    return cpu_physical_memory_set_dirty(mr->ram_addr + addr);
> +    return cpu_physical_memory_range_set_dirty(mr->ram_addr + addr, size);
>  }
>
>  void memory_region_sync_dirty_bitmap(MemoryRegion *mr)
> diff --git a/memory.h b/memory.h
> index 53bf261..1f8b5a5 100644
> --- a/memory.h
> +++ b/memory.h
> @@ -318,10 +318,12 @@ bool memory_region_get_dirty(MemoryRegion *mr,
> target_phys_addr_t addr,
>   *
>   * Marks a page as dirty, after it has been dirtied outside guest code.

a range of bytes

>   *
> - * @mr: the memory region being queried.
> + * @mr: the memory region being dirtied.
>   * @addr: the address (relative to the start of the region) being dirtied.
> + * @size: size of the range being dirtied.
>   */
> -void memory_region_set_dirty(MemoryRegion *mr, target_phys_addr_t addr);
> +void memory_region_set_dirty(MemoryRegion *mr, target_phys_addr_t addr,
> +                             target_phys_addr_t size);
>
>

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