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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 31.05.17 at 10:56, <chao.gao@xxxxxxxxx> wrote:
> 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))

I can't get the code in line with "I shouldn't use vlapic_x2apic_mode()
here", but beyond that the suggested code is indeed what I think
we would want (provided of course this isn't going to break any
existing users).

Jan


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