qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [for-2.9 2/5] pseries: Stubs for HPT resizing


From: David Gibson
Subject: Re: [Qemu-devel] [for-2.9 2/5] pseries: Stubs for HPT resizing
Date: Fri, 9 Dec 2016 20:09:17 +1100
User-agent: Mutt/1.7.1 (2016-10-04)

On Fri, Dec 09, 2016 at 09:18:51AM +0100, Thomas Huth wrote:
> On 09.12.2016 03:23, David Gibson wrote:
> > This introduces stub implementations of the H_RESIZE_HPT_PREPARE and
> > H_RESIZE_HPT_COMMIT hypercalls which we hope to add in a PAPR
> > extension to allow run time resizing of a guest's hash page table.  It
> > also adds a new machine property for controlling whether this new
> > facility is available, and logic to check that against availability
> > with KVM (only supported with KVM PR for now).
> > 
> > Finally, it adds a new string to the hypertas property in the device
> > tree, advertising to the guest the availability of the HPT resizing
> > hypercalls.  This is a tentative suggested value, and would need to be
> > standardized by PAPR before being merged.
> > 
> > Signed-off-by: David Gibson <address@hidden>
> > ---
> >  hw/ppc/spapr.c         | 66 
> > ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  hw/ppc/spapr_hcall.c   | 36 +++++++++++++++++++++++++++
> >  hw/ppc/trace-events    |  2 ++
> >  include/hw/ppc/spapr.h | 11 +++++++++
> >  target-ppc/kvm.c       | 26 ++++++++++++++++++++
> >  target-ppc/kvm_ppc.h   |  5 ++++
> >  6 files changed, 146 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 0f25e83..ecb0822 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -760,6 +760,11 @@ static void spapr_dt_rtas(sPAPRMachineState *spapr, 
> > void *fdt)
> >      if (!kvm_enabled() || kvmppc_spapr_use_multitce()) {
> >          add_str(hypertas, "hcall-multi-tce");
> >      }
> > +
> > +    if (spapr->resize_hpt != SPAPR_RESIZE_HPT_DISABLED) {
> > +        add_str(hypertas, "hcall-hpt-resize");
> > +    }
> > +
> >      _FDT(fdt_setprop(fdt, rtas, "ibm,hypertas-functions",
> >                       hypertas->str, hypertas->len));
> >      g_string_free(hypertas, TRUE);
> > @@ -1839,11 +1844,31 @@ static void ppc_spapr_init(MachineState *machine)
> >      long load_limit, fw_size;
> >      char *filename;
> >      int smt = kvmppc_smt_threads();
> > +    Error *resize_hpt_err = NULL;
> >  
> >      msi_nonbroken = true;
> >  
> >      QLIST_INIT(&spapr->phbs);
> >  
> > +    /* Check HPT resizing availability */
> > +    kvmppc_check_papr_resize_hpt(&resize_hpt_err);
> > +    if (spapr->resize_hpt == SPAPR_RESIZE_HPT_DEFAULT) {
> > +        if (resize_hpt_err) {
> > +            spapr->resize_hpt = SPAPR_RESIZE_HPT_DISABLED;
> > +            error_free(resize_hpt_err);
> > +            resize_hpt_err = NULL;
> > +        } else {
> > +            spapr->resize_hpt = smc->resize_hpt_default;
> > +        }
> > +    }
> > +
> > +    assert(spapr->resize_hpt != SPAPR_RESIZE_HPT_DEFAULT);
> > +
> > +    if ((spapr->resize_hpt != SPAPR_RESIZE_HPT_DISABLED) && 
> > resize_hpt_err) {
> > +        error_report_err(resize_hpt_err);
> > +        exit(1);
> > +    }
> 
> The logic here is IMHO a little bit hard to understand. Why do you need
> the SPAPR_RESIZE_HPT_DEFAULT state here at all? Wouldn't it be
> sufficient to start with SPAPR_RESIZE_HPT_DISABLED by default and to
> only enable it when the extension is available?

Because if the user explicitly sets something, we should never
override itin the logic here.  In order to determine whether the user
has set something explicitly, we need another value that means "not
set explicitly".

> ... at least some
> explaining comments in the code about SPAPR_RESIZE_HPT_DEFAULT could
> help here.

Hm, maybe.

