Re: [Xen-devel] [PATCH] msr: Use defines for bits of MSR_IA32_DEBUGCTLMSR instead of numbers

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 31.01.12 at 09:42, Dietmar Hahn <dietmar.hahn@xxxxxxxxxxxxxx> wrote:
> # HG changeset patch
> # Parent e2722b24dc0962de37215320b05d1bb7c4c42864
> 
> Use defines for bits of MSR_IA32_DEBUGCTLMSR instead of numbers.

That would be nice, but only if done consistently everywhere (missing
at least xen/arch/x86/traps.c:ler_enable(); I'm surprise SVM code
doesn't appear to need any adjustment).

> Signed-off-by: Dietmar Hahn <dietmar.hahn@xxxxxxxxxxxxxx>
> 
> diff -r e2722b24dc09 xen/arch/x86/hvm/vmx/vmx.c
> --- a/xen/arch/x86/hvm/vmx/vmx.c      Thu Jan 26 17:43:31 2012 +0000
> +++ b/xen/arch/x86/hvm/vmx/vmx.c      Tue Jan 31 09:40:31 2012 +0100
> @@ -1944,11 +1944,12 @@ static int vmx_msr_write_intercept(unsig
>          break;
>      case MSR_IA32_DEBUGCTLMSR: {
>          int i, rc = 0;
> -
> -        if ( !msr_content || (msr_content & ~3) )
> +        uint64_t allowed = MSR_IA32_DEBUGCTLMSR_LBR | 
> MSR_IA32_DEBUGCTLMSR_BTF;
> +
> +        if ( !msr_content || (msr_content & ~allowed) )
>              break;
>  
> -        if ( msr_content & 1 )
> +        if ( msr_content & MSR_IA32_DEBUGCTLMSR_LBR )
>          {
>              const struct lbr_info *lbr = last_branch_msr_get();
>              if ( lbr == NULL )
> diff -r e2722b24dc09 xen/include/asm-x86/msr-index.h
> --- a/xen/include/asm-x86/msr-index.h Thu Jan 26 17:43:31 2012 +0000
> +++ b/xen/include/asm-x86/msr-index.h Tue Jan 31 09:40:31 2012 +0100
> @@ -64,12 +64,18 @@
>  #define MSR_MTRRfix4K_F8000          0x0000026f
>  #define MSR_MTRRdefType                      0x000002ff
>  
> +/* MSR_IA32_DEBUGCTLMSR bits: */
> +#define _IA32_DEBUGCTLMSR_LBR                0 /* Last Branch Record */
> +#define _IA32_DEBUGCTLMSR_BTF                1 /* Single Step on Branches */
> +#define MSR_IA32_DEBUGCTLMSR_LBR     (1<<_IA32_DEBUGCTLMSR_LBR)
> +#define MSR_IA32_DEBUGCTLMSR_BTF     (1<<_IA32_DEBUGCTLMSR_BTF)
> +

Please no MSR_ prefix on values not representing MSR indices.

Also I personally prefer to have individual field definitions of an
MSR follow its index definition.

Jan

>  #define MSR_IA32_DEBUGCTLMSR         0x000001d9
>  #define MSR_IA32_LASTBRANCHFROMIP    0x000001db
>  #define MSR_IA32_LASTBRANCHTOIP              0x000001dc
>  #define MSR_IA32_LASTINTFROMIP               0x000001dd
>  #define MSR_IA32_LASTINTTOIP         0x000001de
> - 
> +
>  #define MSR_IA32_MTRR_PHYSBASE0     0x00000200
>  #define MSR_IA32_MTRR_PHYSMASK0     0x00000201
>  #define MSR_IA32_MTRR_PHYSBASE1     0x00000202



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel