[Qemu-devel] [PATCH v4 0/2] ppc/spapr: Fix migration of radix guests

[Qemu-devel] [PATCH v4 0/2] ppc/spapr: Fix migration of radix guests

[Bharata B Rao]

This patchset fixes the migration of sPAPR radix guests.

Changes in v4
-------------
- The earlier approach of keeping the HTAB savevm handlers registered
  only for hash guests by explicitly deregistering does leave out a few
  corner cases which become ugly/difficult to fix. Hence based on
  David Gibson's suggestion, this new approach adds a new
  "no HPT" encoding into the migration stream when guest doesn't
  allocate HPT.
- There are a few assumptions in HTAB savevm handlers that spapr->htab
  will not be set only for KVM guests. This is no longer true with
  radix TCG guests. Fix this assumption. These changes enable TCG migration
  to work. However guest kernel failures are observed post migration which
  need to be fixed separately. This changes could acutally go into
  a separate patch.

v3: https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg05434.html

Bharata B Rao (2):
  spapr: Add a "no HPT" encoding to HTAB migration stream
  spapr: Fix migration of Radix guests

 hw/ppc/spapr.c | 43 +++++++++++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 14 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v4 1/2] spapr: Add a "no HPT" encoding to HTAB migration stream

[Bharata B Rao]

Add a "no HPT" encoding (using value -1) to the HTAB migration
stream (in the place of HPT size) when the guest doesn't allocate HPT.
This will help the target side to match target HPT with the source HPT
and thus enable successful migration.

A few more fixes to enable TCG migration to work correctly are also
included in this commit:

- HTAB savevm handlers have a few asserts on kvm_enabled() when
  spapr->htab != 0. Convert these into conditional checks as it is now
  possible to have no HTAB with TCG radix guests.
- htab_save_setup() asserts for kvm_enabled() when spapr->htab != 0.
  Remove this as we can't assert this for TCG radix guests.

Suggested-by: David Gibson <david@gibson.dropbear.id.au>
              [no HPT encoding suggestion]
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index ab3aab1..b589ed4 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1559,17 +1559,18 @@ static int htab_save_setup(QEMUFile *f, void *opaque)
 {
     sPAPRMachineState *spapr = opaque;
 
-    /* "Iteration" header */
-    qemu_put_be32(f, spapr->htab_shift);
+    /* "Iteration" header: no-HPT or HPT size encoding */
+    if (!spapr->htab_shift) {
+        qemu_put_be32(f, -1);
+    } else {
+        qemu_put_be32(f, spapr->htab_shift);
+    }
 
     if (spapr->htab) {
         spapr->htab_save_index = 0;
         spapr->htab_first_pass = true;
-    } else {
-        assert(kvm_enabled());
     }
 
-
     return 0;
 }
 
@@ -1714,9 +1715,7 @@ static int htab_save_iterate(QEMUFile *f, void *opaque)
     /* Iteration header */
     qemu_put_be32(f, 0);
 
