[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [RFC for-2.12 7/8] spapr: Handle VMX/VSX presence as an s
From: |
David Gibson |
Subject: |
Re: [Qemu-ppc] [RFC for-2.12 7/8] spapr: Handle VMX/VSX presence as an spapr capability flag |
Date: |
Wed, 13 Dec 2017 12:51:37 +1100 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
On Tue, Dec 12, 2017 at 01:54:36PM +0100, Greg Kurz wrote:
> On Mon, 11 Dec 2017 18:08:07 +1100
> David Gibson <address@hidden> wrote:
>
> > We currently have some conditionals in the spapr device tree code to decide
> > whether or not to advertise the availability of the VMX (aka Altivec) and
> > VSX vector extensions to the guest, based on whether the guest cpu has
> > those features.
> >
> > This can lead to confusion and subtle failures on migration, since it makes
> > a guest visible change based only on host capabilities. We now have a
> > better mechanism for this, in spapr capabilities flags, which explicitly
> > depend on user options rather than host capabilities.
> >
> > Rework the advertisement of VSX and VMX based on a new VSX capability. We
> > no longer bother with a conditional for VMX support, because every CPU
> > that's ever been supported by the pseries machine type supports VMX.
> >
> > NOTE: Some userspace distributions (e.g. RHEL7.4) already rely on
> > availability of VSX in libc, so using cap-vsx=off may lead to a fatal
> > SIGILL in init.
> >
> > Signed-off-by: David Gibson <address@hidden>
> > ---
>
> Just one nit, see below, but anyway:
>
> Reviewed-by: Greg Kurz <address@hidden>
>
>
> > hw/ppc/spapr.c | 18 ++++++++++--------
> > hw/ppc/spapr_caps.c | 24 ++++++++++++++++++++++++
> > include/hw/ppc/spapr.h | 3 +++
> > 3 files changed, 37 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index e206825ed9..6b37511e8f 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -556,14 +556,16 @@ static void spapr_populate_cpu_dt(CPUState *cs, void
> > *fdt, int offset,
> > segs, sizeof(segs))));
> > }
> >
> > - /* Advertise VMX/VSX (vector extensions) if available
> > - * 0 / no property == no vector extensions
> > + /* Advertise VSX (vector extensions) if available
> > * 1 == VMX / Altivec available
> > - * 2 == VSX available */
> > - if (env->insns_flags & PPC_ALTIVEC) {
> > - uint32_t vmx = (env->insns_flags2 & PPC2_VSX) ? 2 : 1;
> > -
> > - _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", vmx)));
> > + * 2 == VSX available
> > + *
> > + * Only CPUs for which we create core types in spapr_cpu_core.c
> > + * are possible, and all of those have VMX */
> > + if (spapr_has_cap(spapr, SPAPR_CAP_VSX)) {
> > + _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", 2)));
>
> Maybe a good opportunity to drop the extra parens since the _FDT() macro
> already adds them around its argument.
>
> #define _FDT(exp) \
> do { \
> int ret = (exp); \
Uh, yeah, that won't work. The extra parens are so that the macro
considers the whole string inside as a single argument, even if it
includes ',' characters, which it usually will.
>
> > + } else {
> > + _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", 1)));
> > }
> >
> > /* Advertise DFP (Decimal Floating Point) if available
> > @@ -3829,7 +3831,7 @@ static void spapr_machine_class_init(ObjectClass *oc,
> > void *data)
> > */
> > mc->numa_mem_align_shift = 28;
> >
> > - smc->default_caps = spapr_caps(SPAPR_CAP_HTM);
> > + smc->default_caps = spapr_caps(SPAPR_CAP_HTM | SPAPR_CAP_VSX);
> > spapr_capability_properties(smc, &error_abort);
> > }
> >
> > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> > index 6bba04d60e..b19a9f4417 100644
> > --- a/hw/ppc/spapr_caps.c
> > +++ b/hw/ppc/spapr_caps.c
> > @@ -44,6 +44,11 @@ static sPAPRCapabilityInfo capability_table[] = {
> > .description = "Allow Hardware Transactional Memory (HTM)",
> > .bit = SPAPR_CAP_HTM,
> > },
> > + {
> > + .name = "vsx",
> > + .description = "Allow Vector Scalar Extensions (VSX)",
> > + .bit = SPAPR_CAP_VSX,
> > + },
> > };
> >
> > static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState *spapr,
> > CPUState *cs)
> > @@ -59,6 +64,11 @@ static sPAPRCapabilities
> > default_caps_with_cpu(sPAPRMachineState *spapr, CPUStat
> > caps.mask &= ~SPAPR_CAP_HTM;
> > }
> >
> > + if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_2_06,
> > + 0, spapr->max_compat_pvr)) {
> > + caps.mask &= ~SPAPR_CAP_VSX;
> > + }
> > +
> > return caps;
> > }
> >
> > @@ -149,6 +159,8 @@ const VMStateDescription vmstate_spapr_caps = {
> >
> > static void spapr_allow_caps(sPAPRMachineState *spapr, Error **errp)
> > {
> > + PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> > + CPUPPCState *env = &cpu->env;
> > Error *err = NULL;
> >
> > if (spapr_has_cap(spapr, SPAPR_CAP_HTM)) {
> > @@ -164,6 +176,14 @@ static void spapr_allow_caps(sPAPRMachineState *spapr,
> > Error **errp)
> > }
> > }
> >
> > + g_assert(env->insns_flags & PPC_ALTIVEC);
> > + if (spapr_has_cap(spapr, SPAPR_CAP_VSX)) {
> > + if (!(env->insns_flags2 & PPC2_VSX)) {
> > + error_setg(&err, "VSX support not available, try cap-vsx=off");
> > + goto out;
> > + }
> > + }
> > +
> > out:
> > if (err) {
> > error_propagate(errp, err);
> > @@ -175,6 +195,10 @@ static void spapr_enforce_caps(sPAPRMachineState
> > *spapr, Error **errp)
> > if (!spapr_has_cap(spapr, SPAPR_CAP_HTM)) {
> > /* TODO: Tell KVM not to allow HTM instructions */
> > }
> > +
> > + if (!spapr_has_cap(spapr, SPAPR_CAP_VSX)) {
> > + /* TODO: Any way to disable VSX? */
> > + }
> > }
> >
> > void spapr_caps_reset(sPAPRMachineState *spapr)
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index 9b80fe72ed..4c173cb40c 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -59,6 +59,9 @@ typedef enum {
> > /* Hardware Transactional Memory */
> > #define SPAPR_CAP_HTM 0x0000000000000001ULL
> >
> > +/* Vector Scalar Extensions */
> > +#define SPAPR_CAP_VSX 0x0000000000000002ULL
> > +
> > 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
- [Qemu-ppc] [RFC for-2.12 1/8] spapr: Rename machine init functions for clarity, (continued)
[Qemu-ppc] [RFC for-2.12 6/8] target/ppc: Clean up probing of VMX, VSX and DFP availability on KVM, David Gibson, 2017/12/11
[Qemu-ppc] [RFC for-2.12 4/8] spapr: Validate capabilities on migration, David Gibson, 2017/12/11