Re: [Xen-devel] [PATCH v4] x86: irq: Do not BUG_ON multiple unbind calls for shared pirqs

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 10 March 2020 11:23
> To: paul@xxxxxxx
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; 'Varad Gautam' <vrd@xxxxxxxxx>; 'Julien 
> Grall' <julien@xxxxxxx>;
> 'Roger Pau Monné' <roger.pau@xxxxxxxxxx>; 'Andrew Cooper' 
> <andrew.cooper3@xxxxxxxxxx>
> Subject: Re: [PATCH v4] x86: irq: Do not BUG_ON multiple unbind calls for 
> shared pirqs
> 
> On 09.03.2020 18:47, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@xxxxxxxx>
> >> Sent: 09 March 2020 16:29
> >>
> >> On 06.03.2020 17:02, paul@xxxxxxx wrote:
> >>> --- a/xen/arch/x86/irq.c
> >>> +++ b/xen/arch/x86/irq.c
> >>> @@ -1680,9 +1680,23 @@ static irq_guest_action_t *__pirq_guest_unbind(
> >>>
> >>>      BUG_ON(!(desc->status & IRQ_GUEST));
> >>>
> >>> -    for ( i = 0; (i < action->nr_guests) && (action->guest[i] != d); i++ 
> >>> )
> >>> -        continue;
> >>> -    BUG_ON(i == action->nr_guests);
> >>> +    for ( i = 0; i < action->nr_guests; i++ )
> >>> +        if ( action->guest[i] == d )
> >>> +            break;
> >>> +
> >>> +    if ( i == action->nr_guests ) /* No matching entry */
> >>> +    {
> >>> +        /*
> >>> +         * In case the pirq was shared, unbound for this domain in an 
> >>> earlier
> >>> +         * call, but still existed on the domain's pirq_tree, we still 
> >>> reach
> >>> +         * here if there are any later unbind calls on the same pirq. 
> >>> Return
> >>> +         * if such an unbind happens.
> >>> +         */
> >>> +        ASSERT(action->shareable);
> >>> +        return NULL;
> >>> +    }
> >>> +
> >>> +    ASSERT(action->nr_guests > 0);
> >>
> >> This seems pointless to have here - v3 had it inside the if(),
> >> where it would actually guard against coming here with nr_guests
> >> equal to zero.
> >
> > Why. The code just after this decrements nr_guests so it seems
> > like entirely the right point to have the ASSERT. I can make it
> > ASSERT >= 0, if that makes more sense.
> 
> There's no way to come here when nr_guests == 0. This is because
> in this case the loop will be exited with i being zero, and hence
> the earlier if()'s body will be entered.

Ok, yes, that's true.

> 
> (And no, >= 0 wouldn't make sense to me - it would mean we might
> have a count of -1 after the decrement.)
> 
> >> v3 also used if() and BUG() instead of ASSERT()
> >> inside this if(), which to me would seem more in line with our
> >> current ./CODING_STYLE guidelines of handling unexpected
> >> conditions. Could you clarify why you switched things?
> >>
> >
> > Because I don't think there is need to kill the host in a
> > non-debug context if we hit this condition; it is entirely
> > survivable as far as I can tell so a BUG_ON() did not seem
> > appropriate.
> 
> It'll mean we have a non-sharable IRQ in a place where this is
> not supposed to happen. How can we be sure the system is in a
> state allowing to safely continue? To me, if shareable / non-
> shareable is of any concern here, then it ought to be BUG().
> If it's not, then the ASSERT() ought to be dropped as well.

Ok, I'll convert back to a BUG().

  Paul

> 
> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel