Re: [Xen-devel] [RFC PATCH] hvm/vpt: fix inconsistent views of vIOAPIC in vmx_intr_assist()

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Gao, Chao
> Sent: Friday, April 7, 2017 11:24 AM
> 
> When injecting periodic timer interrupt in vmx_intr_assist(), multiple read
> operation is operated during one event delivery and incur to inconsistent

operation is operated -> operations are done

> views of vIOAPIC. For example, if a periodic timer interrupt is from PIT, when
> set the corresponding bit in vIRR, the corresponding RTE is accessed in
> pt_update_irq(). When this function returns, it accesses the RTE again to get
> the vector it set in vIRR.  Between the two accesses, the content of RTE may
> have been changed by another CPU for no protection method in use. This
> case
> can incur the assertion failure in vmx_intr_assist().  Also after a periodic
> timer interrupt is injected successfully, pt_irq_posted() will decrease the
> count (pending_intr_nr). But if the corresponding RTE has been changed,
> pt_irq_posted() will not decrease the count, resulting one more timer
> interrupt.
> 
> More discussion and history can be found in
> 1.https://lists.xenproject.org/archives/html/xen-devel/2017-
> 03/msg00906.html
> 2.https://lists.xenproject.org/archives/html/xen-devel/2017-
> 01/msg01019.html
> 
> This patch simply uses irq_lock to avoid these inconsistent views of vIOAPIC.
> To fix the deadlock, provide locked version of several functions.
> 
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> ---
>  xen/arch/x86/hvm/irq.c      | 22 ++++++++++++++++------
>  xen/arch/x86/hvm/svm/intr.c | 21 +++++++++++++++------
>  xen/arch/x86/hvm/vmx/intr.c |  9 +++++++++
>  xen/arch/x86/hvm/vpic.c     | 20 ++++++++++++++------
>  xen/arch/x86/hvm/vpt.c      |  4 ++--
>  xen/include/xen/hvm/irq.h   |  4 ++++
>  6 files changed, 60 insertions(+), 20 deletions(-)
> 

[...]
> diff --git a/xen/arch/x86/hvm/vpic.c b/xen/arch/x86/hvm/vpic.c
> index e160bbd..8d02e52 100644
> --- a/xen/arch/x86/hvm/vpic.c
> +++ b/xen/arch/x86/hvm/vpic.c
> @@ -153,14 +153,13 @@ static void __vpic_intack(struct hvm_hw_vpic *vpic,
> int irq)
>      vpic_update_int_output(vpic);
>  }
> 
> -static int vpic_intack(struct hvm_hw_vpic *vpic)
> +static int vpic_intack_locked(struct hvm_hw_vpic *vpic)
>  {
>      int irq = -1;
> 
> -    vpic_lock(vpic);
> -
> +    ASSERT(vpic_is_locked(vpic));
>      if ( !vpic->int_output )
> -        goto out;
> +        return irq;
> 
>      irq = vpic_get_highest_priority_irq(vpic);
>      BUG_ON(irq < 0);
> @@ -175,7 +174,15 @@ static int vpic_intack(struct hvm_hw_vpic *vpic)
>          irq += 8;
>      }
> 
> - out:
> +    return irq;
> +}
> +
> +static int vpic_intack(struct hvm_hw_vpic *vpic)
> +{
> +    int irq;
> +
> +    vpic_lock(vpic);
> +    irq = vpic_intack_locked(vpic);
>      vpic_unlock(vpic);
>      return irq;
>  }
> @@ -487,13 +494,14 @@ int vpic_ack_pending_irq(struct vcpu *v)
>      struct hvm_hw_vpic *vpic = &v->domain->arch.hvm_domain.vpic[0];
> 
>      ASSERT(has_vpic(v->domain));
> +    ASSERT(vpic_is_locked(vpic));
> 
>      TRACE_2D(TRC_HVM_EMUL_PIC_PEND_IRQ_CALL,
> vlapic_accept_pic_intr(v),
>               vpic->int_output);
>      if ( !vlapic_accept_pic_intr(v) || !vpic->int_output )
>          return -1;
> 
> -    irq = vpic_intack(vpic);
> +    irq = vpic_intack_locked(vpic);
>      if ( irq == -1 )
>          return -1;
> 

hvm_vcpu_ack_pending_irq is also invoked in nvmx_intr_intercept,
where you also need surround it with spin_lock/unlock otherwise
above ASSERT will be triggered.


Thanks
Kevin

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