[Qemu-devel] [PATCH 0/2] Migration+huge page fixes

[Qemu-devel] [PATCH 0/2] Migration+huge page fixes

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Hi,
  The attached patch-pair fix migration in the case
where you are using huge pages but you have a RAM size
which is not a multiple of the huge page size.
It's unfortunately legal so there might be VMs out there
that already have it, and it turns out it used to work,
but it broke, probably when I added the support for hugepages
in postcopy.

Here I:
   a) Fix it for normal migration
   b) Ban it for postcopy, since postcopy+hugepage was new
     recently anyway and it's a non-trivial fix.

Dave

Dr. David Alan Gilbert (2):
  migration: Fix non-multiple of page size migration
  postcopy: Require RAMBlocks that are whole pages

 migration/postcopy-ram.c | 16 +++++++++++++---
 migration/ram.c          |  5 ++++-
 2 files changed, 17 insertions(+), 4 deletions(-)

-- 
2.13.0

[Qemu-devel] [PATCH 1/2] migration: Fix non-multiple of page size migration

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Unfortunately it's legal to create a VM with a RAM size that's
not a multiple of the underlying host page or huge page size.
Recently I'd changed things to always send host sized pages,
and that breaks if we have say a 1025MB guest on 2MB hugepages.

Unfortunately we can't just make that illegal since it would break
migration from/to existing oddly configured VMs.

Symptom: qemu-system-x86_64: Illegal RAM offset 40100000
     as it transmits the fraction of the hugepage after the end
     of the RAMBlock (may also cause a crash on the source
     - possibly due to clearing bits after the bitmap)

Reported-by:  Yumei Huang <yuhuang@redhat.com>
Red Hat bug: https://bugzilla.redhat.com/show_bug.cgi?id=1449037

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/ram.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/migration/ram.c b/migration/ram.c
index 293d27ce83..cea8924c02 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1305,6 +1305,8 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
  * a host page in which case the remainder of the hostpage is sent.
  * Only dirty target pages are sent. Note that the host page size may
  * be a huge page for this block.
+ * The saving stops at the boundary of the used_length of the block
+ * if the RAMBlock isn't a multiple of the host page size.
  *
  * Returns the number of pages written or negative on error
  *
@@ -1328,7 +1330,8 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
 
         pages += tmppages;
         pss->page++;
-    } while (pss->page & (pagesize_bits - 1));
+    } while ((pss->page & (pagesize_bits - 1)) &&
+             offset_in_ramblock(pss->block, pss->page << TARGET_PAGE_BITS));
 
     /* The offset we leave with is the last one we looked at */
     pss->page--;
-- 
2.13.0

[Qemu-devel] [PATCH 2/2] postcopy: Require RAMBlocks that are whole pages

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

It turns out that it's legal to create a VM with RAMBlocks that aren't
a multiple of the pagesize in use; e.g. a 1025M main memory using
2M host pages.  That breaks postcopy's atomic placement of pages,
so disallow it.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/postcopy-ram.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index cdadaf6578..e8f43e7bd1 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -96,14 +96,24 @@ static bool ufd_version_check(int ufd)
 
 /* Callback from postcopy_ram_supported_by_host block iterator.
  */
