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

From: Marc Zyngier
Date: Tue Jun 12 2018 - 12:34:14 EST


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.