Re: [Qemu-devel] [RFC PATCH v2 2/4] spapr: Unregister HPT savevm handlers for radix guests

[David Gibson]

On Fri, May 19, 2017 at 11:10:37AM +0530, Bharata B Rao wrote:
> HPT gets created by default for TCG guests 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.
> 
> HTAB savevm handlers registration/unregistration is now done from
> spapr_reallocate_hpt() which itself is called from one of the
> savevm_htab_handlers.htab_load(). To cater to this circular dependency
> spapr_reallocate_hpt() is made global.
> 
> Signed-off-by: Bharata B Rao <address@hidden>

Reviewed-by: David Gibson <address@hidden>


Weirdly, diff seems to have done a terrible job at minimizing the
patch below.  AFAICT this is really just a one line addition to
spapr_free_hpt() and spapr_reallocate_hpt().

> ---
>  hw/ppc/spapr.c         | 99 
> +++++++++++++++++++++++++-------------------------
>  include/hw/ppc/spapr.h |  2 +
>  2 files changed, 52 insertions(+), 49 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 91f7434..daf335c 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1233,53 +1233,7 @@ void spapr_free_hpt(sPAPRMachineState *spapr)
>      spapr->htab = NULL;
>      spapr->htab_shift = 0;
>      close_htab_fd(spapr);
> -}
> -
> -static void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
> -                                 Error **errp)
> -{
> -    long rc;
> -
> -    /* Clean up any HPT info from a previous boot */
> -    spapr_free_hpt(spapr);
> -
> -    rc = kvmppc_reset_htab(shift);
> -    if (rc < 0) {
> -        /* kernel-side HPT needed, but couldn't allocate one */
> -        error_setg_errno(errp, errno,
> -                         "Failed to allocate KVM HPT of order %d (try 
> smaller maxmem?)",
> -                         shift);
> -        /* This is almost certainly fatal, but if the caller really
> -         * wants to carry on with shift == 0, it's welcome to try */
> -    } else if (rc > 0) {
> -        /* kernel-side HPT allocated */
> -        if (rc != shift) {
> -            error_setg(errp,
> -                       "Requested order %d HPT, but kernel allocated order 
> %ld (try smaller maxmem?)",
> -                       shift, rc);
> -        }
> -
> -        spapr->htab_shift = shift;
> -        spapr->htab = NULL;
> -    } else {
> -        /* kernel-side HPT not needed, allocate in userspace instead */
> -        size_t size = 1ULL << shift;
> -        int i;
> -
> -        spapr->htab = qemu_memalign(size, size);
> -        if (!spapr->htab) {
> -            error_setg_errno(errp, errno,
> -                             "Could not allocate HPT of order %d", shift);
> -            return;
> -        }
> -
> -        memset(spapr->htab, 0, size);
> -        spapr->htab_shift = shift;
> -
> -        for (i = 0; i < size / HASH_PTE_SIZE_64; i++) {
> -            DIRTY_HPTE(HPTE(spapr->htab, i));
> -        }
> -    }
> +    unregister_savevm_live(NULL, "spapr/htab", spapr);
>  }
>  
>  void spapr_setup_hpt_and_vrma(sPAPRMachineState *spapr)
> @@ -1879,6 +1833,55 @@ static SaveVMHandlers savevm_htab_handlers = {
>      .load_state = htab_load,
>  };
>  
> +void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
> +                                 Error **errp)
> +{
> +    long rc;
> +
> +    /* Clean up any HPT info from a previous boot */
> +    spapr_free_hpt(spapr);
> +
> +    rc = kvmppc_reset_htab(shift);
> +    if (rc < 0) {
> +        /* kernel-side HPT needed, but couldn't allocate one */
> +        error_setg_errno(errp, errno,
> +                         "Failed to allocate KVM HPT of order %d (try 
> smaller maxmem?)",
> +                         shift);
> +        /* This is almost certainly fatal, but if the caller really
> +         * wants to carry on with shift == 0, it's welcome to try */
> +    } else if (rc > 0) {
> +        /* kernel-side HPT allocated */
> +        if (rc != shift) {
> +            error_setg(errp,
> +                       "Requested order %d HPT, but kernel allocated order 
> %ld (try smaller maxmem?)",
> +                       shift, rc);
> +        }
> +
> +        spapr->htab_shift = shift;
> +        spapr->htab = NULL;
> +    } else {
> +        /* kernel-side HPT not needed, allocate in userspace instead */
> +        size_t size = 1ULL << shift;
> +        int i;
> +
> +        spapr->htab = qemu_memalign(size, size);
> +        if (!spapr->htab) {
> +            error_setg_errno(errp, errno,
> +                             "Could not allocate HPT of order %d", shift);
> +            return;
> +        }
> +
> +        memset(spapr->htab, 0, size);
> +        spapr->htab_shift = shift;
> +
> +        for (i = 0; i < size / HASH_PTE_SIZE_64; i++) {
> +            DIRTY_HPTE(HPTE(spapr->htab, i));
> +        }
> +    }
> +    register_savevm_live(NULL, "spapr/htab", -1, 1,
> +                         &savevm_htab_handlers, spapr);
> +}
> +
>  static void spapr_boot_set(void *opaque, const char *boot_device,
>                             Error **errp)
>  {
> @@ -2341,8 +2344,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/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index f875dc4..e581c4a 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -637,6 +637,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_reallocate_hpt(sPAPRMachineState *spapr, int shift,
> +                                 Error **errp);
>  
>  /* 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

signature.asc
Description: PGP signature