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

["Chang S. Bae" <chang.seok.bae@intel.com>]

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