Re: [PATCH v2 02/36] KVM: arm64: gic-v3: Switch vGIC-v3 to use generated ICH_VMCR_EL2
From: Jonathan Cameron <jonathan.cameron@huawei.com>
Date: 2026-01-06 18:00:53
Also in:
kvm, kvmarm
On Fri, 19 Dec 2025 15:52:36 +0000 Sascha Bischoff [off-list ref] wrote:
From: Sascha Bischoff <redacted> 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 <redacted>
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.
quoted hunk ↗ jump to hunk
--- 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
quoted hunk ↗ jump to hunk
@@ -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 !!
}
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
__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.
quoted hunk ↗ jump to hunk
__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.
quoted hunk ↗ jump to hunk
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()
quoted hunk ↗ jump to hunk
/* 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