Re: [PATCH] x86/CPUID: surface suitable value in EBX of XSTATE subleaf 1

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.08.2022 10:59, Andrew Cooper wrote:
> On 23/08/2022 07:42, Jan Beulich wrote:
>> While the SDM isn't very clear about this, our present behavior make
>> Linux 5.19 unhappy. As of commit 8ad7e8f69695 ("x86/fpu/xsave: Support
>> XSAVEC in the kernel") they're using this CPUID output also to size
>> the compacted area used by XSAVEC. Getting back zero there isn't really
>> liked, yet fpr PV that's the default on capable hardware: XSAVES isn't
> 
> for.
> 
>> exposed to PV domains.
>>
>> Considering that the size reported is that of the compacted save area,
>> I view Linux'es assumption as appropriate (short of the SDM properly
>> considering the case). Therefore we need to populate the field also when
>> only XSAVEC is supported for a guest.
> 
> This is a mess.  The SDM is fairly clear (but only in Vol1) that this
> leaf is specific to XSAVES.

The way it's written my assumption is that they simply didn't care about
XSAVEC when writing this, or they were assuming that both features would
always be supported together (yet even if they are in Intel's hardware,
the architecture should spell out things as if both were entirely
independent, or it should specify that one takes the other as a prereq).

>  The APM has only an equation, which shows
> it as the compacted size without reference to instructions.
> 
> Ideally I'd like the opinion from some architects and a clarification to
> the SDM...

I've made a request through my contact, but of course there's little
hope for a quick technical reply.

>> Fixes: 460b9a4b3630 ("x86/xsaves: enable xsaves/xrstors for hvm guest")
>> Fixes: 8d050ed1097c ("x86: don't expose XSAVES capability to PV guests")
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> CC Marek.  Looks like Jan has found the issue you reported on IRC.
> 
> Jan: Be aware that I submitted
> https://lore.kernel.org/lkml/20220810221909.12768-1-andrew.cooper3@xxxxxxxxxx/
> to Linux to correct some of the diagnostics.
> 
>> ---
>> I actually wonder why we surface the XSAVES feature bit to HVM domains,
>> when we don't support any of the features.
> 
> Because that's what was originally accepted into Xen, and I couldn't
> retract it when fixing CPUID handling at first because it would regress
> across migrate to a newer Xen.  With CPUID data now in the migration
> stream, we could in principle fix it, but at this point it's definitely
> not worth the complexity or risk to adjust.

Hmm.

>> --- a/xen/arch/x86/cpuid.c
>> +++ b/xen/arch/x86/cpuid.c
>> @@ -1142,7 +1142,7 @@ void guest_cpuid(const struct vcpu *v, u
>>          switch ( subleaf )
>>          {
>>          case 1:
>> -            if ( p->xstate.xsaves )
>> +            if ( p->xstate.xsavec || p->xstate.xsaves )
> 
> If we're doing this, then it wants to be xsavec only, with the comment
> being extended to explain why.

Why would that be? Both insns use compacted format, and neither is
dependent upon the other in terms of being supported. IOW XSAVES alone
and XSAVEC alone enabled for a domain should still lead through this
path.

> But this is going to further complicate my several-year-old series
> trying to get Xen's XSTATE handling into a position where we can start
> to offer supervisor states.

Where do you see further complication? The necessary fiddling with XSS
here would of course be dependent upon p->xstate.xsaves alone (or,
maybe better, on the set of enabled features in XSS being non-empty),
but that's simply another (inner) if().

As an aside, I actually wonder what use the supplied size is to user
mode code when any XSS-controlled feature is enabled: They'd allocate
a needlessly large block of memory, as they would only be able to use
XSAVEC.

Jan