On Thu, 2026-03-19 at 10:31 +0000, Jonathan Cameron wrote:

On Tue, 17 Mar 2026 11:42:47 +0000
Sascha Bischoff [off-list ref] wrote:

quoted

Add in a sanitization function for ID_AA64PFR2_EL1, preserving the
already-present behaviour for the FPMR, MTEFAR, and MTESTOREONLY
fields. Add sanitisation for the GCIE field, which is set to IMP if
the host supports a GICv5 guest and NI, otherwise.

Extend the sanitisation that takes place in kvm_vgic_create() to
zero
the ID_AA64PFR2.GCIE field when a non-GICv5 GIC is created. More
importantly, move this sanitisation to a separate function,
kvm_vgic_finalize_sysregs(), and call it from
kvm_finalize_sys_regs().

We are required to finalize the GIC and GCIE fields a second time
in
kvm_finalize_sys_regs() due to how QEMU blindly reads out then
verbatim restores the system register state. This avoids the issue
where both the GCIE and GIC features are marked as present (an
architecturally invalid combination), and hence guests fall over.
See
the comment in kvm_finalize_sys_regs() for more details.

Overall, the following happens:

* Before an irqchip is created, FEAT_GCIE is presented if the host
  supports GICv5-based guests.
* Once an irqchip is created, all other supported irqchips are
hidden
  from the guest; system register state reflects the guest's
irqchip.
* Userspace is allowed to set invalid irqchip feature combinations
in
  the system registers, but...
* ...invalid combinations are removed a second time prior to the
first
  run of the guest, and things hopefully just work.

All of this extra work is required to make sure that "legacy" GICv3
guests based on QEMU transparently work on compatible GICv5 hosts
without modification.

Signed-off-by: Sascha Bischoff <redacted>

Kind of trivial but I'd have split this into a factor out of the
helper
(no functional changes) then the additional stuff.
Meh, it's simple enough to perhaps not be worth the effort.

Anyhow, one comment on what to me looks like a slightly inconsistent
approach
to sanitization. Anyhow, not that important as code is easy enough to
read
and if anything over restricts (which could be relaxed if that ever
becomes
relevant).

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

quoted

---
 arch/arm64/kvm/sys_regs.c       | 70
+++++++++++++++++++++++++++++----
 arch/arm64/kvm/vgic/vgic-init.c | 49 ++++++++++++++++-------
 include/kvm/arm_vgic.h          |  1 +
 3 files changed, 98 insertions(+), 22 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 42c84b7900ff5..140cf35f4eeb4 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c

@@ -1758,6 +1758,7 @@ static u8 pmuver_to_perfmon(u8 pmuver)

 
 static u64 sanitise_id_aa64pfr0_el1(const struct kvm_vcpu *vcpu,
u64 val);
 static u64 sanitise_id_aa64pfr1_el1(const struct kvm_vcpu *vcpu,
u64 val);
+static u64 sanitise_id_aa64pfr2_el1(const struct kvm_vcpu *vcpu,
u64 val);
 static u64 sanitise_id_aa64dfr0_el1(const struct kvm_vcpu *vcpu,
u64 val);
 
 /* Read a sanitised cpufeature ID register by sys_reg_desc */

@@ -1783,10 +1784,7 @@ static u64 __kvm_read_sanitised_id_reg(const

struct kvm_vcpu *vcpu,
 		val = sanitise_id_aa64pfr1_el1(vcpu, val);
 		break;
 	case SYS_ID_AA64PFR2_EL1:
-		val &= ID_AA64PFR2_EL1_FPMR |
-			(kvm_has_mte(vcpu->kvm) ?
-			 ID_AA64PFR2_EL1_MTEFAR |
ID_AA64PFR2_EL1_MTESTOREONLY :
-			 0);
+		val = sanitise_id_aa64pfr2_el1(vcpu, val);
 		break;
 	case SYS_ID_AA64ISAR1_EL1:
 		if (!vcpu_has_ptrauth(vcpu))

@@ -2027,6 +2025,23 @@ static u64 sanitise_id_aa64pfr1_el1(const

struct kvm_vcpu *vcpu, u64 val)
 	return val;
 }
 
+static u64 sanitise_id_aa64pfr2_el1(const struct kvm_vcpu *vcpu,
u64 val)
+{
+	val &= ID_AA64PFR2_EL1_FPMR |
+	       ID_AA64PFR2_EL1_MTEFAR |
+	       ID_AA64PFR2_EL1_MTESTOREONLY;

Style wise this feels inconsistent.  For these 3 registers the
sanitise simply
clears them if not supported, it doesn't enforce particular values
despite
for example MTESTOREONLY only taking values 0 and 1..

quoted

+
+	if (!kvm_has_mte(vcpu->kvm)) {
+		val &= ~ID_AA64PFR2_EL1_MTEFAR;
+		val &= ~ID_AA64PFR2_EL1_MTESTOREONLY;
+	}
+
+	if (vgic_host_has_gicv5())

..but for this one you are forcing a specific value rather than
clearing whatever
was there if !vgic_host_has_gicv5()

Yeah, I think it is a combination of matching the previous behaviour
(which was to let though fields through verbatim) and what is has been
done previously for the GIC field in the ID_AA64PFR0.

I do agree that the styles are different, but I am not sure changing
that will make things clearer. It is good to be explicit about when
FEAT_GCIE is presented as IMP or not, so would prefer to leave that as
it is. The other checks would require something like:

if (val & ID_AA64PFR2_EL1_MTEFAR) {
	val &= ~ID_AA64PFR2_EL1_MTEFAR;
	val |= SYS_FIELD_PREP_ENUM(ID_AA64PFR2_EL1, MTEFAR, IMP);
}

I'm happy to do this if you feel strongly enough, but otherwise would
prefer to leave this as is for now.

Thanks,
Sascha

I don't mind that much though as still obvious what is going on and
perhaps
we do need to be careful this takes only a 1 or 0.

quoted

+		val |= SYS_FIELD_PREP_ENUM(ID_AA64PFR2_EL1, GCIE,
IMP);
+
+	return val;
+}