Re: [PATCH v11 013/113] x86/cpu: Add helper functions to allocate/free TDX private host key id

[Zhi Wang]

On Fri, 13 Jan 2023 15:21:54 +0000
Sean Christopherson <seanjc@xxxxxxxxxx> wrote:

> On Fri, Jan 13, 2023, Zhi Wang wrote:
> > On Thu, 12 Jan 2023 08:31:21 -0800 isaku.yamahata@xxxxxxxxx wrote:
> > > @@ -132,6 +136,31 @@ static struct notifier_block tdx_memory_nb = {
> > >  	.notifier_call = tdx_memory_notifier,
> > >  };
> > >  
> > > +/* TDX KeyID pool */
> > > +static DEFINE_IDA(tdx_keyid_pool);
> > > +
> > > +int tdx_keyid_alloc(void)
> > > +{
> > > +	if (WARN_ON_ONCE(!tdx_keyid_start || !nr_tdx_keyids))
> > > +		return -EINVAL;
> > 
> > Better mention that tdx_keyid_start and nr_tdx_keyids are defined in
> > another patches.
> 
> Eh, no need.  That sort of information doesn't belong in the changelog
> because when this code is merged it will be a natural sequence.  The
> cover letter explicitly calls out that this needs the kernel patches[*].
>  A footnote could be added, but asking Isaku and co. to document every
> external dependency is asking too much IMO.
> 
> [*] https://lore.kernel.org/lkml/cover.1670566861.git.kai.huang@xxxxxxxxx

Hi:

Thanks. I raised this concern from the reviewers' perspective. For example,
finding something was missing, grep, nothing was found, and jumping to
another window and grep. 

Finally, you can make sure if missing tdx_keyid_start in the patch is a
mistake or a dependency. Then the same happens on nr_tdx_keyids.

It would be nice to just say tdx_hkid_start, nr_tdx_keyid requires an
external patch in the comment. Or, just mention this patch depends
on an external patch in the comment. It will save quite some efforts.