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@xxxxxxxxx> 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?
