Re: [PATCH v5 06/49] arm64/sysreg: Update ICC_CR0_EL1 with LINK and LINK_IDLE fields

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing Context Synchronization Event (ISB) after control-plane system register writes.
- [High] CONSTRAINED UNPREDICTABLE instruction encoding when emitting system instructions that structurally demand a specific fixed register (e.g., `CDEOI`).
- [Medium] Potential clobbering of newly defined `LINK` and `LINK_IDLE` fields in `ICC_CR0_EL1` due to absolute writes.
--

--- Patch [6]: [PATCH v5 06/49] arm64/sysreg: Update ICC_CR0_EL1 with LINK and LINK_IDLE fields ---
commit 0855c41ea55b7d483129dc1d181e0c36c00b65e4
Author: Sascha Bischoff <[email protected]>

arm64/sysreg: Update ICC_CR0_EL1 with LINK and LINK_IDLE fields
    
This commit adds the LINK and LINK_IDLE fields to the ICC_CR0_EL1 sysreg
definition. These fields were missing since the register was added to the
generator.

> diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg
> index e070ada06196f..ed59330ba51c0 100644
> --- a/arch/arm64/tools/sysreg
> +++ b/arch/arm64/tools/sysreg
> @@ -3737,7 +3737,9 @@ Sysreg	ICC_CR0_EL1	3	1	12	0	1
>  Res0	63:39
>  Field	38	PID
>  Field	37:32	IPPT
> -Res0	31:1
> +Res0	31:3
> +Field	2	LINK_IDLE
> +Field	1	LINK

[Severity: Medium]
This isn't a bug introduced by this patch, but does the initialization
code handle these new fields correctly?

If LINK and LINK_IDLE are expected to retain firmware-configured or default
reset values, using an absolute write to initialize only the EN bit will clear
them to zero, potentially breaking hardware functionality.

For instance, this pattern is seen in drivers/irqchip/irq-gic-v5.c in
gicv5_cpu_enable_interrupts():

	cr0 = FIELD_PREP(ICC_CR0_EL1_EN, 1);
	write_sysreg_s(cr0, SYS_ICC_CR0_EL1);

A similar absolute write clearing the newly defined bits is also present in
tools/testing/selftests/kvm/include/arm64/gic_v5.h.

Could you confirm if the consumers are robust to this change?

[Severity: High]
This is a pre-existing issue, but I noticed a missing context synchronization
event after control-plane system register writes in the GICv5 driver.

Executing instructions in an architecturally undefined pipeline state can result
in unexpected exceptions, corrupted accesses, or dropped interrupts.

Multiple functions batch writes to ICC_*_EL1 registers or return immediately
after a write without issuing an isb().

For example, in drivers/irqchip/irq-gic-v5.c:gicv5_cpu_disable_interrupts():

	cr0 = FIELD_PREP(ICC_CR0_EL1_EN, 0);
	write_sysreg_s(cr0, SYS_ICC_CR0_EL1);
}

A similar pattern batches consecutive writes in gicv5_cpu_enable_interrupts():

	write_sysreg_s(0, SYS_ICC_PPI_ENABLER0_EL1);
	write_sysreg_s(0, SYS_ICC_PPI_ENABLER1_EL1);

Are these writes supposed to have an isb() as the very next instruction to
synchronize the pipeline?

[Severity: High]
This is a pre-existing issue, but is there a risk of constrained unpredictable
instruction encoding when emitting system instructions like CDEOI?

The write_sysreg_s macro relies on the compiler's "rZ" constraint for register
allocation.

System instructions like GIC CDEOI architecturally mandate the use of XZR
(register 31). Allowing the compiler to select an arbitrary general-purpose
register containing zero misencodes the instruction.

This occurs in tools/testing/selftests/kvm/include/arm64/gic_v5.h:

	#define gic_insn(v, insn)	write_sysreg_s(v, GICV5_OP_GIC_##insn)

and is used in drivers/irqchip/irq-gic-v5.c:gicv5_handle_irq():

	gic_insn(0, CDEOI);

Should this be hardcoded to use XZR to prevent the hardware from treating it
as constrained unpredictable?

>  Field	0	EN
>  EndSysreg

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6