Re: [Xen-devel] [PATCH 2/2] x86/HVM: don't calculate XSTATE area sizes in software

[Wei Liu]

On Wed, Jun 01, 2016 at 09:50:10AM -0600, Jan Beulich wrote:
> >>> On 01.06.16 at 17:35,  wrote:
> > On 01/06/16 16:06, Jan Beulich wrote:
> >> @@ -3440,42 +3440,24 @@ void hvm_cpuid(unsigned int input, unsig
> >>  *eax = *ebx = *ecx = *edx = 0;
> >>  break;
> >>  }
> >> -/* EBX value of main leaf 0 depends on enabled xsave features */
> >> -if ( count == 0 && v->arch.xcr0 ) 
> >> -{
> >> -/* reset EBX to default value first */
> >> -*ebx = XSTATE_AREA_MIN_SIZE; 
> >> -for ( sub_leaf = 2; sub_leaf < 63; sub_leaf++ )
> >> -{
> >> -if ( !(v->arch.xcr0 & (1ULL << sub_leaf)) )
> >> -continue;
> >> -domain_cpuid(d, input, sub_leaf, &_eax, &_ebx, &_ecx, 
> >> - &_edx);
> >> -if ( (_eax + _ebx) > *ebx )
> >> -*ebx = _eax + _ebx;
> >> -}
> >> -}
> >> -
> >> -if ( count == 1 )
> >> +switch ( count )
> >>  {
> >> +case 1:
> >>  *eax &= hvm_featureset[FEATURESET_Da1];
> >> -
> >> -if ( *eax & cpufeat_mask(X86_FEATURE_XSAVES) )
> >> +if ( !(*eax & cpufeat_mask(X86_FEATURE_XSAVES)) )
> >>  {
> >> -uint64_t xfeatures = v->arch.xcr0 | 
> >> v->arch.hvm_vcpu.msr_xss;
> >> -
> >> -*ebx = XSTATE_AREA_MIN_SIZE;
> >> -if ( xfeatures & ~XSTATE_FP_SSE )
> >> -for ( sub_leaf = 2; sub_leaf < 63; sub_leaf++ )
> >> -if ( xfeatures & (1ULL << sub_leaf) )
> >> -{
> >> -if ( test_bit(sub_leaf, _align) )
> >> -*ebx = ROUNDUP(*ebx, 64);
> >> -*ebx += xstate_sizes[sub_leaf];
> >> -}
> >> -}
> >> -else
> >>  *ebx = *ecx = *edx = 0;
> >> +break;
> >> +}
> >> +/* fall through */
> >> +case 0:
> >> +/*
> >> + * Always read CPUID.0xD[ECX=0/1].EBX from hardware, rather 
> >> than
> >> + * domain policy.  It varies with enabled xstate, and the 
> >> correct
> >> + * xcr0/xss are in context.
> >> + */
> >> +cpuid_count(input, count, , ebx, , );
> >> +break;
> > 
> > It would be helpful for my PKU bugfix if you could avoid collapsing this
> > into a fallthough, as the fallthough would need to be undone. 
> > Otherwise, Reviewed-by: Andrew Cooper 
> 
> Converting this back is easy to do, but I'll nevertheless wait for
> Wei's opinion re 4.7 inclusion, as otherwise I'll eventually need to
> rebase on top of yours anyway.
> 

I think this is fine for 4.7. And I will leave it to you two to
coordinate the rest.

Wei.

> Jan
> 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 2/2] x86/HVM: don't calculate XSTATE area sizes in software

[Jan Beulich]

>>> On 01.06.16 at 17:35,  wrote:
> On 01/06/16 16:06, Jan Beulich wrote:
>> @@ -3440,42 +3440,24 @@ void hvm_cpuid(unsigned int input, unsig
>>  *eax = *ebx = *ecx = *edx = 0;
>>  break;
>>  }
>> -/* EBX value of main leaf 0 depends on enabled xsave features */
>> -if ( count == 0 && v->arch.xcr0 ) 
>> -{
>> -/* reset EBX to default value first */
>> -*ebx = XSTATE_AREA_MIN_SIZE; 
>> -for ( sub_leaf = 2; sub_leaf < 63; sub_leaf++ )
>> -{
>> -if ( !(v->arch.xcr0 & (1ULL << sub_leaf)) )
>> -continue;
>> -domain_cpuid(d, input, sub_leaf, &_eax, &_ebx, &_ecx, 
>> - &_edx);
>> -if ( (_eax + _ebx) > *ebx )
>> -*ebx = _eax + _ebx;
>> -}
>> -}
>> -
>> -if ( count == 1 )
>> +switch ( count )
>>  {
>> +case 1:
>>  *eax &= hvm_featureset[FEATURESET_Da1];
>> -
>> -if ( *eax & cpufeat_mask(X86_FEATURE_XSAVES) )
>> +if ( !(*eax & cpufeat_mask(X86_FEATURE_XSAVES)) )
>>  {
>> -uint64_t xfeatures = v->arch.xcr0 | 
>> v->arch.hvm_vcpu.msr_xss;
>> -
>> -*ebx = XSTATE_AREA_MIN_SIZE;
>> -if ( xfeatures & ~XSTATE_FP_SSE )
>> -for ( sub_leaf = 2; sub_leaf < 63; sub_leaf++ )
>> -if ( xfeatures & (1ULL << sub_leaf) )
>> -{
>> -if ( test_bit(sub_leaf, _align) )
>> -*ebx = ROUNDUP(*ebx, 64);
>> -*ebx += xstate_sizes[sub_leaf];
>> -}
>> -}
>> -else
>>  *ebx = *ecx = *edx = 0;
>> +break;
>> +}
>> +/* fall through */
>> +case 0:
>> +/*
>> + * Always read CPUID.0xD[ECX=0/1].EBX from hardware, rather than
>> + * domain policy.  It varies with enabled xstate, and the 
>> correct
>> + * xcr0/xss are in context.
>> + */
>> +cpuid_count(input, count, , ebx, , );
>> +break;
> 
> It would be helpful for my PKU bugfix if you could avoid collapsing this
> into a fallthough, as the fallthough would need to be undone. 
> Otherwise, Reviewed-by: Andrew Cooper 

Converting this back is easy to do, but I'll nevertheless wait for
Wei's opinion re 4.7 inclusion, as otherwise I'll eventually need to
rebase on top of yours anyway.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 2/2] x86/HVM: don't calculate XSTATE area sizes in software

[Wei Liu]

On Wed, Jun 01, 2016 at 04:35:44PM +0100, Andrew Cooper wrote:
> On 01/06/16 16:06, Jan Beulich wrote:
> > @@ -3440,42 +3440,24 @@ void hvm_cpuid(unsigned int input, unsig
> >  *eax = *ebx = *ecx = *edx = 0;
> >  break;
> >  }
> > -/* EBX value of main leaf 0 depends on enabled xsave features */
> > -if ( count == 0 && v->arch.xcr0 ) 
> > -{
> > -/* reset EBX to default value first */
> > -*ebx = XSTATE_AREA_MIN_SIZE; 
> > -for ( sub_leaf = 2; sub_leaf < 63; sub_leaf++ )
> > -{
> > -if ( !(v->arch.xcr0 & (1ULL << sub_leaf)) )
> > -continue;
> > -domain_cpuid(d, input, sub_leaf, &_eax, &_ebx, &_ecx, 
> > - &_edx);
> > -if ( (_eax + _ebx) > *ebx )
> > -*ebx = _eax + _ebx;
> > -}
> > -}
> > -
> > -if ( count == 1 )
> > +switch ( count )
> >  {
> > +case 1:
> >  *eax &= hvm_featureset[FEATURESET_Da1];
> > -
> > -if ( *eax & cpufeat_mask(X86_FEATURE_XSAVES) )
> > +if ( !(*eax & cpufeat_mask(X86_FEATURE_XSAVES)) )
> >  {
> > -uint64_t xfeatures = v->arch.xcr0 | 
> > v->arch.hvm_vcpu.msr_xss;
> > -
> > -*ebx = XSTATE_AREA_MIN_SIZE;
> > -if ( xfeatures & ~XSTATE_FP_SSE )
> > -for ( sub_leaf = 2; sub_leaf < 63; sub_leaf++ )
> > -if ( xfeatures & (1ULL << sub_leaf) )
> > -{
> > -if ( test_bit(sub_leaf, _align) )
> > -*ebx = ROUNDUP(*ebx, 64);
> > -*ebx += xstate_sizes[sub_leaf];
> > -}
> > -}
> > -else
> >  *ebx = *ecx = *edx = 0;
> > +break;
> > +}
> > +/* fall through */
> > +case 0:
> > +/*
> > + * Always read CPUID.0xD[ECX=0/1].EBX from hardware, rather 
> > than
> > + * domain policy.  It varies with enabled xstate, and the 
> > correct
> > + * xcr0/xss are in context.
> > + */
> > +cpuid_count(input, count, , ebx, , );
> > +break;
> 
> It would be helpful for my PKU bugfix if you could avoid collapsing this
> into a fallthough, as the fallthough would need to be undone. 
> Otherwise, Reviewed-by: Andrew Cooper 
> 

