Skip to contentSkip to conversation

KVM: arm64: nv: Respect HA when computing translated TCR

PATCH In review Oliver Upton 3 participants 6 messages Oct 29

Subsystems (3)
lore
Share & tools
lore

Oliver Upton

author maintainer
#0 lore raw

PATCH 1/1 KVM: arm64: nv: Respect HA when computing translated TCR

Add the missing forwarding of TCR_EL2.HA to TCR_EL1.HA, which are different bit indices, to ensure that FEAT_HAF actually works for an L1 hypervisor in the EL2 translation regime.

Fixes d5bbb76f4474 ("KVM: arm64: nv: Expose hardware access flag management to NV guests")

---
Spinning this off from the HAFDBS series since this fixes an issue with FEAT_HAF which is already supported in NV.

 arch/arm64/include/asm/kvm_nested.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
arch/arm64/include/asm/kvm_nested.h +2 -1
index 0f899addafa8..2811ec466d88 100644
@@ -29,7 +29,8 @@ static inline u64 translate_tcr_el2_to_tcr_el1(u64 tcr)
 	       (tcr & TCR_EL2_TG0_MASK) |
 	       (tcr & TCR_EL2_ORGN0_MASK) |
 	       (tcr & TCR_EL2_IRGN0_MASK) |
-	       (tcr & TCR_EL2_T0SZ_MASK);
+	       (tcr & TCR_EL2_T0SZ_MASK) |
+	       ((tcr & TCR_EL2_HA) ? TCR_HA : 0);
 }
 
 static inline u64 translate_cptr_el2_to_cpacr_el1(u64 cptr_el2)

base-commit: 9e9283e4c941e806d2735f09b66763487bc48b50

Signature · 1 line
2.47.3
reviewer
#1 lore raw

Hi Oliver,

On Thu, 29 Oct 2026 09:05:22 +0000, Oliver Upton <[email protected]> wrote:
[...]

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

[...]

arch/arm64/include/asm/kvm_nested.h static inline u64 translate_tcr_el2_to_tcr_el1(u64 tcr) · line 34

Quoted 8 lines
arch/arm64/include/asm/kvm_nested.h +2 -1
@@ -29,7 +29,8 @@ static inline u64 translate_tcr_el2_to_tcr_el1(u64 tcr)
               (tcr & TCR_EL2_TG0_MASK) |
               (tcr & TCR_EL2_ORGN0_MASK) |
               (tcr & TCR_EL2_IRGN0_MASK) |
-              (tcr & TCR_EL2_T0SZ_MASK);
+              (tcr & TCR_EL2_T0SZ_MASK) |
+              ((tcr & TCR_EL2_HA) ? TCR_HA : 0);
 }

Should you forward SH0 in this patch as well? It's missing here, so the walks are Non-shareable, and looking at the ARM ARM, this would make every hardware Access flag update CONSTRAINED UNPREDICTABLE (R_WLTNL, and the IsWBShareable() check in AArch64_S1Translate()). An L1 that programmed SH0 as Inner Shareable could then take an unsupported atomic update fault.

HPD isn't forwarded either, although at.c reads it from TCR_EL2 for an E2H=0 guest. Was that intentional?

Cheers,
/fuad

maintainer
#2 lore raw
Quoted 7 lines · Fuad Tabba

Hi Oliver,

On Thu, 29 Oct 2026 09:05:22 +0000, Oliver Upton <[email protected]> wrote:
[...]

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

[...]

Quoted 15 lines

arch/arm64/include/asm/kvm_nested.h static inline u64 translate_tcr_el2_to_tcr_el1(u64 tcr) · line 34

Quoted 8 lines
arch/arm64/include/asm/kvm_nested.h +2 -1
@@ -29,7 +29,8 @@ static inline u64 translate_tcr_el2_to_tcr_el1(u64 tcr)
               (tcr & TCR_EL2_TG0_MASK) |
               (tcr & TCR_EL2_ORGN0_MASK) |
               (tcr & TCR_EL2_IRGN0_MASK) |
-              (tcr & TCR_EL2_T0SZ_MASK);
+              (tcr & TCR_EL2_T0SZ_MASK) |
+              ((tcr & TCR_EL2_HA) ? TCR_HA : 0);
 }

Should you forward SH0 in this patch as well? It's missing here, so the walks are Non-shareable, and looking at the ARM ARM, this would make every hardware Access flag update CONSTRAINED UNPREDICTABLE (R_WLTNL, and the IsWBShareable() check in AArch64_S1Translate()). An L1 that programmed SH0 as Inner Shareable could then take an unsupported atomic update fault.

That's a good point. However, my reading of the ARM ARM indicates that as long as S2 shareability is Inner-Shareable, the resulting shareability is Inner-Shareable (see R_ZNLRJ), so we're actually safe. This should be fixed though.

Quoted 2 lines

HPD isn't forwarded either, although at.c reads it from TCR_EL2 for an E2H=0 guest. Was that intentional?

