Re: [Xen-devel] [PATCH 1/4] xen/events: Clear cpu_evtchn_mask before resuming

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 04/29/2015 12:32 PM, David Vrabel wrote:
> On 28/04/15 19:29, Boris Ostrovsky wrote:
> > On 04/28/2015 12:28 PM, David Vrabel wrote:
> > > On 28/04/15 16:52, Boris Ostrovsky wrote:
> > > > When a guest is resumed, the hypervisor may change event channel
> > > > assignments. If this happens and the guest uses 2-level events it
> > > > is possible for the interrupt to be claimed by wrong VCPU since
> > > > cpu_evtchn_mask bits may be stale. This can happen even though
> > > > evtchn_2l_bind_to_cpu() attempts to clear old bits: irq_info that
> > > > is passed in is not necessarily the original one (from pre-migration
> > > > times) but instead is freshly allocated during resume and so any
> > > > information about which CPU the channel was bound to is lost.
> > > >
> > > > Thus we should clear the mask during resume.
> > > >
> > > > We also need to make sure that bits for xenstore and console channels
> > > > are set when these two subsystems are resumed. While rebind_evtchn_irq()
> > > > (which is invoked for both of them on a resume) calls
> > > > irq_set_affinity(),
> > > > the latter will in fact postpone setting affinity until handling the
> > > > interrupt. But because cpu_evtchn_mask will have bits for these two
> > > > cleared
> > > > we won't be able to take the interrupt.
> > > >
> > > > Setting IRQ_MOVE_PCNTXT flag for the two irqs avoids this problem by
> > > > allowing to set affinity immediately, which is safe for
> > > > event-channel-based
> > > > interrupts.
> > > [...]
> > > > --- a/drivers/tty/hvc/hvc_xen.c
> > > > +++ b/drivers/tty/hvc/hvc_xen.c
> > > > @@ -533,6 +533,7 @@ static int __init xen_hvc_init(void)
> > > >              info = vtermno_to_xencons(HVC_COOKIE);
> > > >            info->irq = bind_evtchn_to_irq(info->evtchn);
> > > > +        irq_set_status_flags(info->irq, IRQ_MOVE_PCNTXT);
> > > >        }
> > > >        if (info->irq < 0)
> > > >            info->irq = 0; /* NO_IRQ */
> > > > diff --git a/drivers/xen/events/events_2l.c
> > > > b/drivers/xen/events/events_2l.c
> > > > index 5db43fc..7dd4631 100644
> > > > --- a/drivers/xen/events/events_2l.c
> > > > +++ b/drivers/xen/events/events_2l.c
> > > > @@ -345,6 +345,15 @@ irqreturn_t xen_debug_interrupt(int irq, void
> > > > *dev_id)
> > > >        return IRQ_HANDLED;
> > > >    }
> > > >    +static void evtchn_2l_resume(void)
> > > > +{
> > > > +    int i;
> > > > +
> > > > +    for_each_online_cpu(i)
> > > > +        memset(per_cpu(cpu_evtchn_mask, i), 0, sizeof(xen_ulong_t) *
> > > > +                EVTCHN_2L_NR_CHANNELS/BITS_PER_EVTCHN_WORD);
> > > > +}
> > > > +
> > > >    static const struct evtchn_ops evtchn_ops_2l = {
> > > >        .max_channels      = evtchn_2l_max_channels,
> > > >        .nr_channels       = evtchn_2l_max_channels,
> > > > @@ -356,6 +365,7 @@ static const struct evtchn_ops evtchn_ops_2l = {
> > > >        .mask              = evtchn_2l_mask,
> > > >        .unmask            = evtchn_2l_unmask,
> > > >        .handle_events     = evtchn_2l_handle_events,
> > > > +    .resume               = evtchn_2l_resume,
> > > >    };
> > > >      void __init xen_evtchn_2l_init(void)
> > > > diff --git a/drivers/xen/xenbus/xenbus_comms.c
> > > > b/drivers/xen/xenbus/xenbus_comms.c
> > > > index fdb0f33..30203d1 100644
> > > > --- a/drivers/xen/xenbus/xenbus_comms.c
> > > > +++ b/drivers/xen/xenbus/xenbus_comms.c
> > > > @@ -231,6 +231,7 @@ int xb_init_comms(void)
> > > >            }
> > > >              xenbus_irq = err;
> > > > +        irq_set_status_flags(xenbus_irq, IRQ_MOVE_PCNTXT);
> > > IRQ_MOVE_PCNTXT means "Interrupt can be migrated from process context"
> > > which doesn't really sound relevant to me here?
> > >
> > > Thomas Glexnier is really not happy with mis-use of IRQ APIs.
> > Yes, this is somewhat expanding definition of IRQ_MOVE_PCNTXT but then
> > there are other devices that do the same (HPET, for one).
> Ok.  But why set it on only these two event channels and not every one?
On resume in xen_irq_resume() we do

        for_each_possible_cpu(cpu) {
                restore_cpu_virqs(cpu);
                restore_cpu_ipis(cpu);
        }
        restore_pirqs();

And console and xenstore are neither of the those three types. 
rebind_irq_to_cpu() essentially plays the role of these restore_*() 
routines, except it does not make the hypercall to let the hypervisor 
assign notify_vcpu_id and it doesn't do explicit bind_evtchn_to_cpu().
Which makes me think that perhaps we should modify rebind_irq_to_cpu() 
to in fact do these two things. We may still need to keep 
irq_set_affinity() so that generic affinity data is properly set up but 
this will be deferred until the first interrupt.
> > >   From the commit log the evtchn_2l_resume() fucntion that's added sounds
> > > like it fixes the problem on its own?
> > It in fact makes this problem worse since now that cpu_evtchn_mask is
> > cleared during resume we cannot process the interrupt anymore in
> > evtchn_2l_handle_events(): irqs have to be bound to a cpu in order for
> > an interrupt to be processed.
> Perhaps evtchn_2l_resume() should set the local cpu mask for any bound
> event channels? And then you wouldn't need IRQ_MOVE_PCNTX.
But then (at least in 2-level case) more than one VCPUs may pick the 
same interrupt, won't they? Because the local cpu mask is what tells a 
VCPU that it is allowed to claim an interrupt.
-boris

> > We already have this issue even without clearing the mask if, say,
> > xenstore channel changes or if it gets bound to cpu other than 0 before
> > suspend. It's just that we rarely (if ever) see this happen.
> >
> > We can explicitly call events_base.c:set_affinity_irq() from
> > rebind_evtchn_irq() after irq_set_affinity() if we want to avoid using
> > IRQ_MOVE_PCNTXT flag but that will also looks kind of strange.
> David

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