| Message ID | 574F161802000078000F07FD@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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, &xstate_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, &dummy, ebx, &dummy, &dummy); > + 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.cooper3@citrix.com> ~Andrew
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, &xstate_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, &dummy, ebx, &dummy, &dummy); > > + 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.cooper3@citrix.com> > Release-acked-by: Wei Liu <wei.liu2@citrix.com>
>>> On 01.06.16 at 17:35, <andrew.cooper3@citrix.com> 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, &xstate_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, &dummy, ebx, &dummy, &dummy); >> + 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.cooper3@citrix.com> 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
On Wed, Jun 01, 2016 at 09:50:10AM -0600, Jan Beulich wrote: > >>> On 01.06.16 at 17:35, <andrew.cooper3@citrix.com> 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, &xstate_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, &dummy, ebx, &dummy, &dummy); > >> + 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.cooper3@citrix.com> > > 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 >
--- 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, &xstate_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, &dummy, ebx, &dummy, &dummy); + break; } break;