lib: fwts_safe_mem: really force reads

Message ID 20170812104710.13543-1-colin.king@canonical.com
State Accepted
Headers show

Commit Message

Colin Ian King Aug. 12, 2017, 10:47 a.m.
From: Colin Ian King <colin.king@canonical.com>

Being totally paranoid here, but I'd like to really ensure
data is always being read by forcing a prefetch on each data item
being read and force the compiler to never optimize this out by
stashing the read data into a circular buffer.  Smarter compilers
on some arches were optimizing out the original reads; I think
this change will really force reads.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/lib/src/fwts_safe_mem.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Comments

Alex Hung Aug. 14, 2017, 6:57 p.m. | #1
On 2017-08-12 03:47 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Being totally paranoid here, but I'd like to really ensure
> data is always being read by forcing a prefetch on each data item
> being read and force the compiler to never optimize this out by
> stashing the read data into a circular buffer.  Smarter compilers
> on some arches were optimizing out the original reads; I think
> this change will really force reads.
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/lib/src/fwts_safe_mem.c | 15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/src/lib/src/fwts_safe_mem.c b/src/lib/src/fwts_safe_mem.c
> index 1feb1ff2..f3f639b1 100644
> --- a/src/lib/src/fwts_safe_mem.c
> +++ b/src/lib/src/fwts_safe_mem.c
> @@ -68,8 +68,10 @@ int OPTIMIZE0 fwts_safe_memcpy(void *dst, const void *src, const size_t n)
>    */
>   int OPTIMIZE0 fwts_safe_memread(const void *src, const size_t n)
>   {
> -	const volatile uint8_t *ptr = src;
> -	const volatile uint8_t *end = ptr + n;
> +	static uint8_t buffer[256];
> +	const uint8_t *ptr, *end = src + n;
> +	uint8_t *bufptr;
> +	const uint8_t *bufend = buffer + sizeof(buffer);
>   
>   	if (sigsetjmp(jmpbuf, 1) != 0)
>   		return FWTS_ERROR;
> @@ -77,8 +79,13 @@ int OPTIMIZE0 fwts_safe_memread(const void *src, const size_t n)
>   	fwts_sig_handler_set(SIGSEGV, sig_handler, &old_segv_action);
>   	fwts_sig_handler_set(SIGBUS, sig_handler, &old_bus_action);
>   
> -	while (ptr < end)
> -		(void)*(ptr++);
> +	for (bufptr = buffer, ptr = src; ptr < end; ptr++) {
> +		/* Force data to be read */
> +		__builtin_prefetch(ptr, 0, 3);
> +		*bufptr = *ptr;
> +		bufptr++;
> +		bufptr = (bufptr >= bufend) ? buffer : bufptr;
> +	}
>   
>   	fwts_sig_handler_restore(SIGSEGV, &old_segv_action);
>   	fwts_sig_handler_restore(SIGBUS, &old_bus_action);
> 


Acked-by: Alex Hung <alex.hung@canonical.com>
Naresh Bhat Aug. 16, 2017, 7:21 a.m. | #2
On 12 August 2017 at 16:17, Colin King <colin.king@canonical.com> wrote:

> From: Colin Ian King <colin.king@canonical.com>
>
> Being totally paranoid here, but I'd like to really ensure
> data is always being read by forcing a prefetch on each data item
> being read and force the compiler to never optimize this out by
> stashing the read data into a circular buffer.  Smarter compilers
> on some arches were optimizing out the original reads; I think
> this change will really force reads.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  src/lib/src/fwts_safe_mem.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/src/lib/src/fwts_safe_mem.c b/src/lib/src/fwts_safe_mem.c
> index 1feb1ff2..f3f639b1 100644
> --- a/src/lib/src/fwts_safe_mem.c
> +++ b/src/lib/src/fwts_safe_mem.c
> @@ -68,8 +68,10 @@ int OPTIMIZE0 fwts_safe_memcpy(void *dst, const void
> *src, const size_t n)
>   */
>  int OPTIMIZE0 fwts_safe_memread(const void *src, const size_t n)
>  {
> -       const volatile uint8_t *ptr = src;
> -       const volatile uint8_t *end = ptr + n;
> +       static uint8_t buffer[256];
> +       const uint8_t *ptr, *end = src + n;
> +       uint8_t *bufptr;
> +       const uint8_t *bufend = buffer + sizeof(buffer);
>
>         if (sigsetjmp(jmpbuf, 1) != 0)
>                 return FWTS_ERROR;
> @@ -77,8 +79,13 @@ int OPTIMIZE0 fwts_safe_memread(const void *src, const
> size_t n)
>         fwts_sig_handler_set(SIGSEGV, sig_handler, &old_segv_action);
>         fwts_sig_handler_set(SIGBUS, sig_handler, &old_bus_action);
>
> -       while (ptr < end)
> -               (void)*(ptr++);
> +       for (bufptr = buffer, ptr = src; ptr < end; ptr++) {
> +               /* Force data to be read */
> +               __builtin_prefetch(ptr, 0, 3);
> +               *bufptr = *ptr;
> +               bufptr++;
> +               bufptr = (bufptr >= bufend) ? buffer : bufptr;
> +       }
>
>         fwts_sig_handler_restore(SIGSEGV, &old_segv_action);
>         fwts_sig_handler_restore(SIGBUS, &old_bus_action);
> --
> 2.11.0
>

Thanks for the patch.  Just back from long week-end.

Acked-By/Tested-By: Naresh Bhat <naresh.bhat@linaro.org>
ivanhu Aug. 21, 2017, 8:17 a.m. | #3
On 08/12/2017 06:47 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Being totally paranoid here, but I'd like to really ensure
> data is always being read by forcing a prefetch on each data item
> being read and force the compiler to never optimize this out by
> stashing the read data into a circular buffer.  Smarter compilers
> on some arches were optimizing out the original reads; I think
> this change will really force reads.
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/lib/src/fwts_safe_mem.c | 15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/src/lib/src/fwts_safe_mem.c b/src/lib/src/fwts_safe_mem.c
> index 1feb1ff2..f3f639b1 100644
> --- a/src/lib/src/fwts_safe_mem.c
> +++ b/src/lib/src/fwts_safe_mem.c
> @@ -68,8 +68,10 @@ int OPTIMIZE0 fwts_safe_memcpy(void *dst, const void *src, const size_t n)
>    */
>   int OPTIMIZE0 fwts_safe_memread(const void *src, const size_t n)
>   {
> -	const volatile uint8_t *ptr = src;
> -	const volatile uint8_t *end = ptr + n;
> +	static uint8_t buffer[256];
> +	const uint8_t *ptr, *end = src + n;
> +	uint8_t *bufptr;
> +	const uint8_t *bufend = buffer + sizeof(buffer);
>   
>   	if (sigsetjmp(jmpbuf, 1) != 0)
>   		return FWTS_ERROR;
> @@ -77,8 +79,13 @@ int OPTIMIZE0 fwts_safe_memread(const void *src, const size_t n)
>   	fwts_sig_handler_set(SIGSEGV, sig_handler, &old_segv_action);
>   	fwts_sig_handler_set(SIGBUS, sig_handler, &old_bus_action);
>   
> -	while (ptr < end)
> -		(void)*(ptr++);
> +	for (bufptr = buffer, ptr = src; ptr < end; ptr++) {
> +		/* Force data to be read */
> +		__builtin_prefetch(ptr, 0, 3);
> +		*bufptr = *ptr;
> +		bufptr++;
> +		bufptr = (bufptr >= bufend) ? buffer : bufptr;
> +	}
>   
>   	fwts_sig_handler_restore(SIGSEGV, &old_segv_action);
>   	fwts_sig_handler_restore(SIGBUS, &old_bus_action);
> 


Acked-by: Ivan Hu <ivan.hu@canonical.com>

Patch

diff --git a/src/lib/src/fwts_safe_mem.c b/src/lib/src/fwts_safe_mem.c
index 1feb1ff2..f3f639b1 100644
--- a/src/lib/src/fwts_safe_mem.c
+++ b/src/lib/src/fwts_safe_mem.c
@@ -68,8 +68,10 @@  int OPTIMIZE0 fwts_safe_memcpy(void *dst, const void *src, const size_t n)
  */
 int OPTIMIZE0 fwts_safe_memread(const void *src, const size_t n)
 {
-	const volatile uint8_t *ptr = src;
-	const volatile uint8_t *end = ptr + n;
+	static uint8_t buffer[256];
+	const uint8_t *ptr, *end = src + n;
+	uint8_t *bufptr;
+	const uint8_t *bufend = buffer + sizeof(buffer);
 
 	if (sigsetjmp(jmpbuf, 1) != 0)
 		return FWTS_ERROR;
@@ -77,8 +79,13 @@  int OPTIMIZE0 fwts_safe_memread(const void *src, const size_t n)
 	fwts_sig_handler_set(SIGSEGV, sig_handler, &old_segv_action);
 	fwts_sig_handler_set(SIGBUS, sig_handler, &old_bus_action);
 
-	while (ptr < end)
-		(void)*(ptr++);
+	for (bufptr = buffer, ptr = src; ptr < end; ptr++) {
+		/* Force data to be read */
+		__builtin_prefetch(ptr, 0, 3);
+		*bufptr = *ptr;
+		bufptr++;
+		bufptr = (bufptr >= bufend) ? buffer : bufptr;
+	}
 
 	fwts_sig_handler_restore(SIGSEGV, &old_segv_action);
 	fwts_sig_handler_restore(SIGBUS, &old_bus_action);