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

From: Marc Zyngier <maz@kernel.org>
To: Mark Brown <broonie@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Lorenzo Pieralisi <lpieralisi@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Sami Mujawar <Sami.Mujawar@arm.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev
Subject: Re: [PATCH v2 04/14] arm64/sysreg: Add definitions for immediate versions of MSR ALLINT
Date: Mon, 05 Dec 2022 16:38:53 +0000	[thread overview]
Message-ID: <86wn75ls9u.wl-maz@kernel.org> (raw)
In-Reply-To: <20221112151708.175147-5-broonie@kernel.org>

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel