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

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

On 27/06/18 14:50, Roger Pau Monné wrote:
> 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@citrix.com>
>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>
> 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)?.

Good questions.  0xbff can't be used because the SandyBridge uarch have
some undocumented MSRs implemented in that range.

There appears to be no justification for 0xff in the patch which
introduced it, and 0x83f would be a more appropriate upper bound. I'll
submit a separate fix for this, as there is a far far more efficient way
do this operation.

>
>>  
>> -                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).

FIRST/LAST are entirely fine in this context, as far as English goes. 
Last tends to be less ambiguous than end when it comes to fencepost
errors, as it is an inclusive term.

~Andrew

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