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.
> Even better, if you're willing
> to take a dependency and/or wait a few weeks for my series to land[*], you should
> be able to use asm_safe() to streamline this even further.
>
> [*] https://lkml.kernel.org/r/20230406025117.738014-1-seanjc%40google.com
Looks like a nice cleanup, indeed. However, the conversion should be
straight forward, so I don't think this change has to "wait" for it.
The linked bug report turned 1 just two weeks ago. About time to get it
some more traction. :D
That said, I'll do a spring cleanup of emulator64.c along with my change
to address the points you mentioned in the other test functions as well.
But likely not before next week.
>
>
>> + : : [msb]"a"(1ul << 63));
>
> Use BIT_ULL(). Actually, scratch that, we have a NONCANONICAL macro. It _probably_
> won't matter, but who know what will happen with things like LAM and LASS.
Thanks, will change.
>
> 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. 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 ;)
>
> 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?
Thanks,
Mathias
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
But I can understand that adding more code adapting to "existing style"
makes the problem only worse. So it's fine with me.
