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.