[PATCH 0/5] KVM: selftests: Fix "fix hypercall test" build errors

[PATCH 0/5] KVM: selftests: Fix "fix hypercall test" build errors

[Sean Christopherson]

After a toolchain upgrade (I think), the x86 fix_hypercall_test started
throwing warnings due to -Werror=array-bounds rightly complaining that
the test is generating an out-of-bounds array access.

The "obvious" fix is to replace the memcpy() with a memcmp() and compare
only the exact size of the hypercall instruction.  That worked, until I
fiddled with the code a bit more and suddenly the test started jumping into
the weeds due to gcc generating a call to the external memcmp() through the
PLT, which isn't supported in the selftests.

To fix that mess, which has been a pitfall for quite some time, provide
implementations of memcmp(), memcpy(), and memset() to effectively override
the compiler built-ins.  My thought is to start with the helpers that are
most likely to be used in guest code, and then add more as needed.

Tested on x86 and ARM, compile tested on RISC-V and s390.  Full testing on
RISC-V and s390 would be welcome, the seemingly benign addition of memxxx()
helpers managed to break ARM due to gcc generating an infinite loop for
memset() (see patch 1 for details).

Sean Christopherson (5):
  KVM: selftests: Implement memcmp(), memcpy(), and memset() for guest
    use
  KVM: selftests: Compare insn opcodes directly in fix_hypercall_test
  KVM: selftests: Remove unnecessary register shuffling in
    fix_hypercall_test
  KVM: selftests: Explicitly verify KVM doesn't patch hypercall if
    quirk==off
  KVM: selftests: Dedup subtests of fix_hypercall_test

 tools/testing/selftests/kvm/Makefile          |   8 +-
 .../selftests/kvm/include/kvm_util_base.h     |  10 ++
 tools/testing/selftests/kvm/lib/kvm_string.c  |  33 +++++
 .../selftests/kvm/x86_64/fix_hypercall_test.c | 124 ++++++++----------
 4 files changed, 107 insertions(+), 68 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/lib/kvm_string.c


base-commit: 29250ba51bc1cbe8a87e923f76978b87c3247a8c
-- 
2.37.2.789.g6183377224-goog

[PATCH 1/5] KVM: selftests: Implement memcmp(), memcpy(), and memset() for guest use

[Sean Christopherson]

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
+
 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 $@
+
 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"
+
+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

Re: [PATCH 1/5] KVM: selftests: Implement memcmp(), memcpy(), and memset() for guest use

[David Matlack]

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
> 

Re: [PATCH 1/5] KVM: selftests: Implement memcmp(), memcpy(), and memset() for guest use

[Sean Christopherson]

On Thu, Sep 22, 2022, David Matlack wrote:
> > +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.

Yes and no.  I deliberately chose kvm_string to avoid confusion with
tools/lib/string.c and tools/include/nolibc/string.h.  The implementations
themselves aren't KVM specific, but the reason the file _exists_ is 100% unique
to KVM as there is no other environment where tools and/or selftests link to
glibc but need to override the string ops.

I'm not completely opposed to calling it string.c, but my preference is to keep
it kvm_string.c so that it's slightly more obvious that KVM selftests are a
special snowflake.

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

Nope, I added the include because I also added declarations in kvm_util_base.h,
but that's unnecessary because stddef.h also provides the declarations, and those
_must_ match the prototypes of the definitions.  So yeah, this is better written as:

// SPDX-License-Identifier: GPL-2.0-only
#include <stddef.h>

/*
 * 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)
{
	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;
}

Re: [PATCH 1/5] KVM: selftests: Implement memcmp(), memcpy(), and memset() for guest use

[David Matlack]

On Thu, Sep 22, 2022 at 10:40 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Thu, Sep 22, 2022, David Matlack wrote:
> > > +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.
>
> Yes and no.  I deliberately chose kvm_string to avoid confusion with
> tools/lib/string.c and tools/include/nolibc/string.h.  The implementations
> themselves aren't KVM specific, but the reason the file _exists_ is 100% unique
> to KVM as there is no other environment where tools and/or selftests link to
> glibc but need to override the string ops.
>
> I'm not completely opposed to calling it string.c, but my preference is to keep
> it kvm_string.c so that it's slightly more obvious that KVM selftests are a
> special snowflake.

Makes sense, the "kvm" prefix just made me do a double-take. Perhaps
string_overrides.c? That would make it clear that this file is
overriding string functions. And the fact that this file is in the KVM
selftests directory signals that the overrides are specific to the KVM
selftests.


>
> > > 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?
>
> Nope, I added the include because I also added declarations in kvm_util_base.h,
> but that's unnecessary because stddef.h also provides the declarations, and those
> _must_ match the prototypes of the definitions.  So yeah, this is better written as:
>
> // SPDX-License-Identifier: GPL-2.0-only
> #include <stddef.h>
>
> /*
>  * 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)
> {
>         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;
> }

Re: [PATCH 1/5] KVM: selftests: Implement memcmp(), memcpy(), and memset() for guest use

[Sean Christopherson]

On Thu, Sep 22, 2022, David Matlack wrote:
> On Thu, Sep 22, 2022 at 10:40 AM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Thu, Sep 22, 2022, David Matlack wrote:
> > > > +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.
> >
> > Yes and no.  I deliberately chose kvm_string to avoid confusion with
> > tools/lib/string.c and tools/include/nolibc/string.h.  The implementations
> > themselves aren't KVM specific, but the reason the file _exists_ is 100% unique
> > to KVM as there is no other environment where tools and/or selftests link to
> > glibc but need to override the string ops.
> >
> > I'm not completely opposed to calling it string.c, but my preference is to keep
> > it kvm_string.c so that it's slightly more obvious that KVM selftests are a
> > special snowflake.
> 
> Makes sense, the "kvm" prefix just made me do a double-take. Perhaps
> string_overrides.c? That would make it clear that this file is
> overriding string functions. And the fact that this file is in the KVM
> selftests directory signals that the overrides are specific to the KVM
> selftests.

I like that, string_overrides.c it is.

[PATCH 2/5] KVM: selftests: Compare insn opcodes directly in fix_hypercall_test

[Sean Christopherson]

Directly compare the expected versus observed hypercall instructions when
verifying that KVM patched in the native hypercall (FIX_HYPERCALL_INSN
quirk enabled).  gcc rightly complains that doing a 4-byte memcpy() with
an "unsigned char" as the source generates an out-of-bounds accesses.

Alternatively, "exp" and "obs" could be declared as 3-byte arrays, but
there's no known reason to copy locally instead of comparing directly.

In function ‘assert_hypercall_insn’,
    inlined from ‘guest_main’ at x86_64/fix_hypercall_test.c:91:2:
x86_64/fix_hypercall_test.c:63:9: error: array subscript ‘unsigned int[0]’
 is partly outside array bounds of ‘unsigned char[1]’ [-Werror=array-bounds]
   63 |         memcpy(&exp, exp_insn, sizeof(exp));
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
x86_64/fix_hypercall_test.c: In function ‘guest_main’:
x86_64/fix_hypercall_test.c:42:22: note: object ‘vmx_hypercall_insn’ of size 1
   42 | extern unsigned char vmx_hypercall_insn;
      |                      ^~~~~~~~~~~~~~~~~~
x86_64/fix_hypercall_test.c:25:22: note: object ‘svm_hypercall_insn’ of size 1
   25 | extern unsigned char svm_hypercall_insn;
      |                      ^~~~~~~~~~~~~~~~~~
In function ‘assert_hypercall_insn’,
    inlined from ‘guest_main’ at x86_64/fix_hypercall_test.c:91:2:
x86_64/fix_hypercall_test.c:64:9: error: array subscript ‘unsigned int[0]’
 is partly outside array bounds of ‘unsigned char[1]’ [-Werror=array-bounds]
   64 |         memcpy(&obs, obs_insn, sizeof(obs));
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
x86_64/fix_hypercall_test.c: In function ‘guest_main’:
x86_64/fix_hypercall_test.c:25:22: note: object ‘svm_hypercall_insn’ of size 1
   25 | extern unsigned char svm_hypercall_insn;
      |                      ^~~~~~~~~~~~~~~~~~
x86_64/fix_hypercall_test.c:42:22: note: object ‘vmx_hypercall_insn’ of size 1
   42 | extern unsigned char vmx_hypercall_insn;
      |                      ^~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make: *** [../lib.mk:135: tools/testing/selftests/kvm/x86_64/fix_hypercall_test] Error 1

Fixes: 6c2fa8b20d0c ("selftests: KVM: Test KVM_X86_QUIRK_FIX_HYPERCALL_INSN")
Cc: Oliver Upton <oliver.upton@linux.dev>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 .../selftests/kvm/x86_64/fix_hypercall_test.c | 32 +++++++++----------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c b/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c
index b1905d280ef5..2512df357ab3 100644
--- a/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c
+++ b/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c
@@ -14,6 +14,9 @@
 #include "kvm_util.h"
 #include "processor.h"
 
+/* VMCALL and VMMCALL are both 3-byte opcodes. */
+#define HYPERCALL_INSN_SIZE	3
+
 static bool ud_expected;
 
 static void guest_ud_handler(struct ex_regs *regs)
@@ -22,7 +25,7 @@ static void guest_ud_handler(struct ex_regs *regs)
 	GUEST_DONE();
 }
 
-extern unsigned char svm_hypercall_insn;
+extern unsigned char svm_hypercall_insn[HYPERCALL_INSN_SIZE];
 static uint64_t svm_do_sched_yield(uint8_t apic_id)
 {
 	uint64_t ret;
@@ -39,7 +42,7 @@ static uint64_t svm_do_sched_yield(uint8_t apic_id)
 	return ret;
 }
 
-extern unsigned char vmx_hypercall_insn;
+extern unsigned char vmx_hypercall_insn[HYPERCALL_INSN_SIZE];
 static uint64_t vmx_do_sched_yield(uint8_t apic_id)
 {
 	uint64_t ret;
@@ -56,16 +59,6 @@ static uint64_t vmx_do_sched_yield(uint8_t apic_id)
 	return ret;
 }
 
-static void assert_hypercall_insn(unsigned char *exp_insn, unsigned char *obs_insn)
-{
-	uint32_t exp = 0, obs = 0;
-
-	memcpy(&exp, exp_insn, sizeof(exp));
-	memcpy(&obs, obs_insn, sizeof(obs));
-
-	GUEST_ASSERT_EQ(exp, obs);
-}
-
 static void guest_main(void)
 {
 	unsigned char *native_hypercall_insn, *hypercall_insn;
@@ -74,12 +67,12 @@ static void guest_main(void)
 	apic_id = GET_APIC_ID_FIELD(xapic_read_reg(APIC_ID));
 
 	if (is_intel_cpu()) {
-		native_hypercall_insn = &vmx_hypercall_insn;
-		hypercall_insn = &svm_hypercall_insn;
+		native_hypercall_insn = vmx_hypercall_insn;
+		hypercall_insn = svm_hypercall_insn;
 		svm_do_sched_yield(apic_id);
 	} else if (is_amd_cpu()) {
-		native_hypercall_insn = &svm_hypercall_insn;
-		hypercall_insn = &vmx_hypercall_insn;
+		native_hypercall_insn = svm_hypercall_insn;
+		hypercall_insn = vmx_hypercall_insn;
 		vmx_do_sched_yield(apic_id);
 	} else {
 		GUEST_ASSERT(0);
@@ -87,8 +80,13 @@ static void guest_main(void)
 		return;
 	}
 
+	/*
+	 * The hypercall didn't #UD (guest_ud_handler() signals "done" if a #UD
+	 * occurs).  Verify that a #UD is NOT expected and that KVM patched in
+	 * the native hypercall.
+	 */
 	GUEST_ASSERT(!ud_expected);
-	assert_hypercall_insn(native_hypercall_insn, hypercall_insn);
+	GUEST_ASSERT(!memcmp(native_hypercall_insn, hypercall_insn, HYPERCALL_INSN_SIZE));
 	GUEST_DONE();
 }
 
-- 
2.37.2.789.g6183377224-goog

Re: [PATCH 2/5] KVM: selftests: Compare insn opcodes directly in fix_hypercall_test

[Oliver Upton]

On Thu, Sep 08, 2022 at 11:31:31PM +0000, Sean Christopherson wrote:
> Directly compare the expected versus observed hypercall instructions when
> verifying that KVM patched in the native hypercall (FIX_HYPERCALL_INSN
> quirk enabled).  gcc rightly complains that doing a 4-byte memcpy() with
> an "unsigned char" as the source generates an out-of-bounds accesses.
> 
> Alternatively, "exp" and "obs" could be declared as 3-byte arrays, but
> there's no known reason to copy locally instead of comparing directly.

I was trying to print just the instruction bytes if such a comparison
failed, but that's already a bust given that it was a 4-byte copy.

Having said that, the assertion should be clear enough.

> In function ‘assert_hypercall_insn’,
>     inlined from ‘guest_main’ at x86_64/fix_hypercall_test.c:91:2:
> x86_64/fix_hypercall_test.c:63:9: error: array subscript ‘unsigned int[0]’
>  is partly outside array bounds of ‘unsigned char[1]’ [-Werror=array-bounds]
>    63 |         memcpy(&exp, exp_insn, sizeof(exp));
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> x86_64/fix_hypercall_test.c: In function ‘guest_main’:
> x86_64/fix_hypercall_test.c:42:22: note: object ‘vmx_hypercall_insn’ of size 1
>    42 | extern unsigned char vmx_hypercall_insn;
>       |                      ^~~~~~~~~~~~~~~~~~
> x86_64/fix_hypercall_test.c:25:22: note: object ‘svm_hypercall_insn’ of size 1
>    25 | extern unsigned char svm_hypercall_insn;
>       |                      ^~~~~~~~~~~~~~~~~~
> In function ‘assert_hypercall_insn’,
>     inlined from ‘guest_main’ at x86_64/fix_hypercall_test.c:91:2:
> x86_64/fix_hypercall_test.c:64:9: error: array subscript ‘unsigned int[0]’
>  is partly outside array bounds of ‘unsigned char[1]’ [-Werror=array-bounds]
>    64 |         memcpy(&obs, obs_insn, sizeof(obs));
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> x86_64/fix_hypercall_test.c: In function ‘guest_main’:
> x86_64/fix_hypercall_test.c:25:22: note: object ‘svm_hypercall_insn’ of size 1
>    25 | extern unsigned char svm_hypercall_insn;
>       |                      ^~~~~~~~~~~~~~~~~~
> x86_64/fix_hypercall_test.c:42:22: note: object ‘vmx_hypercall_insn’ of size 1
>    42 | extern unsigned char vmx_hypercall_insn;
>       |                      ^~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> make: *** [../lib.mk:135: tools/testing/selftests/kvm/x86_64/fix_hypercall_test] Error 1
> 
> Fixes: 6c2fa8b20d0c ("selftests: KVM: Test KVM_X86_QUIRK_FIX_HYPERCALL_INSN")
> Cc: Oliver Upton <oliver.upton@linux.dev>
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Oliver Upton <oliver.upton@linux.dev>

