Messages in this thread | | | Date | Thu, 12 Nov 2020 15:39:14 +0000 | From | Marc Zyngier <> | Subject | Re: [PATCH 2/2] irqchip/gic-v3-its: Disable vSGI upon (CPUIF < v4.1) detection |
| |
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...
| |