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

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.

Powered by blists - more mailing lists