Re: [Xen-devel] [PATCHv1 1/2] passthrough: use per-interrupt lock when injecting an interrupt

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

.snip..
> > @@ -481,6 +489,8 @@ int pt_irq_destroy_bind(
> >      pirq = pirq_info(d, machine_gsi);
> >      pirq_dpci = pirq_dpci(pirq);
> >  
> > +    spin_lock(&pirq_dpci->lock);
> 
> Considering that code further down in this function checks
> pirq_dpci to be non-NULL, this doesn't look safe (or else those
> checks should go away, as after this addition they would be
> likely to trigger e.g. Coverity warnings).

?  The checks are for pirq_dpci->dom.

> 
> > @@ -621,7 +633,7 @@ int hvm_do_IRQ_dpci(struct domain *d, struct pirq *pirq)
> >      return 1;
> >  }
> >  
> > -/* called with d->event_lock held */
> > +/* called with pirq_dhci->lock held */
> >  static void __msi_pirq_eoi(struct hvm_pirq_dpci *pirq_dpci)
> 
> The fact that this is a safe change to the locking model imo needs
> to be spelled out explicitly in the commit message. Afaict it's safe
> only because desc_guest_eoi() uses pirq for nothing else than to
> (atomically!) clear pirq->masked.
> 
> > @@ -675,7 +687,7 @@ static void hvm_dirq_assist(struct domain *d, struct 
> > hvm_pirq_dpci *pirq_dpci)
> >  {
> >      ASSERT(d->arch.hvm_domain.irq.dpci);
> >  
> > -    spin_lock(&d->event_lock);
> > +    spin_lock(&pirq_dpci->lock);
> >      if ( test_and_clear_bool(pirq_dpci->masked) )
> >      {
> >          struct pirq *pirq = dpci_pirq(pirq_dpci);
> 
> Along the same lines - it's not obvious that the uses of pirq here are
> safe with event_lock no longer held. In particular I wonder about
> send_guest_pirq() - despite the other use in __do_IRQ_guest() not
> doing any locking either I'm not convinced this is correct.


It seems that the event channel mechanism only uses the event channel
lock when expanding and initializing (FIFO). For the old mechanism
it was for binding, closing (uhuh), status, reset, and set_priority.

The 'closing' is interesting. What if the guest was closing the port
while we are to notify it of an interrupt (so inside hvm_dirq_assist).

evtchn_close will take the event_lock call pirq_cleanup_check
and calls pt_pirq_cleanup_check. We check for STATE_RUN (still set
as dpci_softirq hasn't finished calling hvm_dirq_assist) so it returns
zero .. and does not call radix_tree_delete.

Then evtchn_close goes to 'unlink_pirq_port' and then follows up with
'unmap_domain_pirq_emuirq'. That sets emuirq to IRQ_UNBOUND.

OK, now hvm_dirq_assist is now inside the first 'if' and checks:

684         if ( hvm_domain_use_pirq(d, pirq) )

which would have been true, but now is false (as emuirq is now
IRQ_UNBOUND). 

Lets also assume this is for an MSI.

The 'pt_irq_need_timer' returns 0:

142     return !(flags & (HVM_IRQ_DPCI_GUEST_MSI | HVM_IRQ_DPCI_TRANSLATE));

which hits this:

723         ASSERT(pt_irq_need_timer(pirq_dpci->flags));

and blows up.

If we are not compiled with 'debug=y' then we would set a timer which
8 ms later calls 'pt_irq_time_out'. It then clears the STATE_RUN and
the softirq is done.

So lets switch back to 'evtchn_close'.. which unlocks the event_lock
and is done.

The 'pt_irq_time_out' is pretty much a nop - takes the event_lock,
does not iterate over the list as it is an MSI one so no legacy interrupts.
Calls pt_irq_guest_eoi over the radix tree - and pt_irq_guest_eoi does nothing
as the _HVM_IRQ_DPCI_EOI_LATCH_SHIFT is not set.

I think that is the only issue - when using the legacy event channels
and the guest fiddling with the pirq. I suppose that means anybody
using 'unmap_domain_pirq_emuirq' will hit this ASSERT(). That means:

 arch_domain_soft_reset
 physdev_unmap_pirq
 evtchn_close

Thought you could 'fix' this by doing:


diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
index bda9374..6ea59db 100644
--- a/xen/drivers/passthrough/io.c
+++ b/xen/drivers/passthrough/io.c
@@ -720,8 +720,8 @@ static void hvm_dirq_assist(struct domain *d, struct 
hvm_pirq_dpci *pirq_dpci)
          * guest will never deal with the irq, then the physical interrupt line
          * will never be deasserted.
          */
-        ASSERT(pt_irq_need_timer(pirq_dpci->flags));
-        set_timer(&pirq_dpci->timer, NOW() + PT_IRQ_TIME_OUT);
+        if (pt_irq_need_timer(pirq_dpci->flags))
+            set_timer(&pirq_dpci->timer, NOW() + PT_IRQ_TIME_OUT);
     }
     spin_unlock(&d->event_lock);
 }






> 
> Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

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