linux-kernel - Re: [PATCH RESEND 2/3] irqchip/apple-aic: Only access IPI sysregs when use_fast

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: <87zfova32u.ffs@tglx>
Date: Thu, 29 Aug 2024 15:08:57 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Nick Chan <towinchenmi@...il.com>, Hector Martin <marcan@...can.st>,
 Sven Peter <sven@...npeter.dev>, Alyssa Rosenzweig <alyssa@...enzweig.io>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, asahi@...ts.linux.dev,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org
Cc: ~postmarketos/upstreaming@...ts.sr.ht, Nick Chan
 <towinchenmi@...il.com>, Konrad Dybcio <konrad.dybcio@...ainline.org>
Subject: Re: [PATCH RESEND 2/3] irqchip/apple-aic: Only access IPI sysregs
 when use_fast_ipi is true

On Thu, Aug 29 2024 at 19:02, Nick Chan wrote:
> Starting from the A11 (T8015) SoC, Apple introuced system registers for
> fast IPI and UNCORE PMC control. These sysregs do not exist on earlier
> A7-A10 SoCs and trying to access them results in an instant crash.
>
> Restrict sysreg access within the AIC driver to configurations where
> use_fast_ipi is true to allow AIC to function properly on A7-A10 SoCs.

use_fast_ipi is an implementation detail and does not mean anything
here. It's sufficient to say:

   Only access system registers on SoCs which provide them.

Hmm?

> While at it, remove the IPI-always-ack path on aic_handle_fiq.

It's not while at it. It's part of handling this correctly.

> If we are able to reach there, we are on an IPI-capable system and
> should be using one of the IPI-capable compatibles, anyway.

'we' can't reach that code ever.

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog


> Signed-off-by: Konrad Dybcio <konrad.dybcio@...ainline.org>
> Signed-off-by: Nick Chan <towinchenmi@...il.com>

This Signed-off-by chain is invalid. If Konrad authored the patch then
you need to have a 'From: Konrad ...' line at the top of the change log.

If you worked together on this then this needs a Co-developed-by
tag. See Documentation/process/...

>  
> -	if (read_sysreg_s(SYS_IMP_APL_IPI_SR_EL1) & IPI_SR_PENDING) {
> -		if (static_branch_likely(&use_fast_ipi)) {
> -			aic_handle_ipi(regs);
> -		} else {
> -			pr_err_ratelimited("Fast IPI fired. Acking.\n");
> -			write_sysreg_s(IPI_SR_PENDING, SYS_IMP_APL_IPI_SR_EL1);
> -		}
> +	if (static_branch_likely(&use_fast_ipi) &&
> +	    (read_sysreg_s(SYS_IMP_APL_IPI_SR_EL1) & IPI_SR_PENDING)) {
> +		aic_handle_ipi(regs);
>  	}
>  
>  	if (TIMER_FIRING(read_sysreg(cntp_ctl_el0)))
> @@ -574,8 +571,9 @@ static void __exception_irq_entry aic_handle_fiq(struct pt_regs *regs)
>  					  AIC_FIQ_HWIRQ(irq));
>  	}
>  
> -	if (FIELD_GET(UPMCR0_IMODE, read_sysreg_s(SYS_IMP_APL_UPMCR0_EL1)) == UPMCR0_IMODE_FIQ &&
> -			(read_sysreg_s(SYS_IMP_APL_UPMSR_EL1) & UPMSR_IACT)) {
> +	if (static_branch_likely(&use_fast_ipi) &&
> +	    (FIELD_GET(UPMCR0_IMODE, read_sysreg_s(SYS_IMP_APL_UPMCR0_EL1)) == UPMCR0_IMODE_FIQ) &&
> +	    (read_sysreg_s(SYS_IMP_APL_UPMSR_EL1) & UPMSR_IACT)) {
>  		/* Same story with uncore PMCs */
>  		pr_err_ratelimited("Uncore PMC FIQ fired. Masking.\n");
>  		sysreg_clear_set_s(SYS_IMP_APL_UPMCR0_EL1, UPMCR0_IMODE,
> @@ -811,7 +809,8 @@ static int aic_init_cpu(unsigned int cpu)
>  	/* Mask all hard-wired per-CPU IRQ/FIQ sources */
>  
>  	/* Pending Fast IPI FIQs */
> -	write_sysreg_s(IPI_SR_PENDING, SYS_IMP_APL_IPI_SR_EL1);
> +	if (static_branch_likely(&use_fast_ipi))
> +		write_sysreg_s(IPI_SR_PENDING, SYS_IMP_APL_IPI_SR_EL1);
>  
>  	/* Timer FIQs */
>  	sysreg_clear_set(cntp_ctl_el0, 0, ARCH_TIMER_CTRL_IT_MASK);
> @@ -832,8 +831,9 @@ static int aic_init_cpu(unsigned int cpu)
>  			   FIELD_PREP(PMCR0_IMODE, PMCR0_IMODE_OFF));
>  
>  	/* Uncore PMC FIQ */
> -	sysreg_clear_set_s(SYS_IMP_APL_UPMCR0_EL1, UPMCR0_IMODE,
> -			   FIELD_PREP(UPMCR0_IMODE, UPMCR0_IMODE_OFF));
> +	if (static_branch_likely(&use_fast_ipi))
> +		sysreg_clear_set_s(SYS_IMP_APL_UPMCR0_EL1, UPMCR0_IMODE,
> +				   FIELD_PREP(UPMCR0_IMODE, UPMCR0_IMODE_OFF));

Please see the bracket rules in the tip maintainers doc.

Thanks,

        tglx