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>
