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 <[email protected]>
---
.../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");
}
static void compare_regs(struct kvm_regs *left, struct kvm_regs *right)
--
2.20.1
On Tue, Oct 8, 2019 at 11:08 AM Vitaly Kuznetsov <[email protected]> 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 <[email protected]>
> ---
> .../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");
> }
A better solution might be something like:
register u32 stage = 0;
for (;;) {
asm volatile("in %[port], %%al"
:
: "b" (stage), [port] "d" (UCALL_PIO_PORT)
: "rax");
stage++;
}
(Gmail no doubt has mangled the indentation. Sorry.)
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 <[email protected]>
> ---
> .../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'.
>
> static void compare_regs(struct kvm_regs *left, struct kvm_regs *right)
> --
> 2.20.1
>
On Tue, Oct 08, 2019 at 11:29:32AM -0700, Jim Mattson wrote:
> On Tue, Oct 8, 2019 at 11:08 AM Vitaly Kuznetsov <[email protected]> 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 <[email protected]>
> > ---
> > .../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");
> > }
> A better solution might be something like:
>
> register u32 stage = 0;
> for (;;) {
> asm volatile("in %[port], %%al"
> :
> : "b" (stage), [port] "d" (UCALL_PIO_PORT)
> : "rax");
> stage++;
> }
>
> (Gmail no doubt has mangled the indentation. Sorry.)
The incoming value of rbx matters, I believe it's set to 0xBAD1DEA and the
host then expects 0xBAD1DEA + 1.
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 <[email protected]>
>> ---
>> .../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
Paolo Bonzini <[email protected]> writes:
> 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 <[email protected]>
>>> ---
>>> .../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:
>
I think what Sean was suggesting is to prevent GCC from inserting
anything (and thus clobbering RBX) between the call to guest_call() and
the beginning of 'asm volatile' block by calling *inside* 'asm volatile'
block instead.
>
> 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.
>
Thanks!
--
Vitaly
On 09/10/19 12:42, Vitaly Kuznetsov wrote:
> Paolo Bonzini <[email protected]> writes:
>> There is no practical difference with Vitaly's patch. The first
>> _vcpu_run has no pre-/post-conditions on the value of %rbx:
>
> I think what Sean was suggesting is to prevent GCC from inserting
> anything (and thus clobbering RBX) between the call to guest_call() and
> the beginning of 'asm volatile' block by calling *inside* 'asm volatile'
> block instead.
Yes, but there is no way that clobbering RBX will break the test,
because RBX is not initialized until after the first _vcpu_run succeeds.
Paolo
Paolo Bonzini <[email protected]> writes:
> On 09/10/19 12:42, Vitaly Kuznetsov wrote:
>> Paolo Bonzini <[email protected]> writes:
>>> There is no practical difference with Vitaly's patch. The first
>>> _vcpu_run has no pre-/post-conditions on the value of %rbx:
>>
>> I think what Sean was suggesting is to prevent GCC from inserting
>> anything (and thus clobbering RBX) between the call to guest_call() and
>> the beginning of 'asm volatile' block by calling *inside* 'asm volatile'
>> block instead.
>
> Yes, but there is no way that clobbering RBX will break the test,
> because RBX is not initialized until after the first _vcpu_run succeeds.
>
Right, we're always resuming after so potential clobbering doesn't
matter. Thanks!
--
Vitaly
On Wed, Oct 09, 2019 at 01:11:24PM +0200, Paolo Bonzini wrote:
> On 09/10/19 12:42, Vitaly Kuznetsov wrote:
> > Paolo Bonzini <[email protected]> writes:
> >> There is no practical difference with Vitaly's patch. The first
> >> _vcpu_run has no pre-/post-conditions on the value of %rbx:
> >
> > I think what Sean was suggesting is to prevent GCC from inserting
> > anything (and thus clobbering RBX) between the call to guest_call() and
> > the beginning of 'asm volatile' block by calling *inside* 'asm volatile'
> > block instead.
>
> Yes, but there is no way that clobbering RBX will break the test,
> because RBX is not initialized until after the first _vcpu_run succeeds.
Ah, nice, wasn't aware of that.