[PATCH v2 1/2] x86/IRQ: make max number of guests for a shared IRQ configurable

[PATCH v2 1/2] x86/IRQ: make max number of guests for a shared IRQ configurable

[Igor Druzhinin]

... and increase the default to 16.

Current limit of 7 is too restrictive for modern systems where one GSI
could be shared by potentially many PCI INTx sources where each of them
corresponds to a device passed through to its own guest. Some systems do not
apply due dilligence in swizzling INTx links in case e.g. INTA is declared as
interrupt pin for the majority of PCI devices behind a single router,
resulting in overuse of a GSI.

Introduce a new command line option to configure that limit and dynamically
allocate an array of the necessary size. Set the default size now to 16 which
is higher than 7 but could later be increased even more if necessary.

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---

Changes in v2:
- introduced a command line option as suggested
- set the default limit to 16 for now

---
 docs/misc/xen-command-line.pandoc |  9 +++++++++
 xen/arch/x86/irq.c                | 19 +++++++++++++------
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index b4a0d60..f5f230c 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1641,6 +1641,15 @@ This option is ignored in **pv-shim** mode.
 ### nr_irqs (x86)
 > `= <integer>`
 
+### irq_max_guests (x86)
+> `= <integer>`
+
+> Default: `16`
+
+Maximum number of guests IRQ could be shared between, i.e. a limit on
+the number of guests it is possible to start each having assigned a device
+sharing a common interrupt line.  Accepts values between 1 and 255.
+
 ### numa (x86)
 > `= on | off | fake=<integer> | noacpi`
 
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 8d1f9a9..5ae9846 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -42,6 +42,10 @@ integer_param("nr_irqs", nr_irqs);
 int __read_mostly opt_irq_vector_map = OPT_IRQ_VECTOR_MAP_DEFAULT;
 custom_param("irq_vector_map", parse_irq_vector_map_param);
 
+/* Max number of guests IRQ could be shared with */
+static unsigned int __read_mostly irq_max_guests;
+integer_param("irq_max_guests", irq_max_guests);
+
 vmask_t global_used_vector_map;
 
 struct irq_desc __read_mostly *irq_desc = NULL;
@@ -435,6 +439,9 @@ int __init init_irq_data(void)
     for ( ; irq < nr_irqs; irq++ )
         irq_to_desc(irq)->irq = irq;
 
+    if ( !irq_max_guests || irq_max_guests > 255)
+        irq_max_guests = 16;
+
 #ifdef CONFIG_PV
     /* Never allocate the hypercall vector or Linux/BSD fast-trap vector. */
     set_bit(LEGACY_SYSCALL_VECTOR, used_vectors);
