Re: [PATCH] x86/fpu: Warn only when CPU-provided sizes less than struct declaration

[Suravee Suthikulpanit]

Dave,

On 12/12/19 1:52 PM, Suravee Suthikulpanit wrote:
> OnÂ12/11/2019Â9:13ÂPM,ÂDaveÂHansenÂwrote:
> > OnÂ12/10/19Â9:24ÂPM,ÂSuraveeÂSuthikulpanitÂwrote:
> > > TheÂvalueÂreturnedÂbyÂECX[1]ÂindicatesÂtheÂalignmentÂofÂstate
> > > componentÂiÂwhenÂtheÂcompactedÂformatÂofÂtheÂextendedÂregionÂofÂan
> > > XSAVEÂareaÂisÂusedÂ(seeÂSectionÂ13.4.3).ÂIfÂECX[1]ÂreturnsÂ0,Âstate
> > > componentÂiÂisÂlocatedÂimmediatelyÂfollowingÂtheÂprecedingÂstate
> > > component;ÂifÂECX[1]ÂreturnsÂ1,ÂstateÂcomponentÂiÂisÂlocatedÂonÂthe
> > > nextÂ64-byteÂboundaryÂfollowingÂtheÂprecedingÂstateÂcomponent.
> >
> > Essentially,ÂifÂanÂimplementationÂneedsÂstateÂalignmentÂorÂ(upÂto)Â64
> > bytesÂofÂpadding,ÂitÂcouldÂuseÂthisÂexistingÂarchitectureÂforÂit.
>
> LetÂmeÂcheckÂwithÂtheÂHWÂfolksÂandÂgetÂbackÂtoÂyouÂonÂthis.

I don't think the 64-byte aligned bit alone would help with the PKRU size mismatch warning in 
check_xstate_against_struct() that we are seeing.

Here is the description from the section 13.4.3, for the compacted format:

* Let j, 2 â j < i, be the greatest value such that XCOMP_BV[j] = 1. Then locationI is
determined by the following values: locationJ; sizeJ, as enumerated in CPUID.(EAX=0DH,ECX=j):EAX;
and the value of alignI, as enumerated in CPUID.(EAX=0DH,ECX=i):ECX[1]:

  â If alignI = 0, locationI = locationJ + sizeJ. (This item implies that state component i is located immediately 
following the preceding state component whose bit is set in XCOMP_BV.)

  â If alignI = 1, locationI = ceiling(locationJ + sizeJ, 64). (This item implies that state component i is located on 
the next 64-byte boundary following the preceding state component whose bit is set in XCOMP_BV.)

According to the description above, since we need to add padding for PKRU state, if we set the aligned bit of the 
subsequent feature after PKRU state, the current Linux kernel would still generate the warning for PKRU since the 
check_xstate_against_struct() does not currently take into account of the align bit (which is only for compact mode).

It would also generate another warning in do_extra_xstate_size_checks() due to XSTATE_WARN_ON(paranoid_xstate_size != 
fpu_kernel_xstate_size) (see https://elixir.bootlin.com/linux/latest/source/arch/x86/kernel/fpu/xstate.c#L608). Note 
that the paranoid_xstate_size takes into account of the 64-byte alignment in the size calculation, but the 
fpu_kernel_xstate_size takes the size reported by CPUID function 0DH, sub-function 0 or 1. So, this logic might need to 
change as well.

As for the PKRU size, it should really be 4 bytes (regardless of the alignment) since that's the size of actual PKRU 
register, but it seems that the 4-byte padding in struct pkru_state was added to match the Intel's CPUID-report size of 
8. Now we also have AMD hardware reporting PKRU size of 32 (also mainly for padding). We shouldn't keep the same warning 
logic in check_xstate_against_struct().

Please let me know if I am still missing anything.

Thanks,
Suravee