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

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

[Lorenzo Pieralisi]

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: Will Deacon <will@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Marc Zyngier <maz@kernel.org>

Lorenzo Pieralisi (2):
  arm64: cpufeature: Add GIC CPUIF v4.1 detection
  irqchip/gic-v3-its: Disable vSGI upon (CPUIF < v4.1) detection

 arch/arm64/include/asm/cpucaps.h |  3 ++-
 arch/arm64/kernel/cpufeature.c   | 10 ++++++++++
 drivers/irqchip/irq-gic-v3-its.c | 20 +++++++++++++++++++-
 3 files changed, 31 insertions(+), 2 deletions(-)

-- 
2.29.1

[PATCH 1/2] arm64: cpufeature: Add GIC CPUIF v4.1 detection

[Lorenzo Pieralisi]

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.

Add a cpufeature and related capability to detect GIC v4.1 CPUIF
features so that the GIC driver can probe it to detect GIC CPUIF
hardware configuration and take action accordingly.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/include/asm/cpucaps.h |  3 ++-
 arch/arm64/kernel/cpufeature.c   | 10 ++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
index 42868dbd29fd..35ef0319f422 100644
--- a/arch/arm64/include/asm/cpucaps.h
+++ b/arch/arm64/include/asm/cpucaps.h
@@ -65,7 +65,8 @@
 #define ARM64_HAS_ARMv8_4_TTL			55
 #define ARM64_HAS_TLB_RANGE			56
 #define ARM64_MTE				57
+#define ARM64_HAS_GIC_CPUIF_VSGI		58
 
-#define ARM64_NCAPS				58
+#define ARM64_NCAPS				59
 
 #endif /* __ASM_CPUCAPS_H */
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index dcc165b3fc04..9eabbaddfe5e 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -2136,6 +2136,16 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
 		.cpu_enable = cpu_enable_mte,
 	},
 #endif /* CONFIG_ARM64_MTE */
+	{
+		.desc = "GIC CPUIF virtual SGI",
+		.capability = ARM64_HAS_GIC_CPUIF_VSGI,
+		.type = ARM64_CPUCAP_BOOT_CPU_FEATURE,
+		.matches = has_cpuid_feature,
+		.sys_reg = SYS_ID_AA64PFR0_EL1,
+		.field_pos = ID_AA64PFR0_GIC_SHIFT,
+		.sign = FTR_UNSIGNED,
+		.min_field_value = 3,
+	},
 	{},
 };
 
-- 
2.29.1

