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

Edgecombe, Rick P
2024-08-21T00:01:43Z

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.