| Message ID | 20231031145259.77199-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] x86/x2apic: introduce a mixed physical/cluster mode | expand |
On 31.10.2023 15:52, Roger Pau Monne wrote: > --- a/xen/arch/x86/genapic/x2apic.c > +++ b/xen/arch/x86/genapic/x2apic.c > @@ -180,6 +180,29 @@ static const struct genapic __initconstrel apic_x2apic_cluster = { > .send_IPI_self = send_IPI_self_x2apic > }; > > +/* > + * Mixed x2APIC mode: use physical for external (device) interrupts, and > + * cluster for inter processor interrupts. Such mode has the benefits of not > + * sharing the vector space with all CPUs on the cluster, while still allowing > + * IPIs to be more efficiently delivered by not having to perform an ICR write > + * for each target CPU. > + */ > +static const struct genapic __initconstrel apic_x2apic_mixed = { > + APIC_INIT("x2apic_mixed", NULL), > + /* > + * NB: IPIs use the send_IPI_{mask,self} hooks only, other fields are > + * exclusively used by external interrupts and hence are set to use > + * Physical destination mode handlers. > + */ > + .int_delivery_mode = dest_Fixed, > + .int_dest_mode = 0 /* physical delivery */, > + .init_apic_ldr = init_apic_ldr_x2apic_cluster, > + .vector_allocation_cpumask = vector_allocation_cpumask_phys, > + .cpu_mask_to_apicid = cpu_mask_to_apicid_phys, > + .send_IPI_mask = send_IPI_mask_x2apic_cluster, > + .send_IPI_self = send_IPI_self_x2apic > +}; I'm afraid the comment is still misleading in one respect: The .init_apic_ldr hook is also set to its Clustered mode handler (and validly so). As before my suggestion would be to leverage that we're using dedicated initializers here and have a Physical mode portion and a Clustered mode one, each clarifying in a brief leading comment where/how the handlers are used. Jan
On 31.10.2023 15:52, Roger Pau Monne wrote: > --- a/xen/arch/x86/genapic/x2apic.c > +++ b/xen/arch/x86/genapic/x2apic.c > @@ -180,6 +180,29 @@ static const struct genapic __initconstrel apic_x2apic_cluster = { > .send_IPI_self = send_IPI_self_x2apic > }; > > +/* > + * Mixed x2APIC mode: use physical for external (device) interrupts, and > + * cluster for inter processor interrupts. Such mode has the benefits of not > + * sharing the vector space with all CPUs on the cluster, while still allowing > + * IPIs to be more efficiently delivered by not having to perform an ICR write > + * for each target CPU. > + */ > +static const struct genapic __initconstrel apic_x2apic_mixed = { > + APIC_INIT("x2apic_mixed", NULL), > + /* > + * NB: IPIs use the send_IPI_{mask,self} hooks only, other fields are > + * exclusively used by external interrupts and hence are set to use > + * Physical destination mode handlers. > + */ > + .int_delivery_mode = dest_Fixed, > + .int_dest_mode = 0 /* physical delivery */, > + .init_apic_ldr = init_apic_ldr_x2apic_cluster, INT_DEST_MODE and the init_apic_ldr hook pointer are used in one place that's neither IPI nor device-IRQ related: What connect_bsp_APIC() logs is already not really correct for x2APIC mode, but clearly things go worse with this further extension of the possible modes. I think this wants adjusting to cover the (now) three x2APIC modes as well. (It could also be updated up front to deal with the two original x2APIC modes, but personally I'd fold the whole change right into here.) Jan
On Thu, Nov 02, 2023 at 02:38:09PM +0100, Jan Beulich wrote: > On 31.10.2023 15:52, Roger Pau Monne wrote: > > --- a/xen/arch/x86/genapic/x2apic.c > > +++ b/xen/arch/x86/genapic/x2apic.c > > @@ -180,6 +180,29 @@ static const struct genapic __initconstrel apic_x2apic_cluster = { > > .send_IPI_self = send_IPI_self_x2apic > > }; > > > > +/* > > + * Mixed x2APIC mode: use physical for external (device) interrupts, and > > + * cluster for inter processor interrupts. Such mode has the benefits of not > > + * sharing the vector space with all CPUs on the cluster, while still allowing > > + * IPIs to be more efficiently delivered by not having to perform an ICR write > > + * for each target CPU. > > + */ > > +static const struct genapic __initconstrel apic_x2apic_mixed = { > > + APIC_INIT("x2apic_mixed", NULL), > > + /* > > + * NB: IPIs use the send_IPI_{mask,self} hooks only, other fields are > > + * exclusively used by external interrupts and hence are set to use > > + * Physical destination mode handlers. > > + */ > > + .int_delivery_mode = dest_Fixed, > > + .int_dest_mode = 0 /* physical delivery */, > > + .init_apic_ldr = init_apic_ldr_x2apic_cluster, > > + .vector_allocation_cpumask = vector_allocation_cpumask_phys, > > + .cpu_mask_to_apicid = cpu_mask_to_apicid_phys, > > + .send_IPI_mask = send_IPI_mask_x2apic_cluster, > > + .send_IPI_self = send_IPI_self_x2apic > > +}; > > I'm afraid the comment is still misleading in one respect: The .init_apic_ldr > hook is also set to its Clustered mode handler (and validly so). As before my > suggestion would be to leverage that we're using dedicated initializers here > and have a Physical mode portion and a Clustered mode one, each clarifying in > a brief leading comment where/how the handlers are used. I've split this as: /* * Mixed x2APIC mode: use physical for external (device) interrupts, and * cluster for inter processor interrupts. Such mode has the benefits of not * sharing the vector space with all CPUs on the cluster, while still allowing * IPIs to be more efficiently delivered by not having to perform an ICR write * for each target CPU. */ static const struct genapic __initconstrel apic_x2apic_mixed = { APIC_INIT("x2apic_mixed", NULL), /* * The following fields are exclusively used by external interrupts and * hence are set to use Physical destination mode handlers. */ .int_delivery_mode = dest_Fixed, .int_dest_mode = 0 /* physical delivery */, .vector_allocation_cpumask = vector_allocation_cpumask_phys, .cpu_mask_to_apicid = cpu_mask_to_apicid_phys, /* * The following fields are exclusively used by IPIs and hence are set to * use Cluster Logical destination mode handlers. Note that init_apic_ldr * is not used by IPIs, but the per-CPU fields it initializes are only used * by the IPI hooks. */ .init_apic_ldr = init_apic_ldr_x2apic_cluster, .send_IPI_mask = send_IPI_mask_x2apic_cluster, .send_IPI_self = send_IPI_self_x2apic }; Pending whether the usage of some of the fields in connect_bsp_APIC() can be removed. Thanks, Roger.
On 03.11.2023 13:48, Roger Pau Monné wrote: > On Thu, Nov 02, 2023 at 02:38:09PM +0100, Jan Beulich wrote: >> On 31.10.2023 15:52, Roger Pau Monne wrote: >>> --- a/xen/arch/x86/genapic/x2apic.c >>> +++ b/xen/arch/x86/genapic/x2apic.c >>> @@ -180,6 +180,29 @@ static const struct genapic __initconstrel apic_x2apic_cluster = { >>> .send_IPI_self = send_IPI_self_x2apic >>> }; >>> >>> +/* >>> + * Mixed x2APIC mode: use physical for external (device) interrupts, and >>> + * cluster for inter processor interrupts. Such mode has the benefits of not >>> + * sharing the vector space with all CPUs on the cluster, while still allowing >>> + * IPIs to be more efficiently delivered by not having to perform an ICR write >>> + * for each target CPU. >>> + */ >>> +static const struct genapic __initconstrel apic_x2apic_mixed = { >>> + APIC_INIT("x2apic_mixed", NULL), >>> + /* >>> + * NB: IPIs use the send_IPI_{mask,self} hooks only, other fields are >>> + * exclusively used by external interrupts and hence are set to use >>> + * Physical destination mode handlers. >>> + */ >>> + .int_delivery_mode = dest_Fixed, >>> + .int_dest_mode = 0 /* physical delivery */, >>> + .init_apic_ldr = init_apic_ldr_x2apic_cluster, >>> + .vector_allocation_cpumask = vector_allocation_cpumask_phys, >>> + .cpu_mask_to_apicid = cpu_mask_to_apicid_phys, >>> + .send_IPI_mask = send_IPI_mask_x2apic_cluster, >>> + .send_IPI_self = send_IPI_self_x2apic >>> +}; >> >> I'm afraid the comment is still misleading in one respect: The .init_apic_ldr >> hook is also set to its Clustered mode handler (and validly so). As before my >> suggestion would be to leverage that we're using dedicated initializers here >> and have a Physical mode portion and a Clustered mode one, each clarifying in >> a brief leading comment where/how the handlers are used. > > I've split this as: > > /* > * Mixed x2APIC mode: use physical for external (device) interrupts, and > * cluster for inter processor interrupts. Such mode has the benefits of not > * sharing the vector space with all CPUs on the cluster, while still allowing > * IPIs to be more efficiently delivered by not having to perform an ICR write > * for each target CPU. > */ > static const struct genapic __initconstrel apic_x2apic_mixed = { > APIC_INIT("x2apic_mixed", NULL), > /* > * The following fields are exclusively used by external interrupts and > * hence are set to use Physical destination mode handlers. > */ > .int_delivery_mode = dest_Fixed, > .int_dest_mode = 0 /* physical delivery */, > .vector_allocation_cpumask = vector_allocation_cpumask_phys, > .cpu_mask_to_apicid = cpu_mask_to_apicid_phys, > /* > * The following fields are exclusively used by IPIs and hence are set to > * use Cluster Logical destination mode handlers. Note that init_apic_ldr > * is not used by IPIs, Not quite correct, I think: This is setting up the receive side of the IPIs (iirc LDR needs to be set for logical delivery mode to be usable). Beyond that lgtm, fwiw. Jan > but the per-CPU fields it initializes are only used > * by the IPI hooks. > */ > .init_apic_ldr = init_apic_ldr_x2apic_cluster, > .send_IPI_mask = send_IPI_mask_x2apic_cluster, > .send_IPI_self = send_IPI_self_x2apic > }; > > Pending whether the usage of some of the fields in connect_bsp_APIC() > can be removed. > > Thanks, Roger.
On Fri, Nov 03, 2023 at 01:51:13PM +0100, Jan Beulich wrote: > On 03.11.2023 13:48, Roger Pau Monné wrote: > > On Thu, Nov 02, 2023 at 02:38:09PM +0100, Jan Beulich wrote: > >> On 31.10.2023 15:52, Roger Pau Monne wrote: > >>> --- a/xen/arch/x86/genapic/x2apic.c > >>> +++ b/xen/arch/x86/genapic/x2apic.c > >>> @@ -180,6 +180,29 @@ static const struct genapic __initconstrel apic_x2apic_cluster = { > >>> .send_IPI_self = send_IPI_self_x2apic > >>> }; > >>> > >>> +/* > >>> + * Mixed x2APIC mode: use physical for external (device) interrupts, and > >>> + * cluster for inter processor interrupts. Such mode has the benefits of not > >>> + * sharing the vector space with all CPUs on the cluster, while still allowing > >>> + * IPIs to be more efficiently delivered by not having to perform an ICR write > >>> + * for each target CPU. > >>> + */ > >>> +static const struct genapic __initconstrel apic_x2apic_mixed = { > >>> + APIC_INIT("x2apic_mixed", NULL), > >>> + /* > >>> + * NB: IPIs use the send_IPI_{mask,self} hooks only, other fields are > >>> + * exclusively used by external interrupts and hence are set to use > >>> + * Physical destination mode handlers. > >>> + */ > >>> + .int_delivery_mode = dest_Fixed, > >>> + .int_dest_mode = 0 /* physical delivery */, > >>> + .init_apic_ldr = init_apic_ldr_x2apic_cluster, > >>> + .vector_allocation_cpumask = vector_allocation_cpumask_phys, > >>> + .cpu_mask_to_apicid = cpu_mask_to_apicid_phys, > >>> + .send_IPI_mask = send_IPI_mask_x2apic_cluster, > >>> + .send_IPI_self = send_IPI_self_x2apic > >>> +}; > >> > >> I'm afraid the comment is still misleading in one respect: The .init_apic_ldr > >> hook is also set to its Clustered mode handler (and validly so). As before my > >> suggestion would be to leverage that we're using dedicated initializers here > >> and have a Physical mode portion and a Clustered mode one, each clarifying in > >> a brief leading comment where/how the handlers are used. > > > > I've split this as: > > > > /* > > * Mixed x2APIC mode: use physical for external (device) interrupts, and > > * cluster for inter processor interrupts. Such mode has the benefits of not > > * sharing the vector space with all CPUs on the cluster, while still allowing > > * IPIs to be more efficiently delivered by not having to perform an ICR write > > * for each target CPU. > > */ > > static const struct genapic __initconstrel apic_x2apic_mixed = { > > APIC_INIT("x2apic_mixed", NULL), > > /* > > * The following fields are exclusively used by external interrupts and > > * hence are set to use Physical destination mode handlers. > > */ > > .int_delivery_mode = dest_Fixed, > > .int_dest_mode = 0 /* physical delivery */, > > .vector_allocation_cpumask = vector_allocation_cpumask_phys, > > .cpu_mask_to_apicid = cpu_mask_to_apicid_phys, > > /* > > * The following fields are exclusively used by IPIs and hence are set to > > * use Cluster Logical destination mode handlers. Note that init_apic_ldr > > * is not used by IPIs, > > Not quite correct, I think: This is setting up the receive side of the IPIs > (iirc LDR needs to be set for logical delivery mode to be usable). Beyond > that lgtm, fwiw. No, LDR is read-only in x2APIC mode (it's rw in xAPIC mode). init_apic_ldr_x2apic_cluster() just reads LDR, but doesn't set it. Thanks, Roger.
On 03.11.2023 14:41, Roger Pau Monné wrote: > On Fri, Nov 03, 2023 at 01:51:13PM +0100, Jan Beulich wrote: >> On 03.11.2023 13:48, Roger Pau Monné wrote: >>> On Thu, Nov 02, 2023 at 02:38:09PM +0100, Jan Beulich wrote: >>>> On 31.10.2023 15:52, Roger Pau Monne wrote: >>>>> --- a/xen/arch/x86/genapic/x2apic.c >>>>> +++ b/xen/arch/x86/genapic/x2apic.c >>>>> @@ -180,6 +180,29 @@ static const struct genapic __initconstrel apic_x2apic_cluster = { >>>>> .send_IPI_self = send_IPI_self_x2apic >>>>> }; >>>>> >>>>> +/* >>>>> + * Mixed x2APIC mode: use physical for external (device) interrupts, and >>>>> + * cluster for inter processor interrupts. Such mode has the benefits of not >>>>> + * sharing the vector space with all CPUs on the cluster, while still allowing >>>>> + * IPIs to be more efficiently delivered by not having to perform an ICR write >>>>> + * for each target CPU. >>>>> + */ >>>>> +static const struct genapic __initconstrel apic_x2apic_mixed = { >>>>> + APIC_INIT("x2apic_mixed", NULL), >>>>> + /* >>>>> + * NB: IPIs use the send_IPI_{mask,self} hooks only, other fields are >>>>> + * exclusively used by external interrupts and hence are set to use >>>>> + * Physical destination mode handlers. >>>>> + */ >>>>> + .int_delivery_mode = dest_Fixed, >>>>> + .int_dest_mode = 0 /* physical delivery */, >>>>> + .init_apic_ldr = init_apic_ldr_x2apic_cluster, >>>>> + .vector_allocation_cpumask = vector_allocation_cpumask_phys, >>>>> + .cpu_mask_to_apicid = cpu_mask_to_apicid_phys, >>>>> + .send_IPI_mask = send_IPI_mask_x2apic_cluster, >>>>> + .send_IPI_self = send_IPI_self_x2apic >>>>> +}; >>>> >>>> I'm afraid the comment is still misleading in one respect: The .init_apic_ldr >>>> hook is also set to its Clustered mode handler (and validly so). As before my >>>> suggestion would be to leverage that we're using dedicated initializers here >>>> and have a Physical mode portion and a Clustered mode one, each clarifying in >>>> a brief leading comment where/how the handlers are used. >>> >>> I've split this as: >>> >>> /* >>> * Mixed x2APIC mode: use physical for external (device) interrupts, and >>> * cluster for inter processor interrupts. Such mode has the benefits of not >>> * sharing the vector space with all CPUs on the cluster, while still allowing >>> * IPIs to be more efficiently delivered by not having to perform an ICR write >>> * for each target CPU. >>> */ >>> static const struct genapic __initconstrel apic_x2apic_mixed = { >>> APIC_INIT("x2apic_mixed", NULL), >>> /* >>> * The following fields are exclusively used by external interrupts and >>> * hence are set to use Physical destination mode handlers. >>> */ >>> .int_delivery_mode = dest_Fixed, >>> .int_dest_mode = 0 /* physical delivery */, >>> .vector_allocation_cpumask = vector_allocation_cpumask_phys, >>> .cpu_mask_to_apicid = cpu_mask_to_apicid_phys, >>> /* >>> * The following fields are exclusively used by IPIs and hence are set to >>> * use Cluster Logical destination mode handlers. Note that init_apic_ldr >>> * is not used by IPIs, >> >> Not quite correct, I think: This is setting up the receive side of the IPIs >> (iirc LDR needs to be set for logical delivery mode to be usable). Beyond >> that lgtm, fwiw. > > No, LDR is read-only in x2APIC mode (it's rw in xAPIC mode). > init_apic_ldr_x2apic_cluster() just reads LDR, but doesn't set it. Oh, right, silly me. Perhaps the function could have a better name (reflecting its purpose, and making more clear that the hook is merely leveraged for the purpose). Jan
diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ca796969990..9b04849b0336 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ Notable changes to Xen will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) +## [unstable UNRELEASED](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=staging) - TBD + +### Added + - On x86 introduce a new x2APIC driver that uses Cluster Logical addressing + mode for IPIs and Physical addressing mode for external interrupts. + ## [4.18.0](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=RELEASE-4.18.0) - 2023-XX-XX ### Changed diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index 6b07d0f3a17f..cbe9b4802c61 100644 --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -2802,6 +2802,15 @@ the watchdog. Permit use of x2apic setup for SMP environments. +### x2apic-mode (x86) +> `= physical | cluster | mixed` + +> Default: `physical` if **FADT** mandates physical mode, otherwise set at +> build time by CONFIG_X2APIC_{PHYSICAL,LOGICAL,MIXED}. + +In the case that x2apic is in use, this option switches between modes to +address APICs in the system as interrupt destinations. + ### x2apic_phys (x86) > `= <boolean>` @@ -2812,6 +2821,9 @@ In the case that x2apic is in use, this option switches between physical and clustered mode. The default, given no hint from the **FADT**, is cluster mode. +**WARNING: `x2apic_phys` is deprecated and superseded by `x2apic-mode`. +The latter takes precedence if both are set.** + ### xenheap_megabytes (arm32) > `= <size>` diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index eac77573bd75..cd9286f295e5 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -228,11 +228,18 @@ config XEN_ALIGN_2M endchoice -config X2APIC_PHYSICAL - bool "x2APIC Physical Destination mode" +choice + prompt "x2APIC Destination mode" + default X2APIC_MIXED help - Use x2APIC Physical Destination mode by default when available. + Select APIC addressing when x2APIC is enabled. + + The default mode is mixed which should provide the best aspects + of both physical and cluster modes. +config X2APIC_PHYSICAL + bool "Physical Destination mode" + help When using this mode APICs are addressed using the Physical Destination mode, which allows using all dynamic vectors on each CPU independently. @@ -242,9 +249,27 @@ config X2APIC_PHYSICAL destination inter processor interrupts (IPIs) slightly slower than Logical Destination mode. - The mode when this option is not selected is Logical Destination. +config X2APIC_CLUSTER + bool "Cluster Destination mode" + help + When using this mode APICs are addressed using the Cluster Logical + Destination mode. + + Cluster Destination has the benefit of sending IPIs faster since + multiple APICs can be targeted as destinations of a single IPI. + However the vector space is shared between all CPUs on the cluster, + and hence using this mode reduces the number of available vectors + when compared to Physical mode. - If unsure, say N. +config X2APIC_MIXED + bool "Mixed Destination mode" + help + When using this mode APICs are addressed using the Cluster Logical + Destination mode for IPIs and Physical mode for external interrupts. + + Should provide the best of both modes. + +endchoice config GUEST bool diff --git a/xen/arch/x86/genapic/x2apic.c b/xen/arch/x86/genapic/x2apic.c index 707deef98c27..7af1a21308b6 100644 --- a/xen/arch/x86/genapic/x2apic.c +++ b/xen/arch/x86/genapic/x2apic.c @@ -180,6 +180,29 @@ static const struct genapic __initconstrel apic_x2apic_cluster = { .send_IPI_self = send_IPI_self_x2apic }; +/* + * Mixed x2APIC mode: use physical for external (device) interrupts, and + * cluster for inter processor interrupts. Such mode has the benefits of not + * sharing the vector space with all CPUs on the cluster, while still allowing + * IPIs to be more efficiently delivered by not having to perform an ICR write + * for each target CPU. + */ +static const struct genapic __initconstrel apic_x2apic_mixed = { + APIC_INIT("x2apic_mixed", NULL), + /* + * NB: IPIs use the send_IPI_{mask,self} hooks only, other fields are + * exclusively used by external interrupts and hence are set to use + * Physical destination mode handlers. + */ + .int_delivery_mode = dest_Fixed, + .int_dest_mode = 0 /* physical delivery */, + .init_apic_ldr = init_apic_ldr_x2apic_cluster, + .vector_allocation_cpumask = vector_allocation_cpumask_phys, + .cpu_mask_to_apicid = cpu_mask_to_apicid_phys, + .send_IPI_mask = send_IPI_mask_x2apic_cluster, + .send_IPI_self = send_IPI_self_x2apic +}; + static int cf_check update_clusterinfo( struct notifier_block *nfb, unsigned long action, void *hcpu) { @@ -220,38 +243,56 @@ static struct notifier_block x2apic_cpu_nfb = { static int8_t __initdata x2apic_phys = -1; boolean_param("x2apic_phys", x2apic_phys); +enum { + unset, physical, cluster, mixed +} static __initdata x2apic_mode = unset; + +static int __init parse_x2apic_mode(const char *s) +{ + if ( !cmdline_strcmp(s, "physical") ) + x2apic_mode = physical; + else if ( !cmdline_strcmp(s, "cluster") ) + x2apic_mode = cluster; + else if ( !cmdline_strcmp(s, "mixed") ) + x2apic_mode = mixed; + else + return EINVAL; + + return 0; +} +custom_param("x2apic-mode", parse_x2apic_mode); + const struct genapic *__init apic_x2apic_probe(void) { - if ( x2apic_phys < 0 ) + /* x2apic-mode option has preference over x2apic_phys. */ + if ( x2apic_phys >= 0 && x2apic_mode == unset ) + x2apic_mode = x2apic_phys ? physical : cluster; + + if ( x2apic_mode == unset ) { - /* - * Force physical mode if there's no (full) interrupt remapping support: - * The ID in clustered mode requires a 32 bit destination field due to - * the usage of the high 16 bits to hold the cluster ID. - */ - x2apic_phys = iommu_intremap != iommu_intremap_full || - (acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL) || - IS_ENABLED(CONFIG_X2APIC_PHYSICAL); - } - else if ( !x2apic_phys ) - switch ( iommu_intremap ) + if ( acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL ) { - case iommu_intremap_off: - case iommu_intremap_restricted: - printk("WARNING: x2APIC cluster mode is not supported %s interrupt remapping -" - " forcing phys mode\n", - iommu_intremap == iommu_intremap_off ? "without" - : "with restricted"); - x2apic_phys = true; - break; - - case iommu_intremap_full: - break; + printk(XENLOG_INFO "ACPI FADT forcing x2APIC physical mode\n"); + x2apic_mode = physical; } + else + x2apic_mode = IS_ENABLED(CONFIG_X2APIC_MIXED) ? mixed + : (IS_ENABLED(CONFIG_X2APIC_PHYSICAL) ? physical + : cluster); + } - if ( x2apic_phys ) + if ( x2apic_mode == physical ) return &apic_x2apic_phys; + if ( x2apic_mode == cluster && iommu_intremap != iommu_intremap_full ) + { + printk("WARNING: x2APIC cluster mode is not supported %s interrupt remapping -" + " forcing mixed mode\n", + iommu_intremap == iommu_intremap_off ? "without" + : "with restricted"); + x2apic_mode = mixed; + } + if ( !this_cpu(cluster_cpus) ) { update_clusterinfo(NULL, CPU_UP_PREPARE, @@ -260,7 +301,7 @@ const struct genapic *__init apic_x2apic_probe(void) register_cpu_notifier(&x2apic_cpu_nfb); } - return &apic_x2apic_cluster; + return x2apic_mode == cluster ? &apic_x2apic_cluster : &apic_x2apic_mixed; } void __init check_x2apic_preenabled(void)
The current implementation of x2APIC requires to either use Cluster Logical or Physical mode for all interrupts. However the selection of Physical vs Logical is not done at APIC setup, an APIC can be addressed both in Physical or Logical destination modes concurrently. Introduce a new x2APIC mode called mixed, which uses Logical Cluster mode for IPIs, and Physical mode for external interrupts, thus attempting to use the best method for each interrupt type. Using Physical mode for external interrupts allows more vectors to be used, and interrupt balancing to be more accurate. Using Logical Cluster mode for IPIs allows less accesses to the ICR register when sending those, as multiple CPUs can be targeted with a single ICR register write. A simple test calling flush_tlb_all() 10000 times in a tight loop on a 96 CPU box gives the following average figures: Physical mode: 26617931ns Mixed mode: 23865337ns So ~10% improvement versus plain Physical mode. Note that Xen uses Cluster mode by default, and hence is already using the fastest way for IPI delivery at the cost of reducing the amount of vectors available system-wide. Make the newly introduced mode the default one. Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - Add change log entry. - Fix indentation and usage of tristate in Kconfig. - Adjust comment regarding hooks used by external interrupts in apic_x2apic_mixed. --- CHANGELOG.md | 6 ++ docs/misc/xen-command-line.pandoc | 12 ++++ xen/arch/x86/Kconfig | 35 ++++++++++-- xen/arch/x86/genapic/x2apic.c | 91 ++++++++++++++++++++++--------- 4 files changed, 114 insertions(+), 30 deletions(-)