[PATCH v3 0/4] KVM: arm64: Add attribute to control GICD_TYPER2.nASSGIcap

[PATCH v3 0/4] KVM: arm64: Add attribute to control GICD_TYPER2.nASSGIcap

[Raghavendra Rao Ananta]

A shortcoming of the GIC architecture is that there's an absolute limit on
the number of vPEs that can be tracked by the ITS. It is possible that
an operator is running a mix of VMs on a system, only wanting to provide
a specific class of VMs with hardware interrupt injection support.

The series introduces KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap vGIC attribute to allow
the userspace to control GICD_TYPER2.nASSGIcap (GICv4.1) on a per-VM basis.

v1: https://lore.kernel.org/kvmarm/20250514192159.1751538-1-rananta@google.com/

v1 -> v2: https://lore.kernel.org/all/20250531012545.709887-1-oliver.upton@linux.dev/
 - Drop all use of GICv4 in the UAPI and KVM-internal helpers in favor
   of nASSGIcap. This changes things around to model a guest feature,
   not a host feature.

 - Consolidate UAPI into a single attribute and expect userspace to use
   to read the attribute for discovery, much like we do with the ID
   registers

 - Squash documentation together with implementation

 - Clean up maintenance IRQ attribute handling, which I ran into as part
   of reviewing this series

v2 -> v3:
 - Update checks in vgic-v3.c and vgic-v4.c to also include nASSGIcap (via
   vgic_supports_direct_sgis()) that's configured by the userspace. (Oliver)

Oliver Upton (2):
  KVM: arm64: Disambiguate support for vSGIs v. vLPIs
  KVM: arm64: vgic-v3: Consolidate MAINT_IRQ handling

Raghavendra Rao Ananta (2):
  KVM: arm64: Introduce attribute to control GICD_TYPER2.nASSGIcap
  KVM: arm64: selftests: Add test for nASSGIcap attribute

 .../virt/kvm/devices/arm-vgic-v3.rst          | 29 ++++++
 arch/arm64/include/uapi/asm/kvm.h             |  3 +
 arch/arm64/kvm/vgic/vgic-init.c               |  7 +-
 arch/arm64/kvm/vgic/vgic-kvm-device.c         | 88 +++++++++++++------
 arch/arm64/kvm/vgic/vgic-mmio-v3.c            | 24 +++--
 arch/arm64/kvm/vgic/vgic-v3.c                 |  5 +-
 arch/arm64/kvm/vgic/vgic-v4.c                 |  6 +-
 arch/arm64/kvm/vgic/vgic.c                    |  4 +-
 arch/arm64/kvm/vgic/vgic.h                    |  7 ++
 include/kvm/arm_vgic.h                        |  3 +
 tools/testing/selftests/kvm/arm64/vgic_init.c | 41 +++++++++
 11 files changed, 176 insertions(+), 41 deletions(-)


base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
--
2.50.0.rc2.692.g299adb8693-goog

[PATCH v3 1/4] KVM: arm64: Disambiguate support for vSGIs v. vLPIs

[Raghavendra Rao Ananta]

From: Oliver Upton <oliver.upton@linux.dev>

vgic_supports_direct_msis() is a bit of a misnomer, as it returns true
if either vSGIs or vLPIs are supported. Pick it apart into a few
predicates and replace some open-coded checks for vSGIs.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/vgic/vgic-init.c    |  4 ++--
 arch/arm64/kvm/vgic/vgic-mmio-v3.c | 16 ++++++++++------
 arch/arm64/kvm/vgic/vgic-v4.c      |  4 ++--
 arch/arm64/kvm/vgic/vgic.c         |  4 ++--
 arch/arm64/kvm/vgic/vgic.h         |  7 +++++++
 5 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
index eb1205654ac8..5e0e4559004b 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -395,7 +395,7 @@ int vgic_init(struct kvm *kvm)
 	 * v4 support so that we get HW-accelerated vSGIs. Otherwise, only
 	 * enable it if we present a virtual ITS to the guest.
 	 */
