* [PATCH RFC] selftests: kvm: fix sync_regs_test with newer gccs
@ 2019-10-08 14:57 Vitaly Kuznetsov
2019-10-08 15:19 ` Paolo Bonzini
0 siblings, 1 reply; 2+ messages in thread
From: Vitaly Kuznetsov @ 2019-10-08 14:57 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kvm, linux-kernel, Sean Christopherson, Jim Mattson
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.
Disassembly show the following:
00000000004019e0 <guest_code>:
4019e0: 55 push %rbp
4019e1: 89 dd mov %ebx,%ebp
4019e3: 53 push %rbx
4019e4: 48 83 ec 08 sub $0x8,%rsp
4019e8: 0f 1f 84 00 00 00 00 nopl 0x0(%rax,%rax,1)
4019ef: 00
4019f0: 31 c9 xor %ecx,%ecx
4019f2: ba 10 90 40 00 mov $0x409010,%edx
4019f7: be 02 00 00 00 mov $0x2,%esi
4019fc: 31 c0 xor %eax,%eax
4019fe: bf 01 00 00 00 mov $0x1,%edi
401a03: 83 c5 01 add $0x1,%ebp
401a06: e8 15 2b 00 00 callq 404520 <ucall>
401a0b: 89 eb mov %ebp,%ebx
401a0d: eb e1 jmp 4019f0 <guest_code+0x10>
401a0f: 90 nop
and apparently this is broken. If we add 'volatile' qualifier to 'stage'
we get the following code:
00000000004019e0 <guest_code>:
4019e0: 53 push %rbx
4019e1: 0f 1f 80 00 00 00 00 nopl 0x0(%rax)
4019e8: 31 c9 xor %ecx,%ecx
4019ea: ba 10 90 40 00 mov $0x409010,%edx
4019ef: be 02 00 00 00 mov $0x2,%esi
4019f4: 31 c0 xor %eax,%eax
4019f6: bf 01 00 00 00 mov $0x1,%edi
4019fb: 83 c3 01 add $0x1,%ebx
4019fe: e8 1d 2b 00 00 callq 404520 <ucall>
401a03: eb e3 jmp 4019e8 <guest_code+0x8>
401a05: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
401a0c: 00 00 00 00
and everything seems to work. The only problem is that I now get a new
warning from gcc:
x86_64/sync_regs_test.c: In function ‘guest_code’:
x86_64/sync_regs_test.c:25:6: warning: optimization may eliminate reads
and/or writes to register variables [-Wvolatile-register-var]
checkpatch.pl doesn't like me either:
"WARNING: Use of volatile is usually wrong: see
Documentation/process/volatile-considered-harmful.rst"
I can think of an 'ultimate' solution to open code ucall() in a single
asm block making sure the register we need is preserved but this looks
like an overkill.
Fixes: 204c91eff798a ("KVM: selftests: do not blindly clobber registers in guest asm")
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
tools/testing/selftests/kvm/x86_64/sync_regs_test.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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..25c54250d591 100644
--- a/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
+++ b/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
@@ -28,7 +28,7 @@ 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");
+ register volatile u32 stage asm("rbx");
for (;;) {
GUEST_SYNC(0);
stage++;
--
2.20.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH RFC] selftests: kvm: fix sync_regs_test with newer gccs
2019-10-08 14:57 [PATCH RFC] selftests: kvm: fix sync_regs_test with newer gccs Vitaly Kuznetsov
@ 2019-10-08 15:19 ` Paolo Bonzini
0 siblings, 0 replies; 2+ messages in thread
From: Paolo Bonzini @ 2019-10-08 15:19 UTC (permalink / raw)
To: Vitaly Kuznetsov; +Cc: kvm, linux-kernel, Sean Christopherson, Jim Mattson
On 08/10/19 16:57, 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.
>
> Disassembly show the following:
>
> 00000000004019e0 <guest_code>:
> 4019e0: 55 push %rbp
> 4019e1: 89 dd mov %ebx,%ebp
> 4019e3: 53 push %rbx
> 4019e4: 48 83 ec 08 sub $0x8,%rsp
> 4019e8: 0f 1f 84 00 00 00 00 nopl 0x0(%rax,%rax,1)
> 4019ef: 00
> 4019f0: 31 c9 xor %ecx,%ecx
> 4019f2: ba 10 90 40 00 mov $0x409010,%edx
> 4019f7: be 02 00 00 00 mov $0x2,%esi
> 4019fc: 31 c0 xor %eax,%eax
> 4019fe: bf 01 00 00 00 mov $0x1,%edi
> 401a03: 83 c5 01 add $0x1,%ebp
> 401a06: e8 15 2b 00 00 callq 404520 <ucall>
> 401a0b: 89 eb mov %ebp,%ebx
> 401a0d: eb e1 jmp 4019f0 <guest_code+0x10>
> 401a0f: 90 nop
>
> and apparently this is broken. If we add 'volatile' qualifier to 'stage'
> we get the following code:
>
> 00000000004019e0 <guest_code>:
> 4019e0: 53 push %rbx
> 4019e1: 0f 1f 80 00 00 00 00 nopl 0x0(%rax)
> 4019e8: 31 c9 xor %ecx,%ecx
> 4019ea: ba 10 90 40 00 mov $0x409010,%edx
> 4019ef: be 02 00 00 00 mov $0x2,%esi
> 4019f4: 31 c0 xor %eax,%eax
> 4019f6: bf 01 00 00 00 mov $0x1,%edi
> 4019fb: 83 c3 01 add $0x1,%ebx
> 4019fe: e8 1d 2b 00 00 callq 404520 <ucall>
> 401a03: eb e3 jmp 4019e8 <guest_code+0x8>
> 401a05: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
> 401a0c: 00 00 00 00
>
> and everything seems to work. The only problem is that I now get a new
> warning from gcc:
>
> x86_64/sync_regs_test.c: In function ‘guest_code’:
> x86_64/sync_regs_test.c:25:6: warning: optimization may eliminate reads
> and/or writes to register variables [-Wvolatile-register-var]
>
> checkpatch.pl doesn't like me either:
>
> "WARNING: Use of volatile is usually wrong: see
> Documentation/process/volatile-considered-harmful.rst"
>
> I can think of an 'ultimate' solution to open code ucall() in a single
> asm block making sure the register we need is preserved but this looks
> like an overkill.
It is probably the best idea though. It's a processor-specific test
anyway. It also doesn't need the full ucall(), just a single in
instruction will do.
Paolo
> Fixes: 204c91eff798a ("KVM: selftests: do not blindly clobber registers in guest asm")
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> tools/testing/selftests/kvm/x86_64/sync_regs_test.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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..25c54250d591 100644
> --- a/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
> @@ -28,7 +28,7 @@ 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");
> + register volatile u32 stage asm("rbx");
> for (;;) {
> GUEST_SYNC(0);
> stage++;
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2019-10-08 15:19 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-08 14:57 [PATCH RFC] selftests: kvm: fix sync_regs_test with newer gccs Vitaly Kuznetsov
2019-10-08 15:19 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).