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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

Jan

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