Re: [PATCH v9 25/26] intel_idle/amx: Add SPR support with XTILEDATA capability

Dave Hansen
2021-07-30T18:41:57Z

> #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(&current->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.