diff mbox series

[RFC] x86/fpu/xstate: Add more diagnostic information on inconsistent xstate sizes

Message ID 20230405183942.734019-1-fenghua.yu@intel.com
State New
Headers
Series [RFC] x86/fpu/xstate: Add more diagnostic information on inconsistent xstate sizes |

Commit Message

Fenghua Yu April 5, 2023, 6:39 p.m. UTC 
  A warning is emitted when xstate sizes are found inconsistent.
But the warning message doesn't show enough information to diagnose
the issue.

Provide more detailed xstate size information to help debug the issue.
As a hypothetical example, on a platform that may report incorrect
xstate size in CPUID.0xd.1:EBX, the diagnostic information after
the warning will show:
[    0.000000] x86/fpu: max_features=0x407
[    0.000000] x86/fpu: xstate_offset[2]:  576, xstate_sizes[2]:  256
[    0.000000] x86/fpu: xstate_offset[10]: 832, xstate_sizes[10]:    8
[    0.000000] x86/fpu: total size: 840 bytes
[    0.000000] x86/fpu: XCR0=0x7, IA32_XSS=0x400
[    0.000000] x86/fpu: kernel_size from CPUID.0xd.0x1:EBX: 576 bytes

XCR0 | IA32_XSS is 0x407 which is consistent with max_features.
CPUID.0xd.0x1:EBX should report the size of the xsave area
containing xstate components corresponding to bits set in
XCR0 | IA32_XSS. But it only reports 576 bytes of xsave area which
doesn't count xstate sizes for AVX (offset 2 and 256 bytes) and
PASID (offset 10 and 8 bytes). This confirms that the platform
reports xstate size incorrectly through the CPUID bits.

Suggest-by: Dave Hansen <dave.hansen@linux.intel.com>
Tested-by: Chintan M Patel <chintan.m.patel@intel.com>
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/x86/kernel/fpu/xstate.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

Comments

