Re: [PATCH 10/16] KVM: x86/tdp_mmu: Support TDX private mapping for TDP MMU

[Huang, Kai]

> 
> > > 
> > >      /* Add new members */
> > > 
> > >      /* Indicates which PT to walk. */
> > >      bool mirrored_pt;
> > 
> > I don't think you need this?  It's only used to select the root for page
> > table walk.  Once it's done, we already have the @sptep to operate on.
> > 
> > And I think you can just get @mirrored_pt from the sptep:
> > 
> > 	mirrored_pt = sptep_to_sp(sptep)->role.mirrored_pt;
> > 
> > Instead, I think we should keep the @is_private to indicate whether the GFN
> > is private or not, which should be distinguished with 'mirrored_pt', which
> > the root page table (and the @sptep) already reflects.
> > 
> > Of course if the @root/@sptep is mirrored_pt, the is_private should be
> > always true, like:
> > 
> > 	WARN_ON_ONCE(sptep_to_sp(sptep)->role.is_mirrored_pt
> > 			&& !is_private);
> > 
> > Am I missing anything?
> 
> You said it not correct to use role. So I tried to find a way to pass down
> is_mirrored and avoid to use role.
> 
> Did you change your mind? or you're fine with new name is_mirrored?
> 
> https://lore.kernel.org/kvm/4ba18e4e-5971-4683-82eb-63c985e98e6b@xxxxxxxxx/
>   > I don't think using kvm_mmu_page.role is correct.
> 
> 

No.  I meant "using kvm_mmu_page.role.mirrored_pt to determine whether to
invoke kvm_x86_ops::xx_private_spt()" is not correct.  Instead, we should
use fault->is_private to determine:

	if (fault->is_private && kvm_x86_ops::xx_private_spt())
		kvm_x86_ops::xx_private_spte();
	else
		// normal TDP MMU operation

The reason is this pattern works not just for TDX, but also for SNP (and
SW_PROTECTED_VM) if they ever need specific page table ops.

Whether we are operating on the mirrored page table or not doesn't matter,
because we have already selected the root page table at the beginning of
kvm_tdp_mmu_map() based on whether the VM needs to use mirrored pt for
private mapping:


	bool mirrored_pt = fault->is_private && kvm_use_mirrored_pt(kvm);

	tdp_mmu_for_each_pte(iter, mmu, mirrored_pt, raw_gfn, raw_gfn +
1) 
	{
		...
	}

#define tdp_mmu_for_each_pte(_iter, _mmu, _mirrored_pt, _start, _end)   \
        for_each_tdp_pte(_iter,                                         \
                 root_to_sp((_mirrored_pt) ? _mmu->private_root_hpa :   \
                                _mmu->root.hpa),                        \
                _start, _end)

If you somehow needs the mirrored_pt in later time when handling the page
fault, you don't need another "mirrored_pt" in tdp_iter, because you can
easily get it from the sptep (or just get from the root):

	mirrored_pt = sptep_to_sp(sptep)->role.mirrored_pt;

What we really need to pass in is the fault->is_private, because we are
not able to get whether a GPN is private based on kvm_shared_gfn_mask()
for SNP and SW_PROTECTED_VM.

Since the current KVM code only mainly passes the @kvm and the @iter for
many TDP MMU functions like tdp_mmu_set_spte_atomic(), the easiest way to
convery the fault->is_private is to add a new 'is_private' (or even
better, 'is_private_gpa' to be more precisely) to tdp_iter.

Otherwise, we either need to explicitly pass the entire @fault (which
might not be a, or @is_private_gpa.

Or perhaps I am missing anything?