-	if (vgic_supports_direct_msis(kvm)) {
+	if (vgic_supports_direct_irqs(kvm)) {
 		ret = vgic_v4_init(kvm);
 		if (ret)
 			goto out;
@@ -443,7 +443,7 @@ static void kvm_vgic_dist_destroy(struct kvm *kvm)
 		dist->vgic_cpu_base = VGIC_ADDR_UNDEF;
 	}
 
-	if (vgic_supports_direct_msis(kvm))
+	if (vgic_supports_direct_irqs(kvm))
 		vgic_v4_teardown(kvm);
 
 	xa_destroy(&dist->lpi_xa);
diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index ae4c0593d114..1a9c5b4418b2 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -50,8 +50,12 @@ bool vgic_has_its(struct kvm *kvm)
 
 bool vgic_supports_direct_msis(struct kvm *kvm)
 {
-	return (kvm_vgic_global_state.has_gicv4_1 ||
-		(kvm_vgic_global_state.has_gicv4 && vgic_has_its(kvm)));
+	return kvm_vgic_global_state.has_gicv4 && vgic_has_its(kvm);
+}
+
+bool vgic_supports_direct_sgis(struct kvm *kvm)
+{
+	return kvm_vgic_global_state.has_gicv4_1 && gic_cpuif_has_vsgi();
 }
 
 /*
@@ -86,7 +90,7 @@ static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
 		}
 		break;
 	case GICD_TYPER2:
-		if (kvm_vgic_global_state.has_gicv4_1 && gic_cpuif_has_vsgi())
+		if (vgic_supports_direct_sgis(vcpu->kvm))
 			value = GICD_TYPER2_nASSGIcap;
 		break;
 	case GICD_IIDR:
@@ -119,7 +123,7 @@ static void vgic_mmio_write_v3_misc(struct kvm_vcpu *vcpu,
 		dist->enabled = val & GICD_CTLR_ENABLE_SS_G1;
 
 		/* Not a GICv4.1? No HW SGIs */
-		if (!kvm_vgic_global_state.has_gicv4_1 || !gic_cpuif_has_vsgi())
+		if (!vgic_supports_direct_sgis(vcpu->kvm))
 			val &= ~GICD_CTLR_nASSGIreq;
 
 		/* Dist stays enabled? nASSGIreq is RO */
@@ -133,7 +137,7 @@ static void vgic_mmio_write_v3_misc(struct kvm_vcpu *vcpu,
 		if (is_hwsgi != dist->nassgireq)
 			vgic_v4_configure_vsgis(vcpu->kvm);
 
-		if (kvm_vgic_global_state.has_gicv4_1 &&
+		if (vgic_supports_direct_sgis(vcpu->kvm) &&
 		    was_enabled != dist->enabled)
 			kvm_make_all_cpus_request(vcpu->kvm, KVM_REQ_RELOAD_GICv4);
 		else if (!was_enabled && dist->enabled)
@@ -178,7 +182,7 @@ static int vgic_mmio_uaccess_write_v3_misc(struct kvm_vcpu *vcpu,
 		}
 	case GICD_CTLR:
 		/* Not a GICv4.1? No HW SGIs */
-		if (!kvm_vgic_global_state.has_gicv4_1)
+		if (vgic_supports_direct_sgis(vcpu->kvm))
 			val &= ~GICD_CTLR_nASSGIreq;
 
 		dist->enabled = val & GICD_CTLR_ENABLE_SS_G1;
diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
index 193946108192..e7e284d47a77 100644
--- a/arch/arm64/kvm/vgic/vgic-v4.c
+++ b/arch/arm64/kvm/vgic/vgic-v4.c
@@ -356,7 +356,7 @@ int vgic_v4_put(struct kvm_vcpu *vcpu)
 {
 	struct its_vpe *vpe = &vcpu->arch.vgic_cpu.vgic_v3.its_vpe;
 
-	if (!vgic_supports_direct_msis(vcpu->kvm) || !vpe->resident)
+	if (!vgic_supports_direct_irqs(vcpu->kvm) || !vpe->resident)
 		return 0;
 
 	return its_make_vpe_non_resident(vpe, vgic_v4_want_doorbell(vcpu));
@@ -367,7 +367,7 @@ int vgic_v4_load(struct kvm_vcpu *vcpu)
 	struct its_vpe *vpe = &vcpu->arch.vgic_cpu.vgic_v3.its_vpe;
 	int err;
 
-	if (!vgic_supports_direct_msis(vcpu->kvm) || vpe->resident)
+	if (!vgic_supports_direct_irqs(vcpu->kvm) || vpe->resident)
 		return 0;
 
 	if (vcpu_get_flag(vcpu, IN_WFI))
diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
index 8f8096d48925..f5148b38120a 100644
--- a/arch/arm64/kvm/vgic/vgic.c
+++ b/arch/arm64/kvm/vgic/vgic.c
@@ -951,7 +951,7 @@ void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
 	 * can be directly injected (GICv4).
 	 */
 	if (list_empty(&vcpu->arch.vgic_cpu.ap_list_head) &&
-	    !vgic_supports_direct_msis(vcpu->kvm))
+	    !vgic_supports_direct_irqs(vcpu->kvm))
 		return;
 
 	DEBUG_SPINLOCK_BUG_ON(!irqs_disabled());
@@ -965,7 +965,7 @@ void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
 	if (can_access_vgic_from_kernel())
 		vgic_restore_state(vcpu);
 
-	if (vgic_supports_direct_msis(vcpu->kvm))
+	if (vgic_supports_direct_irqs(vcpu->kvm))
 		vgic_v4_commit(vcpu);
 }
 
diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h
index 4349084cb9a6..ebf9ed6adeac 100644
--- a/arch/arm64/kvm/vgic/vgic.h
+++ b/arch/arm64/kvm/vgic/vgic.h
@@ -370,6 +370,13 @@ int vgic_its_inv_lpi(struct kvm *kvm, struct vgic_irq *irq);
 int vgic_its_invall(struct kvm_vcpu *vcpu);
 
 bool vgic_supports_direct_msis(struct kvm *kvm);
+bool vgic_supports_direct_sgis(struct kvm *kvm);
+
+static inline bool vgic_supports_direct_irqs(struct kvm *kvm)
+{
+	return vgic_supports_direct_msis(kvm) || vgic_supports_direct_sgis(kvm);
+}
+
 int vgic_v4_init(struct kvm *kvm);
 void vgic_v4_teardown(struct kvm *kvm);
 void vgic_v4_configure_vsgis(struct kvm *kvm);
-- 
2.50.0.rc2.692.g299adb8693-goog

[PATCH v3 2/4] KVM: arm64: vgic-v3: Consolidate MAINT_IRQ handling

[Raghavendra Rao Ananta]

From: Oliver Upton <oliver.upton@linux.dev>

Consolidate the duplicated handling of the VGICv3 maintenance IRQ
attribute as a regular GICv3 attribute, as it is neither a register nor
a common attribute.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/vgic/vgic-kvm-device.c | 51 +++++++++++++--------------
 1 file changed, 25 insertions(+), 26 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c
index f9ae790163fb..e28cf68a49c3 100644
--- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
+++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
@@ -303,12 +303,6 @@ static int vgic_get_common_attr(struct kvm_device *dev,
 			     VGIC_NR_PRIVATE_IRQS, uaddr);
 		break;
 	}
-	case KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ: {
-		u32 __user *uaddr = (u32 __user *)(long)attr->addr;
-
-		r = put_user(dev->kvm->arch.vgic.mi_intid, uaddr);
-		break;
-	}
 	}
 
 	return r;
@@ -523,7 +517,7 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev,
 	struct vgic_reg_attr reg_attr;
 	gpa_t addr;
 	struct kvm_vcpu *vcpu;
-	bool uaccess, post_init = true;
+	bool uaccess;
 	u32 val;
 	int ret;
 
@@ -539,9 +533,6 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev,
 		/* Sysregs uaccess is performed by the sysreg handling code */
 		uaccess = false;
 		break;
