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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.10.15 at 21:18, <konrad.wilk@xxxxxxxxxx> 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) )

? 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.

Jan


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