| Message ID | 20200610164116.770811-1-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: selftests: fix sync_with_host() in smm_test | expand |
On Wed, Jun 10, 2020 at 9:41 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > > It was reported that older GCCs compile smm_test in a way that breaks > it completely: > > kvm_exit: reason EXIT_CPUID rip 0x4014db info 0 0 > func 7ffffffd idx 830 rax 0 rbx 0 rcx 0 rdx 0, cpuid entry not found > ... > kvm_exit: reason EXIT_MSR rip 0x40abd9 info 0 0 > kvm_msr: msr_read 487 = 0x0 (#GP) > ... > > Note, '7ffffffd' was supposed to be '80000001' as we're checking for > SVM. Dropping '-O2' from compiler flags help. Turns out, asm block in > sync_with_host() is wrong. We us 'in 0xe, %%al' instruction to sync > with the host and in 'AL' register we actually pass the parameter > (stage) but after sync 'AL' gets written to but GCC thinks the value > is still there and uses it to compute 'EAX' for 'cpuid'. That smells like VMware's hypercall madness! > smm_test can't fully use standard ucall() framework as we need to > write a very simple SMI handler there. Fix the immediate issue by > making RAX input/output operand. While on it, make sync_with_host() > static inline. > > Reported-by: Marcelo Bandeira Condotta <mcondotta@redhat.com> > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> Reviewed-by: Jim Mattson <jmattson@google.com>
On 10/06/20 18:41, Vitaly Kuznetsov wrote: > It was reported that older GCCs compile smm_test in a way that breaks > it completely: > > kvm_exit: reason EXIT_CPUID rip 0x4014db info 0 0 > func 7ffffffd idx 830 rax 0 rbx 0 rcx 0 rdx 0, cpuid entry not found > ... > kvm_exit: reason EXIT_MSR rip 0x40abd9 info 0 0 > kvm_msr: msr_read 487 = 0x0 (#GP) > ... > > Note, '7ffffffd' was supposed to be '80000001' as we're checking for > SVM. Dropping '-O2' from compiler flags help. Turns out, asm block in > sync_with_host() is wrong. We us 'in 0xe, %%al' instruction to sync > with the host and in 'AL' register we actually pass the parameter > (stage) but after sync 'AL' gets written to but GCC thinks the value > is still there and uses it to compute 'EAX' for 'cpuid'. > > smm_test can't fully use standard ucall() framework as we need to > write a very simple SMI handler there. Fix the immediate issue by > making RAX input/output operand. While on it, make sync_with_host() > static inline. > > Reported-by: Marcelo Bandeira Condotta <mcondotta@redhat.com> > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > --- > tools/testing/selftests/kvm/x86_64/smm_test.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/kvm/x86_64/smm_test.c b/tools/testing/selftests/kvm/x86_64/smm_test.c > index 36314152943d..ae39a220609f 100644 > --- a/tools/testing/selftests/kvm/x86_64/smm_test.c > +++ b/tools/testing/selftests/kvm/x86_64/smm_test.c > @@ -47,10 +47,10 @@ uint8_t smi_handler[] = { > 0x0f, 0xaa, /* rsm */ > }; > > -void sync_with_host(uint64_t phase) > +static inline void sync_with_host(uint64_t phase) > { > asm volatile("in $" XSTR(SYNC_PORT)", %%al \n" > - : : "a" (phase)); > + : "+a" (phase)); > } > > void self_smi(void) > Queued, thanks. Paolo
diff --git a/tools/testing/selftests/kvm/x86_64/smm_test.c b/tools/testing/selftests/kvm/x86_64/smm_test.c index 36314152943d..ae39a220609f 100644 --- a/tools/testing/selftests/kvm/x86_64/smm_test.c +++ b/tools/testing/selftests/kvm/x86_64/smm_test.c @@ -47,10 +47,10 @@ uint8_t smi_handler[] = { 0x0f, 0xaa, /* rsm */ }; -void sync_with_host(uint64_t phase) +static inline void sync_with_host(uint64_t phase) { asm volatile("in $" XSTR(SYNC_PORT)", %%al \n" - : : "a" (phase)); + : "+a" (phase)); } void self_smi(void)
It was reported that older GCCs compile smm_test in a way that breaks it completely: kvm_exit: reason EXIT_CPUID rip 0x4014db info 0 0 func 7ffffffd idx 830 rax 0 rbx 0 rcx 0 rdx 0, cpuid entry not found ... kvm_exit: reason EXIT_MSR rip 0x40abd9 info 0 0 kvm_msr: msr_read 487 = 0x0 (#GP) ... Note, '7ffffffd' was supposed to be '80000001' as we're checking for SVM. Dropping '-O2' from compiler flags help. Turns out, asm block in sync_with_host() is wrong. We us 'in 0xe, %%al' instruction to sync with the host and in 'AL' register we actually pass the parameter (stage) but after sync 'AL' gets written to but GCC thinks the value is still there and uses it to compute 'EAX' for 'cpuid'. smm_test can't fully use standard ucall() framework as we need to write a very simple SMI handler there. Fix the immediate issue by making RAX input/output operand. While on it, make sync_with_host() static inline. Reported-by: Marcelo Bandeira Condotta <mcondotta@redhat.com> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- tools/testing/selftests/kvm/x86_64/smm_test.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)