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

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: kaih.linux@xxxxxxxxx [mailto:kaih.linux@xxxxxxxxx]
> Sent: Friday, June 24, 2016 6:45 PM
> 
> From: Kai Huang <kai.huang@xxxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxx>
> ---
>  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


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