Received-SPF: pass (google.com: domain of linux-kernel+bounces-121341-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 27 Mar 2024 21:07:21 +0800
From: Chao Gao <chao.gao@intel.com>
To: <isaku.yamahata@intel.com>
CC: <kvm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<isaku.yamahata@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>,
	<erdemaktas@google.com>, Sean Christopherson <seanjc@google.com>, Sagi Shahar
	<sagis@google.com>, Kai Huang <kai.huang@intel.com>, <chen.bo@intel.com>,
	<hang.yuan@intel.com>, <tina.zhang@intel.com>
Subject: Re: [PATCH v19 062/130] KVM: x86/tdp_mmu: Support TDX private
 mapping for TDP MMU
Message-ID: <ZgQaCcdb4AshplI6@chao-email>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <fc97847d04f2b469d8f4cfceee84c7ef055ab1ac.1708933498.git.isaku.yamahata@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <fc97847d04f2b469d8f4cfceee84c7ef055ab1ac.1708933498.git.isaku.yamahata@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?QAcFBVU2a6WQaezrkqmRXhD3lyQmS+veJpsHXs+fyCMycPUUKMrxphXL8PWD?=
 =?us-ascii?Q?5IAf36HIvSq4hiYlNMZRVXTfnQ6usYBR799c0PPE6kjWHg4zCqkKD7WrDmcn?=
 =?us-ascii?Q?DVOL9SdtnPBE/+lsU4TZNNseEQyok+KfmgfwzxSVMAVlCPQP9n37Jr/VAqVH?=
 =?us-ascii?Q?12Dlktc9t4geLoD23Bq+oXZiVsf8gsUuyZfBIn+W2KJjmaPcO1Fj/tLKXxHz?=
 =?us-ascii?Q?ffKKnlC8f7UjWlZiEImClmp24KuMse6IZ/8yyzqhTqhGSGndKyW+Y7JQ0dKN?=
 =?us-ascii?Q?E2TXlScv97087IzNoHu1+s1DI6aE6Xxi31GYDZSS/efLYDEQJ+vZdRXUAwtz?=
 =?us-ascii?Q?3pcTq8gfVOgGHPFEVZTHmXe1PifqqazEEM9/LlTvQ0bl3BNM5aVaRnA3vaXq?=
 =?us-ascii?Q?Bu8J7BuzlvCyjEX5+McpDO1IbXuSiTPRhyKWim6SjCqMfN7Kdb2jXWfnZsAt?=
 =?us-ascii?Q?NOQtomv6UK0jh2iKf33Kktp9SS1RrRLMxrF4klBYCYZaFqnAWXxuEF2/jE2o?=
 =?us-ascii?Q?x2PP2ag3SY1HCLnSRHM3zAYr0eZiXMS7oZWnZhscsEQ/G8F1XyAIJ0EBHPcq?=
 =?us-ascii?Q?OwxYscfa88JWfEIPRLVp+bV73yTcHppqxu8xHFPT/tQ96qq9uizuzTKD6cPs?=
 =?us-ascii?Q?EXTlQI1CmS+NfxhOsNr18tYWWh+mo+xSyxlMzUxV7Z0glhlIzo6FYalSAKey?=
 =?us-ascii?Q?k4TVIV3cDVmxZcZh6bT8LYiDtZfdyPDNXvap9r8mY/TJtqaI6ExRIpDKUk6w?=
 =?us-ascii?Q?+P/24Z2EakPmCwYBJA05JhmZ9LU1uXbpiYki7iPNeW/0z5LkqRSuPWu7pz/4?=
 =?us-ascii?Q?AKC7mCeVyfTs5FOIvMjwrlkcSsAgWp5ibWXs5kMpEOPns3pCIqZkvTXBtN/4?=
 =?us-ascii?Q?C8YsM9RUaEvqpgck0M4hyisebBX/oM/aK2jEhEV8doDw7WG7gP0bBAMXe3rg?=
 =?us-ascii?Q?pzAFhow1Mau02sj1ilEeCi8J0wTgh39EClCcGRXAyuiwMepKSA9H8g63kuoo?=
 =?us-ascii?Q?hzse9RcdPby8H832Wjyvx0bZ3MjiRiY09YFNs5kzeo5E4Nraw1TQiE5rjwLP?=
 =?us-ascii?Q?6lYHLI6H9PdtLRX3LLPgIXLoHVeHWznfi9LXDMR9gbmvrRx2Ubj5dHZn97Wm?=
 =?us-ascii?Q?YS4ABuifqRRE2+d2T6v8ikw/QDd+popYW8aATa1ZmhrvfT1OuLFLcLDTiRbE?=
 =?us-ascii?Q?/XIFwLvsyRkpBDeqX3lYb53dPFhZcgtwqcH5JQvbFYyTh44lQuyVx4zN94A/?=
 =?us-ascii?Q?OZqttOzi0BCHJtkA9zzcmZR9zRZt9hB4w+LIijC9huU7+wRg1U27rc0XSjFp?=
 =?us-ascii?Q?AXYWcMDfv3fovk2zYrpLKr6jHfOh/OTTw5mYzsTcvQ9MPuFfLw8m/d8t9PRf?=
 =?us-ascii?Q?klYvnupNxFh5Yk7QTGv8FErjYe7Ky3h+IP5GTp4nwMFjCG/fjlbx03+lQoG6?=
 =?us-ascii?Q?tPrDZresE6M17xcygr6TSSSjTzfmzv7QnlJpXBz7I8xmWXSWVU+po1Zfbgh+?=
 =?us-ascii?Q?Oc7RIRYU06EwFdh+LBb/ORs83Gv+qe/VTUL/PFxQ0pk7W/rGhYcOKFgjeEaF?=
 =?us-ascii?Q?JnBznWjP8l1nCZBV7bwrHesr1xhpSSlqrODTHQlu?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 53f574d9-489c-48e8-1385-08dc4e5edc5a
X-MS-Exchange-CrossTenant-AuthSource: CH3PR11MB8660.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Mar 2024 13:07:31.2991
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: N4f+uFvRky80QUZ5eIpqLsJr18hleXF0xT5xBTnpSzt4VJQiYI4S36BwshH/BWh25erUqDWbtc9ntEjIgMt2cA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4579
X-OriginatorOrg: intel.com

>--- a/arch/x86/kvm/mmu/mmu.c
>+++ b/arch/x86/kvm/mmu/mmu.c
>@@ -3717,7 +3717,12 @@ static int mmu_alloc_direct_roots(struct kvm_vcpu *vcpu)
> 		goto out_unlock;
> 
> 	if (tdp_mmu_enabled) {
>-		root = kvm_tdp_mmu_get_vcpu_root_hpa(vcpu);
>+		if (kvm_gfn_shared_mask(vcpu->kvm) &&
>+		    !VALID_PAGE(mmu->private_root_hpa)) {
>+			root = kvm_tdp_mmu_get_vcpu_root_hpa(vcpu, true);
>+			mmu->private_root_hpa = root;

just
			mmu->private_root_hpa =
				kvm_tdp_mmu_get_vcpu_root_hpa(vcpu, true);
>+		}
>+		root = kvm_tdp_mmu_get_vcpu_root_hpa(vcpu, false);
> 		mmu->root.hpa = root;

ditto

> 	} else if (shadow_root_level >= PT64_ROOT_4LEVEL) {
> 		root = mmu_alloc_root(vcpu, 0, 0, shadow_root_level);
>@@ -4627,7 +4632,7 @@ int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
> 	if (kvm_mmu_honors_guest_mtrrs(vcpu->kvm)) {
> 		for ( ; fault->max_level > PG_LEVEL_4K; --fault->max_level) {
> 			int page_num = KVM_PAGES_PER_HPAGE(fault->max_level);
>-			gfn_t base = gfn_round_for_level(fault->gfn,
>+			gfn_t base = gfn_round_for_level(gpa_to_gfn(fault->addr),
> 							 fault->max_level);

..

> 
> 			if (kvm_mtrr_check_gfn_range_consistency(vcpu, base, page_num))
>@@ -4662,6 +4667,7 @@ int kvm_mmu_map_tdp_page(struct kvm_vcpu *vcpu, gpa_t gpa, u64 error_code,
> 	};
> 
> 	WARN_ON_ONCE(!vcpu->arch.mmu->root_role.direct);
>+	fault.gfn = gpa_to_gfn(fault.addr) & ~kvm_gfn_shared_mask(vcpu->kvm);

Could you clarify when shared bits need to be masked out or kept? shared bits
are masked out here but kept in the hunk right above and ..

>+++ b/arch/x86/kvm/mmu/tdp_iter.h
>@@ -91,7 +91,7 @@ struct tdp_iter {
> 	tdp_ptep_t pt_path[PT64_ROOT_MAX_LEVEL];
> 	/* A pointer to the current SPTE */
> 	tdp_ptep_t sptep;
>-	/* The lowest GFN mapped by the current SPTE */
>+	/* The lowest GFN (shared bits included) mapped by the current SPTE */
> 	gfn_t gfn;

. in @gfn of tdp_iter.

> 	/* The level of the root page given to the iterator */
> 	int root_level;


> 
>-hpa_t kvm_tdp_mmu_get_vcpu_root_hpa(struct kvm_vcpu *vcpu)
>+static struct kvm_mmu_page *kvm_tdp_mmu_get_vcpu_root(struct kvm_vcpu *vcpu,

Maybe fold it into its sole caller.

>+						      bool private)
> {
> 	union kvm_mmu_page_role role = vcpu->arch.mmu->root_role;
> 	struct kvm *kvm = vcpu->kvm;
>@@ -221,6 +225,8 @@ hpa_t kvm_tdp_mmu_get_vcpu_root_hpa(struct kvm_vcpu *vcpu)
> 	 * Check for an existing root before allocating a new one.  Note, the
> 	 * role check prevents consuming an invalid root.
> 	 */
>+	if (private)
>+		kvm_mmu_page_role_set_private(&role);
> 	for_each_tdp_mmu_root(kvm, root, kvm_mmu_role_as_id(role)) {
> 		if (root->role.word == role.word &&
> 		    kvm_tdp_mmu_get_root(root))
>@@ -244,12 +250,17 @@ hpa_t kvm_tdp_mmu_get_vcpu_root_hpa(struct kvm_vcpu *vcpu)
> 	spin_unlock(&kvm->arch.tdp_mmu_pages_lock);
> 
> out:
>-	return __pa(root->spt);
>+	return root;
>+}
>+
>+hpa_t kvm_tdp_mmu_get_vcpu_root_hpa(struct kvm_vcpu *vcpu, bool private)
>+{
>+	return __pa(kvm_tdp_mmu_get_vcpu_root(vcpu, private)->spt);
> }
> 
> static void handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn,
>-				u64 old_spte, u64 new_spte, int level,
>-				bool shared);
>+				u64 old_spte, u64 new_spte,
>+				union kvm_mmu_page_role role, bool shared);
> 
> static void tdp_account_mmu_page(struct kvm *kvm, struct kvm_mmu_page *sp)
> {
>@@ -376,12 +387,78 @@ static void handle_removed_pt(struct kvm *kvm, tdp_ptep_t pt, bool shared)
> 							  REMOVED_SPTE, level);
> 		}
> 		handle_changed_spte(kvm, kvm_mmu_page_as_id(sp), gfn,
>-				    old_spte, REMOVED_SPTE, level, shared);
>+				    old_spte, REMOVED_SPTE, sp->role,
>+				    shared);
>+	}
>+
>+	if (is_private_sp(sp) &&
>+	    WARN_ON(static_call(kvm_x86_free_private_spt)(kvm, sp->gfn, sp->role.level,

WARN_ON_ONCE()?

>+							  kvm_mmu_private_spt(sp)))) {
>+		/*
>+		 * Failed to unlink Secure EPT page and there is nothing to do
>+		 * further.  Intentionally leak the page to prevent the kernel
>+		 * from accessing the encrypted page.
>+		 */
>+		kvm_mmu_init_private_spt(sp, NULL);
> 	}
> 
> 	call_rcu(&sp->rcu_head, tdp_mmu_free_sp_rcu_callback);
> }
> 

> 	rcu_read_lock();
> 
> 	for_each_tdp_pte_min_level(iter, root, PG_LEVEL_4K, start, end) {
>@@ -960,10 +1158,26 @@ static int tdp_mmu_map_handle_target_level(struct kvm_vcpu *vcpu,
> 
> 	if (unlikely(!fault->slot))
> 		new_spte = make_mmio_spte(vcpu, iter->gfn, ACC_ALL);
>-	else
>-		wrprot = make_spte(vcpu, sp, fault->slot, ACC_ALL, iter->gfn,
>-					 fault->pfn, iter->old_spte, fault->prefetch, true,
>-					 fault->map_writable, &new_spte);
>+	else {
>+		unsigned long pte_access = ACC_ALL;
>+		gfn_t gfn = iter->gfn;
>+
>+		if (kvm_gfn_shared_mask(vcpu->kvm)) {
>+			if (fault->is_private)
>+				gfn |= kvm_gfn_shared_mask(vcpu->kvm);

this is an open-coded kvm_gfn_to_shared().

I don't get why a spte is installed for a shared gfn when fault->is_private
is true. could you elaborate?

>+			else
>+				/*
>+				 * TDX shared GPAs are no executable, enforce
>+				 * this for the SDV.
>+				 */

what do you mean by the SDV?

>+				pte_access &= ~ACC_EXEC_MASK;
>+		}
>+
>+		wrprot = make_spte(vcpu, sp, fault->slot, pte_access, gfn,
>+				   fault->pfn, iter->old_spte,
>+				   fault->prefetch, true, fault->map_writable,
>+				   &new_spte);
>+	}
> 
> 	if (new_spte == iter->old_spte)
> 		ret = RET_PF_SPURIOUS;
>@@ -1041,6 +1255,8 @@ int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
> 	struct kvm *kvm = vcpu->kvm;
> 	struct tdp_iter iter;
> 	struct kvm_mmu_page *sp;
>+	gfn_t raw_gfn;
>+	bool is_private = fault->is_private && kvm_gfn_shared_mask(kvm);
> 	int ret = RET_PF_RETRY;
> 
> 	kvm_mmu_hugepage_adjust(vcpu, fault);
>@@ -1049,7 +1265,17 @@ int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
> 
> 	rcu_read_lock();
> 
>-	tdp_mmu_for_each_pte(iter, mmu, fault->gfn, fault->gfn + 1) {
>+	raw_gfn = gpa_to_gfn(fault->addr);
>+
>+	if (is_error_noslot_pfn(fault->pfn) ||
>+	    !kvm_pfn_to_refcounted_page(fault->pfn)) {
>+		if (is_private) {
>+			rcu_read_unlock();
>+			return -EFAULT;

This needs a comment. why this check is necessary? does this imply some
kernel bugs?