Re: [PATCH 07/17] arm64/sysreg: Add definitions for immediate versions of MSR ALLINT

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `_allint_clear()` and `_allint_set()` functions lack compiler memory barriers, allowing dangerous compiler reordering across NMI boundaries.
- [Low] The new `arch/arm64/include/asm/nmi.h` header uses macros from `<asm/sysreg.h>` but fails to include it, breaking self-containment.
--

commit f45a4a26dfbea0878dc6e4ccc4dcf5b6a158b356
Author: Mark Brown <broonie@kernel.org>

arm64/sysreg: Add definitions for immediate versions of MSR ALLINT

This commit adds system register definitions for immediate versions of
MSR ALLINT (SYS_ALLINT_CLR, SYS_ALLINT_SET). It also introduces a new
header, asm/nmi.h, with wrappers _allint_clear() and _allint_set() that
write to these registers using xzr.

Link: https://lore.kernel.org/r/20221112151708.175147-5-broonie@kernel.org

> diff --git a/arch/arm64/include/asm/nmi.h b/arch/arm64/include/asm/nmi.h
> new file mode 100644
> index 0000000000000..eb2c9a8350cc8
> --- /dev/null
> +++ b/arch/arm64/include/asm/nmi.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2022 ARM Ltd.
> + */
> +#ifndef __ASM_NMI_H
> +#define __ASM_NMI_H
> +
> +#ifdef CONFIG_ARM64_NMI

[Severity: Low]
Is a #include <asm/sysreg.h> directive missing in this new header?

It uses the __msr_s macro and SYS_ALLINT_CLR/SET definitions from sysreg.h.
Without the include, it might break self-containment if another file includes
asm/nmi.h first.

> +static __always_inline void _allint_clear(void)
> +{
> +	asm volatile(__msr_s(SYS_ALLINT_CLR, "xzr"));

[Severity: High]
Does this inline assembly need a "memory" clobber?

Without a compiler memory barrier, the compiler might hoist or sink memory
accesses across the NMI masking/unmasking boundary. This could lead to data
corruption or kernel panics when protected data structures are accessed
concurrently.

> +}
> +
> +static __always_inline void _allint_set(void)
> +{
> +	asm volatile(__msr_s(SYS_ALLINT_SET, "xzr"));

[Severity: High]
Should this masking function also use a "memory" clobber to prevent compiler
reordering across the boundary?

> +}
> +#else
> +static __always_inline void _allint_clear(void) { }
> +static __always_inline void _allint_set(void) { }

[Severity: High]
Would it be safer for these fallback implementations to include a barrier()
call?

If they are used outside of strict architectural checks, an empty function
allows the compiler to reorder memory operations across where the NMI critical
section was intended.

> +#endif /* CONFIG_ARM64_NMI */
> +
> +#endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703100135.2512312-1-ruanjinjie@huawei.com?part=7