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