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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Paul Durrant [mailto:paul.durrant@xxxxxxxxxx]
> Sent: 17 January 2018 12:54
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; Jan Beulich
> <jbeulich@xxxxxxxx>; Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel