diff mbox series

[v2,04/24] arm64: Move MAIR_EL1_SET to asm/memory.h

Message ID 20201116204318.63987-5-dbrazdil@google.com (mailing list archive)
State New, archived
Headers show
Series Opt-in always-on nVHE hypervisor | expand

Commit Message

David Brazdil Nov. 16, 2020, 8:42 p.m. UTC
KVM currently initializes MAIR_EL2 to the value of MAIR_EL1. In
preparation for initializing MAIR_EL2 before MAIR_EL1, move the constant
into a shared header file. Since it is used for EL1 and EL2, rename to
MAIR_ELx_SET.

Signed-off-by: David Brazdil <dbrazdil@google.com>
---
 arch/arm64/include/asm/memory.h | 29 ++++++++++++++---------------
 arch/arm64/include/asm/sysreg.h | 30 ++++++++++++++++++++++++++++++
 arch/arm64/mm/proc.S            | 15 +--------------
 3 files changed, 45 insertions(+), 29 deletions(-)

Comments

Marc Zyngier Nov. 23, 2020, 1:52 p.m. UTC | #1
On Mon, 16 Nov 2020 20:42:58 +0000,
David Brazdil <dbrazdil@google.com> wrote:
> 
> KVM currently initializes MAIR_EL2 to the value of MAIR_EL1. In
> preparation for initializing MAIR_EL2 before MAIR_EL1, move the constant
> into a shared header file. Since it is used for EL1 and EL2, rename to
> MAIR_ELx_SET.
> 
> Signed-off-by: David Brazdil <dbrazdil@google.com>
> ---
>  arch/arm64/include/asm/memory.h | 29 ++++++++++++++---------------
>  arch/arm64/include/asm/sysreg.h | 30 ++++++++++++++++++++++++++++++
>  arch/arm64/mm/proc.S            | 15 +--------------
>  3 files changed, 45 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index cd61239bae8c..8ae8fd883a0c 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -13,6 +13,7 @@
>  #include <linux/const.h>
>  #include <linux/sizes.h>
>  #include <asm/page-def.h>
> +#include <asm/sysreg.h>
>  
>  /*
>   * Size of the PCI I/O space. This must remain a power of two so that
> @@ -124,21 +125,6 @@
>   */
>  #define SEGMENT_ALIGN		SZ_64K
>  
> -/*
> - * Memory types available.
> - *
> - * IMPORTANT: MT_NORMAL must be index 0 since vm_get_page_prot() may 'or' in
> - *	      the MT_NORMAL_TAGGED memory type for PROT_MTE mappings. Note
> - *	      that protection_map[] only contains MT_NORMAL attributes.
> - */
> -#define MT_NORMAL		0
> -#define MT_NORMAL_TAGGED	1
> -#define MT_NORMAL_NC		2
> -#define MT_NORMAL_WT		3
> -#define MT_DEVICE_nGnRnE	4
> -#define MT_DEVICE_nGnRE		5
> -#define MT_DEVICE_GRE		6
> -
>  /*
>   * Memory types for Stage-2 translation
>   */
> @@ -152,6 +138,19 @@
>  #define MT_S2_FWB_NORMAL	6
>  #define MT_S2_FWB_DEVICE_nGnRE	1
>  
> +/*
> + * Default MAIR_EL1. MT_NORMAL_TAGGED is initially mapped as Normal memory and
> + * changed during __cpu_setup to Normal Tagged if the system supports MTE.
> + */
> +#define MAIR_ELx_SET							\
> +	(MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRnE) |	\
> +	 MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRE, MT_DEVICE_nGnRE) |	\
> +	 MAIR_ATTRIDX(MAIR_ATTR_DEVICE_GRE, MT_DEVICE_GRE) |		\
> +	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL_NC, MT_NORMAL_NC) |		\
> +	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL) |			\
> +	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL_WT, MT_NORMAL_WT) |		\
> +	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL_TAGGED))
> +
>  #ifdef CONFIG_ARM64_4K_PAGES
>  #define IOREMAP_MAX_ORDER	(PUD_SHIFT)
>  #else
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index e2ef4c2edf06..24e773414cb4 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -635,6 +635,34 @@
>  /* Position the attr at the correct index */
>  #define MAIR_ATTRIDX(attr, idx)		((attr) << ((idx) * 8))
>  
> +/*
> + * Memory types available.
> + *
> + * IMPORTANT: MT_NORMAL must be index 0 since vm_get_page_prot() may 'or' in
> + *	      the MT_NORMAL_TAGGED memory type for PROT_MTE mappings. Note
> + *	      that protection_map[] only contains MT_NORMAL attributes.
> + */
> +#define MT_NORMAL		0
> +#define MT_NORMAL_TAGGED	1
> +#define MT_NORMAL_NC		2
> +#define MT_NORMAL_WT		3
> +#define MT_DEVICE_nGnRnE	4
> +#define MT_DEVICE_nGnRE		5
> +#define MT_DEVICE_GRE		6
> +
> +/*
> + * Default MAIR_ELx. MT_NORMAL_TAGGED is initially mapped as Normal memory and
> + * changed during __cpu_setup to Normal Tagged if the system supports MTE.
> + */
> +#define MAIR_ELx_SET							\
> +	(MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRnE) |	\
> +	 MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRE, MT_DEVICE_nGnRE) |	\
> +	 MAIR_ATTRIDX(MAIR_ATTR_DEVICE_GRE, MT_DEVICE_GRE) |		\
> +	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL_NC, MT_NORMAL_NC) |		\
> +	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL) |			\
> +	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL_WT, MT_NORMAL_WT) |		\
> +	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL_TAGGED))
> +
>  /* id_aa64isar0 */
>  #define ID_AA64ISAR0_RNDR_SHIFT		60
>  #define ID_AA64ISAR0_TLB_SHIFT		56
> @@ -992,6 +1020,7 @@
>  /* Safe value for MPIDR_EL1: Bit31:RES1, Bit30:U:0, Bit24:MT:0 */
>  #define SYS_MPIDR_SAFE_VAL	(BIT(31))
>  
> +#ifndef LINKER_SCRIPT

This is terribly ugly. Why is this included by the linker script? Does
it actually define __ASSEMBLY__?

>  #ifdef __ASSEMBLY__
>  
>  	.irp	num,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30
> @@ -1109,5 +1138,6 @@
>  })
>  
>  #endif
> +#endif	/* LINKER_SCRIPT */
>  
>  #endif	/* __ASM_SYSREG_H */
> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> index 23c326a06b2d..e3b9aa372b96 100644
> --- a/arch/arm64/mm/proc.S
> +++ b/arch/arm64/mm/proc.S
> @@ -45,19 +45,6 @@
>  #define TCR_KASAN_FLAGS 0
>  #endif
>  
> -/*
> - * Default MAIR_EL1. MT_NORMAL_TAGGED is initially mapped as Normal memory and
> - * changed during __cpu_setup to Normal Tagged if the system supports MTE.
> - */
> -#define MAIR_EL1_SET							\
> -	(MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRnE) |	\
> -	 MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRE, MT_DEVICE_nGnRE) |	\
> -	 MAIR_ATTRIDX(MAIR_ATTR_DEVICE_GRE, MT_DEVICE_GRE) |		\
> -	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL_NC, MT_NORMAL_NC) |		\
> -	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL) |			\
> -	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL_WT, MT_NORMAL_WT) |		\
> -	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL_TAGGED))
> -
>  #ifdef CONFIG_CPU_PM
>  /**
>   * cpu_do_suspend - save CPU registers context
> @@ -425,7 +412,7 @@ SYM_FUNC_START(__cpu_setup)
>  	/*
>  	 * Memory region attributes
>  	 */
> -	mov_q	x5, MAIR_EL1_SET
> +	mov_q	x5, MAIR_ELx_SET
>  #ifdef CONFIG_ARM64_MTE
>  	/*
>  	 * Update MAIR_EL1, GCR_EL1 and TFSR*_EL1 if MTE is supported
> -- 
> 2.29.2.299.gdc1121823c-goog
> 
> 

Thanks,

	M.
David Brazdil Nov. 25, 2020, 10:31 a.m. UTC | #2
On Mon, Nov 23, 2020 at 01:52:54PM +0000, Marc Zyngier wrote:
> On Mon, 16 Nov 2020 20:42:58 +0000,
> David Brazdil <dbrazdil@google.com> wrote:
> > 
> > KVM currently initializes MAIR_EL2 to the value of MAIR_EL1. In
> > preparation for initializing MAIR_EL2 before MAIR_EL1, move the constant
> > into a shared header file. Since it is used for EL1 and EL2, rename to
> > MAIR_ELx_SET.
> > 
> > Signed-off-by: David Brazdil <dbrazdil@google.com>
> > ---
> >  arch/arm64/include/asm/memory.h | 29 ++++++++++++++---------------
> >  arch/arm64/include/asm/sysreg.h | 30 ++++++++++++++++++++++++++++++
> >  arch/arm64/mm/proc.S            | 15 +--------------
> >  3 files changed, 45 insertions(+), 29 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> > index cd61239bae8c..8ae8fd883a0c 100644
> > --- a/arch/arm64/include/asm/memory.h
> > +++ b/arch/arm64/include/asm/memory.h
> > @@ -13,6 +13,7 @@
> >  #include <linux/const.h>
> >  #include <linux/sizes.h>
> >  #include <asm/page-def.h>
> > +#include <asm/sysreg.h>
> >  
> >  /*
> >   * Size of the PCI I/O space. This must remain a power of two so that
> > @@ -124,21 +125,6 @@
> >   */
> >  #define SEGMENT_ALIGN		SZ_64K
> >  
> > -/*
> > - * Memory types available.
> > - *
> > - * IMPORTANT: MT_NORMAL must be index 0 since vm_get_page_prot() may 'or' in
> > - *	      the MT_NORMAL_TAGGED memory type for PROT_MTE mappings. Note
> > - *	      that protection_map[] only contains MT_NORMAL attributes.
> > - */
> > -#define MT_NORMAL		0
> > -#define MT_NORMAL_TAGGED	1
> > -#define MT_NORMAL_NC		2
> > -#define MT_NORMAL_WT		3
> > -#define MT_DEVICE_nGnRnE	4
> > -#define MT_DEVICE_nGnRE		5
> > -#define MT_DEVICE_GRE		6
> > -
> >  /*
> >   * Memory types for Stage-2 translation
> >   */
> > @@ -152,6 +138,19 @@
> >  #define MT_S2_FWB_NORMAL	6
> >  #define MT_S2_FWB_DEVICE_nGnRE	1
> >  
> > +/*
> > + * Default MAIR_EL1. MT_NORMAL_TAGGED is initially mapped as Normal memory and
> > + * changed during __cpu_setup to Normal Tagged if the system supports MTE.
> > + */
> > +#define MAIR_ELx_SET							\
> > +	(MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRnE) |	\
> > +	 MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRE, MT_DEVICE_nGnRE) |	\
> > +	 MAIR_ATTRIDX(MAIR_ATTR_DEVICE_GRE, MT_DEVICE_GRE) |		\
> > +	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL_NC, MT_NORMAL_NC) |		\
> > +	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL) |			\
> > +	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL_WT, MT_NORMAL_WT) |		\
> > +	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL_TAGGED))
> > +
> >  #ifdef CONFIG_ARM64_4K_PAGES
> >  #define IOREMAP_MAX_ORDER	(PUD_SHIFT)
> >  #else
> > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> > index e2ef4c2edf06..24e773414cb4 100644
> > --- a/arch/arm64/include/asm/sysreg.h
> > +++ b/arch/arm64/include/asm/sysreg.h
> > @@ -635,6 +635,34 @@
> >  /* Position the attr at the correct index */
> >  #define MAIR_ATTRIDX(attr, idx)		((attr) << ((idx) * 8))
> >  
> > +/*
> > + * Memory types available.
> > + *
> > + * IMPORTANT: MT_NORMAL must be index 0 since vm_get_page_prot() may 'or' in
> > + *	      the MT_NORMAL_TAGGED memory type for PROT_MTE mappings. Note
> > + *	      that protection_map[] only contains MT_NORMAL attributes.
> > + */
> > +#define MT_NORMAL		0
> > +#define MT_NORMAL_TAGGED	1
> > +#define MT_NORMAL_NC		2
> > +#define MT_NORMAL_WT		3
> > +#define MT_DEVICE_nGnRnE	4
> > +#define MT_DEVICE_nGnRE		5
> > +#define MT_DEVICE_GRE		6
> > +
> > +/*
> > + * Default MAIR_ELx. MT_NORMAL_TAGGED is initially mapped as Normal memory and
> > + * changed during __cpu_setup to Normal Tagged if the system supports MTE.
> > + */
> > +#define MAIR_ELx_SET							\
> > +	(MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRnE) |	\
> > +	 MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRE, MT_DEVICE_nGnRE) |	\
> > +	 MAIR_ATTRIDX(MAIR_ATTR_DEVICE_GRE, MT_DEVICE_GRE) |		\
> > +	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL_NC, MT_NORMAL_NC) |		\
> > +	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL) |			\
> > +	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL_WT, MT_NORMAL_WT) |		\
> > +	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL_TAGGED))
> > +
> >  /* id_aa64isar0 */
> >  #define ID_AA64ISAR0_RNDR_SHIFT		60
> >  #define ID_AA64ISAR0_TLB_SHIFT		56
> > @@ -992,6 +1020,7 @@
> >  /* Safe value for MPIDR_EL1: Bit31:RES1, Bit30:U:0, Bit24:MT:0 */
> >  #define SYS_MPIDR_SAFE_VAL	(BIT(31))
> >  
> > +#ifndef LINKER_SCRIPT
> 
> This is terribly ugly. Why is this included by the linker script? Does
> it actually define __ASSEMBLY__?

