Re: [PATCH v2 1/1] irqchip/gic-v4.1: Disable vSGI upon (GIC CPUIF < v4.1) detection

On Mon, Mar 08, 2021 at 07:22:57PM +0000, Marc Zyngier wrote:

Hi Lorenzo,

On Tue, 02 Mar 2021 10:27:44 +0000,
Lorenzo Pieralisi [off-list ref] wrote:

quoted

GIC CPU interfaces versions predating GIC v4.1 were not built to
accommodate vINTID within the vSGI range; as reported in the GIC
specifications (8.2 "Changes to the CPU interface"), it is
CONSTRAINED UNPREDICTABLE to deliver a vSGI to a PE with
ID_AA64PFR0_EL1.GIC < b0011.

Check the GIC CPUIF version by reading the SYS_ID_AA64_PFR0_EL1.

Disable vSGIs if a CPUIF version < 4.1 is detected to prevent using
vSGIs on systems where they may misbehave.

Signed-off-by: Lorenzo Pieralisi <redacted>
Cc: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/vgic/vgic-mmio-v3.c     |  4 ++--
 arch/arm64/kvm/vgic/vgic-v3.c          |  3 ++-
 drivers/irqchip/irq-gic-v3-its.c       |  6 +++++-
 drivers/irqchip/irq-gic-v3.c           | 22 ++++++++++++++++++++++
 include/kvm/arm_vgic.h                 |  1 +
 include/linux/irqchip/arm-gic-common.h |  2 ++
 include/linux/irqchip/arm-gic-v3.h     |  1 +
 7 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index 15a6c98ee92f..66548cd2a715 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c

@@ -86,7 +86,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)
+		if (kvm_vgic_global_state.has_gicv4_1_vsgi)
 			value = GICD_TYPER2_nASSGIcap;
 		break;
 	case GICD_IIDR:

@@ -119,7 +119,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)
+		if (!kvm_vgic_global_state.has_gicv4_1_vsgi)
 			val &= ~GICD_CTLR_nASSGIreq;
 
 		/* Dist stays enabled? nASSGIreq is RO */

diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
index 52915b342351..57b73100e8cc 100644
--- a/arch/arm64/kvm/vgic/vgic-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-v3.c

@@ -533,7 +533,7 @@ int vgic_v3_map_resources(struct kvm *kvm)
 		return ret;
 	}
 
-	if (kvm_vgic_global_state.has_gicv4_1)
+	if (kvm_vgic_global_state.has_gicv4_1_vsgi)
 		vgic_v4_configure_vsgis(kvm);
 
 	return 0;

@@ -589,6 +589,7 @@ int vgic_v3_probe(const struct gic_kvm_info *info)
 	if (info->has_v4) {
 		kvm_vgic_global_state.has_gicv4 = gicv4_enable;
 		kvm_vgic_global_state.has_gicv4_1 = info->has_v4_1 && gicv4_enable;
+		kvm_vgic_global_state.has_gicv4_1_vsgi = info->has_v4_1_vsgi && gicv4_enable;
 		kvm_info("GICv4%s support %sabled\n",
 			 kvm_vgic_global_state.has_gicv4_1 ? ".1" : "",
 			 gicv4_enable ? "en" : "dis");

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index ed46e6057e33..ee2a2ca27d5c 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c

@@ -5412,7 +5412,11 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
 	if (has_v4 & rdists->has_vlpis) {
 		const struct irq_domain_ops *sgi_ops;
 
-		if (has_v4_1)
+		/*
+		 * Enable vSGIs only if the ITS and the
+		 * GIC CPUIF support them.
+		 */
+		if (has_v4_1 && rdists->has_vsgi_cpuif)
 			sgi_ops = &its_sgi_domain_ops;
 		else
 			sgi_ops = NULL;

This doesn't seem right. If you pass NULL for the SGI ops, you also
lose the per-VPE doorbells and stick to the terrible GICv4.0 behaviour
(see the use of has_v4_1() in irq-gic-v4.c). I don't think that is
what you really want.

Yes, I was caught out again - we use the sgi_ops to detect v4.1 behaviour,
I will remove this hunk.

quoted

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index eb0ee356a629..fd6cd9a5de34 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c

@@ -31,6 +31,21 @@
 
 #include "irq-gic-common.h"
 
+#ifdef CONFIG_ARM64
+#include <asm/cpufeature.h>
+
+static inline bool gic_cpuif_has_vsgi(void)
+{
+	unsigned long fld, reg = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1);
+
+	fld = cpuid_feature_extract_unsigned_field(reg, ID_AA64PFR0_GIC_SHIFT);
+
+	return fld >= 0x3;
+}
+#else
+static inline bool gic_cpuif_has_vsgi(void) { return false; }
+#endif

Why do we need to expose this in the GICv3 driver instead of the GICv4
code? At the moment, you track this state:

- in gic_data.rdists.has_vsgi_cpuif
- indirectly in gic_v3_kvm_info.has_v4_1_vsgi
- indirectly in kvm_vgic_global_state.has_gicv4_1_vsgi

Can't we simplify the logic and track it *once*? Or even better, just
evaluate it when required? I hacked the following stuff based on your
patch (untested). What do you think?

Thanks for that I will give it a shot - it makes sense. I will have
a look to see if we can consolidate these v4.0 vs v4.1 checks somehow
before reposting.

Thanks a lot for the review.

Lorenzo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel