[PATCH 0/4] KVM: arm64: Parallel access faults

[PATCH 0/4] KVM: arm64: Parallel access faults

[Oliver Upton]

When I implemented the parallel faults series I was mostly focused on
improving the performance of 8.1+ implementations which bring us
FEAT_HAFDBS. In so doing, I failed to put access faults on the read side
of the MMU lock.

Anyhow, this small series adds support for handling access faults in
parallel, piling on top of the infrastructure from the first parallel
faults series. As most large systems I'm aware of are 8.1+ anyway, I
don't expect this series to provide significant uplift beyond some
oddball machines Marc has lying around. Don't get me wrong, I'd love to
have a D05 to play with too...

Patches 1-3 are the significant portion of the series, and patch 4 was
used to test on an Ampere Altra system by guarding VTCR_EL2.HA with the
kernel's Kconfig option for FEAT_HAFDBS. I've included it as I find it
helpful for testing on newer hardware. Having said that, I won't throw a
fit if it gets dropped either.

Applies to kvmarm/next due to the dependency on the larger parallel
faults series. Tested on Ampere Altra w/ VTCR_EL2.HA=0 as well as a
Raspberry Pi 4.

Oliver Upton (4):
  KVM: arm64: Use KVM's pte type/helpers in handle_access_fault()
  KVM: arm64: Don't serialize if the access flag isn't set
  KVM: arm64: Handle access faults behind the read lock
  KVM: arm64: Condition HW AF updates on config option

 arch/arm64/include/asm/kvm_pgtable.h |  5 +++++
 arch/arm64/kvm/hyp/pgtable.c         | 12 +++++++++---
 arch/arm64/kvm/mmu.c                 | 14 ++++++--------
 3 files changed, 20 insertions(+), 11 deletions(-)


base-commit: 456ce3545dd06700df7fe82173a06b369288bcef
-- 
2.38.1.584.g0f3c55d4c2-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 0/4] KVM: arm64: Parallel access faults

[Oliver Upton]

When I implemented the parallel faults series I was mostly focused on
improving the performance of 8.1+ implementations which bring us
FEAT_HAFDBS. In so doing, I failed to put access faults on the read side
of the MMU lock.

Anyhow, this small series adds support for handling access faults in
parallel, piling on top of the infrastructure from the first parallel
faults series. As most large systems I'm aware of are 8.1+ anyway, I
don't expect this series to provide significant uplift beyond some
oddball machines Marc has lying around. Don't get me wrong, I'd love to
have a D05 to play with too...

Patches 1-3 are the significant portion of the series, and patch 4 was
used to test on an Ampere Altra system by guarding VTCR_EL2.HA with the
kernel's Kconfig option for FEAT_HAFDBS. I've included it as I find it
helpful for testing on newer hardware. Having said that, I won't throw a
fit if it gets dropped either.

Applies to kvmarm/next due to the dependency on the larger parallel
faults series. Tested on Ampere Altra w/ VTCR_EL2.HA=0 as well as a
Raspberry Pi 4.

Oliver Upton (4):
  KVM: arm64: Use KVM's pte type/helpers in handle_access_fault()
  KVM: arm64: Don't serialize if the access flag isn't set
  KVM: arm64: Handle access faults behind the read lock
  KVM: arm64: Condition HW AF updates on config option

 arch/arm64/include/asm/kvm_pgtable.h |  5 +++++
 arch/arm64/kvm/hyp/pgtable.c         | 12 +++++++++---
 arch/arm64/kvm/mmu.c                 | 14 ++++++--------
 3 files changed, 20 insertions(+), 11 deletions(-)


base-commit: 456ce3545dd06700df7fe82173a06b369288bcef
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH 0/4] KVM: arm64: Parallel access faults

[Oliver Upton]

When I implemented the parallel faults series I was mostly focused on
improving the performance of 8.1+ implementations which bring us
FEAT_HAFDBS. In so doing, I failed to put access faults on the read side
of the MMU lock.

Anyhow, this small series adds support for handling access faults in
parallel, piling on top of the infrastructure from the first parallel
faults series. As most large systems I'm aware of are 8.1+ anyway, I
don't expect this series to provide significant uplift beyond some
oddball machines Marc has lying around. Don't get me wrong, I'd love to
have a D05 to play with too...

Patches 1-3 are the significant portion of the series, and patch 4 was
used to test on an Ampere Altra system by guarding VTCR_EL2.HA with the
kernel's Kconfig option for FEAT_HAFDBS. I've included it as I find it
helpful for testing on newer hardware. Having said that, I won't throw a
fit if it gets dropped either.

Applies to kvmarm/next due to the dependency on the larger parallel
faults series. Tested on Ampere Altra w/ VTCR_EL2.HA=0 as well as a
Raspberry Pi 4.

Oliver Upton (4):
  KVM: arm64: Use KVM's pte type/helpers in handle_access_fault()
  KVM: arm64: Don't serialize if the access flag isn't set
  KVM: arm64: Handle access faults behind the read lock
  KVM: arm64: Condition HW AF updates on config option

 arch/arm64/include/asm/kvm_pgtable.h |  5 +++++
 arch/arm64/kvm/hyp/pgtable.c         | 12 +++++++++---
 arch/arm64/kvm/mmu.c                 | 14 ++++++--------
 3 files changed, 20 insertions(+), 11 deletions(-)


base-commit: 456ce3545dd06700df7fe82173a06b369288bcef
-- 
2.38.1.584.g0f3c55d4c2-goog


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

[PATCH 1/4] KVM: arm64: Use KVM's pte type/helpers in handle_access_fault()

[Oliver Upton]

Consistently use KVM's own pte types and helpers in
handle_access_fault().

No functional change intended.

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

diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
index 63f81b27a4e3..192f33b88dc1 100644
--- a/arch/arm64/include/asm/kvm_pgtable.h
+++ b/arch/arm64/include/asm/kvm_pgtable.h
@@ -71,6 +71,11 @@ static inline kvm_pte_t kvm_phys_to_pte(u64 pa)
 	return pte;
 }
 
