[PATCH 0/2] KVM: arm64: pKVM memory transitions cleanup

[PATCH 0/2] KVM: arm64: pKVM memory transitions cleanup

[Oliver Upton]

In order to help resolve my own bikeshedding on the outstanding pKVM
patches [1], small deck of patches to polish up the existing memory
transitions. Mainly:

 - Rejig the layout of pkvm_mem_transition
 - Stop using out pointers to get at the 'completer' addr
 - Use better-fitting terminology (source/target) to describe the
   addresses involved in a memory transition

Applies to 6.1-rc2. Politely compile tested, and that's just about it.

Oliver Upton (2):
  KVM: arm64: Clean out the odd handling of completer_addr
  KVM: arm64: Redefine pKVM memory transitions in terms of source/target

 arch/arm64/kvm/hyp/nvhe/mem_protect.c | 124 +++++++++++---------------
 1 file changed, 52 insertions(+), 72 deletions(-)


base-commit: 247f34f7b80357943234f93f247a1ae6b6c3a740
-- 
2.38.1.273.g43a17bfeac-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] KVM: arm64: Clean out the odd handling of completer_addr

[Oliver Upton]

The layout of struct pkvm_mem_transition is a bit weird; the destination
address for the transition is actually stashed in the initiator address
context. Even weirder so, that address is thrown inside a union and
return from helpers by use of an out pointer.

Rip out the whole mess and move the destination address into the
destination context sub-struct. No functional change intended.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/hyp/nvhe/mem_protect.c | 70 ++++++++++-----------------
 1 file changed, 25 insertions(+), 45 deletions(-)

diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
index 1e78acf9662e..3636a24e1b34 100644
--- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
+++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
@@ -393,17 +393,12 @@ struct pkvm_mem_transition {
 		enum pkvm_component_id	id;
 		/* Address in the initiator's address space */
 		u64			addr;
-
-		union {
-			struct {
-				/* Address in the completer's address space */
-				u64	completer_addr;
-			} host;
-		};
 	} initiator;
 
 	struct {
 		enum pkvm_component_id	id;
+		/* Address in the completer's address space */
+		u64			addr;
 	} completer;
 };
 
@@ -471,43 +466,35 @@ static int __host_set_page_state_range(u64 addr, u64 size,
 	return host_stage2_idmap_locked(addr, size, prot);
 }
 
-static int host_request_owned_transition(u64 *completer_addr,
-					 const struct pkvm_mem_transition *tx)
+static int host_request_owned_transition(const struct pkvm_mem_transition *tx)
 {
 	u64 size = tx->nr_pages * PAGE_SIZE;
 	u64 addr = tx->initiator.addr;
 
-	*completer_addr = tx->initiator.host.completer_addr;
 	return __host_check_page_state_range(addr, size, PKVM_PAGE_OWNED);
 }
 
-static int host_request_unshare(u64 *completer_addr,
-				const struct pkvm_mem_transition *tx)
+static int host_request_unshare(const struct pkvm_mem_transition *tx)
 {
 	u64 size = tx->nr_pages * PAGE_SIZE;
 	u64 addr = tx->initiator.addr;
 
-	*completer_addr = tx->initiator.host.completer_addr;
 	return __host_check_page_state_range(addr, size, PKVM_PAGE_SHARED_OWNED);
 }
 
-static int host_initiate_share(u64 *completer_addr,
-			       const struct pkvm_mem_transition *tx)
+static int host_initiate_share(const struct pkvm_mem_transition *tx)
 {
 	u64 size = tx->nr_pages * PAGE_SIZE;
 	u64 addr = tx->initiator.addr;
 
-	*completer_addr = tx->initiator.host.completer_addr;
 	return __host_set_page_state_range(addr, size, PKVM_PAGE_SHARED_OWNED);
 }
 
