[PATCH v2 0/1] GIC v4.1: Disable VSGI support for GIC CPUIF < v4.1

[PATCH v2 0/1] GIC v4.1: Disable VSGI support for GIC CPUIF < v4.1

[Lorenzo Pieralisi]

This patchset is v2 of a previous version [1].

v1 -> v2:
	- Fixed vGIC behaviour according to v1 [1] review
	- Removed capability detection - rely on sanitised reg read
	- Added vsgi specific flag (for gic and kvm)

[1] https://lore.kernel.org/linux-arm-kernel/20201111162841.3151-1-lorenzo.pieralisi@arm.com

-- Original cover letter --

GIC v4.1 introduced changes to the GIC CPU interface; systems that
integrate CPUs that do not support GIC v4.1 features (as reported in the
ID_AA64PFR0_EL1.GIC bitfield) and a GIC v4.1 controller must disable in
software virtual SGIs support since the CPUIF and GIC controller version
mismatch results in CONSTRAINED UNPREDICTABLE behaviour at architectural
level.

For systems with CPUs reporting ID_AA64PFR0_EL1.GIC == b0001 integrated
in a system with a GIC v4.1 it _should_ still be safe to enable vLPIs
(other than vSGI) since the protocol between the GIC redistributor and
the GIC CPUIF was not changed from GIC v4.0 to GIC v4.1.

Lorenzo Pieralisi (1):
  irqchip/gic-v4.1: Disable vSGI upon (GIC CPUIF < v4.1) detection

 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(-)

-- 
2.29.1

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

[Lorenzo Pieralisi]

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 <lorenzo.pieralisi@arm.com>
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;
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
+
 #define GICD_INT_NMI_PRI	(GICD_INT_DEF_PRI & ~0x80)
 
 #define FLAGS_WORKAROUND_GICR_WAKER_MSM8996	(1ULL << 0)
@@ -1679,6 +1694,8 @@ static int __init gic_init_bases(void __iomem *dist_base,
 	gic_data.rdists.has_direct_lpi = true;
 	gic_data.rdists.has_vpend_valid_dirty = true;
 
+	gic_data.rdists.has_vsgi_cpuif = gic_cpuif_has_vsgi();
+
 	if (WARN_ON(!gic_data.domain) || WARN_ON(!gic_data.rdists.rdist)) {
 		err = -ENOMEM;
 		goto out_free;
@@ -1852,6 +1869,9 @@ static void __init gic_of_setup_kvm_info(struct device_node *node)
 
 	gic_v3_kvm_info.has_v4 = gic_data.rdists.has_vlpis;
 	gic_v3_kvm_info.has_v4_1 = gic_data.rdists.has_rvpeid;
+	gic_v3_kvm_info.has_v4_1_vsgi = gic_data.rdists.has_vsgi_cpuif &&
+					gic_data.rdists.has_rvpeid;
+
 	gic_set_kvm_info(&gic_v3_kvm_info);
 }
 
@@ -2168,6 +2188,8 @@ static void __init gic_acpi_setup_kvm_info(void)
 
 	gic_v3_kvm_info.has_v4 = gic_data.rdists.has_vlpis;
 	gic_v3_kvm_info.has_v4_1 = gic_data.rdists.has_rvpeid;
+	gic_v3_kvm_info.has_v4_1_vsgi = gic_data.rdists.has_vsgi_cpuif &&
+					gic_data.rdists.has_rvpeid;
 	gic_set_kvm_info(&gic_v3_kvm_info);
 }
 
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 3d74f1060bd1..614f11113298 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -71,6 +71,7 @@ struct vgic_global {
 	/* Hardware has GICv4? */
 	bool			has_gicv4;
 	bool			has_gicv4_1;
+	bool			has_gicv4_1_vsgi;
 
 	/* GIC system register CPU interface */
 	struct static_key_false gicv3_cpuif;
diff --git a/include/linux/irqchip/arm-gic-common.h b/include/linux/irqchip/arm-gic-common.h
index fa8c0455c352..c2efd1dd1790 100644
--- a/include/linux/irqchip/arm-gic-common.h
+++ b/include/linux/irqchip/arm-gic-common.h
@@ -34,6 +34,8 @@ struct gic_kvm_info {
 	bool		has_v4;
 	/* rvpeid support */
 	bool		has_v4_1;
+	/* vsgi support */
+	bool		has_v4_1_vsgi;
 };
 
 const struct gic_kvm_info *gic_get_kvm_info(void);
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index f6d092fdb93d..5f00ce2535bf 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -683,6 +683,7 @@ struct rdists {
 	bool			has_vlpis;
 	bool			has_rvpeid;
 	bool			has_direct_lpi;
+	bool			has_vsgi_cpuif;
 	bool			has_vpend_valid_dirty;
 };
 
-- 
2.29.1

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

[Marc Zyngier]

Hi Lorenzo,

On Tue, 02 Mar 2021 10:27:44 +0000,
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> 
> 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 <lorenzo.pieralisi@arm.com>
> 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.

> 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,

	M.

diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index 15a6c98ee92f..2f1b156021a6 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 && gic_cpuif_has_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 || !gic_cpuif_has_vsgi())
 			val &= ~GICD_CTLR_nASSGIreq;
 
 		/* Dist stays enabled? nASSGIreq is RO */
