[PATCH 0/8] Add printf and formatted asserts in the guest

[PATCH 0/8] Add printf and formatted asserts in the guest

[Aaron Lewis]

Extend the ucall framework to offer GUEST_PRINTF and GUEST_ASSERT_FMT
in selftests.  This allows the guest to use string formating for easier
debugging and richer assert messages.

I was originally going to send this series out as an RFC to demonstrate
two different ways of implementing prints in the guest (both through the
same ucall interface).  I ultimately decided against it because the other
approach had enough cons to convince me that this one is better.

As a quick overview on the approach I abondoned, it involved adding enough
support to the guest to allow it to call the LIBC version of vsprintf()
directly.  In order to do that a simple TLS segment had to be added to the
guest, AVX-512 needed to be enabled, and the loadable ELF segments needed
to be copied from the host.  This all seemed very intrusive to the guest.
I tried reducing this burden by getting the string functions to not
use AVX-512 instructions, unfortunately the complier flags I tried didn't
work.  Also, the approach used in this series is far less intrusive to
the guest anyway, so I abondoned it.

That exercise informed how I set up the selftest.  The selftest, aside
from using GUEST_PRINTF and GUEST_ASSERT_FMT, also checks XCR0 to show
that the prints work without AVX-512 being enabled.  Two things I
learned LIBC loves to do when using string functions is use the TLS and
call AVX-512 instructions.  Either of which will cause the test to either
crash or (unintentionally) assert.

I say unintentionally assert because the test ends with a formatted
assert firing.  This is intentional, and is meant to demonstrate the
formatted assert.

That is one reason I don't really expect the selftest to be accepted with
this series.  The other reason is it doesn't test anything in the kernel.
And if the selftest is not accepted then the first two patches can be
omitted too.  The core of the series are patches 3-7.

Patches 1-2:
 - Adds XGETBV/XSETBV and xfeature flags to common code.
 - Needed for the selftest added in the final commit.
 - Also included in:
     https://lore.kernel.org/kvm/20230224223607.1580880-1-aaronlewis@google.com/
 - Can be omitted from the final series along with the selftest.

Patches 3-5:
 - Adds a local version of vsprintf to the selftests for the guest.

Patches 6-7:
 - Adds GUEST_PRINTF and GUEST_ASSERT_FMT to the ucall framework.

Patch 8:
 - Adds a selftest to demonstrate the usage of prints and formatted
   asserts in the guest.
 - This test is a demo and doesn't have to be merged with this series,
   which also means patches 1 and 2 don't have to be merged either.

Aaron Lewis (8):
  KVM: selftests: Hoist XGETBV and XSETBV to make them more accessible
  KVM: selftests: Add XFEATURE masks to common code
  KVM: selftests: Add strnlen() to the string overrides
  KVM: selftests: Copy printf.c to KVM selftests
  KVM: selftests: Add vsprintf() to KVM selftests
  KVM: selftests: Add additional pages to the guest to accommodate ucall
  KVM: selftests: Add string formatting options to ucall
  KVM: selftests: Add a selftest for guest prints and formatted asserts

 tools/testing/selftests/kvm/Makefile          |   3 +
 .../testing/selftests/kvm/include/test_util.h |   2 +
 .../selftests/kvm/include/ucall_common.h      |  20 ++
 .../selftests/kvm/include/x86_64/processor.h  |  36 +++
 tools/testing/selftests/kvm/lib/kvm_util.c    |   4 +
 tools/testing/selftests/kvm/lib/printf.c      | 286 ++++++++++++++++++
 .../selftests/kvm/lib/string_override.c       |   9 +
 .../testing/selftests/kvm/lib/ucall_common.c  |  24 ++
 tools/testing/selftests/kvm/x86_64/amx_test.c |  46 +--
 .../selftests/kvm/x86_64/guest_print_test.c   | 100 ++++++
 10 files changed, 495 insertions(+), 35 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/lib/printf.c
 create mode 100644 tools/testing/selftests/kvm/x86_64/guest_print_test.c

-- 
2.40.0.rc0.216.gc4246ad0f0-goog

[PATCH 1/8] KVM: selftests: Hoist XGETBV and XSETBV to make them more accessible

[Aaron Lewis]

The instructions XGETBV and XSETBV are useful to other tests.  Move
them to processor.h to make them available to be used more broadly.

No functional change intended.

Reviewed-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 .../selftests/kvm/include/x86_64/processor.h  | 19 +++++++++++++++
 tools/testing/selftests/kvm/x86_64/amx_test.c | 24 +++----------------
 2 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index 53ffa43c90db..62a5c3953deb 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -496,6 +496,25 @@ static inline void set_cr4(uint64_t val)
 	__asm__ __volatile__("mov %0, %%cr4" : : "r" (val) : "memory");
 }
 
+static inline u64 xgetbv(u32 index)
+{
+	u32 eax, edx;
+
+	__asm__ __volatile__("xgetbv;"
+		     : "=a" (eax), "=d" (edx)
+		     : "c" (index));
+	return eax | ((u64)edx << 32);
+}
+
+static inline void xsetbv(u32 index, u64 value)
+{
+	u32 eax = value;
+	u32 edx = value >> 32;
+
+	__asm__ __volatile__("xsetbv" :: "a" (eax), "d" (edx), "c" (index));
+}
+
+
 static inline struct desc_ptr get_gdt(void)
 {
 	struct desc_ptr gdt;
diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
index bd72c6eb3b67..4b733ad21831 100644
--- a/tools/testing/selftests/kvm/x86_64/amx_test.c
+++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
@@ -68,24 +68,6 @@ struct xtile_info {
 
 static struct xtile_info xtile;
 
-static inline u64 __xgetbv(u32 index)
-{
-	u32 eax, edx;
-
-	asm volatile("xgetbv;"
-		     : "=a" (eax), "=d" (edx)
-		     : "c" (index));
-	return eax + ((u64)edx << 32);
-}
-
-static inline void __xsetbv(u32 index, u64 value)
-{
-	u32 eax = value;
-	u32 edx = value >> 32;
-
-	asm volatile("xsetbv" :: "a" (eax), "d" (edx), "c" (index));
-}
-
 static inline void __ldtilecfg(void *cfg)
 {
 	asm volatile(".byte 0xc4,0xe2,0x78,0x49,0x00"
@@ -121,7 +103,7 @@ static inline void check_cpuid_xsave(void)
 
 static bool check_xsave_supports_xtile(void)
 {
-	return __xgetbv(0) & XFEATURE_MASK_XTILE;
+	return xgetbv(0) & XFEATURE_MASK_XTILE;
 }
 
 static void check_xtile_info(void)
@@ -177,9 +159,9 @@ static void init_regs(void)
 	cr4 |= X86_CR4_OSXSAVE;
 	set_cr4(cr4);
 
-	xcr0 = __xgetbv(0);
+	xcr0 = xgetbv(0);
 	xcr0 |= XFEATURE_MASK_XTILE;
-	__xsetbv(0x0, xcr0);
+	xsetbv(0x0, xcr0);
 }
 
 static void __attribute__((__flatten__)) guest_code(struct tile_config *amx_cfg,
-- 
2.40.0.rc0.216.gc4246ad0f0-goog

[PATCH 2/8] KVM: selftests: Add XFEATURE masks to common code

[Aaron Lewis]

Add XFEATURE masks to processor.h to make them more broadly available
in KVM selftests.

They were taken from fpu/types.h, which included a difference in
spacing between the ones in amx_test from XTILECFG and XTILEDATA, to
XTILE_CFG and XTILE_DATA.  This has been reflected in amx_test.

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 .../selftests/kvm/include/x86_64/processor.h  | 17 ++++++++++++++
 tools/testing/selftests/kvm/x86_64/amx_test.c | 22 +++++++------------
 2 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index 62a5c3953deb..8e201575ef73 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -48,6 +48,23 @@ extern bool host_cpu_is_amd;
 #define X86_CR4_SMAP		(1ul << 21)
 #define X86_CR4_PKE		(1ul << 22)
 
+#define XFEATURE_MASK_FP		BIT_ULL(0)
+#define XFEATURE_MASK_SSE		BIT_ULL(1)
+#define XFEATURE_MASK_YMM		BIT_ULL(2)
+#define XFEATURE_MASK_BNDREGS		BIT_ULL(3)
+#define XFEATURE_MASK_BNDCSR		BIT_ULL(4)
+#define XFEATURE_MASK_OPMASK		BIT_ULL(5)
+#define XFEATURE_MASK_ZMM_Hi256		BIT_ULL(6)
+#define XFEATURE_MASK_Hi16_ZMM		BIT_ULL(7)
+#define XFEATURE_MASK_XTILE_CFG		BIT_ULL(17)
+#define XFEATURE_MASK_XTILE_DATA	BIT_ULL(18)
+
+#define XFEATURE_MASK_AVX512		(XFEATURE_MASK_OPMASK \
+					 | XFEATURE_MASK_ZMM_Hi256 \
+					 | XFEATURE_MASK_Hi16_ZMM)
+#define XFEATURE_MASK_XTILE		(XFEATURE_MASK_XTILE_DATA \
+					 | XFEATURE_MASK_XTILE_CFG)
+
 /* Note, these are ordered alphabetically to match kvm_cpuid_entry2.  Eww. */
 enum cpuid_output_regs {
 	KVM_CPUID_EAX,
diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
index 4b733ad21831..14a7656620d5 100644
--- a/tools/testing/selftests/kvm/x86_64/amx_test.c
+++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
@@ -33,12 +33,6 @@
 #define MAX_TILES			16
 #define RESERVED_BYTES			14
 
-#define XFEATURE_XTILECFG		17
-#define XFEATURE_XTILEDATA		18
-#define XFEATURE_MASK_XTILECFG		(1 << XFEATURE_XTILECFG)
-#define XFEATURE_MASK_XTILEDATA		(1 << XFEATURE_XTILEDATA)
-#define XFEATURE_MASK_XTILE		(XFEATURE_MASK_XTILECFG | XFEATURE_MASK_XTILEDATA)
-
 #define XSAVE_HDR_OFFSET		512
 
 struct xsave_data {
@@ -187,14 +181,14 @@ static void __attribute__((__flatten__)) guest_code(struct tile_config *amx_cfg,
 	__tilerelease();
 	GUEST_SYNC(5);
 	/* bit 18 not in the XCOMP_BV after xsavec() */
-	set_xstatebv(xsave_data, XFEATURE_MASK_XTILEDATA);
-	__xsavec(xsave_data, XFEATURE_MASK_XTILEDATA);
-	GUEST_ASSERT((get_xstatebv(xsave_data) & XFEATURE_MASK_XTILEDATA) == 0);
+	set_xstatebv(xsave_data, XFEATURE_MASK_XTILE_DATA);
+	__xsavec(xsave_data, XFEATURE_MASK_XTILE_DATA);
+	GUEST_ASSERT((get_xstatebv(xsave_data) & XFEATURE_MASK_XTILE_DATA) == 0);
 
 	/* xfd=0x40000, disable amx tiledata */
-	wrmsr(MSR_IA32_XFD, XFEATURE_MASK_XTILEDATA);
+	wrmsr(MSR_IA32_XFD, XFEATURE_MASK_XTILE_DATA);
 	GUEST_SYNC(6);
-	GUEST_ASSERT(rdmsr(MSR_IA32_XFD) == XFEATURE_MASK_XTILEDATA);
+	GUEST_ASSERT(rdmsr(MSR_IA32_XFD) == XFEATURE_MASK_XTILE_DATA);
 	set_tilecfg(amx_cfg);
 	__ldtilecfg(amx_cfg);
 	/* Trigger #NM exception */
@@ -206,11 +200,11 @@ static void __attribute__((__flatten__)) guest_code(struct tile_config *amx_cfg,
 
 void guest_nm_handler(struct ex_regs *regs)
 {
-	/* Check if #NM is triggered by XFEATURE_MASK_XTILEDATA */
+	/* Check if #NM is triggered by XFEATURE_MASK_XTILE_DATA */
 	GUEST_SYNC(7);
-	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILEDATA);
+	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILE_DATA);
 	GUEST_SYNC(8);
-	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILEDATA);
+	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILE_DATA);
 	/* Clear xfd_err */
 	wrmsr(MSR_IA32_XFD_ERR, 0);
 	/* xfd=0, enable amx */
-- 
2.40.0.rc0.216.gc4246ad0f0-goog

[PATCH 3/8] KVM: selftests: Add strnlen() to the string overrides

[Aaron Lewis]

Add strnlen() to the string overrides to allow it to be called in the
guest.

The implementation for strnlen() was taken from the kernel's generic
version, lib/string.c.

This will be needed when printf is introduced.

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 tools/testing/selftests/kvm/Makefile              | 1 +
 tools/testing/selftests/kvm/lib/string_override.c | 9 +++++++++
 2 files changed, 10 insertions(+)

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 84a627c43795..cda631f10526 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -197,6 +197,7 @@ endif
 CFLAGS += -Wall -Wstrict-prototypes -Wuninitialized -O2 -g -std=gnu99 \
 	-Wno-gnu-variable-sized-type-not-at-end \
 	-fno-builtin-memcmp -fno-builtin-memcpy -fno-builtin-memset \
+	-fno-builtin-strnlen \
 	-fno-stack-protector -fno-PIE -I$(LINUX_TOOL_INCLUDE) \
 	-I$(LINUX_TOOL_ARCH_INCLUDE) -I$(LINUX_HDR_PATH) -Iinclude \
 	-I$(<D) -Iinclude/$(ARCH_DIR) -I ../rseq -I.. $(EXTRA_CFLAGS) \
diff --git a/tools/testing/selftests/kvm/lib/string_override.c b/tools/testing/selftests/kvm/lib/string_override.c
index 632398adc229..5d1c87277c49 100644
--- a/tools/testing/selftests/kvm/lib/string_override.c
+++ b/tools/testing/selftests/kvm/lib/string_override.c
@@ -37,3 +37,12 @@ void *memset(void *s, int c, size_t count)
 		*xs++ = c;
 	return s;
 }
+
+size_t strnlen(const char *s, size_t count)
+{
+	const char *sc;
+
+	for (sc = s; count-- && *sc != '\0'; ++sc)
+		/* nothing */;
+	return sc - s;
+}
-- 
2.40.0.rc0.216.gc4246ad0f0-goog

[PATCH 4/8] KVM: selftests: Copy printf.c to KVM selftests

[Aaron Lewis]

Add a local version of vsprintf() for the guest to use.

The file printf.c was lifted from arch/x86/boot/printf.c.

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 tools/testing/selftests/kvm/lib/printf.c | 307 +++++++++++++++++++++++
 1 file changed, 307 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/lib/printf.c

diff --git a/tools/testing/selftests/kvm/lib/printf.c b/tools/testing/selftests/kvm/lib/printf.c
new file mode 100644
index 000000000000..1237beeb9540
--- /dev/null
+++ b/tools/testing/selftests/kvm/lib/printf.c
@@ -0,0 +1,307 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* -*- linux-c -*- ------------------------------------------------------- *
+ *
+ *   Copyright (C) 1991, 1992 Linus Torvalds
+ *   Copyright 2007 rPath, Inc. - All Rights Reserved
+ *
+ * ----------------------------------------------------------------------- */
+
+/*
+ * Oh, it's a waste of space, but oh-so-yummy for debugging.  This
+ * version of printf() does not include 64-bit support.  "Live with
+ * it."
+ *
+ */
+
+#include "boot.h"
+
+static int skip_atoi(const char **s)
+{
+	int i = 0;
+
+	while (isdigit(**s))
+		i = i * 10 + *((*s)++) - '0';
+	return i;
+}
+
+#define ZEROPAD	1		/* pad with zero */
+#define SIGN	2		/* unsigned/signed long */
+#define PLUS	4		/* show plus */
+#define SPACE	8		/* space if plus */
+#define LEFT	16		/* left justified */
+#define SMALL	32		/* Must be 32 == 0x20 */
+#define SPECIAL	64		/* 0x */
+
+#define __do_div(n, base) ({ \
+int __res; \
+__res = ((unsigned long) n) % (unsigned) base; \
+n = ((unsigned long) n) / (unsigned) base; \
+__res; })
+
+static char *number(char *str, long num, int base, int size, int precision,
+		    int type)
+{
+	/* we are called with base 8, 10 or 16, only, thus don't need "G..."  */
+	static const char digits[16] = "0123456789ABCDEF"; /* "GHIJKLMNOPQRSTUVWXYZ"; */
+
+	char tmp[66];
+	char c, sign, locase;
+	int i;
+
+	/* locase = 0 or 0x20. ORing digits or letters with 'locase'
+	 * produces same digits or (maybe lowercased) letters */
+	locase = (type & SMALL);
+	if (type & LEFT)
+		type &= ~ZEROPAD;
+	if (base < 2 || base > 16)
+		return NULL;
+	c = (type & ZEROPAD) ? '0' : ' ';
+	sign = 0;
+	if (type & SIGN) {
+		if (num < 0) {
+			sign = '-';
+			num = -num;
+			size--;
+		} else if (type & PLUS) {
+			sign = '+';
+			size--;
+		} else if (type & SPACE) {
+			sign = ' ';
+			size--;
+		}
+	}
+	if (type & SPECIAL) {
+		if (base == 16)
+			size -= 2;
+		else if (base == 8)
+			size--;
+	}
+	i = 0;
+	if (num == 0)
+		tmp[i++] = '0';
+	else
+		while (num != 0)
+			tmp[i++] = (digits[__do_div(num, base)] | locase);
+	if (i > precision)
+		precision = i;
+	size -= precision;
+	if (!(type & (ZEROPAD + LEFT)))
+		while (size-- > 0)
+			*str++ = ' ';
+	if (sign)
+		*str++ = sign;
+	if (type & SPECIAL) {
+		if (base == 8)
+			*str++ = '0';
+		else if (base == 16) {
+			*str++ = '0';
+			*str++ = ('X' | locase);
+		}
+	}
+	if (!(type & LEFT))
+		while (size-- > 0)
+			*str++ = c;
+	while (i < precision--)
+		*str++ = '0';
+	while (i-- > 0)
+		*str++ = tmp[i];
+	while (size-- > 0)
+		*str++ = ' ';
+	return str;
+}
+
+int vsprintf(char *buf, const char *fmt, va_list args)
+{
+	int len;
+	unsigned long num;
+	int i, base;
+	char *str;
+	const char *s;
+
+	int flags;		/* flags to number() */
+
+	int field_width;	/* width of output field */
+	int precision;		/* min. # of digits for integers; max
+				   number of chars for from string */
+	int qualifier;		/* 'h', 'l', or 'L' for integer fields */
+
+	for (str = buf; *fmt; ++fmt) {
+		if (*fmt != '%') {
+			*str++ = *fmt;
+			continue;
+		}
+
+		/* process flags */
+		flags = 0;
+	      repeat:
+		++fmt;		/* this also skips first '%' */
+		switch (*fmt) {
+		case '-':
+			flags |= LEFT;
+			goto repeat;
+		case '+':
+			flags |= PLUS;
+			goto repeat;
+		case ' ':
+			flags |= SPACE;
+			goto repeat;
+		case '#':
+			flags |= SPECIAL;
+			goto repeat;
+		case '0':
+			flags |= ZEROPAD;
+			goto repeat;
+		}
+
+		/* get field width */
+		field_width = -1;
+		if (isdigit(*fmt))
+			field_width = skip_atoi(&fmt);
+		else if (*fmt == '*') {
+			++fmt;
+			/* it's the next argument */
+			field_width = va_arg(args, int);
+			if (field_width < 0) {
+				field_width = -field_width;
+				flags |= LEFT;
+			}
+		}
+
+		/* get the precision */
+		precision = -1;
+		if (*fmt == '.') {
+			++fmt;
+			if (isdigit(*fmt))
+				precision = skip_atoi(&fmt);
+			else if (*fmt == '*') {
+				++fmt;
+				/* it's the next argument */
+				precision = va_arg(args, int);
+			}
+			if (precision < 0)
+				precision = 0;
+		}
+
+		/* get the conversion qualifier */
+		qualifier = -1;
+		if (*fmt == 'h' || *fmt == 'l' || *fmt == 'L') {
+			qualifier = *fmt;
+			++fmt;
+		}
+
+		/* default base */
+		base = 10;
+
+		switch (*fmt) {
+		case 'c':
+			if (!(flags & LEFT))
+				while (--field_width > 0)
+					*str++ = ' ';
+			*str++ = (unsigned char)va_arg(args, int);
+			while (--field_width > 0)
+				*str++ = ' ';
+			continue;
+
+		case 's':
+			s = va_arg(args, char *);
+			len = strnlen(s, precision);
+
+			if (!(flags & LEFT))
+				while (len < field_width--)
+					*str++ = ' ';
+			for (i = 0; i < len; ++i)
+				*str++ = *s++;
+			while (len < field_width--)
+				*str++ = ' ';
+			continue;
+
+		case 'p':
+			if (field_width == -1) {
+				field_width = 2 * sizeof(void *);
+				flags |= ZEROPAD;
+			}
+			str = number(str,
+				     (unsigned long)va_arg(args, void *), 16,
+				     field_width, precision, flags);
+			continue;
+
+		case 'n':
+			if (qualifier == 'l') {
+				long *ip = va_arg(args, long *);
+				*ip = (str - buf);
+			} else {
+				int *ip = va_arg(args, int *);
+				*ip = (str - buf);
+			}
+			continue;
+
+		case '%':
+			*str++ = '%';
+			continue;
+
+			/* integer number formats - set up the flags and "break" */
+		case 'o':
+			base = 8;
+			break;
+
+		case 'x':
+			flags |= SMALL;
+		case 'X':
+			base = 16;
+			break;
+
+		case 'd':
+		case 'i':
+			flags |= SIGN;
+		case 'u':
+			break;
+
+		default:
+			*str++ = '%';
+			if (*fmt)
+				*str++ = *fmt;
+			else
+				--fmt;
+			continue;
+		}
+		if (qualifier == 'l')
+			num = va_arg(args, unsigned long);
+		else if (qualifier == 'h') {
+			num = (unsigned short)va_arg(args, int);
+			if (flags & SIGN)
+				num = (short)num;
+		} else if (flags & SIGN)
+			num = va_arg(args, int);
+		else
+			num = va_arg(args, unsigned int);
+		str = number(str, num, base, field_width, precision, flags);
+	}
+	*str = '\0';
+	return str - buf;
+}
+
+int sprintf(char *buf, const char *fmt, ...)
+{
+	va_list args;
+	int i;
+
+	va_start(args, fmt);
+	i = vsprintf(buf, fmt, args);
+	va_end(args);
+	return i;
+}
+
+int printf(const char *fmt, ...)
+{
+	char printf_buf[1024];
+	va_list args;
+	int printed;
+
+	va_start(args, fmt);
+	printed = vsprintf(printf_buf, fmt, args);
+	va_end(args);
+
+	puts(printf_buf);
+
+	return printed;
+}
-- 
2.40.0.rc0.216.gc4246ad0f0-goog

[PATCH 5/8] KVM: selftests: Add vsprintf() to KVM selftests

[Aaron Lewis]

Add string formatting support to the guest by adding a local version
of vsprintf with no dependencies on LIBC.

There were some minor fix-ups needed to get it compiling in selftests:
 - isdigit() was added as a local helper.
 - boot.h was switch for test_util.h.
 - printf and sprintf were removed.  Support for printing will go
   through the ucall framework.

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 tools/testing/selftests/kvm/Makefile          |  1 +
 .../testing/selftests/kvm/include/test_util.h |  2 ++
 tools/testing/selftests/kvm/lib/printf.c      | 33 ++++---------------
 3 files changed, 9 insertions(+), 27 deletions(-)

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index cda631f10526..ce577b564616 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -23,6 +23,7 @@ LIBKVM += lib/guest_modes.c
 LIBKVM += lib/io.c
 LIBKVM += lib/kvm_util.c
 LIBKVM += lib/memstress.c
+LIBKVM += lib/printf.c
 LIBKVM += lib/rbtree.c
 LIBKVM += lib/sparsebit.c
 LIBKVM += lib/test_util.c
diff --git a/tools/testing/selftests/kvm/include/test_util.h b/tools/testing/selftests/kvm/include/test_util.h
index 80d6416f3012..261852598a4a 100644
--- a/tools/testing/selftests/kvm/include/test_util.h
+++ b/tools/testing/selftests/kvm/include/test_util.h
@@ -177,4 +177,6 @@ static inline uint32_t atoi_non_negative(const char *name, const char *num_str)
 	return num;
 }
 
+int vsprintf(char *buf, const char *fmt, va_list args);
+
 #endif /* SELFTEST_KVM_TEST_UTIL_H */
diff --git a/tools/testing/selftests/kvm/lib/printf.c b/tools/testing/selftests/kvm/lib/printf.c
index 1237beeb9540..d356e55cbc28 100644
--- a/tools/testing/selftests/kvm/lib/printf.c
+++ b/tools/testing/selftests/kvm/lib/printf.c
@@ -13,7 +13,12 @@
  *
  */
 
-#include "boot.h"
+#include "test_util.h"
+
+int isdigit(int ch)
+{
+	return (ch >= '0') && (ch <= '9');
+}
 
 static int skip_atoi(const char **s)
 {
@@ -279,29 +284,3 @@ int vsprintf(char *buf, const char *fmt, va_list args)
 	*str = '\0';
 	return str - buf;
 }
-
-int sprintf(char *buf, const char *fmt, ...)
-{
-	va_list args;
-	int i;
-
-	va_start(args, fmt);
-	i = vsprintf(buf, fmt, args);
-	va_end(args);
-	return i;
-}
-
-int printf(const char *fmt, ...)
-{
-	char printf_buf[1024];
-	va_list args;
-	int printed;
-
-	va_start(args, fmt);
-	printed = vsprintf(printf_buf, fmt, args);
-	va_end(args);
-
-	puts(printf_buf);
-
-	return printed;
-}
-- 
2.40.0.rc0.216.gc4246ad0f0-goog

[PATCH 6/8] KVM: selftests: Add additional pages to the guest to accommodate ucall

[Aaron Lewis]

Add additional pages to the guest to account for the number of pages
the ucall framework uses.

This is done in preparation for adding string formatting options to
the guest through ucall helpers.

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 tools/testing/selftests/kvm/include/ucall_common.h | 1 +
 tools/testing/selftests/kvm/lib/kvm_util.c         | 4 ++++
 tools/testing/selftests/kvm/lib/ucall_common.c     | 5 +++++
 3 files changed, 10 insertions(+)

diff --git a/tools/testing/selftests/kvm/include/ucall_common.h b/tools/testing/selftests/kvm/include/ucall_common.h
index 1a6aaef5ccae..0b1fde23729b 100644
--- a/tools/testing/selftests/kvm/include/ucall_common.h
+++ b/tools/testing/selftests/kvm/include/ucall_common.h
@@ -34,6 +34,7 @@ void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu);
 void ucall(uint64_t cmd, int nargs, ...);
 uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc);
 void ucall_init(struct kvm_vm *vm, vm_paddr_t mmio_gpa);
+int ucall_header_size(void);
 
 /*
  * Perform userspace call without any associated data.  This bare call avoids
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 3ea24a5f4c43..e1d6a2f40d2d 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -307,6 +307,7 @@ static uint64_t vm_nr_pages_required(enum vm_guest_mode mode,
 				     uint32_t nr_runnable_vcpus,
 				     uint64_t extra_mem_pages)
 {
+	uint64_t page_size = vm_guest_mode_params[mode].page_size;
 	uint64_t nr_pages;
 
 	TEST_ASSERT(nr_runnable_vcpus,
@@ -335,6 +336,9 @@ static uint64_t vm_nr_pages_required(enum vm_guest_mode mode,
 	 */
 	nr_pages += (nr_pages + extra_mem_pages) / PTES_PER_MIN_PAGE * 2;
 
+	/* Account for the number of pages needed by ucall. */
+	nr_pages += align_up(ucall_header_size(), page_size) / page_size;
+
 	return vm_adjust_num_guest_pages(mode, nr_pages);
 }
 
diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c
index 2f0e2ea941cc..b6a75858fe0d 100644
--- a/tools/testing/selftests/kvm/lib/ucall_common.c
+++ b/tools/testing/selftests/kvm/lib/ucall_common.c
@@ -11,6 +11,11 @@ struct ucall_header {
 	struct ucall ucalls[KVM_MAX_VCPUS];
 };
 
+int ucall_header_size(void)
+{
+	return sizeof(struct ucall_header);
+}
+
 /*
  * ucall_pool holds per-VM values (global data is duplicated by each VM), it
  * must not be accessed from host code.
-- 
2.40.0.rc0.216.gc4246ad0f0-goog

[PATCH 7/8] KVM: selftests: Add string formatting options to ucall

[Aaron Lewis]

Add more flexibility to guest debugging and testing by adding
GUEST_PRINTF() and GUEST_ASSERT_FMT() to the ucall framework.

A buffer to hold the formatted string was added to the ucall struct.
That allows the guest/host to avoid the problem of passing an
arbitrary number of parameters between themselves when resolving the
string.  Instead, the string is resolved in the guest then passed
back to the host to be logged.

The formatted buffer is set to 1024 bytes which increases the size
of the ucall struct.  As a result, this will increase the number of
pages requested for the guest.

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 .../selftests/kvm/include/ucall_common.h      | 19 +++++++++++++++++++
 .../testing/selftests/kvm/lib/ucall_common.c  | 19 +++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/tools/testing/selftests/kvm/include/ucall_common.h b/tools/testing/selftests/kvm/include/ucall_common.h
index 0b1fde23729b..2a4400b6761a 100644
--- a/tools/testing/selftests/kvm/include/ucall_common.h
+++ b/tools/testing/selftests/kvm/include/ucall_common.h
@@ -13,15 +13,18 @@ enum {
 	UCALL_NONE,
 	UCALL_SYNC,
 	UCALL_ABORT,
+	UCALL_PRINTF,
 	UCALL_DONE,
 	UCALL_UNHANDLED,
 };
 
 #define UCALL_MAX_ARGS 7
+#define UCALL_BUFFER_LEN 1024
 
 struct ucall {
 	uint64_t cmd;
 	uint64_t args[UCALL_MAX_ARGS];
+	char buffer[UCALL_BUFFER_LEN];
 
 	/* Host virtual address of this struct. */
 	struct ucall *hva;
@@ -32,6 +35,7 @@ void ucall_arch_do_ucall(vm_vaddr_t uc);
 void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu);
 
 void ucall(uint64_t cmd, int nargs, ...);
+void ucall_fmt(uint64_t cmd, const char *fmt, ...);
 uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc);
 void ucall_init(struct kvm_vm *vm, vm_paddr_t mmio_gpa);
 int ucall_header_size(void);
@@ -47,6 +51,7 @@ int ucall_header_size(void);
 #define GUEST_SYNC_ARGS(stage, arg1, arg2, arg3, arg4)	\
 				ucall(UCALL_SYNC, 6, "hello", stage, arg1, arg2, arg3, arg4)
 #define GUEST_SYNC(stage)	ucall(UCALL_SYNC, 2, "hello", stage)
