On Mon, Jun 5, 2023 at 10:41 PM Sean Christopherson <seanjc@xxxxxxxxxx> 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.
