* [PATCH v2 0/1] GIC v4.1: Disable VSGI support for GIC CPUIF < v4.1 [not found] <0201111162841.3151-1-lorenzo.pieralisi@arm.com> @ 2021-03-02 10:27 ` Lorenzo Pieralisi 2021-03-02 10:27 ` [PATCH v2 1/1] irqchip/gic-v4.1: Disable vSGI upon (GIC CPUIF < v4.1) detection Lorenzo Pieralisi 2021-03-17 10:07 ` [PATCH v3 0/1] GIC v4.1: Disable VSGI support for GIC CPUIF < v4.1 Lorenzo Pieralisi 0 siblings, 2 replies; 8+ messages in thread From: Lorenzo Pieralisi @ 2021-03-02 10:27 UTC (permalink / raw) To: linux-kernel; +Cc: Marc Zyngier, LAKML, KVM 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/1] irqchip/gic-v4.1: Disable vSGI upon (GIC CPUIF < v4.1) detection 2021-03-02 10:27 ` [PATCH v2 0/1] GIC v4.1: Disable VSGI support for GIC CPUIF < v4.1 Lorenzo Pieralisi @ 2021-03-02 10:27 ` Lorenzo Pieralisi 2021-03-08 19:22 ` Marc Zyngier 2021-03-17 10:07 ` [PATCH v3 0/1] GIC v4.1: Disable VSGI support for GIC CPUIF < v4.1 Lorenzo Pieralisi 1 sibling, 1 reply; 8+ messages in thread From: Lorenzo Pieralisi @ 2021-03-02 10:27 UTC (permalink / raw) To: linux-kernel; +Cc: Marc Zyngier, LAKML, KVM 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/1] irqchip/gic-v4.1: Disable vSGI upon (GIC CPUIF < v4.1) detection 2021-03-02 10:27 ` [PATCH v2 1/1] irqchip/gic-v4.1: Disable vSGI upon (GIC CPUIF < v4.1) detection Lorenzo Pieralisi @ 2021-03-08 19:22 ` Marc Zyngier 2021-03-15 11:02 ` Lorenzo Pieralisi 0 siblings, 1 reply; 8+ messages in thread From: Marc Zyngier @ 2021-03-08 19:22 UTC (permalink / raw) To: Lorenzo Pieralisi; +Cc: linux-kernel, LAKML, KVM 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. ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/1] irqchip/gic-v4.1: Disable vSGI upon (GIC CPUIF < v4.1) detection 2021-03-08 19:22 ` Marc Zyngier @ 2021-03-15 11:02 ` Lorenzo Pieralisi 0 siblings, 0 replies; 8+ messages in thread From: Lorenzo Pieralisi @ 2021-03-15 11:02 UTC (permalink / raw) To: Marc Zyngier; +Cc: linux-kernel, LAKML, KVM 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 0/1] GIC v4.1: Disable VSGI support for GIC CPUIF < v4.1 2021-03-02 10:27 ` [PATCH v2 0/1] GIC v4.1: Disable VSGI support for GIC CPUIF < v4.1 Lorenzo Pieralisi 2021-03-02 10:27 ` [PATCH v2 1/1] irqchip/gic-v4.1: Disable vSGI upon (GIC CPUIF < v4.1) detection Lorenzo Pieralisi @ 2021-03-17 10:07 ` Lorenzo Pieralisi 2021-03-17 10:07 ` [PATCH v3 1/1] irqchip/gic-v4.1: Disable vSGI upon (GIC CPUIF < v4.1) detection Lorenzo Pieralisi 1 sibling, 1 reply; 8+ messages in thread From: Lorenzo Pieralisi @ 2021-03-17 10:07 UTC (permalink / raw) To: linux-kernel; +Cc: Marc Zyngier, LAKML, KVM 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 1/1] irqchip/gic-v4.1: Disable vSGI upon (GIC CPUIF < v4.1) detection 2021-03-17 10:07 ` [PATCH v3 0/1] GIC v4.1: Disable VSGI support for GIC CPUIF < v4.1 Lorenzo Pieralisi @ 2021-03-17 10:07 ` Lorenzo Pieralisi 2021-03-17 14:04 ` Marc Zyngier 0 siblings, 1 reply; 8+ messages in thread From: Lorenzo Pieralisi @ 2021-03-17 10:07 UTC (permalink / raw) To: linux-kernel; +Cc: Marc Zyngier, LAKML, KVM 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/1] irqchip/gic-v4.1: Disable vSGI upon (GIC CPUIF < v4.1) detection 2021-03-17 10:07 ` [PATCH v3 1/1] irqchip/gic-v4.1: Disable vSGI upon (GIC CPUIF < v4.1) detection Lorenzo Pieralisi @ 2021-03-17 14:04 ` Marc Zyngier 2021-03-17 14:40 ` Lorenzo Pieralisi 0 siblings, 1 reply; 8+ messages in thread From: Marc Zyngier @ 2021-03-17 14:04 UTC (permalink / raw) To: Lorenzo Pieralisi; +Cc: linux-kernel, LAKML, KVM 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/1] irqchip/gic-v4.1: Disable vSGI upon (GIC CPUIF < v4.1) detection 2021-03-17 14:04 ` Marc Zyngier @ 2021-03-17 14:40 ` Lorenzo Pieralisi 0 siblings, 0 replies; 8+ messages in thread From: Lorenzo Pieralisi @ 2021-03-17 14:40 UTC (permalink / raw) To: Marc Zyngier; +Cc: linux-kernel, LAKML, KVM 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-03-17 14:40 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <0201111162841.3151-1-lorenzo.pieralisi@arm.com>
2021-03-02 10:27 ` [PATCH v2 0/1] GIC v4.1: Disable VSGI support for GIC CPUIF < v4.1 Lorenzo Pieralisi
2021-03-02 10:27 ` [PATCH v2 1/1] irqchip/gic-v4.1: Disable vSGI upon (GIC CPUIF < v4.1) detection Lorenzo Pieralisi
2021-03-08 19:22 ` Marc Zyngier
2021-03-15 11:02 ` Lorenzo Pieralisi
2021-03-17 10:07 ` [PATCH v3 0/1] GIC v4.1: Disable VSGI support for GIC CPUIF < v4.1 Lorenzo Pieralisi
2021-03-17 10:07 ` [PATCH v3 1/1] irqchip/gic-v4.1: Disable vSGI upon (GIC CPUIF < v4.1) detection Lorenzo Pieralisi
2021-03-17 14:04 ` Marc Zyngier
2021-03-17 14:40 ` Lorenzo Pieralisi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).