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.

Hm, I can see your point, but that's going to make the implementation
of HPET level trigger interrupts more complex.

From my reading of the spec, when a level triggered HPET interrupt
fires the ISR bit must be set, regardless of whether the IRR of the
io-apic entry is set or not.

Maybe the right solution is to add a pt_irq_pending that checks the
IRR bit, and a new flag that the caller can set in order to requests
callbacks regardless of whether the interrupt has been injected or
not. Would you agree to that plan?

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

See my proposal above for adding a new option to signal whether the
callback should be executed regardless of whether interrupt injection
succeeded or not.

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

So something like:

pt->source == PTSRC_ioapic ? irq >= hvm_domain_irq(v->domain)->nr_gsis : level

Thanks, Roger.

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