--
Thanks,
Oliver

[PATCH 3/5] KVM: selftests: Remove unnecessary register shuffling in fix_hypercall_test

[Sean Christopherson]

Use input constraints to load RAX and RBX when testing that KVM correctly
does/doesn't patch the "wrong" hypercall.  There's no need to manually
load RAX and RBX, and no reason to clobber them either (KVM is not
supposed to modify anything other than RAX).

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 .../selftests/kvm/x86_64/fix_hypercall_test.c | 22 +++++++------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c b/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c
index 2512df357ab3..dde97be3e719 100644
--- a/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c
+++ b/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c
@@ -30,14 +30,11 @@ static uint64_t svm_do_sched_yield(uint8_t apic_id)
 {
 	uint64_t ret;
 
-	asm volatile("mov %1, %%rax\n\t"
-		     "mov %2, %%rbx\n\t"
-		     "svm_hypercall_insn:\n\t"
+	asm volatile("svm_hypercall_insn:\n\t"
 		     "vmmcall\n\t"
-		     "mov %%rax, %0\n\t"
-		     : "=r"(ret)
-		     : "r"((uint64_t)KVM_HC_SCHED_YIELD), "r"((uint64_t)apic_id)
-		     : "rax", "rbx", "memory");
+		     : "=a"(ret)
+		     : "a"((uint64_t)KVM_HC_SCHED_YIELD), "b"((uint64_t)apic_id)
+		     : "memory");
 
 	return ret;
 }
@@ -47,14 +44,11 @@ static uint64_t vmx_do_sched_yield(uint8_t apic_id)
 {
 	uint64_t ret;
 
-	asm volatile("mov %1, %%rax\n\t"
-		     "mov %2, %%rbx\n\t"
-		     "vmx_hypercall_insn:\n\t"
+	asm volatile("vmx_hypercall_insn:\n\t"
 		     "vmcall\n\t"
-		     "mov %%rax, %0\n\t"
-		     : "=r"(ret)
-		     : "r"((uint64_t)KVM_HC_SCHED_YIELD), "r"((uint64_t)apic_id)
-		     : "rax", "rbx", "memory");
+		     : "=a"(ret)
+		     : "a"((uint64_t)KVM_HC_SCHED_YIELD), "b"((uint64_t)apic_id)
+		     : "memory");
 
 	return ret;
 }
-- 
2.37.2.789.g6183377224-goog

Re: [PATCH 3/5] KVM: selftests: Remove unnecessary register shuffling in fix_hypercall_test

[Oliver Upton]

On Thu, Sep 08, 2022 at 11:31:32PM +0000, Sean Christopherson wrote:
> Use input constraints to load RAX and RBX when testing that KVM correctly
> does/doesn't patch the "wrong" hypercall.  There's no need to manually
> load RAX and RBX, and no reason to clobber them either (KVM is not
> supposed to modify anything other than RAX).

Too much time on 'the other architecture' where we don't have input
constraints to load named registers per-se :)

> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Oliver Upton <oliver.upton@linux.dev>

--
Thanks,
Oliver

[PATCH 4/5] KVM: selftests: Explicitly verify KVM doesn't patch hypercall if quirk==off

[Sean Christopherson]

Explicitly verify that KVM doesn't patch in the native hypercall if the
FIX_HYPERCALL_INSN quirk is disabled.  The test currently verifies that
a #UD occurred, but doesn't actually verify that no patching occurred.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 .../selftests/kvm/x86_64/fix_hypercall_test.c | 35 ++++++++++++++-----
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c b/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c
index dde97be3e719..5925da3b3648 100644
--- a/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c
+++ b/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c
@@ -21,8 +21,8 @@ static bool ud_expected;
 
 static void guest_ud_handler(struct ex_regs *regs)
 {
-	GUEST_ASSERT(ud_expected);
-	GUEST_DONE();
+	regs->rax = -EFAULT;
+	regs->rip += HYPERCALL_INSN_SIZE;
 }
 
 extern unsigned char svm_hypercall_insn[HYPERCALL_INSN_SIZE];
