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

[Alexey Kardashevskiy]

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.



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


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


-- 
Alexey

signature.asc
Description: OpenPGP digital signature