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.pieral...@arm.com>
> > Cc: Marc Zyngier <m...@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
