irqchip/gic-v3: do not access GICR_WAKER if its secured register.

Message ID 20180612145516.30897-1-srinivas.kandagatla@linaro.org
State New
Headers show
Series
  • irqchip/gic-v3: do not access GICR_WAKER if its secured register.
Related show

Commit Message

Srinivas Kandagatla June 12, 2018, 2:55 p.m.
GICR_WAKER can be a secured register, check this before accessing it
as its done in power management code.

Without this patch Qualcomm DB820c board crashes.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

---
 drivers/irqchip/irq-gic-v3.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

-- 
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

Sudeep Holla June 12, 2018, 3:24 p.m. | #1
On Tue, Jun 12, 2018 at 3:55 PM, Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
> GICR_WAKER can be a secured register, check this before accessing it

> as its done in power management code.

>

> Without this patch Qualcomm DB820c board crashes.

>


Are you sure this is the one causing the crash ?

As per GIC specification:
"When GICD_CTLR.DS==1, this register is always accessible.
 When GICD_CTLR.DS==0, this is a Secure register. This register is RAZ/WI
 to Non-secure accesses."


--
Regards,
Sudeep
--
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
Srinivas Kandagatla June 12, 2018, 3:51 p.m. | #2
On 12/06/18 16:24, Sudeep Holla wrote:
> On Tue, Jun 12, 2018 at 3:55 PM, Srinivas Kandagatla

> <srinivas.kandagatla@linaro.org> wrote:

>> GICR_WAKER can be a secured register, check this before accessing it

>> as its done in power management code.

>>

>> Without this patch Qualcomm DB820c board crashes.

>>

> 

> Are you sure this is the one causing the crash ?

> 

Yep, am 100% sure its the write in gic_enable_redist(). Very first zero 
write to GICR_WAKER is the one which is crashing my system.

If I add return before writing then I can boot my system fine.

Not sure why writes are not ignored?

Also why does other parts of the code have checks while accessing this 
register?

thanks,
srini

> As per GIC specification:

> "When GICD_CTLR.DS==1, this register is always accessible.

>   When GICD_CTLR.DS==0, this is a Secure register. This register is RAZ/WI

>   to Non-secure accesses."

> 

> 

> --

> Regards,

> Sudeep

> 

--
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
Marc Zyngier June 12, 2018, 4:34 p.m. | #3
On Tue, 12 Jun 2018 15:55:16 +0100,
Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:
> 

> GICR_WAKER can be a secured register, check this before accessing it

> as its done in power management code.


NAK.

From the GICv3 spec:

* When GICD_CTLR.DS==1, this register is always accessible.
* When GICD_CTLR.DS==0, this is a Secure register. This register is
  RAZ/WI to Non-secure accesses.

> Without this patch Qualcomm DB820c board crashes.


I suggest you find out how the GIC has been integrated on this
platform. If you take a fault on accessing this register, this very
much looks like an integration bug, and it should be quirked as such.

Thanks,

	M.
	
> 

> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

> ---

>  drivers/irqchip/irq-gic-v3.c | 14 ++++++++------

>  1 file changed, 8 insertions(+), 6 deletions(-)

> 

> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c

> index 5a67ec084588..38136d6e9ca5 100644

> --- a/drivers/irqchip/irq-gic-v3.c

> +++ b/drivers/irqchip/irq-gic-v3.c

> @@ -656,6 +656,12 @@ static int gic_dist_supports_lpis(void)

>  	return !!(readl_relaxed(gic_data.dist_base + GICD_TYPER) & GICD_TYPER_LPIS) && !gicv3_nolpi;

>  }

>  

> +/* Check whether it's single security state view */

> +static bool gic_dist_security_disabled(void)

> +{

> +	return readl_relaxed(gic_data.dist_base + GICD_CTLR) & GICD_CTLR_DS;

> +}

> +

>  static void gic_cpu_init(void)

>  {

>  	void __iomem *rbase;

> @@ -664,7 +670,8 @@ static void gic_cpu_init(void)

>  	if (gic_populate_rdist())

>  		return;

>  

> -	gic_enable_redist(true);

> +	if (gic_dist_security_disabled())

> +		gic_enable_redist(true);

>  

>  	rbase = gic_data_rdist_sgi_base();

>  

> @@ -819,11 +826,6 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,

>  #endif

>  

>  #ifdef CONFIG_CPU_PM

> -/* Check whether it's single security state view */

> -static bool gic_dist_security_disabled(void)

> -{

> -	return readl_relaxed(gic_data.dist_base + GICD_CTLR) & GICD_CTLR_DS;

> -}

>  

>  static int gic_cpu_pm_notifier(struct notifier_block *self,

>  			       unsigned long cmd, void *v)

> -- 

> 2.16.2

> 


-- 
Jazz is not dead, it just smell funny.
--
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
Srinivas Kandagatla June 13, 2018, 8:21 a.m. | #4
On 12/06/18 17:34, Marc Zyngier wrote:
> I suggest you find out how the GIC has been integrated on this

> platform. If you take a fault on accessing this register, this very

> much looks like an integration bug, and it should be quirked as such.

Thanks for the suggestion, This is a bug in the firmware which is 
restricting access to this register. We have been working around this 
bug for more than 2 years due to this. Now this platform support is in 
mainline and We/I have no hope that this will be fixed in near future 
atleast for this platform.

I will try to come up with a Quirk specific for this SoC.

thanks,
srini
--
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

Patch

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 5a67ec084588..38136d6e9ca5 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -656,6 +656,12 @@  static int gic_dist_supports_lpis(void)
 	return !!(readl_relaxed(gic_data.dist_base + GICD_TYPER) & GICD_TYPER_LPIS) && !gicv3_nolpi;
 }
 
+/* Check whether it's single security state view */
+static bool gic_dist_security_disabled(void)
+{
+	return readl_relaxed(gic_data.dist_base + GICD_CTLR) & GICD_CTLR_DS;
+}
+
 static void gic_cpu_init(void)
 {
 	void __iomem *rbase;
@@ -664,7 +670,8 @@  static void gic_cpu_init(void)
 	if (gic_populate_rdist())
 		return;
 
-	gic_enable_redist(true);
+	if (gic_dist_security_disabled())
+		gic_enable_redist(true);
 
 	rbase = gic_data_rdist_sgi_base();
 
@@ -819,11 +826,6 @@  static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
 #endif
 
 #ifdef CONFIG_CPU_PM
-/* Check whether it's single security state view */
-static bool gic_dist_security_disabled(void)
-{
-	return readl_relaxed(gic_data.dist_base + GICD_CTLR) & GICD_CTLR_DS;
-}
 
 static int gic_cpu_pm_notifier(struct notifier_block *self,
 			       unsigned long cmd, void *v)