On Mon, 2024-08-19 at 18:35 +0300, Nikolay Borisov wrote: > > + /* > > + * This function initializes only KVM software construct. It > > doesn't > > + * initialize TDX stuff, e.g. TDCS, TDR, TDCX, HKID etc. > > + * It is handled by KVM_TDX_INIT_VM, __tdx_td_init(). > > + */ > > If you need to put a comment like that it means the function has the > wrong name. This comment is pretty weird. The name seems to come from the pattern of the tdx specific x86_ops callbacks. As in: vcpu_create() vt_vcpu_create() vmx_vcpu_create() tdx_vcpu_create() ..matches to: vm_init() vt_vm_init() tdx_vm_init() vmx_vm_init() Maybe we should try to come up with some other prefix that makes it clearer that these are x86_ops callbacks. > > > + > > /* > > * TDX has its own limit of the number of vcpus in addition to > > * KVM_MAX_VCPUS. > > <snip> > > > + > > +static int __tdx_td_init(struct kvm *kvm, struct td_params *td_params, > > + u64 *seamcall_err) > > What criteria did you use to split __tdx_td_init from tdx_td_init? Seems > somewhar arbitrary, I think it's best if the TD VM init code is in a > single function, yet it will be rather large but the code should be > self-explanatory and fairly linear. Additionally I think some of the > code can be factored out in more specific helpers i.e the key > programming bits can be a separate helper. It looks like it has been like that since 2022. I couldn't find any reasoning. I agree this could be organized better.
