* [PATCH] vlapic: fix two flaws in emulating MSR_IA32_APICBASE @ 2017-05-31 7:46 Chao Gao 2017-05-31 8:06 ` Jan Beulich 0 siblings, 1 reply; 6+ messages in thread From: Chao Gao @ 2017-05-31 7:46 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, Jan Beulich, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] vlapic: fix two flaws in emulating MSR_IA32_APICBASE 2017-05-31 7:46 [PATCH] vlapic: fix two flaws in emulating MSR_IA32_APICBASE Chao Gao @ 2017-05-31 8:06 ` Jan Beulich 2017-05-31 8:56 ` Chao Gao 0 siblings, 1 reply; 6+ messages in thread From: Jan Beulich @ 2017-05-31 8:06 UTC (permalink / raw) To: Chao Gao; +Cc: Andrew Cooper, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vlapic: fix two flaws in emulating MSR_IA32_APICBASE 2017-05-31 8:06 ` Jan Beulich @ 2017-05-31 8:56 ` Chao Gao 2017-05-31 9:15 ` Jan Beulich 0 siblings, 1 reply; 6+ messages in thread From: Chao Gao @ 2017-05-31 8:56 UTC (permalink / raw) To: Jan Beulich; +Cc: Andrew Cooper, xen-devel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vlapic: fix two flaws in emulating MSR_IA32_APICBASE 2017-05-31 8:56 ` Chao Gao @ 2017-05-31 9:15 ` Jan Beulich 2017-05-31 11:34 ` Chao Gao 0 siblings, 1 reply; 6+ messages in thread From: Jan Beulich @ 2017-05-31 9:15 UTC (permalink / raw) To: Chao Gao; +Cc: Andrew Cooper, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vlapic: fix two flaws in emulating MSR_IA32_APICBASE 2017-05-31 9:15 ` Jan Beulich @ 2017-05-31 11:34 ` Chao Gao 2017-05-31 11:51 ` Jan Beulich 0 siblings, 1 reply; 6+ messages in thread From: Chao Gao @ 2017-05-31 11:34 UTC (permalink / raw) To: Jan Beulich; +Cc: Andrew Cooper, xen-devel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vlapic: fix two flaws in emulating MSR_IA32_APICBASE 2017-05-31 11:34 ` Chao Gao @ 2017-05-31 11:51 ` Jan Beulich 0 siblings, 0 replies; 6+ messages in thread From: Jan Beulich @ 2017-05-31 11:51 UTC (permalink / raw) To: Chao Gao; +Cc: Andrew Cooper, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-05-31 11:51 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-05-31 7:46 [PATCH] vlapic: fix two flaws in emulating MSR_IA32_APICBASE Chao Gao 2017-05-31 8:06 ` Jan Beulich 2017-05-31 8:56 ` Chao Gao 2017-05-31 9:15 ` Jan Beulich 2017-05-31 11:34 ` Chao Gao 2017-05-31 11:51 ` Jan Beulich
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.