Re: [Xen-devel] [PATCH v3 6/6] vhpet: add support for level triggered interrupts

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.06.18 at 18:07, <roger.pau@xxxxxxxxxx> wrote:
> On Fri, Jun 22, 2018 at 10:00:41AM -0600, Jan Beulich wrote:
>> >>> On 22.06.18 at 17:31, <roger.pau@xxxxxxxxxx> wrote:
>> > On Fri, Jun 22, 2018 at 09:00:27AM -0600, Jan Beulich wrote:
>> >> >>> On 08.06.18 at 17:07, <roger.pau@xxxxxxxxxx> wrote:
>> >> > @@ -394,6 +411,32 @@ static int hpet_write(
>> >> >          }
>> >> >          break;
>> >> >  
>> >> > +    case HPET_STATUS:
>> >> > +        /* write 1 to clear. */
>> >> > +        while ( new_val )
>> >> > +        {
>> >> > +            bool active;
>> >> > +
>> >> > +            i = find_first_set_bit(new_val);
>> >> > +            if ( i >= HPET_TIMER_NUM )
>> >> > +                break;
>> >> > +            __clear_bit(i, &new_val);
>> >> > +            active = __test_and_clear_bit(i, &h->hpet.isr);
>> >> > +            if ( active )
>> >> > +            {
>> >> > +                /*
>> >> > +                 * Should pt->irq better be used here in case the 
>> >> > guest changes
>> >> > +                 * the configured IRQ while it's active? Guest 
>> >> > changing the IRQ
>> >> > +                 * while the interrupt is active is not documented.
>> >> > +                 */
>> >> 
>> >> I think it's better the way you have it, to base things on what is 
>> >> recorded
>> >> in h->hpet.isr. After all that's what has been asserted. In fact I don't 
>> >> see
>> >> how using pt->irq would address the situation: Isn't it that what changes
>> >> first, and hence the de-assert done here would go out of sync with the
>> >> prior assert?
>> > 
>> > What's in the HPET state can be changed by guest writes,
>> 
>> Not exactly - the only way to modify h->hpet.isr is through the code above.
> 
> Right, but ISR only signals which timer has fired, not which IRQ the
> timer was using. That's stored in pt->irq or h->tn[n].irq, and it's
> what Xen needs in order to perform the deassert.

Hmm, that's one way to look at things. The other is that the needed
information can be derived from just HPET data, by using the ISR bit
and check what GSI the respective channel is configured to use. The
status bits are (intentionally afaict) described as "Timer <N> Interrupt
Active" - once the timer fires, the interrupt gets asserted no matter
what, it may just not make it through because of being masked. I'm
not sure which of the two approaches are better in particular for the
case when the interrupt becomes unmasked in the IO-APIC when it's
already asserted. Perhaps, when getting reconfigured while already
asserted (but masked), gsi_assert_count[] would need to be adjusted?
But as you've said before - the spec doesn't mention this case, so
quite likely we can get away without taking special precautions...

Jan



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