On Thu, Jun 22, 2023 at 11:52:56PM +0000, "Huang, Kai" <kai.huang@xxxxxxxxx> wrote: > On Thu, 2023-06-22 at 16:39 -0700, Isaku Yamahata wrote: > > 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. > > > > > > I don't buy the last sentence. Even it's not necessarily for AMD from > hardware's perspective, but the concept remains true for AMD too. So why cannot > we use it for AMD? We can use it for AMD. Let me rephrase it. TDX only uses it now. SEV-SNP may or may not use it at their option. -- Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