vmlinux.lds.S includes memory.h for PAGE_SIZE. And yes, linker scripts are
built with this rule:

      cmd_cpp_lds_S = $(CPP) $(cpp_flags) -P -U$(ARCH) \
	                     -D__ASSEMBLY__ -DLINKER_SCRIPT -o $@ $<

I tried a few things and wasn't completely happy with any of them. I think in
the previous spin you suggested moving this constant to sysreg.h. That works
too but sysreg.h seems to have only architecture constants, memory.h about a
Linux-specific configuration, so I wanted to keep it here.

> 
> >  #ifdef __ASSEMBLY__
> >  
> >  	.irp	num,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30
> > @@ -1109,5 +1138,6 @@
> >  })
> >  
> >  #endif
> > +#endif	/* LINKER_SCRIPT */
> >  
> >  #endif	/* __ASM_SYSREG_H */
> > diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> > index 23c326a06b2d..e3b9aa372b96 100644
> > --- a/arch/arm64/mm/proc.S
> > +++ b/arch/arm64/mm/proc.S
> > @@ -45,19 +45,6 @@
> >  #define TCR_KASAN_FLAGS 0
> >  #endif
> >  
> > -/*
> > - * Default MAIR_EL1. MT_NORMAL_TAGGED is initially mapped as Normal memory and
> > - * changed during __cpu_setup to Normal Tagged if the system supports MTE.
> > - */
> > -#define MAIR_EL1_SET							\
> > -	(MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRnE) |	\
> > -	 MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRE, MT_DEVICE_nGnRE) |	\
> > -	 MAIR_ATTRIDX(MAIR_ATTR_DEVICE_GRE, MT_DEVICE_GRE) |		\
> > -	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL_NC, MT_NORMAL_NC) |		\
> > -	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL) |			\
> > -	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL_WT, MT_NORMAL_WT) |		\
> > -	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL_TAGGED))
> > -
> >  #ifdef CONFIG_CPU_PM
> >  /**
> >   * cpu_do_suspend - save CPU registers context
> > @@ -425,7 +412,7 @@ SYM_FUNC_START(__cpu_setup)
> >  	/*
> >  	 * Memory region attributes
> >  	 */
> > -	mov_q	x5, MAIR_EL1_SET
> > +	mov_q	x5, MAIR_ELx_SET
> >  #ifdef CONFIG_ARM64_MTE
> >  	/*
> >  	 * Update MAIR_EL1, GCR_EL1 and TFSR*_EL1 if MTE is supported
> > -- 
> > 2.29.2.299.gdc1121823c-goog
> > 
> > 
> 
> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.
Marc Zyngier Nov. 25, 2020, 11:21 a.m. UTC | #3
On 2020-11-25 10:31, David Brazdil wrote:
> On Mon, Nov 23, 2020 at 01:52:54PM +0000, Marc Zyngier wrote:
>> On Mon, 16 Nov 2020 20:42:58 +0000,
>> David Brazdil <dbrazdil@google.com> wrote:
>> >
>> > KVM currently initializes MAIR_EL2 to the value of MAIR_EL1. In
>> > preparation for initializing MAIR_EL2 before MAIR_EL1, move the constant
>> > into a shared header file. Since it is used for EL1 and EL2, rename to
>> > MAIR_ELx_SET.
>> >
>> > Signed-off-by: David Brazdil <dbrazdil@google.com>
>> > ---
>> >  arch/arm64/include/asm/memory.h | 29 ++++++++++++++---------------
>> >  arch/arm64/include/asm/sysreg.h | 30 ++++++++++++++++++++++++++++++
>> >  arch/arm64/mm/proc.S            | 15 +--------------
>> >  3 files changed, 45 insertions(+), 29 deletions(-)
>> >
>> > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
>> > index cd61239bae8c..8ae8fd883a0c 100644
>> > --- a/arch/arm64/include/asm/memory.h
>> > +++ b/arch/arm64/include/asm/memory.h
>> > @@ -13,6 +13,7 @@
>> >  #include <linux/const.h>
>> >  #include <linux/sizes.h>
>> >  #include <asm/page-def.h>
>> > +#include <asm/sysreg.h>
>> >
>> >  /*
>> >   * Size of the PCI I/O space. This must remain a power of two so that
>> > @@ -124,21 +125,6 @@
>> >   */
>> >  #define SEGMENT_ALIGN		SZ_64K
>> >
>> > -/*
>> > - * Memory types available.
>> > - *
>> > - * IMPORTANT: MT_NORMAL must be index 0 since vm_get_page_prot() may 'or' in
>> > - *	      the MT_NORMAL_TAGGED memory type for PROT_MTE mappings. Note
>> > - *	      that protection_map[] only contains MT_NORMAL attributes.
>> > - */
>> > -#define MT_NORMAL		0
>> > -#define MT_NORMAL_TAGGED	1
>> > -#define MT_NORMAL_NC		2
>> > -#define MT_NORMAL_WT		3
>> > -#define MT_DEVICE_nGnRnE	4
>> > -#define MT_DEVICE_nGnRE		5
>> > -#define MT_DEVICE_GRE		6
>> > -
>> >  /*
>> >   * Memory types for Stage-2 translation
>> >   */
>> > @@ -152,6 +138,19 @@
>> >  #define MT_S2_FWB_NORMAL	6
>> >  #define MT_S2_FWB_DEVICE_nGnRE	1
>> >
>> > +/*
>> > + * Default MAIR_EL1. MT_NORMAL_TAGGED is initially mapped as Normal memory and
>> > + * changed during __cpu_setup to Normal Tagged if the system supports MTE.
>> > + */
>> > +#define MAIR_ELx_SET							\
>> > +	(MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRnE) |	\
>> > +	 MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRE, MT_DEVICE_nGnRE) |	\
>> > +	 MAIR_ATTRIDX(MAIR_ATTR_DEVICE_GRE, MT_DEVICE_GRE) |		\
>> > +	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL_NC, MT_NORMAL_NC) |		\
>> > +	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL) |			\
>> > +	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL_WT, MT_NORMAL_WT) |		\
>> > +	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL_TAGGED))
>> > +
>> >  #ifdef CONFIG_ARM64_4K_PAGES
>> >  #define IOREMAP_MAX_ORDER	(PUD_SHIFT)
>> >  #else
>> > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
>> > index e2ef4c2edf06..24e773414cb4 100644
>> > --- a/arch/arm64/include/asm/sysreg.h
>> > +++ b/arch/arm64/include/asm/sysreg.h
>> > @@ -635,6 +635,34 @@
>> >  /* Position the attr at the correct index */
>> >  #define MAIR_ATTRIDX(attr, idx)		((attr) << ((idx) * 8))
>> >
>> > +/*
>> > + * Memory types available.
>> > + *
>> > + * IMPORTANT: MT_NORMAL must be index 0 since vm_get_page_prot() may 'or' in
>> > + *	      the MT_NORMAL_TAGGED memory type for PROT_MTE mappings. Note
>> > + *	      that protection_map[] only contains MT_NORMAL attributes.
>> > + */
>> > +#define MT_NORMAL		0
>> > +#define MT_NORMAL_TAGGED	1
>> > +#define MT_NORMAL_NC		2
>> > +#define MT_NORMAL_WT		3
>> > +#define MT_DEVICE_nGnRnE	4
>> > +#define MT_DEVICE_nGnRE		5
>> > +#define MT_DEVICE_GRE		6
>> > +
>> > +/*
>> > + * Default MAIR_ELx. MT_NORMAL_TAGGED is initially mapped as Normal memory and
>> > + * changed during __cpu_setup to Normal Tagged if the system supports MTE.
>> > + */
>> > +#define MAIR_ELx_SET							\
>> > +	(MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRnE) |	\
>> > +	 MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRE, MT_DEVICE_nGnRE) |	\
>> > +	 MAIR_ATTRIDX(MAIR_ATTR_DEVICE_GRE, MT_DEVICE_GRE) |		\
>> > +	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL_NC, MT_NORMAL_NC) |		\
>> > +	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL) |			\
>> > +	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL_WT, MT_NORMAL_WT) |		\
>> > +	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL_TAGGED))
>> > +