-	case KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ:
-		post_init = false;
-		fallthrough;
 	default:
 		uaccess = true;
 	}
@@ -561,7 +552,7 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev,
 
 	mutex_lock(&dev->kvm->arch.config_lock);
 
-	if (post_init != vgic_initialized(dev->kvm)) {
+	if (!vgic_initialized(dev->kvm)) {
 		ret = -EBUSY;
 		goto out;
 	}
@@ -591,19 +582,6 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev,
 		}
 		break;
 	}
-	case KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ:
-		if (!is_write) {
-			val = dev->kvm->arch.vgic.mi_intid;
-			ret = 0;
-			break;
-		}
-
-		ret = -EINVAL;
-		if ((val < VGIC_NR_PRIVATE_IRQS) && (val >= VGIC_NR_SGIS)) {
-			dev->kvm->arch.vgic.mi_intid = val;
-			ret = 0;
-		}
-		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -630,8 +608,24 @@ static int vgic_v3_set_attr(struct kvm_device *dev,
 	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
 	case KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS:
 	case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO:
-	case KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ:
 		return vgic_v3_attr_regs_access(dev, attr, true);
+	case KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ: {
+		u32 __user *uaddr = (u32 __user *)attr->addr;
+		u32 val;
+
+		if (get_user(val, uaddr))
+			return -EFAULT;
+
+		guard(mutex)(&dev->kvm->arch.config_lock);
+		if (vgic_initialized(dev->kvm))
+			return -EBUSY;
+
+		if ((val < VGIC_NR_SGIS) || (val >= VGIC_NR_PRIVATE_IRQS))
+			return -EINVAL;
+
+		dev->kvm->arch.vgic.mi_intid = val;
+		return 0;
+	}
 	default:
 		return vgic_set_common_attr(dev, attr);
 	}
@@ -645,8 +639,13 @@ static int vgic_v3_get_attr(struct kvm_device *dev,
 	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
 	case KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS:
 	case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO:
-	case KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ:
 		return vgic_v3_attr_regs_access(dev, attr, false);
+	case KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ: {
+		u32 __user *uaddr = (u32 __user *)(long)attr->addr;
+
+		guard(mutex)(&dev->kvm->arch.config_lock);
+		return put_user(dev->kvm->arch.vgic.mi_intid, uaddr);
+	}
 	default:
 		return vgic_get_common_attr(dev, attr);
 	}
-- 
2.50.0.rc2.692.g299adb8693-goog

[PATCH v3 3/4] KVM: arm64: Introduce attribute to control GICD_TYPER2.nASSGIcap

[Raghavendra Rao Ananta]

KVM unconditionally advertises GICD_TYPER2.nASSGIcap (which internally
implies vSGIs) on GICv4.1 systems. Allow userspace to change whether a
VM supports the feature. Only allow changes prior to VGIC initialization
as at that point vPEs need to be allocated for the VM.

For convenience, bundle support for vLPIs and vSGIs behind this feature,
allowing userspace to control vPE allocation for VMs in environments
that may be constrained on vPE IDs.

Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 .../virt/kvm/devices/arm-vgic-v3.rst          | 29 +++++++++++++++
 arch/arm64/include/uapi/asm/kvm.h             |  3 ++
 arch/arm64/kvm/vgic/vgic-init.c               |  3 ++
 arch/arm64/kvm/vgic/vgic-kvm-device.c         | 37 +++++++++++++++++++
 arch/arm64/kvm/vgic/vgic-mmio-v3.c            | 10 ++++-
 arch/arm64/kvm/vgic/vgic-v3.c                 |  5 ++-
 arch/arm64/kvm/vgic/vgic-v4.c                 |  2 +-
 include/kvm/arm_vgic.h                        |  3 ++
 8 files changed, 88 insertions(+), 4 deletions(-)

diff --git a/Documentation/virt/kvm/devices/arm-vgic-v3.rst b/Documentation/virt/kvm/devices/arm-vgic-v3.rst
index e860498b1e35..049d77eae591 100644
--- a/Documentation/virt/kvm/devices/arm-vgic-v3.rst
+++ b/Documentation/virt/kvm/devices/arm-vgic-v3.rst
@@ -306,3 +306,32 @@ Groups:
 
     The vINTID specifies which interrupt is generated when the vGIC
     must generate a maintenance interrupt. This must be a PPI.
