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

[PATCH v3 0/5] 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 will allow for better and easier guest debugging.

v2 -> v3
 - s/kvm_vsnprintf/guest_vsnprintf/ [Sean]
 - GUEST_ASSERT on error in guest_vsnprintf() [Sean]
 - Added "Fixes" tag to patch #3 [Sean]
 - Removed memset to zero ucall.buffer to save cycles [Sean]
 - Added REPORT_GUEST_PRINTF() for the host [Sean]
 - Removed ucall_fmt2() and squashed it into __GUEST_ASSERT_FMT [Sean]
 - Fixed stack overflow in guest_print_test Sean called out
 - Refactored test_limits() in guest_print_test to account for updates

v1 -> v2:
 - Added a proper selftest [Sean]
 - Added support for snprintf [Shaoqin]
 - Added ucall_nr_pages_required() [Sean]
 - Added ucall_fmt2() for GUEST_ASSERT_FMT()
 - Dropped the original version of printf.c [Sean]
 - Dropped patches 1-2 and 8 [Sean]

Aaron Lewis (5):
  KVM: selftests: Add strnlen() to the string overrides
  KVM: selftests: Add guest_snprintf() 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/guest_print_test.c  | 222 +++++++++++++
 .../testing/selftests/kvm/include/test_util.h |   3 +
 .../selftests/kvm/include/ucall_common.h      |  24 ++
 .../testing/selftests/kvm/lib/guest_sprintf.c | 307 ++++++++++++++++++
 tools/testing/selftests/kvm/lib/kvm_util.c    |   4 +
 .../selftests/kvm/lib/string_override.c       |   9 +
 .../testing/selftests/kvm/lib/ucall_common.c  |  22 ++
 8 files changed, 594 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/guest_print_test.c
 create mode 100644 tools/testing/selftests/kvm/lib/guest_sprintf.c

-- 
2.41.0.rc0.172.g3f132b7071-goog

[PATCH v3 1/5] 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 ee41ff0c5a86..adbf94cbc67e 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -200,6 +200,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.41.0.rc0.172.g3f132b7071-goog

[PATCH v3 2/5] KVM: selftests: Add guest_snprintf() to KVM selftests

[Aaron Lewis]

Add a local version of guest_snprintf() for use in the guest.

Having a local copy allows the guest access to string formatting
options without dependencies on LIBC.  LIBC is problematic because
it heavily relies on both AVX-512 instructions and a TLS, neither of
which are guaranteed to be set up in the guest.

The file guest_sprintf.c was lifted from arch/x86/boot/printf.c and
adapted to work in the guest, including the addition of buffer length.
I.e. s/sprintf/snprintf/

The functions where prefixed with "guest_" to allow guests to
explicitly call them.

A string formatted by this function is expected to succeed or die.  If
something goes wrong during the formatting process a GUEST_ASSERT()
will be thrown.

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../testing/selftests/kvm/include/test_util.h |   3 +
 .../testing/selftests/kvm/lib/guest_sprintf.c | 307 ++++++++++++++++++
 3 files changed, 311 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/lib/guest_sprintf.c

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index adbf94cbc67e..efbe7e6d8f9b 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/guest_sprintf.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 a6e9f215ce70..89ecacec328a 100644
--- a/tools/testing/selftests/kvm/include/test_util.h
+++ b/tools/testing/selftests/kvm/include/test_util.h
@@ -186,4 +186,7 @@ static inline uint32_t atoi_non_negative(const char *name, const char *num_str)
 	return num;
 }
 
+int guest_vsnprintf(char *buf, int n, const char *fmt, va_list args);
+int guest_snprintf(char *buf, int n, const char *fmt, ...);
+
 #endif /* SELFTEST_KVM_TEST_UTIL_H */
