Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 22 Sep 2022 10:29:39 -0700
From:   David Matlack <dmatlack@google.com>
To:     Sean Christopherson <seanjc@google.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Andrew Jones <andrew.jones@linux.dev>,
        Anup Patel <anup@brainfault.org>,
        Atish Patra <atishp@atishpatra.org>,
        Christian Borntraeger <borntraeger@linux.ibm.com>,
        Janosch Frank <frankja@linux.ibm.com>,
        Claudio Imbrenda <imbrenda@linux.ibm.com>,
        Oliver Upton <oliver.upton@linux.dev>
Subject: Re: [PATCH 1/5] KVM: selftests: Implement memcmp(), memcpy(), and
 memset() for guest use
Message-ID: <Yyybg3DxgLt4NVn+@google.com>
References: <20220908233134.3523339-1-seanjc@google.com>
 <20220908233134.3523339-2-seanjc@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220908233134.3523339-2-seanjc@google.com>
Precedence: bulk

On Thu, Sep 08, 2022 at 11:31:30PM +0000, Sean Christopherson wrote:
> Implement memcmp(), memcpy(), and memset() to override the compiler's
> built-in versions in order to guarantee that the compiler won't generate
> out-of-line calls to external functions via the PLT.  This allows the
> helpers to be safely used in guest code, as KVM selftests don't support
> dynamic loading of guest code.
> 
> Steal the implementations from the kernel's generic versions, sans the
> optimizations in memcmp() for unaligned accesses.
> 
> Put the utilities in a separate compilation unit and build with
> -ffreestanding to fudge around a gcc "feature" where it will optimize
> memset(), memcpy(), etc... by generating a recursive call.  I.e. the
> compiler optimizes itself into infinite recursion.  Alternatively, the
> individual functions could be tagged with
> optimize("no-tree-loop-distribute-patterns"), but using "optimize" for
> anything but debug is discouraged, and Linus NAK'd the use of the flag
> in the kernel proper[*].
> 
> https://lore.kernel.org/lkml/CAHk-=wik-oXnUpfZ6Hw37uLykc-_P0Apyn2XuX-odh-3Nzop8w@mail.gmail.com
> 
> Cc: Andrew Jones <andrew.jones@linux.dev>
> Cc: Anup Patel <anup@brainfault.org>
> Cc: Atish Patra <atishp@atishpatra.org>
> Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
> Cc: Janosch Frank <frankja@linux.ibm.com>
> Cc: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  tools/testing/selftests/kvm/Makefile          |  8 ++++-
>  .../selftests/kvm/include/kvm_util_base.h     | 10 ++++++
>  tools/testing/selftests/kvm/lib/kvm_string.c  | 33 +++++++++++++++++++
>  3 files changed, 50 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/kvm/lib/kvm_string.c
> 
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 4c122f1b1737..92a0c05645b5 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -48,6 +48,8 @@ LIBKVM += lib/rbtree.c
>  LIBKVM += lib/sparsebit.c
>  LIBKVM += lib/test_util.c
>  
> +LIBKVM_STRING += lib/kvm_string.c

Can this file be named lib/string.c instead? This file has nothing to do
with KVM per-se.

> +
>  LIBKVM_x86_64 += lib/x86_64/apic.c
>  LIBKVM_x86_64 += lib/x86_64/handlers.S
>  LIBKVM_x86_64 += lib/x86_64/perf_test_util.c
> @@ -220,7 +222,8 @@ LIBKVM_C := $(filter %.c,$(LIBKVM))
>  LIBKVM_S := $(filter %.S,$(LIBKVM))
>  LIBKVM_C_OBJ := $(patsubst %.c, $(OUTPUT)/%.o, $(LIBKVM_C))
>  LIBKVM_S_OBJ := $(patsubst %.S, $(OUTPUT)/%.o, $(LIBKVM_S))
> -LIBKVM_OBJS = $(LIBKVM_C_OBJ) $(LIBKVM_S_OBJ)
> +LIBKVM_STRING_OBJ := $(patsubst %.c, $(OUTPUT)/%.o, $(LIBKVM_STRING))
> +LIBKVM_OBJS = $(LIBKVM_C_OBJ) $(LIBKVM_S_OBJ) $(LIBKVM_STRING_OBJ)
>  
>  EXTRA_CLEAN += $(LIBKVM_OBJS) cscope.*
>  
> @@ -231,6 +234,9 @@ $(LIBKVM_C_OBJ): $(OUTPUT)/%.o: %.c
>  $(LIBKVM_S_OBJ): $(OUTPUT)/%.o: %.S
>  	$(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c $< -o $@
>  
> +$(LIBKVM_STRING_OBJ): $(OUTPUT)/%.o: %.c
> +	$(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c -ffreestanding $< -o $@

A comment here would be helpful to document why LIBKVM_STRING_OBJ needs
a special case.

> +
>  x := $(shell mkdir -p $(sort $(dir $(TEST_GEN_PROGS))))
>  $(TEST_GEN_PROGS): $(LIBKVM_OBJS)
>  $(TEST_GEN_PROGS_EXTENDED): $(LIBKVM_OBJS)
> diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
> index 24fde97f6121..bdb751f4825c 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> @@ -173,6 +173,16 @@ struct vm_guest_mode_params {
>  };
>  extern const struct vm_guest_mode_params vm_guest_mode_params[];
>  
> +/*
> + * Override the "basic" built-in string helpers so that they can be used in
> + * guest code.  KVM selftests don't support dynamic loading in guest code and
> + * will jump into the weeds if the compiler decides to insert an out-of-line
> + * call via the PLT.
> + */
> +int memcmp(const void *cs, const void *ct, size_t count);
> +void *memcpy(void *dest, const void *src, size_t count);
> +void *memset(void *s, int c, size_t count);
> +
>  int open_path_or_exit(const char *path, int flags);
>  int open_kvm_dev_path_or_exit(void);
>  unsigned int kvm_check_cap(long cap);
> diff --git a/tools/testing/selftests/kvm/lib/kvm_string.c b/tools/testing/selftests/kvm/lib/kvm_string.c
> new file mode 100644
> index 000000000000..a60d56d4e5b8
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/lib/kvm_string.c
> @@ -0,0 +1,33 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include "kvm_util.h"

Is this include necesary?

> +
> +int memcmp(const void *cs, const void *ct, size_t count)
> +{
> +	const unsigned char *su1, *su2;
> +	int res = 0;
> +
> +	for (su1 = cs, su2 = ct; 0 < count; ++su1, ++su2, count--) {
> +		if ((res = *su1 - *su2) != 0)
> +			break;
> +	}
> +	return res;
> +}
> +
> +void *memcpy(void *dest, const void *src, size_t count)
> +{
> +	char *tmp = dest;
> +	const char *s = src;
> +
> +	while (count--)
> +		*tmp++ = *s++;
> +	return dest;
> +}
> +
> +void *memset(void *s, int c, size_t count)
> +{
> +	char *xs = s;
> +
> +	while (count--)
> +		*xs++ = c;
> +	return s;
> +}
> -- 
> 2.37.2.789.g6183377224-goog
>