Messages in this thread | | | Subject | Re: [PATCH v9 25/26] intel_idle/amx: Add SPR support with XTILEDATA capability | From | Dave Hansen <> | Date | Fri, 30 Jul 2021 11:41:51 -0700 |
| |
> #endif /* _ASM_X86_SPECIAL_INSNS_H */ > diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c > index e6c543b5ee1d..fe1ba26cc797 100644 > --- a/drivers/idle/intel_idle.c > +++ b/drivers/idle/intel_idle.c > @@ -54,6 +54,8 @@ > #include <asm/intel-family.h> > #include <asm/mwait.h> > #include <asm/msr.h> > +#include <asm/fpu/internal.h> > +#include <asm/special_insns.h> > > #define INTEL_IDLE_VERSION "0.5.1" > > @@ -155,6 +157,55 @@ static __cpuidle int intel_idle_s2idle(struct cpuidle_device *dev, > return 0; > } > > +/** > + * idle_tile - Initialize TILE registers in INIT-state > + * > + * Leaving state in the dirty TILE registers may prevent the processor from > + * entering lower-power idle states. Use TILERELEASE to initialize the > + * state. Destroying fpregs state is safe after the fpstate update. > + */ > +static inline void idle_tile(void) > +{ > + if (boot_cpu_has(X86_FEATURE_XGETBV1) && (xgetbv(1) & XFEATURE_MASK_XTILE)) { > + tile_release(); > + fpregs_deactivate(¤t->thread.fpu); > + } > +}
This isn't obviously safe code. There's a window in there when we have bogus, destroyed FPU register state but where we might be rescheduled.
I would assume that preempt is off *somewhere* in this, but it would be nice to make sure of that, or at least mention the requirement for it to be off before this code is safe.
I'm also not sure TILERELEASE is *technically* what you want here. xgetbv(1) tells you whether a feature is being tracked by the processor as being in its init state. tile_release() gets a feature into its init state, but does not guarantee that the processor will *track* it as being in the init state.
TILERELEASE is not documented to have an effect on XINUSE (init tracking).
XRSTOR, on the other hand, is at least documented to affect XINUSE.
It sounds like we either need a documentation update, or a clear explanation why TILERELEASE is being used over XRSTOR.
| |