KVM: arm64: Sanitise ID_AA64MMFR3_EL1.S1P[IO]E

PATCH Applied Fuad Tabba 4 participants 11 messages Oct 20 – Oct 26

Subsystems (2)

arm64 KVM arm64

lore

Share & tools

lore

Fuad Tabba

author reviewer

2026-10-20 06:43 UTC #0 lore raw

PATCH v3 1/1 KVM: arm64: Sanitise ID_AA64MMFR3_EL1.S1P[IO]E

From: Marc Zyngier <[email protected]>

KVM uses the VM's ID registers as the source of truth for what the guest has, but userspace can expose S1PIE or S1POE without the features they imply. As per the architecture, both imply TCR2 and ATS1A, and S1POE also implies HPDS. With TCR2 hidden, the world switch skips PIR_EL1, PIRE0_EL1 and POR_EL1 while KVM doesn't trap them, so the guest accesses registers that are neither trapped nor switched. Under VHE, that leaks values between guests. On nVHE and hVHE, pKVM included, the host kernel resumes with the guest's PIR_EL1 and PIRE0_EL1.

On first run, clear S1PIE and S1POE from the VM's ID registers when the features they imply are missing, before anything derives state from them. The guest and userspace then see neither feature, and the guest's accesses to their registers UNDEF.

Fixes 70ed7238297fb ("KVM: arm64: Sanitise ID_AA64MMFR3_EL1")

Fixes d4a89e5aee23e ("KVM: arm64: Expose S1PIE to guests")

Reported-by Mark Brown <[email protected]>

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

Cc [email protected]

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

Co-developed-by Fuad Tabba <[email protected]>

Signed-off-by Fuad Tabba <[email protected]>

---
Changes in v3:
- Sanitise the ID registers instead of changing the trap tables, using Marc's patch (Marc, Sashiko)
- Rebased on kvmarm-7.4
- Link to v2: https://lore.kernel.org/r/[email protected]

Changes in v2:
- Also require FEAT_HPDS for S1POE (Marc)
- Link to v1: https://lore.kernel.org/r/[email protected]

Based on kvmarm-7.4 (dd0bc00e3e920; also applies to kvmarm/fixes 2ea3a929fe01e).

 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/kvm/config.c           | 78 +++++++++++++++++++++++++++++++
 arch/arm64/kvm/sys_regs.c         |  2 +
 3 files changed, 81 insertions(+)

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

index 1c6632216e327..695c389047c91 100644
@@ -1741,6 +1741,7 @@ static inline bool kvm_arch_has_irq_bypass(void)
 	return true;
 }
 
+void kvm_idregs_enforce_dependencies(struct kvm *kvm);
 void compute_fgu(struct kvm *kvm, enum fgt_group_id fgt);
 struct resx get_reg_fixed_bits(struct kvm *kvm, enum vcpu_sysreg reg);
 void check_feature_map(void);

arch/arm64/kvm/config.c +78 -0

114 lines not on this page · raw message

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

index e9cf6cc340349..f10905347f3a1 100644
@@ -5872,6 +5872,8 @@ static int kvm_vm_finalize_sys_regs(struct kvm *kvm)
 	if (kvm_id_regs_final(kvm))
 		return 0;
 
