Re: [PATCH 08/16] KVM: x86/mmu: Bug the VM if kvm_zap_gfn_range() is called for TDX

["Huang, Kai" <kai.huang@intel.com>]

On 16/05/2024 11:14 am, Edgecombe, Rick P wrote:
> On Thu, 2024-05-16 at 10:17 +1200, Huang, Kai wrote:
>>> TDX has several aspects related to the TDP MMU.
>>> 1) Based on the faulting GPA, determine which KVM page table to walk.
>>>       (private-vs-shared)
>>> 2) Need to call TDX SEAMCALL to operate on Secure-EPT instead of direct
>>> memory
>>>       load/store.  TDP MMU needs hooks for it.
>>> 3) The tables must be zapped from the leaf. not the root or the middle.
>>>
>>> For 1) and 2), what about something like this?  TDX backend code will set
>>> kvm->arch.has_mirrored_pt = true; I think we will use kvm_gfn_shared_mask()
>>> only
>>> for address conversion (shared<->private).
> 
> 1 and 2 are not the same as "mirrored" though. You could have a design that
> mirrors half of the EPT and doesn't track it with separate roots. In fact, 1
> might be just a KVM design choice, even for TDX.

I am not sure whether I understand this correctly.  If they are not 
tracked with separate roots, it means they use the same page table (root).

So IIUC what you said is to support "mirror PT" at any sub-tree of the 
page table?

That will only complicate things.  I don't think we should consider 
this.  In reality, we only have TDX and SEV-SNP.  We should have a 
simple solution to cover both of them.

> 
> What we are really trying to do here is not put "is tdx" logic in the generic
> code. We could rely on the fact that TDX is the only one with mirrored TDP, but
> that is kind of what we are already doing with kvm_gfn_shared_mask().
> 
> How about we do helpers for each of your bullets, and they all just check:
> vm_type == KVM_X86_TDX_VM
> 
> So like:
> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
> index a578ea09dfb3..c0beed5b090a 100644
> --- a/arch/x86/kvm/mmu.h
> +++ b/arch/x86/kvm/mmu.h
> @@ -355,4 +355,19 @@ static inline bool kvm_is_private_gpa(const struct kvm
> *kvm, gpa_t gpa)
>          return mask && !(gpa_to_gfn(gpa) & mask);
>   }
>   
> +static inline bool kvm_has_mirrored_tdp(struct kvm *kvm)
> +{
> +       return kvm->arch.vm_type == KVM_X86_TDX_VM;
> +}
> +
> +static inline bool kvm_has_private_root(struct kvm *kvm)
> +{
> +       return kvm->arch.vm_type == KVM_X86_TDX_VM;
> +}

I don't think we need to distinguish the two.

Even we do this, if I understand your saying correctly, 
kvm_has_private_root() isn't just enough -- theoretically we can have a 
mirror pt at a sub-tree at any level.