-    if (!spapr->htab) {
-        assert(kvm_enabled());
-
+    if (!spapr->htab && kvm_enabled()) {
         fd = get_htab_fd(spapr);
         if (fd < 0) {
             return fd;
@@ -1748,7 +1747,7 @@ static int htab_save_complete(QEMUFile *f, void *opaque)
     /* Iteration header */
     qemu_put_be32(f, 0);
 
-    if (!spapr->htab) {
+    if (!spapr->htab && kvm_enabled()) {
         int rc;
 
         assert(kvm_enabled());
@@ -1793,6 +1792,12 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
     if (section_hdr) {
         Error *local_err = NULL;
 
+        if (section_hdr == -1) {
+            spapr_free_hpt(spapr);
+            unregister_savevm(NULL, "spapr/htab", spapr);
+            return 0;
+        }
+
         /* First section gives the htab size */
         spapr_reallocate_hpt(spapr, section_hdr, &local_err);
         if (local_err) {
@@ -1802,9 +1807,7 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
         return 0;
     }
 
-    if (!spapr->htab) {
-        assert(kvm_enabled());
-
+    if (!spapr->htab && kvm_enabled()) {
         fd = kvmppc_get_htab_fd(true);
         if (fd < 0) {
             error_report("Unable to open fd to restore KVM hash table: %s",
@@ -1843,7 +1846,7 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
                 memset(HPTE(spapr->htab, index + n_valid), 0,
                        HASH_PTE_SIZE_64 * n_invalid);
             }
-        } else {
+        } else if (kvm_enabled()) {
             int rc;
 
             assert(fd >= 0);
@@ -1855,7 +1858,7 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
         }
     }
 
-    if (!spapr->htab) {
+    if (!spapr->htab && kvm_enabled()) {
         assert(fd >= 0);
         close(fd);
     }
-- 
2.7.4

Re: [Qemu-devel] [PATCH v4 1/2] spapr: Add a "no HPT" encoding to HTAB migration stream

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5236 bytes --]

On Wed, May 31, 2017 at 04:56:44PM +0530, Bharata B Rao wrote:
> Add a "no HPT" encoding (using value -1) to the HTAB migration
> stream (in the place of HPT size) when the guest doesn't allocate HPT.
> This will help the target side to match target HPT with the source HPT
> and thus enable successful migration.
> 
> A few more fixes to enable TCG migration to work correctly are also
> included in this commit:
> 
> - HTAB savevm handlers have a few asserts on kvm_enabled() when
>   spapr->htab != 0. Convert these into conditional checks as it is now
>   possible to have no HTAB with TCG radix guests.
> - htab_save_setup() asserts for kvm_enabled() when spapr->htab != 0.
>   Remove this as we can't assert this for TCG radix guests.
> 
> Suggested-by: David Gibson <david@gibson.dropbear.id.au>
>               [no HPT encoding suggestion]
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

Looks basically ok, but there are still some details to address.

> ---
>  hw/ppc/spapr.c | 31 +++++++++++++++++--------------
>  1 file changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index ab3aab1..b589ed4 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1559,17 +1559,18 @@ static int htab_save_setup(QEMUFile *f, void *opaque)
>  {
>      sPAPRMachineState *spapr = opaque;
>  
> -    /* "Iteration" header */
> -    qemu_put_be32(f, spapr->htab_shift);
> +    /* "Iteration" header: no-HPT or HPT size encoding */
> +    if (!spapr->htab_shift) {
> +        qemu_put_be32(f, -1);

We're already using htab_shift == 0 to represent no HPT in the runtime
structure; we might as well do the same on the wire.  As a bonus it
slightly simplifies the logic here.

> +    } else {
> +        qemu_put_be32(f, spapr->htab_shift);
> +    }
>  
>      if (spapr->htab) {
>          spapr->htab_save_index = 0;
>          spapr->htab_first_pass = true;
> -    } else {
> -        assert(kvm_enabled());

I think you've oversimplified the assert()s a little.  As above we're
using htab_shift != 0 to canonically indicate the presence of an HPT,
whether it's qemu or kernel managed.  So really the new assert is
	if (spapr->htab_shift) {
		assert(spapr->htab || kvm_enabled());
	}
(which can be simplified depending on context, of course).

>      }
>  
> -
>      return 0;
>  }
>  
> @@ -1714,9 +1715,7 @@ static int htab_save_iterate(QEMUFile *f, void *opaque)
>      /* Iteration header */
>      qemu_put_be32(f, 0);
>  
> -    if (!spapr->htab) {
> -        assert(kvm_enabled());
> -
> +    if (!spapr->htab && kvm_enabled()) {

This isn't quite right.  !spapr->htab means one of two things: there
is no HPT (radix mode) or the HPT is kernel-managed.  kvm_enabled() is
not enough to discern the latter.  In radix mode with KVM you'd
execute this block, which I don't think you want.

>          fd = get_htab_fd(spapr);
>          if (fd < 0) {
>              return fd;
> @@ -1748,7 +1747,7 @@ static int htab_save_complete(QEMUFile *f, void *opaque)
>      /* Iteration header */
>      qemu_put_be32(f, 0);
>  
> -    if (!spapr->htab) {
> +    if (!spapr->htab && kvm_enabled()) {

Same here.

>          int rc;
>  
>          assert(kvm_enabled());
> @@ -1793,6 +1792,12 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
>      if (section_hdr) {
>          Error *local_err = NULL;
>  
> +        if (section_hdr == -1) {
> +            spapr_free_hpt(spapr);
> +            unregister_savevm(NULL, "spapr/htab", spapr);

I don't think we want to register/unregister the htab handlers at
all.  This above would break if you migrated a radix guest, then
rebooted into a hash guest and tried to migrate again.

> +            return 0;
> +        }
> +
>          /* First section gives the htab size */
>          spapr_reallocate_hpt(spapr, section_hdr, &local_err);
>          if (local_err) {
> @@ -1802,9 +1807,7 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
>          return 0;
>      }
>  
> -    if (!spapr->htab) {
> -        assert(kvm_enabled());
> -
> +    if (!spapr->htab && kvm_enabled()) {


Again, not quite right here.
>          fd = kvmppc_get_htab_fd(true);
>          if (fd < 0) {
>              error_report("Unable to open fd to restore KVM hash table: %s",
> @@ -1843,7 +1846,7 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
>                  memset(HPTE(spapr->htab, index + n_valid), 0,
>                         HASH_PTE_SIZE_64 * n_invalid);
>              }
> -        } else {
> +        } else if (kvm_enabled()) {
>              int rc;
>  
>              assert(fd >= 0);
> @@ -1855,7 +1858,7 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
>          }
>      }
>  
> -    if (!spapr->htab) {
> +    if (!spapr->htab && kvm_enabled()) {

Or here.

>          assert(fd >= 0);
>          close(fd);
>      }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v4 1/2] spapr: Add a "no HPT" encoding to HTAB migration stream

[Bharata B Rao]

On Thu, Jun 01, 2017 at 02:54:48PM +1000, David Gibson wrote:
> On Wed, May 31, 2017 at 04:56:44PM +0530, Bharata B Rao wrote:
> > Add a "no HPT" encoding (using value -1) to the HTAB migration
> > stream (in the place of HPT size) when the guest doesn't allocate HPT.
> > This will help the target side to match target HPT with the source HPT
> > and thus enable successful migration.
> > 
> > A few more fixes to enable TCG migration to work correctly are also
> > included in this commit:
> > 
> > - HTAB savevm handlers have a few asserts on kvm_enabled() when
> >   spapr->htab != 0. Convert these into conditional checks as it is now
> >   possible to have no HTAB with TCG radix guests.
> > - htab_save_setup() asserts for kvm_enabled() when spapr->htab != 0.
> >   Remove this as we can't assert this for TCG radix guests.
> > 
> > Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> >               [no HPT encoding suggestion]
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> 
> Looks basically ok, but there are still some details to address.
> 
> > ---
> >  hw/ppc/spapr.c | 31 +++++++++++++++++--------------
> >  1 file changed, 17 insertions(+), 14 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index ab3aab1..b589ed4 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1559,17 +1559,18 @@ static int htab_save_setup(QEMUFile *f, void *opaque)
> >  {
> >      sPAPRMachineState *spapr = opaque;
> >  
> > -    /* "Iteration" header */
> > -    qemu_put_be32(f, spapr->htab_shift);
> > +    /* "Iteration" header: no-HPT or HPT size encoding */
> > +    if (!spapr->htab_shift) {
> > +        qemu_put_be32(f, -1);
> 
> We're already using htab_shift == 0 to represent no HPT in the runtime
> structure; we might as well do the same on the wire.  As a bonus it
> slightly simplifies the logic here.

Non-zero value of iteration header (which is htab_shift) results in
htab_load() at the target to reallocate HTAB.

zero value of iteration header is used by htab_save_iterate() and
htab_save_complete() to tell htab_load() not to freshly allocate HTAB
at the target.

Hence we can't use 0 value to mean no-HPT.

I have addressed the rest of the comments on asserts by ensuring that
those code paths are taken only when HPT is present. v5 has those
changes.

Regards,
Bharata.

Re: [Qemu-devel] [PATCH v4 1/2] spapr: Add a "no HPT" encoding to HTAB migration stream

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3401 bytes --]

On Wed, Jun 07, 2017 at 01:23:32PM +0530, Bharata B Rao wrote:
> On Thu, Jun 01, 2017 at 02:54:48PM +1000, David Gibson wrote:
> > On Wed, May 31, 2017 at 04:56:44PM +0530, Bharata B Rao wrote:
> > > Add a "no HPT" encoding (using value -1) to the HTAB migration
> > > stream (in the place of HPT size) when the guest doesn't allocate HPT.
> > > This will help the target side to match target HPT with the source HPT
> > > and thus enable successful migration.
> > > 
> > > A few more fixes to enable TCG migration to work correctly are also
> > > included in this commit:
> > > 
> > > - HTAB savevm handlers have a few asserts on kvm_enabled() when
> > >   spapr->htab != 0. Convert these into conditional checks as it is now
> > >   possible to have no HTAB with TCG radix guests.
> > > - htab_save_setup() asserts for kvm_enabled() when spapr->htab != 0.
> > >   Remove this as we can't assert this for TCG radix guests.
> > > 
> > > Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> > >               [no HPT encoding suggestion]
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > 
> > Looks basically ok, but there are still some details to address.
> > 
> > > ---
> > >  hw/ppc/spapr.c | 31 +++++++++++++++++--------------
> > >  1 file changed, 17 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index ab3aab1..b589ed4 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1559,17 +1559,18 @@ static int htab_save_setup(QEMUFile *f, void *opaque)
> > >  {
> > >      sPAPRMachineState *spapr = opaque;
> > >  
> > > -    /* "Iteration" header */
> > > -    qemu_put_be32(f, spapr->htab_shift);
> > > +    /* "Iteration" header: no-HPT or HPT size encoding */
> > > +    if (!spapr->htab_shift) {
> > > +        qemu_put_be32(f, -1);
> > 
> > We're already using htab_shift == 0 to represent no HPT in the runtime
> > structure; we might as well do the same on the wire.  As a bonus it
> > slightly simplifies the logic here.
> 
> Non-zero value of iteration header (which is htab_shift) results in
> htab_load() at the target to reallocate HTAB.

Yeah.. but you can change that, right.  Have htab_load() remove the
HPT instead, if section_hdr == 0.

Older qemus that don't understand that certainly weren't going to be
able to take an incoming RPT guest anyway.

> zero value of iteration header is used by htab_save_iterate() and
> htab_save_complete() to tell htab_load() not to freshly allocate HTAB
> at the target.

But that makes no sense anyway.  How the destination goes about
allocating or not allocating the HPT shouldn't be the choice of the
source.  Since the source doesn't know what the destination has
already allocated, it can't possibly know if it's of a matching size,
likewise if the destination doesn't get a size, it can't know if the
current size is correct.

> Hence we can't use 0 value to mean no-HPT.

I really think we can..

> I have addressed the rest of the comments on asserts by ensuring that
> those code paths are taken only when HPT is present. v5 has those
> changes.
> 
> Regards,
> Bharata.
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[Qemu-devel] [PATCH v4 2/2] spapr: Fix migration of Radix guests

[Bharata B Rao]

Fix migration of radix guests by ensuring that we issue
KVM_PPC_CONFIGURE_V3_MMU for radix case post migration.

Reported-by: Nageswara R Sastry <rnsastry@linux.vnet.ibm.com>
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
 hw/ppc/spapr.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b589ed4..3f66a93 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1445,6 +1445,18 @@ static int spapr_post_load(void *opaque, int version_id)
         err = spapr_rtc_import_offset(&spapr->rtc, spapr->rtc_offset);
     }
 
+    if (spapr->patb_entry) {
+        PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
+        bool radix = !!(spapr->patb_entry & PATBE1_GR);
+        bool gtse = !!(cpu->env.spr[SPR_LPCR] & LPCR_GTSE);
+
+        err = kvmppc_configure_v3_mmu(cpu, radix, gtse, spapr->patb_entry);
+        if (err) {
+            error_report("Process table config unsupported by the host");
+            return -EINVAL;
+        }
+    }
+
     return err;
 }
 
-- 
2.7.4