Re: [PATCH 04/10] KVM: selftests: Add GUEST_ASSERT variants to pass values to host

[Andrew Jones]

On Fri, Apr 10, 2020 at 04:17:01PM -0700, Sean Christopherson wrote:
> Add variants of GUEST_ASSERT to pass values back to the host, e.g. to
> help debug/understand a failure when the the cause of the assert isn't
> necessarily binary.
> 
> It'd probably be possible to auto-calculate the number of arguments and
> just have a single GUEST_ASSERT, but there are a limited number of
> variants and silently eating arguments could lead to subtle code bugs.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> ---
>  .../testing/selftests/kvm/include/kvm_util.h  | 25 +++++++++++++++----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> index d4c3e4d9cd92..e38d91bd8ec1 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> @@ -313,11 +313,26 @@ uint64_t get_ucall(struct kvm_vm *vm, uint32_t vcpu_id, struct ucall *uc);
>  
>  #define GUEST_SYNC(stage)	ucall(UCALL_SYNC, 2, "hello", stage)
>  #define GUEST_DONE()		ucall(UCALL_DONE, 0)
> -#define GUEST_ASSERT(_condition) do {			\
> -	if (!(_condition))				\
> -		ucall(UCALL_ABORT, 2,			\
> -			"Failed guest assert: "		\
> -			#_condition, __LINE__);		\
> +#define __GUEST_ASSERT(_condition, _nargs, _args...) do {	\
> +	if (!(_condition))					\
> +		ucall(UCALL_ABORT, 2 + _nargs,			\

Need () around _nargs

> +			"Failed guest assert: "			\
> +			#_condition, __LINE__, _args);		\
>  } while (0)

We can free up another arg and add __FILE__. Something like the following
(untested):

 #include "linux/stringify.h"

 #define __GUEST_ASSERT(_condition, _nargs, _args...) do {                            \
       if (!(_condition))                                                             \
               ucall(UCALL_ABORT, (_nargs) + 1,                                       \
                     "Failed guest assert: "                                          \
                     #_condition " at " __FILE__ ":" __stringify(__LINE__), _args);   \
 } while (0)

>  
> +#define GUEST_ASSERT(_condition) \
> +	__GUEST_ASSERT((_condition), 0, 0)
> +
> +#define GUEST_ASSERT_1(_condition, arg1) \
> +	__GUEST_ASSERT((_condition), 1, (arg1))
> +
> +#define GUEST_ASSERT_2(_condition, arg1, arg2) \
> +	__GUEST_ASSERT((_condition), 2, (arg1), (arg2))
> +
> +#define GUEST_ASSERT_3(_condition, arg1, arg2, arg3) \
> +	__GUEST_ASSERT((_condition), 3, (arg1), (arg2), (arg3))
> +
> +#define GUEST_ASSERT_4(_condition, arg1, arg2, arg3, arg4) \
> +	__GUEST_ASSERT((_condition), 4, (arg1), (arg2), (arg3), (arg4))
> +

nit: don't need the () around any of the macro params above

>  #endif /* SELFTEST_KVM_UTIL_H */
> -- 
> 2.26.0
> 

We could instead add test specific ucalls. To do so, we should first add
UCALL_TEST_SPECIFIC = 32 to the ucall enum, and then tests could extend it

 enum {
  MY_UCALL_1 = UCALL_TEST_SPECIFIC,
  MY_UCALL_2,
 };

With appropriately named test specific ucalls it may allow for clearer
code. At least GUEST_ASSERT_<N> wouldn't take on new meanings for each
test.

Thanks,
drew