diff mbox series

[v7,3/6] powerpc/perf: consolidate read_user_stack_32

Message ID ea3783a1640b707ef9ce4740562850ef1152829b.1567198491.git.msuchanek@suse.de (mailing list archive)
State New, archived
Headers show
Series Disable compat cruft on ppc64le v7 | expand

Commit Message

Michal Suchánek Aug. 30, 2019, 9:03 p.m. UTC 
There are two almost identical copies for 32bit and 64bit.

The function is used only in 32bit code which will be split out in next
patch so consolidate to one function.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
new patch in v6
---
 arch/powerpc/perf/callchain.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

Comments

Michael Ellerman Sept. 2, 2019, 3:53 a.m. UTC | #1 
Michal Suchanek <msuchanek@suse.de> writes:

> There are two almost identical copies for 32bit and 64bit.
>
> The function is used only in 32bit code which will be split out in next
> patch so consolidate to one function.
>
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> new patch in v6
> ---
>  arch/powerpc/perf/callchain.c | 25 +++++++++----------------
>  1 file changed, 9 insertions(+), 16 deletions(-)
>
> diff --git a/arch/powerpc/perf/callchain.c b/arch/powerpc/perf/callchain.c
> index c84bbd4298a0..b7cdcce20280 100644
> --- a/arch/powerpc/perf/callchain.c
> +++ b/arch/powerpc/perf/callchain.c
> @@ -165,22 +165,6 @@ static int read_user_stack_64(unsigned long __user *ptr, unsigned long *ret)
>  	return read_user_stack_slow(ptr, ret, 8);
>  }
>  
> -static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
> -{
> -	if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned int) ||
> -	    ((unsigned long)ptr & 3))
> -		return -EFAULT;
> -
> -	pagefault_disable();
> -	if (!__get_user_inatomic(*ret, ptr)) {
> -		pagefault_enable();
> -		return 0;
> -	}
> -	pagefault_enable();
> -
> -	return read_user_stack_slow(ptr, ret, 4);
> -}
> -
>  static inline int valid_user_sp(unsigned long sp, int is_64)
>  {
>  	if (!sp || (sp & 7) || sp > (is_64 ? TASK_SIZE : 0x100000000UL) - 32)
> @@ -295,6 +279,12 @@ static inline int current_is_64bit(void)
>  }
>  
>  #else  /* CONFIG_PPC64 */
> +static int read_user_stack_slow(void __user *ptr, void *buf, int nb)
> +{
> +	return 0;
> +}
> +#endif /* CONFIG_PPC64 */

Ending the PPC64 else case here, and then restarting it below with an
ifndef means we end up with two parts of the file that define 32-bit
code, with a common chunk in the middle, which I dislike.

I'd rather you add the empty read_user_stack_slow() in the existing
#else section and then move read_user_stack_32() below the whole ifdef
PPC64/else/endif section.

Is there some reason that doesn't work?

cheers

> @@ -313,9 +303,12 @@ static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
>  	rc = __get_user_inatomic(*ret, ptr);
>  	pagefault_enable();
>  
> +	if (IS_ENABLED(CONFIG_PPC64) && rc)
> +		return read_user_stack_slow(ptr, ret, 4);
>  	return rc;
>  }
>  
> +#ifndef CONFIG_PPC64
>  static inline void perf_callchain_user_64(struct perf_callchain_entry_ctx *entry,
>  					  struct pt_regs *regs)
>  {
> -- 
> 2.22.0
Michael Ellerman Sept. 2, 2019, 4:01 a.m. UTC | #2 
Michael Ellerman <mpe@ellerman.id.au> writes:
> Michal Suchanek <msuchanek@suse.de> writes:
...
>> @@ -295,6 +279,12 @@ static inline int current_is_64bit(void)
>>  }
>>  
>>  #else  /* CONFIG_PPC64 */
>> +static int read_user_stack_slow(void __user *ptr, void *buf, int nb)
>> +{
>> +	return 0;
>> +}
>> +#endif /* CONFIG_PPC64 */
>
> Ending the PPC64 else case here, and then restarting it below with an
> ifndef means we end up with two parts of the file that define 32-bit
> code, with a common chunk in the middle, which I dislike.
>
> I'd rather you add the empty read_user_stack_slow() in the existing
> #else section and then move read_user_stack_32() below the whole ifdef
> PPC64/else/endif section.
>
> Is there some reason that doesn't work?

