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

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

On Fri, 9 Jan 2026 17:04:39 +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,

A couple of trivial things inline that you can feel free to ignore.
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

> ---
>  arch/arm64/include/asm/sysreg.h      | 21 ---------
>  arch/arm64/kvm/hyp/vgic-v3-sr.c      | 68 ++++++++++------------------
>  arch/arm64/kvm/vgic/vgic-v3-nested.c |  8 ++--
>  arch/arm64/kvm/vgic/vgic-v3.c        | 48 +++++++++-----------
>  4 files changed, 50 insertions(+), 95 deletions(-)

> diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c
> index 0b670a033fd87..ff10fc71fcd5d 100644
> --- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
> +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c


> @@ -1064,9 +1047,10 @@ 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_PREP(ICC_CTLR_EL1_EOImode_MASK,
> +			  FIELD_GET(ICH_VMCR_EL2_VEOIM, vmcr));
>  	/* CBPR */
> -	val |= (vmcr & ICH_VMCR_CBPR_MASK) >> ICH_VMCR_CBPR_SHIFT;
> +	val |= FIELD_GET(ICH_VMCR_EL2_VCBPR, vmcr);

This one makes me a tiny bit nervous because it's not obvious that this
is kind of FIELD_PREP(FIELD_GET()) like the EOIMode above.

Only a tiny bit though, so it's fine as is.


>  
>  	vcpu_set_reg(vcpu, rt, val);
>  }
> @@ -1075,15 +1059,11 @@ 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;
> -	else
> -		vmcr &= ~ICH_VMCR_CBPR_MASK;
> +	FIELD_MODIFY(ICH_VMCR_EL2_VCBPR, &vmcr,
> +		     FIELD_GET(ICC_CTLR_EL1_CBPR_MASK, val));

I'm not laughing at all and the _MASK here because that header
only defines the MASK form, even for single bits :)


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