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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> so it might
> be more accurate to use pt->irq, which is the IRQ that was asserted by
> the vpt code.

I.e. I'm viewing it exactly the other way around - the guest can affect
pt->irq directly.

>> > +                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.

Oh, right, I was mislead by the comment above saying pt->irq when you
really mean pt[tn].irq

Jan



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