Gah, I missed that you split the whole file later in the series. Any
reason you did it in two steps rather than moving patch 6 earlier in the
series?

cheers
Michal Suchánek Sept. 2, 2019, 8:26 a.m. UTC | #3 
On Mon, 02 Sep 2019 14:01:17 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:

> Michael Ellerman <mpe@ellerman.id.au> writes:
> > Michal Suchanek <msuchanek@suse.de> writes:  
> ...
> >> @@ -295,6 +279,12 @@ static inline int current_is_64bit(void)
> >>  }
> >>  
> >>  #else  /* CONFIG_PPC64 */
> >> +static int read_user_stack_slow(void __user *ptr, void *buf, int nb)
> >> +{
> >> +	return 0;
> >> +}
> >> +#endif /* CONFIG_PPC64 */  
> >
> > Ending the PPC64 else case here, and then restarting it below with an
> > ifndef means we end up with two parts of the file that define 32-bit
> > code, with a common chunk in the middle, which I dislike.
> >
> > I'd rather you add the empty read_user_stack_slow() in the existing
> > #else section and then move read_user_stack_32() below the whole ifdef
> > PPC64/else/endif section.
> >
> > Is there some reason that doesn't work?  
> 
> Gah, I missed that you split the whole file later in the series. Any
> reason you did it in two steps rather than moving patch 6 earlier in the
> series?

To make this patch readable.

Thanks

Michal
Michael Ellerman Sept. 2, 2019, 11:45 p.m. UTC | #4 
Michal Suchánek <msuchanek@suse.de> writes:
> On Mon, 02 Sep 2019 14:01:17 +1000
> Michael Ellerman <mpe@ellerman.id.au> wrote:
>> Michael Ellerman <mpe@ellerman.id.au> writes:
>> > Michal Suchanek <msuchanek@suse.de> writes:  
>> ...
>> >> @@ -295,6 +279,12 @@ static inline int current_is_64bit(void)
>> >>  }
>> >>  
>> >>  #else  /* CONFIG_PPC64 */
>> >> +static int read_user_stack_slow(void __user *ptr, void *buf, int nb)
>> >> +{
>> >> +	return 0;
>> >> +}
>> >> +#endif /* CONFIG_PPC64 */  
>> >
>> > Ending the PPC64 else case here, and then restarting it below with an
>> > ifndef means we end up with two parts of the file that define 32-bit
>> > code, with a common chunk in the middle, which I dislike.
>> >
>> > I'd rather you add the empty read_user_stack_slow() in the existing
>> > #else section and then move read_user_stack_32() below the whole ifdef
>> > PPC64/else/endif section.
>> >
>> > Is there some reason that doesn't work?  
>> 
>> Gah, I missed that you split the whole file later in the series. Any
>> reason you did it in two steps rather than moving patch 6 earlier in the
>> series?
>
> To make this patch readable.

But it's not very readable :)

You also retained the comment about the 32-bit behaviour which is now a
bit confusing, because the function is used on both 32 & 64-bit.

I think moving it as I suggested in my first reply makes for a better
diff, something like eg:

diff --git a/arch/powerpc/perf/callchain.c b/arch/powerpc/perf/callchain.c
index c84bbd4298a0..82c0f81b89a5 100644
--- a/arch/powerpc/perf/callchain.c
+++ b/arch/powerpc/perf/callchain.c
@@ -165,22 +165,6 @@ static int read_user_stack_64(unsigned long __user *ptr, unsigned long *ret)
 	return read_user_stack_slow(ptr, ret, 8);
 }
 
-static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
-{
-	if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned int) ||
-	    ((unsigned long)ptr & 3))
-		return -EFAULT;
-
-	pagefault_disable();
-	if (!__get_user_inatomic(*ret, ptr)) {
-		pagefault_enable();
-		return 0;
-	}
-	pagefault_enable();
-
-	return read_user_stack_slow(ptr, ret, 4);
-}
-
 static inline int valid_user_sp(unsigned long sp, int is_64)
 {
 	if (!sp || (sp & 7) || sp > (is_64 ? TASK_SIZE : 0x100000000UL) - 32)
@@ -295,27 +279,6 @@ static inline int current_is_64bit(void)
 }
 
 #else  /* CONFIG_PPC64 */
