On Thu, Apr 06, 2023, Mathias Krause wrote:
> On 06.04.23 05:02, Sean Christopherson wrote:
> > On Wed, Feb 15, 2023, Mathias Krause wrote:
> >> +static void test_reg_noncanonical(void)
> >> +{
> >> + extern char nc_rsp_start, nc_rsp_end, nc_rbp_start, nc_rbp_end;
> >> + extern char nc_rax_start, nc_rax_end;
> >> + handler old_ss, old_gp;
> >> +
> >> + old_ss = handle_exception(SS_VECTOR, advance_rip_and_note_exception);
> >> + old_gp = handle_exception(GP_VECTOR, advance_rip_and_note_exception);
> >> +
> >> + /* RAX based, should #GP(0) */
> >> + exceptions = 0;
> >> + rip_advance = &nc_rax_end - &nc_rax_start;
> >> + asm volatile("nc_rax_start: orq $0, (%[msb]); nc_rax_end:\n\t"
> >
> > Can't we use ASM_TRY() + exception_vector() + exception_error_code()? Installing
> > a dedicated handler is (slowly) being phased out.
>
> Well, you may have guessed it, but I tried to be "consistent with the
> existing style." Sure this code could get a lot of cleanups too, the
> whole file, actually, like the externs should be 'extern char []'
> instead to point out they're just "labels" and no chars. But, again, I
> just did as was "prior art" in this file. But if moving forward to a
> more modern version is wanted, I can adapt.
Yes, please ignore prior art in KVM-Unit-Tests, so much of its "art" is just awful.
> > And why hardcode use of RAX? Won't any "r" constraint work?
>
> Unfortunately not. It must be neither rsp nor rbp and with "r" the
> compiler is free to choose one of these.
Ah, right, I forgot about RBP.
> It'll unlikely make use of rsp, but rbp is a valid target we need to avoid.
> (Yes, I saw the -no-omit-frame-pointer handling in the Makefiles, but I
> dislike this implicit dependency.)
>
> I can change the constraints to "abcdSD" to give the compiler a little
> bit more freedom, but that makes the inline asm little harder to read,
> IMHO. Hardcoding rax is no real constraint to the compiler either, as
> it's a volatile register anyway. The call to report() will invalidate
> its old value, so I don't see the need for a change -- a comment, at
> best, but that's already there ;)
Hardcoding RAX is totally fine, I just forgot about RBP.
> > E.g. I believe this can be something like:
> >
> > asm_safe_report_ex(GP_VECTOR, "orq $0, (%[noncanonical]), "r" (NONCANONICAL));
> > report(!exception_error_code());
> >
> > Or we could even add asm_safe_report_ex_ec(), e.g.
> >
> > asm_safe_report_ex_ec(GP_VECTOR, 0,
> > "orq $0, (%[noncanonical]), "r" (NONCANONICAL));
>
> Yeah, the latter. Verifying the error code is part of the test, so that
> should be preserved.
>
> The tests as written by me also ensure that an exception actually
> occurred, exactly one, actually. Maybe that should be accounted for in
> asm_safe*() as well?
That's accounted for, the ASM_TRY() machinery treats "0" as no exception (we
sacrified #DE for the greater good). Realistically, the only way to have multiple
exceptions without going into the weeds is if KVM somehow restarted the faulting
instruction. That would essentially require very precise memory corruption to
undo the exception fixup handler's modification of RIP on the stack. And at that
point, one could also argue that KVM could also corrupt the exception counter.
> PS: Would be nice if the entry barrier for new tests wouldn't require to
> handle the accumulated technical debt of the file one's touching ;P
Heh, and if wishes were horses we'd all be eating steak. Just be glad I didn't
ask you to rewrite the entire test ;-)
Joking aside, coercing/extorting contributors into using new/better infrastructure
is the only feasible way to keep KVM's test infrastructure somewhat manageable.
E.g. I would love to be able to dedicate a substantial portion of my time to
cleaning up the many warts KUT, but the unfortunate reality is that test infrastructure
is always going to be lower priority than the product itself.
> But I can understand that adding more code adapting to "existing style"
> makes the problem only worse. So it's fine with me.