+static inline kvm_pfn_t kvm_pte_to_pfn(kvm_pte_t pte)
+{
+	return __phys_to_pfn(kvm_pte_to_phys(pte));
+}
+
 static inline u64 kvm_granule_shift(u32 level)
 {
 	/* Assumes KVM_PGTABLE_MAX_LEVELS is 4 */
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index a3c71b5172cd..886ad5ee767a 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -1399,20 +1399,18 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 /* Resolve the access fault by making the page young again. */
 static void handle_access_fault(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa)
 {
-	pte_t pte;
-	kvm_pte_t kpte;
+	kvm_pte_t pte;
 	struct kvm_s2_mmu *mmu;
 
 	trace_kvm_access_fault(fault_ipa);
 
 	write_lock(&vcpu->kvm->mmu_lock);
 	mmu = vcpu->arch.hw_mmu;
-	kpte = kvm_pgtable_stage2_mkyoung(mmu->pgt, fault_ipa);
+	pte = kvm_pgtable_stage2_mkyoung(mmu->pgt, fault_ipa);
 	write_unlock(&vcpu->kvm->mmu_lock);
 
-	pte = __pte(kpte);
-	if (pte_valid(pte))
-		kvm_set_pfn_accessed(pte_pfn(pte));
+	if (kvm_pte_valid(pte))
+		kvm_set_pfn_accessed(kvm_pte_to_pfn(pte));
 }
 
 /**
-- 
2.38.1.584.g0f3c55d4c2-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 1/4] KVM: arm64: Use KVM's pte type/helpers in handle_access_fault()

[Oliver Upton]

Consistently use KVM's own pte types and helpers in
handle_access_fault().

No functional change intended.

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

diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
index 63f81b27a4e3..192f33b88dc1 100644
--- a/arch/arm64/include/asm/kvm_pgtable.h
+++ b/arch/arm64/include/asm/kvm_pgtable.h
@@ -71,6 +71,11 @@ static inline kvm_pte_t kvm_phys_to_pte(u64 pa)
 	return pte;
 }
 
+static inline kvm_pfn_t kvm_pte_to_pfn(kvm_pte_t pte)
+{
+	return __phys_to_pfn(kvm_pte_to_phys(pte));
+}
+
 static inline u64 kvm_granule_shift(u32 level)
 {
 	/* Assumes KVM_PGTABLE_MAX_LEVELS is 4 */
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index a3c71b5172cd..886ad5ee767a 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -1399,20 +1399,18 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 /* Resolve the access fault by making the page young again. */
 static void handle_access_fault(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa)
 {
-	pte_t pte;
-	kvm_pte_t kpte;
+	kvm_pte_t pte;
 	struct kvm_s2_mmu *mmu;
 
 	trace_kvm_access_fault(fault_ipa);
 
 	write_lock(&vcpu->kvm->mmu_lock);
 	mmu = vcpu->arch.hw_mmu;
-	kpte = kvm_pgtable_stage2_mkyoung(mmu->pgt, fault_ipa);
+	pte = kvm_pgtable_stage2_mkyoung(mmu->pgt, fault_ipa);
 	write_unlock(&vcpu->kvm->mmu_lock);
 
-	pte = __pte(kpte);
-	if (pte_valid(pte))
-		kvm_set_pfn_accessed(pte_pfn(pte));
+	if (kvm_pte_valid(pte))
+		kvm_set_pfn_accessed(kvm_pte_to_pfn(pte));
 }
 
 /**
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH 1/4] KVM: arm64: Use KVM's pte type/helpers in handle_access_fault()

[Oliver Upton]

Consistently use KVM's own pte types and helpers in
handle_access_fault().

No functional change intended.

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

diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
index 63f81b27a4e3..192f33b88dc1 100644
--- a/arch/arm64/include/asm/kvm_pgtable.h
+++ b/arch/arm64/include/asm/kvm_pgtable.h
@@ -71,6 +71,11 @@ static inline kvm_pte_t kvm_phys_to_pte(u64 pa)
 	return pte;
 }
 
+static inline kvm_pfn_t kvm_pte_to_pfn(kvm_pte_t pte)
+{
+	return __phys_to_pfn(kvm_pte_to_phys(pte));
+}
+
 static inline u64 kvm_granule_shift(u32 level)
 {
 	/* Assumes KVM_PGTABLE_MAX_LEVELS is 4 */
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index a3c71b5172cd..886ad5ee767a 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -1399,20 +1399,18 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 /* Resolve the access fault by making the page young again. */
 static void handle_access_fault(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa)
 {
-	pte_t pte;
-	kvm_pte_t kpte;
+	kvm_pte_t pte;
 	struct kvm_s2_mmu *mmu;
 
 	trace_kvm_access_fault(fault_ipa);
 
 	write_lock(&vcpu->kvm->mmu_lock);
 	mmu = vcpu->arch.hw_mmu;
-	kpte = kvm_pgtable_stage2_mkyoung(mmu->pgt, fault_ipa);
+	pte = kvm_pgtable_stage2_mkyoung(mmu->pgt, fault_ipa);
 	write_unlock(&vcpu->kvm->mmu_lock);
 
-	pte = __pte(kpte);
-	if (pte_valid(pte))
-		kvm_set_pfn_accessed(pte_pfn(pte));
+	if (kvm_pte_valid(pte))
+		kvm_set_pfn_accessed(kvm_pte_to_pfn(pte));
 }
 
 /**
-- 
2.38.1.584.g0f3c55d4c2-goog


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

[PATCH 2/4] KVM: arm64: Don't serialize if the access flag isn't set

[Oliver Upton]

Of course, if the PTE wasn't changed then there are absolutely no
serialization requirements. Skip the DSB for an unsuccessful update to
the access flag.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/hyp/pgtable.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index b11cf2c618a6..9626f615d9b8 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -1094,9 +1094,13 @@ 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;
-	stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
-				 &pte, NULL, 0);
-	dsb(ishst);
+	int ret;
+
+	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
+				       &pte, NULL, 0);
+	if (!ret)
+		dsb(ishst);
+
 	return pte;
 }
 
-- 
2.38.1.584.g0f3c55d4c2-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 2/4] KVM: arm64: Don't serialize if the access flag isn't set

[Oliver Upton]

Of course, if the PTE wasn't changed then there are absolutely no
serialization requirements. Skip the DSB for an unsuccessful update to
the access flag.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/hyp/pgtable.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index b11cf2c618a6..9626f615d9b8 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -1094,9 +1094,13 @@ 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;
-	stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
-				 &pte, NULL, 0);
-	dsb(ishst);
+	int ret;
+
+	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
+				       &pte, NULL, 0);
+	if (!ret)
+		dsb(ishst);
+
 	return pte;
 }
 
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH 2/4] KVM: arm64: Don't serialize if the access flag isn't set

[Oliver Upton]

Of course, if the PTE wasn't changed then there are absolutely no
serialization requirements. Skip the DSB for an unsuccessful update to
the access flag.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/hyp/pgtable.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index b11cf2c618a6..9626f615d9b8 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -1094,9 +1094,13 @@ 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;
-	stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
-				 &pte, NULL, 0);
-	dsb(ishst);
+	int ret;
+
+	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
+				       &pte, NULL, 0);
+	if (!ret)
+		dsb(ishst);
+
 	return pte;
 }
 
-- 
2.38.1.584.g0f3c55d4c2-goog


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

[PATCH 3/4] KVM: arm64: Handle access faults behind the read lock

[Oliver Upton]

As the underlying software walkers are able to traverse and update
stage-2 in parallel there is no need to serialize access faults.

Only take the read lock when handling an access fault.

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

diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index 9626f615d9b8..1a3dd9774707 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -1097,7 +1097,7 @@ kvm_pte_t kvm_pgtable_stage2_mkyoung(struct kvm_pgtable *pgt, u64 addr)
 	int ret;
 
 	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
-				       &pte, NULL, 0);
+				       &pte, NULL, KVM_PGTABLE_WALK_SHARED);
 	if (!ret)
 		dsb(ishst);
 
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 886ad5ee767a..347985a56414 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -1404,10 +1404,10 @@ static void handle_access_fault(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa)
 
 	trace_kvm_access_fault(fault_ipa);
 
-	write_lock(&vcpu->kvm->mmu_lock);
+	read_lock(&vcpu->kvm->mmu_lock);
 	mmu = vcpu->arch.hw_mmu;
 	pte = kvm_pgtable_stage2_mkyoung(mmu->pgt, fault_ipa);
-	write_unlock(&vcpu->kvm->mmu_lock);
+	read_unlock(&vcpu->kvm->mmu_lock);
 
 	if (kvm_pte_valid(pte))
 		kvm_set_pfn_accessed(kvm_pte_to_pfn(pte));
-- 
2.38.1.584.g0f3c55d4c2-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 3/4] KVM: arm64: Handle access faults behind the read lock

[Oliver Upton]

As the underlying software walkers are able to traverse and update
stage-2 in parallel there is no need to serialize access faults.

Only take the read lock when handling an access fault.

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

diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index 9626f615d9b8..1a3dd9774707 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -1097,7 +1097,7 @@ kvm_pte_t kvm_pgtable_stage2_mkyoung(struct kvm_pgtable *pgt, u64 addr)
 	int ret;
 
 	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
-				       &pte, NULL, 0);
+				       &pte, NULL, KVM_PGTABLE_WALK_SHARED);
 	if (!ret)
 		dsb(ishst);
 
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 886ad5ee767a..347985a56414 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -1404,10 +1404,10 @@ static void handle_access_fault(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa)
 
 	trace_kvm_access_fault(fault_ipa);
 
-	write_lock(&vcpu->kvm->mmu_lock);
+	read_lock(&vcpu->kvm->mmu_lock);
 	mmu = vcpu->arch.hw_mmu;
 	pte = kvm_pgtable_stage2_mkyoung(mmu->pgt, fault_ipa);
-	write_unlock(&vcpu->kvm->mmu_lock);
+	read_unlock(&vcpu->kvm->mmu_lock);
 
 	if (kvm_pte_valid(pte))
 		kvm_set_pfn_accessed(kvm_pte_to_pfn(pte));
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH 3/4] KVM: arm64: Handle access faults behind the read lock

[Oliver Upton]

As the underlying software walkers are able to traverse and update
stage-2 in parallel there is no need to serialize access faults.

Only take the read lock when handling an access fault.

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

diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index 9626f615d9b8..1a3dd9774707 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -1097,7 +1097,7 @@ kvm_pte_t kvm_pgtable_stage2_mkyoung(struct kvm_pgtable *pgt, u64 addr)
 	int ret;
 
 	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
-				       &pte, NULL, 0);
+				       &pte, NULL, KVM_PGTABLE_WALK_SHARED);
 	if (!ret)
 		dsb(ishst);
 
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 886ad5ee767a..347985a56414 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -1404,10 +1404,10 @@ static void handle_access_fault(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa)
 
 	trace_kvm_access_fault(fault_ipa);
 
-	write_lock(&vcpu->kvm->mmu_lock);
+	read_lock(&vcpu->kvm->mmu_lock);
 	mmu = vcpu->arch.hw_mmu;
 	pte = kvm_pgtable_stage2_mkyoung(mmu->pgt, fault_ipa);
-	write_unlock(&vcpu->kvm->mmu_lock);
+	read_unlock(&vcpu->kvm->mmu_lock);
 
 	if (kvm_pte_valid(pte))
 		kvm_set_pfn_accessed(kvm_pte_to_pfn(pte));
-- 
2.38.1.584.g0f3c55d4c2-goog


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

[PATCH 4/4] KVM: arm64: Condition HW AF updates on config option

[Oliver Upton]

As it currently stands, KVM makes use of FEAT_HAFDBS unconditionally.
Use of the feature in the rest of the kernel is guarded by an associated
Kconfig option.

Align KVM with the rest of the kernel and only enable VTCR_HA when
ARM64_HW_AFDBM is enabled. This can be helpful for testing changes to
the stage-2 access fault path on Armv8.1+ implementations.

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

diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index 1a3dd9774707..9c651b6d4092 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -584,12 +584,14 @@ u64 kvm_get_vtcr(u64 mmfr0, u64 mmfr1, u32 phys_shift)
 		lvls = 2;
 	vtcr |= VTCR_EL2_LVLS_TO_SL0(lvls);
 
+#ifdef CONFIG_ARM64_HW_AFDBM
 	/*
 	 * Enable the Hardware Access Flag management, unconditionally
 	 * on all CPUs. The features is RES0 on CPUs without the support
 	 * and must be ignored by the CPUs.
 	 */
 	vtcr |= VTCR_EL2_HA;
+#endif /* CONFIG_ARM64_HW_AFDBM */
 
 	/* Set the vmid bits */
 	vtcr |= (get_vmid_bits(mmfr1) == 16) ?
-- 
2.38.1.584.g0f3c55d4c2-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 4/4] KVM: arm64: Condition HW AF updates on config option

