Re: [PATCH 14/25] KVM: TDX: initialize VM with TDX specific parameters

[Edgecombe, Rick P]

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.