Re: [Xen-devel] [PATCH] vlapic: fix two flaws in emulating MSR_IA32_APICBASE

[Chao Gao <chao.gao@xxxxxxxxx>]

On Wed, May 31, 2017 at 02:06:50AM -0600, Jan Beulich wrote:
>>>> On 31.05.17 at 09:46, <chao.gao@xxxxxxxxx> wrote:
>> --- a/xen/arch/x86/hvm/vlapic.c
>> +++ b/xen/arch/x86/hvm/vlapic.c
>> @@ -1003,14 +1003,12 @@ bool_t vlapic_msr_set(struct vlapic *vlapic, 
>> uint64_t value)
>>          }
>>          else
>>          {
>> -            if ( unlikely(vlapic_x2apic_mode(vlapic)) )
>> -                return 0;
>>              vlapic->hw.disabled |= VLAPIC_HW_DISABLED;
>>              pt_may_unmask_irq(vlapic_domain(vlapic), NULL);
>>          }
>
>Especially with you not adding any code here, ...
>
>> --- a/xen/include/asm-x86/hvm/vlapic.h
>> +++ b/xen/include/asm-x86/hvm/vlapic.h
>> @@ -53,6 +53,9 @@
>>      ((vlapic)->hw.apic_base_msr & MSR_IA32_APICBASE_BASE)
>>  #define vlapic_x2apic_mode(vlapic)                              \
>>      ((vlapic)->hw.apic_base_msr & MSR_IA32_APICBASE_EXTD)
>> +#define vlapic_xapic_mode(vlapic)                               \
>> +    (((vlapic)->hw.apic_base_msr & MSR_IA32_APICBASE_ENABLE) && \
>> +     !vlapic_x2apic_mode(vlapic))
>
>... I think it is imperative that both macros are fully symmetric,
>i.e. the enabled check should either be present in both or (less
>desirable) absent.

The reason I think is we have an assumption here that
vlapic->hw.apic_base_msr is in a valid state. so if EXTD=1, we can
conclude vlapic is in x2apic mode. But if only EN=1, we can't conclude
vlapic is in xapic mode.
Or, do you just mean I shouldn't use vlapic_x2apic_mode() here, like
this:

#define vlapic_x2apic_mode(vlapic)   \
    (vlapic_enabled(vlapic) && \
     (vlapic->hw.apic_base_msr & MSR_IA32_APICBASE_EXTD))

#define vlapic_xapic_mode(vlapic)   \
    (vlapic_enabled(vlapic) && \
     !(vlapic->hw.apic_base_msr & MSR_IA32_APICBASE_EXTD))

Thanks
Chao

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