[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7ba6da25-9ce4-f146-8480-c2614154fbb4@redhat.com>
Date: Wed, 9 Nov 2022 10:09:50 +0100
From: Paolo Bonzini <pbonzini@...hat.com>
To: Sean Christopherson <seanjc@...gle.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 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
Powered by blists - more mailing lists