[PATCH v2 0/3] xen/events: Fix Global and Domain VIRQ tracking

[PATCH v2 0/3] xen/events: Fix Global and Domain VIRQ tracking

[Jason Andryuk]

v2 is now a series first changing find_virq() to return -EEXIST in some
cases, and then tracking cpu movement of VIRQs.  This keeps ensures the
cpu is correct when calling unbind_from_irq().

Jason Andryuk (3):
  xen/events: Cleanup find_virq() return codes
  xen/events: Return -EEXIST for bound VIRQs
  xen/events: Update virq_to_irq on migration

 drivers/xen/events/events_base.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

-- 
2.50.1

[PATCH v2 1/3] xen/events: Cleanup find_virq() return codes

[Jason Andryuk]

rc is overwritten by the evtchn_status hypercall in each iteration, so
the return value will be whatever the last iteration is.  Change to an
explicit -ENOENT for an un-found virq and return 0 on a successful
match.

Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
---
v2:
New
---
 drivers/xen/events/events_base.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 41309d38f78c..199afe59f357 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -1318,7 +1318,7 @@ static int find_virq(unsigned int virq, unsigned int cpu, evtchn_port_t *evtchn)
 {
 	struct evtchn_status status;
 	evtchn_port_t port;
-	int rc = -ENOENT;
+	int rc;
 
 	memset(&status, 0, sizeof(status));
 	for (port = 0; port < xen_evtchn_max_channels(); port++) {
@@ -1331,10 +1331,10 @@ static int find_virq(unsigned int virq, unsigned int cpu, evtchn_port_t *evtchn)
 			continue;
 		if (status.u.virq == virq && status.vcpu == xen_vcpu_nr(cpu)) {
 			*evtchn = port;
-			break;
+			return 0;
 		}
 	}
-	return rc;
+	return -ENOENT;
 }
 
 /**
-- 
2.50.1

[PATCH v2 2/3] xen/events: Return -EEXIST for bound VIRQs

[Jason Andryuk]

Change find_virq() to return -EEXIST when a VIRQ is bound to a
different CPU than the one passed in.  With that, remove the BUG_ON()
from bind_virq_to_irq() to propogate the error upwards.

Some VIRQs are per-cpu, but others are per-domain or global.  Those must
be bound to CPU0 and can then migrate elsewhere.  The lookup for
per-domain and global will probably fail when migrated off CPU 0,
especially when the current CPU is tracked.  This now returns -EEXIST
instead of BUG_ON().

A second call to bind a per-domain or global VIRQ is not expected, but
make it non-fatal to avoid trying to look up the irq, since we don't
know which per_cpu(virq_to_irq) it will be in.

Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
---
V2:
New
---
 drivers/xen/events/events_base.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 199afe59f357..a85bc43f4344 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -1314,10 +1314,12 @@ int bind_interdomain_evtchn_to_irq_lateeoi(struct xenbus_device *dev,
 }
 EXPORT_SYMBOL_GPL(bind_interdomain_evtchn_to_irq_lateeoi);
 
-static int find_virq(unsigned int virq, unsigned int cpu, evtchn_port_t *evtchn)
+static int find_virq(unsigned int virq, unsigned int cpu, evtchn_port_t *evtchn,
+		     bool percpu)
 {
 	struct evtchn_status status;
 	evtchn_port_t port;
+	bool exists = false;
 	int rc;
 
 	memset(&status, 0, sizeof(status));
@@ -1329,12 +1331,16 @@ static int find_virq(unsigned int virq, unsigned int cpu, evtchn_port_t *evtchn)
 			continue;
 		if (status.status != EVTCHNSTAT_virq)
 			continue;
-		if (status.u.virq == virq && status.vcpu == xen_vcpu_nr(cpu)) {
+		if (status.u.virq != virq)
+			continue;
+		if (status.vcpu == xen_vcpu_nr(cpu)) {
 			*evtchn = port;
 			return 0;
+		} else if (!percpu) {
+			exists = true;
 		}
 	}
-	return -ENOENT;
+	return exists ? -EEXIST : -ENOENT;
 }
 
 /**
@@ -1381,8 +1387,9 @@ int bind_virq_to_irq(unsigned int virq, unsigned int cpu, bool percpu)
 			evtchn = bind_virq.port;
 		else {
 			if (ret == -EEXIST)
-				ret = find_virq(virq, cpu, &evtchn);
-			BUG_ON(ret < 0);
+				ret = find_virq(virq, cpu, &evtchn, percpu);
+			if (ret)
+				goto out;
 		}
 
 		ret = xen_irq_info_virq_setup(info, cpu, evtchn, virq);
-- 
2.50.1

[PATCH v2 3/3] xen/events: Update virq_to_irq on migration

[Jason Andryuk]

VIRQs come in 3 flavors, per-VPU, per-domain, and global, and the VIRQs
are tracked in per-cpu virq_to_irq arrays.

Per-domain and global VIRQs must be bound on CPU 0, and
bind_virq_to_irq() sets the per_cpu virq_to_irq at registration time
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.  There won't be any irq_info for the irq, so
things break.

Make xen_rebind_evtchn_to_cpu() update the per_cpu virq_to_irq mappings
to keep them update to date with the current cpu.  This ensures the
correct virq_to_irq is cleared in __unbind_from_irq().

Fixes: e46cdb66c8fc ("xen: event channels")
Cc: stable@vger.kernel.org
Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
---
V2:
Different approach changing virq_to_irq
---
 drivers/xen/events/events_base.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index a85bc43f4344..4e9db7b92dde 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -1772,6 +1772,7 @@ static int xen_rebind_evtchn_to_cpu(struct irq_info *info, unsigned int tcpu)
 {
 	struct evtchn_bind_vcpu bind_vcpu;
 	evtchn_port_t evtchn = info ? info->evtchn : 0;
+	int old_cpu = info ? info->cpu : tcpu;
 
 	if (!VALID_EVTCHN(evtchn))
 		return -1;
@@ -1795,8 +1796,18 @@ static int xen_rebind_evtchn_to_cpu(struct irq_info *info, unsigned int tcpu)
 	 * it, but don't do the xenlinux-level rebind in that case.
 	 */
 	if (HYPERVISOR_event_channel_op(EVTCHNOP_bind_vcpu, &bind_vcpu) >= 0)
+	{
 		bind_evtchn_to_cpu(info, tcpu, false);
 
+		if (info->type == IRQT_VIRQ) {
+			int virq = info->u.virq;
+			int irq = per_cpu(virq_to_irq, old_cpu)[virq];
+
+			per_cpu(virq_to_irq, old_cpu)[virq] = -1;
+			per_cpu(virq_to_irq, tcpu)[virq] = irq;
+		}
+	}
+
 	do_unmask(info, EVT_MASK_REASON_TEMPORARY);
 
 	return 0;
-- 
2.50.1

Re: [PATCH v2 1/3] xen/events: Cleanup find_virq() return codes

[Jan Beulich]

On 26.08.2025 02:55, Jason Andryuk wrote:
> rc is overwritten by the evtchn_status hypercall in each iteration, so
> the return value will be whatever the last iteration is.

Which may even be a false "success". Especially for that it feels like ...

>  Change to an
> explicit -ENOENT for an un-found virq and return 0 on a successful
> match.
> 
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>

... this also wants a Fixes: tag and perhaps a Cc: to stable@.

> --- a/drivers/xen/events/events_base.c
> +++ b/drivers/xen/events/events_base.c
> @@ -1318,7 +1318,7 @@ static int find_virq(unsigned int virq, unsigned int cpu, evtchn_port_t *evtchn)
>  {
>  	struct evtchn_status status;
>  	evtchn_port_t port;
> -	int rc = -ENOENT;
> +	int rc;

Maybe best to also move this into the more narrow scope (loop body)?
Either way:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

>  	memset(&status, 0, sizeof(status));

Having this outside of the loop is a little odd, too: It makes assumptions
on the behavior of the hypervisor (like not altering the structure upon
error). Yet likely not something to deal with right here.

Jan

Re: [PATCH v2 1/3] xen/events: Cleanup find_virq() return codes

[Jason Andryuk]

On 2025-08-26 03:22, Jan Beulich wrote:
> On 26.08.2025 02:55, Jason Andryuk wrote:
>> rc is overwritten by the evtchn_status hypercall in each iteration, so
>> the return value will be whatever the last iteration is.
> 
> Which may even be a false "success". Especially for that it feels like ...

I'll state that here...

> 
>>   Change to an
>> explicit -ENOENT for an un-found virq and return 0 on a successful
>> match.
>>
>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> 
> ... this also wants a Fixes: tag and perhaps a Cc: to stable@.

and add these.

> 
>> --- a/drivers/xen/events/events_base.c
>> +++ b/drivers/xen/events/events_base.c
>> @@ -1318,7 +1318,7 @@ static int find_virq(unsigned int virq, unsigned int cpu, evtchn_port_t *evtchn)
>>   {
>>   	struct evtchn_status status;
>>   	evtchn_port_t port;
>> -	int rc = -ENOENT;
>> +	int rc;
> 
> Maybe best to also move this into the more narrow scope (loop body)?

Sounds good.

> Either way:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

> 
>>   	memset(&status, 0, sizeof(status));
> 
> Having this outside of the loop is a little odd, too: It makes assumptions
> on the behavior of the hypervisor (like not altering the structure upon
> error). Yet likely not something to deal with right here.

Agreed.

Thanks,
Jason

Re: [PATCH v2 1/3] xen/events: Cleanup find_virq() return codes

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1271 bytes --]

On 26.08.25 17:03, Jason Andryuk wrote:
> On 2025-08-26 03:22, Jan Beulich wrote:
>> On 26.08.2025 02:55, Jason Andryuk wrote:
>>> rc is overwritten by the evtchn_status hypercall in each iteration, so
>>> the return value will be whatever the last iteration is.
>>
>> Which may even be a false "success". Especially for that it feels like ...
> 
> I'll state that here...
> 
>>
>>>   Change to an
>>> explicit -ENOENT for an un-found virq and return 0 on a successful
>>> match.
>>>
>>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
>>
>> ... this also wants a Fixes: tag and perhaps a Cc: to stable@.
> 
> and add these.
> 
>>
>>> --- a/drivers/xen/events/events_base.c
>>> +++ b/drivers/xen/events/events_base.c
>>> @@ -1318,7 +1318,7 @@ static int find_virq(unsigned int virq, unsigned int 
>>> cpu, evtchn_port_t *evtchn)
>>>   {
>>>       struct evtchn_status status;
>>>       evtchn_port_t port;
>>> -    int rc = -ENOENT;
>>> +    int rc;
>>
>> Maybe best to also move this into the more narrow scope (loop body)?
> 
> Sounds good.
> 
>> Either way:
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>

With the changes you promised to do:

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 2/3] xen/events: Return -EEXIST for bound VIRQs

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 2766 bytes --]

On 26.08.25 02:55, Jason Andryuk wrote:
> Change find_virq() to return -EEXIST when a VIRQ is bound to a
> different CPU than the one passed in.  With that, remove the BUG_ON()
> from bind_virq_to_irq() to propogate the error upwards.
> 
> Some VIRQs are per-cpu, but others are per-domain or global.  Those must
> be bound to CPU0 and can then migrate elsewhere.  The lookup for
> per-domain and global will probably fail when migrated off CPU 0,
> especially when the current CPU is tracked.  This now returns -EEXIST
> instead of BUG_ON().
> 
> A second call to bind a per-domain or global VIRQ is not expected, but
> make it non-fatal to avoid trying to look up the irq, since we don't
> know which per_cpu(virq_to_irq) it will be in.
> 
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> ---
> V2:
> New
> ---
>   drivers/xen/events/events_base.c | 17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
> index 199afe59f357..a85bc43f4344 100644
> --- a/drivers/xen/events/events_base.c
> +++ b/drivers/xen/events/events_base.c
> @@ -1314,10 +1314,12 @@ int bind_interdomain_evtchn_to_irq_lateeoi(struct xenbus_device *dev,
>   }
>   EXPORT_SYMBOL_GPL(bind_interdomain_evtchn_to_irq_lateeoi);
>   
> -static int find_virq(unsigned int virq, unsigned int cpu, evtchn_port_t *evtchn)
> +static int find_virq(unsigned int virq, unsigned int cpu, evtchn_port_t *evtchn,
> +		     bool percpu)
>   {
>   	struct evtchn_status status;
>   	evtchn_port_t port;
> +	bool exists = false;
>   	int rc;
>   
>   	memset(&status, 0, sizeof(status));
> @@ -1329,12 +1331,16 @@ static int find_virq(unsigned int virq, unsigned int cpu, evtchn_port_t *evtchn)
>   			continue;
>   		if (status.status != EVTCHNSTAT_virq)
>   			continue;
> -		if (status.u.virq == virq && status.vcpu == xen_vcpu_nr(cpu)) {
> +		if (status.u.virq != virq)
> +			continue;
> +		if (status.vcpu == xen_vcpu_nr(cpu)) {
>   			*evtchn = port;
>   			return 0;
> +		} else if (!percpu) {
> +			exists = true;
>   		}
>   	}
> -	return -ENOENT;
> +	return exists ? -EEXIST : -ENOENT;
>   }
>   
>   /**
> @@ -1381,8 +1387,9 @@ int bind_virq_to_irq(unsigned int virq, unsigned int cpu, bool percpu)
>   			evtchn = bind_virq.port;
>   		else {
>   			if (ret == -EEXIST)
> -				ret = find_virq(virq, cpu, &evtchn);
> -			BUG_ON(ret < 0);
> +				ret = find_virq(virq, cpu, &evtchn, percpu);
> +			if (ret)
> +				goto out;

I think you are leaking info here. I guess a call of __unbind_from_irq() is
wanted like in the error case below (note that the case of no valid evtchn is
handled there just fine).


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 3/3] xen/events: Update virq_to_irq on migration

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 2470 bytes --]

On 26.08.25 02:55, Jason Andryuk wrote:
> VIRQs come in 3 flavors, per-VPU, per-domain, and global, and the VIRQs
> are tracked in per-cpu virq_to_irq arrays.
> 
> Per-domain and global VIRQs must be bound on CPU 0, and
> bind_virq_to_irq() sets the per_cpu virq_to_irq at registration time
> 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.  There won't be any irq_info for the irq, so
> things break.
> 
> Make xen_rebind_evtchn_to_cpu() update the per_cpu virq_to_irq mappings
> to keep them update to date with the current cpu.  This ensures the
> correct virq_to_irq is cleared in __unbind_from_irq().
> 
> Fixes: e46cdb66c8fc ("xen: event channels")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> ---
> V2:
> Different approach changing virq_to_irq
> ---
>   drivers/xen/events/events_base.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
> index a85bc43f4344..4e9db7b92dde 100644
> --- a/drivers/xen/events/events_base.c
> +++ b/drivers/xen/events/events_base.c
> @@ -1772,6 +1772,7 @@ static int xen_rebind_evtchn_to_cpu(struct irq_info *info, unsigned int tcpu)
>   {
>   	struct evtchn_bind_vcpu bind_vcpu;
>   	evtchn_port_t evtchn = info ? info->evtchn : 0;
> +	int old_cpu = info ? info->cpu : tcpu;

I'd prefer not to initialize old_cpu just here ...

>   
>   	if (!VALID_EVTCHN(evtchn))
>   		return -1;

... as here info is always valid, so you can just use "old_cpu = info->cpu;"
(probably just after the hypercall).

> @@ -1795,8 +1796,18 @@ static int xen_rebind_evtchn_to_cpu(struct irq_info *info, unsigned int tcpu)
>   	 * it, but don't do the xenlinux-level rebind in that case.
>   	 */
>   	if (HYPERVISOR_event_channel_op(EVTCHNOP_bind_vcpu, &bind_vcpu) >= 0)
> +	{

Kernel style, please.

>   		bind_evtchn_to_cpu(info, tcpu, false);
>   
> +		if (info->type == IRQT_VIRQ) {
> +			int virq = info->u.virq;
> +			int irq = per_cpu(virq_to_irq, old_cpu)[virq];
> +
> +			per_cpu(virq_to_irq, old_cpu)[virq] = -1;
> +			per_cpu(virq_to_irq, tcpu)[virq] = irq;
> +		}
> +	}
> +
>   	do_unmask(info, EVT_MASK_REASON_TEMPORARY);
>   
>   	return 0;


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 2/3] xen/events: Return -EEXIST for bound VIRQs

[Jason Andryuk]

On 2025-08-27 11:21, Jürgen Groß wrote:
> On 26.08.25 02:55, Jason Andryuk wrote:
>> Change find_virq() to return -EEXIST when a VIRQ is bound to a
>> different CPU than the one passed in.  With that, remove the BUG_ON()
>> from bind_virq_to_irq() to propogate the error upwards.
>>
>> Some VIRQs are per-cpu, but others are per-domain or global.  Those must
>> be bound to CPU0 and can then migrate elsewhere.  The lookup for
>> per-domain and global will probably fail when migrated off CPU 0,
>> especially when the current CPU is tracked.  This now returns -EEXIST
>> instead of BUG_ON().
>>
>> A second call to bind a per-domain or global VIRQ is not expected, but
>> make it non-fatal to avoid trying to look up the irq, since we don't
>> know which per_cpu(virq_to_irq) it will be in.
>>
>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
>
>> @@ -1381,8 +1387,9 @@ int bind_virq_to_irq(unsigned int virq, unsigned 
>> int cpu, bool percpu)
>>               evtchn = bind_virq.port;
>>           else {
>>               if (ret == -EEXIST)
>> -                ret = find_virq(virq, cpu, &evtchn);
>> -            BUG_ON(ret < 0);
>> +                ret = find_virq(virq, cpu, &evtchn, percpu);
>> +            if (ret)
>> +                goto out;
> 
> I think you are leaking info here. I guess a call of __unbind_from_irq() is
> wanted like in the error case below (note that the case of no valid 
> evtchn is
> handled there just fine).

Ok, thanks for catching that.

I'm going to add Cc: stable to the next version of this.  While it 
doesn't have a Fixes associated, we want this as a prerequisite for patch 3.

Regards,
Jason