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 19:48, Michael S. Tsirkin wrote:
> On Mon, Aug 19, 2013 at 07:37:44PM +0200, Laszlo Ersek wrote:
>> 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?
> 
> There's no requirement that mr->size is a multiple of TARGET_PAGE_SIZE.
> The only requirement is for a RAM mr size, and that
> comes from migration. Even that is simply a bug.
> 
> 
>> I think v3 is perhaps less intrusive (as in, it doesn't raise (2)).
> 
> Yes but it's early days in the 1.7 cycle so I think it makes
> sense to opt for a cleaner/smaller API even if this might trigger
> some latent bugs.
> 
>>
>> ((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
> 
> I think this is to represent things like PCI regions which can
> in theory cover the whole 64 bit range.
> You can't represent size of the whole 64 bit range in a 64 bit
> integer.
> We can't migrate RAM that large so no real issue.

Okay then.

By now both Peter and Paolo should be willing to R-b the v4 series;
here's mine:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>