Re: [PATCH] x86_64: new and improved memset()

From: Borislav Petkov
Date: Sat Sep 14 2019 - 07:37:59 EST


On Sat, Sep 14, 2019 at 01:33:45PM +0300, Alexey Dobriyan wrote:
> --- a/arch/x86/include/asm/string_64.h
> +++ b/arch/x86/include/asm/string_64.h
> @@ -15,7 +15,111 @@ extern void *memcpy(void *to, const void *from, size_t len);
> extern void *__memcpy(void *to, const void *from, size_t len);
>
> #define __HAVE_ARCH_MEMSET
> +#if defined(_ARCH_X86_BOOT) || defined(CONFIG_FORTIFY_SOURCE)
> void *memset(void *s, int c, size_t n);
> +#else
> +#include <asm/alternative.h>
> +#include <asm/cpufeatures.h>
> +
> +/* Internal, do not use. */
> +static __always_inline void memset0(void *s, size_t n)
> +{
> + /* Internal, do not use. */
> + void _memset0_mov(void);
> + void _memset0_rep_stosq(void);
> + void memset0_mov(void);
> + void memset0_rep_stosq(void);
> + void memset0_rep_stosb(void);
> +
> + if (__builtin_constant_p(n) && n == 0) {
> + } else if (__builtin_constant_p(n) && n == 1) {
> + *(uint8_t *)s = 0;
> + } else if (__builtin_constant_p(n) && n == 2) {
> + *(uint16_t *)s = 0;
> + } else if (__builtin_constant_p(n) && n == 4) {
> + *(uint32_t *)s = 0;
> + } else if (__builtin_constant_p(n) && n == 6) {
> + *(uint32_t *)s = 0;
> + *(uint16_t *)(s + 4) = 0;
> + } else if (__builtin_constant_p(n) && n == 8) {
> + *(uint64_t *)s = 0;
> + } else if (__builtin_constant_p(n) && (n & 7) == 0) {
> + alternative_call_2(
> + _memset0_mov,
> + _memset0_rep_stosq, X86_FEATURE_REP_GOOD,
> + memset0_rep_stosb, X86_FEATURE_ERMS,
> + ASM_OUTPUT2("=D" (s), "=c" (n)),
> + "D" (s), "c" (n)
> + : "rax", "cc", "memory"
> + );
> + } else {
> + alternative_call_2(
> + memset0_mov,
> + memset0_rep_stosq, X86_FEATURE_REP_GOOD,
> + memset0_rep_stosb, X86_FEATURE_ERMS,
> + ASM_OUTPUT2("=D" (s), "=c" (n)),
> + "D" (s), "c" (n)
> + : "rax", "rsi", "cc", "memory"
> + );
> + }
> +}
> +
> +/* Internal, do not use. */
> +static __always_inline void memsetx(void *s, int c, size_t n)
> +{
> + /* Internal, do not use. */
> + void _memsetx_mov(void);
> + void _memsetx_rep_stosq(void);
> + void memsetx_mov(void);
> + void memsetx_rep_stosq(void);
> + void memsetx_rep_stosb(void);
> +
> + const uint64_t ccc = (uint8_t)c * 0x0101010101010101ULL;
> +
> + if (__builtin_constant_p(n) && n == 0) {
> + } else if (__builtin_constant_p(n) && n == 1) {
> + *(uint8_t *)s = ccc;
> + } else if (__builtin_constant_p(n) && n == 2) {
> + *(uint16_t *)s = ccc;
> + } else if (__builtin_constant_p(n) && n == 4) {
> + *(uint32_t *)s = ccc;
> + } else if (__builtin_constant_p(n) && n == 8) {
> + *(uint64_t *)s = ccc;
> + } else if (__builtin_constant_p(n) && (n & 7) == 0) {
> + alternative_call_2(
> + _memsetx_mov,
> + _memsetx_rep_stosq, X86_FEATURE_REP_GOOD,
> + memsetx_rep_stosb, X86_FEATURE_ERMS,
> + ASM_OUTPUT2("=D" (s), "=c" (n)),
> + "D" (s), "c" (n), "a" (ccc)
> + : "cc", "memory"
> + );
> + } else {
> + alternative_call_2(
> + memsetx_mov,
> + memsetx_rep_stosq, X86_FEATURE_REP_GOOD,
> + memsetx_rep_stosb, X86_FEATURE_ERMS,
> + ASM_OUTPUT2("=D" (s), "=c" (n)),
> + "D" (s), "c" (n), "a" (ccc)
> + : "rsi", "cc", "memory"
> + );
> + }
> +}
> +
> +static __always_inline void *memset(void *s, int c, size_t n)
> +{
> + if (__builtin_constant_p(c)) {
> + if (c == 0) {
> + memset0(s, n);
> + } else {
> + memsetx(s, c, n);
> + }
> + return s;
> + } else {
> + return __builtin_memset(s, c, n);
> + }
> +}

I'm willing to take something like that only when such complexity is
justified by numbers. I.e., I'm much more inclined to capping it under
32 and 64 byte sizes and keeping it simple.

...

> +ENTRY(_memset0_mov)
> + xor eax, eax
> +.globl _memsetx_mov
> +_memsetx_mov:
> + add rcx, rdi
> + cmp rdi, rcx
> + je 1f
> +2:
> + mov [rdi], rax
> + add rdi, 8
> + cmp rdi, rcx
> + jne 2b
> +1:
> + ret
> +ENDPROC(_memset0_mov)
> +ENDPROC(_memsetx_mov)
> +EXPORT_SYMBOL(_memset0_mov)
> +EXPORT_SYMBOL(_memsetx_mov)
> +
> +ENTRY(memset0_mov)
> + xor eax, eax
> +.globl memsetx_mov
> +memsetx_mov:
> + lea rsi, [rdi + rcx]
> + cmp rdi, rsi
> + je 1f
> +2:
> + mov [rdi], al
> + add rdi, 1
> + cmp rdi, rsi
> + jne 2b
> +1:
> + ret

Say what now? Intel syntax? You must be joking...

> +ENDPROC(memset0_mov)
> +ENDPROC(memsetx_mov)
> +EXPORT_SYMBOL(memset0_mov)
> +EXPORT_SYMBOL(memsetx_mov)

Too many exported symbols. Again, I'll much more prefer a cleaner,
smaller solution than one where readability suffers greatly at the
expense of *maybe* getting a bit better performance.

> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -28,7 +28,7 @@ KBUILD_CFLAGS := $(cflags-y) -DDISABLE_BRANCH_PROFILING \
> -D__NO_FORTIFY \
> $(call cc-option,-ffreestanding) \
> $(call cc-option,-fno-stack-protector) \
> - -D__DISABLE_EXPORTS
> + -D__DISABLE_EXPORTS -D_ARCH_X86_BOOT

Yeah, something like that is inevitable, I've come to realize too. ;-\

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette