Re: [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability

[David Gibson]

On Tue, Dec 12, 2017 at 03:06:33PM +1100, Alexey Kardashevskiy wrote:
> On 12/12/17 14:40, David Gibson wrote:
> > On Tue, Dec 12, 2017 at 02:28:33PM +1100, Alexey Kardashevskiy wrote:
> >> On 12/12/17 11:20, David Gibson wrote:
> >>> 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(). 
> >>
> >>
> >> I'd think AccelClass is the place to implement
> >> is_cap_supported()/enable_cap()/disable_cap(), and spapr would not have to
> >> go via these kvm_enabled()/tcg_enabled() but it is probably out of scope.
> > 
> > Not really.  The 3 caps so far are related to the cpu and/or
> > accelerator, but even there they only really make sense in a context
> > where the hypervisor can block access to features.  They wouldn't
> > really make sense for powernv (or, say, ppce500) where the guest OS
> > controls the HFSCR or equivalent ways of controlling feature access.
> 
> ok, fair enough.
> 
> 
> >>> 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?
> >>
> >>
> >> If let's say we have a capability which is off by default but I want to
> >> enable it anyway, then should both _allow() and _enforce() try to enable
> >> it?
> > 
> > No, if the feature is on, _allow() makes sure it can be implemented
> > with the cpu/accel, _enforce() does nothing.
> > 
> > If the feature os off, _allow() does nothing, _enforce makes a best
> > effort to prevent it from working.
> 
> Ah. _allow() can only enable, _enforce() can only disable. ok.
> 
> This is what I meant elsewhere - the "enforce" word sounds to me (one bad
> engligh speaker :) ) more like "enable" (forcefully) than "disable".
> "forbid" seems better but I do not insist.

Right, but I wanted to avoid "forbid" because I was already using it
for forbidden_caps which has different semantics.  forbidden_caps is
those caps the _user_ says shouldn't be there and are therefore
excluded from the "effective caps" regardless of other defaults.
"enforce" is about taking the effective caps and matching them to
actual hardware capabilities.  i.e. they're operating at different
levels.  Maybe this makes it clearer:

USER                            QEMU STATE                    HARDWARE

                  caps_reset()                 allow/enforce
forced_caps       ----------->  effective_caps ------------>  HFSCR
forbidden_caps         ^                             ^        KVM caps
                       |                             |        ...
         (defaults) ---/                             |
                                   (hw abilities) ---/

> >> It just looks nicer to have a single place which enables or disables a
> >> capability. Dunno...
> > 
> > I could put both the allowing and enforcing in one function, say
> > spapr_caps_apply().  But then I'd need an alternative mechanism for
> > reporting fatal vs. non-fatal errors.  Maybe worth it, I'll have a
> > look.
> 
> And now it is assumed that all failures in _allow() are non-fatal?

No, failures in _allow() are always fatal.  Failures in _enforce() aren't.

> >> btw you do not need to check for kvm_enabled() when checking for
> >> kvmppc_has_cap_htm() as the latter should return non-initialized cap_htm
> >> (i.e. false).
> > 
> > I know, but that was much more subtle than I liked so I changed it
> > deliberately.
> 
> 
> Well, it has "kvmppc" in the name, not that subtle imho ;) Minor thing though.

The kvmppc in the name is exactly why I didn't like it: I don't think
it's at all obvious that kvmppc_whatever() has a meaningful value when
KVM is not present.

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