-static int host_initiate_unshare(u64 *completer_addr,
-				 const struct pkvm_mem_transition *tx)
+static int host_initiate_unshare(const struct pkvm_mem_transition *tx)
 {
 	u64 size = tx->nr_pages * PAGE_SIZE;
 	u64 addr = tx->initiator.addr;
 
-	*completer_addr = tx->initiator.host.completer_addr;
 	return __host_set_page_state_range(addr, size, PKVM_PAGE_OWNED);
 }
 
@@ -537,7 +524,7 @@ static bool __hyp_ack_skip_pgtable_check(const struct pkvm_mem_transition *tx)
 		 tx->initiator.id != PKVM_ID_HOST);
 }
 
-static int hyp_ack_share(u64 addr, const struct pkvm_mem_transition *tx,
+static int hyp_ack_share(const struct pkvm_mem_transition *tx,
 			 enum kvm_pgtable_prot perms)
 {
 	u64 size = tx->nr_pages * PAGE_SIZE;
@@ -548,34 +535,35 @@ static int hyp_ack_share(u64 addr, const struct pkvm_mem_transition *tx,
 	if (__hyp_ack_skip_pgtable_check(tx))
 		return 0;
 
-	return __hyp_check_page_state_range(addr, size, PKVM_NOPAGE);
+	return __hyp_check_page_state_range(tx->completer.addr, size, PKVM_NOPAGE);
 }
 
-static int hyp_ack_unshare(u64 addr, const struct pkvm_mem_transition *tx)
+static int hyp_ack_unshare(const struct pkvm_mem_transition *tx)
 {
 	u64 size = tx->nr_pages * PAGE_SIZE;
 
 	if (__hyp_ack_skip_pgtable_check(tx))
 		return 0;
 
-	return __hyp_check_page_state_range(addr, size,
+	return __hyp_check_page_state_range(tx->completer.addr, size,
 					    PKVM_PAGE_SHARED_BORROWED);
 }
 
-static int hyp_complete_share(u64 addr, const struct pkvm_mem_transition *tx,
+static int hyp_complete_share(const struct pkvm_mem_transition *tx,
 			      enum kvm_pgtable_prot perms)
 {
-	void *start = (void *)addr, *end = start + (tx->nr_pages * PAGE_SIZE);
+	void *start = (void *)tx->completer.addr;
+	void *end = start + (tx->nr_pages * PAGE_SIZE);
 	enum kvm_pgtable_prot prot;
 
 	prot = pkvm_mkstate(perms, PKVM_PAGE_SHARED_BORROWED);
 	return pkvm_create_mappings_locked(start, end, prot);
 }
 
-static int hyp_complete_unshare(u64 addr, const struct pkvm_mem_transition *tx)
+static int hyp_complete_unshare(const struct pkvm_mem_transition *tx)
 {
 	u64 size = tx->nr_pages * PAGE_SIZE;
-	int ret = kvm_pgtable_hyp_unmap(&pkvm_pgtable, addr, size);
+	int ret = kvm_pgtable_hyp_unmap(&pkvm_pgtable, tx->completer.addr, size);
 
 	return (ret != size) ? -EFAULT : 0;
 }
@@ -583,12 +571,11 @@ static int hyp_complete_unshare(u64 addr, const struct pkvm_mem_transition *tx)
 static int check_share(struct pkvm_mem_share *share)
 {
 	const struct pkvm_mem_transition *tx = &share->tx;
-	u64 completer_addr;
 	int ret;
 
 	switch (tx->initiator.id) {
 	case PKVM_ID_HOST:
-		ret = host_request_owned_transition(&completer_addr, tx);
+		ret = host_request_owned_transition(tx);
 		break;
 	default:
 		ret = -EINVAL;
@@ -599,7 +586,7 @@ static int check_share(struct pkvm_mem_share *share)
 
 	switch (tx->completer.id) {
 	case PKVM_ID_HYP:
-		ret = hyp_ack_share(completer_addr, tx, share->completer_prot);
+		ret = hyp_ack_share(tx, share->completer_prot);
 		break;
 	default:
 		ret = -EINVAL;
@@ -611,12 +598,11 @@ static int check_share(struct pkvm_mem_share *share)
 static int __do_share(struct pkvm_mem_share *share)
 {
 	const struct pkvm_mem_transition *tx = &share->tx;
-	u64 completer_addr;
 	int ret;
 
 	switch (tx->initiator.id) {
 	case PKVM_ID_HOST:
-		ret = host_initiate_share(&completer_addr, tx);
+		ret = host_initiate_share(tx);
 		break;
 	default:
 		ret = -EINVAL;
@@ -627,7 +613,7 @@ static int __do_share(struct pkvm_mem_share *share)
 
 	switch (tx->completer.id) {
 	case PKVM_ID_HYP:
-		ret = hyp_complete_share(completer_addr, tx, share->completer_prot);
+		ret = hyp_complete_share(tx, share->completer_prot);
 		break;
 	default:
 		ret = -EINVAL;
@@ -659,12 +645,11 @@ static int do_share(struct pkvm_mem_share *share)
 static int check_unshare(struct pkvm_mem_share *share)
 {
 	const struct pkvm_mem_transition *tx = &share->tx;
-	u64 completer_addr;
 	int ret;
 
 	switch (tx->initiator.id) {
 	case PKVM_ID_HOST:
-		ret = host_request_unshare(&completer_addr, tx);
+		ret = host_request_unshare(tx);
 		break;
 	default:
 		ret = -EINVAL;
@@ -675,7 +660,7 @@ static int check_unshare(struct pkvm_mem_share *share)
 
 	switch (tx->completer.id) {
 	case PKVM_ID_HYP:
-		ret = hyp_ack_unshare(completer_addr, tx);
+		ret = hyp_ack_unshare(tx);
 		break;
 	default:
 		ret = -EINVAL;
@@ -687,12 +672,11 @@ static int check_unshare(struct pkvm_mem_share *share)
 static int __do_unshare(struct pkvm_mem_share *share)
 {
 	const struct pkvm_mem_transition *tx = &share->tx;
-	u64 completer_addr;
 	int ret;
 
 	switch (tx->initiator.id) {
 	case PKVM_ID_HOST:
-		ret = host_initiate_unshare(&completer_addr, tx);
+		ret = host_initiate_unshare(tx);
 		break;
 	default:
 		ret = -EINVAL;
@@ -703,7 +687,7 @@ static int __do_unshare(struct pkvm_mem_share *share)
 
 	switch (tx->completer.id) {
 	case PKVM_ID_HYP:
-		ret = hyp_complete_unshare(completer_addr, tx);
+		ret = hyp_complete_unshare(tx);
 		break;
 	default:
 		ret = -EINVAL;
@@ -743,12 +727,10 @@ int __pkvm_host_share_hyp(u64 pfn)
 			.initiator	= {
 				.id	= PKVM_ID_HOST,
 				.addr	= host_addr,
-				.host	= {
-					.completer_addr = hyp_addr,
-				},
 			},
 			.completer	= {
 				.id	= PKVM_ID_HYP,
+				.addr	= hyp_addr,
 			},
 		},
 		.completer_prot	= PAGE_HYP,
@@ -776,12 +758,10 @@ int __pkvm_host_unshare_hyp(u64 pfn)
 			.initiator	= {
 				.id	= PKVM_ID_HOST,
 				.addr	= host_addr,
-				.host	= {
-					.completer_addr = hyp_addr,
-				},
 			},
 			.completer	= {
 				.id	= PKVM_ID_HYP,
+				.addr	= hyp_addr,
 			},
 		},
 		.completer_prot	= PAGE_HYP,
-- 
2.38.1.273.g43a17bfeac-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] KVM: arm64: Redefine pKVM memory transitions in terms of source/target

[Oliver Upton]

Perhaps it is just me, but the 'initiator' and 'completer' terms are
slightly confusing descriptors for the addresses involved in a memory
transition. Apply a rename to instead describe memory transitions in
terms of a source and target address.

No functional change intended.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/hyp/nvhe/mem_protect.c | 68 +++++++++++++--------------
 1 file changed, 34 insertions(+), 34 deletions(-)

diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
index 3636a24e1b34..3ea389a8166f 100644
--- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
+++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
@@ -391,20 +391,20 @@ struct pkvm_mem_transition {
 
 	struct {
 		enum pkvm_component_id	id;
-		/* Address in the initiator's address space */
+		/* Address in the source's address space */
 		u64			addr;
-	} initiator;
+	} source;
 
 	struct {
 		enum pkvm_component_id	id;
-		/* Address in the completer's address space */
+		/* Address in the target's address space */
 		u64			addr;
-	} completer;
+	} target;
 };
 
 struct pkvm_mem_share {
 	const struct pkvm_mem_transition	tx;
-	const enum kvm_pgtable_prot		completer_prot;
+	const enum kvm_pgtable_prot		target_prot;
 };
 
 struct check_walk_data {
@@ -469,7 +469,7 @@ static int __host_set_page_state_range(u64 addr, u64 size,
 static int host_request_owned_transition(const struct pkvm_mem_transition *tx)
 {
 	u64 size = tx->nr_pages * PAGE_SIZE;
-	u64 addr = tx->initiator.addr;
+	u64 addr = tx->source.addr;
 
 	return __host_check_page_state_range(addr, size, PKVM_PAGE_OWNED);
 }
@@ -477,7 +477,7 @@ static int host_request_owned_transition(const struct pkvm_mem_transition *tx)
 static int host_request_unshare(const struct pkvm_mem_transition *tx)
 {
 	u64 size = tx->nr_pages * PAGE_SIZE;
-	u64 addr = tx->initiator.addr;
+	u64 addr = tx->source.addr;
 
 	return __host_check_page_state_range(addr, size, PKVM_PAGE_SHARED_OWNED);
 }
@@ -485,7 +485,7 @@ static int host_request_unshare(const struct pkvm_mem_transition *tx)
 static int host_initiate_share(const struct pkvm_mem_transition *tx)
 {
 	u64 size = tx->nr_pages * PAGE_SIZE;
-	u64 addr = tx->initiator.addr;
+	u64 addr = tx->source.addr;
 
 	return __host_set_page_state_range(addr, size, PKVM_PAGE_SHARED_OWNED);
 }
@@ -493,7 +493,7 @@ static int host_initiate_share(const struct pkvm_mem_transition *tx)
 static int host_initiate_unshare(const struct pkvm_mem_transition *tx)
 {
 	u64 size = tx->nr_pages * PAGE_SIZE;
-	u64 addr = tx->initiator.addr;
+	u64 addr = tx->source.addr;
 
 	return __host_set_page_state_range(addr, size, PKVM_PAGE_OWNED);
 }
@@ -521,7 +521,7 @@ static int __hyp_check_page_state_range(u64 addr, u64 size,
 static bool __hyp_ack_skip_pgtable_check(const struct pkvm_mem_transition *tx)
 {
 	return !(IS_ENABLED(CONFIG_NVHE_EL2_DEBUG) ||
-		 tx->initiator.id != PKVM_ID_HOST);
+		 tx->source.id != PKVM_ID_HOST);
 }
 
 static int hyp_ack_share(const struct pkvm_mem_transition *tx,
@@ -535,7 +535,7 @@ static int hyp_ack_share(const struct pkvm_mem_transition *tx,
 	if (__hyp_ack_skip_pgtable_check(tx))
 		return 0;
 
-	return __hyp_check_page_state_range(tx->completer.addr, size, PKVM_NOPAGE);
+	return __hyp_check_page_state_range(tx->target.addr, size, PKVM_NOPAGE);
 }
 
 static int hyp_ack_unshare(const struct pkvm_mem_transition *tx)
@@ -545,14 +545,14 @@ static int hyp_ack_unshare(const struct pkvm_mem_transition *tx)
 	if (__hyp_ack_skip_pgtable_check(tx))
 		return 0;
 
-	return __hyp_check_page_state_range(tx->completer.addr, size,
+	return __hyp_check_page_state_range(tx->target.addr, size,
 					    PKVM_PAGE_SHARED_BORROWED);
 }
 
 static int hyp_complete_share(const struct pkvm_mem_transition *tx,
 			      enum kvm_pgtable_prot perms)
 {
-	void *start = (void *)tx->completer.addr;
+	void *start = (void *)tx->target.addr;
 	void *end = start + (tx->nr_pages * PAGE_SIZE);
 	enum kvm_pgtable_prot prot;
 
@@ -563,7 +563,7 @@ static int hyp_complete_share(const struct pkvm_mem_transition *tx,
 static int hyp_complete_unshare(const struct pkvm_mem_transition *tx)
 {
 	u64 size = tx->nr_pages * PAGE_SIZE;
-	int ret = kvm_pgtable_hyp_unmap(&pkvm_pgtable, tx->completer.addr, size);
+	int ret = kvm_pgtable_hyp_unmap(&pkvm_pgtable, tx->target.addr, size);
 
 	return (ret != size) ? -EFAULT : 0;
 }
@@ -573,7 +573,7 @@ static int check_share(struct pkvm_mem_share *share)
 	const struct pkvm_mem_transition *tx = &share->tx;
 	int ret;
 
-	switch (tx->initiator.id) {
+	switch (tx->source.id) {
 	case PKVM_ID_HOST:
 		ret = host_request_owned_transition(tx);
 		break;
@@ -584,9 +584,9 @@ static int check_share(struct pkvm_mem_share *share)
 	if (ret)
 		return ret;
 
-	switch (tx->completer.id) {
+	switch (tx->target.id) {
 	case PKVM_ID_HYP:
-		ret = hyp_ack_share(tx, share->completer_prot);
+		ret = hyp_ack_share(tx, share->target_prot);
 		break;
 	default:
 		ret = -EINVAL;
@@ -600,7 +600,7 @@ static int __do_share(struct pkvm_mem_share *share)
 	const struct pkvm_mem_transition *tx = &share->tx;
 	int ret;
 
-	switch (tx->initiator.id) {
+	switch (tx->source.id) {
 	case PKVM_ID_HOST:
 		ret = host_initiate_share(tx);
 		break;
@@ -611,9 +611,9 @@ static int __do_share(struct pkvm_mem_share *share)
 	if (ret)
 		return ret;
 
-	switch (tx->completer.id) {
+	switch (tx->target.id) {
 	case PKVM_ID_HYP:
-		ret = hyp_complete_share(tx, share->completer_prot);
+		ret = hyp_complete_share(tx, share->target_prot);
 		break;
 	default:
 		ret = -EINVAL;
@@ -628,8 +628,8 @@ static int __do_share(struct pkvm_mem_share *share)
  * The page owner grants access to another component with a given set
  * of permissions.
  *
- * Initiator: OWNED	=> SHARED_OWNED
- * Completer: NOPAGE	=> SHARED_BORROWED
+ * Source: OWNED	=> SHARED_OWNED
+ * Target: NOPAGE	=> SHARED_BORROWED
  */
 static int do_share(struct pkvm_mem_share *share)
 {
@@ -647,7 +647,7 @@ static int check_unshare(struct pkvm_mem_share *share)
 	const struct pkvm_mem_transition *tx = &share->tx;
 	int ret;
 
-	switch (tx->initiator.id) {
+	switch (tx->source.id) {
 	case PKVM_ID_HOST:
 		ret = host_request_unshare(tx);
 		break;
@@ -658,7 +658,7 @@ static int check_unshare(struct pkvm_mem_share *share)
 	if (ret)
 		return ret;
 
-	switch (tx->completer.id) {
+	switch (tx->target.id) {
 	case PKVM_ID_HYP:
 		ret = hyp_ack_unshare(tx);
 		break;
@@ -674,7 +674,7 @@ static int __do_unshare(struct pkvm_mem_share *share)
 	const struct pkvm_mem_transition *tx = &share->tx;
 	int ret;
 
-	switch (tx->initiator.id) {
+	switch (tx->source.id) {
 	case PKVM_ID_HOST:
 		ret = host_initiate_unshare(tx);
 		break;
@@ -685,7 +685,7 @@ static int __do_unshare(struct pkvm_mem_share *share)
 	if (ret)
 		return ret;
 
-	switch (tx->completer.id) {
+	switch (tx->target.id) {
 	case PKVM_ID_HYP:
 		ret = hyp_complete_unshare(tx);
 		break;
@@ -702,8 +702,8 @@ static int __do_unshare(struct pkvm_mem_share *share)
  * The page owner revokes access from another component for a range of
  * pages which were previously shared using do_share().
  *
- * Initiator: SHARED_OWNED	=> OWNED
- * Completer: SHARED_BORROWED	=> NOPAGE
+ * Source: SHARED_OWNED	=> OWNED
+ * Target: SHARED_BORROWED	=> NOPAGE
  */
 static int do_unshare(struct pkvm_mem_share *share)
 {
@@ -724,16 +724,16 @@ int __pkvm_host_share_hyp(u64 pfn)
 	struct pkvm_mem_share share = {
 		.tx	= {
 			.nr_pages	= 1,
-			.initiator	= {
+			.source	= {
 				.id	= PKVM_ID_HOST,
 				.addr	= host_addr,
 			},
-			.completer	= {
+			.target	= {
 				.id	= PKVM_ID_HYP,
 				.addr	= hyp_addr,
 			},
 		},
-		.completer_prot	= PAGE_HYP,
+		.target_prot	= PAGE_HYP,
 	};
 
 	host_lock_component();
@@ -755,16 +755,16 @@ int __pkvm_host_unshare_hyp(u64 pfn)
 	struct pkvm_mem_share share = {
 		.tx	= {
 			.nr_pages	= 1,
-			.initiator	= {
+			.source	= {
 				.id	= PKVM_ID_HOST,
 				.addr	= host_addr,
 			},
-			.completer	= {
+			.target	= {
 				.id	= PKVM_ID_HYP,
 				.addr	= hyp_addr,
 			},
 		},
-		.completer_prot	= PAGE_HYP,
+		.target_prot	= PAGE_HYP,
 	};
 
 	host_lock_component();
-- 
2.38.1.273.g43a17bfeac-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] KVM: arm64: Redefine pKVM memory transitions in terms of source/target

[Quentin Perret]

Hey Oliver,

On Friday 28 Oct 2022 at 08:34:48 (+0000), Oliver Upton wrote:
> Perhaps it is just me, but the 'initiator' and 'completer' terms are
> slightly confusing descriptors for the addresses involved in a memory
> transition. Apply a rename to instead describe memory transitions in
> terms of a source and target address.

Just to provide some rationale for the initiator/completer terminology,
the very first implementation we did of this used 'sender/recipient (or
something along those lines I think), and we ended up confusing
ourselves massively. The main issue is that memory doesn't necessarily
'flow' in the same direction as the transition. It's all fine for a
donation or a share, but reclaim and unshare become funny. 'The
recipient of an unshare' can be easily misunderstood, I think.

So yeah, we ended up with initiator/completer, which may not be the
prettiest terminology, but it was useful to disambiguate things at
least.

Thanks for the review!
Quentin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] KVM: arm64: Redefine pKVM memory transitions in terms of source/target

[Oliver Upton]

Quentin,

On Fri, Oct 28, 2022 at 09:57:04AM +0000, Quentin Perret wrote:
> Hey Oliver,
> 
> On Friday 28 Oct 2022 at 08:34:48 (+0000), Oliver Upton wrote:
> > Perhaps it is just me, but the 'initiator' and 'completer' terms are
> > slightly confusing descriptors for the addresses involved in a memory
> > transition. Apply a rename to instead describe memory transitions in
> > terms of a source and target address.
> 
> Just to provide some rationale for the initiator/completer terminology,
> the very first implementation we did of this used 'sender/recipient (or
> something along those lines I think), and we ended up confusing
> ourselves massively. The main issue is that memory doesn't necessarily
> 'flow' in the same direction as the transition. It's all fine for a
> donation or a share, but reclaim and unshare become funny. 'The
> recipient of an unshare' can be easily misunderstood, I think.
> 
> So yeah, we ended up with initiator/completer, which may not be the
> prettiest terminology, but it was useful to disambiguate things at
> least.

I see, thanks for the background :) If I've managed to re-ambiguate the
language here then LMK. Frankly, I'm more strongly motivated on the
first patch anyway.

--
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] KVM: arm64: Clean out the odd handling of completer_addr

[Will Deacon]

Hi Oliver,

On Fri, Oct 28, 2022 at 08:34:47AM +0000, Oliver Upton wrote:
> The layout of struct pkvm_mem_transition is a bit weird; the destination
> address for the transition is actually stashed in the initiator address
> context. Even weirder so, that address is thrown inside a union and
> return from helpers by use of an out pointer.
> 
> Rip out the whole mess and move the destination address into the
> destination context sub-struct. No functional change intended.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/kvm/hyp/nvhe/mem_protect.c | 70 ++++++++++-----------------
>  1 file changed, 25 insertions(+), 45 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> index 1e78acf9662e..3636a24e1b34 100644
> --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> @@ -393,17 +393,12 @@ struct pkvm_mem_transition {
>  		enum pkvm_component_id	id;
>  		/* Address in the initiator's address space */
>  		u64			addr;
> -
> -		union {
> -			struct {
> -				/* Address in the completer's address space */
> -				u64	completer_addr;
> -			} host;
> -		};
>  	} initiator;
>  
>  	struct {
>  		enum pkvm_component_id	id;
> +		/* Address in the completer's address space */
> +		u64			addr;
>  	} completer;
>  };

I'm reasonably sure we'll end up putting this back like we had it as we gain
support for guest-initiatied transitions, where we have to walk the guest
stage-2 page-table to figure out the physical address of the memory being
shared, which is the host (completer's) IPA thanks to the identity mapping
there.

So here's what I'll do: I'll post a v6 of the EL2 state series, and I'll
include this at the end (before the RFC patch) and let Marc decide whether
to go ahead with it. I do agree that it cleans things up for now but, as
above, I think that's likely to be a short-lived change.

Sound reasonable?

Cheers,

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] KVM: arm64: Redefine pKVM memory transitions in terms of source/target

[Will Deacon]

On Fri, Oct 28, 2022 at 10:23:36AM +0000, Oliver Upton wrote:
> On Fri, Oct 28, 2022 at 09:57:04AM +0000, Quentin Perret wrote:
> > On Friday 28 Oct 2022 at 08:34:48 (+0000), Oliver Upton wrote:
> > > Perhaps it is just me, but the 'initiator' and 'completer' terms are
> > > slightly confusing descriptors for the addresses involved in a memory
> > > transition. Apply a rename to instead describe memory transitions in
> > > terms of a source and target address.
> > 
> > Just to provide some rationale for the initiator/completer terminology,
> > the very first implementation we did of this used 'sender/recipient (or
> > something along those lines I think), and we ended up confusing
> > ourselves massively. The main issue is that memory doesn't necessarily
> > 'flow' in the same direction as the transition. It's all fine for a
> > donation or a share, but reclaim and unshare become funny. 'The
> > recipient of an unshare' can be easily misunderstood, I think.
> > 
> > So yeah, we ended up with initiator/completer, which may not be the
> > prettiest terminology, but it was useful to disambiguate things at
> > least.
> 
> I see, thanks for the background :) If I've managed to re-ambiguate the
> language here then LMK. Frankly, I'm more strongly motivated on the
> first patch anyway.

Having been previously tangled up in the confusion mentioned by Quentin, I'm
also strongly in favour of leaving the terminology as-is for the time being.
Once we have some of the more interesting memory transitions (i.e.
approaching the cross-product of host/guest/hyp/trustzone doing
share/unshare/donate) then I think we'll be in a much better position to
improve the naming, but whatever we change now is very unlikely to stick and
the patches as we have them now are at least consistent.

I replied separately on the first patch, as I don't really have a strong
opinion on that one.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel