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 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, so it might
be more accurate to use pt->irq, which is the IRQ that was asserted by
the vpt code. In any case, I'm not specially trilled because a guest
changing this while the interrupt is active is certainly asking for
trouble.

> > +                hvm_ioapic_deassert(v->domain, timer_int_route(h, i));
> > +                if ( hpet_enabled(h) && timer_enabled(h, i) &&
> > +                     timer_level(h, i) && timer_is_periodic(h, i) )
> > +                    set_start_timer(i);
> > +            }
> > +        }
> > +        break;
> 
> What I'm wondering though: Does there really need to be a loop here?
> How would more than one bit get set in h->hpet.isr?

The current HPET code exposes 3 timers, and all of them can be set to
level triggered, so in theory you could clear the 3 ISR bits with one
write, hence the loop.

Roger.

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