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@xxxxxxx/T/#m8ca554a6d4bad7fa94dedefcf5914df19c9b8051
I could be wrong, though.
