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

[Fenghua Yu]

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@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?

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