Re: [PATCH v3 16/17] KVM: x86/tdp_mmu: Propagate tearing down mirror page tables

[Binbin Wu <binbin.wu@linux.intel.com>]

On 6/20/2024 6:36 AM, Rick Edgecombe wrote:
> From: Isaku Yamahata <isaku.yamahata@intel.com>
>
> Integrate hooks for mirroring page table operations for cases where TDX
> will zap PTEs or free page tables.
>
> Like other Coco technologies, TDX has the concept of private and shared
> memory. For TDX the private and shared mappings are managed on separate
> EPT roots. The private half is managed indirectly though calls into a
> protected runtime environment called the TDX module, where the shared half
> is managed within KVM in normal page tables.
>
> Since calls into the TDX module are relatively slow, walking private page
> tables by making calls into the TDX module would not be efficient. Because
> of this, previous changes have taught the TDP MMU to keep a mirror root,
> which is separate, unmapped TDP root that private operations can be
> directed to. Currently this root is disconnected from the guest. Now add
> plumbing to propagate changes to the "external" page tables being
> mirrored. Just create the x86_ops for now, leave plumbing the operations
> into the TDX module for future patches.
>
> Add two operations for tearing down page tables, one for freeing page
> tables (free_external_spt) and one for zapping PTEs (remove_external_spte).
> Define them such that remove_external_spte will perform a TLB flush as
> well. (in TDX terms "ensure there are no active translations").
>
> TDX MMU support will exclude certain MMU operations, so only plug in the
> mirroring x86 ops where they will be needed. For zapping/freeing, only
> hook tdp_mmu_iter_set_spte() which is use used for mapping and linking
                                         ^
                                         extra "use"

Also, this sentence is a bit confusing about "used for mapping and linking".

> PTs. Don't bother hooking tdp_mmu_set_spte_atomic() as it is only used for
> zapping PTEs in operations unsupported by TDX: zapping collapsible PTEs and
> kvm_mmu_zap_all_fast().
>
> In previous changes to address races around concurrent populating using
> tdp_mmu_set_spte_atomic(), a solution was introduced to temporarily set
> REMOVED_SPTE in the mirrored page tables while performing the external
   ^
  FROZEN_SPTE

> operations. Such a solution is not needed for the tear down paths in TDX
> as these will always be performed with the mmu_lock held for write.
> Sprinkle some KVM_BUG_ON()s to reflect this.
>
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> Co-developed-by: Kai Huang <kai.huang@intel.com>
> Signed-off-by: Kai Huang <kai.huang@intel.com>
> Co-developed-by: Yan Zhao <yan.y.zhao@intel.com>
> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> Co-developed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> ---
> TDX MMU Prep v3:
>   - Rename mirrored->external (Paolo)
>   - Drop new_spte arg from reflect_removed_spte() (Paolo)
>   - ...and drop was_present and is_present bools (Paolo)
>   - Use base_gfn instead of sp->gfn (Paolo)
>   - Better comment on logic that bugs if doing tdp_mmu_set_spte() on
>     present PTE. (Paolo)
>   - Move comment around KVM_BUG_ON() in __tdp_mmu_set_spte_atomic() to this
>     patch, and add better comment. (Paolo)
>   - In remove_external_spte(), remove was_leaf bool, skip duplicates
>     present check and add comment.
>   - Rename REMOVED_SPTE to FROZEN_SPTE (Paolo)
>
> TDX MMU Prep v2:
>   - Split from "KVM: x86/tdp_mmu: Support TDX private mapping for TDP MMU"
>   - Rename x86_ops from "private" to "reflect"
>   - In response to "sp->mirrored_spt" rename helpers to "mirrored"
>   - Remove unused present mirroring support in tdp_mmu_set_spte()
>   - Merge reflect_zap_spte() into reflect_remove_spte()
>   - Move mirror zapping logic out of handle_changed_spte()
>   - Add some KVM_BUG_ONs
> ---
>   arch/x86/include/asm/kvm-x86-ops.h |  2 ++
>   arch/x86/include/asm/kvm_host.h    |  8 +++++
>   arch/x86/kvm/mmu/tdp_mmu.c         | 51 +++++++++++++++++++++++++++++-
>   3 files changed, 60 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
> index 3ef19fcb5e42..18a83b211c90 100644
> --- a/arch/x86/include/asm/kvm-x86-ops.h
> +++ b/arch/x86/include/asm/kvm-x86-ops.h
> @@ -97,6 +97,8 @@ KVM_X86_OP_OPTIONAL_RET0(get_mt_mask)
>   KVM_X86_OP(load_mmu_pgd)
>   KVM_X86_OP_OPTIONAL(link_external_spt)
>   KVM_X86_OP_OPTIONAL(set_external_spte)
> +KVM_X86_OP_OPTIONAL(free_external_spt)
> +KVM_X86_OP_OPTIONAL(remove_external_spte)
>   KVM_X86_OP(has_wbinvd_exit)
>   KVM_X86_OP(get_l2_tsc_offset)
>   KVM_X86_OP(get_l2_tsc_multiplier)
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 12ff04135a0e..dca623ffa903 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1745,6 +1745,14 @@ struct kvm_x86_ops {
>   	int (*set_external_spte)(struct kvm *kvm, gfn_t gfn, enum pg_level level,
>   				 kvm_pfn_t pfn_for_gfn);
>   
> +	/* Update external page tables for page table about to be freed */
Nit: Add "." at the end of the sentence.

> +	int (*free_external_spt)(struct kvm *kvm, gfn_t gfn, enum pg_level level,
> +				 void *external_spt);
> +
> +	/* Update external page table from spte getting removed, and flush TLB */
Ditto

> +	int (*remove_external_spte)(struct kvm *kvm, gfn_t gfn, enum pg_level level,
> +				    kvm_pfn_t pfn_for_gfn);
> +
>   	bool (*has_wbinvd_exit)(void);
>   
>   	u64 (*get_l2_tsc_offset)(struct kvm_vcpu *vcpu);
[...]