[PATCH 2/2] irqchip/gic-v3-its: Disable vSGI upon (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 == b0001.

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@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
---
 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;
-- 
2.29.1

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

[Marc Zyngier]

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@arm.com>
> Cc: Marc Zyngier <maz@kernel.org>
> ---
>  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...

Re: [PATCH 1/2] arm64: cpufeature: Add GIC CPUIF v4.1 detection

[Marc Zyngier]

On 2020-11-11 16:28, Lorenzo Pieralisi wrote:
> 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.
> 
> Add a cpufeature and related capability to detect GIC v4.1 CPUIF
> features so that the GIC driver can probe it to detect GIC CPUIF
> hardware configuration and take action accordingly.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/include/asm/cpucaps.h |  3 ++-
>  arch/arm64/kernel/cpufeature.c   | 10 ++++++++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/cpucaps.h 
> b/arch/arm64/include/asm/cpucaps.h
> index 42868dbd29fd..35ef0319f422 100644
> --- a/arch/arm64/include/asm/cpucaps.h
> +++ b/arch/arm64/include/asm/cpucaps.h
> @@ -65,7 +65,8 @@
>  #define ARM64_HAS_ARMv8_4_TTL			55
>  #define ARM64_HAS_TLB_RANGE			56
>  #define ARM64_MTE				57
> +#define ARM64_HAS_GIC_CPUIF_VSGI		58
> 
> -#define ARM64_NCAPS				58
> +#define ARM64_NCAPS				59
> 
>  #endif /* __ASM_CPUCAPS_H */
> diff --git a/arch/arm64/kernel/cpufeature.c 
> b/arch/arm64/kernel/cpufeature.c
> index dcc165b3fc04..9eabbaddfe5e 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -2136,6 +2136,16 @@ static const struct arm64_cpu_capabilities
> arm64_features[] = {
>  		.cpu_enable = cpu_enable_mte,
>  	},
>  #endif /* CONFIG_ARM64_MTE */
> +	{
> +		.desc = "GIC CPUIF virtual SGI",

nit: that's not really what this feature is. It only means that the
sysreg interface complies to v4.1. Which on its own is totally rubbish,
because the sysreg don't change behaviour between 3.0/4.0 and 4.1.

> +		.capability = ARM64_HAS_GIC_CPUIF_VSGI,
> +		.type = ARM64_CPUCAP_BOOT_CPU_FEATURE,
> +		.matches = has_cpuid_feature,
> +		.sys_reg = SYS_ID_AA64PFR0_EL1,
> +		.field_pos = ID_AA64PFR0_GIC_SHIFT,
> +		.sign = FTR_UNSIGNED,
> +		.min_field_value = 3,
> +	},

Do we really need a new cap for that? Or can we rely on simply looking
at the sanitised feature set? I'm not overly keen on advertising a 
feature
at CPU boot time if we discover later on that we cannot use it because 
all
we have in a non-4.1 GIC.

Another thing is that we currently assume that *all* CPUs will be the 
same
at the point where we setup the GIC (we only have a single CPU booted at 
that
point).

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

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

[Lorenzo Pieralisi]

On Thu, Nov 12, 2020 at 09:36:10AM +0000, Marc Zyngier wrote:
> 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?

Hi Marc,

long story short: there are systems being designed with this
configuration, vSGIs may or may not work on them, to prevent
*potential* misbehaviour I am disabling vSGIs, I am not fixing
anything, it is belt and braces.

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

We may wait and fix it *if* this breaks, I would argue though that at
that point it is not a quirk since architecturally we know that vSGIs
may not work in this configuration.

> 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@arm.com>
> > Cc: Marc Zyngier <maz@kernel.org>
> > ---
> >  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?

No, I obviously missed the VGIC bits built on top of
GICD_TYPER2.nASSGIcap.

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

Yes (AFAICS GICD_TYPER2.nASSGIcap is not in the public specs though,
that's why I missed it while vetting architectural state that is
affecting vSGIs).

I should change the logic in vgic_mmio_{uaccess}_write_v3_misc() to
handle it properly - to redefine the logic around

kvm_vgic_global_state.has_gicv4_1

somehow.

Thanks for the review.

Lorenzo

Re: [PATCH 1/2] arm64: cpufeature: Add GIC CPUIF v4.1 detection

[Lorenzo Pieralisi]

On Thu, Nov 12, 2020 at 11:20:08AM +0000, Marc Zyngier wrote:
> On 2020-11-11 16:28, Lorenzo Pieralisi wrote:
> > 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.
> > 
> > Add a cpufeature and related capability to detect GIC v4.1 CPUIF
> > features so that the GIC driver can probe it to detect GIC CPUIF
> > hardware configuration and take action accordingly.
> > 
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/include/asm/cpucaps.h |  3 ++-
> >  arch/arm64/kernel/cpufeature.c   | 10 ++++++++++
> >  2 files changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/include/asm/cpucaps.h
> > b/arch/arm64/include/asm/cpucaps.h
> > index 42868dbd29fd..35ef0319f422 100644
> > --- a/arch/arm64/include/asm/cpucaps.h
> > +++ b/arch/arm64/include/asm/cpucaps.h
> > @@ -65,7 +65,8 @@
> >  #define ARM64_HAS_ARMv8_4_TTL			55
> >  #define ARM64_HAS_TLB_RANGE			56
> >  #define ARM64_MTE				57
> > +#define ARM64_HAS_GIC_CPUIF_VSGI		58
> > 
> > -#define ARM64_NCAPS				58
> > +#define ARM64_NCAPS				59
> > 
> >  #endif /* __ASM_CPUCAPS_H */
> > diff --git a/arch/arm64/kernel/cpufeature.c
> > b/arch/arm64/kernel/cpufeature.c
> > index dcc165b3fc04..9eabbaddfe5e 100644
> > --- a/arch/arm64/kernel/cpufeature.c
> > +++ b/arch/arm64/kernel/cpufeature.c
> > @@ -2136,6 +2136,16 @@ static const struct arm64_cpu_capabilities
> > arm64_features[] = {
> >  		.cpu_enable = cpu_enable_mte,
> >  	},
> >  #endif /* CONFIG_ARM64_MTE */
> > +	{
> > +		.desc = "GIC CPUIF virtual SGI",
> 
> nit: that's not really what this feature is. It only means that the
> sysreg interface complies to v4.1. Which on its own is totally rubbish,
> because the sysreg don't change behaviour between 3.0/4.0 and 4.1.

True.

> > +		.capability = ARM64_HAS_GIC_CPUIF_VSGI,
> > +		.type = ARM64_CPUCAP_BOOT_CPU_FEATURE,
> > +		.matches = has_cpuid_feature,
> > +		.sys_reg = SYS_ID_AA64PFR0_EL1,
> > +		.field_pos = ID_AA64PFR0_GIC_SHIFT,
> > +		.sign = FTR_UNSIGNED,
> > +		.min_field_value = 3,
> > +	},
> 
> Do we really need a new cap for that? Or can we rely on simply looking
> at the sanitised feature set? I'm not overly keen on advertising a
> feature
> at CPU boot time if we discover later on that we cannot use it because
> all
> we have in a non-4.1 GIC.

Yes I thought about that, posted exactly to get this feedback.

You have a point. It is also wrong to force secondaries to die if
they mismatch with the boot CPU without knowing what *actual* GIC
controller we have in the system.

> Another thing is that we currently assume that *all* CPUs will be the
> same
> at the point where we setup the GIC (we only have a single CPU booted at
> that
> point).

Yes - we would taint (?) if that's not the case right ? So using a
sanitised feature looks sane to me, certainly saner than using a new
cap and I think that's all we can and should do.

Thanks,
Lorenzo

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

[Marc Zyngier]

On 2020-11-12 14:40, Lorenzo Pieralisi wrote:
> On Thu, Nov 12, 2020 at 09:36:10AM +0000, Marc Zyngier wrote:
>> 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?
> 
> Hi Marc,
> 
> long story short: there are systems being designed with this
> configuration, vSGIs may or may not work on them, to prevent
> *potential* misbehaviour I am disabling vSGIs, I am not fixing
> anything, it is belt and braces.
> 
>> 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.
> 
> We may wait and fix it *if* this breaks, I would argue though that at
> that point it is not a quirk since architecturally we know that vSGIs
> may not work in this configuration.

I don't mind either way, as I doubt I'll see this kind of system any 
time
soon. I'm just mildly annoyed at the missed opportunity to do something
better...

> 
>> 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@arm.com>
>> > Cc: Marc Zyngier <maz@kernel.org>
>> > ---
>> >  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?
> 
> No, I obviously missed the VGIC bits built on top of
> GICD_TYPER2.nASSGIcap.
> 
>> 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().
> 
> Yes (AFAICS GICD_TYPER2.nASSGIcap is not in the public specs though,
> that's why I missed it while vetting architectural state that is
> affecting vSGIs).

You can find it in the errata to the spec (I just checked the October 
2020
version). I doubt it is public though, and people have been asking for 
this
update to be published for a while now.

> I should change the logic in vgic_mmio_{uaccess}_write_v3_misc() to
> handle it properly - to redefine the logic around
> 
> kvm_vgic_global_state.has_gicv4_1
> 
> somehow.

You probably need a separate predicate, indicating HW-baked vSGI 
support.

Thanks,

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