Chang S. Bae April 7, 2023, 6:22 p.m. UTC | #1 
On 4/5/2023 11:39 AM, Fenghua Yu wrote:
> A warning is emitted when xstate sizes are found inconsistent.
> But the warning message doesn't show enough information to diagnose
> the issue.
> 
> Provide more detailed xstate size information to help debug the issue.
> As a hypothetical example, on a platform that may report incorrect
> xstate size in CPUID.0xd.1:EBX, the diagnostic information after
> the warning will show:
> [    0.000000] x86/fpu: max_features=0x407
> [    0.000000] x86/fpu: xstate_offset[2]:  576, xstate_sizes[2]:  256
> [    0.000000] x86/fpu: xstate_offset[10]: 832, xstate_sizes[10]:    8
> [    0.000000] x86/fpu: total size: 840 bytes
> [    0.000000] x86/fpu: XCR0=0x7, IA32_XSS=0x400
> [    0.000000] x86/fpu: kernel_size from CPUID.0xd.0x1:EBX: 576 bytes
> 
> XCR0 | IA32_XSS is 0x407 which is consistent with max_features.
> CPUID.0xd.0x1:EBX should report the size of the xsave area
> containing xstate components corresponding to bits set in
> XCR0 | IA32_XSS. But it only reports 576 bytes of xsave area which
> doesn't count xstate sizes for AVX (offset 2 and 256 bytes) and
> PASID (offset 10 and 8 bytes). This confirms that the platform
> reports xstate size incorrectly through the CPUID bits.
> 
> Suggest-by: Dave Hansen <dave.hansen@linux.intel.com>
> Tested-by: Chintan M Patel <chintan.m.patel@intel.com>
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> ---
>   arch/x86/kernel/fpu/xstate.c | 33 +++++++++++++++++++++++++++++++--
>   1 file changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 0bab497c9436..5f27fcdc6c90 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -602,8 +602,37 @@ static bool __init paranoid_xstate_size_valid(unsigned int kernel_size)
>   		}
>   	}
>   	size = xstate_calculate_size(fpu_kernel_cfg.max_features, compacted);
> -	XSTATE_WARN_ON(size != kernel_size,
> -		       "size %u != kernel_size %u\n", size, kernel_size);
> +	if (size != kernel_size) {
> +		u64 xcr0, ia32_xss;
> +
> +		XSTATE_WARN_ON(1, "size %u != kernel_size %u\n",
> +			       size, kernel_size);
> +
> +		/* Show more information to help diagnose the size issue. */
> +		pr_info("x86/fpu: max_features=0x%llx\n",
> +			fpu_kernel_cfg.max_features);
> +		print_xstate_offset_size();
> +		pr_info("x86/fpu: total size: %u bytes\n", size);
> +		xcr0 = xgetbv(XCR_XFEATURE_ENABLED_MASK);
> +		if (compacted) {
> +			rdmsrl(MSR_IA32_XSS, ia32_xss);

This shouldn't be directly read here because of the LBR state component.

See the function comment:

  * Independent 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.

But, isn't that max_features bitmask pretty much about it?

> +			pr_info("x86/fpu: XCR0=0x%llx, IA32_XSS=0x%llx\n",
> +				xcr0, ia32_xss);
> +		} else {
> +			pr_info("x86/fpu: XCR0=0x%llx\n", xcr0);
> +		}
> +		/*
> +		 * In compact case, CPUID.0xd.0x1:EBX reports the size of
> +		 * the XSAVE size containing all the state components
> +		 * corresponding to bits set in XCR0 | IA32_XSS.
> +		 *
> +		 * Otherwise, CPUID.0xd.0x0:EBX reports the size of an XSAVE
> +		 * area containing all the *user* state components
> +		 * corresponding to bits set in XCR0.
> +		 */
> +		pr_info("x86/fpu: kernel_size from CPUID.0xd.0x%x:EBX: %u bytes\n",
> +			compacted ? 1 : 0, kernel_size);

Include Thiago who asked this.

Thanks,
Chang
Fenghua Yu April 10, 2023, 8:43 p.m. UTC | #2 
Hi, Chang,

On 4/7/23 11:22, Chang S. Bae wrote:
> On 4/5/2023 11:39 AM, Fenghua Yu wrote:
>> A warning is emitted when xstate sizes are found inconsistent.
>> But the warning message doesn't show enough information to diagnose
>> the issue.
>>
>> Provide more detailed xstate size information to help debug the issue.
>> As a hypothetical example, on a platform that may report incorrect
>> xstate size in CPUID.0xd.1:EBX, the diagnostic information after
>> the warning will show:
>> [    0.000000] x86/fpu: max_features=0x407
>> [    0.000000] x86/fpu: xstate_offset[2]:  576, xstate_sizes[2]:  256
>> [    0.000000] x86/fpu: xstate_offset[10]: 832, xstate_sizes[10]:    8
>> [    0.000000] x86/fpu: total size: 840 bytes
>> [    0.000000] x86/fpu: XCR0=0x7, IA32_XSS=0x400
>> [    0.000000] x86/fpu: kernel_size from CPUID.0xd.0x1:EBX: 576 bytes
>>
>> XCR0 | IA32_XSS is 0x407 which is consistent with max_features.
>> CPUID.0xd.0x1:EBX should report the size of the xsave area
>> containing xstate components corresponding to bits set in
>> XCR0 | IA32_XSS. But it only reports 576 bytes of xsave area which
>> doesn't count xstate sizes for AVX (offset 2 and 256 bytes) and
>> PASID (offset 10 and 8 bytes). This confirms that the platform
>> reports xstate size incorrectly through the CPUID bits.
>>
>> Suggest-by: Dave Hansen <dave.hansen@linux.intel.com>
>> Tested-by: Chintan M Patel <chintan.m.patel@intel.com>
>> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
>> ---
>>   arch/x86/kernel/fpu/xstate.c | 33 +++++++++++++++++++++++++++++++--
>>   1 file changed, 31 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
>> index 0bab497c9436..5f27fcdc6c90 100644
>> --- a/arch/x86/kernel/fpu/xstate.c
>> +++ b/arch/x86/kernel/fpu/xstate.c
>> @@ -602,8 +602,37 @@ static bool __init 
>> paranoid_xstate_size_valid(unsigned int kernel_size)
>>           }
>>       }
>>       size = xstate_calculate_size(fpu_kernel_cfg.max_features, 
>> compacted);
>> -    XSTATE_WARN_ON(size != kernel_size,
>> -               "size %u != kernel_size %u\n", size, kernel_size);
>> +    if (size != kernel_size) {
>> +        u64 xcr0, ia32_xss;
>> +
>> +        XSTATE_WARN_ON(1, "size %u != kernel_size %u\n",
>> +                   size, kernel_size);
>> +
>> +        /* Show more information to help diagnose the size issue. */
>> +        pr_info("x86/fpu: max_features=0x%llx\n",
>> +            fpu_kernel_cfg.max_features);
>> +        print_xstate_offset_size();
>> +        pr_info("x86/fpu: total size: %u bytes\n", size);
>> +        xcr0 = xgetbv(XCR_XFEATURE_ENABLED_MASK);
>> +        if (compacted) {
>> +            rdmsrl(MSR_IA32_XSS, ia32_xss);
> 
> This shouldn't be directly read here because of the LBR state component.
> 
> See the function comment:
> 
>   * Independent 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.
> 
> But, isn't that max_features bitmask pretty much about it?

How about getting IA32_XSS from xfeatures_mask_supervisor()? That's how 
to get kernel_size by setting IA32_XSS without independent features in 
get_xsave_compacted_size().

Thanks.

-Fenghua
Chang S. Bae April 11, 2023, 4:29 p.m. UTC | #3 
On 4/10/2023 1:43 PM, Fenghua Yu wrote:
> On 4/7/23 11:22, Chang S. Bae wrote:
>> On 4/5/2023 11:39 AM, Fenghua Yu wrote:
>>>
>>> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
>>> index 0bab497c9436..5f27fcdc6c90 100644
>>> --- a/arch/x86/kernel/fpu/xstate.c
>>> +++ b/arch/x86/kernel/fpu/xstate.c
>>> @@ -602,8 +602,37 @@ static bool __init 
>>> paranoid_xstate_size_valid(unsigned int kernel_size)
>>>           }
>>>       }
>>>       size = xstate_calculate_size(fpu_kernel_cfg.max_features, 
>>> compacted);
>>> -    XSTATE_WARN_ON(size != kernel_size,
>>> -               "size %u != kernel_size %u\n", size, kernel_size);
>>> +    if (size != kernel_size) {
>>> +        u64 xcr0, ia32_xss;
>>> +
>>> +        XSTATE_WARN_ON(1, "size %u != kernel_size %u\n",
>>> +                   size, kernel_size);
>>> +
>>> +        /* Show more information to help diagnose the size issue. */
>>> +        pr_info("x86/fpu: max_features=0x%llx\n",
>>> +            fpu_kernel_cfg.max_features);
>>> +        print_xstate_offset_size();
>>> +        pr_info("x86/fpu: total size: %u bytes\n", size);
>>> +        xcr0 = xgetbv(XCR_XFEATURE_ENABLED_MASK);
>>> +        if (compacted) {
>>> +            rdmsrl(MSR_IA32_XSS, ia32_xss);
>>
>> This shouldn't be directly read here because of the LBR state component.
>>
>> See the function comment:
>>
>>   * Independent 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.
>>
>> But, isn't that max_features bitmask pretty much about it?
> 
> How about getting IA32_XSS from xfeatures_mask_supervisor()? That's how 
> to get kernel_size by setting IA32_XSS without independent features in 
> get_xsave_compacted_size()
I think what it tests here is comparing the sizes between the kernel 
code and microcode calculations on the same input, which is the 
max_features bitmask.

We know that the kernel code calculates the size based on it and also 
takes it to write down there -- XCR0 and IA32_XSS. Then, showing that 
bitmask looks to be enough I thought, no?

I still expect some acknowledgment of what is coded here for the kernel 
calculation details.

Thanks,
Chang
Fenghua Yu April 12, 2023, 1:21 a.m. UTC | #4 
Hi, Chang,

On 4/11/23 09:29, Chang S. Bae wrote:
> On 4/10/2023 1:43 PM, Fenghua Yu wrote:
>> On 4/7/23 11:22, Chang S. Bae wrote:
>>> On 4/5/2023 11:39 AM, Fenghua Yu wrote:
>>>>
>>>> diff --git a/arch/x86/kernel/fpu/xstate.c 
>>>> b/arch/x86/kernel/fpu/xstate.c
>>>> index 0bab497c9436..5f27fcdc6c90 100644
>>>> --- a/arch/x86/kernel/fpu/xstate.c
>>>> +++ b/arch/x86/kernel/fpu/xstate.c
>>>> @@ -602,8 +602,37 @@ static bool __init 
>>>> paranoid_xstate_size_valid(unsigned int kernel_size)
>>>>           }
>>>>       }
>>>>       size = xstate_calculate_size(fpu_kernel_cfg.max_features, 
>>>> compacted);
>>>> -    XSTATE_WARN_ON(size != kernel_size,
>>>> -               "size %u != kernel_size %u\n", size, kernel_size);
>>>> +    if (size != kernel_size) {
>>>> +        u64 xcr0, ia32_xss;
>>>> +
>>>> +        XSTATE_WARN_ON(1, "size %u != kernel_size %u\n",
>>>> +                   size, kernel_size);
>>>> +
>>>> +        /* Show more information to help diagnose the size issue. */
>>>> +        pr_info("x86/fpu: max_features=0x%llx\n",
>>>> +            fpu_kernel_cfg.max_features);
>>>> +        print_xstate_offset_size();
>>>> +        pr_info("x86/fpu: total size: %u bytes\n", size);
>>>> +        xcr0 = xgetbv(XCR_XFEATURE_ENABLED_MASK);
>>>> +        if (compacted) {
>>>> +            rdmsrl(MSR_IA32_XSS, ia32_xss);
>>>
>>> This shouldn't be directly read here because of the LBR state component.
>>>
>>> See the function comment:
>>>
>>>   * Independent 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.
>>>
>>> But, isn't that max_features bitmask pretty much about it?
>>
>> How about getting IA32_XSS from xfeatures_mask_supervisor()? That's 
>> how to get kernel_size by setting IA32_XSS without independent 
>> features in get_xsave_compacted_size()
> I think what it tests here is comparing the sizes between the kernel 
> code and microcode calculations on the same input, which is the 
> max_features bitmask.
> 
> We know that the kernel code calculates the size based on it and also 
> takes it to write down there -- XCR0 and IA32_XSS. Then, showing that 
> bitmask looks to be enough I thought, no?

First of all, max_features is shown already.

Kernel_size from CPUID.0xd.0x1:EBX takes XCR0 | IA32_XSS as input. 
Platform may take wrong XCR0 or IA32_XSS and get wrong kernel_size. The 
purpose of this patch is to provide more debug info to help debug 
platform/kernel issue. So instead of a whole max_features, xgetbv() to 
get XCR0 and xfeatures_mask_supervisor() to get IA32_XSS provides more 
debug info in case platform may have issue in XCR0 or IA32_XSS.

In other words, splitting max_features into XCR0 and IA32_XSS and 
showing them individually provide more useful debug info than one single 
max_features value.

Does it make sense?

> 
> I still expect some acknowledgment of what is coded here for the kernel 
> calculation details.

The kernel calculation is shown in
+        print_xstate_offset_size();
+        pr_info("x86/fpu: total size: %u bytes\n", size);

Isn't that detailed enough to show offset and size of each xstate and 
sum of sizes?

After that,
+	pr_info("x86/fpu: kernel_size from CPUID.0xd.0x%x:EBX: %u bytes\n",
+               compacted ? 1 : 0, kernel_size);
shows how kernel_size is calculated from CPUID?

Using the above debug info, a real platform CPUID issue is shown clearly.

What other details are needed?

Thanks.

-Fenghua
Chang S. Bae April 12, 2023, 4:38 a.m. UTC | #5 
On 4/11/2023 6:21 PM, Fenghua Yu wrote:
> 
> First of all, max_features is shown already.

Yes.

> Kernel_size from CPUID.0xd.0x1:EBX takes XCR0 | IA32_XSS as input. 
> Platform may take wrong XCR0 or IA32_XSS and get wrong kernel_size. The  > purpose of this patch is to provide more debug info to help debug
> platform/kernel issue. So instead of a whole max_features, xgetbv() to 
> get XCR0 and xfeatures_mask_supervisor() to get IA32_XSS provides more 
> debug info in case platform may have issue in XCR0 or IA32_XSS.
> 
> In other words, splitting max_features into XCR0 and IA32_XSS and 
> showing them individually provide more useful debug info than one single 
> max_features value. >
> Does it make sense?

Hmm, I don't get it. I don't think whether the microcode takes those 
register values wrong or miscalculates the size does matter here.

print_xstate_offset_size() or something can decode the mask and readily 
shows off how it was calculated here. Then, probably that's it.

>> I still expect some acknowledgment of what is coded here for the 
>> kernel calculation details.
> 
> The kernel calculation is shown in
> +        print_xstate_offset_size();
> +        pr_info("x86/fpu: total size: %u bytes\n", size);
> 
> Isn't that detailed enough to show offset and size of each xstate and 
> sum of sizes?
> 
> After that,
> +    pr_info("x86/fpu: kernel_size from CPUID.0xd.0x%x:EBX: %u bytes\n",
> +               compacted ? 1 : 0, kernel_size);
> shows how kernel_size is calculated from CPUID?
> 
> Using the above debug info, a real platform CPUID issue is shown clearly.
> 
> What other details are needed?

I recall it was also asked to show which features are off or mismatched 
as compared to the CPU calculation. I'm not so sure about it.

Thanks,
Chang
Dave Hansen April 12, 2023, 2:35 p.m. UTC | #6 
On 4/11/23 18:21, Fenghua Yu wrote:
> In other words, splitting max_features into XCR0 and IA32_XSS and
> showing them individually provide more useful debug info than one single
> max_features value.
> 
> Does it make sense?

Not to me.

>> I still expect some acknowledgment of what is coded here for the
>> kernel calculation details.
> 
> The kernel calculation is shown in
> +        print_xstate_offset_size();
> +        pr_info("x86/fpu: total size: %u bytes\n", size);
> 
> Isn't that detailed enough to show offset and size of each xstate and
> sum of sizes?
> 
> After that,
> +    pr_info("x86/fpu: kernel_size from CPUID.0xd.0x%x:EBX: %u bytes\n",
> +               compacted ? 1 : 0, kernel_size);
> shows how kernel_size is calculated from CPUID?
> 
> Using the above debug info, a real platform CPUID issue is shown clearly.
> 
> What other details are needed?

I was kinda hoping this would be a simple, non-controversial patch that
would get us better debugging info the next time that the microcode or a
bad VMM screws up.  This patch isn't turning out to be as simple as I hoped.

I was wrong.  Let's just drop this.
diff mbox series

Patch

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 0bab497c9436..5f27fcdc6c90 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -602,8 +602,37 @@  static bool __init paranoid_xstate_size_valid(unsigned int kernel_size)
 		}
 	}
 	size = xstate_calculate_size(fpu_kernel_cfg.max_features, compacted);
