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

[Michael Roth]

Quoting David Gibson (2017-12-11 01:08:03)
> 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 */

pa_size used to disable HTM for p7 and lower, but now that we set
capabilities in spapr_caps_reset() with machine type in consideration it
doesn't seem like this check is needed anymore, or should at least be
changed to an assert().

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

spapr_caps_reset() calls this with cs=first_cpu already. I think it's
simpler to stick with this and just drop the cs arg altogether though,
since the idea of default_caps_with_cpu() getting called with other
CPUStates leads one to consider whether additional cpus could change the
capability set later (e.g. hotplug), when really first_cpu already does
the clamping.

>      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 */
> +    }
>  }
> 
>  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;
> -- 
> 2.14.3
>