Release-acked-by: Wei Liu 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 2/2] x86/HVM: don't calculate XSTATE area sizes in software

[Andrew Cooper]

On 01/06/16 16:06, Jan Beulich wrote:
> @@ -3440,42 +3440,24 @@ void hvm_cpuid(unsigned int input, unsig
>  *eax = *ebx = *ecx = *edx = 0;
>  break;
>  }
> -/* EBX value of main leaf 0 depends on enabled xsave features */
> -if ( count == 0 && v->arch.xcr0 ) 
> -{
> -/* reset EBX to default value first */
> -*ebx = XSTATE_AREA_MIN_SIZE; 
> -for ( sub_leaf = 2; sub_leaf < 63; sub_leaf++ )
> -{
> -if ( !(v->arch.xcr0 & (1ULL << sub_leaf)) )
> -continue;
> -domain_cpuid(d, input, sub_leaf, &_eax, &_ebx, &_ecx, 
> - &_edx);
> -if ( (_eax + _ebx) > *ebx )
> -*ebx = _eax + _ebx;
> -}
> -}
> -
> -if ( count == 1 )
> +switch ( count )
>  {
> +case 1:
>  *eax &= hvm_featureset[FEATURESET_Da1];
> -
> -if ( *eax & cpufeat_mask(X86_FEATURE_XSAVES) )
> +if ( !(*eax & cpufeat_mask(X86_FEATURE_XSAVES)) )
>  {
> -uint64_t xfeatures = v->arch.xcr0 | v->arch.hvm_vcpu.msr_xss;
> -
> -*ebx = XSTATE_AREA_MIN_SIZE;
> -if ( xfeatures & ~XSTATE_FP_SSE )
> -for ( sub_leaf = 2; sub_leaf < 63; sub_leaf++ )
> -if ( xfeatures & (1ULL << sub_leaf) )
> -{
> -if ( test_bit(sub_leaf, _align) )
> -*ebx = ROUNDUP(*ebx, 64);
> -*ebx += xstate_sizes[sub_leaf];
> -}
> -}
> -else
>  *ebx = *ecx = *edx = 0;
> +break;
> +}
> +/* fall through */
> +case 0:
> +/*
> + * Always read CPUID.0xD[ECX=0/1].EBX from hardware, rather than
> + * domain policy.  It varies with enabled xstate, and the correct
> + * xcr0/xss are in context.
> + */
> +cpuid_count(input, count, , ebx, , );
> +break;

It would be helpful for my PKU bugfix if you could avoid collapsing this
into a fallthough, as the fallthough would need to be undone. 
Otherwise, Reviewed-by: Andrew Cooper 

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 2/2] x86/HVM: don't calculate XSTATE area sizes in software

[Jan Beulich]

Use hardware output instead, brining HVM behavior in line with PV one
in this regard.

