[PATCH 0/5] KVM: arm64: Handle unaligned memslots in kvm_(test_)_age_gfn()

[PATCH 0/5] KVM: arm64: Handle unaligned memslots in kvm_(test_)_age_gfn()

[Oliver Upton]

There is no requirement for userspace to align KVM memslots to an
appropriate boundary for the backing memory source. For example,
userspace could choose to use 2M THP and place the memslot at a 4K
offset within the hugepage.

Under these conditions, KVM can only map at the 4K level, effectively
shattering the hugepage into a table of PTEs.

kvm_(test_)_age_gfn() operate on the assumption that the GFN range is of
a single page or hugepage size. Furthermore, this assumption bleeds into
the table walkers, which walk until reaching the first leaf PTE.

This completely falls apart in the aforementioned example where the
memslot is unalgined, as there could be many leaf PTEs that need to be
visited for a single hugepage. Furthermore, this configuration can lead
to the WARN in kvm_age_gfn() firing as well.

This series addresses the issue by rejigging the related page table
walkers to traverse a specified range of memory. I also roped in some
cleanups that I did along the way (i.e. patches 1 and 5), but can easily
respin w/o them if desired.

Tested on Ampere Altra w/ kvmtool and KVM selftests. I was going to
implement a reproducer for the issue in KVM selftests, but there is a
significant amount of idiot-proofing around the memslots helpers that
prevent an unaligned memslot. Might get back to that when I have more
patience for it.

Applies to 6.2-rc3.

Oliver Upton (5):
  KVM: arm64: Hoist S2 PTE definitions into kvm_pgtable.h
  KVM: arm64: Add a mask for all leaf PTE attributes
  KVM: arm64: Only return attributes from stage2_update_leaf_attrs()
  KVM: arm64: Correctly handle page aging notifiers for unaligned
    memlsot
  KVM: arm64: Consistently use KVM's types/helpers in kvm_age_gfn()

 arch/arm64/include/asm/kvm_pgtable.h | 66 +++++++++++++++++++----
 arch/arm64/kvm/hyp/pgtable.c         | 79 ++++++++--------------------
 arch/arm64/kvm/mmu.c                 | 18 +++----
 3 files changed, 88 insertions(+), 75 deletions(-)


base-commit: b7bfaa761d760e72a969d116517eaa12e404c262
-- 
2.39.0.314.g84b9a713c41-goog


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

[PATCH 1/5] KVM: arm64: Hoist S2 PTE definitions into kvm_pgtable.h

[Oliver Upton]

KVM's definitions for S2 PTEs are scattered between kvm_pgtable.h and
pgtable.c. Keep it all in one place by moving it all into the header.

No functional change intended.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/include/asm/kvm_pgtable.h | 34 ++++++++++++++++++++++++++++
 arch/arm64/kvm/hyp/pgtable.c         | 34 ----------------------------
 2 files changed, 34 insertions(+), 34 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
index 63f81b27a4e3..8071698cc680 100644
--- a/arch/arm64/include/asm/kvm_pgtable.h
+++ b/arch/arm64/include/asm/kvm_pgtable.h
@@ -39,6 +39,40 @@ typedef u64 kvm_pte_t;
 
 #define KVM_PTE_VALID			BIT(0)
 
+#define KVM_PTE_TYPE			BIT(1)
+#define KVM_PTE_TYPE_BLOCK		0
+#define KVM_PTE_TYPE_PAGE		1
+#define KVM_PTE_TYPE_TABLE		1
+
+#define KVM_PTE_LEAF_ATTR_LO		GENMASK(11, 2)
+
+#define KVM_PTE_LEAF_ATTR_LO_S1_ATTRIDX	GENMASK(4, 2)
+#define KVM_PTE_LEAF_ATTR_LO_S1_AP	GENMASK(7, 6)
+#define KVM_PTE_LEAF_ATTR_LO_S1_AP_RO	3
+#define KVM_PTE_LEAF_ATTR_LO_S1_AP_RW	1
+#define KVM_PTE_LEAF_ATTR_LO_S1_SH	GENMASK(9, 8)
+#define KVM_PTE_LEAF_ATTR_LO_S1_SH_IS	3
+#define KVM_PTE_LEAF_ATTR_LO_S1_AF	BIT(10)
+
+#define KVM_PTE_LEAF_ATTR_LO_S2_MEMATTR	GENMASK(5, 2)
+#define KVM_PTE_LEAF_ATTR_LO_S2_S2AP_R	BIT(6)
+#define KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W	BIT(7)
+#define KVM_PTE_LEAF_ATTR_LO_S2_SH	GENMASK(9, 8)
+#define KVM_PTE_LEAF_ATTR_LO_S2_SH_IS	3
+#define KVM_PTE_LEAF_ATTR_LO_S2_AF	BIT(10)
+
+#define KVM_PTE_LEAF_ATTR_HI		GENMASK(63, 51)
+
+#define KVM_PTE_LEAF_ATTR_HI_SW		GENMASK(58, 55)
+
+#define KVM_PTE_LEAF_ATTR_HI_S1_XN	BIT(54)
+
+#define KVM_PTE_LEAF_ATTR_HI_S2_XN	BIT(54)
+
+#define KVM_PTE_LEAF_ATTR_S2_PERMS	(KVM_PTE_LEAF_ATTR_LO_S2_S2AP_R | \
+					 KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W | \
+					 KVM_PTE_LEAF_ATTR_HI_S2_XN)
+
 #define KVM_PTE_ADDR_MASK		GENMASK(47, PAGE_SHIFT)
 #define KVM_PTE_ADDR_51_48		GENMASK(15, 12)
 
diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index b11cf2c618a6..3efe3aca3cb9 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -12,40 +12,6 @@
 #include <asm/stage2_pgtable.h>
 
 
-#define KVM_PTE_TYPE			BIT(1)
-#define KVM_PTE_TYPE_BLOCK		0
-#define KVM_PTE_TYPE_PAGE		1
-#define KVM_PTE_TYPE_TABLE		1
-
-#define KVM_PTE_LEAF_ATTR_LO		GENMASK(11, 2)
-
-#define KVM_PTE_LEAF_ATTR_LO_S1_ATTRIDX	GENMASK(4, 2)
-#define KVM_PTE_LEAF_ATTR_LO_S1_AP	GENMASK(7, 6)
-#define KVM_PTE_LEAF_ATTR_LO_S1_AP_RO	3
-#define KVM_PTE_LEAF_ATTR_LO_S1_AP_RW	1
-#define KVM_PTE_LEAF_ATTR_LO_S1_SH	GENMASK(9, 8)
-#define KVM_PTE_LEAF_ATTR_LO_S1_SH_IS	3
-#define KVM_PTE_LEAF_ATTR_LO_S1_AF	BIT(10)
-
-#define KVM_PTE_LEAF_ATTR_LO_S2_MEMATTR	GENMASK(5, 2)
-#define KVM_PTE_LEAF_ATTR_LO_S2_S2AP_R	BIT(6)
-#define KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W	BIT(7)
-#define KVM_PTE_LEAF_ATTR_LO_S2_SH	GENMASK(9, 8)
-#define KVM_PTE_LEAF_ATTR_LO_S2_SH_IS	3
-#define KVM_PTE_LEAF_ATTR_LO_S2_AF	BIT(10)
-
-#define KVM_PTE_LEAF_ATTR_HI		GENMASK(63, 51)
-
-#define KVM_PTE_LEAF_ATTR_HI_SW		GENMASK(58, 55)
-
-#define KVM_PTE_LEAF_ATTR_HI_S1_XN	BIT(54)
-
-#define KVM_PTE_LEAF_ATTR_HI_S2_XN	BIT(54)
-
-#define KVM_PTE_LEAF_ATTR_S2_PERMS	(KVM_PTE_LEAF_ATTR_LO_S2_S2AP_R | \
-					 KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W | \
-					 KVM_PTE_LEAF_ATTR_HI_S2_XN)
-
 #define KVM_INVALID_PTE_OWNER_MASK	GENMASK(9, 2)
 #define KVM_MAX_OWNER_ID		1
 
-- 
2.39.0.314.g84b9a713c41-goog


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

[PATCH 2/5] KVM: arm64: Add a mask for all leaf PTE attributes

[Oliver Upton]

No functional change intended.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/include/asm/kvm_pgtable.h | 3 +++
 arch/arm64/kvm/hyp/pgtable.c         | 7 +++----
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
index 8071698cc680..f8f6b4d2735a 100644
--- a/arch/arm64/include/asm/kvm_pgtable.h
+++ b/arch/arm64/include/asm/kvm_pgtable.h
@@ -73,6 +73,9 @@ typedef u64 kvm_pte_t;
 					 KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W | \
 					 KVM_PTE_LEAF_ATTR_HI_S2_XN)
 
