Received-SPF: pass (google.com: domain of linux-kernel+bounces-181804-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 17 May 2024 15:44:27 +0800
From: Chao Gao <chao.gao@intel.com>
To: Rick Edgecombe <rick.p.edgecombe@intel.com>
CC: <pbonzini@redhat.com>, <seanjc@google.com>, <kvm@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <isaku.yamahata@gmail.com>,
	<erdemaktas@google.com>, <sagis@google.com>, <yan.y.zhao@intel.com>,
	<dmatlack@google.com>
Subject: Re: [PATCH 03/16] KVM: x86/tdp_mmu: Add a helper function to walk
 down the TDP MMU
Message-ID: <ZkcK29svLIlX3Jr9@chao-email>
References: <20240515005952.3410568-1-rick.p.edgecombe@intel.com>
 <20240515005952.3410568-4-rick.p.edgecombe@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240515005952.3410568-4-rick.p.edgecombe@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?SFQeo2AHW+JgK8VOE+7TxuLDOpCrJD91iBjaRPtm7WaMdWsNGkmn0RR1+9yP?=
 =?us-ascii?Q?4l3+WTnucytCtGMjHUJL0QUlzomCre/hWcuP88QEtw8Ixd58+oyOcM6KHlSm?=
 =?us-ascii?Q?syIVFslifSh569rBpiZemaUx5T37BLE2y+yw2flMDGDflPxZ4tXvm/KpOhG8?=
 =?us-ascii?Q?7pKbRsAKo3X9ru5E+OklzbyPPlw/f6hXNctt922inVreGWxKFjp48Whv9ZpI?=
 =?us-ascii?Q?dMGNX5Cg93iYTXdg19lpEHzcntrWx2WWm3cpQFpXUNlqTJH8vA7Uh+Y8UyxQ?=
 =?us-ascii?Q?LoZxMixFJ7BE5b21bspYI4/cpu6qmXLxwtTItJLcrGFHs6iJ+4CLk2ZfQBX1?=
 =?us-ascii?Q?cicJxxJAmKb1CJ+CNzD6h1dixLZbOaEVA2m15BAy8w6umu8fWei31CqKR6W+?=
 =?us-ascii?Q?myW/ERS7oy0bK20bRajY9hveMrdtFsSe9TO+WOrPYworVJDnjwj6iix80jJ+?=
 =?us-ascii?Q?fo1NeIaveznjl2mXyZoEeJKy+VIgmdGY/q80sz/Lv2aU+YH6Swc7mHvoR7zX?=
 =?us-ascii?Q?nDHr5q9uusW9dlRFzIZ3WO8VKSqd4yy9ODdT6+dshi3xagEgB7PcrAujN7v3?=
 =?us-ascii?Q?K2O1jsfdKLccjjsRDafWE7ZH41LGbSlZEdOXyc1O6w/nhgUEeyTmPrsxGHLq?=
 =?us-ascii?Q?+QLSrvAdxoejKzS7sAdSNYNjxVvuK5NdnGWhyWrsN0d8do6S9xiToWFMRanC?=
 =?us-ascii?Q?cipQWx7595edfhQgELX5vgyab4+zIMQ4Lm1981IPYW+Pbh0URuTRyy6s4AFi?=
 =?us-ascii?Q?XC8kpSi1ZgWK800OiU3DkmGCqwRQDGjlFksStAbX9PXStHlzAIS96iOSECN2?=
 =?us-ascii?Q?fgZX0/4403CjJEoSA6UJ/sSbTGNF/I3OS6XvohHBwGPt9n3fkjstXmnZ6m4j?=
 =?us-ascii?Q?l9WaIa/T6DhNaXZWoF5xUSIqLewLXFeojfnXduZvQVAdEktsyS9NdOHWRtZH?=
 =?us-ascii?Q?G+UjfRJbUNp0GD8kPN9wfMp4TiEnFnrh7pfAW0DXZWVYw1ScL4gl2BZ1brrv?=
 =?us-ascii?Q?T2w/ua/ostSoynXPQirqgPLCg8ry5jHKuqmF6LdRWQZuUQjsw1NBZzq+1Gb5?=
 =?us-ascii?Q?9qdGpyoKMXtvJn/EZwlDvFvVisogNfIMNPFsqvYDrR2/hrQLuovQLzWkftJ3?=
 =?us-ascii?Q?bB4R/uLddxPn36pLOgeVUFB7ALl5lXk6oAvtZ/cQXY3VOBLSe5/66CHUno5N?=
 =?us-ascii?Q?R+XHhH2P5sIzV8aWPBPss+YHRCfz/fxdyMos4eLXra7IwyqUz0oIHxEDV9rl?=
 =?us-ascii?Q?VHu9cyWy1CjPctEPre+FfefNyHciM41uKyxNX3u6iDrWYsZ+Bp/hx9G+rKVo?=
 =?us-ascii?Q?srjsIu+wavklLrB6CTXKL/QsBpkchMkQHDTORUOc3B+lWCNz8wSc8bkskPX7?=
 =?us-ascii?Q?kXWkeWxhHEi5lqStWtBTJadD+8t//j5kmfSvxC8RvkSM2PcKoylcBjCBBGwM?=
 =?us-ascii?Q?IZseHVs17UXK+YKQSnKP1u0i1NnwHytoC60aIuCihQ+rajCplUt99JBOyr3a?=
 =?us-ascii?Q?Ga1BsOCLSe56L/81L1W9pnNJMiWnE2ARvaBv567ZePUwtRqMHk41z77UDbcv?=
 =?us-ascii?Q?PElTKcxDX30fQEzskgMQag/qKbAONp5DxeCQ4HGq?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 805396d9-f2fc-4f37-c4fb-08dc76453310
X-MS-Exchange-CrossTenant-AuthSource: CH3PR11MB8660.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 May 2024 07:44:36.4546
 (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: St5PV7qAmkgudEIihorgZE8inMO6NLkmR6mMSHsyXAjUW2MV7DqFB6cshz7xlZnIbrwLV/b1/4Io5czgxnutzg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR11MB8544
X-OriginatorOrg: intel.com

On Tue, May 14, 2024 at 05:59:39PM -0700, Rick Edgecombe wrote:
>From: Isaku Yamahata <isaku.yamahata@intel.com>
>
>Export a function to walk down the TDP without modifying it.
>
>Future changes will support pre-populating TDX private memory. In order to
>implement this KVM will need to check if a given GFN is already
>pre-populated in the mirrored EPT, and verify the populated private memory
>PFN matches the current one.[1]
>
>There is already a TDP MMU walker, kvm_tdp_mmu_get_walk() for use within
>the KVM MMU that almost does what is required. However, to make sense of
>the results, MMU internal PTE helpers are needed. Refactor the code to
>provide a helper that can be used outside of the KVM MMU code.
>
>Refactoring the KVM page fault handler to support this lookup usage was
>also considered, but it was an awkward fit.
>
>Link: https://lore.kernel.org/kvm/ZfBkle1eZFfjPI8l@google.com/ [1]
>Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
>Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
>---
>This helper will be used in the future change that implements
>KVM_TDX_INIT_MEM_REGION. Please refer to the following commit for the
>usage:
>https://github.com/intel/tdx/commit/2832c6d87a4e6a46828b193173550e80b31240d4
>
>TDX MMU Part 1:
> - New patch
>---
> arch/x86/kvm/mmu.h         |  3 +++
> arch/x86/kvm/mmu/tdp_mmu.c | 37 +++++++++++++++++++++++++++++++++----
> 2 files changed, 36 insertions(+), 4 deletions(-)
>
>diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
>index dc80e72e4848..3c7a88400cbb 100644
>--- a/arch/x86/kvm/mmu.h
>+++ b/arch/x86/kvm/mmu.h
>@@ -275,6 +275,9 @@ extern bool tdp_mmu_enabled;
> #define tdp_mmu_enabled false
> #endif
> 
>+int kvm_tdp_mmu_get_walk_private_pfn(struct kvm_vcpu *vcpu, u64 gpa,
>+				     kvm_pfn_t *pfn);
>+
> static inline bool kvm_memslots_have_rmaps(struct kvm *kvm)
> {
> 	return !tdp_mmu_enabled || kvm_shadow_root_allocated(kvm);
>diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
>index 1259dd63defc..1086e3b2aa5c 100644
>--- a/arch/x86/kvm/mmu/tdp_mmu.c
>+++ b/arch/x86/kvm/mmu/tdp_mmu.c
>@@ -1772,16 +1772,14 @@ bool kvm_tdp_mmu_write_protect_gfn(struct kvm *kvm,
>  *
>  * Must be called between kvm_tdp_mmu_walk_lockless_{begin,end}.
>  */
>-int kvm_tdp_mmu_get_walk(struct kvm_vcpu *vcpu, u64 addr, u64 *sptes,
>-			 int *root_level)
>+static int __kvm_tdp_mmu_get_walk(struct kvm_vcpu *vcpu, u64 addr, u64 *sptes,
>+				  bool is_private)

is_private isn't used.

> {
> 	struct tdp_iter iter;
> 	struct kvm_mmu *mmu = vcpu->arch.mmu;
> 	gfn_t gfn = addr >> PAGE_SHIFT;
> 	int leaf = -1;
> 
>-	*root_level = vcpu->arch.mmu->root_role.level;
>-
> 	tdp_mmu_for_each_pte(iter, mmu, gfn, gfn + 1) {
> 		leaf = iter.level;
> 		sptes[leaf] = iter.old_spte;
>@@ -1790,6 +1788,37 @@ int kvm_tdp_mmu_get_walk(struct kvm_vcpu *vcpu, u64 addr, u64 *sptes,
> 	return leaf;
> }
> 
>+int kvm_tdp_mmu_get_walk(struct kvm_vcpu *vcpu, u64 addr, u64 *sptes,
>+			 int *root_level)
>+{
>+	*root_level = vcpu->arch.mmu->root_role.level;
>+
>+	return __kvm_tdp_mmu_get_walk(vcpu, addr, sptes, false);
>+}
>+
>+int kvm_tdp_mmu_get_walk_private_pfn(struct kvm_vcpu *vcpu, u64 gpa,
>+				     kvm_pfn_t *pfn)

private_pfn probably is a misnomer. shared/private is an attribute of
GPA rather than pfn. Since the function is to get pfn from gpa, how about
kvm_tdp_mmu_gpa_to_pfn()?

And the function is limited to handle private gpa only. It is an artificial
limitation we can get rid of easily. e.g., by making the function take
"is_private" boolean and relay it to __kvm_tdp_mmu_get_walk(). I know TDX
just calls the function to convert private gpa but having a generic API
can accommodate future use cases (e.g., get hpa from shared gpa) w/o the
need of refactoring.

>+{
>+	u64 sptes[PT64_ROOT_MAX_LEVEL + 1], spte;
>+	int leaf;
>+
>+	lockdep_assert_held(&vcpu->kvm->mmu_lock);
>+
>+	rcu_read_lock();
>+	leaf = __kvm_tdp_mmu_get_walk(vcpu, gpa, sptes, true);
>+	rcu_read_unlock();
>+	if (leaf < 0)
>+		return -ENOENT;
>+
>+	spte = sptes[leaf];
>+	if (!(is_shadow_present_pte(spte) && is_last_spte(spte, leaf)))
>+		return -ENOENT;
>+
>+	*pfn = spte_to_pfn(spte);
>+	return leaf;
>+}
>+EXPORT_SYMBOL_GPL(kvm_tdp_mmu_get_walk_private_pfn);
>+
> /*
>  * Returns the last level spte pointer of the shadow page walk for the given
>  * gpa, and sets *spte to the spte value. This spte may be non-preset. If no
>-- 
>2.34.1
>
>