[PATCH 0/2] KVM: selftests: Fix Clang build issues in KVM_ASM_SAFE()

[PATCH 0/2] KVM: selftests: Fix Clang build issues in KVM_ASM_SAFE()

[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.

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

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

[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
                          ^

Fixes: 3b23054cd3f5 ("KVM: selftests: Add x86-64 support for exception fixup")
Signed-off-by: David Matlack <dmatlack@google.com>
---
 tools/testing/selftests/kvm/include/x86_64/processor.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index 45edf45821d0..51c6661aca77 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -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
-- 
2.37.1.359.gd136c6c3e2-goog

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

[Sean Christopherson]

On Fri, Jul 22, 2022, David Matlack wrote:
> 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 <dmatlack@google.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

>  tools/testing/selftests/kvm/include/x86_64/processor.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
> index 45edf45821d0..51c6661aca77 100644
> --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
> +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
> @@ -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?!?!?

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

[David Matlack]

On Fri, Jul 22, 2022 at 5:01 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Fri, Jul 22, 2022, David Matlack wrote:
> > 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 <dmatlack@google.com>
> > ---
>
> Reviewed-by: Sean Christopherson <seanjc@google.com>
>
> >  tools/testing/selftests/kvm/include/x86_64/processor.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
> > index 45edf45821d0..51c6661aca77 100644
> > --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
> > +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
> > @@ -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?!?!?

Heh, yup. I was surprised when it worked.

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

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

[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 <dmatlack@google.com>
---
 tools/testing/selftests/kvm/include/x86_64/processor.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index 51c6661aca77..0cbc71b7af50 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -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"				\
-- 
2.37.1.359.gd136c6c3e2-goog

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

[Sean Christopherson]

On Fri, Jul 22, 2022, David Matlack wrote:
> 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.

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.

> Fixes: 3b23054cd3f5 ("KVM: selftests: Add x86-64 support for exception fixup")
> Signed-off-by: David Matlack <dmatlack@google.com>

Reviewed-by: Sean Christopherson <seanjc@google.com>

Re: [PATCH 0/2] KVM: selftests: Fix Clang build issues in KVM_ASM_SAFE()

[David Matlack]

On Fri, Jul 22, 2022 at 4:48 PM David Matlack <dmatlack@google.com> wrote:
>
> 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").

>
> 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
>

Re: [PATCH 0/2] KVM: selftests: Fix Clang build issues in KVM_ASM_SAFE()

[Paolo Bonzini]

On 8/11/22 00:53, David Matlack wrote:
> On Fri, Jul 22, 2022 at 4:48 PM David Matlack <dmatlack@google.com> wrote:
>>
>> 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").

I had missed them, so thanks for pinging.

Paolo

Re: [PATCH 0/2] KVM: selftests: Fix Clang build issues in KVM_ASM_SAFE()

[Paolo Bonzini]

Queued, thanks.

Paolo