KVM: arm64: VNCR TLB invalidation fixes

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] S1E2 TLBI instructions fail to invalidate the VNCR pseudo-TLB when TGE=0 or E2H=0.
- [High] Hardcoded TR_EL20 in kvm_translate_vncr misinterprets TCR_EL2 for non-VHE guests. --

--- Patch [1]: [PATCH v2 1/8] KVM: arm64: Remove VM-wide VNCR mapping counter --- commit 9a09630623c9341a08c8ecea540015307bde098c
Author: Marc Zyngier <[email protected]>
Subject: KVM: arm64: Remove VM-wide VNCR mapping counter

The global VNCR mapping counter is used to decide whether an L1 provided VNCR page is mapped in L0 on any CPU at the point of dealing with a TLB invalidation. It is incremented when a mapping is made in the fixmap, and decremented when unmapped.

arch/arm64/kvm/hyp/vhe/switch.c static bool kvm_hyp_handle_tlbi_el2(struct kvm_vcpu *vcpu, u64 *exit_code) · line 430

Quoted 11 lines

arch/arm64/kvm/hyp/vhe/switch.c +1 -2

index bbe9cebd3d9d5..c09b1d411c584 100644
@@ -427,8 +427,7 @@ static bool kvm_hyp_handle_tlbi_el2(struct kvm_vcpu *vcpu, u64 *exit_code)
 	 * If we have to check for any VNCR mapping being invalidated,
 	 * go back to the slow path for further processing.
 	 */
-	if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu) &&
-	    atomic_read(&vcpu->kvm->arch.vncr_map_count))
+	if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu))

[Severity: High]
This is a pre-existing issue, but does this condition incorrectly limit the slow-path fallback to only E2H=1 and TGE=1 contexts?

If a non-VHE guest (E2H=0) or a VHE guest running a VM (TGE=0) executes an S1E2 TLBI instruction (e.g. TLBI VAE2), the fast-path handler kvm_hyp_handle_tlbi_el2() performs the hardware TLBI but will evaluate the above condition as false.

The global VNCR mapping counter is used to decide whether an L1 provided VNCR page is mapped in L0 on any CPU at the point of dealing with a TLB invalidation. It is incremented when a mapping is made in the fixmap, and decremented when unmapped.

As it turns out, this tracking has several flaws:

- we are trying to invalidate TLBs, and the mapping is only an opportunistic consequence of the TLB. Checking this counter to decide whether a TLB needs to be invalidated may result in missed invalidations.

- an L1 vcpu invalidating its own TLB (a very likely case) will not succeed in invalidating the VNCR pseudo TLB because that page is not mapped in L0 at this stage.

Given that this tracking fails at delivering the minimum guarantees that are required and is only a performance optimisation, remove it completely.

Fixes 4ffa72ad8f37e ("KVM: arm64: nv: Add S1 TLB invalidation primitive for VNCR_EL2")

Reviewed-by Yuan Yao <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

Cc [email protected]

---
 arch/arm64/include/asm/kvm_host.h | 3 ---
 arch/arm64/kvm/hyp/vhe/switch.c   | 3 +--
 arch/arm64/kvm/nested.c           | 3 ---
 3 files changed, 1 insertion(+), 8 deletions(-)

arch/arm64/include/asm/kvm_host.h +0 -3

index bae2c4f92ef5c..ac16f96c878d6 100644
@@ -411,9 +411,6 @@ struct kvm_arch {
 	/* Masks for VNCR-backed and general EL2 sysregs */
 	struct kvm_sysreg_masks	*sysreg_masks;
 
-	/* Count the number of VNCR_EL2 currently mapped */
-	atomic_t vncr_map_count;
-
 	/*
 	 * For an untrusted host VM, 'pkvm.handle' is used to lookup
 	 * the associated pKVM instance in the hypervisor.

arch/arm64/kvm/hyp/vhe/switch.c +0 -0

index bbe9cebd3d9d5..c09b1d411c584 100644

arch/arm64/kvm/hyp/vhe/switch.c +0 -0

 	__kvm_skip_instr(vcpu);

arch/arm64/kvm/nested.c +0 -2

index dfb96edbdc43c..f3c75954cf36c 100644
@@ -48,7 +48,6 @@ void kvm_init_nested(struct kvm *kvm)
 {
 	kvm->arch.nested_mmus = NULL;
 	kvm->arch.nested_mmus_size = 0;
-	atomic_set(&kvm->arch.vncr_map_count, 0);
 }
 
 static int init_nested_s2_mmu(struct kvm *kvm, struct kvm_s2_mmu *mmu)

@@ -890,7 +889,6 @@ static void this_cpu_reset_vncr_fixmap(struct kvm_vcpu *vcpu)
 	clear_fixmap(vncr_fixmap(vcpu->arch.vncr_tlb->cpu));
 	vcpu->arch.vncr_tlb->cpu = -1;
 	host_data_clear_flag(L1_VNCR_MAPPED);
-	atomic_dec(&vcpu->kvm->arch.vncr_map_count);
 }
 
 void kvm_vcpu_put_hw_mmu(struct kvm_vcpu *vcpu)

arch/arm64/kvm/nested.c +0 -1

--

2.47.3

Bear with me being verbose here, as this is both nascent review + learning :)

Quoted 5 lines

The global VNCR mapping counter is used to decide whether an L1 provided VNCR page is mapped in L0 on any CPU at the point of dealing with a TLB invalidation. It is incremented when a mapping is made in the fixmap, and decremented when unmapped.

Quoted 6 lines

As it turns out, this tracking has several flaws:

- we are trying to invalidate TLBs, and the mapping is only an opportunistic consequence of the TLB. Checking this counter to decide whether a TLB needs to be invalidated may result in missed invalidations.

Is it largely the self-invalidation mentioned below or are there other cases?

Quoted 5 lines

- an L1 vcpu invalidating its own TLB (a very likely case) will not succeed in invalidating the VNCR pseudo TLB because that page is not mapped in L0 at this stage.

Ahh yes this is pretty compelling then!

Quoted 8 lines

Given that this tracking fails at delivering the minimum guarantees that are required and is only a performance optimisation, remove it completely.

Fixes 4ffa72ad8f37e ("KVM: arm64: nv: Add S1 TLB invalidation primitive for VNCR_EL2")

Reviewed-by Yuan Yao <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

The change LGTM, it neatly removes the described mechanism which is well evidenced.

Comments below that are largely me talking out loud as I learn things :)

Acked-by Lorenzo Stoakes (ARM) <[email protected]>

Quoted 28 lines

Cc [email protected]

---
 arch/arm64/include/asm/kvm_host.h | 3 ---
 arch/arm64/kvm/hyp/vhe/switch.c   | 3 +--
 arch/arm64/kvm/nested.c           | 3 ---
 3 files changed, 1 insertion(+), 8 deletions(-)

arch/arm64/include/asm/kvm_host.h +0 -3

index bae2c4f92ef5c..ac16f96c878d6 100644
@@ -411,9 +411,6 @@ struct kvm_arch {
 	/* Masks for VNCR-backed and general EL2 sysregs */
 	struct kvm_sysreg_masks	*sysreg_masks;
 
-	/* Count the number of VNCR_EL2 currently mapped */
-	atomic_t vncr_map_count;
-
 	/*
 	 * For an untrusted host VM, 'pkvm.handle' is used to lookup
 	 * the associated pKVM instance in the hypervisor.

arch/arm64/kvm/hyp/vhe/switch.c +0 -0

index bbe9cebd3d9d5..c09b1d411c584 100644
@@ -427,8 +427,7 @@ static bool kvm_hyp_handle_tlbi_el2(struct kvm_vcpu *vcpu, u64 *exit_code)
 	 * If we have to check for any VNCR mapping being invalidated,
 	 * go back to the slow path for further processing.

arch/arm64/kvm/hyp/vhe/switch.c static bool kvm_hyp_handle_tlbi_el2(struct kvm_vcpu *vcpu, u64 *exit_code) · line 431

Quoted 5 lines

arch/arm64/kvm/hyp/vhe/switch.c +1 -2

 	 */
-	if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu) &&
-	    atomic_read(&vcpu->kvm->arch.vncr_map_count))
+	if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu))
 		return false;

So this seems to be the crux of it - seems to be 'is there any possibility that we will need to check for VNCR mappings being invalidated?'

Checks:

* vcpu_el2_e2h_is_set() - is the guest host kernel (?)'s hcr_el2.e2h enabled? From what I gather hcr_el2.e2h is what allows sysreg_EL1 -> sysreg_EL2 for the host kernel to allow unmodified kernels to run in EL2.

IOW - is the guest host kernel VHE?

* vcpu_el2_tge_is_set() - Similarly tests for the hcr_el2.tge bit - and this seems to be is 'EL1 -> EL2 redirection on?' - IOW - is this a kernel running in EL2?

Actually I see in is_hyp_ctxt():

* We are in a hypervisor context if the vcpu mode is EL2 or
* E2H and TGE bits are set. The latter means we are in the user space
* of the VHE kernel. ARMv8.1 ARM describes this as 'InHost'

So I _think_ the combination of the two is checking to see if you're the L0 kernel that _could_ send TLBi's that need to be handled?

Previously it seemed the logic was 'if there are no VNCR mappings present then we can optimise by short-circuiting the rest of the processing in kvm_hyp_handle_sysreg_vhe()'.

It seems that the hardware TLBi has been processed by now so it's actually more like - there's still work to be done maintaining the software TLB and that's done elsewhere.

I think this is all vaguely sane :)

Computing the effects of a TLB invalidation involves looking at the size of the mapping cached by the TLB. For S1 mappings such as VNCR, this is deducted from the combination of the base granule size and the mapping level.

However, this implies that the S1 MMU is *on*. When the MMU is off, we indicate this with the level being set to a "creative" value of -127 (S1_MMU_DISABLED).

This ends-up being misinterpreted by pgshift_level_to_ttl() as it doesn't handle negative levels at all (the level is immediately cast to a u8 and only the bottom two bits considered), leading to an invalidation size of 0. Not helpful.

Tidy-up pgshift_level_to_ttl() to handle these negative levels, and ttl_to_size() to always return SZ_1G when no valid TTL is present. This allows the removal of open-coded checks for similar situations.

Note that the check for a negative value not explicitely checking for

S1_MMU_DISABLED is deliberate, so that actual negative levels introduced with LVA2 and D128 can take the same path if we ever support them.

Fixes 7270cc9157f47 ("KVM: arm64: nv: Handle VNCR_EL2 invalidation from MMU notifiers")

Reported-by Hyunwoo Kim <[email protected]>

Link https://lore.kernel.org/r/ameGoxbn2wzBq2kL@v4bel open here

Signed-off-by Marc Zyngier <[email protected]>

Cc [email protected]

---
 arch/arm64/kvm/nested.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

arch/arm64/kvm/nested.c +1 -1

index f3c75954cf36c..035cda256e2a5 100644
@@ -505,7 +505,7 @@ int kvm_walk_nested_s2(struct kvm_vcpu *vcpu, phys_addr_t gipa,
 	return ret;
 }
 