+
+  KVM_DEV_ARM_VGIC_GRP_FEATURES
+   Attributes:
+
+    KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap
+      Control whether support for SGIs without an active state is exposed
+      to the VM. attr->addr points to a __u8 value which indicates whether
+      he feature is enabled / disabled.
+
+      A value of 0 indicates that the feature is disabled. A nonzero value
+      indicates that the feature is enabled.
+
+      This attribute can only be set prior to initializing the VGIC (i.e.
+      KVM_DEV_ARM_VGIC_CTRL_INIT).
+
+      Support for SGIs without an active state depends on hardware support.
+      Userspace can discover support for the feature by reading the
+      attribute after creating a VGICv3. It is possible that
+      KVM_DEV_ARM_VGIC_CTRL_INIT can later fail if this feature is enabled
+      and KVM is unable to allocate GIC vPEs for the VM.
+
+  Errors:
+
+    =======  ========================================================
+    -ENXIO   Invalid attribute in attr->attr
+    -EFAULT  Invalid user address in attr->addr
+    -EBUSY   The VGIC has already been initialized
+    -EINVAL  KVM doesn't support the requested feature setting
+    =======  ========================================================
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index ed5f3892674c..41e9ce412afd 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -417,6 +417,7 @@ enum {
 #define KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO  7
 #define KVM_DEV_ARM_VGIC_GRP_ITS_REGS 8
 #define KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ  9
+#define KVM_DEV_ARM_VGIC_GRP_FEATURES 10
 #define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT	10
 #define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK \
 			(0x3fffffULL << KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT)
@@ -429,6 +430,8 @@ enum {
 #define   KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES	3
 #define   KVM_DEV_ARM_ITS_CTRL_RESET		4
 
+#define   KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap	0
+
 /* Device Control API on vcpu fd */
 #define KVM_ARM_VCPU_PMU_V3_CTRL	0
 #define   KVM_ARM_VCPU_PMU_V3_IRQ		0
diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
index 5e0e4559004b..944e24750ac4 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -157,6 +157,9 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
 
 	kvm->arch.vgic.in_kernel = true;
 	kvm->arch.vgic.vgic_model = type;
+	if (type == KVM_DEV_TYPE_ARM_VGIC_V3)
+		kvm->arch.vgic.nassgicap = kvm_vgic_global_state.has_gicv4_1 &&
+					   gic_cpuif_has_vsgi();
 
 	kvm->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF;
 
diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c
index e28cf68a49c3..629f56063a13 100644
--- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
+++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
@@ -626,6 +626,26 @@ static int vgic_v3_set_attr(struct kvm_device *dev,
 		dev->kvm->arch.vgic.mi_intid = val;
 		return 0;
 	}
+	case KVM_DEV_ARM_VGIC_GRP_FEATURES: {
+		u8 __user *uaddr = (u8 __user *)attr->addr;
+		u8 val;
+
+		if (attr->attr != KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap)
+			return -ENXIO;
+
+		if (get_user(val, uaddr))
+			return -EFAULT;
+
+		guard(mutex)(&dev->kvm->arch.config_lock);
+		if (vgic_initialized(dev->kvm))
+			return -EBUSY;
+
+		if (!(kvm_vgic_global_state.has_gicv4_1 && gic_cpuif_has_vsgi()) && val)
+			return -EINVAL;
+
+		dev->kvm->arch.vgic.nassgicap = val;
+		return 0;
+	}
 	default:
 		return vgic_set_common_attr(dev, attr);
 	}
@@ -646,6 +666,17 @@ static int vgic_v3_get_attr(struct kvm_device *dev,
 		guard(mutex)(&dev->kvm->arch.config_lock);
 		return put_user(dev->kvm->arch.vgic.mi_intid, uaddr);
 	}
+	case KVM_DEV_ARM_VGIC_GRP_FEATURES: {
+		u8 __user *uaddr = (u8 __user *)attr->addr;
+		u8 val;
+
+		if (attr->attr != KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap)
+			return -ENXIO;
+
+		guard(mutex)(&dev->kvm->arch.config_lock);
+		val = dev->kvm->arch.vgic.nassgicap;
+		return put_user(val, uaddr);
+	}
 	default:
 		return vgic_get_common_attr(dev, attr);
 	}
@@ -683,8 +714,14 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
 			return 0;
 		case KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES:
 			return 0;
+		default:
+			return -ENXIO;
 		}
+	case KVM_DEV_ARM_VGIC_GRP_FEATURES:
+		return attr->attr != KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap ?
+		       -ENXIO : 0;
 	}
+
 	return -ENXIO;
 }
 
diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index 1a9c5b4418b2..43f59e70e1a2 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -50,12 +50,20 @@ bool vgic_has_its(struct kvm *kvm)
 
 bool vgic_supports_direct_msis(struct kvm *kvm)
 {
+	/*
+	 * Deliberately conflate vLPI and vSGI support on GICv4.1 hardware,
+	 * indirectly allowing userspace to control whether or not vPEs are
+	 * allocated for the VM.
+	 */
+	if (kvm_vgic_global_state.has_gicv4_1 && !vgic_supports_direct_sgis(kvm))
+		return false;
+
 	return kvm_vgic_global_state.has_gicv4 && vgic_has_its(kvm);
 }
 
 bool vgic_supports_direct_sgis(struct kvm *kvm)
 {
-	return kvm_vgic_global_state.has_gicv4_1 && gic_cpuif_has_vsgi();
+	return kvm->arch.vgic.nassgicap;
 }
 
 /*
diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
index b9ad7c42c5b0..cb6bda9b3c6c 100644
--- a/arch/arm64/kvm/vgic/vgic-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-v3.c
@@ -404,7 +404,8 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
 	 * The above vgic initialized check also ensures that the allocation
 	 * and enabling of the doorbells have already been done.
 	 */
-	if (kvm_vgic_global_state.has_gicv4_1) {
+	if (kvm_vgic_global_state.has_gicv4_1 &&
+	    vgic_supports_direct_irqs(kvm)) {
 		unmap_all_vpes(kvm);
 		vlpi_avail = true;
 	}
@@ -581,7 +582,7 @@ int vgic_v3_map_resources(struct kvm *kvm)
 		return -EBUSY;
 	}
 
-	if (kvm_vgic_global_state.has_gicv4_1)
+	if (vgic_supports_direct_sgis(kvm))
 		vgic_v4_configure_vsgis(kvm);
 
 	return 0;
diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
index e7e284d47a77..25e9da9e7a2d 100644
--- a/arch/arm64/kvm/vgic/vgic-v4.c
+++ b/arch/arm64/kvm/vgic/vgic-v4.c
@@ -245,7 +245,7 @@ int vgic_v4_init(struct kvm *kvm)
 
 	lockdep_assert_held(&kvm->arch.config_lock);
 
-	if (!kvm_vgic_global_state.has_gicv4)
+	if (!vgic_supports_direct_irqs(kvm))
 		return 0; /* Nothing to see here... move along. */
 
 	if (dist->its_vm.vpes)
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 4a34f7f0a864..1b4886f3fb20 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -264,6 +264,9 @@ struct vgic_dist {
 	/* distributor enabled */
 	bool			enabled;
 
+	/* Supports SGIs without active state */
+	bool			nassgicap;
+
 	/* Wants SGIs without active state */
 	bool			nassgireq;
 
-- 
2.50.0.rc2.692.g299adb8693-goog

[PATCH v3 4/4] KVM: arm64: selftests: Add test for nASSGIcap attribute

[Raghavendra Rao Ananta]

Extend vgic_init to test the nASSGIcap attribute, asserting that it is
configurable (within reason) prior to initializing the VGIC.
Additionally, check that userspace cannot set the attribute after the
VGIC has been initialized.

Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 tools/testing/selftests/kvm/arm64/vgic_init.c | 41 +++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/tools/testing/selftests/kvm/arm64/vgic_init.c b/tools/testing/selftests/kvm/arm64/vgic_init.c
index b3b5fb0ff0a9..aaecba432dbc 100644
--- a/tools/testing/selftests/kvm/arm64/vgic_init.c
+++ b/tools/testing/selftests/kvm/arm64/vgic_init.c
@@ -675,6 +675,46 @@ static void test_v3_its_region(void)
 	vm_gic_destroy(&v);
 }
 
