[PATCH v4 22/22] x86/fpu/xstate: Introduce boot-parameters to control state component support

[PATCH v4 22/22] x86/fpu/xstate: Introduce boot-parameters to control state component support

[Chang S. Bae]

"xstate.disable=0x60000" will disable AMX on a system that has AMX compiled
into XFEATURE_MASK_USER_ENABLED.

"xstate.enable=0x60000" will enable AMX on a system that does NOT have AMX
compiled into XFEATURE_MASK_USER_ENABLED (assuming the kernel is new enough
to support this feature).

Rename XFEATURE_MASK_USER_SUPPORTED to XFEATURE_MASK_USER_ENABLED to be
aligned with the new parameters.

While this cmdline is currently enabled only for AMX, it is intended to be
easily enabled to be useful for future XSAVE-enabled features.

Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Reviewed-by: Len Brown <len.brown@intel.com>
Cc: x86@kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
Changes from v3:
* Fixed a few typos. (Randy Dunlap)

Changes from v2:
* Changed the kernel tainted when any unknown state is enabled. (Andy
  Lutomirski)
* Simplified the cmdline handling.
* Edited the changelog.

Changes from v1:
* Renamed the user state mask define (Andy Lutomirski and Dave Hansen)
* Changed the error message (Dave Hansen)
* Fixed xfeatures_mask_user()
* Rebased the upstream kernel (5.10) -- revived the param parse function
---
 .../admin-guide/kernel-parameters.txt         | 15 +++++
 arch/x86/include/asm/fpu/types.h              |  6 ++
 arch/x86/include/asm/fpu/xstate.h             | 24 +++----
 arch/x86/kernel/fpu/init.c                    | 65 +++++++++++++++++--
 4 files changed, 93 insertions(+), 17 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index a10b545c2070..ec79f63979a4 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -6014,6 +6014,21 @@
 			which allow the hypervisor to 'idle' the guest on lock
 			contention.
 
+	xstate.enable=	[X86-64]
+	xstate.disable=	[X86-64]
+			The kernel is compiled with a default xstate bitmask --
+			enabling it to use the XSAVE hardware to efficiently
+			save and restore thread states on context switch.
+			xstate.enable allows adding to that default mask at
+			boot-time without recompiling the kernel just to support
+			the new thread state. (Note that the kernel will ignore
+			any bits in the mask that do not correspond to features
+			that are actually available in CPUID.)  xstate.disable
+			allows clearing bits in the default mask, forcing the
+			kernel to forget that it supports the specified thread
+			state. When a bit set for both, the kernel takes
+			xstate.disable as a priority.
+
 	xirc2ps_cs=	[NET,PCMCIA]
 			Format:
 			<irq>,<irq_mask>,<io>,<full_duplex>,<do_sound>,<lockup_hack>[,<irq2>[,<irq3>[,<irq4>]]]
diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h
index 2f297aa85d8f..967d38cc7eb1 100644
--- a/arch/x86/include/asm/fpu/types.h
+++ b/arch/x86/include/asm/fpu/types.h
@@ -149,6 +149,12 @@ enum xfeature {
 #define XFEATURE_MASK_XTILE		(XFEATURE_MASK_XTILE_DATA \
 					 | XFEATURE_MASK_XTILE_CFG)
 
+#define XFEATURE_REGION_MASK(max_bit, min_bit) \
+	((BIT_ULL((max_bit) - (min_bit) + 1) - 1) << (min_bit))
+
+#define XFEATURE_MASK_CONFIGURABLE \
+	XFEATURE_REGION_MASK(XFEATURE_XTILE_DATA, XFEATURE_XTILE_CFG)
+
 #define FIRST_EXTENDED_XFEATURE	XFEATURE_YMM
 
 struct reg_128_bit {
diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index 9e5c28f3beaa..1e64afea9f68 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -25,17 +25,17 @@
 
 #define XSAVE_ALIGNMENT     64
 
-/* All currently supported user features */
-#define XFEATURE_MASK_USER_SUPPORTED (XFEATURE_MASK_FP | \
-				      XFEATURE_MASK_SSE | \
-				      XFEATURE_MASK_YMM | \
-				      XFEATURE_MASK_OPMASK | \
-				      XFEATURE_MASK_ZMM_Hi256 | \
-				      XFEATURE_MASK_Hi16_ZMM	 | \
-				      XFEATURE_MASK_PKRU | \
-				      XFEATURE_MASK_BNDREGS | \
-				      XFEATURE_MASK_BNDCSR | \
-				      XFEATURE_MASK_XTILE)
+/* All currently enabled user features */
+#define XFEATURE_MASK_USER_ENABLED (XFEATURE_MASK_FP | \
+				    XFEATURE_MASK_SSE | \
+				    XFEATURE_MASK_YMM | \
+				    XFEATURE_MASK_OPMASK | \
+				    XFEATURE_MASK_ZMM_Hi256 | \
+				    XFEATURE_MASK_Hi16_ZMM	 | \
+				    XFEATURE_MASK_PKRU | \
+				    XFEATURE_MASK_BNDREGS | \
+				    XFEATURE_MASK_BNDCSR | \
+				    XFEATURE_MASK_XTILE)
 
 /* All currently supported supervisor features */
 #define XFEATURE_MASK_SUPERVISOR_SUPPORTED (XFEATURE_MASK_PASID)
@@ -87,7 +87,7 @@ static inline u64 xfeatures_mask_supervisor(void)
 
 static inline u64 xfeatures_mask_user(void)
 {
-	return xfeatures_mask_all & XFEATURE_MASK_USER_SUPPORTED;
+	return xfeatures_mask_all & ~(XFEATURE_MASK_SUPERVISOR_ALL);
 }
 
 static inline u64 xfeatures_mask_supervisor_dynamic(void)
diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index 046889f31037..0166d3eb9916 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -5,6 +5,7 @@
 #include <asm/fpu/internal.h>
 #include <asm/tlbflush.h>
 #include <asm/setup.h>
+#include <asm/cmdline.h>
 
 #include <linux/sched.h>
 #include <linux/sched/task.h>
@@ -215,14 +216,45 @@ static void __init fpu__init_system_xstate_size_legacy(void)
 /*
  * Find supported xfeatures based on cpu features and command-line input.
  * This must be called after fpu__init_parse_early_param() is called and
- * xfeatures_mask is enumerated.
+ * xfeatures_mask_all is enumerated.
  */
+
+static u64 xstate_enable;
+static u64 xstate_disable;
+
 u64 __init fpu__get_supported_xfeatures_mask(void)
 {
-	u64 mask = XFEATURE_MASK_USER_SUPPORTED | XFEATURE_MASK_SUPERVISOR_SUPPORTED;
-
-	if (!IS_ENABLED(CONFIG_X86_64))
-		mask &= ~(XFEATURE_MASK_XTILE);
+	u64 mask = XFEATURE_MASK_USER_ENABLED | XFEATURE_MASK_SUPERVISOR_SUPPORTED;
+
+	if (!IS_ENABLED(CONFIG_X86_64)) {
+		mask  &= ~(XFEATURE_MASK_XTILE);
+	} else if (xstate_enable || xstate_disable) {
+		u64 custom = mask;
+		u64 unknown;
+
+		custom |= xstate_enable;
+		custom &= ~xstate_disable;
+
+		unknown = custom & ~mask;
+		if (unknown) {
+			/*
+			 * User should fully understand the result of using undocumented
+			 * xstate component.
+			 */
+			add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
+			pr_warn("x86/fpu: Attempt to enable unknown xstate features 0x%llx\n",
+				unknown);
+			WARN_ON_FPU(1);
+		}
+
+		if ((custom & XFEATURE_MASK_XTILE) != XFEATURE_MASK_XTILE) {
+			pr_warn("x86/fpu: Error in xstate.disable. Additionally disabling 0x%x components.\n",
+				XFEATURE_MASK_XTILE);
+			custom &= ~(XFEATURE_MASK_XTILE);
+		}
+
+		mask = custom;
+	}
 
 	return mask;
 }
