diff mbox series

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

Message ID 20180613114340.32550-1-srinivas.kandagatla@linaro.org
State New
Headers show
Series [RFC] irqchip/gic-v3: Add quirk for msm8996 secured registers | expand

Commit Message

Srinivas Kandagatla June 13, 2018, 11:43 a.m. UTC 
Access to GICR_WAKER is restricted on msm8996 SoC. Its been more
than 2 years of wait for this to be fixed in firmware which is
not going anywhere. So add a quirk to not write to this register.
With this quirk MSM8996 can atleast boot out of mainline,
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(+)

-- 
2.16.2

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Srinivas Kandagatla June 14, 2018, 5:54 p.m. UTC | #1 
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.

> 

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Boyd June 14, 2018, 8:33 p.m. UTC | #2 
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?

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Boyd June 15, 2018, 5:53 p.m. UTC | #3 
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.

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Andersson June 26, 2018, 8:28 p.m. UTC | #4 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

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)
+
 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)
+{
+	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",
+		.iidr	= 0x00001070,	/* MSM8996 */
+		.mask	= 0x0000ffff,
+		.init	= gicv3_enable_quirk_msm8996,
+	},
+};
+
+static void gic_v3_enable_quirks(struct gic_chip_data *gic_data)
+{
+	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();