Re: [Xen-devel] [PATCH 2/4] x86/IRQ: bail early from irq_guest_eoi_timer_fn() when nothing is in flight

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.06.19 at 13:34, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 06/06/2019 09:17, Jan Beulich wrote:
>>>>> On 05.06.19 at 19:15, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 08/05/2019 13:46, Jan Beulich wrote:
>>>> @@ -1130,8 +1130,10 @@ static void irq_guest_eoi_timer_fn(void
>>>>          }
>>>>      }
>>>>  
>>>> -    if ( action->in_flight != 0 )
>>>> -        goto out;
>>>> +    if ( action->in_flight )
>>>> +        printk(XENLOG_G_WARNING
>>>> +               "IRQ%d: %d handlers still in flight at forced EOI\n",
>>>> +               desc->irq, action->in_flight);
>>> AFACIT, this condition can be triggered by a buggy/malicious guest, by
>>> it simply ignoring or masking the line interrupt at the vIO-APIC.
>> I don't think it can, no. Or else the ASSERT_UNREACHABLE() below
>> here would be invalid to add.
> 
> Which ASSERT_UNREACHABLE() ?  I know Roger asked for one, but I don't
> see it anywhere in the code.

Because so far there was no real reason to re-post. It's right here,
as Roger did ask for, and as I did (hesitantly) agree:

    if ( action->in_flight )
    {
        printk(XENLOG_G_WARNING
               "IRQ%u: %d/%d handler(s) still in flight at forced EOI\n",
               irq, action->in_flight, action->nr_guests);
        ASSERT_UNREACHABLE();
    }

>>> The message would be far more useful if it identified the domain in
>>> question, which looks like it can be obtained from the middle of the loop.
>> That very loop has just taken care of decrementing ->in_flight for
>> all such guests.
>>
>> Also note that there could be more than one offending domain, for
>> shared IRQs. Plus the loop you're referring to can specifically _not_
>> be used for identifying the domain(s), because for the ones
>> processed there we _did_ decrement ->in_flight. If this message
>> gets logged, we simply have no idea why ->in_flight is _still_ non-
>> zero. This could be a BUG_ON(), but it seems more in line with our
>> general idea of how we would like to deal with such cases to try
>> and keep the system running here in release builds.
> 
> Ok - lets go with this for now.  It is a net improvement, and we can
> evaluate the guest-triggerability at a later point.
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Thanks much. I'll assume this holds also for the adjustments
requested by Roger.

Jan



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