Re: [patch 05/15] x86/tlb: Move __flush_tlb() out of line

From: Tom Lendacky
Date: Mon Apr 20 2020 - 10:27:03 EST

On 4/20/20 9:03 AM, JÃrgen Groà wrote:
On 20.04.20 15:48, Tom Lendacky wrote:
On 4/19/20 3:31 PM, Thomas Gleixner wrote:
cpu_tlbstate is exported because various TLB related functions need access
to it, but cpu_tlbstate is sensitive information which should only be
accessed by well contained kernel functions and not be directly exposed to
modules.

The various TLB flush functions need access to cpu_tlbstate. As a first
step move __flush_tlb() out of line and hide the native function. The
latter can be static when CONFIG_PARAVIRT is disabled.

Consolidate the name space while at it and remove the pointless extra
wrapper in the paravirt code.

No functional change.

Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
Cc: Thomas Lendacky <Thomas.Lendacky@xxxxxxx>
Cc: Juergen Gross <jgross@xxxxxxxx>
---
 arch/x86/include/asm/paravirt.h | 4 +++-
 arch/x86/include/asm/tlbflush.h | 29 +++++------------------------
 arch/x86/kernel/cpu/mtrr/generic.c | 4 ++--
 arch/x86/kernel/paravirt.c | 7 +------
 arch/x86/mm/mem_encrypt.c | 2 +-
 arch/x86/mm/tlb.c | 33 ++++++++++++++++++++++++++++++++-
 arch/x86/platform/uv/tlb_uv.c | 2 +-
 7 files changed, 45 insertions(+), 36 deletions(-)

--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -47,7 +47,9 @@ static inline void slow_down_io(void)
 #endif
 }
-static inline void __flush_tlb(void)
+void native_flush_tlb_local(void);
+
+static inline void __flush_tlb_local(void)
 {
ÂÂÂÂÂ PVOP_VCALL0(mmu.flush_tlb_user);
 }
--- a/arch/x86/include/asm/tlbflush.h
+++ b/arch/x86/include/asm/tlbflush.h
@@ -140,12 +140,13 @@ static inline unsigned long build_cr3_no
ÂÂÂÂÂ return __sme_pa(pgd) | kern_pcid(asid) | CR3_NOFLUSH;
 }
+void flush_tlb_local(void);
+
 #ifdef CONFIG_PARAVIRT
 #include <asm/paravirt.h>
 #else
-#define __flush_tlb() __native_flush_tlb()
-#define __flush_tlb_global() __native_flush_tlb_global()
-#define __flush_tlb_one_user(addr) __native_flush_tlb_one_user(addr)
+#define __flush_tlb_global()ÂÂÂÂÂÂÂ __native_flush_tlb_global()
+#define __flush_tlb_one_user(addr)ÂÂÂ __native_flush_tlb_one_user(addr)
 #endif
 struct tlb_context {
@@ -371,24 +372,6 @@ static inline void invalidate_user_asid(
 }
 /*
- * flush the entire current user mapping
- */
-static inline void __native_flush_tlb(void)
-{
-ÂÂÂ /*
-ÂÂÂÂ * Preemption or interrupts must be disabled to protect the access
-ÂÂÂÂ * to the per CPU variable and to prevent being preempted between
-ÂÂÂÂ * read_cr3() and write_cr3().
-ÂÂÂÂ */
-ÂÂÂ WARN_ON_ONCE(preemptible());
-
-ÂÂÂ invalidate_user_asid(this_cpu_read(cpu_tlbstate.loaded_mm_asid));
-
-ÂÂÂ /* If current->mm == NULL then the read_cr3() "borrows" an mm */
-ÂÂÂ native_write_cr3(__native_read_cr3());
-}
-
-/*
ÂÂ * flush everything
ÂÂ */
 static inline void __native_flush_tlb_global(void)
@@ -461,7 +444,7 @@ static inline void __flush_tlb_all(void)
ÂÂÂÂÂÂÂÂÂ /*
ÂÂÂÂÂÂÂÂÂÂ * !PGE -> !PCID (setup_pcid()), thus every flush is total.
ÂÂÂÂÂÂÂÂÂÂ */
-ÂÂÂÂÂÂÂ __flush_tlb();
+ÂÂÂÂÂÂÂ flush_tlb_local();
ÂÂÂÂÂ }
 }
