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

[Isaku Yamahata <isaku.yamahata@gmail.com>]

On Thu, Jun 22, 2023 at 11:52:56PM +0000,
"Huang, Kai" <kai.huang@intel.com> 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@intel.com> 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@gmail.com>