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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Jun 25, 2018 at 02:26:00AM -0600, Jan Beulich wrote:
> >>> 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?

As you say, maybe we should do a deassert of the previous interrupt if
ISR is set when reconfiguring?

> But as you've said before - the spec doesn't mention this case, so
> quite likely we can get away without taking special precautions...

IMO, as a starting implementation I would leave this as-is (no
deassert on reconfigure). We can always add more logic later if issues
are discovered.

Thanks, Roger.

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