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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <66718994df9082469a623bca3f24c8cadae41d3e.camel@intel.com>
Date: Wed, 21 Aug 2024 00:01:39 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "kvm@...r.kernel.org" <kvm@...r.kernel.org>, "pbonzini@...hat.com"
	<pbonzini@...hat.com>, "nik.borisov@...e.com" <nik.borisov@...e.com>,
	"seanjc@...gle.com" <seanjc@...gle.com>
CC: "Li, Xiaoyao" <xiaoyao.li@...el.com>, "Yamahata, Isaku"
	<isaku.yamahata@...el.com>, "tony.lindgren@...ux.intel.com"
	<tony.lindgren@...ux.intel.com>, "Huang, Kai" <kai.huang@...el.com>,
	"isaku.yamahata@...il.com" <isaku.yamahata@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 14/25] KVM: TDX: initialize VM with TDX specific
 parameters

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.