Re: [PATCH v2 3/6] x86/setup: init nr_irqs after having detected x2APIC support

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.06.2022 10:54, Roger Pau Monne wrote:
> Logic in ioapic_init() that sets the number of available vectors for
> external interrupts requires knowing the x2APIC Destination Mode.  As
> such move the call after x2APIC BSP setup.

"requires" reads as if this was the case already, which I don't think
is true. The dependency likely appears with the next patch (didn't
look there, yet).

> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -2653,16 +2653,6 @@ void __init ioapic_init(void)
>                 max_gsi_irqs, nr_irqs_gsi);
>          nr_irqs_gsi = max_gsi_irqs;
>      }
> -
> -    if ( nr_irqs == 0 )
> -        nr_irqs = cpu_has_apic ?
> -                  max(0U + num_present_cpus() * NR_DYNAMIC_VECTORS,
> -                      8 * nr_irqs_gsi) :
> -                  nr_irqs_gsi;
> -    else if ( nr_irqs < 16 )
> -        nr_irqs = 16;
> -    printk(XENLOG_INFO "IRQ limits: %u GSI, %u MSI/MSI-X\n",
> -           nr_irqs_gsi, nr_irqs - nr_irqs_gsi);
>  }
>  
>  unsigned int arch_hwdom_irqs(domid_t domid)
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -420,6 +420,16 @@ int __init init_irq_data(void)
>      struct irq_desc *desc;
>      int irq, vector;
>  
> +    if ( nr_irqs == 0 )
> +        nr_irqs = cpu_has_apic ? max(0U + num_present_cpus() *
> +                                     NR_DYNAMIC_VECTORS, 8 * nr_irqs_gsi)
> +                               : nr_irqs_gsi;

Splitting a function argument across lines and then putting the next
argument on the same line is, well, confusing. May I suggest to either
stick to the original line splitting or to go to

    if ( nr_irqs == 0 )
        nr_irqs = cpu_has_apic
                  ? max(0U + num_present_cpus() * NR_DYNAMIC_VECTORS,
                        8 * nr_irqs_gsi)
                  : nr_irqs_gsi;

?

Jan