[Qemu-devel] Re: [PATCH v2 2/6] Introduce bit-based phys_ram_dirty for VGA, CODE, MIGRATION and MASTER.

[Yoshiaki Tamura]

Avi Kivity wrote:
> On 04/06/2010 03:51 AM, Yoshiaki Tamura wrote:
> > Replaces byte-based phys_ram_dirty bitmap with three bit-based
> > phys_ram_dirty
> > bitmap. On allocation, it sets all bits in the bitmap.
>
> > index c74b0a4..9733892 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -110,7 +110,7 @@ uint8_t *code_gen_ptr;
> >
> > #if !defined(CONFIG_USER_ONLY)
> > int phys_ram_fd;
> > -uint8_t *phys_ram_dirty;
> > +unsigned long *phys_ram_dirty[NUM_DIRTY_FLAGS];
> > static int in_migration;
> >
> > typedef struct RAMBlock {
> > @@ -2825,10 +2825,32 @@ ram_addr_t qemu_ram_alloc(ram_addr_t size)
> > new_block->next = ram_blocks;
> > ram_blocks = new_block;
> >
> > - phys_ram_dirty = qemu_realloc(phys_ram_dirty,
> > - (last_ram_offset + size)>> TARGET_PAGE_BITS);
> > - memset(phys_ram_dirty + (last_ram_offset>> TARGET_PAGE_BITS),
> > - 0xff, size>> TARGET_PAGE_BITS);
> > +/* temporarily copy from qemu-kvm.git/qemu-kvm.h */
> > +#define ALIGN(x, y) (((x)+(y)-1)& ~((y)-1))
> > +#define BITMAP_SIZE(m) (ALIGN(((m)>>TARGET_PAGE_BITS),
> > HOST_LONG_BITS) / 8)
>
> Please put in some header file, maybe qemu-common.h.

OK.  BTW, is qemu-kvm.h planned to go upstream?

> > +
> > + if (BITMAP_SIZE(last_ram_offset + size) !=
> > BITMAP_SIZE(last_ram_offset)) {
> > + phys_ram_dirty[MASTER_DIRTY_FLAG] =
> > + qemu_realloc(phys_ram_dirty[MASTER_DIRTY_FLAG],
> > + BITMAP_SIZE(last_ram_offset + size));
> > + phys_ram_dirty[VGA_DIRTY_FLAG]
> > + = qemu_realloc(phys_ram_dirty[VGA_DIRTY_FLAG],
> > + BITMAP_SIZE(last_ram_offset + size));
> > + phys_ram_dirty[CODE_DIRTY_FLAG] =
> > + qemu_realloc(phys_ram_dirty[CODE_DIRTY_FLAG],
> > + BITMAP_SIZE(last_ram_offset + size));
> > + phys_ram_dirty[MIGRATION_DIRTY_FLAG] =
> > + qemu_realloc(phys_ram_dirty[MIGRATION_DIRTY_FLAG],
> > + BITMAP_SIZE(last_ram_offset + size));
> > + memset((uint8_t *)phys_ram_dirty[MASTER_DIRTY_FLAG] +
> > + BITMAP_SIZE(last_ram_offset), 0xff, BITMAP_SIZE(size));
> > + memset((uint8_t *)phys_ram_dirty[VGA_DIRTY_FLAG] +
> > + BITMAP_SIZE(last_ram_offset), 0xff, BITMAP_SIZE(size));
> > + memset((uint8_t *)phys_ram_dirty[CODE_DIRTY_FLAG] +
> > + BITMAP_SIZE(last_ram_offset), 0xff, BITMAP_SIZE(size));
> > + memset((uint8_t *)phys_ram_dirty[MIGRATION_DIRTY_FLAG] +
> > + BITMAP_SIZE(last_ram_offset), 0xff, BITMAP_SIZE(size));
> > + }
>
> Should be nicer as a loop calling a helper to allocate each bitmap. This
> patch won't work by itself, will it? I think you need to merge it with
> the succeeding patches.

Yeah.  I originally wrote as you suggested, but I needed to skip 0x03 because of 
the reason written below.

> +/* Use DIRTY_FLAG as indexes of bit-based phys_ram_dirty.
> +   0x03 is empty to make it compatible with byte-based bitmap. */
> +#define MASTER_DIRTY_FLAG    0x00
>  #define VGA_DIRTY_FLAG       0x01
>  #define CODE_DIRTY_FLAG      0x02
> -#define MIGRATION_DIRTY_FLAG 0x08
> +#define MIGRATION_DIRTY_FLAG 0x04

If you think if (i != 0x03) is better, I would modify it to a loop.

And sorry, you need to merge the succeeding patches to make it work.