Linux: error: too few operands for instruction in arch/x86/kvm/svm/sev.c

[nathanchance]

When using LLVM_IAS=1:

$ make -skj"$(nproc)" LLVM=1 LLVM_IAS=1 O=out distclean defconfig

$ scripts/config --file out/.config -e KVM -e KVM_AMD

$ make -skj"$(nproc)" LLVM=1 LLVM_IAS=1 O=out olddefconfig arch/x86/kvm/svm/sev.o
../arch/x86/kvm/svm/sev.c:2004:15: error: too few operands for instruction
        asm volatile(__ex("vmsave") : : "a" (__sme_page_pa(sd->save_area)) : "memory");
                     ^
../arch/x86/kvm/svm/sev.c:28:17: note: expanded from macro '__ex'
#define __ex(x) __kvm_handle_fault_on_reboot(x)
                ^
../arch/x86/include/asm/kvm_host.h:1646:10: note: expanded from macro '__kvm_handle_fault_on_reboot'
        "666: \n\t"                                                     \
                ^
<inline asm>:2:2: note: instantiated into assembly here
        vmsave
        ^
1 error generated.
...

Introduced by https://git.kernel.org/next/linux-next/c/861377730aa9db4cbaa0f3bd3f4d295c152732c4.

[nathanchance]

cc @topperc

Looks like vmsave by itself should be treated as vmsave %rax for 64-bit and vmsave %eax for 32-bit? At least I think that is how GCC handles it and that is how the AMD architecture manual reads to me but this is far outside of my area of expertise.

[nathanchance]

This works in the meantime:

From 1757b6036e33d316e9f96db97ee22599a5498153 Mon Sep 17 00:00:00 2001
From: Nathan Chancellor <[email protected]>
Date: Fri, 18 Dec 2020 00:18:05 -0700
Subject: [PATCH] KVM: SVM: Make vmsave call compatible with LLVM's assembler

LLVM's assembler does not accept 'vmsave' alone, it needs the register
as well. This matches the other 'vmsave' call in vmenter.S.

Signed-off-by: Nathan Chancellor <[email protected]>
---
 arch/x86/kvm/svm/sev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 9858d5ae9ddd..5ae49e9ee63d 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2001,7 +2001,7 @@ void sev_es_vcpu_load(struct vcpu_svm *svm, int cpu)
     * of which one step is to perform a VMLOAD. Since hardware does not
     * perform a VMSAVE on VMRUN, the host savearea must be updated.
     */
-   asm volatile(__ex("vmsave") : : "a" (__sme_page_pa(sd->save_area)) : "memory");
+   asm volatile(__ex("vmsave %%"_ASM_AX) : : "a" (__sme_page_pa(sd->save_area)) : "memory");

    /*
     * Certain MSRs are restored on VMEXIT, only save ones that aren't
-- 
2.29.2

[topperc]

I'll fix LLVM

[topperc]

Incomplete patch is here https://reviews.llvm.org/D93524 Need to fix up the tests still.

[nathanchance]

@topperc thank you for the quick fix! I will try to test it soon.

I am curious if we should still try to patch the kernel though. LTO depends on LLVM_IAS=1 and we currently gate LTO at clang 11.0.0 and newer and we will have no way to get that fix into release/11.x. cc @samitolvanen for some input.

[samitolvanen]

I am curious if we should still try to patch the kernel though.

Yes, please send out the patch. This looks like a reasonable fix and upstream has been happy to accept similar patches before.

[nathanchance]

Patch submitted on the kernel side: https://lore.kernel.org/r/[email protected]

[nickdesaulniers]

thanks @topperc and @nathanchance

[nickdesaulniers]

Sounds like the kernel patch was accepted, but I don't think it's been pushed anywhere public yet?

[dileks]

I fell over the same issue in Linux v5.11-rc1 with LLVM toolchain v11.0.1-rc2 when building a clang-ias Debian kernel.

[dileks]

@topperc

Should your fix for LLVM be backported to release/11.x?

[nathanchance]

@tstellar seemed to indicate previously that it is too late to add fixes to release/11.x. That is why I figured that pushing for a kernel side fix was also worthwhile. I still don't see my patch pushed anywhere but I assume it should be in 5.11-rc2.

[dileks]

@nathanchance

Linus himself answered me (see link below):

Looks like Paolo already picked up the fix, so this should be fixed
when I get the next kvm pull request.

That said, you might want to make sure the  LLVM people know about this too.

The whole "implicit arguments" is a common thing in x86, where lots of
instructions don't need to spell them out explicitly, because of fixed
register allocation (example: divide/multiply instructions only work
with ax/dx as a target etc).

Link: https://marc.info/?l=linux-kernel&m=160919745906825&w=2

[nathanchance]

Which has already been done so it seems like we are good :)

[dileks]

s/good/the best

[dileks]

Cherry-picked commit f47b07315a3c ("[X86] Teach assembler to accept vmsave/vmload/vmrun/invlpga/skinit with or without the fixed register operands") from LLVM upstream on top of LLVM version 11.0.1-rc2 fixes the issue for me - without the need of the kernel-side fix from @nathanchance.

[nathanchance]

Kernel patch has been merged: https://git.kernel.org/linus/f65cf84ee769767536dc367acc9568ddb6e4c9f4