Wait: You now have MAIR_ELx_SET defined at two locations. Surely that's
one too many.

>> >  /* id_aa64isar0 */
>> >  #define ID_AA64ISAR0_RNDR_SHIFT		60
>> >  #define ID_AA64ISAR0_TLB_SHIFT		56
>> > @@ -992,6 +1020,7 @@
>> >  /* Safe value for MPIDR_EL1: Bit31:RES1, Bit30:U:0, Bit24:MT:0 */
>> >  #define SYS_MPIDR_SAFE_VAL	(BIT(31))
>> >
>> > +#ifndef LINKER_SCRIPT
>> 
>> This is terribly ugly. Why is this included by the linker script? Does
>> it actually define __ASSEMBLY__?
> 
> vmlinux.lds.S includes memory.h for PAGE_SIZE. And yes, linker scripts 
> are
> built with this rule:
> 
>       cmd_cpp_lds_S = $(CPP) $(cpp_flags) -P -U$(ARCH) \
> 	                     -D__ASSEMBLY__ -DLINKER_SCRIPT -o $@ $<
> 
> I tried a few things and wasn't completely happy with any of them. I 
> think in
> the previous spin you suggested moving this constant to sysreg.h. That 
> works
> too but sysreg.h seems to have only architecture constants, memory.h 
> about a
> Linux-specific configuration, so I wanted to keep it here.

MAIR_ELx_SET isn't really Linux specific. Or rather, not more specific 
than
any of the other configurations we have. On the other hand, the S1 MT_* 
stuff
is totally arbitrary, and does fit in memory.h, together with the rest 
of
the indexes for the memory types.

I came up with the following patch on top of this series that seems to
compile without issue.

         M.

diff --git a/arch/arm64/include/asm/memory.h 
b/arch/arm64/include/asm/memory.h
index 8ae8fd883a0c..01685d81e9d4 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -13,7 +13,6 @@
  #include <linux/const.h>
  #include <linux/sizes.h>
  #include <asm/page-def.h>
-#include <asm/sysreg.h>

  /*
   * Size of the PCI I/O space. This must remain a power of two so that
@@ -139,17 +138,19 @@
  #define MT_S2_FWB_DEVICE_nGnRE	1

  /*
- * Default MAIR_EL1. MT_NORMAL_TAGGED is initially mapped as Normal 
memory and
- * changed during __cpu_setup to Normal Tagged if the system supports 
MTE.
+ * Memory types available.
+ *
+ * IMPORTANT: MT_NORMAL must be index 0 since vm_get_page_prot() may 
'or' in
+ *	      the MT_NORMAL_TAGGED memory type for PROT_MTE mappings. Note
+ *	      that protection_map[] only contains MT_NORMAL attributes.
   */