-static int test_range_shared(const char *block_name, void *host_addr,
+static int test_ramblock_postcopiable(const char *block_name, void *host_addr,
                              ram_addr_t offset, ram_addr_t length, void *opaque)
 {
-    if (qemu_ram_is_shared(qemu_ram_block_by_name(block_name))) {
+    RAMBlock *rb = qemu_ram_block_by_name(block_name);
+    size_t pagesize = qemu_ram_pagesize(rb);
+
+    if (qemu_ram_is_shared(rb)) {
         error_report("Postcopy on shared RAM (%s) is not yet supported",
                      block_name);
         return 1;
     }
+
+    if (length % pagesize) {
+        error_report("Postcopy requires RAM blocks to be a page size multiple,"
+                     " block %s is 0x" RAM_ADDR_FMT " bytes with a "
+                     "page size of 0x%zx", block_name, length, pagesize);
+        return 1;
+    }
     return 0;
 }
 
@@ -140,7 +150,7 @@ bool postcopy_ram_supported_by_host(void)
     }
 
     /* We don't support postcopy with shared RAM yet */
-    if (qemu_ram_foreach_block(test_range_shared, NULL)) {
+    if (qemu_ram_foreach_block(test_ramblock_postcopiable, NULL)) {
         goto out;
     }
 
-- 
2.13.0

Re: [Qemu-devel] [PATCH 1/2] migration: Fix non-multiple of page size migration

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Unfortunately it's legal to create a VM with a RAM size that's
> not a multiple of the underlying host page or huge page size.
> Recently I'd changed things to always send host sized pages,
> and that breaks if we have say a 1025MB guest on 2MB hugepages.
>
> Unfortunately we can't just make that illegal since it would break
> migration from/to existing oddly configured VMs.
>
> Symptom: qemu-system-x86_64: Illegal RAM offset 40100000
>      as it transmits the fraction of the hugepage after the end
>      of the RAMBlock (may also cause a crash on the source
>      - possibly due to clearing bits after the bitmap)
>
> Reported-by:  Yumei Huang <yuhuang@redhat.com>
> Red Hat bug: https://bugzilla.redhat.com/show_bug.cgi?id=1449037
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>


Reviewed-by: Juan Quintela <quintela@redhat.com>

We should really require for new machine types that ramblocks be
multiples of the page size.  This is just asking for trouble.

Re: [Qemu-devel] [PATCH 2/2] postcopy: Require RAMBlocks that are whole pages

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> It turns out that it's legal to create a VM with RAMBlocks that aren't
> a multiple of the pagesize in use; e.g. a 1025M main memory using
> 2M host pages.  That breaks postcopy's atomic placement of pages,
> so disallow it.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>


>      }
>  
>      /* We don't support postcopy with shared RAM yet */
> -    if (qemu_ram_foreach_block(test_range_shared, NULL)) {
> +    if (qemu_ram_foreach_block(test_ramblock_postcopiable, NULL)) {

When I was looking at this code, I still don't know why
qemu_ram_foreach_block() don't pass the block directly.  It needs it
almost all callers.

When I saw it I was about to change it, but got sidetracked on other
things :-p

Later, Juan.

Re: [Qemu-devel] [PATCH 2/2] postcopy: Require RAMBlocks that are whole pages

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > It turns out that it's legal to create a VM with RAMBlocks that aren't
> > a multiple of the pagesize in use; e.g. a 1025M main memory using
> > 2M host pages.  That breaks postcopy's atomic placement of pages,
> > so disallow it.
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>

Thanks

> >      }
> >  
> >      /* We don't support postcopy with shared RAM yet */
> > -    if (qemu_ram_foreach_block(test_range_shared, NULL)) {
> > +    if (qemu_ram_foreach_block(test_ramblock_postcopiable, NULL)) {
> 
> When I was looking at this code, I still don't know why
> qemu_ram_foreach_block() don't pass the block directly.  It needs it
> almost all callers.
> 
> When I saw it I was about to change it, but got sidetracked on other
> things :-p

I think originally it passed very little information at all, and
that RAMBlocks were these mystical things no one outside exec.c
was really supposed to know about.

Dave

> Later, Juan.
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/2] postcopy: Require RAMBlocks that are whole pages

[Peter Xu]

On Thu, May 18, 2017 at 09:08:34AM +0100, Dr. David Alan Gilbert wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
> > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > >
> > > It turns out that it's legal to create a VM with RAMBlocks that aren't
> > > a multiple of the pagesize in use; e.g. a 1025M main memory using
> > > 2M host pages.  That breaks postcopy's atomic placement of pages,
> > > so disallow it.
> > >
> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > 
> > Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
> Thanks
> 
> > >      }
> > >  
> > >      /* We don't support postcopy with shared RAM yet */
> > > -    if (qemu_ram_foreach_block(test_range_shared, NULL)) {
> > > +    if (qemu_ram_foreach_block(test_ramblock_postcopiable, NULL)) {
> > 
> > When I was looking at this code, I still don't know why
> > qemu_ram_foreach_block() don't pass the block directly.  It needs it
> > almost all callers.
> > 
> > When I saw it I was about to change it, but got sidetracked on other
> > things :-p
> 
> I think originally it passed very little information at all, and
> that RAMBlocks were these mystical things no one outside exec.c
> was really supposed to know about.

(Yeah I got the same question before. That's why I got
 RAMBLOCK_FOREACH() but didn't use qemu_ram_foreach_block() since I
 need at least page size info for the block...)

-- 
Peter Xu