+static void test_v3_nassgicap(void)
+{
+	struct kvm_vcpu *vcpus[NR_VCPUS];
+	struct vm_gic vm;
+	__u8 nassgicap;
+	int ret;
+
+	vm = vm_gic_create_with_vcpus(KVM_DEV_TYPE_ARM_VGIC_V3, NR_VCPUS, vcpus);
+	TEST_REQUIRE(!__kvm_has_device_attr(vm.gic_fd, KVM_DEV_ARM_VGIC_GRP_FEATURES,
+					    KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap));
+
+	kvm_device_attr_get(vm.gic_fd, KVM_DEV_ARM_VGIC_GRP_FEATURES,
+			    KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap, &nassgicap);
+	if (!nassgicap) {
+		nassgicap = true;
+		ret = __kvm_device_attr_set(vm.gic_fd, KVM_DEV_ARM_VGIC_GRP_FEATURES,
+					    KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap, &nassgicap);
+		TEST_ASSERT(ret && errno == EINVAL,
+			    "Enabled nASSGIcap even though it's unavailable");
+	} else {
+		nassgicap = false;
+		kvm_device_attr_set(vm.gic_fd, KVM_DEV_ARM_VGIC_GRP_FEATURES,
+				    KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap, &nassgicap);
+
+		nassgicap = true;
+		kvm_device_attr_set(vm.gic_fd, KVM_DEV_ARM_VGIC_GRP_FEATURES,
+				    KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap, &nassgicap);
+	}
+
+	kvm_device_attr_set(vm.gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
+			    KVM_DEV_ARM_VGIC_CTRL_INIT, NULL);
+
+	ret = __kvm_device_attr_set(vm.gic_fd, KVM_DEV_ARM_VGIC_GRP_FEATURES,
+				    KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap, &nassgicap);
+	TEST_ASSERT(ret && errno == EBUSY,
+		    "Configured nASSGIcap after initializing the VGIC");
+
+	vm_gic_destroy(&vm);
+}
+
 /*
  * Returns 0 if it's possible to create GIC device of a given type (V2 or V3).
  */
@@ -730,6 +770,7 @@ void run_tests(uint32_t gic_dev_type)
 		test_v3_last_bit_single_rdist();
 		test_v3_redist_ipa_range_check_at_vcpu_run();
 		test_v3_its_region();
+		test_v3_nassgicap();
 	}
 }
 
-- 
2.50.0.rc2.692.g299adb8693-goog

Re: [PATCH v3 0/4] KVM: arm64: Add attribute to control GICD_TYPER2.nASSGIcap

[Oliver Upton]

On Fri, Jun 13, 2025 at 03:52:34PM +0000, Raghavendra Rao Ananta wrote:
> A shortcoming of the GIC architecture is that there's an absolute limit on
> the number of vPEs that can be tracked by the ITS. It is possible that
> an operator is running a mix of VMs on a system, only wanting to provide
> a specific class of VMs with hardware interrupt injection support.
> 
> The series introduces KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap vGIC attribute to allow
> the userspace to control GICD_TYPER2.nASSGIcap (GICv4.1) on a per-VM basis.
> 
> v1: https://lore.kernel.org/kvmarm/20250514192159.1751538-1-rananta@google.com/
> 
> v1 -> v2: https://lore.kernel.org/all/20250531012545.709887-1-oliver.upton@linux.dev/
>  - Drop all use of GICv4 in the UAPI and KVM-internal helpers in favor
>    of nASSGIcap. This changes things around to model a guest feature,
>    not a host feature.
> 
>  - Consolidate UAPI into a single attribute and expect userspace to use
>    to read the attribute for discovery, much like we do with the ID
>    registers
> 
>  - Squash documentation together with implementation
> 
>  - Clean up maintenance IRQ attribute handling, which I ran into as part
>    of reviewing this series
> 
> v2 -> v3:
>  - Update checks in vgic-v3.c and vgic-v4.c to also include nASSGIcap (via
>    vgic_supports_direct_sgis()) that's configured by the userspace. (Oliver)
> 
> Oliver Upton (2):
>   KVM: arm64: Disambiguate support for vSGIs v. vLPIs
>   KVM: arm64: vgic-v3: Consolidate MAINT_IRQ handling

Make sure you run checkpatch next time before sending out, it should've
warned you about sending patches w/o including your SOB.

Thanks,
Oliver

Re: [PATCH v3 0/4] KVM: arm64: Add attribute to control GICD_TYPER2.nASSGIcap

[Raghavendra Rao Ananta]

