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

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

On Fri, Jun 22, 2018 at 08:23:02AM -0600, Jan Beulich wrote:
> >>> 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.

I've been thinking about this and I think the above comment is not
fully correct. Assertion of latched level interrupts is meaningful
because gsi_assert_count should be increased regardless of the state
of IRR.

Your comment made me realize that periodic level triggered interrupts
don't make much sense in vpt. IO-APIC level triggered interrupts will
require external deassertion of the line, in which case such
deassertion should also take care of reprogramming the timer if
required (like it's done in the next patch for vhpet when clearing
ISR).

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

Even if hvm_ioapic_assert returns -1 gsi_assert_count will be
increased, so I think it makes sense to also call the callback in this
case, because the line has been asserted.

Roger.

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