On 08/10/19 20:36, Sean Christopherson wrote:
> On Tue, Oct 08, 2019 at 08:08:08PM +0200, Vitaly Kuznetsov wrote:
>> Commit 204c91eff798a ("KVM: selftests: do not blindly clobber registers in
>> guest asm") was intended to make test more gcc-proof, however, the result
>> is exactly the opposite: on newer gccs (e.g. 8.2.1) the test breaks with
>>
>> ==== Test Assertion Failure ====
>> x86_64/sync_regs_test.c:168: run->s.regs.regs.rbx == 0xBAD1DEA + 1
>> pid=14170 tid=14170 - Invalid argument
>> 1 0x00000000004015b3: main at sync_regs_test.c:166 (discriminator
>> 6)
>> 2 0x00007f413fb66412: ?? ??:0
>> 3 0x000000000040191d: _start at ??:?
>> rbx sync regs value incorrect 0x1.
>>
>> Apparently, compile is still free to play games with registers even
>> when they have variables attaches.
>>
>> Re-write guest code with 'asm volatile' by embedding ucall there and
>> making sure rbx is preserved.
>>
>> Fixes: 204c91eff798a ("KVM: selftests: do not blindly clobber registers in
>> guest asm")
>> Signed-off-by: Vitaly Kuznetsov <vkuzn...@redhat.com>
>> ---
>> .../selftests/kvm/x86_64/sync_regs_test.c | 21 ++++++++++---------
>> 1 file changed, 11 insertions(+), 10 deletions(-)
>>
>> diff --git a/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
>> b/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
>> index 11c2a70a7b87..5c8224256294 100644
>> --- a/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
>> +++ b/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
>> @@ -22,18 +22,19 @@
>>
>> #define VCPU_ID 5
>>
>> +#define UCALL_PIO_PORT ((uint16_t)0x1000)
>> +
>> +/*
>> + * ucall is embedded here to protect against compiler reshuffling registers
>> + * before calling a function. In this test we only need to get KVM_EXIT_IO
>> + * vmexit and preserve RBX, no additional information is needed.
>> + */
>> void guest_code(void)
>> {
>> - /*
>> - * use a callee-save register, otherwise the compiler
>> - * saves it around the call to GUEST_SYNC.
>> - */
>> - register u32 stage asm("rbx");
>> - for (;;) {
>> - GUEST_SYNC(0);
>> - stage++;
>> - asm volatile ("" : : "r" (stage));
>> - }
>> + asm volatile("1: in %[port], %%al\n"
>> + "add $0x1, %%rbx\n"
>> + "jmp 1b"
>> + : : [port] "d" (UCALL_PIO_PORT) : "rax", "rbx");
>> }
>
> To make the code truly bulletproof, is it possible to rename guest_code()
> to guest_code_wrapper() and then export 1: as guest_code? VM-Enter will
> jump directly to the relevant code and gcc can't touch rbx. E.g.:
>
> asm volatile("1: ..."
> ".global guest_code"
> "guest_code: " _ASM_PTR " 1b");
>
> Not sure if that works with how the selftests are compiled. It may also
> be possible to simply replace '1' with 'guest_code'.
There is no practical difference with Vitaly's patch. The first
_vcpu_run has no pre-/post-conditions on the value of %rbx:
run->kvm_valid_regs = TEST_SYNC_FIELDS;
rv = _vcpu_run(vm, VCPU_ID);
TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
"Unexpected exit reason: %u (%s),\n",
run->exit_reason,
exit_reason_str(run->exit_reason));
/*
* Then it goes on comparing regs/sregs/events, but does not
* check for specific values.
*/
As soon as that first _vcpu_run succeeds, you're stuck in the in/add/jmp
loop and the compiler can't trick you anymore.
So, I'm queuing the patch.
Paolo