-static unsigned int ttl_to_size(u8 ttl)
+static unsigned int __ttl_to_size(u8 ttl)
 {
 	int level = ttl & 3;
 	int gran = (ttl >> 2) & 3;

arch/arm64/kvm/nested.c +9 -0

+
+static u8 pgshift_level_to_ttl(u16 shift, s8 level)
 {
 	u8 ttl;
 
+	/*
+	 * If we don't have a proper level, fallback to the maximum
+	 * size.
+	 */
+	if (level < 0)
+		return 0;
+
 	switch(shift) {
 	case 12:
 		ttl = TLBI_TTL_TG_4K;

arch/arm64/kvm/nested.c +1 -2

+	max_size = __ttl_to_size(ttl);
 
 	if (!max_size) {
 		/* Compute the maximum extent of the invalidation */

@@ -1124,8 +1140,6 @@ static void compute_s1_tlbi_range(struct kvm_vcpu *vcpu, u32 inst, u64 val,
 	case OP_TLBI_VALE1OSNXS:
 		scope->type = TLBI_VA;
 		scope->size = ttl_to_size(FIELD_GET(TLBI_TTL_MASK, val));
-		if (!scope->size)
-			scope->size = SZ_1G;
 		scope->va = tlbi_va_s1_to_va(val) & ~(scope->size - 1);
 		scope->asid = FIELD_GET(TLBIR_ASID_MASK, val);
 		break;

arch/arm64/kvm/nested.c +0 -1

 		scope->va = tlbi_va_s1_to_va(val) & ~(scope->size - 1);
 		break;
 	case OP_TLBI_RVAE2:
--

2.47.3

So actually if granularity is TLBI_TTL_TG_4K this will return SZ_1G and 0 in the other cases unless I'm getting something wrong here?

This is really more a 'maybe worth mentioning in the commit log to be pedantic' kind of thing :)

IOW you could luck out before with SZ_1G for TLBI_TTL_TG_4K.

Quoted 5 lines

Tidy-up pgshift_level_to_ttl() to handle these negative levels, and ttl_to_size() to always return SZ_1G when no valid TTL is present. This allows the removal of open-coded checks for similar situations.

I guess SZ_1G is a reasonable default here?

Quoted 3 lines

Note that the check for a negative value not explicitely checking for

NIT: explicitely -> explicitly

Quoted 29 lines

S1_MMU_DISABLED is deliberate, so that actual negative levels introduced with LVA2 and D128 can take the same path if we ever support them.

Fixes 7270cc9157f47 ("KVM: arm64: nv: Handle VNCR_EL2 invalidation from MMU notifiers")

Reported-by Hyunwoo Kim <[email protected]>

Link https://lore.kernel.org/r/ameGoxbn2wzBq2kL@v4bel open here

Signed-off-by Marc Zyngier <[email protected]>

Cc [email protected]

---
 arch/arm64/kvm/nested.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

arch/arm64/kvm/nested.c +1 -1

index f3c75954cf36c..035cda256e2a5 100644
@@ -505,7 +505,7 @@ int kvm_walk_nested_s2(struct kvm_vcpu *vcpu, phys_addr_t gipa,
 	return ret;
 }
 
-static unsigned int ttl_to_size(u8 ttl)
+static unsigned int __ttl_to_size(u8 ttl)
 {
 	int level = ttl & 3;
 	int gran = (ttl >> 2) & 3;

@@ -561,10 +561,22 @@ static unsigned int ttl_to_size(u8 ttl)
 	return max_size;
 }
 

arch/arm64/kvm/nested.c static unsigned int ttl_to_size(u8 ttl) · line 567

Quoted 5 lines

arch/arm64/kvm/nested.c +4 -1

-static u8 pgshift_level_to_ttl(u16 shift, u8 level)
+static unsigned int ttl_to_size(u8 ttl)
+{
+	return __ttl_to_size(ttl) ?: SZ_1G;
+}

Might be worth a comment about the default?

Quoted 20 lines

arch/arm64/kvm/nested.c +9 -0

+
+static u8 pgshift_level_to_ttl(u16 shift, s8 level)
 {
 	u8 ttl;
 
+	/*
+	 * If we don't have a proper level, fallback to the maximum
+	 * size.
+	 */
+	if (level < 0)
+		return 0;
+
 	switch(shift) {
 	case 12:
 		ttl = TLBI_TTL_TG_4K;

@@ -675,7 +687,11 @@ unsigned long compute_tlb_inval_range(struct kvm_s2_mmu *mmu, u64 val)
 		ttl = get_guest_mapping_ttl(mmu, addr);
 	}
 

arch/arm64/kvm/nested.c unsigned long compute_tlb_inval_range(struct kvm_s2_mmu *mmu, u64 val) · line 693

Quoted 5 lines

arch/arm64/kvm/nested.c +4 -1

-	max_size = ttl_to_size(ttl);
+	/*
+	 * Don't use the default 1GB fallback, as we can adapt to the
+	 * max mapping size we allow at S2.
+	 */

Being a bit pedantic here but I wonder if simply just to say 'Adapt to the max mapping size allowed at S2' as the fallback is inferred?

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Range TLBI instructions for Stage-1 TTBR1 addresses silently fail to invalidate VNCR entries because the base address is not sign-extended.
- [High] The tlbi_va_s1_to_va macro and read_vncr_el2 sign-extend from bit 48, which corrupts 52-bit Virtual Addresses (LVA). --

--- Patch [4]: [PATCH v2 4/8] KVM: arm64: Correctly handle end of VA space TLBI invalidation --- commit 0f00ac726029153c77159941056d984a8dc753cf
Author: Marc Zyngier <[email protected]>
Subject: KVM: arm64: Correctly handle end of VA space TLBI invalidation

This commit fixes an issue with TLB invalidation by VA where address rollover at the end of the TTBR1 VA space caused invalidations to fail. It introduces a common helper, vncr_tlb_intersects(), which uses inclusive end addresses to prevent this issue for both VA and IPA invalidations.

Quoted 5 lines

arch/arm64/kvm/nested.c +0 -0

index 939813aebaafa..67cc5b6d80705 100644
@@ -1000,6 +1000,20 @@ static void invalidate_vncr(struct vncr_tlb *vt)

[ ... ]

arch/arm64/kvm/nested.c static void invalidate_vncr(struct vncr_tlb *vt) · line 1013

Quoted 12 lines

arch/arm64/kvm/nested.c +12 -0

+static bool vncr_tlb_intersects(struct vncr_tlb *vt, u64 addr,
+				u64 scope_start, u64 scope_size)
+{
+	u64 tlb_size, tlb_start, tlb_end, scope_end;
+
+	tlb_size = ttl_to_size(pgshift_level_to_ttl(vt->wi.pgshift, vt->wr.level));
+
+	tlb_start = addr & ~(tlb_size - 1);
+	tlb_end = tlb_start + tlb_size - 1;
+	scope_end = scope_start + scope_size - 1;
+
+	return !(tlb_end < scope_start || tlb_start > scope_end);

[Severity: High]
This is a pre-existing issue, but could this intersection check fail for Stage-1 TTBR1 Range TLBIs due to a missing sign extension?

When a guest issues a Range TLBI for a TTBR1 virtual address, decode_range_tlbi() zero-extends the base address:

arch/arm64/include/asm/kvm_nested.h:decode_range_tlbi() {
    ...
    base = (val & GENMASK(36, 0)) << shift;
    ...
}

In compute_s1_tlbi_range(), this is directly assigned to scope->va.

Since scope->va is passed as scope_start here and is zero-extended (a large positive unsigned number), won't it fail to intersect with vt->gva, which is sign-extended (a negative number for TTBR1)?

Quoted 6 lines

arch/arm64/kvm/nested.c +2 -0

+}
+
 /*
  * VNCR TLB invalidation occurs from MMU notifiers or TLBI instructions, and
  * either can race against a vcpu not being onlined yet (no pseudo-TLB

[ ... ]

arch/arm64/kvm/nested.c static void invalidate_vncr_va(struct kvm *kvm, · line 1072

Quoted 12 lines

arch/arm64/kvm/nested.c +1 -2

@@ -1060,28 +1064,19 @@ static void invalidate_vncr_va(struct kvm *kvm,
 	lockdep_assert_held_write(&kvm->mmu_lock);
 
 	kvm_for_each_vncr_tlb(i, vcpu, vt, kvm) {
 		switch (scope->type) {
 		case TLBI_ALL:
 			break;
 
 		case TLBI_VA:
-			if (va_end <= scope->va ||
-			    va_start >= (scope->va + scope->size))
+			if (!vncr_tlb_intersects(vt, vt->gva, scope->va, scope->size))

[Severity: High]
This isn't a bug introduced by this patch, but does KVM incorrectly truncate 52-bit Virtual Addresses (LVA) to 48 bits when constructing these VAs?

The tlbi_va_s1_to_va() macro and read_vncr_el2() both hardcode a 48-bit sign extension:

arch/arm64/kvm/nested.c:read_vncr_el2() {
    ...
    return (u64)sign_extend64(__vcpu_sys_reg(vcpu, VNCR_EL2), 48);
}

If a guest uses 52-bit VAs (FEAT_LVA) and places the VNCR page at a TTBR1 address where bit 48 is 0, wouldn't the upper bits (49-63) be zero-extended instead of sign-extended?

This seems like it would corrupt the VA into a TTBR0 address, and Range TLBIs (which preserve bits up to 53 in decode_range_tlbi) would fail to intersect with the corrupted 48-bit vt->gva.

Our TLB invalidation by VA code is based on comparing two ranges, one defined by the TLB, and one defined by the TLBI instruction.

Each range is defined by a start and a size. However, the way the comparison is done doesn't account for address rollover, as it compares an address with (base + size). This works nicely until this expression represent the last page/block in the TTBR1 VA space, as the result is a big fat 0. And a failed TLB invalidation.

Rewrite the comparison in a way that is immune to the address rollover (making the end address inclusive instead of exclusive), and move this into a common helper that is used by both VA and IPA invalidations, as suggested by Hyunwoo Kim (although the IPA version didn't suffer from this particular problem, obviously).

Fixes 4ffa72ad8f37e ("KVM: arm64: nv: Add S1 TLB invalidation primitive for VNCR_EL2")

Reviewed-by Yuan Yao <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

Cc [email protected]

---
 arch/arm64/kvm/nested.c | 43 ++++++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 24 deletions(-)

arch/arm64/kvm/nested.c +0 -0

index 27bc7ee4b3382..8a602d074dbb4 100644
@@ -999,6 +999,20 @@ static void invalidate_vncr(struct vncr_tlb *vt)
 		clear_fixmap(vncr_fixmap(vt->cpu));
 }
 

arch/arm64/kvm/nested.c +7 -24

+}
+
 /*
  * VNCR TLB invalidation occurs from MMU notifiers or TLBI instructions, and
  * either can race against a vcpu not being onlined yet (no pseudo-TLB

@@ -1021,19 +1035,9 @@ static void kvm_invalidate_vncr_ipa(struct kvm *kvm, u64 start, u64 end)
 	if (!kvm_has_feat(kvm, ID_AA64MMFR4_EL1, NV_frac, NV2_ONLY))
 		return;
 
-	kvm_for_each_vncr_tlb(i, vcpu, vt, kvm) {
-		u64 ipa_start, ipa_end, ipa_size;
-
-		ipa_size = ttl_to_size(pgshift_level_to_ttl(vt->wi.pgshift,
-							    vt->wr.level));
-		ipa_start = vt->wr.pa & ~(ipa_size - 1);
-		ipa_end = ipa_start + ipa_size;
-
-		if (ipa_end <= start || ipa_start >= end)
-			continue;
-
-		invalidate_vncr(vt);
-	}
+	kvm_for_each_vncr_tlb(i, vcpu, vt, kvm)
+		if (vncr_tlb_intersects(vt, vt->wr.pa, start, end - start))
+			invalidate_vncr(vt);
 }
 
 struct s1e2_tlbi_scope {

@@ -1059,28 +1063,19 @@ static void invalidate_vncr_va(struct kvm *kvm,
 	lockdep_assert_held_write(&kvm->mmu_lock);
 
 	kvm_for_each_vncr_tlb(i, vcpu, vt, kvm) {
-		u64 va_start, va_end, va_size;
-
-		va_size = ttl_to_size(pgshift_level_to_ttl(vt->wi.pgshift,
-							   vt->wr.level));
-		va_start = vt->gva & ~(va_size - 1);
-		va_end = va_start + va_size;
-
 		switch (scope->type) {
 		case TLBI_ALL:
 			break;
 
 		case TLBI_VA:
-			if (va_end <= scope->va ||
-			    va_start >= (scope->va + scope->size))
+			if (!vncr_tlb_intersects(vt, vt->gva, scope->va, scope->size))
 				continue;
 			if (vt->wr.nG && vt->wr.asid != scope->asid)
 				continue;
 			break;
 
 		case TLBI_VAA:
-			if (va_end <= scope->va ||
-			    va_start >= (scope->va + scope->size))
+			if (!vncr_tlb_intersects(vt, vt->gva, scope->va, scope->size))
 				continue;
 			break;
 

Signature · 1 line

2.47.3

Hi Marc,

Quoted 31 lines · Marc Zyngier

Our TLB invalidation by VA code is based on comparing two ranges, one defined by the TLB, and one defined by the TLBI instruction.

Each range is defined by a start and a size. However, the way the comparison is done doesn't account for address rollover, as it compares an address with (base + size). This works nicely until this expression represent the last page/block in the TTBR1 VA space, as the result is a big fat 0. And a failed TLB invalidation.

Rewrite the comparison in a way that is immune to the address rollover (making the end address inclusive instead of exclusive), and move this into a common helper that is used by both VA and IPA invalidations, as suggested by Hyunwoo Kim (although the IPA version didn't suffer from this particular problem, obviously).

Fixes 4ffa72ad8f37e ("KVM: arm64: nv: Add S1 TLB invalidation primitive for VNCR_EL2")

Reviewed-by Yuan Yao <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

Cc [email protected]

---
 arch/arm64/kvm/nested.c | 43 ++++++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 24 deletions(-)

arch/arm64/kvm/nested.c +0 -0

index 27bc7ee4b3382..8a602d074dbb4 100644
@@ -999,6 +999,20 @@ static void invalidate_vncr(struct vncr_tlb *vt)
 		clear_fixmap(vncr_fixmap(vt->cpu));
 }
 

arch/arm64/kvm/nested.c static void invalidate_vncr(struct vncr_tlb *vt) · line 1011

Quoted 10 lines

arch/arm64/kvm/nested.c +10 -0

+static bool vncr_tlb_intersects(struct vncr_tlb *vt, u64 addr,
+				u64 scope_start, u64 scope_size)
+{
+	u64 tlb_size, tlb_start, tlb_end, scope_end;
+
+	tlb_size = ttl_to_size(pgshift_level_to_ttl(vt->wi.pgshift, vt->wr.level));
+
+	tlb_start = addr & ~(tlb_size - 1);
+	tlb_end = tlb_start + tlb_size - 1;
+	scope_end = scope_start + scope_size - 1;

I think if scope_end overflows,

arch/arm64/kvm/nested.c static void invalidate_vncr(struct vncr_tlb *vt) · line 1013

Quoted 2 lines

arch/arm64/kvm/nested.c +2 -0

+
+	return !(tlb_end < scope_start || tlb_start > scope_end);

tlb_start > scope_end can evaluate to true and we return false even when there is overlap near the end of the TTBR1 address space.

Therefore I think we need to saturate scope_end to ULONG_MAX when overflow happens. This is also what the architecture does when looking at the pseudocode J1.2.3.442 TLBIRange (M.c).

Quoted 48 lines · Wei-Lin Chang

Hi Marc,

Quoted 31 lines · Marc Zyngier

Our TLB invalidation by VA code is based on comparing two ranges, one defined by the TLB, and one defined by the TLBI instruction.

Each range is defined by a start and a size. However, the way the comparison is done doesn't account for address rollover, as it compares an address with (base + size). This works nicely until this expression represent the last page/block in the TTBR1 VA space, as the result is a big fat 0. And a failed TLB invalidation.

Rewrite the comparison in a way that is immune to the address rollover (making the end address inclusive instead of exclusive), and move this into a common helper that is used by both VA and IPA invalidations, as suggested by Hyunwoo Kim (although the IPA version didn't suffer from this particular problem, obviously).

Fixes 4ffa72ad8f37e ("KVM: arm64: nv: Add S1 TLB invalidation primitive for VNCR_EL2")

Reviewed-by Yuan Yao <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

Cc [email protected]

---
 arch/arm64/kvm/nested.c | 43 ++++++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 24 deletions(-)

arch/arm64/kvm/nested.c +0 -0

index 27bc7ee4b3382..8a602d074dbb4 100644
@@ -999,6 +999,20 @@ static void invalidate_vncr(struct vncr_tlb *vt)
 		clear_fixmap(vncr_fixmap(vt->cpu));
 }
 

arch/arm64/kvm/nested.c static void invalidate_vncr(struct vncr_tlb *vt) · line 1011

Quoted 10 lines

arch/arm64/kvm/nested.c +10 -0

+static bool vncr_tlb_intersects(struct vncr_tlb *vt, u64 addr,
+				u64 scope_start, u64 scope_size)
+{
+	u64 tlb_size, tlb_start, tlb_end, scope_end;
+
+	tlb_size = ttl_to_size(pgshift_level_to_ttl(vt->wi.pgshift, vt->wr.level));
+
+	tlb_start = addr & ~(tlb_size - 1);
+	tlb_end = tlb_start + tlb_size - 1;
+	scope_end = scope_start + scope_size - 1;

I think if scope_end overflows,

Quoted 5 lines

arch/arm64/kvm/nested.c static void invalidate_vncr(struct vncr_tlb *vt) · line 1013

Quoted 2 lines

arch/arm64/kvm/nested.c +2 -0

+
+	return !(tlb_end < scope_start || tlb_start > scope_end);

tlb_start > scope_end can evaluate to true and we return false even when there is overlap near the end of the TTBR1 address space.

I assume that you are implicitly talking about TLB Range Invalidation, right? Because we otherwise align base on size, making overflows impossible with this patch.

Quoted 4 lines

Therefore I think we need to saturate scope_end to ULONG_MAX when overflow happens. This is also what the architecture does when looking at the pseudocode J1.2.3.442 TLBIRange (M.c).

That's one possible implementation, because the pseudocode is dealing with start/end directly, but that's not how our scope works (it deals with start and size).

For that we need to cap the *size*, similarly to what is done in handle_ripas2e1is() (see patch #6). Something like the hack below.

	M.

arch/arm64/include/asm/kvm_nested.h +0 -0

index 21d0f4cbe07f1..5b8edb2e8a87d 100644
@@ -305,6 +305,12 @@ static inline u64 decode_range_tlbi(u64 val, u64 *range, u16 *asid)
 	num	= FIELD_GET(GENMASK(43, 39), val);

arch/arm64/include/asm/kvm_nested.h +0 -1

-- 

Without deviation from the norm, progress is not possible.

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Acquiring mmu_lock with preemption disabled causes a sleep-in-atomic bug on PREEMPT_RT kernels. --

--- Patch [5]: [PATCH v2 5/8] KVM: arm64: Handle VNCR TLB invalidation race with vcpu_put() VNCR unmapping --- commit 7a7c469ac4158e77c3548a526ab62a924ed75161
Author: Marc Zyngier <[email protected]>

KVM: arm64: Handle VNCR TLB invalidation race with vcpu_put() VNCR unmapping

This patch addresses a race condition between VNCR TLB invalidation and VNCR unmapping during vcpu_put(). It introduces an atomic exchange for resetting the cpu tracking field and a common function for unmapping, ensuring these operations happen atomically. This closes the TOCTOU race and prevents concurrent mapping faults.

Link https://lore.kernel.org/r/[email protected] open here

Quoted 4 lines

arch/arm64/kvm/nested.c +0 -0

index 67cc5b6d80705..05f0e7e89e982 100644

[ ... ]

arch/arm64/kvm/nested.c static void kvm_map_l1_vncr(struct kvm_vcpu *vcpu)

Quoted 1 line

arch/arm64/kvm/nested.c +0 -0

@@ -1584,8 +1607,6 @@ static void kvm_map_l1_vncr(struct kvm_vcpu *vcpu)

[Severity: High]
This is a pre-existing issue, but does acquiring mmu_lock inside a preemption-disabled region here cause a sleep-in-atomic bug on RT kernels?

Looking at kvm_map_l1_vncr() from the context, it starts with:

static void kvm_map_l1_vncr(struct kvm_vcpu *vcpu)

{
        struct vncr_tlb *vt = vcpu->arch.vncr_tlb;
        pgprot_t prot;

        guard(preempt)();
        guard(read_lock)(&vcpu->kvm->mmu_lock);

Since read_lock() is implemented as an RT-mutex on PREEMPT_RT and can block, could this sequence trigger a scheduling-while-atomic panic if the lock is contended?

While VNCR TLB invalidation always occurs under the MMU lock, vcpu_put() doesn't, while it unmaps the VNCR page.

The problem is that the invalidation evaluates vncr_tlb::cpu to decide whether an unmapping needs to take place (cpu != -1) before performing it. On the other hand, this_cpu_reset_vncr_fixmap() unconditionally unmaps if L1_VNCR_MAPPED is set.

These two obviously can race, with a TOCTOU pattern on the TLBI path, and a BUG_ON() on the vcpu_put() path. And the two can end-up calling vncr_fixmap(-1), with extra lethal effects.

static void this_cpu_reset_vncr_fixmap(struct kvm_vcpu *vcpu) {

arch/arm64/kvm/nested.c +0 -0

 	if (!host_data_test_flag(L1_VNCR_MAPPED))
 		return;
 

arch/arm64/kvm/nested.c +11 -7

+	 *
+	 * Note that we do not care about the TLBI nuking the fixmap behind
+	 * the back of an running vcpu. This will only generate a fault and
+	 * possibly a retranslation.
+	 */
+	if (unmap_l1_vncr(vcpu->arch.vncr_tlb) == -1)
+		clear_fixmap(vncr_fixmap(smp_processor_id()));
 	host_data_clear_flag(L1_VNCR_MAPPED);
 }

@@ -995,8 +1019,7 @@ u16 get_asid_by_regime(struct kvm_vcpu *vcpu, enum trans_regime regime)
 static void invalidate_vncr(struct vncr_tlb *vt)
 {
 	vt->valid = false;
-	if (vt->cpu != -1)
-		clear_fixmap(vncr_fixmap(vt->cpu));
+	unmap_l1_vncr(vt);
 }
 
 static bool vncr_tlb_intersects(struct vncr_tlb *vt, u64 addr,

@@ -1452,7 +1475,7 @@ static int kvm_translate_vncr(struct kvm_vcpu *vcpu, bool *is_gmem)
 		vt->hpa = pfn << PAGE_SHIFT;
 		vt->hpa_writable = writable;
 		vt->valid = true;
-		vt->cpu = -1;
+		atomic_set(&vt->cpu, -1);
 
 		kvm_make_request(KVM_REQ_MAP_L1_VNCR_EL2, vcpu);
 		kvm_release_faultin_page(vcpu->kvm, page, false, vt->wr.pw && vt->hpa_writable);

@@ -1583,8 +1606,6 @@ static void kvm_map_l1_vncr(struct kvm_vcpu *vcpu)
 	if (vt->wr.nG && get_asid_by_regime(vcpu, TR_EL20) != vt->wr.asid)
 		return;
 
-	vt->cpu = smp_processor_id();
-
 	if (vt->hpa_writable && vt->wr.pw && vt->wr.pr)
 		prot = PAGE_KERNEL;
 	else if (vt->wr.pr)

@@ -1599,7 +1620,8 @@ static void kvm_map_l1_vncr(struct kvm_vcpu *vcpu)
 	 * FIXME: WO doesn't work at all, need POE support in the kernel.
 	 */
 	if (pgprot_val(prot) != pgprot_val(PAGE_NONE)) {
-		__set_fixmap(vncr_fixmap(vt->cpu), vt->hpa, prot);
+		atomic_set(&vt->cpu, smp_processor_id());
+		__set_fixmap(vncr_fixmap(atomic_read(&vt->cpu)), vt->hpa, prot);
 		host_data_set_flag(L1_VNCR_MAPPED);
 	}
 }
--

2.47.3

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The fast-path TLBI handler incorrectly skips the slow path for native EL2 TLBI instructions when E2H or TGE is clear, bypassing software VNCR TLB invalidation and leaving stale mappings. --

--- Patch [8]: [PATCH v2 8/8] KVM: arm64: Add VNCR TLB tracking again --- commit 65741607ee5db3e8d57cf0e1773bb1031e83a3ee
Author: Marc Zyngier <[email protected]>

KVM: arm64: Add VNCR TLB tracking again

This commit introduces a VM-wide counter to track valid VNCR TLBs. It aims to improve performance by taking the TLBI slow path only when there are active VNCR TLBs that need invalidation.

Quoted 3 lines

arch/arm64/kvm/hyp/vhe/switch.c +0 -0

[ ... ]

Quoted 13 lines

arch/arm64/kvm/hyp/vhe/switch.c +6 -2

@@ -424,10 +424,15 @@ static bool kvm_hyp_handle_tlbi_el2(struct kvm_vcpu *vcpu, u64 *exit_code)
 		return false;
 
 	/*
-	 * If we have to check for any VNCR mapping being invalidated,
-	 * go back to the slow path for further processing.
+	 * If we have to check for any VNCR TLB being invalidated, go back
+	 * to the slow path for further processing.
+	 *
+	 * The synchronisation betweem TLBI and walk is provided by the
+	 * speculative increment of the TLB counter on walk, and the
+	 * invalidation counter. Yes, this is fiddly.
 	 */

arch/arm64/kvm/hyp/vhe/switch.c static bool kvm_hyp_handle_tlbi_el2(struct kvm_vcpu *vcpu, u64 *exit_code) · line 436

Quoted 4 lines

arch/arm64/kvm/hyp/vhe/switch.c +2 -1

-	if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu))
+	if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu) &&
+	    atomic_read(&vcpu->kvm->arch.vncr_tlb_count))
 		return false;

[Severity: High]
This is a pre-existing issue, but does this condition incorrectly skip the slow path for native EL2 TLBI instructions when E2H or TGE is clear?

Prompted by a patch [0] from Hyunwoo Kim which was addressing a pretty annoying corner case, I spent some time looking at what was wrong in our VNCR TLBI code. And there was a few things to say about it...

- The TLB tracking to get into the slow path is wrong. It tracks what is mapped, but not the established TLBs. Kind of annoying.

[...]