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

[David Gibson <david@gibson.dropbear.id.au>]

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

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

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