Re: [Xen-devel] [PATCH v2 1/2] x86/vpt: execute callbacks for masked interrupts

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

On Mon, Apr 09, 2018 at 09:34:57AM -0600, Jan Beulich wrote:
> >>> On 30.03.18 at 14:35, <roger.pau@xxxxxxxxxx> wrote:
> > Execute periodic_time callbacks even if the interrupt is not actually
> > injected because the IRQ is masked.
> > 
> > Current callbacks from emulated timer devices only update emulated
> > registers, which from my reading of the specs should happen regardless
> > of whether the interrupt has been injected or not.
> 
> While generally I agree, it also means extra work done. Looking
> at the PIT case, for example, there's no strict need to do the
> update when the IRQ is masked, as the value being updated is
> only used to subtract from get_guest_time()'s return value.
> Similarly for the LAPIC case.
> 
> In the RTC case your change actually looks risky, due to the
> pt_dead_ticks logic. I can't help getting the impression that the
> IRQ being off for 10 ticks would lead to no RTC interrupts at all
> anymore for the guest (until something resets that counter),
> which seems wrong to me.

Hm, right. The RTC is already handled specially in order to not
disable the timer but also don't call the handler if the IRQ is
masked.

Maybe the right solution is to add some flags to the vpt code,
something like:

 - DISABLE_ON_MASKED: only valid for periodic interrupts. Destroy the
   timer if the IRQ is masked when the timer fires.
 - SKIP_CALLBACK_ON_MASKED: do not execute the timer callback if the
   IRQ is masked when the timer fires.

That AFAICT should allow Xen to keep the previous behaviour for
existing timer code (and remove the RTC special casing).

> > @@ -282,6 +305,12 @@ int pt_update_irq(struct vcpu *v)
> >  
> >      if ( earliest_pt == NULL )
> >      {
> > +        /*
> > +         * NB: although the to_purge list is local, calls to
> > +         * destroy_periodic_time can still remove items from the list, 
> > hence
> 
> pt_adjust_vcpu() as well as it looks.
> 
> > +         * the need to hold the lock while accessing it.
> > +         */
> > +        execute_callbacks(v, &to_purge);
> >          spin_unlock(&v->arch.hvm_vcpu.tm_lock);
> >          return -1;
> >      }
> > @@ -290,6 +319,8 @@ int pt_update_irq(struct vcpu *v)
> >      irq = earliest_pt->irq;
> >      is_lapic = (earliest_pt->source == PTSRC_lapic);
> >  
> > +    execute_callbacks(v, &to_purge);
> > +
> >      spin_unlock(&v->arch.hvm_vcpu.tm_lock);
> 
> It seems to me that with your addition some code restructuring
> would actually be desirable, such that execute_callbacks() (and
> the lock release) would occur just once. Perhaps the mid-function
> return could be avoided altogether.

OK, I can do that. Let's first agree on the interface though.

Thanks, Roger.

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