On Thu, Jun 22, 2023 at 10:31:08PM +0000, "Huang, Kai" <kai.huang@xxxxxxxxx> wrote: > > If there are better ways to handle *how* > > that's done I don't have any complaints there, but moving/adding bits > > to GPA/error_flags after fault time just seems unecessary to me when > > fault->is_private field can serve that purpose just as well. > > Perhaps you missed my point. My point is arch.mmu_private_fault_mask and > arch.gfn_shared_mask seem redundant because the logic around them are exactly > the same. I do believe we should have fault->is_private passing to the common > MMU code. > > In fact, now I am wondering why we need to have "mmu_private_fault_mask" and > "gfn_shared_mask" in _common_ KVM MMU code. We already have enough mechanism in > KVM common code: > > 1) fault->is_private > 2) kvm_mmu_page_role.private > 3) an Xarray to tell whether a GFN is private or shared > > I am not convinced that we need to have "mmu_private_fault_mask" and > "gfn_shared_mask" in common KVM MMU code. Instead, they can be in AMD and > Intel's vendor code. > > Maybe it makes sense to have "gfn_shared_mask" in the KVM common code so that > the fault handler can just strip away the "shared bit" at the very beginning (at > least for TDX), but for the rest of the time I think we should already have > enough infrastructure to handle private/shared mapping. > > Btw, one minor issue is, if I recall correctly, for TDX the shared bit must be > applied to the GFN for shared mapping in normal EPT. I guess AMD doesn't need > that for shared mapping. So "gfn_shared_mask" maybe useful in this case, but > w/o it I believe we can also achieve in another way via vendor callback. "2) kvm_mmu_page_role.private" above has different meaning. a). The fault is private or not. b). page table the fault handler is walking is private or conventional. a.) is common for SNP, TDX and PROTECTED_VM. It makes sense in kvm_mmu_do_page_fault() and __kvm_faultin_pfn(). After kvm_faultin_pfn(), the fault handler can mostly forget it for SNP and PROTECTED_VM. (large page adjustment needs it, though.) This is what we're discussing in this thread. b.) is specific to TDX. TDX KVM MMU introduces one more page table. -- Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
