Messages in this thread | | | Date | Fri, 29 Apr 2016 09:40:09 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH 07/21] X86_64, UV: Enable/Disable UV1 only workaround code |
| |
* 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
| |