Skip to contentSkip to conversation

KVM: selftests: Fix Clang build issues in KVM_ASM_SAFE()

PATCH Applied David Matlack 3 participants 9 messages Jul 22, 2022 – Aug 12, 2022

Subsystems (2)
KVM Kselftest
lore
Share & tools
lore

David Matlack

author
#0 lore raw

cover KVM: selftests: Fix Clang build issues in KVM_ASM_SAFE()

The LLVM integrated assembler has some differences from the GNU assembler, and recent commit 3b23054cd3f5 ("KVM: selftests: Add x86-64 support for exception fixup") seems to be tripping over a few of those differences.

David Matlack (2):
KVM: selftests: Fix KVM_EXCEPTION_MAGIC build with Clang
KVM: selftests: Fix ambiguous mov in KVM_ASM_SAFE()

 tools/testing/selftests/kvm/include/x86_64/processor.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

base-commit: 1a4d88a361af4f2e91861d632c6a1fe87a9665c2

Signature · 1 line
2.37.1.359.gd136c6c3e2-goog
author
#1 lore raw

PATCH 1/2 KVM: selftests: Fix KVM_EXCEPTION_MAGIC build with Clang

Change KVM_EXCEPTION_MAGIC to use the all-caps "ULL", rather than lower case. This fixes a build failure with Clang:

