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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

I still don't like them not using the enabled/disabled helpers we
already have. If vlapic_{en,dis}abled() aren't correct to use
here, use vlapic_{hw,sw}_disabled(). Or alternatively we should
change or get rid of some or all of those.

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

Now that's a good reason to leave vlapic_x2apic_mode() alone
(but add a comment to that effect) - I did simply forget about
this aspect.

Jan

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