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

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

[Srinivas Kandagatla]

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

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

Re: [PATCH] irqchip/gic-v3: do not access GICR_WAKER if its secured register.

[Sudeep Holla]

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

Re: [PATCH] irqchip/gic-v3: do not access GICR_WAKER if its secured register.

[Srinivas Kandagatla]

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
> 

Re: [PATCH] irqchip/gic-v3: do not access GICR_WAKER if its secured register.

[Marc Zyngier]

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.

Re: [PATCH] irqchip/gic-v3: do not access GICR_WAKER if its secured register.

[Srinivas Kandagatla]

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

Re: [PATCH] irqchip/gic-v3: do not access GICR_WAKER if its secured register.

[Sudeep Holla]

On Wed, Jun 13, 2018 at 9:21 AM, Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
>
> 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.
>

From what you are saying, you were aware of the firmware bug for 2 years as
you had work-around from then. I wish the firmware had come up with the fix
in those lost 2 years. I understand if it was discovered now, but that doesn't
seem to be the case here.

Firmware fixes are now becoming inevitable, so the attitude that firmware can't
be fixed has to slowing change. I am not saying I am against the quirk, but in
general I have seen the firmware support of QCOM parts are really bad. I waited
for almost 2 years for PSCI firmware on previous dragonboard where developer
are trying to upstream some feature.

--
Regards,
Sudeep

Re: [PATCH] irqchip/gic-v3: do not access GICR_WAKER if its secured register.

[Srinivas Kandagatla]

On 13/06/18 09:45, Sudeep Holla wrote:
> On Wed, Jun 13, 2018 at 9:21 AM, Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> wrote:
>>
>>
>> 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.
>>
> 
>  From what you are saying, you were aware of the firmware bug for 2 years as
> you had work-around from then. I wish the firmware had come up with the fix
> in those lost 2 years. I understand if it was discovered now, but that doesn't
> seem to be the case here.
> 
> Firmware fixes are now becoming inevitable, so the attitude that firmware can't
> be fixed has to slowing change. I am not saying I am against the quirk, but in
> general I have seen the firmware support of QCOM parts are really bad. I waited
> for almost 2 years for PSCI firmware on previous dragonboard where developer
> are trying to upstream some feature.
I agree with you!

We did wait for more than 2 years for firmware to be fixed. But it never 
happened and very little hopes to get it fixed.

I did try the quirk based on GICD_IIDR and I could boot linus master on 
DB820c!

I will send it out as RFC soon.

thanks,
srini
> 
> --
> Regards,
> Sudeep
>