On Fri, Jun 13, 2025 at 1:53 PM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> On Fri, Jun 13, 2025 at 03:52:34PM +0000, Raghavendra Rao Ananta wrote:
> > A shortcoming of the GIC architecture is that there's an absolute limit on
> > the number of vPEs that can be tracked by the ITS. It is possible that
> > an operator is running a mix of VMs on a system, only wanting to provide
> > a specific class of VMs with hardware interrupt injection support.
> >
> > The series introduces KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap vGIC attribute to allow
> > the userspace to control GICD_TYPER2.nASSGIcap (GICv4.1) on a per-VM basis.
> >
> > v1: https://lore.kernel.org/kvmarm/20250514192159.1751538-1-rananta@google.com/
> >
> > v1 -> v2: https://lore.kernel.org/all/20250531012545.709887-1-oliver.upton@linux.dev/
> >  - Drop all use of GICv4 in the UAPI and KVM-internal helpers in favor
> >    of nASSGIcap. This changes things around to model a guest feature,
> >    not a host feature.
> >
> >  - Consolidate UAPI into a single attribute and expect userspace to use
> >    to read the attribute for discovery, much like we do with the ID
> >    registers
> >
> >  - Squash documentation together with implementation
> >
> >  - Clean up maintenance IRQ attribute handling, which I ran into as part
> >    of reviewing this series
> >
> > v2 -> v3:
> >  - Update checks in vgic-v3.c and vgic-v4.c to also include nASSGIcap (via
> >    vgic_supports_direct_sgis()) that's configured by the userspace. (Oliver)
> >
> > Oliver Upton (2):
> >   KVM: arm64: Disambiguate support for vSGIs v. vLPIs
> >   KVM: arm64: vgic-v3: Consolidate MAINT_IRQ handling
>
> Make sure you run checkpatch next time before sending out, it should've
> warned you about sending patches w/o including your SOB.
>
Hmm, I do run checkpatch before sending, but I don't see any warning as such.

Example:
$ ./scripts/checkpatch.pl
v3-0001-KVM-arm64-Disambiguate-support-for-vSGIs-v.-vLPIs.patch
total: 0 errors, 0 warnings, 107 lines checked

v3-0001-KVM-arm64-Disambiguate-support-for-vSGIs-v.-vLPIs.patch has no
obvious style problems and is ready for submission.

I do see an option to tell the script to ignore the check:
--no-signoff, so I'm guessing it should check by default? Or is there
any other option?

Thank you.
Raghavendra

Re: [PATCH v3 3/4] KVM: arm64: Introduce attribute to control GICD_TYPER2.nASSGIcap

[Marc Zyngier]

On Fri, 13 Jun 2025 16:52:37 +0100,
Raghavendra Rao Ananta <rananta@google.com> wrote:
> 
> KVM unconditionally advertises GICD_TYPER2.nASSGIcap (which internally
> implies vSGIs) on GICv4.1 systems. Allow userspace to change whether a
> VM supports the feature. Only allow changes prior to VGIC initialization
> as at that point vPEs need to be allocated for the VM.
> 
> For convenience, bundle support for vLPIs and vSGIs behind this feature,
> allowing userspace to control vPE allocation for VMs in environments
> that may be constrained on vPE IDs.
> 
> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  .../virt/kvm/devices/arm-vgic-v3.rst          | 29 +++++++++++++++
>  arch/arm64/include/uapi/asm/kvm.h             |  3 ++
>  arch/arm64/kvm/vgic/vgic-init.c               |  3 ++
>  arch/arm64/kvm/vgic/vgic-kvm-device.c         | 37 +++++++++++++++++++
>  arch/arm64/kvm/vgic/vgic-mmio-v3.c            | 10 ++++-
>  arch/arm64/kvm/vgic/vgic-v3.c                 |  5 ++-
>  arch/arm64/kvm/vgic/vgic-v4.c                 |  2 +-
>  include/kvm/arm_vgic.h                        |  3 ++
>  8 files changed, 88 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/virt/kvm/devices/arm-vgic-v3.rst b/Documentation/virt/kvm/devices/arm-vgic-v3.rst
> index e860498b1e35..049d77eae591 100644
> --- a/Documentation/virt/kvm/devices/arm-vgic-v3.rst
> +++ b/Documentation/virt/kvm/devices/arm-vgic-v3.rst
> @@ -306,3 +306,32 @@ Groups:
>  
>      The vINTID specifies which interrupt is generated when the vGIC
>      must generate a maintenance interrupt. This must be a PPI.
> +
> +  KVM_DEV_ARM_VGIC_GRP_FEATURES
> +   Attributes:
> +
> +    KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap
> +      Control whether support for SGIs without an active state is exposed
> +      to the VM. attr->addr points to a __u8 value which indicates whether
> +      he feature is enabled / disabled.

s/he/the/

> +
> +      A value of 0 indicates that the feature is disabled. A nonzero value
> +      indicates that the feature is enabled.
> +
> +      This attribute can only be set prior to initializing the VGIC (i.e.
> +      KVM_DEV_ARM_VGIC_CTRL_INIT).
> +
> +      Support for SGIs without an active state depends on hardware support.
> +      Userspace can discover support for the feature by reading the
> +      attribute after creating a VGICv3. It is possible that
> +      KVM_DEV_ARM_VGIC_CTRL_INIT can later fail if this feature is enabled
> +      and KVM is unable to allocate GIC vPEs for the VM.

Can you please add a sentence about the default behaviour? We
currently rely on the GICv4.1 capabilities to be available by default,
and it'd be important to capture this.

