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

[Jan Beulich <JBeulich@xxxxxxxx>]

On 03.07.2019 16:46, Varad Gautam wrote:
> It is possible to receive multiple __pirq_guest_unbind calls for the same pirq
> if the pirq has not yet been removed from the domain's pirq_tree.

I'd appreciate if you would make clear under what conditions this can
happen, as I'm getting the impression that it's not the BUG_ON() that
wants removing here, but that instead some caller may need fixing, or
that instead the pirq tree removal needs to happen earlier. Afaict the
call from evtchn_close() can't happen more than once, for example, and
I wouldn't be surprised at all if one of the callers from
xen/drivers/passthrough/ wasn't sufficiently gated.

> To apply stable-4.11 onwards.

That's based on you having found a broken commit in the 4.11 development
window (if so, please name the commit), or simply because 4.10 is about
to go out of general support?

> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -1711,7 +1711,15 @@ static irq_guest_action_t *__pirq_guest_unbind(
>   
>       for ( i = 0; (i < action->nr_guests) && (action->guest[i] != d); i++ )
>           continue;
> -    BUG_ON(i == action->nr_guests);
> +    if ( i == action->nr_guests ) {
> +        /* 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. 
> */

Style: The opening brace is misplaced, plus the comment is not
properly formatted and has overly long lines.

> +        if ( action->nr_guests > 0 && action->shareable )
> +            return NULL;

Why does the sharable aspect matter here? Or asked differently, why
can the same situation (multiple unbind requests) not arise for
non-sharable IRQs?

Similary, why would this same situation not arise for the last guest
getting unbound from the IRQ? There is an "action == NULL" check
earlier on, but if multiple calls were legitimate, then the
dprintk() there should go away (or be gated suitably) as well.

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