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@xxxxxxxxxx>
> ---
> .../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;
> }