[Oliver Upton]

As it currently stands, KVM makes use of FEAT_HAFDBS unconditionally.
Use of the feature in the rest of the kernel is guarded by an associated
Kconfig option.

Align KVM with the rest of the kernel and only enable VTCR_HA when
ARM64_HW_AFDBM is enabled. This can be helpful for testing changes to
the stage-2 access fault path on Armv8.1+ implementations.

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

diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index 1a3dd9774707..9c651b6d4092 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -584,12 +584,14 @@ u64 kvm_get_vtcr(u64 mmfr0, u64 mmfr1, u32 phys_shift)
 		lvls = 2;
 	vtcr |= VTCR_EL2_LVLS_TO_SL0(lvls);
 
+#ifdef CONFIG_ARM64_HW_AFDBM
 	/*
 	 * Enable the Hardware Access Flag management, unconditionally
 	 * on all CPUs. The features is RES0 on CPUs without the support
 	 * and must be ignored by the CPUs.
 	 */
 	vtcr |= VTCR_EL2_HA;
+#endif /* CONFIG_ARM64_HW_AFDBM */
 
 	/* Set the vmid bits */
 	vtcr |= (get_vmid_bits(mmfr1) == 16) ?
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH 4/4] KVM: arm64: Condition HW AF updates on config option

[Oliver Upton]

As it currently stands, KVM makes use of FEAT_HAFDBS unconditionally.
Use of the feature in the rest of the kernel is guarded by an associated
Kconfig option.

Align KVM with the rest of the kernel and only enable VTCR_HA when
ARM64_HW_AFDBM is enabled. This can be helpful for testing changes to
the stage-2 access fault path on Armv8.1+ implementations.

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

diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index 1a3dd9774707..9c651b6d4092 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -584,12 +584,14 @@ u64 kvm_get_vtcr(u64 mmfr0, u64 mmfr1, u32 phys_shift)
 		lvls = 2;
 	vtcr |= VTCR_EL2_LVLS_TO_SL0(lvls);
 
