Re: [PATCH 2/3] KVM: arm64: vgic: Allow userspace to set IIDR revision 1

[Marc Zyngier <maz@kernel.org>]

On Tue, 07 Apr 2026 21:27:03 +0100,
David Woodhouse <dwmw2@infradead.org> wrote:
> 
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> Allow userspace to select GICD_IIDR revision 1, which restores the
> original pre-d53c2c29ae0d ("KVM: arm/arm64: vgic: Allow configuration
> of interrupt groups") behaviour where interrupt groups are not
> guest-configurable.

I'm a bit surprised by this.

Either your guest knows that the group registers are not writable and
already deals with the buggy behaviour by not configuring the groups
(or configuring them in a way that matches what the implementation
does). Or it configures them differently and totally fails to handle
the interrupts as they are delivered using the wrong exception type,
if at all.

I'd expect that your guests fall in the former category and not the
latter, as we'd be discussing a very different problem. And my vague
recollection of this issue is that we had established that as long as
the reset values were unchanged, there was no harm in letting things
rip.

So what is this *really* fixing?

> 
> When revision 1 is selected:
>  - GICv2: IGROUPR reads as zero (group 0), writes are ignored
>  - GICv3: IGROUPR reads as all-ones (group 1), writes are ignored
>  - v2_groups_user_writable is not set
> 
> This is implemented by checking the implementation revision in
> vgic_mmio_read_group() and vgic_mmio_write_group() and returning
> the fixed values when the revision is below 2.
> 
> Fixes: d53c2c29ae0d ("KVM: arm/arm64: vgic: Allow configuration of interrupt groups")
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>  arch/arm64/kvm/vgic/vgic-mmio-v2.c |  5 +++++
>  arch/arm64/kvm/vgic/vgic-mmio-v3.c |  4 ++++
>  arch/arm64/kvm/vgic/vgic-mmio.c    | 15 +++++++++++++++
>  include/kvm/arm_vgic.h             |  1 +
>  4 files changed, 25 insertions(+)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v2.c b/arch/arm64/kvm/vgic/vgic-mmio-v2.c
> index 0643e333db35..14aa49f86f60 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v2.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v2.c
> @@ -20,6 +20,8 @@
>   * Revision 1: Report GICv2 interrupts as group 0 instead of group 1
>   * Revision 2: Interrupt groups are guest-configurable and signaled using
>   * 	       their configured groups.
> + * Revision 3: GICv2 behaviour is unchanged from revision 2.
> + * 	       (GICv3 gains GICR_CTLR.{IR,CES}; see vgic-mmio-v3.c)

nit: I don't think we need cross-version documentation, so just drop
the second line.

>   */
>  
>  static unsigned long vgic_mmio_read_v2_misc(struct kvm_vcpu *vcpu,
> @@ -93,6 +95,9 @@ static int vgic_mmio_uaccess_write_v2_misc(struct kvm_vcpu *vcpu,
>  		 */
>  		reg = FIELD_GET(GICD_IIDR_REVISION_MASK, val);
>  		switch (reg) {
> +		case KVM_VGIC_IMP_REV_1:
> +			dist->implementation_rev = reg;
> +			return 0;
>  		case KVM_VGIC_IMP_REV_2:
>  		case KVM_VGIC_IMP_REV_3:
>  			vcpu->kvm->arch.vgic.v2_groups_user_writable = true;

nit: move the v1 handling down with a fallthrough in v2/v3 so that we
don't duplicate the basic handling:

diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v2.c b/arch/arm64/kvm/vgic/vgic-mmio-v2.c
index 406845b3117cf..34f8b2b615fc5 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v2.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v2.c
@@ -96,6 +96,8 @@ static int vgic_mmio_uaccess_write_v2_misc(struct kvm_vcpu *vcpu,
 		case KVM_VGIC_IMP_REV_2:
 		case KVM_VGIC_IMP_REV_3:
 			vcpu->kvm->arch.vgic.v2_groups_user_writable = true;
+			fallthrough;
+		case KVM_VGIC_IMP_REV_1:
 			dist->implementation_rev = reg;
 			return 0;
 		default:

> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> index 5913a20d8301..0130db71cfc9 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> @@ -74,8 +74,11 @@ bool vgic_supports_direct_sgis(struct kvm *kvm)
>  /*
>   * The Revision field in the IIDR have the following meanings:
>   *
> + * Revision 1: Interrupt groups are not guest-configurable.
> + * 	       IGROUPR reads as all-ones (group 1), writes ignored.
>   * Revision 2: Interrupt groups are guest-configurable and signaled using
>   * 	       their configured groups.
> + * Revision 3: GICR_CTLR.{IR,CES} are advertised.
>   */
>  
>  static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
> @@ -196,6 +199,7 @@ static int vgic_mmio_uaccess_write_v3_misc(struct kvm_vcpu *vcpu,
>  
>  		reg = FIELD_GET(GICD_IIDR_REVISION_MASK, val);
>  		switch (reg) {
> +		case KVM_VGIC_IMP_REV_1:
>  		case KVM_VGIC_IMP_REV_2:
>  		case KVM_VGIC_IMP_REV_3:
>  			dist->implementation_rev = reg;
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c b/arch/arm64/kvm/vgic/vgic-mmio.c
> index a573b1f0c6cb..9eb95f13b9b6 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio.c
> @@ -48,6 +48,17 @@ unsigned long vgic_mmio_read_group(struct kvm_vcpu *vcpu,
>  	u32 value = 0;
>  	int i;
>  
> +	/*
> +	 * Revision 1 and below: groups are not guest-configurable.
> +	 * GICv2 reports all interrupts as group 0 (RAZ).
> +	 * GICv3 reports all interrupts as group 1 (RAO).
> +	 */
> +	if (vgic_get_implementation_rev(vcpu) < KVM_VGIC_IMP_REV_2) {
> +		if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
> +			return -1UL;

Hmmm... This can only be a 32bit access, so returning non-zero bits in
the top half is weird. It will be narrowed down before hitting the
guest, but still. Anyway, see below.

> +		return 0;
> +	}
> +

Why is this needed at all? The group reset values are already the
expected ones for v1, and as long as we prevent writes, the reported
group configuration will be as expected.

>  	/* Loop over all IRQs affected by this read */
>  	for (i = 0; i < len * 8; i++) {
>  		struct vgic_irq *irq = vgic_get_vcpu_irq(vcpu, intid + i);
> @@ -73,6 +84,10 @@ void vgic_mmio_write_group(struct kvm_vcpu *vcpu, gpa_t addr,
>  	int i;
>  	unsigned long flags;
>  
> +	/* Revision 1 and below: groups are not guest-configurable. */
> +	if (vgic_get_implementation_rev(vcpu) < KVM_VGIC_IMP_REV_2)
> +		return;
> +
>  	for (i = 0; i < len * 8; i++) {
>  		struct vgic_irq *irq = vgic_get_vcpu_irq(vcpu, intid + i);
>  
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index f2eafc65bbf4..90fb6cd3c91c 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -248,6 +248,7 @@ struct vgic_dist {
>  
>  	/* Implementation revision as reported in the GICD_IIDR */
>  	u32			implementation_rev;
> +#define KVM_VGIC_IMP_REV_1	1 /* GICv2 interrupts as group 0 */
>  #define KVM_VGIC_IMP_REV_2	2 /* GICv2 restorable groups */
>  #define KVM_VGIC_IMP_REV_3	3 /* GICv3 GICR_CTLR.{IW,CES,RWP} */
>  #define KVM_VGIC_IMP_REV_LATEST	KVM_VGIC_IMP_REV_3

Thanks,

	M.

-- 
Jazz isn't dead. It just smells funny.