Re: [Qemu-devel] [PATCH v4 1/2] arch_init: align MR size to target page size

[Laszlo Ersek <lersek@redhat.com>]

On 08/19/13 16:26, Michael S. Tsirkin wrote:
> Migration code assumes that each MR is a multiple of TARGET_PAGE_SIZE:
> MR size is divided by TARGET_PAGE_SIZE, so if it isn't migration
> never completes.
> But this isn't really required for regions set up with
> memory_region_init_ram, since that calls qemu_ram_alloc
> which aligns size up using TARGET_PAGE_ALIGN.
> 
> Align MR size up to full target page sizes, this way
> migration completes even if we create a RAM MR
> which is not a full target page size.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  arch_init.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 68a7ab7..ac8eb59 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -342,7 +342,8 @@ ram_addr_t migration_bitmap_find_and_reset_dirty(MemoryRegion *mr,
>  {
>      unsigned long base = mr->ram_addr >> TARGET_PAGE_BITS;
>      unsigned long nr = base + (start >> TARGET_PAGE_BITS);
> -    unsigned long size = base + (int128_get64(mr->size) >> TARGET_PAGE_BITS);
> +    uint64_t mr_size = TARGET_PAGE_ALIGN(memory_region_size(mr));
> +    unsigned long size = base + (mr_size >> TARGET_PAGE_BITS);
>  
>      unsigned long next;
>  
> 

(1) The patch (and the update to 2/2) seem correct to me.

(2) But is this patch complete?

Long version:

(1) The "only" danger in migration_bitmap_find_and_reset_dirty(),
AFAICS, is over-subscripting "migration_bitmap" with find_next_bit().

However, ram_save_setup() seems to initialize "migration_bitmap" for
"ram_pages" bits, and "ram_pages" comes from last_ram_offset().

last_ram_offset() in turn finds the highest offset any RAMBlock has.

The RAMBlock backing the fw_cfg file has already rounded-up size, so I
think "migration_bitmap" will have a bit allocated for the last
(possibly not fully populated) page of any fw_cfg RAMBlock. So this
patch should be correct.


(2) Regarding completeness, are we sure that nothing else depends on
mr->size being an integer multiple of TARGET_PAGE_SIZE?

I think v3 is perhaps less intrusive (as in, it doesn't raise (2)).


((3) memory_region_size() is slightly different from
int128_get64(mr->size); it has a special case for int128_2_64() -- and I
don't understand that. int128_2_64() represents 2 raised to the power of
64. It seems to be the replacement for UINT64_MAX.)

Thanks
Laszlo