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 <m...@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.
--
MST