+#ifdef CONFIG_ARM64_HW_AFDBM
 	/*
 	 * Enable the Hardware Access Flag management, unconditionally
 	 * on all CPUs. The features is RES0 on CPUs without the support
 	 * and must be ignored by the CPUs.
 	 */
 	vtcr |= VTCR_EL2_HA;
+#endif /* CONFIG_ARM64_HW_AFDBM */
 
 	/* Set the vmid bits */
 	vtcr |= (get_vmid_bits(mmfr1) == 16) ?
-- 
2.38.1.584.g0f3c55d4c2-goog


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

Re: [PATCH 2/4] KVM: arm64: Don't serialize if the access flag isn't set

[Ricardo Koller]

On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote:
> Of course, if the PTE wasn't changed then there are absolutely no
> serialization requirements. Skip the DSB for an unsuccessful update to
> the access flag.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/kvm/hyp/pgtable.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> index b11cf2c618a6..9626f615d9b8 100644
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c
> @@ -1094,9 +1094,13 @@ 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;
> -	stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> -				 &pte, NULL, 0);
> -	dsb(ishst);
> +	int ret;
> +
> +	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> +				       &pte, NULL, 0);
> +	if (!ret)
> +		dsb(ishst);

At the moment, the only reason for stage2_update_leaf_attrs() to not
update the PTE is if it's not valid:

	if (!kvm_pte_valid(pte))
			return 0;

I guess you could check that as well:

+	if (!ret || kvm_pte_valid(pte))
+		dsb(ishst);

> +
>  	return pte;
>  }
>  
> -- 
> 2.38.1.584.g0f3c55d4c2-goog
> 
> 

Re: [PATCH 2/4] KVM: arm64: Don't serialize if the access flag isn't set

[Ricardo Koller]

On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote:
> Of course, if the PTE wasn't changed then there are absolutely no
> serialization requirements. Skip the DSB for an unsuccessful update to
> the access flag.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/kvm/hyp/pgtable.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> index b11cf2c618a6..9626f615d9b8 100644
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c
> @@ -1094,9 +1094,13 @@ 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;
> -	stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> -				 &pte, NULL, 0);
> -	dsb(ishst);
> +	int ret;
> +
> +	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> +				       &pte, NULL, 0);
> +	if (!ret)
> +		dsb(ishst);

At the moment, the only reason for stage2_update_leaf_attrs() to not
update the PTE is if it's not valid:

	if (!kvm_pte_valid(pte))
			return 0;

I guess you could check that as well:

+	if (!ret || kvm_pte_valid(pte))
+		dsb(ishst);

> +
>  	return pte;
>  }
>  
> -- 
> 2.38.1.584.g0f3c55d4c2-goog
> 
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/4] KVM: arm64: Don't serialize if the access flag isn't set

[Ricardo Koller]

On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote:
> Of course, if the PTE wasn't changed then there are absolutely no
> serialization requirements. Skip the DSB for an unsuccessful update to
> the access flag.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/kvm/hyp/pgtable.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> index b11cf2c618a6..9626f615d9b8 100644
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c
> @@ -1094,9 +1094,13 @@ 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;
> -	stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> -				 &pte, NULL, 0);
> -	dsb(ishst);
> +	int ret;
> +
> +	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> +				       &pte, NULL, 0);
> +	if (!ret)
> +		dsb(ishst);

At the moment, the only reason for stage2_update_leaf_attrs() to not
update the PTE is if it's not valid:

	if (!kvm_pte_valid(pte))
			return 0;

I guess you could check that as well:

+	if (!ret || kvm_pte_valid(pte))
+		dsb(ishst);

> +
>  	return pte;
>  }
>  
> -- 
> 2.38.1.584.g0f3c55d4c2-goog
> 
> 

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

Re: [PATCH 3/4] KVM: arm64: Handle access faults behind the read lock

[Ricardo Koller]

On Tue, Nov 29, 2022 at 07:19:45PM +0000, Oliver Upton wrote:
> As the underlying software walkers are able to traverse and update
> stage-2 in parallel there is no need to serialize access faults.
> 
> Only take the read lock when handling an access fault.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/kvm/hyp/pgtable.c | 2 +-
>  arch/arm64/kvm/mmu.c         | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> index 9626f615d9b8..1a3dd9774707 100644
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c
> @@ -1097,7 +1097,7 @@ kvm_pte_t kvm_pgtable_stage2_mkyoung(struct kvm_pgtable *pgt, u64 addr)
>  	int ret;
>  
>  	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> -				       &pte, NULL, 0);
> +				       &pte, NULL, KVM_PGTABLE_WALK_SHARED);
>  	if (!ret)
>  		dsb(ishst);
>  
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 886ad5ee767a..347985a56414 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -1404,10 +1404,10 @@ static void handle_access_fault(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa)
>  
>  	trace_kvm_access_fault(fault_ipa);
>  
> -	write_lock(&vcpu->kvm->mmu_lock);
> +	read_lock(&vcpu->kvm->mmu_lock);
>  	mmu = vcpu->arch.hw_mmu;
>  	pte = kvm_pgtable_stage2_mkyoung(mmu->pgt, fault_ipa);
> -	write_unlock(&vcpu->kvm->mmu_lock);
> +	read_unlock(&vcpu->kvm->mmu_lock);
>  
>  	if (kvm_pte_valid(pte))
>  		kvm_set_pfn_accessed(kvm_pte_to_pfn(pte));
> -- 
> 2.38.1.584.g0f3c55d4c2-goog
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Reviewed-by: Ricardo Koller <ricarkol@google.com>

Re: [PATCH 3/4] KVM: arm64: Handle access faults behind the read lock

[Ricardo Koller]

On Tue, Nov 29, 2022 at 07:19:45PM +0000, Oliver Upton wrote:
> As the underlying software walkers are able to traverse and update
> stage-2 in parallel there is no need to serialize access faults.
> 
> Only take the read lock when handling an access fault.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/kvm/hyp/pgtable.c | 2 +-
>  arch/arm64/kvm/mmu.c         | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> index 9626f615d9b8..1a3dd9774707 100644
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c
> @@ -1097,7 +1097,7 @@ kvm_pte_t kvm_pgtable_stage2_mkyoung(struct kvm_pgtable *pgt, u64 addr)
>  	int ret;
>  
>  	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> -				       &pte, NULL, 0);
> +				       &pte, NULL, KVM_PGTABLE_WALK_SHARED);
>  	if (!ret)
>  		dsb(ishst);
>  
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 886ad5ee767a..347985a56414 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -1404,10 +1404,10 @@ static void handle_access_fault(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa)
>  
>  	trace_kvm_access_fault(fault_ipa);
>  
> -	write_lock(&vcpu->kvm->mmu_lock);
> +	read_lock(&vcpu->kvm->mmu_lock);
>  	mmu = vcpu->arch.hw_mmu;
>  	pte = kvm_pgtable_stage2_mkyoung(mmu->pgt, fault_ipa);
> -	write_unlock(&vcpu->kvm->mmu_lock);
> +	read_unlock(&vcpu->kvm->mmu_lock);
>  
>  	if (kvm_pte_valid(pte))
>  		kvm_set_pfn_accessed(kvm_pte_to_pfn(pte));
> -- 
> 2.38.1.584.g0f3c55d4c2-goog
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Reviewed-by: Ricardo Koller <ricarkol@google.com>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 3/4] KVM: arm64: Handle access faults behind the read lock

