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

[Jonathan Cameron <jonathan.cameron@huawei.com>]

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()

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;
> +}