Messages in this thread Patch in this message | | | Date | Mon, 7 Nov 2022 18:36:21 +0100 | From | Paolo Bonzini <> | Subject | Re: [PATCH 1/8] KVM: SVM: extract VMCB accessors to a new file |
| |
On 11/7/22 18:08, Sean Christopherson wrote: > On Mon, Nov 07, 2022, Paolo Bonzini wrote: >> Having inline functions confuses the compilation of asm-offsets.c, >> which cannot find kvm_cache_regs.h because arch/x86/kvm is not in >> asm-offset.c's include path. Just extract the functions to a >> new file. >> >> 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/svm/avic.c | 1 + >> arch/x86/kvm/svm/nested.c | 1 + >> arch/x86/kvm/svm/sev.c | 1 + >> arch/x86/kvm/svm/svm.c | 1 + >> arch/x86/kvm/svm/svm.h | 200 ------------------------------ >> arch/x86/kvm/svm/svm_onhyperv.c | 1 + >> arch/x86/kvm/svm/vmcb.h | 211 ++++++++++++++++++++++++++++++++ > > I don't think vmcb.h is a good name. The logical inclusion sequence would be for > svm.h to include vmcb.h, e.g. SVM requires knowledge about VMCBs, but this requires > vmcb.h to include svm.h to dereference "struct vcpu_svm". > Unlike VMX's vmcs.h, the new file isn't a "pure" VMCB helper, it also holds a > decent amount of KVM's SVM logic.
Yes, it's basically the wrappers that KVM uses to access the VMCB fields.
> What about making KVM self-sufficient?
You mean having a different asm-offsets.h file just for arch/x86/kvm/?
> The includes in asm-offsets.c are quite ugly > > #include "../kvm/vmx/vmx.h" > #include "../kvm/svm/svm.h" > > or as a stopgap to make backporting easier, just include kvm_cache_regs.h?
The problem is that the _existing_ include of kvm_cache_regs.h in svm.h fails, with
arch/x86/kernel/../kvm/svm/svm.h:25:10: fatal error: kvm_cache_regs.h: No such file or directory 25 | #include "kvm_cache_regs.h" | ^~~~~~~~~~~~~~~~~~ compilation terminated.
The other two solutions here are:
1) move kvm_cache_regs.h to arch/x86/include/asm/ so it can be included normally
2) extract the structs to arch/x86/kvm/svm/svm_types.h and include that from asm-offsets.h, basically the opposite of this patch.
(2) is my preference if having a different asm-offsets.h file turns out to be too complex. We can do the same for VMX as well.
Paolo
>> void svm_leave_nested(struct kvm_vcpu *vcpu); >> diff --git a/arch/x86/kvm/svm/svm_onhyperv.c b/arch/x86/kvm/svm/svm_onhyperv.c >> index 8cdc62c74a96..ae0a101329e6 100644 >> --- a/arch/x86/kvm/svm/svm_onhyperv.c >> +++ b/arch/x86/kvm/svm/svm_onhyperv.c >> @@ -8,6 +8,7 @@ >> #include <asm/mshyperv.h> >> >> #include "svm.h" >> +#include "vmcb.h" >> #include "svm_ops.h" >> >> #include "hyperv.h" >> diff --git a/arch/x86/kvm/svm/vmcb.h b/arch/x86/kvm/svm/vmcb.h >> new file mode 100644 >> index 000000000000..8757cda27e3a >> --- /dev/null >> +++ b/arch/x86/kvm/svm/vmcb.h >> @@ -0,0 +1,211 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Kernel-based Virtual Machine driver for Linux >> + * >> + * AMD SVM support - VMCB accessors >> + */ >> + >> +#ifndef __SVM_VMCB_H >> +#define __SVM_VMCB_H >> + >> +#include "kvm_cache_regs.h" > This should include "svm.h" instead of relying on the parent to include said file. >
| |