Skip to contentSkip to conversation

KVM: arm64: Fix S1/S2 combination when FWB==1 and S2 has Device memory type

PATCH Applied Marc Zyngier 2 participants 5 messages Nov 25, 2024 – Nov 26, 2024

Subsystems (2)
arm64 KVM arm64
lore
Share & tools
lore

Marc Zyngier

author maintainer
#0 lore raw

PATCH 1/1 KVM: arm64: Fix S1/S2 combination when FWB==1 and S2 has Device memory type

The G.a revision of the ARM ARM had it pretty clear that HCR_EL2.FWB had no influence on "The way that stage 1 memory types and attributes are combined with stage 2 Device type and attributes." (D5.5.5).

However, this wording was lost in further revisions of the architecture.

Restore the intended behaviour, which is to take the strongest memory type of S1 and S2 in this case, as if FWB was 0. The specification is being fixed accordingly.

Fixes be04cebf3e788 ("KVM: arm64: nv: Add emulation of AT S12E{0,1}{R,W}")
Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/kvm/at.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
arch/arm64/kvm/at.c +9 -2
index 8c5d7990e5b31..98cb499fa4b11 100644
@@ -739,8 +739,15 @@ static u64 compute_par_s12(struct kvm_vcpu *vcpu, u64 s1_par,
 			final_attr = s1_parattr;
 			break;
 		default:
-			/* MemAttr[2]=0, Device from S2 */
-			final_attr = s2_memattr & GENMASK(1,0) << 2;
+			/*
+			 * MemAttr[2]=0, Device from S2.
+			 *
+			 * FWB does not influence the way that stage 1
+ 			 * memory types and attributes are combined
+ 			 * with stage 2 Device type and attributes.
+			 */
+			final_attr = min(s2_memattr_to_attr(s2_memattr),
+					 s1_parattr);
 		}
 	} else {
 		/* Combination of R_HMNDG, R_TNHFM and R_GQFSF */
Signature · 1 line
2.39.2
#1 lore raw
Quoted 9 lines · Marc Zyngier

The G.a revision of the ARM ARM had it pretty clear that HCR_EL2.FWB had no influence on "The way that stage 1 memory types and attributes are combined with stage 2 Device type and attributes." (D5.5.5).

However, this wording was lost in further revisions of the architecture.

Restore the intended behaviour, which is to take the strongest memory type of S1 and S2 in this case, as if FWB was 0. The specification is being fixed accordingly.

Since you're already asking for a spec fix, could you mention that the column headers in DDI0487K.a Table D8-95 are incorrect? MemAttr[1:0] is used twice, although I believe the first column is actually MemAttr[3:2].

Quoted 15 lines
Fixes be04cebf3e788 ("KVM: arm64: nv: Add emulation of AT S12E{0,1}{R,W}")
Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/kvm/at.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
arch/arm64/kvm/at.c +0 -1
index 8c5d7990e5b31..98cb499fa4b11 100644
@@ -739,8 +739,15 @@ static u64 compute_par_s12(struct kvm_vcpu *vcpu, u64 s1_par,
 			final_attr = s1_parattr;
 			break;
 		default:
-			/* MemAttr[2]=0, Device from S2 */

arch/arm64/kvm/at.c static u64 compute_par_s12(struct kvm_vcpu *vcpu, u64 s1_par, · line 750

Quoted 10 lines
arch/arm64/kvm/at.c +9 -1
-			final_attr = s2_memattr & GENMASK(1,0) << 2;
+			/*
+			 * MemAttr[2]=0, Device from S2.
+			 *
+			 * FWB does not influence the way that stage 1
+ 			 * memory types and attributes are combined
+ 			 * with stage 2 Device type and attributes.
+			 */
+			final_attr = min(s2_memattr_to_attr(s2_memattr),
+					 s1_parattr);

Otherwise, LGTM.

Signature · 2 lines
Thanks,
Oliver
author maintainer
#2 lore raw
Quoted 8 lines · Oliver Upton
Quoted 6 lines · Marc Zyngier

The G.a revision of the ARM ARM had it pretty clear that HCR_EL2.FWB had no influence on "The way that stage 1 memory types and attributes are combined with stage 2 Device type and attributes." (D5.5.5).

However, this wording was lost in further revisions of the architecture.

Quoted 7 lines
Quoted 3 lines

Restore the intended behaviour, which is to take the strongest memory type of S1 and S2 in this case, as if FWB was 0. The specification is being fixed accordingly.

Since you're already asking for a spec fix, could you mention that the column headers in DDI0487K.a Table D8-95 are incorrect? MemAttr[1:0] is used twice, although I believe the first column is actually MemAttr[3:2].

That one has already been fixed as D22366, as described in the Known Issues document for version K.a (issue 07) [1].

Thanks,

	M.

[1] https://developer.arm.com/documentation/102105/latest/

Signature · 1 line
Without deviation from the norm, progress is not possible.
#3 lore raw
Quoted 14 lines · Marc Zyngier
Quoted 12 lines · Oliver Upton
Quoted 9 lines · Marc Zyngier

The G.a revision of the ARM ARM had it pretty clear that HCR_EL2.FWB had no influence on "The way that stage 1 memory types and attributes are combined with stage 2 Device type and attributes." (D5.5.5).

However, this wording was lost in further revisions of the architecture.

Restore the intended behaviour, which is to take the strongest memory type of S1 and S2 in this case, as if FWB was 0. The specification is being fixed accordingly.

Quoted 6 lines
Quoted 3 lines

Since you're already asking for a spec fix, could you mention that the column headers in DDI0487K.a Table D8-95 are incorrect? MemAttr[1:0] is used twice, although I believe the first column is actually MemAttr[3:2].

That one has already been fixed as D22366, as described in the Known Issues document for version K.a (issue 07) [1].

Ah, right on. Thanks!

Signature · 2 lines
Best,
Oliver
Applied
#4 lore raw
In reply to Marc Zyngier
Quoted 11 lines · Marc Zyngier

The G.a revision of the ARM ARM had it pretty clear that HCR_EL2.FWB had no influence on "The way that stage 1 memory types and attributes are combined with stage 2 Device type and attributes." (D5.5.5).

However, this wording was lost in further revisions of the architecture.

Restore the intended behaviour, which is to take the strongest memory type of S1 and S2 in this case, as if FWB was 0. The specification is being fixed accordingly.

[...]

Applied to fixes, thanks!

[1/1] KVM: arm64: Fix S1/S2 combination when FWB==1 and S2 has Device memory type

      https://git.kernel.org/kvmarm/kvmarm/c/6fc3a49f2385

--
Best,
Oliver