1. Patchew
  2. QEMU
  3. [Qemu-devel] [RFC PATCH v1 0/6] ppc/spapr: Fix migration of radix guests
  4. View patch

[Qemu-devel] [RFC PATCH v1 5/6] spapr: Unregister HPT savevm handlers for radix guests

Bharata B Rao posted 6 patches 8 years, 8 months ago
There is a newer version of this series
[Qemu-devel] [RFC PATCH v1 5/6] spapr: Unregister HPT savevm handlers for radix guests
Posted by Bharata B Rao 8 years, 8 months ago 
HPT gets created by default and later when the guest turns out to be
a radix guest, the HPT is destroyed when guest does H_REGISTER_PROC_TBL
hcall. Let HTAB savevm handlers registration and unregistration follow
the same model so that we don't end up having unrequired HTAB savevm
handlers for radix guests.

This also ensures that HTAB savevm handlers seemlessly get destroyed and
recreated like HTAB itself when hash guest reboots.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c         | 15 +++++++++++++--
 hw/ppc/spapr_hcall.c   |  1 +
 include/hw/ppc/spapr.h |  2 ++
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 521eef1..05abfc1 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1237,6 +1237,7 @@ static void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
 
     /* Clean up any HPT info from a previous boot */
     spapr_free_hpt(spapr);
+    spapr_htab_savevm_unregister(spapr);
 
     rc = kvmppc_reset_htab(shift);
     if (rc < 0) {
@@ -1275,6 +1276,7 @@ static void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
             DIRTY_HPTE(HPTE(spapr->htab, i));
         }
     }
+    spapr_htab_savevm_register(spapr);
 }
 
 void spapr_setup_hpt_and_vrma(sPAPRMachineState *spapr)
@@ -1874,6 +1876,17 @@ static SaveVMHandlers savevm_htab_handlers = {
     .load_state = htab_load,
 };
 
