| Message ID | 20230424225854.4023978-6-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 a second ucall_fmt() function that takes two format strings instead > of one. This provides more flexibility because the string format in > GUEST_ASSERT_FMT() is no linger limited to only using literals. ... > -#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); \ > +#define __GUEST_ASSERT_FMT(_condition, _condstr, _fmt, _args...) \ > +do { \ > + if (!(_condition)) \ > + ucall_fmt2(UCALL_ABORT, \ > + "Failed guest assert: " _condstr " at %s:%ld\n ",\ I don't see any reason to add ucall_fmt2(), just do the string smushing in __GUEST_ASSERT_FMT(). I doubt there will be many, if any, uses for this outside of GUEST_ASSERT_FMT(). Even your test example is contrived, e.g. it would be just as easy, and arguably more robusted, to #define the expected vs. actual formats as it is to assign them to global variables. In other words, this #define __GUEST_ASSERT_FMT(_condition, _str, _fmt, _args...) \ do { \ char fmt_buffer[512]; \ \ if (!(_condition)) { \ kvm_snprintf(fmt_buffer, sizeof(fmt_buffer), "%s\n %s", \ "Failed guest assert: " _str " at %s:%ld", _fmt); \ ucall_fmt(UCALL_ABORT, fmt_buffer, __FILE__, __LINE__, ##_args);\ } \ } while (0) is a preferable to copy+pasting an entirely new ucall_fmt2(). (Feel free to use a different name for the on-stack array, e.g. just "fmt"). > + _fmt, __FILE__, __LINE__, ##_args); \ > } while (0) > > #define GUEST_ASSERT_FMT(_condition, _fmt, _args...) \ > diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c > index c09e57c8ef77..d0f1ad6c0c44 100644 > --- a/tools/testing/selftests/kvm/lib/ucall_common.c > +++ b/tools/testing/selftests/kvm/lib/ucall_common.c > @@ -76,6 +76,30 @@ static void ucall_free(struct ucall *uc) > clear_bit(uc - ucall_pool->ucalls, ucall_pool->in_use); > } > > +void ucall_fmt2(uint64_t cmd, const char *fmt1, const char *fmt2, ...) > +{ > + const int fmt_len = 128; > + char fmt[fmt_len]; Just do char fmt[128]; (or whatever size is chosen) > + struct ucall *uc; > + va_list va; > + int len; > + > + len = kvm_snprintf(fmt, fmt_len, "%s%s", fmt1, fmt2); and then here do sizeof(fmt). It's self-documenting, and makes it really, really hard to screw up and use the wrong format. Regarding the size, can you look into why using 1024 for the buffer fails? This really should use the max allowed UCALL buffer size, but I'm seeing shutdowns when pushing above 512 bytes (I didn't try to precisely find the threshold). Selftests are supposed to allocate 5 * 4KiB stacks, so the guest shouldn't be getting anywhere near a stack overflow. > + if (len > fmt_len) For KVM selftests use case, callers shouldn't need to sanity check, that should be something kvm_snprintf() itself handles. I'll follow-up in that patch.
On Mon, Jun 5, 2023 at 10:41 PM Sean Christopherson <seanjc@google.com> wrote: > > On Mon, Apr 24, 2023, Aaron Lewis wrote: > > Add a second ucall_fmt() function that takes two format strings instead > > of one. This provides more flexibility because the string format in > > GUEST_ASSERT_FMT() is no linger limited to only using literals. > > ... > > > -#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); \ > > +#define __GUEST_ASSERT_FMT(_condition, _condstr, _fmt, _args...) \ > > +do { \ > > + if (!(_condition)) \ > > + ucall_fmt2(UCALL_ABORT, \ > > + "Failed guest assert: " _condstr " at %s:%ld\n ",\ > > I don't see any reason to add ucall_fmt2(), just do the string smushing in > __GUEST_ASSERT_FMT(). I doubt there will be many, if any, uses for this outside > of GUEST_ASSERT_FMT(). Even your test example is contrived, e.g. it would be > just as easy, and arguably more robusted, to #define the expected vs. actual formats > as it is to assign them to global variables. The way the test was first set up I needed them to be globals, but as it evolved I realized that requirement no longer held. I kept them as globals though to demonstrate they didn't have to be literals. I think that gives the API more flexibility and consistency. > > In other words, this > > #define __GUEST_ASSERT_FMT(_condition, _str, _fmt, _args...) \ > do { \ > char fmt_buffer[512]; \ > \ > if (!(_condition)) { \ > kvm_snprintf(fmt_buffer, sizeof(fmt_buffer), "%s\n %s", \ > "Failed guest assert: " _str " at %s:%ld", _fmt); \ > ucall_fmt(UCALL_ABORT, fmt_buffer, __FILE__, __LINE__, ##_args);\ > } \ > } while (0) > > is a preferable to copy+pasting an entirely new ucall_fmt2(). (Feel free to use > a different name for the on-stack array, e.g. just "fmt"). > > > + _fmt, __FILE__, __LINE__, ##_args); \ > > } while (0) > > > > #define GUEST_ASSERT_FMT(_condition, _fmt, _args...) \ > > diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c > > index c09e57c8ef77..d0f1ad6c0c44 100644 > > --- a/tools/testing/selftests/kvm/lib/ucall_common.c > > +++ b/tools/testing/selftests/kvm/lib/ucall_common.c > > @@ -76,6 +76,30 @@ static void ucall_free(struct ucall *uc) > > clear_bit(uc - ucall_pool->ucalls, ucall_pool->in_use); > > } > > > > +void ucall_fmt2(uint64_t cmd, const char *fmt1, const char *fmt2, ...) > > +{ > > + const int fmt_len = 128; > > + char fmt[fmt_len]; > > Just do > > char fmt[128]; > > (or whatever size is chosen) > > > + struct ucall *uc; > > + va_list va; > > + int len; > > + > > + len = kvm_snprintf(fmt, fmt_len, "%s%s", fmt1, fmt2); > > and then here do sizeof(fmt). It's self-documenting, and makes it really, really > hard to screw up and use the wrong format. > > Regarding the size, can you look into why using 1024 for the buffer fails? This > really should use the max allowed UCALL buffer size, but I'm seeing shutdowns when > pushing above 512 bytes (I didn't try to precisely find the threshold). Selftests > are supposed to allocate 5 * 4KiB stacks, so the guest shouldn't be getting anywhere > near a stack overflow. D'oh! This is the result of a bad pattern used in the test. vcpu_regs_get(vcpu, ®s); regs.rip = (uintptr_t)guest_code; vcpu_regs_set(vcpu, ®s); I set the RIP, but not the RSP. That makes the stack grow out of control when called a lot like we do in this test. It's also x86 specific and this test no longer lives in that directory. For now I'll change the guest code to run in a loop to restart it, but I've used this pattern before and it's useful at times. Also, looping forces me to sync a global for the guest, and I think I'd rather pass the parameters directly into the guest code. Maybe it would make sense to implement proper support for this in the selftest library at some point to allow us to use this pattern and have it be less error prone. With the fix we are able to set the size to the UCALL buffer size. > > > + if (len > fmt_len) > > For KVM selftests use case, callers shouldn't need to sanity check, that should be > something kvm_snprintf() itself handles. I'll follow-up in that patch.
diff --git a/tools/testing/selftests/kvm/include/ucall_common.h b/tools/testing/selftests/kvm/include/ucall_common.h index 7281a6892779..3e8135aaa812 100644 --- a/tools/testing/selftests/kvm/include/ucall_common.h +++ b/tools/testing/selftests/kvm/include/ucall_common.h @@ -36,6 +36,7 @@ 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_fmt2(uint64_t cmd, const char *fmt1, const char *fmt2, ...); 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); @@ -61,12 +62,12 @@ 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); \ +#define __GUEST_ASSERT_FMT(_condition, _condstr, _fmt, _args...) \ +do { \ + if (!(_condition)) \ + ucall_fmt2(UCALL_ABORT, \ + "Failed guest assert: " _condstr " at %s:%ld\n ",\ + _fmt, __FILE__, __LINE__, ##_args); \ } while (0) #define GUEST_ASSERT_FMT(_condition, _fmt, _args...) \ diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c index c09e57c8ef77..d0f1ad6c0c44 100644 --- a/tools/testing/selftests/kvm/lib/ucall_common.c +++ b/tools/testing/selftests/kvm/lib/ucall_common.c @@ -76,6 +76,30 @@ static void ucall_free(struct ucall *uc) clear_bit(uc - ucall_pool->ucalls, ucall_pool->in_use); } +void ucall_fmt2(uint64_t cmd, const char *fmt1, const char *fmt2, ...) +{ + const int fmt_len = 128; + char fmt[fmt_len]; + struct ucall *uc; + va_list va; + int len; + + len = kvm_snprintf(fmt, fmt_len, "%s%s", fmt1, fmt2); + if (len > fmt_len) + ucall_arch_do_ucall(GUEST_UCALL_FAILED); + + uc = ucall_alloc(); + uc->cmd = cmd; + + va_start(va, fmt2); + 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_fmt(uint64_t cmd, const char *fmt, ...) { struct ucall *uc;
Add a second ucall_fmt() function that takes two format strings instead of one. This provides more flexibility because the string format in GUEST_ASSERT_FMT() is no linger limited to only using literals. This provides better consistency between GUEST_PRINTF() and GUEST_ASSERT_FMT() as GUEST_PRINTF() is not limited to only using literals either. Signed-off-by: Aaron Lewis <aaronlewis@google.com> --- .../selftests/kvm/include/ucall_common.h | 13 +++++----- .../testing/selftests/kvm/lib/ucall_common.c | 24 +++++++++++++++++++ 2 files changed, 31 insertions(+), 6 deletions(-)