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

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

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@linaro.org>
>> ---
>>   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.
>