Re: [RFC PATCH] irqchip/gic-v3: Add quirk for msm8996 secured registers

[Bjorn Andersson]

On Fri 15 Jun 10:53 PDT 2018, Stephen Boyd wrote:

> Quoting Marc Zyngier (2018-06-15 01:16:02)
> > On 14/06/18 21:33, Stephen Boyd wrote:
> > > Quoting Srinivas Kandagatla (2018-06-14 10:54:43)
> > >>>
> > >>>> +{
> > >>>> +    struct gic_chip_data *d = data;
> > >>>> +
> > >>>> +    d->flags |= GICV3_FLAGS_WORKAROUND_IW_GICR_WAKER;
> > >>>> +
> > >>>> +    return true;
> > >>>> +}
> > >>>> +
> > >>>> +static const struct gic_quirk gicv3_quirks[] = {
> > >>>> +    {
> > >>>> +            .desc   = "GICV3: Qualcomm MSM8996 WAKER IW",
> > >>>
> > >>> Please the erratum number in the message. It should read something like:
> > >>>
> > >>>               "GICv3: Qualcomm erratum BIGNUMBERHERE"
> > >>>
> > >>>> +            .iidr   = 0x00001070,   /* MSM8996 */
> > >>>> +            .mask   = 0x0000ffff,
> > >>>
> > >>> Please match the full GICD_IIDR register, not just the implementer and
> > >>> the revision. Unless you expect all the QC systems to have the same
> > >>> behaviour?
> > >> There seems to be more than one SoC that has this issue,  I will dig up 
> > >> more info before sending next version.
> > >>
> > > 
> > > It depends on the firmware and if that firmware decides to block or
> > > allow access to this register space. I don't see how it can be quirked
> > > based on the IIDR at all because there could be different firmware on
> > > the board that doesn't block access to the register. Can a DT property
> > > work?
> > 
> > Are you saying that the IIDR doesn't isn't unique per implementation of
> > the firmware (which, as far as the kernel is concerned in this case,
> > implements the GIC)? That would be another erratum. If we did change the
> > behaviour of the vGIC in KVM, we'd certainly change the IIDR value! This
> > is the exact same case.
> 
> I don't know for certain. All I know is that we can't assume all QC
> systems have a firmware that brings the whole system down when you read
> the WAKER register. Hopefully Srini can find out if reads to IIDR are
> being trapped by the hypervisor and emulated as something different.
> 

I took another look at the internal documentation related to this change
and concluded that it was introduced for the "lead device" on the 8996
platform. As such I expect that all publicly available 8996 devices have
this implementation.

Looking through some relevant platforms it seems like it's only 8996
that that sports a GICv3 with an IIDR of 0x1070 (e.g. 8994 has a QGICv2
with the same IIDR...), so the quirk seems reasonable in scope.

Regards,
Bjorn