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 +, Marc Zyngier wrote:
> Hi Lorenzo,
>
> On Tue, 02 Mar 2021 10:27:44 +,
> Lorenzo Pieralisi 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
> > Cc: Marc Zyngier
> > ---
> > 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 = _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
> > +
> > +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
>
Re: [PATCH v2 1/1] irqchip/gic-v4.1: Disable vSGI upon (GIC CPUIF < v4.1) detection
Hi Lorenzo,
On Tue, 02 Mar 2021 10:27:44 +,
Lorenzo Pieralisi 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
> Cc: Marc Zyngier
> ---
> 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 = _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
> +
> +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
[PATCH v2 1/1] irqchip/gic-v4.1: Disable vSGI upon (GIC CPUIF < v4.1) detection
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
Cc: Marc Zyngier
---
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 = _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
+
+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 =
