qemu-ppc
[Top][All Lists]
Advanced

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

Re: [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Me


From: David Gibson
Subject: Re: [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability
Date: Tue, 12 Dec 2017 11:20:17 +1100
User-agent: Mutt/1.9.1 (2017-09-22)

On Mon, Dec 11, 2017 at 08:20:30PM +1100, Alexey Kardashevskiy wrote:
> On 11/12/17 18:08, David Gibson wrote:
> > This adds an spapr capability bit for Hardware Transactional Memory.  By
> > default it is enabled for all machine type versions with POWER8 and later
> > CPUs.
> > 
> > This means that "qemu -machine pseries,accel=tcg =cpu POWER8" will now
> > fail to start, however that configuration was already broken in that it
> > would provide a nonstandard environment to the guest, which could break
> > migration.
> > 
> > Signed-off-by: David Gibson <address@hidden>
> > 
> > # Conflicts:
> > #   hw/ppc/spapr_caps.c
> > ---
> >  hw/ppc/spapr.c         | 13 +++++++------
> >  hw/ppc/spapr_caps.c    | 41 ++++++++++++++++++++++++++++++++++++-----
> >  include/hw/ppc/spapr.h |  3 +++
> >  3 files changed, 46 insertions(+), 11 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index a921feeb03..0487d67894 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -253,7 +253,8 @@ static int spapr_fixup_cpu_numa_dt(void *fdt, int 
> > offset, PowerPCCPU *cpu)
> >  }
> >  
> >  /* Populate the "ibm,pa-features" property */
> > -static void spapr_populate_pa_features(PowerPCCPU *cpu, void *fdt, int 
> > offset,
> > +static void spapr_populate_pa_features(sPAPRMachineState *spapr, 
> > PowerPCCPU *cpu,
> > +                                       void *fdt, int offset,
> >                                         bool legacy_guest)
> >  {
> >      CPUPPCState *env = &cpu->env;
> > @@ -318,7 +319,7 @@ static void spapr_populate_pa_features(PowerPCCPU *cpu, 
> > void *fdt, int offset,
> >           */
> >          pa_features[3] |= 0x20;
> >      }
> > -    if (kvmppc_has_cap_htm() && pa_size > 24) {
> > +    if (spapr_has_cap(spapr, SPAPR_CAP_HTM) && pa_size > 24) {
> >          pa_features[24] |= 0x80;    /* Transactional memory support */
> >      }
> >      if (legacy_guest && pa_size > 40) {
> > @@ -384,8 +385,8 @@ static int spapr_fixup_cpu_dt(void *fdt, 
> > sPAPRMachineState *spapr)
> >              return ret;
> >          }
> >  
> > -        spapr_populate_pa_features(cpu, fdt, offset,
> > -                                         
> > spapr->cas_legacy_guest_workaround);
> > +        spapr_populate_pa_features(spapr, cpu, fdt, offset,
> > +                                   spapr->cas_legacy_guest_workaround);
> >      }
> >      return ret;
> >  }
> > @@ -579,7 +580,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void 
> > *fdt, int offset,
> >                            page_sizes_prop, page_sizes_prop_size)));
> >      }
> >  
> > -    spapr_populate_pa_features(cpu, fdt, offset, false);
> > +    spapr_populate_pa_features(spapr, cpu, fdt, offset, false);
> >  
> >      _FDT((fdt_setprop_cell(fdt, offset, "ibm,chip-id",
> >                             cs->cpu_index / vcpus_per_socket)));
> > @@ -3823,7 +3824,7 @@ static void spapr_machine_class_init(ObjectClass *oc, 
> > void *data)
> >       */
> >      mc->numa_mem_align_shift = 28;
> >  
> > -    smc->default_caps = spapr_caps(0);
> > +    smc->default_caps = spapr_caps(SPAPR_CAP_HTM);
> >      spapr_capability_properties(smc, &error_abort);
> >  }
> >  
> > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> > index f1721cc68f..5afb4d9d5f 100644
> > --- a/hw/ppc/spapr_caps.c
> > +++ b/hw/ppc/spapr_caps.c
> > @@ -24,6 +24,10 @@
> >  #include "qemu/osdep.h"
> >  #include "qapi/error.h"
> >  #include "qapi/visitor.h"
> > +#include "sysemu/hw_accel.h"
> > +#include "target/ppc/cpu.h"
> > +#include "cpu-models.h"
> > +#include "kvm_ppc.h"
> >  
> >  #include "hw/ppc/spapr.h"
> >  
> > @@ -34,30 +38,57 @@ typedef struct sPAPRCapabilityInfo {
> >  } sPAPRCapabilityInfo;
> >  
> >  static sPAPRCapabilityInfo capability_table[] = {
> > +    {
> > +        .name = "htm",
> > +        .description = "Allow Hardware Transactional Memory (HTM)",
> > +        .bit = SPAPR_CAP_HTM,
> > +    },
> >  };
> >  
> >  static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState *spapr, 
> > CPUState *cs)
> >  {
> >      sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> > +    PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> >      sPAPRCapabilities caps;
> >  
> >      caps = smc->default_caps;
> >  
> > -    /* TODO: clamp according to cpu model */
> > +    if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_2_07,
> > +                          0, spapr->max_compat_pvr)) {
> > +        caps.mask &= ~SPAPR_CAP_HTM;
> > +    }
> >  
> >      return caps;
> >  }
> >  
> >  static void spapr_allow_caps(sPAPRMachineState *spapr, Error **errp)
> >  {
> > -    /* TODO: make sure all requested caps are allowed with the current
> > -     * accelerator, cpu etc. */
> > +    Error *err = NULL;
> > +
> > +    if (spapr_has_cap(spapr, SPAPR_CAP_HTM)) {
> > +        if (tcg_enabled()) {
> > +            error_setg(&err,
> > +"No Transactional Memory support in TCG, try cap-htm=off"
> > +                );
> > +            goto out;
> > +        } else if (kvm_enabled() && !kvmppc_has_cap_htm()) {
> > +            error_setg(&err,
> > +"KVM implementation does not support Transactional Memory, try cap-htm=off"
> > +                );
> > +        }
> > +    }
> > +
> > +out:
> > +    if (err) {
> > +        error_propagate(errp, err);
> > +    }
> >  }
> >  
> >  static void spapr_enforce_caps(sPAPRMachineState *spapr, Error **errp)
> >  {
> > -    /* TODO: to the extent possible, prevent the guest from accessing
> > -     * features controlled by disabled caps */
> > +    if (!spapr_has_cap(spapr, SPAPR_CAP_HTM)) {
> > +        /* TODO: Tell KVM not to allow HTM instructions */
> 
> 
> Are the capabilities expected to be enabled by default in the KVM? There is
> no such "TODO" in spapr_allow_caps while it should probably be.

In short, yes.  At present this is a "read only" cap in KVM.  It is
always enabled when the host can support HTM and can't be changed.  We
could - and probably should - add a way to disable the capability, but
we couldn't change it to being off by default without breaking
compatibility.

> I would rather implement spapr_get_supported_caps() which would tell full
> list of what the accelerator can do for the spapr machine; and
> spapr_set_caps() which would compare supported caps what we got from
> defaults and the command line and then enable requested or disable
> forbidden caps, explicitly.

Hrm.  I don't really see how this helps.  We'd still need cap-by-cap
logic to actually implement the enabling and/or disabling: the
mechanism need not be the same for all caps.  And in fact we'd still
need the cap by cap logic to implement spapr_get_supported_caps().  If
we have to do that, we might as well check for failures at the same
time as doing the actual enabling and disabling,

> Right now both spapr_allow_caps() and
> spapr_enforce_caps() both have to check for spapr_has_cap() and (at least
> what TODO suggests) for kvm_enabled() and do something to KVM in
> this regard.

Why is that bad?


> 
> 
> 
> 
> > +    }
> >  }
> >  
> >  void spapr_caps_reset(sPAPRMachineState *spapr)
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index fffe10ee72..4215b113f4 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -54,6 +54,9 @@ typedef enum {
> >   * Capabilities
> >   */
> >  
> > +/* Hardware Transactional Memory */
> > +#define SPAPR_CAP_HTM               0x0000000000000001ULL
> > +
> >  typedef struct sPAPRCapabilities sPAPRCapabilities;
> >  struct sPAPRCapabilities {
> >      uint64_t mask;
> > 
> 
> 

-- 
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]