> +
> +  Errors:
> +
> +    =======  ========================================================
> +    -ENXIO   Invalid attribute in attr->attr
> +    -EFAULT  Invalid user address in attr->addr
> +    -EBUSY   The VGIC has already been initialized
> +    -EINVAL  KVM doesn't support the requested feature setting
> +    =======  ========================================================
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index ed5f3892674c..41e9ce412afd 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -417,6 +417,7 @@ enum {
>  #define KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO  7
>  #define KVM_DEV_ARM_VGIC_GRP_ITS_REGS 8
>  #define KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ  9
> +#define KVM_DEV_ARM_VGIC_GRP_FEATURES 10
>  #define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT	10
>  #define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK \
>  			(0x3fffffULL << KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT)
> @@ -429,6 +430,8 @@ enum {
>  #define   KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES	3
>  #define   KVM_DEV_ARM_ITS_CTRL_RESET		4
>  
> +#define   KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap	0
> +
>  /* Device Control API on vcpu fd */
>  #define KVM_ARM_VCPU_PMU_V3_CTRL	0
>  #define   KVM_ARM_VCPU_PMU_V3_IRQ		0
> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> index 5e0e4559004b..944e24750ac4 100644
> --- a/arch/arm64/kvm/vgic/vgic-init.c
> +++ b/arch/arm64/kvm/vgic/vgic-init.c
> @@ -157,6 +157,9 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
>  
>  	kvm->arch.vgic.in_kernel = true;
>  	kvm->arch.vgic.vgic_model = type;
> +	if (type == KVM_DEV_TYPE_ARM_VGIC_V3)
> +		kvm->arch.vgic.nassgicap = kvm_vgic_global_state.has_gicv4_1 &&
> +					   gic_cpuif_has_vsgi();
>  
>  	kvm->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF;
>  
> diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> index e28cf68a49c3..629f56063a13 100644
> --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
> +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> @@ -626,6 +626,26 @@ static int vgic_v3_set_attr(struct kvm_device *dev,
>  		dev->kvm->arch.vgic.mi_intid = val;
>  		return 0;
>  	}
> +	case KVM_DEV_ARM_VGIC_GRP_FEATURES: {
> +		u8 __user *uaddr = (u8 __user *)attr->addr;
> +		u8 val;
> +
> +		if (attr->attr != KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap)
> +			return -ENXIO;
> +
> +		if (get_user(val, uaddr))
> +			return -EFAULT;
> +
> +		guard(mutex)(&dev->kvm->arch.config_lock);
> +		if (vgic_initialized(dev->kvm))
> +			return -EBUSY;
> +
> +		if (!(kvm_vgic_global_state.has_gicv4_1 && gic_cpuif_has_vsgi()) && val)
> +			return -EINVAL;
> +
> +		dev->kvm->arch.vgic.nassgicap = val;
> +		return 0;
> +	}
>  	default:
>  		return vgic_set_common_attr(dev, attr);
>  	}
> @@ -646,6 +666,17 @@ static int vgic_v3_get_attr(struct kvm_device *dev,
>  		guard(mutex)(&dev->kvm->arch.config_lock);
>  		return put_user(dev->kvm->arch.vgic.mi_intid, uaddr);
>  	}
> +	case KVM_DEV_ARM_VGIC_GRP_FEATURES: {
> +		u8 __user *uaddr = (u8 __user *)attr->addr;
> +		u8 val;
> +
> +		if (attr->attr != KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap)
> +			return -ENXIO;
> +
> +		guard(mutex)(&dev->kvm->arch.config_lock);
> +		val = dev->kvm->arch.vgic.nassgicap;
> +		return put_user(val, uaddr);
> +	}
>  	default:
>  		return vgic_get_common_attr(dev, attr);
>  	}
> @@ -683,8 +714,14 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
>  			return 0;
>  		case KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES:
>  			return 0;
> +		default:
> +			return -ENXIO;
>  		}
> +	case KVM_DEV_ARM_VGIC_GRP_FEATURES:
> +		return attr->attr != KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap ?
> +		       -ENXIO : 0;

Do we really want to advertise KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap even
when we don't have GICv4.1? This seems rather odd. My take on this API
is that this should report whether the feature is configurable, making
it backward compatible with older versions of KVM.

Thanks,

	M.

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

Re: [PATCH v3 3/4] KVM: arm64: Introduce attribute to control GICD_TYPER2.nASSGIcap

[Oliver Upton]

On Sat, Jun 21, 2025 at 09:50:48AM +0100, Marc Zyngier wrote:
> On Fri, 13 Jun 2025 16:52:37 +0100, Raghavendra Rao Ananta <rananta@google.com> wrote:
> > @@ -683,8 +714,14 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
> >  			return 0;
> >  		case KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES:
> >  			return 0;
> > +		default:
> > +			return -ENXIO;
> >  		}
> > +	case KVM_DEV_ARM_VGIC_GRP_FEATURES:
> > +		return attr->attr != KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap ?
> > +		       -ENXIO : 0;
> 
> Do we really want to advertise KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap even
> when we don't have GICv4.1? This seems rather odd. My take on this API
> is that this should report whether the feature is configurable, making
> it backward compatible with older versions of KVM.

So this was because of me, as I wanted nASSGIcap to behave exactly like
the ID registers. I do think exposing the capability unconditionally is
useful, as otherwise there's no way to definitively say whether or not
the underlying platform supports GICv4.1.

KVM_HAS_DEVICE_ATTR can't be used alone for probing since old kernels
use GICv4.1 but don't expose the attribute.

Does that make sense?

Thanks,
Oliver

Re: [PATCH v3 3/4] KVM: arm64: Introduce attribute to control GICD_TYPER2.nASSGIcap

[Marc Zyngier]

On Mon, 23 Jun 2025 09:40:46 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Sat, Jun 21, 2025 at 09:50:48AM +0100, Marc Zyngier wrote:
> > On Fri, 13 Jun 2025 16:52:37 +0100, Raghavendra Rao Ananta <rananta@google.com> wrote:
> > > @@ -683,8 +714,14 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
> > >  			return 0;
> > >  		case KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES:
> > >  			return 0;
> > > +		default:
> > > +			return -ENXIO;
> > >  		}
> > > +	case KVM_DEV_ARM_VGIC_GRP_FEATURES:
> > > +		return attr->attr != KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap ?
> > > +		       -ENXIO : 0;
> > 
> > Do we really want to advertise KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap even
> > when we don't have GICv4.1? This seems rather odd. My take on this API
> > is that this should report whether the feature is configurable, making
> > it backward compatible with older versions of KVM.
> 
> So this was because of me, as I wanted nASSGIcap to behave exactly like
> the ID registers. I do think exposing the capability unconditionally is
> useful, as otherwise there's no way to definitively say whether or not
> the underlying platform supports GICv4.1.
> 
> KVM_HAS_DEVICE_ATTR can't be used alone for probing since old kernels
> use GICv4.1 but don't expose the attribute.
> 
> Does that make sense?

My own reasoning is that if we expose the capability, userspace is
able to use it and rely on it to take effect (VPE allocation error
notwithstanding). This is not the case with this approach, and that's
at odds with the other attributes.

