| Message ID | 1459414657-7558-4-git-send-email-shuai.ruan@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 31.03.16 at 10:57, <shuai.ruan@linux.intel.com> wrote: > Refer to SDM 13.4.3 Extended Region of an XSAVE Area. The value return No section numbers please - they tend to change. > by ecx[1] with cpuid function 0xdh and sub-fucntion i (i>1) indicates Either "0xd" or "0dh". And "function". > --- a/xen/arch/x86/traps.c > +++ b/xen/arch/x86/traps.c > @@ -1020,6 +1020,18 @@ void pv_cpuid(struct cpu_user_regs *regs) > a &= (boot_cpu_data.x86_capability[cpufeat_word(X86_FEATURE_XSAVEOPT)] & > ~cpufeat_mask(X86_FEATURE_XSAVES)); > b = c = d = 0; > + if ( cpu_has_xsavec ) > + { > + b = XSTATE_AREA_MIN_SIZE; Is this really correct namely when curr->arch.xcr0 == 0? If not, the if() below should perhaps be combined with the if() above (and then the same would apply to hvm_cpuid()). > + if ( curr->arch.xcr0 ) > + for( subleaf = 2; subleaf < 63; subleaf++ ) > + if ( (1ULL << subleaf) & curr->arch.xcr0 ) The first if() is redundant with this second one. If you really mean to avoid the loop, then please also only check bits 2..62 in the first if(). Jan
On Tue, Apr 05, 2016 at 02:31:40AM -0600, Jan Beulich wrote: > >>> On 31.03.16 at 10:57, <shuai.ruan@linux.intel.com> wrote: > > Refer to SDM 13.4.3 Extended Region of an XSAVE Area. The value return > > No section numbers please - they tend to change. > > > by ecx[1] with cpuid function 0xdh and sub-fucntion i (i>1) indicates > > Either "0xd" or "0dh". And "function". > > > --- a/xen/arch/x86/traps.c > > +++ b/xen/arch/x86/traps.c > > @@ -1020,6 +1020,18 @@ void pv_cpuid(struct cpu_user_regs *regs) > > a &= (boot_cpu_data.x86_capability[cpufeat_word(X86_FEATURE_XSAVEOPT)] & > > ~cpufeat_mask(X86_FEATURE_XSAVES)); > > b = c = d = 0; > > + if ( cpu_has_xsavec ) > > + { > > + b = XSTATE_AREA_MIN_SIZE; > > Is this really correct namely when curr->arch.xcr0 == 0? If not, the > if() below should perhaps be combined with the if() above (and then > the same would apply to hvm_cpuid()). > > > + if ( curr->arch.xcr0 ) > > + for( subleaf = 2; subleaf < 63; subleaf++ ) > > + if ( (1ULL << subleaf) & curr->arch.xcr0 ) > > The first if() is redundant with this second one. If you really > mean to avoid the loop, then please also only check bits > 2..62 in the first if(). > Ok for all comments above. Another question is whether we should add this in pv_cpuid() or not. (which we have discussed in the previous thread). Refer to SDM Volume 1 "13.2 ENUMERATION OF CPU SUPPORT FOR XSAVE INSTRUCTIONS AND XSAVE- SUPPORTED FEATURES" — CPUID function 0DH, sub-function 1. ... "EBX enumerates the size (in bytes) required by the XSAVES instruction for an XSAVE area containing all the state components corresponding to bits currently set in XCR0 | IA32_XSS." From the descriptions above, EBX only be used when XSAVES is enabled. So I think we should not deal with pv_cpuid() here. Any comments ? > Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
>>> Shuai Ruan <shuai.ruan@linux.intel.com> 04/06/16 8:59 AM >>> >Another question is whether we should add this in pv_cpuid() or not. >(which we have discussed in the previous thread). > >Refer to SDM Volume 1 >"13.2 ENUMERATION OF CPU SUPPORT FOR XSAVE INSTRUCTIONS AND XSAVE- >SUPPORTED FEATURES" >— CPUID function 0DH, sub-function 1. >... >"EBX enumerates the size (in bytes) required by the XSAVES instruction >for an XSAVE area containing all >the state components corresponding to bits currently set in XCR0 | >IA32_XSS." > >From the descriptions above, EBX only be used when XSAVES is enabled. >So I think we should not deal with pv_cpuid() here. If it's mandated to be XSAVES only - yes. But why are you asking such a question? It was you who proposed the change to pv_cpuid(), and it is you who should be understanding the specifics of the various pieces of CPUID output related to XSAVE better than me. Jan
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 5aef3cb..c6cd4fb 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -4743,14 +4743,18 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx, } if ( count == 1 ) { - if ( cpu_has_xsaves && cpu_has_vmx_xsaves ) + if ( (cpu_has_xsaves && cpu_has_vmx_xsaves) || cpu_has_xsavec ) { *ebx = XSTATE_AREA_MIN_SIZE; if ( v->arch.xcr0 | v->arch.hvm_vcpu.msr_xss ) for ( sub_leaf = 2; sub_leaf < 63; sub_leaf++ ) if ( (v->arch.xcr0 | v->arch.hvm_vcpu.msr_xss) & (1ULL << sub_leaf) ) + { + if ( test_bit(sub_leaf, &xstate_align) ) + *ebx = ROUNDUP(*ebx, 64); *ebx += xstate_sizes[sub_leaf]; + } } else *ebx = *ecx = *edx = 0; diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c index 6fbb1cf..8694da6 100644 --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -1020,6 +1020,18 @@ void pv_cpuid(struct cpu_user_regs *regs) a &= (boot_cpu_data.x86_capability[cpufeat_word(X86_FEATURE_XSAVEOPT)] & ~cpufeat_mask(X86_FEATURE_XSAVES)); b = c = d = 0; + if ( cpu_has_xsavec ) + { + b = XSTATE_AREA_MIN_SIZE; + if ( curr->arch.xcr0 ) + for( subleaf = 2; subleaf < 63; subleaf++ ) + if ( (1ULL << subleaf) & curr->arch.xcr0 ) + { + if ( test_bit(subleaf, &xstate_align) ) + b = ROUNDUP(b, 64); + b += xstate_sizes[subleaf]; + } + } break; } break; diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c index f4ea54d..850b778 100644 --- a/xen/arch/x86/xstate.c +++ b/xen/arch/x86/xstate.c @@ -26,7 +26,7 @@ u64 __read_mostly xfeature_mask; static unsigned int *__read_mostly xstate_offsets; unsigned int *__read_mostly xstate_sizes; -static u64 __read_mostly xstate_align; +u64 __read_mostly xstate_align; static unsigned int __read_mostly xstate_features; static uint32_t __read_mostly mxcsr_mask = 0x0000ffbf; diff --git a/xen/include/asm-x86/xstate.h b/xen/include/asm-x86/xstate.h index 91d1c39..535443a 100644 --- a/xen/include/asm-x86/xstate.h +++ b/xen/include/asm-x86/xstate.h @@ -50,6 +50,7 @@ #define XSTATE_ALIGN64 (1U << 1) extern u64 xfeature_mask; +extern u64 xstate_align; extern unsigned int *xstate_sizes; /* extended state save area */