Re: [PATCH 07/21] X86_64, UV: Enable/Disable UV1 only workaround code

[Ingo Molnar <mingo@kernel.org>]

* Mike Travis <travis@sgi.com> wrote:

> This patch allows enabling or disabling of the "EXTRA APIC ID BITS"
> workaround found only on UV100/UV1000 systems.  This workaround was
> required by the Intel Nahelm Processor (used in that architecture)
> because it could only support a limited number of CPU threads within
> the SSI.  This was due to only having 8 APIC ID bits thus a workaround
> was added to add and remove extra bits to the APIC ID.  It also required
> a specialized UV1 only apic driver.
> 
> Signed-off-by: Mike Travis <travis@sgi.com>
> Tested-by: Nathan Zimmer <nzimmer@sgi.com>
> ---
>  arch/x86/Kconfig                   |   14 ++++++++++++++
>  arch/x86/include/asm/uv/uv_hub.h   |   11 ++++-------
>  arch/x86/include/asm/uv/uv_mmrs.h  |    2 ++
>  arch/x86/kernel/apic/x2apic_uv_x.c |   26 +++++++++++++++++++++++---
>  4 files changed, 43 insertions(+), 10 deletions(-)
> 
> --- linux.orig/arch/x86/Kconfig
> +++ linux/arch/x86/Kconfig
> @@ -489,6 +489,20 @@ config X86_UV
>  	  This option is needed in order to support SGI Ultraviolet systems.
>  	  If you don't have one of these, you should say N here.
>  
> +config X86_UV1_SUPPORTED
> +	bool "SGI Ultraviolet Series 1 Supported"
> +	depends on X86_UV
> +	default n
> +	---help---
> +	  Set this option if you have a UV100/UV1000 system.  By setting
> +	  this option extra execution time and space is introduced for
> +	  workarounds designed for processors limited to only 8 apicid bits.
> +	  This limited the number of processors that could be supported in
> +	  an SSI.  With the Intel release of the SandyBridge Processor (used
> +	  in UV2000 systems), the "x2apic" mode was introduced to extend
> +	  the number of apicid bits.  Thus more processors are supported
> +	  without these workarounds and the specialized UV1 only apic driver.
> +
>  # Following is an alphabetically sorted list of 32 bit extended platforms
>  # Please maintain the alphabetic order if and when there are additions
>  
> --- linux.orig/arch/x86/include/asm/uv/uv_hub.h
> +++ linux/arch/x86/include/asm/uv/uv_hub.h
> @@ -289,25 +289,21 @@ union uvh_apicid {
>  #define UV4_GLOBAL_MMR32_SIZE		(16UL * 1024 * 1024)
>  
>  #define UV_LOCAL_MMR_BASE		(				\
> -					is_uv1_hub() ? UV1_LOCAL_MMR_BASE : \
>  					is_uv2_hub() ? UV2_LOCAL_MMR_BASE : \
>  					is_uv3_hub() ? UV3_LOCAL_MMR_BASE : \
>  					/*is_uv4_hub*/ UV4_LOCAL_MMR_BASE)
>  
>  #define UV_GLOBAL_MMR32_BASE		(				\
> -					is_uv1_hub() ? UV1_GLOBAL_MMR32_BASE : \
>  					is_uv2_hub() ? UV2_GLOBAL_MMR32_BASE : \
>  					is_uv3_hub() ? UV3_GLOBAL_MMR32_BASE : \
>  					/*is_uv4_hub*/ UV4_GLOBAL_MMR32_BASE)
>  
>  #define UV_LOCAL_MMR_SIZE		(				\
> -					is_uv1_hub() ? UV1_LOCAL_MMR_SIZE : \
>  					is_uv2_hub() ? UV2_LOCAL_MMR_SIZE : \
>  					is_uv3_hub() ? UV3_LOCAL_MMR_SIZE : \
>  					/*is_uv4_hub*/ UV4_LOCAL_MMR_SIZE)
>  
>  #define UV_GLOBAL_MMR32_SIZE		(				\
> -					is_uv1_hub() ? UV1_GLOBAL_MMR32_SIZE : \
>  					is_uv2_hub() ? UV2_GLOBAL_MMR32_SIZE : \
>  					is_uv3_hub() ? UV3_GLOBAL_MMR32_SIZE : \
>  					/*is_uv4_hub*/ UV4_GLOBAL_MMR32_SIZE)

