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

[Marc Zyngier <maz@kernel.org>]

On Fri, 30 Jan 2026 17:13:18 +0000,
Sascha Bischoff <Sascha.Bischoff@arm.com> wrote:
> 
> On Fri, 2026-01-30 at 11:38 +0000, Marc Zyngier wrote:
> > On Wed, 28 Jan 2026 18:02:09 +0000,
> > Sascha Bischoff <Sascha.Bischoff@arm.com> wrote:
> > > 
> > > Set the guest's view of the GCIE field to IMP when running a GICv5
> > > VM,
> > > NI otherwise. Reject any writes to the register that try to do
> > > anything but set GCIE to IMP when running a GICv5 VM.
> > > 
> > > As part of this change, we're also required to extend
> > > vgic_is_v3_compat() to check for the actual vgic_model. This has
> > > one
> > > potential issue - if any of the vgic_is_v*() checks are used prior
> > > to
> > > setting the vgic_model (that is, before kvm_vgic_create) then
> > > vgic_model will be set to 0, which can result in a false-positive.
> > > 
> > > Co-authored-by: Timothy Hayes <timothy.hayes@arm.com>
> > > Signed-off-by: Timothy Hayes <timothy.hayes@arm.com>
> > > Signed-off-by: Sascha Bischoff <sascha.bischoff@arm.com>
> > > Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> > > ---
> > >  arch/arm64/kvm/sys_regs.c  | 42 ++++++++++++++++++++++++++++++----
> > > ----
> > >  arch/arm64/kvm/vgic/vgic.h | 10 ++++++++-
> > >  2 files changed, 43 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > > index 88a57ca36d96..73dd2bd85c4f 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))
> > > @@ -2024,6 +2022,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;
> > > +
> > > +	if (!kvm_has_mte(vcpu->kvm)) {
> > > +		val &= ~ID_AA64PFR2_EL1_MTEFAR;
> > > +		val &= ~ID_AA64PFR2_EL1_MTESTOREONLY;
> > > +	}
> > > +
> > > +	if (vgic_is_v5(vcpu->kvm))
> > > +		val |= SYS_FIELD_PREP_ENUM(ID_AA64PFR2_EL1, GCIE,
> > > IMP);
> > 
> > You probably want to clear the field before or'ing something in, or
> > you may be promising more than we'd expect.
> 
> The GCIE field should already be zeroed at this point as it is filtered
> out to begin with. If we have GICv5 (so FEAT_GCIE) we're explicitly
> setting this field to IMP, else NI.

As you can tell by now, I can't read. Apologies for the noise.

Thanks,

	M.

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