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


> 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? It just looks nicer to have a single place which enables or disables a
capability. Dunno...


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



>>
>>
>>
>>> +    }
>>>  }
>>>  
>>>  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;
>>>
>>
>>
> 


-- 
Alexey

signature.asc
Description: OpenPGP digital signature