Re: [PATCH 1/6] x86/msr: Clean up the MSR_EFER constants

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 27/06/18 11:39, Roger Pau Monné wrote:
> On Tue, Jun 26, 2018 at 02:18:13PM +0100, Andrew Cooper wrote:
>> The bit position constants are only used by the trampoline asm, but the
>> code is shorter and clearer when using the mask constants.  This halves
>> the number of constants used.
>>
>> Consistently use _AC() for the bit constants, and start to use spaces
>> for indentation.  Furthermore, EFER contains the NX-Enable bit, to
>> rename the constant to EFER_NXE to match both the AMD and Intel specs.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>
> Just a couple of comments.
>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>>  xen/arch/x86/boot/trampoline.S  |  2 +-
>>  xen/arch/x86/boot/wakeup.S      |  7 +++----
>>  xen/arch/x86/cpu/intel.c        |  2 +-
>>  xen/arch/x86/efi/efi-boot.h     |  2 +-
>>  xen/arch/x86/hvm/hvm.c          |  4 ++--
>>  xen/include/asm-x86/hvm/hvm.h   |  2 +-
>>  xen/include/asm-x86/msr-index.h | 33 ++++++++++++---------------------
>>  7 files changed, 21 insertions(+), 31 deletions(-)
>>
>> diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S
>> index 5588c79..7e77e61e 100644
>> --- a/xen/arch/x86/boot/trampoline.S
>> +++ b/xen/arch/x86/boot/trampoline.S
>> @@ -138,7 +138,7 @@ trampoline_protmode_entry:
>>          or      $EFER_LME|EFER_SCE,%eax   /* Long Mode + SYSCALL/SYSRET */
>>          bt      $cpufeat_bit(X86_FEATURE_NX),%edi /* No Execute? */
>>          jnc     1f
>> -        btsl    $_EFER_NX,%eax  /* No Execute     */
>> +        or      $EFER_NXE, %eax /* No Execute */
>>  1:      wrmsr
>>  
>>          mov     $(X86_CR0_PG | X86_CR0_AM | X86_CR0_WP | X86_CR0_NE |\
>> diff --git a/xen/arch/x86/boot/wakeup.S b/xen/arch/x86/boot/wakeup.S
>> index f9632ee..a37680b 100644
>> --- a/xen/arch/x86/boot/wakeup.S
>> +++ b/xen/arch/x86/boot/wakeup.S
>> @@ -144,11 +144,10 @@ wakeup_32:
>>          jz      .Lskip_eferw
>>          movl    $MSR_EFER,%ecx
>>          rdmsr
>> -        btsl    $_EFER_LME,%eax /* Long Mode      */
>> -        btsl    $_EFER_SCE,%eax /* SYSCALL/SYSRET */
>> -        btl     $20,%edi        /* No Execute?    */
>> +        or      $EFER_LME | EFER_SCE, %eax /* Long Mode + SYSCALL/SYSRET */
> I usually do: '$(EFER_LME | EFER_SCE), ...' because I think is clearer.

Ok.

>
>> +        bt      $cpufeat_bit(X86_FEATURE_NX), %edi /* No Execute? */
>>          jnc     1f
>> -        btsl    $_EFER_NX,%eax  /* No Execute     */
>> +        or      $EFER_NXE, %eax  /* No Execute */
>>  1:      wrmsr
>>  .Lskip_eferw:
>> diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
>> index 8fbccc8..1b10f3e 100644
>> --- a/xen/include/asm-x86/msr-index.h
>> +++ b/xen/include/asm-x86/msr-index.h
>> @@ -4,7 +4,6 @@
>>  /* CPU model specific register (MSR) numbers */
>>  
>>  /* x86-64 specific MSRs */
>> -#define MSR_EFER		0xc0000080 /* extended feature register */
>>  #define MSR_STAR		0xc0000081 /* legacy mode SYSCALL target */
>>  #define MSR_LSTAR		0xc0000082 /* long mode SYSCALL target */
>>  #define MSR_CSTAR		0xc0000083 /* compat mode SYSCALL target */
>> @@ -14,26 +13,6 @@
>>  #define MSR_SHADOW_GS_BASE	0xc0000102 /* SwapGS GS shadow */
>>  #define MSR_TSC_AUX		0xc0000103 /* Auxiliary TSC */
>>  
>> -/* EFER bits: */
>> -#define _EFER_SCE		0  /* SYSCALL/SYSRET */
>> -#define _EFER_LME		8  /* Long mode enable */
>> -#define _EFER_LMA		10 /* Long mode active (read-only) */
>> -#define _EFER_NX		11 /* No execute enable */
>> -#define _EFER_SVME		12 /* AMD: SVM enable */
>> -#define _EFER_LMSLE		13 /* AMD: Long-mode segment limit enable */
>> -#define _EFER_FFXSE		14 /* AMD: Fast FXSAVE/FXRSTOR enable */
>> -
>> -#define EFER_SCE		(1<<_EFER_SCE)
>> -#define EFER_LME		(1<<_EFER_LME)
>> -#define EFER_LMA		(1<<_EFER_LMA)
>> -#define EFER_NX			(1<<_EFER_NX)
>> -#define EFER_SVME		(1<<_EFER_SVME)
>> -#define EFER_LMSLE		(1<<_EFER_LMSLE)
>> -#define EFER_FFXSE		(1<<_EFER_FFXSE)
>> -
>> -#define EFER_KNOWN_MASK		(EFER_SCE | EFER_LME | EFER_LMA | EFER_NX | \
>> -				 EFER_SVME | EFER_LMSLE | EFER_FFXSE)
>> -
>>  /* Speculation Controls. */
>>  #define MSR_SPEC_CTRL			0x00000048
>>  #define SPEC_CTRL_IBRS			(_AC(1, ULL) << 0)
>> @@ -49,6 +28,18 @@
>>  #define ARCH_CAPS_RSBA			(_AC(1, ULL) << 2)
>>  #define ARCH_CAPS_SSB_NO		(_AC(1, ULL) << 4)
>>  
>> +#define MSR_EFER                        0xc0000080 /* Extended Feature Enable Register */
>> +#define EFER_SCE                        (_AC(1, ULL) <<  0) /* SYSCALL Enable */
>> +#define EFER_LME                        (_AC(1, ULL) <<  8) /* Long Mode Enable */
>> +#define EFER_LMA                        (_AC(1, ULL) << 10) /* Long Mode Active */
>> +#define EFER_NXE                        (_AC(1, ULL) << 11) /* No Execute Enable */
>> +#define EFER_SVME                       (_AC(1, ULL) << 12) /* Secure Virtual Machine Enable */
>> +#define EFER_LMSLE                      (_AC(1, ULL) << 13) /* Long Mode Segment Limit Enable */
>> +#define EFER_FFXSE                      (_AC(1, ULL) << 14) /* Fast FXSAVE/FXRSTOR */
> Isn't the align a little bit too much?

See later patches in this series.  EFER is the exception, being so short.

>
> And for clarity I would also indent the bitmasks, so:
>
> #define MSR_EFER      0xc0000080          /* Extended Feature Enable Register */
> #define   EFER_SCE    (_AC(1, ULL) <<  0) /* SYSCALL Enable */

I'm open to idea of indenting the bit constants, if everyone else
agrees.  This mostly affects the next patch, which introduces a style
expectation.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel