[PATCH 0/7] xen/events: do some cleanups in events_base.c

[PATCH 0/7] xen/events: do some cleanups in events_base.c

[Juergen Gross]

As a followup to the XSA-441 patch this series is doing a minor bug fix
and some cleanups of events_base.c (with some minor effects outside of
it).

Juergen Gross (7):
  xen/events: fix delayed eoi list handling
  xen/events: remove unused functions
  xen/events: reduce externally visible helper functions
  xen/events: remove some simple helpers from events_base.c
  xen/events: drop xen_allocate_irqs_dynamic()
  xen/events: modify internal [un]bind interfaces
  xen/events: remove some info_for_irq() calls in pirq handling

 drivers/xen/events/events_2l.c       |   8 +-
 drivers/xen/events/events_base.c     | 557 +++++++++++++--------------
 drivers/xen/events/events_internal.h |   1 -
 include/xen/events.h                 |   8 +-
 4 files changed, 276 insertions(+), 298 deletions(-)

-- 
2.35.3

[PATCH 1/7] xen/events: fix delayed eoi list handling

[Juergen Gross]

When delaying eoi handling of events, the related elements are queued
into the percpu lateeoi list. In case the list isn't empty, the
elements should be sorted by the time when eoi handling is to happen.

Unfortunately a new element will never be queued at the start of the
list, even if it has a handling time lower than all other list
elements.

Fix that by handling that case the same way as for an empty list.

Fixes: e99502f76271 ("xen/events: defer eoi in case of excessive number of events")
Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/events/events_base.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 1b2136fe0fa5..0e458b1c0c8c 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -601,7 +601,9 @@ static void lateeoi_list_add(struct irq_info *info)
 
 	spin_lock_irqsave(&eoi->eoi_list_lock, flags);
 
-	if (list_empty(&eoi->eoi_list)) {
+	elem = list_first_entry_or_null(&eoi->eoi_list, struct irq_info,
+					eoi_list);
+	if (!elem || info->eoi_time < elem->eoi_time) {
 		list_add(&info->eoi_list, &eoi->eoi_list);
 		mod_delayed_work_on(info->eoi_cpu, system_wq,
 				    &eoi->delayed, delay);
-- 
2.35.3

Re: [PATCH 1/7] xen/events: fix delayed eoi list handling

[Oleksandr Tyshchenko]

On 16.10.23 09:28, Juergen Gross wrote:


Hello Juergen

> When delaying eoi handling of events, the related elements are queued
> into the percpu lateeoi list. In case the list isn't empty, the
> elements should be sorted by the time when eoi handling is to happen.
> 
> Unfortunately a new element will never be queued at the start of the
> list, even if it has a handling time lower than all other list
> elements.
> 
> Fix that by handling that case the same way as for an empty list.
> 
> Fixes: e99502f76271 ("xen/events: defer eoi in case of excessive number of events")
> Reported-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Juergen Gross <jgross@suse.com>


Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

> ---
>   drivers/xen/events/events_base.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
> index 1b2136fe0fa5..0e458b1c0c8c 100644
> --- a/drivers/xen/events/events_base.c
> +++ b/drivers/xen/events/events_base.c
> @@ -601,7 +601,9 @@ static void lateeoi_list_add(struct irq_info *info)
>   
>   	spin_lock_irqsave(&eoi->eoi_list_lock, flags);
>   
> -	if (list_empty(&eoi->eoi_list)) {
> +	elem = list_first_entry_or_null(&eoi->eoi_list, struct irq_info,
> +					eoi_list);
> +	if (!elem || info->eoi_time < elem->eoi_time) {
>   		list_add(&info->eoi_list, &eoi->eoi_list);
>   		mod_delayed_work_on(info->eoi_cpu, system_wq,
>   				    &eoi->delayed, delay);

[PATCH 2/7] xen/events: remove unused functions

[Juergen Gross]

There are no users of xen_irq_from_pirq() and xen_set_irq_pending().

Remove those functions.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/events/events_base.c | 30 ------------------------------
 include/xen/events.h             |  4 ----
 2 files changed, 34 deletions(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 0e458b1c0c8c..1d797dd85d0e 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -1165,29 +1165,6 @@ int xen_destroy_irq(int irq)
 	return rc;
 }
 
-int xen_irq_from_pirq(unsigned pirq)
-{
-	int irq;
-
-	struct irq_info *info;
-
-	mutex_lock(&irq_mapping_update_lock);
-
-	list_for_each_entry(info, &xen_irq_list_head, list) {
-		if (info->type != IRQT_PIRQ)
-			continue;
-		irq = info->irq;
-		if (info->u.pirq.pirq == pirq)
-			goto out;
-	}
-	irq = -1;
-out:
-	mutex_unlock(&irq_mapping_update_lock);
-
-	return irq;
-}
-
-
 int xen_pirq_from_irq(unsigned irq)
 {
 	return pirq_from_irq(irq);
@@ -2026,13 +2003,6 @@ void xen_clear_irq_pending(int irq)
 		event_handler_exit(info);
 }
 EXPORT_SYMBOL(xen_clear_irq_pending);
-void xen_set_irq_pending(int irq)
-{
-	evtchn_port_t evtchn = evtchn_from_irq(irq);
-
-	if (VALID_EVTCHN(evtchn))
-		set_evtchn(evtchn);
-}
 
 bool xen_test_irq_pending(int irq)
 {
diff --git a/include/xen/events.h b/include/xen/events.h
index 23932b0673dc..a129cafa80ed 100644
--- a/include/xen/events.h
+++ b/include/xen/events.h
@@ -88,7 +88,6 @@ void xen_irq_resume(void);
 
 /* Clear an irq's pending state, in preparation for polling on it */
 void xen_clear_irq_pending(int irq);
-void xen_set_irq_pending(int irq);
 bool xen_test_irq_pending(int irq);
 
 /* Poll waiting for an irq to become pending.  In the usual case, the
@@ -122,9 +121,6 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc,
 /* De-allocates the above mentioned physical interrupt. */
 int xen_destroy_irq(int irq);
 
-/* Return irq from pirq */
-int xen_irq_from_pirq(unsigned pirq);
-
 /* Return the pirq allocated to the irq. */
 int xen_pirq_from_irq(unsigned irq);
 
-- 
2.35.3

Re: [PATCH 2/7] xen/events: remove unused functions

[Oleksandr Tyshchenko]

On 16.10.23 09:28, Juergen Gross wrote:

Hello Juergen


> There are no users of xen_irq_from_pirq() and xen_set_irq_pending().
> 
> Remove those functions.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>


Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>


> ---
>   drivers/xen/events/events_base.c | 30 ------------------------------
>   include/xen/events.h             |  4 ----
>   2 files changed, 34 deletions(-)
> 
> diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
> index 0e458b1c0c8c..1d797dd85d0e 100644
> --- a/drivers/xen/events/events_base.c
> +++ b/drivers/xen/events/events_base.c
> @@ -1165,29 +1165,6 @@ int xen_destroy_irq(int irq)
>   	return rc;
>   }
>   
> -int xen_irq_from_pirq(unsigned pirq)
> -{
> -	int irq;
> -
> -	struct irq_info *info;
> -
> -	mutex_lock(&irq_mapping_update_lock);
> -
> -	list_for_each_entry(info, &xen_irq_list_head, list) {
> -		if (info->type != IRQT_PIRQ)
> -			continue;
> -		irq = info->irq;
> -		if (info->u.pirq.pirq == pirq)
> -			goto out;
> -	}
> -	irq = -1;
> -out:
> -	mutex_unlock(&irq_mapping_update_lock);
> -
> -	return irq;
> -}
> -
> -
>   int xen_pirq_from_irq(unsigned irq)
>   {
>   	return pirq_from_irq(irq);
> @@ -2026,13 +2003,6 @@ void xen_clear_irq_pending(int irq)
>   		event_handler_exit(info);
>   }
>   EXPORT_SYMBOL(xen_clear_irq_pending);
> -void xen_set_irq_pending(int irq)
> -{
> -	evtchn_port_t evtchn = evtchn_from_irq(irq);
> -
> -	if (VALID_EVTCHN(evtchn))
> -		set_evtchn(evtchn);
> -}
>   
>   bool xen_test_irq_pending(int irq)
>   {
> diff --git a/include/xen/events.h b/include/xen/events.h
> index 23932b0673dc..a129cafa80ed 100644
> --- a/include/xen/events.h
> +++ b/include/xen/events.h
> @@ -88,7 +88,6 @@ void xen_irq_resume(void);
>   
>   /* Clear an irq's pending state, in preparation for polling on it */
>   void xen_clear_irq_pending(int irq);
> -void xen_set_irq_pending(int irq);
>   bool xen_test_irq_pending(int irq);
>   
>   /* Poll waiting for an irq to become pending.  In the usual case, the
> @@ -122,9 +121,6 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc,
>   /* De-allocates the above mentioned physical interrupt. */
>   int xen_destroy_irq(int irq);
>   
> -/* Return irq from pirq */
> -int xen_irq_from_pirq(unsigned pirq);
> -
>   /* Return the pirq allocated to the irq. */
>   int xen_pirq_from_irq(unsigned irq);
>   

[PATCH 3/7] xen/events: reduce externally visible helper functions

[Juergen Gross]

get_evtchn_to_irq() has only one external user while irq_from_evtchn()
provides the same functionality and is exported for a wider user base.
Modify the only external user of get_evtchn_to_irq() to use
irq_from_evtchn() instead and make get_evtchn_to_irq() static.

evtchn_from_irq() and irq_from_virq() have a single external user and
can easily be combined to a new helper irq_evtchn_from_virq() allowing
to drop irq_from_virq() and to make evtchn_from_irq() static.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/events/events_2l.c       |  8 ++++----
 drivers/xen/events/events_base.c     | 13 +++++++++----
 drivers/xen/events/events_internal.h |  1 -
 include/xen/events.h                 |  4 ++--
 4 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/xen/events/events_2l.c b/drivers/xen/events/events_2l.c
index b8f2f971c2f0..e3585330cf98 100644
--- a/drivers/xen/events/events_2l.c
+++ b/drivers/xen/events/events_2l.c
@@ -171,11 +171,11 @@ static void evtchn_2l_handle_events(unsigned cpu, struct evtchn_loop_ctrl *ctrl)
 	int i;
 	struct shared_info *s = HYPERVISOR_shared_info;
 	struct vcpu_info *vcpu_info = __this_cpu_read(xen_vcpu);
+	evtchn_port_t evtchn;
 
 	/* Timer interrupt has highest priority. */
-	irq = irq_from_virq(cpu, VIRQ_TIMER);
+	irq = irq_evtchn_from_virq(cpu, VIRQ_TIMER, &evtchn);
 	if (irq != -1) {
-		evtchn_port_t evtchn = evtchn_from_irq(irq);
 		word_idx = evtchn / BITS_PER_LONG;
 		bit_idx = evtchn % BITS_PER_LONG;
 		if (active_evtchns(cpu, s, word_idx) & (1ULL << bit_idx))
@@ -328,9 +328,9 @@ irqreturn_t xen_debug_interrupt(int irq, void *dev_id)
 	for (i = 0; i < EVTCHN_2L_NR_CHANNELS; i++) {
 		if (sync_test_bit(i, BM(sh->evtchn_pending))) {
 			int word_idx = i / BITS_PER_EVTCHN_WORD;
-			printk("  %d: event %d -> irq %d%s%s%s\n",
+			printk("  %d: event %d -> irq %u%s%s%s\n",
 			       cpu_from_evtchn(i), i,
-			       get_evtchn_to_irq(i),
+			       irq_from_evtchn(i),
 			       sync_test_bit(word_idx, BM(&v->evtchn_pending_sel))
 			       ? "" : " l2-clear",
 			       !sync_test_bit(i, BM(sh->evtchn_mask))
diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 1d797dd85d0e..97d71c5e7c28 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -246,7 +246,7 @@ static int set_evtchn_to_irq(evtchn_port_t evtchn, unsigned int irq)
 	return 0;
 }
 
-int get_evtchn_to_irq(evtchn_port_t evtchn)
+static int get_evtchn_to_irq(evtchn_port_t evtchn)
 {
 	if (evtchn >= xen_evtchn_max_channels())
 		return -1;
@@ -412,7 +412,7 @@ static void xen_irq_info_cleanup(struct irq_info *info)
 /*
  * Accessors for packed IRQ information.
  */
-evtchn_port_t evtchn_from_irq(unsigned irq)
+static evtchn_port_t evtchn_from_irq(unsigned int irq)
 {
 	const struct irq_info *info = NULL;
 
@@ -430,9 +430,14 @@ unsigned int irq_from_evtchn(evtchn_port_t evtchn)
 }
 EXPORT_SYMBOL_GPL(irq_from_evtchn);
 
-int irq_from_virq(unsigned int cpu, unsigned int virq)
+int irq_evtchn_from_virq(unsigned int cpu, unsigned int virq,
+			 evtchn_port_t *evtchn)
 {
-	return per_cpu(virq_to_irq, cpu)[virq];
+	int irq = per_cpu(virq_to_irq, cpu)[virq];
+
+	*evtchn = evtchn_from_irq(irq);
+
+	return irq;
 }
 
 static enum ipi_vector ipi_from_irq(unsigned irq)
diff --git a/drivers/xen/events/events_internal.h b/drivers/xen/events/events_internal.h
index 4d3398eff9cd..19ae31695edc 100644
--- a/drivers/xen/events/events_internal.h
+++ b/drivers/xen/events/events_internal.h
@@ -33,7 +33,6 @@ struct evtchn_ops {
 
 extern const struct evtchn_ops *evtchn_ops;
 
-int get_evtchn_to_irq(evtchn_port_t evtchn);
 void handle_irq_for_port(evtchn_port_t port, struct evtchn_loop_ctrl *ctrl);
 
 unsigned int cpu_from_evtchn(evtchn_port_t evtchn);
diff --git a/include/xen/events.h b/include/xen/events.h
index a129cafa80ed..3b07409f8032 100644
--- a/include/xen/events.h
+++ b/include/xen/events.h
@@ -100,8 +100,8 @@ void xen_poll_irq_timeout(int irq, u64 timeout);
 
 /* Determine the IRQ which is bound to an event channel */
 unsigned int irq_from_evtchn(evtchn_port_t evtchn);
-int irq_from_virq(unsigned int cpu, unsigned int virq);
-evtchn_port_t evtchn_from_irq(unsigned irq);
+int irq_evtchn_from_virq(unsigned int cpu, unsigned int virq,
+			 evtchn_port_t *evtchn);
 
 int xen_set_callback_via(uint64_t via);
 int xen_evtchn_do_upcall(void);
-- 
2.35.3

Re: [PATCH 3/7] xen/events: reduce externally visible helper functions

[Oleksandr Tyshchenko]

On 16.10.23 09:28, Juergen Gross wrote:


Hello Juergen

> get_evtchn_to_irq() has only one external user while irq_from_evtchn()
> provides the same functionality and is exported for a wider user base.
> Modify the only external user of get_evtchn_to_irq() to use
> irq_from_evtchn() instead and make get_evtchn_to_irq() static.
> 
> evtchn_from_irq() and irq_from_virq() have a single external user and
> can easily be combined to a new helper irq_evtchn_from_virq() allowing
> to drop irq_from_virq() and to make evtchn_from_irq() static.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>


Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Two NITs *NOT* directly related to current patch below.


> ---
>   drivers/xen/events/events_2l.c       |  8 ++++----
>   drivers/xen/events/events_base.c     | 13 +++++++++----
>   drivers/xen/events/events_internal.h |  1 -
>   include/xen/events.h                 |  4 ++--
>   4 files changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/xen/events/events_2l.c b/drivers/xen/events/events_2l.c
> index b8f2f971c2f0..e3585330cf98 100644
> --- a/drivers/xen/events/events_2l.c
> +++ b/drivers/xen/events/events_2l.c
> @@ -171,11 +171,11 @@ static void evtchn_2l_handle_events(unsigned cpu, struct evtchn_loop_ctrl *ctrl)
>   	int i;
>   	struct shared_info *s = HYPERVISOR_shared_info;
>   	struct vcpu_info *vcpu_info = __this_cpu_read(xen_vcpu);
> +	evtchn_port_t evtchn;
>   
>   	/* Timer interrupt has highest priority. */
> -	irq = irq_from_virq(cpu, VIRQ_TIMER);
> +	irq = irq_evtchn_from_virq(cpu, VIRQ_TIMER, &evtchn);
>   	if (irq != -1) {
> -		evtchn_port_t evtchn = evtchn_from_irq(irq);

Most users of evtchn_from_irq() check returned evtchn via VALID_EVTCHN() 
as it might be 0. But this user doesn't.


>   		word_idx = evtchn / BITS_PER_LONG;
>   		bit_idx = evtchn % BITS_PER_LONG;
>   		if (active_evtchns(cpu, s, word_idx) & (1ULL << bit_idx))
> @@ -328,9 +328,9 @@ irqreturn_t xen_debug_interrupt(int irq, void *dev_id)
>   	for (i = 0; i < EVTCHN_2L_NR_CHANNELS; i++) {
>   		if (sync_test_bit(i, BM(sh->evtchn_pending))) {
>   			int word_idx = i / BITS_PER_EVTCHN_WORD;
> -			printk("  %d: event %d -> irq %d%s%s%s\n",
> +			printk("  %d: event %d -> irq %u%s%s%s\n",

checkpatch.pl says:

WARNING: printk() should include KERN_<LEVEL> facility level
#37: FILE: drivers/xen/events/events_2l.c:331:
+			printk("  %d: event %d -> irq %u%s%s%s\n",


>   			       cpu_from_evtchn(i), i,
> -			       get_evtchn_to_irq(i),
> +			       irq_from_evtchn(i),
>   			       sync_test_bit(word_idx, BM(&v->evtchn_pending_sel))
>   			       ? "" : " l2-clear",
>   			       !sync_test_bit(i, BM(sh->evtchn_mask))
> 

[snip]

[PATCH 4/7] xen/events: remove some simple helpers from events_base.c

[Juergen Gross]

The helper functions type_from_irq() and cpu_from_irq() are just one
line functions used only internally.

Open code them where needed. At the same time modify and rename
get_evtchn_to_irq() to return a struct irq_info instead of the IRQ
number.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/events/events_base.c | 97 +++++++++++++-------------------
 1 file changed, 38 insertions(+), 59 deletions(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 97d71c5e7c28..4ada3b6a4164 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -246,15 +246,6 @@ static int set_evtchn_to_irq(evtchn_port_t evtchn, unsigned int irq)
 	return 0;
 }
 
-static int get_evtchn_to_irq(evtchn_port_t evtchn)
-{
-	if (evtchn >= xen_evtchn_max_channels())
-		return -1;
-	if (evtchn_to_irq[EVTCHN_ROW(evtchn)] == NULL)
-		return -1;
-	return READ_ONCE(evtchn_to_irq[EVTCHN_ROW(evtchn)][EVTCHN_COL(evtchn)]);
-}
-
 /* Get info for IRQ */
 static struct irq_info *info_for_irq(unsigned irq)
 {
@@ -272,6 +263,19 @@ static void set_info_for_irq(unsigned int irq, struct irq_info *info)
 		irq_set_chip_data(irq, info);
 }
 
+static struct irq_info *evtchn_to_info(evtchn_port_t evtchn)
+{
+	int irq;
+
+	if (evtchn >= xen_evtchn_max_channels())
+		return NULL;
+	if (evtchn_to_irq[EVTCHN_ROW(evtchn)] == NULL)
+		return NULL;
+	irq = READ_ONCE(evtchn_to_irq[EVTCHN_ROW(evtchn)][EVTCHN_COL(evtchn)]);
+
+	return (irq < 0) ? NULL : info_for_irq(irq);
+}
+
 /* Per CPU channel accounting */
 static void channels_on_cpu_dec(struct irq_info *info)
 {
@@ -426,7 +430,9 @@ static evtchn_port_t evtchn_from_irq(unsigned int irq)
 
 unsigned int irq_from_evtchn(evtchn_port_t evtchn)
 {
-	return get_evtchn_to_irq(evtchn);
+	struct irq_info *info = evtchn_to_info(evtchn);
+
+	return info ? info->irq : -1;
 }
 EXPORT_SYMBOL_GPL(irq_from_evtchn);
 
@@ -470,25 +476,11 @@ static unsigned pirq_from_irq(unsigned irq)
 	return info->u.pirq.pirq;
 }
 
-static enum xen_irq_type type_from_irq(unsigned irq)
-{
-	return info_for_irq(irq)->type;
-}
-
-static unsigned cpu_from_irq(unsigned irq)
-{
-	return info_for_irq(irq)->cpu;
-}
-
 unsigned int cpu_from_evtchn(evtchn_port_t evtchn)
 {
-	int irq = get_evtchn_to_irq(evtchn);
-	unsigned ret = 0;
-
-	if (irq != -1)
-		ret = cpu_from_irq(irq);
+	struct irq_info *info = evtchn_to_info(evtchn);
 
-	return ret;
+	return info ? info->cpu : 0;
 }
 
 static void do_mask(struct irq_info *info, u8 reason)
@@ -537,13 +529,12 @@ static bool pirq_needs_eoi_flag(unsigned irq)
 static void bind_evtchn_to_cpu(evtchn_port_t evtchn, unsigned int cpu,
 			       bool force_affinity)
 {
-	int irq = get_evtchn_to_irq(evtchn);
-	struct irq_info *info = info_for_irq(irq);
+	struct irq_info *info = evtchn_to_info(evtchn);
 
-	BUG_ON(irq == -1);
+	BUG_ON(info == NULL);
 
 	if (IS_ENABLED(CONFIG_SMP) && force_affinity) {
-		struct irq_data *data = irq_get_irq_data(irq);
+		struct irq_data *data = irq_get_irq_data(info->irq);
 
 		irq_data_update_affinity(data, cpumask_of(cpu));
 		irq_data_update_effective_affinity(data, cpumask_of(cpu));
@@ -976,13 +967,13 @@ static void __unbind_from_irq(unsigned int irq)
 	}
 
 	if (VALID_EVTCHN(evtchn)) {
-		unsigned int cpu = cpu_from_irq(irq);
+		unsigned int cpu = info->cpu;
 		struct xenbus_device *dev;
 
 		if (!info->is_static)
 			xen_evtchn_close(evtchn);
 
-		switch (type_from_irq(irq)) {
+		switch (info->type) {
 		case IRQT_VIRQ:
 			per_cpu(virq_to_irq, cpu)[virq_from_irq(irq)] = -1;
 			break;
@@ -1181,15 +1172,16 @@ static int bind_evtchn_to_irq_chip(evtchn_port_t evtchn, struct irq_chip *chip,
 {
 	int irq;
 	int ret;
+	struct irq_info *info;
 
 	if (evtchn >= xen_evtchn_max_channels())
 		return -ENOMEM;
 
 	mutex_lock(&irq_mapping_update_lock);
 
-	irq = get_evtchn_to_irq(evtchn);
+	info = evtchn_to_info(evtchn);
 
-	if (irq == -1) {
+	if (!info) {
 		irq = xen_allocate_irq_dynamic();
 		if (irq < 0)
 			goto out;
@@ -1212,8 +1204,8 @@ static int bind_evtchn_to_irq_chip(evtchn_port_t evtchn, struct irq_chip *chip,
 		 */
 		bind_evtchn_to_cpu(evtchn, 0, false);
 	} else {
-		struct irq_info *info = info_for_irq(irq);
-		WARN_ON(info == NULL || info->type != IRQT_EVTCHN);
+		WARN_ON(info->type != IRQT_EVTCHN);
+		irq = info->irq;
 	}
 
 out:
@@ -1551,13 +1543,7 @@ EXPORT_SYMBOL_GPL(xen_set_irq_priority);
 
 int evtchn_make_refcounted(evtchn_port_t evtchn, bool is_static)
 {
-	int irq = get_evtchn_to_irq(evtchn);
-	struct irq_info *info;
-
-	if (irq == -1)
-		return -ENOENT;
-
-	info = info_for_irq(irq);
+	struct irq_info *info = evtchn_to_info(evtchn);
 
 	if (!info)
 		return -ENOENT;
@@ -1573,7 +1559,6 @@ EXPORT_SYMBOL_GPL(evtchn_make_refcounted);
 
 int evtchn_get(evtchn_port_t evtchn)
 {
-	int irq;
 	struct irq_info *info;
 	int err = -ENOENT;
 
@@ -1582,11 +1567,7 @@ int evtchn_get(evtchn_port_t evtchn)
 
 	mutex_lock(&irq_mapping_update_lock);
 
-	irq = get_evtchn_to_irq(evtchn);
-	if (irq == -1)
-		goto done;
-
-	info = info_for_irq(irq);
+	info = evtchn_to_info(evtchn);
 
 	if (!info)
 		goto done;
@@ -1606,10 +1587,11 @@ EXPORT_SYMBOL_GPL(evtchn_get);
 
 void evtchn_put(evtchn_port_t evtchn)
 {
-	int irq = get_evtchn_to_irq(evtchn);
-	if (WARN_ON(irq == -1))
+	struct irq_info *info = evtchn_to_info(evtchn);
+
+	if (WARN_ON(!info))
 		return;
-	unbind_from_irq(irq);
+	unbind_from_irq(info->irq);
 }
 EXPORT_SYMBOL_GPL(evtchn_put);
 
@@ -1639,12 +1621,10 @@ struct evtchn_loop_ctrl {
 
 void handle_irq_for_port(evtchn_port_t port, struct evtchn_loop_ctrl *ctrl)
 {
-	int irq;
-	struct irq_info *info;
+	struct irq_info *info = evtchn_to_info(port);
 	struct xenbus_device *dev;
 
-	irq = get_evtchn_to_irq(port);
-	if (irq == -1)
+	if (!info)
 		return;
 
 	/*
@@ -1669,7 +1649,6 @@ void handle_irq_for_port(evtchn_port_t port, struct evtchn_loop_ctrl *ctrl)
 		}
 	}
 
-	info = info_for_irq(irq);
 	if (xchg_acquire(&info->is_active, 1))
 		return;
 
@@ -1683,7 +1662,7 @@ void handle_irq_for_port(evtchn_port_t port, struct evtchn_loop_ctrl *ctrl)
 		info->eoi_time = get_jiffies_64() + event_eoi_delay;
 	}
 
-	generic_handle_irq(irq);
+	generic_handle_irq(info->irq);
 }
 
 int xen_evtchn_do_upcall(void)
@@ -1741,7 +1720,7 @@ void rebind_evtchn_irq(evtchn_port_t evtchn, int irq)
 	mutex_lock(&irq_mapping_update_lock);
 
 	/* After resume the irq<->evtchn mappings are all cleared out */
-	BUG_ON(get_evtchn_to_irq(evtchn) != -1);
+	BUG_ON(evtchn_to_info(evtchn));
 	/* Expect irq to have been bound before,
 	   so there should be a proper type */
 	BUG_ON(info->type == IRQT_UNBOUND);
-- 
2.35.3

Re: [PATCH 4/7] xen/events: remove some simple helpers from events_base.c

[Oleksandr Tyshchenko]

On 16.10.23 09:28, Juergen Gross wrote:


Hello Juergen.


> The helper functions type_from_irq() and cpu_from_irq() are just one
> line functions used only internally.
> 
> Open code them where needed. At the same time modify and rename
> get_evtchn_to_irq() to return a struct irq_info instead of the IRQ
> number.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>



[snip]



> 
> @@ -1181,15 +1172,16 @@ static int bind_evtchn_to_irq_chip(evtchn_port_t evtchn, struct irq_chip *chip,
>   {
>   	int irq;
>   	int ret;
> +	struct irq_info *info;
>   
>   	if (evtchn >= xen_evtchn_max_channels())
>   		return -ENOMEM;


I assume this check is called here (*before* holding a lock) by 
intention, as evtchn_to_info() below contains the same check.

>   
>   	mutex_lock(&irq_mapping_update_lock);
>   
> -	irq = get_evtchn_to_irq(evtchn);
> +	info = evtchn_to_info(evtchn) >
> -	if (irq == -1) {
> +	if (!info) {
>   		irq = xen_allocate_irq_dynamic();
>   		if (irq < 0)
>   			goto out;
> @@ -1212,8 +1204,8 @@ static int bind_evtchn_to_irq_chip(evtchn_port_t evtchn, struct irq_chip *chip,
>   		 */
>   		bind_evtchn_to_cpu(evtchn, 0, false);
>   	} else {
> -		struct irq_info *info = info_for_irq(irq);
> -		WARN_ON(info == NULL || info->type != IRQT_EVTCHN);
> +		WARN_ON(info->type != IRQT_EVTCHN);
> +		irq = info->irq;
>   	}


This hunk doesn't apply clearly to the latest state, because of 
"9e90e58c11b7 xen: evtchn: Allow shared registration of IRQ handers" 
went in. Please rebase.


With that:
Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>


Also checkpatch.pl warns about BUG_ON usage in several places, but again 
you didn't introduce them in current patch, just touched their args.


[snip]

Re: [PATCH 4/7] xen/events: remove some simple helpers from events_base.c

[Juergen Gross]

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

On 13.11.23 18:35, Oleksandr Tyshchenko wrote:
> 
> 
> On 16.10.23 09:28, Juergen Gross wrote:
> 
> 
> Hello Juergen.
> 
> 
>> The helper functions type_from_irq() and cpu_from_irq() are just one
>> line functions used only internally.
>>
>> Open code them where needed. At the same time modify and rename
>> get_evtchn_to_irq() to return a struct irq_info instead of the IRQ
>> number.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> 
> 
> [snip]
> 
> 
> 
>>
>> @@ -1181,15 +1172,16 @@ static int bind_evtchn_to_irq_chip(evtchn_port_t evtchn, struct irq_chip *chip,
>>    {
>>    	int irq;
>>    	int ret;
>> +	struct irq_info *info;
>>    
>>    	if (evtchn >= xen_evtchn_max_channels())
>>    		return -ENOMEM;
> 
> 
> I assume this check is called here (*before* holding a lock) by
> intention, as evtchn_to_info() below contains the same check.

Yes.

> 
>>    
>>    	mutex_lock(&irq_mapping_update_lock);
>>    
>> -	irq = get_evtchn_to_irq(evtchn);
>> +	info = evtchn_to_info(evtchn) >
>> -	if (irq == -1) {
>> +	if (!info) {
>>    		irq = xen_allocate_irq_dynamic();
>>    		if (irq < 0)
>>    			goto out;
>> @@ -1212,8 +1204,8 @@ static int bind_evtchn_to_irq_chip(evtchn_port_t evtchn, struct irq_chip *chip,
>>    		 */
>>    		bind_evtchn_to_cpu(evtchn, 0, false);
>>    	} else {
>> -		struct irq_info *info = info_for_irq(irq);
>> -		WARN_ON(info == NULL || info->type != IRQT_EVTCHN);
>> +		WARN_ON(info->type != IRQT_EVTCHN);
>> +		irq = info->irq;
>>    	}
> 
> 
> This hunk doesn't apply clearly to the latest state, because of
> "9e90e58c11b7 xen: evtchn: Allow shared registration of IRQ handers"
> went in. Please rebase.
> 
> 
> With that:
> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Thanks,


Juergen


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

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

[PATCH 5/7] xen/events: drop xen_allocate_irqs_dynamic()

[Juergen Gross]

Instead of having a common function for allocating a single IRQ or a
consecutive number of IRQs, split up the functionality into the callers
of xen_allocate_irqs_dynamic().

This allows to handle any allocation error in xen_irq_init() gracefully
instead of panicing the system. Let xen_irq_init() return the irq_info
pointer or NULL in case of an allocation error.

Additionally set the IRQ into irq_info already at allocation time, as
otherwise the IRQ would be '0' (which is a valid IRQ number) until
being set.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/events/events_base.c | 74 +++++++++++++++++++-------------
 1 file changed, 44 insertions(+), 30 deletions(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 4ada3b6a4164..58e5549ee1db 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -302,6 +302,13 @@ static void channels_on_cpu_inc(struct irq_info *info)
 	info->is_accounted = 1;
 }
 
+static void xen_irq_free_desc(unsigned int irq)
+{
+	/* Legacy IRQ descriptors are managed by the arch. */
+	if (irq >= nr_legacy_irqs())
+		irq_free_desc(irq);
+}
+
 static void delayed_free_irq(struct work_struct *work)
 {
 	struct irq_info *info = container_of(to_rcu_work(work), struct irq_info,
@@ -313,9 +320,7 @@ static void delayed_free_irq(struct work_struct *work)
 
 	kfree(info);
 
-	/* Legacy IRQ descriptors are managed by the arch. */
-	if (irq >= nr_legacy_irqs())
-		irq_free_desc(irq);
+	xen_irq_free_desc(irq);
 }
 
 /* Constructors for packed IRQ information. */
@@ -330,7 +335,6 @@ static int xen_irq_info_common_setup(struct irq_info *info,
 	BUG_ON(info->type != IRQT_UNBOUND && info->type != type);
 
 	info->type = type;
-	info->irq = irq;
 	info->evtchn = evtchn;
 	info->cpu = cpu;
 	info->mask_reason = EVT_MASK_REASON_EXPLICIT;
@@ -730,47 +734,45 @@ void xen_irq_lateeoi(unsigned int irq, unsigned int eoi_flags)
 }
 EXPORT_SYMBOL_GPL(xen_irq_lateeoi);
 
-static void xen_irq_init(unsigned irq)
+static struct irq_info *xen_irq_init(unsigned int irq)
 {
 	struct irq_info *info;
 
 	info = kzalloc(sizeof(*info), GFP_KERNEL);
-	if (info == NULL)
-		panic("Unable to allocate metadata for IRQ%d\n", irq);
+	if (info) {
+		info->irq = irq;
+		info->type = IRQT_UNBOUND;
+		info->refcnt = -1;
+		INIT_RCU_WORK(&info->rwork, delayed_free_irq);
 
-	info->type = IRQT_UNBOUND;
-	info->refcnt = -1;
-	INIT_RCU_WORK(&info->rwork, delayed_free_irq);
+		set_info_for_irq(irq, info);
+		/*
+		 * Interrupt affinity setting can be immediate. No point
+		 * in delaying it until an interrupt is handled.
+		 */
+		irq_set_status_flags(irq, IRQ_MOVE_PCNTXT);
 
-	set_info_for_irq(irq, info);
-	/*
-	 * Interrupt affinity setting can be immediate. No point
-	 * in delaying it until an interrupt is handled.
-	 */
-	irq_set_status_flags(irq, IRQ_MOVE_PCNTXT);
+		INIT_LIST_HEAD(&info->eoi_list);
+		list_add_tail(&info->list, &xen_irq_list_head);
+	}
 
-	INIT_LIST_HEAD(&info->eoi_list);
-	list_add_tail(&info->list, &xen_irq_list_head);
+	return info;
 }
 
-static int __must_check xen_allocate_irqs_dynamic(int nvec)
+static inline int __must_check xen_allocate_irq_dynamic(void)
 {
-	int i, irq = irq_alloc_descs(-1, 0, nvec, -1);
+	int irq = irq_alloc_desc_from(0, -1);
 
 	if (irq >= 0) {
-		for (i = 0; i < nvec; i++)
-			xen_irq_init(irq + i);
+		if (!xen_irq_init(irq)) {
+			xen_irq_free_desc(irq);
+			irq = -1;
+		}
 	}
 
 	return irq;
 }
 
-static inline int __must_check xen_allocate_irq_dynamic(void)
-{
-
-	return xen_allocate_irqs_dynamic(1);
-}
-
 static int __must_check xen_allocate_irq_gsi(unsigned gsi)
 {
 	int irq;
@@ -790,7 +792,10 @@ static int __must_check xen_allocate_irq_gsi(unsigned gsi)
 	else
 		irq = irq_alloc_desc_at(gsi, -1);
 
-	xen_irq_init(irq);
+	if (!xen_irq_init(irq)) {
+		xen_irq_free_desc(irq);
+		irq = -1;
+	}
 
 	return irq;
 }
@@ -960,6 +965,11 @@ static void __unbind_from_irq(unsigned int irq)
 	evtchn_port_t evtchn = evtchn_from_irq(irq);
 	struct irq_info *info = info_for_irq(irq);
 
+	if (!info) {
+		xen_irq_free_desc(irq);
+		return;
+	}
+
 	if (info->refcnt > 0) {
 		info->refcnt--;
 		if (info->refcnt != 0)
@@ -1097,11 +1107,14 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc,
 
 	mutex_lock(&irq_mapping_update_lock);
 
-	irq = xen_allocate_irqs_dynamic(nvec);
+	irq = irq_alloc_descs(-1, 0, nvec, -1);
 	if (irq < 0)
 		goto out;
 
 	for (i = 0; i < nvec; i++) {
+		if (!xen_irq_init(irq + i))
+			goto error_irq;
+
 		irq_set_chip_and_handler_name(irq + i, &xen_pirq_chip, handle_edge_irq, name);
 
 		ret = xen_irq_info_pirq_setup(irq + i, 0, pirq + i, 0, domid,
@@ -1725,6 +1738,7 @@ void rebind_evtchn_irq(evtchn_port_t evtchn, int irq)
 	   so there should be a proper type */
 	BUG_ON(info->type == IRQT_UNBOUND);
 
+	info->irq = irq;
 	(void)xen_irq_info_evtchn_setup(irq, evtchn, NULL);
 
 	mutex_unlock(&irq_mapping_update_lock);
-- 
2.35.3

Re: [PATCH 5/7] xen/events: drop xen_allocate_irqs_dynamic()

[Oleksandr Tyshchenko]

On 16.10.23 09:28, Juergen Gross wrote:


Hello Juergen

> Instead of having a common function for allocating a single IRQ or a
> consecutive number of IRQs, split up the functionality into the callers
> of xen_allocate_irqs_dynamic().
> 
> This allows to handle any allocation error in xen_irq_init() gracefully
> instead of panicing the system. Let xen_irq_init() return the irq_info
> pointer or NULL in case of an allocation error.
> 
> Additionally set the IRQ into irq_info already at allocation time, as
> otherwise the IRQ would be '0' (which is a valid IRQ number) until
> being set.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>   drivers/xen/events/events_base.c | 74 +++++++++++++++++++-------------
>   1 file changed, 44 insertions(+), 30 deletions(-)
> 

[snip]

> @@ -1725,6 +1738,7 @@ void rebind_evtchn_irq(evtchn_port_t evtchn, int irq)
>   	   so there should be a proper type */
>   	BUG_ON(info->type == IRQT_UNBOUND);
>   
> +	info->irq = irq;


I failed to understand why this is added here. Doesn't irq remain the 
same, and info->irq remains valid? Could you please clarify.

Other changes lgtm.


>   	(void)xen_irq_info_evtchn_setup(irq, evtchn, NULL);
>   
>   	mutex_unlock(&irq_mapping_update_lock);

Re: [PATCH 5/7] xen/events: drop xen_allocate_irqs_dynamic()

[Juergen Gross]

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

On 14.11.23 09:20, Oleksandr Tyshchenko wrote:
> 
> 
> On 16.10.23 09:28, Juergen Gross wrote:
> 
> 
> Hello Juergen
> 
>> Instead of having a common function for allocating a single IRQ or a
>> consecutive number of IRQs, split up the functionality into the callers
>> of xen_allocate_irqs_dynamic().
>>
>> This allows to handle any allocation error in xen_irq_init() gracefully
>> instead of panicing the system. Let xen_irq_init() return the irq_info
>> pointer or NULL in case of an allocation error.
>>
>> Additionally set the IRQ into irq_info already at allocation time, as
>> otherwise the IRQ would be '0' (which is a valid IRQ number) until
>> being set.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>    drivers/xen/events/events_base.c | 74 +++++++++++++++++++-------------
>>    1 file changed, 44 insertions(+), 30 deletions(-)
>>
> 
> [snip]
> 
>> @@ -1725,6 +1738,7 @@ void rebind_evtchn_irq(evtchn_port_t evtchn, int irq)
>>    	   so there should be a proper type */
>>    	BUG_ON(info->type == IRQT_UNBOUND);
>>    
>> +	info->irq = irq;
> 
> 
> I failed to understand why this is added here. Doesn't irq remain the
> same, and info->irq remains valid? Could you please clarify.

The IRQ remains the same, but the event channel could change.

This setting of info->irq compensates for the related removal in
xen_irq_info_common_setup().

> 
> Other changes lgtm.


Juergen


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

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

Re: [PATCH 5/7] xen/events: drop xen_allocate_irqs_dynamic()

[Oleksandr Tyshchenko]

On 14.11.23 10:35, Juergen Gross wrote:


Hello Juergen


> On 14.11.23 09:20, Oleksandr Tyshchenko wrote:
>>
>>
>> On 16.10.23 09:28, Juergen Gross wrote:
>>
>>
>> Hello Juergen
>>
>>> Instead of having a common function for allocating a single IRQ or a
>>> consecutive number of IRQs, split up the functionality into the callers
>>> of xen_allocate_irqs_dynamic().
>>>
>>> This allows to handle any allocation error in xen_irq_init() gracefully
>>> instead of panicing the system. Let xen_irq_init() return the irq_info
>>> pointer or NULL in case of an allocation error.
>>>
>>> Additionally set the IRQ into irq_info already at allocation time, as
>>> otherwise the IRQ would be '0' (which is a valid IRQ number) until
>>> being set.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>>    drivers/xen/events/events_base.c | 74 
>>> +++++++++++++++++++-------------
>>>    1 file changed, 44 insertions(+), 30 deletions(-)
>>>
>>
>> [snip]
>>
>>> @@ -1725,6 +1738,7 @@ void rebind_evtchn_irq(evtchn_port_t evtchn, 
>>> int irq)
>>>           so there should be a proper type */
>>>        BUG_ON(info->type == IRQT_UNBOUND);
>>> +    info->irq = irq;
>>
>>
>> I failed to understand why this is added here. Doesn't irq remain the
>> same, and info->irq remains valid? Could you please clarify.
> 
> The IRQ remains the same, but the event channel could change.
> 
> This setting of info->irq compensates for the related removal in
> xen_irq_info_common_setup().


Thanks for the clarification, you can add my

Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

> 
>>
>> Other changes lgtm.
> 
> 
> Juergen
> 

[PATCH 6/7] xen/events: modify internal [un]bind interfaces

[Juergen Gross]

Modify the internal bind- and unbind-interfaces to take a struct
irq_info parameter. When allocating a new IRQ pass the pointer from
the allocating function further up.

This will reduce the number of info_for_irq() calls and make the code
more efficient.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/events/events_base.c | 252 +++++++++++++++----------------
 1 file changed, 121 insertions(+), 131 deletions(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 58e5549ee1db..47a33d3cbfcb 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -325,7 +325,6 @@ static void delayed_free_irq(struct work_struct *work)
 
 /* Constructors for packed IRQ information. */
 static int xen_irq_info_common_setup(struct irq_info *info,
-				     unsigned irq,
 				     enum xen_irq_type type,
 				     evtchn_port_t evtchn,
 				     unsigned short cpu)
@@ -340,23 +339,22 @@ static int xen_irq_info_common_setup(struct irq_info *info,
 	info->mask_reason = EVT_MASK_REASON_EXPLICIT;
 	raw_spin_lock_init(&info->lock);
 
-	ret = set_evtchn_to_irq(evtchn, irq);
+	ret = set_evtchn_to_irq(evtchn, info->irq);
 	if (ret < 0)
 		return ret;
 
-	irq_clear_status_flags(irq, IRQ_NOREQUEST|IRQ_NOAUTOEN);
+	irq_clear_status_flags(info->irq, IRQ_NOREQUEST | IRQ_NOAUTOEN);
 
 	return xen_evtchn_port_setup(evtchn);
 }
 
-static int xen_irq_info_evtchn_setup(unsigned irq,
+static int xen_irq_info_evtchn_setup(struct irq_info *info,
 				     evtchn_port_t evtchn,
 				     struct xenbus_device *dev)
 {
-	struct irq_info *info = info_for_irq(irq);
 	int ret;
 
-	ret = xen_irq_info_common_setup(info, irq, IRQT_EVTCHN, evtchn, 0);
+	ret = xen_irq_info_common_setup(info, IRQT_EVTCHN, evtchn, 0);
 	info->u.interdomain = dev;
 	if (dev)
 		atomic_inc(&dev->event_channels);
@@ -364,49 +362,36 @@ static int xen_irq_info_evtchn_setup(unsigned irq,
 	return ret;
 }
 
-static int xen_irq_info_ipi_setup(unsigned cpu,
-				  unsigned irq,
-				  evtchn_port_t evtchn,
-				  enum ipi_vector ipi)
+static int xen_irq_info_ipi_setup(struct irq_info *info, unsigned int cpu,
+				  evtchn_port_t evtchn, enum ipi_vector ipi)
 {
-	struct irq_info *info = info_for_irq(irq);
-
 	info->u.ipi = ipi;
 
-	per_cpu(ipi_to_irq, cpu)[ipi] = irq;
+	per_cpu(ipi_to_irq, cpu)[ipi] = info->irq;
 
-	return xen_irq_info_common_setup(info, irq, IRQT_IPI, evtchn, 0);
+	return xen_irq_info_common_setup(info, IRQT_IPI, evtchn, 0);
 }
 
-static int xen_irq_info_virq_setup(unsigned cpu,
-				   unsigned irq,
-				   evtchn_port_t evtchn,
-				   unsigned virq)
+static int xen_irq_info_virq_setup(struct irq_info *info, unsigned int cpu,
+				   evtchn_port_t evtchn, unsigned int virq)
 {
-	struct irq_info *info = info_for_irq(irq);
-
 	info->u.virq = virq;
 
-	per_cpu(virq_to_irq, cpu)[virq] = irq;
+	per_cpu(virq_to_irq, cpu)[virq] = info->irq;
 
-	return xen_irq_info_common_setup(info, irq, IRQT_VIRQ, evtchn, 0);
+	return xen_irq_info_common_setup(info, IRQT_VIRQ, evtchn, 0);
 }
 
-static int xen_irq_info_pirq_setup(unsigned irq,
-				   evtchn_port_t evtchn,
-				   unsigned pirq,
-				   unsigned gsi,
-				   uint16_t domid,
-				   unsigned char flags)
+static int xen_irq_info_pirq_setup(struct irq_info *info, evtchn_port_t evtchn,
+				   unsigned int pirq, unsigned int gsi,
+				   uint16_t domid, unsigned char flags)
 {
-	struct irq_info *info = info_for_irq(irq);
-
 	info->u.pirq.pirq = pirq;
 	info->u.pirq.gsi = gsi;
 	info->u.pirq.domid = domid;
 	info->u.pirq.flags = flags;
 
-	return xen_irq_info_common_setup(info, irq, IRQT_PIRQ, evtchn, 0);
+	return xen_irq_info_common_setup(info, IRQT_PIRQ, evtchn, 0);
 }
 
 static void xen_irq_info_cleanup(struct irq_info *info)
@@ -450,20 +435,16 @@ int irq_evtchn_from_virq(unsigned int cpu, unsigned int virq,
 	return irq;
 }
 
-static enum ipi_vector ipi_from_irq(unsigned irq)
+static enum ipi_vector ipi_from_irq(struct irq_info *info)
 {
-	struct irq_info *info = info_for_irq(irq);
-
 	BUG_ON(info == NULL);
 	BUG_ON(info->type != IRQT_IPI);
 
 	return info->u.ipi;
 }
 
-static unsigned virq_from_irq(unsigned irq)
+static unsigned int virq_from_irq(struct irq_info *info)
 {
-	struct irq_info *info = info_for_irq(irq);
-
 	BUG_ON(info == NULL);
 	BUG_ON(info->type != IRQT_VIRQ);
 
@@ -530,13 +511,9 @@ static bool pirq_needs_eoi_flag(unsigned irq)
 	return info->u.pirq.flags & PIRQ_NEEDS_EOI;
 }
 
-static void bind_evtchn_to_cpu(evtchn_port_t evtchn, unsigned int cpu,
+static void bind_evtchn_to_cpu(struct irq_info *info, unsigned int cpu,
 			       bool force_affinity)
 {
-	struct irq_info *info = evtchn_to_info(evtchn);
-
-	BUG_ON(info == NULL);
-
 	if (IS_ENABLED(CONFIG_SMP) && force_affinity) {
 		struct irq_data *data = irq_get_irq_data(info->irq);
 
@@ -544,7 +521,7 @@ static void bind_evtchn_to_cpu(evtchn_port_t evtchn, unsigned int cpu,
 		irq_data_update_effective_affinity(data, cpumask_of(cpu));
 	}
 
-	xen_evtchn_port_bind_to_cpu(evtchn, cpu, info->cpu);
+	xen_evtchn_port_bind_to_cpu(info->evtchn, cpu, info->cpu);
 
 	channels_on_cpu_dec(info);
 	info->cpu = cpu;
@@ -759,23 +736,24 @@ static struct irq_info *xen_irq_init(unsigned int irq)
 	return info;
 }
 
-static inline int __must_check xen_allocate_irq_dynamic(void)
+static struct irq_info *xen_allocate_irq_dynamic(void)
 {
 	int irq = irq_alloc_desc_from(0, -1);
+	struct irq_info *info = NULL;
 
 	if (irq >= 0) {
-		if (!xen_irq_init(irq)) {
+		info = xen_irq_init(irq);
+		if (!info)
 			xen_irq_free_desc(irq);
-			irq = -1;
-		}
 	}
 
-	return irq;
+	return info;
 }
 
-static int __must_check xen_allocate_irq_gsi(unsigned gsi)
+static struct irq_info *xen_allocate_irq_gsi(unsigned int gsi)
 {
 	int irq;
+	struct irq_info *info;
 
 	/*
 	 * A PV guest has no concept of a GSI (since it has no ACPI
@@ -792,18 +770,15 @@ static int __must_check xen_allocate_irq_gsi(unsigned gsi)
 	else
 		irq = irq_alloc_desc_at(gsi, -1);
 
-	if (!xen_irq_init(irq)) {
+	info = xen_irq_init(irq);
+	if (!info)
 		xen_irq_free_desc(irq);
-		irq = -1;
-	}
 
-	return irq;
+	return info;
 }
 
-static void xen_free_irq(unsigned irq)
+static void xen_free_irq(struct irq_info *info)
 {
-	struct irq_info *info = info_for_irq(irq);
-
 	if (WARN_ON(!info))
 		return;
 
@@ -894,7 +869,7 @@ static unsigned int __startup_pirq(unsigned int irq)
 		goto err;
 
 	info->evtchn = evtchn;
-	bind_evtchn_to_cpu(evtchn, 0, false);
+	bind_evtchn_to_cpu(info, 0, false);
 
 	rc = xen_evtchn_port_setup(evtchn);
 	if (rc)
@@ -960,10 +935,9 @@ int xen_irq_from_gsi(unsigned gsi)
 }
 EXPORT_SYMBOL_GPL(xen_irq_from_gsi);
 
-static void __unbind_from_irq(unsigned int irq)
+static void __unbind_from_irq(struct irq_info *info, unsigned int irq)
 {
-	evtchn_port_t evtchn = evtchn_from_irq(irq);
-	struct irq_info *info = info_for_irq(irq);
+	evtchn_port_t evtchn;
 
 	if (!info) {
 		xen_irq_free_desc(irq);
@@ -976,6 +950,8 @@ static void __unbind_from_irq(unsigned int irq)
 			return;
 	}
 
+	evtchn = info->evtchn;
+
 	if (VALID_EVTCHN(evtchn)) {
 		unsigned int cpu = info->cpu;
 		struct xenbus_device *dev;
@@ -985,10 +961,10 @@ static void __unbind_from_irq(unsigned int irq)
 
 		switch (info->type) {
 		case IRQT_VIRQ:
-			per_cpu(virq_to_irq, cpu)[virq_from_irq(irq)] = -1;
+			per_cpu(virq_to_irq, cpu)[virq_from_irq(info)] = -1;
 			break;
 		case IRQT_IPI:
-			per_cpu(ipi_to_irq, cpu)[ipi_from_irq(irq)] = -1;
+			per_cpu(ipi_to_irq, cpu)[ipi_from_irq(info)] = -1;
 			break;
 		case IRQT_EVTCHN:
 			dev = info->u.interdomain;
@@ -1002,7 +978,7 @@ static void __unbind_from_irq(unsigned int irq)
 		xen_irq_info_cleanup(info);
 	}
 
-	xen_free_irq(irq);
+	xen_free_irq(info);
 }
 
 /*
@@ -1018,24 +994,24 @@ static void __unbind_from_irq(unsigned int irq)
 int xen_bind_pirq_gsi_to_irq(unsigned gsi,
 			     unsigned pirq, int shareable, char *name)
 {
-	int irq;
+	struct irq_info *info;
 	struct physdev_irq irq_op;
 	int ret;
 
 	mutex_lock(&irq_mapping_update_lock);
 
-	irq = xen_irq_from_gsi(gsi);
-	if (irq != -1) {
+	ret = xen_irq_from_gsi(gsi);
+	if (ret != -1) {
 		pr_info("%s: returning irq %d for gsi %u\n",
-			__func__, irq, gsi);
+			__func__, ret, gsi);
 		goto out;
 	}
 
-	irq = xen_allocate_irq_gsi(gsi);
-	if (irq < 0)
+	info = xen_allocate_irq_gsi(gsi);
+	if (!info)
 		goto out;
 
-	irq_op.irq = irq;
+	irq_op.irq = info->irq;
 	irq_op.vector = 0;
 
 	/* Only the privileged domain can do this. For non-priv, the pcifront
@@ -1043,20 +1019,19 @@ int xen_bind_pirq_gsi_to_irq(unsigned gsi,
 	 * this in the priv domain. */
 	if (xen_initial_domain() &&
 	    HYPERVISOR_physdev_op(PHYSDEVOP_alloc_irq_vector, &irq_op)) {
-		xen_free_irq(irq);
-		irq = -ENOSPC;
+		xen_free_irq(info);
+		ret = -ENOSPC;
 		goto out;
 	}
 
-	ret = xen_irq_info_pirq_setup(irq, 0, pirq, gsi, DOMID_SELF,
+	ret = xen_irq_info_pirq_setup(info, 0, pirq, gsi, DOMID_SELF,
 			       shareable ? PIRQ_SHAREABLE : 0);
 	if (ret < 0) {
-		__unbind_from_irq(irq);
-		irq = ret;
+		__unbind_from_irq(info, info->irq);
 		goto out;
 	}
 
-	pirq_query_unmask(irq);
+	pirq_query_unmask(info->irq);
 	/* We try to use the handler with the appropriate semantic for the
 	 * type of interrupt: if the interrupt is an edge triggered
 	 * interrupt we use handle_edge_irq.
@@ -1073,16 +1048,18 @@ int xen_bind_pirq_gsi_to_irq(unsigned gsi,
 	 * is the right choice either way.
 	 */
 	if (shareable)
-		irq_set_chip_and_handler_name(irq, &xen_pirq_chip,
+		irq_set_chip_and_handler_name(info->irq, &xen_pirq_chip,
 				handle_fasteoi_irq, name);
 	else
-		irq_set_chip_and_handler_name(irq, &xen_pirq_chip,
+		irq_set_chip_and_handler_name(info->irq, &xen_pirq_chip,
 				handle_edge_irq, name);
 
+	ret = info->irq;
+
 out:
 	mutex_unlock(&irq_mapping_update_lock);
 
-	return irq;
+	return ret;
 }
 
 #ifdef CONFIG_PCI_MSI
@@ -1104,6 +1081,7 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc,
 			     int pirq, int nvec, const char *name, domid_t domid)
 {
 	int i, irq, ret;
+	struct irq_info *info;
 
 	mutex_lock(&irq_mapping_update_lock);
 
@@ -1112,12 +1090,13 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc,
 		goto out;
 
 	for (i = 0; i < nvec; i++) {
-		if (!xen_irq_init(irq + i))
+		info = xen_irq_init(irq + i);
+		if (!info)
 			goto error_irq;
 
 		irq_set_chip_and_handler_name(irq + i, &xen_pirq_chip, handle_edge_irq, name);
 
-		ret = xen_irq_info_pirq_setup(irq + i, 0, pirq + i, 0, domid,
+		ret = xen_irq_info_pirq_setup(info, 0, pirq + i, 0, domid,
 					      i == 0 ? 0 : PIRQ_MSI_GROUP);
 		if (ret < 0)
 			goto error_irq;
@@ -1129,9 +1108,12 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc,
 out:
 	mutex_unlock(&irq_mapping_update_lock);
 	return irq;
+
 error_irq:
-	while (nvec--)
-		__unbind_from_irq(irq + nvec);
+	while (nvec--) {
+		info = info_for_irq(irq + nvec);
+		__unbind_from_irq(info, irq + nvec);
+	}
 	mutex_unlock(&irq_mapping_update_lock);
 	return ret;
 }
@@ -1167,7 +1149,7 @@ int xen_destroy_irq(int irq)
 		}
 	}
 
-	xen_free_irq(irq);
+	xen_free_irq(info);
 
 out:
 	mutex_unlock(&irq_mapping_update_lock);
@@ -1183,8 +1165,7 @@ EXPORT_SYMBOL_GPL(xen_pirq_from_irq);
 static int bind_evtchn_to_irq_chip(evtchn_port_t evtchn, struct irq_chip *chip,
 				   struct xenbus_device *dev)
 {
-	int irq;
-	int ret;
+	int ret = -ENOMEM;
 	struct irq_info *info;
 
 	if (evtchn >= xen_evtchn_max_channels())
@@ -1195,17 +1176,16 @@ static int bind_evtchn_to_irq_chip(evtchn_port_t evtchn, struct irq_chip *chip,
 	info = evtchn_to_info(evtchn);
 
 	if (!info) {
-		irq = xen_allocate_irq_dynamic();
-		if (irq < 0)
+		info = xen_allocate_irq_dynamic();
+		if (!info)
 			goto out;
 
-		irq_set_chip_and_handler_name(irq, chip,
+		irq_set_chip_and_handler_name(info->irq, chip,
 					      handle_edge_irq, "event");
 
-		ret = xen_irq_info_evtchn_setup(irq, evtchn, dev);
+		ret = xen_irq_info_evtchn_setup(info, evtchn, dev);
 		if (ret < 0) {
-			__unbind_from_irq(irq);
-			irq = ret;
+			__unbind_from_irq(info, info->irq);
 			goto out;
 		}
 		/*
@@ -1215,16 +1195,17 @@ static int bind_evtchn_to_irq_chip(evtchn_port_t evtchn, struct irq_chip *chip,
 		 * affinity setting is not invoked on them so nothing would
 		 * bind the channel.
 		 */
-		bind_evtchn_to_cpu(evtchn, 0, false);
+		bind_evtchn_to_cpu(info, 0, false);
 	} else {
 		WARN_ON(info->type != IRQT_EVTCHN);
-		irq = info->irq;
 	}
 
+	ret = info->irq;
+
 out:
 	mutex_unlock(&irq_mapping_update_lock);
 
-	return irq;
+	return ret;
 }
 
 int bind_evtchn_to_irq(evtchn_port_t evtchn)
@@ -1243,18 +1224,19 @@ static int bind_ipi_to_irq(unsigned int ipi, unsigned int cpu)
 {
 	struct evtchn_bind_ipi bind_ipi;
 	evtchn_port_t evtchn;
-	int ret, irq;
+	struct irq_info *info;
+	int ret;
 
 	mutex_lock(&irq_mapping_update_lock);
 
-	irq = per_cpu(ipi_to_irq, cpu)[ipi];
+	ret = per_cpu(ipi_to_irq, cpu)[ipi];
 
-	if (irq == -1) {
-		irq = xen_allocate_irq_dynamic();
-		if (irq < 0)
+	if (ret == -1) {
+		info = xen_allocate_irq_dynamic();
+		if (!info)
 			goto out;
 
-		irq_set_chip_and_handler_name(irq, &xen_percpu_chip,
+		irq_set_chip_and_handler_name(info->irq, &xen_percpu_chip,
 					      handle_percpu_irq, "ipi");
 
 		bind_ipi.vcpu = xen_vcpu_nr(cpu);
@@ -1263,25 +1245,25 @@ static int bind_ipi_to_irq(unsigned int ipi, unsigned int cpu)
 			BUG();
 		evtchn = bind_ipi.port;
 
-		ret = xen_irq_info_ipi_setup(cpu, irq, evtchn, ipi);
+		ret = xen_irq_info_ipi_setup(info, cpu, evtchn, ipi);
 		if (ret < 0) {
-			__unbind_from_irq(irq);
-			irq = ret;
+			__unbind_from_irq(info, info->irq);
 			goto out;
 		}
 		/*
 		 * Force the affinity mask to the target CPU so proc shows
 		 * the correct target.
 		 */
-		bind_evtchn_to_cpu(evtchn, cpu, true);
+		bind_evtchn_to_cpu(info, cpu, true);
+		ret = info->irq;
 	} else {
-		struct irq_info *info = info_for_irq(irq);
+		info = info_for_irq(ret);
 		WARN_ON(info == NULL || info->type != IRQT_IPI);
 	}
 
  out:
 	mutex_unlock(&irq_mapping_update_lock);
-	return irq;
+	return ret;
 }
 
 static int bind_interdomain_evtchn_to_irq_chip(struct xenbus_device *dev,
@@ -1349,22 +1331,23 @@ int bind_virq_to_irq(unsigned int virq, unsigned int cpu, bool percpu)
 {
 	struct evtchn_bind_virq bind_virq;
 	evtchn_port_t evtchn = 0;
-	int irq, ret;
+	struct irq_info *info;
+	int ret;
 
 	mutex_lock(&irq_mapping_update_lock);
 
-	irq = per_cpu(virq_to_irq, cpu)[virq];
+	ret = per_cpu(virq_to_irq, cpu)[virq];
 
-	if (irq == -1) {
-		irq = xen_allocate_irq_dynamic();
-		if (irq < 0)
+	if (ret == -1) {
+		info = xen_allocate_irq_dynamic();
+		if (!info)
 			goto out;
 
 		if (percpu)
-			irq_set_chip_and_handler_name(irq, &xen_percpu_chip,
+			irq_set_chip_and_handler_name(info->irq, &xen_percpu_chip,
 						      handle_percpu_irq, "virq");
 		else
-			irq_set_chip_and_handler_name(irq, &xen_dynamic_chip,
+			irq_set_chip_and_handler_name(info->irq, &xen_dynamic_chip,
 						      handle_edge_irq, "virq");
 
 		bind_virq.virq = virq;
@@ -1379,10 +1362,9 @@ int bind_virq_to_irq(unsigned int virq, unsigned int cpu, bool percpu)
 			BUG_ON(ret < 0);
 		}
 
-		ret = xen_irq_info_virq_setup(cpu, irq, evtchn, virq);
+		ret = xen_irq_info_virq_setup(info, cpu, evtchn, virq);
 		if (ret < 0) {
-			__unbind_from_irq(irq);
-			irq = ret;
+			__unbind_from_irq(info, info->irq);
 			goto out;
 		}
 
@@ -1390,22 +1372,26 @@ int bind_virq_to_irq(unsigned int virq, unsigned int cpu, bool percpu)
 		 * Force the affinity mask for percpu interrupts so proc
 		 * shows the correct target.
 		 */
-		bind_evtchn_to_cpu(evtchn, cpu, percpu);
+		bind_evtchn_to_cpu(info, cpu, percpu);
+		ret = info->irq;
 	} else {
-		struct irq_info *info = info_for_irq(irq);
+		info = info_for_irq(ret);
 		WARN_ON(info == NULL || info->type != IRQT_VIRQ);
 	}
 
 out:
 	mutex_unlock(&irq_mapping_update_lock);
 
-	return irq;
+	return ret;
 }
 
 static void unbind_from_irq(unsigned int irq)
 {
+	struct irq_info *info;
+
 	mutex_lock(&irq_mapping_update_lock);
-	__unbind_from_irq(irq);
+	info = info_for_irq(irq);
+	__unbind_from_irq(info, irq);
 	mutex_unlock(&irq_mapping_update_lock);
 }
 
@@ -1739,11 +1725,11 @@ void rebind_evtchn_irq(evtchn_port_t evtchn, int irq)
 	BUG_ON(info->type == IRQT_UNBOUND);
 
 	info->irq = irq;
-	(void)xen_irq_info_evtchn_setup(irq, evtchn, NULL);
+	(void)xen_irq_info_evtchn_setup(info, evtchn, NULL);
 
 	mutex_unlock(&irq_mapping_update_lock);
 
-	bind_evtchn_to_cpu(evtchn, info->cpu, false);
+	bind_evtchn_to_cpu(info, info->cpu, false);
 
 	/* Unmask the event channel. */
 	enable_irq(irq);
@@ -1777,7 +1763,7 @@ 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(evtchn, tcpu, false);
+		bind_evtchn_to_cpu(info, tcpu, false);
 
 	do_unmask(info, EVT_MASK_REASON_TEMPORARY);
 
@@ -1928,7 +1914,7 @@ static void restore_pirqs(void)
 		if (rc) {
 			pr_warn("xen map irq failed gsi=%d irq=%d pirq=%d rc=%d\n",
 				gsi, irq, pirq, rc);
-			xen_free_irq(irq);
+			xen_free_irq(info);
 			continue;
 		}
 
@@ -1942,13 +1928,15 @@ static void restore_cpu_virqs(unsigned int cpu)
 {
 	struct evtchn_bind_virq bind_virq;
 	evtchn_port_t evtchn;
+	struct irq_info *info;
 	int virq, irq;
 
 	for (virq = 0; virq < NR_VIRQS; virq++) {
 		if ((irq = per_cpu(virq_to_irq, cpu)[virq]) == -1)
 			continue;
+		info = info_for_irq(irq);
 
-		BUG_ON(virq_from_irq(irq) != virq);
+		BUG_ON(virq_from_irq(info) != virq);
 
 		/* Get a new binding from Xen. */
 		bind_virq.virq = virq;
@@ -1959,9 +1947,9 @@ static void restore_cpu_virqs(unsigned int cpu)
 		evtchn = bind_virq.port;
 
 		/* Record the new mapping. */
-		(void)xen_irq_info_virq_setup(cpu, irq, evtchn, virq);
+		xen_irq_info_virq_setup(info, cpu, evtchn, virq);
 		/* The affinity mask is still valid */
-		bind_evtchn_to_cpu(evtchn, cpu, false);
+		bind_evtchn_to_cpu(info, cpu, false);
 	}
 }
 
@@ -1969,13 +1957,15 @@ static void restore_cpu_ipis(unsigned int cpu)
 {
 	struct evtchn_bind_ipi bind_ipi;
 	evtchn_port_t evtchn;
+	struct irq_info *info;
 	int ipi, irq;
 
 	for (ipi = 0; ipi < XEN_NR_IPIS; ipi++) {
 		if ((irq = per_cpu(ipi_to_irq, cpu)[ipi]) == -1)
 			continue;
+		info = info_for_irq(irq);
 
-		BUG_ON(ipi_from_irq(irq) != ipi);
+		BUG_ON(ipi_from_irq(info) != ipi);
 
 		/* Get a new binding from Xen. */
 		bind_ipi.vcpu = xen_vcpu_nr(cpu);
@@ -1985,9 +1975,9 @@ static void restore_cpu_ipis(unsigned int cpu)
 		evtchn = bind_ipi.port;
 
 		/* Record the new mapping. */
-		(void)xen_irq_info_ipi_setup(cpu, irq, evtchn, ipi);
+		xen_irq_info_ipi_setup(info, cpu, evtchn, ipi);
 		/* The affinity mask is still valid */
-		bind_evtchn_to_cpu(evtchn, cpu, false);
+		bind_evtchn_to_cpu(info, cpu, false);
 	}
 }
 
-- 
2.35.3

Re: [PATCH 6/7] xen/events: modify internal [un]bind interfaces

[Oleksandr Tyshchenko]

On 16.10.23 09:28, Juergen Gross wrote:


Hello Juergen

> Modify the internal bind- and unbind-interfaces to take a struct
> irq_info parameter. When allocating a new IRQ pass the pointer from
> the allocating function further up.
> 
> This will reduce the number of info_for_irq() calls and make the code
> more efficient.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>


I didn't spot obvious issues with current patch, other than just the 
fact that patch needs rebasing (some hunks cannot be applied because of
"e64e7c74b99e xen/events: avoid using info_for_irq() in 
xen_send_IPI_one()" went in).

I was going to ask why "pirq_query_unmask()/pirq_from_irq()" wasn't
converted to take a struct irq_info parameter as well, but looking at 
the rest I noticed this was already done in subsequent commit.

With proper rebasing:
Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>


[snip]

[PATCH 7/7] xen/events: remove some info_for_irq() calls in pirq handling

[Juergen Gross]

Instead of the IRQ number user the struct irq_info pointer as parameter
in the internal pirq related functions. This allows to drop some calls
of info_for_irq().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/events/events_base.c | 113 +++++++++++++++++++------------
 1 file changed, 68 insertions(+), 45 deletions(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 47a33d3cbfcb..c75dff276894 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -172,7 +172,7 @@ static int **evtchn_to_irq;
 #ifdef CONFIG_X86
 static unsigned long *pirq_eoi_map;
 #endif
-static bool (*pirq_needs_eoi)(unsigned irq);
+static bool (*pirq_needs_eoi)(struct irq_info *info);
 
 #define EVTCHN_ROW(e)  (e / (PAGE_SIZE/sizeof(**evtchn_to_irq)))
 #define EVTCHN_COL(e)  (e % (PAGE_SIZE/sizeof(**evtchn_to_irq)))
@@ -188,7 +188,6 @@ static struct irq_chip xen_lateeoi_chip;
 static struct irq_chip xen_percpu_chip;
 static struct irq_chip xen_pirq_chip;
 static void enable_dynirq(struct irq_data *data);
-static void disable_dynirq(struct irq_data *data);
 
 static DEFINE_PER_CPU(unsigned int, irq_epoch);
 
@@ -451,10 +450,8 @@ static unsigned int virq_from_irq(struct irq_info *info)
 	return info->u.virq;
 }
 
-static unsigned pirq_from_irq(unsigned irq)
+static unsigned int pirq_from_irq(struct irq_info *info)
 {
-	struct irq_info *info = info_for_irq(irq);
-
 	BUG_ON(info == NULL);
 	BUG_ON(info->type != IRQT_PIRQ);
 
@@ -497,15 +494,14 @@ static void do_unmask(struct irq_info *info, u8 reason)
 }
 
 #ifdef CONFIG_X86
-static bool pirq_check_eoi_map(unsigned irq)
+static bool pirq_check_eoi_map(struct irq_info *info)
 {
-	return test_bit(pirq_from_irq(irq), pirq_eoi_map);
+	return test_bit(pirq_from_irq(info), pirq_eoi_map);
 }
 #endif
 
-static bool pirq_needs_eoi_flag(unsigned irq)
+static bool pirq_needs_eoi_flag(struct irq_info *info)
 {
-	struct irq_info *info = info_for_irq(irq);
 	BUG_ON(info->type != IRQT_PIRQ);
 
 	return info->u.pirq.flags & PIRQ_NEEDS_EOI;
@@ -799,14 +795,13 @@ static void event_handler_exit(struct irq_info *info)
 	clear_evtchn(info->evtchn);
 }
 
-static void pirq_query_unmask(int irq)
+static void pirq_query_unmask(struct irq_info *info)
 {
 	struct physdev_irq_status_query irq_status;
-	struct irq_info *info = info_for_irq(irq);
 
 	BUG_ON(info->type != IRQT_PIRQ);
 
-	irq_status.irq = pirq_from_irq(irq);
+	irq_status.irq = info->u.pirq.pirq;
 	if (HYPERVISOR_physdev_op(PHYSDEVOP_irq_status_query, &irq_status))
 		irq_status.flags = 0;
 
@@ -815,35 +810,57 @@ static void pirq_query_unmask(int irq)
 		info->u.pirq.flags |= PIRQ_NEEDS_EOI;
 }
 
-static void eoi_pirq(struct irq_data *data)
+static void do_eoi_pirq(struct irq_info *info)
 {
-	struct irq_info *info = info_for_irq(data->irq);
-	evtchn_port_t evtchn = info ? info->evtchn : 0;
-	struct physdev_eoi eoi = { .irq = pirq_from_irq(data->irq) };
+	struct physdev_eoi eoi = { .irq = pirq_from_irq(info) };
 	int rc = 0;
 
-	if (!VALID_EVTCHN(evtchn))
+	if (!VALID_EVTCHN(info->evtchn))
 		return;
 
 	event_handler_exit(info);
 
-	if (pirq_needs_eoi(data->irq)) {
+	if (pirq_needs_eoi(info)) {
 		rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
 		WARN_ON(rc);
 	}
 }
 
+static void eoi_pirq(struct irq_data *data)
+{
+	struct irq_info *info = info_for_irq(data->irq);
+
+	do_eoi_pirq(info);
+}
+
+static void do_disable_dynirq(struct irq_info *info)
+{
+	if (VALID_EVTCHN(info->evtchn))
+		do_mask(info, EVT_MASK_REASON_EXPLICIT);
+}
+
+static void disable_dynirq(struct irq_data *data)
+{
+	struct irq_info *info = info_for_irq(data->irq);
+
+	if (info)
+		do_disable_dynirq(info);
+}
+
 static void mask_ack_pirq(struct irq_data *data)
 {
-	disable_dynirq(data);
-	eoi_pirq(data);
+	struct irq_info *info = info_for_irq(data->irq);
+
+	if (info) {
+		do_disable_dynirq(info);
+		do_eoi_pirq(info);
+	}
 }
 
-static unsigned int __startup_pirq(unsigned int irq)
+static unsigned int __startup_pirq(struct irq_info *info)
 {
 	struct evtchn_bind_pirq bind_pirq;
-	struct irq_info *info = info_for_irq(irq);
-	evtchn_port_t evtchn = evtchn_from_irq(irq);
+	evtchn_port_t evtchn = info->evtchn;
 	int rc;
 
 	BUG_ON(info->type != IRQT_PIRQ);
@@ -851,20 +868,20 @@ static unsigned int __startup_pirq(unsigned int irq)
 	if (VALID_EVTCHN(evtchn))
 		goto out;
 
-	bind_pirq.pirq = pirq_from_irq(irq);
+	bind_pirq.pirq = pirq_from_irq(info);
 	/* NB. We are happy to share unless we are probing. */
 	bind_pirq.flags = info->u.pirq.flags & PIRQ_SHAREABLE ?
 					BIND_PIRQ__WILL_SHARE : 0;
 	rc = HYPERVISOR_event_channel_op(EVTCHNOP_bind_pirq, &bind_pirq);
 	if (rc != 0) {
-		pr_warn("Failed to obtain physical IRQ %d\n", irq);
+		pr_warn("Failed to obtain physical IRQ %d\n", info->irq);
 		return 0;
 	}
 	evtchn = bind_pirq.port;
 
-	pirq_query_unmask(irq);
+	pirq_query_unmask(info);
 
-	rc = set_evtchn_to_irq(evtchn, irq);
+	rc = set_evtchn_to_irq(evtchn, info->irq);
 	if (rc)
 		goto err;
 
@@ -878,26 +895,28 @@ static unsigned int __startup_pirq(unsigned int irq)
 out:
 	do_unmask(info, EVT_MASK_REASON_EXPLICIT);
 
-	eoi_pirq(irq_get_irq_data(irq));
+	do_eoi_pirq(info);
 
 	return 0;
 
 err:
-	pr_err("irq%d: Failed to set port to irq mapping (%d)\n", irq, rc);
+	pr_err("irq%d: Failed to set port to irq mapping (%d)\n", info->irq,
+	       rc);
 	xen_evtchn_close(evtchn);
 	return 0;
 }
 
 static unsigned int startup_pirq(struct irq_data *data)
 {
-	return __startup_pirq(data->irq);
+	struct irq_info *info = info_for_irq(data->irq);
+
+	return __startup_pirq(info);
 }
 
 static void shutdown_pirq(struct irq_data *data)
 {
-	unsigned int irq = data->irq;
-	struct irq_info *info = info_for_irq(irq);
-	evtchn_port_t evtchn = evtchn_from_irq(irq);
+	struct irq_info *info = info_for_irq(data->irq);
+	evtchn_port_t evtchn = info->evtchn;
 
 	BUG_ON(info->type != IRQT_PIRQ);
 
@@ -1031,7 +1050,7 @@ int xen_bind_pirq_gsi_to_irq(unsigned gsi,
 		goto out;
 	}
 
-	pirq_query_unmask(info->irq);
+	pirq_query_unmask(info);
 	/* We try to use the handler with the appropriate semantic for the
 	 * type of interrupt: if the interrupt is an edge triggered
 	 * interrupt we use handle_edge_irq.
@@ -1158,7 +1177,9 @@ int xen_destroy_irq(int irq)
 
 int xen_pirq_from_irq(unsigned irq)
 {
-	return pirq_from_irq(irq);
+	struct irq_info *info = info_for_irq(irq);
+
+	return pirq_from_irq(info);
 }
 EXPORT_SYMBOL_GPL(xen_pirq_from_irq);
 
@@ -1820,28 +1841,30 @@ static void enable_dynirq(struct irq_data *data)
 		do_unmask(info, EVT_MASK_REASON_EXPLICIT);
 }
 
-static void disable_dynirq(struct irq_data *data)
+static void do_ack_dynirq(struct irq_info *info)
 {
-	struct irq_info *info = info_for_irq(data->irq);
-	evtchn_port_t evtchn = info ? info->evtchn : 0;
+	evtchn_port_t evtchn = info->evtchn;
 
 	if (VALID_EVTCHN(evtchn))
-		do_mask(info, EVT_MASK_REASON_EXPLICIT);
+		event_handler_exit(info);
 }
 
 static void ack_dynirq(struct irq_data *data)
 {
 	struct irq_info *info = info_for_irq(data->irq);
-	evtchn_port_t evtchn = info ? info->evtchn : 0;
 
-	if (VALID_EVTCHN(evtchn))
-		event_handler_exit(info);
+	if (info)
+		do_ack_dynirq(info);
 }
 
 static void mask_ack_dynirq(struct irq_data *data)
 {
-	disable_dynirq(data);
-	ack_dynirq(data);
+	struct irq_info *info = info_for_irq(data->irq);
+
+	if (info) {
+		do_disable_dynirq(info);
+		do_ack_dynirq(info);
+	}
 }
 
 static void lateeoi_ack_dynirq(struct irq_data *data)
@@ -1920,7 +1943,7 @@ static void restore_pirqs(void)
 
 		printk(KERN_DEBUG "xen: --> irq=%d, pirq=%d\n", irq, map_irq.pirq);
 
-		__startup_pirq(irq);
+		__startup_pirq(info);
 	}
 }
 
-- 
2.35.3

Re: [PATCH 7/7] xen/events: remove some info_for_irq() calls in pirq handling

[Oleksandr Tyshchenko]

On 16.10.23 09:28, Juergen Gross wrote:

Hello Juergen


> Instead of the IRQ number user the struct irq_info pointer as parameter
> in the internal pirq related functions. This allows to drop some calls
> of info_for_irq().
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>


Looks good, so

Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>


Just one NIT below ...


[snip]

>   
> -static void pirq_query_unmask(int irq)
> +static void pirq_query_unmask(struct irq_info *info)
>   {
>   	struct physdev_irq_status_query irq_status;
> -	struct irq_info *info = info_for_irq(irq);
>   
>   	BUG_ON(info->type != IRQT_PIRQ);
>   
> -	irq_status.irq = pirq_from_irq(irq);
> +	irq_status.irq = info->u.pirq.pirq;


  ... what is the reason to open-code pirq_from_irq() here?
For example, __startup_pirq() continues to use helper in almost the same 
situation ...


[snip]

>   
> -static unsigned int __startup_pirq(unsigned int irq)
> +static unsigned int __startup_pirq(struct irq_info *info)
>   {
>   	struct evtchn_bind_pirq bind_pirq;
> -	struct irq_info *info = info_for_irq(irq);
> -	evtchn_port_t evtchn = evtchn_from_irq(irq);
> +	evtchn_port_t evtchn = info->evtchn;
>   	int rc;
>   
>   	BUG_ON(info->type != IRQT_PIRQ);
> @@ -851,20 +868,20 @@ static unsigned int __startup_pirq(unsigned int irq)
>   	if (VALID_EVTCHN(evtchn))
>   		goto out;
>   
> -	bind_pirq.pirq = pirq_from_irq(irq);
> +	bind_pirq.pirq = pirq_from_irq(info);

    ... here



[snip]

Re: [PATCH 7/7] xen/events: remove some info_for_irq() calls in pirq handling

[Juergen Gross]

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

On 14.11.23 19:16, Oleksandr Tyshchenko wrote:
> 
> 
> On 16.10.23 09:28, Juergen Gross wrote:
> 
> Hello Juergen
> 
> 
>> Instead of the IRQ number user the struct irq_info pointer as parameter
>> in the internal pirq related functions. This allows to drop some calls
>> of info_for_irq().
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> 
> Looks good, so
> 
> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> 
> Just one NIT below ...
> 
> 
> [snip]
> 
>>    
>> -static void pirq_query_unmask(int irq)
>> +static void pirq_query_unmask(struct irq_info *info)
>>    {
>>    	struct physdev_irq_status_query irq_status;
>> -	struct irq_info *info = info_for_irq(irq);
>>    
>>    	BUG_ON(info->type != IRQT_PIRQ);
>>    
>> -	irq_status.irq = pirq_from_irq(irq);
>> +	irq_status.irq = info->u.pirq.pirq;
> 
> 
>    ... what is the reason to open-code pirq_from_irq() here?
> For example, __startup_pirq() continues to use helper in almost the same
> situation ...

Good catch. I'll change that, especially as it allows to remove the BUG_ON()
which is in the helper already.


Juergen

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

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

Re: [PATCH 0/7] xen/events: do some cleanups in events_base.c

[Juergen Gross]

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

On 16.10.23 08:28, Juergen Gross wrote:
> As a followup to the XSA-441 patch this series is doing a minor bug fix
> and some cleanups of events_base.c (with some minor effects outside of
> it).
> 
> Juergen Gross (7):
>    xen/events: fix delayed eoi list handling
>    xen/events: remove unused functions
>    xen/events: reduce externally visible helper functions
>    xen/events: remove some simple helpers from events_base.c
>    xen/events: drop xen_allocate_irqs_dynamic()
>    xen/events: modify internal [un]bind interfaces
>    xen/events: remove some info_for_irq() calls in pirq handling
> 
>   drivers/xen/events/events_2l.c       |   8 +-
>   drivers/xen/events/events_base.c     | 557 +++++++++++++--------------
>   drivers/xen/events/events_internal.h |   1 -
>   include/xen/events.h                 |   8 +-
>   4 files changed, 276 insertions(+), 298 deletions(-)
> 

Any comments?


Juergen

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

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