Message-ID: <e59e5e1a-a7bc-4649-abbe-f0e64fbfcff3@linux.intel.com>
Date: Thu, 20 Jun 2024 13:15:16 +0800
From: Binbin Wu <binbin.wu@...ux.intel.com>
To: Rick Edgecombe <rick.p.edgecombe@...el.com>
Cc: seanjc@...gle.com, pbonzini@...hat.com, kvm@...r.kernel.org,
 kai.huang@...el.com, dmatlack@...gle.com, erdemaktas@...gle.com,
 isaku.yamahata@...il.com, linux-kernel@...r.kernel.org, sagis@...gle.com,
 yan.y.zhao@...el.com, Isaku Yamahata <isaku.yamahata@...el.com>
Subject: Re: [PATCH v3 15/17] KVM: x86/tdp_mmu: Propagate building mirror page
 tables



On 6/20/2024 6:36 AM, Rick Edgecombe wrote:
> From: Isaku Yamahata <isaku.yamahata@...el.com>
>
> Integrate hooks for mirroring page table operations for cases where TDX
> will set PTEs or link 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 any actual guest
> mapping. 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 setting up external page tables, one for linking
> new page tables and one for setting leaf PTEs. Don't add any op for
> configuring the root PFN, as TDX handles this itself. Don't provide a
> way to set permissions on the PTEs also, as TDX doesn't support it.
>
> This results is MMU "mirroring" support that is very targeted towards TDX.
                ^
                extra "is"?
> Since it is likely there will be no other user, the main benefit of making
> the support generic is to keep TDX specific *looking* code outside of the
> MMU. As a generic feature it will make enough sense from TDX's
> perspective. For developers unfamiliar with TDX arch it can express the
> general concepts such that they can continue to work in the code.
>
> TDX MMU support will exclude certain MMU operations, so only plug in the
> mirroring x86 ops where they will be needed. For setting/linking, only
> hook tdp_mmu_set_spte_atomic() which is use used for mapping and linking
> PTs. Don't bother hooking tdp_mmu_iter_set_spte() as it is only used for
> setting PTEs in operations unsupported by TDX: splitting huge pages and
> write protecting. Sprinkle a KVM_BUG_ON()s to document as code that these
                             ^
                             extra "a"
> paths are not supported for mirrored page tables. For zapping operations,
> leave those for near future changes.
>
> Many operations in the TDP MMU depend on atomicity of the PTE update.
> While the mirror PTE on KVM's side can be updated atomically, the update
> that happens inside the external operations (S-EPT updates via TDX module
> call) can't happen atomically with the mirror update. The following race
> could result during two vCPU's populating private memory:
>
> * vcpu 1: atomically update 2M level mirror EPT entry to be present
> * vcpu 2: read 2M level EPT entry that is present
> * vcpu 2: walk down into 4K level EPT
> * vcpu 2: atomically update 4K level mirror EPT entry to be present
> * vcpu 2: set_exterma;_spte() to update 4K secure EPT entry => error
>            because 2M secure EPT entry is not populated yet
> * vcpu 1: link_external_spt() to update 2M secure EPT entry
>
> Prevent this by setting the mirror PTE to FROZEN_SPTE while the reflect
> operations are performed. Only write the actual mirror PTE value once the
             ^
             maybe "are being"
> reflect operations has completed. When trying to set a PTE to present and
> encountering a removed SPTE, retry the fault.
                 ^
                 frozen
>
> By doing this the race is prevented as follows:
> * vcpu 1: atomically update 2M level EPT entry to be FROZEN_SPTE
> * vcpu 2: read 2M level EPT entry that is FROZEN_SPTE
> * vcpu 2: find that the EPT entry is frozen
>            abandon page table walk to resume guest execution
> * vcpu 1: link_external_spt() to update 2M secure EPT entry
> * vcpu 1: atomically update 2M level EPT entry to be present (unfreeze)
> * vcpu 2: resume guest execution
>            Depending on vcpu 1 state, vcpu 2 may result in EPT violation
>            again or make progress on guest execution
>
> Signed-off-by: Isaku Yamahata <isaku.yamahata@...el.com>
> Co-developed-by: Kai Huang <kai.huang@...el.com>
> Signed-off-by: Kai Huang <kai.huang@...el.com>
> Co-developed-by: Yan Zhao <yan.y.zhao@...el.com>
> Signed-off-by: Yan Zhao <yan.y.zhao@...el.com>
> Co-developed-by: Rick Edgecombe <rick.p.edgecombe@...el.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@...el.com>
> ---
> TDX MMU Prep v3:
>   - Rename mirrored->external (Paolo)
>   - Better comment on logic that bugs if doing tdp_mmu_set_spte() on
>     present PTE. (Paolo)
>   - Move zapping KVM_BUG_ON() to proper patch
>   - Use spte_to_child_sp() (Paolo)
>   - Drop unnessary comment in __tdp_mmu_set_spte_atomic() (Paolo)
>   - Rename pfn->pfn_for_gfn to match remove_external_pte in next patch.
>   - 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"
>   - Drop unused old_pfn and new_pfn in handle_changed_spte()
>   - Drop redundant is_shadow_present_pte() check in __tdp_mmu_set_spte_atomic
>   - Adjust some warnings and KVM_BUG_ONs
> ---
>   arch/x86/include/asm/kvm-x86-ops.h |  2 +
>   arch/x86/include/asm/kvm_host.h    |  7 +++
>   arch/x86/kvm/mmu/tdp_mmu.c         | 98 ++++++++++++++++++++++++++----
>   3 files changed, 94 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
> index 566d19b02483..3ef19fcb5e42 100644
> --- a/arch/x86/include/asm/kvm-x86-ops.h
> +++ b/arch/x86/include/asm/kvm-x86-ops.h
> @@ -95,6 +95,8 @@ KVM_X86_OP_OPTIONAL_RET0(set_tss_addr)
>   KVM_X86_OP_OPTIONAL_RET0(set_identity_map_addr)
>   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(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 d67e88a69fc4..12ff04135a0e 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1738,6 +1738,13 @@ struct kvm_x86_ops {
>   	void (*load_mmu_pgd)(struct kvm_vcpu *vcpu, hpa_t root_hpa,
>   			     int root_level);
>   
> +	/* Update external mapping with page table link */
Nit: Add "." at the end of the sentence.

> +	int (*link_external_spt)(struct kvm *kvm, gfn_t gfn, enum pg_level level,
> +				void *mirrored_spt);
> +	/* Update the external page table from spte getting set */
Ditto.

> +	int (*set_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);
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index 582e5a045bb7..bc1ad127046d 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -244,7 +244,7 @@ void kvm_tdp_mmu_alloc_root(struct kvm_vcpu *vcpu, bool mirror)
>   	struct kvm_mmu_page *root;
>   
>   	if (mirror)
> -		role.is_mirror = 1;
> +		role.is_mirror = true;
If this is needed, it's better to be done in "KVM: x86/tdp_mmu: Support 
mirror root for TDP MMU".

>   
>   	/*
>   	 * Check for an existing root before acquiring the pages lock to avoid
> @@ -440,6 +440,59 @@ static void handle_removed_pt(struct kvm *kvm, tdp_ptep_t pt, bool shared)
>   	call_rcu(&sp->rcu_head, tdp_mmu_free_sp_rcu_callback);
>   }
>   
[...]

Powered by blists - more mailing lists