diff --git a/tools/testing/selftests/kvm/lib/guest_sprintf.c b/tools/testing/selftests/kvm/lib/guest_sprintf.c
new file mode 100644
index 000000000000..c4a69d8aeb68
--- /dev/null
+++ b/tools/testing/selftests/kvm/lib/guest_sprintf.c
@@ -0,0 +1,307 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include "test_util.h"
+#include "kvm_util.h"
+#include "ucall_common.h"
+
+#define APPEND_BUFFER_SAFE(str, end, v) \
+do {					\
+	GUEST_ASSERT(str < end);	\
+	*str++ = (v);			\
+} while (0)
+
+static int isdigit(int ch)
+{
+	return (ch >= '0') && (ch <= '9');
+}
+
+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 = ((uint64_t) n) % (uint32_t) base;	\
+	n = ((uint64_t) n) / (uint32_t) base;		\
+	__res;						\
+})
+
+static char *number(char *str, const char *end, 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)
+			APPEND_BUFFER_SAFE(str, end, ' ');
+	if (sign)
+		APPEND_BUFFER_SAFE(str, end, sign);
+	if (type & SPECIAL) {
+		if (base == 8)
+			APPEND_BUFFER_SAFE(str, end, '0');
+		else if (base == 16) {
+			APPEND_BUFFER_SAFE(str, end, '0');
+			APPEND_BUFFER_SAFE(str, end, 'x');
+		}
+	}
+	if (!(type & LEFT))
+		while (size-- > 0)
+			APPEND_BUFFER_SAFE(str, end, c);
+	while (i < precision--)
+		APPEND_BUFFER_SAFE(str, end, '0');
+	while (i-- > 0)
+		APPEND_BUFFER_SAFE(str, end, tmp[i]);
+	while (size-- > 0)
+		APPEND_BUFFER_SAFE(str, end, ' ');
+
+	return str;
+}
+
+int guest_vsnprintf(char *buf, int n, const char *fmt, va_list args)
+{
+	char *str, *end;
+	const char *s;
+	uint64_t num;
+	int i, base;
+	int len;
+
+	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 */
+
+	end = buf + n;
+	GUEST_ASSERT(buf < end);
+	GUEST_ASSERT(n > 0);
+
+	for (str = buf; *fmt; ++fmt) {
+		if (*fmt != '%') {
+			APPEND_BUFFER_SAFE(str, end, *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)
+					APPEND_BUFFER_SAFE(str, end, ' ');
+			APPEND_BUFFER_SAFE(str, end,
+					    (uint8_t)va_arg(args, int));
+			while (--field_width > 0)
+				APPEND_BUFFER_SAFE(str, end, ' ');
+			continue;
+
+		case 's':
+			s = va_arg(args, char *);
+			len = strnlen(s, precision);
+
+			if (!(flags & LEFT))
+				while (len < field_width--)
+					APPEND_BUFFER_SAFE(str, end, ' ');
+			for (i = 0; i < len; ++i)
+				APPEND_BUFFER_SAFE(str, end, *s++);
+			while (len < field_width--)
+				APPEND_BUFFER_SAFE(str, end, ' ');
+			continue;
+
+		case 'p':
+			if (field_width == -1) {
+				field_width = 2 * sizeof(void *);
+				flags |= SPECIAL | SMALL | ZEROPAD;
+			}
+			str = number(str, end,
+				     (uint64_t)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 '%':
+			APPEND_BUFFER_SAFE(str, end, '%');
+			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:
+			APPEND_BUFFER_SAFE(str, end, '%');
+			if (*fmt)
+				APPEND_BUFFER_SAFE(str, end, *fmt);
+			else
+				--fmt;
+			continue;
+		}
+		if (qualifier == 'l')
+			num = va_arg(args, uint64_t);
+		else if (qualifier == 'h') {
+			num = (uint16_t)va_arg(args, int);
+			if (flags & SIGN)
+				num = (int16_t)num;
+		} else if (flags & SIGN)
+			num = va_arg(args, int);
+		else
+			num = va_arg(args, uint32_t);
+		str = number(str, end, num, base, field_width, precision, flags);
+	}
+
+	GUEST_ASSERT(str < end);
+	*str = '\0';
+	return str - buf;
+}
+
+int guest_snprintf(char *buf, int n, const char *fmt, ...)
+{
+	va_list va;
+	int len;
+
+	va_start(va, fmt);
+	len = guest_vsnprintf(buf, n, fmt, va);
+	va_end(va);
+
+	return len;
+}
-- 
2.41.0.rc0.172.g3f132b7071-goog

[PATCH v3 3/5] 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 headers need.  The only reason things worked before is the
ucall headers are fairly small.  If they were ever to increase in
size the guest could run out of memory.

This is done in preparation for adding string formatting options to
the guest through the ucall framework which increases the size of
the ucall headers.

Fixes: 426729b2cf2e ("KVM: selftests: Add ucall pool based implementation")
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..bcbb362aa77f 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_nr_pages_required(uint64_t page_size);
 
 /*
  * 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 298c4372fb1a..80b3df2a79e6 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -312,6 +312,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,
@@ -340,6 +341,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 += ucall_nr_pages_required(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..77ada362273d 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_nr_pages_required(uint64_t page_size)
+{
+	return align_up(sizeof(struct ucall_header), page_size) / page_size;
+}
+
 /*
  * ucall_pool holds per-VM values (global data is duplicated by each VM), it
  * must not be accessed from host code.
-- 
2.41.0.rc0.172.g3f132b7071-goog

[PATCH v3 4/5] 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.

The buffer size was chosen to accommodate most use cases, and based on
similar usage.  E.g. printf() uses the same size buffer in
arch/x86/boot/printf.c.

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

diff --git a/tools/testing/selftests/kvm/include/ucall_common.h b/tools/testing/selftests/kvm/include/ucall_common.h
index bcbb362aa77f..b4f4c88e8d84 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_nr_pages_required(uint64_t page_size);
@@ -47,8 +51,11 @@ int ucall_nr_pages_required(uint64_t page_size);
 #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)
 
+#define REPORT_GUEST_PRINTF(_ucall) pr_info("%s", _ucall.buffer)
+
 enum guest_assert_builtin_args {
 	GUEST_ERROR_STRING,
 	GUEST_FILE,
@@ -56,6 +63,20 @@ enum guest_assert_builtin_args {
 	GUEST_ASSERT_BUILTIN_NARGS
 };
 
+#define __GUEST_ASSERT_FMT(_condition, _str, _fmt, _args...)			\
+do {										\
+	char fmt[UCALL_BUFFER_LEN];						\
+										\
+	if (!(_condition)) {							\
+		guest_snprintf(fmt, sizeof(fmt), "%s\n  %s",			\
+			     "Failed guest assert: " _str " at %s:%ld", _fmt);	\
+		ucall_fmt(UCALL_ABORT, fmt, __FILE__, __LINE__, ##_args);	\
+	}									\
+} while (0)
+
+#define GUEST_ASSERT_FMT(_condition, _fmt, _args...)	\
+	__GUEST_ASSERT_FMT(_condition, #_condition, _fmt, ##_args)
+
 #define __GUEST_ASSERT(_condition, _condstr, _nargs, _args...)		\
 do {									\
 	if (!(_condition))						\
@@ -81,6 +102,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 77ada362273d..b507db91139b 100644
--- a/tools/testing/selftests/kvm/lib/ucall_common.c
+++ b/tools/testing/selftests/kvm/lib/ucall_common.c
@@ -75,6 +75,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);
+	guest_vsnprintf(uc->buffer, UCALL_BUFFER_LEN, 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.41.0.rc0.172.g3f132b7071-goog

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

[Aaron Lewis]

The purpose of this test is to exercise the various features in KVM's
local snprintf() and compare them to LIBC's snprintf() to ensure they
behave the same.

This is not an exhaustive test.  KVM's local snprintf() does not
implement all the features LIBC does, e.g. KVM's local snprintf() does
not support floats or doubles, so testing for those features were
excluded.

Testing was added for the features that are expected to work to
support a minimal version of printf() in the guest.

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

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index efbe7e6d8f9b..85c35ea10ffd 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -122,6 +122,7 @@ TEST_GEN_PROGS_x86_64 += access_tracking_perf_test
 TEST_GEN_PROGS_x86_64 += demand_paging_test
 TEST_GEN_PROGS_x86_64 += dirty_log_test
 TEST_GEN_PROGS_x86_64 += dirty_log_perf_test
+TEST_GEN_PROGS_x86_64 += guest_print_test
 TEST_GEN_PROGS_x86_64 += hardware_disable_test
 TEST_GEN_PROGS_x86_64 += kvm_create_max_vcpus
 TEST_GEN_PROGS_x86_64 += kvm_page_table_test
diff --git a/tools/testing/selftests/kvm/guest_print_test.c b/tools/testing/selftests/kvm/guest_print_test.c
new file mode 100644
index 000000000000..7dd0a36e23c2
--- /dev/null
+++ b/tools/testing/selftests/kvm/guest_print_test.c
@@ -0,0 +1,222 @@
+// 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"
+
+struct guest_vals {
+	uint64_t a;
+	uint64_t b;
+	uint64_t type;
+};
+
+struct guest_vals vals;
+
+/* GUEST_PRINTF()/GUEST_ASSERT_FMT() does not support float or double. */
+#define TYPE_LIST					\
+TYPE(test_type_i64,  I64,  "%ld",   int64_t)		\
+TYPE(test_type_u64,  U64u, "%lu",   uint64_t)		\
+TYPE(test_type_x64,  U64x, "0x%lx", uint64_t)		\
+TYPE(test_type_X64,  U64X, "0x%lX", uint64_t)		\
+TYPE(test_type_u32,  U32u, "%u",    uint32_t)		\
+TYPE(test_type_x32,  U32x, "0x%x",  uint32_t)		\
+TYPE(test_type_X32,  U32X, "0x%X",  uint32_t)		\
+TYPE(test_type_int,  INT,  "%d",    int)		\
+TYPE(test_type_char, CHAR, "%c",    char)		\
+TYPE(test_type_str,  STR,  "'%s'",  const char *)	\
+TYPE(test_type_ptr,  PTR,  "%p",    uintptr_t)
+
+enum args_type {
+#define TYPE(fn, ext, fmt_t, T) TYPE_##ext,
+	TYPE_LIST
+#undef TYPE
+};
+
+static void run_test(struct kvm_vcpu *vcpu, const char *expected_printf,
+		     const char *expected_assert);
+
+#define BUILD_TYPE_STRINGS_AND_HELPER(fn, ext, fmt_t, T)		     \
+const char *PRINTF_FMT_##ext = "Got params a = " fmt_t " and b = " fmt_t;    \
+const char *ASSERT_FMT_##ext = "Expected " fmt_t ", got " fmt_t " instead";  \
+static void fn(struct kvm_vcpu *vcpu, T a, T b)				     \
+{									     \
+	char expected_printf[UCALL_BUFFER_LEN];				     \
+	char expected_assert[UCALL_BUFFER_LEN];				     \
+									     \
+	snprintf(expected_printf, UCALL_BUFFER_LEN, PRINTF_FMT_##ext, a, b); \
+	snprintf(expected_assert, UCALL_BUFFER_LEN, ASSERT_FMT_##ext, a, b); \
+	vals = (struct guest_vals){ (uint64_t)a, (uint64_t)b, TYPE_##ext };  \
+	sync_global_to_guest(vcpu->vm, vals);				     \
+	run_test(vcpu, expected_printf, expected_assert);		     \
+}
+
+#define TYPE(fn, ext, fmt_t, T) \
+		BUILD_TYPE_STRINGS_AND_HELPER(fn, ext, fmt_t, T)
+	TYPE_LIST
+#undef TYPE
+
+static void guest_code(void)
+{
+	while (1) {
+		switch (vals.type) {
+#define TYPE(fn, ext, fmt_t, T)							\
+		case TYPE_##ext:						\
+			GUEST_PRINTF(PRINTF_FMT_##ext, vals.a, vals.b);		\
+			GUEST_ASSERT_FMT(vals.a == vals.b,			\
+					 ASSERT_FMT_##ext, vals.a, vals.b);	\
+			break;
+	TYPE_LIST
+#undef TYPE
+		default:
+			GUEST_SYNC(vals.type);
+		}
+
+		GUEST_DONE();
+	}
+}
+
+/*
+ * Unfortunately this gets a little messy because 'assert_msg' doesn't
+ * just contains the matching string, it also contains additional assert
+ * info.  Fortunately the part that matches should be at the very end of
+ * 'assert_msg'.
+ */
+static void ucall_abort(const char *assert_msg, const char *expected_assert_msg)
+{
+	int len_str = strlen(assert_msg);
+	int len_substr = strlen(expected_assert_msg);
+	int offset = len_str - len_substr;
+
+	TEST_ASSERT(len_substr <= len_str,
+		    "Expected to find a substring, len_str: %d, len_substr: %d",
+		    len_str, len_substr);
+
+	TEST_ASSERT(strcmp(&assert_msg[offset], expected_assert_msg) == 0,
+		    "Unexpected mismatch. Expected: '%s', got: '%s'",
+		    expected_assert_msg, &assert_msg[offset]);
+}
+
+static void run_test(struct kvm_vcpu *vcpu, const char *expected_printf,
+		     const char *expected_assert)
+{
+	struct kvm_run *run = vcpu->run;
+	struct ucall uc;
+
+	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:
+			TEST_FAIL("Unknown 'args_type' = %lu", uc.args[1]);
+			break;
+		case UCALL_PRINTF:
+			TEST_ASSERT(strcmp(uc.buffer, expected_printf) == 0,
+				    "Unexpected mismatch. Expected: '%s', got: '%s'",
+				    expected_printf, uc.buffer);
+			break;
+		case UCALL_ABORT:
+			ucall_abort(uc.buffer, expected_assert);
+			break;
+		case UCALL_DONE:
+			return;
+		default:
+			TEST_FAIL("Unknown ucall %lu", uc.cmd);
+		}
+	}
+}
+
+static void guest_code_limits(void)
+{
+	char test_str[UCALL_BUFFER_LEN + 10];
+
+	memset(test_str, 'a', sizeof(test_str));
+	test_str[sizeof(test_str) - 1] = 0;
+
+	GUEST_PRINTF("%s", test_str);
+}
+
+static void test_limits(void)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_run *run;
+	struct kvm_vm *vm;
+	struct ucall uc;
+
+	vm = vm_create_with_one_vcpu(&vcpu, guest_code_limits);
+	run = vcpu->run;
+	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));
+
+	TEST_ASSERT(get_ucall(vcpu, &uc) == UCALL_ABORT,
+		    "Unexpected ucall command: %lu,  Expected: %u (UCALL_ABORT)\n",
+		    uc.cmd, UCALL_ABORT);
+
+	kvm_vm_free(vm);
+}
+
+int main(int argc, char *argv[])
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+
+	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
+
+	test_type_i64(vcpu, -1, -1);
+	test_type_i64(vcpu, -1,  1);
+	test_type_i64(vcpu, 0x1234567890abcdef, 0x1234567890abcdef);
+	test_type_i64(vcpu, 0x1234567890abcdef, 0x1234567890abcdee);
+
+	test_type_u64(vcpu, 0x1234567890abcdef, 0x1234567890abcdef);
+	test_type_u64(vcpu, 0x1234567890abcdef, 0x1234567890abcdee);
+	test_type_x64(vcpu, 0x1234567890abcdef, 0x1234567890abcdef);
+	test_type_x64(vcpu, 0x1234567890abcdef, 0x1234567890abcdee);
+	test_type_X64(vcpu, 0x1234567890abcdef, 0x1234567890abcdef);
+	test_type_X64(vcpu, 0x1234567890abcdef, 0x1234567890abcdee);
+
+	test_type_u32(vcpu, 0x90abcdef, 0x90abcdef);
+	test_type_u32(vcpu, 0x90abcdef, 0x90abcdee);
+	test_type_x32(vcpu, 0x90abcdef, 0x90abcdef);
+	test_type_x32(vcpu, 0x90abcdef, 0x90abcdee);
+	test_type_X32(vcpu, 0x90abcdef, 0x90abcdef);
+	test_type_X32(vcpu, 0x90abcdef, 0x90abcdee);
+
+	test_type_int(vcpu, -1, -1);
+	test_type_int(vcpu, -1,  1);
+	test_type_int(vcpu,  1,  1);
+
+	test_type_char(vcpu, 'a', 'a');
+	test_type_char(vcpu, 'a', 'A');
+	test_type_char(vcpu, 'a', 'b');
+
+	test_type_str(vcpu, "foo", "foo");
+	test_type_str(vcpu, "foo", "bar");
+
+	test_type_ptr(vcpu, 0x1234567890abcdef, 0x1234567890abcdef);
+	test_type_ptr(vcpu, 0x1234567890abcdef, 0x1234567890abcdee);
+
+	kvm_vm_free(vm);
+
+	test_limits();
+
+	return 0;
+}
-- 
2.41.0.rc0.172.g3f132b7071-goog

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