In file included from x86_64/hyperv_features.c:13:
include/x86_64/processor.h:825:9: error: unexpected token in argument list

          return kvm_asm_safe("wrmsr", "a"(val & -1u), "d"(val >> 32), "c"(msr));
                 ^
  include/x86_64/processor.h:802:15: note: expanded from macro 'kvm_asm_safe'
          asm volatile(KVM_ASM_SAFE(insn)                 \
                       ^
  include/x86_64/processor.h:785:2: note: expanded from macro 'KVM_ASM_SAFE'
          "mov $" __stringify(KVM_EXCEPTION_MAGIC) ", %%r9\n\t"   \
          ^
  <inline asm>:1:18: note: instantiated into assembly here
          mov $0xabacadabaull, %r9
                          ^
Fixes 3b23054cd3f5 ("KVM: selftests: Add x86-64 support for exception fixup")
Signed-off-by David Matlack <[email protected]>
---
 tools/testing/selftests/kvm/include/x86_64/processor.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/testing/selftests/kvm/include/x86_64/processor.h +1 -1
index 45edf45821d0..51c6661aca77 100644
@@ -754,7 +754,7 @@ void vm_install_exception_handler(struct kvm_vm *vm, int vector,
 			void (*handler)(struct ex_regs *));
 
 /* If a toddler were to say "abracadabra". */
-#define KVM_EXCEPTION_MAGIC 0xabacadabaull
+#define KVM_EXCEPTION_MAGIC 0xabacadabaULL
 
 /*
  * KVM selftest exception fixup uses registers to coordinate with the exception
Signature · 1 line
2.37.1.359.gd136c6c3e2-goog
reviewer Reviewed-by on v1 1/2
#2 lore raw

Re: [PATCH 1/2] KVM: selftests: Fix KVM_EXCEPTION_MAGIC build with Clang

Quoted 17 lines · David Matlack

Change KVM_EXCEPTION_MAGIC to use the all-caps "ULL", rather than lower case. This fixes a build failure with Clang:

In file included from x86_64/hyperv_features.c:13:
include/x86_64/processor.h:825:9: error: unexpected token in argument list

          return kvm_asm_safe("wrmsr", "a"(val & -1u), "d"(val >> 32), "c"(msr));
                 ^
  include/x86_64/processor.h:802:15: note: expanded from macro 'kvm_asm_safe'
          asm volatile(KVM_ASM_SAFE(insn)                 \
                       ^
  include/x86_64/processor.h:785:2: note: expanded from macro 'KVM_ASM_SAFE'
          "mov $" __stringify(KVM_EXCEPTION_MAGIC) ", %%r9\n\t"   \
          ^
  <inline asm>:1:18: note: instantiated into assembly here
          mov $0xabacadabaull, %r9
                          ^
Quoted 3 lines
Fixes 3b23054cd3f5 ("KVM: selftests: Add x86-64 support for exception fixup")
Signed-off-by David Matlack <[email protected]>

---

Reviewed-by Sean Christopherson <[email protected]>

tools/testing/selftests/kvm/include/x86_64/processor.h void vm_install_exception_handler(struct kvm_vm *vm, int vector, · line 757

Quoted 13 lines
 tools/testing/selftests/kvm/include/x86_64/processor.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/testing/selftests/kvm/include/x86_64/processor.h +1 -1
index 45edf45821d0..51c6661aca77 100644
@@ -754,7 +754,7 @@ void vm_install_exception_handler(struct kvm_vm *vm, int vector,
 			void (*handler)(struct ex_regs *));
 
 /* If a toddler were to say "abracadabra". */
-#define KVM_EXCEPTION_MAGIC 0xabacadabaull
+#define KVM_EXCEPTION_MAGIC 0xabacadabaULL

Really?!?!?! That's what makes clang happy?!?!?

author
#3 lore raw

Re: [PATCH 1/2] KVM: selftests: Fix KVM_EXCEPTION_MAGIC build with Clang

Quoted 35 lines · Sean Christopherson
Quoted 17 lines · David Matlack

Change KVM_EXCEPTION_MAGIC to use the all-caps "ULL", rather than lower case. This fixes a build failure with Clang:

In file included from x86_64/hyperv_features.c:13:
include/x86_64/processor.h:825:9: error: unexpected token in argument list

          return kvm_asm_safe("wrmsr", "a"(val & -1u), "d"(val >> 32), "c"(msr));
                 ^
  include/x86_64/processor.h:802:15: note: expanded from macro 'kvm_asm_safe'
          asm volatile(KVM_ASM_SAFE(insn)                 \
                       ^
  include/x86_64/processor.h:785:2: note: expanded from macro 'KVM_ASM_SAFE'
          "mov $" __stringify(KVM_EXCEPTION_MAGIC) ", %%r9\n\t"   \
          ^
  <inline asm>:1:18: note: instantiated into assembly here
          mov $0xabacadabaull, %r9
                          ^
Quoted 3 lines
Fixes 3b23054cd3f5 ("KVM: selftests: Add x86-64 support for exception fixup")
Signed-off-by David Matlack <[email protected]>

---

Reviewed-by Sean Christopherson <[email protected]>
Quoted 10 lines
 tools/testing/selftests/kvm/include/x86_64/processor.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/testing/selftests/kvm/include/x86_64/processor.h +0 -0
index 45edf45821d0..51c6661aca77 100644
@@ -754,7 +754,7 @@ void vm_install_exception_handler(struct kvm_vm *vm, int vector,
                      void (*handler)(struct ex_regs *));
Quoted 5 lines

tools/testing/selftests/kvm/include/x86_64/processor.h void vm_install_exception_handler(struct kvm_vm *vm, int vector, · line 757

Quoted 3 lines
tools/testing/selftests/kvm/include/x86_64/processor.h +1 -1
 /* If a toddler were to say "abracadabra". */
-#define KVM_EXCEPTION_MAGIC 0xabacadabaull
+#define KVM_EXCEPTION_MAGIC 0xabacadabaULL

Really?!?!?! That's what makes clang happy?!?!?

Heh, yup. I was surprised when it worked.

Hopefully some of the Clang folks CC'd can shed some light on why.

author
#4 lore raw

PATCH 2/2 KVM: selftests: Fix ambiguous mov in KVM_ASM_SAFE()

In reply to David Matlack

Change the mov in KVM_ASM_SAFE() that zeroes @vector to a movb to make it unambiguous.

This fixes a build failure with Clang since, unlike the GNU assembler, the LLVM integrated assembler rejects ambiguous X86 instructions that don't have suffixes:

In file included from x86_64/hyperv_features.c:13:
include/x86_64/processor.h:825:9: error: ambiguous instructions require an explicit suffix (could be 'movb', 'movw', 'movl', or 'movq')

          return kvm_asm_safe("wrmsr", "a"(val & -1u), "d"(val >> 32), "c"(msr));
                 ^
  include/x86_64/processor.h:802:15: note: expanded from macro 'kvm_asm_safe'
          asm volatile(KVM_ASM_SAFE(insn)                 \
                       ^
  include/x86_64/processor.h:788:16: note: expanded from macro 'KVM_ASM_SAFE'
          "1: " insn "\n\t"                                       \
                        ^
  <inline asm>:5:2: note: instantiated into assembly here
          mov $0, 15(%rsp)
          ^

It seems like this change could introduce undesirable behavior in the future, e.g. if someone used a type larger than a u8 for @vector, since KVM_ASM_SAFE() will only zero the bottom byte. I tried changing the type of @vector to an int to see what would happen. GCC failed to compile due to a size mismatch between movb and %eax. Clang succeeded in compiling, but the generated code looked correct, so perhaps it will not be an issue. That being said it seems like there could be a better solution to this issue that does not assume @vector is a u8.

Fixes 3b23054cd3f5 ("KVM: selftests: Add x86-64 support for exception fixup")
Signed-off-by David Matlack <[email protected]>
---
 tools/testing/selftests/kvm/include/x86_64/processor.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/testing/selftests/kvm/include/x86_64/processor.h +1 -1
index 51c6661aca77..0cbc71b7af50 100644
@@ -786,7 +786,7 @@ void vm_install_exception_handler(struct kvm_vm *vm, int vector,
 	"lea 1f(%%rip), %%r10\n\t"				\
 	"lea 2f(%%rip), %%r11\n\t"				\
 	"1: " insn "\n\t"					\
-	"mov $0, %[vector]\n\t"					\
+	"movb $0, %[vector]\n\t"				\
 	"jmp 3f\n\t"						\
 	"2:\n\t"						\
 	"mov  %%r9b, %[vector]\n\t"				\
Signature · 1 line
2.37.1.359.gd136c6c3e2-goog
reviewer Reviewed-by on v1 2/2
#5 lore raw

Re: [PATCH 2/2] KVM: selftests: Fix ambiguous mov in KVM_ASM_SAFE()

Quoted 21 lines · David Matlack

Change the mov in KVM_ASM_SAFE() that zeroes @vector to a movb to make it unambiguous.

This fixes a build failure with Clang since, unlike the GNU assembler, the LLVM integrated assembler rejects ambiguous X86 instructions that don't have suffixes:

In file included from x86_64/hyperv_features.c:13:
include/x86_64/processor.h:825:9: error: ambiguous instructions require an explicit suffix (could be 'movb', 'movw', 'movl', or 'movq')

          return kvm_asm_safe("wrmsr", "a"(val & -1u), "d"(val >> 32), "c"(msr));
                 ^
  include/x86_64/processor.h:802:15: note: expanded from macro 'kvm_asm_safe'
          asm volatile(KVM_ASM_SAFE(insn)                 \
                       ^
  include/x86_64/processor.h:788:16: note: expanded from macro 'KVM_ASM_SAFE'
          "1: " insn "\n\t"                                       \
                        ^
  <inline asm>:5:2: note: instantiated into assembly here
          mov $0, 15(%rsp)
          ^
Quoted 8 lines

It seems like this change could introduce undesirable behavior in the future, e.g. if someone used a type larger than a u8 for @vector, since KVM_ASM_SAFE() will only zero the bottom byte. I tried changing the type of @vector to an int to see what would happen. GCC failed to compile due to a size mismatch between movb and %eax. Clang succeeded in compiling, but the generated code looked correct, so perhaps it will not be an issue. That being said it seems like there could be a better solution to this issue that does not assume @vector is a u8.

Hrm, IIRC my intent was to not care about the size of "vector", but that may just be revisionist thinking because the:

	"mov  %%r9b, %[vector]\n\t"				\

suggests it's nothing more than a screwed up. A static assert on the size would be nice, but I don't know how to make that work since the macros dump directly into the asm.

Quoted 2 lines
Fixes 3b23054cd3f5 ("KVM: selftests: Add x86-64 support for exception fixup")
Signed-off-by David Matlack <[email protected]>
Reviewed-by Sean Christopherson <[email protected]>
author
#6 lore raw
In reply to David Matlack
Quoted 5 lines · David Matlack

The LLVM integrated assembler has some differences from the GNU assembler, and recent commit 3b23054cd3f5 ("KVM: selftests: Add x86-64 support for exception fixup") seems to be tripping over a few of those differences.

Paolo, what do you think about these changes? I'm still hitting the mentioned Clang build issues on kvm/master, commit 93472b797153 ("Merge tag 'kvmarm-5.20' of
git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm into HEAD").

Quoted 13 lines

David Matlack (2):
KVM: selftests: Fix KVM_EXCEPTION_MAGIC build with Clang
KVM: selftests: Fix ambiguous mov in KVM_ASM_SAFE()

 tools/testing/selftests/kvm/include/x86_64/processor.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

base-commit: 1a4d88a361af4f2e91861d632c6a1fe87a9665c2 --
2.37.1.359.gd136c6c3e2-goog

maintainer
#7 lore raw
Quoted 7 lines · David Matlack
Quoted 5 lines · David Matlack

The LLVM integrated assembler has some differences from the GNU assembler, and recent commit 3b23054cd3f5 ("KVM: selftests: Add x86-64 support for exception fixup") seems to be tripping over a few of those differences.

Quoted 5 lines

Paolo, what do you think about these changes? I'm still hitting the mentioned Clang build issues on kvm/master, commit 93472b797153 ("Merge tag 'kvmarm-5.20' of
git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm into HEAD").

I had missed them, so thanks for pinging.

Paolo