* [PATCH] KVM/nVMX: Use __vmx_vcpu_run in nested_vmx_check_vmentry_hw
@ 2020-10-29 13:41 Uros Bizjak
2020-12-16 9:24 ` Uros Bizjak
0 siblings, 1 reply; 8+ messages in thread
From: Uros Bizjak @ 2020-10-29 13:41 UTC (permalink / raw)
To: kvm; +Cc: Uros Bizjak, Paolo Bonzini, Sean Christopherson
Replace inline assembly in nested_vmx_check_vmentry_hw
with a call to __vmx_vcpu_run. The function is not
performance critical, so (double) GPR save/restore
in __vmx_vcpu_run can be tolerated, as far as performance
effects are concerned.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
---
arch/x86/kvm/vmx/nested.c | 32 +++-----------------------------
arch/x86/kvm/vmx/vmx.c | 2 --
arch/x86/kvm/vmx/vmx.h | 1 +
3 files changed, 4 insertions(+), 31 deletions(-)
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 89af692deb7e..6ab62bf277c4 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -12,6 +12,7 @@
#include "nested.h"
#include "pmu.h"
#include "trace.h"
+#include "vmx.h"
#include "x86.h"
static bool __read_mostly enable_shadow_vmcs = 1;
@@ -3056,35 +3057,8 @@ 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 = __vmx_vcpu_run(vmx, (unsigned long *)&vcpu->arch.regs,
+ 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/vmx.c b/arch/x86/kvm/vmx/vmx.c
index d14c94d0aff1..0f390c748b18 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6591,8 +6591,6 @@ static fastpath_t vmx_exit_handlers_fastpath(struct kvm_vcpu *vcpu)
}
}
-bool __vmx_vcpu_run(struct vcpu_vmx *vmx, unsigned long *regs, bool launched);
-
static noinstr void vmx_vcpu_enter_exit(struct kvm_vcpu *vcpu,
struct vcpu_vmx *vmx)
{
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index f6f66e5c6510..32db3b033e9b 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -339,6 +339,7 @@ void vmx_set_virtual_apic_mode(struct kvm_vcpu *vcpu);
struct vmx_uret_msr *vmx_find_uret_msr(struct vcpu_vmx *vmx, u32 msr);
void pt_update_intercept_for_msr(struct kvm_vcpu *vcpu);
void vmx_update_host_rsp(struct vcpu_vmx *vmx, unsigned long host_rsp);
+bool __vmx_vcpu_run(struct vcpu_vmx *vmx, unsigned long *regs, bool launched);
int vmx_find_loadstore_msr_slot(struct vmx_msrs *m, u32 msr);
void vmx_ept_load_pdptrs(struct kvm_vcpu *vcpu);
--
2.26.2
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] KVM/nVMX: Use __vmx_vcpu_run in nested_vmx_check_vmentry_hw
2020-10-29 13:41 [PATCH] KVM/nVMX: Use __vmx_vcpu_run in nested_vmx_check_vmentry_hw Uros Bizjak
@ 2020-12-16 9:24 ` Uros Bizjak
2020-12-16 20:15 ` Krish Sadhukhan
2020-12-17 18:04 ` Sean Christopherson
0 siblings, 2 replies; 8+ messages in thread
From: Uros Bizjak @ 2020-12-16 9:24 UTC (permalink / raw)
To: kvm; +Cc: Paolo Bonzini, Sean Christopherson
Ping. This patch didn't receive any feedback.
Thanks,
Uros.
On Thu, Oct 29, 2020 at 2:41 PM Uros Bizjak <ubizjak@gmail.com> wrote:
>
> Replace inline assembly in nested_vmx_check_vmentry_hw
> with a call to __vmx_vcpu_run. The function is not
> performance critical, so (double) GPR save/restore
> in __vmx_vcpu_run can be tolerated, as far as performance
> effects are concerned.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> ---
> arch/x86/kvm/vmx/nested.c | 32 +++-----------------------------
> arch/x86/kvm/vmx/vmx.c | 2 --
> arch/x86/kvm/vmx/vmx.h | 1 +
> 3 files changed, 4 insertions(+), 31 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 89af692deb7e..6ab62bf277c4 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -12,6 +12,7 @@
> #include "nested.h"
> #include "pmu.h"
> #include "trace.h"
> +#include "vmx.h"
> #include "x86.h"
>
> static bool __read_mostly enable_shadow_vmcs = 1;
> @@ -3056,35 +3057,8 @@ 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 = __vmx_vcpu_run(vmx, (unsigned long *)&vcpu->arch.regs,
> + 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/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index d14c94d0aff1..0f390c748b18 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -6591,8 +6591,6 @@ static fastpath_t vmx_exit_handlers_fastpath(struct kvm_vcpu *vcpu)
> }
> }
>
> -bool __vmx_vcpu_run(struct vcpu_vmx *vmx, unsigned long *regs, bool launched);
> -
> static noinstr void vmx_vcpu_enter_exit(struct kvm_vcpu *vcpu,
> struct vcpu_vmx *vmx)
> {
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index f6f66e5c6510..32db3b033e9b 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -339,6 +339,7 @@ void vmx_set_virtual_apic_mode(struct kvm_vcpu *vcpu);
> struct vmx_uret_msr *vmx_find_uret_msr(struct vcpu_vmx *vmx, u32 msr);
> void pt_update_intercept_for_msr(struct kvm_vcpu *vcpu);
> void vmx_update_host_rsp(struct vcpu_vmx *vmx, unsigned long host_rsp);
> +bool __vmx_vcpu_run(struct vcpu_vmx *vmx, unsigned long *regs, bool launched);
> int vmx_find_loadstore_msr_slot(struct vmx_msrs *m, u32 msr);
> void vmx_ept_load_pdptrs(struct kvm_vcpu *vcpu);
>
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] KVM/nVMX: Use __vmx_vcpu_run in nested_vmx_check_vmentry_hw
2020-12-16 9:24 ` Uros Bizjak
@ 2020-12-16 20:15 ` Krish Sadhukhan
2020-12-16 21:05 ` Sean Christopherson
2020-12-17 18:04 ` Sean Christopherson
1 sibling, 1 reply; 8+ messages in thread
From: Krish Sadhukhan @ 2020-12-16 20:15 UTC (permalink / raw)
To: Uros Bizjak, kvm; +Cc: Paolo Bonzini, Sean Christopherson
On 12/16/20 1:24 AM, Uros Bizjak wrote:
> Ping. This patch didn't receive any feedback.
>
> Thanks,
> Uros.
>
> On Thu, Oct 29, 2020 at 2:41 PM Uros Bizjak <ubizjak@gmail.com> wrote:
>> Replace inline assembly in nested_vmx_check_vmentry_hw
>> with a call to __vmx_vcpu_run. The function is not
>> performance critical, so (double) GPR save/restore
>> in __vmx_vcpu_run can be tolerated, as far as performance
>> effects are concerned.
>>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Sean Christopherson <sean.j.christopherson@intel.com>
>> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
>> ---
>> arch/x86/kvm/vmx/nested.c | 32 +++-----------------------------
>> arch/x86/kvm/vmx/vmx.c | 2 --
>> arch/x86/kvm/vmx/vmx.h | 1 +
>> 3 files changed, 4 insertions(+), 31 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>> index 89af692deb7e..6ab62bf277c4 100644
>> --- a/arch/x86/kvm/vmx/nested.c
>> +++ b/arch/x86/kvm/vmx/nested.c
>> @@ -12,6 +12,7 @@
>> #include "nested.h"
>> #include "pmu.h"
>> #include "trace.h"
>> +#include "vmx.h"
>> #include "x86.h"
>>
>> static bool __read_mostly enable_shadow_vmcs = 1;
>> @@ -3056,35 +3057,8 @@ 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 = __vmx_vcpu_run(vmx, (unsigned long *)&vcpu->arch.regs,
>> + 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/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index d14c94d0aff1..0f390c748b18 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -6591,8 +6591,6 @@ static fastpath_t vmx_exit_handlers_fastpath(struct kvm_vcpu *vcpu)
>> }
>> }
>>
>> -bool __vmx_vcpu_run(struct vcpu_vmx *vmx, unsigned long *regs, bool launched);
>> -
>> static noinstr void vmx_vcpu_enter_exit(struct kvm_vcpu *vcpu,
>> struct vcpu_vmx *vmx)
>> {
>> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
>> index f6f66e5c6510..32db3b033e9b 100644
>> --- a/arch/x86/kvm/vmx/vmx.h
>> +++ b/arch/x86/kvm/vmx/vmx.h
>> @@ -339,6 +339,7 @@ void vmx_set_virtual_apic_mode(struct kvm_vcpu *vcpu);
>> struct vmx_uret_msr *vmx_find_uret_msr(struct vcpu_vmx *vmx, u32 msr);
>> void pt_update_intercept_for_msr(struct kvm_vcpu *vcpu);
>> void vmx_update_host_rsp(struct vcpu_vmx *vmx, unsigned long host_rsp);
>> +bool __vmx_vcpu_run(struct vcpu_vmx *vmx, unsigned long *regs, bool launched);
>> int vmx_find_loadstore_msr_slot(struct vmx_msrs *m, u32 msr);
>> void vmx_ept_load_pdptrs(struct kvm_vcpu *vcpu);
>>
>> --
>> 2.26.2
>>
Semantically __vmx_vcpu_run() is called to enter guest mode. In
nested_vmx_check_vmentry_hw(), we are not entering guest mode. Guest
mode is entered when nested_vmx_enter_non_root_mode() calls
enter_guest_mode().
Secondly, why not just replace the first half of the assembly block with
a call to vmx_update_host_rsp() and leave the rest as is ?
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] KVM/nVMX: Use __vmx_vcpu_run in nested_vmx_check_vmentry_hw
2020-12-16 20:15 ` Krish Sadhukhan
@ 2020-12-16 21:05 ` Sean Christopherson
0 siblings, 0 replies; 8+ messages in thread
From: Sean Christopherson @ 2020-12-16 21:05 UTC (permalink / raw)
To: Krish Sadhukhan; +Cc: Uros Bizjak, kvm, Paolo Bonzini
On Wed, Dec 16, 2020, Krish Sadhukhan wrote:
> > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > > index d14c94d0aff1..0f390c748b18 100644
> > > --- a/arch/x86/kvm/vmx/vmx.c
> > > +++ b/arch/x86/kvm/vmx/vmx.c
> > > @@ -6591,8 +6591,6 @@ static fastpath_t vmx_exit_handlers_fastpath(struct kvm_vcpu *vcpu)
> > > }
> > > }
> > >
> > > -bool __vmx_vcpu_run(struct vcpu_vmx *vmx, unsigned long *regs, bool launched);
> > > -
> > > static noinstr void vmx_vcpu_enter_exit(struct kvm_vcpu *vcpu,
> > > struct vcpu_vmx *vmx)
> > > {
> > > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> > > index f6f66e5c6510..32db3b033e9b 100644
> > > --- a/arch/x86/kvm/vmx/vmx.h
> > > +++ b/arch/x86/kvm/vmx/vmx.h
> > > @@ -339,6 +339,7 @@ void vmx_set_virtual_apic_mode(struct kvm_vcpu *vcpu);
> > > struct vmx_uret_msr *vmx_find_uret_msr(struct vcpu_vmx *vmx, u32 msr);
> > > void pt_update_intercept_for_msr(struct kvm_vcpu *vcpu);
> > > void vmx_update_host_rsp(struct vcpu_vmx *vmx, unsigned long host_rsp);
> > > +bool __vmx_vcpu_run(struct vcpu_vmx *vmx, unsigned long *regs, bool launched);
> > > int vmx_find_loadstore_msr_slot(struct vmx_msrs *m, u32 msr);
> > > void vmx_ept_load_pdptrs(struct kvm_vcpu *vcpu);
> > >
> > > --
> > > 2.26.2
> > >
> Semantically __vmx_vcpu_run() is called to enter guest mode. In
> nested_vmx_check_vmentry_hw(), we are not entering guest mode. Guest mode is
> entered when nested_vmx_enter_non_root_mode() calls enter_guest_mode().
Naming aside, this patch intentionally redefines the semantics to mean "execute
VM-Enter that may or may not succeed". And as called out in the changelog, the
overhead of the GPR save/load/restore is tolerable; reusing code and avoiding
ugly inline asm is more important.
> Secondly, why not just replace the first half of the assembly block with a
> call to vmx_update_host_rsp() and leave the rest as is ?
As above, though not called out in the changelog, the goal is to move away from
the inline asm without introducing another asm subroutine.
Uros, I'll try to double check and review this later today.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] KVM/nVMX: Use __vmx_vcpu_run in nested_vmx_check_vmentry_hw
2020-12-16 9:24 ` Uros Bizjak
2020-12-16 20:15 ` Krish Sadhukhan
@ 2020-12-17 18:04 ` Sean Christopherson
2020-12-17 18:12 ` Uros Bizjak
1 sibling, 1 reply; 8+ messages in thread
From: Sean Christopherson @ 2020-12-17 18:04 UTC (permalink / raw)
To: Uros Bizjak; +Cc: kvm, Paolo Bonzini
For future patches, please Cc LKML (in additional to KVM) so that the automatic
archiving and patchwork stuff kicks in. Thanks!
On Wed, Dec 16, 2020, Uros Bizjak wrote:
> Ping. This patch didn't receive any feedback.
>
> On Thu, Oct 29, 2020 at 2:41 PM Uros Bizjak <ubizjak@gmail.com> wrote:
> >
> > Replace inline assembly in nested_vmx_check_vmentry_hw
> > with a call to __vmx_vcpu_run. The function is not
> > performance critical, so (double) GPR save/restore
> > in __vmx_vcpu_run can be tolerated, as far as performance
> > effects are concerned.
> >
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> > Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> > ---
vmx_vmenter() in vmx/vmenter.S can and should now use SYM_FUNC_START_LOCAL
instead of SYM_FUNC_LOCAL. Other than that nit:
Reviewed-and-tested-by: Sean Christopherson <seanjc@google.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] KVM/nVMX: Use __vmx_vcpu_run in nested_vmx_check_vmentry_hw
2020-12-17 18:04 ` Sean Christopherson
@ 2020-12-17 18:12 ` Uros Bizjak
0 siblings, 0 replies; 8+ messages in thread
From: Uros Bizjak @ 2020-12-17 18:12 UTC (permalink / raw)
To: Sean Christopherson; +Cc: kvm, Paolo Bonzini
On Thu, Dec 17, 2020 at 7:04 PM Sean Christopherson <seanjc@google.com> wrote:
>
> For future patches, please Cc LKML (in additional to KVM) so that the automatic
> archiving and patchwork stuff kicks in. Thanks!
>
> On Wed, Dec 16, 2020, Uros Bizjak wrote:
> > Ping. This patch didn't receive any feedback.
> >
> > On Thu, Oct 29, 2020 at 2:41 PM Uros Bizjak <ubizjak@gmail.com> wrote:
> > >
> > > Replace inline assembly in nested_vmx_check_vmentry_hw
> > > with a call to __vmx_vcpu_run. The function is not
> > > performance critical, so (double) GPR save/restore
> > > in __vmx_vcpu_run can be tolerated, as far as performance
> > > effects are concerned.
> > >
> > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> > > Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> > > ---
>
> vmx_vmenter() in vmx/vmenter.S can and should now use SYM_FUNC_START_LOCAL
> instead of SYM_FUNC_LOCAL. Other than that nit:
>
> Reviewed-and-tested-by: Sean Christopherson <seanjc@google.com>
Thanks!
I'll prepare a v2 (and added LKML, as you suggested).
Uros.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] KVM/nVMX: Use __vmx_vcpu_run in nested_vmx_check_vmentry_hw
@ 2020-12-17 18:44 Uros Bizjak
2020-12-18 9:52 ` Paolo Bonzini
0 siblings, 1 reply; 8+ messages in thread
From: Uros Bizjak @ 2020-12-17 18:44 UTC (permalink / raw)
To: kvm, linux-kernel; +Cc: Uros Bizjak, Paolo Bonzini, Sean Christopherson
Replace inline assembly in nested_vmx_check_vmentry_hw
with a call to __vmx_vcpu_run. The function is not
performance critical, so (double) GPR save/restore
in __vmx_vcpu_run can be tolerated, as far as performance
effects are concerned.
v2: Mark vmx_vmenter SYM_FUNC_START_LOCAL.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Sean Christopherson <seanjc@google.com>
Reviewed-and-tested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
---
arch/x86/kvm/vmx/nested.c | 32 +++-----------------------------
arch/x86/kvm/vmx/vmenter.S | 2 +-
arch/x86/kvm/vmx/vmx.c | 2 --
arch/x86/kvm/vmx/vmx.h | 1 +
4 files changed, 5 insertions(+), 32 deletions(-)
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 89af692deb7e..6ab62bf277c4 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -12,6 +12,7 @@
#include "nested.h"
#include "pmu.h"
#include "trace.h"
+#include "vmx.h"
#include "x86.h"
static bool __read_mostly enable_shadow_vmcs = 1;
@@ -3056,35 +3057,8 @@ 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 = __vmx_vcpu_run(vmx, (unsigned long *)&vcpu->arch.regs,
+ 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 90ad7a6246e3..14abe1e37359 100644
--- a/arch/x86/kvm/vmx/vmenter.S
+++ b/arch/x86/kvm/vmx/vmenter.S
@@ -44,7 +44,7 @@
* they VM-Fail, whereas a successful VM-Enter + VM-Exit will jump
* to vmx_vmexit.
*/
-SYM_FUNC_START(vmx_vmenter)
+SYM_FUNC_START_LOCAL(vmx_vmenter)
/* EFLAGS.ZF is set if VMCS.LAUNCHED == 0 */
je 2f
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 47b8357b9751..72b496c54bc9 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6593,8 +6593,6 @@ static fastpath_t vmx_exit_handlers_fastpath(struct kvm_vcpu *vcpu)
}
}
-bool __vmx_vcpu_run(struct vcpu_vmx *vmx, unsigned long *regs, bool launched);
-
static noinstr void vmx_vcpu_enter_exit(struct kvm_vcpu *vcpu,
struct vcpu_vmx *vmx)
{
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index f6f66e5c6510..32db3b033e9b 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -339,6 +339,7 @@ void vmx_set_virtual_apic_mode(struct kvm_vcpu *vcpu);
struct vmx_uret_msr *vmx_find_uret_msr(struct vcpu_vmx *vmx, u32 msr);
void pt_update_intercept_for_msr(struct kvm_vcpu *vcpu);
void vmx_update_host_rsp(struct vcpu_vmx *vmx, unsigned long host_rsp);
+bool __vmx_vcpu_run(struct vcpu_vmx *vmx, unsigned long *regs, bool launched);
int vmx_find_loadstore_msr_slot(struct vmx_msrs *m, u32 msr);
void vmx_ept_load_pdptrs(struct kvm_vcpu *vcpu);
--
2.26.2
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] KVM/nVMX: Use __vmx_vcpu_run in nested_vmx_check_vmentry_hw
2020-12-17 18:44 Uros Bizjak
@ 2020-12-18 9:52 ` Paolo Bonzini
0 siblings, 0 replies; 8+ messages in thread
From: Paolo Bonzini @ 2020-12-18 9:52 UTC (permalink / raw)
To: Uros Bizjak, kvm, linux-kernel; +Cc: Sean Christopherson
On 17/12/20 19:44, Uros Bizjak wrote:
> Replace inline assembly in nested_vmx_check_vmentry_hw
> with a call to __vmx_vcpu_run. The function is not
> performance critical, so (double) GPR save/restore
> in __vmx_vcpu_run can be tolerated, as far as performance
> effects are concerned.
>
> v2: Mark vmx_vmenter SYM_FUNC_START_LOCAL.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Sean Christopherson <seanjc@google.com>
> Reviewed-and-tested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> ---
> arch/x86/kvm/vmx/nested.c | 32 +++-----------------------------
> arch/x86/kvm/vmx/vmenter.S | 2 +-
> arch/x86/kvm/vmx/vmx.c | 2 --
> arch/x86/kvm/vmx/vmx.h | 1 +
> 4 files changed, 5 insertions(+), 32 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 89af692deb7e..6ab62bf277c4 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -12,6 +12,7 @@
> #include "nested.h"
> #include "pmu.h"
> #include "trace.h"
> +#include "vmx.h"
> #include "x86.h"
>
> static bool __read_mostly enable_shadow_vmcs = 1;
> @@ -3056,35 +3057,8 @@ 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 = __vmx_vcpu_run(vmx, (unsigned long *)&vcpu->arch.regs,
> + 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 90ad7a6246e3..14abe1e37359 100644
> --- a/arch/x86/kvm/vmx/vmenter.S
> +++ b/arch/x86/kvm/vmx/vmenter.S
> @@ -44,7 +44,7 @@
> * they VM-Fail, whereas a successful VM-Enter + VM-Exit will jump
> * to vmx_vmexit.
> */
> -SYM_FUNC_START(vmx_vmenter)
> +SYM_FUNC_START_LOCAL(vmx_vmenter)
> /* EFLAGS.ZF is set if VMCS.LAUNCHED == 0 */
> je 2f
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 47b8357b9751..72b496c54bc9 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -6593,8 +6593,6 @@ static fastpath_t vmx_exit_handlers_fastpath(struct kvm_vcpu *vcpu)
> }
> }
>
> -bool __vmx_vcpu_run(struct vcpu_vmx *vmx, unsigned long *regs, bool launched);
> -
> static noinstr void vmx_vcpu_enter_exit(struct kvm_vcpu *vcpu,
> struct vcpu_vmx *vmx)
> {
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index f6f66e5c6510..32db3b033e9b 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -339,6 +339,7 @@ void vmx_set_virtual_apic_mode(struct kvm_vcpu *vcpu);
> struct vmx_uret_msr *vmx_find_uret_msr(struct vcpu_vmx *vmx, u32 msr);
> void pt_update_intercept_for_msr(struct kvm_vcpu *vcpu);
> void vmx_update_host_rsp(struct vcpu_vmx *vmx, unsigned long host_rsp);
> +bool __vmx_vcpu_run(struct vcpu_vmx *vmx, unsigned long *regs, bool launched);
> int vmx_find_loadstore_msr_slot(struct vmx_msrs *m, u32 msr);
> void vmx_ept_load_pdptrs(struct kvm_vcpu *vcpu);
>
>
Queued, thanks.
Paolo
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-12-18 9:54 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-29 13:41 [PATCH] KVM/nVMX: Use __vmx_vcpu_run in nested_vmx_check_vmentry_hw Uros Bizjak
2020-12-16 9:24 ` Uros Bizjak
2020-12-16 20:15 ` Krish Sadhukhan
2020-12-16 21:05 ` Sean Christopherson
2020-12-17 18:04 ` Sean Christopherson
2020-12-17 18:12 ` Uros Bizjak
2020-12-17 18:44 Uros Bizjak
2020-12-18 9:52 ` Paolo Bonzini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.