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@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
>  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.