Date: Mon, 20 May 2024 10:38:58 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>, "Yamahata, Isaku"
	<isaku.yamahata@...el.com>
CC: "dmatlack@...gle.com" <dmatlack@...gle.com>, "isaku.yamahata@...il.com"
	<isaku.yamahata@...il.com>, "seanjc@...gle.com" <seanjc@...gle.com>,
	"sagis@...gle.com" <sagis@...gle.com>, "isaku.yamahata@...ux.intel.com"
	<isaku.yamahata@...ux.intel.com>, "Aktas, Erdem" <erdemaktas@...gle.com>,
	"Zhao, Yan Y" <yan.y.zhao@...el.com>, "kvm@...r.kernel.org"
	<kvm@...r.kernel.org>, "pbonzini@...hat.com" <pbonzini@...hat.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 10/16] KVM: x86/tdp_mmu: Support TDX private mapping for
 TDP MMU

On Sat, 2024-05-18 at 15:41 +0000, Edgecombe, Rick P wrote:
> On Sat, 2024-05-18 at 05:42 +0000, Huang, Kai wrote:
> > 
> > No.  I meant "using kvm_mmu_page.role.mirrored_pt to determine whether to
> > invoke kvm_x86_ops::xx_private_spt()" is not correct.
> 
> I agree this looks wrong.
> 
> >   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.
> 
> I think the problem is there are a lot of things that are more on the mirrored
> concept side:
>  - Allocating the "real" PTE pages (i.e. sp->private_spt)
>  - Setting the PTE when the mirror changes
>  - Zapping the real PTE when the mirror is zapped (and there is no fault)
>  - etc
> 
> And on the private side there is just knowing that private faults should operate
> on the mirror root.

... and issue SEAMCALL to operate the real private page table?

> 
> The xx_private_spte() operations are actually just updating the real PTE for the
> mirror. In some ways it doesn't have to be about "private". It could be a mirror
> of something else and still need the updates. For SNP and others they don't need
> to do anything like that. (AFAIU)

AFAICT xx_private_spte() should issue SEAMCALL to operate the real private
page table?

> 
> So based on that, I tried to change the naming of xx_private_spt() to reflect
> that. Like:
> if (role.mirrored)
>   update_mirrored_pte()
> 
> The TDX code could encapsulate that mirrored updates need to update private EPT.
> Then I had a helper that answered the question of whether to handle private
> faults on the mirrored root.

I am fine with this too, but I am also fine with the existing pattern:

That we update the mirrored_pt using normal TDP MMU operation, and then
invoke the xx_private_spte() for private GPA.

My only true comment is, to me it seems more reasonable to invoke
xx_private_spte() based on fault->is_private, but not on
'use_mirrored_pt'.

See my reply to your question whether SNP needs special handling below.

> 
> The FREEZE stuff actually made a bit more sense too, because it was clear it
> wasn't a special TDX private memory thing, but just about the atomicity.
> 
> The problem was I couldn't get rid of all special things that are private (can't
> remember what now).
> 
> I wonder if I should give it a more proper try. What do you think?
> 
> At this point, I was just going to change the "mirrored" name to
> "private_mirrored". Then code that does either mirrored things or private things
> both looks correct. Basically making it clear that the MMU only supports
> mirroring private memory.

I don't have preference on name.  "mirrored_private" also works for me.

> 
> > 
> > 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:
> 
> I think it does matter, especially for the other operations (not faults). Did
> you look at the other things checking the role?

Yeah I shouldn't say "doesn't matter".  I meant we can get this from the
iter->spetp or the root.

> 
> > 
> > 
> >         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.
> 
> SNP and SW_PROTECTED_VM (today) don't need do anything special here, right?

Conceptually, I think SNP also needs to at least issue some command(s) to
update the RMP table to reflect the GFN<->PFN relationship.  From this
point, I do see a fit.

I briefly looked into SNP patchset, and I also raised the discussion there
(with you and Isaku copied):

https://lore.kernel.org/lkml/20240501085210.2213060-1-michael.roth@amd.com/T/#m8ca554a6d4bad7fa94dedefcf5914df19c9b8051

I could be wrong, though.

Powered by blists - more mailing lists