Re: [PATCH RFC v9 04/51] KVM: x86: Determine shared/private faults using a configurable mask

[Huang, Kai]

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?

Note "shared_gfn_mask" and kvm_mmu_page_role.private is kinda redundant too, I
do believe we should avoid introducing a lot fields to KVM *common* code due to
vendor differences (i.e., in this case, "gfn_shared_mask" and
"mmu_private_fault_mask").