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

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

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:
> >> > --- a/xen/arch/x86/hvm/hpet.c
> >> > +++ b/xen/arch/x86/hvm/hpet.c
> >> > @@ -223,6 +223,17 @@ static void hpet_stop_timer(HPETState *h, unsigned 
> >> > int 
> > tn,
> >> >      hpet_get_comparator(h, tn, guest_time);
> >> >  }
> >> >  
> >> > +static void hpet_timer_fired(struct vcpu *v, void *data)
> >> > +{
> >> > +    unsigned int tn = (unsigned int)data;
> >> 
> >> I don't think this cast will go through without warning on all gcc 
> >> versions 
> > we
> >> care about.
> > 
> > Hm, should be casted to unsigned long I guess so it's the same size.
> > 
> >> > +    HPETState *h = vcpu_vhpet(v);
> >> > +
> >> > +    write_lock(&h->lock);
> >> > +    ASSERT(!test_bit(tn, &h->hpet.isr));
> >> > +    __set_bit(tn, &h->hpet.isr);
> >> 
> >>     if ( __test_and_set_bit() )
> >>         ASSERT_UNREACHABLE();
> >> 
> >> ?
> >> 
> >> Seeing this I can understand why you want to call the callback the way
> >> you do in the previous patch. I continue to be unconvinced this second
> >> call is generally correct (and sufficient). Simply consider the RTC case,
> >> where in theory the IRQ could also be level triggered.
> > 
> > See my reply to the other patch.
> > 
> >> > @@ -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.

Roger.

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