Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 14 Jan 2023 11:38:35 +0200
From:   Zhi Wang <zhi.wang.linux@gmail.com>
To:     Sean Christopherson <seanjc@google.com>
Cc:     isaku.yamahata@intel.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, isaku.yamahata@gmail.com,
        Paolo Bonzini <pbonzini@redhat.com>, erdemaktas@google.com,
        Sagi Shahar <sagis@google.com>,
        David Matlack <dmatlack@google.com>
Subject: Re: [PATCH v11 013/113] x86/cpu: Add helper functions to
 allocate/free TDX private host key id
Message-ID: <20230114113835.0000019e@gmail.com>
In-Reply-To: <Y8F3Eo2lKB4hupQI@google.com>
References: <cover.1673539699.git.isaku.yamahata@intel.com>
        <241994f1f6782753f3307fe999a3dad434477c16.1673539699.git.isaku.yamahata@intel.com>
        <20230113144712.00006f41@gmail.com>
        <Y8F3Eo2lKB4hupQI@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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

> On Fri, Jan 13, 2023, Zhi Wang wrote:
> > On Thu, 12 Jan 2023 08:31:21 -0800 isaku.yamahata@intel.com 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@intel.com

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.