> > @@ -2236,6 +2261,40 @@ static void spapr_set_modern_hotplug_events(Object 
> > *obj, bool value,
> >      spapr->use_hotplug_event_source = value;
> >  }
> >  
> > +static char *spapr_get_resize_hpt(Object *obj, Error **errp)
> > +{
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> > +
> > +    switch (spapr->resize_hpt) {
> > +    case SPAPR_RESIZE_HPT_DEFAULT:
> > +        return g_strdup("default");
> > +    case SPAPR_RESIZE_HPT_DISABLED:
> > +        return g_strdup("disabled");
> > +    case SPAPR_RESIZE_HPT_ENABLED:
> > +        return g_strdup("enabled");
> > +    case SPAPR_RESIZE_HPT_REQUIRED:
> > +        return g_strdup("required");
> > +    }
> > +    assert(0);
> > +}
> > +
> > +static void spapr_set_resize_hpt(Object *obj, const char *value, Error 
> > **errp)
> > +{
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> > +
> > +    if (strcmp(value, "default") == 0) {
> > +        spapr->resize_hpt = SPAPR_RESIZE_HPT_DEFAULT;
> > +    } else if (strcmp(value, "disabled") == 0) {
> > +        spapr->resize_hpt = SPAPR_RESIZE_HPT_DISABLED;
> > +    } else if (strcmp(value, "enabled") == 0) {
> > +        spapr->resize_hpt = SPAPR_RESIZE_HPT_ENABLED;
> > +    } else if (strcmp(value, "required") == 0) {
> > +        spapr->resize_hpt = SPAPR_RESIZE_HPT_REQUIRED;
> > +    } else {
> > +        error_setg(errp, "Bad value for \"resize-hpt\" property");
> > +    }
> > +}
> > +
> >  static void spapr_machine_initfn(Object *obj)
> >  {
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> > @@ -2256,6 +2315,12 @@ static void spapr_machine_initfn(Object *obj)
> >                                      " place of standard EPOW events when 
> > possible"
> >                                      " (required for memory hot-unplug 
> > support)",
> >                                      NULL);
> > +
> > +    object_property_add_str(obj, "resize-hpt",
> > +                            spapr_get_resize_hpt, spapr_set_resize_hpt, 
> > NULL);
> > +    object_property_set_description(obj, "resize-hpt",
> > +                                    "Resizing of the Hash Page Table 
> > (enabled, disabled, required)",
> > +                                    NULL);
> 
> ... and "default" ?
> 
> >  }
> >  
> >  static void spapr_machine_finalizefn(Object *obj)
> > @@ -2707,6 +2772,7 @@ static void spapr_machine_class_init(ObjectClass *oc, 
> > void *data)
> >  
> >      smc->dr_lmb_enabled = true;
> >      smc->tcg_default_cpu = "POWER8";
> > +    smc->resize_hpt_default = SPAPR_RESIZE_HPT_DISABLED;
> >      mc->query_hotpluggable_cpus = spapr_query_hotpluggable_cpus;
> >      fwc->get_dev_path = spapr_get_fw_dev_path;
> >      nc->nmi_monitor_handler = spapr_nmi;
> ...
> > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> > index 15e12f3..dcd8cef 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -22,6 +22,7 @@
> >  #include <linux/kvm.h>
> >  
> >  #include "qemu-common.h"
> > +#include "qapi/error.h"
> >  #include "qemu/error-report.h"
> >  #include "cpu.h"
> >  #include "qemu/timer.h"
> > @@ -2269,6 +2270,7 @@ int kvmppc_reset_htab(int shift_hint)
> >          /* Full emulation, tell caller to allocate htab itself */
> >          return 0;
> >      }
> > +
> 
> Unnecessary white space change.
> 
> >      if (kvm_check_extension(kvm_state, KVM_CAP_PPC_ALLOC_HTAB)) {
> >          int ret;
> >          ret = kvm_vm_ioctl(kvm_state, KVM_PPC_ALLOCATE_HTAB, &shift);
> > @@ -2672,3 +2674,27 @@ int kvmppc_enable_hwrng(void)
> >  
> >      return kvmppc_enable_hcall(kvm_state, H_RANDOM);
> >  }
> > +
> > +void kvmppc_check_papr_resize_hpt(Error **errp)
> > +{
> > +    if (!kvm_enabled()) {
> > +        return;
> > +    }
> > +
> > +    /* TODO: Check specific capabilities for HPT resize aware host kernels 
> > */
> > +
> > +    /*
> > +     * It's tempting to try to check directly if the HPT is under our
> > +     * control or KVM's, which is what's really relevant here.
> > +     * Unfortunately, in order to correctly size the HPT, we need to
> > +     * know if we can do resizing, _before_ we attempt to allocate it
> > +     * with KVM.  Before that point, we don't officially know whether
> > +     * we'll control the HPT or not.  So we have to use a fallback
> > +     * test for PR vs HV KVM to predict that.
> > +     */
> > +    if (kvmppc_is_pr(kvm_state)) {
> > +        return;
> > +    }
> 
> Couldn't we get HPT resizing on KVM-PR one day, too?

I guess I need to reword that.  HPT resizing should work on PR right
now (but I haven't been able to test it yet) - because qemu still
allocates and manages the HPT on PR, the qemu side logic is
sufficient.

> > +    error_setg(errp, "Hash page table resizing not available with this KVM 
> > version");
> > +}
> > diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
> > index 841a29b..3e852ba 100644
> > --- a/target-ppc/kvm_ppc.h
> > +++ b/target-ppc/kvm_ppc.h
> > @@ -59,6 +59,7 @@ bool kvmppc_has_cap_htm(void);
> >  int kvmppc_enable_hwrng(void);
> >  int kvmppc_put_books_sregs(PowerPCCPU *cpu);
> >  PowerPCCPUClass *kvm_ppc_get_host_cpu_class(void);
> > +void kvmppc_check_papr_resize_hpt(Error **errp);
> >  
> >  #else
> >  
> > @@ -270,6 +271,10 @@ static inline PowerPCCPUClass 
> > *kvm_ppc_get_host_cpu_class(void)
> >      return NULL;
> >  }
> >  
> > +static inline void kvmppc_check_papr_resize_hpt(Error **errp)
> > +{
> > +    return;
> > +}
> >  #endif
> 
>  Thomas
> 

-- 
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: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]