+	kvm_idregs_enforce_dependencies(kvm);
+
 	/*
 	 * This hacks into the ID registers, so only perform it when the
 	 * first vcpu runs, or the kvm_set_vm_id_reg() helper will scream.

base-commit: dd0bc00e3e9201fd49b62ff9cc07890c89003afe

Signature · 1 line

2.47.3

Lorenzo Stoakes (ARM)

Reviewed-by on v3 1/1

2026-10-20 11:35 UTC #1 lore raw

Quoted 11 lines · Fuad Tabba

From: Marc Zyngier <[email protected]>
KVM uses the VM's ID registers as the source of truth for what the guest has, but userspace can expose S1PIE or S1POE without the features they imply. As per the architecture, both imply TCR2 and ATS1A, and S1POE also implies HPDS. With TCR2 hidden, the world switch skips PIR_EL1, PIRE0_EL1 and POR_EL1 while KVM doesn't trap them, so the guest accesses registers that are neither trapped nor switched. Under VHE, that leaks values between guests. On nVHE and hVHE, pKVM included, the host kernel resumes with the guest's PIR_EL1 and PIRE0_EL1.

Quoted 7 lines

On first run, clear S1PIE and S1POE from the VM's ID registers when the features they imply are missing, before anything derives state from them. The guest and userspace then see neither feature, and the guest's accesses to their registers UNDEF.
Fixes 70ed7238297fb ("KVM: arm64: Sanitise ID_AA64MMFR3_EL1")
Fixes d4a89e5aee23e ("KVM: arm64: Expose S1PIE to guests")

I guess kernels that are missing 19868b67e3e7 and 2ce80f624032 will need that stuff ported too?

Quoted 7 lines

Reported-by Mark Brown <[email protected]>
Link https://lore.kernel.org/r/[email protected] open here
Link https://lore.kernel.org/r/[email protected] open here
Cc [email protected]
Signed-off-by Marc Zyngier <[email protected]>
Co-developed-by Fuad Tabba <[email protected]>
Signed-off-by Fuad Tabba <[email protected]>

Thanks for helping this along Fuad, nice to see all of this come together :)

So it seems like this is a much nicer solution to the problem overall - rather than having e.g. kvm_has_s1pie() -> feat_s1pie(), enforce the dependency by applying res0/1 to the id reg in kvm_idregs_enforce_dependencies().

And I see that it uses the guaranteed correct ordering from commit 19868b67e3e7 ("KVM: arm64: Finalize guest-wide sysregs prior to per-vCPU sysregs") also.

Nits below but the logic looks sound to me so:

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

For the GCS series, when Mark respins, I'm guessing it'll be something like:

+static bool feat_gcs(struct kvm *kvm)
+{
+ return kvm_has_feat(kvm, FEAT_GCS) && feat_s1pie(kvm);
+}

...

+static const struct reg_bits_to_feat_map id_aa64pfr1_el1_feat_map[] = {
+ NEEDS_FEAT(ID_AA64PFR1_EL1_GCS, feat_gcs),
+};

...

+static const DECLARE_IDREG_FEAT_MAP(id_aa64pfr1_el1_desc, ID_AA64PFR1_EL1,
+ id_aa64pfr1_el1_feat_map, FEAT_AA64EL1);

(or with my suggestion below, DECLARE_FEAT_MAP())

...

static const struct reg_feat_map_desc *idreg_feat_maps[] = {
	&id_aa64mmfr3_el1_desc,

+ &id_aa64pfr1_el1_desc,
};

Right?

Quoted 36 lines

---
Changes in v3:
- Sanitise the ID registers instead of changing the trap tables, using Marc's patch (Marc, Sashiko)
- Rebased on kvmarm-7.4
- Link to v2: https://lore.kernel.org/r/[email protected]
Changes in v2:
- Also require FEAT_HPDS for S1POE (Marc)
- Link to v1: https://lore.kernel.org/r/[email protected]
Based on kvmarm-7.4 (dd0bc00e3e920; also applies to kvmarm/fixes 2ea3a929fe01e).
 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/kvm/config.c           | 78 +++++++++++++++++++++++++++++++
 arch/arm64/kvm/sys_regs.c         |  2 +
 3 files changed, 81 insertions(+)
arch/arm64/include/asm/kvm_host.h +1 -0
index 1c6632216e327..695c389047c91 100644
@@ -1741,6 +1741,7 @@ static inline bool kvm_arch_has_irq_bypass(void)
 	return true;
 }
 
+void kvm_idregs_enforce_dependencies(struct kvm *kvm);
 void compute_fgu(struct kvm *kvm, enum fgt_group_id fgt);
 struct resx get_reg_fixed_bits(struct kvm *kvm, enum vcpu_sysreg reg);
 void check_feature_map(void);
arch/arm64/kvm/config.c +0 -0
index 1053676551aff..d856c0b5c3907 100644

arch/arm64/kvm/config.c struct reg_feat_map_desc { · line 62

Quoted 4 lines

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

@@ -59,6 +59,7 @@ struct reg_feat_map_desc {
 	const struct reg_bits_to_feat_map feat_map;
 	const struct reg_bits_to_feat_map *bit_feat_map;
 	const unsigned int		  bit_feat_map_sz;
+	const unsigned int		  idreg_idx;

This field feels a bit hazardous - 0 is a valid index and now things that don't declare it are setting a valid value.

That feels like establishing a bit of a footgun for later.

It'd be nice to have something that, when 0, means not present.

Maybe just store the register directly like:

	const u32			reg;

And...

Quoted 12 lines

arch/arm64/kvm/config.c +5 -0

 };
 
 #define __NEEDS_FEAT_3(m, f, w, id, fld, lim)		\

@@ -130,6 +131,21 @@ struct reg_feat_map_desc {
 		.bit_feat_map_sz	= ARRAY_SIZE(m),		\
 	}
 
+/*
+ * Declare the dependency between the features advertised in an id
+ * register, and the set of other features they depend on. This
+ * generates a struct reg_feat_map_desc.
+ */

