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

[Chang S. Bae]

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@xxxxxxxxxxxxxxx>
> Tested-by: Chintan M Patel <chintan.m.patel@xxxxxxxxx>
> Signed-off-by: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> ---
>   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