syscalls/mprotect04: Use builtin_clear_cache() to sync caches

PATCH New Peter Maydell 3 participants 9 messages Jul 26, 2016 – Aug 9, 2016

Share & tools

Peter Maydell

author

2016-07-26 16:31 UTC #0 lore raw

PATCH 1/1 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 <[email protected]>

---
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

configure.ac +1 -0

index e0e9c1b..9901612 100644
@@ -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

m4/ltp-builtin_clear_cache.m4 new file +30 -0

new file mode 100644
index 0000000..74e4ccd
@@ -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
+])

testcases/kernel/syscalls/mprotect/mprotect04.c +12 -8

index c94e25c..1173afd 100644
@@ -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;

Signature · 1 line

1.9.1

Jan Stancek

2016-08-01 10:29 UTC #1 lore raw

PATCH 1/1 syscalls/mprotect04: Use __builtin___clear_cache() to sync caches

----- Original Message -----

Quoted 13 lines

From: "Peter Maydell" <[email protected]>
To: [email protected]
Cc: [email protected], "Cyril Hrubis" <[email protected]>, "Jan Stancek" <[email protected]>, "Mark Rutland" <[email protected]>
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.

Quoted 7 lines

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 <[email protected]>

Looks good to me.

Quoted 2 lines

---
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

Peter Maydell

author

2016-08-01 11:06 UTC #2 lore raw

PATCH 1/1 syscalls/mprotect04: Use __builtin___clear_cache() to sync caches

Quoted 6 lines · Jan Stancek

----- Original Message -----
Quoted 2 lines
From: "Peter Maydell" <[email protected]>
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

Cyril Hrubis

2016-08-01 11:31 UTC #3 lore raw

PATCH 1/1 syscalls/mprotect04: Use __builtin___clear_cache() to sync caches

In reply to Peter Maydell

Hi!

Quoted 33 lines

testcases/kernel/syscalls/mprotect/mprotect04.c +11 -3

+#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)) {

testcases/kernel/syscalls/mprotect/mprotect04.c static void *get_func(void *mem) · line 221

Quoted 9 lines

testcases/kernel/syscalls/mprotect/mprotect04.c +1 -5

@@ -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.

Signature · 2 lines

Cyril Hrubis
[email protected]

Peter Maydell

author

2016-08-01 12:23 UTC #4 lore raw

PATCH 1/1 syscalls/mprotect04: Use __builtin___clear_cache() to sync caches

Quoted 6 lines · Cyril Hrubis

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

Quoted 8 lines

# 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

Cyril Hrubis

2016-08-01 16:17 UTC #5 lore raw

PATCH 1/1 syscalls/mprotect04: Use __builtin___clear_cache() to sync caches

Hi!

Quoted 5 lines

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.

Signature · 2 lines

Cyril Hrubis
[email protected]

Peter Maydell

author

2016-08-05 10:08 UTC #6 lore raw

PATCH 1/1 syscalls/mprotect04: Use __builtin___clear_cache() to sync caches

Quoted 10 lines · Cyril Hrubis

Hi!
Quoted 5 lines
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

Cyril Hrubis

2016-08-09 12:13 UTC #7 lore raw

PATCH 1/1 syscalls/mprotect04: Use __builtin___clear_cache() to sync caches

Hi!

Quoted 6 lines

Quoted 3 lines
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.

Signature · 2 lines

Cyril Hrubis
[email protected]

Peter Maydell

author

2016-08-09 12:25 UTC #8 lore raw

PATCH 1/1 syscalls/mprotect04: Use __builtin___clear_cache() to sync caches

Quoted 9 lines · Cyril Hrubis

Hi!
Quoted 6 lines
Quoted 3 lines
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