Re: [PATCH v6 11/39] KVM: arm64: gic-v5: Sanitize ID_AA64PFR2_EL1.GCIE

[Sascha Bischoff <Sascha.Bischoff@arm.com>]

On Thu, 2026-03-19 at 10:31 +0000, Jonathan Cameron wrote:
> On Tue, 17 Mar 2026 11:42:47 +0000
> Sascha Bischoff <Sascha.Bischoff@arm.com> wrote:
> 
> > 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 <sascha.bischoff@arm.com>
> 
> 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>
> 
> 
> > ---
> >  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..
> 
> 
> > +
> > +	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.
> 
> 
> > +		val |= SYS_FIELD_PREP_ENUM(ID_AA64PFR2_EL1, GCIE,
> > IMP);
> > +
> > +	return val;
> > +}
> 
>