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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

> @@ -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?

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

Jan



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