arch/arm64/kvm/config.c struct reg_feat_map_desc { · line 148

Quoted 10 lines

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

+#define DECLARE_IDREG_FEAT_MAP(n, r, m, f)				\
+	struct reg_feat_map_desc n = {					\
+		.name			= #r,				\
+		.feat_map		= NEEDS_FEAT(~(r##_RES0 |	\
+						       r##_RES1), f),	\
+		.bit_feat_map		= m,				\
+		.bit_feat_map_sz	= ARRAY_SIZE(m),		\
+		.idreg_idx		= IDREG_IDX(SYS_##r),		\
+	}
+

...Here you could avoid the duplication and just update DECLARE_FEAT_MAP() to do:

+ .reg = SYS_##r, \

And when you need to check for the feature you can use is_feature_id_reg(r->reg) [that's static in sys_regs.c and would need to be exported].

Quoted 24 lines

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

 #define FEAT_SPE		ID_AA64DFR0_EL1, PMSVer, IMP
 #define FEAT_BRBE		ID_AA64DFR0_EL1, BRBE, IMP
 #define FEAT_TRC_SR		ID_AA64DFR0_EL1, TraceVer, IMP

@@ -205,6 +221,7 @@ struct reg_feat_map_desc {
 #define FEAT_MEC		ID_AA64MMFR3_EL1, MEC, IMP
 #define FEAT_HAFT		ID_AA64MMFR1_EL1, HAFDBS, HAFT
 #define FEAT_HDBSS		ID_AA64MMFR1_EL1, HAFDBS, HDBSS
+#define FEAT_HPDS		ID_AA64MMFR1_EL1, HPDS, IMP
 #define FEAT_HPDS2		ID_AA64MMFR1_EL1, HPDS, HPDS2
 #define FEAT_BTI		ID_AA64PFR1_EL1, BT, IMP
 #define FEAT_ExS		ID_AA64MMFR0_EL1, EXS, IMP

@@ -280,6 +297,25 @@ static bool feat_anerr(struct kvm *kvm)
 		kvm_has_feat(kvm, ID_AA64MMFR3_EL1, SNERR, FEAT_ANERR));
 }
 
+/*
+ * S1PIE and S1POE imply TCR2 and ATS1A, and S1POE also implies HPDS, but
+ * userspace can set them independently.
+ */
+static bool feat_s1pie(struct kvm *kvm)
+{
+	return (kvm_has_feat(kvm, FEAT_S1PIE) &&
+		kvm_has_feat(kvm, FEAT_TCR2) &&
+		kvm_has_feat(kvm, FEAT_ATS1A));

arch/arm64/kvm/config.c static bool feat_anerr(struct kvm *kvm) · line 318

Quoted 10 lines

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

+}
+
+static bool feat_s1poe(struct kvm *kvm)
+{
+	return (kvm_has_feat(kvm, FEAT_S1POE) &&
+		kvm_has_feat(kvm, FEAT_TCR2) &&
+		kvm_has_feat(kvm, FEAT_ATS1A) &&
+		kvm_has_feat(kvm, FEAT_HPDS));
+}
+

Kinda neat to have a super-readable, clear definition of the dependencies here like this :)

Quoted 19 lines

arch/arm64/kvm/config.c +8 -0

 static bool feat_sme_smps(struct kvm *kvm)
 {
 	/*

@@ -1299,6 +1335,14 @@ static const struct reg_bits_to_feat_map vtcr_el2_feat_map[] = {
 static const DECLARE_FEAT_MAP(vtcr_el2_desc, VTCR_EL2,
 			      vtcr_el2_feat_map, FEAT_AA64EL2);
 
+static const struct reg_bits_to_feat_map id_aa64mmfr3_el1_feat_map[] = {
+	NEEDS_FEAT(ID_AA64MMFR3_EL1_S1PIE, feat_s1pie),
+	NEEDS_FEAT(ID_AA64MMFR3_EL1_S1POE, feat_s1poe),
+};
+
+static const DECLARE_IDREG_FEAT_MAP(id_aa64mmfr3_el1_desc, ID_AA64MMFR3_EL1,
+				    id_aa64mmfr3_el1_feat_map, FEAT_AA64EL1);
+
 static const struct reg_bits_to_feat_map ich_hfgrtr_feat_map[] = {
 	NEEDS_FEAT(ICH_HFGRTR_EL2_ICC_APR_EL1 |
 		   ICH_HFGRTR_EL2_ICC_IDRn_EL1 |

arch/arm64/kvm/config.c void __init check_feature_map(void) · line 1459

Quoted 8 lines

arch/arm64/kvm/config.c +5 -0

@@ -1408,6 +1452,40 @@ void __init check_feature_map(void)
 	check_reg_desc(&ich_hfgitr_desc);
 }
 
+static const struct reg_feat_map_desc *idreg_feat_maps[] = {
+	&id_aa64mmfr3_el1_desc,
+};
+
+static bool idreg_deps(struct kvm *kvm, const struct reg_feat_map_desc *r)

To me this reads confusingly - idreg_apply_deps() would be clearer I think.

arch/arm64/kvm/config.c void __init check_feature_map(void) · line 1465

Quoted 6 lines

arch/arm64/kvm/config.c +6 -0

+{
+	struct resx resx = {};
+	u64 val;
+
+	for (int i = 0; i < r->bit_feat_map_sz; i++) {
+		if ((r->bit_feat_map[i].flags & CALL_FUNC) &&

What if it isn't a CALL_FUNC feature?

Could this loop just be replaced with compute_resx_bits()?

arch/arm64/kvm/config.c void __init check_feature_map(void) · line 1467

Quoted 2 lines

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

+		    !r->bit_feat_map[i].match(kvm))
+			resx.res0 |= reg_feat_map_bits(&r->bit_feat_map[i]);

OK, neat, this e.g. invokes feat_s1pie, feat_s1poe for aa64mmfr3_el1.

arch/arm64/kvm/config.c void __init check_feature_map(void) · line 1472

Quoted 5 lines

arch/arm64/kvm/config.c +5 -0

+	}
+
+	val = kvm->arch.id_regs[r->idreg_idx];
+	kvm->arch.id_regs[r->idreg_idx] &= ~resx.res0;
+	kvm->arch.id_regs[r->idreg_idx] |= resx.res1;

I see from the previous review that res1 is for future proofing, but redundant (but harmless) atm.

Quoted 4 lines

arch/arm64/kvm/config.c +4 -0

+
+	return val != kvm->arch.id_regs[r->idreg_idx];
+}
+

arch/arm64/kvm/config.c void __init check_feature_map(void) · line 1486

Quoted 10 lines

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

+void kvm_idregs_enforce_dependencies(struct kvm *kvm)
+{
+	bool again;
+
+	do {
+		again = false;
+
+		for (int i = 0; i < ARRAY_SIZE(idreg_feat_maps); i++)
+			again |= idreg_deps(kvm, idreg_feat_maps[i]);
+	} while (again);

This is pretty confusing at first glance, so - iterate through all idreg_feat_maps, if any changes happened, then loop again.

Could it be e.g.:

static bool idreg_apply_all_deps(struct kvm *kvm)

{
	bool changed = false;

	for (int i = 0; i < ARRAY_SIZE(idreg_feat_maps); i++)
		changed |= idreg_apply_deps(kvm, idreg_feat_maps[i]);
	return changed;
}

void kvm_idregs_enforce_dependencies(struct kvm *kvm)

{
	/* Propagate chained dependencies. */
	while (idreg_apply_all_deps(kvm))
		;
}

Quoted 19 lines

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

+}
+
 static bool idreg_feat_match(struct kvm *kvm, const struct reg_bits_to_feat_map *map)
 {
 	u64 regval = kvm->arch.id_regs[map->regidx];

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

index e9cf6cc340349..f10905347f3a1 100644
@@ -5872,6 +5872,8 @@ static int kvm_vm_finalize_sys_regs(struct kvm *kvm)
 	if (kvm_id_regs_final(kvm))
 		return 0;
 
+	kvm_idregs_enforce_dependencies(kvm);
+
 	/*
 	 * This hacks into the ID registers, so only perform it when the
 	 * first vcpu runs, or the kvm_set_vm_id_reg() helper will scream.

Quoted 4 lines

base-commit: dd0bc00e3e9201fd49b62ff9cc07890c89003afe --
2.47.3

--
Cheers, Lorenzo

Fuad Tabba

author reviewer

2026-10-20 19:31 UTC #2 lore raw

Hi Lorenzo,

On Tue, 20 Oct 2026 12:35:14 +0100, "Lorenzo Stoakes (ARM)" <[email protected]> wrote:
[...]

Quoted 5 lines

Quoted 2 lines
Fixes 70ed7238297fb ("KVM: arm64: Sanitise ID_AA64MMFR3_EL1")
Fixes d4a89e5aee23e ("KVM: arm64: Expose S1PIE to guests")
I guess kernels that are missing 19868b67e3e7 and 2ce80f624032 will need that stuff ported too?

I think 2ce80f624032 would have to go back too, since without it a VMM can still write the ID registers after they've been sanitised. 19868b67e3e7 mostly provides the call site: the version I tested on 7.3-rc3 called kvm_idregs_enforce_dependencies() at the top of kvm_finalize_sys_regs() instead. Kernels before 6.16 don't have config.c, so the backport there would be a different patch. I'd leave the stable side to Marc though.

[...]

Quoted 6 lines

static const struct reg_feat_map_desc *idreg_feat_maps[] = {
        &id_aa64mmfr3_el1_desc,

+ &id_aa64pfr1_el1_desc,
};

Right?

That's how I'd expect it to look. Since kvm_idregs_enforce_dependencies() repeats until nothing changes, feat_gcs() could also test FEAT_S1PIE directly: by then, S1PIE has been cleared if its own dependencies are missing.

[...]

Quoted 1 line

Quoted 1 line
arch/arm64/kvm/config.c +0 -0

[...]

Quoted 8 lines

arch/arm64/kvm/config.c struct reg_feat_map_desc { · line 62
Quoted 5 lines
arch/arm64/kvm/config.c +1 -0
@@ -59,6 +59,7 @@ struct reg_feat_map_desc {
        const struct reg_bits_to_feat_map feat_map;
        const struct reg_bits_to_feat_map *bit_feat_map;
        const unsigned int                bit_feat_map_sz;
+       const unsigned int                idreg_idx;
This field feels a bit hazardous - 0 is a valid index and now things that don't declare it are setting a valid value.

idreg_idx is only read for the descriptors in idreg_feat_maps[], which all come from DECLARE_IDREG_FEAT_MAP(), so the 0 in the others is never used. Storing the encoding would work as well, I'll leave that one to Marc.

[...]

Quoted 3 lines

arch/arm64/kvm/config.c void __init check_feature_map(void) · line 1459
Quoted 1 line
arch/arm64/kvm/config.c +1 -0
+static bool idreg_deps(struct kvm *kvm, const struct reg_feat_map_desc *r)
To me this reads confusingly - idreg_apply_deps() would be clearer I think.

Agreed.

[...]

Quoted 6 lines

arch/arm64/kvm/config.c void __init check_feature_map(void) · line 1465
Quoted 2 lines
arch/arm64/kvm/config.c +2 -0
+       for (int i = 0; i < r->bit_feat_map_sz; i++) {
+               if ((r->bit_feat_map[i].flags & CALL_FUNC) &&
What if it isn't a CALL_FUNC feature?
Could this loop just be replaced with compute_resx_bits()?

It's skipped, so a dependency written as a plain field match would be silently ignored. Both entries today are CALL_FUNC, but compute_resx_bits() handles every kind of entry and would also give the res1 line something to set. It'd mean moving idreg_deps() below it.

Marc, compute_resx_bits(), the rename and the loop rework are all fine by me. If you agree, I can respin with them, or you can fold them in when applying, whichever is easier for you.

Thanks,
/fuad

Lorenzo Stoakes (ARM)

2026-10-21 13:28 UTC #3 lore raw

Quoted 11 lines · Fuad Tabba

Hi Lorenzo,
On Tue, 20 Oct 2026 12:35:14 +0100, "Lorenzo Stoakes (ARM)" <[email protected]> wrote:
[...]
Quoted 5 lines
Quoted 2 lines
Fixes 70ed7238297fb ("KVM: arm64: Sanitise ID_AA64MMFR3_EL1")
Fixes d4a89e5aee23e ("KVM: arm64: Expose S1PIE to guests")
I guess kernels that are missing 19868b67e3e7 and 2ce80f624032 will need that stuff ported too?

Quoted 7 lines

I think 2ce80f624032 would have to go back too, since without it a VMM can still write the ID registers after they've been sanitised. 19868b67e3e7 mostly provides the call site: the version I tested on 7.3-rc3 called kvm_idregs_enforce_dependencies() at the top of kvm_finalize_sys_regs() instead. Kernels before 6.16 don't have config.c, so the backport there would be a different patch. I'd leave the stable side to Marc though.

Ack :) I guess this is mostly 'stable looks painful' :)

Quoted 7 lines

[...]
Quoted 5 lines
static const struct reg_feat_map_desc *idreg_feat_maps[] = {
        &id_aa64mmfr3_el1_desc,
+ &id_aa64pfr1_el1_desc,
};

Quoted 6 lines

Quoted 1 line
Right?
That's how I'd expect it to look. Since kvm_idregs_enforce_dependencies() repeats until nothing changes, feat_gcs() could also test FEAT_S1PIE directly: by then, S1PIE has been cleared if its own dependencies are missing.

Great, so that clears I think the really main outstanding question for that series. Really nice! :)

Quoted 13 lines

[...]
Quoted 1 line
Quoted 1 line
arch/arm64/kvm/config.c +0 -0
[...]
Quoted 8 lines
arch/arm64/kvm/config.c struct reg_feat_map_desc { · line 62
Quoted 5 lines
arch/arm64/kvm/config.c +1 -0
@@ -59,6 +59,7 @@ struct reg_feat_map_desc {
        const struct reg_bits_to_feat_map feat_map;
        const struct reg_bits_to_feat_map *bit_feat_map;
        const unsigned int                bit_feat_map_sz;
+       const unsigned int                idreg_idx;
This field feels a bit hazardous - 0 is a valid index and now things that don't declare it are setting a valid value.

Quoted 4 lines

idreg_idx is only read for the descriptors in idreg_feat_maps[], which all come from DECLARE_IDREG_FEAT_MAP(), so the 0 in the others is never used. Storing the encoding would work as well, I'll leave that one to Marc.

Yeah not used, but you're defaulting to a valid value, so it's got real footgun potential.

Quoted 22 lines

[...]
Quoted 3 lines
Quoted 1 line
+static bool idreg_deps(struct kvm *kvm, const struct reg_feat_map_desc *r)
To me this reads confusingly - idreg_apply_deps() would be clearer I think.
Agreed.
[...]
Quoted 6 lines
arch/arm64/kvm/config.c void __init check_feature_map(void) · line 1465
Quoted 2 lines
arch/arm64/kvm/config.c +2 -0
+       for (int i = 0; i < r->bit_feat_map_sz; i++) {
+               if ((r->bit_feat_map[i].flags & CALL_FUNC) &&
What if it isn't a CALL_FUNC feature?
Could this loop just be replaced with compute_resx_bits()?
It's skipped, so a dependency written as a plain field match would be silently ignored. Both entries today are CALL_FUNC, but compute_resx_bits() handles every kind of entry and would also give the res1 line something to set. It'd mean moving idreg_deps() below it.

Quoted 6 lines

Marc, compute_resx_bits(), the rename and the loop rework are all fine by me. If you agree, I can respin with them, or you can fold them in when applying, whichever is easier for you.
Thanks,
/fuad

Thanks! :)

--
Cheers, Lorenzo

Marc Zyngier

maintainer

2026-10-21 14:03 UTC #4 lore raw

In reply to Fuad Tabba

Quoted 10 lines · Fuad Tabba

Quoted 9 lines
arch/arm64/kvm/config.c struct reg_feat_map_desc { · line 62
Quoted 6 lines
arch/arm64/kvm/config.c +1 -0
@@ -59,6 +59,7 @@ struct reg_feat_map_desc {
        const struct reg_bits_to_feat_map feat_map;
        const struct reg_bits_to_feat_map *bit_feat_map;
        const unsigned int                bit_feat_map_sz;
+       const unsigned int                idreg_idx;
This field feels a bit hazardous - 0 is a valid index and now things that don't declare it are setting a valid value.

Quoted 4 lines

idreg_idx is only read for the descriptors in idreg_feat_maps[], which all come from DECLARE_IDREG_FEAT_MAP(), so the 0 in the others is never used. Storing the encoding would work as well, I'll leave that one to Marc.

I don't think we should baby-sit people who hack this code. If you put something that is not the output of IDREG_IDX(), you get to keep the pieces. If you use this out of context, it will blow up in your face.

Quoted 14 lines

[...]
Quoted 3 lines
Quoted 1 line
+static bool idreg_deps(struct kvm *kvm, const struct reg_feat_map_desc *r)
To me this reads confusingly - idreg_apply_deps() would be clearer I think.
Agreed.
[...]
Quoted 5 lines
arch/arm64/kvm/config.c void __init check_feature_map(void) · line 1465
Quoted 2 lines
arch/arm64/kvm/config.c +2 -0
+       for (int i = 0; i < r->bit_feat_map_sz; i++) {
+               if ((r->bit_feat_map[i].flags & CALL_FUNC) &&
What if it isn't a CALL_FUNC feature?

Quoted 7 lines

Quoted 1 line
Could this loop just be replaced with compute_resx_bits()?
It's skipped, so a dependency written as a plain field match would be silently ignored. Both entries today are CALL_FUNC, but compute_resx_bits() handles every kind of entry and would also give the res1 line something to set. It'd mean moving idreg_deps() below it.

I really do not want to drag compute_resx_bits() into this. None of that stuff is RESx. It is bad enough that we use the same structures to represent different type of dependencies. I also want to add new ways to affect ID registers that really do not fit the RESx handling.

Quoted 3 lines

Marc, compute_resx_bits(), the rename and the loop rework are all fine by me. If you agree, I can respin with them, or you can fold them in when applying, whichever is easier for you.

I don't care about the name, but I don't want the rest of the proposed changes.

Thanks,

M.

Signature · 1 line

Without deviation from the norm, progress is not possible.

Lorenzo Stoakes (ARM)

2026-10-21 15:01 UTC #5 lore raw

Quoted 12 lines · Marc Zyngier

Quoted 10 lines · Fuad Tabba
Quoted 9 lines
arch/arm64/kvm/config.c struct reg_feat_map_desc { · line 62
Quoted 6 lines
arch/arm64/kvm/config.c +1 -0
@@ -59,6 +59,7 @@ struct reg_feat_map_desc {
        const struct reg_bits_to_feat_map feat_map;
        const struct reg_bits_to_feat_map *bit_feat_map;
        const unsigned int                bit_feat_map_sz;
+       const unsigned int                idreg_idx;
This field feels a bit hazardous - 0 is a valid index and now things that don't declare it are setting a valid value.

Quoted 8 lines

Quoted 4 lines
idreg_idx is only read for the descriptors in idreg_feat_maps[], which all come from DECLARE_IDREG_FEAT_MAP(), so the 0 in the others is never used. Storing the encoding would work as well, I'll leave that one to Marc.
I don't think we should baby-sit people who hack this code. If you put something that is not the output of IDREG_IDX(), you get to keep the pieces. If you use this out of context, it will blow up in your face.

That's fair enough, I did worry a bit about identifying the idreg variant vs. not from a reg_feat_map_desc instance though?

Quoted 14 lines

Quoted 6 lines
Quoted 6 lines
arch/arm64/kvm/config.c void __init check_feature_map(void) · line 1465
Quoted 3 lines
arch/arm64/kvm/config.c +3 -0
+static bool idreg_deps(struct kvm *kvm, const struct reg_feat_map_desc *r)
+       for (int i = 0; i < r->bit_feat_map_sz; i++) {
+               if ((r->bit_feat_map[i].flags & CALL_FUNC) &&
What if it isn't a CALL_FUNC feature?
Quoted 7 lines
Quoted 1 line
Could this loop just be replaced with compute_resx_bits()?
It's skipped, so a dependency written as a plain field match would be silently ignored. Both entries today are CALL_FUNC, but compute_resx_bits() handles every kind of entry and would also give the res1 line something to set. It'd mean moving idreg_deps() below it.

Quoted 4 lines

I really do not want to drag compute_resx_bits() into this. None of that stuff is RESx. It is bad enough that we use the same structures to represent different type of dependencies. I also want to add new ways to affect ID registers that really do not fit the RESx handling.

I wonder if it's worth breaking out the resx stuff into something separate that both call? As a follow-up obviously.

Quoted 7 lines

Quoted 4 lines
Marc, compute_resx_bits(), the rename and the loop rework are all fine by me. If you agree, I can respin with them, or you can fold them in when applying, whichever is easier for you.
I don't care about the name, but I don't want the rest of the proposed changes.

Fine with me ;) the logic all looks fine so these were nits.

As you are indifferent on name, do you mind having the two functions one calling another? That is a lot clearer to my eyes. But also not a showstopper, obviously!

--
Cheers, Lorenzo

Fuad Tabba

author reviewer

2026-10-21 18:38 UTC #6 lore raw

Hi Marc,

On Wed, 21 Oct 2026 15:03:04 +0100, Marc Zyngier <[email protected]> wrote:
[...]

Quoted 2 lines

I don't care about the name, but I don't want the rest of the proposed changes.

Let's leave the rest as it is. For the name, would you rename it when applying, or would you rather I respin?

Thanks,
/fuad

Mark Brown

Reviewed-by on v3 1/1

2026-10-21 13:42 UTC #7 lore raw

In reply to Fuad Tabba

Quoted 4 lines · Fuad Tabba

On first run, clear S1PIE and S1POE from the VM's ID registers when the features they imply are missing, before anything derives state from them. The guest and userspace then see neither feature, and the guest's accesses to their registers UNDEF.

Updating the ID registers should ensure we don't have any confusion in the later KVM code due to the standard use of ID register based checks. I didn't spot any further registers where we have nested checks that would need similar updates.

arch/arm64/kvm/config.c void __init check_feature_map(void) · line 1468

Quoted 10 lines

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

+static bool idreg_deps(struct kvm *kvm, const struct reg_feat_map_desc *r)
+{
+	struct resx resx = {};
+	u64 val;
+
+	for (int i = 0; i < r->bit_feat_map_sz; i++) {
+		if ((r->bit_feat_map[i].flags & CALL_FUNC) &&
+		    !r->bit_feat_map[i].match(kvm))
+			resx.res0 |= reg_feat_map_bits(&r->bit_feat_map[i]);
+	}

It would be nice to have it be more obvious to the VMM and/or it's users that it messed up, as things stand features it thought it had set will just silently vanish from the guest - they'd need to know that this is something that might happen and check after finalization or similar. That's probably more a documentation/education thing than anything for the patch.

We might want to support limits other than 0 at some point, but we can deal with that if it becomes an actual issue and it's likely we would just decide that the VMM shouldn't be configuring invalid things in the first place so we don't care about being helpful.

Lorenzo raised all the more detailed code things I had, but either way:

Reviewed-by Mark Brown <[email protected]>

Marc Zyngier

maintainer

2026-10-21 14:04 UTC #8 lore raw

Quoted 7 lines · Mark Brown

It would be nice to have it be more obvious to the VMM and/or it's users that it messed up, as things stand features it thought it had set will just silently vanish from the guest - they'd need to know that this is something that might happen and check after finalization or similar. That's probably more a documentation/education thing than anything for the patch.

The VMM is an integral part of implementing the architecture. If the VMM doesn't cooperate, that's its problem.

M.

Signature · 1 line

Without deviation from the norm, progress is not possible.

Mark Brown

2026-10-21 14:19 UTC #9 lore raw

Quoted 11 lines · Marc Zyngier

Quoted 6 lines · Mark Brown
It would be nice to have it be more obvious to the VMM and/or it's users that it messed up, as things stand features it thought it had set will just silently vanish from the guest - they'd need to know that this is something that might happen and check after finalization or similar. That's probably more a documentation/education thing than anything for the patch.
The VMM is an integral part of implementing the architecture. If the VMM doesn't cooperate, that's its problem.

Absolutely, it's a bug if the VMM is doing this. What I'm saying is that it could be helpful if the bug was made visible, with the silent fixup VMM bugs could easily lurk around for longer before someone notices the problem than they might otherwise.

Marc Zyngier

maintainer Applied

2026-10-26 12:01 UTC #10 lore raw

In reply to Fuad Tabba

Quoted 10 lines · Fuad Tabba

KVM uses the VM's ID registers as the source of truth for what the guest has, but userspace can expose S1PIE or S1POE without the features they imply. As per the architecture, both imply TCR2 and ATS1A, and S1POE also implies HPDS. With TCR2 hidden, the world switch skips PIR_EL1, PIRE0_EL1 and POR_EL1 while KVM doesn't trap them, so the guest accesses registers that are neither trapped nor switched. Under VHE, that leaks values between guests. On nVHE and hVHE, pKVM included, the host kernel resumes with the guest's PIR_EL1 and PIRE0_EL1.
[...]

Applied to fixes, thanks!

[1/1] KVM: arm64: Sanitise ID_AA64MMFR3_EL1.S1P[IO]E

      commit: 68c11eed4f8097ef1b71c30679e96eb654df29e0

Cheers,

M.

Signature · 1 line

Without deviation from the norm, progress is not possible.