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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan

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




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