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

[Chao Gao <chao.gao@xxxxxxxxx>]

On Wed, May 31, 2017 at 03:15:28AM -0600, Jan Beulich wrote:
>>>> 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).

When I am seeking for evidences to persuade you that it will not
break the existing users, I found that
1. In SDM ADVANCED PROGRAMMABLE INTERRUPT CONTROLLER (APIC) ->
Extended XAPIC (x2APIC) -> Detecting and Enabling x2APIC Mode, there
is a table to illustrate which mode the APIC is in. It doesn't mention
the global enable/disable bit in MSR_IA32_APICBASE or software
enable/disable bit in spurious vector register.

2. In the same chapter Local APIC->Local APIC state->Local APIC state
after It Has Been Software Disabled, the APIC been software disabled
responds normally to SIPI message. To match SIPI's destination, we
use the marco vlapic_x2apic_mode(), it is weird to return false when 
an APIC with MSI_IA32_APICBASE EN=1 EXTD=1 but been software disabled,
what's more, it is software disabled by default when enabling x2APIC.

So I suggest the code like below:
#define vlapic_x2apic_mode(vlapic) \
    (((vlapic)->hw.apic_base_msr & MSR_IA32_APICBASE_ENABLE) && \
     ((vlapic)->hw.apic_base_msr & MSR_IA32_APICBASE_EXTD))

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

They are also symmetric. This change definitely won't break any user
for the EN=0 and EXTD=1 is an invalid state.

Also I found we should return #GP when finding guest trying to do an
illegal transition. Will change this too.

Thanks
Chao

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