+#define GUEST_PRINTF(fmt, _args...) ucall_fmt(UCALL_PRINTF, fmt, ##_args)
 #define GUEST_DONE()		ucall(UCALL_DONE, 0)
 
 enum guest_assert_builtin_args {
@@ -56,6 +61,18 @@ enum guest_assert_builtin_args {
 	GUEST_ASSERT_BUILTIN_NARGS
 };
 
+#define __GUEST_ASSERT_FMT(_condition, _condstr, format, _args...)	\
+do {									\
+	if (!(_condition))						\
+		ucall_fmt(UCALL_ABORT,					\
+		          "Failed guest assert: " _condstr		\
+		          " at %s:%ld\n  " format, 			\
+			  __FILE__, __LINE__, ##_args);			\
+} while (0)
+
+#define GUEST_ASSERT_FMT(_condition, format, _args...)	\
+	__GUEST_ASSERT_FMT(_condition, #_condition, format, ##_args)
+
 #define __GUEST_ASSERT(_condition, _condstr, _nargs, _args...)		\
 do {									\
 	if (!(_condition))						\
@@ -81,6 +98,8 @@ do {									\
 
 #define GUEST_ASSERT_EQ(a, b) __GUEST_ASSERT((a) == (b), #a " == " #b, 2, a, b)
 
+#define REPORT_GUEST_ASSERT_FMT(_ucall) TEST_FAIL("%s", _ucall.buffer)
+
 #define __REPORT_GUEST_ASSERT(_ucall, fmt, _args...)			\
 	TEST_FAIL("%s at %s:%ld\n" fmt,					\
 		  (const char *)(_ucall).args[GUEST_ERROR_STRING],	\
diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c
index b6a75858fe0d..92ebc5db1c41 100644
--- a/tools/testing/selftests/kvm/lib/ucall_common.c
+++ b/tools/testing/selftests/kvm/lib/ucall_common.c
@@ -54,7 +54,9 @@ static struct ucall *ucall_alloc(void)
 	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
 		if (!test_and_set_bit(i, ucall_pool->in_use)) {
 			uc = &ucall_pool->ucalls[i];
+			uc->cmd = UCALL_NONE;
 			memset(uc->args, 0, sizeof(uc->args));
+			memset(uc->buffer, 0, sizeof(uc->buffer));
 			return uc;
 		}
 	}
@@ -75,6 +77,23 @@ static void ucall_free(struct ucall *uc)
 	clear_bit(uc - ucall_pool->ucalls, ucall_pool->in_use);
 }
 
+void ucall_fmt(uint64_t cmd, const char *fmt, ...)
+{
+	struct ucall *uc;
+	va_list va;
+
+	uc = ucall_alloc();
+	uc->cmd = cmd;
+
+	va_start(va, fmt);
+	vsprintf(uc->buffer, fmt, va);
+	va_end(va);
+
+	ucall_arch_do_ucall((vm_vaddr_t)uc->hva);
+
+	ucall_free(uc);
+}
+
 void ucall(uint64_t cmd, int nargs, ...)
 {
 	struct ucall *uc;
-- 
2.40.0.rc0.216.gc4246ad0f0-goog

[PATCH 8/8] KVM: selftests: Add a selftest for guest prints and formatted asserts

[Aaron Lewis]

Add a selftest to demonstrate the use of prints and formatted asserts
in the guest through the ucall framework.

This test isn't intended to be accepted upstream and intentionally
asserts at the end to demonstrate GUEST_ASSERT_FMT().

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/x86_64/guest_print_test.c   | 100 ++++++++++++++++++
 2 files changed, 101 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/x86_64/guest_print_test.c

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index ce577b564616..8f7238da6b84 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -63,6 +63,7 @@ TEST_PROGS_x86_64 += x86_64/nx_huge_pages_test.sh
 TEST_GEN_PROGS_x86_64 = x86_64/cpuid_test
 TEST_GEN_PROGS_x86_64 += x86_64/cr4_cpuid_sync_test
 TEST_GEN_PROGS_x86_64 += x86_64/get_msr_index_features
+TEST_GEN_PROGS_x86_64 += x86_64/guest_print_test
 TEST_GEN_PROGS_x86_64 += x86_64/exit_on_emulation_failure_test
 TEST_GEN_PROGS_x86_64 += x86_64/fix_hypercall_test
 TEST_GEN_PROGS_x86_64 += x86_64/hyperv_clock
diff --git a/tools/testing/selftests/kvm/x86_64/guest_print_test.c b/tools/testing/selftests/kvm/x86_64/guest_print_test.c
new file mode 100644
index 000000000000..2ee16a9b3ebf
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86_64/guest_print_test.c
@@ -0,0 +1,100 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * A test for GUEST_PRINTF
+ *
+ * Copyright 2022, Google, Inc. and/or its affiliates.
+ */
+
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+
+#include "test_util.h"
+
+#include "kvm_util.h"
+#include "processor.h"
+
+static void guest_code(void)
+{
+	const char *s = "from the guest!";
+	uint64_t xcr0;
+
+	set_cr4(get_cr4() | X86_CR4_OSXSAVE);
+	xcr0 = xgetbv(0);
+
+	GUEST_PRINTF("XCR0 = 0x%lx\n", xcr0);
+
+	/*
+	 * Assert that XCR0 is at RESET, and that AVX-512 is not enabled.
+	 * LIBC loves to use AVX-512 instructions when doing string
+	 * formatting, which would be a pain to require of the guest as
+	 * a prerequisite of using print formatting functions.
+	 */
+	GUEST_ASSERT_FMT(xcr0 == XFEATURE_MASK_FP,
+			 "Expected an XCR0 value of 0x%lx, got 0x%lx instead.",
+			 XFEATURE_MASK_FP, xcr0);
+
+	/*
+	 * When %s is used in the string format the guest's version of printf
+	 * uses strnlen(), which will use AVX-512 instructions if routed
+	 * through the LIBC version.  To prevent that from happening strnlen()
+	 * has been added to the string_override functions.
+	 */
+	GUEST_PRINTF("Hello %s\n", s);
+
+	GUEST_SYNC(0);
+
+	/* Demonstrate GUEST_ASSERT_FMT by invoking it. */
+	xsetbv(0, xcr0 | XFEATURE_MASK_SSE);
+	xcr0 = xgetbv(0);
+	GUEST_ASSERT_FMT(xcr0 == XFEATURE_MASK_FP,
+			 "Expected an XCR0 value of 0x%lx, got 0x%lx instead.",
+			 XFEATURE_MASK_FP, xcr0);
+
+	GUEST_DONE();
+}
+
+int main(int argc, char *argv[])
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_run *run;
+	struct kvm_vm *vm;
+	struct ucall uc;
+
+	/* Tell stdout not to buffer its content */
+	setbuf(stdout, NULL);
+
+	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
+	run = vcpu->run;
+
+	while (1) {
+		vcpu_run(vcpu);
+
+		TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
+			    "Unexpected exit reason: %u (%s),\n",
+			    run->exit_reason,
+			    exit_reason_str(run->exit_reason));
+
+		switch (get_ucall(vcpu, &uc)) {
+		case UCALL_SYNC:
+			printf("Hello from the host!\n");
+			break;
+		case UCALL_PRINTF:
+			printf("%s", uc.buffer);
+			break;
+		case UCALL_ABORT:
+			REPORT_GUEST_ASSERT_FMT(uc);
+			break;
+		case UCALL_DONE:
+			goto done;
+		default:
+			TEST_FAIL("Unknown ucall %lu", uc.cmd);
+		}
+	}
+
+done:
+	kvm_vm_free(vm);
+	return 0;
+}
-- 
2.40.0.rc0.216.gc4246ad0f0-goog

Re: [PATCH 7/8] KVM: selftests: Add string formatting options to ucall

[Shaoqin Huang]

On 3/1/23 13:34, Aaron Lewis wrote:
> Add more flexibility to guest debugging and testing by adding
> GUEST_PRINTF() and GUEST_ASSERT_FMT() to the ucall framework.
> 
> A buffer to hold the formatted string was added to the ucall struct.
> That allows the guest/host to avoid the problem of passing an
> arbitrary number of parameters between themselves when resolving the
> string.  Instead, the string is resolved in the guest then passed
> back to the host to be logged.
> 
> The formatted buffer is set to 1024 bytes which increases the size
> of the ucall struct.  As a result, this will increase the number of
> pages requested for the guest.
> 
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---
>   .../selftests/kvm/include/ucall_common.h      | 19 +++++++++++++++++++
>   .../testing/selftests/kvm/lib/ucall_common.c  | 19 +++++++++++++++++++
>   2 files changed, 38 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/include/ucall_common.h b/tools/testing/selftests/kvm/include/ucall_common.h
> index 0b1fde23729b..2a4400b6761a 100644
> --- a/tools/testing/selftests/kvm/include/ucall_common.h
> +++ b/tools/testing/selftests/kvm/include/ucall_common.h
> @@ -13,15 +13,18 @@ enum {
>   	UCALL_NONE,
>   	UCALL_SYNC,
>   	UCALL_ABORT,
> +	UCALL_PRINTF,
>   	UCALL_DONE,
>   	UCALL_UNHANDLED,
>   };
>   
>   #define UCALL_MAX_ARGS 7
> +#define UCALL_BUFFER_LEN 1024
>   
>   struct ucall {
>   	uint64_t cmd;
>   	uint64_t args[UCALL_MAX_ARGS];
> +	char buffer[UCALL_BUFFER_LEN];
Hi Aaron,

A simple question, what if someone print too long in guest which exceed 
the UCALL_BUFFER_LEN, it seems buffer overflow will happen since 
vsprintf will not check the buffer length.

Just in case, someone may don't know the limit and print too long.

Thanks,
Shaoqin
>   
>   	/* Host virtual address of this struct. */
>   	struct ucall *hva;
> @@ -32,6 +35,7 @@ void ucall_arch_do_ucall(vm_vaddr_t uc);
>   void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu);
>   
>   void ucall(uint64_t cmd, int nargs, ...);
> +void ucall_fmt(uint64_t cmd, const char *fmt, ...);
>   uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc);
>   void ucall_init(struct kvm_vm *vm, vm_paddr_t mmio_gpa);
>   int ucall_header_size(void);
> @@ -47,6 +51,7 @@ int ucall_header_size(void);
>   #define GUEST_SYNC_ARGS(stage, arg1, arg2, arg3, arg4)	\
>   				ucall(UCALL_SYNC, 6, "hello", stage, arg1, arg2, arg3, arg4)
>   #define GUEST_SYNC(stage)	ucall(UCALL_SYNC, 2, "hello", stage)
> +#define GUEST_PRINTF(fmt, _args...) ucall_fmt(UCALL_PRINTF, fmt, ##_args)
>   #define GUEST_DONE()		ucall(UCALL_DONE, 0)
>   
>   enum guest_assert_builtin_args {
> @@ -56,6 +61,18 @@ enum guest_assert_builtin_args {
>   	GUEST_ASSERT_BUILTIN_NARGS
>   };
>   
> +#define __GUEST_ASSERT_FMT(_condition, _condstr, format, _args...)	\
> +do {									\
> +	if (!(_condition))						\
> +		ucall_fmt(UCALL_ABORT,					\
> +		          "Failed guest assert: " _condstr		\
> +		          " at %s:%ld\n  " format, 			\
> +			  __FILE__, __LINE__, ##_args);			\
> +} while (0)
> +
> +#define GUEST_ASSERT_FMT(_condition, format, _args...)	\
> +	__GUEST_ASSERT_FMT(_condition, #_condition, format, ##_args)
> +
>   #define __GUEST_ASSERT(_condition, _condstr, _nargs, _args...)		\
>   do {									\
>   	if (!(_condition))						\
> @@ -81,6 +98,8 @@ do {									\
>   
>   #define GUEST_ASSERT_EQ(a, b) __GUEST_ASSERT((a) == (b), #a " == " #b, 2, a, b)
>   
> +#define REPORT_GUEST_ASSERT_FMT(_ucall) TEST_FAIL("%s", _ucall.buffer)
> +
>   #define __REPORT_GUEST_ASSERT(_ucall, fmt, _args...)			\
>   	TEST_FAIL("%s at %s:%ld\n" fmt,					\
>   		  (const char *)(_ucall).args[GUEST_ERROR_STRING],	\
> diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c
> index b6a75858fe0d..92ebc5db1c41 100644
> --- a/tools/testing/selftests/kvm/lib/ucall_common.c
> +++ b/tools/testing/selftests/kvm/lib/ucall_common.c
> @@ -54,7 +54,9 @@ static struct ucall *ucall_alloc(void)
>   	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
>   		if (!test_and_set_bit(i, ucall_pool->in_use)) {
>   			uc = &ucall_pool->ucalls[i];
> +			uc->cmd = UCALL_NONE;
>   			memset(uc->args, 0, sizeof(uc->args));
> +			memset(uc->buffer, 0, sizeof(uc->buffer));
>   			return uc;
>   		}
>   	}
> @@ -75,6 +77,23 @@ static void ucall_free(struct ucall *uc)
>   	clear_bit(uc - ucall_pool->ucalls, ucall_pool->in_use);
>   }
>   
> +void ucall_fmt(uint64_t cmd, const char *fmt, ...)
> +{
> +	struct ucall *uc;
> +	va_list va;
> +
> +	uc = ucall_alloc();
> +	uc->cmd = cmd;
> +
> +	va_start(va, fmt);
> +	vsprintf(uc->buffer, fmt, va);
> +	va_end(va);
> +
> +	ucall_arch_do_ucall((vm_vaddr_t)uc->hva);
> +
> +	ucall_free(uc);
> +}
> +
>   void ucall(uint64_t cmd, int nargs, ...)
>   {
>   	struct ucall *uc;

Re: [PATCH 7/8] KVM: selftests: Add string formatting options to ucall

[Aaron Lewis]

On Wed, Mar 1, 2023 at 12:07 AM Shaoqin Huang <shahuang@redhat.com> wrote:
>
>
>
> On 3/1/23 13:34, Aaron Lewis wrote:
> > Add more flexibility to guest debugging and testing by adding
> > GUEST_PRINTF() and GUEST_ASSERT_FMT() to the ucall framework.
> >
> > A buffer to hold the formatted string was added to the ucall struct.
> > That allows the guest/host to avoid the problem of passing an
> > arbitrary number of parameters between themselves when resolving the
> > string.  Instead, the string is resolved in the guest then passed
> > back to the host to be logged.
> >
> > The formatted buffer is set to 1024 bytes which increases the size
> > of the ucall struct.  As a result, this will increase the number of
> > pages requested for the guest.
> >
> > Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> > ---
> >   .../selftests/kvm/include/ucall_common.h      | 19 +++++++++++++++++++
> >   .../testing/selftests/kvm/lib/ucall_common.c  | 19 +++++++++++++++++++
> >   2 files changed, 38 insertions(+)
> >
> > diff --git a/tools/testing/selftests/kvm/include/ucall_common.h b/tools/testing/selftests/kvm/include/ucall_common.h
> > index 0b1fde23729b..2a4400b6761a 100644
> > --- a/tools/testing/selftests/kvm/include/ucall_common.h
> > +++ b/tools/testing/selftests/kvm/include/ucall_common.h
> > @@ -13,15 +13,18 @@ enum {
> >       UCALL_NONE,
> >       UCALL_SYNC,
> >       UCALL_ABORT,
> > +     UCALL_PRINTF,
> >       UCALL_DONE,
> >       UCALL_UNHANDLED,
> >   };
> >
> >   #define UCALL_MAX_ARGS 7
> > +#define UCALL_BUFFER_LEN 1024
> >
> >   struct ucall {
> >       uint64_t cmd;
> >       uint64_t args[UCALL_MAX_ARGS];
> > +     char buffer[UCALL_BUFFER_LEN];
> Hi Aaron,
>
> A simple question, what if someone print too long in guest which exceed
> the UCALL_BUFFER_LEN, it seems buffer overflow will happen since
> vsprintf will not check the buffer length.
>
> Just in case, someone may don't know the limit and print too long.
>
> Thanks,
> Shaoqin
> >

In the followup I can check the length of the string written in
ucall_fmt() and return an overflow assert to the host instead if one
is detected.

> >       /* Host virtual address of this struct. */
> >       struct ucall *hva;
> > @@ -32,6 +35,7 @@ void ucall_arch_do_ucall(vm_vaddr_t uc);
> >   void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu);
> >
> >   void ucall(uint64_t cmd, int nargs, ...);
> > +void ucall_fmt(uint64_t cmd, const char *fmt, ...);
> >   uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc);
> >   void ucall_init(struct kvm_vm *vm, vm_paddr_t mmio_gpa);
> >   int ucall_header_size(void);
> > @@ -47,6 +51,7 @@ int ucall_header_size(void);
> >   #define GUEST_SYNC_ARGS(stage, arg1, arg2, arg3, arg4)      \
> >                               ucall(UCALL_SYNC, 6, "hello", stage, arg1, arg2, arg3, arg4)
> >   #define GUEST_SYNC(stage)   ucall(UCALL_SYNC, 2, "hello", stage)
> > +#define GUEST_PRINTF(fmt, _args...) ucall_fmt(UCALL_PRINTF, fmt, ##_args)
> >   #define GUEST_DONE()                ucall(UCALL_DONE, 0)
> >
> >   enum guest_assert_builtin_args {
> > @@ -56,6 +61,18 @@ enum guest_assert_builtin_args {
> >       GUEST_ASSERT_BUILTIN_NARGS
> >   };
> >
> > +#define __GUEST_ASSERT_FMT(_condition, _condstr, format, _args...)   \
> > +do {                                                                 \
> > +     if (!(_condition))                                              \
> > +             ucall_fmt(UCALL_ABORT,                                  \
> > +                       "Failed guest assert: " _condstr              \
> > +                       " at %s:%ld\n  " format,                      \
> > +                       __FILE__, __LINE__, ##_args);                 \
> > +} while (0)
> > +
> > +#define GUEST_ASSERT_FMT(_condition, format, _args...)       \
> > +     __GUEST_ASSERT_FMT(_condition, #_condition, format, ##_args)
> > +
> >   #define __GUEST_ASSERT(_condition, _condstr, _nargs, _args...)              \
> >   do {                                                                        \
> >       if (!(_condition))                                              \
> > @@ -81,6 +98,8 @@ do {                                                                        \
> >
> >   #define GUEST_ASSERT_EQ(a, b) __GUEST_ASSERT((a) == (b), #a " == " #b, 2, a, b)
> >
> > +#define REPORT_GUEST_ASSERT_FMT(_ucall) TEST_FAIL("%s", _ucall.buffer)
> > +
> >   #define __REPORT_GUEST_ASSERT(_ucall, fmt, _args...)                        \
> >       TEST_FAIL("%s at %s:%ld\n" fmt,                                 \
> >                 (const char *)(_ucall).args[GUEST_ERROR_STRING],      \
> > diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c
> > index b6a75858fe0d..92ebc5db1c41 100644
> > --- a/tools/testing/selftests/kvm/lib/ucall_common.c
> > +++ b/tools/testing/selftests/kvm/lib/ucall_common.c
> > @@ -54,7 +54,9 @@ static struct ucall *ucall_alloc(void)
> >       for (i = 0; i < KVM_MAX_VCPUS; ++i) {
> >               if (!test_and_set_bit(i, ucall_pool->in_use)) {
> >                       uc = &ucall_pool->ucalls[i];
> > +                     uc->cmd = UCALL_NONE;
> >                       memset(uc->args, 0, sizeof(uc->args));
> > +                     memset(uc->buffer, 0, sizeof(uc->buffer));
> >                       return uc;
> >               }
> >       }
> > @@ -75,6 +77,23 @@ static void ucall_free(struct ucall *uc)
> >       clear_bit(uc - ucall_pool->ucalls, ucall_pool->in_use);
> >   }
> >
> > +void ucall_fmt(uint64_t cmd, const char *fmt, ...)
> > +{
> > +     struct ucall *uc;
> > +     va_list va;
> > +
> > +     uc = ucall_alloc();
> > +     uc->cmd = cmd;
> > +
> > +     va_start(va, fmt);
> > +     vsprintf(uc->buffer, fmt, va);
> > +     va_end(va);
> > +
> > +     ucall_arch_do_ucall((vm_vaddr_t)uc->hva);
> > +
> > +     ucall_free(uc);
> > +}
> > +
> >   void ucall(uint64_t cmd, int nargs, ...)
> >   {
> >       struct ucall *uc;
>

Re: [PATCH 0/8] Add printf and formatted asserts in the guest

[Sean Christopherson]

On Wed, Mar 01, 2023, Aaron Lewis wrote:
> I say unintentionally assert because the test ends with a formatted
> assert firing.  This is intentional, and is meant to demonstrate the
> formatted assert.
> 
> That is one reason I don't really expect the selftest to be accepted with
> this series.  The other reason is it doesn't test anything in the kernel.

I don't have any objection to a selftest that tests selftests.  But it should
actually be a proper test and not something the user has to manually verify.
One thought would be to have the host side of the test pass in params to the
guest, and then have the the guest assert (or not) with a hardcoded format string.

Then on the host side, don't treat UCALL_ABORT as a failure and instead verify
that it fired when expected, and also provided the correct string, e.g. with a
strcmp() or whatever.  And do the same for GUEST_PRINTF/UCALL_PRINTF.

And it should be arch-agnostic, because at a galnce, the actual guts in patches 3-7
don't have an arch specific enabling.

E.g. something like this, and then use PRINTF_STRING and ASSERT_STRING in the
host to generate and verify the string.

#define PRINTF_STRING "Got params a = '0x%lx' and b = '0x%lx instead'"
#define ASSERT_STRING "Expected 0x%lx, got 0x%lx instead"

static void guest_code(uint64_t a, uint64_t b)
{
	GUEST_PRINTF(PRINTF_STRING, a, b);
	GUEST_ASSERT_FMT(a == b, ASSERT_FMT, a, b);
	GUEST_DONE();
}

> And if the selftest is not accepted then the first two patches can be
> omitted too.  The core of the series are patches 3-7.

As above, the first two patches should be omitted anyways, because guest_print_test.c
shouldn't be x86-specific.

Re: [PATCH 5/8] KVM: selftests: Add vsprintf() to KVM selftests

[Sean Christopherson]

On Wed, Mar 01, 2023, Aaron Lewis wrote:
> Add string formatting support to the guest by adding a local version
> of vsprintf with no dependencies on LIBC.

Heh, this confused me for a second.  Just squash this with the previous patch,
copying an entire file only to yank parts out is unnecessary and confusing.

> There were some minor fix-ups needed to get it compiling in selftests:
>  - isdigit() was added as a local helper.
>  - boot.h was switch for test_util.h.
>  - printf and sprintf were removed.  Support for printing will go
>    through the ucall framework.

As usual, just state what the patch does, not what you did in the past.

>  #endif /* SELFTEST_KVM_TEST_UTIL_H */
> diff --git a/tools/testing/selftests/kvm/lib/printf.c b/tools/testing/selftests/kvm/lib/printf.c
> index 1237beeb9540..d356e55cbc28 100644
> --- a/tools/testing/selftests/kvm/lib/printf.c
> +++ b/tools/testing/selftests/kvm/lib/printf.c
> @@ -13,7 +13,12 @@
>   *
>   */
>  
> -#include "boot.h"
> +#include "test_util.h"
> +
> +int isdigit(int ch)

static?

> +{
> +	return (ch >= '0') && (ch <= '9');
> +}

Re: [PATCH 4/8] KVM: selftests: Copy printf.c to KVM selftests

[Sean Christopherson]

On Wed, Mar 01, 2023, Aaron Lewis wrote:
> Add a local version of vsprintf() for the guest to use.
> 
> The file printf.c was lifted from arch/x86/boot/printf.c.

Is there by any shance a version of 
> +/*
> + * Oh, it's a waste of space, but oh-so-yummy for debugging.  This
> + * version of printf() does not include 64-bit support.  "Live with

But selftests are 64-bit only, at least on x86.

> +static char *number(char *str, long num, int base, int size, int precision,
> +		    int type)

Do we actually need a custom number()?  I.e. can we sub in a libc equivalent?
That would reduce the craziness of this file by more than a few degrees.

Re: [PATCH 6/8] KVM: selftests: Add additional pages to the guest to accommodate ucall

[Sean Christopherson]

On Wed, Mar 01, 2023, Aaron Lewis wrote:
> Add additional pages to the guest to account for the number of pages
> the ucall framework uses.
> 
> This is done in preparation for adding string formatting options to
> the guest through ucall helpers.
> 
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---
>  tools/testing/selftests/kvm/include/ucall_common.h | 1 +
>  tools/testing/selftests/kvm/lib/kvm_util.c         | 4 ++++
>  tools/testing/selftests/kvm/lib/ucall_common.c     | 5 +++++
>  3 files changed, 10 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/include/ucall_common.h b/tools/testing/selftests/kvm/include/ucall_common.h
> index 1a6aaef5ccae..0b1fde23729b 100644
> --- a/tools/testing/selftests/kvm/include/ucall_common.h
> +++ b/tools/testing/selftests/kvm/include/ucall_common.h
> @@ -34,6 +34,7 @@ void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu);
>  void ucall(uint64_t cmd, int nargs, ...);
>  uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc);
>  void ucall_init(struct kvm_vm *vm, vm_paddr_t mmio_gpa);
> +int ucall_header_size(void);

I like keeping the ucall_header definition in ucall_common.c, but we should hide
away the detail that it's a "header" (which doesn't even really make sense to me).

E.g. maybe ucall_nr_pages_required()?  And then obviously do the align_up() and
whatnot in the helper.

Re: [PATCH 7/8] KVM: selftests: Add string formatting options to ucall

[Sean Christopherson]

On Wed, Mar 01, 2023, Aaron Lewis wrote:
> Add more flexibility to guest debugging and testing by adding
> GUEST_PRINTF() and GUEST_ASSERT_FMT() to the ucall framework.
> 
> A buffer to hold the formatted string was added to the ucall struct.
> That allows the guest/host to avoid the problem of passing an
> arbitrary number of parameters between themselves when resolving the
> string.  Instead, the string is resolved in the guest then passed
> back to the host to be logged.
> 
> The formatted buffer is set to 1024 bytes which increases the size
> of the ucall struct.  As a result, this will increase the number of
> pages requested for the guest.

Why 1024?  I don't particuarly have an opinion, but some explanation of where this
magic number comes from would be helpful.
 
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---
>  .../selftests/kvm/include/ucall_common.h      | 19 +++++++++++++++++++
>  .../testing/selftests/kvm/lib/ucall_common.c  | 19 +++++++++++++++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/include/ucall_common.h b/tools/testing/selftests/kvm/include/ucall_common.h
> index 0b1fde23729b..2a4400b6761a 100644
> --- a/tools/testing/selftests/kvm/include/ucall_common.h
> +++ b/tools/testing/selftests/kvm/include/ucall_common.h
> @@ -13,15 +13,18 @@ enum {
>  	UCALL_NONE,
>  	UCALL_SYNC,
>  	UCALL_ABORT,
> +	UCALL_PRINTF,
>  	UCALL_DONE,
>  	UCALL_UNHANDLED,
>  };
>  
>  #define UCALL_MAX_ARGS 7
> +#define UCALL_BUFFER_LEN 1024
>  
>  struct ucall {
>  	uint64_t cmd;
>  	uint64_t args[UCALL_MAX_ARGS];
> +	char buffer[UCALL_BUFFER_LEN];
>  
>  	/* Host virtual address of this struct. */
>  	struct ucall *hva;
> @@ -32,6 +35,7 @@ void ucall_arch_do_ucall(vm_vaddr_t uc);
>  void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu);
>  
>  void ucall(uint64_t cmd, int nargs, ...);
> +void ucall_fmt(uint64_t cmd, const char *fmt, ...);
>  uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc);
>  void ucall_init(struct kvm_vm *vm, vm_paddr_t mmio_gpa);
>  int ucall_header_size(void);
> @@ -47,6 +51,7 @@ int ucall_header_size(void);
>  #define GUEST_SYNC_ARGS(stage, arg1, arg2, arg3, arg4)	\
>  				ucall(UCALL_SYNC, 6, "hello", stage, arg1, arg2, arg3, arg4)
>  #define GUEST_SYNC(stage)	ucall(UCALL_SYNC, 2, "hello", stage)
> +#define GUEST_PRINTF(fmt, _args...) ucall_fmt(UCALL_PRINTF, fmt, ##_args)
>  #define GUEST_DONE()		ucall(UCALL_DONE, 0)
>  
>  enum guest_assert_builtin_args {
> @@ -56,6 +61,18 @@ enum guest_assert_builtin_args {
>  	GUEST_ASSERT_BUILTIN_NARGS
>  };
>  
> +#define __GUEST_ASSERT_FMT(_condition, _condstr, format, _args...)	\
> +do {									\
> +	if (!(_condition))						\
> +		ucall_fmt(UCALL_ABORT,					\
> +		          "Failed guest assert: " _condstr		\
> +		          " at %s:%ld\n  " format, 			\

Please don't wrap strings, especially not in macros.  Just run past 80 chars if necessary.

> +			  __FILE__, __LINE__, ##_args);			\
> +} while (0)
> +
> +#define GUEST_ASSERT_FMT(_condition, format, _args...)	\
> +	__GUEST_ASSERT_FMT(_condition, #_condition, format, ##_args)
> +
>  #define __GUEST_ASSERT(_condition, _condstr, _nargs, _args...)		\
>  do {									\
>  	if (!(_condition))						\
> @@ -81,6 +98,8 @@ do {									\
>  
>  #define GUEST_ASSERT_EQ(a, b) __GUEST_ASSERT((a) == (b), #a " == " #b, 2, a, b)
>  
> +#define REPORT_GUEST_ASSERT_FMT(_ucall) TEST_FAIL("%s", _ucall.buffer)
> +
>  #define __REPORT_GUEST_ASSERT(_ucall, fmt, _args...)			\
>  	TEST_FAIL("%s at %s:%ld\n" fmt,					\
>  		  (const char *)(_ucall).args[GUEST_ERROR_STRING],	\
> diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c
> index b6a75858fe0d..92ebc5db1c41 100644
> --- a/tools/testing/selftests/kvm/lib/ucall_common.c
> +++ b/tools/testing/selftests/kvm/lib/ucall_common.c
> @@ -54,7 +54,9 @@ static struct ucall *ucall_alloc(void)
>  	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
>  		if (!test_and_set_bit(i, ucall_pool->in_use)) {
>  			uc = &ucall_pool->ucalls[i];
> +			uc->cmd = UCALL_NONE;

This is unnecessary, there's one caller and it immediately sets cmd.  If it's a
sticking point, force the caller to pass in @cmd and move the WRITE_ONCE() here.
Oh, and if you do that, put it in a separate patch.

>  			memset(uc->args, 0, sizeof(uc->args));
> +			memset(uc->buffer, 0, sizeof(uc->buffer));
>  			return uc;
>  		}

Re: [PATCH 4/8] KVM: selftests: Copy printf.c to KVM selftests

[Aaron Lewis]

On Thu, Mar 23, 2023, Sean Christopherson wrote:
> On Wed, Mar 01, 2023, Aaron Lewis wrote:
> > Add a local version of vsprintf() for the guest to use.
> > 
> > The file printf.c was lifted from arch/x86/boot/printf.c.
> 
> Is there by any shance a version of 
> > +/*
> > + * Oh, it's a waste of space, but oh-so-yummy for debugging.  This
> > + * version of printf() does not include 64-bit support.  "Live with
> 
> But selftests are 64-bit only, at least on x86.
> 

I think that's a legacy comment.  AFAICT this code supports 64-bit values.
I'll remove the comment to avoid confusion.

> > +static char *number(char *str, long num, int base, int size, int precision,
> > +		    int type)
> 
> Do we actually need a custom number()?  I.e. can we sub in a libc equivalent?
> That would reduce the craziness of this file by more than a few degrees.
> 

Yeah, I think we need it.  One of the biggest problems I'm trying to avoid
here is the use of LIBC in a guest.  Using it always seems to end poorly
because guests generally don't set up AVX-512 or a TLS segmet, nor should
they have to.  Calling into LIBC seems to require both of them too often,
so it seems like it's better to just avoid it.

Also, in working on v2 I upgraded vsprintf() to vsnprintf() which required
custom changes to number().

Re: [PATCH 5/8] KVM: selftests: Add vsprintf() to KVM selftests

[Aaron Lewis]

On Thu, Mar 23, 2023, Sean Christopherson wrote:
> On Wed, Mar 01, 2023, Aaron Lewis wrote:
> > Add string formatting support to the guest by adding a local version
> > of vsprintf with no dependencies on LIBC.
> 
> Heh, this confused me for a second.  Just squash this with the previous patch,
> copying an entire file only to yank parts out is unnecessary and confusing.
> 

Sure, I can squash them together.  I thought doing it this way would
make it easier to review because you have a diff against the original in
this series.  If that's not helpful there's no point in having it.

> > There were some minor fix-ups needed to get it compiling in selftests:
> >  - isdigit() was added as a local helper.
> >  - boot.h was switch for test_util.h.
> >  - printf and sprintf were removed.  Support for printing will go
> >    through the ucall framework.
> 
> As usual, just state what the patch does, not what you did in the past.
> 
> >  #endif /* SELFTEST_KVM_TEST_UTIL_H */
> > diff --git a/tools/testing/selftests/kvm/lib/printf.c b/tools/testing/selftests/kvm/lib/printf.c
> > index 1237beeb9540..d356e55cbc28 100644
> > --- a/tools/testing/selftests/kvm/lib/printf.c
> > +++ b/tools/testing/selftests/kvm/lib/printf.c
> > @@ -13,7 +13,12 @@
> >   *
> >   */
> >  
> > -#include "boot.h"
> > +#include "test_util.h"
> > +
> > +int isdigit(int ch)
> 
> static?
> 
> > +{
> > +	return (ch >= '0') && (ch <= '9');
> > +}
> 

Re: [PATCH 4/8] KVM: selftests: Copy printf.c to KVM selftests

[Sean Christopherson]

On Mon, Apr 17, 2023, Aaron Lewis wrote:
> On Thu, Mar 23, 2023, Sean Christopherson wrote:
> > > +static char *number(char *str, long num, int base, int size, int precision,
> > > +		    int type)
> > 
> > Do we actually need a custom number()?  I.e. can we sub in a libc equivalent?
> > That would reduce the craziness of this file by more than a few degrees.
> 
> Yeah, I think we need it.  One of the biggest problems I'm trying to avoid
> here is the use of LIBC in a guest.  Using it always seems to end poorly
> because guests generally don't set up AVX-512 or a TLS segmet, nor should
> they have to.  Calling into LIBC seems to require both of them too often,
> so it seems like it's better to just avoid it.

True, we'd probably end up in a world of hurt.

I was going to suggest copy+pasting from a different source, e.g. musl, in the
hopes of reducing the crazy by a degree, but after looking at the musl source,
that's a terrible idea :-)

And copying from the kernel has the advantage of keeping any bugs/quirks that
users may be expecting and/or relying on.

Re: [PATCH 5/8] KVM: selftests: Add vsprintf() to KVM selftests

[Sean Christopherson]

On Mon, Apr 17, 2023, Aaron Lewis wrote:
> On Thu, Mar 23, 2023, Sean Christopherson wrote:
> > On Wed, Mar 01, 2023, Aaron Lewis wrote:
> > > Add string formatting support to the guest by adding a local version
> > > of vsprintf with no dependencies on LIBC.
> > 
> > Heh, this confused me for a second.  Just squash this with the previous patch,
> > copying an entire file only to yank parts out is unnecessary and confusing.
> > 
> 
> Sure, I can squash them together.  I thought doing it this way would
> make it easier to review because you have a diff against the original in
> this series.  If that's not helpful there's no point in having it.

The problem is that including the original first means reviewing the entirety of
the original patch in the context of KVM selftests.

Re: [PATCH 4/8] KVM: selftests: Copy printf.c to KVM selftests

[Andrew Jones]

On Tue, Apr 18, 2023 at 08:03:53AM -0700, Sean Christopherson wrote:
> On Mon, Apr 17, 2023, Aaron Lewis wrote:
> > On Thu, Mar 23, 2023, Sean Christopherson wrote:
> > > > +static char *number(char *str, long num, int base, int size, int precision,
> > > > +		    int type)
> > > 
> > > Do we actually need a custom number()?  I.e. can we sub in a libc equivalent?
> > > That would reduce the craziness of this file by more than a few degrees.
> > 
> > Yeah, I think we need it.  One of the biggest problems I'm trying to avoid
> > here is the use of LIBC in a guest.  Using it always seems to end poorly
> > because guests generally don't set up AVX-512 or a TLS segmet, nor should
> > they have to.  Calling into LIBC seems to require both of them too often,
> > so it seems like it's better to just avoid it.
> 
> True, we'd probably end up in a world of hurt.
> 
> I was going to suggest copy+pasting from a different source, e.g. musl, in the
> hopes of reducing the crazy by a degree, but after looking at the musl source,
> that's a terrible idea :-)
> 
> And copying from the kernel has the advantage of keeping any bugs/quirks that
> users may be expecting and/or relying on.

What about trying to use tools/include/nolibc/? Maybe we could provide our
own tools/include/nolibc/arch-*.h files where the my_syscall* macros get
implemented with ucalls, and then the ucalls would implement the syscalls,
possibly just forwarding the parameters to real syscalls. We can implement
copy_from/to_guest() functions to deal with pointer parameters.

Thanks,
drew

Re: [PATCH 4/8] KVM: selftests: Copy printf.c to KVM selftests

[Sean Christopherson]

On Tue, Apr 18, 2023, Andrew Jones wrote:
> On Tue, Apr 18, 2023 at 08:03:53AM -0700, Sean Christopherson wrote:
> > On Mon, Apr 17, 2023, Aaron Lewis wrote:
> > > On Thu, Mar 23, 2023, Sean Christopherson wrote:
> > > > > +static char *number(char *str, long num, int base, int size, int precision,
> > > > > +		    int type)
> > > > 
> > > > Do we actually need a custom number()?  I.e. can we sub in a libc equivalent?
> > > > That would reduce the craziness of this file by more than a few degrees.
> > > 
> > > Yeah, I think we need it.  One of the biggest problems I'm trying to avoid
> > > here is the use of LIBC in a guest.  Using it always seems to end poorly
> > > because guests generally don't set up AVX-512 or a TLS segmet, nor should
> > > they have to.  Calling into LIBC seems to require both of them too often,
> > > so it seems like it's better to just avoid it.
> > 
> > True, we'd probably end up in a world of hurt.
> > 
> > I was going to suggest copy+pasting from a different source, e.g. musl, in the
> > hopes of reducing the crazy by a degree, but after looking at the musl source,
> > that's a terrible idea :-)
> > 
> > And copying from the kernel has the advantage of keeping any bugs/quirks that
> > users may be expecting and/or relying on.
> 
> What about trying to use tools/include/nolibc/? Maybe we could provide our
> own tools/include/nolibc/arch-*.h files where the my_syscall* macros get
> implemented with ucalls, and then the ucalls would implement the syscalls,
> possibly just forwarding the parameters to real syscalls. We can implement
> copy_from/to_guest() functions to deal with pointer parameters.

Hmm, I was going to say that pulling in nolibc would conflict with the host side's
need for an actual libc, but I think we could solve that conundrum by putting
ucall_fmt() in a dedicated file and compiling it separately, a la string_override.c.

However, I don't think we'd want to override my_syscall to do a ucall.  If I'm
reading the code correctly, that would trigger a ucall after every escape sequence,
which isn't what we want, expecially for a GUEST_ASSERT.

That's solvable by having my_syscall3() be a memcpy() to the buffer provided by
KVM's guest-side vsprintf(), but that still leaves the question of whether or not
taking a dependency on nolibc.h would be a net positive.

E.g. pulling in nolibc.h as-is would well and truly put ucall_fmt.c (or whatever
it's called) into its own world, e.g. it would end up with different typedefs for
all basic types.  Those shouldn't cause problems, but it'd be a weird setup.  And
I don't think we can rule out the possibility of the nolibc dependency causing
subtle problems, e.g. what will happen when linking due to both nolibc and
string_override.c defining globally visible mem{cmp,cpy,set}() functions.

Another minor issue is that nolibc's vfprintf() handles a subset of escapes compared
to the kernel's vsprintf().  That probably won't be a big deal in practice, but it's
again a potential maintenance concern for us in the future.

I'm definitely torn.  As much as I dislike the idea of copy+pasting mode code into
KVM selftests, I think pulling in nolibc would bring its own set of problems.

My vote is probably to copy+paste, at least for the initial implementation.  Being
able to do the equivalent of printf() in the guest would be a huge improvement for
debugging and triaging selftests, i.e. is something I would like to see landed
fairly quickly.  Copy+pasting seems like it gives us the fastest path forward,
e.g. doesn't risk getting bogged down with weird linker errors and whatnot.

Re: [PATCH 4/8] KVM: selftests: Copy printf.c to KVM selftests

[Andrew Jones]

On Thu, Apr 20, 2023 at 10:50:07AM -0700, Sean Christopherson wrote:
> On Tue, Apr 18, 2023, Andrew Jones wrote:
> > On Tue, Apr 18, 2023 at 08:03:53AM -0700, Sean Christopherson wrote:
> > > On Mon, Apr 17, 2023, Aaron Lewis wrote:
> > > > On Thu, Mar 23, 2023, Sean Christopherson wrote:
> > > > > > +static char *number(char *str, long num, int base, int size, int precision,
> > > > > > +		    int type)
> > > > > 
> > > > > Do we actually need a custom number()?  I.e. can we sub in a libc equivalent?
> > > > > That would reduce the craziness of this file by more than a few degrees.
> > > > 
> > > > Yeah, I think we need it.  One of the biggest problems I'm trying to avoid
> > > > here is the use of LIBC in a guest.  Using it always seems to end poorly
> > > > because guests generally don't set up AVX-512 or a TLS segmet, nor should
> > > > they have to.  Calling into LIBC seems to require both of them too often,
> > > > so it seems like it's better to just avoid it.
> > > 
> > > True, we'd probably end up in a world of hurt.
> > > 
> > > I was going to suggest copy+pasting from a different source, e.g. musl, in the
> > > hopes of reducing the crazy by a degree, but after looking at the musl source,
> > > that's a terrible idea :-)
> > > 
> > > And copying from the kernel has the advantage of keeping any bugs/quirks that
> > > users may be expecting and/or relying on.
> > 
> > What about trying to use tools/include/nolibc/? Maybe we could provide our
> > own tools/include/nolibc/arch-*.h files where the my_syscall* macros get
> > implemented with ucalls, and then the ucalls would implement the syscalls,
> > possibly just forwarding the parameters to real syscalls. We can implement
> > copy_from/to_guest() functions to deal with pointer parameters.
> 
> Hmm, I was going to say that pulling in nolibc would conflict with the host side's
> need for an actual libc, but I think we could solve that conundrum by putting
> ucall_fmt() in a dedicated file and compiling it separately, a la string_override.c.
> 
> However, I don't think we'd want to override my_syscall to do a ucall.  If I'm
> reading the code correctly, that would trigger a ucall after every escape sequence,
> which isn't what we want, expecially for a GUEST_ASSERT.
> 
> That's solvable by having my_syscall3() be a memcpy() to the buffer provided by
> KVM's guest-side vsprintf(), but that still leaves the question of whether or not
> taking a dependency on nolibc.h would be a net positive.
> 
> E.g. pulling in nolibc.h as-is would well and truly put ucall_fmt.c (or whatever
> it's called) into its own world, e.g. it would end up with different typedefs for
> all basic types.  Those shouldn't cause problems, but it'd be a weird setup.  And
> I don't think we can rule out the possibility of the nolibc dependency causing
> subtle problems, e.g. what will happen when linking due to both nolibc and
> string_override.c defining globally visible mem{cmp,cpy,set}() functions.
> 
> Another minor issue is that nolibc's vfprintf() handles a subset of escapes compared
> to the kernel's vsprintf().  That probably won't be a big deal in practice, but it's
> again a potential maintenance concern for us in the future.
> 
> I'm definitely torn.  As much as I dislike the idea of copy+pasting mode code into
> KVM selftests, I think pulling in nolibc would bring its own set of problems.
> 
> My vote is probably to copy+paste, at least for the initial implementation.  Being
> able to do the equivalent of printf() in the guest would be a huge improvement for
> debugging and triaging selftests, i.e. is something I would like to see landed
> fairly quickly.

Fully agree.

> Copy+pasting seems like it gives us the fastest path forward,
> e.g. doesn't risk getting bogged down with weird linker errors and whatnot.

Works for me.

Thanks,
drew