Messages in this thread Patch in this message | | | Date | Fri, 22 Mar 2024 10:37:20 +1300 | Subject | Re: [PATCH v19 024/130] KVM: TDX: Add placeholders for TDX VM/vcpu structure | From | "Huang, Kai" <> |
| |
On 26/02/2024 9:25 pm, Yamahata, Isaku wrote: > From: Isaku Yamahata <isaku.yamahata@intel.com> > > Add placeholders TDX VM/vcpu structure that overlays with VMX VM/vcpu > structures. Initialize VM structure size and vcpu size/align so that x86 > KVM common code knows those size irrespective of VMX or TDX. Those > structures will be populated as guest creation logic develops. > > Add helper functions to check if the VM is guest TD and add conversion > functions between KVM VM/VCPU and TDX VM/VCPU.
The changelog is essentially only saying "doing what" w/o "why".
Please at least explain why you invented the 'struct kvm_tdx' and 'struct vcpu_tdx', and why they are invented in this way.
E.g., can we extend 'struct kvm_vmx' for TDX?
struct kvm_tdx { struct kvm_vmx vmx; ... };
> > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> > > --- > v19: > - correctly update ops.vm_size, vcpu_size and, vcpu_align by Xiaoyao > > v14 -> v15: > - use KVM_X86_TDX_VM > > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> > --- > arch/x86/kvm/vmx/main.c | 14 ++++++++++++ > arch/x86/kvm/vmx/tdx.c | 1 + > arch/x86/kvm/vmx/tdx.h | 50 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 65 insertions(+) > create mode 100644 arch/x86/kvm/vmx/tdx.h > > diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c > index 18aef6e23aab..e11edbd19e7c 100644 > --- a/arch/x86/kvm/vmx/main.c > +++ b/arch/x86/kvm/vmx/main.c > @@ -5,6 +5,7 @@ > #include "vmx.h" > #include "nested.h" > #include "pmu.h" > +#include "tdx.h" > > static bool enable_tdx __ro_after_init; > module_param_named(tdx, enable_tdx, bool, 0444); > @@ -18,6 +19,9 @@ static __init int vt_hardware_setup(void) > return ret; > > enable_tdx = enable_tdx && !tdx_hardware_setup(&vt_x86_ops); > + if (enable_tdx) > + vt_x86_ops.vm_size = max_t(unsigned int, vt_x86_ops.vm_size, > + sizeof(struct kvm_tdx)); >
Now I see why you included 'struct kvm_x86_ops' as function parameter.
Please move it to this patch.
> return 0; > } > @@ -215,8 +219,18 @@ static int __init vt_init(void) > * Common KVM initialization _must_ come last, after this, /dev/kvm is > * exposed to userspace! > */ > + /* > + * kvm_x86_ops is updated with vt_x86_ops. vt_x86_ops.vm_size must > + * be set before kvm_x86_vendor_init(). > + */ > vcpu_size = sizeof(struct vcpu_vmx); > vcpu_align = __alignof__(struct vcpu_vmx); > + if (enable_tdx) { > + vcpu_size = max_t(unsigned int, vcpu_size, > + sizeof(struct vcpu_tdx)); > + vcpu_align = max_t(unsigned int, vcpu_align, > + __alignof__(struct vcpu_tdx)); > + }
Since you are updating vm_size in vt_hardware_setup(), I am wondering whether we can do similar thing for vcpu_size and vcpu_align.
That is, we put them both to 'struct kvm_x86_ops', and you update them in vt_hardware_setup().
kvm_init() can then just access them directly in this way both 'vcpu_size' and 'vcpu_align' function parameters can be removed.
> r = kvm_init(vcpu_size, vcpu_align, THIS_MODULE); > if (r) > goto err_kvm_init;
| |