[PATCH v9 5/7] powerpc/tlb: Add local flush for page given mm_struct and psize

[Christophe Leroy]

Le 25/10/2022 à 06:44, Benjamin Gray a écrit :
> Adds a local TLB flush operation that works given an mm_struct, VA to
> flush, and page size representation. Most implementations mirror the
> surrounding code. The book3s/32/tlbflush.h implementation is left as
> a WARN_ONCE_ON because it is more complicated and not required for

s/ONCE_ON/ONCE

> anything as yet.

Is a WARN_ONCE() really needed ? Can't a BUILD_BUG() be used instead ?


> 
> This removes the need to create a vm_area_struct, which the temporary
> patching mm work does not need.
> 
> Signed-off-by: Benjamin Gray <bgray at linux.ibm.com>
> ---
> v9:	* Replace book3s/32/tlbflush.h implementation with warning
> ---
>   arch/powerpc/include/asm/book3s/32/tlbflush.h      | 9 +++++++++
>   arch/powerpc/include/asm/book3s/64/tlbflush-hash.h | 5 +++++
>   arch/powerpc/include/asm/book3s/64/tlbflush.h      | 8 ++++++++
>   arch/powerpc/include/asm/nohash/tlbflush.h         | 8 ++++++++
>   4 files changed, 30 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/book3s/32/tlbflush.h b/arch/powerpc/include/asm/book3s/32/tlbflush.h
> index ba1743c52b56..14d989d41f75 100644
> --- a/arch/powerpc/include/asm/book3s/32/tlbflush.h
> +++ b/arch/powerpc/include/asm/book3s/32/tlbflush.h
> @@ -2,6 +2,8 @@
>   #ifndef _ASM_POWERPC_BOOK3S_32_TLBFLUSH_H
>   #define _ASM_POWERPC_BOOK3S_32_TLBFLUSH_H
>   
> +#include <asm/bug.h>
> +
>   #define MMU_NO_CONTEXT      (0)
>   /*
>    * TLB flushing for "classic" hash-MMU 32-bit CPUs, 6xx, 7xx, 7xxx
> @@ -74,6 +76,13 @@ static inline void local_flush_tlb_page(struct vm_area_struct *vma,
>   {
>   	flush_tlb_page(vma, vmaddr);
>   }
> +
> +static inline void local_flush_tlb_page_psize(struct mm_struct *mm,
> +					      unsigned long vmaddr, int psize)
> +{
> +	WARN_ONCE(true, "local TLB flush not implemented");

Is it possible to use BUILD_BUG() instead ?

> +}
> +
>   static inline void local_flush_tlb_mm(struct mm_struct *mm)
>   {
>   	flush_tlb_mm(mm);
> diff --git a/arch/powerpc/include/asm/book3s/64/tlbflush-hash.h b/arch/powerpc/include/asm/book3s/64/tlbflush-hash.h
> index fab8332fe1ad..8fd9dc49b2a1 100644
> --- a/arch/powerpc/include/asm/book3s/64/tlbflush-hash.h
> +++ b/arch/powerpc/include/asm/book3s/64/tlbflush-hash.h
> @@ -94,6 +94,11 @@ static inline void hash__local_flush_tlb_page(struct vm_area_struct *vma,
>   {
>   }
>   
> +static inline void hash__local_flush_tlb_page_psize(struct mm_struct *mm,
> +						    unsigned long vmaddr, int psize)
> +{
> +}
> +

Is it worth an empty function ?

>   static inline void hash__flush_tlb_page(struct vm_area_struct *vma,
>   				    unsigned long vmaddr)
>   {
> diff --git a/arch/powerpc/include/asm/book3s/64/tlbflush.h b/arch/powerpc/include/asm/book3s/64/tlbflush.h
> index 67655cd60545..2d839dd5c08c 100644
> --- a/arch/powerpc/include/asm/book3s/64/tlbflush.h
> +++ b/arch/powerpc/include/asm/book3s/64/tlbflush.h
> @@ -92,6 +92,14 @@ static inline void local_flush_tlb_page(struct vm_area_struct *vma,
>   	return hash__local_flush_tlb_page(vma, vmaddr);
>   }
>   
> +static inline void local_flush_tlb_page_psize(struct mm_struct *mm,
> +					      unsigned long vmaddr, int psize)
> +{
> +	if (radix_enabled())
> +		return radix__local_flush_tlb_page_psize(mm, vmaddr, psize);
> +	return hash__local_flush_tlb_page_psize(mm, vmaddr, psize);

Those functions are 'void', shouldn't need the "return".

Could just be:

+static inline void local_flush_tlb_page_psize(struct mm_struct *mm,
+					      unsigned long vmaddr, int psize)
+{
+	if (radix_enabled())
+		radix__local_flush_tlb_page_psize(mm, vmaddr, psize);
+}


> +}
> +
>   static inline void local_flush_all_mm(struct mm_struct *mm)
>   {
>   	if (radix_enabled())
> diff --git a/arch/powerpc/include/asm/nohash/tlbflush.h b/arch/powerpc/include/asm/nohash/tlbflush.h
> index bdaf34ad41ea..432bca4cac62 100644
> --- a/arch/powerpc/include/asm/nohash/tlbflush.h
> +++ b/arch/powerpc/include/asm/nohash/tlbflush.h
> @@ -45,6 +45,12 @@ static inline void local_flush_tlb_page(struct vm_area_struct *vma, unsigned lon
>   	asm volatile ("tlbie %0; sync" : : "r" (vmaddr) : "memory");
>   }
>   
> +static inline void local_flush_tlb_page_psize(struct mm_struct *mm,
> +					      unsigned long vmaddr, int psize)
> +{
> +	asm volatile ("tlbie %0; sync" : : "r" (vmaddr) : "memory");
> +}
> +
>   static inline void flush_tlb_kernel_range(unsigned long start, unsigned long end)
>   {
>   	start &= PAGE_MASK;
> @@ -58,6 +64,8 @@ static inline void flush_tlb_kernel_range(unsigned long start, unsigned long end
>   extern void flush_tlb_kernel_range(unsigned long start, unsigned long end);
>   extern void local_flush_tlb_mm(struct mm_struct *mm);
>   extern void local_flush_tlb_page(struct vm_area_struct *vma, unsigned long vmaddr);
> +extern void local_flush_tlb_page_psize(struct mm_struct *mm,
> +				       unsigned long vmaddr, int psize);

That function doesn't seem to be defined anywhere. Is this prototype 
just to make compiler happy ? If so a static inline with a BUILD_BUG 
would likely be better, it would allow detection of a build problem at 
compile time instead of link time.

By the way, 'extern' keyword is pointless and deprecated for functions 
prototypes, please don't add new ones, even if other historical 
prototypes have one.

>   
>   extern void __local_flush_tlb_page(struct mm_struct *mm, unsigned long vmaddr,
>   				   int tsize, int ind);