[Sean Christopherson]

On Wed, Jun 07, 2023, Aaron Lewis wrote:
> Extend the ucall framework to offer GUEST_PRINTF() and GUEST_ASSERT_FMT()
> in selftests.  This will allow for better and easier guest debugging.

This. Is.  Awesome.  Seriously, this is amazing!

I have one or two nits, but theyre so minor I already forgot what they were.

The one thing I think we should change is the final output of the assert.  Rather
than report the host TEST_FAIL as the assert:

  # ./svm_nested_soft_inject_test
  Running soft int test
  ==== Test Assertion Failure ====
    x86_64/svm_nested_soft_inject_test.c:191: false
    pid=169827 tid=169827 errno=4 - Interrupted system call
       1	0x0000000000401b52: run_test at svm_nested_soft_inject_test.c:191
       2	0x00000000004017d2: main at svm_nested_soft_inject_test.c:212
       3	0x00000000004159d3: __libc_start_call_main at libc-start.o:?
       4	0x000000000041701f: __libc_start_main_impl at ??:?
       5	0x0000000000401660: _start at ??:?
    Failed guest assert: regs->rip != (unsigned long)l2_guest_code_int at x86_64/svm_nested_soft_inject_test.c:39
    Expected IRQ at RIP 0x401e80, received IRQ at 0x401e80

show the guest assert as the primary assert.

  Running soft int test
  ==== Test Assertion Failure ====
    x86_64/svm_nested_soft_inject_test.c:39: regs->rip != (unsigned long)l2_guest_code_int
    pid=214104 tid=214104 errno=4 - Interrupted system call
       1	0x0000000000401b35: run_test at svm_nested_soft_inject_test.c:191
       2	0x00000000004017d2: main at svm_nested_soft_inject_test.c:212
       3	0x0000000000415b03: __libc_start_call_main at libc-start.o:?
       4	0x000000000041714f: __libc_start_main_impl at ??:?
       5	0x0000000000401660: _start at ??:?
    Expected IRQ at RIP 0x401e50, received IRQ at 0x401e50

That way users don't have to manually find the "real" assert.  Ditto for any kind
of automated reporting.  The site of the test_fail() invocation in the host is
still captured in the stack trace (though that too could be something to fix over
time), so unless I'm missing something, there's no information lost.

The easiest thing I can think of is to add a second buffer to hold the exp+file+line.
Then, test_assert() just needs to skip that particular line of formatting.

If you don't object, I'll post a v4 with the below folded in somewhere (after
more testing), and put this on the fast track for 6.6.

Side topic, if anyone lurking out there wants an easy (but tedious and boring)
starter project, we should convert all tests to the newfangled formatting and
drop GUEST_ASSERT_N entirely.  Once all tests are converted, GUEST_ASSERT_FMT()
and REPORT_GUEST_ASSERT_FMT can drop the "FMT" postfix.

---
 .../selftests/kvm/include/ucall_common.h      | 19 ++++++++---------
 tools/testing/selftests/kvm/lib/assert.c      | 13 +++++++-----
 .../testing/selftests/kvm/lib/ucall_common.c  | 21 +++++++++++++++++++
 3 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/ucall_common.h b/tools/testing/selftests/kvm/include/ucall_common.h
index b4f4c88e8d84..3bc4e62bec1b 100644
--- a/tools/testing/selftests/kvm/include/ucall_common.h
+++ b/tools/testing/selftests/kvm/include/ucall_common.h
@@ -25,6 +25,7 @@ struct ucall {
 	uint64_t cmd;
 	uint64_t args[UCALL_MAX_ARGS];
 	char buffer[UCALL_BUFFER_LEN];
+	char aux_buffer[UCALL_BUFFER_LEN];
 
 	/* Host virtual address of this struct. */
 	struct ucall *hva;
@@ -36,6 +37,8 @@ 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, ...);
+void ucall_assert(uint64_t cmd, const char *exp, const char *file,
+		  unsigned int line, 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_nr_pages_required(uint64_t page_size);
@@ -63,15 +66,10 @@ enum guest_assert_builtin_args {
 	GUEST_ASSERT_BUILTIN_NARGS
 };
 
-#define __GUEST_ASSERT_FMT(_condition, _str, _fmt, _args...)			\
-do {										\
-	char fmt[UCALL_BUFFER_LEN];						\
-										\
-	if (!(_condition)) {							\
-		guest_snprintf(fmt, sizeof(fmt), "%s\n  %s",			\
-			     "Failed guest assert: " _str " at %s:%ld", _fmt);	\
-		ucall_fmt(UCALL_ABORT, fmt, __FILE__, __LINE__, ##_args);	\
-	}									\
+#define __GUEST_ASSERT_FMT(_condition, _str, _fmt, _args...)				\
+do {											\
+	if (!(_condition)) 								\
+		ucall_assert(UCALL_ABORT, _str, __FILE__, __LINE__, _fmt, ##_args);	\
 } while (0)
 
 #define GUEST_ASSERT_FMT(_condition, _fmt, _args...)	\
@@ -102,7 +100,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_FMT(ucall)					\
+	test_assert(false, (ucall).aux_buffer, NULL, 0, "%s", (ucall).buffer);
 
 #define __REPORT_GUEST_ASSERT(_ucall, fmt, _args...)			\
 	TEST_FAIL("%s at %s:%ld\n" fmt,					\
diff --git a/tools/testing/selftests/kvm/lib/assert.c b/tools/testing/selftests/kvm/lib/assert.c
index 2bd25b191d15..74d94a34cf1a 100644
--- a/tools/testing/selftests/kvm/lib/assert.c
+++ b/tools/testing/selftests/kvm/lib/assert.c
@@ -75,11 +75,14 @@ test_assert(bool exp, const char *exp_str,
 	if (!(exp)) {
 		va_start(ap, fmt);
 
-		fprintf(stderr, "==== Test Assertion Failure ====\n"
-			"  %s:%u: %s\n"
-			"  pid=%d tid=%d errno=%d - %s\n",
-			file, line, exp_str, getpid(), _gettid(),
-			errno, strerror(errno));
+		fprintf(stderr, "==== Test Assertion Failure ====\n");
+		/* If @file is NULL, @exp_str contains a preformatted string. */
+		if (file)
+			fprintf(stderr, "  %s:%u: %s\n", file, line, exp_str);
+		else
+			fprintf(stderr, "  %s\n", exp_str);
+		fprintf(stderr, "  pid=%d tid=%d errno=%d - %s\n",
+			getpid(), _gettid(), errno, strerror(errno));
 		test_dump_stack();
 		if (fmt) {
 			fputs("  ", stderr);
diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c
index b507db91139b..e7741aadf2ce 100644
--- a/tools/testing/selftests/kvm/lib/ucall_common.c
+++ b/tools/testing/selftests/kvm/lib/ucall_common.c
@@ -75,6 +75,27 @@ static void ucall_free(struct ucall *uc)
 	clear_bit(uc - ucall_pool->ucalls, ucall_pool->in_use);
 }
 
+void ucall_assert(uint64_t cmd, const char *exp, const char *file,
+		  unsigned int line, const char *fmt, ...)
+{
+	struct ucall *uc;
+	va_list va;
+
+	uc = ucall_alloc();
+	uc->cmd = cmd;
+
+	guest_snprintf(uc->aux_buffer, sizeof(uc->aux_buffer),
+		       "%s:%u: %s", file, line, exp);
+
+	va_start(va, fmt);
+	guest_vsnprintf(uc->buffer, UCALL_BUFFER_LEN, fmt, va);
+	va_end(va);
+
+	ucall_arch_do_ucall((vm_vaddr_t)uc->hva);
+
+	ucall_free(uc);
+}
+
 void ucall_fmt(uint64_t cmd, const char *fmt, ...)
 {
 	struct ucall *uc;

base-commit: 8dc29dfc010293957c5ca24271748a3c8f047a76
-- 

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

[JinrongLiang]

在 2023/7/27 06:41, Sean Christopherson 写道:
> On Wed, Jun 07, 2023, Aaron Lewis wrote:
>> Extend the ucall framework to offer GUEST_PRINTF() and GUEST_ASSERT_FMT()
>> in selftests.  This will allow for better and easier guest debugging.
> 
> This. Is.  Awesome.  Seriously, this is amazing!
> 
> I have one or two nits, but theyre so minor I already forgot what they were.
> 
> The one thing I think we should change is the final output of the assert.  Rather
> than report the host TEST_FAIL as the assert:
> 
>    # ./svm_nested_soft_inject_test
>    Running soft int test
>    ==== Test Assertion Failure ====
>      x86_64/svm_nested_soft_inject_test.c:191: false
>      pid=169827 tid=169827 errno=4 - Interrupted system call
>         1	0x0000000000401b52: run_test at svm_nested_soft_inject_test.c:191
>         2	0x00000000004017d2: main at svm_nested_soft_inject_test.c:212
>         3	0x00000000004159d3: __libc_start_call_main at libc-start.o:?
>         4	0x000000000041701f: __libc_start_main_impl at ??:?
>         5	0x0000000000401660: _start at ??:?
>      Failed guest assert: regs->rip != (unsigned long)l2_guest_code_int at x86_64/svm_nested_soft_inject_test.c:39
>      Expected IRQ at RIP 0x401e80, received IRQ at 0x401e80
> 
> show the guest assert as the primary assert.
> 
>    Running soft int test
>    ==== Test Assertion Failure ====
>      x86_64/svm_nested_soft_inject_test.c:39: regs->rip != (unsigned long)l2_guest_code_int
>      pid=214104 tid=214104 errno=4 - Interrupted system call
>         1	0x0000000000401b35: run_test at svm_nested_soft_inject_test.c:191
>         2	0x00000000004017d2: main at svm_nested_soft_inject_test.c:212
>         3	0x0000000000415b03: __libc_start_call_main at libc-start.o:?
>         4	0x000000000041714f: __libc_start_main_impl at ??:?
>         5	0x0000000000401660: _start at ??:?
>      Expected IRQ at RIP 0x401e50, received IRQ at 0x401e50
> 
> That way users don't have to manually find the "real" assert.  Ditto for any kind
> of automated reporting.  The site of the test_fail() invocation in the host is
> still captured in the stack trace (though that too could be something to fix over
> time), so unless I'm missing something, there's no information lost.
> 
> The easiest thing I can think of is to add a second buffer to hold the exp+file+line.
> Then, test_assert() just needs to skip that particular line of formatting.
> 
> If you don't object, I'll post a v4 with the below folded in somewhere (after
> more testing), and put this on the fast track for 6.6.
> 
> Side topic, if anyone lurking out there wants an easy (but tedious and boring)
> starter project, we should convert all tests to the newfangled formatting and
> drop GUEST_ASSERT_N entirely.  Once all tests are converted, GUEST_ASSERT_FMT()
> and REPORT_GUEST_ASSERT_FMT can drop the "FMT" postfix.

I'd be happy to get the job done.

However, before I proceed, could you please provide a more detailed 
example or further guidance on the desired formatting and the specific 
changes you would like to see?

Thanks

Jinrong Liang

> 
> ---
>   .../selftests/kvm/include/ucall_common.h      | 19 ++++++++---------
>   tools/testing/selftests/kvm/lib/assert.c      | 13 +++++++-----
>   .../testing/selftests/kvm/lib/ucall_common.c  | 21 +++++++++++++++++++
>   3 files changed, 38 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/ucall_common.h b/tools/testing/selftests/kvm/include/ucall_common.h
> index b4f4c88e8d84..3bc4e62bec1b 100644
> --- a/tools/testing/selftests/kvm/include/ucall_common.h
> +++ b/tools/testing/selftests/kvm/include/ucall_common.h
> @@ -25,6 +25,7 @@ struct ucall {
>   	uint64_t cmd;
>   	uint64_t args[UCALL_MAX_ARGS];
>   	char buffer[UCALL_BUFFER_LEN];
> +	char aux_buffer[UCALL_BUFFER_LEN];
>   
>   	/* Host virtual address of this struct. */
>   	struct ucall *hva;
> @@ -36,6 +37,8 @@ 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, ...);
> +void ucall_assert(uint64_t cmd, const char *exp, const char *file,
> +		  unsigned int line, 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_nr_pages_required(uint64_t page_size);
> @@ -63,15 +66,10 @@ enum guest_assert_builtin_args {
>   	GUEST_ASSERT_BUILTIN_NARGS
>   };
>   
> -#define __GUEST_ASSERT_FMT(_condition, _str, _fmt, _args...)			\
> -do {										\
> -	char fmt[UCALL_BUFFER_LEN];						\
> -										\
> -	if (!(_condition)) {							\
> -		guest_snprintf(fmt, sizeof(fmt), "%s\n  %s",			\
> -			     "Failed guest assert: " _str " at %s:%ld", _fmt);	\
> -		ucall_fmt(UCALL_ABORT, fmt, __FILE__, __LINE__, ##_args);	\
> -	}									\
> +#define __GUEST_ASSERT_FMT(_condition, _str, _fmt, _args...)				\
> +do {											\
> +	if (!(_condition)) 								\
> +		ucall_assert(UCALL_ABORT, _str, __FILE__, __LINE__, _fmt, ##_args);	\
>   } while (0)
>   
>   #define GUEST_ASSERT_FMT(_condition, _fmt, _args...)	\
> @@ -102,7 +100,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_FMT(ucall)					\
> +	test_assert(false, (ucall).aux_buffer, NULL, 0, "%s", (ucall).buffer);
>   
>   #define __REPORT_GUEST_ASSERT(_ucall, fmt, _args...)			\
>   	TEST_FAIL("%s at %s:%ld\n" fmt,					\
> diff --git a/tools/testing/selftests/kvm/lib/assert.c b/tools/testing/selftests/kvm/lib/assert.c
> index 2bd25b191d15..74d94a34cf1a 100644
> --- a/tools/testing/selftests/kvm/lib/assert.c
> +++ b/tools/testing/selftests/kvm/lib/assert.c
> @@ -75,11 +75,14 @@ test_assert(bool exp, const char *exp_str,
>   	if (!(exp)) {
>   		va_start(ap, fmt);
>   
> -		fprintf(stderr, "==== Test Assertion Failure ====\n"
> -			"  %s:%u: %s\n"
> -			"  pid=%d tid=%d errno=%d - %s\n",
> -			file, line, exp_str, getpid(), _gettid(),
> -			errno, strerror(errno));
> +		fprintf(stderr, "==== Test Assertion Failure ====\n");
> +		/* If @file is NULL, @exp_str contains a preformatted string. */
> +		if (file)
> +			fprintf(stderr, "  %s:%u: %s\n", file, line, exp_str);
> +		else
> +			fprintf(stderr, "  %s\n", exp_str);
> +		fprintf(stderr, "  pid=%d tid=%d errno=%d - %s\n",
> +			getpid(), _gettid(), errno, strerror(errno));
>   		test_dump_stack();
>   		if (fmt) {
>   			fputs("  ", stderr);
> diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c
> index b507db91139b..e7741aadf2ce 100644
> --- a/tools/testing/selftests/kvm/lib/ucall_common.c
> +++ b/tools/testing/selftests/kvm/lib/ucall_common.c
> @@ -75,6 +75,27 @@ static void ucall_free(struct ucall *uc)
>   	clear_bit(uc - ucall_pool->ucalls, ucall_pool->in_use);
>   }
>   
> +void ucall_assert(uint64_t cmd, const char *exp, const char *file,
> +		  unsigned int line, const char *fmt, ...)
> +{
> +	struct ucall *uc;
> +	va_list va;
> +
> +	uc = ucall_alloc();
> +	uc->cmd = cmd;
> +
> +	guest_snprintf(uc->aux_buffer, sizeof(uc->aux_buffer),
> +		       "%s:%u: %s", file, line, exp);
> +
> +	va_start(va, fmt);
> +	guest_vsnprintf(uc->buffer, UCALL_BUFFER_LEN, fmt, va);
> +	va_end(va);
> +
> +	ucall_arch_do_ucall((vm_vaddr_t)uc->hva);
> +
> +	ucall_free(uc);
> +}
> +
>   void ucall_fmt(uint64_t cmd, const char *fmt, ...)
>   {
>   	struct ucall *uc;
> 
> base-commit: 8dc29dfc010293957c5ca24271748a3c8f047a76

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

[Aaron Lewis]

>
> The easiest thing I can think of is to add a second buffer to hold the exp+file+line.
> Then, test_assert() just needs to skip that particular line of formatting.
>
> If you don't object, I'll post a v4 with the below folded in somewhere (after
> more testing), and put this on the fast track for 6.6.
>

Yes, please update in v4.  That should cause less confusion when
reading the assert.  I like it!

> Side topic, if anyone lurking out there wants an easy (but tedious and boring)
> starter project, we should convert all tests to the newfangled formatting and
> drop GUEST_ASSERT_N entirely.  Once all tests are converted, GUEST_ASSERT_FMT()
> and REPORT_GUEST_ASSERT_FMT can drop the "FMT" postfix.
>

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

[Sean Christopherson]

On Thu, Jul 27, 2023, Aaron Lewis wrote:
> >
> > The easiest thing I can think of is to add a second buffer to hold the exp+file+line.
> > Then, test_assert() just needs to skip that particular line of formatting.

Gah, had a brain fart.  There's no need to format the expression+file+line in the
guest, we can pass pointers to the expression and file, just like we already do
for the existing guest asserts.

v4 coming soon...

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

[Sean Christopherson]

On Thu, Jul 27, 2023, JinrongLiang wrote:
> 
> 在 2023/7/27 06:41, Sean Christopherson 写道:
> > Side topic, if anyone lurking out there wants an easy (but tedious and boring)
> > starter project, we should convert all tests to the newfangled formatting and
> > drop GUEST_ASSERT_N entirely.  Once all tests are converted, GUEST_ASSERT_FMT()
> > and REPORT_GUEST_ASSERT_FMT can drop the "FMT" postfix.
> 
> I'd be happy to get the job done.
> 
> However, before I proceed, could you please provide a more detailed example
> or further guidance on the desired formatting and the specific changes you
> would like to see?

Hrm, scratch that request.  I was thinking we could convert tests one-by-one, but
that won't work well because to do a one-by-one conversions, tests that use
GUEST_ASSERT_EQ() would need to first convert to e.g. GUEST_ASSERT_EQ_FMT() and
then convert back, which would be a silly amount of churn just to a void a single
selftests-wide patch.

It probably makes sense to just convert everything as part of this series.  There
are quite a few asserts that need a message, but not *that* many.

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

[Sean Christopherson]

On Thu, Jul 27, 2023, Sean Christopherson wrote:
> On Thu, Jul 27, 2023, JinrongLiang wrote:
> > 
> > 在 2023/7/27 06:41, Sean Christopherson 写道:
> > > Side topic, if anyone lurking out there wants an easy (but tedious and boring)
> > > starter project, we should convert all tests to the newfangled formatting and
> > > drop GUEST_ASSERT_N entirely.  Once all tests are converted, GUEST_ASSERT_FMT()
> > > and REPORT_GUEST_ASSERT_FMT can drop the "FMT" postfix.
> > 
> > I'd be happy to get the job done.
> > 
> > However, before I proceed, could you please provide a more detailed example
> > or further guidance on the desired formatting and the specific changes you
> > would like to see?
> 
> Hrm, scratch that request.  I was thinking we could convert tests one-by-one, but
> that won't work well because to do a one-by-one conversions, tests that use
> GUEST_ASSERT_EQ() would need to first convert to e.g. GUEST_ASSERT_EQ_FMT() and
> then convert back, which would be a silly amount of churn just to a void a single
> selftests-wide patch.
> 
> It probably makes sense to just convert everything as part of this series.  There
> are quite a few asserts that need a message, but not *that* many.

Aha!  And there's already a "tree"-wide patch in this area to rename ASSERT_EQ()
to TEST_ASSERT_EQ()[*].  I'll include that in v4 as well, and then piggyback on it
to implement the new and improved GUEST_ASSERT_EQ().

[*] https://lore.kernel.org/all/20230712075910.22480-2-thuth@redhat.com

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

[Jinrong Liang]

Sean Christopherson <seanjc@google.com> 于2023年7月28日周五 03:21写道：
>
> On Thu, Jul 27, 2023, Sean Christopherson wrote:
> > On Thu, Jul 27, 2023, JinrongLiang wrote:
> > >
> > > 在 2023/7/27 06:41, Sean Christopherson 写道:
> > > > Side topic, if anyone lurking out there wants an easy (but tedious and boring)
> > > > starter project, we should convert all tests to the newfangled formatting and
> > > > drop GUEST_ASSERT_N entirely.  Once all tests are converted, GUEST_ASSERT_FMT()
> > > > and REPORT_GUEST_ASSERT_FMT can drop the "FMT" postfix.
> > >
> > > I'd be happy to get the job done.
> > >
> > > However, before I proceed, could you please provide a more detailed example
> > > or further guidance on the desired formatting and the specific changes you
> > > would like to see?
> >
> > Hrm, scratch that request.  I was thinking we could convert tests one-by-one, but
> > that won't work well because to do a one-by-one conversions, tests that use
> > GUEST_ASSERT_EQ() would need to first convert to e.g. GUEST_ASSERT_EQ_FMT() and
> > then convert back, which would be a silly amount of churn just to a void a single
> > selftests-wide patch.
> >
> > It probably makes sense to just convert everything as part of this series.  There
> > are quite a few asserts that need a message, but not *that* many.
>
> Aha!  And there's already a "tree"-wide patch in this area to rename ASSERT_EQ()
> to TEST_ASSERT_EQ()[*].  I'll include that in v4 as well, and then piggyback on it
> to implement the new and improved GUEST_ASSERT_EQ().
>
> [*] https://lore.kernel.org/all/20230712075910.22480-2-thuth@redhat.com

Thank you for your response and suggestions.  I believe your
recommendation is an excellent approach.

Please let me know if there's anything else I can help with or any
specific tasks you'd like me to work on. I appreciate your guidance
and the opportunity to contribute to KVM.