| Message ID | 20250213161423.449435-5-riel@surriel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | AMD broadcast TLB invalidation | expand |
On 2/13/25 08:13, Rik van Riel wrote: > + /* Max number of pages INVLPGB can invalidate in one shot */ > + if (boot_cpu_has(X86_FEATURE_INVLPGB)) { > + cpuid(0x80000008, &eax, &ebx, &ecx, &edx); > + invlpgb_count_max = (edx & 0xffff) + 1; > + } > } My only nit with this is that this adds a seventh place that the kernel reads leaf 0x80000008. The users seem really random, though. Some cross-vendor stuff, some very AMD-specific stuff. So, I'll leave that at a nit since I don't have a good suggestion for solving it: Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
On Thu, Feb 13, 2025 at 11:13:55AM -0500, Rik van Riel wrote: > The CPU advertises the maximum number of pages that can be shot down > with one INVLPGB instruction in the CPUID data. > > Save that information for later use. > > Signed-off-by: Rik van Riel <riel@surriel.com> > Tested-by: Manali Shukla <Manali.Shukla@amd.com> > Tested-by: Brendan Jackman <jackmanb@google.com> > Tested-by: Michael Kelley <mhklinux@outlook.com> > --- > arch/x86/Kconfig.cpu | 5 +++++ > arch/x86/include/asm/cpufeatures.h | 1 + > arch/x86/include/asm/tlbflush.h | 7 +++++++ > arch/x86/kernel/cpu/amd.c | 8 ++++++++ > 4 files changed, 21 insertions(+) > > diff --git a/arch/x86/Kconfig.cpu b/arch/x86/Kconfig.cpu > index 2a7279d80460..abe013a1b076 100644 > --- a/arch/x86/Kconfig.cpu > +++ b/arch/x86/Kconfig.cpu > @@ -395,6 +395,10 @@ config X86_VMX_FEATURE_NAMES > def_bool y > depends on IA32_FEAT_CTL > > +config X86_BROADCAST_TLB_FLUSH > + def_bool y > + depends on CPU_SUP_AMD && 64BIT > + > menuconfig PROCESSOR_SELECT > bool "Supported processor vendors" if EXPERT > help > @@ -431,6 +435,7 @@ config CPU_SUP_CYRIX_32 > config CPU_SUP_AMD > default y > bool "Support AMD processors" if PROCESSOR_SELECT > + select X86_BROADCAST_TLB_FLUSH CPU_SUP_AMD selects X86_BROADCAST_TLB_FLUSH which depends on CPU_SUP_AMD which selects X86_BROADCAST_TLB_FLUSH which depends on CPU_SUP_AMD... Why do you really need yet another Kconfig symbol? Just whack X86_BROADCAST_TLB_FLUSH - it'll be enabled by default on everything anyway. > @@ -1139,6 +1141,12 @@ static void cpu_detect_tlb_amd(struct cpuinfo_x86 *c) > tlb_lli_2m[ENTRIES] = eax & mask; > > tlb_lli_4m[ENTRIES] = tlb_lli_2m[ENTRIES] >> 1; > + > + /* Max number of pages INVLPGB can invalidate in one shot */ > + if (boot_cpu_has(X86_FEATURE_INVLPGB)) { > + cpuid(0x80000008, &eax, &ebx, &ecx, &edx); > + invlpgb_count_max = (edx & 0xffff) + 1; > + } get_cpu_cap() already reads that leaf. You don't need to do it here again.
On Wed, 2025-02-19 at 12:56 +0100, Borislav Petkov wrote: > On Thu, Feb 13, 2025 at 11:13:55AM -0500, Rik van Riel wrote: > > The CPU advertises the maximum number of pages that can be shot > > down > > with one INVLPGB instruction in the CPUID data. > > > > Save that information for later use. > > > > Signed-off-by: Rik van Riel <riel@surriel.com> > > Tested-by: Manali Shukla <Manali.Shukla@amd.com> > > Tested-by: Brendan Jackman <jackmanb@google.com> > > Tested-by: Michael Kelley <mhklinux@outlook.com> > > --- > > arch/x86/Kconfig.cpu | 5 +++++ > > arch/x86/include/asm/cpufeatures.h | 1 + > > arch/x86/include/asm/tlbflush.h | 7 +++++++ > > arch/x86/kernel/cpu/amd.c | 8 ++++++++ > > 4 files changed, 21 insertions(+) > > > > diff --git a/arch/x86/Kconfig.cpu b/arch/x86/Kconfig.cpu > > index 2a7279d80460..abe013a1b076 100644 > > --- a/arch/x86/Kconfig.cpu > > +++ b/arch/x86/Kconfig.cpu > > @@ -395,6 +395,10 @@ config X86_VMX_FEATURE_NAMES > > def_bool y > > depends on IA32_FEAT_CTL > > > > +config X86_BROADCAST_TLB_FLUSH > > + def_bool y > > + depends on CPU_SUP_AMD && 64BIT > > + > > menuconfig PROCESSOR_SELECT > > bool "Supported processor vendors" if EXPERT > > help > > @@ -431,6 +435,7 @@ config CPU_SUP_CYRIX_32 > > config CPU_SUP_AMD > > default y > > bool "Support AMD processors" if PROCESSOR_SELECT > > + select X86_BROADCAST_TLB_FLUSH > > CPU_SUP_AMD selects X86_BROADCAST_TLB_FLUSH which depends on > CPU_SUP_AMD which > selects X86_BROADCAST_TLB_FLUSH which depends on CPU_SUP_AMD... > > Why do you really need yet another Kconfig symbol? Just whack > X86_BROADCAST_TLB_FLUSH - it'll be enabled by default on everything > anyway. Dave specifically asked me to do things that way. > > > @@ -1139,6 +1141,12 @@ static void cpu_detect_tlb_amd(struct > > cpuinfo_x86 *c) > > tlb_lli_2m[ENTRIES] = eax & mask; > > > > tlb_lli_4m[ENTRIES] = tlb_lli_2m[ENTRIES] >> 1; > > + > > + /* Max number of pages INVLPGB can invalidate in one shot > > */ > > + if (boot_cpu_has(X86_FEATURE_INVLPGB)) { > > + cpuid(0x80000008, &eax, &ebx, &ecx, &edx); > > + invlpgb_count_max = (edx & 0xffff) + 1; > > + } > > get_cpu_cap() already reads that leaf. You don't need to do it here > again. > Should I modify get_cpu_cap() to store c->x86_capabilities[CPUID_8000_0008_EDX] ? Currently only the EBX data is stored there, while invlpgb_count_max comes from EDX.
On Wed, Feb 19, 2025 at 12:52:56PM -0500, Rik van Riel wrote: > Should I modify get_cpu_cap() to store > c->x86_capabilities[CPUID_8000_0008_EDX] ? > > Currently only the EBX data is stored there, > while invlpgb_count_max comes from EDX. No, you should simply assign invlpgb_count_max there.
On 2/19/25 09:52, Rik van Riel wrote: >> CPU_SUP_AMD selects X86_BROADCAST_TLB_FLUSH which depends on >> CPU_SUP_AMD which >> selects X86_BROADCAST_TLB_FLUSH which depends on CPU_SUP_AMD... >> >> Why do you really need yet another Kconfig symbol? Just whack >> X86_BROADCAST_TLB_FLUSH - it'll be enabled by default on everything >> anyway. > Dave specifically asked me to do things that way. Yeah, I'm in camp "moar Kconfig!" and Boris is in camp "too many Kconfigs!". I'm OK getting rid of the Kconfig as long as we have _some_ other way to nicely identify the INVLPGB code (which I think the X86_FEATURE gives us) _and_ that the overhead isn't too bad from leaving it compiled in all the time. The mmu_context is the one place that's kinda nasty without a Kconfig option. I guess it's only a few bytes but it's a super silly waste of a few bytes on i386, for example. I also really like the _logical_ clarity that comes from something like: +#ifdef CONFIG_X86_BROADCAST_TLB_FLUSH + u16 global_asid; + bool asid_transition; +#endif as opposed to: +#ifdef CONFIG_64BIT + /* Only used with X86_FEATURE_INVLPGB */ + u16 global_asid; + bool asid_transition; +#endif So, while I respect Boris's concern about Kconfig proliferation, I see the user-invisible ones (the ones without a prompt) as pretty harmless. Sorry for the maintainer crossfire, btw. I definitely don't want to be thrashing you around too much.
diff --git a/arch/x86/Kconfig.cpu b/arch/x86/Kconfig.cpu index 2a7279d80460..abe013a1b076 100644 --- a/arch/x86/Kconfig.cpu +++ b/arch/x86/Kconfig.cpu @@ -395,6 +395,10 @@ config X86_VMX_FEATURE_NAMES def_bool y depends on IA32_FEAT_CTL +config X86_BROADCAST_TLB_FLUSH + def_bool y + depends on CPU_SUP_AMD && 64BIT + menuconfig PROCESSOR_SELECT bool "Supported processor vendors" if EXPERT help @@ -431,6 +435,7 @@ config CPU_SUP_CYRIX_32 config CPU_SUP_AMD default y bool "Support AMD processors" if PROCESSOR_SELECT + select X86_BROADCAST_TLB_FLUSH help This enables detection, tunings and quirks for AMD processors diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h index 508c0dad116b..b5c66b7465ba 100644 --- a/arch/x86/include/asm/cpufeatures.h +++ b/arch/x86/include/asm/cpufeatures.h @@ -338,6 +338,7 @@ #define X86_FEATURE_CLZERO (13*32+ 0) /* "clzero" CLZERO instruction */ #define X86_FEATURE_IRPERF (13*32+ 1) /* "irperf" Instructions Retired Count */ #define X86_FEATURE_XSAVEERPTR (13*32+ 2) /* "xsaveerptr" Always save/restore FP error pointers */ +#define X86_FEATURE_INVLPGB (13*32+ 3) /* INVLPGB and TLBSYNC instruction supported. */ #define X86_FEATURE_RDPRU (13*32+ 4) /* "rdpru" Read processor register at user level */ #define X86_FEATURE_WBNOINVD (13*32+ 9) /* "wbnoinvd" WBNOINVD instruction */ #define X86_FEATURE_AMD_IBPB (13*32+12) /* Indirect Branch Prediction Barrier */ diff --git a/arch/x86/include/asm/tlbflush.h b/arch/x86/include/asm/tlbflush.h index 3da645139748..e026a5cc388e 100644 --- a/arch/x86/include/asm/tlbflush.h +++ b/arch/x86/include/asm/tlbflush.h @@ -183,6 +183,13 @@ static inline void cr4_init_shadow(void) extern unsigned long mmu_cr4_features; extern u32 *trampoline_cr4_features; +/* How many pages can we invalidate with one INVLPGB. */ +#ifdef CONFIG_X86_BROADCAST_TLB_FLUSH +extern u16 invlpgb_count_max; +#else +#define invlpgb_count_max 1 +#endif + extern void initialize_tlbstate_and_flush(void); /* diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c index 54194f5995de..3e8180354303 100644 --- a/arch/x86/kernel/cpu/amd.c +++ b/arch/x86/kernel/cpu/amd.c @@ -29,6 +29,8 @@ #include "cpu.h" +u16 invlpgb_count_max __ro_after_init; + static inline int rdmsrl_amd_safe(unsigned msr, unsigned long long *p) { u32 gprs[8] = { 0 }; @@ -1139,6 +1141,12 @@ static void cpu_detect_tlb_amd(struct cpuinfo_x86 *c) tlb_lli_2m[ENTRIES] = eax & mask; tlb_lli_4m[ENTRIES] = tlb_lli_2m[ENTRIES] >> 1; + + /* Max number of pages INVLPGB can invalidate in one shot */ + if (boot_cpu_has(X86_FEATURE_INVLPGB)) { + cpuid(0x80000008, &eax, &ebx, &ecx, &edx); + invlpgb_count_max = (edx & 0xffff) + 1; + } } static const struct cpu_dev amd_cpu_dev = {