[PATCH 0/3] KVM: arm64: vgic: Fix IIDR revision handling and add revision 1

[PATCH 0/3] KVM: arm64: vgic: Fix IIDR revision handling and add revision 1

[David Woodhouse]

The uaccess write handlers for GICD_IIDR extract the revision field
from the wrong variable, making it impossible for userspace to actually
change the implementation revision. Fix that.

Additionally, allow userspace to select IIDR revision 1, restoring the 
behaviour from before commit d53c2c29ae0d ("KVM: arm/arm64: vgic: Allow 
configuration of interrupt groups") behaviour where interrupt groups are 
not guest-configurable. This is needed by hypervisors that were 
reverting that commit to preserve the original guest-visible 
semantics.

When revision 1 is selected:
 - GICv2: IGROUPR reads as zero (group 0), writes ignored
 - GICv3: IGROUPR reads as all-ones (group 1), writes ignored

The IIDR revision comments in both vgic-mmio-v2.c and vgic-mmio-v3.c
are updated to document all three revisions.

David Woodhouse (3):
      KVM: arm64: vgic: Fix IIDR revision field extracted from wrong value
      KVM: arm64: vgic: Allow userspace to set IIDR revision 1
      KVM: arm64: selftests: Add vgic IIDR revision test

 arch/arm64/kvm/vgic/vgic-mmio-v2.c                 |   7 +-
 arch/arm64/kvm/vgic/vgic-mmio-v3.c                 |   6 +-
 arch/arm64/kvm/vgic/vgic-mmio.c                    |  15 +++
 include/kvm/arm_vgic.h                             |   1 +
 tools/testing/selftests/kvm/Makefile.kvm           |   1 +
 .../testing/selftests/kvm/arm64/vgic_group_iidr.c  | 112 +++++++++++++++++++++
 6 files changed, 140 insertions(+), 2 deletions(-)

[PATCH 1/3] KVM: arm64: vgic: Fix IIDR revision field extracted from wrong value

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

The uaccess write handlers for GICD_IIDR in both GICv2 and GICv3
extract the revision field from 'reg' (the current IIDR value read back
from the emulated distributor) instead of 'val' (the value userspace is
trying to write). This means userspace can never actually change the
implementation revision — the extracted value is always the current one.

Fix the FIELD_GET to use 'val' so that userspace can select a different
revision for migration compatibility.

Fixes: 49a1a2c70a7f ("KVM: arm64: vgic-v3: Advertise GICR_CTLR.{IR, CES} as a new GICD_IIDR revision")
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 arch/arm64/kvm/vgic/vgic-mmio-v2.c | 2 +-
 arch/arm64/kvm/vgic/vgic-mmio-v3.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v2.c b/arch/arm64/kvm/vgic/vgic-mmio-v2.c
index 406845b3117c..0643e333db35 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v2.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v2.c
@@ -91,7 +91,7 @@ static int vgic_mmio_uaccess_write_v2_misc(struct kvm_vcpu *vcpu,
 		 * migration from old kernels to new kernels with legacy
 		 * userspace.
 		 */
-		reg = FIELD_GET(GICD_IIDR_REVISION_MASK, reg);
+		reg = FIELD_GET(GICD_IIDR_REVISION_MASK, val);
 		switch (reg) {
 		case KVM_VGIC_IMP_REV_2:
 		case KVM_VGIC_IMP_REV_3:
diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index 89edb84d1ac6..5913a20d8301 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -194,7 +194,7 @@ static int vgic_mmio_uaccess_write_v3_misc(struct kvm_vcpu *vcpu,
 		if ((reg ^ val) & ~GICD_IIDR_REVISION_MASK)
 			return -EINVAL;
 
-		reg = FIELD_GET(GICD_IIDR_REVISION_MASK, reg);
+		reg = FIELD_GET(GICD_IIDR_REVISION_MASK, val);
 		switch (reg) {
 		case KVM_VGIC_IMP_REV_2:
 		case KVM_VGIC_IMP_REV_3:
-- 
2.51.0

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

[David Woodhouse]

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.

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)
  */
 
 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;
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;
+		return 0;
+	}
+
 	/* 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
-- 
2.51.0

[PATCH 3/3] KVM: arm64: selftests: Add vgic IIDR revision test

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

Test that the GICD_IIDR implementation revision correctly controls
guest-visible behaviour for GICv3:

  Revision 1: IGROUPR reads as all-ones (group 1), writes are ignored.
              GICR_CTLR.{IR,CES} not advertised.
  Revision 2: IGROUPR is guest-configurable (read/write).
              GICR_CTLR.{IR,CES} not advertised.
  Revision 3: IGROUPR is guest-configurable (read/write).
              GICR_CTLR.{IR,CES} advertised.

For each revision, the test sets the IIDR via KVM_DEV_ARM_VGIC_GRP_DIST_REGS
before initializing the vGIC, then runs a guest that verifies the
expected IGROUPR and GICR_CTLR behaviour.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 tools/testing/selftests/kvm/Makefile.kvm      |   1 +
 .../selftests/kvm/arm64/vgic_group_iidr.c     | 112 ++++++++++++++++++
 2 files changed, 113 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/arm64/vgic_group_iidr.c

diff --git a/tools/testing/selftests/kvm/Makefile.kvm b/tools/testing/selftests/kvm/Makefile.kvm
index 6471fa214a9f..df729a70124f 100644
--- a/tools/testing/selftests/kvm/Makefile.kvm
+++ b/tools/testing/selftests/kvm/Makefile.kvm
@@ -177,6 +177,7 @@ TEST_GEN_PROGS_arm64 += arm64/vcpu_width_config
 TEST_GEN_PROGS_arm64 += arm64/vgic_init
 TEST_GEN_PROGS_arm64 += arm64/vgic_irq
 TEST_GEN_PROGS_arm64 += arm64/vgic_lpi_stress
+TEST_GEN_PROGS_arm64 += arm64/vgic_group_iidr
 TEST_GEN_PROGS_arm64 += arm64/vpmu_counter_access
 TEST_GEN_PROGS_arm64 += arm64/no-vgic-v3
 TEST_GEN_PROGS_arm64 += arm64/idreg-idst
diff --git a/tools/testing/selftests/kvm/arm64/vgic_group_iidr.c b/tools/testing/selftests/kvm/arm64/vgic_group_iidr.c
new file mode 100644
index 000000000000..d5c20a41162c
--- /dev/null
+++ b/tools/testing/selftests/kvm/arm64/vgic_group_iidr.c
@@ -0,0 +1,112 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * vgic_group_iidr.c - Test IGROUPR behaviour across IIDR revisions
+ *
+ * Validate that the GICD_IIDR implementation revision controls
+ * IGROUPR semantics for GICv3:
+ *   Rev 1: IGROUPR reads as all-ones (group 1), writes ignored
+ *   Rev 2+: IGROUPR is guest-configurable (read/write)
+ */
+#include <linux/sizes.h>
+
+#include "test_util.h"
+#include "kvm_util.h"
+#include "processor.h"
+#include "gic.h"
+#include "gic_v3.h"
+#include "vgic.h"
+
+#define NR_IRQS		128
+#define SPI_IGROUPR	(GICD_IGROUPR + (32 / 32) * 4) /* intids 32-63 */
+
+static volatile uint64_t shared_rev;
+
+static void guest_code(void)
+{
+	uint32_t val;
+
+	val = readl(GICD_BASE_GVA + SPI_IGROUPR);
+
+	if (shared_rev == 1) {
+		/* Rev 1: all group 1 (RAO), writes ignored */
+		GUEST_ASSERT_EQ(val, 0xffffffff);
+		writel(0x0, GICD_BASE_GVA + SPI_IGROUPR);
+		val = readl(GICD_BASE_GVA + SPI_IGROUPR);
+		GUEST_ASSERT_EQ(val, 0xffffffff);
+	} else {
+		/* Rev 2/3: writable, default 0 */
+		writel(0xa5a5a5a5, GICD_BASE_GVA + SPI_IGROUPR);
+		val = readl(GICD_BASE_GVA + SPI_IGROUPR);
+		GUEST_ASSERT_EQ(val, 0xa5a5a5a5);
+	}
+
+	/* Rev 3: GICR_CTLR advertises IR and CES. Rev 1/2: it does not. */
+	val = readl(GICR_BASE_GVA + GICR_CTLR);
+	if (shared_rev >= 3)
+		GUEST_ASSERT(val & (GICR_CTLR_IR | GICR_CTLR_CES));
+	else
+		GUEST_ASSERT(!(val & (GICR_CTLR_IR | GICR_CTLR_CES)));
+
+	GUEST_DONE();
+}
+
+static void run_test(int rev)
+{
+	struct kvm_vcpu *vcpus[1];
+	struct kvm_vm *vm;
+	struct ucall uc;
+	uint32_t iidr;
+	int gic_fd;
+
+	pr_info("Testing IIDR revision %d\n", rev);
+
+	test_disable_default_vgic();
+	vm = vm_create_with_vcpus(1, guest_code, vcpus);
+
+	gic_fd = __vgic_v3_setup(vm, 1, NR_IRQS);
+	TEST_ASSERT(gic_fd >= 0, "Failed to create vGICv3");
+
+	/* Set the requested IIDR revision before init. */
+	kvm_device_attr_get(gic_fd, KVM_DEV_ARM_VGIC_GRP_DIST_REGS,
+			    GICD_IIDR, &iidr);
+	iidr &= ~GICD_IIDR_REVISION_MASK;
+	iidr |= rev << GICD_IIDR_REVISION_SHIFT;
+	kvm_device_attr_set(gic_fd, KVM_DEV_ARM_VGIC_GRP_DIST_REGS,
+			    GICD_IIDR, &iidr);
+
+	__vgic_v3_init(gic_fd);
+
+	/* Verify the revision was applied. */
+	kvm_device_attr_get(gic_fd, KVM_DEV_ARM_VGIC_GRP_DIST_REGS,
+			    GICD_IIDR, &iidr);
+	TEST_ASSERT(((iidr & GICD_IIDR_REVISION_MASK) >> GICD_IIDR_REVISION_SHIFT) == rev,
+		    "IIDR revision readback: expected %d, got %d",
+		    rev, (iidr & GICD_IIDR_REVISION_MASK) >> GICD_IIDR_REVISION_SHIFT);
+
+	/* Tell the guest which revision we set. */
+	sync_global_to_guest(vm, shared_rev);
+	shared_rev = rev;
+	sync_global_to_guest(vm, shared_rev);
+
+	vcpu_run(vcpus[0]);
+	switch (get_ucall(vcpus[0], &uc)) {
+	case UCALL_ABORT:
+		REPORT_GUEST_ASSERT(uc);
+		break;
+	case UCALL_DONE:
+		break;
+	default:
+		TEST_FAIL("Unexpected ucall %lu", uc.cmd);
+	}
+
+	close(gic_fd);
+	kvm_vm_free(vm);
+}
+
+int main(int argc, char *argv[])
+{
+	run_test(1);
+	run_test(2);
+	run_test(3);
+	return 0;
+}
-- 
2.51.0

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

[Marc Zyngier]

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.

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

[Woodhouse, David]

[-- Attachment #1.1: Type: text/plain, Size: 2128 bytes --]

On Wed, 2026-04-08 at 08:54 +0100, Marc Zyngier wrote:
> 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.

What if the guest boots under a new host kernel and finds the group
registers are writable, and then is live migrated to an old host kernel
on which they are not?

What about hibernation, if the *boot* kernel in the guest configures
the groups, but then transfers control back to the resumed guest kernel
which had not?

> So what is this *really* fixing?

I look at that question the other way round.

KVM has an established procedure for allowing userspace to control
guest-visible changes, using the IIDR. First the host kernel which
*supports* the change is rolled out, and only then does the VMM start
to enable it for new launches.

Even if we can address the questions above, and even if we can convince
ourselves that those are the *only* questions to ask... why not follow
the normal, safe, procedure? Especially given that there is already an
IIDR value which corresponds to it.

We don't *have* to YOLO it... and I don't want to :)



[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5964 bytes --]

[-- Attachment #2.1: Type: text/plain, Size: 215 bytes --]




Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.



[-- Attachment #2.2: Type: text/html, Size: 228 bytes --]