Re: [PATCH 10/16] KVM: x86/tdp_mmu: Support TDX private mapping for TDP MMU

[Isaku Yamahata]

On Mon, May 20, 2024 at 11:39:06PM +0000,
"Edgecombe, Rick P" <rick.p.edgecombe@xxxxxxxxx> 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@xxxxxxxxx>