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

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

[Fuad Tabba]

From: Marc Zyngier <maz@kernel.org>

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 <broonie@kernel.org>
Link: https://lore.kernel.org/r/20260930-arm64-gcs-v21-2-3556644cd927@kernel.org
Link: https://lore.kernel.org/r/8633u229lq.wl-maz@kernel.org
Cc: stable@vger.kernel.org
Signed-off-by: Marc Zyngier <maz@kernel.org>
Co-developed-by: Fuad Tabba <fuad.tabba@linux.dev>
Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
---
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/20261018080921.3942517-1-fuad.tabba@linux.dev

Changes in v2:
- Also require FEAT_HPDS for S1POE (Marc)
- Link to v1: https://lore.kernel.org/r/20261015153041.3770032-1-fuad.tabba@linux.dev

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(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 1c6632216e327..695c389047c91 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -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);
diff --git a/arch/arm64/kvm/config.c b/arch/arm64/kvm/config.c
index 1053676551aff..d856c0b5c3907 100644
--- a/arch/arm64/kvm/config.c
+++ b/arch/arm64/kvm/config.c
@@ -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;
 };
 
 #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.
+ */
+#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),		\
+	}
+
 #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));
+}
+
+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));
+}
+
 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 |
@@ -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)
+{
+	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]);
+	}
+
+	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;
+
+	return val != kvm->arch.id_regs[r->idreg_idx];
+}
+
+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);
+}
+
 static bool idreg_feat_match(struct kvm *kvm, const struct reg_bits_to_feat_map *map)
 {
 	u64 regval = kvm->arch.id_regs[map->regidx];
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index e9cf6cc340349..f10905347f3a1 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -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
-- 
2.47.3

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

[Lorenzo Stoakes (ARM)]

On Tue, Oct 20, 2026 at 07:43:06AM +0100, Fuad Tabba wrote:
> From: Marc Zyngier <maz@kernel.org>
>
> 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")

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

> Reported-by: Mark Brown <broonie@kernel.org>
> Link: https://lore.kernel.org/r/20260930-arm64-gcs-v21-2-3556644cd927@kernel.org
> Link: https://lore.kernel.org/r/8633u229lq.wl-maz@kernel.org
> Cc: stable@vger.kernel.org
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Co-developed-by: Fuad Tabba <fuad.tabba@linux.dev>
> Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>

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) <ljs@kernel.org>

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?

> ---
> 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/20261018080921.3942517-1-fuad.tabba@linux.dev
>
> Changes in v2:
> - Also require FEAT_HPDS for S1POE (Marc)
> - Link to v1: https://lore.kernel.org/r/20261015153041.3770032-1-fuad.tabba@linux.dev
>
> 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(+)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 1c6632216e327..695c389047c91 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -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);
> diff --git a/arch/arm64/kvm/config.c b/arch/arm64/kvm/config.c
> index 1053676551aff..d856c0b5c3907 100644
> --- a/arch/arm64/kvm/config.c
> +++ b/arch/arm64/kvm/config.c
> @@ -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...

>  };
>
>  #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.
> + */
> +#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].


>  #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));
> +}
> +
> +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 :)

>  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 |
> @@ -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.

> +{
> +	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()?

> +		    !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.


> +	}
> +
> +	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.

> +
> +	return val != kvm->arch.id_regs[r->idreg_idx];
> +}
> +
> +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))
		;
}

> +}
> +
>  static bool idreg_feat_match(struct kvm *kvm, const struct reg_bits_to_feat_map *map)
>  {
>  	u64 regval = kvm->arch.id_regs[map->regidx];
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index e9cf6cc340349..f10905347f3a1 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -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
> --
> 2.47.3
>

--
Cheers, Lorenzo

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

[Fuad Tabba]

Hi Lorenzo,

On Tue, 20 Oct 2026 12:35:14 +0100, "Lorenzo Stoakes (ARM)"
<ljs@kernel.org> wrote:
[...]
>> 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.

[...]
> 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.

[...]
>> diff --git a/arch/arm64/kvm/config.c b/arch/arm64/kvm/config.c
[...]
>> @@ -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.

[...]
>> +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.

[...]
>> +       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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1643 bytes --]

On Tue, Oct 20, 2026 at 07:43:06AM +0100, Fuad Tabba wrote:

> 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.

> +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 <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Lorenzo Stoakes (ARM)]

On Tue, Oct 20, 2026 at 08:30:56PM +0100, Fuad Tabba wrote:
> Hi Lorenzo,
>
> On Tue, 20 Oct 2026 12:35:14 +0100, "Lorenzo Stoakes (ARM)"
> <ljs@kernel.org> wrote:
> [...]
> >> 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.

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

>
> [...]
> > 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.

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

>
> [...]
> >> diff --git a/arch/arm64/kvm/config.c b/arch/arm64/kvm/config.c
> [...]
> >> @@ -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.

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

>
> [...]
> >> +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.
>
> [...]
> >> +       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

Thanks! :)

--
Cheers, Lorenzo

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

[Marc Zyngier]

On Tue, 20 Oct 2026 20:30:56 +0100,
Fuad Tabba <fuad.tabba@linux.dev> wrote:
> 
> >> @@ -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.

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.

> 
> [...]
> >> +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.
> 
> [...]
> >> +       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.

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.

> 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.

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

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

[Marc Zyngier]

On Wed, 21 Oct 2026 14:02:54 +0100,
Mark Brown <broonie@kernel.org> wrote:
> 
> 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.

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 864 bytes --]

On Wed, Oct 21, 2026 at 03:04:51PM +0100, Marc Zyngier wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Lorenzo Stoakes (ARM)]

On Wed, Oct 21, 2026 at 03:03:04PM +0100, Marc Zyngier wrote:
> On Tue, 20 Oct 2026 20:30:56 +0100,
> Fuad Tabba <fuad.tabba@linux.dev> wrote:
> >
> > >> @@ -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.
>
> 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?

> > >> +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?
> > >
> > > 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.

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

>
> > 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