Re: [PATCH 02/32] KVM: arm64: gic-v3: Switch vGIC-v3 to use generated ICH_VMCR_EL2

[Marc Zyngier <maz@kernel.org>]

On Fri, 12 Dec 2025 15:22:35 +0000,
Sascha Bischoff <Sascha.Bischoff@arm.com> wrote:
> 
> From: Sascha Bischoff <Sascha.Bischoff@arm.com>
> 
> The VGIC-v3 code relied on hand-written definitions for the
> ICH_VMCR_EL2 register. This register, and the associated fields, is
> now generated as part of the sysreg framework. Move to using the
> generated definitions instead of the hand-written ones.
> 
> There are no functional changes as part of this change.
> 
> Signed-off-by: Sascha Bischoff <sascha.bischoff@arm.com>
> ---
>  arch/arm64/include/asm/sysreg.h      | 21 ---------
>  arch/arm64/kvm/hyp/vgic-v3-sr.c      | 64 ++++++++++++----------------
>  arch/arm64/kvm/vgic/vgic-v3-nested.c |  8 ++--
>  arch/arm64/kvm/vgic/vgic-v3.c        | 48 ++++++++++-----------
>  4 files changed, 54 insertions(+), 87 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 9df51accbb025..b3b8b8cd7bf1e 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -560,7 +560,6 @@
>  #define SYS_ICC_SRE_EL2			sys_reg(3, 4, 12, 9, 5)
>  #define SYS_ICH_EISR_EL2		sys_reg(3, 4, 12, 11, 3)
>  #define SYS_ICH_ELRSR_EL2		sys_reg(3, 4, 12, 11, 5)
> -#define SYS_ICH_VMCR_EL2		sys_reg(3, 4, 12, 11, 7)
>  
>  #define __SYS__LR0_EL2(x)		sys_reg(3, 4, 12, 12, x)
>  #define SYS_ICH_LR0_EL2			__SYS__LR0_EL2(0)
> @@ -988,26 +987,6 @@
>  #define ICH_LR_PRIORITY_SHIFT	48
>  #define ICH_LR_PRIORITY_MASK	(0xffULL << ICH_LR_PRIORITY_SHIFT)
>  
> -/* ICH_VMCR_EL2 bit definitions */
> -#define ICH_VMCR_ACK_CTL_SHIFT	2
> -#define ICH_VMCR_ACK_CTL_MASK	(1 << ICH_VMCR_ACK_CTL_SHIFT)
> -#define ICH_VMCR_FIQ_EN_SHIFT	3
> -#define ICH_VMCR_FIQ_EN_MASK	(1 << ICH_VMCR_FIQ_EN_SHIFT)
> -#define ICH_VMCR_CBPR_SHIFT	4
> -#define ICH_VMCR_CBPR_MASK	(1 << ICH_VMCR_CBPR_SHIFT)
> -#define ICH_VMCR_EOIM_SHIFT	9
> -#define ICH_VMCR_EOIM_MASK	(1 << ICH_VMCR_EOIM_SHIFT)
> -#define ICH_VMCR_BPR1_SHIFT	18
> -#define ICH_VMCR_BPR1_MASK	(7 << ICH_VMCR_BPR1_SHIFT)
> -#define ICH_VMCR_BPR0_SHIFT	21
> -#define ICH_VMCR_BPR0_MASK	(7 << ICH_VMCR_BPR0_SHIFT)
> -#define ICH_VMCR_PMR_SHIFT	24
> -#define ICH_VMCR_PMR_MASK	(0xffUL << ICH_VMCR_PMR_SHIFT)
> -#define ICH_VMCR_ENG0_SHIFT	0
> -#define ICH_VMCR_ENG0_MASK	(1 << ICH_VMCR_ENG0_SHIFT)
> -#define ICH_VMCR_ENG1_SHIFT	1
> -#define ICH_VMCR_ENG1_MASK	(1 << ICH_VMCR_ENG1_SHIFT)
> -
>  /*
>   * Permission Indirection Extension (PIE) permission encodings.
>   * Encodings with the _O suffix, have overlays applied (Permission Overlay Extension).
> diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c
> index 0b670a033fd87..24a2074f3a8cf 100644
> --- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
> +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
> @@ -569,11 +569,11 @@ static int __vgic_v3_highest_priority_lr(struct kvm_vcpu *vcpu, u32 vmcr,
>  			continue;
>  
>  		/* Group-0 interrupt, but Group-0 disabled? */
> -		if (!(val & ICH_LR_GROUP) && !(vmcr & ICH_VMCR_ENG0_MASK))
> +		if (!(val & ICH_LR_GROUP) && !(vmcr & ICH_VMCR_EL2_VENG0_MASK))
>  			continue;
>  
>  		/* Group-1 interrupt, but Group-1 disabled? */
> -		if ((val & ICH_LR_GROUP) && !(vmcr & ICH_VMCR_ENG1_MASK))
> +		if ((val & ICH_LR_GROUP) && !(vmcr & ICH_VMCR_EL2_VENG1_MASK))
>  			continue;
>  
>  		/* Not the highest priority? */
> @@ -646,19 +646,19 @@ static int __vgic_v3_get_highest_active_priority(void)
>  
>  static unsigned int __vgic_v3_get_bpr0(u32 vmcr)
>  {
> -	return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
> +	return FIELD_GET(ICH_VMCR_EL2_VBPR0, vmcr);
>  }
>  
>  static unsigned int __vgic_v3_get_bpr1(u32 vmcr)
>  {
>  	unsigned int bpr;
>  
> -	if (vmcr & ICH_VMCR_CBPR_MASK) {
> +	if (vmcr & ICH_VMCR_EL2_VCBPR_MASK) {
>  		bpr = __vgic_v3_get_bpr0(vmcr);
>  		if (bpr < 7)
>  			bpr++;
>  	} else {
> -		bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
> +		bpr = FIELD_GET(ICH_VMCR_EL2_VBPR1, vmcr);
>  	}
>  
>  	return bpr;
> @@ -758,7 +758,7 @@ static void __vgic_v3_read_iar(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
>  	if (grp != !!(lr_val & ICH_LR_GROUP))
>  		goto spurious;
>  
> -	pmr = (vmcr & ICH_VMCR_PMR_MASK) >> ICH_VMCR_PMR_SHIFT;
> +	pmr = FIELD_GET(ICH_VMCR_EL2_VPMR, vmcr);
>  	lr_prio = (lr_val & ICH_LR_PRIORITY_MASK) >> ICH_LR_PRIORITY_SHIFT;
>  	if (pmr <= lr_prio)
>  		goto spurious;
> @@ -806,7 +806,7 @@ static int ___vgic_v3_write_dir(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
>  	int lr;
>  
>  	/* EOImode == 0, nothing to be done here */
> -	if (!(vmcr & ICH_VMCR_EOIM_MASK))
> +	if (!FIELD_GET(ICH_VMCR_EL2_VEOIM_MASK, vmcr))

nit: FIELD_GET() doesn't bring anything here. Similar comment applies
to most 'if (val & MASK)' constructs that get changed here.

>  		return 1;
>  
>  	/* No deactivate to be performed on an LPI */
> @@ -849,7 +849,7 @@ static void __vgic_v3_write_eoir(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
>  	}
>  
>  	/* EOImode == 1 and not an LPI, nothing to be done here */
> -	if ((vmcr & ICH_VMCR_EOIM_MASK) && !(vid >= VGIC_MIN_LPI))
> +	if (FIELD_GET(ICH_VMCR_EL2_VEOIM_MASK, vmcr) && !(vid >= VGIC_MIN_LPI))
>  		return;
>  
>  	lr_prio = (lr_val & ICH_LR_PRIORITY_MASK) >> ICH_LR_PRIORITY_SHIFT;
> @@ -865,12 +865,12 @@ static void __vgic_v3_write_eoir(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
>  
>  static void __vgic_v3_read_igrpen0(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
>  {
> -	vcpu_set_reg(vcpu, rt, !!(vmcr & ICH_VMCR_ENG0_MASK));
> +	vcpu_set_reg(vcpu, rt, !!FIELD_GET(ICH_VMCR_EL2_VENG0_MASK, vmcr));

Here, !! is actually really superfluous and makes it harder to
understand what is being done. Similar thing for IRGPEN1.

Apart from these two points, this looks OK to me.

Thanks,

	M.

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