From: Kan Liang <[email protected]>
An xstate size check warning is triggered on the machine which supports
Architectural LBRs.
[ 0.000000] XSAVE consistency problem, dumping leaves
[ 0.000000] WARNING: CPU: 0 PID: 0 at
arch/x86/kernel/fpu/xstate.c:649 fpu__init_system_xstate+0x4d4/0xd0e
[ 0.000000] Modules linked in:
[ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted intel-arch_lbr+
[ 0.000000] RIP: 0010:fpu__init_system_xstate+0x4d4/0xd0e
The xstate size check routine, init_xstate_size(), compares the size
retrieved from the hardware with the size of task->fpu, which is
calculated by the software.
The size from the hardware is the total size of the enabled xstates in
XCR0 | IA32_XSS. Architectural LBR state is a dynamic supervisor
feature, which sets the corresponding bit in the IA32_XSS at boot time.
The size from the hardware includes the size of the Architectural LBR
state.
However, a dynamic supervisor feature doesn't allocate a buffer in the
task->fpu. The size of task->fpu doesn't include the size of the
Architectural LBR state. The mismatch will trigger the warning.
Three options as below were considered to fix the issue:
- Correct the size from the hardware by subtracting the size of the
dynamic supervisor features.
The purpose of the check is to compare the size CPU told with the size
of the XSAVE buffer, which is calculated by the software. If the
software mucks with the number from hardware, it removes the value of
the check.
This option is not a good option.
- Prevent the hardware from counting the size of the dynamic supervisor
feature by temporarily removing the corresponding bits in IA32_XSS.
Two extra MSR writes are required to flip the IA32_XSS. The option is
not pretty, but it is workable. The check is only called once at early
boot time. The synchronization or context-switching doesn't need to be
worried.
This option is implemented here.
- Remove the check entirely, because the check hasn't found any real
problems. The option may be an alternative as option 2.
This option is not implemented here.
Add a new function, get_xsaves_size_no_dynamic(), which retrieves the
total size without the dynamic supervisor features from the hardware.
The size will be used to compare with the size of task->fpu.
Fixes: f0dccc9da4c0 ("x86/fpu/xstate: Support dynamic supervisor feature for LBR")
Reported-by: Chang S. Bae <[email protected]>
Reviewed-by: Dave Hansen <[email protected]>
Signed-off-by: Kan Liang <[email protected]>
---
arch/x86/kernel/fpu/xstate.c | 33 ++++++++++++++++++++++++++++++++-
1 file changed, 32 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 10cf878..a4e4ac4 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -611,6 +611,10 @@ static void check_xstate_against_struct(int nr)
* This essentially double-checks what the cpu told us about
* how large the XSAVE buffer needs to be. We are recalculating
* it to be safe.
+ *
+ * Dynamic XSAVE features allocate their own buffers and are not
+ * covered by these checks. Only the size of the buffer for task->fpu
+ * is checked here.
*/
static void do_extra_xstate_size_checks(void)
{
@@ -673,6 +677,33 @@ static unsigned int __init get_xsaves_size(void)
return ebx;
}
+/*
+ * Get the total size of the enabled xstates without the dynamic supervisor
+ * features.
+ */
+static unsigned int __init get_xsaves_size_no_dynamic(void)
+{
+ u64 mask = xfeatures_mask_dynamic();
+ unsigned int size;
+
+ if (!mask)
+ return get_xsaves_size();
+
+ /* Disable dynamic features. */
+ wrmsrl(MSR_IA32_XSS, xfeatures_mask_supervisor());
+
+ /*
+ * Ask the hardware what size is required of the buffer.
+ * This is the size required from the task->fpu buffer.
+ */
+ size = get_xsaves_size();
+
+ /* Re-enable dynamic features so XSAVES will work on them again. */
+ wrmsrl(MSR_IA32_XSS, xfeatures_mask_supervisor() | mask);
+
+ return size;
+}
+
static unsigned int __init get_xsave_size(void)
{
unsigned int eax, ebx, ecx, edx;
@@ -710,7 +741,7 @@ static int __init init_xstate_size(void)
xsave_size = get_xsave_size();
if (boot_cpu_has(X86_FEATURE_XSAVES))
- possible_xstate_size = get_xsaves_size();
+ possible_xstate_size = get_xsaves_size_no_dynamic();
else
possible_xstate_size = xsave_size;
--
2.7.4
On Mon, Jul 20, 2020 at 06:50:51AM -0700, [email protected] wrote:
...
> static unsigned int __init get_xsave_size(void)
> {
> unsigned int eax, ebx, ecx, edx;
> @@ -710,7 +741,7 @@ static int __init init_xstate_size(void)
> xsave_size = get_xsave_size();
>
> if (boot_cpu_has(X86_FEATURE_XSAVES))
> - possible_xstate_size = get_xsaves_size();
> + possible_xstate_size = get_xsaves_size_no_dynamic();
> else
> possible_xstate_size = xsave_size;
Hi! Maybe we could enhance get_xsaves_size instead ? The get_xsaves_size is
static and __init function (thus not a hot path) used once as far as I see.
Say
static unsigned int __init get_xsaves_size(void)
{
u64 mask = xfeatures_mask_dynamic();
unsigned int eax, ebx, ecx, edx;
/*
* In case if dynamic features are present make
* sure they are not accounted in the result since
* the buffer should be allocated separately from
* task->fpu.
*/
if (mask)
wrmsrl(MSR_IA32_XSS, xfeatures_mask_supervisor());
/*
* - CPUID function 0DH, sub-function 1:
* EBX enumerates the size (in bytes) required by
* the XSAVES instruction for an XSAVE area
* containing all the state components
* corresponding to bits currently set in
* XCR0 | IA32_XSS.
*/
cpuid_count(XSTATE_CPUID, 1, &eax, &ebx, &ecx, &edx);
if (mask)
wrmsrl(MSR_IA32_XSS, xfeatures_mask_supervisor() | mask);
return ebx;
}
but if you expect more use of get_xsaves_size_no_dynamic() and
get_xsaves_size() in future then sure, we need a separate function.
The benefit from such extension is that when you read get_xsaves_size
you'll notice the dependency on dynamic features immediaely.
Though I'm fine with current patch as well, up to you. Thanks for the patch!
Reviewed-by: Cyrill Gorcunov <[email protected]>
On 7/20/2020 1:33 PM, Cyrill Gorcunov wrote:
> On Mon, Jul 20, 2020 at 06:50:51AM -0700, [email protected] wrote:
> ...
>> static unsigned int __init get_xsave_size(void)
>> {
>> unsigned int eax, ebx, ecx, edx;
>> @@ -710,7 +741,7 @@ static int __init init_xstate_size(void)
>> xsave_size = get_xsave_size();
>>
>> if (boot_cpu_has(X86_FEATURE_XSAVES))
>> - possible_xstate_size = get_xsaves_size();
>> + possible_xstate_size = get_xsaves_size_no_dynamic();
>> else
>> possible_xstate_size = xsave_size;
>
> Hi! Maybe we could enhance get_xsaves_size instead ? The get_xsaves_size is
> static and __init function (thus not a hot path) used once as far as I see.
> Say
>
> static unsigned int __init get_xsaves_size(void)
> {
> u64 mask = xfeatures_mask_dynamic();
> unsigned int eax, ebx, ecx, edx;
>
> /*
> * In case if dynamic features are present make
> * sure they are not accounted in the result since
> * the buffer should be allocated separately from
> * task->fpu.
> */
> if (mask)
> wrmsrl(MSR_IA32_XSS, xfeatures_mask_supervisor());
>
> /*
> * - CPUID function 0DH, sub-function 1:
> * EBX enumerates the size (in bytes) required by
> * the XSAVES instruction for an XSAVE area
> * containing all the state components
> * corresponding to bits currently set in
> * XCR0 | IA32_XSS.
> */
> cpuid_count(XSTATE_CPUID, 1, &eax, &ebx, &ecx, &edx);
>
> if (mask)
> wrmsrl(MSR_IA32_XSS, xfeatures_mask_supervisor() | mask);
>
> return ebx;
> }
>
> but if you expect more use of get_xsaves_size_no_dynamic() and
> get_xsaves_size() in future then sure, we need a separate function.
>
For now, I don't have more use of
get_xsaves_size_no_dynamic()/get_xsaves_size(). I don't know if anyone
else will use them later.
> The benefit from such extension is that when you read get_xsaves_size
> you'll notice the dependency on dynamic features immediaely.
>
> Though I'm fine with current patch as well, up to you. Thanks for the patch!
>
Personally, I prefer to keep the current patch because I like the name
get_xsaves_size_no_dynamic(), which explicitly tells the dynamic
features are excluded.
> Reviewed-by: Cyrill Gorcunov <[email protected]>
>
Thanks for the review.
Kan
The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: ec8602b79088b0f3556d9c7a3a05313bc4e4a96f
Gitweb: https://git.kernel.org/tip/ec8602b79088b0f3556d9c7a3a05313bc4e4a96f
Author: Kan Liang <[email protected]>
AuthorDate: Mon, 20 Jul 2020 06:50:51 -07:00
Committer: Ingo Molnar <[email protected]>
CommitterDate: Thu, 06 Aug 2020 17:11:59 +02:00
x86/fpu/xstate: Fix an xstate size check warning with architectural LBRs
An xstate size check warning is triggered on machines which support
Architectural LBRs.
XSAVE consistency problem, dumping leaves
WARNING: CPU: 0 PID: 0 at arch/x86/kernel/fpu/xstate.c:649 fpu__init_system_xstate+0x4d4/0xd0e
Modules linked in:
CPU: 0 PID: 0 Comm: swapper Not tainted intel-arch_lbr+
RIP: 0010:fpu__init_system_xstate+0x4d4/0xd0e
The xstate size check routine, init_xstate_size(), compares the size
retrieved from the hardware with the size of task->fpu, which is
calculated by the software.
The size from the hardware is the total size of the enabled xstates in
XCR0 | IA32_XSS. Architectural LBR state is a dynamic supervisor
feature, which sets the corresponding bit in the IA32_XSS at boot time.
The size from the hardware includes the size of the Architectural LBR
state.
However, a dynamic supervisor feature doesn't allocate a buffer in the
task->fpu. The size of task->fpu doesn't include the size of the
Architectural LBR state. The mismatch will trigger the warning.
Three options as below were considered to fix the issue:
- Correct the size from the hardware by subtracting the size of the
dynamic supervisor features.
The purpose of the check is to compare the size CPU told with the size
of the XSAVE buffer, which is calculated by the software. If the
software mucks with the number from hardware, it removes the value of
the check.
This option is not a good option.
- Prevent the hardware from counting the size of the dynamic supervisor
feature by temporarily removing the corresponding bits in IA32_XSS.
Two extra MSR writes are required to flip the IA32_XSS. The option is
not pretty, but it is workable. The check is only called once at early
boot time. The synchronization or context-switching doesn't need to be
worried.
This option is implemented here.
- Remove the check entirely, because the check hasn't found any real
problems. The option may be an alternative as option 2.
This option is not implemented here.
Add a new function, get_xsaves_size_no_dynamic(), which retrieves the
total size without the dynamic supervisor features from the hardware.
The size will be used to compare with the size of task->fpu.
Fixes: f0dccc9da4c0 ("x86/fpu/xstate: Support dynamic supervisor feature for LBR")
Reported-by: Chang S. Bae <[email protected]>
Signed-off-by: Kan Liang <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
Reviewed-by: Dave Hansen <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
arch/x86/kernel/fpu/xstate.c | 33 ++++++++++++++++++++++++++++++++-
1 file changed, 32 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index be2a68a..6073e34 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -611,6 +611,10 @@ static void check_xstate_against_struct(int nr)
* This essentially double-checks what the cpu told us about
* how large the XSAVE buffer needs to be. We are recalculating
* it to be safe.
+ *
+ * Dynamic XSAVE features allocate their own buffers and are not
+ * covered by these checks. Only the size of the buffer for task->fpu
+ * is checked here.
*/
static void do_extra_xstate_size_checks(void)
{
@@ -673,6 +677,33 @@ static unsigned int __init get_xsaves_size(void)
return ebx;
}
+/*
+ * Get the total size of the enabled xstates without the dynamic supervisor
+ * features.
+ */
+static unsigned int __init get_xsaves_size_no_dynamic(void)
+{
+ u64 mask = xfeatures_mask_dynamic();
+ unsigned int size;
+
+ if (!mask)
+ return get_xsaves_size();
+
+ /* Disable dynamic features. */
+ wrmsrl(MSR_IA32_XSS, xfeatures_mask_supervisor());
+
+ /*
+ * Ask the hardware what size is required of the buffer.
+ * This is the size required for the task->fpu buffer.
+ */
+ size = get_xsaves_size();
+
+ /* Re-enable dynamic features so XSAVES will work on them again. */
+ wrmsrl(MSR_IA32_XSS, xfeatures_mask_supervisor() | mask);
+
+ return size;
+}
+
static unsigned int __init get_xsave_size(void)
{
unsigned int eax, ebx, ecx, edx;
@@ -710,7 +741,7 @@ static int __init init_xstate_size(void)
xsave_size = get_xsave_size();
if (boot_cpu_has(X86_FEATURE_XSAVES))
- possible_xstate_size = get_xsaves_size();
+ possible_xstate_size = get_xsaves_size_no_dynamic();
else
possible_xstate_size = xsave_size;
The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: 76d10256a97a7cab72b123d54b766a3c17da658c
Gitweb: https://git.kernel.org/tip/76d10256a97a7cab72b123d54b766a3c17da658c
Author: Kan Liang <[email protected]>
AuthorDate: Mon, 20 Jul 2020 06:50:51 -07:00
Committer: Ingo Molnar <[email protected]>
CommitterDate: Fri, 07 Aug 2020 01:32:00 +02:00
x86/fpu/xstate: Fix an xstate size check warning with architectural LBRs
An xstate size check warning is triggered on machines which support
Architectural LBRs.
XSAVE consistency problem, dumping leaves
WARNING: CPU: 0 PID: 0 at arch/x86/kernel/fpu/xstate.c:649 fpu__init_system_xstate+0x4d4/0xd0e
Modules linked in:
CPU: 0 PID: 0 Comm: swapper Not tainted intel-arch_lbr+
RIP: 0010:fpu__init_system_xstate+0x4d4/0xd0e
The xstate size check routine, init_xstate_size(), compares the size
retrieved from the hardware with the size of task->fpu, which is
calculated by the software.
The size from the hardware is the total size of the enabled xstates in
XCR0 | IA32_XSS. Architectural LBR state is a dynamic supervisor
feature, which sets the corresponding bit in the IA32_XSS at boot time.
The size from the hardware includes the size of the Architectural LBR
state.
However, a dynamic supervisor feature doesn't allocate a buffer in the
task->fpu. The size of task->fpu doesn't include the size of the
Architectural LBR state. The mismatch will trigger the warning.
Three options as below were considered to fix the issue:
- Correct the size from the hardware by subtracting the size of the
dynamic supervisor features.
The purpose of the check is to compare the size CPU told with the size
of the XSAVE buffer, which is calculated by the software. If the
software mucks with the number from hardware, it removes the value of
the check.
This option is not a good option.
- Prevent the hardware from counting the size of the dynamic supervisor
feature by temporarily removing the corresponding bits in IA32_XSS.
Two extra MSR writes are required to flip the IA32_XSS. The option is
not pretty, but it is workable. The check is only called once at early
boot time. The synchronization or context-switching doesn't need to be
worried.
This option is implemented here.
- Remove the check entirely, because the check hasn't found any real
problems. The option may be an alternative as option 2.
This option is not implemented here.
Add a new function, get_xsaves_size_no_dynamic(), which retrieves the
total size without the dynamic supervisor features from the hardware.
The size will be used to compare with the size of task->fpu.
Fixes: f0dccc9da4c0 ("x86/fpu/xstate: Support dynamic supervisor feature for LBR")
Reported-by: Chang S. Bae <[email protected]>
Signed-off-by: Kan Liang <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
Reviewed-by: Dave Hansen <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
arch/x86/kernel/fpu/xstate.c | 33 ++++++++++++++++++++++++++++++++-
1 file changed, 32 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index be2a68a..6073e34 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -611,6 +611,10 @@ static void check_xstate_against_struct(int nr)
* This essentially double-checks what the cpu told us about
* how large the XSAVE buffer needs to be. We are recalculating
* it to be safe.
+ *
+ * Dynamic XSAVE features allocate their own buffers and are not
+ * covered by these checks. Only the size of the buffer for task->fpu
+ * is checked here.
*/
static void do_extra_xstate_size_checks(void)
{
@@ -673,6 +677,33 @@ static unsigned int __init get_xsaves_size(void)
return ebx;
}
+/*
+ * Get the total size of the enabled xstates without the dynamic supervisor
+ * features.
+ */
+static unsigned int __init get_xsaves_size_no_dynamic(void)
+{
+ u64 mask = xfeatures_mask_dynamic();
+ unsigned int size;
+
+ if (!mask)
+ return get_xsaves_size();
+
+ /* Disable dynamic features. */
+ wrmsrl(MSR_IA32_XSS, xfeatures_mask_supervisor());
+
+ /*
+ * Ask the hardware what size is required of the buffer.
+ * This is the size required for the task->fpu buffer.
+ */
+ size = get_xsaves_size();
+
+ /* Re-enable dynamic features so XSAVES will work on them again. */
+ wrmsrl(MSR_IA32_XSS, xfeatures_mask_supervisor() | mask);
+
+ return size;
+}
+
static unsigned int __init get_xsave_size(void)
{
unsigned int eax, ebx, ecx, edx;
@@ -710,7 +741,7 @@ static int __init init_xstate_size(void)
xsave_size = get_xsave_size();
if (boot_cpu_has(X86_FEATURE_XSAVES))
- possible_xstate_size = get_xsaves_size();
+ possible_xstate_size = get_xsaves_size_no_dynamic();
else
possible_xstate_size = xsave_size;