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

[Srinivas Kandagatla]

Thanks for the review!

On 13/06/18 13:59, Marc Zyngier wrote:
> On 13/06/18 12:43, Srinivas Kandagatla wrote:
> > Access to GICR_WAKER is restricted on msm8996 SoC. Its been more
>
> Restricted by what? Firmware? Hypervisor? (most likely the later).
Yes, its the Hypervisor (TZ firmware)

> > than 2 years of wait for this to be fixed in firmware which is
>
> This surely bodes very well in this day and age, where firmware update
> are becoming just as important as updating your kernel and your
> userspace to fix security problems. I'm impressed.
>
> > not going anywhere. So add a quirk to not write to this register.
> > With this quirk MSM8996 can atleast boot out of mainline,
>
> at least
yep.

> > which can help community to work with boards based on MSM8996.
> >
> > Without this patch Qualcomm DB820c board reboots when GICR_WAKER
> > is written to.
> >
> > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> > ---
> >   drivers/irqchip/irq-gic-v3.c | 32 ++++++++++++++++++++++++++++++++
> >   1 file changed, 32 insertions(+)
> >
> > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > index 76ea56d779a1..d1bb2c0cce02 100644
> > --- a/drivers/irqchip/irq-gic-v3.c
> > +++ b/drivers/irqchip/irq-gic-v3.c
> > @@ -47,6 +47,8 @@ struct redist_region {
> >   	bool			single_redist;
> >   };
> >   
> > +#define GICV3_FLAGS_WORKAROUND_IW_GICR_WAKER	(1ULL << 0)
>
> Drop the GICV3 prefix. What does IW means here? Please provide an
I will drop the prefix.

> erratum number for this, and add a description to
> Documentation/arm64/silicon-errata.txt.

Am not sure there is any silicon level errata associated with this, as 
its related to firmware. Any way I will try to dig up the docs to see if 
there is any.

> > +
> >   struct gic_chip_data {
> >   	struct fwnode_handle	*fwnode;
> >   	void __iomem		*dist_base;
> > @@ -55,6 +57,7 @@ struct gic_chip_data {
> >   	struct irq_domain	*domain;
> >   	u64			redist_stride;
> >   	u32			nr_redist_regions;
> > +	u64			flags;
> >   	bool			has_rss;
> >   	unsigned int		irq_nr;
> >   	struct partition_desc	*ppi_descs[16];
> > @@ -139,6 +142,9 @@ static void gic_enable_redist(bool enable)
> >   	u32 count = 1000000;	/* 1s! */
> >   	u32 val;
> >   
> > +	if (gic_data.flags & GICV3_FLAGS_WORKAROUND_IW_GICR_WAKER)
> > +		return;
> > +
> >   	rbase = gic_data_rdist_rd_base();
> >   
> >   	val = readl_relaxed(rbase + GICR_WAKER);
> > @@ -1064,6 +1070,31 @@ static const struct irq_domain_ops partition_domain_ops = {
> >   	.select = gic_irq_domain_select,
> >   };
> >   
> > +static bool __maybe_unused gicv3_enable_quirk_msm8996(void *data)
>
> All the functions are prefixed with gic, not gicv3. The function name
> should reflect the erratum number.
will fix it in next version.

> > +{
> > +	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.

> > +		.init	= gicv3_enable_quirk_msm8996,
> > +	},
> > +};
> > +
> > +static void gic_v3_enable_quirks(struct gic_chip_data *gic_data)
>
> gic, not gic_v3.
yep.

Thanks,
srini

> > +{
> > +	u32 iidr = readl_relaxed(gic_data->dist_base + GICD_IIDR);
> > +
> > +	gic_enable_quirks(iidr, gicv3_quirks, gic_data);
> > +}
> > +
> >   static int __init gic_init_bases(void __iomem *dist_base,
> >   				 struct redist_region *rdist_regs,
> >   				 u32 nr_redist_regions,
> > @@ -1126,6 +1157,7 @@ static int __init gic_init_bases(void __iomem *dist_base,
> >   	if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis())
> >   		its_init(handle, &gic_data.rdists, gic_data.domain);
> >   
> > +	gic_v3_enable_quirks(&gic_data);
> >   	gic_smp_init();
> >   	gic_dist_init();
> >   	gic_cpu_init();
>
> Thanks,
>
> 	M.