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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Thu, Oct 29, 2015 at 03:11:39AM -0600, Jan Beulich wrote:
> >>> On 28.10.15 at 21:18, <konrad.wilk@oracle.com> wrote:
> >> > @@ -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.
> 
> What about
> 
>         /* clear the mirq info */
>         if ( pirq_dpci && (pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) )
> 
> and
> 
>     if ( pirq_dpci && (pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) &&
>          list_empty(&pirq_dpci->digl_list) )

Yes. I was looking at the function code (pt_irq_create_bind) not the
pt_irq_destroy_bind!
> 
> ? In fact I can't spot any access to pirq_dpci->dom in this function.
> 
> >> > @@ -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.
> 
> Well, the event lock is also used for some pIRQ management iirc.

Correct - pirq_guest_bind, pirq_guest_unbind, map_domain_pirq,
map_domain_emuirq_pirq, unmap_domain_pirq_emuirq, and so on.

> 
> Jan
>