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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240521022505.GB29916@ls.amr.corp.intel.com>
Date: Mon, 20 May 2024 19:25:05 -0700
From: Isaku Yamahata <isaku.yamahata@...el.com>
To: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
Cc: "Huang, Kai" <kai.huang@...el.com>,
	"Yamahata, Isaku" <isaku.yamahata@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"seanjc@...gle.com" <seanjc@...gle.com>,
	"sagis@...gle.com" <sagis@...gle.com>,
	"isaku.yamahata@...ux.intel.com" <isaku.yamahata@...ux.intel.com>,
	"isaku.yamahata@...il.com" <isaku.yamahata@...il.com>,
	"Zhao, Yan Y" <yan.y.zhao@...el.com>,
	"kvm@...r.kernel.org" <kvm@...r.kernel.org>,
	"dmatlack@...gle.com" <dmatlack@...gle.com>,
	"pbonzini@...hat.com" <pbonzini@...hat.com>,
	"Aktas, Erdem" <erdemaktas@...gle.com>
Subject: Re: [PATCH 10/16] KVM: x86/tdp_mmu: Support TDX private mapping for
 TDP MMU

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:
> > 
> > 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@...el.com>