Message-ID: <c155b1ee39618c0da5fee3efd05b417e690776f3.camel@intel.com>
Date: Tue, 21 May 2024 02:57:15 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "Yamahata, Isaku" <isaku.yamahata@...el.com>
CC: "pbonzini@...hat.com" <pbonzini@...hat.com>, "isaku.yamahata@...il.com"
	<isaku.yamahata@...il.com>, "seanjc@...gle.com" <seanjc@...gle.com>, "Huang,
 Kai" <kai.huang@...el.com>, "sagis@...gle.com" <sagis@...gle.com>,
	"isaku.yamahata@...ux.intel.com" <isaku.yamahata@...ux.intel.com>, "Zhao, Yan
 Y" <yan.y.zhao@...el.com>, "Aktas, Erdem" <erdemaktas@...gle.com>,
	"kvm@...r.kernel.org" <kvm@...r.kernel.org>, "dmatlack@...gle.com"
	<dmatlack@...gle.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 10/16] KVM: x86/tdp_mmu: Support TDX private mapping for
 TDP MMU

On Mon, 2024-05-20 at 19:25 -0700, Isaku Yamahata wrote:
> On Mon, May 20, 2024 at 11:39:06PM +0000,
> "Edgecombe, Rick P" <rick.p.edgecombe@...el.com> wrote:
> 
> > On Mon, 2024-05-20 at 12:02 -0700, Rick Edgecombe wrote:
> > 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.

Yea, direct is kind of overloaded. But it actually is "direct" in the
role.direct sense at least.

> 
> 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.

It seems like it would use too much memory. Looking up the mask once per
iteration doesn't seem too terrible to me.

> 
> 
> > 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.

We need a better name for sp->mirrored_spt and related functions. It is not the
mirror page table, it's the actual page table that is getting mirrored

It would be nice to have a good generic name (not private) for what the mirrored
page tables are mirroring. Mirror vs mirrored is too close, but I couldn't think
of anything. Reflect only seems to fit as a verb.


Another nice thing about this separation, I think we can break the big patch
apart a bit. I think maybe I'll start re-arranging things into patches. Unless
there is any objection to the whole direction. Kai?

Powered by blists - more mailing lists