Received-SPF: pass (google.com: domain of linux-kernel+bounces-212097-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Wed, 12 Jun 2024 11:39:05 -0700
From: Isaku Yamahata <isaku.yamahata@intel.com>
To: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>
Cc: "pbonzini@redhat.com" <pbonzini@redhat.com>,
	"seanjc@google.com" <seanjc@google.com>,
	"Huang, Kai" <kai.huang@intel.com>,
	"sagis@google.com" <sagis@google.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Aktas, Erdem" <erdemaktas@google.com>,
	"Zhao, Yan Y" <yan.y.zhao@intel.com>,
	"dmatlack@google.com" <dmatlack@google.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"Yamahata, Isaku" <isaku.yamahata@intel.com>,
	"isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>,
	isaku.yamahata@linux.intel.com
Subject: Re: [PATCH v2 11/15] KVM: x86/tdp_mmu: Reflect tearing down mirror
 page tables
Message-ID: <20240612183905.GJ386318@ls.amr.corp.intel.com>
References: <20240530210714.364118-1-rick.p.edgecombe@intel.com>
 <20240530210714.364118-12-rick.p.edgecombe@intel.com>
 <CABgObfbA1oBc-D++DyoQ-o6uO0vEpp6R9bMo8UjvmRJ73AZzKQ@mail.gmail.com>
 <c2bab3f7157e6f3b71723cebc0533ef0a908a3b5.camel@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <c2bab3f7157e6f3b71723cebc0533ef0a908a3b5.camel@intel.com>

On Fri, Jun 07, 2024 at 09:46:27PM +0000,
"Edgecombe, Rick P" <rick.p.edgecombe@intel.com> wrote:

> > /* Here we only care about zapping the external leaf PTEs. */
> > if (!is_last_spte(old_spte, level))
> > 
> > > +       kvm_pfn_t old_pfn = spte_to_pfn(old_spte);
> > > +       int ret;
> > > +
> > > +       /*
> > > +        * Allow only leaf page to be zapped. Reclaim non-leaf page tables
> > > page
> > 
> > This comment left me confused, so I'll try to rephrase and see if I
> > can explain what happens. Correct me if I'm wrong.
> > 
> > The only paths to handle_removed_pt() are:
> > - kvm_tdp_mmu_zap_leafs()
> > - kvm_tdp_mmu_zap_invalidated_roots()
> > 
> > but because kvm_mmu_zap_all_fast() does not operate on mirror roots,
> > the latter can only happen at VM destruction time.
> > 
> > But it's not clear why it's worth mentioning it here, or even why it
> > is special at all. Isn't that just what handle_removed_pt() does at
> > the end? Why does it matter that it's only done at VM destruction
> > time?
> > 
> > In other words, it seems to me that this comment is TMI. And if I am
> > wrong (which may well be), the extra information should explain the
> > "why" in more detail, and it should be around the call to
> > reflect_free_spt, not here.
> 
> TDX of course has the limitation around the ordering of the zapping S-EPT. So I
> read the comment to be referring to how the implementation avoids zapping any
> non-leaf PTEs during TD runtime.
> 
> But I'm going to have to circle back here after investigating a bit more. Isaku,
> any comments on this comment and conditional?

It's for large page page merge/split.  At this point, it seems only confusing.
We need only leaf zapping.  Maybe reflect_removed_leaf_spte() or something.
Later we can come back when large page support.
-- 
Isaku Yamahata <isaku.yamahata@intel.com>