[LTP] [PATCH] syscalls/mprotect04: Use __builtin___clear_cache() to sync caches

[LTP] [PATCH] syscalls/mprotect04: Use __builtin___clear_cache() to sync caches

[Peter Maydell]

In commit cf9a0800cd0 code was added to mprotect04 to synchronize
the instruction and data caches on PowerPC before executing
the copied code. This is also necessary on other architectures
which have split instruction and data caches and need explicit
cache maintenance operations, like ARM.

The GCC builtin __builtin___clear_cache() will correctly handle
this for all architectures, so switch to using it. The builtin
was only implemented at around GCC 4.1, so use a configure
check so that we will skip the test with TCONF if the compiler
doesn't support the builtin.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Disclaimer: untested on PPC.

 configure.ac                                    |  1 +
 m4/ltp-builtin_clear_cache.m4                   | 30 +++++++++++++++++++++++++
 testcases/kernel/syscalls/mprotect/mprotect04.c | 20 ++++++++++-------
 3 files changed, 43 insertions(+), 8 deletions(-)
 create mode 100644 m4/ltp-builtin_clear_cache.m4

diff --git a/configure.ac b/configure.ac
index e0e9c1b..9901612 100644
--- a/configure.ac
+++ b/configure.ac
@@ -188,5 +188,6 @@ LTP_CHECK_PWRITEV
 LTP_CHECK_EPOLL_PWAIT
 LTP_CHECK_KEYUTILS_SUPPORT
 LTP_CHECK_SYNC_ADD_AND_FETCH
+LTP_CHECK_BUILTIN_CLEAR_CACHE
 
 AC_OUTPUT
diff --git a/m4/ltp-builtin_clear_cache.m4 b/m4/ltp-builtin_clear_cache.m4
new file mode 100644
index 0000000..74e4ccd
--- /dev/null
+++ b/m4/ltp-builtin_clear_cache.m4
@@ -0,0 +1,30 @@
+dnl
+dnl Copyright (c) Linux Test Project, 2016
+dnl
+dnl This program is free software;  you can redistribute it and/or modify
+dnl it under the terms of the GNU General Public License as published by
+dnl the Free Software Foundation; either version 2 of the License, or
+dnl (at your option) any later version.
+dnl
+dnl This program is distributed in the hope that it will be useful,
+dnl but WITHOUT ANY WARRANTY;  without even the implied warranty of
+dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
+dnl the GNU General Public License for more details.
+dnl
+
+AC_DEFUN([LTP_CHECK_BUILTIN_CLEAR_CACHE],[dnl
+	AC_MSG_CHECKING([for __builtin___clear_cache])
+	AC_LINK_IFELSE([AC_LANG_SOURCE([[
+int main(void) {
+	char arr[16];
+	__builtin___clear_cache(arr, arr + sizeof(arr));
+        return 0;
+}]])],[has_bcc="yes"])
+
+if test "x$has_bcc" = xyes; then
+	AC_DEFINE(HAVE_BUILTIN_CLEAR_CACHE,1,[Define to 1 if you have __builtin___clear_cache])
+	AC_MSG_RESULT(yes)
+else
+	AC_MSG_RESULT(no)
+fi
+])
diff --git a/testcases/kernel/syscalls/mprotect/mprotect04.c b/testcases/kernel/syscalls/mprotect/mprotect04.c
index c94e25c..1173afd 100644
--- a/testcases/kernel/syscalls/mprotect/mprotect04.c
+++ b/testcases/kernel/syscalls/mprotect/mprotect04.c
@@ -24,6 +24,7 @@
  *      'prot' is set to PROT_EXEC.
  */
 
+#include "config.h"
 #include <signal.h>
 #include <setjmp.h>
 #include <sys/types.h>
@@ -178,6 +179,16 @@ static int page_present(void *p)
 	return 0;
 }
 
+static void clear_cache(void *start, int len)
+{
+#if HAVE_BUILTIN_CLEAR_CACHE == 1
+	__builtin___clear_cache(start, start + len);
+#else
+	tst_brkm(TCONF, cleanup,
+		"compiler doesn't have __builtin___clear_cache()");
+#endif
+}
+
 /*
  * Copy page where &exec_func resides. Also try to copy subsequent page
  * in case exec_func is close to page boundary.
@@ -189,10 +200,7 @@ static void *get_func(void *mem)
 	uintptr_t func_page_offset = (uintptr_t)&exec_func & (page_sz - 1);
 	void *func_copy_start = mem + func_page_offset;
 	void *page_to_copy = (void *)((uintptr_t)&exec_func & page_mask);
-#ifdef __powerpc__
 	void *mem_start = mem;
-	uintptr_t i;
-#endif
 
 	/* copy 1st page, if it's not present something is wrong */
 	if (!page_present(page_to_copy)) {
@@ -210,11 +218,7 @@ static void *get_func(void *mem)
 	else
 		memset(mem, 0, page_sz);
 
-#ifdef __powerpc__
-	for (i = 0; i < copy_sz; i += 4)
-		__asm__ __volatile__("dcbst 0,%0; sync; icbi 0,%0; sync; isync"
-			:: "r"(mem_start + i));
-#endif
+	clear_cache(mem_start, copy_sz);
 
 	/* return pointer to area where copy of exec_func resides */
 	return func_copy_start;
-- 
1.9.1

[LTP] [PATCH] syscalls/mprotect04: Use __builtin___clear_cache() to sync caches

[Jan Stancek]

----- Original Message -----
> From: "Peter Maydell" <peter.maydell@linaro.org>
> To: ltp@lists.linux.it
> Cc: patches@linaro.org, "Cyril Hrubis" <chrubis@suse.cz>, "Jan Stancek" <jstancek@redhat.com>, "Mark Rutland"
> <mark.rutland@arm.com>
> Sent: Tuesday, 26 July, 2016 6:31:05 PM
> Subject: [PATCH] syscalls/mprotect04: Use __builtin___clear_cache() to sync caches
> 
> In commit cf9a0800cd0 code was added to mprotect04 to synchronize
> the instruction and data caches on PowerPC before executing
> the copied code. This is also necessary on other architectures
> which have split instruction and data caches and need explicit
> cache maintenance operations, like ARM.
> 
> The GCC builtin __builtin___clear_cache() will correctly handle
> this for all architectures, so switch to using it. The builtin
> was only implemented at around GCC 4.1, so use a configure
> check so that we will skip the test with TCONF if the compiler
> doesn't support the builtin.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Looks good to me.

> ---
> Disclaimer: untested on PPC.

I don't have such HW available at the moment, but I tested this
on RHEL5.6/6.0/7.2 x86_64. RHEL5.6 exits with TCONF as expected.

Regards,
Jan

[LTP] [PATCH] syscalls/mprotect04: Use __builtin___clear_cache() to sync caches

[Peter Maydell]

On 1 August 2016 at 11:29, Jan Stancek <jstancek@redhat.com> wrote:
> ----- Original Message -----
>> From: "Peter Maydell" <peter.maydell@linaro.org>
>> Disclaimer: untested on PPC.
>
> I don't have such HW available at the moment, but I tested this
> on RHEL5.6/6.0/7.2 x86_64. RHEL5.6 exits with TCONF as expected.

Thanks. I gave the test case a quick spin on the gcc compile
farm's PPC64 box ("IBM POWER7 / 64 GB RAM / IBM Power 730 Express
server / Fedora 18"); the test passes OK, but on the other hand
it passes even if you hack out the cache ops entirely, so
either it's not the right PPC hardware to reveal the problem
or it only happens if you're thrashing the machine.

thanks
-- PMM

[LTP] [PATCH] syscalls/mprotect04: Use __builtin___clear_cache() to sync caches

[Cyril Hrubis]

Hi!
> +#include "config.h"
>  #include <signal.h>
>  #include <setjmp.h>
>  #include <sys/types.h>
> @@ -178,6 +179,16 @@ static int page_present(void *p)
>  	return 0;
>  }
>  
> +static void clear_cache(void *start, int len)
> +{
> +#if HAVE_BUILTIN_CLEAR_CACHE == 1
> +	__builtin___clear_cache(start, start + len);
> +#else
> +	tst_brkm(TCONF, cleanup,
> +		"compiler doesn't have __builtin___clear_cache()");
> +#endif
> +}
> +
>  /*
>   * Copy page where &exec_func resides. Also try to copy subsequent page
>   * in case exec_func is close to page boundary.
> @@ -189,10 +200,7 @@ static void *get_func(void *mem)
>  	uintptr_t func_page_offset = (uintptr_t)&exec_func & (page_sz - 1);
>  	void *func_copy_start = mem + func_page_offset;
>  	void *page_to_copy = (void *)((uintptr_t)&exec_func & page_mask);
> -#ifdef __powerpc__
>  	void *mem_start = mem;
> -	uintptr_t i;
> -#endif
>  
>  	/* copy 1st page, if it's not present something is wrong */
>  	if (!page_present(page_to_copy)) {
> @@ -210,11 +218,7 @@ static void *get_func(void *mem)
>  	else
>  		memset(mem, 0, page_sz);
>  
> -#ifdef __powerpc__
> -	for (i = 0; i < copy_sz; i += 4)
> -		__asm__ __volatile__("dcbst 0,%0; sync; icbi 0,%0; sync; isync"
> -			:: "r"(mem_start + i));
> -#endif
> +	clear_cache(mem_start, copy_sz);

As far as I can tell this is not needed on x86 machines so maybe we
should do something as:

...

#else
# if !defined(__x86_64__) && !defined(__i386__)
	tst_brkm(TCONF, cleanup,
		"compiler doesn't have __builtin___clear_cache()");
# endif
#endif


So that the test is not disabled on older x86 machines.


Otherwise it looks good to me.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] syscalls/mprotect04: Use __builtin___clear_cache() to sync caches

