This series comprises two related fixes:
- the FILL_RETURN_BUFFER macro in -next needs to access percpu data,
hence the GS segment base needs to be loaded before FILL_RETURN_BUFFER.
This means moving guest vmload/vmsave and host vmload to assembly
(patches 5 and 6).
- because AMD wants the OS to set STIBP to 1 before executing the
return thunk (un)training sequence, IA32_SPEC_CTRL must be restored
before UNTRAIN_RET, too. This must also be moved to assembly and,
for consistency, the guest SPEC_CTRL is also loaded in there
(patch 7).
Neither is particularly hard, however because of 32-bit systems one needs
to keep the number of arguments to __svm_vcpu_run to three or fewer.
One is taken for whether IA32_SPEC_CTRL is intercepted, and one for the
host save area, so all accesses to the vcpu_svm struct have to be done
from assembly too. This is done in patches 2 to 4, and it turns out
not to be that bad; in fact I think the code is simpler than before
after these prerequisites, and even at the end of the series it is not
much harder to follow despite doing a lot more stuff. Care has been
taken to keep the "normal" and SEV-ES code as similar as possible,
even though the latter would not hit the three argument barrier.
The above summary leaves out the more mundane patches 1 and 8. The
former introduces a separate asm-offsets.c file for KVM, so that
kernel/asm-offsets.c does not have to do ugly includes with ../ paths.
The latter is dead code removal.
Thanks,
Paolo
v1->v2: use a separate asm-offsets.c file instead of hacking around
the arch/x86/kvm/svm/svm.h file; this could have been done
also with just a "#ifndef COMPILE_OFFSETS", but Sean's
suggestion is cleaner and there is a precedent in
drivers/memory/ for private asm-offsets files
keep preparatory cleanups together at the beginning of the
series
move SPEC_CTRL save/restore out of line [Jim]
Paolo Bonzini (8):
KVM: x86: use a separate asm-offsets.c file
KVM: SVM: replace regs argument of __svm_vcpu_run with vcpu_svm
KVM: SVM: adjust register allocation for __svm_vcpu_run
KVM: SVM: retrieve VMCB from assembly
KVM: SVM: move guest vmsave/vmload to assembly
KVM: SVM: restore host save area from assembly
KVM: SVM: move MSR_IA32_SPEC_CTRL save/restore to assembly
x86, KVM: remove unnecessary argument to x86_virt_spec_ctrl and
callers
arch/x86/include/asm/spec-ctrl.h | 10 +-
arch/x86/kernel/asm-offsets.c | 6 -
arch/x86/kernel/cpu/bugs.c | 15 +-
arch/x86/kvm/Makefile | 12 ++
arch/x86/kvm/kvm-asm-offsets.c | 28 ++++
arch/x86/kvm/svm/svm.c | 53 +++----
arch/x86/kvm/svm/svm.h | 4 +-
arch/x86/kvm/svm/svm_ops.h | 5 -
arch/x86/kvm/svm/vmenter.S | 241 ++++++++++++++++++++++++-------
arch/x86/kvm/vmx/vmenter.S | 2 +-
10 files changed, 259 insertions(+), 117 deletions(-)
create mode 100644 arch/x86/kvm/kvm-asm-offsets.c
--
2.31.1
In preparation for moving vmload/vmsave to __svm_vcpu_run,
keep the pointer to the struct vcpu_svm in %rdi. This way
it is possible to load svm->vmcb01.pa in %rax without
clobbering the pointer to svm itself.
No functional change intended.
Cc: [email protected]
Fixes: f14eec0a3203 ("KVM: SVM: move more vmentry code to assembly")
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/kvm/svm/vmenter.S | 38 +++++++++++++++++++-------------------
1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/arch/x86/kvm/svm/vmenter.S b/arch/x86/kvm/svm/vmenter.S
index f0ff41103e4c..531510ab6072 100644
--- a/arch/x86/kvm/svm/vmenter.S
+++ b/arch/x86/kvm/svm/vmenter.S
@@ -54,29 +54,29 @@ SYM_FUNC_START(__svm_vcpu_run)
/* Save @vmcb. */
push %_ASM_ARG1
- /* Move @svm to RAX. */
- mov %_ASM_ARG2, %_ASM_AX
+ /* Move @svm to RDI. */
+ mov %_ASM_ARG2, %_ASM_DI
+
+ /* "POP" @vmcb to RAX. */
+ pop %_ASM_AX
/* Load guest registers. */
- mov VCPU_RCX(%_ASM_AX), %_ASM_CX
- mov VCPU_RDX(%_ASM_AX), %_ASM_DX
- mov VCPU_RBX(%_ASM_AX), %_ASM_BX
- mov VCPU_RBP(%_ASM_AX), %_ASM_BP
- mov VCPU_RSI(%_ASM_AX), %_ASM_SI
- mov VCPU_RDI(%_ASM_AX), %_ASM_DI
+ mov VCPU_RCX(%_ASM_DI), %_ASM_CX
+ mov VCPU_RDX(%_ASM_DI), %_ASM_DX
+ mov VCPU_RBX(%_ASM_DI), %_ASM_BX
+ mov VCPU_RBP(%_ASM_DI), %_ASM_BP
+ mov VCPU_RSI(%_ASM_DI), %_ASM_SI
#ifdef CONFIG_X86_64
- mov VCPU_R8 (%_ASM_AX), %r8
- mov VCPU_R9 (%_ASM_AX), %r9
- mov VCPU_R10(%_ASM_AX), %r10
- mov VCPU_R11(%_ASM_AX), %r11
- mov VCPU_R12(%_ASM_AX), %r12
- mov VCPU_R13(%_ASM_AX), %r13
- mov VCPU_R14(%_ASM_AX), %r14
- mov VCPU_R15(%_ASM_AX), %r15
+ mov VCPU_R8 (%_ASM_DI), %r8
+ mov VCPU_R9 (%_ASM_DI), %r9
+ mov VCPU_R10(%_ASM_DI), %r10
+ mov VCPU_R11(%_ASM_DI), %r11
+ mov VCPU_R12(%_ASM_DI), %r12
+ mov VCPU_R13(%_ASM_DI), %r13
+ mov VCPU_R14(%_ASM_DI), %r14
+ mov VCPU_R15(%_ASM_DI), %r15
#endif
-
- /* "POP" @vmcb to RAX. */
- pop %_ASM_AX
+ mov VCPU_RDI(%_ASM_DI), %_ASM_DI
/* Enter guest mode */
sti
--
2.31.1
This already removes the ugly #includes from asm-offsets.c, but especially
it avoids a future error when asm-offsets will try to include svm/svm.h.
This would not work for kernel/asm-offsets.c, because svm/svm.h
includes kvm_cache_regs.h which is not in the include path when
compiling asm-offsets.c. The problem is not there if the .c file is
in arch/x86/kvm.
Tested with both in-tree and separate-objtree compilation.
Suggested-by: Sean Christopherson <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/kernel/asm-offsets.c | 6 ------
arch/x86/kvm/Makefile | 9 +++++++++
arch/x86/kvm/kvm-asm-offsets.c | 18 ++++++++++++++++++
arch/x86/kvm/vmx/vmenter.S | 2 +-
4 files changed, 28 insertions(+), 7 deletions(-)
create mode 100644 arch/x86/kvm/kvm-asm-offsets.c
diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
index cb50589a7102..437308004ef2 100644
--- a/arch/x86/kernel/asm-offsets.c
+++ b/arch/x86/kernel/asm-offsets.c
@@ -19,7 +19,6 @@
#include <asm/suspend.h>
#include <asm/tlbflush.h>
#include <asm/tdx.h>
-#include "../kvm/vmx/vmx.h"
#ifdef CONFIG_XEN
#include <xen/interface/xen.h>
@@ -108,9 +107,4 @@ static void __used common(void)
OFFSET(TSS_sp0, tss_struct, x86_tss.sp0);
OFFSET(TSS_sp1, tss_struct, x86_tss.sp1);
OFFSET(TSS_sp2, tss_struct, x86_tss.sp2);
-
- if (IS_ENABLED(CONFIG_KVM_INTEL)) {
- BLANK();
- OFFSET(VMX_spec_ctrl, vcpu_vmx, spec_ctrl);
- }
}
diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile
index 30f244b64523..a02cf9baacc8 100644
--- a/arch/x86/kvm/Makefile
+++ b/arch/x86/kvm/Makefile
@@ -34,3 +34,12 @@ endif
obj-$(CONFIG_KVM) += kvm.o
obj-$(CONFIG_KVM_INTEL) += kvm-intel.o
obj-$(CONFIG_KVM_AMD) += kvm-amd.o
+
+AFLAGS_vmx/vmenter.o := -iquote $(obj)
+$(obj)/vmx/vmenter.o: $(obj)/kvm-asm-offsets.h
+
+$(obj)/kvm-asm-offsets.h: $(obj)/kvm-asm-offsets.s FORCE
+ $(call filechk,offsets,__KVM_ASM_OFFSETS_H__)
+
+targets += kvm-asm-offsets.s
+clean-files += kvm-asm-offsets.h
diff --git a/arch/x86/kvm/kvm-asm-offsets.c b/arch/x86/kvm/kvm-asm-offsets.c
new file mode 100644
index 000000000000..9d84f2b32d7f
--- /dev/null
+++ b/arch/x86/kvm/kvm-asm-offsets.c
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Generate definitions needed by assembly language modules.
+ * This code generates raw asm output which is post-processed to extract
+ * and format the required data.
+ */
+#define COMPILE_OFFSETS
+
+#include <linux/kbuild.h>
+#include "vmx/vmx.h"
+
+static void __used common(void)
+{
+ if (IS_ENABLED(CONFIG_KVM_INTEL)) {
+ BLANK();
+ OFFSET(VMX_spec_ctrl, vcpu_vmx, spec_ctrl);
+ }
+}
diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
index 8477d8bdd69c..0b5db4de4d09 100644
--- a/arch/x86/kvm/vmx/vmenter.S
+++ b/arch/x86/kvm/vmx/vmenter.S
@@ -1,12 +1,12 @@
/* SPDX-License-Identifier: GPL-2.0 */
#include <linux/linkage.h>
#include <asm/asm.h>
-#include <asm/asm-offsets.h>
#include <asm/bitsperlong.h>
#include <asm/kvm_vcpu_regs.h>
#include <asm/nospec-branch.h>
#include <asm/percpu.h>
#include <asm/segment.h>
+#include "kvm-asm-offsets.h"
#include "run_flags.h"
#define WORD_SIZE (BITS_PER_LONG / 8)
--
2.31.1
x86_virt_spec_ctrl only deals with the paravirtualized
MSR_IA32_VIRT_SPEC_CTRL now and does not handle MSR_IA32_SPEC_CTRL
anymore; remove the corresponding, unused argument.
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/include/asm/spec-ctrl.h | 10 +++++-----
arch/x86/kernel/cpu/bugs.c | 2 +-
arch/x86/kvm/svm/svm.c | 4 ++--
3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/arch/x86/include/asm/spec-ctrl.h b/arch/x86/include/asm/spec-ctrl.h
index 5393babc0598..cb0386fc4dc3 100644
--- a/arch/x86/include/asm/spec-ctrl.h
+++ b/arch/x86/include/asm/spec-ctrl.h
@@ -13,7 +13,7 @@
* Takes the guest view of SPEC_CTRL MSR as a parameter and also
* the guest's version of VIRT_SPEC_CTRL, if emulated.
*/
-extern void x86_virt_spec_ctrl(u64 guest_spec_ctrl, u64 guest_virt_spec_ctrl, bool guest);
+extern void x86_virt_spec_ctrl(u64 guest_virt_spec_ctrl, bool guest);
/**
* x86_spec_ctrl_set_guest - Set speculation control registers for the guest
@@ -24,9 +24,9 @@ extern void x86_virt_spec_ctrl(u64 guest_spec_ctrl, u64 guest_virt_spec_ctrl, bo
* Avoids writing to the MSR if the content/bits are the same
*/
static inline
-void x86_spec_ctrl_set_guest(u64 guest_spec_ctrl, u64 guest_virt_spec_ctrl)
+void x86_spec_ctrl_set_guest(u64 guest_virt_spec_ctrl)
{
- x86_virt_spec_ctrl(guest_spec_ctrl, guest_virt_spec_ctrl, true);
+ x86_virt_spec_ctrl(guest_virt_spec_ctrl, true);
}
/**
@@ -38,9 +38,9 @@ void x86_spec_ctrl_set_guest(u64 guest_spec_ctrl, u64 guest_virt_spec_ctrl)
* Avoids writing to the MSR if the content/bits are the same
*/
static inline
-void x86_spec_ctrl_restore_host(u64 guest_spec_ctrl, u64 guest_virt_spec_ctrl)
+void x86_spec_ctrl_restore_host(u64 guest_virt_spec_ctrl)
{
- x86_virt_spec_ctrl(guest_spec_ctrl, guest_virt_spec_ctrl, false);
+ x86_virt_spec_ctrl(guest_virt_spec_ctrl, false);
}
/* AMD specific Speculative Store Bypass MSR data */
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 6ec0b7ce7453..3e3230cccaa7 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -200,7 +200,7 @@ void __init check_bugs(void)
* MSR_IA32_SPEC_CTRL for SSBD.
*/
void
-x86_virt_spec_ctrl(u64 guest_spec_ctrl, u64 guest_virt_spec_ctrl, bool setguest)
+x86_virt_spec_ctrl(u64 guest_virt_spec_ctrl, bool setguest)
{
u64 guestval, hostval;
struct thread_info *ti = current_thread_info();
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 512bc06a4ba1..e6706fd88cfa 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3998,7 +3998,7 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu)
* being speculatively taken.
*/
if (!static_cpu_has(X86_FEATURE_V_SPEC_CTRL))
- x86_spec_ctrl_set_guest(svm->spec_ctrl, svm->virt_spec_ctrl);
+ x86_spec_ctrl_set_guest(svm->virt_spec_ctrl);
svm_vcpu_enter_exit(vcpu, spec_ctrl_intercepted);
@@ -4006,7 +4006,7 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu)
reload_tss(vcpu);
if (!static_cpu_has(X86_FEATURE_V_SPEC_CTRL))
- x86_spec_ctrl_restore_host(svm->spec_ctrl, svm->virt_spec_ctrl);
+ x86_spec_ctrl_restore_host(svm->virt_spec_ctrl);
if (!sev_es_guest(vcpu->kvm)) {
vcpu->arch.cr2 = svm->vmcb->save.cr2;
--
2.31.1
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: [email protected]
Fixes: f14eec0a3203 ("KVM: SVM: move more vmentry code to assembly")
Signed-off-by: Paolo Bonzini <[email protected]>
---
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
--
2.31.1
On Tue, Nov 08, 2022 at 10:15:24AM -0500, Paolo Bonzini wrote:
> This series comprises two related fixes:
>
> - the FILL_RETURN_BUFFER macro in -next needs to access percpu data,
> hence the GS segment base needs to be loaded before FILL_RETURN_BUFFER.
> This means moving guest vmload/vmsave and host vmload to assembly
> (patches 5 and 6).
>
> - because AMD wants the OS to set STIBP to 1 before executing the
> return thunk (un)training sequence, IA32_SPEC_CTRL must be restored
> before UNTRAIN_RET, too. This must also be moved to assembly and,
> for consistency, the guest SPEC_CTRL is also loaded in there
> (patch 7).
>
> Neither is particularly hard, however because of 32-bit systems one needs
> to keep the number of arguments to __svm_vcpu_run to three or fewer.
> One is taken for whether IA32_SPEC_CTRL is intercepted, and one for the
> host save area, so all accesses to the vcpu_svm struct have to be done
> from assembly too. This is done in patches 2 to 4, and it turns out
> not to be that bad; in fact I think the code is simpler than before
> after these prerequisites, and even at the end of the series it is not
> much harder to follow despite doing a lot more stuff. Care has been
> taken to keep the "normal" and SEV-ES code as similar as possible,
> even though the latter would not hit the three argument barrier.
>
> The above summary leaves out the more mundane patches 1 and 8. The
> former introduces a separate asm-offsets.c file for KVM, so that
> kernel/asm-offsets.c does not have to do ugly includes with ../ paths.
> The latter is dead code removal.
>
> Thanks,
>
> Paolo
>
> v1->v2: use a separate asm-offsets.c file instead of hacking around
> the arch/x86/kvm/svm/svm.h file; this could have been done
> also with just a "#ifndef COMPILE_OFFSETS", but Sean's
> suggestion is cleaner and there is a precedent in
> drivers/memory/ for private asm-offsets files
>
> keep preparatory cleanups together at the beginning of the
> series
>
> move SPEC_CTRL save/restore out of line [Jim]
>
> Paolo Bonzini (8):
> KVM: x86: use a separate asm-offsets.c file
> KVM: SVM: replace regs argument of __svm_vcpu_run with vcpu_svm
> KVM: SVM: adjust register allocation for __svm_vcpu_run
> KVM: SVM: retrieve VMCB from assembly
> KVM: SVM: move guest vmsave/vmload to assembly
> KVM: SVM: restore host save area from assembly
> KVM: SVM: move MSR_IA32_SPEC_CTRL save/restore to assembly
> x86, KVM: remove unnecessary argument to x86_virt_spec_ctrl and
> callers
>
> arch/x86/include/asm/spec-ctrl.h | 10 +-
> arch/x86/kernel/asm-offsets.c | 6 -
> arch/x86/kernel/cpu/bugs.c | 15 +-
> arch/x86/kvm/Makefile | 12 ++
> arch/x86/kvm/kvm-asm-offsets.c | 28 ++++
> arch/x86/kvm/svm/svm.c | 53 +++----
> arch/x86/kvm/svm/svm.h | 4 +-
> arch/x86/kvm/svm/svm_ops.h | 5 -
> arch/x86/kvm/svm/vmenter.S | 241 ++++++++++++++++++++++++-------
> arch/x86/kvm/vmx/vmenter.S | 2 +-
> 10 files changed, 259 insertions(+), 117 deletions(-)
> create mode 100644 arch/x86/kvm/kvm-asm-offsets.c
>
> --
> 2.31.1
>
>
I applied this series on next-20221108, which has the call depth
tracking patches, and I no longer see the panic when starting a guest on
my AMD test system and I can still start a simple nested guest without
any problems, which is about the extent of my regular KVM testing. I
did test the same kernel on my Intel systems and saw no problems there
but that seems expected given the diffstat. Thank you for the quick
response and fixes!
Tested-by: Nathan Chancellor <[email protected]>
One small nit I noticed: kvm-asm-offsets.h should be added to a
.gitignore file in arch/x86/kvm.
$ git status --short
?? arch/x86/kvm/kvm-asm-offsets.h
Cheers,
Nathan
On Tue, Nov 08, 2022, Paolo Bonzini wrote:
> In preparation for moving vmload/vmsave to __svm_vcpu_run,
> keep the pointer to the struct vcpu_svm in %rdi. This way
> it is possible to load svm->vmcb01.pa in %rax without
> clobbering the pointer to svm itself.
>
> No functional change intended.
>
> Cc: [email protected]
> Fixes: f14eec0a3203 ("KVM: SVM: move more vmentry code to assembly")
Same nit, Fixes: shouldn't be provided.
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