| Message ID | 20200326160712.28803-1-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: VMX: Add a trampoline to fix VMREAD error handling | expand |
On 26/03/20 17:07, Sean Christopherson wrote: > Add a hand coded assembly trampoline to preserve volatile registers > across vmread_error(), and to handle the calling convention differences > between 64-bit and 32-bit due to asmlinkage on vmread_error(). Pass > @field and @fault on the stack when invoking the trampoline to avoid > clobbering volatile registers in the context of the inline assembly. > > Calling vmread_error() directly from inline assembly is partially broken > on 64-bit, and completely broken on 32-bit. On 64-bit, it will clobber > %rdi and %rsi (used to pass @field and @fault) and any volatile regs > written by vmread_error(). On 32-bit, asmlinkage means vmread_error() > expects the parameters to be passed on the stack, not via regs. > > Opportunistically zero out the result in the trampoline to save a few > bytes of code for every VMREAD. A happy side effect of the trampoline > is that the inline code footprint is reduced by three bytes on 64-bit > due to PUSH/POP being more efficent (in terms of opcode bytes) than MOV. > > Fixes: 6e2020977e3e6 ("KVM: VMX: Add error handling to VMREAD helper") > Cc: stable@vger.kernel.org > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > > Becuase there just isn't enough custom assembly in VMX :-) > > Simply reverting isn't a great option because we'd lose error reporting > for VMREAD failure, i.e. it'd return garbage with no other indication that > something went awry. > > Tested all paths (fail, fault w/o rebooting, fault w/ rebooting) on both > 64-bit and 32-bit. > > arch/x86/kvm/vmx/ops.h | 28 +++++++++++++----- > arch/x86/kvm/vmx/vmenter.S | 58 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 79 insertions(+), 7 deletions(-) > > diff --git a/arch/x86/kvm/vmx/ops.h b/arch/x86/kvm/vmx/ops.h > index 45eaedee2ac0..09b0937d56b1 100644 > --- a/arch/x86/kvm/vmx/ops.h > +++ b/arch/x86/kvm/vmx/ops.h > @@ -12,7 +12,8 @@ > > #define __ex(x) __kvm_handle_fault_on_reboot(x) > > -asmlinkage void vmread_error(unsigned long field, bool fault); > +__attribute__((regparm(0))) void vmread_error_trampoline(unsigned long field, > + bool fault); > void vmwrite_error(unsigned long field, unsigned long value); > void vmclear_error(struct vmcs *vmcs, u64 phys_addr); > void vmptrld_error(struct vmcs *vmcs, u64 phys_addr); > @@ -70,15 +71,28 @@ static __always_inline unsigned long __vmcs_readl(unsigned long field) > asm volatile("1: vmread %2, %1\n\t" > ".byte 0x3e\n\t" /* branch taken hint */ > "ja 3f\n\t" > - "mov %2, %%" _ASM_ARG1 "\n\t" > - "xor %%" _ASM_ARG2 ", %%" _ASM_ARG2 "\n\t" > - "2: call vmread_error\n\t" > - "xor %k1, %k1\n\t" > + > + /* > + * VMREAD failed. Push '0' for @fault, push the failing > + * @field, and bounce through the trampoline to preserve > + * volatile registers. > + */ > + "push $0\n\t" > + "push %2\n\t" > + "2:call vmread_error_trampoline\n\t" > + > + /* > + * Unwind the stack. Note, the trampoline zeros out the > + * memory for @fault so that the result is '0' on error. > + */ > + "pop %2\n\t" > + "pop %1\n\t" > "3:\n\t" > > + /* VMREAD faulted. As above, except push '1' for @fault. */ > ".pushsection .fixup, \"ax\"\n\t" > - "4: mov %2, %%" _ASM_ARG1 "\n\t" > - "mov $1, %%" _ASM_ARG2 "\n\t" > + "4: push $1\n\t" > + "push %2\n\t" > "jmp 2b\n\t" > ".popsection\n\t" > _ASM_EXTABLE(1b, 4b) > diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S > index 81ada2ce99e7..861ae40e7144 100644 > --- a/arch/x86/kvm/vmx/vmenter.S > +++ b/arch/x86/kvm/vmx/vmenter.S > @@ -234,3 +234,61 @@ SYM_FUNC_START(__vmx_vcpu_run) > 2: mov $1, %eax > jmp 1b > SYM_FUNC_END(__vmx_vcpu_run) > + > +/** > + * vmread_error_trampoline - Trampoline from inline asm to vmread_error() > + * @field: VMCS field encoding that failed > + * @fault: %true if the VMREAD faulted, %false if it failed > + > + * Save and restore volatile registers across a call to vmread_error(). Note, > + * all parameters are passed on the stack. > + */ > +SYM_FUNC_START(vmread_error_trampoline) > + push %_ASM_BP > + mov %_ASM_SP, %_ASM_BP > + > + push %_ASM_AX > + push %_ASM_CX > + push %_ASM_DX > +#ifdef CONFIG_X86_64 > + push %rdi > + push %rsi > + push %r8 > + push %r9 > + push %r10 > + push %r11 > +#endif > +#ifdef CONFIG_X86_64 > + /* Load @field and @fault to arg1 and arg2 respectively. */ > + mov 3*WORD_SIZE(%rbp), %_ASM_ARG2 > + mov 2*WORD_SIZE(%rbp), %_ASM_ARG1 > +#else > + /* Parameters are passed on the stack for 32-bit (see asmlinkage). */ > + push 3*WORD_SIZE(%ebp) > + push 2*WORD_SIZE(%ebp) > +#endif > + > + call vmread_error > + > +#ifndef CONFIG_X86_64 > + add $8, %esp > +#endif > + > + /* Zero out @fault, which will be popped into the result register. */ > + _ASM_MOV $0, 3*WORD_SIZE(%_ASM_BP) > + > +#ifdef CONFIG_X86_64 > + pop %r11 > + pop %r10 > + pop %r9 > + pop %r8 > + pop %rsi > + pop %rdi > +#endif > + pop %_ASM_DX > + pop %_ASM_CX > + pop %_ASM_AX > + pop %_ASM_BP > + > + ret > +SYM_FUNC_END(vmread_error_trampoline) > Queued, thanks. Paolo
diff --git a/arch/x86/kvm/vmx/ops.h b/arch/x86/kvm/vmx/ops.h index 45eaedee2ac0..09b0937d56b1 100644 --- a/arch/x86/kvm/vmx/ops.h +++ b/arch/x86/kvm/vmx/ops.h @@ -12,7 +12,8 @@ #define __ex(x) __kvm_handle_fault_on_reboot(x) -asmlinkage void vmread_error(unsigned long field, bool fault); +__attribute__((regparm(0))) void vmread_error_trampoline(unsigned long field, + bool fault); void vmwrite_error(unsigned long field, unsigned long value); void vmclear_error(struct vmcs *vmcs, u64 phys_addr); void vmptrld_error(struct vmcs *vmcs, u64 phys_addr); @@ -70,15 +71,28 @@ static __always_inline unsigned long __vmcs_readl(unsigned long field) asm volatile("1: vmread %2, %1\n\t" ".byte 0x3e\n\t" /* branch taken hint */ "ja 3f\n\t" - "mov %2, %%" _ASM_ARG1 "\n\t" - "xor %%" _ASM_ARG2 ", %%" _ASM_ARG2 "\n\t" - "2: call vmread_error\n\t" - "xor %k1, %k1\n\t" + + /* + * VMREAD failed. Push '0' for @fault, push the failing + * @field, and bounce through the trampoline to preserve + * volatile registers. + */ + "push $0\n\t" + "push %2\n\t" + "2:call vmread_error_trampoline\n\t" + + /* + * Unwind the stack. Note, the trampoline zeros out the + * memory for @fault so that the result is '0' on error. + */ + "pop %2\n\t" + "pop %1\n\t" "3:\n\t" + /* VMREAD faulted. As above, except push '1' for @fault. */ ".pushsection .fixup, \"ax\"\n\t" - "4: mov %2, %%" _ASM_ARG1 "\n\t" - "mov $1, %%" _ASM_ARG2 "\n\t" + "4: push $1\n\t" + "push %2\n\t" "jmp 2b\n\t" ".popsection\n\t" _ASM_EXTABLE(1b, 4b) diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S index 81ada2ce99e7..861ae40e7144 100644 --- a/arch/x86/kvm/vmx/vmenter.S +++ b/arch/x86/kvm/vmx/vmenter.S @@ -234,3 +234,61 @@ SYM_FUNC_START(__vmx_vcpu_run) 2: mov $1, %eax jmp 1b SYM_FUNC_END(__vmx_vcpu_run) + +/** + * vmread_error_trampoline - Trampoline from inline asm to vmread_error() + * @field: VMCS field encoding that failed + * @fault: %true if the VMREAD faulted, %false if it failed + + * Save and restore volatile registers across a call to vmread_error(). Note, + * all parameters are passed on the stack. + */ +SYM_FUNC_START(vmread_error_trampoline) + push %_ASM_BP + mov %_ASM_SP, %_ASM_BP + + push %_ASM_AX + push %_ASM_CX + push %_ASM_DX +#ifdef CONFIG_X86_64 + push %rdi + push %rsi + push %r8 + push %r9 + push %r10 + push %r11 +#endif +#ifdef CONFIG_X86_64 + /* Load @field and @fault to arg1 and arg2 respectively. */ + mov 3*WORD_SIZE(%rbp), %_ASM_ARG2 + mov 2*WORD_SIZE(%rbp), %_ASM_ARG1 +#else + /* Parameters are passed on the stack for 32-bit (see asmlinkage). */ + push 3*WORD_SIZE(%ebp) + push 2*WORD_SIZE(%ebp) +#endif + + call vmread_error + +#ifndef CONFIG_X86_64 + add $8, %esp +#endif + + /* Zero out @fault, which will be popped into the result register. */ + _ASM_MOV $0, 3*WORD_SIZE(%_ASM_BP) + +#ifdef CONFIG_X86_64 + pop %r11 + pop %r10 + pop %r9 + pop %r8 + pop %rsi + pop %rdi +#endif + pop %_ASM_DX + pop %_ASM_CX + pop %_ASM_AX + pop %_ASM_BP + + ret +SYM_FUNC_END(vmread_error_trampoline)
Add a hand coded assembly trampoline to preserve volatile registers across vmread_error(), and to handle the calling convention differences between 64-bit and 32-bit due to asmlinkage on vmread_error(). Pass @field and @fault on the stack when invoking the trampoline to avoid clobbering volatile registers in the context of the inline assembly. Calling vmread_error() directly from inline assembly is partially broken on 64-bit, and completely broken on 32-bit. On 64-bit, it will clobber %rdi and %rsi (used to pass @field and @fault) and any volatile regs written by vmread_error(). On 32-bit, asmlinkage means vmread_error() expects the parameters to be passed on the stack, not via regs. Opportunistically zero out the result in the trampoline to save a few bytes of code for every VMREAD. A happy side effect of the trampoline is that the inline code footprint is reduced by three bytes on 64-bit due to PUSH/POP being more efficent (in terms of opcode bytes) than MOV. Fixes: 6e2020977e3e6 ("KVM: VMX: Add error handling to VMREAD helper") Cc: stable@vger.kernel.org Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- Becuase there just isn't enough custom assembly in VMX :-) Simply reverting isn't a great option because we'd lose error reporting for VMREAD failure, i.e. it'd return garbage with no other indication that something went awry. Tested all paths (fail, fault w/o rebooting, fault w/ rebooting) on both 64-bit and 32-bit. arch/x86/kvm/vmx/ops.h | 28 +++++++++++++----- arch/x86/kvm/vmx/vmenter.S | 58 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 7 deletions(-)