+void spapr_htab_savevm_register(sPAPRMachineState *spapr)
+{
+    register_savevm_live(NULL, "spapr/htab", -1, 1,
+                         &savevm_htab_handlers, spapr);
+}
+
+void spapr_htab_savevm_unregister(sPAPRMachineState *spapr)
+{
+    unregister_savevm_live(NULL, "spapr/htab", spapr);
+}
+
 static void spapr_boot_set(void *opaque, const char *boot_device,
                            Error **errp)
 {
@@ -2336,8 +2349,6 @@ static void ppc_spapr_init(MachineState *machine)
      * interface, this is a legacy from the sPAPREnvironment structure
      * which predated MachineState but had a similar function */
     vmstate_register(NULL, 0, &vmstate_spapr, spapr);
-    register_savevm_live(NULL, "spapr/htab", -1, 1,
-                         &savevm_htab_handlers, spapr);
 
     /* used by RTAS */
     QTAILQ_INIT(&spapr->ccs_list);
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index be79e3d..768aa57 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -914,6 +914,7 @@ static void spapr_check_setup_free_hpt(sPAPRMachineState *spapr,
     } else if (!(patbe_old & PATBE1_GR)) {
         /* HASH->RADIX : Free HPT */
         spapr_free_hpt(spapr);
+        spapr_htab_savevm_unregister(spapr);
     } else if (!(patbe_new & PATBE1_GR)) {
         /* RADIX->HASH || NOTHING->HASH : Allocate HPT */
         spapr_setup_hpt_and_vrma(spapr);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 6f9cb85..5b39a26 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -636,6 +636,8 @@ void spapr_hotplug_req_remove_by_count_indexed(sPAPRDRConnectorType drc_type,
                                                uint32_t count, uint32_t index);
 void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset,
                                     sPAPRMachineState *spapr);
+void spapr_htab_savevm_register(sPAPRMachineState *spapr);
+void spapr_htab_savevm_unregister(sPAPRMachineState *spapr);
 
 /* rtas-configure-connector state */
 struct sPAPRConfigureConnectorState {
-- 
2.7.4
Re: [Qemu-devel] [RFC PATCH v1 5/6] spapr: Unregister HPT savevm handlers for radix guests
Posted by David Gibson 8 years, 8 months ago 
On Wed, May 17, 2017 at 09:19:21AM +0530, Bharata B Rao wrote:
> HPT gets created by default and later when the guest turns out to be
> a radix guest, the HPT is destroyed when guest does H_REGISTER_PROC_TBL
> hcall.

I don't think that's entirely accurate.  At least in some KVM
configurations, we assume radix first, and only allocate the HPT once
the guest confirms it is doing hash.

> Let HTAB savevm handlers registration and unregistration follow
> the same model so that we don't end up having unrequired HTAB savevm
> handlers for radix guests.
> 
> This also ensures that HTAB savevm handlers seemlessly get destroyed and
> recreated like HTAB itself when hash guest reboots.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c         | 15 +++++++++++++--
>  hw/ppc/spapr_hcall.c   |  1 +
>  include/hw/ppc/spapr.h |  2 ++
>  3 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 521eef1..05abfc1 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1237,6 +1237,7 @@ static void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
>  
>      /* Clean up any HPT info from a previous boot */
>      spapr_free_hpt(spapr);
> +    spapr_htab_savevm_unregister(spapr);

I'd prefer that the unregister be folded into spapr_free_hpt().
Basically we want calls that create or remove the HPT and handle
everything - allocation/freeing if necessary, informing KVM if
necessary, and registering/deregistering the savevm handlers if
necesary.

I think that will also remove the need for the trivial
spapr_htab_savevm_{un,}register() wrappers.

>  
>      rc = kvmppc_reset_htab(shift);
>      if (rc < 0) {
> @@ -1275,6 +1276,7 @@ static void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
>              DIRTY_HPTE(HPTE(spapr->htab, i));
>          }
>      }
> +    spapr_htab_savevm_register(spapr);
>  }
>  
>  void spapr_setup_hpt_and_vrma(sPAPRMachineState *spapr)
> @@ -1874,6 +1876,17 @@ static SaveVMHandlers savevm_htab_handlers = {
>      .load_state = htab_load,
>  };
>  
> +void spapr_htab_savevm_register(sPAPRMachineState *spapr)
> +{
> +    register_savevm_live(NULL, "spapr/htab", -1, 1,
> +                         &savevm_htab_handlers, spapr);
> +}
> +
> +void spapr_htab_savevm_unregister(sPAPRMachineState *spapr)
> +{
> +    unregister_savevm_live(NULL, "spapr/htab", spapr);
> +}
> +
>  static void spapr_boot_set(void *opaque, const char *boot_device,
>                             Error **errp)
>  {
> @@ -2336,8 +2349,6 @@ static void ppc_spapr_init(MachineState *machine)
>       * interface, this is a legacy from the sPAPREnvironment structure
>       * which predated MachineState but had a similar function */
>      vmstate_register(NULL, 0, &vmstate_spapr, spapr);
> -    register_savevm_live(NULL, "spapr/htab", -1, 1,
> -                         &savevm_htab_handlers, spapr);
>  
>      /* used by RTAS */
>      QTAILQ_INIT(&spapr->ccs_list);
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index be79e3d..768aa57 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -914,6 +914,7 @@ static void spapr_check_setup_free_hpt(sPAPRMachineState *spapr,
>      } else if (!(patbe_old & PATBE1_GR)) {
>          /* HASH->RADIX : Free HPT */
>          spapr_free_hpt(spapr);
> +        spapr_htab_savevm_unregister(spapr);
>      } else if (!(patbe_new & PATBE1_GR)) {
>          /* RADIX->HASH || NOTHING->HASH : Allocate HPT */
>          spapr_setup_hpt_and_vrma(spapr);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 6f9cb85..5b39a26 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -636,6 +636,8 @@ void spapr_hotplug_req_remove_by_count_indexed(sPAPRDRConnectorType drc_type,
>                                                 uint32_t count, uint32_t index);
>  void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset,
>                                      sPAPRMachineState *spapr);
> +void spapr_htab_savevm_register(sPAPRMachineState *spapr);
> +void spapr_htab_savevm_unregister(sPAPRMachineState *spapr);
>  
>  /* rtas-configure-connector state */
>  struct sPAPRConfigureConnectorState {

-- 
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
Re: [Qemu-devel] [RFC PATCH v1 5/6] spapr: Unregister HPT savevm handlers for radix guests
Posted by Bharata B Rao 8 years, 8 months ago 
On Wed, May 17, 2017 at 04:59:33PM +1000, David Gibson wrote:
> On Wed, May 17, 2017 at 09:19:21AM +0530, Bharata B Rao wrote:
> > HPT gets created by default and later when the guest turns out to be
> > a radix guest, the HPT is destroyed when guest does H_REGISTER_PROC_TBL
> > hcall.
> 
> I don't think that's entirely accurate.  At least in some KVM
> configurations, we assume radix first, and only allocate the HPT once
> the guest confirms it is doing hash.

Right, that statement is true for TCG radix guests, will rephrase it.

> 
> > Let HTAB savevm handlers registration and unregistration follow
> > the same model so that we don't end up having unrequired HTAB savevm
> > handlers for radix guests.
> > 
> > This also ensures that HTAB savevm handlers seemlessly get destroyed and
> > recreated like HTAB itself when hash guest reboots.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  hw/ppc/spapr.c         | 15 +++++++++++++--
> >  hw/ppc/spapr_hcall.c   |  1 +
> >  include/hw/ppc/spapr.h |  2 ++
> >  3 files changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 521eef1..05abfc1 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1237,6 +1237,7 @@ static void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
> >  
> >      /* Clean up any HPT info from a previous boot */
> >      spapr_free_hpt(spapr);
> > +    spapr_htab_savevm_unregister(spapr);
> 
> I'd prefer that the unregister be folded into spapr_free_hpt().
> Basically we want calls that create or remove the HPT and handle
> everything - allocation/freeing if necessary, informing KVM if
> necessary, and registering/deregistering the savevm handlers if
> necesary.
> 
> I think that will also remove the need for the trivial
> spapr_htab_savevm_{un,}register() wrappers.

Sure, will consolidate this in the next version.

Regards,
Bharata.
Re: [Qemu-devel] [RFC PATCH v1 5/6] spapr: Unregister HPT savevm handlers for radix guests
Posted by David Gibson 8 years, 8 months ago 
On Wed, May 17, 2017 at 12:48:21PM +0530, Bharata B Rao wrote:
> On Wed, May 17, 2017 at 04:59:33PM +1000, David Gibson wrote:
> > On Wed, May 17, 2017 at 09:19:21AM +0530, Bharata B Rao wrote:
> > > HPT gets created by default and later when the guest turns out to be
> > > a radix guest, the HPT is destroyed when guest does H_REGISTER_PROC_TBL
> > > hcall.
> > 
> > I don't think that's entirely accurate.  At least in some KVM
> > configurations, we assume radix first, and only allocate the HPT once
> > the guest confirms it is doing hash.
> 
> Right, that statement is true for TCG radix guests, will rephrase
> it.

And may not remain true even for TCG radix guests.  Since for TCG mode
we don't actually need either an HPT or an RPT while the guest is in
real mode, I've suggested postponing HPT allocation to CAS time for
TCG as well; I think that will make for slightly fewer differeneces
between the KVM and TCG paths.

> > > Let HTAB savevm handlers registration and unregistration follow
> > > the same model so that we don't end up having unrequired HTAB savevm
> > > handlers for radix guests.
> > > 
> > > This also ensures that HTAB savevm handlers seemlessly get destroyed and
> > > recreated like HTAB itself when hash guest reboots.
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  hw/ppc/spapr.c         | 15 +++++++++++++--
> > >  hw/ppc/spapr_hcall.c   |  1 +
> > >  include/hw/ppc/spapr.h |  2 ++
> > >  3 files changed, 16 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 521eef1..05abfc1 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1237,6 +1237,7 @@ static void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
> > >  
> > >      /* Clean up any HPT info from a previous boot */
> > >      spapr_free_hpt(spapr);
> > > +    spapr_htab_savevm_unregister(spapr);
> > 
> > I'd prefer that the unregister be folded into spapr_free_hpt().
> > Basically we want calls that create or remove the HPT and handle
> > everything - allocation/freeing if necessary, informing KVM if
> > necessary, and registering/deregistering the savevm handlers if
> > necesary.
> > 
> > I think that will also remove the need for the trivial
> > spapr_htab_savevm_{un,}register() wrappers.
> 
> Sure, will consolidate this in the next version.
> 
> 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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.