[Peter Maydell]

On 1 August 2016 at 12:31, Cyril Hrubis <chrubis@suse.cz> wrote:
> As far as I can tell this is not needed on x86 machines so maybe we
> should do something as:
>
> ...
>
> #else
> # if !defined(__x86_64__) && !defined(__i386__)
>         tst_brkm(TCONF, cleanup,
>                 "compiler doesn't have __builtin___clear_cache()");
> # endif
> #endif
>
>
> So that the test is not disabled on older x86 machines.

I considered that, but I felt it was not worth having
an architecture-specific ifdef in the code purely to support
running this test case with compilers that are a decade
old (and will only be getting rarer in the future) on one
specific architecture...

thanks
-- PMM

[LTP] [PATCH] syscalls/mprotect04: Use __builtin___clear_cache() to sync caches

[Cyril Hrubis]

Hi!
> I considered that, but I felt it was not worth having
> an architecture-specific ifdef in the code purely to support
> running this test case with compilers that are a decade
> old (and will only be getting rarer in the future) on one
> specific architecture...

Well I would be slightly inclined not to introduce false regressions
with newer LTP versions. But given that this affects only really old
compilers we may as well get with your version.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] syscalls/mprotect04: Use __builtin___clear_cache() to sync caches

[Peter Maydell]

On 1 August 2016 at 17:17, Cyril Hrubis <chrubis@suse.cz> wrote:
> Hi!
>> I considered that, but I felt it was not worth having
>> an architecture-specific ifdef in the code purely to support
>> running this test case with compilers that are a decade
>> old (and will only be getting rarer in the future) on one
>> specific architecture...
>
> Well I would be slightly inclined not to introduce false regressions
> with newer LTP versions. But given that this affects only really old
> compilers we may as well get with your version.

Hi -- are you planning to commit this version of the patch
or would you prefer me to spin a v2 with the i386 ifdeffery ?

thanks
-- PMM

[LTP] [PATCH] syscalls/mprotect04: Use __builtin___clear_cache() to sync caches

[Cyril Hrubis]

Hi!
> > Well I would be slightly inclined not to introduce false regressions
> > with newer LTP versions. But given that this affects only really old
> > compilers we may as well get with your version.
> 
> Hi -- are you planning to commit this version of the patch
> or would you prefer me to spin a v2 with the i386 ifdeffery ?

I've pushed the first version, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] syscalls/mprotect04: Use __builtin___clear_cache() to sync caches

[Peter Maydell]

On 9 August 2016 at 13:13, Cyril Hrubis <chrubis@suse.cz> wrote:
> Hi!
>> > Well I would be slightly inclined not to introduce false regressions
>> > with newer LTP versions. But given that this affects only really old
>> > compilers we may as well get with your version.
>>
>> Hi -- are you planning to commit this version of the patch
>> or would you prefer me to spin a v2 with the i386 ifdeffery ?
>
> I've pushed the first version, thanks.

That's great, thanks.

-- PMM