-	XSTATE_WARN_ON(size != kernel_size,
-		       "size %u != kernel_size %u\n", size, kernel_size);
+	if (size != kernel_size) {
+		u64 xcr0, ia32_xss;
+
+		XSTATE_WARN_ON(1, "size %u != kernel_size %u\n",
+			       size, kernel_size);
+
+		/* Show more information to help diagnose the size issue. */
+		pr_info("x86/fpu: max_features=0x%llx\n",
+			fpu_kernel_cfg.max_features);
+		print_xstate_offset_size();
+		pr_info("x86/fpu: total size: %u bytes\n", size);
+		xcr0 = xgetbv(XCR_XFEATURE_ENABLED_MASK);
+		if (compacted) {
+			rdmsrl(MSR_IA32_XSS, ia32_xss);
+			pr_info("x86/fpu: XCR0=0x%llx, IA32_XSS=0x%llx\n",
+				xcr0, ia32_xss);
+		} else {
+			pr_info("x86/fpu: XCR0=0x%llx\n", xcr0);
+		}
+		/*
+		 * In compact case, CPUID.0xd.0x1:EBX reports the size of
+		 * the XSAVE size containing all the state components
+		 * corresponding to bits set in XCR0 | IA32_XSS.
+		 *
+		 * Otherwise, CPUID.0xd.0x0:EBX reports the size of an XSAVE
+		 * area containing all the *user* state components
+		 * corresponding to bits set in XCR0.
+		 */
+		pr_info("x86/fpu: kernel_size from CPUID.0xd.0x%x:EBX: %u bytes\n",
+			compacted ? 1 : 0, kernel_size);
+	}
 	return size == kernel_size;
 }