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

[Marc Zyngier <maz@kernel.org>]

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.

> +
> +	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? It'd be worth
extending the "no-vgic-v3" test to also cover GICv5.

Thanks,

	M.

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