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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f17eb0da-c77e-cc95-b88c-6947a68f5f9a@intel.com>
Date:   Fri, 30 Jul 2021 11:41:51 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     "Chang S. Bae" <chang.seok.bae@...el.com>, bp@...e.de,
        luto@...nel.org, tglx@...utronix.de, mingo@...nel.org,
        x86@...nel.org
Cc:     len.brown@...el.com, thiago.macieira@...el.com,
        jing2.liu@...el.com, ravi.v.shankar@...el.com,
        linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v9 25/26] intel_idle/amx: Add SPR support with XTILEDATA
 capability

>  #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.