@@ -537,8 +520,6 @@ struct flush_tlb_info {
ÂÂÂÂÂ boolÂÂÂÂÂÂÂÂÂÂÂ freed_tables;
 };
-#define local_flush_tlb() __flush_tlb()
-
 #define flush_tlb_mm(mm) \
ÂÂÂÂÂÂÂÂÂ flush_tlb_mm_range(mm, 0UL, TLB_FLUSH_ALL, 0UL, true)
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -761,7 +761,7 @@ static void prepare_set(void) __acquires
ÂÂÂÂÂ /* Flush all TLBs via a mov %cr3, %reg; mov %reg, %cr3 */
ÂÂÂÂÂ count_vm_tlb_event(NR_TLB_LOCAL_FLUSH_ALL);
-ÂÂÂ __flush_tlb();
+ÂÂÂ flush_tlb_local();
ÂÂÂÂÂ /* Save MTRR state */
ÂÂÂÂÂ rdmsr(MSR_MTRRdefType, deftype_lo, deftype_hi);
@@ -778,7 +778,7 @@ static void post_set(void) __releases(se
 {
ÂÂÂÂÂ /* Flush TLBs (no need to flush caches - they are disabled) */
ÂÂÂÂÂ count_vm_tlb_event(NR_TLB_LOCAL_FLUSH_ALL);
-ÂÂÂ __flush_tlb();
+ÂÂÂ flush_tlb_local();
ÂÂÂÂÂ /* Intel (P6) standard MTRRs */
ÂÂÂÂÂ mtrr_wrmsr(MSR_MTRRdefType, deftype_lo, deftype_hi);
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -160,11 +160,6 @@ unsigned paravirt_patch_insns(void *insn
ÂÂÂÂÂ return insn_len;
 }
-static void native_flush_tlb(void)
-{
-ÂÂÂ __native_flush_tlb();
-}
-
 /*
ÂÂ * Global pages have to be flushed a bit differently. Not a real
ÂÂ * performance problem because this does not happen often.
@@ -359,7 +354,7 @@ struct paravirt_patch_template pv_ops =
 #endif /* CONFIG_PARAVIRT_XXL */
ÂÂÂÂÂ /* Mmu ops. */
-ÂÂÂ .mmu.flush_tlb_userÂÂÂ = native_flush_tlb,
+ÂÂÂ .mmu.flush_tlb_userÂÂÂ = native_flush_tlb_local,
ÂÂÂÂÂ .mmu.flush_tlb_kernelÂÂÂ = native_flush_tlb_global,
ÂÂÂÂÂ .mmu.flush_tlb_one_userÂÂÂ = native_flush_tlb_one_user,
ÂÂÂÂÂ .mmu.flush_tlb_othersÂÂÂ = native_flush_tlb_others,
--- a/arch/x86/mm/mem_encrypt.c
+++ b/arch/x86/mm/mem_encrypt.c
@@ -134,7 +134,7 @@ static void __init __sme_early_map_unmap
ÂÂÂÂÂÂÂÂÂ size = (size <= PMD_SIZE) ? 0 : size - PMD_SIZE;
ÂÂÂÂÂ } while (size);
-ÂÂÂ __native_flush_tlb();
+ÂÂÂ flush_tlb_local();

This invoked __native_flush_tlb() because of how early it is called and the paravirt ops support isn't set up yet, resulting in a crash if not invoking the native version directly. So this needs a "native" version of the tlb flush to invoke.

I don't think this is still true. With my rework of pvops to have all
functions in one struct which is initialized statically initially
everything should work from the time the kernel is mapped.

In case it doesn't there is something very wrong IMO.

The memory encryption support was implemented in 4.14, so it's quite possible that this isn't an issue now. I'll test out the patch and verify it. What release did your pvops rework land in?

Thanks,
Tom



Juergen