Re: [PATCH] xen/events: Fix Global and Domain VIRQ tracking

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2025-08-12 15:10, Andrew Cooper wrote:
> On 12/08/2025 8:00 pm, Jason Andryuk wrote:
> > VIRQs come in 3 flavors, per-VPU, per-domain, and global.  The existing
> > tracking of VIRQs is handled by per-cpu variables virq_to_irq.
> >
> > The issue is that bind_virq_to_irq() sets the per_cpu virq_to_irq at
> > registration time - typically CPU 0.  Later, the interrupt can migrate,
> > and info->cpu is updated.  When calling unbind_from_irq(), the per-cpu
> > virq_to_irq is cleared for a different cpu.  If bind_virq_to_irq() is
> > called again with CPU 0, the stale irq is returned.
> >
> > Change the virq_to_irq tracking to use CPU 0 for per-domain and global
> > VIRQs.  As there can be at most one of each, there is no need for
> > per-vcpu tracking.  Also, per-domain and global VIRQs need to be
> > registered on CPU 0 and can later move, so this matches the expectation.
> >
> > Fixes: e46cdb66c8fc ("xen: event channels")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Jason Andryuk <jason.andryuk@xxxxxxx>
> > ---
> > Fixes is the introduction of the virq_to_irq per-cpu array.
> >
> > This was found with the out-of-tree argo driver during suspend/resume.
> > On suspend, the per-domain VIRQ_ARGO is unbound.  On resume, the driver
> > attempts to bind VIRQ_ARGO.  The stale irq is returned, but the
> > WARN_ON(info == NULL || info->type != IRQT_VIRQ) in bind_virq_to_irq()
> > triggers for NULL info.  The bind fails and execution continues with the
> > driver trying to clean up by unbinding.  This eventually faults over the
> > NULL info.
> I don't think the Fixes: tag is entirely appropriate.
>
> per-domain VIRQs were created (unexpectedly) by the merge of ARGO into
> Xen.  It was during some unrelated cleanup that this was noticed and
> bugfixed into working.  i.e. the ARGO VIRQ is the singular weird one here.
>
> In Xen we did accept that per-domain VIRQs now exist; they had for
> several releases before we realised.
AFAICT, global VIRQs have the same issue - I think they just aren't 
unbound and rebound.  I just happened to trigger this with the 
per-domain ARGO VIRQ.
I double checked, and the cpu is updated like so:

set_affinity_irq()
  rebind_irq_to_cpu()
    bind_evtchn_to_cpu()
      cpu_evtchn[chn] = cpu;

unbind_from_irq()
    case IRQT_VIRQ:
        per_cpu(virq_to_irq, cpu_from_evtchn(evtchn))
                [index_from_irq(irq)] = -1;

cpu_from_evtchn()
    return cpu_evtchn[evtchn];

So global VIRQs were mis-tracked even in the Fixes commit.

Regards,
Jason

> > ---
> >   drivers/xen/events/events_base.c | 17 ++++++++++++++++-
> >   1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
> > index 41309d38f78c..a27e4d7f061e 100644
> > --- a/drivers/xen/events/events_base.c
> > +++ b/drivers/xen/events/events_base.c
> > @@ -159,7 +159,19 @@ static DEFINE_MUTEX(irq_mapping_update_lock);
> >   
> >   static LIST_HEAD(xen_irq_list_head);
> >   
> > -/* IRQ <-> VIRQ mapping. */
> > +static bool is_per_vcpu_virq(int virq) {
> > +       switch (virq) {
> > +       case VIRQ_TIMER:
> > +       case VIRQ_DEBUG:
> > +       case VIRQ_XENOPROF:
> > +       case VIRQ_XENPMU:
> > +               return true;
> > +       default:
> > +               return false;
> > +       }
> > +}
> > +
> > +/* IRQ <-> VIRQ mapping.  Global/Domain virqs are tracked in cpu 0.  */
> >   static DEFINE_PER_CPU(int [NR_VIRQS], virq_to_irq) = {[0 ... NR_VIRQS-1] = 
> > -1};
> >   
> >   /* IRQ <-> IPI mapping */
> > @@ -974,6 +986,9 @@ static void __unbind_from_irq(struct irq_info *info, 
> > unsigned int irq)
> >   
> >   		switch (info->type) {
> >                 case IRQT_VIRQ:
> > +                       if (!is_per_vcpu_virq(virq_from_irq(info)))
> > +                               cpu = 0;
> > +
> >                         per_cpu(virq_to_irq, cpu)[virq_from_irq(info)] = -1;
> >                         break;
> >                 case IRQT_IPI: