This is needed in order to keep the number of arguments to 3 or less, after adding hsave_pa and spec_ctrl_intercepted. 32-bit builds only support passing three arguments in registers, fortunately all other data is reachable from the vcpu_svm struct.
It is not strictly necessary for __svm_sev_es_vcpu_run, but staying consistent is a good idea since it makes __svm_sev_es_vcpu_run a stripped version of _svm_vcpu_run.
No functional change intended.
Cc: stable@vger.kernel.org Fixes: f14eec0a3203 ("KVM: SVM: move more vmentry code to assembly") Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- arch/x86/kvm/kvm-asm-offsets.c | 2 ++ arch/x86/kvm/svm/svm.c | 5 ++--- arch/x86/kvm/svm/svm.h | 4 ++-- arch/x86/kvm/svm/vmenter.S | 20 ++++++++++---------- 4 files changed, 16 insertions(+), 15 deletions(-)
diff --git a/arch/x86/kvm/kvm-asm-offsets.c b/arch/x86/kvm/kvm-asm-offsets.c index 30db96852e2d..f1b694e431ae 100644 --- a/arch/x86/kvm/kvm-asm-offsets.c +++ b/arch/x86/kvm/kvm-asm-offsets.c @@ -15,6 +15,8 @@ static void __used common(void) if (IS_ENABLED(CONFIG_KVM_AMD)) { BLANK(); OFFSET(SVM_vcpu_arch_regs, vcpu_svm, vcpu.arch.regs); + OFFSET(SVM_current_vmcb, vcpu_svm, current_vmcb); + OFFSET(KVM_VMCB_pa, kvm_vmcb_info, pa); }
if (IS_ENABLED(CONFIG_KVM_INTEL)) { diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c index b412bc5773c5..0c86c435c51f 100644 --- a/arch/x86/kvm/svm/svm.c +++ b/arch/x86/kvm/svm/svm.c @@ -3914,12 +3914,11 @@ static fastpath_t svm_exit_handlers_fastpath(struct kvm_vcpu *vcpu) static noinstr void svm_vcpu_enter_exit(struct kvm_vcpu *vcpu) { struct vcpu_svm *svm = to_svm(vcpu); - unsigned long vmcb_pa = svm->current_vmcb->pa;
guest_state_enter_irqoff();
if (sev_es_guest(vcpu->kvm)) { - __svm_sev_es_vcpu_run(vmcb_pa); + __svm_sev_es_vcpu_run(svm); } else { struct svm_cpu_data *sd = per_cpu(svm_data, vcpu->cpu);
@@ -3930,7 +3929,7 @@ static noinstr void svm_vcpu_enter_exit(struct kvm_vcpu *vcpu) * vmcb02 when switching vmcbs for nested virtualization. */ vmload(svm->vmcb01.pa); - __svm_vcpu_run(vmcb_pa, svm); + __svm_vcpu_run(svm); vmsave(svm->vmcb01.pa);
vmload(__sme_page_pa(sd->save_area)); diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h index 447e25c9101a..7ff1879e73c5 100644 --- a/arch/x86/kvm/svm/svm.h +++ b/arch/x86/kvm/svm/svm.h @@ -683,7 +683,7 @@ void sev_es_unmap_ghcb(struct vcpu_svm *svm);
/* vmenter.S */
-void __svm_sev_es_vcpu_run(unsigned long vmcb_pa); -void __svm_vcpu_run(unsigned long vmcb_pa, struct vcpu_svm *svm); +void __svm_sev_es_vcpu_run(struct vcpu_svm *svm); +void __svm_vcpu_run(struct vcpu_svm *svm);
#endif diff --git a/arch/x86/kvm/svm/vmenter.S b/arch/x86/kvm/svm/vmenter.S index 531510ab6072..d07bac1952c5 100644 --- a/arch/x86/kvm/svm/vmenter.S +++ b/arch/x86/kvm/svm/vmenter.S @@ -32,7 +32,6 @@
/** * __svm_vcpu_run - Run a vCPU via a transition to SVM guest mode - * @vmcb_pa: unsigned long * @svm: struct vcpu_svm * */ SYM_FUNC_START(__svm_vcpu_run) @@ -49,16 +48,16 @@ SYM_FUNC_START(__svm_vcpu_run) push %_ASM_BX
/* Save @svm. */ - push %_ASM_ARG2 - - /* Save @vmcb. */ push %_ASM_ARG1
+.ifnc _ASM_ARG1, _ASM_DI /* Move @svm to RDI. */ - mov %_ASM_ARG2, %_ASM_DI + mov %_ASM_ARG1, %_ASM_DI +.endif
- /* "POP" @vmcb to RAX. */ - pop %_ASM_AX + /* Get svm->current_vmcb->pa into RAX. */ + mov SVM_current_vmcb(%_ASM_DI), %_ASM_AX + mov KVM_VMCB_pa(%_ASM_AX), %_ASM_AX
/* Load guest registers. */ mov VCPU_RCX(%_ASM_DI), %_ASM_CX @@ -170,7 +169,7 @@ SYM_FUNC_END(__svm_vcpu_run)
/** * __svm_sev_es_vcpu_run - Run a SEV-ES vCPU via a transition to SVM guest mode - * @vmcb_pa: unsigned long + * @svm: struct vcpu_svm * */ SYM_FUNC_START(__svm_sev_es_vcpu_run) push %_ASM_BP @@ -185,8 +184,9 @@ SYM_FUNC_START(__svm_sev_es_vcpu_run) #endif push %_ASM_BX
- /* Move @vmcb to RAX. */ - mov %_ASM_ARG1, %_ASM_AX + /* Get svm->current_vmcb->pa into RAX. */ + mov SVM_current_vmcb(%_ASM_ARG1), %_ASM_AX + mov KVM_VMCB_pa(%_ASM_AX), %_ASM_AX
/* Enter guest mode */ sti
On Tue, Nov 08, 2022, Paolo Bonzini wrote:
This is needed in order to keep the number of arguments to 3 or less, after adding hsave_pa and spec_ctrl_intercepted. 32-bit builds only support passing three arguments in registers, fortunately all other data is reachable from the vcpu_svm struct.
Is it actually a problem if parameters are passed on the stack? The assembly code mostly creates a stack frame, i.e. %ebp can be used to pull values off the stack.
I dont think it will matter in the end (more below), but hypothetically if we ended up with
void __svm_vcpu_run(struct kvm_vcpu *vcpu, unsigned long vmcb_pa, unsigned long gsave_pa, unsigned long hsave_pa, bool spec_ctrl_intercepted);
then the asm prologue would be something like:
/* * Save @vcpu, @gsave_pa, @hsave_pa, and @spec_ctrl_intercepted, all of * which are needed after VM-Exit. */ push %_ASM_ARG1 push %_ASM_ARG3
#ifdef CONFIG_X86_64 push %_ASM_ARG4 push %_ASM_ARG5 #else push %_ASM_ARG4_EBP(%ebp) push %_ASM_ARG5_EBP(%ebp) #endif
which is a few extra memory accesses, especially for 32-bit, but no one cares about 32-bit and I highly doubt a few extra PUSH+POP instructions will be noticeable.
Threading in yesterday's conversation...
What about adding dedicated structs to hold the non-regs params for VM-Enter and VMRUN? Grabbing stuff willy-nilly in the assembly code makes the flows difficult to read as there's nothing in the C code that describes what fields are actually used.
What fields are actually used is (like with any other function) "potentially all, you'll have to read the source code and in fact you can just read asm-offsets.c instead". What I mean is, I cannot offhand see or remember what fields are touched by svm_prepare_switch_to_guest, why would __svm_vcpu_run be any different?
It's different because if it were a normal C function, it would simply take @vcpu, and maybe @spec_ctrl_intercepted to shave cycles after CLGI. But because it's assembly and doesn't have to_svm() readily available (among other restrictions), __svm_vcpu_run() ends up taking a mishmash of parameters, which for me makes it rather difficult to understand what to expect.
Oooh, and after much staring I realized that the address of the host save area is passed in because grabbing it after VM-Exit can't work. That's subtle, and passing it in isn't strictly necessary; there's no reason the assembly code can't grab it and stash it on the stack.
What about killing a few birds with one stone? Move the host save area PA to its own per-CPU variable, and then grab that from assembly as well. Then it's a bit more obvious why the address needs to be saved on the stack across VMRUN, and my whining about the prototype being funky goes away. __svm_vcpu_run() and __svm_sev_es_vcpu_run() would have identical prototypes too.
Attached patches would slot in early in the series. Tested SVM and SME-enabled kernels, didn't test the SEV-ES bits.
On 11/9/22 01:53, Sean Christopherson wrote:
On Tue, Nov 08, 2022, Paolo Bonzini wrote:
This is needed in order to keep the number of arguments to 3 or less, after adding hsave_pa and spec_ctrl_intercepted. 32-bit builds only support passing three arguments in registers, fortunately all other data is reachable from the vcpu_svm struct.
Is it actually a problem if parameters are passed on the stack? The assembly code mostly creates a stack frame, i.e. %ebp can be used to pull values off the stack.
It's not, but given how little love 32-bit KVM receives, I prefer to stick to the subset of the ABI that is "equivalent" to 64-bit.
no one cares about 32-bit and I highly doubt a few extra PUSH+POP instructions will be noticeable.
Same reasoning (no one cares about 32-bits), different conclusions...
What fields are actually used is (like with any other function) "potentially all, you'll have to read the source code and in fact you can just read asm-offsets.c instead". What I mean is, I cannot offhand see or remember what fields are touched by svm_prepare_switch_to_guest, why would __svm_vcpu_run be any different?
It's different because if it were a normal C function, it would simply take @vcpu, and maybe @spec_ctrl_intercepted to shave cycles after CLGI.
Not just for that, but especially to avoid making msr_write_intercepted() noinstr.
But because it's assembly and doesn't have to_svm() readily available (among other restrictions), __svm_vcpu_run() ends up taking a mishmash of parameters, which for me makes it rather difficult to understand what to expect.
Yeah, there could be three reasons to have parameters in assembly:
* you just need them (@svm)
* it's too much of a pain to compute it in assembly (@spec_ctrl_intercepted, @hsave_pa)
* it needs to be computed outside the clgi/stgi region (not happening here, only mentioned for completeness)
As this patch shows, @vmcb is not much of a pain to compute in assembly: it is just two instructions, and not passing it in simplifies register allocation (the weird push/pop goes away) because all the arguments except @svm/_ASM_ARG1 are needed only after vmexit.
Oooh, and after much staring I realized that the address of the host save area is passed in because grabbing it after VM-Exit can't work. That's subtle, and passing it in isn't strictly necessary; there's no reason the assembly code can't grab it and stash it on the stack.
Right, in fact that's not the reason why it's passed in---it's just to avoid coding page_to_pfn() in assembly, and to limit the differences between the regular and SEV-ES cases. But using a per-CPU variable is fine (either in addition to the struct page, which "wastes" 8 bytes per CPU, or as a replacement).
What about killing a few birds with one stone? Move the host save area PA to its own per-CPU variable, and then grab that from assembly as well.
I would still place it in struct svm_cpu_data itself, I'll see how it looks and possibly post v3.
Paolo
linux-stable-mirror@lists.linaro.org
-
Paolo Bonzini -
Sean Christopherson