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

[Blue Swirl]

On Sun, Jan 22, 2012 at 13:07, Blue Swirl <address@hidden> wrote:
> Instead of each target knowing or guessing the guest page size,
> just pass the desired size of dirtied memory area.
>
> Signed-off-by: Blue Swirl <address@hidden>
> ---
>  arch_init.c     |    2 +-
>  exec-obsolete.h |   16 ++++++++++++++++
>  hw/cirrus_vga.c |   16 ++++++----------
>  hw/g364fb.c     |   11 +++--------
>  hw/qxl.c        |    5 +----
>  hw/tcx.c        |   14 +++-----------
>  hw/vga.c        |    6 +++---
>  hw/vhost.c      |    2 +-
>  kvm-all.c       |    7 ++++---
>  memory.c        |    5 +++--
>  memory.h        |   11 +++++++----
>  xen-all.c       |    3 ++-
>  12 files changed, 50 insertions(+), 48 deletions(-)
>
> diff --git a/arch_init.c b/arch_init.c
> index 95ac682..2366511 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -285,7 +285,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int
> stage, void *opaque)
>             for (addr = 0; addr < block->length; addr += TARGET_PAGE_SIZE) {
>                 if (!memory_region_get_dirty(block->mr, addr,
>                                              DIRTY_MEMORY_MIGRATION)) {
> -                    memory_region_set_dirty(block->mr, addr);
> +                    memory_region_set_dirty(block->mr, addr, 
> TARGET_PAGE_SIZE);
>                 }
>             }
>         }
> diff --git a/exec-obsolete.h b/exec-obsolete.h
> index 22e0ba5..e37cc5d 100644
> --- a/exec-obsolete.h
> +++ b/exec-obsolete.h
> @@ -76,6 +76,22 @@ static inline int
> cpu_physical_memory_set_dirty_flags(ram_addr_t addr,
>     return ram_list.phys_dirty[addr >> TARGET_PAGE_BITS] |= dirty_flags;
>  }
>
> +static inline void cpu_physical_memory_set_dirty_range(ram_addr_t start,
> +                                                       ram_addr_t length,
> +                                                       int dirty_flags)
> +{
> +    int mask;
> +    uint8_t *p;
> +    ram_addr_t addr, end;
> +
> +    end = start + length;
> +    mask = ~dirty_flags;

This is incorrect, dirty_flags should be used directly.

> +    p = ram_list.phys_dirty + (start >> TARGET_PAGE_BITS);
> +    for (addr = start; addr <= end; addr += TARGET_PAGE_SIZE) {
> +        *p++ |= mask;
> +    }
> +}
> +
>  static inline void cpu_physical_memory_mask_dirty_range(ram_addr_t start,
>                                                         ram_addr_t length,
>                                                         int dirty_flags)
> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> index 8506bb5..a031079 100644
> --- a/hw/cirrus_vga.c
> +++ b/hw/cirrus_vga.c
> @@ -619,10 +619,7 @@ static void
> cirrus_invalidate_region(CirrusVGAState * s, int off_begin,
>        off_cur = off_begin;
>        off_cur_end = (off_cur + bytesperline) & s->cirrus_addr_mask;
>        off_cur &= TARGET_PAGE_MASK;
> -       while (off_cur < off_cur_end) {
> -           memory_region_set_dirty(&s->vga.vram, off_cur);
> -           off_cur += TARGET_PAGE_SIZE;
> -       }
> +        memory_region_set_dirty(&s->vga.vram, off_cur, off_cur_end - 
> off_cur);
>        off_begin += off_pitch;
>     }
>  }
> @@ -1923,8 +1920,7 @@ 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, 8);
>  }
>
>  static void cirrus_mem_writeb_mode4and5_16bpp(CirrusVGAState * s,
> @@ -1948,8 +1944,7 @@ static void
> cirrus_mem_writeb_mode4and5_16bpp(CirrusVGAState * s,
>        val <<= 1;
>        dst += 2;
>     }
> -    memory_region_set_dirty(&s->vga.vram, offset);
> -    memory_region_set_dirty(&s->vga.vram, offset + 15);
> +    memory_region_set_dirty(&s->vga.vram, offset, 16);
>  }
>
>  /***************************************
> @@ -2039,7 +2034,8 @@ static void cirrus_vga_mem_write(void *opaque,
>                mode = s->vga.gr[0x05] & 0x7;
>                if (mode < 4 || mode > 5 || ((s->vga.gr[0x0B] & 0x4) == 0)) {
>                    *(s->vga.vram_ptr + bank_offset) = mem_value;
> -                   memory_region_set_dirty(&s->vga.vram, bank_offset);
> +                    memory_region_set_dirty(&s->vga.vram, bank_offset,
> +                                            sizeof(mem_value));
>                } else {
>                    if ((s->vga.gr[0x0B] & 0x14) != 0x14) {
>                        cirrus_mem_writeb_mode4and5_8bpp(s, mode,
> @@ -2311,7 +2307,7 @@ static void cirrus_linear_write(void *opaque,
> target_phys_addr_t addr,
>        mode = s->vga.gr[0x05] & 0x7;
>        if (mode < 4 || mode > 5 || ((s->vga.gr[0x0B] & 0x4) == 0)) {
>            *(s->vga.vram_ptr + addr) = (uint8_t) val;
> -           memory_region_set_dirty(&s->vga.vram, addr);
> +            memory_region_set_dirty(&s->vga.vram, addr, 1);
>        } else {
>            if ((s->vga.gr[0x0B] & 0x14) != 0x14) {
>                cirrus_mem_writeb_mode4and5_8bpp(s, mode, addr, val);
> diff --git a/hw/g364fb.c b/hw/g364fb.c
> index 33ec149..02ec7b5 100644
> --- a/hw/g364fb.c
> +++ b/hw/g364fb.c
> @@ -268,12 +268,9 @@ static void g364fb_update_display(void *opaque)
>  static inline void g364fb_invalidate_display(void *opaque)
>  {
>     G364State *s = opaque;
> -    int i;
>
>     s->blanked = 0;
> -    for (i = 0; i < s->vram_size; i += G364_PAGE_SIZE) {
> -        memory_region_set_dirty(&s->mem_vram, i);
> -    }
> +    memory_region_set_dirty(&s->mem_vram, 0, s->vram_size);
>  }
>
>  static void g364fb_reset(G364State *s)
> @@ -385,7 +382,7 @@ static void g364fb_update_depth(G364State *s)
>
>  static void g364_invalidate_cursor_position(G364State *s)
>  {
> -    int ymin, ymax, start, end, i;
> +    int ymin, ymax, start, end;
>
>     /* invalidate only near the cursor */
>     ymin = s->cursor_position & 0xfff;
> @@ -393,9 +390,7 @@ static void g364_invalidate_cursor_position(G364State *s)
>     start = ymin * ds_get_linesize(s->ds);
>     end = (ymax + 1) * ds_get_linesize(s->ds);
>
> -    for (i = start; i < end; i += G364_PAGE_SIZE) {
> -        memory_region_set_dirty(&s->mem_vram, i);
> -    }
> +    memory_region_set_dirty(&s->mem_vram, start, end - start);
>  }
>
>  static void g364fb_ctrl_write(void *opaque,
> diff --git a/hw/qxl.c b/hw/qxl.c
> index bdd36f9..6442193 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -343,10 +343,7 @@ static void init_qxl_ram(PCIQXLDevice *d)
>  /* can be called from spice server thread context */
>  static void qxl_set_dirty(MemoryRegion *mr, ram_addr_t addr, ram_addr_t end)
>  {
> -    while (addr < end) {
> -        memory_region_set_dirty(mr, addr);
> -        addr += TARGET_PAGE_SIZE;
> -    }
> +    memory_region_set_dirty(mr, addr, end - addr);
>  }
>
>  static void qxl_rom_set_dirty(PCIQXLDevice *qxl)
> diff --git a/hw/tcx.c b/hw/tcx.c
> index 75a28f2..b6a2753 100644
> --- a/hw/tcx.c
> +++ b/hw/tcx.c
> @@ -61,21 +61,13 @@ static void tcx24_screen_dump(void *opaque, const
> char *filename);
>
>  static void tcx_set_dirty(TCXState *s)
>  {
> -    unsigned int i;
> -
> -    for (i = 0; i < MAXX * MAXY; i += TARGET_PAGE_SIZE) {
> -        memory_region_set_dirty(&s->vram_mem, i);
> -    }
> +    memory_region_set_dirty(&s->vram_mem, 0, MAXX * MAXY);
>  }
>
>  static void tcx24_set_dirty(TCXState *s)
>  {
> -    unsigned int i;
> -
> -    for (i = 0; i < MAXX * MAXY * 4; i += TARGET_PAGE_SIZE) {
> -        memory_region_set_dirty(&s->vram_mem, s->vram24_offset + i);
> -        memory_region_set_dirty(&s->vram_mem, s->cplane_offset + i);
> -    }
> +    memory_region_set_dirty(&s->vram_mem, s->vram24_offset, MAXX * MAXY * 4);
> +    memory_region_set_dirty(&s->vram_mem, s->cplane_offset, MAXX * MAXY * 4);
>  }
>
>  static void update_palette_entries(TCXState *s, int start, int end)
> diff --git a/hw/vga.c b/hw/vga.c
> index 950f97e..4dc2610 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -854,7 +854,7 @@ void vga_mem_writeb(VGACommonState *s,
> target_phys_addr_t addr, uint32_t val)
>             printf("vga: chain4: [0x" TARGET_FMT_plx "]\n", addr);
>  #endif
>             s->plane_updated |= mask; /* only used to detect font change */
> -            memory_region_set_dirty(&s->vram, addr);
> +            memory_region_set_dirty(&s->vram, addr, 1);
>         }
>     } else if (s->gr[5] & 0x10) {
>         /* odd/even mode (aka text mode mapping) */
> @@ -867,7 +867,7 @@ void vga_mem_writeb(VGACommonState *s,
> target_phys_addr_t addr, uint32_t val)
>             printf("vga: odd/even: [0x" TARGET_FMT_plx "]\n", addr);
>  #endif
>             s->plane_updated |= mask; /* only used to detect font change */
> -            memory_region_set_dirty(&s->vram, addr);
> +            memory_region_set_dirty(&s->vram, addr, 1);
>         }
>     } else {
>         /* standard VGA latched access */
> @@ -941,7 +941,7 @@ void vga_mem_writeb(VGACommonState *s,
> target_phys_addr_t addr, uint32_t val)
>         printf("vga: latch: [0x" TARGET_FMT_plx "] mask=0x%08x val=0x%08x\n",
>                addr * 4, write_mask, val);
>  #endif
> -        memory_region_set_dirty(&s->vram, addr << 2);
> +        memory_region_set_dirty(&s->vram, addr << 2, sizeof(uint32_t));
>     }
>  }
>
> diff --git a/hw/vhost.c b/hw/vhost.c
> index 4778521..5ece659 100644
> --- a/hw/vhost.c
> +++ b/hw/vhost.c
> @@ -55,7 +55,7 @@ static void vhost_dev_sync_region(struct vhost_dev *dev,
>             ram_addr_t ram_addr;
>             bit -= 1;
>             ram_addr = section->offset_within_region + bit * VHOST_LOG_PAGE;
> -            memory_region_set_dirty(section->mr, ram_addr);
> +            memory_region_set_dirty(section->mr, ram_addr, VHOST_LOG_PAGE);
>             log &= ~(0x1ull << bit);
>         }
>         addr += VHOST_LOG_CHUNK;
> diff --git a/kvm-all.c b/kvm-all.c
> index 9e84d35..d0b6c31 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -361,7 +361,7 @@ static int
> kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
>                 page_number = i * HOST_LONG_BITS + j;
>                 addr1 = page_number * TARGET_PAGE_SIZE;
>                 addr = section->offset_within_region + addr1;
> -                memory_region_set_dirty(section->mr, addr);
> +                memory_region_set_dirty(section->mr, addr, TARGET_PAGE_SIZE);
>             } while (c != 0);
>         }
>     }
> @@ -372,8 +372,9 @@ static int
> kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
>
>  /**
>  * kvm_physical_sync_dirty_bitmap - Grab dirty bitmap from kernel space
> - * This function updates qemu's dirty bitmap using
> cpu_physical_memory_set_dirty().
> - * This means all bits are set to dirty.
> + * This function updates qemu's dirty bitmap using
> + * memory_region_set_dirty().  This means all bits are set
> + * to dirty.
>  *
>  * @start_add: start of logged region.
>  * @end_addr: end of logged region.
> diff --git a/memory.c b/memory.c
> index 6201a37..0cf5d41 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1106,10 +1106,11 @@ bool memory_region_get_dirty(MemoryRegion *mr,
> target_phys_addr_t addr,
>     return cpu_physical_memory_get_dirty(mr->ram_addr + addr, 1 << client);
>  }
>
> -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_set_dirty_range(mr->ram_addr + addr, size, 
> -1);
>  }
>
>  void memory_region_sync_dirty_bitmap(MemoryRegion *mr)
> diff --git a/memory.h b/memory.h
> index d48b08b..d160008 100644
> --- a/memory.h
> +++ b/memory.h
> @@ -380,14 +380,17 @@ bool memory_region_get_dirty(MemoryRegion *mr,
> target_phys_addr_t addr,
>                              unsigned client);
>
>  /**
> - * memory_region_set_dirty: Mark a page as dirty in a memory region.
> + * memory_region_set_dirty: Mark a range of bytes as dirty in a memory 
> region.
>  *
> - * Marks a page as dirty, after it has been dirtied outside guest code.
> + * Marks a range of bytes as dirty, after it has been dirtied outside
> + * guest code.
>  *
> - * @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);
>
>  /**
>  * memory_region_sync_dirty_bitmap: Synchronize a region's dirty bitmap with
> diff --git a/xen-all.c b/xen-all.c
> index d1fc597..fd39168 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -447,7 +447,8 @@ static void xen_sync_dirty_bitmap(XenIOState *state,
>             j = ffsl(map) - 1;
>             map &= ~(1ul << j);
>             memory_region_set_dirty(framebuffer,
> -                                    (i * width + j) * TARGET_PAGE_SIZE);
> +                                    (i * width + j) * TARGET_PAGE_SIZE,
> +                                    TARGET_PAGE_SIZE);
>         };
>     }
>  }
> --
> 1.7.9.rc0