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

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

On Mon, Feb 26, 2018 at 12:35:54PM +0000, Wei Liu wrote:
> On Fri, Feb 23, 2018 at 01:27:41PM +0000, Roger Pau Monne 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.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Cc: Stefan Bader <stefan.bader@xxxxxxxxxxxxx>
> > ---
> >  xen/arch/x86/hvm/vpt.c | 30 +++++++++++++++++++++++++++++-
> >  1 file changed, 29 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
> > index 181f4cb631..1a24fbaa44 100644
> > --- a/xen/arch/x86/hvm/vpt.c
> > +++ b/xen/arch/x86/hvm/vpt.c
> > @@ -247,9 +247,31 @@ static void pt_timer_fn(void *data)
> >      pt_unlock(pt);
> >  }
> >  
> > +static void execute_callbacks(struct vcpu *v, struct list_head *tm)
> > +{
> > +    spin_lock(&v->arch.hvm_vcpu.tm_lock);
> > +    while ( !list_empty(tm) )
> > +    {
> > +        struct periodic_time *pt = list_first_entry(tm, struct 
> > periodic_time,
> > +                                                    list);
> > +        time_cb *cb = pt->cb;
> > +        void *cb_priv = pt->priv;
> > +
> > +        list_del(&pt->list);
> > +        pt->on_list = 0;
> > +        spin_unlock(&v->arch.hvm_vcpu.tm_lock);
> > +
> > +        cb(v, cb_priv);
> > +
> > +        spin_lock(&v->arch.hvm_vcpu.tm_lock);
> > +    }
> > +    spin_unlock(&v->arch.hvm_vcpu.tm_lock);
> > +}
> > +
> >  int pt_update_irq(struct vcpu *v)
> >  {
> >      struct list_head *head = &v->arch.hvm_vcpu.tm_list;
> > +    LIST_HEAD(purged);
> 
> to_purge?

My point is that they have already been purged from the pt->list, but
I really don't have a preference.

> >      struct periodic_time *pt, *temp, *earliest_pt;
> >      uint64_t max_lag;
> >      int irq, is_lapic, pt_vector;
> > @@ -267,7 +289,10 @@ int pt_update_irq(struct vcpu *v)
> >              {
> >                  /* suspend timer emulation */
> >                  list_del(&pt->list);
> > -                pt->on_list = 0;
> > +                if ( pt->cb )
> > +                    list_add(&pt->list, &purged);
> > +                else
> > +                    pt->on_list = 0;
> >              }
> >              else
> >              {
> > @@ -283,6 +308,7 @@ int pt_update_irq(struct vcpu *v)
> >      if ( earliest_pt == NULL )
> >      {
> >          spin_unlock(&v->arch.hvm_vcpu.tm_lock);
> > +        execute_callbacks(v, &purged);
> 
> It would be better to check if the list is not empty before calling the
> function to avoid the extra lock / unlock.

The lock is also protecting the 'purged' list, so I think that for
consistency the lock needs to be held before accessing it.

Since this is only a empty check *and* there can't be any
additions to the list at this point I guess it would be safe to test
for emptiness without holding the lock, but I find it kind of
confusing.

Thanks, Roger.

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