@@ -1028,7 +1035,6 @@ int __init setup_irq(unsigned int irq, unsigned int irqflags,
  * HANDLING OF GUEST-BOUND PHYSICAL IRQS
  */
 
-#define IRQ_MAX_GUESTS 7
 typedef struct {
     u8 nr_guests;
     u8 in_flight;
@@ -1039,7 +1045,7 @@ typedef struct {
 #define ACKTYPE_EOI    2     /* EOI on the CPU that was interrupted  */
     cpumask_var_t cpu_eoi_map; /* CPUs that need to EOI this interrupt */
     struct timer eoi_timer;
-    struct domain *guest[IRQ_MAX_GUESTS];
+    struct domain *guest[];
 } irq_guest_action_t;
 
 /*
@@ -1564,7 +1570,8 @@ int pirq_guest_bind(struct vcpu *v, struct pirq *pirq, int will_share)
         if ( newaction == NULL )
         {
             spin_unlock_irq(&desc->lock);
-            if ( (newaction = xmalloc(irq_guest_action_t)) != NULL &&
+            if ( (newaction = xmalloc_bytes(sizeof(irq_guest_action_t) +
+                  irq_max_guests * sizeof(action->guest[0]))) != NULL &&
                  zalloc_cpumask_var(&newaction->cpu_eoi_map) )
                 goto retry;
             xfree(newaction);
@@ -1633,11 +1640,11 @@ int pirq_guest_bind(struct vcpu *v, struct pirq *pirq, int will_share)
         goto retry;
     }
 
-    if ( action->nr_guests == IRQ_MAX_GUESTS )
+    if ( action->nr_guests == irq_max_guests )
     {
         printk(XENLOG_G_INFO "Cannot bind IRQ%d to dom%d. "
-               "Already at max share.\n",
-               pirq->pirq, v->domain->domain_id);
+               "Already at max share %u, increase with irq_max_guests= option.\n",
+               pirq->pirq, v->domain->domain_id, irq_max_guests);
         rc = -EBUSY;
         goto unlock_out;
     }
-- 
2.7.4

[PATCH v2 2/2] x86/IRQ: allocate guest array of max size only for shareable IRQs

[Igor Druzhinin]

... and increase default "irq_max_guests" to 32.

It's not necessary to have an array of a size more than 1 for non-shareable
IRQs and it might impact scalability in case of high "irq_max_guests"
values being used - every IRQ in the system including MSIs would be
supplied with an array of that size.

Since it's now less impactful to use higher "irq_max_guests" value - bump
the default to 32. That should give more headroom for future systems.

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---

New in v2.
This is suggested by Jan and is optional for me.

---
 docs/misc/xen-command-line.pandoc | 2 +-
 xen/arch/x86/irq.c                | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index f5f230c..dea2a22 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1644,7 +1644,7 @@ This option is ignored in **pv-shim** mode.
 ### irq_max_guests (x86)
 > `= <integer>`
 
-> Default: `16`
+> Default: `32`
 
 Maximum number of guests IRQ could be shared between, i.e. a limit on
 the number of guests it is possible to start each having assigned a device
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 5ae9846..70b7a53 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -440,7 +440,7 @@ int __init init_irq_data(void)
         irq_to_desc(irq)->irq = irq;
 
     if ( !irq_max_guests || irq_max_guests > 255)
-        irq_max_guests = 16;
+        irq_max_guests = 32;
 
 #ifdef CONFIG_PV
     /* Never allocate the hypercall vector or Linux/BSD fast-trap vector. */
@@ -1540,6 +1540,7 @@ int pirq_guest_bind(struct vcpu *v, struct pirq *pirq, int will_share)
     unsigned int        irq;
     struct irq_desc         *desc;
     irq_guest_action_t *action, *newaction = NULL;
+    unsigned int        max_nr_guests = will_share ? irq_max_guests : 1;
     int                 rc = 0;
 
     WARN_ON(!spin_is_locked(&v->domain->event_lock));
@@ -1571,7 +1572,7 @@ int pirq_guest_bind(struct vcpu *v, struct pirq *pirq, int will_share)
         {
             spin_unlock_irq(&desc->lock);
             if ( (newaction = xmalloc_bytes(sizeof(irq_guest_action_t) +
-                  irq_max_guests * sizeof(action->guest[0]))) != NULL &&
+                  max_nr_guests * sizeof(action->guest[0]))) != NULL &&
                  zalloc_cpumask_var(&newaction->cpu_eoi_map) )
                 goto retry;
             xfree(newaction);
@@ -1640,7 +1641,7 @@ int pirq_guest_bind(struct vcpu *v, struct pirq *pirq, int will_share)
         goto retry;
     }
 
-    if ( action->nr_guests == irq_max_guests )
+    if ( action->nr_guests == max_nr_guests )
     {
         printk(XENLOG_G_INFO "Cannot bind IRQ%d to dom%d. "
                "Already at max share %u, increase with irq_max_guests= option.\n",
-- 
2.7.4

Re: [PATCH v2 1/2] x86/IRQ: make max number of guests for a shared IRQ configurable

[Jan Beulich]

On 03.12.2020 02:58, Igor Druzhinin wrote:
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1641,6 +1641,15 @@ This option is ignored in **pv-shim** mode.
>  ### nr_irqs (x86)
>  > `= <integer>`
>  
> +### irq_max_guests (x86)

As a rule of thumb, new options want to use - instead of _ as
separators. There was a respective discussion quite some time
ago. My patch to treat - and _ equally when parsing options
wasn't liked by Andrew ...

> @@ -435,6 +439,9 @@ int __init init_irq_data(void)
>      for ( ; irq < nr_irqs; irq++ )
>          irq_to_desc(irq)->irq = irq;
>  
> +    if ( !irq_max_guests || irq_max_guests > 255)

The 255 is a consequence of the struct field being u8, aiui?
There should be a direct connection between the two, i.e. when
changing the underlying property preferably only one place
should require touching (possible e.g. by converting to a bit
field with its width established via a #define), or comments
on either side should point at the other one.

Also as a nit, there's a blank missing ahead of the closing
paren.

> @@ -1564,7 +1570,8 @@ int pirq_guest_bind(struct vcpu *v, struct pirq *pirq, int will_share)
>          if ( newaction == NULL )
>          {
>              spin_unlock_irq(&desc->lock);
> -            if ( (newaction = xmalloc(irq_guest_action_t)) != NULL &&
> +            if ( (newaction = xmalloc_bytes(sizeof(irq_guest_action_t) +
> +                  irq_max_guests * sizeof(action->guest[0]))) != NULL &&

As said in the (later) reply to v1, please try to make use of
xmalloc_flex_struct() here.

> @@ -1633,11 +1640,11 @@ int pirq_guest_bind(struct vcpu *v, struct pirq *pirq, int will_share)
>          goto retry;
>      }
>  
> -    if ( action->nr_guests == IRQ_MAX_GUESTS )
> +    if ( action->nr_guests == irq_max_guests )
>      {
>          printk(XENLOG_G_INFO "Cannot bind IRQ%d to dom%d. "
> -               "Already at max share.\n",
> -               pirq->pirq, v->domain->domain_id);
> +               "Already at max share %u, increase with irq_max_guests= option.\n",
> +               pirq->pirq, v->domain->domain_id, irq_max_guests);

If you already need to largely redo this printk(), please
- switch to %pd at the same time,
- don't have the format string extend across multiple lines,
- preferably avoid to use full stops (we don't use any in the vast
  majority of log messages), e.g. by making it "cannot bind IRQ%d
  to %pd, already at max share %u (increase with irq-max-guests=
  option)", if you want to stay close to the original wording (I
  think this could be expressed more compactly as well).

Jan

Re: [PATCH v2 2/2] x86/IRQ: allocate guest array of max size only for shareable IRQs

[Jan Beulich]

On 03.12.2020 02:58, Igor Druzhinin wrote:
> @@ -1540,6 +1540,7 @@ int pirq_guest_bind(struct vcpu *v, struct pirq *pirq, int will_share)
>      unsigned int        irq;
>      struct irq_desc         *desc;
>      irq_guest_action_t *action, *newaction = NULL;
> +    unsigned int        max_nr_guests = will_share ? irq_max_guests : 1;
>      int                 rc = 0;
>  
>      WARN_ON(!spin_is_locked(&v->domain->event_lock));
> @@ -1571,7 +1572,7 @@ int pirq_guest_bind(struct vcpu *v, struct pirq *pirq, int will_share)
>          {
>              spin_unlock_irq(&desc->lock);
>              if ( (newaction = xmalloc_bytes(sizeof(irq_guest_action_t) +
> -                  irq_max_guests * sizeof(action->guest[0]))) != NULL &&
> +                  max_nr_guests * sizeof(action->guest[0]))) != NULL &&
>                   zalloc_cpumask_var(&newaction->cpu_eoi_map) )
>                  goto retry;
>              xfree(newaction);
> @@ -1640,7 +1641,7 @@ int pirq_guest_bind(struct vcpu *v, struct pirq *pirq, int will_share)
>          goto retry;
>      }
>  
> -    if ( action->nr_guests == irq_max_guests )
> +    if ( action->nr_guests == max_nr_guests )
>      {
>          printk(XENLOG_G_INFO "Cannot bind IRQ%d to dom%d. "
>                 "Already at max share %u, increase with irq_max_guests= option.\n",

Just as a minor remark - I don't think this last hunk is necessary,
as the !will_share case won't make it here unless action->nr_guests
is still zero. At which point the need for the new local variable
would also disappear. But I'm not going to insist, as there may be
the view that the code is more clear this way. However, if clarity
(in the sense of "obviously correct") is the goal, then I think
using >= instead of == would now become preferable.

Jan

Re: [PATCH v2 1/2] x86/IRQ: make max number of guests for a shared IRQ configurable

[Jan Beulich]

On 03.12.2020 02:58, Igor Druzhinin wrote:
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1641,6 +1641,15 @@ This option is ignored in **pv-shim** mode.
>  ### nr_irqs (x86)
>  > `= <integer>`
>  
> +### irq_max_guests (x86)
> +> `= <integer>`
> +
> +> Default: `16`
> +
> +Maximum number of guests IRQ could be shared between, i.e. a limit on
> +the number of guests it is possible to start each having assigned a device
> +sharing a common interrupt line.  Accepts values between 1 and 255.

Reading through this again, could "IRQ" be expanded to "any individual
IRQ" or some such?

Jan