Re: [PATCH v2 04/14] arm64/sysreg: Add definitions for immediate versions of MSR ALLINT

Marc Zyngier
2022-12-05T16:38:56Z

On Sat, 12 Nov 2022 15:16:58 +0000,
Mark Brown <broonie@kernel.org> wrote:
>
> Encodings are provided for ALLINT which allow setting of ALLINT.ALLINT
> using an immediate rather than requiring that a register be loaded with
> the value to write. Since these don't currently fit within the scheme we
> have for sysreg generation add manual encodings like we currently do for
> other similar registers such as SVCR.
>
> Since it is required that these immediate versions be encoded with xzr
> as the source register provide asm wrapper which ensure this is the
> case.
>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
> arch/arm64/include/asm/daifflags.h | 1 +
> arch/arm64/include/asm/nmi.h | 18 ++++++++++++++++++
> arch/arm64/include/asm/sysreg.h | 2 ++
> 3 files changed, 21 insertions(+)
> create mode 100644 arch/arm64/include/asm/nmi.h
>
> diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
> index 55f57dfa8e2f..b3bed2004342 100644
> --- a/arch/arm64/include/asm/daifflags.h
> +++ b/arch/arm64/include/asm/daifflags.h
> @@ -141,4 +141,5 @@ static inline void local_daif_inherit(struct pt_regs *regs)
> */
> write_sysreg(flags, daif);
> }
> +
> #endif

Spurious change?

> diff --git a/arch/arm64/include/asm/nmi.h b/arch/arm64/include/asm/nmi.h
> new file mode 100644
> index 000000000000..067e2554e144
> --- /dev/null
> +++ b/arch/arm64/include/asm/nmi.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2022 ARM Ltd.
> + */
> +#ifndef __ASM_NMI_H
> +#define __ASM_NMI_H
> +
> +static __always_inline void _allint_clear(void)
> +{
> + asm volatile(__msr_s(SYS_ALLINT_CLR, "xzr"));
> +}
> +
> +static __always_inline void _allint_set(void)
> +{
> + asm volatile(__msr_s(SYS_ALLINT_SET, "xzr"));
> +}
> +
> +#endif

If this *really* must be a separate include file, it should at least
directly include its dependencies. My gut feeling is that it would be
better placed in daiflags.h.

> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 7d301700d1a9..0c07b740c750 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -126,6 +126,8 @@
> * System registers, organised loosely by encoding but grouped together
> * where the architected name contains an index. e.g. ID_MMFR<n>_EL1.
> */
> +#define SYS_ALLINT_CLR sys_reg(0, 1, 4, 0, 0)
> +#define SYS_ALLINT_SET sys_reg(0, 1, 4, 1, 0)

This only covers the immediate versions of ALLINT, and misses the
definition for the register version, aka sys_reg(3, 0, 4, 3, 0).

M.

--
Without deviation from the norm, progress is not possible.