Re: [PATCH v2 4/6] x86/irq: fix setting irq limits

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.06.2022 10:54, Roger Pau Monne wrote:
> Current code to calculate nr_irqs assumes the APIC destination mode to
> be physical, so all vectors on each possible CPU is available for use
> by a different interrupt source. This is not true when using Logical
> (Cluster) destination mode, where CPUs in the same cluster share the
> vector space.
> 
> Fix by calculating the maximum Cluster ID and use it to derive the
> number of clusters in the system. Note the code assumes Cluster IDs to
> be contiguous, or else we will set nr_irqs to a number higher than the
> real amount of vectors (still not fatal).

While not fatal, it could be (pretty) wasteful. Iirc discontiguous
cluster numbers aren't unusual. It shouldn't be that difficult to
actually count clusters.

> The number of clusters is then used instead of the number of present
> CPUs when calculating the value of nr_irqs.

This takes care of x2APIC clustered mode, but leaves xAPIC flat mode
still as broken as before. vec_spaces should be 1 in that case,
shouldn't it?

> --- a/xen/arch/x86/include/asm/apic.h
> +++ b/xen/arch/x86/include/asm/apic.h
> @@ -27,6 +27,8 @@ enum apic_mode {
>  extern bool iommu_x2apic_enabled;
>  extern u8 apic_verbosity;
>  extern bool directed_eoi_enabled;
> +extern uint16_t x2apic_max_cluster_id;

I don't see a need to use a fixed width type here; unsigned int will
be quite fine afaict (and be in line with ./CODING_STYLE). Or if you
want to save space, then it still would better be "unsigned short".

> @@ -199,6 +201,9 @@ static int MP_processor_info_x(struct 
> mpc_config_processor *m,
>               def_to_bigsmp = true;
>       }
>  
> +     x2apic_max_cluster_id = max(x2apic_max_cluster_id,
> +                                 (uint16_t)(apicid >> 4));
> +
>       return cpu;
>  }

I assume you don't mean to also account for hotplug CPUs? If so,
please add a respective statement to the description.

Jan