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

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

On Fri, 19 Dec 2025 15:52:36 +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>
Hi Sascha

Happy new year.  There is a bit in here that isn't obviously going
to result in no functional change. I'm too lazy to chase where the value
goes to check it it's a real bug or not.

Otherwise this is inconsistent on whether the _MASK or define without
it from the sysreg generated header is used in FIELD_GET() / FIELD_PREP()

I'd always use the _MASK version.

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


> @@ -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, vmcr & ICH_VMCR_EL2_VENG0_MASK);
>  }
>  
>  static void __vgic_v3_read_igrpen1(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
>  {
> -	vcpu_set_reg(vcpu, rt, !!(vmcr & ICH_VMCR_ENG1_MASK));
> +	vcpu_set_reg(vcpu, rt, vmcr & ICH_VMCR_EL2_VENG1_MASK);

It's more than possible it doesn't matter, but this isn't functionally
equivalent.
The original set passed 1 as the val parameter to vcpu_set_reg(), and now it passes 2.

Given these don't take a bool I'd use FIELD_GET() for both this and the veng0 one above.
Or put back the horrible !!

>  }

> @@ -916,10 +916,8 @@ static void __vgic_v3_write_bpr0(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
>  	if (val < bpr_min)
>  		val = bpr_min;
>  
> -	val <<= ICH_VMCR_BPR0_SHIFT;
> -	val &= ICH_VMCR_BPR0_MASK;
> -	vmcr &= ~ICH_VMCR_BPR0_MASK;
> -	vmcr |= val;
> +	vmcr &= ~ICH_VMCR_EL2_VBPR0_MASK;
> +	vmcr |= FIELD_PREP(ICH_VMCR_EL2_VBPR0, val);

You could uses FIELD_MODIFY() though that would mean using the _MASK
define for both places.  Not sure why the sysreg script generates both
(always have same actual value). I guess the idea is it is a little
shorter if you don't want to be explicit that the intent is to use it
as a mask.

I'd just use the _MASK defines throughout rather than trying for another
consistent scheme. 




>  
>  	__vgic_v3_write_vmcr(vmcr);
>  }
> @@ -929,17 +927,15 @@ static void __vgic_v3_write_bpr1(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
>  	u64 val = vcpu_get_reg(vcpu, rt);
>  	u8 bpr_min = __vgic_v3_bpr_min();
>  
> -	if (vmcr & ICH_VMCR_CBPR_MASK)
> +	if (FIELD_GET(ICH_VMCR_EL2_VCBPR_MASK, val))
>  		return;
>  
>  	/* Enforce BPR limiting */
>  	if (val < bpr_min)
>  		val = bpr_min;
>  
> -	val <<= ICH_VMCR_BPR1_SHIFT;
> -	val &= ICH_VMCR_BPR1_MASK;
> -	vmcr &= ~ICH_VMCR_BPR1_MASK;
> -	vmcr |= val;
> +	vmcr &= ~ICH_VMCR_EL2_VBPR1_MASK;
> +	vmcr |= FIELD_PREP(ICH_VMCR_EL2_VBPR1, val);

As above, FIELD_MODIFY() makes this a one liner.

>  
>  	__vgic_v3_write_vmcr(vmcr);
>  }
> @@ -1029,19 +1025,15 @@ static void __vgic_v3_read_hppir(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
>  
>  static void __vgic_v3_read_pmr(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
>  {
> -	vmcr &= ICH_VMCR_PMR_MASK;
> -	vmcr >>= ICH_VMCR_PMR_SHIFT;
> -	vcpu_set_reg(vcpu, rt, vmcr);
> +	vcpu_set_reg(vcpu, rt, FIELD_GET(ICH_VMCR_EL2_VPMR, vmcr));
>  }
>  
>  static void __vgic_v3_write_pmr(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
>  {
>  	u32 val = vcpu_get_reg(vcpu, rt);
>  
> -	val <<= ICH_VMCR_PMR_SHIFT;
> -	val &= ICH_VMCR_PMR_MASK;
> -	vmcr &= ~ICH_VMCR_PMR_MASK;
> -	vmcr |= val;
> +	vmcr &= ~ICH_VMCR_EL2_VPMR_MASK;
> +	vmcr |= FIELD_PREP(ICH_VMCR_EL2_VPMR, val);

FIELD_MODIFY() should be fine here I think.

>  
>  	write_gicreg(vmcr, ICH_VMCR_EL2);
>  }
> @@ -1064,9 +1056,9 @@ static void __vgic_v3_read_ctlr(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
>  	/* A3V */
>  	val |= ((vtr >> 21) & 1) << ICC_CTLR_EL1_A3V_SHIFT;
>  	/* EOImode */
> -	val |= ((vmcr & ICH_VMCR_EOIM_MASK) >> ICH_VMCR_EOIM_SHIFT) << ICC_CTLR_EL1_EOImode_SHIFT;
> +	val |= FIELD_GET(ICH_VMCR_EL2_VEOIM, vmcr) << ICC_CTLR_EL1_EOImode_SHIFT;

Bit ugly to mix and match styles.
ICC_CTRL_EL1_EOImode_MASK is defined so you could do a FIELD_PREP()

>  	/* CBPR */
> -	val |= (vmcr & ICH_VMCR_CBPR_MASK) >> ICH_VMCR_CBPR_SHIFT;
> +	val |= FIELD_GET(ICH_VMCR_EL2_VCBPR, vmcr);
>  
>  	vcpu_set_reg(vcpu, rt, val);
>  }
> @@ -1076,14 +1068,14 @@ static void __vgic_v3_write_ctlr(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
>  	u32 val = vcpu_get_reg(vcpu, rt);
>  
>  	if (val & ICC_CTLR_EL1_CBPR_MASK)
> -		vmcr |= ICH_VMCR_CBPR_MASK;
> +		vmcr |= ICH_VMCR_EL2_VCBPR_MASK;
>  	else
> -		vmcr &= ~ICH_VMCR_CBPR_MASK;
> +		vmcr &= ~ICH_VMCR_EL2_VCBPR_MASK;
These could be something like

	FIELD_MODIFY(ICH_VMCR_EL2_VCBPR_MASK, &vmcr,
		     FIELD_GET(ICC_CTRL_EL1_CBPR_MASK, val));

More compact. Whether more readable is a little bit more debatable.

>  
>  	if (val & ICC_CTLR_EL1_EOImode_MASK)
> -		vmcr |= ICH_VMCR_EOIM_MASK;
> +		vmcr |= ICH_VMCR_EL2_VEOIM_MASK;
>  	else
> -		vmcr &= ~ICH_VMCR_EOIM_MASK;
> +		vmcr &= ~ICH_VMCR_EL2_VEOIM_MASK;
>  
>  	write_gicreg(vmcr, ICH_VMCR_EL2);
>  }

Thanks,

Jonathan