diff --git a/drivers/irqchip/irq-gic-v4.c b/drivers/irqchip/irq-gic-v4.c
index 5d1dc9915272..864fa9bbda4c 100644
--- a/drivers/irqchip/irq-gic-v4.c
+++ b/drivers/irqchip/irq-gic-v4.c
@@ -87,17 +87,37 @@ static struct irq_domain *gic_domain;
 static const struct irq_domain_ops *vpe_domain_ops;
 static const struct irq_domain_ops *sgi_domain_ops;
 
+#ifdef CONFIG_ARM64
+#include <asm/cpufeature.h>
+
+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
+bool gic_cpuif_has_vsgi(void) { }
+#endif
+
 static bool has_v4_1(void)
 {
 	return !!sgi_domain_ops;
 }
 
+static bool has_v4_1_sgi(void)
+{
+	return has_v4_1() && gic_cpuif_has_vsgi();
+}
+
 static int its_alloc_vcpu_sgis(struct its_vpe *vpe, int idx)
 {
 	char *name;
 	int sgi_base;
 
-	if (!has_v4_1())
+	if (!has_v4_1_sgi())
 		return 0;
 
 	name = kasprintf(GFP_KERNEL, "GICv4-sgi-%d", task_pid_nr(current));
@@ -182,7 +202,7 @@ static void its_free_sgi_irqs(struct its_vm *vm)
 {
 	int i;
 
-	if (!has_v4_1())
+	if (!has_v4_1_sgi())
 		return;
 
 	for (i = 0; i < vm->nr_vpes; i++) {
diff --git a/include/linux/irqchip/arm-gic-v4.h b/include/linux/irqchip/arm-gic-v4.h
index 943c3411ca10..2c63375bbd43 100644
--- a/include/linux/irqchip/arm-gic-v4.h
+++ b/include/linux/irqchip/arm-gic-v4.h
@@ -145,4 +145,6 @@ int its_init_v4(struct irq_domain *domain,
 		const struct irq_domain_ops *vpe_ops,
 		const struct irq_domain_ops *sgi_ops);
 
+bool gic_cpuif_has_vsgi(void);
+
 #endif

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

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

[Lorenzo Pieralisi]

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 <lorenzo.pieralisi@arm.com> wrote:
> > 
> > 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 <lorenzo.pieralisi@arm.com>
> > 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.

> > 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

> Thanks,
> 
> 	M.
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> index 15a6c98ee92f..2f1b156021a6 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 && gic_cpuif_has_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 || !gic_cpuif_has_vsgi())
>  			val &= ~GICD_CTLR_nASSGIreq;
>  
>  		/* Dist stays enabled? nASSGIreq is RO */
> diff --git a/drivers/irqchip/irq-gic-v4.c b/drivers/irqchip/irq-gic-v4.c
> index 5d1dc9915272..864fa9bbda4c 100644
> --- a/drivers/irqchip/irq-gic-v4.c
> +++ b/drivers/irqchip/irq-gic-v4.c
> @@ -87,17 +87,37 @@ static struct irq_domain *gic_domain;
>  static const struct irq_domain_ops *vpe_domain_ops;
>  static const struct irq_domain_ops *sgi_domain_ops;
>  
> +#ifdef CONFIG_ARM64
> +#include <asm/cpufeature.h>
> +
> +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
> +bool gic_cpuif_has_vsgi(void) { }
> +#endif
> +
>  static bool has_v4_1(void)
>  {
>  	return !!sgi_domain_ops;
>  }
>  
> +static bool has_v4_1_sgi(void)
> +{
> +	return has_v4_1() && gic_cpuif_has_vsgi();
> +}
> +
>  static int its_alloc_vcpu_sgis(struct its_vpe *vpe, int idx)
>  {
>  	char *name;
>  	int sgi_base;
>  
> -	if (!has_v4_1())
> +	if (!has_v4_1_sgi())
>  		return 0;
>  
>  	name = kasprintf(GFP_KERNEL, "GICv4-sgi-%d", task_pid_nr(current));
> @@ -182,7 +202,7 @@ static void its_free_sgi_irqs(struct its_vm *vm)
>  {
>  	int i;
>  
> -	if (!has_v4_1())
> +	if (!has_v4_1_sgi())
>  		return;
>  
>  	for (i = 0; i < vm->nr_vpes; i++) {
> diff --git a/include/linux/irqchip/arm-gic-v4.h b/include/linux/irqchip/arm-gic-v4.h
> index 943c3411ca10..2c63375bbd43 100644
> --- a/include/linux/irqchip/arm-gic-v4.h
> +++ b/include/linux/irqchip/arm-gic-v4.h
> @@ -145,4 +145,6 @@ int its_init_v4(struct irq_domain *domain,
>  		const struct irq_domain_ops *vpe_ops,
>  		const struct irq_domain_ops *sgi_ops);
>  
> +bool gic_cpuif_has_vsgi(void);
> +
>  #endif
> 
> -- 
> Without deviation from the norm, progress is not possible.

[PATCH v3 0/1] GIC v4.1: Disable VSGI support for GIC CPUIF < v4.1

[Lorenzo Pieralisi]

This patchset is v3 of a previous version [1].

v2 -> v3:
	- Coalesced all checks in one function (Marc's feedback)
	- Allow sgi_ops on cpuif mismatch (to keep v4.1 doorbell
          mechanism that works fine even if GIC CPUIF < v4.1)

v1 -> v2:
	- Fixed vGIC behaviour according to v1 [1] review
	- Removed capability detection - rely on sanitised reg read
	- Added vsgi specific flag (for gic and kvm)

[1] https://lore.kernel.org/linux-arm-kernel/20210302102744.12692-1-lorenzo.pieralisi@arm.com

-- Original cover letter --

GIC v4.1 introduced changes to the GIC CPU interface; systems that
integrate CPUs that do not support GIC v4.1 features (as reported in the
ID_AA64PFR0_EL1.GIC bitfield) and a GIC v4.1 controller must disable in
software virtual SGIs support since the CPUIF and GIC controller version
mismatch results in CONSTRAINED UNPREDICTABLE behaviour at architectural
level.

For systems with CPUs reporting ID_AA64PFR0_EL1.GIC == b0001 integrated
in a system with a GIC v4.1 it _should_ still be safe to enable vLPIs
(other than vSGI) since the protocol between the GIC redistributor and
the GIC CPUIF was not changed from GIC v4.0 to GIC v4.1.

Cc: Marc Zyngier <maz@kernel.org>

Lorenzo Pieralisi (1):
  irqchip/gic-v4.1: Disable vSGI upon (GIC CPUIF < v4.1) detection

 arch/arm64/kvm/vgic/vgic-mmio-v3.c |  4 ++--
 drivers/irqchip/irq-gic-v4.c       | 27 +++++++++++++++++++++++++--
 include/linux/irqchip/arm-gic-v4.h |  2 ++
 3 files changed, 29 insertions(+), 4 deletions(-)

-- 
2.29.1

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

[Lorenzo Pieralisi]

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 <lorenzo.pieralisi@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/vgic/vgic-mmio-v3.c |  4 ++--
 drivers/irqchip/irq-gic-v4.c       | 27 +++++++++++++++++++++++++--
 include/linux/irqchip/arm-gic-v4.h |  2 ++
 3 files changed, 29 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..2f1b156021a6 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 && gic_cpuif_has_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 || !gic_cpuif_has_vsgi())
 			val &= ~GICD_CTLR_nASSGIreq;
 
 		/* Dist stays enabled? nASSGIreq is RO */
diff --git a/drivers/irqchip/irq-gic-v4.c b/drivers/irqchip/irq-gic-v4.c
index 5d1dc9915272..4ea71b28f9f5 100644
--- a/drivers/irqchip/irq-gic-v4.c
+++ b/drivers/irqchip/irq-gic-v4.c
@@ -87,17 +87,40 @@ static struct irq_domain *gic_domain;
 static const struct irq_domain_ops *vpe_domain_ops;
 static const struct irq_domain_ops *sgi_domain_ops;
 
+#ifdef CONFIG_ARM64
+#include <asm/cpufeature.h>
+
+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
+bool gic_cpuif_has_vsgi(void)
+{
+	return false;
+}
+#endif
+
 static bool has_v4_1(void)
 {
 	return !!sgi_domain_ops;
 }
 
+static bool has_v4_1_sgi(void)
+{
+	return has_v4_1() && gic_cpuif_has_vsgi();
+}
+
 static int its_alloc_vcpu_sgis(struct its_vpe *vpe, int idx)
 {
 	char *name;
 	int sgi_base;
 
-	if (!has_v4_1())
+	if (!has_v4_1_sgi())
 		return 0;
 
 	name = kasprintf(GFP_KERNEL, "GICv4-sgi-%d", task_pid_nr(current));
@@ -182,7 +205,7 @@ static void its_free_sgi_irqs(struct its_vm *vm)
 {
 	int i;
 
-	if (!has_v4_1())
+	if (!has_v4_1_sgi())
 		return;
 
 	for (i = 0; i < vm->nr_vpes; i++) {
diff --git a/include/linux/irqchip/arm-gic-v4.h b/include/linux/irqchip/arm-gic-v4.h
index 943c3411ca10..2c63375bbd43 100644
--- a/include/linux/irqchip/arm-gic-v4.h
+++ b/include/linux/irqchip/arm-gic-v4.h
@@ -145,4 +145,6 @@ int its_init_v4(struct irq_domain *domain,
 		const struct irq_domain_ops *vpe_ops,
 		const struct irq_domain_ops *sgi_ops);
 
+bool gic_cpuif_has_vsgi(void);
+
 #endif
-- 
2.29.1

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

[Marc Zyngier]

Hi Lorenzo,

Wed, 17 Mar 2021 10:07:19 +0000,
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> 
> 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 <lorenzo.pieralisi@arm.com>
> Cc: Marc Zyngier <maz@kernel.org>

Does it need to go in as a fix? or can it just be pushed into 5.13?
Given that there is no such HW in the wild just yet, I'm inclined to
do the latter...

Thanks,

	M.

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

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

[Lorenzo Pieralisi]

On Wed, Mar 17, 2021 at 02:04:36PM +0000, Marc Zyngier wrote:
> Hi Lorenzo,
> 
> Wed, 17 Mar 2021 10:07:19 +0000,
> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> > 
> > 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 <lorenzo.pieralisi@arm.com>
> > Cc: Marc Zyngier <maz@kernel.org>
> 
> Does it need to go in as a fix? or can it just be pushed into 5.13?
> Given that there is no such HW in the wild just yet, I'm inclined to
> do the latter...

I agree with you; it is to make the driver/vgic robust against HW
misconfigurations (because that's what they are and I am not aware of
any _existing_ HW with such a misconfiguration - yet), 5.13 will
perfectly do.

Thanks a lot.

Lorenzo