[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 12:28:49 +1100 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
On Mon, Dec 11, 2017 at 06:06:03PM -0600, Michael Roth wrote:
> 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().
So, technically I think you're right that it's impossible to hit this,
but the reasons are kind of subtle and complex so I'd prefer to leave
the check in.
The user _can_ force on the HTM feature even with a POWER8, but in
practice that will be caught before here in spapr_allow_caps(). I
think relying on that's a bit fragile though: someone could for
example hack up a TCG model that looks like a POWER7 but supports
HTM. If you did so that would cause this code to overrun the
pa_features buffer.
> > }
> > 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.
Oops, this is left over from an earlier version.
> 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.
I've gone the other way, using the parameter properly. You have a
point here, but I don't love using the global first_cpu at all, and if
we have to I prefer to move global references to the highest place in
the call stack that it's practical to do so.
>
> > 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;
>
--
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
- Re: [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability, (continued)
- Re: [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability, David Gibson, 2017/12/11
- Re: [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability, Alexey Kardashevskiy, 2017/12/11
- Re: [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability, David Gibson, 2017/12/11
- Re: [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability, Alexey Kardashevskiy, 2017/12/11
- Re: [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability, David Gibson, 2017/12/12
- Re: [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability, Alexey Kardashevskiy, 2017/12/12
- Re: [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability, David Gibson, 2017/12/12
Re: [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability, Daniel Henrique Barboza, 2017/12/11
Re: [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability, Michael Roth, 2017/12/11
- Re: [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability,
David Gibson <=
Re: [Qemu-ppc] [RFC for-2.12 3/8] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability, Suraj Jitindar Singh, 2017/12/11
[Qemu-ppc] [RFC for-2.12 2/8] spapr: Capabilities infrastructure, David Gibson, 2017/12/11