Re: [Xen-devel] [PATCH] xen: x86: remove duplicated IA32_FEATURE_CONTROL MSR macro

2016-06-27 Thread Tian, Kevin 
> From: Huang, Kai [mailto:kai.hu...@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_SENTER0x8000
> >>> +#define IA32_FEATURE_CONTROL_MSR_SGX_ENABLE   0x4
> >>
> >> 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.

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

Re: [Xen-devel] [PATCH] xen: x86: remove duplicated IA32_FEATURE_CONTROL MSR macro

2016-06-24 Thread Huang, Kai 

Hi Kevin, Jan,

Thanks for comments.

On 6/24/2016 11:31 PM, Jan Beulich wrote:

On 24.06.16 at 12:56,  wrote:

 From: kaih.li...@gmail.com [mailto:kaih.li...@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_CTLS0x490
 #define MSR_IA32_VMX_VMFUNC 0x491
-#define IA32_FEATURE_CONTROL_MSR0x3a
+#define MSR_IA32_FEATURE_CONTROL0x3a


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_SENTER0x8000
+#define IA32_FEATURE_CONTROL_MSR_SGX_ENABLE   0x4


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




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

Re: [Xen-devel] [PATCH] xen: x86: remove duplicated IA32_FEATURE_CONTROL MSR macro

2016-06-24 Thread Jan Beulich 
>>> On 24.06.16 at 12:56,  wrote:
>>  From: kaih.li...@gmail.com [mailto:kaih.li...@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_CTLS0x490
>>  #define MSR_IA32_VMX_VMFUNC 0x491
>> -#define IA32_FEATURE_CONTROL_MSR0x3a
>> +#define MSR_IA32_FEATURE_CONTROL0x3a
> 
> 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_SENTER0x8000
>> +#define IA32_FEATURE_CONTROL_MSR_SGX_ENABLE   0x4
> 
> 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


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

Re: [Xen-devel] [PATCH] xen: x86: remove duplicated IA32_FEATURE_CONTROL MSR macro

2016-06-24 Thread Tian, Kevin 
> From: kaih.li...@gmail.com [mailto:kaih.li...@gmail.com]
> Sent: Friday, June 24, 2016 6:45 PM
> 
> From: Kai Huang 
> 
> 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 
> ---
>  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, _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_CTLS0x490
>  #define MSR_IA32_VMX_VMFUNC 0x491
> -#define IA32_FEATURE_CONTROL_MSR0x3a
> +#define MSR_IA32_FEATURE_CONTROL0x3a

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

[Xen-devel] [PATCH] xen: x86: remove duplicated IA32_FEATURE_CONTROL MSR macro

2016-06-24 Thread kaih . linux 
From: Kai Huang 

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 
---
 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, _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_CTLS0x490
 #define MSR_IA32_VMX_VMFUNC 0x491
-#define IA32_FEATURE_CONTROL_MSR0x3a
+#define MSR_IA32_FEATURE_CONTROL0x3a
 #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_SENTER0x8000
+#define IA32_FEATURE_CONTROL_MSR_SGX_ENABLE   0x4
 
 /* K7/K8 MSRs. Not complete. See the architecture manual for a more
complete list. */
@@ -288,7 +289,6 @@
 #define MSR_IA32_PLATFORM_ID   0x0017
 #define MSR_IA32_EBL_CR_POWERON0x002a
 #define MSR_IA32_EBC_FREQUENCY_ID  0x002c
-#define MSR_IA32_FEATURE_CONTROL