[PATCH] x86/hvm: more sure APIC assist is aborted if guest EOIs APIC

[PATCH] x86/hvm: more sure APIC assist is aborted if guest EOIs APIC

[Paul Durrant]

It appears there is a case where Windows enables the APIC assist
enlightenment (see Microsoft Hypervisor Top Level Functional Spec. 5.0b,
section 10.3.5) but does not use it. This scenario is perfectly valid
according to the documentation, but causes the state machine in Xen to
become confused leading to a domain_crash() such as the following:

(XEN) d4: VIRIDIAN GUEST_OS_ID: vendor: 1 os: 4 major: 6 minor: 1 sp: 0
      build: 1db0
(XEN) d4: VIRIDIAN HYPERCALL: enabled: 1 pfn: 3ffff
(XEN) d4v0: VIRIDIAN VP_ASSIST_PAGE: enabled: 1 pfn: 3fffe
(XEN) domain_crash called from viridian.c:452
(XEN) Domain 4 (vcpu#0) crashed on cpu#1:

The following sequence of events is an example of how this can happen:

- On return to guest vlapic_has_pending_irq() finds a bit set in the IRR.
- vlapic_ack_pending_irq() calls viridian_start_apic_assist() which latches
  the vector, sets the bit in the ISR and clears it from the IRR.
- The guest then processes the interrupt but EOIs it normally, therefore
  clearing the bit in the ISR.
- On next return to guest vlapic_has_pending_irq() calls
  viridian_complete_apic_assist(), which discovers the assist bit still set
  in the shared page and therefore leaves the latched vector in place, but
  also finds another bit set in the IRR.
- vlapic_ack_pending_irq() is then called but, because the ISR is was
  cleared by the EOI, another call is made to viridian_start_apic_assist()
  and this then calls domain_crash() because it finds the latched vector
  has not been cleared.

This patch adds a call to viridian_abort_apic_assist() into vlapic EOI
handler to make sure any latched vector is cleared when the ISR is cleared.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/vlapic.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 50f53bdaef..3184970c6e 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -422,6 +422,13 @@ void vlapic_EOI_set(struct vlapic *vlapic)
     if ( vector == -1 )
         return;
 
+    /*
+     * It is possible that APIC assist has been enabled by the guest but
+     * it has chosen not to use it, by EOIing normally. It is therefore
+     * necessary to abort any APIC assist that may have been started
+     * to avoid confusing the state machine.
+     */
+    viridian_abort_apic_assist(vlapic_vcpu(vlapic));
     vlapic_clear_vector(vector, &vlapic->regs->data[APIC_ISR]);
 
     if ( hvm_funcs.handle_eoi )
-- 
2.11.0


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

Re: [PATCH] x86/hvm: more sure APIC assist is aborted if guest EOIs APIC

[Paul Durrant]

> -----Original Message-----
> From: Paul Durrant [mailto:paul.durrant@citrix.com]
> Sent: 17 January 2018 12:54
> To: xen-devel@lists.xenproject.org
> Cc: Paul Durrant <Paul.Durrant@citrix.com>; Jan Beulich
> <jbeulich@suse.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>
> Subject: [PATCH] x86/hvm: more sure APIC assist is aborted if guest EOIs
> APIC

Sorry, I typo-ed the title. If there the no objections to the patch then hopefully this can be fixed up on commit.

  Paul

> 
> It appears there is a case where Windows enables the APIC assist
> enlightenment (see Microsoft Hypervisor Top Level Functional Spec. 5.0b,
> section 10.3.5) but does not use it. This scenario is perfectly valid
> according to the documentation, but causes the state machine in Xen to
> become confused leading to a domain_crash() such as the following:
> 
> (XEN) d4: VIRIDIAN GUEST_OS_ID: vendor: 1 os: 4 major: 6 minor: 1 sp: 0
>       build: 1db0
> (XEN) d4: VIRIDIAN HYPERCALL: enabled: 1 pfn: 3ffff
> (XEN) d4v0: VIRIDIAN VP_ASSIST_PAGE: enabled: 1 pfn: 3fffe
> (XEN) domain_crash called from viridian.c:452
> (XEN) Domain 4 (vcpu#0) crashed on cpu#1:
> 
> The following sequence of events is an example of how this can happen:
> 
> - On return to guest vlapic_has_pending_irq() finds a bit set in the IRR.
> - vlapic_ack_pending_irq() calls viridian_start_apic_assist() which latches
>   the vector, sets the bit in the ISR and clears it from the IRR.
> - The guest then processes the interrupt but EOIs it normally, therefore
>   clearing the bit in the ISR.
> - On next return to guest vlapic_has_pending_irq() calls
>   viridian_complete_apic_assist(), which discovers the assist bit still set
>   in the shared page and therefore leaves the latched vector in place, but
>   also finds another bit set in the IRR.
> - vlapic_ack_pending_irq() is then called but, because the ISR is was
>   cleared by the EOI, another call is made to viridian_start_apic_assist()
>   and this then calls domain_crash() because it finds the latched vector
>   has not been cleared.
> 
> This patch adds a call to viridian_abort_apic_assist() into vlapic EOI
> handler to make sure any latched vector is cleared when the ISR is cleared.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  xen/arch/x86/hvm/vlapic.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 50f53bdaef..3184970c6e 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -422,6 +422,13 @@ void vlapic_EOI_set(struct vlapic *vlapic)
>      if ( vector == -1 )
>          return;
> 
> +    /*
> +     * It is possible that APIC assist has been enabled by the guest but
> +     * it has chosen not to use it, by EOIing normally. It is therefore
> +     * necessary to abort any APIC assist that may have been started
> +     * to avoid confusing the state machine.
> +     */
> +    viridian_abort_apic_assist(vlapic_vcpu(vlapic));
>      vlapic_clear_vector(vector, &vlapic->regs->data[APIC_ISR]);
> 
>      if ( hvm_funcs.handle_eoi )
> --
> 2.11.0


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

Re: [PATCH] x86/hvm: more sure APIC assist is aborted if guest EOIs APIC

[Jan Beulich]

>>> On 17.01.18 at 13:53, <paul.durrant@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -422,6 +422,13 @@ void vlapic_EOI_set(struct vlapic *vlapic)
>      if ( vector == -1 )
>          return;
>  
> +    /*
> +     * It is possible that APIC assist has been enabled by the guest but
> +     * it has chosen not to use it, by EOIing normally. It is therefore
> +     * necessary to abort any APIC assist that may have been started
> +     * to avoid confusing the state machine.
> +     */
> +    viridian_abort_apic_assist(vlapic_vcpu(vlapic));
>      vlapic_clear_vector(vector, &vlapic->regs->data[APIC_ISR]);

Fundamentally fine, but is clearing bit 0 in the shared location a
valid thing to do in this case? Plus shouldn't you pass in the vector,
so the abort would only happen for the very vector that is pending?
Iirc with nested interrupts normal EOI might still need to be used
by Windows (as only a single vector can be in flight in the assist), so
clearing the assist for the wrong vector might actually cause harm.

Jan


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

Re: [PATCH] x86/hvm: more sure APIC assist is aborted if guest EOIs APIC

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 18 January 2018 08:33
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH] x86/hvm: more sure APIC assist is aborted if guest EOIs
> APIC
> 
> >>> On 17.01.18 at 13:53, <paul.durrant@citrix.com> wrote:
> > --- a/xen/arch/x86/hvm/vlapic.c
> > +++ b/xen/arch/x86/hvm/vlapic.c
> > @@ -422,6 +422,13 @@ void vlapic_EOI_set(struct vlapic *vlapic)
> >      if ( vector == -1 )
> >          return;
> >
> > +    /*
> > +     * It is possible that APIC assist has been enabled by the guest but
> > +     * it has chosen not to use it, by EOIing normally. It is therefore
> > +     * necessary to abort any APIC assist that may have been started
> > +     * to avoid confusing the state machine.
> > +     */
> > +    viridian_abort_apic_assist(vlapic_vcpu(vlapic));
> >      vlapic_clear_vector(vector, &vlapic->regs->data[APIC_ISR]);
> 
> Fundamentally fine, but is clearing bit 0 in the shared location a
> valid thing to do in this case? Plus shouldn't you pass in the vector,
> so the abort would only happen for the very vector that is pending?

I believe that it is ok.

> Iirc with nested interrupts normal EOI might still need to be used
> by Windows (as only a single vector can be in flight in the assist), so
> clearing the assist for the wrong vector might actually cause harm.
> 

If there are nested interrupts then the assist should be aborted anyway. Assist only helps if there is one interrupt pending so should not have been started if something higher priority already in the ISR so, if we see an EOI for a vector that is not the one latched in assist I think this is also a bug in the state machine. I can add verification of that, if you'd like.

  Paul

> Jan


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

Re: [PATCH] x86/hvm: more sure APIC assist is aborted if guest EOIs APIC

[Jan Beulich]

>>> On 18.01.18 at 10:06, <Paul.Durrant@citrix.com> wrote:
>>  -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 18 January 2018 08:33
>> To: Paul Durrant <Paul.Durrant@citrix.com>
>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-
>> devel@lists.xenproject.org 
>> Subject: Re: [PATCH] x86/hvm: more sure APIC assist is aborted if guest EOIs
>> APIC
>> 
>> >>> On 17.01.18 at 13:53, <paul.durrant@citrix.com> wrote:
>> > --- a/xen/arch/x86/hvm/vlapic.c
>> > +++ b/xen/arch/x86/hvm/vlapic.c
>> > @@ -422,6 +422,13 @@ void vlapic_EOI_set(struct vlapic *vlapic)
>> >      if ( vector == -1 )
>> >          return;
>> >
>> > +    /*
>> > +     * It is possible that APIC assist has been enabled by the guest but
>> > +     * it has chosen not to use it, by EOIing normally. It is therefore
>> > +     * necessary to abort any APIC assist that may have been started
>> > +     * to avoid confusing the state machine.
>> > +     */
>> > +    viridian_abort_apic_assist(vlapic_vcpu(vlapic));
>> >      vlapic_clear_vector(vector, &vlapic->regs->data[APIC_ISR]);
>> 
>> Fundamentally fine, but is clearing bit 0 in the shared location a
>> valid thing to do in this case? Plus shouldn't you pass in the vector,
>> so the abort would only happen for the very vector that is pending?
> 
> I believe that it is ok.
> 
>> Iirc with nested interrupts normal EOI might still need to be used
>> by Windows (as only a single vector can be in flight in the assist), so
>> clearing the assist for the wrong vector might actually cause harm.
>> 
> 
> If there are nested interrupts then the assist should be aborted anyway. 
> Assist only helps if there is one interrupt pending so should not have been 
> started if something higher priority already in the ISR

I was thinking about the opposite case - assist active with a higher
priority interrupt coming in (and then being EOI-ed). But I guess
that's already being taken care of by the existing call to
viridian_abort_apic_assist().

> so, if we see an EOI 
> for a vector that is not the one latched in assist I think this is also a bug 
> in the state machine. I can add verification of that, if you'd like.

At least one-time warnings would seem helpful here, so we have
some sort of indication that some assumption is being violated.

Jan


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

Re: [PATCH] x86/hvm: more sure APIC assist is aborted if guest EOIs APIC

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 18 January 2018 09:39
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-
> devel@lists.xenproject.org
> Subject: RE: [PATCH] x86/hvm: more sure APIC assist is aborted if guest EOIs
> APIC
> 
> >>> On 18.01.18 at 10:06, <Paul.Durrant@citrix.com> wrote:
> >>  -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 18 January 2018 08:33
> >> To: Paul Durrant <Paul.Durrant@citrix.com>
> >> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-
> >> devel@lists.xenproject.org
> >> Subject: Re: [PATCH] x86/hvm: more sure APIC assist is aborted if guest
> EOIs
> >> APIC
> >>
> >> >>> On 17.01.18 at 13:53, <paul.durrant@citrix.com> wrote:
> >> > --- a/xen/arch/x86/hvm/vlapic.c
> >> > +++ b/xen/arch/x86/hvm/vlapic.c
> >> > @@ -422,6 +422,13 @@ void vlapic_EOI_set(struct vlapic *vlapic)
> >> >      if ( vector == -1 )
> >> >          return;
> >> >
> >> > +    /*
> >> > +     * It is possible that APIC assist has been enabled by the guest but
> >> > +     * it has chosen not to use it, by EOIing normally. It is therefore
> >> > +     * necessary to abort any APIC assist that may have been started
> >> > +     * to avoid confusing the state machine.
> >> > +     */
> >> > +    viridian_abort_apic_assist(vlapic_vcpu(vlapic));
> >> >      vlapic_clear_vector(vector, &vlapic->regs->data[APIC_ISR]);
> >>
> >> Fundamentally fine, but is clearing bit 0 in the shared location a
> >> valid thing to do in this case? Plus shouldn't you pass in the vector,
> >> so the abort would only happen for the very vector that is pending?
> >
> > I believe that it is ok.
> >
> >> Iirc with nested interrupts normal EOI might still need to be used
> >> by Windows (as only a single vector can be in flight in the assist), so
> >> clearing the assist for the wrong vector might actually cause harm.
> >>
> >
> > If there are nested interrupts then the assist should be aborted anyway.
> > Assist only helps if there is one interrupt pending so should not have been
> > started if something higher priority already in the ISR
> 
> I was thinking about the opposite case - assist active with a higher
> priority interrupt coming in (and then being EOI-ed). But I guess
> that's already being taken care of by the existing call to
> viridian_abort_apic_assist().
> 

Yes, that's right.

> > so, if we see an EOI
> > for a vector that is not the one latched in assist I think this is also a bug
> > in the state machine. I can add verification of that, if you'd like.
> 
> At least one-time warnings would seem helpful here, so we have
> some sort of indication that some assumption is being violated.
> 

Ok. I'll re-work it so that abort is passed a vector and warns on mismatch.

  Paul

> Jan


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

Re: [PATCH] x86/hvm: more sure APIC assist is aborted if guest EOIs APIC

[Jan Beulich]

>>> On 18.01.18 at 10:55, <Paul.Durrant@citrix.com> wrote:
> Ok. I'll re-work it so that abort is passed a vector and warns on mismatch.

You mean on the new path only, I suppose? The old path should
not trigger warnings (aiui it would always do so).

Jan


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

Re: [PATCH] x86/hvm: more sure APIC assist is aborted if guest EOIs APIC

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 18 January 2018 10:17
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-
> devel@lists.xenproject.org
> Subject: RE: [PATCH] x86/hvm: more sure APIC assist is aborted if guest EOIs
> APIC
> 
> >>> On 18.01.18 at 10:55, <Paul.Durrant@citrix.com> wrote:
> > Ok. I'll re-work it so that abort is passed a vector and warns on mismatch.
> 
> You mean on the new path only, I suppose? The old path should
> not trigger warnings (aiui it would always do so).

I think, in the existing abort case, the latched vector should always be the lowest pending bit in the ISR.

  Paul

> 
> Jan


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

Re: [PATCH] x86/hvm: more sure APIC assist is aborted if guest EOIs APIC

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
> Of Paul Durrant
> Sent: 18 January 2018 10:37
> To: 'Jan Beulich' <JBeulich@suse.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-
> devel@lists.xenproject.org
> Subject: Re: [Xen-devel] [PATCH] x86/hvm: more sure APIC assist is aborted
> if guest EOIs APIC
> 
> > -----Original Message-----
> > From: Jan Beulich [mailto:JBeulich@suse.com]
> > Sent: 18 January 2018 10:17
> > To: Paul Durrant <Paul.Durrant@citrix.com>
> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-
> > devel@lists.xenproject.org
> > Subject: RE: [PATCH] x86/hvm: more sure APIC assist is aborted if guest
> EOIs
> > APIC
> >
> > >>> On 18.01.18 at 10:55, <Paul.Durrant@citrix.com> wrote:
> > > Ok. I'll re-work it so that abort is passed a vector and warns on mismatch.
> >
> > You mean on the new path only, I suppose? The old path should
> > not trigger warnings (aiui it would always do so).
> 
> I think, in the existing abort case, the latched vector should always be the
> lowest pending bit in the ISR.
> 

Actually reading through the code and the spec. again, I'm not convinced by the logic even with the EOI fix. It looks to me like vlapic_has_pending_irq() could leave an existing assist in place and return a higher priority irr value. This guest would then service the higher priority interrupt, skip the EOI because the assist bit is set, but then the next call to vlapic_has_pending_irq() would erroneously clear the lower priority vector from the ISR because we originally latched the lower priority vector.
As the spec. points out... only the first EOI can avoided, thus I'm not sure trying to track the vector in the viridian code is worth it. It looks like the correct thing to do as test whether an EOI was avoided and then clear the highest priority vector in the ISR (since, if we hadn't squashed it, that would have been the first EOI). We can test when an EOI is avoided because the bit in the shared page would have transitioned from 1 to 0.

  Paul

>   Paul
> 
> >
> > Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86/hvm: more sure APIC assist is aborted if guest EOIs APIC

[Jan Beulich]

>>> On 18.01.18 at 12:21, <Paul.Durrant@citrix.com> wrote:
> Actually reading through the code and the spec. again, I'm not convinced by 
> the logic even with the EOI fix. It looks to me like vlapic_has_pending_irq() 
> could leave an existing assist in place and return a higher priority irr 
> value. This guest would then service the higher priority interrupt, skip the 
> EOI because the assist bit is set, but then the next call to 
> vlapic_has_pending_irq() would erroneously clear the lower priority vector 
> from the ISR because we originally latched the lower priority vector.
> As the spec. points out... only the first EOI can avoided, thus I'm not sure 
> trying to track the vector in the viridian code is worth it. It looks like 
> the correct thing to do as test whether an EOI was avoided and then clear the 
> highest priority vector in the ISR (since, if we hadn't squashed it, that 
> would have been the first EOI). We can test when an EOI is avoided because 
> the bit in the shared page would have transitioned from 1 to 0.

Sounds plausible, looking forward to a patch.

Jan


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