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

[Michael S. Tsirkin]

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 <address@hidden>
> > ---
> >  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.


-- 
MST