On Tue, Aug 04, 2020 at 07:48:17PM -0400, Arvind Sankar wrote:
> Since commits
>
> c041b5ad8640 ("x86, boot: Create a separate string.h file to provide
> standard string functions")
> fb4cac573ef6 ("x86, boot: Move memcmp() into string.h and string.c")
>
> the decompressor stub has been using the compiler's builtin memcpy,
> memset and memcmp functions, _except_ where it would likely have the
> largest impact, in the decompression code itself.
>
> Remove the #undef's of memcpy and memset in misc.c so that the
> decompressor code also uses the compiler builtins.
>
> The rationale given in the comment doesn't really apply: just because
> some functions use the out-of-line version is no reason to not use the
> builtin version in the rest.
>
> Replace the comment with an explanation of why memzero and memmove are
> being #define'd.
>
> Drop the suggestion to #undef in boot/string.h as well: the out-of-line
> versions are not really optimized versions, they're generic code that's
> good enough for the preboot environment. The compiler will likely
> generate better code for constant-size memcpy/memset/memcmp if it is
> allowed to.
>
> Most decompressors' performance is unchanged, with the exception of LZ4
> and 64-bit ZSTD.
>
> Before After ARCH
> LZ4 73ms 10ms 32
> LZ4 120ms 10ms 64
> ZSTD 90ms 74ms 64
>
> Measurements on QEMU on 2.2GHz Broadwell Xeon, using defconfig kernels.
>
> Decompressor code size has small differences, with the largest being
> that 64-bit ZSTD decreases just over 2k. The largest code size increase
> was on 64-bit XZ, of about 400 bytes.
>
> Signed-off-by: Arvind Sankar <[email protected]>
> Suggested-by: Nick Terrell <[email protected]>
> Tested-by: Nick Terrell <[email protected]>
Did anyone pick this up? (Ingo can you snag it, or maybe akpm who took
the LZ4-specific patch already?) This looks sane to me and provides some
surprising performance benefits. :)
Acked-by: Kees Cook <[email protected]>
-Kees
> ---
> arch/x86/boot/compressed/misc.c | 7 ++-----
> arch/x86/boot/string.h | 5 +----
> 2 files changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> index 39e592d0e0b4..e478e40fbe5a 100644
> --- a/arch/x86/boot/compressed/misc.c
> +++ b/arch/x86/boot/compressed/misc.c
> @@ -30,12 +30,9 @@
> #define STATIC static
>
> /*
> - * Use normal definitions of mem*() from string.c. There are already
> - * included header files which expect a definition of memset() and by
> - * the time we define memset macro, it is too late.
> + * Provide definitions of memzero and memmove as some of the decompressors
> will
> + * try to define their own functions if these are not defined as macros.
> */
> -#undef memcpy
> -#undef memset
> #define memzero(s, n) memset((s), 0, (n))
> #define memmove memmove
>
> diff --git a/arch/x86/boot/string.h b/arch/x86/boot/string.h
> index 995f7b7ad512..a232da487cd2 100644
> --- a/arch/x86/boot/string.h
> +++ b/arch/x86/boot/string.h
> @@ -11,10 +11,7 @@ void *memcpy(void *dst, const void *src, size_t len);
> void *memset(void *dst, int c, size_t len);
> int memcmp(const void *s1, const void *s2, size_t len);
>
> -/*
> - * Access builtin version by default. If one needs to use optimized version,
> - * do "undef memcpy" in .c file and link against right string.c
> - */
> +/* Access builtin version by default. */
> #define memcpy(d,s,l) __builtin_memcpy(d,s,l)
> #define memset(d,c,l) __builtin_memset(d,c,l)
> #define memcmp __builtin_memcmp
> --
> 2.26.2
>
--
Kees Cook
