[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Y2r6FqZyT4XxUkYB@google.com>
Date: Wed, 9 Nov 2022 00:53:42 +0000
From: Sean Christopherson <seanjc@...gle.com>
To: Paolo Bonzini <pbonzini@...hat.com>
Cc: linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
nathan@...nel.org, thomas.lendacky@....com,
andrew.cooper3@...rix.com, peterz@...radead.org,
jmattson@...gle.com, stable@...r.kernel.org
Subject: Re: [PATCH v2 4/8] KVM: SVM: retrieve VMCB from assembly
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.
View attachment "0001-KVM-SVM-Add-a-helper-to-convert-a-SME-aware-PA-back-.patch" of type "text/x-diff" (3056 bytes)
View attachment "0002-KVM-SVM-Snapshot-host-save-area-PA-in-dedicated-per-.patch" of type "text/x-diff" (4806 bytes)
Powered by blists - more mailing lists