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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.06.18 at 17:07, <roger.pau@xxxxxxxxxx> wrote:
> @@ -316,7 +317,9 @@ int pt_update_irq(struct vcpu *v)
>          if ( pt->pending_intr_nr )
>          {
>              /* RTC code takes care of disabling the timer itself. */
> -            if ( (pt->irq != RTC_IRQ || !pt->priv) && pt_irq_masked(pt) )
> +            if ( (pt->irq != RTC_IRQ || !pt->priv) && pt_irq_masked(pt) &&
> +                 /* Level interrupts should be asserted even if masked. */
> +                 !pt->level )
>              {
>                  /* suspend timer emulation */

Especially with this comment I'm not convinced this change is fully
correct: Once a level triggered interrupt is latched in IRR, no
further assertions are meaningful, and hence emulation could (and
hence should) still be stopped to reduce resource consumption.

> @@ -374,13 +378,36 @@ int pt_update_irq(struct vcpu *v)
>          break;
>  
>      case PTSRC_ioapic:
> -        /*
> -         * NB: At the moment IO-APIC routed interrupts generated by vpt 
> devices
> -         * (HPET) are edge-triggered.
> -         */
> -        pt_vector = hvm_ioapic_assert(v->domain, irq, false);
> +        pt_vector = hvm_ioapic_assert(v->domain, irq, level);
>          if ( pt_vector < 0 || !vlapic_test_irq(vcpu_vlapic(v), pt_vector) )
> +        {
>              pt_vector = -1;
> +            if ( level )
> +            {
> +                /*
> +                 * Level interrupts are asserted even if the interrupt is
> +                 * masked, so also execute the callback associated with the
> +                 * timer.
> +                 */
> +                time_cb *cb = NULL;
> +                void *cb_priv;
> +
> +                spin_lock(&v->arch.hvm_vcpu.tm_lock);
> +                /* Make sure the timer is still on the list. */
> +                list_for_each_entry ( pt, &v->arch.hvm_vcpu.tm_list, list )
> +                    if ( pt == earliest_pt )
> +                    {
> +                        pt_irq_fired(v, pt);
> +                        cb = pt->cb;
> +                        cb_priv = pt->priv;
> +                        break;
> +                    }
> +                spin_unlock(&v->arch.hvm_vcpu.tm_lock);
> +
> +                if ( cb != NULL )
> +                    cb(v, cb_priv);
> +            }
> +        }
>          break;

I'm not fully convinced, especially in the case that hvm_ioapic_assert()
returned a negative value: Either the callback needs to be called in all
cases (even if an edge triggered interrupt was not successfully asserted),
or only when an interrupt really gets surfaced to the guest.

> @@ -447,12 +474,13 @@ void pt_migrate(struct vcpu *v)
>  
>  void create_periodic_time(
>      struct vcpu *v, struct periodic_time *pt, uint64_t delta,
> -    uint64_t period, uint8_t irq, time_cb *cb, void *data)
> +    uint64_t period, uint8_t irq, time_cb *cb, void *data, bool level)
>  {
>      if ( !pt->source ||
>           (irq >= NR_ISAIRQS && pt->source == PTSRC_isa) ||
>           (irq >= hvm_domain_irq(v->domain)->nr_gsis &&
> -          pt->source == PTSRC_ioapic) )
> +          pt->source == PTSRC_ioapic) ||
> +         (level && pt->source != PTSRC_ioapic) )

Could I talk you into avoiding the double checking against PTSRC_ioapic,
by using a conditional expression?

Jan



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