[Ricardo Koller]

On Tue, Nov 29, 2022 at 07:19:45PM +0000, Oliver Upton wrote:
> As the underlying software walkers are able to traverse and update
> stage-2 in parallel there is no need to serialize access faults.
> 
> Only take the read lock when handling an access fault.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/kvm/hyp/pgtable.c | 2 +-
>  arch/arm64/kvm/mmu.c         | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> index 9626f615d9b8..1a3dd9774707 100644
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c
> @@ -1097,7 +1097,7 @@ kvm_pte_t kvm_pgtable_stage2_mkyoung(struct kvm_pgtable *pgt, u64 addr)
>  	int ret;
>  
>  	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> -				       &pte, NULL, 0);
> +				       &pte, NULL, KVM_PGTABLE_WALK_SHARED);
>  	if (!ret)
>  		dsb(ishst);
>  
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 886ad5ee767a..347985a56414 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -1404,10 +1404,10 @@ static void handle_access_fault(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa)
>  
>  	trace_kvm_access_fault(fault_ipa);
>  
> -	write_lock(&vcpu->kvm->mmu_lock);
> +	read_lock(&vcpu->kvm->mmu_lock);
>  	mmu = vcpu->arch.hw_mmu;
>  	pte = kvm_pgtable_stage2_mkyoung(mmu->pgt, fault_ipa);
> -	write_unlock(&vcpu->kvm->mmu_lock);
> +	read_unlock(&vcpu->kvm->mmu_lock);
>  
>  	if (kvm_pte_valid(pte))
>  		kvm_set_pfn_accessed(kvm_pte_to_pfn(pte));
> -- 
> 2.38.1.584.g0f3c55d4c2-goog
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Reviewed-by: Ricardo Koller <ricarkol@google.com>

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

Re: [PATCH 2/4] KVM: arm64: Don't serialize if the access flag isn't set

[Oliver Upton]

Hi Ricardo,

Thanks for having a look.

On Tue, Nov 29, 2022 at 12:52:12PM -0800, Ricardo Koller wrote:
> On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote:

[...]

> > +	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> > +				       &pte, NULL, 0);
> > +	if (!ret)
> > +		dsb(ishst);
> 
> At the moment, the only reason for stage2_update_leaf_attrs() to not
> update the PTE is if it's not valid:
> 
> 	if (!kvm_pte_valid(pte))
> 			return 0;
> 
> I guess you could check that as well:
> 
> +	if (!ret || kvm_pte_valid(pte))
> +		dsb(ishst);

Thanks for catching this.

Instead of pivoting on the returned PTE value, how about we return
-EAGAIN from the early return in stage2_attr_walker()? It would better
match the pattern used elsewhere in the pgtable code.

--
Thanks,
Oliver

Re: [PATCH 2/4] KVM: arm64: Don't serialize if the access flag isn't set

[Oliver Upton]

Hi Ricardo,

Thanks for having a look.

On Tue, Nov 29, 2022 at 12:52:12PM -0800, Ricardo Koller wrote:
> On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote:

[...]

> > +	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> > +				       &pte, NULL, 0);
> > +	if (!ret)
> > +		dsb(ishst);
> 
> At the moment, the only reason for stage2_update_leaf_attrs() to not
> update the PTE is if it's not valid:
> 
> 	if (!kvm_pte_valid(pte))
> 			return 0;
> 
> I guess you could check that as well:
> 
> +	if (!ret || kvm_pte_valid(pte))
> +		dsb(ishst);

Thanks for catching this.

Instead of pivoting on the returned PTE value, how about we return
-EAGAIN from the early return in stage2_attr_walker()? It would better
match the pattern used elsewhere in the pgtable code.

--
Thanks,
Oliver
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/4] KVM: arm64: Don't serialize if the access flag isn't set

[Oliver Upton]

Hi Ricardo,

Thanks for having a look.

On Tue, Nov 29, 2022 at 12:52:12PM -0800, Ricardo Koller wrote:
> On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote:

[...]

> > +	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> > +				       &pte, NULL, 0);
> > +	if (!ret)
> > +		dsb(ishst);
> 
> At the moment, the only reason for stage2_update_leaf_attrs() to not
> update the PTE is if it's not valid:
> 
> 	if (!kvm_pte_valid(pte))
> 			return 0;
> 
> I guess you could check that as well:
> 
> +	if (!ret || kvm_pte_valid(pte))
> +		dsb(ishst);

Thanks for catching this.

Instead of pivoting on the returned PTE value, how about we return
-EAGAIN from the early return in stage2_attr_walker()? It would better
match the pattern used elsewhere in the pgtable code.

--
Thanks,
Oliver

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

Re: [PATCH 2/4] KVM: arm64: Don't serialize if the access flag isn't set

[Ricardo Koller]

On Tue, Nov 29, 2022 at 09:15:21PM +0000, Oliver Upton wrote:
> Hi Ricardo,
> 
> Thanks for having a look.
> 
> On Tue, Nov 29, 2022 at 12:52:12PM -0800, Ricardo Koller wrote:
> > On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote:
> 
> [...]
> 
> > > +	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> > > +				       &pte, NULL, 0);
> > > +	if (!ret)
> > > +		dsb(ishst);
> > 
> > At the moment, the only reason for stage2_update_leaf_attrs() to not
> > update the PTE is if it's not valid:
> > 
> > 	if (!kvm_pte_valid(pte))
> > 			return 0;
> > 
> > I guess you could check that as well:
> > 
> > +	if (!ret || kvm_pte_valid(pte))
> > +		dsb(ishst);
> 
> Thanks for catching this.
> 
> Instead of pivoting on the returned PTE value, how about we return
> -EAGAIN from the early return in stage2_attr_walker()? It would better
> match the pattern used elsewhere in the pgtable code.

That works, although I would use another return code (e.g., EINVAL)? as
that's not exactly a "try again" type of error.

> 
> --
> Thanks,
> Oliver

Re: [PATCH 2/4] KVM: arm64: Don't serialize if the access flag isn't set

[Ricardo Koller]

On Tue, Nov 29, 2022 at 09:15:21PM +0000, Oliver Upton wrote:
> Hi Ricardo,
> 
> Thanks for having a look.
> 
> On Tue, Nov 29, 2022 at 12:52:12PM -0800, Ricardo Koller wrote:
> > On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote:
> 
> [...]
> 
> > > +	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> > > +				       &pte, NULL, 0);
> > > +	if (!ret)
> > > +		dsb(ishst);
> > 
> > At the moment, the only reason for stage2_update_leaf_attrs() to not
> > update the PTE is if it's not valid:
> > 
> > 	if (!kvm_pte_valid(pte))
> > 			return 0;
> > 
> > I guess you could check that as well:
> > 
> > +	if (!ret || kvm_pte_valid(pte))
> > +		dsb(ishst);
> 
> Thanks for catching this.
> 
> Instead of pivoting on the returned PTE value, how about we return
> -EAGAIN from the early return in stage2_attr_walker()? It would better
> match the pattern used elsewhere in the pgtable code.

That works, although I would use another return code (e.g., EINVAL)? as
that's not exactly a "try again" type of error.

> 
> --
> Thanks,
> Oliver
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/4] KVM: arm64: Don't serialize if the access flag isn't set

[Ricardo Koller]

On Tue, Nov 29, 2022 at 09:15:21PM +0000, Oliver Upton wrote:
> Hi Ricardo,
> 
> Thanks for having a look.
> 
> On Tue, Nov 29, 2022 at 12:52:12PM -0800, Ricardo Koller wrote:
> > On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote:
> 
> [...]
> 
> > > +	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> > > +				       &pte, NULL, 0);
> > > +	if (!ret)
> > > +		dsb(ishst);
> > 
> > At the moment, the only reason for stage2_update_leaf_attrs() to not
> > update the PTE is if it's not valid:
> > 
> > 	if (!kvm_pte_valid(pte))
> > 			return 0;
> > 
> > I guess you could check that as well:
> > 
> > +	if (!ret || kvm_pte_valid(pte))
> > +		dsb(ishst);
> 
> Thanks for catching this.
> 
> Instead of pivoting on the returned PTE value, how about we return
> -EAGAIN from the early return in stage2_attr_walker()? It would better
> match the pattern used elsewhere in the pgtable code.

That works, although I would use another return code (e.g., EINVAL)? as
that's not exactly a "try again" type of error.

> 
> --
> Thanks,
> Oliver

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

Re: [PATCH 2/4] KVM: arm64: Don't serialize if the access flag isn't set

[Marc Zyngier]

On Wed, 30 Nov 2022 01:23:20 +0000,
Ricardo Koller <ricarkol@google.com> wrote:
> 
> On Tue, Nov 29, 2022 at 09:15:21PM +0000, Oliver Upton wrote:
> > Hi Ricardo,
> > 
> > Thanks for having a look.
> > 
> > On Tue, Nov 29, 2022 at 12:52:12PM -0800, Ricardo Koller wrote:
> > > On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote:
> > 
> > [...]
> > 
> > > > +	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> > > > +				       &pte, NULL, 0);
> > > > +	if (!ret)
> > > > +		dsb(ishst);
> > > 
> > > At the moment, the only reason for stage2_update_leaf_attrs() to not
> > > update the PTE is if it's not valid:
> > > 
> > > 	if (!kvm_pte_valid(pte))
> > > 			return 0;
> > > 
> > > I guess you could check that as well:
> > > 
> > > +	if (!ret || kvm_pte_valid(pte))
> > > +		dsb(ishst);
> > 
> > Thanks for catching this.
> > 
> > Instead of pivoting on the returned PTE value, how about we return
> > -EAGAIN from the early return in stage2_attr_walker()? It would better
> > match the pattern used elsewhere in the pgtable code.
> 
> That works, although I would use another return code (e.g., EINVAL)? as
> that's not exactly a "try again" type of error.

EINVAL usually is an indication of something that went horribly wrong.

But is that really a failure mode? Here, failing to update the PTE
should not be considered a failure, but just a benign race: access
fault being taken on a CPU and the page being evicted on another (not
unlikely, as the page was marked old before).

And if I'm correct above, this is definitely a "try again" situation:
you probably won't take the same type of fault the second time though.

Thanks,

	M.

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

Re: [PATCH 2/4] KVM: arm64: Don't serialize if the access flag isn't set

[Marc Zyngier]

On Wed, 30 Nov 2022 01:23:20 +0000,
Ricardo Koller <ricarkol@google.com> wrote:
> 
> On Tue, Nov 29, 2022 at 09:15:21PM +0000, Oliver Upton wrote:
> > Hi Ricardo,
> > 
> > Thanks for having a look.
> > 
> > On Tue, Nov 29, 2022 at 12:52:12PM -0800, Ricardo Koller wrote:
> > > On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote:
> > 
> > [...]
> > 
> > > > +	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> > > > +				       &pte, NULL, 0);
> > > > +	if (!ret)
> > > > +		dsb(ishst);
> > > 
> > > At the moment, the only reason for stage2_update_leaf_attrs() to not
> > > update the PTE is if it's not valid:
> > > 
> > > 	if (!kvm_pte_valid(pte))
> > > 			return 0;
> > > 
> > > I guess you could check that as well:
> > > 
> > > +	if (!ret || kvm_pte_valid(pte))
> > > +		dsb(ishst);
> > 
> > Thanks for catching this.
> > 
> > Instead of pivoting on the returned PTE value, how about we return
> > -EAGAIN from the early return in stage2_attr_walker()? It would better
> > match the pattern used elsewhere in the pgtable code.
> 
> That works, although I would use another return code (e.g., EINVAL)? as
> that's not exactly a "try again" type of error.

EINVAL usually is an indication of something that went horribly wrong.

But is that really a failure mode? Here, failing to update the PTE
should not be considered a failure, but just a benign race: access
fault being taken on a CPU and the page being evicted on another (not
unlikely, as the page was marked old before).

And if I'm correct above, this is definitely a "try again" situation:
you probably won't take the same type of fault the second time though.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/4] KVM: arm64: Don't serialize if the access flag isn't set

[Marc Zyngier]

On Wed, 30 Nov 2022 01:23:20 +0000,
Ricardo Koller <ricarkol@google.com> wrote:
> 
> On Tue, Nov 29, 2022 at 09:15:21PM +0000, Oliver Upton wrote:
> > Hi Ricardo,
> > 
> > Thanks for having a look.
> > 
> > On Tue, Nov 29, 2022 at 12:52:12PM -0800, Ricardo Koller wrote:
> > > On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote:
> > 
> > [...]
> > 
> > > > +	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> > > > +				       &pte, NULL, 0);
> > > > +	if (!ret)
> > > > +		dsb(ishst);
> > > 
> > > At the moment, the only reason for stage2_update_leaf_attrs() to not
> > > update the PTE is if it's not valid:
> > > 
> > > 	if (!kvm_pte_valid(pte))
> > > 			return 0;
> > > 
> > > I guess you could check that as well:
> > > 
> > > +	if (!ret || kvm_pte_valid(pte))
> > > +		dsb(ishst);
> > 
> > Thanks for catching this.
> > 
> > Instead of pivoting on the returned PTE value, how about we return
> > -EAGAIN from the early return in stage2_attr_walker()? It would better
> > match the pattern used elsewhere in the pgtable code.
> 
> That works, although I would use another return code (e.g., EINVAL)? as
> that's not exactly a "try again" type of error.

EINVAL usually is an indication of something that went horribly wrong.

But is that really a failure mode? Here, failing to update the PTE
should not be considered a failure, but just a benign race: access
fault being taken on a CPU and the page being evicted on another (not
unlikely, as the page was marked old before).

And if I'm correct above, this is definitely a "try again" situation:
you probably won't take the same type of fault the second time though.

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 0/4] KVM: arm64: Parallel access faults

