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

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

[Bharata B Rao]

This patchset fixes the migration of sPAPR radix guests.

Migration of hash and radix guests individually has been tested on
TCG and KVM (P8 and P9 hosts). Changeover from HPT to RPT and vice versa
via reboot during migration isn't tested yet since it is possible
to test that currently only with TCG and TCG reboot is being fixed.

Changes in v6:
--------------
- Ensure any allocated HPT is free by the target when the source
  doesn't send HPT.

v5: https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg01637.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 | 41 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 37 insertions(+), 4 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v6 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.

Suggested-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 8b541d9..c425499 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1558,13 +1558,19 @@ static int htab_save_setup(QEMUFile *f, void *opaque)
     sPAPRMachineState *spapr = opaque;
 
     /* "Iteration" header */
-    qemu_put_be32(f, spapr->htab_shift);
+    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());
+        if (spapr->htab_shift) {
+            assert(kvm_enabled());
+        }
     }
 
 
@@ -1710,7 +1716,12 @@ static int htab_save_iterate(QEMUFile *f, void *opaque)
     int rc = 0;
 
     /* Iteration header */
-    qemu_put_be32(f, 0);
+    if (!spapr->htab_shift) {
+        qemu_put_be32(f, -1);
+        return 0;
+    } else {
+        qemu_put_be32(f, 0);
+    }
 
     if (!spapr->htab) {
         assert(kvm_enabled());
@@ -1744,7 +1755,12 @@ static int htab_save_complete(QEMUFile *f, void *opaque)
     int fd;
 
     /* Iteration header */
-    qemu_put_be32(f, 0);
+    if (!spapr->htab_shift) {
+        qemu_put_be32(f, -1);
+        return 0;
+    } else {
+        qemu_put_be32(f, 0);
+    }
 
     if (!spapr->htab) {
         int rc;
@@ -1788,6 +1804,11 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
 
     section_hdr = qemu_get_be32(f);
 
+    if (section_hdr == -1) {
+        spapr_free_hpt(spapr);
+        return 0;
+    }
+
     if (section_hdr) {
         Error *local_err = NULL;
 
-- 
2.7.4

[Qemu-devel] [PATCH v6 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 c425499..b2217f3 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1443,6 +1443,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

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

[David Gibson]

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

On Mon, Jun 12, 2017 at 11:02:33AM +0530, Bharata B Rao wrote:
> This patchset fixes the migration of sPAPR radix guests.
> 
> Migration of hash and radix guests individually has been tested on
> TCG and KVM (P8 and P9 hosts). Changeover from HPT to RPT and vice versa
> via reboot during migration isn't tested yet since it is possible
> to test that currently only with TCG and TCG reboot is being fixed.
> 
> Changes in v6:
> --------------
> - Ensure any allocated HPT is free by the target when the source
>   doesn't send HPT.

I've applied these to ppc-for-2.10, because I'm pretty sure they make
things better than they were.  There are some things I'd really like
to see addressed in followup patches, however.

-- 
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 v6 1/2] spapr: Add a "no HPT" encoding to HTAB migration stream

[David Gibson]

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

On Mon, Jun 12, 2017 at 11:02:34AM +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.
> 
> Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 8b541d9..c425499 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1558,13 +1558,19 @@ static int htab_save_setup(QEMUFile *f, void *opaque)
>      sPAPRMachineState *spapr = opaque;
>  
>      /* "Iteration" header */
> -    qemu_put_be32(f, spapr->htab_shift);
> +    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());
> +        if (spapr->htab_shift) {
> +            assert(kvm_enabled());
> +        }
>      }
>  
>  
> @@ -1710,7 +1716,12 @@ static int htab_save_iterate(QEMUFile *f, void *opaque)
>      int rc = 0;
>  
>      /* Iteration header */
> -    qemu_put_be32(f, 0);
> +    if (!spapr->htab_shift) {
> +        qemu_put_be32(f, -1);
> +        return 0;
> +    } else {
> +        qemu_put_be32(f, 0);
> +    }
>  
>      if (!spapr->htab) {
>          assert(kvm_enabled());
> @@ -1744,7 +1755,12 @@ static int htab_save_complete(QEMUFile *f, void *opaque)
>      int fd;
>  
>      /* Iteration header */
> -    qemu_put_be32(f, 0);
> +    if (!spapr->htab_shift) {
> +        qemu_put_be32(f, -1);
> +        return 0;
> +    } else {
> +        qemu_put_be32(f, 0);
> +    }

Do you actually need the modifications for _iterate and _complete?  I
would have thought you just wouldn't need to send any more of the HPT
stream at all after sending the -1 header.  

We should also adjust the downtime estimation logic so we don't allow
for transferring an HPT that isn't there.

>      if (!spapr->htab) {
>          int rc;
> @@ -1788,6 +1804,11 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
>  
>      section_hdr = qemu_get_be32(f);
>  
> +    if (section_hdr == -1) {
> +        spapr_free_hpt(spapr);
> +        return 0;
> +    }

Strictly speaking we probably shouldn't just return here.  We should
wait and see if there is more data in the stream.  Any actual content
(i.e. section_hdr == 0 sections) would be an error at this point.
However, a reset to an HPT guest would be represented by a new
non-zero section header, then more data.  This isn't urgent, but it
would be nice to fix at some point.

>      if (section_hdr) {
>          Error *local_err = NULL;
>  

-- 
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 v6 2/2] spapr: Fix migration of Radix guests

[David Gibson]

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

On Mon, Jun 12, 2017 at 11:02:35AM +0530, Bharata B Rao wrote:
> 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>

I believe we shouldn't assume a particular existing state on the
destination when processing an incoming stream (it's normally done
immediatley after startup, but I'm not sure it has to be).

So, we shouldn't only call configure_v3_mmu when patb_entry != 0.  For
the case of an incoming hash guest, I believe we should explicitly
configure KVM to HPT mode, rather than assume it's there already.

> ---
>  hw/ppc/spapr.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index c425499..b2217f3 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1443,6 +1443,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;
>  }
>  

-- 
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 v6 1/2] spapr: Add a "no HPT" encoding to HTAB migration stream

[Bharata B Rao]

On Mon, Jun 12, 2017 at 05:10:44PM +0800, David Gibson wrote:
> On Mon, Jun 12, 2017 at 11:02:34AM +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.
> > 
> > Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  hw/ppc/spapr.c | 29 +++++++++++++++++++++++++----
> >  1 file changed, 25 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 8b541d9..c425499 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1558,13 +1558,19 @@ static int htab_save_setup(QEMUFile *f, void *opaque)
> >      sPAPRMachineState *spapr = opaque;
> >  
> >      /* "Iteration" header */
> > -    qemu_put_be32(f, spapr->htab_shift);
> > +    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());
> > +        if (spapr->htab_shift) {
> > +            assert(kvm_enabled());
> > +        }
> >      }
> >  
> >  
> > @@ -1710,7 +1716,12 @@ static int htab_save_iterate(QEMUFile *f, void *opaque)
> >      int rc = 0;
> >  
> >      /* Iteration header */
> > -    qemu_put_be32(f, 0);
> > +    if (!spapr->htab_shift) {
> > +        qemu_put_be32(f, -1);
> > +        return 0;
> > +    } else {
> > +        qemu_put_be32(f, 0);
> > +    }
> >  
> >      if (!spapr->htab) {
> >          assert(kvm_enabled());
> > @@ -1744,7 +1755,12 @@ static int htab_save_complete(QEMUFile *f, void *opaque)
> >      int fd;
> >  
> >      /* Iteration header */
> > -    qemu_put_be32(f, 0);
> > +    if (!spapr->htab_shift) {
> > +        qemu_put_be32(f, -1);
> > +        return 0;
> > +    } else {
> > +        qemu_put_be32(f, 0);
> > +    }
> 
> Do you actually need the modifications for _iterate and _complete?  I
> would have thought you just wouldn't need to send any more of the HPT
> stream at all after sending the -1 header.  

_setup, _iterate, _complete handler routines for HTAB always get interspersed
with similar routines for ram savevm handlers as per what I have seen.
And moreover these are called by the core migration code and hence we they get
called, we need these changes to ensure that we don't attempt to send HPT
stream.

> 
> We should also adjust the downtime estimation logic so we don't allow
> for transferring an HPT that isn't there.

I will have to check that out.

> 
> >      if (!spapr->htab) {
> >          int rc;
> > @@ -1788,6 +1804,11 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
> >  
> >      section_hdr = qemu_get_be32(f);
> >  
> > +    if (section_hdr == -1) {
> > +        spapr_free_hpt(spapr);
> > +        return 0;
> > +    }
> 
> Strictly speaking we probably shouldn't just return here.  We should
> wait and see if there is more data in the stream.

Because of the way the source sends data (with HPT data getting
interspersed with ram data, I don't think we can say for sure if
we got or will get any HPT data following the no-HPT indication.

> Any actual content
> (i.e. section_hdr == 0 sections) would be an error at this point.
> However, a reset to an HPT guest would be represented by a new
> non-zero section header, then more data.  This isn't urgent, but it
> would be nice to fix at some point.

Sure.

Regards,
Bharata.

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

[David Gibson]

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

On Tue, Jun 13, 2017 at 10:18:18AM +0530, Bharata B Rao wrote:
> On Mon, Jun 12, 2017 at 05:10:44PM +0800, David Gibson wrote:
> > On Mon, Jun 12, 2017 at 11:02:34AM +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.
> > > 
> > > Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  hw/ppc/spapr.c | 29 +++++++++++++++++++++++++----
> > >  1 file changed, 25 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 8b541d9..c425499 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1558,13 +1558,19 @@ static int htab_save_setup(QEMUFile *f, void *opaque)
> > >      sPAPRMachineState *spapr = opaque;
> > >  
> > >      /* "Iteration" header */
> > > -    qemu_put_be32(f, spapr->htab_shift);
> > > +    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());
> > > +        if (spapr->htab_shift) {
> > > +            assert(kvm_enabled());
> > > +        }
> > >      }
> > >  
> > >  
> > > @@ -1710,7 +1716,12 @@ static int htab_save_iterate(QEMUFile *f, void *opaque)
> > >      int rc = 0;
> > >  
> > >      /* Iteration header */
> > > -    qemu_put_be32(f, 0);
> > > +    if (!spapr->htab_shift) {
> > > +        qemu_put_be32(f, -1);
> > > +        return 0;
> > > +    } else {
> > > +        qemu_put_be32(f, 0);
> > > +    }
> > >  
> > >      if (!spapr->htab) {
> > >          assert(kvm_enabled());
> > > @@ -1744,7 +1755,12 @@ static int htab_save_complete(QEMUFile *f, void *opaque)
> > >      int fd;
> > >  
> > >      /* Iteration header */
> > > -    qemu_put_be32(f, 0);
> > > +    if (!spapr->htab_shift) {
> > > +        qemu_put_be32(f, -1);
> > > +        return 0;
> > > +    } else {
> > > +        qemu_put_be32(f, 0);
> > > +    }
> > 
> > Do you actually need the modifications for _iterate and _complete?  I
> > would have thought you just wouldn't need to send any more of the HPT
> > stream at all after sending the -1 header.  
> 
> _setup, _iterate, _complete handler routines for HTAB always get interspersed
> with similar routines for ram savevm handlers as per what I have seen.
> And moreover these are called by the core migration code and hence we they get
> called, we need these changes to ensure that we don't attempt to send HPT
> stream.

Ah, yes of course.

> > We should also adjust the downtime estimation logic so we don't allow
> > for transferring an HPT that isn't there.
> 
> I will have to check that out.
> 
> > 
> > >      if (!spapr->htab) {
> > >          int rc;
> > > @@ -1788,6 +1804,11 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
> > >  
> > >      section_hdr = qemu_get_be32(f);
> > >  
> > > +    if (section_hdr == -1) {
> > > +        spapr_free_hpt(spapr);
> > > +        return 0;
> > > +    }
> > 
> > Strictly speaking we probably shouldn't just return here.  We should
> > wait and see if there is more data in the stream.
> 
> Because of the way the source sends data (with HPT data getting
> interspersed with ram data, I don't think we can say for sure if
> we got or will get any HPT data following the no-HPT indication.

We definitely shouldn't - there's no way the destination could handle
it without knowing the size first.  We could get a new header giving
an HPT size, and then get HPT data.


> > Any actual content
> > (i.e. section_hdr == 0 sections) would be an error at this point.
> > However, a reset to an HPT guest would be represented by a new
> > non-zero section header, then more data.  This isn't urgent, but it
> > would be nice to fix at some point.
> 
> Sure.
> 
> 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 --]