[PATCH] vlapic: fix two flaws in emulating MSR_IA32_APICBASE

[PATCH] vlapic: fix two flaws in emulating MSR_IA32_APICBASE

[Chao Gao]

According to SDM Chapter ADVANCED PROGRAMMABLE INTERRUPT CONTROLLER (APIC)
-> Extended XAPIC (x2APIC) -> x2APIC State Transitions, The existing code to
handle guest's writing MSR_IA32_APICBASE has two flaws:
1. Transition from x2APIC Mode to Disabled Mode is allowed but wrongly
disabled currently. Fix it by removing the related check.
2. Transition from x2APIC Mode to xAPIC Mode is illegal but wrongly allowed
currently. Considering changing ENABLE bit of the MSR has been handled,
it can be fixed by only allowing transition from xAPIC Mode to x2APIC Mode
(the other two transitions: from x2APIC mode to xAPIC Mode, from disabled mode
to invalid state (EN=0, EXTD=1) are disabled).

Signed-off-by: Chao Gao <chao.gao@intel.com>
---
 xen/arch/x86/hvm/vlapic.c        | 6 ++----
 xen/include/asm-x86/hvm/vlapic.h | 3 +++
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index cf8ee50..4320c6e 100644
--- 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);
         }
     }
-    else if ( !(value & MSR_IA32_APICBASE_ENABLE) &&
-              unlikely(value & MSR_IA32_APICBASE_EXTD) )
+    else if ( ((vlapic->hw.apic_base_msr ^ value) & MSR_IA32_APICBASE_EXTD) &&
+              unlikely(!vlapic_xapic_mode(vlapic)) )
         return 0;
 
     vlapic->hw.apic_base_msr = value;
diff --git a/xen/include/asm-x86/hvm/vlapic.h b/xen/include/asm-x86/hvm/vlapic.h
index 4656293..e07fca5 100644
--- 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))
 
 /*
  * Generic APIC bitmap vector update & search routines.
-- 
1.8.3.1


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

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

[Jan Beulich]

>>> On 31.05.17 at 09:46, <chao.gao@intel.com> 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.

I'm also not convinced checking the MSR bit for enabled state
is fully in line with how other code checks enabled state (see
vlapic_enabled() et al).

Jan


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

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

[Chao Gao]

On Wed, May 31, 2017 at 02:06:50AM -0600, Jan Beulich wrote:
>>>> On 31.05.17 at 09:46, <chao.gao@intel.com> 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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 31.05.17 at 10:56, <chao.gao@intel.com> wrote:
> On Wed, May 31, 2017 at 02:06:50AM -0600, Jan Beulich wrote:
>>>>> On 31.05.17 at 09:46, <chao.gao@intel.com> 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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Chao Gao]

On Wed, May 31, 2017 at 03:15:28AM -0600, Jan Beulich wrote:
>>>> On 31.05.17 at 10:56, <chao.gao@intel.com> wrote:
>> On Wed, May 31, 2017 at 02:06:50AM -0600, Jan Beulich wrote:
>>>>>> On 31.05.17 at 09:46, <chao.gao@intel.com> 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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 31.05.17 at 13:34, <chao.gao@intel.com> wrote:
> On Wed, May 31, 2017 at 03:15:28AM -0600, Jan Beulich wrote:
>>>>> On 31.05.17 at 10:56, <chao.gao@intel.com> wrote:
>>> On Wed, May 31, 2017 at 02:06:50AM -0600, Jan Beulich wrote:
>>>>>>> On 31.05.17 at 09:46, <chao.gao@intel.com> 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@lists.xen.org
https://lists.xen.org/xen-devel