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

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

On Mon, Jul 02, 2018 at 09:09:31AM -0600, Jan Beulich wrote:
> >>> On 02.07.18 at 16:54, <roger.pau@xxxxxxxxxx> wrote:
> > Ping?
> 
> I don't understand: There's no open question in the quoted mail.

It's more of a refute of your argument about assert of level triggered
interrupts not being meaningful if the interrupt is latched in IRR,
see below.

> > On Mon, Jun 25, 2018 at 01:19:19PM +0200, Roger Pau Monné wrote:
> >> 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.

As said, I think that an assert of a level triggered interrupt is
still meaningful, because gsi_assert_count should be incremented
regardless of the state of the 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).

I think level triggered interrupts should not be allowed in periodic
mode in vpt, since a level triggered interrupt requires an external
deassertion, that should take care of setting the vpt timer while
deasserting the line.

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

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