-/*
- * On 32-bit we just access the address and let hash_page create a
- * HPTE if necessary, so there is no need to fall back to reading
- * the page tables.  Since this is called at interrupt level,
- * do_page_fault() won't treat a DSI as a page fault.
- */
-static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
-{
-	int rc;
-
-	if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned int) ||
-	    ((unsigned long)ptr & 3))
-		return -EFAULT;
-
-	pagefault_disable();
-	rc = __get_user_inatomic(*ret, ptr);
-	pagefault_enable();
-
-	return rc;
-}
-
 static inline void perf_callchain_user_64(struct perf_callchain_entry_ctx *entry,
 					  struct pt_regs *regs)
 {
@@ -333,6 +296,11 @@ static inline int valid_user_sp(unsigned long sp, int is_64)
 	return 1;
 }
 
+static int read_user_stack_slow(void __user *ptr, void *buf, int nb)
+{
+	return 0;
+}
+
 #define __SIGNAL_FRAMESIZE32	__SIGNAL_FRAMESIZE
 #define sigcontext32		sigcontext
 #define mcontext32		mcontext
@@ -341,6 +309,33 @@ static inline int valid_user_sp(unsigned long sp, int is_64)
 
 #endif /* CONFIG_PPC64 */
 
+static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
+{
+	int rc;
+
+	if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned int) ||
+	    ((unsigned long)ptr & 3))
+		return -EFAULT;
+
+	pagefault_disable();
+	rc = __get_user_inatomic(*ret, ptr);
+	pagefault_enable();
+
+	/*
+	 * On 32-bit we just access the address and let hash_page() create a
+	 * HPTE if necessary, so there is no need to fall back to reading the
+	 * page tables. Since this is called at interrupt level, do_page_fault()
+	 * won't treat a DSI as a page fault.
+	 *
+	 * On 64-bit if the access faults we fall back to
+	 * read_user_stack_slow(), see the comment there for more details.
+	 */
+	if (IS_ENABLED(CONFIG_PPC64) && rc)
+		return read_user_stack_slow(ptr, ret, 4);
+
+	return rc;
+}
+
 /*
  * Layout for non-RT signal frames
  */


cheers
diff mbox series

Patch

diff --git a/arch/powerpc/perf/callchain.c b/arch/powerpc/perf/callchain.c
index c84bbd4298a0..b7cdcce20280 100644
--- a/arch/powerpc/perf/callchain.c
+++ b/arch/powerpc/perf/callchain.c
@@ -165,22 +165,6 @@  static int read_user_stack_64(unsigned long __user *ptr, unsigned long *ret)
 	return read_user_stack_slow(ptr, ret, 8);
 }
 
-static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
-{
-	if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned int) ||
-	    ((unsigned long)ptr & 3))
-		return -EFAULT;
-
-	pagefault_disable();
-	if (!__get_user_inatomic(*ret, ptr)) {
-		pagefault_enable();
-		return 0;
-	}
-	pagefault_enable();
-
-	return read_user_stack_slow(ptr, ret, 4);
-}
-
 static inline int valid_user_sp(unsigned long sp, int is_64)
 {
 	if (!sp || (sp & 7) || sp > (is_64 ? TASK_SIZE : 0x100000000UL) - 32)
@@ -295,6 +279,12 @@  static inline int current_is_64bit(void)
 }
 
 #else  /* CONFIG_PPC64 */
+static int read_user_stack_slow(void __user *ptr, void *buf, int nb)
+{
+	return 0;
+}
+#endif /* CONFIG_PPC64 */
+
 /*
  * On 32-bit we just access the address and let hash_page create a
  * HPTE if necessary, so there is no need to fall back to reading
@@ -313,9 +303,12 @@  static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
 	rc = __get_user_inatomic(*ret, ptr);
 	pagefault_enable();
 
+	if (IS_ENABLED(CONFIG_PPC64) && rc)
+		return read_user_stack_slow(ptr, ret, 4);
 	return rc;
 }
 
+#ifndef CONFIG_PPC64
 static inline void perf_callchain_user_64(struct perf_callchain_entry_ctx *entry,
 					  struct pt_regs *regs)
 {