Messages in this thread | | | Subject | Re: [PATCH v2 2/4] arch/x86: Refactor tlbflush and l1d flush | From | "Singh, Balbir" <> | Date | Wed, 8 Apr 2020 00:22:36 +0000 |
| |
On Tue, 2020-04-07 at 11:25 -0700, Kees Cook wrote: > > On Mon, Apr 06, 2020 at 01:19:44PM +1000, Balbir Singh wrote: > > Refactor the existing assembly bits into smaller helper functions > > and also abstract L1D_FLUSH into a helper function. Use these > > functions in kvm for L1D flushing. > > > > Signed-off-by: Balbir Singh <sblbir@amazon.com> > > --- > > arch/x86/include/asm/cacheflush.h | 3 ++ > > arch/x86/kernel/l1d_flush.c | 49 +++++++++++++++++++++++++++++++ > > arch/x86/kvm/vmx/vmx.c | 31 ++++--------------- > > 3 files changed, 57 insertions(+), 26 deletions(-) > > > > diff --git a/arch/x86/include/asm/cacheflush.h > > b/arch/x86/include/asm/cacheflush.h > > index 6419a4cef0e8..66a46db7aadd 100644 > > --- a/arch/x86/include/asm/cacheflush.h > > +++ b/arch/x86/include/asm/cacheflush.h > > @@ -10,5 +10,8 @@ > > void clflush_cache_range(void *addr, unsigned int size); > > void *alloc_l1d_flush_pages(void); > > void cleanup_l1d_flush_pages(void *l1d_flush_pages); > > +void populate_tlb_with_flush_pages(void *l1d_flush_pages); > > +void flush_l1d_cache_sw(void *l1d_flush_pages); > > +int flush_l1d_cache_hw(void); > > > > #endif /* _ASM_X86_CACHEFLUSH_H */ > > diff --git a/arch/x86/kernel/l1d_flush.c b/arch/x86/kernel/l1d_flush.c > > index 05f375c33423..60499f773046 100644 > > --- a/arch/x86/kernel/l1d_flush.c > > +++ b/arch/x86/kernel/l1d_flush.c > > @@ -34,3 +34,52 @@ void cleanup_l1d_flush_pages(void *l1d_flush_pages) > > free_pages((unsigned long)l1d_flush_pages, L1D_CACHE_ORDER); > > } > > EXPORT_SYMBOL_GPL(cleanup_l1d_flush_pages); > > + > > +void populate_tlb_with_flush_pages(void *l1d_flush_pages) > > +{ > > + int size = PAGE_SIZE << L1D_CACHE_ORDER; > > + > > + asm volatile( > > + /* First ensure the pages are in the TLB */ > > + "xorl %%eax, %%eax\n" > > + ".Lpopulate_tlb:\n\t" > > + "movzbl (%[flush_pages], %%" _ASM_AX "), %%ecx\n\t" > > + "addl $4096, %%eax\n\t" > > + "cmpl %%eax, %[size]\n\t" > > + "jne .Lpopulate_tlb\n\t" > > + "xorl %%eax, %%eax\n\t" > > + "cpuid\n\t" > > + :: [flush_pages] "r" (l1d_flush_pages), > > + [size] "r" (size) > > + : "eax", "ebx", "ecx", "edx"); > > +} > > +EXPORT_SYMBOL_GPL(populate_tlb_with_flush_pages); > > + > > +int flush_l1d_cache_hw(void) > > +{ > > + if (static_cpu_has(X86_FEATURE_FLUSH_L1D)) { > > + wrmsrl(MSR_IA32_FLUSH_CMD, L1D_FLUSH); > > + return 1; > > + } > > + return 0; > > +} > > This return value is backwards from the kernel's normal use of "int". I > would expect 0 to mean "success" and non-zero to mean "failure". How > about: > > int flush_l1d_cache_hw(void) > { > if (static_cpu_has(X86_FEATURE_FLUSH_L1D)) { > wrmsrl(MSR_IA32_FLUSH_CMD, L1D_FLUSH); > return 0; > } > return -ENOTSUPP; > } >
Will do
> > > +EXPORT_SYMBOL_GPL(flush_l1d_cache_hw); > > + > > +void flush_l1d_cache_sw(void *l1d_flush_pages) > > +{ > > + int size = PAGE_SIZE << L1D_CACHE_ORDER; > > + > > + asm volatile( > > + /* Fill the cache */ > > + "xorl %%eax, %%eax\n" > > + ".Lfill_cache:\n" > > + "movzbl (%[flush_pages], %%" _ASM_AX "), %%ecx\n\t" > > + "addl $64, %%eax\n\t" > > + "cmpl %%eax, %[size]\n\t" > > + "jne .Lfill_cache\n\t" > > + "lfence\n" > > + :: [flush_pages] "r" (l1d_flush_pages), > > + [size] "r" (size) > > + : "eax", "ecx"); > > +} > > +EXPORT_SYMBOL_GPL(flush_l1d_cache_sw); > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > > index 209e63798435..29dc5a5bb6ab 100644 > > --- a/arch/x86/kvm/vmx/vmx.c > > +++ b/arch/x86/kvm/vmx/vmx.c > > @@ -5956,8 +5956,6 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu, > > */ > > static void vmx_l1d_flush(struct kvm_vcpu *vcpu) > > { > > - int size = PAGE_SIZE << L1D_CACHE_ORDER; > > - > > /* > > * This code is only executed when the the flush mode is 'cond' or > > * 'always' > > @@ -5986,32 +5984,13 @@ static void vmx_l1d_flush(struct kvm_vcpu *vcpu) > > > > vcpu->stat.l1d_flush++; > > > > - if (static_cpu_has(X86_FEATURE_FLUSH_L1D)) { > > - wrmsrl(MSR_IA32_FLUSH_CMD, L1D_FLUSH); > > + if (flush_l1d_cache_hw()) > > return; > > - } > > Then this becomes: > > if (flush_l1d_cache_hw() == 0) > return; > > (Or change it to a "bool" with and use true/false and leave the above > call as-is.) > > Either way: > > Reviewed-by: Kees Cook <keescook@chromium.org> >
Thanks, Balbir Singh.
| |