| Message ID | 574F15F402000078000F07F9@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/06/16 16:05, Jan Beulich wrote: > In line with other recent changes, these should be fully white listed, > requiring us to zero them until the obtain a meaning we support. > > Without XSAVE support, all xstate sub-leaves should be zero. > > Also move away from checking host XSAVE support - we really ought to > consider the guest flag for that purpose. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, with one suggestion > > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -3433,7 +3433,13 @@ void hvm_cpuid(unsigned int input, unsig > *edx = v->vcpu_id * 2; > break; > > - case 0xd: > + case XSTATE_CPUID: > + hvm_cpuid(1, NULL, NULL, &_ecx, NULL); > + if ( !(_ecx & cpufeat_mask(X86_FEATURE_XSAVE)) || count >= 63 ) > + { > + *eax = *ebx = *ecx = *edx = 0; > + break; > + } > /* EBX value of main leaf 0 depends on enabled xsave features */ > if ( count == 0 && v->arch.xcr0 ) > { > --- a/xen/arch/x86/traps.c > +++ b/xen/arch/x86/traps.c > @@ -928,6 +928,8 @@ void pv_cpuid(struct cpu_user_regs *regs > > switch ( leaf ) > { > + uint32_t tmp; > + > case 0x00000001: > c &= pv_featureset[FEATURESET_1c]; > d &= pv_featureset[FEATURESET_1d]; > @@ -1085,14 +1087,19 @@ void pv_cpuid(struct cpu_user_regs *regs > break; > > case XSTATE_CPUID: > - if ( !cpu_has_xsave ) > + if ( !((!is_control_domain(currd) && !is_hardware_domain(currd) I would recommend extra brackets on this line, to avoid the possible mis-interpretation of !is_control_domain(currd) && (!is_hardware_domain(currd) ? ... > + ? ({ > + uint32_t ecx; > + > + domain_cpuid(currd, 1, 0, &tmp, &tmp, &ecx, &tmp); > + ecx & pv_featureset[FEATURESET_1c]; > + }) > + : cpuid_ecx(1)) & cpufeat_mask(X86_FEATURE_XSAVE)) || > + subleaf >= 63 ) This is rather nasty code. I am glad that my longterm plans involve removing it all. ~Andrew
On Wed, Jun 01, 2016 at 09:05:56AM -0600, Jan Beulich wrote: > In line with other recent changes, these should be fully white listed, > requiring us to zero them until the obtain a meaning we support. > "they obtain ..." > Without XSAVE support, all xstate sub-leaves should be zero. > > Also move away from checking host XSAVE support - we really ought to > consider the guest flag for that purpose. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Release-acked-by: Wei Liu <wei.liu2@citrix.com>
--- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -3433,7 +3433,13 @@ void hvm_cpuid(unsigned int input, unsig *edx = v->vcpu_id * 2; break; - case 0xd: + case XSTATE_CPUID: + hvm_cpuid(1, NULL, NULL, &_ecx, NULL); + if ( !(_ecx & cpufeat_mask(X86_FEATURE_XSAVE)) || count >= 63 ) + { + *eax = *ebx = *ecx = *edx = 0; + break; + } /* EBX value of main leaf 0 depends on enabled xsave features */ if ( count == 0 && v->arch.xcr0 ) { --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -928,6 +928,8 @@ void pv_cpuid(struct cpu_user_regs *regs switch ( leaf ) { + uint32_t tmp; + case 0x00000001: c &= pv_featureset[FEATURESET_1c]; d &= pv_featureset[FEATURESET_1d]; @@ -1085,14 +1087,19 @@ void pv_cpuid(struct cpu_user_regs *regs break; case XSTATE_CPUID: - if ( !cpu_has_xsave ) + if ( !((!is_control_domain(currd) && !is_hardware_domain(currd) + ? ({ + uint32_t ecx; + + domain_cpuid(currd, 1, 0, &tmp, &tmp, &ecx, &tmp); + ecx & pv_featureset[FEATURESET_1c]; + }) + : cpuid_ecx(1)) & cpufeat_mask(X86_FEATURE_XSAVE)) || + subleaf >= 63 ) goto unsupported; switch ( subleaf ) { case 0: - { - uint32_t tmp; - /* * Always read CPUID.0xD[ECX=0].EBX from hardware, rather than * domain policy. It varies with enabled xstate, and the correct @@ -1101,7 +1108,6 @@ void pv_cpuid(struct cpu_user_regs *regs if ( !is_control_domain(currd) && !is_hardware_domain(currd) ) cpuid_count(leaf, subleaf, &tmp, &b, &tmp, &tmp); break; - } case 1: a &= pv_featureset[FEATURESET_Da1];