| Message ID | 20230424225854.4023978-5-aaronlewis@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add printf and formatted asserts in the guest | expand |
On Mon, Apr 24, 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. > > 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. ... > #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]; ... > diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c > index 77ada362273d..c09e57c8ef77 100644 > --- a/tools/testing/selftests/kvm/lib/ucall_common.c > +++ b/tools/testing/selftests/kvm/lib/ucall_common.c > @@ -55,6 +55,7 @@ static struct ucall *ucall_alloc(void) > if (!test_and_set_bit(i, ucall_pool->in_use)) { > uc = &ucall_pool->ucalls[i]; > memset(uc->args, 0, sizeof(uc->args)); > + memset(uc->buffer, 0, sizeof(uc->buffer)); The use in boot/printf.c isn't a great reference point. That "allocation" is on-stack and effectively free, whereas the use here "requires" zeroing the buffer during allocation. I usually tell people to not worry about selftests performance, but zeroing 1KiB on every ucall seems a bit excessive. However, that's more of an argument to not zero than it is to try and squeak by with a smaller size. The guest really should explicitly tell the host how much of the buffer. And with that, there should be no need to zero the buffer because the host isn't relying on the memory being zeroed. On a somehwat related topic, this patch should also introduce a macro/helper to retrieve and print the buffer on the backend. Partly to reduce copy+paste, partly to make it easier to review (i.e. show the end-to-end), and partly so that the ucall code can craft a more explicit contract.
On Mon, Jun 5, 2023 at 9:44 PM Sean Christopherson <seanjc@google.com> wrote: > > On Mon, Apr 24, 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. > > > > 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. > > ... > > #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]; > > ... > > > diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c > > index 77ada362273d..c09e57c8ef77 100644 > > --- a/tools/testing/selftests/kvm/lib/ucall_common.c > > +++ b/tools/testing/selftests/kvm/lib/ucall_common.c > > @@ -55,6 +55,7 @@ static struct ucall *ucall_alloc(void) > > if (!test_and_set_bit(i, ucall_pool->in_use)) { > > uc = &ucall_pool->ucalls[i]; > > memset(uc->args, 0, sizeof(uc->args)); > > + memset(uc->buffer, 0, sizeof(uc->buffer)); > > The use in boot/printf.c isn't a great reference point. That "allocation" is > on-stack and effectively free, whereas the use here "requires" zeroing the buffer > during allocation. I usually tell people to not worry about selftests performance, > but zeroing 1KiB on every ucall seems a bit excessive. > > However, that's more of an argument to not zero than it is to try and squeak by > with a smaller size. The guest really should explicitly tell the host how much > of the buffer. And with that, there should be no need to zero the buffer because > the host isn't relying on the memory being zeroed. I don't think zeroing the buffer is actually necessary. It is more of a nice-to-have for extra paranoia. The printf function ensures the string is NULL terminated, so I think it should be safe to just drop it and save the cycles. With the added assert in patch 2, plus a few more I'm planning on adding, guest_printf() either properly writes a string or dies. You brought up a good point in that selftests generally fail hard rather than hiding errors, so asserting makes sense there. That also means there is no real need to pass the length of the string to the host. The string should be properly written if guest_printf() returns successfully. > > On a somehwat related topic, this patch should also introduce a macro/helper to > retrieve and print the buffer on the backend. Partly to reduce copy+paste, partly > to make it easier to review (i.e. show the end-to-end), and partly so that the > ucall code can craft a more explicit contract. If guest_printf() returns successfully, then the expectation is that the string was correctly written, which makes the contract pretty simple. I'm thinking something like this? #define REPORT_GUEST_PRINTF(_ucall) pr_info("%s", _ucall.buffer)
diff --git a/tools/testing/selftests/kvm/include/ucall_common.h b/tools/testing/selftests/kvm/include/ucall_common.h index bcbb362aa77f..7281a6892779 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,6 +51,7 @@ 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) enum guest_assert_builtin_args { @@ -56,6 +61,17 @@ enum guest_assert_builtin_args { GUEST_ASSERT_BUILTIN_NARGS }; +#define __GUEST_ASSERT_FMT(_condition, _condstr, _fmt, _args...) \ +do { \ + if (!(_condition)) \ + ucall_fmt(UCALL_ABORT, \ + "Failed guest assert: " _condstr " at %s:%ld\n " _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 +97,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..c09e57c8ef77 100644 --- a/tools/testing/selftests/kvm/lib/ucall_common.c +++ b/tools/testing/selftests/kvm/lib/ucall_common.c @@ -55,6 +55,7 @@ static struct ucall *ucall_alloc(void) if (!test_and_set_bit(i, ucall_pool->in_use)) { uc = &ucall_pool->ucalls[i]; memset(uc->args, 0, sizeof(uc->args)); + memset(uc->buffer, 0, sizeof(uc->buffer)); return uc; } } @@ -75,6 +76,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); + kvm_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;
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 | 18 ++++++++++++++++++ tools/testing/selftests/kvm/lib/ucall_common.c | 18 ++++++++++++++++++ 2 files changed, 36 insertions(+)