-#define MAIR_ELx_SET							\
-	(MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRnE) |	\
-	 MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRE, MT_DEVICE_nGnRE) |	\
-	 MAIR_ATTRIDX(MAIR_ATTR_DEVICE_GRE, MT_DEVICE_GRE) |		\
-	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL_NC, MT_NORMAL_NC) |		\
-	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL) |			\
-	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL_WT, MT_NORMAL_WT) |		\
-	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL_TAGGED))
+#define MT_NORMAL		0
+#define MT_NORMAL_TAGGED	1
+#define MT_NORMAL_NC		2
+#define MT_NORMAL_WT		3
+#define MT_DEVICE_nGnRnE	4
+#define MT_DEVICE_nGnRE		5
+#define MT_DEVICE_GRE		6

  #ifdef CONFIG_ARM64_4K_PAGES
  #define IOREMAP_MAX_ORDER	(PUD_SHIFT)
diff --git a/arch/arm64/include/asm/sysreg.h 
b/arch/arm64/include/asm/sysreg.h
index 24e773414cb4..c9534fba3afe 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -635,21 +635,6 @@
  /* Position the attr at the correct index */
  #define MAIR_ATTRIDX(attr, idx)		((attr) << ((idx) * 8))

-/*
- * Memory types available.
- *
- * IMPORTANT: MT_NORMAL must be index 0 since vm_get_page_prot() may 
'or' in
- *	      the MT_NORMAL_TAGGED memory type for PROT_MTE mappings. Note
- *	      that protection_map[] only contains MT_NORMAL attributes.
- */
-#define MT_NORMAL		0
-#define MT_NORMAL_TAGGED	1
-#define MT_NORMAL_NC		2
-#define MT_NORMAL_WT		3
-#define MT_DEVICE_nGnRnE	4
-#define MT_DEVICE_nGnRE		5
-#define MT_DEVICE_GRE		6
-
  /*
   * Default MAIR_ELx. MT_NORMAL_TAGGED is initially mapped as Normal 
memory and
   * changed during __cpu_setup to Normal Tagged if the system supports 
MTE.
@@ -1020,7 +1005,6 @@
  /* Safe value for MPIDR_EL1: Bit31:RES1, Bit30:U:0, Bit24:MT:0 */
  #define SYS_MPIDR_SAFE_VAL	(BIT(31))

-#ifndef LINKER_SCRIPT
  #ifdef __ASSEMBLY__

  
	.irp	num,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30
@@ -1138,6 +1122,5 @@
  })

  #endif
-#endif	/* LINKER_SCRIPT */

  #endif	/* __ASM_SYSREG_H */
David Brazdil Nov. 25, 2020, 1:26 p.m. UTC | #4
> > > > +/*
> > > > + * Memory types available.
> > > > + *
> > > > + * IMPORTANT: MT_NORMAL must be index 0 since vm_get_page_prot() may 'or' in
> > > > + *	      the MT_NORMAL_TAGGED memory type for PROT_MTE mappings. Note
> > > > + *	      that protection_map[] only contains MT_NORMAL attributes.
> > > > + */
> > > > +#define MT_NORMAL		0
> > > > +#define MT_NORMAL_TAGGED	1
> > > > +#define MT_NORMAL_NC		2
> > > > +#define MT_NORMAL_WT		3
> > > > +#define MT_DEVICE_nGnRnE	4
> > > > +#define MT_DEVICE_nGnRE		5
> > > > +#define MT_DEVICE_GRE		6
> > > > +
> > > > +/*
> > > > + * Default MAIR_ELx. MT_NORMAL_TAGGED is initially mapped as Normal memory and
> > > > + * changed during __cpu_setup to Normal Tagged if the system supports MTE.
> > > > + */
> > > > +#define MAIR_ELx_SET							\
> > > > +	(MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRnE) |	\
> > > > +	 MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRE, MT_DEVICE_nGnRE) |	\
> > > > +	 MAIR_ATTRIDX(MAIR_ATTR_DEVICE_GRE, MT_DEVICE_GRE) |		\
> > > > +	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL_NC, MT_NORMAL_NC) |		\
> > > > +	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL) |			\
> > > > +	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL_WT, MT_NORMAL_WT) |		\
> > > > +	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL_TAGGED))
> > > > +
> 
> Wait: You now have MAIR_ELx_SET defined at two locations. Surely that's
> one too many.
> 

