On Wed, Oct 07, 2020 at 04:43:12PM +0200, Uros Bizjak wrote:
> Move the big inline assembly block from nested_vmx_check_vmentry_hw
> to vmenter.S assembly file, taking into account all ABI requirements.
>
> The new function is modelled after __vmx_vcpu_run, and also calls
> vmx_update_host_rsp instead of open-coding the function in assembly.
Is there specific motivation for this change? The inline asm is ugly, but
it's contained.
If we really want to get rid of the inline asm, I'd probably vote to simply
use __vmx_vcpu_run() instead of adding another assembly helper. The (double)
GPR save/restore is wasteful, but this flow is basically anti-performance
anyways. Outside of KVM developers, I doubt anyone actually enables this path.
> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Cc: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> Signed-off-by: Uros Bizjak <ubizjak@xxxxxxxxx>
> ---
> arch/x86/kvm/vmx/nested.c | 32 +++-----------------------------
> arch/x86/kvm/vmx/vmenter.S | 36 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 39 insertions(+), 29 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 1bb6b31eb646..7b26e983e31c 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -3012,6 +3012,8 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
> return 0;
> }
>
> +bool __nested_vmx_check_vmentry_hw(struct vcpu_vmx *vmx, bool launched);
> +
> static int nested_vmx_check_vmentry_hw(struct kvm_vcpu *vcpu)
> {
> struct vcpu_vmx *vmx = to_vmx(vcpu);
> @@ -3050,35 +3052,7 @@ static int nested_vmx_check_vmentry_hw(struct kvm_vcpu *vcpu)
> vmx->loaded_vmcs->host_state.cr4 = cr4;
> }
>
> - asm(
> - "sub $%c[wordsize], %%" _ASM_SP "\n\t" /* temporarily adjust RSP for CALL */
> - "cmp %%" _ASM_SP ", %c[host_state_rsp](%[loaded_vmcs]) \n\t"
> - "je 1f \n\t"
> - __ex("vmwrite %%" _ASM_SP ", %[HOST_RSP]") "\n\t"
> - "mov %%" _ASM_SP ", %c[host_state_rsp](%[loaded_vmcs]) \n\t"
> - "1: \n\t"
> - "add $%c[wordsize], %%" _ASM_SP "\n\t" /* un-adjust RSP */
> -
> - /* Check if vmlaunch or vmresume is needed */
> - "cmpb $0, %c[launched](%[loaded_vmcs])\n\t"
> -
> - /*
> - * VMLAUNCH and VMRESUME clear RFLAGS.{CF,ZF} on VM-Exit, set
> - * RFLAGS.CF on VM-Fail Invalid and set RFLAGS.ZF on VM-Fail
> - * Valid. vmx_vmenter() directly "returns" RFLAGS, and so the
> - * results of VM-Enter is captured via CC_{SET,OUT} to vm_fail.
> - */
> - "call vmx_vmenter\n\t"
> -
> - CC_SET(be)
> - : ASM_CALL_CONSTRAINT, CC_OUT(be) (vm_fail)
> - : [HOST_RSP]"r"((unsigned long)HOST_RSP),
> - [loaded_vmcs]"r"(vmx->loaded_vmcs),
> - [launched]"i"(offsetof(struct loaded_vmcs, launched)),
> - [host_state_rsp]"i"(offsetof(struct loaded_vmcs, host_state.rsp)),
> - [wordsize]"i"(sizeof(ulong))
> - : "memory"
> - );
> + vm_fail = __nested_vmx_check_vmentry_hw(vmx, vmx->loaded_vmcs->launched);
>
> if (vmx->msr_autoload.host.nr)
> vmcs_write32(VM_EXIT_MSR_LOAD_COUNT, vmx->msr_autoload.host.nr);
> diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
> index 799db084a336..9fdcbd9320dc 100644
> --- a/arch/x86/kvm/vmx/vmenter.S
> +++ b/arch/x86/kvm/vmx/vmenter.S
> @@ -234,6 +234,42 @@ SYM_FUNC_START(__vmx_vcpu_run)
> jmp 1b
> SYM_FUNC_END(__vmx_vcpu_run)
>
> +/**
> + * __nested_vmx_check_vmentry_hw - Run a vCPU via a transition to
> + * a nested VMX guest mode
This function comment is incorrect, this helper doesn't run the vCPU, it
simply executes VMLAUNCH or VMRESUME, which are expected to fail (and we're
in BUG_ON territory if they don't).
> + * @vmx: struct vcpu_vmx * (forwarded to vmx_update_host_rsp)
> + * @launched: %true if the VMCS has been launched
> + *
> + * Returns:
> + * 0 on VM-Exit, 1 on VM-Fail
> + */
> +SYM_FUNC_START(__nested_vmx_check_vmentry_hw)
> + push %_ASM_BP
> + mov %_ASM_SP, %_ASM_BP
> +
> + push %_ASM_BX
> +
> + /* Copy @launched to BL, _ASM_ARG2 is volatile. */
> + mov %_ASM_ARG2B, %bl
> +
> + /* Adjust RSP to account for the CALL to vmx_vmenter(). */
> + lea -WORD_SIZE(%_ASM_SP), %_ASM_ARG2
> + call vmx_update_host_rsp
> +
> + /* Check if vmlaunch or vmresume is needed */
> + cmpb $0, %bl
> +
> + /* Enter guest mode */
> + call vmx_vmenter
> +
> + /* Return 0 on VM-Exit, 1 on VM-Fail */
> + setbe %al
> +
> + pop %_ASM_BX
> +
> + pop %_ASM_BP
> + ret
> +SYM_FUNC_END(__nested_vmx_check_vmentry_hw)
>
> .section .text, "ax"
>
> --
> 2.26.2
>
