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 21, 2017 12:23 PM
> 
> >> @@ -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.
> >
> 
> nvmx_intr_intercept(), currently, is only called from region
> protected by irq_lock. So i think the ASSERT won't be triggered.
> But I also need add an ASSERT in nvmx_intr_intercept().
> 

You're right. I overlooked it.

btw I'm thinking whether adding lock in vmx_intr_assist is
necessary... Can you think whether below option may
work?

Do lock protection within pt_update_irq. Make sure vector
not changed between setting vIRR and finding pt_irq_vector.
Within the lock also saves the vector to earliest_pt.

Then vmx_intr_assist will get the exact vector programmed
to vIRR from pt_irq_vector. Then in pt_intr_post, compare
saved vector to intack.vector, instead of re-searching
pt_irq_vector again.

This way even RTE is changed after pt_update_irq it won't
impact the whole flow here, since new vector hasn't got
chance to hit vIRR. then we don't need adding lock in 
vmx_intr_assist at all.

Thanks
Kevin

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