[PATCH v2 02/36] KVM: arm64: gic-v3: Switch vGIC-v3 to use generated ICH_VMCR_EL2
Sascha Bischoff
Sascha.Bischoff at arm.com
Wed Jan 7 02:55:18 PST 2026
On Tue, 2026-01-06 at 18:00 +0000, Jonathan Cameron wrote:
> On Fri, 19 Dec 2025 15:52:36 +0000
> Sascha Bischoff <Sascha.Bischoff at arm.com> wrote:
>
> > From: Sascha Bischoff <Sascha.Bischoff at 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 at 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.
Hi Jonathan,
I've updated the code to 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 !!
Ah, that's a good catch, and might well be an unintended functional
change looking into it. I've switched to using FIELD_GET() for both.
>
> > }
>
> > @@ -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.
FIELD_MODIFY() is a great shout here. Done & thanks.
Yeah, I'd tried to use _MASK when explicitly using it as a mask, and
without in FIELD_x() (and still managed to be inconsistent with that).
I've now used _MASK everywhere.
>
>
>
>
> >
> > __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.
>
Done.
> >
> > __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.
>
Done.
> >
> > 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()
Done.
>
> > /* 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.
I've gone with this for now. I think it is sufficiently readable.
>
> >
> > 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
>
Thanks a lot!
Sascha
More information about the linux-arm-kernel
mailing list