[Marc Zyngier]

On Tue, 29 Nov 2022 19:19:42 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> When I implemented the parallel faults series I was mostly focused on
> improving the performance of 8.1+ implementations which bring us
> FEAT_HAFDBS. In so doing, I failed to put access faults on the read side
> of the MMU lock.
> 
> Anyhow, this small series adds support for handling access faults in
> parallel, piling on top of the infrastructure from the first parallel
> faults series. As most large systems I'm aware of are 8.1+ anyway, I
> don't expect this series to provide significant uplift beyond some
> oddball machines Marc has lying around. Don't get me wrong, I'd love to
> have a D05 to play with too...

Hey, that puts the whole fruity range of machines in the oddball
department too, as they don't implement any of HAFDBS!

The feature being optional, I wouldn't be surprised if others would
either not implement it (or disable it to hide that it is b0rk3n...).

	M.

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

Re: [PATCH 0/4] KVM: arm64: Parallel access faults

[Marc Zyngier]

On Tue, 29 Nov 2022 19:19:42 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> When I implemented the parallel faults series I was mostly focused on
> improving the performance of 8.1+ implementations which bring us
> FEAT_HAFDBS. In so doing, I failed to put access faults on the read side
> of the MMU lock.
> 
> Anyhow, this small series adds support for handling access faults in
> parallel, piling on top of the infrastructure from the first parallel
> faults series. As most large systems I'm aware of are 8.1+ anyway, I
> don't expect this series to provide significant uplift beyond some
> oddball machines Marc has lying around. Don't get me wrong, I'd love to
> have a D05 to play with too...

Hey, that puts the whole fruity range of machines in the oddball
department too, as they don't implement any of HAFDBS!

The feature being optional, I wouldn't be surprised if others would
either not implement it (or disable it to hide that it is b0rk3n...).

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 0/4] KVM: arm64: Parallel access faults

[Marc Zyngier]

On Tue, 29 Nov 2022 19:19:42 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> When I implemented the parallel faults series I was mostly focused on
> improving the performance of 8.1+ implementations which bring us
> FEAT_HAFDBS. In so doing, I failed to put access faults on the read side
> of the MMU lock.
> 
> Anyhow, this small series adds support for handling access faults in
> parallel, piling on top of the infrastructure from the first parallel
> faults series. As most large systems I'm aware of are 8.1+ anyway, I
> don't expect this series to provide significant uplift beyond some
> oddball machines Marc has lying around. Don't get me wrong, I'd love to
> have a D05 to play with too...

Hey, that puts the whole fruity range of machines in the oddball
department too, as they don't implement any of HAFDBS!

The feature being optional, I wouldn't be surprised if others would
either not implement it (or disable it to hide that it is b0rk3n...).

	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 2/4] KVM: arm64: Don't serialize if the access flag isn't set

[Ricardo Koller]

On Wed, Nov 30, 2022 at 08:21:17AM +0000, Marc Zyngier wrote:
> On Wed, 30 Nov 2022 01:23:20 +0000,
> Ricardo Koller <ricarkol@google.com> wrote:
> > 
> > On Tue, Nov 29, 2022 at 09:15:21PM +0000, Oliver Upton wrote:
> > > Hi Ricardo,
> > > 
> > > Thanks for having a look.
> > > 
> > > On Tue, Nov 29, 2022 at 12:52:12PM -0800, Ricardo Koller wrote:
> > > > On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote:
> > > 
> > > [...]
> > > 
> > > > > +	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> > > > > +				       &pte, NULL, 0);
> > > > > +	if (!ret)
> > > > > +		dsb(ishst);
> > > > 
> > > > At the moment, the only reason for stage2_update_leaf_attrs() to not
> > > > update the PTE is if it's not valid:
> > > > 
> > > > 	if (!kvm_pte_valid(pte))
> > > > 			return 0;
> > > > 
> > > > I guess you could check that as well:
> > > > 
> > > > +	if (!ret || kvm_pte_valid(pte))
> > > > +		dsb(ishst);
> > > 
> > > Thanks for catching this.
> > > 
> > > Instead of pivoting on the returned PTE value, how about we return
> > > -EAGAIN from the early return in stage2_attr_walker()? It would better
> > > match the pattern used elsewhere in the pgtable code.
> > 
> > That works, although I would use another return code (e.g., EINVAL)? as
> > that's not exactly a "try again" type of error.
> 
> EINVAL usually is an indication of something that went horribly wrong.
> 
> But is that really a failure mode? Here, failing to update the PTE
> should not be considered a failure, but just a benign race: access
> fault being taken on a CPU and the page being evicted on another (not
> unlikely, as the page was marked old before).

I see, I agree, what you describe not look like a failure.

> 
> And if I'm correct above, this is definitely a "try again" situation:
> you probably won't take the same type of fault the second time though.
> 
> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/4] KVM: arm64: Don't serialize if the access flag isn't set

[Ricardo Koller]

On Wed, Nov 30, 2022 at 08:21:17AM +0000, Marc Zyngier wrote:
> On Wed, 30 Nov 2022 01:23:20 +0000,
> Ricardo Koller <ricarkol@google.com> wrote:
> > 
> > On Tue, Nov 29, 2022 at 09:15:21PM +0000, Oliver Upton wrote:
> > > Hi Ricardo,
> > > 
> > > Thanks for having a look.
> > > 
> > > On Tue, Nov 29, 2022 at 12:52:12PM -0800, Ricardo Koller wrote:
> > > > On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote:
> > > 
> > > [...]
> > > 
> > > > > +	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> > > > > +				       &pte, NULL, 0);
> > > > > +	if (!ret)
> > > > > +		dsb(ishst);
> > > > 
> > > > At the moment, the only reason for stage2_update_leaf_attrs() to not
> > > > update the PTE is if it's not valid:
> > > > 
> > > > 	if (!kvm_pte_valid(pte))
> > > > 			return 0;
> > > > 
> > > > I guess you could check that as well:
> > > > 
> > > > +	if (!ret || kvm_pte_valid(pte))
> > > > +		dsb(ishst);
> > > 
> > > Thanks for catching this.
> > > 
> > > Instead of pivoting on the returned PTE value, how about we return
> > > -EAGAIN from the early return in stage2_attr_walker()? It would better
> > > match the pattern used elsewhere in the pgtable code.
> > 
> > That works, although I would use another return code (e.g., EINVAL)? as
> > that's not exactly a "try again" type of error.
> 
> EINVAL usually is an indication of something that went horribly wrong.
> 
> But is that really a failure mode? Here, failing to update the PTE
> should not be considered a failure, but just a benign race: access
> fault being taken on a CPU and the page being evicted on another (not
> unlikely, as the page was marked old before).

I see, I agree, what you describe not look like a failure.

> 
> And if I'm correct above, this is definitely a "try again" situation:
> you probably won't take the same type of fault the second time though.
> 
> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.
> 

Re: [PATCH 2/4] KVM: arm64: Don't serialize if the access flag isn't set

[Ricardo Koller]

On Wed, Nov 30, 2022 at 08:21:17AM +0000, Marc Zyngier wrote:
> On Wed, 30 Nov 2022 01:23:20 +0000,
> Ricardo Koller <ricarkol@google.com> wrote:
> > 
> > On Tue, Nov 29, 2022 at 09:15:21PM +0000, Oliver Upton wrote:
> > > Hi Ricardo,
> > > 
> > > Thanks for having a look.
> > > 
> > > On Tue, Nov 29, 2022 at 12:52:12PM -0800, Ricardo Koller wrote:
> > > > On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote:
> > > 
> > > [...]
> > > 
> > > > > +	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> > > > > +				       &pte, NULL, 0);
> > > > > +	if (!ret)
> > > > > +		dsb(ishst);
> > > > 
> > > > At the moment, the only reason for stage2_update_leaf_attrs() to not
> > > > update the PTE is if it's not valid:
> > > > 
> > > > 	if (!kvm_pte_valid(pte))
> > > > 			return 0;
> > > > 
> > > > I guess you could check that as well:
> > > > 
> > > > +	if (!ret || kvm_pte_valid(pte))
> > > > +		dsb(ishst);
> > > 
> > > Thanks for catching this.
> > > 
> > > Instead of pivoting on the returned PTE value, how about we return
> > > -EAGAIN from the early return in stage2_attr_walker()? It would better
> > > match the pattern used elsewhere in the pgtable code.
> > 
> > That works, although I would use another return code (e.g., EINVAL)? as
> > that's not exactly a "try again" type of error.
> 
> EINVAL usually is an indication of something that went horribly wrong.
> 
> But is that really a failure mode? Here, failing to update the PTE
> should not be considered a failure, but just a benign race: access
> fault being taken on a CPU and the page being evicted on another (not
> unlikely, as the page was marked old before).

I see, I agree, what you describe not look like a failure.

> 
> And if I'm correct above, this is definitely a "try again" situation:
> you probably won't take the same type of fault the second time though.
> 
> 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 2/4] KVM: arm64: Don't serialize if the access flag isn't set

[Oliver Upton]

On Tue, Nov 29, 2022 at 09:15:21PM +0000, Oliver Upton wrote:
> Hi Ricardo,
> 
> Thanks for having a look.
> 
> On Tue, Nov 29, 2022 at 12:52:12PM -0800, Ricardo Koller wrote:
> > On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote:
> 
> [...]
> 
> > > +	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> > > +				       &pte, NULL, 0);
> > > +	if (!ret)
> > > +		dsb(ishst);
> > 
> > At the moment, the only reason for stage2_update_leaf_attrs() to not
> > update the PTE is if it's not valid:
> > 
> > 	if (!kvm_pte_valid(pte))
> > 			return 0;
> > 
> > I guess you could check that as well:
> > 
> > +	if (!ret || kvm_pte_valid(pte))
> > +		dsb(ishst);
> 
> Thanks for catching this.
> 
> Instead of pivoting on the returned PTE value, how about we return
> -EAGAIN from the early return in stage2_attr_walker()? It would better
> match the pattern used elsewhere in the pgtable code.

Bugh...

Returning EAGAIN has some unfortunate consequences that I've missed
until now...

The stage2 attr walker is used to handle faults as well as range-based
operations. In the former case, EAGAIN is sane as we retry execution but
the latter is not. I stupidly got hung up on write protection not
working as intended for some time.

I think that callers into the page table walker should indicate whether
or not the walk is to address a fault. If it is not,
__kvm_pgtable_visit() and __kvm_pgtable_walk() should chug along instead
of bailing for EAGAIN.

Let me mess around with this and figure out what is least ugly.

--
Thanks,
Oliver
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/4] KVM: arm64: Don't serialize if the access flag isn't set

[Oliver Upton]

On Tue, Nov 29, 2022 at 09:15:21PM +0000, Oliver Upton wrote:
> Hi Ricardo,
> 
> Thanks for having a look.
> 
> On Tue, Nov 29, 2022 at 12:52:12PM -0800, Ricardo Koller wrote:
> > On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote:
> 
> [...]
> 
> > > +	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> > > +				       &pte, NULL, 0);
> > > +	if (!ret)
> > > +		dsb(ishst);
> > 
> > At the moment, the only reason for stage2_update_leaf_attrs() to not
> > update the PTE is if it's not valid:
> > 
> > 	if (!kvm_pte_valid(pte))
> > 			return 0;
> > 
> > I guess you could check that as well:
> > 
> > +	if (!ret || kvm_pte_valid(pte))
> > +		dsb(ishst);
> 
> Thanks for catching this.
> 
> Instead of pivoting on the returned PTE value, how about we return
> -EAGAIN from the early return in stage2_attr_walker()? It would better
> match the pattern used elsewhere in the pgtable code.

Bugh...

Returning EAGAIN has some unfortunate consequences that I've missed
until now...

The stage2 attr walker is used to handle faults as well as range-based
operations. In the former case, EAGAIN is sane as we retry execution but
the latter is not. I stupidly got hung up on write protection not
working as intended for some time.

I think that callers into the page table walker should indicate whether
or not the walk is to address a fault. If it is not,
__kvm_pgtable_visit() and __kvm_pgtable_walk() should chug along instead
of bailing for EAGAIN.

Let me mess around with this and figure out what is least ugly.

--
Thanks,
Oliver

Re: [PATCH 2/4] KVM: arm64: Don't serialize if the access flag isn't set

[Oliver Upton]

On Tue, Nov 29, 2022 at 09:15:21PM +0000, Oliver Upton wrote:
> Hi Ricardo,
> 
> Thanks for having a look.
> 
> On Tue, Nov 29, 2022 at 12:52:12PM -0800, Ricardo Koller wrote:
> > On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote:
> 
> [...]
> 
> > > +	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> > > +				       &pte, NULL, 0);
> > > +	if (!ret)
> > > +		dsb(ishst);
> > 
> > At the moment, the only reason for stage2_update_leaf_attrs() to not
> > update the PTE is if it's not valid:
> > 
> > 	if (!kvm_pte_valid(pte))
> > 			return 0;
> > 
> > I guess you could check that as well:
> > 
> > +	if (!ret || kvm_pte_valid(pte))
> > +		dsb(ishst);
> 
> Thanks for catching this.
> 
> Instead of pivoting on the returned PTE value, how about we return
> -EAGAIN from the early return in stage2_attr_walker()? It would better
> match the pattern used elsewhere in the pgtable code.

Bugh...

Returning EAGAIN has some unfortunate consequences that I've missed
until now...

The stage2 attr walker is used to handle faults as well as range-based
operations. In the former case, EAGAIN is sane as we retry execution but
the latter is not. I stupidly got hung up on write protection not
working as intended for some time.

I think that callers into the page table walker should indicate whether
or not the walk is to address a fault. If it is not,
__kvm_pgtable_visit() and __kvm_pgtable_walk() should chug along instead
of bailing for EAGAIN.

Let me mess around with this and figure out what is least ugly.

--
Thanks,
Oliver

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