Another bug (E2H==0 really doesn't see much love). This needs fixing, but also relies on sanitising TCR_EL2. Thankfully, there is [1] which requires some reviewing... ;-)

Thanks,

	M.

[1] https://lore.kernel.org/r/[email protected]

Signature · 1 line
Without deviation from the norm, progress is not possible.
reviewer
#3 lore raw

Hi Marc,

On Thu, 29 Oct 2026 10:58:54 +0000, Marc Zyngier <[email protected]> wrote:
[...]

Quoted 4 lines

That's a good point. However, my reading of the ARM ARM indicates that as long as S2 shareability is Inner-Shareable, the resulting shareability is Inner-Shareable (see R_ZNLRJ), so we're actually safe. This should be fixed though.

If my reading is correct, I don't think R_ZNLRJ is enough here. AArch64_S1Translate() (J1.2.4.57 in M.d) checks the walk attributes before it translates the update through stage 2:

    // Either the access flag was clear or AP[2]/nDirty was set
    if new_desc != descriptor then
        if (!IsWBShareable(descipaddr.memattrs) &&
            ConstrainUnpredictableBool(
                Unpredictable_Unsupported_Atomic_HW_Update)) then
            fault.statuscode = Fault_UnsupportedAtomicHWUpdate;
            return (fault, ARBITRARY : AddressDescriptor);
        end;
        ...
        if regime == Regime_EL10 && EL2Enabled() then
            ...
            (s2fault, descpaddr) = AArch64_S2Translate(fault,
                    descipaddr, s1aarch64, s2aligned, descaccess);

descipaddr is what AArch64_S1Walk() returns, and it holds the stage 1 walk attributes (walkaddress.memattrs = walkstate.memattrs, with SH0 from TCR_EL1), never combined with stage 2. R_VSXXT also covers an update "not supported in the stage 1 translation due to the defined memory type": that one is a stage 1 abort, taken to EL1.

But it's not like the ARM ARM is an easy read :P

Cheers,
/fuad

author maintainer
#4 lore raw
Quoted 27 lines · Fuad Tabba

Hi Marc,

On Thu, 29 Oct 2026 10:58:54 +0000, Marc Zyngier <[email protected]> wrote:
[...]

Quoted 4 lines

That's a good point. However, my reading of the ARM ARM indicates that as long as S2 shareability is Inner-Shareable, the resulting shareability is Inner-Shareable (see R_ZNLRJ), so we're actually safe. This should be fixed though.

If my reading is correct, I don't think R_ZNLRJ is enough here. AArch64_S1Translate() (J1.2.4.57 in M.d) checks the walk attributes before it translates the update through stage 2:

    // Either the access flag was clear or AP[2]/nDirty was set
    if new_desc != descriptor then
        if (!IsWBShareable(descipaddr.memattrs) &&
            ConstrainUnpredictableBool(
                Unpredictable_Unsupported_Atomic_HW_Update)) then
            fault.statuscode = Fault_UnsupportedAtomicHWUpdate;
            return (fault, ARBITRARY : AddressDescriptor);
        end;
        ...
        if regime == Regime_EL10 && EL2Enabled() then
            ...
            (s2fault, descpaddr) = AArch64_S2Translate(fault,
                    descipaddr, s1aarch64, s2aligned, descaccess);
Quoted 7 lines

descipaddr is what AArch64_S1Walk() returns, and it holds the stage 1 walk attributes (walkaddress.memattrs = walkstate.memattrs, with SH0 from TCR_EL1), never combined with stage 2. R_VSXXT also covers an update "not supported in the stage 1 translation due to the defined memory type": that one is a stage 1 abort, taken to EL1.

But it's not like the ARM ARM is an easy read :P

This is definitely helpful but my understanding is that the pseudocode is *not* authoritative. I think the line "If a translation table hardware update is not atomic as observed by other agents ..." in R_NNNMB gives an implementation enough freedom to decide something Non-Shareable at S1 is actually atomic after the result of S2 translation (e.g. S2FWB).

So if the implementation decides it is atomic, no MMU fault is generated and the update happens like normal. Now, with that being IMP DEF there's a good chance the behavior can vary, like we saw on the output of translation on N3 [*].

I'll add another patch to propagate SH0 and HPD, thanks Fuad.

[*]: c8631ea59b65 ("KVM: arm64: selftests: Explicitly set the page attrs to Inner-Shareable")

Best,
Oliver

reviewer
#5 lore raw

On Thu, 29 Oct 2026 at 18:37, Oliver Upton <[email protected]> wrote:
[...]

Quoted 6 lines

This is definitely helpful but my understanding is that the pseudocode is *not* authoritative. I think the line "If a translation table hardware update is not atomic as observed by other agents ..." in R_NNNMB gives an implementation enough freedom to decide something Non-Shareable at S1 is actually atomic after the result of S2 translation (e.g. S2FWB).

I was under the impression that the pseudocode was more authoritative than the textual description (bugs aside, which the text can also have). Maybe someone at ARM could help settle this? :)

Quoted 6 lines

So if the implementation decides it is atomic, no MMU fault is generated and the update happens like normal. Now, with that being IMP DEF there's a good chance the behavior can vary, like we saw on the output of translation on N3 [*].

I'll add another patch to propagate SH0 and HPD, thanks Fuad.

Thanks!
/fuad