Re: [PATCH] x86/APIC: Remove x2APIC pure cluster mode

[oleksii.kurochko@xxxxxxxxx]

On Mon, 2024-09-23 at 15:35 +0100, Matthew Barnes wrote:
> With the introduction of mixed x2APIC mode (using cluster addressing
> for
> IPIs and physical for external interrupts) the use of pure cluster
> mode
> doesn't have any benefit.
> 
> Remove the mode itself, leaving only the code required for logical
> addressing when sending IPIs.
> 
> Implements: https://gitlab.com/xen-project/xen/-/issues/189
> 
> Signed-off-by: Matthew Barnes <matthew.barnes@xxxxxxxxx>
> ---
>  CHANGELOG.md                      |  1 +
Acked-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>

~ Oleksii

>  docs/misc/xen-command-line.pandoc |  4 +--
>  xen/arch/x86/Kconfig              | 12 --------
>  xen/arch/x86/genapic/x2apic.c     | 50 +++--------------------------
> --
>  4 files changed, 7 insertions(+), 60 deletions(-)
> 
> diff --git a/CHANGELOG.md b/CHANGELOG.md
> index 26e7d8dd2ac4..335e98b2e1a7 100644
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -9,6 +9,7 @@ The format is based on [Keep a
> Changelog](https://keepachangelog.com/en/1.0.0/)
>  ### Changed
>   - On x86:
>     - Prefer ACPI reboot over UEFI ResetSystem() run time service
> call.
> +   - Remove x2APIC cluster mode, leaving only physical and mixed
> modes.
>  
>  ### Added
>  
> diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-
> command-line.pandoc
> index 959cf45b55d9..5ce63044ade8 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -2842,10 +2842,10 @@ the watchdog.
>  Permit use of x2apic setup for SMP environments.
>  
>  ### x2apic-mode (x86)
> -> `= physical | cluster | mixed`
> +> `= physical | mixed`
>  
>  > Default: `physical` if **FADT** mandates physical mode, otherwise
> set at
> ->          build time by CONFIG_X2APIC_{PHYSICAL,LOGICAL,MIXED}.
> +>          build time by CONFIG_X2APIC_{PHYSICAL,MIXED}.
>  
>  In the case that x2apic is in use, this option switches between
> modes to
>  address APICs in the system as interrupt destinations.
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index 62f0b5e0f4c5..ab862b083fce 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -275,18 +275,6 @@ config X2APIC_PHYSICAL
>         destination inter processor interrupts (IPIs) slightly
> slower than
>         Logical Destination mode.
>  
> -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.
> -
>  config X2APIC_MIXED
>       bool "Mixed Destination mode"
>       help
> diff --git a/xen/arch/x86/genapic/x2apic.c
> b/xen/arch/x86/genapic/x2apic.c
> index d531035fa42c..c277f4f79b0a 100644
> --- a/xen/arch/x86/genapic/x2apic.c
> +++ b/xen/arch/x86/genapic/x2apic.c
> @@ -63,26 +63,6 @@ static void cf_check
> init_apic_ldr_x2apic_cluster(void)
>      cpumask_set_cpu(this_cpu, per_cpu(cluster_cpus, this_cpu));
>  }
>  
> -static const cpumask_t *cf_check
> vector_allocation_cpumask_x2apic_cluster(
> -    int cpu)
> -{
> -    return per_cpu(cluster_cpus, cpu);
> -}
> -
> -static unsigned int cf_check cpu_mask_to_apicid_x2apic_cluster(
> -    const cpumask_t *cpumask)
> -{
> -    unsigned int cpu = cpumask_any(cpumask);
> -    unsigned int dest = per_cpu(cpu_2_logical_apicid, cpu);
> -    const cpumask_t *cluster_cpus = per_cpu(cluster_cpus, cpu);
> -
> -    for_each_cpu ( cpu, cluster_cpus )
> -        if ( cpumask_test_cpu(cpu, cpumask) )
> -            dest |= per_cpu(cpu_2_logical_apicid, cpu);
> -
> -    return dest;
> -}
> -
>  static void cf_check send_IPI_self_x2apic(uint8_t vector)
>  {
>      apic_wrmsr(APIC_SELF_IPI, vector);
> @@ -169,17 +149,6 @@ static const struct genapic
> __initconst_cf_clobber apic_x2apic_phys = {
>      .send_IPI_self = send_IPI_self_x2apic
>  };
>  
> -static const struct genapic __initconst_cf_clobber
> apic_x2apic_cluster = {
> -    APIC_INIT("x2apic_cluster", NULL),
> -    .int_delivery_mode = dest_LowestPrio,
> -    .int_dest_mode = 1 /* logical delivery */,
> -    .init_apic_ldr = init_apic_ldr_x2apic_cluster,
> -    .vector_allocation_cpumask =
> vector_allocation_cpumask_x2apic_cluster,
> -    .cpu_mask_to_apicid = cpu_mask_to_apicid_x2apic_cluster,
> -    .send_IPI_mask = send_IPI_mask_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
> @@ -252,15 +221,13 @@ static int8_t __initdata x2apic_phys = -1;
>  boolean_param("x2apic_phys", x2apic_phys);
>  
>  enum {
> -   unset, physical, cluster, mixed
> +   unset, physical, mixed
>  } static __initdata x2apic_mode = unset;
>  
>  static int __init cf_check 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
> @@ -274,7 +241,7 @@ const struct genapic *__init
> apic_x2apic_probe(void)
>  {
>      /* Honour the legacy cmdline setting if it's the only one
> provided. */
>      if ( x2apic_mode == unset && x2apic_phys >= 0 )
> -        x2apic_mode = x2apic_phys ? physical : cluster;
> +        x2apic_mode = x2apic_phys ? physical : mixed;
>  
>      if ( x2apic_mode == unset )
>      {
> @@ -286,21 +253,12 @@ const struct genapic *__init
> apic_x2apic_probe(void)
>          else
>              x2apic_mode = IS_ENABLED(CONFIG_X2APIC_MIXED) ? mixed
>                            : (IS_ENABLED(CONFIG_X2APIC_PHYSICAL) ?
> physical
> -                                                                :
> cluster);
> +                                                                :
> mixed);
>      }
>  
>      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,
> @@ -309,7 +267,7 @@ const struct genapic *__init
> apic_x2apic_probe(void)
>          register_cpu_notifier(&x2apic_cpu_nfb);
>      }
>  
> -    return x2apic_mode == cluster ? &apic_x2apic_cluster :
> &apic_x2apic_mixed;
> +    return &apic_x2apic_mixed;
>  }
>  
>  void __init check_x2apic_preenabled(void)