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:16:21 +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>,
        Sean Christopherson <sean.j.christopherson@intel.com>,
        Kai Huang <kai.huang@intel.com>
Subject: Re: [PATCH v11 018/113] KVM: TDX: create/destroy VM structure
Message-ID: <20230114111621.00001840@gmail.com>
In-Reply-To: <Y8F1uPsW56fVdhmC@google.com>
References: <cover.1673539699.git.isaku.yamahata@intel.com>
        <68fa413e61d7471657174bc7c83bde5c842e251f.1673539699.git.isaku.yamahata@intel.com>
        <20230113151258.00006a6d@gmail.com>
        <Y8F1uPsW56fVdhmC@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:16:08 +0000
Sean Christopherson <seanjc@google.com> wrote:

> On Fri, Jan 13, 2023, Zhi Wang wrote:
> > On Thu, 12 Jan 2023 08:31:26 -0800 isaku.yamahata@intel.com wrote:
> > > +static void tdx_reclaim_td_page(unsigned long td_page_pa)
> > > +{
> > > +	if (!td_page_pa)
> > > +		return;
> > > +	/*
> > > +	 * TDCX are being reclaimed.  TDX module maps TDCX with HKID
> > > +	 * assigned to the TD.  Here the cache associated to the TD
> > > +	 * was already flushed by TDH.PHYMEM.CACHE.WB before here,
> > > So
> > > +	 * cache doesn't need to be flushed again.
> > > +	 */
> > > +	if (WARN_ON(tdx_reclaim_page(td_page_pa, false, 0)))
> 
> The WARN_ON() can go, tdx_reclaim_page() has WARN_ON_ONCE() +
> pr_tdx_error() in all error paths.
> 
> > > +		/* If reclaim failed, leak the page. */
> > 
> > Better add a FIXME: here as this has to be fixed later.
> 
> No, leaking the page is all KVM can reasonably do here.  An improved
> comment would be helpful, but no code change is required.
> tdx_reclaim_page() returns an error if and only if there's an
> unexpected, fatal error, e.g. a SEAMCALL with bad params, incorrect
> concurrency in KVM, a TDX Module bug, etc.  Retrying at a later point is
> highly unlikely to be successful.

Hi:

The word "leaking" sounds like a situation left unhandled temporarily.

I checked the source code of the TDX module[1] for the possible reason to
fail when reviewing this patch:

tdx-module-v1.0.01.01.zip\src\vmm_dispatcher\api_calls\tdh_phymem_page_reclaim.c
tdx-module-v1.0.01.01.zip\src\vmm_dispatcher\api_calls\tdh_phymem_page_wbinvd.c

a. Invalid parameters. For example, page is not aligned, PA HKID is not zero...

For invalid parameters, a WARN_ON_ONCE() + return value is good enough as
that is how kernel handles similar situations. The caller takes the
responsibility.
 
b. Locks has been taken in TDX module. TDR page has been locked due to another
SEAMCALL, another SEAMCALL is doing PAMT walk and holding PAMT lock... 

This needs to be improved later either by retry or taking tdx_lock to avoid
TDX module fails on this.

[1] https://www.intel.com/content/www/us/en/download/738875/738876/intel-trust-domain-extension-intel-tdx-module.html