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

[Jason Andryuk <jason.andryuk@amd.com>]

On 2025-08-14 03:05, Jürgen Groß wrote:
> On 13.08.25 17:03, Jason Andryuk wrote:
>> On 2025-08-12 15:00, 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
> 
> This is what needs to be fixed. At migration the per_cpu virq_to_irq of the
> source and the target cpu need to be updated to reflect that migration.

I considered this, and even implemented it, before changing my approach. 
  My concern was that the single VIRQ is now in one of the N per_cpu 
virq_to_irq arrays.  A second attempt to register on CPU 0 will probably 
find -1 and continue and issue the hypercall.

It looks like Xen tracks virq on the bind_virq vcpu, so 
per-domain/global stays on vcpu0.  Binding again would return -EEXISTS. 
find_virq() would not match the virq if it was re-bound to a different vcpu.

If we don't care about handling duplicate registration, then updating 
the virq_to_irq tables should be is fine.

>>> 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@vger.kernel.org
>>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
>>> ---
>>> 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.
>>> ---
>>>   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:
>>
>> Thinking about it a little more, bind_virq_to_irq() should ensure cpu 
>> == 0 for per-domain and global VIRQs to ensure the property holds.  
>> Also virq_to_irq 
> 
> In Xen's evtchn_bind_virq() there is:
> 
>      if ( type != VIRQ_VCPU && vcpu != 0 )
>          return -EINVAL;
> 
> Making sure in Linux that there is never a violation of that restriction 
> would
> require to always have an up-to-date table of all possible VIRQs and their
> type, which I'd like to avoid.

Yes, I agree with this.

> I think it is the user of the VIRQ who is responsible to ensure cpu 0 is 
> passed
> to bind_virq_to_irq(), as this user knows that such a restriction 
> applies to
> the VIRQ in question (at least he should know that).
> 
> Special handling for really used VIRQs in the kernel can have some special
> handling, of course, as they are known already and should be used 
> correctly.

Thanks,
Jason