Received-SPF: pass (google.com: domain of linux-kernel+bounces-184364-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 20 May 2024 19:25:05 -0700
From: Isaku Yamahata <isaku.yamahata@intel.com>
To: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>
Cc: "Huang, Kai" <kai.huang@intel.com>,
	"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@linux.intel.com" <isaku.yamahata@linux.intel.com>,
	"isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>,
	"Zhao, Yan Y" <yan.y.zhao@intel.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"dmatlack@google.com" <dmatlack@google.com>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	"Aktas, Erdem" <erdemaktas@google.com>
Subject: Re: [PATCH 10/16] KVM: x86/tdp_mmu: Support TDX private mapping for
 TDP MMU
Message-ID: <20240521022505.GB29916@ls.amr.corp.intel.com>
References: <eb7417cccf1065b9ac5762c4215195150c114ef8.camel@intel.com>
 <20240516194209.GL168153@ls.amr.corp.intel.com>
 <ffd24fa5-b573-4334-95c6-42429fd9ee38@intel.com>
 <20240517081440.GM168153@ls.amr.corp.intel.com>
 <b6ca3e0a18d7a472d89eeb48aaa22f5b019a769c.camel@intel.com>
 <0d48522f37d75d63f09d2a5091e3fa91913531ee.camel@intel.com>
 <791ab3de8170d90909f3e053bf91485784d36c61.camel@intel.com>
 <20240520185817.GA22775@ls.amr.corp.intel.com>
 <91444be8576ac220fb66cd8546697912988c4a87.camel@intel.com>
 <2ee12c152c8db9f5e4acd131b95411bac0abb22c.camel@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <2ee12c152c8db9f5e4acd131b95411bac0abb22c.camel@intel.com>

On Mon, May 20, 2024 at 11:39:06PM +0000,
"Edgecombe, Rick P" <rick.p.edgecombe@intel.com> wrote:

> On Mon, 2024-05-20 at 12:02 -0700, Rick Edgecombe wrote:
> > 
> > reflect is a nice name. I'm trying this path right now. I'll share a branch.
> 
> Here is the branch:
> https://github.com/rpedgeco/linux/commit/674cd68b6ba626e48fe2446797d067e38dca80e3

Thank you for sharing it. It makes it easy to create further patches on top of
it.

..

> In this solution, the tdp_mmu.c doesn't have a concept of private vs shared EPT
> or GPA aliases. It just knows KVM_PROCESS_PRIVATE/SHARED, and fault->is_private.
> 
> Based on the PROCESS enums or fault->is_private, helpers in mmu.h encapsulate
> whether to operate on the normal "direct" roots or the mirrored roots. When
> !TDX, it always operates on direct.
> 
> The code that does PTE setting/zapping etc, calls out the mirrored "reflect"
> helper and does the extra atomicity stuff when it sees the mirrored role bit.
> 
> In Isaku's code to make gfn's never have shared bits, there was still the
> concept of "shared" in the TDP MMU. But now since the TDP MMU focuses on
> mirrored vs direct instead, an abstraction is introduced to just ask for the
> mask for the root. For TDX the direct root is for shared memory, so instead the
> kvm_gfn_direct_mask() gets applied when operating on the direct root.

"direct" is better than "shared".  It might be confusing with the existing
role.direct, but I don't think of better other name.

I resorted to pass around kvm for gfn_direct_mask to the iterator.  Alternative
way is to stash it in struct kvm_mmu_page of root somehow.  Then, we can strip
kvm from the iterator and the related macros.


> I think there are still some things to be polished in the branch, but overall it
> does a good job of cleaning up the confusion about the connection between
> private and mirrored. And also between this and the previous changes, improves
> littering the generic MMU code with private/shared alias concepts.
> 
> At the same time, I think the abstractions have a small cost in clarity if you
> are looking at the code from TDX's perspective. It probably wont raise any
> eyebrows for people used to tracing nested EPT violations through paging_tmpl.h.
> But compared to naming everything mirrored_private, there is more obfuscation of
> the bits twiddled.

The rename makes the code much less confusing.  I noticed that mirror and
mirrored are mixed. I'm not sure whether it's intentional or accidental.
-- 
Isaku Yamahata <isaku.yamahata@intel.com>