But taking a step back: if we want to control the nASSGIcap bit, why
don't we allow writing to GICD_TYPER2 from userspace? This does
matches your view that we treat it as an ID register (GICD_TYPER2
matches this definition if you squint hard enough). It also avoids
adding new UAPI with unusual semantics.

Has this been considered?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v3 3/4] KVM: arm64: Introduce attribute to control GICD_TYPER2.nASSGIcap

[Oliver Upton]

On Mon, Jun 23, 2025 at 10:05:42AM +0100, Marc Zyngier wrote:
> On Mon, 23 Jun 2025 09:40:46 +0100,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> > 
> > On Sat, Jun 21, 2025 at 09:50:48AM +0100, Marc Zyngier wrote:
> > > On Fri, 13 Jun 2025 16:52:37 +0100, Raghavendra Rao Ananta <rananta@google.com> wrote:
> > > > @@ -683,8 +714,14 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
> > > >  			return 0;
> > > >  		case KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES:
> > > >  			return 0;
> > > > +		default:
> > > > +			return -ENXIO;
> > > >  		}
> > > > +	case KVM_DEV_ARM_VGIC_GRP_FEATURES:
> > > > +		return attr->attr != KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap ?
> > > > +		       -ENXIO : 0;
> > > 
> > > Do we really want to advertise KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap even
> > > when we don't have GICv4.1? This seems rather odd. My take on this API
> > > is that this should report whether the feature is configurable, making
> > > it backward compatible with older versions of KVM.
> > 
> > So this was because of me, as I wanted nASSGIcap to behave exactly like
> > the ID registers. I do think exposing the capability unconditionally is
> > useful, as otherwise there's no way to definitively say whether or not
> > the underlying platform supports GICv4.1.
> > 
> > KVM_HAS_DEVICE_ATTR can't be used alone for probing since old kernels
> > use GICv4.1 but don't expose the attribute.
> > 
> > Does that make sense?
> 
> My own reasoning is that if we expose the capability, userspace is
> able to use it and rely on it to take effect (VPE allocation error
> notwithstanding). This is not the case with this approach, and that's
> at odds with the other attributes.
> 
> But taking a step back: if we want to control the nASSGIcap bit, why
> don't we allow writing to GICD_TYPER2 from userspace? This does
> matches your view that we treat it as an ID register (GICD_TYPER2
> matches this definition if you squint hard enough). It also avoids
> adding new UAPI with unusual semantics.

This approach would bring its own set of complications. At least right
now we allocate vPEs at vgic_init() but prevent register accesses prior
to initialization. If we want to bake this thing into GICD_TYPER2
directly we either need to relax this register to be accessed before
init or defer the vPE allocation later on.

I'm worried that the latter approach is gonna be a mess, and the
attribute was done to avoid a one-off accessor in the VGIC state. But if
you'd like to see it done that way then that's OK with me.

Thanks,
Oliver

Re: [PATCH v3 3/4] KVM: arm64: Introduce attribute to control GICD_TYPER2.nASSGIcap

[Marc Zyngier]

On Mon, 23 Jun 2025 10:25:53 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Mon, Jun 23, 2025 at 10:05:42AM +0100, Marc Zyngier wrote:
> > On Mon, 23 Jun 2025 09:40:46 +0100,
> > Oliver Upton <oliver.upton@linux.dev> wrote:
> > > 
> > > On Sat, Jun 21, 2025 at 09:50:48AM +0100, Marc Zyngier wrote:
> > > > On Fri, 13 Jun 2025 16:52:37 +0100, Raghavendra Rao Ananta <rananta@google.com> wrote:
> > > > > @@ -683,8 +714,14 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
> > > > >  			return 0;
> > > > >  		case KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES:
> > > > >  			return 0;
> > > > > +		default:
> > > > > +			return -ENXIO;
> > > > >  		}
> > > > > +	case KVM_DEV_ARM_VGIC_GRP_FEATURES:
> > > > > +		return attr->attr != KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap ?
> > > > > +		       -ENXIO : 0;
> > > > 
> > > > Do we really want to advertise KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap even
> > > > when we don't have GICv4.1? This seems rather odd. My take on this API
> > > > is that this should report whether the feature is configurable, making
> > > > it backward compatible with older versions of KVM.
> > > 
> > > So this was because of me, as I wanted nASSGIcap to behave exactly like
> > > the ID registers. I do think exposing the capability unconditionally is
> > > useful, as otherwise there's no way to definitively say whether or not
> > > the underlying platform supports GICv4.1.
> > > 
> > > KVM_HAS_DEVICE_ATTR can't be used alone for probing since old kernels
> > > use GICv4.1 but don't expose the attribute.
> > > 
> > > Does that make sense?
> > 
> > My own reasoning is that if we expose the capability, userspace is
> > able to use it and rely on it to take effect (VPE allocation error
> > notwithstanding). This is not the case with this approach, and that's
> > at odds with the other attributes.
> > 
> > But taking a step back: if we want to control the nASSGIcap bit, why
> > don't we allow writing to GICD_TYPER2 from userspace? This does
> > matches your view that we treat it as an ID register (GICD_TYPER2
> > matches this definition if you squint hard enough). It also avoids
> > adding new UAPI with unusual semantics.
> 
> This approach would bring its own set of complications. At least right
> now we allocate vPEs at vgic_init() but prevent register accesses prior
> to initialization. If we want to bake this thing into GICD_TYPER2
> directly we either need to relax this register to be accessed before
> init or defer the vPE allocation later on.
> 
> I'm worried that the latter approach is gonna be a mess, and the
> attribute was done to avoid a one-off accessor in the VGIC state. But if
> you'd like to see it done that way then that's OK with me.

I'm not convinced we need to change much. For example, we already
allow userspace writes to GICD_IIDR to set the version of the
emulation prior to vgic_init(). It doesn't feel like allowing TYPER2
writes to occur in a similar spot require anything invasive.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.