| Message ID | 20220630085439.83193-5-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/irq: switch x2APIC default destination mode | expand |
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
diff --git a/xen/arch/x86/genapic/x2apic.c b/xen/arch/x86/genapic/x2apic.c index 7dfc793514..ad95564e90 100644 --- a/xen/arch/x86/genapic/x2apic.c +++ b/xen/arch/x86/genapic/x2apic.c @@ -228,7 +228,7 @@ static struct notifier_block x2apic_cpu_nfb = { .notifier_call = update_clusterinfo }; -static int8_t __initdata x2apic_phys = -1; +int8_t __initdata x2apic_phys = -1; boolean_param("x2apic_phys", x2apic_phys); const struct genapic *__init apic_x2apic_probe(void) diff --git a/xen/arch/x86/include/asm/apic.h b/xen/arch/x86/include/asm/apic.h index 7625c0ecd6..6060628836 100644 --- 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; +extern int8_t x2apic_phys; void check_x2apic_preenabled(void); void x2apic_bsp_setup(void); diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c index b51e25f696..b64d18c450 100644 --- a/xen/arch/x86/irq.c +++ b/xen/arch/x86/irq.c @@ -421,9 +421,15 @@ int __init init_irq_data(void) int irq, vector; if ( nr_irqs == 0 ) - nr_irqs = cpu_has_apic ? max(0U + num_present_cpus() * - NR_DYNAMIC_VECTORS, 8 * nr_irqs_gsi) + { + unsigned int vec_spaces = + (x2apic_enabled && !x2apic_phys) ? x2apic_max_cluster_id + 1 + : num_present_cpus(); + + nr_irqs = cpu_has_apic ? max(vec_spaces * NR_DYNAMIC_VECTORS, + 8 * nr_irqs_gsi) : nr_irqs_gsi; + } else if ( nr_irqs < 16 ) nr_irqs = 16; diff --git a/xen/arch/x86/mpparse.c b/xen/arch/x86/mpparse.c index d8ccab2449..dc112bffc7 100644 --- a/xen/arch/x86/mpparse.c +++ b/xen/arch/x86/mpparse.c @@ -131,6 +131,8 @@ static int __init mpf_checksum(unsigned char *mp, int len) return sum & 0xFF; } +uint16_t __initdata x2apic_max_cluster_id; + /* Return xen's logical cpu_id of the new added cpu or <0 if error */ static int MP_processor_info_x(struct mpc_config_processor *m, u32 apicid, bool hotplug) @@ -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; }
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). The number of clusters is then used instead of the number of present CPUs when calculating the value of nr_irqs. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/genapic/x2apic.c | 2 +- xen/arch/x86/include/asm/apic.h | 2 ++ xen/arch/x86/irq.c | 10 ++++++++-- xen/arch/x86/mpparse.c | 5 +++++ 4 files changed, 16 insertions(+), 3 deletions(-)