Hm, are you sure this can be removed?


> @@ -445,9 +441,6 @@ static inline int uv_apicid_to_pnode(int
>   */
>  static inline int uv_apicid_to_socket(int apicid)
>  {
> -	if (is_uv1_hub())
> -		return (apicid >> (uv_hub_info->apic_pnode_shift - 1)) & 1;
> -	else
>  		return 0;
>  }
>  
> @@ -704,7 +697,11 @@ static inline void uv_set_cpu_scir_bits(
>  	}
>  }
>  
> +#ifdef	UV1_HUB_IS_SUPPORTED
>  extern unsigned int uv_apicid_hibits;
> +#else
> +#define	uv_apicid_hibits	0
> +#endif
>  static unsigned long uv_hub_ipi_value(int apicid, int vector, int mode)
>  {
>  	apicid |= uv_apicid_hibits;
> --- linux.orig/arch/x86/include/asm/uv/uv_mmrs.h
> +++ linux/arch/x86/include/asm/uv/uv_mmrs.h
> @@ -95,7 +95,9 @@
>  #define UV4_HUB_PART_NUMBER	0x99a1
>  
>  /* Compat: Indicate which UV Hubs are supported. */
> +#ifdef	CONFIG_X86_UV1_SUPPORTED
>  #define UV1_HUB_IS_SUPPORTED	1
> +#endif
>  #define UV2_HUB_IS_SUPPORTED	1
>  #define UV3_HUB_IS_SUPPORTED	1
>  #define UV4_HUB_IS_SUPPORTED	1
> --- linux.orig/arch/x86/kernel/apic/x2apic_uv_x.c
> +++ linux/arch/x86/kernel/apic/x2apic_uv_x.c
> @@ -39,7 +39,9 @@
>  #include <asm/x86_init.h>
>  #include <asm/nmi.h>
>  
> +#ifdef	UV1_HUB_IS_SUPPORTED
>  DEFINE_PER_CPU(int, x2apic_extra_bits);
> +#endif
>  
>  #define PR_DEVEL(fmt, args...)	pr_devel("%s: " fmt, __func__, args)
>  
> @@ -50,8 +52,10 @@ static u64 gru_dist_lmask, gru_dist_umas
>  static union uvh_apicid uvh_apicid;
>  int uv_min_hub_revision_id;
>  EXPORT_SYMBOL_GPL(uv_min_hub_revision_id);
> +#ifdef	UV1_HUB_IS_SUPPORTED
>  unsigned int uv_apicid_hibits;
>  EXPORT_SYMBOL_GPL(uv_apicid_hibits);
> +#endif
>  
>  static struct apic apic_x2apic_uv_x;
>  
> @@ -145,6 +149,7 @@ static void __init early_get_apic_pnode_
>   * interrupts potentially passing through the UV HUB.  This prevents
>   * a deadlock between interrupts and IO port operations.
>   */
> +#ifdef	UV1_HUB_IS_SUPPORTED
>  static void __init uv_set_apicid_hibit(void)
>  {
>  	union uv1h_lb_target_physical_apic_id_mask_u apicid_mask;
> @@ -156,6 +161,7 @@ static void __init uv_set_apicid_hibit(v
>  			apicid_mask.s1.bit_enables & UV_APICID_HIBIT_MASK;
>  	}
>  }
> +#endif
>  
>  static int __init uv_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
>  {
> @@ -190,13 +196,14 @@ static int __init uv_acpi_madt_oem_check
>  		uv_system_type = UV_X2APIC;
>  		uv_apic = 0;
>  
> +#ifdef	UV1_HUB_IS_SUPPORTED
>  	} else if (!strcmp(oem_table_id, "UVH")) {	/* only UV1 systems */
>  		uv_system_type = UV_NON_UNIQUE_APIC;
>  		__this_cpu_write(x2apic_extra_bits,
>  			pnodeid << uvh_apicid.s.pnode_shift);
>  		uv_set_apicid_hibit();
>  		uv_apic = 1;
> -
> +#endif
>  	} else	if (!strcmp(oem_table_id, "UVL")) {	/* only used for */
>  		uv_system_type = UV_LEGACY_APIC;	/* very small systems */
>  		uv_apic = 0;
> @@ -252,7 +259,6 @@ static int uv_wakeup_secondary(int phys_
>  	int pnode;
>  
>  	pnode = uv_apicid_to_pnode(phys_apicid);
> -	phys_apicid |= uv_apicid_hibits;
>  	val = (1UL << UVH_IPI_INT_SEND_SHFT) |
>  	    (phys_apicid << UVH_IPI_INT_APIC_ID_SHFT) |
>  	    ((start_rip << UVH_IPI_INT_VECTOR_SHFT) >> 12) |
> @@ -344,7 +350,7 @@ uv_cpu_mask_to_apicid_and(const struct c
>  	}
>  
>  	if (likely(cpu < nr_cpu_ids)) {
> -		*apicid = per_cpu(x86_cpu_to_apicid, cpu) | uv_apicid_hibits;
> +		*apicid = per_cpu(x86_cpu_to_apicid, cpu);
>  		return 0;
>  	}
>  
> @@ -353,21 +359,29 @@ uv_cpu_mask_to_apicid_and(const struct c
>  
>  static unsigned int x2apic_get_apic_id(unsigned long x)
>  {
> +#ifdef	UV1_HUB_IS_SUPPORTED
>  	unsigned int id;
>  
>  	WARN_ON(preemptible() && num_online_cpus() > 1);
>  	id = x | __this_cpu_read(x2apic_extra_bits);
>  
>  	return id;
> +#else
> +	return x;
> +#endif
>  }
>  
>  static unsigned long set_apic_id(unsigned int id)
>  {
> +#ifdef	UV1_HUB_IS_SUPPORTED
>  	unsigned long x;
>  
>  	/* maskout x2apic_extra_bits ? */
>  	x = id;
>  	return x;
> +#else
> +	return id;
> +#endif
>  }

Hm, this hunk does not appear to be doing anything.

>  
>  static unsigned int uv_read_apic_id(void)
> @@ -442,10 +456,16 @@ static struct apic __refdata apic_x2apic
>  	.safe_wait_icr_idle		= native_safe_x2apic_wait_icr_idle,
>  };
>  
> +#ifdef	UV1_HUB_IS_SUPPORTED
>  static void set_x2apic_extra_bits(int pnode)
>  {
>  	__this_cpu_write(x2apic_extra_bits, pnode << uvh_apicid.s.pnode_shift);
>  }
> +#else
> +static inline void set_x2apic_extra_bits(int pnode)
> +{
> +}
> +#endif

So Kconfig values and #ifdefs are not really user friendly nor developer friendly. 
Wouldn't it be possible to eliminate all the #ifdefs and simply use something like 
this:

  static void set_x2apic_extra_bits(int pnode)
  {
	if (unlikely(uv_apic))
		__this_cpu_write(x2apic_extra_bits, pnode << uvh_apicid.s.pnode_shift);
  }

etc. The unlikely() will make sure this never impacts any fast path. You can also 
mark 'uv_apic' with __read_mostly, to make sure it's nicely cached.

(You could also rename 'uv_apic' to 'legacy_uv1_apic' to make it really clear that 
this is only for older systems.)

Hm?

Thanks,

	Ingo