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

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

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.

> 
> > +
> > +	return val;
> > +}
> > +
> >  static u64 sanitise_id_aa64dfr0_el1(const struct kvm_vcpu *vcpu,
> > u64 val)
> >  {
> >  	val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64DFR0_EL1,
> > DebugVer, V8P8);
> > @@ -2221,6 +2236,16 @@ static int set_id_aa64pfr1_el1(struct
> > kvm_vcpu *vcpu,
> >  	return set_id_reg(vcpu, rd, user_val);
> >  }
> >  
> > +static int set_id_aa64pfr2_el1(struct kvm_vcpu *vcpu,
> > +			       const struct sys_reg_desc *rd, u64
> > user_val)
> > +{
> > +	if (vgic_is_v5(vcpu->kvm) &&
> > +	    FIELD_GET(ID_AA64PFR2_EL1_GCIE_MASK, user_val) !=
> > ID_AA64PFR2_EL1_GCIE_IMP)
> > +		return -EINVAL;
> > +
> > +	return set_id_reg(vcpu, rd, user_val);
> > +}
> > +
> >  /*
> >   * Allow userspace to de-feature a stage-2 translation granule but
> > prevent it
> >   * from claiming the impossible.
> > @@ -3202,10 +3227,11 @@ static const struct sys_reg_desc
> > sys_reg_descs[] = {
> >  				       ID_AA64PFR1_EL1_RES0 |
> >  				       ID_AA64PFR1_EL1_MPAM_frac |
> >  				       ID_AA64PFR1_EL1_MTE)),
> > -	ID_WRITABLE(ID_AA64PFR2_EL1,
> > -		    ID_AA64PFR2_EL1_FPMR |
> > -		    ID_AA64PFR2_EL1_MTEFAR |
> > -		    ID_AA64PFR2_EL1_MTESTOREONLY),
> > +	ID_FILTERED(ID_AA64PFR2_EL1, id_aa64pfr2_el1,
> > +		    ~(ID_AA64PFR2_EL1_FPMR |
> > +		      ID_AA64PFR2_EL1_MTEFAR |
> > +		      ID_AA64PFR2_EL1_MTESTOREONLY |
> > +		      ID_AA64PFR2_EL1_GCIE)),
> >  	ID_UNALLOCATED(4,3),
> >  	ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0),
> >  	ID_HIDDEN(ID_AA64SMFR0_EL1),
> 
> Don't you also need something in kvm_finalize_sys_regs() to hide
> GICv5
> altogether if no irqchip has been instantiated?

Ah, I think you're right. I'll make sure we hide GICv5 there if we
don't have an in-kernel irqchip!

>  It'd be worth
> extending the "no-vgic-v3" test to also cover GICv5.

OK, I'm looking into that.

Thanks,
Sascha

> 
> Thanks,
> 
> 	M.
>