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.
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)
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.
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.
>
> 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?
>
>
> 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?
>
> 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?
I think two things:
- fault->is_private is only for faults, and we have other cases where we call
out to kvm_x86_ops.xx_private() things.
- Calling out to update something else is really more about the "mirrored"
concept then about private.