Signed-off-by: Jan Beulich 

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3362,7 +3362,7 @@ void hvm_cpuid(unsigned int input, unsig
 
 switch ( input )
 {
-unsigned int sub_leaf, _eax, _ebx, _ecx, _edx;
+unsigned int _ecx, _edx;
 
 case 0x1:
 /* Fix up VLAPIC details. */
@@ -3440,42 +3440,24 @@ void hvm_cpuid(unsigned int input, unsig
 *eax = *ebx = *ecx = *edx = 0;
 break;
 }
-/* EBX value of main leaf 0 depends on enabled xsave features */
-if ( count == 0 && v->arch.xcr0 ) 
-{
-/* reset EBX to default value first */
-*ebx = XSTATE_AREA_MIN_SIZE; 
-for ( sub_leaf = 2; sub_leaf < 63; sub_leaf++ )
-{
-if ( !(v->arch.xcr0 & (1ULL << sub_leaf)) )
-continue;
-domain_cpuid(d, input, sub_leaf, &_eax, &_ebx, &_ecx, 
- &_edx);
-if ( (_eax + _ebx) > *ebx )
-*ebx = _eax + _ebx;
-}
-}
-
-if ( count == 1 )
+switch ( count )
 {
+case 1:
 *eax &= hvm_featureset[FEATURESET_Da1];
-
-if ( *eax & cpufeat_mask(X86_FEATURE_XSAVES) )
+if ( !(*eax & cpufeat_mask(X86_FEATURE_XSAVES)) )
 {
-uint64_t xfeatures = v->arch.xcr0 | v->arch.hvm_vcpu.msr_xss;
-
-*ebx = XSTATE_AREA_MIN_SIZE;
-if ( xfeatures & ~XSTATE_FP_SSE )
-for ( sub_leaf = 2; sub_leaf < 63; sub_leaf++ )
-if ( xfeatures & (1ULL << sub_leaf) )
-{
-if ( test_bit(sub_leaf, _align) )
-*ebx = ROUNDUP(*ebx, 64);
-*ebx += xstate_sizes[sub_leaf];
-}
-}
-else
 *ebx = *ecx = *edx = 0;
+break;
+}
+/* fall through */
+case 0:
+/*
+ * Always read CPUID.0xD[ECX=0/1].EBX from hardware, rather than
+ * domain policy.  It varies with enabled xstate, and the correct
+ * xcr0/xss are in context.
+ */
+cpuid_count(input, count, , ebx, , );
+break;
 }
 break;
 



x86/HVM: don't calculate XSTATE area sizes in software

Use hardware output instead, brining HVM behavior in line with PV one
in this regard.

Signed-off-by: Jan Beulich 

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3362,7 +3362,7 @@ void hvm_cpuid(unsigned int input, unsig
 
 switch ( input )
 {
-unsigned int sub_leaf, _eax, _ebx, _ecx, _edx;
+unsigned int _ecx, _edx;
 
 case 0x1:
 /* Fix up VLAPIC details. */
@@ -3440,42 +3440,24 @@ void hvm_cpuid(unsigned int input, unsig
 *eax = *ebx = *ecx = *edx = 0;
 break;
 }
-/* EBX value of main leaf 0 depends on enabled xsave features */
-if ( count == 0 && v->arch.xcr0 ) 
-{
-/* reset EBX to default value first */
-*ebx = XSTATE_AREA_MIN_SIZE; 
-for ( sub_leaf = 2; sub_leaf < 63; sub_leaf++ )
-{
-if ( !(v->arch.xcr0 & (1ULL << sub_leaf)) )
-continue;
-domain_cpuid(d, input, sub_leaf, &_eax, &_ebx, &_ecx, 
- &_edx);
-if ( (_eax + _ebx) > *ebx )
-*ebx = _eax + _ebx;
-}
-}
-
-if ( count == 1 )
+switch ( count )
 {
+case 1:
 *eax &= hvm_featureset[FEATURESET_Da1];
-
-if ( *eax & cpufeat_mask(X86_FEATURE_XSAVES) )
+if ( !(*eax & cpufeat_mask(X86_FEATURE_XSAVES)) )
 {
-uint64_t xfeatures = v->arch.xcr0 | v->arch.hvm_vcpu.msr_xss;
-
-*ebx = XSTATE_AREA_MIN_SIZE;
-if ( xfeatures & ~XSTATE_FP_SSE )
-for ( sub_leaf = 2; sub_leaf < 63; sub_leaf++ )
-if ( xfeatures & (1ULL << sub_leaf) )
-{
-if ( test_bit(sub_leaf, _align) )
-*ebx = ROUNDUP(*ebx, 64);
-*ebx += xstate_sizes[sub_leaf];
-}
-}
-else
 *ebx = *ecx = *edx = 0;
+break;
+}
+/* fall through */
+case 0:
+/*
+ * Always read CPUID.0xD[ECX=0/1].EBX from hardware, rather than
+ * domain policy.  It varies with enabled xstate, and the correct
+ * xcr0/xss are in context.
+ */
+cpuid_count(input,