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