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

[Sean Christopherson <seanjc@google.com>]

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;
>  		}