Oops, told you I tried different things...

> > > >  /* id_aa64isar0 */
> > > >  #define ID_AA64ISAR0_RNDR_SHIFT		60
> > > >  #define ID_AA64ISAR0_TLB_SHIFT		56
> > > > @@ -992,6 +1020,7 @@
> > > >  /* Safe value for MPIDR_EL1: Bit31:RES1, Bit30:U:0, Bit24:MT:0 */
> > > >  #define SYS_MPIDR_SAFE_VAL	(BIT(31))
> > > >
> > > > +#ifndef LINKER_SCRIPT
> > > 
> > > This is terribly ugly. Why is this included by the linker script? Does
> > > it actually define __ASSEMBLY__?
> > 
> > vmlinux.lds.S includes memory.h for PAGE_SIZE. And yes, linker scripts
> > are
> > built with this rule:
> > 
> >       cmd_cpp_lds_S = $(CPP) $(cpp_flags) -P -U$(ARCH) \
> > 	                     -D__ASSEMBLY__ -DLINKER_SCRIPT -o $@ $<
> > 
> > I tried a few things and wasn't completely happy with any of them. I
> > think in
> > the previous spin you suggested moving this constant to sysreg.h. That
> > works
> > too but sysreg.h seems to have only architecture constants, memory.h
> > about a
> > Linux-specific configuration, so I wanted to keep it here.
> 
> MAIR_ELx_SET isn't really Linux specific. Or rather, not more specific than
> any of the other configurations we have. On the other hand, the S1 MT_*
> stuff
> is totally arbitrary, and does fit in memory.h, together with the rest of
> the indexes for the memory types.
> 
> I came up with the following patch on top of this series that seems to
> compile without issue.

That seems to have an implicit dependency of sysreg.h on memory.h, doesn't it?
I had it the other way round initially. I also tried including memory.h in
sysreg.h. That creates a circular dependency mmdebug.h -> bug.h -> ... ->
sysreg.h -> memory.h -> mmdebug.h. Pretty annoying. I could try to fix that,
or create a new header file... :(
Marc Zyngier Nov. 25, 2020, 1:33 p.m. UTC | #5
On 2020-11-25 13:26, David Brazdil wrote:

>> I came up with the following patch on top of this series that seems to
>> compile without issue.
> 
> That seems to have an implicit dependency of sysreg.h on memory.h, 
> doesn't it?
> I had it the other way round initially. I also tried including memory.h 
> in
> sysreg.h. That creates a circular dependency mmdebug.h -> bug.h -> ... 
> ->
> sysreg.h -> memory.h -> mmdebug.h. Pretty annoying. I could try to fix 
> that,
> or create a new header file... :(

I don't think we need this. Any low-level source using MAIR_ELx_SET is 
bound
to require memory.h as well, one way or another. As this is all 
#defines,
it won't break anything unless actively used.

And given that this is used in exactly *two* places, I don't believe 
there
is a need for over-engineering this.

Thanks,

         M.
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index cd61239bae8c..8ae8fd883a0c 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -13,6 +13,7 @@ 
 #include <linux/const.h>
 #include <linux/sizes.h>
 #include <asm/page-def.h>
+#include <asm/sysreg.h>
 
 /*
  * Size of the PCI I/O space. This must remain a power of two so that
@@ -124,21 +125,6 @@ 
  */
 #define SEGMENT_ALIGN		SZ_64K
 
-/*
- * Memory types available.
- *
- * IMPORTANT: MT_NORMAL must be index 0 since vm_get_page_prot() may 'or' in
- *	      the MT_NORMAL_TAGGED memory type for PROT_MTE mappings. Note
- *	      that protection_map[] only contains MT_NORMAL attributes.
- */
-#define MT_NORMAL		0
-#define MT_NORMAL_TAGGED	1
-#define MT_NORMAL_NC		2
-#define MT_NORMAL_WT		3
-#define MT_DEVICE_nGnRnE	4
-#define MT_DEVICE_nGnRE		5
-#define MT_DEVICE_GRE		6
-
 /*
  * Memory types for Stage-2 translation
  */
@@ -152,6 +138,19 @@ 
 #define MT_S2_FWB_NORMAL	6
 #define MT_S2_FWB_DEVICE_nGnRE	1
 
+/*
+ * Default MAIR_EL1. MT_NORMAL_TAGGED is initially mapped as Normal memory and
+ * changed during __cpu_setup to Normal Tagged if the system supports MTE.
+ */
+#define MAIR_ELx_SET							\
+	(MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRnE) |	\
+	 MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRE, MT_DEVICE_nGnRE) |	\
+	 MAIR_ATTRIDX(MAIR_ATTR_DEVICE_GRE, MT_DEVICE_GRE) |		\
+	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL_NC, MT_NORMAL_NC) |		\
+	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL) |			\
+	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL_WT, MT_NORMAL_WT) |		\
+	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL_TAGGED))
+
 #ifdef CONFIG_ARM64_4K_PAGES
 #define IOREMAP_MAX_ORDER	(PUD_SHIFT)
 #else
diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index e2ef4c2edf06..24e773414cb4 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -635,6 +635,34 @@ 
 /* Position the attr at the correct index */
 #define MAIR_ATTRIDX(attr, idx)		((attr) << ((idx) * 8))
 
+/*
+ * Memory types available.
+ *
+ * IMPORTANT: MT_NORMAL must be index 0 since vm_get_page_prot() may 'or' in
+ *	      the MT_NORMAL_TAGGED memory type for PROT_MTE mappings. Note
+ *	      that protection_map[] only contains MT_NORMAL attributes.
+ */
+#define MT_NORMAL		0
+#define MT_NORMAL_TAGGED	1
+#define MT_NORMAL_NC		2
+#define MT_NORMAL_WT		3
+#define MT_DEVICE_nGnRnE	4
+#define MT_DEVICE_nGnRE		5
+#define MT_DEVICE_GRE		6
+
+/*
+ * Default MAIR_ELx. MT_NORMAL_TAGGED is initially mapped as Normal memory and
+ * changed during __cpu_setup to Normal Tagged if the system supports MTE.
+ */
+#define MAIR_ELx_SET							\
+	(MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRnE) |	\
+	 MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRE, MT_DEVICE_nGnRE) |	\
+	 MAIR_ATTRIDX(MAIR_ATTR_DEVICE_GRE, MT_DEVICE_GRE) |		\
+	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL_NC, MT_NORMAL_NC) |		\
+	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL) |			\
+	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL_WT, MT_NORMAL_WT) |		\
+	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL_TAGGED))
+
 /* id_aa64isar0 */
 #define ID_AA64ISAR0_RNDR_SHIFT		60
 #define ID_AA64ISAR0_TLB_SHIFT		56
@@ -992,6 +1020,7 @@ 
 /* Safe value for MPIDR_EL1: Bit31:RES1, Bit30:U:0, Bit24:MT:0 */
 #define SYS_MPIDR_SAFE_VAL	(BIT(31))
 
+#ifndef LINKER_SCRIPT
 #ifdef __ASSEMBLY__
 
 	.irp	num,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30
@@ -1109,5 +1138,6 @@ 
 })
 
 #endif
+#endif	/* LINKER_SCRIPT */
 
 #endif	/* __ASM_SYSREG_H */
diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
index 23c326a06b2d..e3b9aa372b96 100644
--- a/arch/arm64/mm/proc.S
+++ b/arch/arm64/mm/proc.S
@@ -45,19 +45,6 @@ 
 #define TCR_KASAN_FLAGS 0
 #endif
 
-/*
- * Default MAIR_EL1. MT_NORMAL_TAGGED is initially mapped as Normal memory and
- * changed during __cpu_setup to Normal Tagged if the system supports MTE.
- */
-#define MAIR_EL1_SET							\
-	(MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRnE) |	\
-	 MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRE, MT_DEVICE_nGnRE) |	\
-	 MAIR_ATTRIDX(MAIR_ATTR_DEVICE_GRE, MT_DEVICE_GRE) |		\
-	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL_NC, MT_NORMAL_NC) |		\
-	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL) |			\
-	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL_WT, MT_NORMAL_WT) |		\
-	 MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL_TAGGED))
-
 #ifdef CONFIG_CPU_PM
 /**
  * cpu_do_suspend - save CPU registers context
@@ -425,7 +412,7 @@  SYM_FUNC_START(__cpu_setup)
 	/*
 	 * Memory region attributes
 	 */
-	mov_q	x5, MAIR_EL1_SET
+	mov_q	x5, MAIR_ELx_SET
 #ifdef CONFIG_ARM64_MTE
 	/*
 	 * Update MAIR_EL1, GCR_EL1 and TFSR*_EL1 if MTE is supported