Re: [Xen-devel] [PATCH 6/6] x86/msr: Clean up the x2APIC MSR constants

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Jun 26, 2018 at 02:18:18PM +0100, Andrew Cooper wrote:
> The name MSR_IA32_APICBASE_MSR doesn't logically relate to its purpose.
> Rename it to MSR_X2APIC_FIRST and introduce a corresponding
> MSR_X2APIC_LAST to avoid opencoding the length of the x2APIC MSR range.
> 
> For the specific registers, drop the IA32 infix, break the APIC part
> away from the register name, and drop the MSR suffix.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Although I have some questions about the existing code.

> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index d5334c9..48e2f8c 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2995,19 +2995,19 @@ void vmx_vlapic_msr_changed(struct vcpu *v)
>                  SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE;
>              if ( cpu_has_vmx_apic_reg_virt )
>              {
> -                for ( msr = MSR_IA32_APICBASE_MSR;
> -                      msr <= MSR_IA32_APICBASE_MSR + 0xff; msr++ )
> +                for ( msr = MSR_X2APIC_FIRST;
> +                      msr <= MSR_X2APIC_FIRST + 0xff; msr++ )
>                      vmx_clear_msr_intercept(v, msr, VMX_MSR_R);

I realize this is existing code, but do you know why 0xff is used here
instead of 0xbff (MSR_X2APIC_LAST) or 0x83f (which is the last
implemented x2APIC MSR)?.

>  
> -                vmx_set_msr_intercept(v, MSR_IA32_APICPPR_MSR, VMX_MSR_R);
> -                vmx_set_msr_intercept(v, MSR_IA32_APICTMICT_MSR, VMX_MSR_R);
> -                vmx_set_msr_intercept(v, MSR_IA32_APICTMCCT_MSR, VMX_MSR_R);
> +                vmx_set_msr_intercept(v, MSR_X2APIC_PPR, VMX_MSR_R);
> +                vmx_set_msr_intercept(v, MSR_X2APIC_TMICT, VMX_MSR_R);
> +                vmx_set_msr_intercept(v, MSR_X2APIC_TMCCT, VMX_MSR_R);
>              }
>              if ( cpu_has_vmx_virtual_intr_delivery )
>              {
> -                vmx_clear_msr_intercept(v, MSR_IA32_APICTPR_MSR, VMX_MSR_W);
> -                vmx_clear_msr_intercept(v, MSR_IA32_APICEOI_MSR, VMX_MSR_W);
> -                vmx_clear_msr_intercept(v, MSR_IA32_APICSELF_MSR, VMX_MSR_W);
> +                vmx_clear_msr_intercept(v, MSR_X2APIC_TPR, VMX_MSR_W);
> +                vmx_clear_msr_intercept(v, MSR_X2APIC_EOI, VMX_MSR_W);
> +                vmx_clear_msr_intercept(v, MSR_X2APIC_SELF, VMX_MSR_W);
>              }
>          }
>          else
> @@ -3016,8 +3016,8 @@ void vmx_vlapic_msr_changed(struct vcpu *v)
>      }
>      if ( !(v->arch.hvm_vmx.secondary_exec_control &
>             SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE) )
> -        for ( msr = MSR_IA32_APICBASE_MSR;
> -              msr <= MSR_IA32_APICBASE_MSR + 0xff; msr++ )
> +        for ( msr = MSR_X2APIC_FIRST;
> +              msr <= MSR_X2APIC_FIRST + 0xff; msr++ )
>              vmx_set_msr_intercept(v, msr, VMX_MSR_RW);
>  
>      vmx_update_secondary_exec_control(v);
> diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
> index ce2e847..9d96e96 100644
> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -49,6 +49,16 @@
>  #define MSR_MISC_FEATURES_ENABLES       0x00000140
>  #define MISC_FEATURES_CPUID_FAULTING    (_AC(1, ULL) <<  0)
>  
> +#define MSR_X2APIC_FIRST                0x00000800
> +#define MSR_X2APIC_LAST                 0x00000bff

I would use START and END because I think it's more natural rather
that FIRST and LAST which to me seem to involve there being multiple
x2APIC inside this range (but I'm not a native speaker, so FIRST and
LAST might be just fine).

Roger.

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