Re: [Xen-devel] [PATCH v3 11/13] xen/arm: send IPIs to inject irqs into guest vcpus running on different pcpus

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2013-04-26 at 14:39 +0100, Stefano Stabellini wrote:
> On Thu, 25 Apr 2013, Ian Campbell wrote:
> > On Wed, 2013-04-24 at 20:07 +0100, Stefano Stabellini wrote:
> > > If we need to inject an irq into a guest that is running on a different
> > 
> >                                      ^VCPU
> > 
> > > processor, we shouldn't just enqueue the irq into the lr_pending and
> > > inflight lists and wait for something to interrupt the guest execution.
> > > Send an IPI to the target pcpu so that Xen can inject the new interrupt
> > > returning to guest.
> > > 
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > 
> > Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > 
> > > ---
> > >  xen/arch/arm/vgic.c |    2 ++
> > >  1 files changed, 2 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > > index 8912aab..547a2ab 100644
> > > --- a/xen/arch/arm/vgic.c
> > > +++ b/xen/arch/arm/vgic.c
> > > @@ -664,6 +664,8 @@ out:
> > >      spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> > >      /* we have a new higher priority irq, inject it into the guest */
> > >      vcpu_unblock(v);
> > > +    if ( v != current && v->is_running )
> > > +        smp_send_event_check_mask(cpumask_of(v->processor));
> > 
> > I'm a bit concerned about races here, e.g. where v is busy going to
> > sleep.
> > I don't know enough about the behaviour of the scheduler and
> > vcpu_unblock etc to say, but it worries me...
>  
> That is that the vcpu_unblock call above is for.
> I think that should be enough to make sure that the scheduler doesn't
> shelve the vcpu.

If the cpu is already running then vcpu_unblock won't do anything
(useful).

If between vcpu_unblock and if ( v->is_running ) the vcpu, currently
running on another PCPU, blocks then you will not wake it up.

> > x86 has vcpu_kick which saves v->is_running before the unblock and then
> > sends a softirq with much the same checks as you have -- that seems like
> > a plausible race avoidance strategy, although whether IPI vs softirq is
> > an important distinction I've no idea...
> 
> Yes, it looks like the code should be race free, although it might be
> possible to improve it by checking is_running before vcpu_unblock to
> avoid a useless IPI. I could make that change.

I think you should, even if I'm worrying about nothing it is more
obviously correct and a spurious wake is better than a missed one.

> I don't think that the softirq vs IPI thing matters: the purpose is to
> force the other vcpu to check for events in the near future, the exact
> point in time shouldn't make a difference.

The comment on x86 suggests that the softirq serves a wider purpose,
which is why I was asking:
    /*
     * Nothing to do here: we merely prevent notifiers from racing with checks
     * executed on return to guest context with interrupts enabled. See, for
     * example, xxx_intr_assist() executed on return to HVM guest context.
     */

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel