Received-SPF: pass (google.com: domain of linux-kernel+bounces-194494-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 29 May 2024 09:55:20 -0700
From: Isaku Yamahata <isaku.yamahata@intel.com>
To: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>
Cc: "Yamahata, Isaku" <isaku.yamahata@intel.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"seanjc@google.com" <seanjc@google.com>,
	"sagis@google.com" <sagis@google.com>,
	"isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>,
	"Zhao, Yan Y" <yan.y.zhao@intel.com>,
	"Aktas, Erdem" <erdemaktas@google.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	"dmatlack@google.com" <dmatlack@google.com>,
	isaku.yamahata@linux.intel.com
Subject: Re: [PATCH 10/16] KVM: x86/tdp_mmu: Support TDX private mapping for
 TDP MMU
Message-ID: <20240529165520.GH386318@ls.amr.corp.intel.com>
References: <20240515005952.3410568-1-rick.p.edgecombe@intel.com>
 <20240515005952.3410568-11-rick.p.edgecombe@intel.com>
 <a5ad658b1eec38f621315431a24e028187b5ca2d.camel@intel.com>
 <20240529015749.GF386318@ls.amr.corp.intel.com>
 <6a831f1b429ab6d1dd3e60a947cab7ab0b3f7149.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: <6a831f1b429ab6d1dd3e60a947cab7ab0b3f7149.camel@intel.com>

On Wed, May 29, 2024 at 02:13:24AM +0000,
"Edgecombe, Rick P" <rick.p.edgecombe@intel.com> wrote:

> On Tue, 2024-05-28 at 18:57 -0700, Isaku Yamahata wrote:
> > > --- a/arch/x86/kvm/mmu/tdp_mmu.c
> > > +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> > > @@ -438,6 +438,9 @@ static void handle_removed_pt(struct kvm *kvm,
> > > tdp_ptep_t
> > > pt, bool shared)
> > >                           */
> > >                          old_spte = kvm_tdp_mmu_write_spte(sptep, old_spte,
> > >                                                            REMOVED_SPTE,
> > > level);
> > > +
> > > +                       if (is_mirror_sp(sp))
> > > +                               reflect_removed_spte(kvm, gfn, old_spte,
> > > REMOVED_SPTE, level);
> > 
> > The callback before handling lower level will result in error.
> 
> Hmm, yea the order is changed. It didn't result in an error for some reason
> though. Can you elaborate?

TDH.MEM.{PAGE, SEPT}.REMOVE() needs to be issued from the leaf.  I guess
zapping is done at only leaf by tdp_mmu_zap_leafs(). Subtree zapping case wasn't
exercised.


> > > Otherwise, we could move the "set present" mirroring operations into
> > > handle_changed_spte(), and have some earlier conditional logic do the
> > > REMOVED_SPTE parts. It starts to become more scattered.
> > > Anyway, it's just a code clarity thing arising from having hard time
> > > explaining
> > > the design in the log. Any opinions?
> > 
> > Originally I tried to consolidate the callbacks by following TDP MMU using
> > handle_changed_spte().
> 
> How did it handle the REMOVED_SPTE part of the set_present() path?

is_removed_pt() was used. It was ugly.
-- 
Isaku Yamahata <isaku.yamahata@intel.com>