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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Bharata B Rao-3
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


Reply | Threaded
Open this post in threaded view
|

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

Bharata B Rao-3
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 <[hidden email]>
Signed-off-by: Bharata B Rao <[hidden email]>
---
 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


Reply | Threaded
Open this post in threaded view
|

[PATCH v6 2/2] spapr: Fix migration of Radix guests

Bharata B Rao-3
In reply to this post by Bharata B Rao-3
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 <[hidden email]>
Signed-off-by: Bharata B Rao <[hidden email]>
Reviewed-by: Suraj Jitindar Singh <[hidden email]>
---
 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


Reply | Threaded
Open this post in threaded view
|

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

David Gibson
In reply to this post by Bharata B Rao-3
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

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

David Gibson
In reply to this post by Bharata B Rao-3
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 <[hidden email]>
> Signed-off-by: Bharata B Rao <[hidden email]>
> ---
>  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

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v6 2/2] spapr: Fix migration of Radix guests

David Gibson
In reply to this post by Bharata B Rao-3
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 <[hidden email]>
> Signed-off-by: Bharata B Rao <[hidden email]>
> Reviewed-by: Suraj Jitindar Singh <[hidden email]>

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

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Bharata B Rao-3
In reply to this post by David Gibson
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 <[hidden email]>
> > Signed-off-by: Bharata B Rao <[hidden email]>
> > ---
> >  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.


Reply | Threaded
Open this post in threaded view
|

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

David Gibson
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 <[hidden email]>
> > > Signed-off-by: Bharata B Rao <[hidden email]>
> > > ---
> > >  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

signature.asc (836 bytes) Download Attachment