2020-11-16 22:27:26

by David Brazdil

[permalink] [raw]
Subject: [PATCH v2 04/24] arm64: Move MAIR_EL1_SET to asm/memory.h

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 <[email protected]>
---
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
#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


2020-11-23 13:57:30

by Marc Zyngier

[permalink] [raw]
Subject: Re: [PATCH v2 04/24] arm64: Move MAIR_EL1_SET to asm/memory.h

On Mon, 16 Nov 2020 20:42:58 +0000,
David Brazdil <[email protected]> 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 <[email protected]>
> ---
> 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.

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

2020-11-25 10:34:03

by David Brazdil

[permalink] [raw]
Subject: Re: [PATCH v2 04/24] arm64: Move MAIR_EL1_SET to asm/memory.h

On Mon, Nov 23, 2020 at 01:52:54PM +0000, Marc Zyngier wrote:
> On Mon, 16 Nov 2020 20:42:58 +0000,
> David Brazdil <[email protected]> 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 <[email protected]>
> > ---
> > 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.

2020-11-25 11:25:10

by Marc Zyngier

[permalink] [raw]
Subject: Re: [PATCH v2 04/24] arm64: Move MAIR_EL1_SET to asm/memory.h

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 <[email protected]> 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 <[email protected]>
>> > ---
>> > 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 */

--
Jazz is not dead. It just smells funny...

2020-11-25 13:30:29

by David Brazdil

[permalink] [raw]
Subject: Re: [PATCH v2 04/24] arm64: Move MAIR_EL1_SET to asm/memory.h

> > > > +/*
> > > > + * 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... :(

2020-11-25 13:37:47

by Marc Zyngier

[permalink] [raw]
Subject: Re: [PATCH v2 04/24] arm64: Move MAIR_EL1_SET to asm/memory.h

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.
--
Jazz is not dead. It just smells funny...