linux-kernel - Re: [PATCH] irqchip/gic: Don't write to GICD

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3c8d9248-39c2-9b70-7a41-0b51e6ba5a3c@arm.com>
Date:   Thu, 6 Apr 2017 10:26:58 +0100
From:   Marc Zyngier <marc.zyngier@....com>
To:     Mikko Perttunen <mperttunen@...dia.com>, tglx@...utronix.de,
        jason@...edaemon.net
Cc:     linux-kernel@...r.kernel.org, talho@...dia.com,
        aniruddhab@...dia.com, Matt Craighead <mcraighead@...dia.com>
Subject: Re: [PATCH] irqchip/gic: Don't write to GICD_ICFGR0

On 06/04/17 09:17, Mikko Perttunen wrote:
> From: Matt Craighead <mcraighead@...dia.com>
> 
> According to the GICv2 specification, the GICD_ICFGR0,
> or GIC_DIST_CONFIG[0] register is read-only. Therefore
> avoid writing to it.

Have you verified that this also applies to pre-v2 GICs?

> 
> Signed-off-by: Matt Craighead <mcraighead@...dia.com>
> [mperttunen@...dia.com: commit message rewritten]
> Signed-off-by: Mikko Perttunen <mperttunen@...dia.com>
> ---
>  drivers/irqchip/irq-gic.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 1b1df4f770bd..d9c0000050e0 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -609,7 +609,7 @@ void gic_dist_restore(struct gic_chip_data *gic)
>  
>  	writel_relaxed(GICD_DISABLE, dist_base + GIC_DIST_CTRL);
>  
> -	for (i = 0; i < DIV_ROUND_UP(gic_irqs, 16); i++)
> +	for (i = 1; i < DIV_ROUND_UP(gic_irqs, 16); i++)
>  		writel_relaxed(gic->saved_spi_conf[i],
>  			dist_base + GIC_DIST_CONFIG + i * 4);
>  
> @@ -699,7 +699,7 @@ void gic_cpu_restore(struct gic_chip_data *gic)
>  	}
>  
>  	ptr = raw_cpu_ptr(gic->saved_ppi_conf);
> -	for (i = 0; i < DIV_ROUND_UP(32, 16); i++)
> +	for (i = 1; i < DIV_ROUND_UP(32, 16); i++)
>  		writel_relaxed(ptr[i], dist_base + GIC_DIST_CONFIG + i * 4);

Assuming that the above stands for all GICs, it feels like there is room
for simplification here. But you haven't dealt with the save side, so
what's the point?

Also, you're missing out some other stuff which is (by definition) RO as
well, such as the target registers for SGIs and PPIs. Finally, there is
the question of the allocated memory for these registers.

Overall, I'm not sure what this patch is trying to achieve. It doesn't
fix a bug, and is not complete enough to do something useful (even
though it would only be saving a handful of bytes).

Maybe you can explain what you're trying to do here?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...