+#define KVM_PTE_LEAF_ATTRS		(KVM_PTE_LEAF_ATTR_LO | \
+					 KVM_PTE_LEAF_ATTR_HI)
+
 #define KVM_PTE_ADDR_MASK		GENMASK(47, PAGE_SHIFT)
 #define KVM_PTE_ADDR_51_48		GENMASK(15, 12)
 
diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index 3efe3aca3cb9..26f61462527d 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -111,7 +111,7 @@ static kvm_pte_t kvm_init_valid_leaf_pte(u64 pa, kvm_pte_t attr, u32 level)
 	u64 type = (level == KVM_PGTABLE_MAX_LEVELS - 1) ? KVM_PTE_TYPE_PAGE :
 							   KVM_PTE_TYPE_BLOCK;
 
-	pte |= attr & (KVM_PTE_LEAF_ATTR_LO | KVM_PTE_LEAF_ATTR_HI);
+	pte |= attr & KVM_PTE_LEAF_ATTRS;
 	pte |= FIELD_PREP(KVM_PTE_TYPE, type);
 	pte |= KVM_PTE_VALID;
 
@@ -1027,10 +1027,9 @@ static int stage2_update_leaf_attrs(struct kvm_pgtable *pgt, u64 addr,
 				    u32 *level, enum kvm_pgtable_walk_flags flags)
 {
 	int ret;
-	kvm_pte_t attr_mask = KVM_PTE_LEAF_ATTR_LO | KVM_PTE_LEAF_ATTR_HI;
 	struct stage2_attr_data data = {
-		.attr_set	= attr_set & attr_mask,
-		.attr_clr	= attr_clr & attr_mask,
+		.attr_set	= attr_set & KVM_PTE_LEAF_ATTRS,
+		.attr_clr	= attr_clr & KVM_PTE_LEAF_ATTRS,
 	};
 	struct kvm_pgtable_walker walker = {
 		.cb		= stage2_attr_walker,
-- 
2.39.0.314.g84b9a713c41-goog


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

[PATCH 3/5] KVM: arm64: Only return attributes from stage2_update_leaf_attrs()

[Oliver Upton]

Returning a single PTE from stage2_update_leaf_attrs() doesn't make a
great deal of sense given that the function could be used to apply a
change to a range of PTEs. Instead, return a bitwise OR of attributes
from all the visited PTEs.

As the walker is no longer returning the full PTE, drop the check for a
valid PTE in kvm_age_gfn().

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/hyp/pgtable.c | 32 +++++++++++++++++---------------
 arch/arm64/kvm/mmu.c         |  2 +-
 2 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index 26f61462527d..a3d599e3af60 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -980,7 +980,7 @@ int kvm_pgtable_stage2_unmap(struct kvm_pgtable *pgt, u64 addr, u64 size)
 struct stage2_attr_data {
 	kvm_pte_t			attr_set;
 	kvm_pte_t			attr_clr;
-	kvm_pte_t			pte;
+	kvm_pte_t			attr_old;
 	u32				level;
 };
 
@@ -995,7 +995,7 @@ static int stage2_attr_walker(const struct kvm_pgtable_visit_ctx *ctx,
 		return 0;
 
 	data->level = ctx->level;
-	data->pte = pte;
+	data->attr_old |= pte & KVM_PTE_LEAF_ATTRS;
 	pte &= ~data->attr_clr;
 	pte |= data->attr_set;
 
@@ -1004,7 +1004,7 @@ static int stage2_attr_walker(const struct kvm_pgtable_visit_ctx *ctx,
 	 * but worst-case the access flag update gets lost and will be
 	 * set on the next access instead.
 	 */
-	if (data->pte != pte) {
+	if (ctx->old != pte) {
 		/*
 		 * Invalidate instruction cache before updating the guest
 		 * stage-2 PTE if we are going to add executable permission.
@@ -1023,7 +1023,7 @@ static int stage2_attr_walker(const struct kvm_pgtable_visit_ctx *ctx,
 
 static int stage2_update_leaf_attrs(struct kvm_pgtable *pgt, u64 addr,
 				    u64 size, kvm_pte_t attr_set,
-				    kvm_pte_t attr_clr, kvm_pte_t *orig_pte,
+				    kvm_pte_t attr_clr, kvm_pte_t *attr_old,
 				    u32 *level, enum kvm_pgtable_walk_flags flags)
 {
 	int ret;
@@ -1041,11 +1041,12 @@ static int stage2_update_leaf_attrs(struct kvm_pgtable *pgt, u64 addr,
 	if (ret)
 		return ret;
 
-	if (orig_pte)
-		*orig_pte = data.pte;
+	if (attr_old)
+		*attr_old = data.attr_old;
 
 	if (level)
 		*level = data.level;
+
 	return 0;
 }
 
@@ -1058,32 +1059,33 @@ int kvm_pgtable_stage2_wrprotect(struct kvm_pgtable *pgt, u64 addr, u64 size)
 
 kvm_pte_t kvm_pgtable_stage2_mkyoung(struct kvm_pgtable *pgt, u64 addr)
 {
-	kvm_pte_t pte = 0;
+	kvm_pte_t attr_old = 0;
+
 	stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
-				 &pte, NULL, 0);
+				 &attr_old, NULL, 0);
 	dsb(ishst);
-	return pte;
+	return attr_old;
 }
 
 kvm_pte_t kvm_pgtable_stage2_mkold(struct kvm_pgtable *pgt, u64 addr)
 {
-	kvm_pte_t pte = 0;
+	kvm_pte_t attr_old = 0;
 	stage2_update_leaf_attrs(pgt, addr, 1, 0, KVM_PTE_LEAF_ATTR_LO_S2_AF,
-				 &pte, NULL, 0);
+				 &attr_old, NULL, 0);
 	/*
 	 * "But where's the TLBI?!", you scream.
 	 * "Over in the core code", I sigh.
 	 *
 	 * See the '->clear_flush_young()' callback on the KVM mmu notifier.
 	 */
-	return pte;
+	return attr_old;
 }
 
 bool kvm_pgtable_stage2_is_young(struct kvm_pgtable *pgt, u64 addr)
 {
-	kvm_pte_t pte = 0;
-	stage2_update_leaf_attrs(pgt, addr, 1, 0, 0, &pte, NULL, 0);
-	return pte & KVM_PTE_LEAF_ATTR_LO_S2_AF;
+	kvm_pte_t attr_old = 0;
+	stage2_update_leaf_attrs(pgt, addr, 1, 0, 0, &attr_old, NULL, 0);
+	return attr_old & KVM_PTE_LEAF_ATTR_LO_S2_AF;
 }
 
 int kvm_pgtable_stage2_relax_perms(struct kvm_pgtable *pgt, u64 addr,
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 31d7fa4c7c14..0741f3a8ddca 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -1618,7 +1618,7 @@ bool kvm_age_gfn(struct kvm *kvm, struct kvm_gfn_range *range)
 	kpte = kvm_pgtable_stage2_mkold(kvm->arch.mmu.pgt,
 					range->start << PAGE_SHIFT);
 	pte = __pte(kpte);
-	return pte_valid(pte) && pte_young(pte);
+	return pte_young(pte);
 }
 
 bool kvm_test_age_gfn(struct kvm *kvm, struct kvm_gfn_range *range)
-- 
2.39.0.314.g84b9a713c41-goog


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

Re: [PATCH 3/5] KVM: arm64: Only return attributes from stage2_update_leaf_attrs()

[Marc Zyngier]

[+ Suzuki and Zenghui who are missing from the Cc list]

On Wed, 11 Jan 2023 00:02:58 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Returning a single PTE from stage2_update_leaf_attrs() doesn't make a
> great deal of sense given that the function could be used to apply a
> change to a range of PTEs. Instead, return a bitwise OR of attributes
> from all the visited PTEs.

I find this amalgamation of attributes quite confusing, and I have a
hard time attaching semantics to the resulting collection of bits.

It also means that you cannot reason about a particular attribute
being 0 if any of the neighbour PTEs has this bit set.

> 
> As the walker is no longer returning the full PTE, drop the check for a
> valid PTE in kvm_age_gfn().

But then what does it mean to check for a potentially invalid PTE?

The helpers explicitly say:

/*
 * The following only work if pte_present(). Undefined behaviour otherwise.
 */
#define pte_present(pte)	(!!(pte_val(pte) & (PTE_VALID | PTE_PROT_NONE)))
#define pte_young(pte)		(!!(pte_val(pte) & PTE_AF))

and you seem to be violating this requirement.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

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

Re: [PATCH 3/5] KVM: arm64: Only return attributes from stage2_update_leaf_attrs()

[Oliver Upton]

Hey Marc,

On Wed, Jan 11, 2023 at 08:52:25AM +0000, Marc Zyngier wrote:
> [+ Suzuki and Zenghui who are missing from the Cc list]

Doh! Just switched over to working out of a new git tree and didn't move
over my cc-cmd. Apologies to you two.

> On Wed, 11 Jan 2023 00:02:58 +0000,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> > 
> > Returning a single PTE from stage2_update_leaf_attrs() doesn't make a
> > great deal of sense given that the function could be used to apply a
> > change to a range of PTEs. Instead, return a bitwise OR of attributes
> > from all the visited PTEs.
> 
> I find this amalgamation of attributes quite confusing, and I have a
> hard time attaching semantics to the resulting collection of bits.
> 
> It also means that you cannot reason about a particular attribute
> being 0 if any of the neighbour PTEs has this bit set.

Very true. What I had really wanted to do was make a walker that allows
software to check the state of specific attribute bit(s) within a range
of memory instead of returning all of them. I decided against it because
it would put more churn on other callers or require a new walker
entirely.

Anyway, I can go about that change to make this a bit easier to reason
about. Thoughts?

> > 
> > As the walker is no longer returning the full PTE, drop the check for a
> > valid PTE in kvm_age_gfn().
> 
> But then what does it mean to check for a potentially invalid PTE?
> 
> The helpers explicitly say:
> 
> /*
>  * The following only work if pte_present(). Undefined behaviour otherwise.
>  */
> #define pte_present(pte)	(!!(pte_val(pte) & (PTE_VALID | PTE_PROT_NONE)))
> #define pte_young(pte)		(!!(pte_val(pte) & PTE_AF))
> 
> and you seem to be violating this requirement.

Indeed I have. It all still works because the call returns 0 if no valid
PTE was found in the walk but that's nowhere near as obvious as what we
had before.

--
Thanks,
Oliver

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

Re: [PATCH 3/5] KVM: arm64: Only return attributes from stage2_update_leaf_attrs()

[Oliver Upton]

On Wed, Jan 11, 2023 at 05:21:13PM +0000, Oliver Upton wrote:
> Hey Marc,
> 
> On Wed, Jan 11, 2023 at 08:52:25AM +0000, Marc Zyngier wrote:
> > [+ Suzuki and Zenghui who are missing from the Cc list]
> 
> Doh! Just switched over to working out of a new git tree and didn't move
> over my cc-cmd. Apologies to you two.
> 
> > On Wed, 11 Jan 2023 00:02:58 +0000,
> > Oliver Upton <oliver.upton@linux.dev> wrote:
> > > 
> > > Returning a single PTE from stage2_update_leaf_attrs() doesn't make a
> > > great deal of sense given that the function could be used to apply a
> > > change to a range of PTEs. Instead, return a bitwise OR of attributes
> > > from all the visited PTEs.
> > 
> > I find this amalgamation of attributes quite confusing, and I have a
> > hard time attaching semantics to the resulting collection of bits.
> > 
> > It also means that you cannot reason about a particular attribute
> > being 0 if any of the neighbour PTEs has this bit set.
> 
> Very true. What I had really wanted to do was make a walker that allows
> software to check the state of specific attribute bit(s) within a range
> of memory instead of returning all of them. I decided against it because
> it would put more churn on other callers or require a new walker
> entirely.
> 
> Anyway, I can go about that change to make this a bit easier to reason
> about. Thoughts?

LOL, I thought I hadn't replied which is why I didn't hear anything
back. Ball is in your court, Marc, any thoughts?

--
Thanks,
Oliver

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

Re: [PATCH 3/5] KVM: arm64: Only return attributes from stage2_update_leaf_attrs()

[Marc Zyngier]

On Thu, 02 Feb 2023 22:08:37 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Wed, Jan 11, 2023 at 05:21:13PM +0000, Oliver Upton wrote:
> > Hey Marc,
> > 
> > On Wed, Jan 11, 2023 at 08:52:25AM +0000, Marc Zyngier wrote:
> > > [+ Suzuki and Zenghui who are missing from the Cc list]
> > 
> > Doh! Just switched over to working out of a new git tree and didn't move
> > over my cc-cmd. Apologies to you two.
> > 
> > > On Wed, 11 Jan 2023 00:02:58 +0000,
> > > Oliver Upton <oliver.upton@linux.dev> wrote:
> > > > 
> > > > Returning a single PTE from stage2_update_leaf_attrs() doesn't make a
> > > > great deal of sense given that the function could be used to apply a
> > > > change to a range of PTEs. Instead, return a bitwise OR of attributes
> > > > from all the visited PTEs.
> > > 
> > > I find this amalgamation of attributes quite confusing, and I have a
> > > hard time attaching semantics to the resulting collection of bits.
> > > 
> > > It also means that you cannot reason about a particular attribute
> > > being 0 if any of the neighbour PTEs has this bit set.
> > 
> > Very true. What I had really wanted to do was make a walker that allows
> > software to check the state of specific attribute bit(s) within a range
> > of memory instead of returning all of them. I decided against it because
> > it would put more churn on other callers or require a new walker
> > entirely.
> > 
> > Anyway, I can go about that change to make this a bit easier to reason
> > about. Thoughts?
> 
> LOL, I thought I hadn't replied which is why I didn't hear anything
> back. Ball is in your court, Marc, any thoughts?

Huh, I clearly missed that email. Sorry about the delay.

Having a separate walker doesn't strike me as unreasonable. This
clearly is something different from the existing walkers, and it is
bound to be very little code anyway.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

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

[PATCH 4/5] KVM: arm64: Correctly handle page aging notifiers for unaligned memlsot

[Oliver Upton]

Userspace is allowed to select any PAGE_SIZE aligned hva to back guest
memory. This is even the case with hugepages, although it is a rather
suboptimal configuration as PTE level mappings are used at stage-2.

The page aging notifiers have an assumption that the spefified range
is exactly one page/block of memory, which in the aforementioned case is
not necessarily true. All together this leads to a rather obvious kernel
WARN when using an unaligned memslot:

However, the WARN is only part of the issue as the table walkers visit
at most a single leaf PTE. For hugepage-backed memory that is at a
suboptimal alignment in the memslot, page aging entirely misses accesses
to the hugepage at an offset greater than PAGE_SIZE.

Pass through the size of the notifier range to the table walkers and
traverse the full range of memory requested. While at it, drop the WARN
from before as it is clearly a valid condition.

Reported-by: Reiji Watanabe <reijiw@google.com>
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/include/asm/kvm_pgtable.h | 24 ++++++++++++++----------
 arch/arm64/kvm/hyp/pgtable.c         |  8 ++++----
 arch/arm64/kvm/mmu.c                 | 10 ++++++----
 3 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
index f8f6b4d2735a..81e04a24cc76 100644
--- a/arch/arm64/include/asm/kvm_pgtable.h
+++ b/arch/arm64/include/asm/kvm_pgtable.h
@@ -584,22 +584,24 @@ int kvm_pgtable_stage2_wrprotect(struct kvm_pgtable *pgt, u64 addr, u64 size);
 kvm_pte_t kvm_pgtable_stage2_mkyoung(struct kvm_pgtable *pgt, u64 addr);
 
 /**
- * kvm_pgtable_stage2_mkold() - Clear the access flag in a page-table entry.
+ * kvm_pgtable_stage2_mkold() - Clear the access flag in a range of page-table
+ *				entries.
  * @pgt:	Page-table structure initialised by kvm_pgtable_stage2_init*().
- * @addr:	Intermediate physical address to identify the page-table entry.
+ * @addr:	Intermediate physical address to identify the start of the
+ *		range.
  *
  * The offset of @addr within a page is ignored.
  *
- * If there is a valid, leaf page-table entry used to translate @addr, then
- * clear the access flag in that entry.
+ * If there is a valid, leaf page-table entry used to translate the specified
+ * range, then clear the access flag in that entry.
  *
  * Note that it is the caller's responsibility to invalidate the TLB after
  * calling this function to ensure that the updated permissions are visible
  * to the CPUs.
  *
- * Return: The old page-table entry prior to clearing the flag, 0 on failure.
+ * Return: Bitwise-OR of the prior to clearing the flag, 0 on failure.
  */
-kvm_pte_t kvm_pgtable_stage2_mkold(struct kvm_pgtable *pgt, u64 addr);
+kvm_pte_t kvm_pgtable_stage2_mkold(struct kvm_pgtable *pgt, u64 addr, u64 size);
 
 /**
  * kvm_pgtable_stage2_relax_perms() - Relax the permissions enforced by a
@@ -622,16 +624,18 @@ int kvm_pgtable_stage2_relax_perms(struct kvm_pgtable *pgt, u64 addr,
 				   enum kvm_pgtable_prot prot);
 
 /**
- * kvm_pgtable_stage2_is_young() - Test whether a page-table entry has the
- *				   access flag set.
+ * kvm_pgtable_stage2_is_young() - Test whether a range of page-table entries
+ *				   have the access flag set.
  * @pgt:	Page-table structure initialised by kvm_pgtable_stage2_init*().
  * @addr:	Intermediate physical address to identify the page-table entry.
+ * @size:	Size of the range to test.
  *
  * The offset of @addr within a page is ignored.
  *
- * Return: True if the page-table entry has the access flag set, false otherwise.
+ * Return: True if any of the page-table entries within the range have the
+ * access flag, false otherwise.
  */
-bool kvm_pgtable_stage2_is_young(struct kvm_pgtable *pgt, u64 addr);
+bool kvm_pgtable_stage2_is_young(struct kvm_pgtable *pgt, u64 addr, u64 size);
 
 /**
  * kvm_pgtable_stage2_flush_range() - Clean and invalidate data cache to Point
diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index a3d599e3af60..791f7e81671e 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -1067,10 +1067,10 @@ kvm_pte_t kvm_pgtable_stage2_mkyoung(struct kvm_pgtable *pgt, u64 addr)
 	return attr_old;
 }
 
-kvm_pte_t kvm_pgtable_stage2_mkold(struct kvm_pgtable *pgt, u64 addr)
+kvm_pte_t kvm_pgtable_stage2_mkold(struct kvm_pgtable *pgt, u64 addr, u64 size)
 {
 	kvm_pte_t attr_old = 0;
-	stage2_update_leaf_attrs(pgt, addr, 1, 0, KVM_PTE_LEAF_ATTR_LO_S2_AF,
+	stage2_update_leaf_attrs(pgt, addr, size, 0, KVM_PTE_LEAF_ATTR_LO_S2_AF,
 				 &attr_old, NULL, 0);
 	/*
 	 * "But where's the TLBI?!", you scream.
@@ -1081,10 +1081,10 @@ kvm_pte_t kvm_pgtable_stage2_mkold(struct kvm_pgtable *pgt, u64 addr)
 	return attr_old;
 }
 
-bool kvm_pgtable_stage2_is_young(struct kvm_pgtable *pgt, u64 addr)
+bool kvm_pgtable_stage2_is_young(struct kvm_pgtable *pgt, u64 addr, u64 size)
 {
 	kvm_pte_t attr_old = 0;
-	stage2_update_leaf_attrs(pgt, addr, 1, 0, 0, &attr_old, NULL, 0);
+	stage2_update_leaf_attrs(pgt, addr, size, 0, 0, &attr_old, NULL, 0);
 	return attr_old & KVM_PTE_LEAF_ATTR_LO_S2_AF;
 }
 
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 0741f3a8ddca..0b8e2a57f81a 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -1613,21 +1613,23 @@ bool kvm_age_gfn(struct kvm *kvm, struct kvm_gfn_range *range)
 	if (!kvm->arch.mmu.pgt)
 		return false;
 
-	WARN_ON(size != PAGE_SIZE && size != PMD_SIZE && size != PUD_SIZE);
-
 	kpte = kvm_pgtable_stage2_mkold(kvm->arch.mmu.pgt,
-					range->start << PAGE_SHIFT);
+					range->start << PAGE_SHIFT,
+					size);
 	pte = __pte(kpte);
 	return pte_young(pte);
 }
 
 bool kvm_test_age_gfn(struct kvm *kvm, struct kvm_gfn_range *range)
 {
+	u64 size = (range->end - range->start) << PAGE_SHIFT;
+
 	if (!kvm->arch.mmu.pgt)
 		return false;
 
 	return kvm_pgtable_stage2_is_young(kvm->arch.mmu.pgt,
-					   range->start << PAGE_SHIFT);
+					   range->start << PAGE_SHIFT,
+					   size);
 }
 
 phys_addr_t kvm_mmu_get_httbr(void)
-- 
2.39.0.314.g84b9a713c41-goog


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

Re: [PATCH 4/5] KVM: arm64: Correctly handle page aging notifiers for unaligned memlsot

[Marc Zyngier]

On Wed, 11 Jan 2023 00:02:59 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Userspace is allowed to select any PAGE_SIZE aligned hva to back guest
> memory. This is even the case with hugepages, although it is a rather
> suboptimal configuration as PTE level mappings are used at stage-2.
> 
> The page aging notifiers have an assumption that the spefified range
> is exactly one page/block of memory, which in the aforementioned case is
> not necessarily true. All together this leads to a rather obvious kernel
> WARN when using an unaligned memslot:
> 
> However, the WARN is only part of the issue as the table walkers visit
> at most a single leaf PTE. For hugepage-backed memory that is at a
> suboptimal alignment in the memslot, page aging entirely misses accesses
> to the hugepage at an offset greater than PAGE_SIZE.
> 
> Pass through the size of the notifier range to the table walkers and
> traverse the full range of memory requested. While at it, drop the WARN
> from before as it is clearly a valid condition.

Rather than changing the low-level walker, with the oddity that it
generates (patch #3), couldn't we instead just iterate over the range
and only process one entry at a time? All we need to know is the level
of the last processed entry to progress to the following block...

Thoughts?

	M.

-- 
Without deviation from the norm, progress is not possible.

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

[PATCH 5/5] KVM: arm64: Consistently use KVM's types/helpers in kvm_age_gfn()

[Oliver Upton]

There's no real need for indirection through the kernel's PTE types
here. Add a new helper to test if the access flag is set on a PTE and
use it to roll over kvm_age_gfn() to only using kvm_pte_t.

No functional change intended.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/include/asm/kvm_pgtable.h |  5 +++++
 arch/arm64/kvm/hyp/pgtable.c         |  2 +-
 arch/arm64/kvm/mmu.c                 | 12 +++++-------
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
index 81e04a24cc76..a81cd1ea64cb 100644
--- a/arch/arm64/include/asm/kvm_pgtable.h
+++ b/arch/arm64/include/asm/kvm_pgtable.h
@@ -86,6 +86,11 @@ static inline bool kvm_pte_valid(kvm_pte_t pte)
 	return pte & KVM_PTE_VALID;
 }
 
+static inline bool kvm_pte_young(kvm_pte_t pte)
+{
+	return pte & KVM_PTE_LEAF_ATTR_LO_S2_AF;
+}
+
 static inline u64 kvm_pte_to_phys(kvm_pte_t pte)
 {
 	u64 pa = pte & KVM_PTE_ADDR_MASK;
diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index 791f7e81671e..19f5094dfd93 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -1085,7 +1085,7 @@ bool kvm_pgtable_stage2_is_young(struct kvm_pgtable *pgt, u64 addr, u64 size)
 {
 	kvm_pte_t attr_old = 0;
 	stage2_update_leaf_attrs(pgt, addr, size, 0, 0, &attr_old, NULL, 0);
-	return attr_old & KVM_PTE_LEAF_ATTR_LO_S2_AF;
+	return kvm_pte_young(attr_old);
 }
 
 int kvm_pgtable_stage2_relax_perms(struct kvm_pgtable *pgt, u64 addr,
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 0b8e2a57f81a..3297475dcfcc 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -1607,17 +1607,15 @@ bool kvm_set_spte_gfn(struct kvm *kvm, struct kvm_gfn_range *range)
 bool kvm_age_gfn(struct kvm *kvm, struct kvm_gfn_range *range)
 {
 	u64 size = (range->end - range->start) << PAGE_SHIFT;
-	kvm_pte_t kpte;
-	pte_t pte;
+	kvm_pte_t pte;
 
 	if (!kvm->arch.mmu.pgt)
 		return false;
 
-	kpte = kvm_pgtable_stage2_mkold(kvm->arch.mmu.pgt,
-					range->start << PAGE_SHIFT,
-					size);
-	pte = __pte(kpte);
-	return pte_young(pte);
+	pte = kvm_pgtable_stage2_mkold(kvm->arch.mmu.pgt,
+				       range->start << PAGE_SHIFT,
+				       size);
+	return kvm_pte_young(pte);
 }
 
 bool kvm_test_age_gfn(struct kvm *kvm, struct kvm_gfn_range *range)
-- 
2.39.0.314.g84b9a713c41-goog


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