| Message ID | 1466765107-29077-1-git-send-email-kai.huang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> From: kaih.linux@gmail.com [mailto:kaih.linux@gmail.com] > Sent: Friday, June 24, 2016 6:45 PM > > From: Kai Huang <kai.huang@linux.intel.com> > > Below commit introduced a new macro MSR_IA32_FEATURE_CONTROL for > IA32_FEATURE_CONTROL MSR but it didn't remove old IA32_FEATURE_CONTROL_MSR > macro. The new one has better naming convention, so remove the old as > duplication and replace the relevant code with new one. > > mwait-idle: prevent SKL-H boot failure when C8+C9+C10 enabled > > Some SKL-H configurations require "max_cstate=7" to boot. > While that is an effective workaround, it disables C10. > > ...... > > Above commit also used SGX_ENABLE (bit 18) in IA32_FEATURE_CONTROL MSR without a > macro for it. A new macro IA32_FEATURE_CONTROL_MSR_SGX_ENABLE is also added for > better code and future use. > > Signed-off-by: Kai Huang <kai.huang@linux.intel.com> > --- > xen/arch/x86/cpu/mwait-idle.c | 2 +- > xen/arch/x86/hvm/vmx/vmcs.c | 4 ++-- > xen/arch/x86/hvm/vmx/vmx.c | 4 ++-- > xen/arch/x86/hvm/vmx/vvmx.c | 2 +- > xen/include/asm-x86/msr-index.h | 4 ++-- > 5 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c > index e062e21..d6488f7 100644 > --- a/xen/arch/x86/cpu/mwait-idle.c > +++ b/xen/arch/x86/cpu/mwait-idle.c > @@ -1006,7 +1006,7 @@ static void __init sklh_idle_state_table_update(void) > rdmsrl(MSR_IA32_FEATURE_CONTROL, msr); > > /* if SGX is enabled */ > - if (msr & (1 << 18)) > + if (msr & IA32_FEATURE_CONTROL_MSR_SGX_ENABLE) > return; > } > > diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c > index 848ac33..33d83fc 100644 > --- a/xen/arch/x86/hvm/vmx/vmcs.c > +++ b/xen/arch/x86/hvm/vmx/vmcs.c > @@ -604,7 +604,7 @@ int vmx_cpu_up(void) > return -EINVAL; > } > > - rdmsr(IA32_FEATURE_CONTROL_MSR, eax, edx); > + rdmsr(MSR_IA32_FEATURE_CONTROL, eax, edx); > > bios_locked = !!(eax & IA32_FEATURE_CONTROL_MSR_LOCK); > if ( bios_locked ) > @@ -623,7 +623,7 @@ int vmx_cpu_up(void) > eax |= IA32_FEATURE_CONTROL_MSR_ENABLE_VMXON_OUTSIDE_SMX; > if ( test_bit(X86_FEATURE_SMX, &boot_cpu_data.x86_capability) ) > eax |= IA32_FEATURE_CONTROL_MSR_ENABLE_VMXON_INSIDE_SMX; > - wrmsr(IA32_FEATURE_CONTROL_MSR, eax, 0); > + wrmsr(MSR_IA32_FEATURE_CONTROL, eax, 0); > } > > if ( (rc = vmx_init_vmcs_config()) != 0 ) > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c > index 54cdb86..c23b1e9 100644 > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -2622,7 +2622,7 @@ static int vmx_msr_read_intercept(unsigned int msr, uint64_t > *msr_content) > case MSR_IA32_DEBUGCTLMSR: > __vmread(GUEST_IA32_DEBUGCTL, msr_content); > break; > - case IA32_FEATURE_CONTROL_MSR: > + case MSR_IA32_FEATURE_CONTROL: > case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_VMFUNC: > if ( !nvmx_msr_read_intercept(msr, msr_content) ) > goto gp_fault; > @@ -2848,7 +2848,7 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t > msr_content) > > break; > } > - case IA32_FEATURE_CONTROL_MSR: > + case MSR_IA32_FEATURE_CONTROL: > case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_TRUE_ENTRY_CTLS: > if ( !nvmx_msr_write_intercept(msr, msr_content) ) > goto gp_fault; > diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c > index c6a39e9..4b9ec6a 100644 > --- a/xen/arch/x86/hvm/vmx/vvmx.c > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > @@ -1941,7 +1941,7 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 > *msr_content) > data = gen_vmx_msr(data, VMX_ENTRY_CTLS_DEFAULT1, host_data); > break; > > - case IA32_FEATURE_CONTROL_MSR: > + case MSR_IA32_FEATURE_CONTROL: > data = IA32_FEATURE_CONTROL_MSR_LOCK | > IA32_FEATURE_CONTROL_MSR_ENABLE_VMXON_OUTSIDE_SMX; > break; > diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h > index e0f7f8d..5962cbf 100644 > --- a/xen/include/asm-x86/msr-index.h > +++ b/xen/include/asm-x86/msr-index.h > @@ -133,12 +133,13 @@ > #define MSR_IA32_VMX_TRUE_EXIT_CTLS 0x48f > #define MSR_IA32_VMX_TRUE_ENTRY_CTLS 0x490 > #define MSR_IA32_VMX_VMFUNC 0x491 > -#define IA32_FEATURE_CONTROL_MSR 0x3a > +#define MSR_IA32_FEATURE_CONTROL 0x3a Instead of moving MSR definition up here, better move all related lines down since original place is more sorted regarding to 0x3a. > #define IA32_FEATURE_CONTROL_MSR_LOCK 0x0001 > #define IA32_FEATURE_CONTROL_MSR_ENABLE_VMXON_INSIDE_SMX 0x0002 > #define IA32_FEATURE_CONTROL_MSR_ENABLE_VMXON_OUTSIDE_SMX 0x0004 > #define IA32_FEATURE_CONTROL_MSR_SENTER_PARAM_CTL 0x7f00 > #define IA32_FEATURE_CONTROL_MSR_ENABLE_SENTER 0x8000 > +#define IA32_FEATURE_CONTROL_MSR_SGX_ENABLE 0x40000 suppose above macros better be changed in same style? Or is it really meaningful to keep whole MSR name in every bit definition? Is it clearly enough to just keep strings after _MSR_? > > /* K7/K8 MSRs. Not complete. See the architecture manual for a more > complete list. */ > @@ -288,7 +289,6 @@ > #define MSR_IA32_PLATFORM_ID 0x00000017 > #define MSR_IA32_EBL_CR_POWERON 0x0000002a > #define MSR_IA32_EBC_FREQUENCY_ID 0x0000002c > -#define MSR_IA32_FEATURE_CONTROL 0x0000003a > #define MSR_IA32_TSC_ADJUST 0x0000003b > > #define MSR_IA32_APICBASE 0x0000001b > -- > 2.7.4
>>> On 24.06.16 at 12:56, <kevin.tian@intel.com> wrote: >> From: kaih.linux@gmail.com [mailto:kaih.linux@gmail.com] >> Sent: Friday, June 24, 2016 6:45 PM >> --- a/xen/include/asm-x86/msr-index.h >> +++ b/xen/include/asm-x86/msr-index.h >> @@ -133,12 +133,13 @@ >> #define MSR_IA32_VMX_TRUE_EXIT_CTLS 0x48f >> #define MSR_IA32_VMX_TRUE_ENTRY_CTLS 0x490 >> #define MSR_IA32_VMX_VMFUNC 0x491 >> -#define IA32_FEATURE_CONTROL_MSR 0x3a >> +#define MSR_IA32_FEATURE_CONTROL 0x3a > > Instead of moving MSR definition up here, better move all related lines > down since original place is more sorted regarding to 0x3a. I agree. >> #define IA32_FEATURE_CONTROL_MSR_LOCK 0x0001 >> #define IA32_FEATURE_CONTROL_MSR_ENABLE_VMXON_INSIDE_SMX 0x0002 >> #define IA32_FEATURE_CONTROL_MSR_ENABLE_VMXON_OUTSIDE_SMX 0x0004 >> #define IA32_FEATURE_CONTROL_MSR_SENTER_PARAM_CTL 0x7f00 >> #define IA32_FEATURE_CONTROL_MSR_ENABLE_SENTER 0x8000 >> +#define IA32_FEATURE_CONTROL_MSR_SGX_ENABLE 0x40000 > > suppose above macros better be changed in same style? Or is it > really meaningful to keep whole MSR name in every bit definition? > Is it clearly enough to just keep strings after _MSR_? I partly agree. The _MSR_ infix is clearly pointless. I wouldn't, however, like to see the IA32_FEATURE_CONTROL_ prefix dropped, as it helps associating the bits with their MSR. Jan
Hi Kevin, Jan, Thanks for comments. On 6/24/2016 11:31 PM, Jan Beulich wrote: >>>> On 24.06.16 at 12:56, <kevin.tian@intel.com> wrote: >>> From: kaih.linux@gmail.com [mailto:kaih.linux@gmail.com] >>> Sent: Friday, June 24, 2016 6:45 PM >>> --- a/xen/include/asm-x86/msr-index.h >>> +++ b/xen/include/asm-x86/msr-index.h >>> @@ -133,12 +133,13 @@ >>> #define MSR_IA32_VMX_TRUE_EXIT_CTLS 0x48f >>> #define MSR_IA32_VMX_TRUE_ENTRY_CTLS 0x490 >>> #define MSR_IA32_VMX_VMFUNC 0x491 >>> -#define IA32_FEATURE_CONTROL_MSR 0x3a >>> +#define MSR_IA32_FEATURE_CONTROL 0x3a >> >> Instead of moving MSR definition up here, better move all related lines >> down since original place is more sorted regarding to 0x3a. > > I agree. Sure. I'll move this macro down, along with the bit macros. > >>> #define IA32_FEATURE_CONTROL_MSR_LOCK 0x0001 >>> #define IA32_FEATURE_CONTROL_MSR_ENABLE_VMXON_INSIDE_SMX 0x0002 >>> #define IA32_FEATURE_CONTROL_MSR_ENABLE_VMXON_OUTSIDE_SMX 0x0004 >>> #define IA32_FEATURE_CONTROL_MSR_SENTER_PARAM_CTL 0x7f00 >>> #define IA32_FEATURE_CONTROL_MSR_ENABLE_SENTER 0x8000 >>> +#define IA32_FEATURE_CONTROL_MSR_SGX_ENABLE 0x40000 >> >> suppose above macros better be changed in same style? Or is it >> really meaningful to keep whole MSR name in every bit definition? >> Is it clearly enough to just keep strings after _MSR_? > > I partly agree. The _MSR_ infix is clearly pointless. I wouldn't, > however, like to see the IA32_FEATURE_CONTROL_ prefix > dropped, as it helps associating the bits with their MSR. Sure. I think we can have consensus on just removing the _MSR_ infix, so the bit macros will be like IA32_FEATURE_CONTROL_LOCK, IA32_FEATURE_CONTROL_SGX_ENABLE, etc? Thanks, -Kai > > Jan > >
> From: Huang, Kai [mailto:kai.huang@linux.intel.com] > Sent: Saturday, June 25, 2016 6:16 AM > > > >>> #define IA32_FEATURE_CONTROL_MSR_LOCK 0x0001 > >>> #define IA32_FEATURE_CONTROL_MSR_ENABLE_VMXON_INSIDE_SMX 0x0002 > >>> #define IA32_FEATURE_CONTROL_MSR_ENABLE_VMXON_OUTSIDE_SMX 0x0004 > >>> #define IA32_FEATURE_CONTROL_MSR_SENTER_PARAM_CTL 0x7f00 > >>> #define IA32_FEATURE_CONTROL_MSR_ENABLE_SENTER 0x8000 > >>> +#define IA32_FEATURE_CONTROL_MSR_SGX_ENABLE 0x40000 > >> > >> suppose above macros better be changed in same style? Or is it > >> really meaningful to keep whole MSR name in every bit definition? > >> Is it clearly enough to just keep strings after _MSR_? > > > > I partly agree. The _MSR_ infix is clearly pointless. I wouldn't, > > however, like to see the IA32_FEATURE_CONTROL_ prefix > > dropped, as it helps associating the bits with their MSR. > > Sure. I think we can have consensus on just removing the _MSR_ infix, so > the bit macros will be like IA32_FEATURE_CONTROL_LOCK, > IA32_FEATURE_CONTROL_SGX_ENABLE, etc? > yes, sounds good to me.
diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c index e062e21..d6488f7 100644 --- a/xen/arch/x86/cpu/mwait-idle.c +++ b/xen/arch/x86/cpu/mwait-idle.c @@ -1006,7 +1006,7 @@ static void __init sklh_idle_state_table_update(void) rdmsrl(MSR_IA32_FEATURE_CONTROL, msr); /* if SGX is enabled */ - if (msr & (1 << 18)) + if (msr & IA32_FEATURE_CONTROL_MSR_SGX_ENABLE) return; } diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c index 848ac33..33d83fc 100644 --- a/xen/arch/x86/hvm/vmx/vmcs.c +++ b/xen/arch/x86/hvm/vmx/vmcs.c @@ -604,7 +604,7 @@ int vmx_cpu_up(void) return -EINVAL; } - rdmsr(IA32_FEATURE_CONTROL_MSR, eax, edx); + rdmsr(MSR_IA32_FEATURE_CONTROL, eax, edx); bios_locked = !!(eax & IA32_FEATURE_CONTROL_MSR_LOCK); if ( bios_locked ) @@ -623,7 +623,7 @@ int vmx_cpu_up(void) eax |= IA32_FEATURE_CONTROL_MSR_ENABLE_VMXON_OUTSIDE_SMX; if ( test_bit(X86_FEATURE_SMX, &boot_cpu_data.x86_capability) ) eax |= IA32_FEATURE_CONTROL_MSR_ENABLE_VMXON_INSIDE_SMX; - wrmsr(IA32_FEATURE_CONTROL_MSR, eax, 0); + wrmsr(MSR_IA32_FEATURE_CONTROL, eax, 0); } if ( (rc = vmx_init_vmcs_config()) != 0 ) diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 54cdb86..c23b1e9 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -2622,7 +2622,7 @@ static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content) case MSR_IA32_DEBUGCTLMSR: __vmread(GUEST_IA32_DEBUGCTL, msr_content); break; - case IA32_FEATURE_CONTROL_MSR: + case MSR_IA32_FEATURE_CONTROL: case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_VMFUNC: if ( !nvmx_msr_read_intercept(msr, msr_content) ) goto gp_fault; @@ -2848,7 +2848,7 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content) break; } - case IA32_FEATURE_CONTROL_MSR: + case MSR_IA32_FEATURE_CONTROL: case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_TRUE_ENTRY_CTLS: if ( !nvmx_msr_write_intercept(msr, msr_content) ) goto gp_fault; diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c index c6a39e9..4b9ec6a 100644 --- a/xen/arch/x86/hvm/vmx/vvmx.c +++ b/xen/arch/x86/hvm/vmx/vvmx.c @@ -1941,7 +1941,7 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content) data = gen_vmx_msr(data, VMX_ENTRY_CTLS_DEFAULT1, host_data); break; - case IA32_FEATURE_CONTROL_MSR: + case MSR_IA32_FEATURE_CONTROL: data = IA32_FEATURE_CONTROL_MSR_LOCK | IA32_FEATURE_CONTROL_MSR_ENABLE_VMXON_OUTSIDE_SMX; break; diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h index e0f7f8d..5962cbf 100644 --- a/xen/include/asm-x86/msr-index.h +++ b/xen/include/asm-x86/msr-index.h @@ -133,12 +133,13 @@ #define MSR_IA32_VMX_TRUE_EXIT_CTLS 0x48f #define MSR_IA32_VMX_TRUE_ENTRY_CTLS 0x490 #define MSR_IA32_VMX_VMFUNC 0x491 -#define IA32_FEATURE_CONTROL_MSR 0x3a +#define MSR_IA32_FEATURE_CONTROL 0x3a #define IA32_FEATURE_CONTROL_MSR_LOCK 0x0001 #define IA32_FEATURE_CONTROL_MSR_ENABLE_VMXON_INSIDE_SMX 0x0002 #define IA32_FEATURE_CONTROL_MSR_ENABLE_VMXON_OUTSIDE_SMX 0x0004 #define IA32_FEATURE_CONTROL_MSR_SENTER_PARAM_CTL 0x7f00 #define IA32_FEATURE_CONTROL_MSR_ENABLE_SENTER 0x8000 +#define IA32_FEATURE_CONTROL_MSR_SGX_ENABLE 0x40000 /* K7/K8 MSRs. Not complete. See the architecture manual for a more complete list. */ @@ -288,7 +289,6 @@ #define MSR_IA32_PLATFORM_ID 0x00000017 #define MSR_IA32_EBL_CR_POWERON 0x0000002a #define MSR_IA32_EBC_FREQUENCY_ID 0x0000002c -#define MSR_IA32_FEATURE_CONTROL 0x0000003a #define MSR_IA32_TSC_ADJUST 0x0000003b #define MSR_IA32_APICBASE 0x0000001b