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.

Note the printing of the APIC addressing mode done in connect_bsp_APIC() has
been removed, as with the newly introduced mixed mode this would require more
fine grained printing, or else would be incorrect.  The addressing mode can
already be derived from the APIC driver in use, which is printed by different
helpers.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Split comment regarding usage of genapic fields.
 - Remove the printing of the APIC addressing mode in connect_bsp_APIC().

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                      |  2 +
 docs/misc/xen-command-line.pandoc | 12 ++++
 xen/arch/x86/Kconfig              | 35 +++++++++--
 xen/arch/x86/apic.c               |  6 +-
 xen/arch/x86/genapic/x2apic.c     | 96 +++++++++++++++++++++++--------
 5 files changed, 116 insertions(+), 35 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index b184dde8b15f..80deba5d2550 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -9,6 +9,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 ### Changed
 
 ### Added
+ - On x86 introduce a new x2APIC driver that uses Cluster Logical addressing
+   mode for IPIs and Physical addressing mode for external interrupts.
 
 ### Removed
 
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 9a19a04157cb..8e65f8bd18bf 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2804,6 +2804,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>`
 
@@ -2814,6 +2823,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/apic.c b/xen/arch/x86/apic.c
index f1264ce7ed1e..6acdd0ec1468 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -229,11 +229,7 @@ void __init connect_bsp_APIC(void)
         outb(0x01, 0x23);
     }
 
-    printk("Enabling APIC mode:  %s.  Using %d I/O APICs\n",
-           !INT_DEST_MODE ? "Physical"
-                          : init_apic_ldr == init_apic_ldr_flat ? "Flat"
-                                                                : "Clustered",
-           nr_ioapics);
+    printk("Enabling APIC mode.  Using %d I/O APICs\n", nr_ioapics);
     enable_apic_mode();
 }
 
diff --git a/xen/arch/x86/genapic/x2apic.c b/xen/arch/x86/genapic/x2apic.c
index 707deef98c27..875952adc42d 100644
--- a/xen/arch/x86/genapic/x2apic.c
+++ b/xen/arch/x86/genapic/x2apic.c
@@ -180,6 +180,34 @@ 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),
+    /*
+     * 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
+};
+
 static int cf_check update_clusterinfo(
     struct notifier_block *nfb, unsigned long action, void *hcpu)
 {
@@ -220,38 +248,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 +306,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)
-- 
2.42.0

On 03/11/2023 2:45 pm, Roger Pau Monne wrote:
> The current implementation of x2APIC requires to either use Cluster Logical or

I'd suggest starting with "Xen's current ..." to make it clear that this
is our logic, not a property of x2APIC itself.

> 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

s/less/fewer/

> 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.

96 looks suspiciously like an Intel number.  In nothing else, you ought
to say which CPU is it, because microarchitecture matters.  By any
chance can we try this on one of the Bergamos, to give us a datapoint at
512?

As far as the stats go, it would be helpful to give a number for Cluster
mode too, because if it's different from Mixed mode (in this test), then
it shows an error in our reasoning.

> Make the newly introduced mode the default one.
>
> Note the printing of the APIC addressing mode done in connect_bsp_APIC() has
> been removed, as with the newly introduced mixed mode this would require more
> fine grained printing, or else would be incorrect.  The addressing mode can
> already be derived from the APIC driver in use, which is printed by different
> helpers.
>
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Thankyou for doing this.  It's far less invasive than I was fearing.

One real bug (which Gitlab will block on), one other suspected bug that
probably nothing will notice. Minor notes otherwise.

> diff --git a/CHANGELOG.md b/CHANGELOG.md
> index b184dde8b15f..80deba5d2550 100644
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -9,6 +9,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
>  ### Changed
>  
>  ### Added
> + - On x86 introduce a new x2APIC driver that uses Cluster Logical addressing
> +   mode for IPIs and Physical addressing mode for external interrupts.

Could I request that you copy the structure of 4.18 post-reformat, in
the hope that I don't have to repeat that patch for 4.19.

Something like

 - On x86:
   - Introduce a new ...

> 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"

"x2APIC Driver default" is going to be more meaningful to a non-expert
reading this menu entry, IMO.

> diff --git a/xen/arch/x86/genapic/x2apic.c b/xen/arch/x86/genapic/x2apic.c
> index 707deef98c27..875952adc42d 100644
> --- a/xen/arch/x86/genapic/x2apic.c
> +++ b/xen/arch/x86/genapic/x2apic.c
> @@ -180,6 +180,34 @@ 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),

Newline here please.

> +    /*
> +     * 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,

And here.

> +    /*
> +     * 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

Trailing comma please.

> +};
> +
>  static int cf_check update_clusterinfo(
>      struct notifier_block *nfb, unsigned long action, void *hcpu)
>  {
> @@ -220,38 +248,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)

cf_check

> +{
> +    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;

-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;

I know this is just a rearrangement, but IMO it's clearer to follow as:

/* Honour the legacy cmdline setting if it's the only one provided. */
if (  x2apic_mode == unset && x2apic_phys >= 0 )
    ...

There are too many x2apic's in the first comment to read what's going on
at a glance.

~Andrew

On Fri, Nov 03, 2023 at 03:10:18PM +0000, Andrew Cooper wrote:
> On 03/11/2023 2:45 pm, Roger Pau Monne wrote:
> > The current implementation of x2APIC requires to either use Cluster Logical or
> 
> I'd suggest starting with "Xen's current ..." to make it clear that this
> is our logic, not a property of x2APIC itself.
> 
> > 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
> 
> s/less/fewer/
> 
> > 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.
> 
> 96 looks suspiciously like an Intel number.  In nothing else, you ought
> to say which CPU is it, because microarchitecture matters.  By any
> chance can we try this on one of the Bergamos, to give us a datapoint at
> 512?

Let me see if I can grab the only one that's not broken.

Those figures are from an Intel IceLake IIRC.  Cluster mode is the
default, so this change shouldn't effect the performance of builds
that use the default settings.

> > 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"
> 
> "x2APIC Driver default" is going to be more meaningful to a non-expert
> reading this menu entry, IMO.

I will leave the helps as-is.

> > +};
> > +
> >  static int cf_check update_clusterinfo(
> >      struct notifier_block *nfb, unsigned long action, void *hcpu)
> >  {
> > @@ -220,38 +248,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)
> 
> cf_check

I'm probably confused, but other users of custom_param() do have the
cf_check attribute, see parse_spec_ctrl() for example.

Thanks, Roger.

On 03/11/2023 3:34 pm, Roger Pau Monné wrote:
> On Fri, Nov 03, 2023 at 03:10:18PM +0000, Andrew Cooper wrote:
>> On 03/11/2023 2:45 pm, Roger Pau Monne wrote:
>>> 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.
>> 96 looks suspiciously like an Intel number.  In nothing else, you ought
>> to say which CPU is it, because microarchitecture matters.  By any
>> chance can we try this on one of the Bergamos, to give us a datapoint at
>> 512?
> Let me see if I can grab the only one that's not broken.
>
> Those figures are from an Intel IceLake IIRC.  Cluster mode is the
> default, so this change shouldn't effect the performance of builds
> that use the default settings.

"shouldn't" being the operative word.

You're presenting evidence to try and convince the reader that the
reasoning is correct.

Therefore, it is important to confirm your assumptions, and that means
measuring and reporting all 3.

Part of the analysis should say "we expected mixed and cluster to be the
same, and the results show that".

And obviously, if mixed and cluster are wildly different, then we take a
step back and think harder.
>>> 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"
>> "x2APIC Driver default" is going to be more meaningful to a non-expert
>> reading this menu entry, IMO.
> I will leave the helps as-is.
Yeah, the help text is fine.  It's just the title itself.

>
>>> +};
>>> +
>>>  static int cf_check update_clusterinfo(
>>>      struct notifier_block *nfb, unsigned long action, void *hcpu)
>>>  {
>>> @@ -220,38 +248,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)
>> cf_check
> I'm probably confused, but other users of custom_param() do have the
> cf_check attribute, see parse_spec_ctrl() for example.

Yes, and this function needs one but is missing it.

cf_check equates to "This function needs an ENDBR", which it does
because it's invoked by function pointer.

It likely doesn't expode on a CET-active machine because this logic runs
prior to turning CET-IBT on, and you'll only get a build error in the
buster-ibt pipeline which has a still-unupstreamed GCC patch.

~Andrew

On Fri, Nov 03, 2023 at 03:44:30PM +0000, Andrew Cooper wrote:
> On 03/11/2023 3:34 pm, Roger Pau Monné wrote:
> > On Fri, Nov 03, 2023 at 03:10:18PM +0000, Andrew Cooper wrote:
> >> On 03/11/2023 2:45 pm, Roger Pau Monne wrote:
> >>> 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.
> >> 96 looks suspiciously like an Intel number.  In nothing else, you ought
> >> to say which CPU is it, because microarchitecture matters.  By any
> >> chance can we try this on one of the Bergamos, to give us a datapoint at
> >> 512?
> > Let me see if I can grab the only one that's not broken.
> >
> > Those figures are from an Intel IceLake IIRC.  Cluster mode is the
> > default, so this change shouldn't effect the performance of builds
> > that use the default settings.
> 
> "shouldn't" being the operative word.
> 
> You're presenting evidence to try and convince the reader that the
> reasoning is correct.
> 
> Therefore, it is important to confirm your assumptions, and that means
> measuring and reporting all 3.
> 
> Part of the analysis should say "we expected mixed and cluster to be the
> same, and the results show that".
> 
> And obviously, if mixed and cluster are wildly different, then we take a
> step back and think harder.

If they are different I'm definitely not sending the patch :).

> >>> +};
> >>> +
> >>>  static int cf_check update_clusterinfo(
> >>>      struct notifier_block *nfb, unsigned long action, void *hcpu)
> >>>  {
> >>> @@ -220,38 +248,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)
> >> cf_check
> > I'm probably confused, but other users of custom_param() do have the
> > cf_check attribute, see parse_spec_ctrl() for example.
> 
> Yes, and this function needs one but is missing it.
> 
> cf_check equates to "This function needs an ENDBR", which it does
> because it's invoked by function pointer.
> 
> It likely doesn't expode on a CET-active machine because this logic runs
> prior to turning CET-IBT on, and you'll only get a build error in the
> buster-ibt pipeline which has a still-unupstreamed GCC patch.

That was my guess, that CET wasn't yet active by the time this is
called.

For consistency we should fix all handlers of custom_param() (and
other command line parsing helpers) so they uniformly use cf_check,
otherwise it's likely I will make the same mistake when copy&paste to
introduce a new option.

Thanks, Roger.

On Fri, Nov 03, 2023 at 04:34:17PM +0100, Roger Pau Monné wrote:
> On Fri, Nov 03, 2023 at 03:10:18PM +0000, Andrew Cooper wrote:
> > On 03/11/2023 2:45 pm, Roger Pau Monne wrote:
> > > +};
> > > +
> > >  static int cf_check update_clusterinfo(
> > >      struct notifier_block *nfb, unsigned long action, void *hcpu)
> > >  {
> > > @@ -220,38 +248,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)
> > 
> > cf_check
> 
> I'm probably confused, but other users of custom_param() do have the
> cf_check attribute, see parse_spec_ctrl() for example.

... other users of custom_param() do _not_ have the ...

Sorry.