@@ -236,12 +268,35 @@ static void __init fpu__init_system_ctx_switch(void)
 	on_boot_cpu = 0;
 }
 
+/*
+ * Longest parameter of 'xstate.enable=' is 22 octal number characters with '0' prefix and
+ * an extra '\0' for termination.
+ */
+#define MAX_XSTATE_MASK_CHARS	24
+/*
+ * We parse xstate parameters early because fpu__init_system() is executed before
+ * parse_early_param().
+ */
+static void __init fpu__init_parse_early_param(void)
+{
+	char arg[MAX_XSTATE_MASK_CHARS];
+
+	if (cmdline_find_option(boot_command_line, "xstate.enable", arg, sizeof(arg)) &&
+	    !kstrtoull(arg, 0, &xstate_enable))
+		xstate_enable &= XFEATURE_MASK_CONFIGURABLE;
+
+	if (cmdline_find_option(boot_command_line, "xstate.disable", arg, sizeof(arg)) &&
+	    !kstrtoull(arg, 0, &xstate_disable))
+		xstate_disable &= XFEATURE_MASK_CONFIGURABLE;
+}
+
 /*
  * Called on the boot CPU once per system bootup, to set up the initial
  * FPU state that is later cloned into all processes:
  */
 void __init fpu__init_system(struct cpuinfo_x86 *c)
 {
+	fpu__init_parse_early_param();
 	fpu__init_system_early_generic(c);
 
 	/*
-- 
2.17.1

Re: [PATCH v4 22/22] x86/fpu/xstate: Introduce boot-parameters to control state component support

[Randy Dunlap]

On 2/21/21 10:56 AM, Chang S. Bae wrote:
> "xstate.disable=0x60000" will disable AMX on a system that has AMX compiled
> into XFEATURE_MASK_USER_ENABLED.
> 
> "xstate.enable=0x60000" will enable AMX on a system that does NOT have AMX
> compiled into XFEATURE_MASK_USER_ENABLED (assuming the kernel is new enough
> to support this feature).
> 
> Rename XFEATURE_MASK_USER_SUPPORTED to XFEATURE_MASK_USER_ENABLED to be
> aligned with the new parameters.
> 
> While this cmdline is currently enabled only for AMX, it is intended to be
> easily enabled to be useful for future XSAVE-enabled features.
> 

Hi,
Can we tell people (in this Doc file) where to look up the values that can be
used in xstate.enable and xstate.disable?

thanks.

> Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
> Reviewed-by: Len Brown <len.brown@intel.com>
> Cc: x86@kernel.org
> Cc: linux-doc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
> Changes from v3:
> * Fixed a few typos. (Randy Dunlap)
> 
> Changes from v2:
> * Changed the kernel tainted when any unknown state is enabled. (Andy
>   Lutomirski)
> * Simplified the cmdline handling.
> * Edited the changelog.
> 
> Changes from v1:
> * Renamed the user state mask define (Andy Lutomirski and Dave Hansen)
> * Changed the error message (Dave Hansen)
> * Fixed xfeatures_mask_user()
> * Rebased the upstream kernel (5.10) -- revived the param parse function
> ---
>  .../admin-guide/kernel-parameters.txt         | 15 +++++
>  arch/x86/include/asm/fpu/types.h              |  6 ++
>  arch/x86/include/asm/fpu/xstate.h             | 24 +++----
>  arch/x86/kernel/fpu/init.c                    | 65 +++++++++++++++++--
>  4 files changed, 93 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index a10b545c2070..ec79f63979a4 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -6014,6 +6014,21 @@
>  			which allow the hypervisor to 'idle' the guest on lock
>  			contention.
>  
> +	xstate.enable=	[X86-64]
> +	xstate.disable=	[X86-64]
> +			The kernel is compiled with a default xstate bitmask --
> +			enabling it to use the XSAVE hardware to efficiently
> +			save and restore thread states on context switch.
> +			xstate.enable allows adding to that default mask at
> +			boot-time without recompiling the kernel just to support
> +			the new thread state. (Note that the kernel will ignore
> +			any bits in the mask that do not correspond to features
> +			that are actually available in CPUID.)  xstate.disable
> +			allows clearing bits in the default mask, forcing the
> +			kernel to forget that it supports the specified thread
> +			state. When a bit set for both, the kernel takes
> +			xstate.disable as a priority.
> +
>  	xirc2ps_cs=	[NET,PCMCIA]
>  			Format:
>  			<irq>,<irq_mask>,<io>,<full_duplex>,<do_sound>,<lockup_hack>[,<irq2>[,<irq3>[,<irq4>]]]



-- 
~Randy

Re: [PATCH v4 22/22] x86/fpu/xstate: Introduce boot-parameters to control state component support

[Bae, Chang Seok]

On Feb 21, 2021, at 11:30, Randy Dunlap <rdunlap@infradead.org> wrote:
> Can we tell people (in this Doc file) where to look up the values that can be
> used in xstate.enable and xstate.disable?

Perhaps add something like this with the change below:
    “See comment before function fpu__init_parse_early_param() in
     arch/x86/kernel/fpu/init.c."

/*
 * The kernel parameter "xstate.enable='mask'" and "xstate.disable='mask'" have a
 * mask value in a subset of XFEATURE_MASK_CONFIGURABLE.
 *
 * The longest parameter is 22 octal number characters with '0' prefix and an extra
 * '\0' for termination.
 */
#define MAX_XSTATE_MASK_CHARS   24

/**
 * fpu__init_parse_early_param() - parse the xstate kernel parameters
 *
 * Parse them early because fpu__init_system() is executed before
 * parse_early_param().
 */
static void __init fpu__init_parse_early_param(void)

Thanks,
Chang

Re: [PATCH v4 22/22] x86/fpu/xstate: Introduce boot-parameters to control state component support

[Randy Dunlap]

On 2/21/21 12:10 PM, Bae, Chang Seok wrote:
> On Feb 21, 2021, at 11:30, Randy Dunlap <rdunlap@infradead.org> wrote:
>> Can we tell people (in this Doc file) where to look up the values that can be
>> used in xstate.enable and xstate.disable?
> 
> Perhaps add something like this with the change below:
>     “See comment before function fpu__init_parse_early_param() in
>      arch/x86/kernel/fpu/init.c."

Hi,

I was thinking more along the lines of where can I find the value
0x60000 or BIT(22) or BIT(19), for example and see what they mean,
even though it will likely be some abbreviation.


> /*
>  * The kernel parameter "xstate.enable='mask'" and "xstate.disable='mask'" have a
>  * mask value in a subset of XFEATURE_MASK_CONFIGURABLE.
>  *
>  * The longest parameter is 22 octal number characters with '0' prefix and an extra
>  * '\0' for termination.
>  */
> #define MAX_XSTATE_MASK_CHARS   24
> 
> /**
>  * fpu__init_parse_early_param() - parse the xstate kernel parameters
>  *
>  * Parse them early because fpu__init_system() is executed before
>  * parse_early_param().
>  */
> static void __init fpu__init_parse_early_param(void)

thanks.
-- 
~Randy

Re: [PATCH v4 22/22] x86/fpu/xstate: Introduce boot-parameters to control state component support

[Thomas Gleixner]

On Sun, Feb 21 2021 at 10:56, Chang S. Bae wrote:
> "xstate.disable=0x60000" will disable AMX on a system that has AMX compiled
> into XFEATURE_MASK_USER_ENABLED.
>
> "xstate.enable=0x60000" will enable AMX on a system that does NOT have AMX
> compiled into XFEATURE_MASK_USER_ENABLED (assuming the kernel is new enough
> to support this feature).

This makes no sense at all.

> Rename XFEATURE_MASK_USER_SUPPORTED to XFEATURE_MASK_USER_ENABLED to be
> aligned with the new parameters.
>
> While this cmdline is currently enabled only for AMX, it is intended to be
> easily enabled to be useful for future XSAVE-enabled features.

I have a hard time to map this changelog to the actual code.

> +/* All currently enabled user features */
> +#define XFEATURE_MASK_USER_ENABLED (XFEATURE_MASK_FP | \
> +				    XFEATURE_MASK_SSE | \
> +				    XFEATURE_MASK_YMM | \
> +				    XFEATURE_MASK_OPMASK | \
> +				    XFEATURE_MASK_ZMM_Hi256 | \
> +				    XFEATURE_MASK_Hi16_ZMM	 | \
> +				    XFEATURE_MASK_PKRU | \
> +				    XFEATURE_MASK_BNDREGS | \
> +				    XFEATURE_MASK_BNDCSR | \
> +				    XFEATURE_MASK_XTILE)
  
> +
> +static u64 xstate_enable;
> +static u64 xstate_disable;

This needs to be kept around forever because it's used where outside of
__init code?

>  u64 __init fpu__get_supported_xfeatures_mask(void)
>  {
> -	u64 mask = XFEATURE_MASK_USER_SUPPORTED | XFEATURE_MASK_SUPERVISOR_SUPPORTED;
> -
> -	if (!IS_ENABLED(CONFIG_X86_64))
> -		mask &= ~(XFEATURE_MASK_XTILE);
> +	u64 mask = XFEATURE_MASK_USER_ENABLED | XFEATURE_MASK_SUPERVISOR_SUPPORTED;
> +
> +	if (!IS_ENABLED(CONFIG_X86_64)) {
> +		mask  &= ~(XFEATURE_MASK_XTILE);
> +	} else if (xstate_enable || xstate_disable) {
> +		u64 custom = mask;
> +		u64 unknown;
> +
> +		custom |= xstate_enable;
> +		custom &= ~xstate_disable;
> +
> +		unknown = custom & ~mask;
> +		if (unknown) {
> +			/*
> +			 * User should fully understand the result of using undocumented
> +			 * xstate component.
> +			 */

What is to understand here? Absolutely nothing.  This has been tried to
be smuggled into the kernel ever so often and it's again in something
which claims to do something else and the changelog is silent about it.

The argument 'it allows easier testing of new features' is absolutely
not true simply because the rest of the kernel knows absolutely nothing
about the feature and stuff would go south anyway.

We won't enable features which are unknown ever. Keep that presilicon
test gunk where it belongs: In the Intel poison cabinet along with the
rest of the code which nobody ever want's to see.

> +		}
> +
> +		if ((custom & XFEATURE_MASK_XTILE) != XFEATURE_MASK_XTILE) {
> +			pr_warn("x86/fpu: Error in xstate.disable. Additionally disabling 0x%x components.\n",
> +				XFEATURE_MASK_XTILE);

What?

If the user added: xstate.disable=0x60000 to the command line, then the
code above:

> +		custom &= ~xstate_disable;

has cleared XFEATURE_MASK_XTILE in custom which makes that check true,
the warning emitted and then 

> +			custom &= ~(XFEATURE_MASK_XTILE);

this part clears out XFEATURE_MASK_XTILE once more.

> +		}

What the heck.

> +/*
> + * Longest parameter of 'xstate.enable=' is 22 octal number characters with '0' prefix and
> + * an extra '\0' for termination.
> + */
> +#define MAX_XSTATE_MASK_CHARS	24
> +/*
> + * We parse xstate parameters early because fpu__init_system() is executed before
> + * parse_early_param().
> + */
> +static void __init fpu__init_parse_early_param(void)
> +{
> +	char arg[MAX_XSTATE_MASK_CHARS];
> +
> +	if (cmdline_find_option(boot_command_line, "xstate.enable", arg, sizeof(arg)) &&
> +	    !kstrtoull(arg, 0, &xstate_enable))
> +		xstate_enable &= XFEATURE_MASK_CONFIGURABLE;

This enable thing is not going to happen.

> +	if (cmdline_find_option(boot_command_line, "xstate.disable", arg, sizeof(arg)) &&
> +	    !kstrtoull(arg, 0, &xstate_disable))
> +		xstate_disable &= XFEATURE_MASK_CONFIGURABLE;
> +}
> +

This parser needs to be called for X86_32 because?

Thanks,

        tglx

Re: [PATCH v4 22/22] x86/fpu/xstate: Introduce boot-parameters to control state component support

[Len Brown]

On Sat, Mar 20, 2021 at 4:57 PM Thomas Gleixner <tglx@linutronix.de> wrote:

> We won't enable features which are unknown ever. Keep that presilicon
> test gunk where it belongs: In the Intel poison cabinet along with the
> rest of the code which nobody ever want's to see.

I agree, it would be irresponsible to enable unvalidated features by default,
and pre-silicon "test gunk" should be kept out of the upstream kernel.

This patch series is intended solely to enable fully validated
hardware features,
with product quality kernel support.

The reason that the actual AMX feature isn't mentioned until the 16th
patch in this series
is because all of the patches before it are generic state save/restore patches,
that are not actually specific to AMX.

We call AMX a "simple state feature" -- it actually requires NO KERNEL ENABLING
above the generic state save/restore to fully support userspace AMX
applications.

While not all ISA extensions can be simple state features, we do expect
future features to share this trait, and so we want to be sure that it is simple
to update the kernel to turn those features on (and when necessary, off).

There will be a future CPUID attribute that will help us identify
future simple-state features.
For AMX, of course, we simply know.

So after the generic state management support, the kernel enabling of AMX
is not actually required to run applications.  Just like when a new instruction
is added that re-uses existing state -- the application or library can check
CPUID and just use it.  It is a formality (perhaps an obsolete one), that
we add every feature flag to /proc/cpuid for the "benefit" of userspace.

The reason we propose this cmdline switch is
1. Ability of customers to disable a feature right away if an issue is found.
Unlike the CPUid cmdline that works on flags, this is the ability to turn
off a feature based on its state number.  Ie.  There could be 20 features
that use the same state, and you can turn them all off at once this way.

2. Ability of customers to enable a feature that is disabled by default
in their kernel.  Yes, this will taint their kernel (thanks Andy),
but we have customers that want to run the new feature on day 0
before they have got a distro update to change the default, and this
gives them a way to do that.

Yeah, the cmdline syntax is not a user-friendly mnemonic, and I don't know
that making it so would be an improvement.
Like the CPUID cmdline, it is precise, it is future-proof, and it is
used only in special situations.

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH v4 22/22] x86/fpu/xstate: Introduce boot-parameters to control state component support

[Dave Hansen]

On 3/25/21 3:59 PM, Len Brown wrote:
> We call AMX a "simple state feature" -- it actually requires NO KERNEL ENABLING
> above the generic state save/restore to fully support userspace AMX
> applications.
> 
> While not all ISA extensions can be simple state features, we do expect
> future features to share this trait, and so we want to be sure that it is simple
> to update the kernel to turn those features on (and when necessary, off).

From some IRC chats with Thomaas and Andy, I think it's safe to say that
they're not comfortable blindly enabling even our "simple features".  I
think we're going to need at least some additional architecture to get
us to a point where everyone will be comfortable.

For instance, AMX might be "simple", but there are really only kludgy
ways to get it back to the init state.  Plus, it's *not* simple in that
state left in the registers can have permanent (as long as the state
remains) power and performance impact.

Also, we probably need to expand the "simple" architecture documentation
a bit.  For instance, we need to promise that things like pkeys which
can cause kernel exceptions will never be enumerated as "simple".

Re: [PATCH v4 22/22] x86/fpu/xstate: Introduce boot-parameters to control state component support

[Andy Lutomirski]

On Thu, Mar 25, 2021 at 3:59 PM Len Brown <lenb@kernel.org> wrote:
>
> On Sat, Mar 20, 2021 at 4:57 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> > We won't enable features which are unknown ever. Keep that presilicon
> > test gunk where it belongs: In the Intel poison cabinet along with the
> > rest of the code which nobody ever want's to see.
>
> I agree, it would be irresponsible to enable unvalidated features by default,
> and pre-silicon "test gunk" should be kept out of the upstream kernel.
>
> This patch series is intended solely to enable fully validated
> hardware features,
> with product quality kernel support.
>
> The reason that the actual AMX feature isn't mentioned until the 16th
> patch in this series
> is because all of the patches before it are generic state save/restore patches,
> that are not actually specific to AMX.
>
> We call AMX a "simple state feature" -- it actually requires NO KERNEL ENABLING
> above the generic state save/restore to fully support userspace AMX
> applications.

Regardless of what you call AMX, AMX requires kernel enabling.
Specifically, it appears that leaving AMX in use in the XINUSE sense
degrades system performance and/or power.  And the way to handle that
in kernel (TILERELEASE) cannot possibly be construed as generic.
Here's a little summary of XSTATE features that have failed to be
simple:

 - XMM: seemed simple, but the performance issues switching between
legacy and VEX are still unresolved.  And they affect the kernel, and
people have noticed and complained.

 - ZMM and the high parts of X/YMM: Intel *still* hasn't documented
the actual performance rules.  Reports from people trying to reverse
engineer it suggest that it's horrible on all but the very newest
chips.  For some reason, glibc uses it.  And it broke sigaltstack.  I
have NAKked in-kernel AVX-512 usage until Intel answers a long list of
questions.  No progress yet.

 - PKRU: makes no sense as an XSAVE feature.

 - AMX: XFD, as I understand it, has virtualization problems.  And the
TILERELEASE issue is unresolved.

Intel's track record here is poor.  If you want the kernel to trust
Intel going forward, Intel needs to build trust first.

> So after the generic state management support, the kernel enabling of AMX
> is not actually required to run applications.  Just like when a new instruction
> is added that re-uses existing state -- the application or library can check
> CPUID and just use it.  It is a formality (perhaps an obsolete one), that
> we add every feature flag to /proc/cpuid for the "benefit" of userspace.

Even this isn't true.  AVX-512 already Broke ABI (tm).  Sorry for the
big evil words, but existing programs that worked on Linux stopped
working due to kernel enablement of AVX-512.  AMX has the same
problem, except more than an order of magnitude worse.  No credible
resolution has shown up, and the only remotely credible idea anyone
has mentioned is to actually mask AMX in XCR0 until an application
opts in to an as-yet-undetermined new ABI.

--Andy

Re: [PATCH v4 22/22] x86/fpu/xstate: Introduce boot-parameters to control state component support

[Thomas Gleixner]

Len,

On Thu, Mar 25 2021 at 18:59, Len Brown wrote:
> On Sat, Mar 20, 2021 at 4:57 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
>> We won't enable features which are unknown ever. Keep that presilicon
>> test gunk where it belongs: In the Intel poison cabinet along with the
>> rest of the code which nobody ever want's to see.
>
> I agree, it would be irresponsible to enable unvalidated features by default,
> and pre-silicon "test gunk" should be kept out of the upstream kernel.

Well, that's not my experience from the past and sorry for being
paranoid about that.

> This patch series is intended solely to enable fully validated
> hardware features, with product quality kernel support.

The fact, that the function is broken as provided, definitely supports
that product quality argument.

> The reason that the actual AMX feature isn't mentioned until the 16th
> patch in this series is because all of the patches before it are
> generic state save/restore patches, that are not actually specific to
> AMX.

That's related to 22/22 in which way?

> We call AMX a "simple state feature" -- it actually requires NO KERNEL
> ENABLING above the generic state save/restore to fully support
> userspace AMX applications.

Aside of the unanswered questions vs. the impact of letting it in
initialized state along with the unsolved problem of sigaltstacks...

> While not all ISA extensions can be simple state features, we do
> expect future features to share this trait, and so we want to be sure
> that it is simple to update the kernel to turn those features on (and
> when necessary, off).

History tells me a different story.

> There will be a future CPUID attribute that will help us identify
> future simple-state features.
> For AMX, of course, we simply know.

You believe so, but do you know for sure?

I neither know for sure nor do I believe any of this at all.

Please provide the architectural document which guarantees that and does
so in a way that it can be evaluated by the kernel. Have not seen that,
so it does not exist at all.

  Future CPUID attributes are as useful as the tweet of today.

> So after the generic state management support, the kernel enabling of AMX
> is not actually required to run applications.  Just like when a new instruction
> is added that re-uses existing state -- the application or library can check
> CPUID and just use it.  It is a formality (perhaps an obsolete one), that
> we add every feature flag to /proc/cpuid for the "benefit" of
> userspace.

It's not a formality when the instruction requires kernel support and
from the history of the various incarnations of this command line option
it's just a given that this is going belly up.

Even the current incarnation is broken just from looking at it, so what
the heck are you talking about?

> The reason we propose this cmdline switch is
> 1. Ability of customers to disable a feature right away if an issue is found.
> Unlike the CPUid cmdline that works on flags, this is the ability to turn
> off a feature based on its state number.  Ie.  There could be 20 features
> that use the same state, and you can turn them all off at once this
> way.

I'm fine with that, but then the disabling has to handle all the things
related to it and not just be on a 'pray that it works' base.

> 2. Ability of customers to enable a feature that is disabled by default
> in their kernel.  Yes, this will taint their kernel (thanks Andy),
> but we have customers that want to run the new feature on day 0
> before they have got a distro update to change the default, and this
> gives them a way to do that.

You might know my opinion from previous discussions about this topic,
but let me repeat it for completeness sake:

   This is a generic kernel exposed to a gazillion of users and a
   minority of them want to have the ability to enable insane
   stuff on the command line because:

     1) Intel is not able to provide them a test kernel package

     2) Their favourite $DISTROVENDOR is not able to provide them a
        test kernel package

     3) Intel did not manage to get the support for this upstream
        on time so the $DISTROVENDOR was able to backport it into
        their Frankenkernel

   So you seriously want us to have a command line option to enable
   whatever the feature of today is because of #1-#3?

   Sure, from a Intel managerial POV that's all cool. Not so much when
   you put your community hat on and think about the consequences.

   Aside of that none of the above #1 - #3 is a technical argument.  See
   Documentation/process/* for further enlightment.

Of course none of your arguments above have shown up in the changelog of
this command line patch. And none of the potential side effects or down
sides have been mentioned.

Don't blame Chang Bae for that. That patch carries a:

      Reviewed-by: Len Brown <len.brown@intel.com>

I really have to ask whether you actually looked at the code and the
changelog or just tagged it because some internal procedure requires it.

Either way ....

> Yeah, the cmdline syntax is not a user-friendly mnemonic, and I don't know
> that making it so would be an improvement.
> Like the CPUID cmdline, it is precise, it is future-proof, and it is
> used only in special situations.

The CPUID commandline option is yet another trainwreck which is neither
precise nor future proof if you dare to take a deep technical look. It
should have never been merged and it should be ripped out rather than
proliferated. If you think otherwise then please provide a proper proof
that this commandline option is correct under all circumstances before
abusing it as an argument.

Please try again when you have

  - a reviewable and functional correct implementation

  - including the ability to evalute that via architectural CPUID

  - a changelog which provides an argument which is based on solely
    technical criteria instead of wishful managerial thinking or being
    just void of content like the current one.

Sorry for looking at this solely from the technical side and thereby
ignoring all the managerial powerpoint slide illusions.

Now putting my managerial hat on:

    Given the history of that command line option, I have no idea why
    this has even be tried to piggy pack on AMX at all. It's an
    orthogonal problem and absolutely not required to make AMX supported
    in the first place.

    Hrm, unless you expect that a lot of users will need to disable AMX
    because ... But that would be a technical reason not to enable it
    in the first place, which is not desired from a managerial/marketing
    POV, right?

Thanks,

        tglx

Re: [PATCH v4 22/22] x86/fpu/xstate: Introduce boot-parameters to control state component support

[Len Brown]

On Thu, Mar 25, 2021 at 7:10 PM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 3/25/21 3:59 PM, Len Brown wrote:
> > We call AMX a "simple state feature" -- it actually requires NO KERNEL ENABLING
> > above the generic state save/restore to fully support userspace AMX
> > applications.
> >
> > While not all ISA extensions can be simple state features, we do expect
> > future features to share this trait, and so we want to be sure that it is simple
> > to update the kernel to turn those features on (and when necessary, off).
>
> From some IRC chats with Thomaas and Andy, I think it's safe to say that
> they're not comfortable blindly enabling even our "simple features".  I
> think we're going to need at least some additional architecture to get
> us to a point where everyone will be comfortable.

Hi Dave,

There is no code in this patch series, including patch 22, that enables
an unvalidated feature by default.

Yes, I fully accept that patch 22 allows a user to enable something
that a distro didn't validate.

If there is a new requirement that the kernel cmdline not allow anything
that a distro didn't explicitly validate, then about 99.9% of the kernel cmdline
options that exist today would need to be removed.

Does such a requirement exist, or does it not?

thanks,
-Len

Re: [PATCH v4 22/22] x86/fpu/xstate: Introduce boot-parameters to control state component support

[Len Brown]

On Thu, Mar 25, 2021 at 9:42 PM Andy Lutomirski <luto@kernel.org> wrote:

> Regardless of what you call AMX, AMX requires kernel enabling.

I submit, that after the generic XFD support is in place,
there is exactly 1 bit that needs to be flipped to enable
user applications to benefit from AMX.

I submit the patch that knows about AMX and double checks the
state size is superfluous.

I submit that updating /proc/cpuinfo is superfluous.

What AMX-specific kernel enabling did I miss?

> Specifically, it appears that leaving AMX in use in the XINUSE sense
> degrades system performance and/or power.

Please share the specifics about what performance or power issue you anticipate.

thanks,
-Len

Re: [PATCH v4 22/22] x86/fpu/xstate: Introduce boot-parameters to control state component support

[Len Brown]

On Thu, Mar 25, 2021 at 9:50 PM Thomas Gleixner <tglx@linutronix.de> wrote:

> Please provide the architectural document which guarantees that and does
> so in a way that it can be evaluated by the kernel. Have not seen that,
> so it does not exist at all.
>
>   Future CPUID attributes are as useful as the tweet of today.

I will do so the moment I am permitted.
I'm fine with dropping patch 22 until it can rely on the assurance of
that architectural feature.

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH v4 22/22] x86/fpu/xstate: Introduce boot-parameters to control state component support

[Andy Lutomirski]

On Fri, Mar 26, 2021 at 8:34 AM Len Brown <lenb@kernel.org> wrote:
>
> On Thu, Mar 25, 2021 at 9:42 PM Andy Lutomirski <luto@kernel.org> wrote:
>
> > Regardless of what you call AMX, AMX requires kernel enabling.
>
> I submit, that after the generic XFD support is in place,
> there is exactly 1 bit that needs to be flipped to enable
> user applications to benefit from AMX.

The TILERELEASE opcode itself is rather longer than one bit, and the
supporting code to invoke it at the right time, to avoid corrupting
user state, and avoid causing performance regressions merely by
existing will be orders of magnitude more than 1 bit.  Of course, all
of this is zero bits in the current series because the code is
missing.entirely.

To avoid email thread blowup:

> If there is a new requirement that the kernel cmdline not allow anything
> that a distro didn't explicitly validate, then about 99.9% of the kernel cmdline
> options that exist today would need to be removed.
>
> Does such a requirement exist, or does it not?

This isn't just about validation.  There's also ABI, performance, and
correctness:

ABI: The AVX-512 enablement *already* broke user ABI.  Sadly no one
told anyone in the kernel community until about 5 years after the
fact, and it's a bit late to revert AVX-512.  But we don't want to
enable AMX until the ABI has a reasonable chance of being settled.
Ditto for future features.  As it stands, if you xstate.enable some
16MB feature, the system may well simply fail to boot as too many user
processes explode.

Performance:

We *still* don't know the performance implications of leaving the AMX
features in use inappropriately.  Does it completely destroy idle?
Will it literally operate CPUs out of spec such that Intel's
reliability estimates will be invalidated?  (We had that with NVMe
APST.  Let's not repeat this with XSTATE.)  The performance impacts
and transitions for AVX-512 are, to put it charitably, forthcoming.

Correctness: PKRU via the kernel's normal XSAVE path would simply be
incorrect.  Do we really trust that this won't get repeated?  Also,
frankly, a command line option that may well break lots of userspace
but that we fully expect Intel to recommend setting is not a good
thing.

--Andy

Re: [PATCH v4 22/22] x86/fpu/xstate: Introduce boot-parameters to control state component support

[Len Brown]

On Fri, Mar 26, 2021 at 11:48 AM Andy Lutomirski <luto@kernel.org> wrote:

> > I submit, that after the generic XFD support is in place,
> > there is exactly 1 bit that needs to be flipped to enable
> > user applications to benefit from AMX.
>
> The TILERELEASE opcode itself is rather longer than one bit, and the
> supporting code to invoke it at the right time, to avoid corrupting
> user state, and avoid causing performance regressions merely by
> existing will be orders of magnitude more than 1 bit.  Of course, all
> of this is zero bits in the current series because the code is
> missing.entirely.

Please explain why the kernel must know about the TILERELEASE
instruction in order for an AMX application to run properly.

> This isn't just about validation.  There's also ABI, performance, and
> correctness.

Thank you for agreeing that this is not about unvalidated features.

> ABI: The AVX-512 enablement *already* broke user ABI.  Sadly no one
> told anyone in the kernel community until about 5 years after the
> fact, and it's a bit late to revert AVX-512.  But we don't want to
> enable AMX until the ABI has a reasonable chance of being settled.
> Ditto for future features.  As it stands, if you xstate.enable some
> 16MB feature, the system may well simply fail to boot as too many user
> processes explode.

At Dave's suggestion, we had a 64 *KB* sanity check on this path.
Boris forced us to remove it, because we could not tell him
how we chose the number 64.

I would be delighted to see a check for 64 KB restored, and that
it be a rejection, rather than warning.  At this point, as there is no way
go down that path without manually modifying the kernel, it would
devolve into a sanity check for a hardware (CPUID) bug.

> Performance:
>
> We *still* don't know the performance implications of leaving the AMX
> features in use inappropriately.  Does it completely destroy idle?

No.

> Will it literally operate CPUs out of spec such that Intel's
> reliability estimates will be invalidated?

No.

>  (We had that with NVMe APST.  Let's not repeat this with XSTATE.)

I acknowledge that the possibility of broken hardware always exists.
However, I don't see how the experience with broken NVMe actually applies here,
other than general paranoia about new features (which is, arguably, healthy).

>  The performance impacts
> and transitions for AVX-512 are, to put it charitably, forthcoming.

I acknowledge the parallels with AVX-512, in that AMX adds new instructions,
and it has even bigger registers.  I also acknowledge that the AVX-512 rollout
(and arguably, its brief existence on client CPUs) was problematic.

My understanding is that Intel continues to learn (a lot) from its mistakes.
I believe that the AVX-512 credits problem has been largely eliminated
on newer Xeons.

My understanding is that AMX is implemented only in CPUs that actually
have the hardware to properly support AMX.  If it were not, then that would
be a problem for Intel to deal with in hardware, not a problem for Linux
to deal with in software.

> Correctness: PKRU via the kernel's normal XSAVE path would simply be
> incorrect.  Do we really trust that this won't get repeated?  Also,
> frankly, a command line option that may well break lots of userspace
> but that we fully expect Intel to recommend setting is not a good
> thing.

There is no analogy between AMX and PKRU, except the fact that they
are both features, and at one time, both were new.

I am unaware of anybody at Intel recommending that any cmdline
be set that would break userspace.

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH v4 22/22] x86/fpu/xstate: Introduce boot-parameters to control state component support

[Andy Lutomirski]

On Fri, Mar 26, 2021 at 10:54 AM Len Brown <lenb@kernel.org> wrote:
>
> On Fri, Mar 26, 2021 at 11:48 AM Andy Lutomirski <luto@kernel.org> wrote:
>
> > > I submit, that after the generic XFD support is in place,
> > > there is exactly 1 bit that needs to be flipped to enable
> > > user applications to benefit from AMX.
> >
> > The TILERELEASE opcode itself is rather longer than one bit, and the
> > supporting code to invoke it at the right time, to avoid corrupting
> > user state, and avoid causing performance regressions merely by
> > existing will be orders of magnitude more than 1 bit.  Of course, all
> > of this is zero bits in the current series because the code is
> > missing.entirely.
>
> Please explain why the kernel must know about the TILERELEASE
> instruction in order for an AMX application to run properly.

I'm just repeating things already said, and this is getting
ridiculous.  TILERELEASE isn't needed for an AMX application to run
properly -- it's needed for the rest of the system to run properly, at
least according to Intel's published docs.  Quoting the current ISE
document:

3.3 RECOMMENDATIONS FOR SYSTEM SOFTWARE

System software may disable use of Intel AMX by clearing XCR0[18:17],
by clearing CR4.OSXSAVE, or by setting
IA32_XFD[18]. It is recommended that system software initialize AMX
state (e.g., by executing TILERELEASE)
before doing so. This is because maintaining AMX state in a
non-initialized state may have negative power and
performance implications.

Since you reviewed the patch set, I assume you are familiar with how
Linux manages XSTATE.  Linux does *not* eagerly load XSTATE on context
switch.  Instead, Linux loads XSTATE when the kernel needs it loaded
or before executing user code.  This means that the kernel can (and
does, and it's a performance win) execute kernel thread code and/or go
idle, *including long-term deep idle*, with user XSTATE loaded.


>
> > This isn't just about validation.  There's also ABI, performance, and
> > correctness.
>
> Thank you for agreeing that this is not about unvalidated features.
>
> > ABI: The AVX-512 enablement *already* broke user ABI.  Sadly no one
> > told anyone in the kernel community until about 5 years after the
> > fact, and it's a bit late to revert AVX-512.  But we don't want to
> > enable AMX until the ABI has a reasonable chance of being settled.
> > Ditto for future features.  As it stands, if you xstate.enable some
> > 16MB feature, the system may well simply fail to boot as too many user
> > processes explode.
>
> At Dave's suggestion, we had a 64 *KB* sanity check on this path.
> Boris forced us to remove it, because we could not tell him
> how we chose the number 64.
>
> I would be delighted to see a check for 64 KB restored, and that
> it be a rejection, rather than warning.  At this point, as there is no way
> go down that path without manually modifying the kernel, it would
> devolve into a sanity check for a hardware (CPUID) bug.

This is nuts.  The ABI is ALREADY BROKEN.  How does picking a random
number quantifying additional breakage help?  We do not have a good
design for AVX-512 in Linux, we don't have a good design for AMX in
Linux, and we absolutely don't have a good design for the secret
feature we don't know about yet in Linux.

Re: [PATCH v4 22/22] x86/fpu/xstate: Introduce boot-parameters to control state component support

[Borislav Petkov]

On Fri, Mar 26, 2021 at 01:53:47PM -0400, Len Brown wrote:
> At Dave's suggestion, we had a 64 *KB* sanity check on this path.
> Boris forced us to remove it, because we could not tell him
> how we chose the number 64.

The only 64 I can remember is

#define XSTATE_BUFFER_MAX_BYTES              (64 * 1024)

What does an arbitrary number have to do with signal handling and
pushing a fat frame on the sigaltstack?

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg

Re: [PATCH v4 22/22] x86/fpu/xstate: Introduce boot-parameters to control state component support

[Thomas Gleixner]

Len,

On Fri, Mar 26 2021 at 11:27, Len Brown wrote:
> On Thu, Mar 25, 2021 at 7:10 PM Dave Hansen <dave.hansen@intel.com> wrote:
>> From some IRC chats with Thomaas and Andy, I think it's safe to say that
>> they're not comfortable blindly enabling even our "simple features".  I
>> think we're going to need at least some additional architecture to get
>> us to a point where everyone will be comfortable.
>
> There is no code in this patch series, including patch 22, that enables
> an unvalidated feature by default.
>
> Yes, I fully accept that patch 22 allows a user to enable something
> that a distro didn't validate.

That's not the point. And neither Andy nor myself asked for distros to
validate and approve anything.

> If there is a new requirement that the kernel cmdline not allow anything
> that a distro didn't explicitly validate, then about 99.9% of the kernel cmdline
> options that exist today would need to be removed.
>
> Does such a requirement exist, or does it not?

Nobody said that, but patches to remove command line options are always
welcome. Can we start with the most horrible of all we have today, i.e
"clearcpuid=", please?

Thanks,

        tglx

Re: [PATCH v4 22/22] x86/fpu/xstate: Introduce boot-parameters to control state component support

[Len Brown]

On Fri, Mar 26, 2021 at 2:17 PM Borislav Petkov <bp@alien8.de> wrote:
>
> On Fri, Mar 26, 2021 at 01:53:47PM -0400, Len Brown wrote:
> > At Dave's suggestion, we had a 64 *KB* sanity check on this path.
> > Boris forced us to remove it, because we could not tell him
> > how we chose the number 64.
>
> The only 64 I can remember is
>
> #define XSTATE_BUFFER_MAX_BYTES              (64 * 1024)
>
> What does an arbitrary number have to do with signal handling and
> pushing a fat frame on the sigaltstack?

You are right.  If that is where the check was, it was the wrong place.
It should be part of that sanity check code where CPUID is parsed.

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH v4 22/22] x86/fpu/xstate: Introduce boot-parameters to control state component support

[Len Brown]

> 3.3 RECOMMENDATIONS FOR SYSTEM SOFTWARE
>
> System software may disable use of Intel AMX by clearing XCR0[18:17],
> by clearing CR4.OSXSAVE, or by setting
> IA32_XFD[18]. It is recommended that system software initialize AMX
> state (e.g., by executing TILERELEASE)
> before doing so. This is because maintaining AMX state in a
> non-initialized state may have negative power and
> performance implications.

I agree that the wording here about disabling AMX is ominous.

The hardware initializes with AMX disabled.
The kernel probes AMX, enables it in XCR0, and keeps it enabled.

Initially, XFD is "armed" for all tasks.
When a task accesses AMX state, #NM fires, we allocate a context
switch buffer, and we "disarm" XFD for that task.
As we have that buffer in-hand for the lifetime of the task, we never
"arm" XFD for that task again.

XFD is context switched, and so the next time it is set, is when we
are restoring some other task's state.

n.b. I'm describing the Linux flow.  The VMM scenario is a little different.

> Since you reviewed the patch set, I assume you are familiar with how
> Linux manages XSTATE.  Linux does *not* eagerly load XSTATE on context
> switch.  Instead, Linux loads XSTATE when the kernel needs it loaded
> or before executing user code.  This means that the kernel can (and
> does, and it's a performance win) execute kernel thread code and/or go
> idle, *including long-term deep idle*, with user XSTATE loaded.

Yes, this scenario is clear.

There are several cases.

1. Since TMM registers are volatile, a routine using TMM that wants
them to persist across a call must save them,
    and will TILERELEASE before invoking that call.  That is the
calling convention,
    and I expect that if it is not followed, debugging (of tools) will
occur until it is.

    The only way for a user program's XSTATE to be present during the
kernel's call to idle
    is if it sleep via a system call when no other task wants to run
on that CPU.

    Since system calls are calls, in this case, AMX INIT=1 during idle.
    All deep C-state are enabled, the idle CPU is able to contribute
it's maximum turbo buget to its peers.

2. A correct program with live TMM registers takes an interrupt, and
we enter the kernel AMX INIT=0.
    Yes, we will enter the syscall at the frequency of the app (like
we always do).
    Yes, turbo frequency may be limited by the activity of this
processor and its peers (like it always is)

   2a. If we return to the same program, then depending on how long
the syscall runs, we may execute
         the program and the system call code at a frequency lower
than we might if AMX INIT=1 at time of interrupt.

   2b. If we context switch to a task that has AMX INIT=1, then any
AMX-imposed limits on turbo
         are immediately gone.

    Note for 2b.  4 generations have passed since SKX had significant
delay releasing AVX-512 credits.
    The delay in the first hardware that supports AXM should be
negligible for both AVX-512 and AMX.

3. A buggy or purposely bogus program is fully empowered to violate
the programming conventions.
    Say such a program called a long sleep, and nothing else wanted to
run on that CPU, so the kernel
    went idle with AMX INIT=0.  Indeed, this could retard the core
from getting into the deepest available
    C-state, which could impact the turbo budget of neighboring cores.
However, if that were some kind
    of DOS, it would be simpler and more effective to simply hog a CPU
by running code.  Also, as soon
    as another thread switches in with INIT=1, there is no concept of
AMX frequency caps. (see note for 2b)

I do not see a situation where the kernel needs to issue TILERELEASE
(though a VMM likely would).
What did I miss?

thanks,
Len Brown, Intel Open Source Technology Center

ps. I will respond to your ABI thoughts on your new ABI thread.

Re: [PATCH v4 22/22] x86/fpu/xstate: Introduce boot-parameters to control state component support

[Thomas Gleixner]

Len,

On Sat, Mar 27 2021 at 00:53, Len Brown wrote:
>> 3.3 RECOMMENDATIONS FOR SYSTEM SOFTWARE
>>
>> System software may disable use of Intel AMX by clearing XCR0[18:17],
>> by clearing CR4.OSXSAVE, or by setting
>> IA32_XFD[18]. It is recommended that system software initialize AMX
>> state (e.g., by executing TILERELEASE)
>> before doing so. This is because maintaining AMX state in a
>> non-initialized state may have negative power and
>> performance implications.
>
> I agree that the wording here about disabling AMX is ominous.

Which is what I pointed out 7 days ago already, but that got lost in the
ABI and command line noise... Thanks Andy for bringing it back!

> The hardware initializes with AMX disabled.
> The kernel probes AMX, enables it in XCR0, and keeps it enabled.
>
> Initially, XFD is "armed" for all tasks.
> When a task accesses AMX state, #NM fires, we allocate a context
> switch buffer, and we "disarm" XFD for that task.
> As we have that buffer in-hand for the lifetime of the task, we never
> "arm" XFD for that task again.
>
> XFD is context switched, and so the next time it is set, is when we
> are restoring some other task's state.
>
> n.b. I'm describing the Linux flow.  The VMM scenario is a little different.
>
>> Since you reviewed the patch set, I assume you are familiar with how
>> Linux manages XSTATE.  Linux does *not* eagerly load XSTATE on context
>> switch.  Instead, Linux loads XSTATE when the kernel needs it loaded
>> or before executing user code.  This means that the kernel can (and
>> does, and it's a performance win) execute kernel thread code and/or go
>> idle, *including long-term deep idle*, with user XSTATE loaded.
>
> Yes, this scenario is clear.
>
> There are several cases.
>
> 1. Since TMM registers are volatile, a routine using TMM that wants
> them to persist across a call must save them,
>     and will TILERELEASE before invoking that call.  That is the
> calling convention,
>     and I expect that if it is not followed, debugging (of tools) will
> occur until it is.
>
>     The only way for a user program's XSTATE to be present during the
> kernel's call to idle
>     is if it sleep via a system call when no other task wants to run
> on that CPU.
>
>     Since system calls are calls, in this case, AMX INIT=1 during
>     idle.

What is the guarantee for that? A calling convention?

That's uninteresting because that's only the recommended and desired
state and not the guaranteed state.

>     All deep C-state are enabled, the idle CPU is able to contribute
> it's maximum turbo buget to its peers.
>
> 2. A correct program with live TMM registers takes an interrupt, and
> we enter the kernel AMX INIT=0.
>     Yes, we will enter the syscall at the frequency of the app (like
> we always do).

That's about interrupts not syscalls and I assume this should be all
s/syscall/interrupt/ for the whole #2 including 2a

>     Yes, turbo frequency may be limited by the activity of this
> processor and its peers (like it always is)
>
>    2a. If we return to the same program, then depending on how long
> the syscall runs, we may execute
>          the program and the system call code at a frequency lower
> than we might if AMX INIT=1 at time of interrupt.

So the frequency effect is relevant for the duration of the interrupt
and the eventually appended soft interrupt, right?

The program state is uninteresting because even if the kernel would
do XSAVES, TILERELEASE on interrupt entry then it would restore the
state before returning and then the program would have the same
conditions as before the interrupt.

>    2b. If we context switch to a task that has AMX INIT=1, then any
> AMX-imposed limits on turbo
>          are immediately gone.

Immediately on context switch? Definitely not.

      switch_to(prev, next)
        XSAVES(prev)
        eventually set XFD[18]

The point where AMX INIT=1 of 'next' becomes relevant is on return to
user space where XRSTORS happens. Up to that point AMX INIT=0 stays in
effect.

Now what guarantees that 'next' is returning to user space immediately?

Nothing.

If it's a user task this can be a wakeup for whatever which might cause
another wait depending on the callchain that task is in. It can be
preempted before reaching XRSTORS which is the point that matters to
flip the AMX INIT state back to 1.

It can be a kernel task or a chain of kernel tasks with arbitrary
runtime.

As a consequence the scheduler might migrate 'prev' from CPU_A to CPU_L
and what happens to that state on CPU_A? Does it magically move along
with 'prev' to CPU_L? I can't see how, but what do I know about magic.

So now the chain of kernel tasks finishes and there is nothing to do,
CPU_A goes idle with AMX INIT=0, which prevents the CPU from going deep,
drains power, can't contribute to the turbo state or whatever undesired
side effects that has.

You can get the same effect not only by device interrupts but also by
regular task migration, ptrace, breakpoints, any form of traps,
exception the task triggers in user space, user space freezing, kill -9
and .....

> 3. A buggy or purposely bogus program is fully empowered to violate
> the programming conventions.
>     Say such a program called a long sleep, and nothing else wanted to
> run on that CPU, so the kernel
>     went idle with AMX INIT=0.  Indeed, this could retard the core
> from getting into the deepest available
>     C-state, which could impact the turbo budget of neighboring cores.
> However, if that were some kind
>     of DOS, it would be simpler and more effective to simply hog a CPU
> by running code.  Also, as soon
>     as another thread switches in with INIT=1, there is no concept of
> AMX frequency caps. (see note for 2b)

It's irrelevant whether this is intentionally buggy or not. It's equally
irrelevant whether this is a stupid attempt of DOS or not.

What's relevant is that this has undesired side effects of various
sorts.

> I do not see a situation where the kernel needs to issue TILERELEASE
> (though a VMM likely would).

So #3 does not qualify for you? Interesting POV.

> What did I miss?

See #2.b

What's the actual downside of issuing TILERELEASE conditionally
depending on prev->AMX INIT=0? Is it slooooow or what's the real
problem here?

Thanks,

        tglx

Re: [PATCH v4 22/22] x86/fpu/xstate: Introduce boot-parameters to control state component support

[Len Brown]

On Sat, Mar 27, 2021 at 6:20 PM Thomas Gleixner <tglx@linutronix.de> wrote:

> What's the actual downside of issuing TILERELEASE conditionally
> depending on prev->AMX INIT=0? Is it slooooow or what's the real
> problem here?

TILERELEASE is fast, so there should be no down-side to execute it.
Indeed, checking whether you need to execute it or not will probably take
longer than executing TILERELEASE.  My point (perhaps academic)
is that Linux should not have to know about TILERELEASE, or execute it.

Re: running in the kernel with AMX INIT=0

AMX INIT=0 will prevent c6 on that core.  I don't expect to see this
in the syscall path, though if a user wanted to neglect to issue TILERELEASE,
there is nothing forcing them to do so.

It can certainly happen on the interrupt path, but on the interrupt patch
I don't know if we can end up requesting c6 -- perhaps on a forced
task migration?

Re:  frequency credits in the kernel with AMX INIT=0.

It works exactly the same way as AMX INIT=1.
That is to say, the frequency credits don't key off of AMX INIT,
they key off of the actual use of the AMX execution unit, and
the credits free up several orders of magnitude faster
(both for AVX-512 and AMX) on this hardware as in previous generations.

As a result, if we interrupt an AMX program, and run for an extended
period of time in the kernel without XRESTOR to clear out his AMX INIT=0 state,
that will not have any impact on the frequency we run inside the kernel any more
than if he had AMX INIT=1 state.

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH v4 22/22] x86/fpu/xstate: Introduce boot-parameters to control state component support

[Thomas Gleixner]

On Mon, Mar 29 2021 at 09:31, Len Brown wrote:
> On Sat, Mar 27, 2021 at 6:20 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
>> What's the actual downside of issuing TILERELEASE conditionally
>> depending on prev->AMX INIT=0? Is it slooooow or what's the real
>> problem here?
>
> TILERELEASE is fast, so there should be no down-side to execute it.
> Indeed, checking whether you need to execute it or not will probably take
> longer than executing TILERELEASE.  My point (perhaps academic)
> is that Linux should not have to know about TILERELEASE, or execute it.
>
> Re: running in the kernel with AMX INIT=0
>
> AMX INIT=0 will prevent c6 on that core.  I don't expect to see this
> in the syscall path, though if a user wanted to neglect to issue TILERELEASE,
> there is nothing forcing them to do so.
>
> It can certainly happen on the interrupt path, but on the interrupt patch
> I don't know if we can end up requesting c6 -- perhaps on a forced
> task migration?

I think I clearly described how it can end up in that situation and that
there are a gazillion ways to get there.

If I decide at 5PM to call it a day after hitting the breakpoint, then I
really would appreciate that the machine goes deep idle instead of
staying at C1(E) until 9AM when I come back.

> Re:  frequency credits in the kernel with AMX INIT=0.
>
> It works exactly the same way as AMX INIT=1.
> That is to say, the frequency credits don't key off of AMX INIT,
> they key off of the actual use of the AMX execution unit, and
> the credits free up several orders of magnitude faster
> (both for AVX-512 and AMX) on this hardware as in previous generations.
>
> As a result, if we interrupt an AMX program, and run for an extended
> period of time in the kernel without XRESTOR to clear out his AMX INIT=0 state,
> that will not have any impact on the frequency we run inside the kernel any more
> than if he had AMX INIT=1 state.

Ok. That's clearly missing in documentation, but it does not solve the C
state issue at all.

Thanks,

        tglx