Re: [PATCH v4 1/8] arm64/mte: Move shift from definition of TCF0 enumeration values

[Mark Rutland <mark.rutland@arm.com>]

Hi Mark,

I've obviously in favour of scripting, and the changes below look sound to me, so:

Acked-by: Mark Rutland <mark.rutland@arm.com>

I have a couple of minor comments on the commit message, and one suggestion on
the actual code, but the ack stands either way.

On Tue, Apr 19, 2022 at 11:43:22AM +0100, Mark Brown wrote:
> In preparation for automatic generation of SCTLR_EL1 register definitions
> move the shifting of the enumeration values for the TCF0 field from the
> defines in the header to the point where they are used.

It might be worth saying explicitly that this is because the scripting
consistently define enum values *without* shifting.

Likewise saying that the MASK definition is deliberately left shifted as that
defines the bit position of the field, and can be used with FIELD_GET() /
FIELD_PREP().

You can also add something like:

  There should be no functional change as a result of this patch

... to make it clear that this is just a refactoring.

> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/include/asm/sysreg.h | 8 ++++----
>  arch/arm64/kernel/mte.c         | 6 +++---
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index fbf5f8bb9055..ff7693902686 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -678,10 +678,10 @@
>  #define SCTLR_EL1_ATA0		(BIT(42))
>  
>  #define SCTLR_EL1_TCF0_SHIFT	38
> -#define SCTLR_EL1_TCF0_NONE	(UL(0x0) << SCTLR_EL1_TCF0_SHIFT)
> -#define SCTLR_EL1_TCF0_SYNC	(UL(0x1) << SCTLR_EL1_TCF0_SHIFT)
> -#define SCTLR_EL1_TCF0_ASYNC	(UL(0x2) << SCTLR_EL1_TCF0_SHIFT)
> -#define SCTLR_EL1_TCF0_ASYMM	(UL(0x3) << SCTLR_EL1_TCF0_SHIFT)
> +#define SCTLR_EL1_TCF0_NONE	UL(0x0)
> +#define SCTLR_EL1_TCF0_SYNC	UL(0x1)
> +#define SCTLR_EL1_TCF0_ASYNC	UL(0x2)
> +#define SCTLR_EL1_TCF0_ASYMM	UL(0x3)
>  #define SCTLR_EL1_TCF0_MASK	(UL(0x3) << SCTLR_EL1_TCF0_SHIFT)
>  
>  #define SCTLR_EL1_BT1		(BIT(36))
> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> index 78b3e0f8e997..77614f8bc463 100644
> --- a/arch/arm64/kernel/mte.c
> +++ b/arch/arm64/kernel/mte.c
> @@ -216,11 +216,11 @@ static void mte_update_sctlr_user(struct task_struct *task)
>  	 * default order.
>  	 */
>  	if (resolved_mte_tcf & MTE_CTRL_TCF_ASYMM)
> -		sctlr |= SCTLR_EL1_TCF0_ASYMM;
> +		sctlr |= SCTLR_EL1_TCF0_ASYMM << SCTLR_EL1_TCF0_SHIFT;

Generally we should probably use FIELD_PREP()/FIELD_GET() in C code, so maybe
we should make this:

		sctlr |= FIELD_PREP(SCTLR_EL1_TCF0_MASK, SCTLR_EL1_TCF0_ASYMM);

Going forward, it might be worth having some helpers that allow us to avoid the
redundant bits, and save humans having to care about how we namespace the
definitions, e.g.

		sctlr |= SYS_FIELD_PREP(SCTLR_EL1, TCF0, 0x0);
	is:
		sctlr |= FIELD_PREP(SCTLR_EL1_TCF0_MASK, 0x0);


		sctlr |= SYS_FIELD_PREP_ENUM(SCTLR_EL1, TCF0, ASYMM);
	is
		sctlr |= FIELD_PREP(SCTLR_EL1_TCF0_MASK, SCTLR_EL1_TCF0_ASYMM);

Thanks,
Mark.

>  	else if (resolved_mte_tcf & MTE_CTRL_TCF_ASYNC)
> -		sctlr |= SCTLR_EL1_TCF0_ASYNC;
> +		sctlr |= SCTLR_EL1_TCF0_ASYNC << SCTLR_EL1_TCF0_SHIFT;
>  	else if (resolved_mte_tcf & MTE_CTRL_TCF_SYNC)
> -		sctlr |= SCTLR_EL1_TCF0_SYNC;
> +		sctlr |= SCTLR_EL1_TCF0_SYNC << SCTLR_EL1_TCF0_SHIFT;
>  	task->thread.sctlr_user = sctlr;
>  }
>  
> -- 
> 2.30.2
> 

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