@@ -57,17 +57,18 @@ static void guest_main(void)
 {
 	unsigned char *native_hypercall_insn, *hypercall_insn;
 	uint8_t apic_id;
+	uint64_t ret;
 
 	apic_id = GET_APIC_ID_FIELD(xapic_read_reg(APIC_ID));
 
 	if (is_intel_cpu()) {
 		native_hypercall_insn = vmx_hypercall_insn;
 		hypercall_insn = svm_hypercall_insn;
-		svm_do_sched_yield(apic_id);
+		ret = svm_do_sched_yield(apic_id);
 	} else if (is_amd_cpu()) {
 		native_hypercall_insn = svm_hypercall_insn;
 		hypercall_insn = vmx_hypercall_insn;
-		vmx_do_sched_yield(apic_id);
+		ret = vmx_do_sched_yield(apic_id);
 	} else {
 		GUEST_ASSERT(0);
 		/* unreachable */
@@ -75,12 +76,28 @@ static void guest_main(void)
 	}
 
 	/*
-	 * The hypercall didn't #UD (guest_ud_handler() signals "done" if a #UD
-	 * occurs).  Verify that a #UD is NOT expected and that KVM patched in
-	 * the native hypercall.
+	 * If the quirk is disabled, verify that guest_ud_handler() "returned"
+	 * -EFAULT and that KVM did NOT patch the hypercall.  If the quirk is
+	 * enabled, verify that the hypercall succeeded and that KVM patched in
+	 * the "right" hypercall.
 	 */
-	GUEST_ASSERT(!ud_expected);
-	GUEST_ASSERT(!memcmp(native_hypercall_insn, hypercall_insn, HYPERCALL_INSN_SIZE));
+	if (ud_expected) {
+		GUEST_ASSERT(ret == (uint64_t)-EFAULT);
+
+		/*
+		 * Divergence should occur only on the last byte, as the VMCALL
+		 * (0F 01 C1) and VMMCALL (0F 01 D9) share the first two bytes.
+		 */
+		GUEST_ASSERT(!memcmp(native_hypercall_insn, hypercall_insn,
+				     HYPERCALL_INSN_SIZE - 1));
+		GUEST_ASSERT(memcmp(native_hypercall_insn, hypercall_insn,
+				    HYPERCALL_INSN_SIZE));
+	} else {
+		GUEST_ASSERT(!ret);
+		GUEST_ASSERT(!memcmp(native_hypercall_insn, hypercall_insn,
+			     HYPERCALL_INSN_SIZE));
+	}
+
 	GUEST_DONE();
 }
 
-- 
2.37.2.789.g6183377224-goog

Re: [PATCH 4/5] KVM: selftests: Explicitly verify KVM doesn't patch hypercall if quirk==off

[Oliver Upton]

On Thu, Sep 08, 2022 at 11:31:33PM +0000, Sean Christopherson wrote:
> Explicitly verify that KVM doesn't patch in the native hypercall if the
> FIX_HYPERCALL_INSN quirk is disabled.  The test currently verifies that
> a #UD occurred, but doesn't actually verify that no patching occurred.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  .../selftests/kvm/x86_64/fix_hypercall_test.c | 35 ++++++++++++++-----
>  1 file changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c b/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c
> index dde97be3e719..5925da3b3648 100644
> --- a/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c
> @@ -21,8 +21,8 @@ static bool ud_expected;
>  
>  static void guest_ud_handler(struct ex_regs *regs)
>  {
> -	GUEST_ASSERT(ud_expected);
> -	GUEST_DONE();
> +	regs->rax = -EFAULT;
> +	regs->rip += HYPERCALL_INSN_SIZE;
>  }
>  
>  extern unsigned char svm_hypercall_insn[HYPERCALL_INSN_SIZE];
> @@ -57,17 +57,18 @@ static void guest_main(void)
>  {
>  	unsigned char *native_hypercall_insn, *hypercall_insn;
>  	uint8_t apic_id;
> +	uint64_t ret;
>  
>  	apic_id = GET_APIC_ID_FIELD(xapic_read_reg(APIC_ID));
>  
>  	if (is_intel_cpu()) {
>  		native_hypercall_insn = vmx_hypercall_insn;
>  		hypercall_insn = svm_hypercall_insn;
> -		svm_do_sched_yield(apic_id);
> +		ret = svm_do_sched_yield(apic_id);
>  	} else if (is_amd_cpu()) {
>  		native_hypercall_insn = svm_hypercall_insn;
>  		hypercall_insn = vmx_hypercall_insn;
> -		vmx_do_sched_yield(apic_id);
> +		ret = vmx_do_sched_yield(apic_id);
>  	} else {
>  		GUEST_ASSERT(0);
>  		/* unreachable */
> @@ -75,12 +76,28 @@ static void guest_main(void)
>  	}
>  
>  	/*
> -	 * The hypercall didn't #UD (guest_ud_handler() signals "done" if a #UD
> -	 * occurs).  Verify that a #UD is NOT expected and that KVM patched in
> -	 * the native hypercall.
> +	 * If the quirk is disabled, verify that guest_ud_handler() "returned"
> +	 * -EFAULT and that KVM did NOT patch the hypercall.  If the quirk is
> +	 * enabled, verify that the hypercall succeeded and that KVM patched in
> +	 * the "right" hypercall.
>  	 */
> -	GUEST_ASSERT(!ud_expected);
> -	GUEST_ASSERT(!memcmp(native_hypercall_insn, hypercall_insn, HYPERCALL_INSN_SIZE));
> +	if (ud_expected) {
> +		GUEST_ASSERT(ret == (uint64_t)-EFAULT);
> +
> +		/*
> +		 * Divergence should occur only on the last byte, as the VMCALL
> +		 * (0F 01 C1) and VMMCALL (0F 01 D9) share the first two bytes.
> +		 */
> +		GUEST_ASSERT(!memcmp(native_hypercall_insn, hypercall_insn,
> +				     HYPERCALL_INSN_SIZE - 1));
> +		GUEST_ASSERT(memcmp(native_hypercall_insn, hypercall_insn,
> +				    HYPERCALL_INSN_SIZE));

Should we just keep the assertions consistent for both cases (patched
and unpatched)?

--
Thanks,
Oliver

> +	} else {
> +		GUEST_ASSERT(!ret);
> +		GUEST_ASSERT(!memcmp(native_hypercall_insn, hypercall_insn,
> +			     HYPERCALL_INSN_SIZE));
> +	}
> +
>  	GUEST_DONE();
>  }
>  
> -- 
> 2.37.2.789.g6183377224-goog
> 

Re: [PATCH 4/5] KVM: selftests: Explicitly verify KVM doesn't patch hypercall if quirk==off

[Sean Christopherson]

On Mon, Sep 19, 2022, Oliver Upton wrote:
> On Thu, Sep 08, 2022 at 11:31:33PM +0000, Sean Christopherson wrote:
> > @@ -75,12 +76,28 @@ static void guest_main(void)
> >  	}
> >  
> >  	/*
> > -	 * The hypercall didn't #UD (guest_ud_handler() signals "done" if a #UD
> > -	 * occurs).  Verify that a #UD is NOT expected and that KVM patched in
> > -	 * the native hypercall.
> > +	 * If the quirk is disabled, verify that guest_ud_handler() "returned"
> > +	 * -EFAULT and that KVM did NOT patch the hypercall.  If the quirk is
> > +	 * enabled, verify that the hypercall succeeded and that KVM patched in
> > +	 * the "right" hypercall.
> >  	 */
> > -	GUEST_ASSERT(!ud_expected);
> > -	GUEST_ASSERT(!memcmp(native_hypercall_insn, hypercall_insn, HYPERCALL_INSN_SIZE));
> > +	if (ud_expected) {
> > +		GUEST_ASSERT(ret == (uint64_t)-EFAULT);
> > +
> > +		/*
> > +		 * Divergence should occur only on the last byte, as the VMCALL
> > +		 * (0F 01 C1) and VMMCALL (0F 01 D9) share the first two bytes.
> > +		 */
> > +		GUEST_ASSERT(!memcmp(native_hypercall_insn, hypercall_insn,
> > +				     HYPERCALL_INSN_SIZE - 1));
> > +		GUEST_ASSERT(memcmp(native_hypercall_insn, hypercall_insn,
> > +				    HYPERCALL_INSN_SIZE));
> 
> Should we just keep the assertions consistent for both cases (patched
> and unpatched)?

Not sure I follow what you're suggesting.  By "consistent" do you mean doing
something like snapshotting hypercall_insn and verifying that it's not changed?

[PATCH 5/5] KVM: selftests: Dedup subtests of fix_hypercall_test

[Sean Christopherson]

Combine fix_hypercall_test's two subtests into a common routine, the only
difference between the two is whether or not  disable the quirk.  Passing
a boolean is a little gross, but using an enum to make it super obvious
that the callers are enabling/disabling the quirk seems like overkill.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 .../selftests/kvm/x86_64/fix_hypercall_test.c | 45 ++++++-------------
 1 file changed, 13 insertions(+), 32 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c b/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c
index 5925da3b3648..4bbc4b95136f 100644
--- a/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c
+++ b/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c
@@ -17,7 +17,7 @@
 /* VMCALL and VMMCALL are both 3-byte opcodes. */
 #define HYPERCALL_INSN_SIZE	3
 
-static bool ud_expected;
+static bool quirk_disabled;
 
 static void guest_ud_handler(struct ex_regs *regs)
 {
@@ -81,7 +81,7 @@ static void guest_main(void)
 	 * enabled, verify that the hypercall succeeded and that KVM patched in
 	 * the "right" hypercall.
 	 */
-	if (ud_expected) {
+	if (quirk_disabled) {
 		GUEST_ASSERT(ret == (uint64_t)-EFAULT);
 
 		/*
@@ -101,13 +101,6 @@ static void guest_main(void)
 	GUEST_DONE();
 }
 
-static void setup_ud_vector(struct kvm_vcpu *vcpu)
-{
-	vm_init_descriptor_tables(vcpu->vm);
-	vcpu_init_descriptor_tables(vcpu);
-	vm_install_exception_handler(vcpu->vm, UD_VECTOR, guest_ud_handler);
-}
-
 static void enter_guest(struct kvm_vcpu *vcpu)
 {
 	struct kvm_run *run = vcpu->run;
@@ -128,35 +121,23 @@ static void enter_guest(struct kvm_vcpu *vcpu)
 	}
 }
 
-static void test_fix_hypercall(void)
+static void test_fix_hypercall(bool disable_quirk)
 {
 	struct kvm_vcpu *vcpu;
 	struct kvm_vm *vm;
 
 	vm = vm_create_with_one_vcpu(&vcpu, guest_main);
-	setup_ud_vector(vcpu);
 
-	ud_expected = false;
-	sync_global_to_guest(vm, ud_expected);
+	vm_init_descriptor_tables(vcpu->vm);
+	vcpu_init_descriptor_tables(vcpu);
+	vm_install_exception_handler(vcpu->vm, UD_VECTOR, guest_ud_handler);
 
-	virt_pg_map(vm, APIC_DEFAULT_GPA, APIC_DEFAULT_GPA);
+	if (disable_quirk)
+		vm_enable_cap(vm, KVM_CAP_DISABLE_QUIRKS2,
+			      KVM_X86_QUIRK_FIX_HYPERCALL_INSN);
 
-	enter_guest(vcpu);
-}
-
-static void test_fix_hypercall_disabled(void)
-{
-	struct kvm_vcpu *vcpu;
-	struct kvm_vm *vm;
-
-	vm = vm_create_with_one_vcpu(&vcpu, guest_main);
-	setup_ud_vector(vcpu);
-
-	vm_enable_cap(vm, KVM_CAP_DISABLE_QUIRKS2,
-		      KVM_X86_QUIRK_FIX_HYPERCALL_INSN);
-
-	ud_expected = true;
-	sync_global_to_guest(vm, ud_expected);
+	quirk_disabled = disable_quirk;
+	sync_global_to_guest(vm, quirk_disabled);
 
 	virt_pg_map(vm, APIC_DEFAULT_GPA, APIC_DEFAULT_GPA);
 
@@ -167,6 +148,6 @@ int main(void)
 {
 	TEST_REQUIRE(kvm_check_cap(KVM_CAP_DISABLE_QUIRKS2) & KVM_X86_QUIRK_FIX_HYPERCALL_INSN);
 
-	test_fix_hypercall();
-	test_fix_hypercall_disabled();
+	test_fix_hypercall(false);
+	test_fix_hypercall(true);
 }
-- 
2.37.2.789.g6183377224-goog

Re: [PATCH 5/5] KVM: selftests: Dedup subtests of fix_hypercall_test

[Oliver Upton]

On Thu, Sep 08, 2022 at 11:31:34PM +0000, Sean Christopherson wrote:
> Combine fix_hypercall_test's two subtests into a common routine, the only
> difference between the two is whether or not  disable the quirk.  Passing
> a boolean is a little gross, but using an enum to make it super obvious
> that the callers are enabling/disabling the quirk seems like overkill.
> 

Eh, good 'nuff.

> No functional change intended.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Oliver Upton <oliver.upton@linux.dev>

--
Thanks,
Oliver

Re: [PATCH 0/5] KVM: selftests: Fix "fix hypercall test" build errors

[Christian Borntraeger]

Am 09.09.22 um 01:31 schrieb Sean Christopherson:
> After a toolchain upgrade (I think), the x86 fix_hypercall_test started
> throwing warnings due to -Werror=array-bounds rightly complaining that
> the test is generating an out-of-bounds array access.
> 
> The "obvious" fix is to replace the memcpy() with a memcmp() and compare
> only the exact size of the hypercall instruction.  That worked, until I
> fiddled with the code a bit more and suddenly the test started jumping into
> the weeds due to gcc generating a call to the external memcmp() through the
> PLT, which isn't supported in the selftests.
> 
> To fix that mess, which has been a pitfall for quite some time, provide
> implementations of memcmp(), memcpy(), and memset() to effectively override
> the compiler built-ins.  My thought is to start with the helpers that are
> most likely to be used in guest code, and then add more as needed.
> 
> Tested on x86 and ARM, compile tested on RISC-V and s390.  Full testing on
> RISC-V and s390 would be welcome, the seemingly benign addition of memxxx()
> helpers managed to break ARM due to gcc generating an infinite loop for
> memset() (see patch 1 for details).

Seems to run fine on s390.

Re: [PATCH 0/5] KVM: selftests: Fix "fix hypercall test" build errors

[David Matlack]

On Thu, Sep 8, 2022 at 4:34 PM Sean Christopherson <seanjc@google.com> wrote:
>
> After a toolchain upgrade (I think), the x86 fix_hypercall_test started
> throwing warnings due to -Werror=array-bounds rightly complaining that
> the test is generating an out-of-bounds array access.
>
> The "obvious" fix is to replace the memcpy() with a memcmp() and compare
> only the exact size of the hypercall instruction.  That worked, until I
> fiddled with the code a bit more and suddenly the test started jumping into
> the weeds due to gcc generating a call to the external memcmp() through the
> PLT, which isn't supported in the selftests.
>
> To fix that mess, which has been a pitfall for quite some time, provide
> implementations of memcmp(), memcpy(), and memset() to effectively override
> the compiler built-ins.  My thought is to start with the helpers that are
> most likely to be used in guest code, and then add more as needed.

Ah ha! This also fixes an issue I've long since noticed and finally
got around to debugging this morning. userspace_io_test fails for me
when built with Clang but passes with GCC. It turns out Clang
generates a call to <memset@plt>, whereas GCC directly generates rep
stos, to clear @buffer in guest_code().

Re: [PATCH 0/5] KVM: selftests: Fix "fix hypercall test" build errors

[Jim Mattson]

On Thu, Sep 22, 2022 at 10:20 AM David Matlack <dmatlack@google.com> wrote:
>
> On Thu, Sep 8, 2022 at 4:34 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > After a toolchain upgrade (I think), the x86 fix_hypercall_test started
> > throwing warnings due to -Werror=array-bounds rightly complaining that
> > the test is generating an out-of-bounds array access.
> >
> > The "obvious" fix is to replace the memcpy() with a memcmp() and compare
> > only the exact size of the hypercall instruction.  That worked, until I
> > fiddled with the code a bit more and suddenly the test started jumping into
> > the weeds due to gcc generating a call to the external memcmp() through the
> > PLT, which isn't supported in the selftests.
> >
> > To fix that mess, which has been a pitfall for quite some time, provide
> > implementations of memcmp(), memcpy(), and memset() to effectively override
> > the compiler built-ins.  My thought is to start with the helpers that are
> > most likely to be used in guest code, and then add more as needed.
>
> Ah ha! This also fixes an issue I've long since noticed and finally
> got around to debugging this morning. userspace_io_test fails for me
> when built with Clang but passes with GCC. It turns out Clang
> generates a call to <memset@plt>, whereas GCC directly generates rep
> stos, to clear @buffer in guest_code().

Hey! Did I miss a revert of commit ed290e1c20da ("KVM: selftests: Fix
nested SVM tests when built with clang") in that patch set?

Re: [PATCH 0/5] KVM: selftests: Fix "fix hypercall test" build errors

[Sean Christopherson]

On Thu, Sep 22, 2022, Jim Mattson wrote:
> On Thu, Sep 22, 2022 at 10:20 AM David Matlack <dmatlack@google.com> wrote:
> >
> > On Thu, Sep 8, 2022 at 4:34 PM Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > After a toolchain upgrade (I think), the x86 fix_hypercall_test started
> > > throwing warnings due to -Werror=array-bounds rightly complaining that
> > > the test is generating an out-of-bounds array access.
> > >
> > > The "obvious" fix is to replace the memcpy() with a memcmp() and compare
> > > only the exact size of the hypercall instruction.  That worked, until I
> > > fiddled with the code a bit more and suddenly the test started jumping into
> > > the weeds due to gcc generating a call to the external memcmp() through the
> > > PLT, which isn't supported in the selftests.
> > >
> > > To fix that mess, which has been a pitfall for quite some time, provide
> > > implementations of memcmp(), memcpy(), and memset() to effectively override
> > > the compiler built-ins.  My thought is to start with the helpers that are
> > > most likely to be used in guest code, and then add more as needed.
> >
> > Ah ha! This also fixes an issue I've long since noticed and finally
> > got around to debugging this morning. userspace_io_test fails for me
> > when built with Clang but passes with GCC. It turns out Clang
> > generates a call to <memset@plt>, whereas GCC directly generates rep
> > stos, to clear @buffer in guest_code().
> 
> Hey! Did I miss a revert of commit ed290e1c20da ("KVM: selftests: Fix
> nested SVM tests when built with clang") in that patch set?

LOL, no, no you did not.  I'll do that in v2.