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 <[email protected]>
Cc: Thomas Lendacky <[email protected]>
Cc: Juergen Gross <[email protected]>
---
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();
}
void __init sme_unmap_bootdata(char *real_mode_data)
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -18,6 +18,13 @@
#include "mm_internal.h"
+#ifdef CONFIG_PARAVIRT
+# define STATIC_NOPV
+#else
+# define STATIC_NOPV static
+# define __flush_tlb_local native_flush_tlb_local
+#endif
+
/*
* TLB flushing, formerly SMP-only
* c/o Linus Torvalds.
@@ -645,7 +652,7 @@ static void flush_tlb_func_common(const
trace_tlb_flush(reason, nr_invalidate);
} else {
/* Full flush. */
- local_flush_tlb();
+ flush_tlb_local();
if (local)
count_vm_tlb_event(NR_TLB_LOCAL_FLUSH_ALL);
trace_tlb_flush(reason, TLB_FLUSH_ALL);
@@ -884,6 +891,30 @@ unsigned long __get_current_cr3_fast(voi
EXPORT_SYMBOL_GPL(__get_current_cr3_fast);
/*
+ * Flush the entire current user mapping
+ */
+STATIC_NOPV void native_flush_tlb_local(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());
+}
+
+void flush_tlb_local(void)
+{
+ __flush_tlb_local();
+}
+EXPORT_SYMBOL_GPL(flush_tlb_local);
+
+/*
* arch_tlbbatch_flush() performs a full TLB flush regardless of the active mm.
* This means that the 'struct flush_tlb_info' that describes which mappings to
* flush is actually fixed. We therefore set a single fixed struct and use it in
--- a/arch/x86/platform/uv/tlb_uv.c
+++ b/arch/x86/platform/uv/tlb_uv.c
@@ -293,7 +293,7 @@ static void bau_process_message(struct m
* This must be a normal message, or retry of a normal message
*/
if (msg->address == TLB_FLUSH_ALL) {
- local_flush_tlb();
+ flush_tlb_local();
stat->d_alltlb++;
} else {
__flush_tlb_one_user(msg->address);
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 <[email protected]>
> Cc: Thomas Lendacky <[email protected]>
> Cc: Juergen Gross <[email protected]>
> ---
> 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.
Thanks,
Tom
> }
>
> void __init sme_unmap_bootdata(char *real_mode_data)
> --- a/arch/x86/mm/tlb.c
> +++ b/arch/x86/mm/tlb.c
> @@ -18,6 +18,13 @@
>
> #include "mm_internal.h"
>
> +#ifdef CONFIG_PARAVIRT
> +# define STATIC_NOPV
> +#else
> +# define STATIC_NOPV static
> +# define __flush_tlb_local native_flush_tlb_local
> +#endif
> +
> /*
> * TLB flushing, formerly SMP-only
> * c/o Linus Torvalds.
> @@ -645,7 +652,7 @@ static void flush_tlb_func_common(const
> trace_tlb_flush(reason, nr_invalidate);
> } else {
> /* Full flush. */
> - local_flush_tlb();
> + flush_tlb_local();
> if (local)
> count_vm_tlb_event(NR_TLB_LOCAL_FLUSH_ALL);
> trace_tlb_flush(reason, TLB_FLUSH_ALL);
> @@ -884,6 +891,30 @@ unsigned long __get_current_cr3_fast(voi
> EXPORT_SYMBOL_GPL(__get_current_cr3_fast);
>
> /*
> + * Flush the entire current user mapping
> + */
> +STATIC_NOPV void native_flush_tlb_local(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());
> +}
> +
> +void flush_tlb_local(void)
> +{
> + __flush_tlb_local();
> +}
> +EXPORT_SYMBOL_GPL(flush_tlb_local);
> +
> +/*
> * arch_tlbbatch_flush() performs a full TLB flush regardless of the active mm.
> * This means that the 'struct flush_tlb_info' that describes which mappings to
> * flush is actually fixed. We therefore set a single fixed struct and use it in
> --- a/arch/x86/platform/uv/tlb_uv.c
> +++ b/arch/x86/platform/uv/tlb_uv.c
> @@ -293,7 +293,7 @@ static void bau_process_message(struct m
> * This must be a normal message, or retry of a normal message
> */
> if (msg->address == TLB_FLUSH_ALL) {
> - local_flush_tlb();
> + flush_tlb_local();
> stat->d_alltlb++;
> } else {
> __flush_tlb_one_user(msg->address);
>
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 <[email protected]>
>>> Cc: Thomas Lendacky <[email protected]>
>>> Cc: Juergen Gross <[email protected]>
>>> ---
>>> 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
On 20.04.20 16:26, Tom Lendacky wrote:
> 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 <[email protected]>
>>>> Cc: Thomas Lendacky <[email protected]>
>>>> Cc: Juergen Gross <[email protected]>
>>>> ---
>>>> 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?
4.20.
Juergen
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 <[email protected]>
>> Cc: Thomas Lendacky <[email protected]>
>> Cc: Juergen Gross <[email protected]>
>> ---
>> 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.
Juergen
On 4/20/20 9:38 AM, Jürgen Groß wrote:
> On 20.04.20 16:26, Tom Lendacky wrote:
>> 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 <[email protected]>
>>>>> Cc: Thomas Lendacky <[email protected]>
>>>>> Cc: Juergen Gross <[email protected]>
>>>>> ---
>>>>> 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?
>
> 4.20.
Ok, my (limited) testing appears good at this point. I see a couple of
calls to ftrace (__fentry__) that just return before invoking the native
version of the TLB flushing function.
I don't remember what the original problem was that required the call to
__native_flush_tlb(), maybe some overall kernel instrumentation that once
removed via the Makefile took care of it. But it appears ok now, so
disregard my original comment.
Thanks,
Tom
>
>
> Juergen
>