Re: [Xen-devel] [PATCH v2 08/10] x86/SVM: Add interrupt management code via AVIC

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 31/12/2016 05:45, Suravee Suthikulpanit wrote:
> Enabling AVIC implicitly disables the V_IRQ, V_INTR_PRIO, V_IGN_TPR,
> and V_INTR_VECTOR fields in the VMCB Control Word. Therefore, this patch
> introduces new interrupt injection code via AVIC backing page.
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Cc: Jan Beulich <JBeulich@xxxxxxxx>
> Cc: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> ---
>  xen/arch/x86/hvm/svm/avic.c        | 28 ++++++++++++++++++++++++++++
>  xen/arch/x86/hvm/svm/intr.c        |  4 ++++
>  xen/arch/x86/hvm/svm/svm.c         | 12 ++++++++++--
>  xen/include/asm-x86/hvm/svm/avic.h |  1 +
>  4 files changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/avic.c b/xen/arch/x86/hvm/svm/avic.c
> index 6351c8e..faa5e45 100644
> --- a/xen/arch/x86/hvm/svm/avic.c
> +++ b/xen/arch/x86/hvm/svm/avic.c
> @@ -636,6 +636,34 @@ void svm_avic_vmexit_do_noaccel(struct cpu_user_regs 
> *regs)
>      return;
>  }
>  
> +void svm_avic_deliver_posted_intr(struct vcpu *v, u8 vec)
> +{
> +    struct vlapic *vlapic = vcpu_vlapic(v);
> +
> +    /* Fallback to use non-AVIC if vcpu is not enabled with AVIC. */
> +    if ( !svm_avic_vcpu_enabled(v) )
> +    {
> +        if ( !vlapic_test_and_set_vector(vec, &vlapic->regs->data[APIC_IRR]) 
> )
> +            vcpu_kick(v);
> +        return;
> +    }
> +
> +    if ( !(guest_cpu_user_regs()->eflags & X86_EFLAGS_IF) )
> +        return;

Won't this discard the interrupt?

> +
> +    if ( vlapic_test_and_set_vector(vec, &vlapic->regs->data[APIC_IRR]) )
> +        return;
> +
> +    /*
> +     * If vcpu is running on another cpu, hit the doorbell to signal
> +     * it to process interrupt. Otherwise, kick it.
> +     */
> +    if ( v->is_running && (v != current) )
> +        wrmsrl(AVIC_DOORBELL, cpu_data[v->processor].apicid);

Hmm - my gut feeling is that this is racy without holding the scheduler
lock for the target pcpu.  Nothing (I am aware of) excludes ->is_running
and ->processor changing behind our back.

CC'ing George and Dario for their input.

~Andrew

> +    else
> +        vcpu_kick(v);
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
>


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