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

From: Marc Zyngier
Date: Thu Nov 12 2020 - 04:36:15 EST


Hi Lorenzo,

On 2020-11-11 16:28, 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 == b0001.

Hmmm. This goes against the very reason v4.1 was designed the way
it is, which was that all existing implementation supporting GICv4.0
would seamlessly let virtual SGIs in, and it would "just work".

If we start enforcing this, I question the very design of the architecture,
because we could have done so much better by changing the CPU interface.

What has changed in two years? Have you spotted a fundamental problem?
My concern is that if we prevent it, we're going to end-up with quirks
allowing it anyway, because people will realise that it actually works.

In the meantime, to the meat of the change:


Check the GIC CPUIF version through the arm64 capabilities
infrastructure and 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@xxxxxxx>
Cc: Marc Zyngier <maz@xxxxxxxxxx>
---
drivers/irqchip/irq-gic-v3-its.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 0fec31931e11..6ed4ba60ba7e 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -39,6 +39,20 @@

#include "irq-gic-common.h"

+#ifdef CONFIG_ARM64
+#include <asm/cpufeature.h>
+
+static inline bool gic_cpuif_has_vsgi(void)
+{
+ return cpus_have_const_cap(ARM64_HAS_GIC_CPUIF_VSGI);
+}
+#else
+static inline bool gic_cpuif_has_vsgi(void)
+{
+ return false;
+}
+#endif
+
#define ITS_FLAGS_CMDQ_NEEDS_FLUSHING (1ULL << 0)
#define ITS_FLAGS_WORKAROUND_CAVIUM_22375 (1ULL << 1)
#define ITS_FLAGS_WORKAROUND_CAVIUM_23144 (1ULL << 2)
@@ -5415,7 +5429,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 && gic_cpuif_has_vsgi())
sgi_ops = &its_sgi_domain_ops;
else
sgi_ops = NULL;

Is that enough?

KVM is still going to expose GICD_TYPER2.nASSGIcap, making things even
more confusing for the guest: it will be able to select active-less SGIs
via GICD_CTLR.nASSGIreq, and if I'm not mistaken, we'd still try to switch
to HW-backed SGIs, leading to some *very* unpleasant things in
gic_v4_enable_vsgis().

Thanks,

M.
--
Jazz is not dead. It just smells funny...