[PATCH 1/4] arm64: cpu_errata: fix override-init warnings

[PATCH 1/4] arm64: cpu_errata: fix override-init warnings

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

The CPU table causes a handful of warnings because of fields that
have more than one initializer, e.g.

arch/arm64/kernel/cpu_errata.c:127:13: warning: initialized field overwritten [-Woverride-init]
  127 |  .matches = is_affected_midr_range,   \
      |             ^~~~~~~~~~~~~~~~~~~~~~
arch/arm64/kernel/cpu_errata.c:139:2: note: in expansion of macro 'CAP_MIDR_RANGE'
  139 |  CAP_MIDR_RANGE(model, v_min, r_min, v_max, r_max)
      |  ^~~~~~~~~~~~~~
arch/arm64/kernel/cpu_errata.c:151:2: note: in expansion of macro 'ERRATA_MIDR_RANGE'
  151 |  ERRATA_MIDR_RANGE(model, var, rev, var, rev)
      |  ^~~~~~~~~~~~~~~~~
arch/arm64/kernel/cpu_errata.c:317:3: note: in expansion of macro 'ERRATA_MIDR_REV'
  317 |   ERRATA_MIDR_REV(MIDR_BRAHMA_B53, 0, 0),
      |   ^~~~~~~~~~~~~~~

Address all of these by removing the extra initializer that
has no effect on the output.

Fixes: 1cf45b8fdbb8 ("arm64: apply ARM64_ERRATUM_843419 workaround for Brahma-B53 core")
Fixes: bf87bb0881d0 ("arm64: Allow booting of late CPUs affected by erratum 1418040")
Fixes: 93916beb7014 ("arm64: Enable workaround for Cavium TX2 erratum 219 when running SMT")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/arm64/kernel/cpu_errata.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index 24d75af344b1..2321f52e396f 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -307,13 +307,11 @@ static const struct midr_range erratum_845719_list[] = {
 static const struct arm64_cpu_capabilities erratum_843419_list[] = {
 	{
 		/* Cortex-A53 r0p[01234] */
-		.matches = is_affected_midr_range,
 		ERRATA_MIDR_REV_RANGE(MIDR_CORTEX_A53, 0, 0, 4),
 		MIDR_FIXED(0x4, BIT(8)),
 	},
 	{
 		/* Brahma-B53 r0p[0] */
-		.matches = is_affected_midr_range,
 		ERRATA_MIDR_REV(MIDR_BRAHMA_B53, 0, 0),
 	},
 	{},
@@ -475,7 +473,7 @@ const struct arm64_cpu_capabilities arm64_errata[] = {
 	{
 		.desc = "ARM erratum 1418040",
 		.capability = ARM64_WORKAROUND_1418040,
-		ERRATA_MIDR_RANGE_LIST(erratum_1418040_list),
+		CAP_MIDR_RANGE_LIST(erratum_1418040_list),
 		/*
 		 * We need to allow affected CPUs to come in late, but
 		 * also need the non-affected CPUs to be able to come
@@ -504,8 +502,8 @@ const struct arm64_cpu_capabilities arm64_errata[] = {
 	{
 		.desc = "Cavium ThunderX2 erratum 219 (KVM guest sysreg trapping)",
 		.capability = ARM64_WORKAROUND_CAVIUM_TX2_219_TVM,
-		ERRATA_MIDR_RANGE_LIST(tx2_family_cpus),
 		.matches = needs_tx2_tvm_workaround,
+		.midr_range_list = tx2_family_cpus,
 	},
 	{
 		.desc = "Cavium ThunderX2 erratum 219 (PRFM removal)",
-- 
2.27.0

[PATCH 2/4] arm64: hide more compat_vdso code

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

When CONFIG_COMPAT_VDSO is disabled, we get a warning
about a potential out-of-bounds access:

arch/arm64/kernel/vdso.c: In function 'aarch32_vdso_mremap':
arch/arm64/kernel/vdso.c:86:37: warning: array subscript 1 is above array bounds of 'struct vdso_abi_info[1]' [-Warray-bounds]
   86 |  unsigned long vdso_size = vdso_info[abi].vdso_code_end -
      |                            ~~~~~~~~~^~~~~

This is all in dead code however that the compiler is unable to
eliminate by itself.

Change the array to individual local variables that can be
dropped in dead code elimination to let the compiler understand
this better.

Fixes: 0cbc2659123e ("arm64: vdso32: Remove a bunch of #ifdef CONFIG_COMPAT_VDSO guards")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/arm64/kernel/vdso.c | 56 ++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 31 deletions(-)

diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c
index debb8995d57f..0b69d2894742 100644
--- a/arch/arm64/kernel/vdso.c
+++ b/arch/arm64/kernel/vdso.c
@@ -286,36 +286,9 @@ static int aarch32_vdso_mremap(const struct vm_special_mapping *sm,
 	return __vdso_remap(VDSO_ABI_AA32, sm, new_vma);
 }
 
-enum aarch32_map {
-	AA32_MAP_VECTORS, /* kuser helpers */
-	AA32_MAP_SIGPAGE,
-	AA32_MAP_VVAR,
-	AA32_MAP_VDSO,
-};
-
 static struct page *aarch32_vectors_page __ro_after_init;
 static struct page *aarch32_sig_page __ro_after_init;
 
-static struct vm_special_mapping aarch32_vdso_maps[] = {
-	[AA32_MAP_VECTORS] = {
-		.name	= "[vectors]", /* ABI */
-		.pages	= &aarch32_vectors_page,
-	},
-	[AA32_MAP_SIGPAGE] = {
-		.name	= "[sigpage]", /* ABI */
-		.pages	= &aarch32_sig_page,
-	},
-	[AA32_MAP_VVAR] = {
-		.name = "[vvar]",
-		.fault = vvar_fault,
-		.mremap = vvar_mremap,
-	},
-	[AA32_MAP_VDSO] = {
-		.name = "[vdso]",
-		.mremap = aarch32_vdso_mremap,
-	},
-};
-
 static int aarch32_alloc_kuser_vdso_page(void)
 {
 	extern char __kuser_helper_start[], __kuser_helper_end[];
@@ -352,14 +325,25 @@ static int aarch32_alloc_sigpage(void)
 	return 0;
 }
 
+static struct vm_special_mapping aarch32_vdso_map_vvar = {
+	.name = "[vvar]",
+	.fault = vvar_fault,
+	.mremap = vvar_mremap,
+};
+
+static struct vm_special_mapping aarch32_vdso_map_vdso = {
+	.name = "[vdso]",
+	.mremap = aarch32_vdso_mremap,
+};
+
 static int __aarch32_alloc_vdso_pages(void)
 {
 
 	if (!IS_ENABLED(CONFIG_COMPAT_VDSO))
 		return 0;
 
-	vdso_info[VDSO_ABI_AA32].dm = &aarch32_vdso_maps[AA32_MAP_VVAR];
-	vdso_info[VDSO_ABI_AA32].cm = &aarch32_vdso_maps[AA32_MAP_VDSO];
+	vdso_info[VDSO_ABI_AA32].dm = &aarch32_vdso_map_vvar;
+	vdso_info[VDSO_ABI_AA32].cm = &aarch32_vdso_map_vdso;
 
 	return __vdso_init(VDSO_ABI_AA32);
 }
@@ -380,6 +364,11 @@ static int __init aarch32_alloc_vdso_pages(void)
 }
 arch_initcall(aarch32_alloc_vdso_pages);
 
+static struct vm_special_mapping aarch32_vdso_map_vectors = {
+	.name	= "[vectors]", /* ABI */
+	.pages	= &aarch32_vectors_page,
+};
+
 static int aarch32_kuser_helpers_setup(struct mm_struct *mm)
 {
 	void *ret;
@@ -394,11 +383,16 @@ static int aarch32_kuser_helpers_setup(struct mm_struct *mm)
 	ret = _install_special_mapping(mm, AARCH32_VECTORS_BASE, PAGE_SIZE,
 				       VM_READ | VM_EXEC |
 				       VM_MAYREAD | VM_MAYEXEC,
-				       &aarch32_vdso_maps[AA32_MAP_VECTORS]);
+				       &aarch32_vdso_map_vectors);
 
 	return PTR_ERR_OR_ZERO(ret);
 }
 
+static struct vm_special_mapping aarch32_vdso_map_sigpage = {
+	.name	= "[sigpage]", /* ABI */
+	.pages	= &aarch32_sig_page,
+};
+
 static int aarch32_sigreturn_setup(struct mm_struct *mm)
 {
 	unsigned long addr;
@@ -417,7 +411,7 @@ static int aarch32_sigreturn_setup(struct mm_struct *mm)
 	ret = _install_special_mapping(mm, addr, PAGE_SIZE,
 				       VM_READ | VM_EXEC | VM_MAYREAD |
 				       VM_MAYWRITE | VM_MAYEXEC,
-				       &aarch32_vdso_maps[AA32_MAP_SIGPAGE]);
+				       &aarch32_vdso_map_sigpage);
 	if (IS_ERR(ret))
 		goto out;
 
-- 
2.27.0

[PATCH 3/4] arm64: avoid -Woverride-init warning

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

The icache_policy_str[] definition causes a warning when extra
warning flags are enabled:

arch/arm64/kernel/cpuinfo.c:38:26: warning: initialized field overwritten [-Woverride-init]
   38 |  [ICACHE_POLICY_VIPT]  = "VIPT",
      |                          ^~~~~~
arch/arm64/kernel/cpuinfo.c:38:26: note: (near initialization for 'icache_policy_str[2]')
arch/arm64/kernel/cpuinfo.c:39:26: warning: initialized field overwritten [-Woverride-init]
   39 |  [ICACHE_POLICY_PIPT]  = "PIPT",
      |                          ^~~~~~
arch/arm64/kernel/cpuinfo.c:39:26: note: (near initialization for 'icache_policy_str[3]')
arch/arm64/kernel/cpuinfo.c:40:27: warning: initialized field overwritten [-Woverride-init]
   40 |  [ICACHE_POLICY_VPIPT]  = "VPIPT",
      |                           ^~~~~~~
arch/arm64/kernel/cpuinfo.c:40:27: note: (near initialization for 'icache_policy_str[0]')

There is no real need for the default initializer here, as printing a
NULL string is harmless. Rewrite the logic to have an explicit
reserved value for the only one that uses the default value.

This partially reverts the commit that removed ICACHE_POLICY_AIVIVT.

Fixes: 155433cb365e ("arm64: cache: Remove support for ASID-tagged VIVT I-caches")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/arm64/include/asm/cache.h | 1 +
 arch/arm64/kernel/cpuinfo.c    | 5 +++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/cache.h b/arch/arm64/include/asm/cache.h
index a4d1b5f771f6..16e1e16e7e61 100644
--- a/arch/arm64/include/asm/cache.h
+++ b/arch/arm64/include/asm/cache.h
@@ -24,6 +24,7 @@
 #define CTR_L1IP(ctr)		(((ctr) >> CTR_L1IP_SHIFT) & CTR_L1IP_MASK)
 
 #define ICACHE_POLICY_VPIPT	0
+#define ICACHE_POLICY_RESERVED	1
 #define ICACHE_POLICY_VIPT	2
 #define ICACHE_POLICY_PIPT	3
 
diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
index 6a7bb3729d60..b63269c7fcdb 100644
--- a/arch/arm64/kernel/cpuinfo.c
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -34,10 +34,10 @@ DEFINE_PER_CPU(struct cpuinfo_arm64, cpu_data);
 static struct cpuinfo_arm64 boot_cpu_data;
 
 static const char *icache_policy_str[] = {
-	[0 ... ICACHE_POLICY_PIPT]	= "RESERVED/UNKNOWN",
+	[ICACHE_POLICY_VPIPT]		= "VPIPT",
+	[ICACHE_POLICY_RESERVED]	= "RESERVED/UNKNOWN",
 	[ICACHE_POLICY_VIPT]		= "VIPT",
 	[ICACHE_POLICY_PIPT]		= "PIPT",
-	[ICACHE_POLICY_VPIPT]		= "VPIPT",
 };
 
 unsigned long __icache_flags;
@@ -335,6 +335,7 @@ static void cpuinfo_detect_icache_policy(struct cpuinfo_arm64 *info)
 		set_bit(ICACHEF_VPIPT, &__icache_flags);
 		break;
 	default:
+	case ICACHE_POLICY_RESERVED:
 	case ICACHE_POLICY_VIPT:
 		/* Assume aliasing */
 		set_bit(ICACHEF_ALIASING, &__icache_flags);
-- 
2.27.0

[PATCH 4/4] arm64: traps: fix -Woverride-init warnings

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

There are many warnings in this file when we re-enable the
Woverride-init flag:

arch/arm64/kernel/traps.c:704:26: warning: initialized field overwritten [-Woverride-init]
  704 |  [ESR_ELx_EC_UNKNOWN]  = "Unknown/Uncategorized",
      |                          ^~~~~~~~~~~~~~~~~~~~~~~
arch/arm64/kernel/traps.c:704:26: note: (near initialization for 'esr_class_str[0]')
arch/arm64/kernel/traps.c:705:22: warning: initialized field overwritten [-Woverride-init]
  705 |  [ESR_ELx_EC_WFx]  = "WFI/WFE",
      |                      ^~~~~~~~~

This is harmless since they are only informational strings,
but it's easy to change the code to ignore missing initialization
and instead warn about possible duplicate initializers.

Fixes: 60a1f02c9e91 ("arm64: decode ESR_ELx.EC when reporting exceptions")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/arm64/kernel/traps.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 8af4e0e85736..d21cb25f9e1f 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -700,7 +700,6 @@ void do_sysinstr(unsigned int esr, struct pt_regs *regs)
 NOKPROBE_SYMBOL(do_sysinstr);
 
 static const char *esr_class_str[] = {
-	[0 ... ESR_ELx_EC_MAX]		= "UNRECOGNIZED EC",
 	[ESR_ELx_EC_UNKNOWN]		= "Unknown/Uncategorized",
 	[ESR_ELx_EC_WFx]		= "WFI/WFE",
 	[ESR_ELx_EC_CP15_32]		= "CP15 MCR/MRC",
@@ -746,7 +745,7 @@ static const char *esr_class_str[] = {
 
 const char *esr_get_class_string(u32 esr)
 {
-	return esr_class_str[ESR_ELx_EC(esr)];
+	return esr_class_str[ESR_ELx_EC(esr)] ?: "UNRECOGNIZED EC";
 }
 
 /*
-- 
2.27.0

Re: [PATCH 4/4] arm64: traps: fix -Woverride-init warnings

[Mark Rutland]

Hi Arnd,

On Mon, Oct 26, 2020 at 05:03:31PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> There are many warnings in this file when we re-enable the
> Woverride-init flag:
> 
> arch/arm64/kernel/traps.c:704:26: warning: initialized field overwritten [-Woverride-init]
>   704 |  [ESR_ELx_EC_UNKNOWN]  = "Unknown/Uncategorized",
>       |                          ^~~~~~~~~~~~~~~~~~~~~~~
> arch/arm64/kernel/traps.c:704:26: note: (near initialization for 'esr_class_str[0]')
> arch/arm64/kernel/traps.c:705:22: warning: initialized field overwritten [-Woverride-init]
>   705 |  [ESR_ELx_EC_WFx]  = "WFI/WFE",
>       |                      ^~~~~~~~~
> 
> This is harmless since they are only informational strings,
> but it's easy to change the code to ignore missing initialization
> and instead warn about possible duplicate initializers.

This has come up before, and IMO the warning is more hindrance than
helpful, given the prevalance of spurious warnings, and the (again IMO)
the rework needed to avoid those making the code harder to reason about.

We use this pattern all througout the kernel (e.g. in the syscall
wrappers), so unless the plan is to avoid this everywhere, I don't think
that we should alter individual cases. I also don't think that the Fixes
tag is appropriate given the code is correct.

Could we instead convince the compiler folk to give us better tools to
deal with this? For example, if we could annotate assignmments as
overridable or being an override, it'd be possible to distinguish the
benign cases from bad ones, without forcing us to have dynamic checks.

Thanks,
Mark.

> 
> Fixes: 60a1f02c9e91 ("arm64: decode ESR_ELx.EC when reporting exceptions")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  arch/arm64/kernel/traps.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 8af4e0e85736..d21cb25f9e1f 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -700,7 +700,6 @@ void do_sysinstr(unsigned int esr, struct pt_regs *regs)
>  NOKPROBE_SYMBOL(do_sysinstr);
>  
>  static const char *esr_class_str[] = {
> -	[0 ... ESR_ELx_EC_MAX]		= "UNRECOGNIZED EC",
>  	[ESR_ELx_EC_UNKNOWN]		= "Unknown/Uncategorized",
>  	[ESR_ELx_EC_WFx]		= "WFI/WFE",
>  	[ESR_ELx_EC_CP15_32]		= "CP15 MCR/MRC",
> @@ -746,7 +745,7 @@ static const char *esr_class_str[] = {
>  
>  const char *esr_get_class_string(u32 esr)
>  {
> -	return esr_class_str[ESR_ELx_EC(esr)];
> +	return esr_class_str[ESR_ELx_EC(esr)] ?: "UNRECOGNIZED EC";
>  }
>  
>  /*
> -- 
> 2.27.0
> 

Re: [PATCH 4/4] arm64: traps: fix -Woverride-init warnings

[Arnd Bergmann]

On Mon, Oct 26, 2020 at 5:23 PM Mark Rutland <mark.rutland@arm.com> wrote:
> On Mon, Oct 26, 2020 at 05:03:31PM +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > There are many warnings in this file when we re-enable the
> > Woverride-init flag:
> >
> > arch/arm64/kernel/traps.c:704:26: warning: initialized field overwritten [-Woverride-init]
> >   704 |  [ESR_ELx_EC_UNKNOWN]  = "Unknown/Uncategorized",
> >       |                          ^~~~~~~~~~~~~~~~~~~~~~~
> > arch/arm64/kernel/traps.c:704:26: note: (near initialization for 'esr_class_str[0]')
> > arch/arm64/kernel/traps.c:705:22: warning: initialized field overwritten [-Woverride-init]
> >   705 |  [ESR_ELx_EC_WFx]  = "WFI/WFE",
> >       |                      ^~~~~~~~~
> >
> > This is harmless since they are only informational strings,
> > but it's easy to change the code to ignore missing initialization
> > and instead warn about possible duplicate initializers.
>
> This has come up before, and IMO the warning is more hindrance than
> helpful, given the prevalance of spurious warnings, and the (again IMO)
> the rework needed to avoid those making the code harder to reason about.
>
> We use this pattern all througout the kernel (e.g. in the syscall
> wrappers), so unless the plan is to avoid this everywhere, I don't think
> that we should alter individual cases.

I have patches for all instances, yes.

> I also don't think that the Fixes tag is appropriate given the code is correct.

I tend to add fixes tags even for false-positive warnings, as they
are helpful whenever someone tries to backport the warning
suppression patch to older kernels. That could easily be dropped
here of course.

> Could we instead convince the compiler folk to give us better tools to
> deal with this? For example, if we could annotate assignmments as
> overridable or being an override, it'd be possible to distinguish the
> benign cases from bad ones, without forcing us to have dynamic checks.

There are only a handful of instances that need this, and half of these
are in arch/arm64/. I have another patch that disables the warning
locally in arch/arm64/kernel/{perf_event,sys,sys32}.c and
arch/arm64/kvm/handle_exit.c, but this needs some extra
infrastructure to make it possible to disable it for both gcc
and clang (which use different warning flags for it), so I did not
include the patch in this series.

I had also considered disabling the two warning flags for the
entire arch/arm64/kernel/ directory, but that seemed wrong
after I noticed the cpu_errata.c warnings that are indeed suspicious
and could lead to bugs when additional changes are made to
that file.

     Arnd

Re: [PATCH 2/4] arm64: hide more compat_vdso code

[Mark Rutland]

On Mon, Oct 26, 2020 at 05:03:29PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> When CONFIG_COMPAT_VDSO is disabled, we get a warning
> about a potential out-of-bounds access:
> 
> arch/arm64/kernel/vdso.c: In function 'aarch32_vdso_mremap':
> arch/arm64/kernel/vdso.c:86:37: warning: array subscript 1 is above array bounds of 'struct vdso_abi_info[1]' [-Warray-bounds]
>    86 |  unsigned long vdso_size = vdso_info[abi].vdso_code_end -
>       |                            ~~~~~~~~~^~~~~
> 
> This is all in dead code however that the compiler is unable to
> eliminate by itself.
> 
> Change the array to individual local variables that can be
> dropped in dead code elimination to let the compiler understand
> this better.
> 
> Fixes: 0cbc2659123e ("arm64: vdso32: Remove a bunch of #ifdef CONFIG_COMPAT_VDSO guards")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

This looks like a nice cleanup to me! I agree we don't need the array
here.

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

Thanks,
Mark.

> ---
>  arch/arm64/kernel/vdso.c | 56 ++++++++++++++++++----------------------
>  1 file changed, 25 insertions(+), 31 deletions(-)
> 
> diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c
> index debb8995d57f..0b69d2894742 100644
> --- a/arch/arm64/kernel/vdso.c
> +++ b/arch/arm64/kernel/vdso.c
> @@ -286,36 +286,9 @@ static int aarch32_vdso_mremap(const struct vm_special_mapping *sm,
>  	return __vdso_remap(VDSO_ABI_AA32, sm, new_vma);
>  }
>  
> -enum aarch32_map {
> -	AA32_MAP_VECTORS, /* kuser helpers */
> -	AA32_MAP_SIGPAGE,
> -	AA32_MAP_VVAR,
> -	AA32_MAP_VDSO,
> -};
> -
>  static struct page *aarch32_vectors_page __ro_after_init;
>  static struct page *aarch32_sig_page __ro_after_init;
>  
> -static struct vm_special_mapping aarch32_vdso_maps[] = {
> -	[AA32_MAP_VECTORS] = {
> -		.name	= "[vectors]", /* ABI */
> -		.pages	= &aarch32_vectors_page,
> -	},
> -	[AA32_MAP_SIGPAGE] = {
> -		.name	= "[sigpage]", /* ABI */
> -		.pages	= &aarch32_sig_page,
> -	},
> -	[AA32_MAP_VVAR] = {
> -		.name = "[vvar]",
> -		.fault = vvar_fault,
> -		.mremap = vvar_mremap,
> -	},
> -	[AA32_MAP_VDSO] = {
> -		.name = "[vdso]",
> -		.mremap = aarch32_vdso_mremap,
> -	},
> -};
> -
>  static int aarch32_alloc_kuser_vdso_page(void)
>  {
>  	extern char __kuser_helper_start[], __kuser_helper_end[];
> @@ -352,14 +325,25 @@ static int aarch32_alloc_sigpage(void)
>  	return 0;
>  }
>  
> +static struct vm_special_mapping aarch32_vdso_map_vvar = {
> +	.name = "[vvar]",
> +	.fault = vvar_fault,
> +	.mremap = vvar_mremap,
> +};
> +
> +static struct vm_special_mapping aarch32_vdso_map_vdso = {
> +	.name = "[vdso]",
> +	.mremap = aarch32_vdso_mremap,
> +};
> +
>  static int __aarch32_alloc_vdso_pages(void)
>  {
>  
>  	if (!IS_ENABLED(CONFIG_COMPAT_VDSO))
>  		return 0;
>  
> -	vdso_info[VDSO_ABI_AA32].dm = &aarch32_vdso_maps[AA32_MAP_VVAR];
> -	vdso_info[VDSO_ABI_AA32].cm = &aarch32_vdso_maps[AA32_MAP_VDSO];
> +	vdso_info[VDSO_ABI_AA32].dm = &aarch32_vdso_map_vvar;
> +	vdso_info[VDSO_ABI_AA32].cm = &aarch32_vdso_map_vdso;
>  
>  	return __vdso_init(VDSO_ABI_AA32);
>  }
> @@ -380,6 +364,11 @@ static int __init aarch32_alloc_vdso_pages(void)
>  }
>  arch_initcall(aarch32_alloc_vdso_pages);
>  
> +static struct vm_special_mapping aarch32_vdso_map_vectors = {
> +	.name	= "[vectors]", /* ABI */
> +	.pages	= &aarch32_vectors_page,
> +};
> +
>  static int aarch32_kuser_helpers_setup(struct mm_struct *mm)
>  {
>  	void *ret;
> @@ -394,11 +383,16 @@ static int aarch32_kuser_helpers_setup(struct mm_struct *mm)
>  	ret = _install_special_mapping(mm, AARCH32_VECTORS_BASE, PAGE_SIZE,
>  				       VM_READ | VM_EXEC |
>  				       VM_MAYREAD | VM_MAYEXEC,
> -				       &aarch32_vdso_maps[AA32_MAP_VECTORS]);
> +				       &aarch32_vdso_map_vectors);
>  
>  	return PTR_ERR_OR_ZERO(ret);
>  }
>  
> +static struct vm_special_mapping aarch32_vdso_map_sigpage = {
> +	.name	= "[sigpage]", /* ABI */
> +	.pages	= &aarch32_sig_page,
> +};
> +
>  static int aarch32_sigreturn_setup(struct mm_struct *mm)
>  {
>  	unsigned long addr;
> @@ -417,7 +411,7 @@ static int aarch32_sigreturn_setup(struct mm_struct *mm)
>  	ret = _install_special_mapping(mm, addr, PAGE_SIZE,
>  				       VM_READ | VM_EXEC | VM_MAYREAD |
>  				       VM_MAYWRITE | VM_MAYEXEC,
> -				       &aarch32_vdso_maps[AA32_MAP_SIGPAGE]);
> +				       &aarch32_vdso_map_sigpage);
>  	if (IS_ERR(ret))
>  		goto out;
>  
> -- 
> 2.27.0
> 

Re: [PATCH 3/4] arm64: avoid -Woverride-init warning

[Mark Rutland]

On Mon, Oct 26, 2020 at 05:03:30PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The icache_policy_str[] definition causes a warning when extra
> warning flags are enabled:
> 
> arch/arm64/kernel/cpuinfo.c:38:26: warning: initialized field overwritten [-Woverride-init]
>    38 |  [ICACHE_POLICY_VIPT]  = "VIPT",
>       |                          ^~~~~~
> arch/arm64/kernel/cpuinfo.c:38:26: note: (near initialization for 'icache_policy_str[2]')
> arch/arm64/kernel/cpuinfo.c:39:26: warning: initialized field overwritten [-Woverride-init]
>    39 |  [ICACHE_POLICY_PIPT]  = "PIPT",
>       |                          ^~~~~~
> arch/arm64/kernel/cpuinfo.c:39:26: note: (near initialization for 'icache_policy_str[3]')
> arch/arm64/kernel/cpuinfo.c:40:27: warning: initialized field overwritten [-Woverride-init]
>    40 |  [ICACHE_POLICY_VPIPT]  = "VPIPT",
>       |                           ^~~~~~~
> arch/arm64/kernel/cpuinfo.c:40:27: note: (near initialization for 'icache_policy_str[0]')
> 
> There is no real need for the default initializer here, as printing a
> NULL string is harmless. Rewrite the logic to have an explicit
> reserved value for the only one that uses the default value.
> 
> This partially reverts the commit that removed ICACHE_POLICY_AIVIVT.
> 
> Fixes: 155433cb365e ("arm64: cache: Remove support for ASID-tagged VIVT I-caches")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

> ---
>  arch/arm64/include/asm/cache.h | 1 +
>  arch/arm64/kernel/cpuinfo.c    | 5 +++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/cache.h b/arch/arm64/include/asm/cache.h
> index a4d1b5f771f6..16e1e16e7e61 100644
> --- a/arch/arm64/include/asm/cache.h
> +++ b/arch/arm64/include/asm/cache.h
> @@ -24,6 +24,7 @@
>  #define CTR_L1IP(ctr)		(((ctr) >> CTR_L1IP_SHIFT) & CTR_L1IP_MASK)
>  
>  #define ICACHE_POLICY_VPIPT	0
> +#define ICACHE_POLICY_RESERVED	1
>  #define ICACHE_POLICY_VIPT	2
>  #define ICACHE_POLICY_PIPT	3
>  
> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
> index 6a7bb3729d60..b63269c7fcdb 100644
> --- a/arch/arm64/kernel/cpuinfo.c
> +++ b/arch/arm64/kernel/cpuinfo.c
> @@ -34,10 +34,10 @@ DEFINE_PER_CPU(struct cpuinfo_arm64, cpu_data);
>  static struct cpuinfo_arm64 boot_cpu_data;
>  
>  static const char *icache_policy_str[] = {
> -	[0 ... ICACHE_POLICY_PIPT]	= "RESERVED/UNKNOWN",
> +	[ICACHE_POLICY_VPIPT]		= "VPIPT",
> +	[ICACHE_POLICY_RESERVED]	= "RESERVED/UNKNOWN",
>  	[ICACHE_POLICY_VIPT]		= "VIPT",
>  	[ICACHE_POLICY_PIPT]		= "PIPT",
> -	[ICACHE_POLICY_VPIPT]		= "VPIPT",
>  };

Given it's not clear that ICACHE_POLICY_PIPT is the max value, I agree
this is a bit cleaner. I don't have a nicer way of making this clearer.

[...]

> @@ -335,6 +335,7 @@ static void cpuinfo_detect_icache_policy(struct cpuinfo_arm64 *info)
>  		set_bit(ICACHEF_VPIPT, &__icache_flags);
>  		break;
>  	default:
> +	case ICACHE_POLICY_RESERVED:
>  	case ICACHE_POLICY_VIPT:
>  		/* Assume aliasing */
>  		set_bit(ICACHEF_ALIASING, &__icache_flags);
>
... but it's a bit weird to have both the default and
ICACHE_POLICY_RESERVED cases. If we get rid of the default case, does
any compiler warn? I suspect the masking in CTR_L1IP() might be
sufficient to let the compiler see we've handled all cases.

Thanks,
Mark.

Re: [PATCH 4/4] arm64: traps: fix -Woverride-init warnings

[Peter Maydell]

On Mon, 26 Oct 2020 at 16:23, Mark Rutland <mark.rutland@arm.com> wrote:
>
> Hi Arnd,
>
> On Mon, Oct 26, 2020 at 05:03:31PM +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > There are many warnings in this file when we re-enable the
> > Woverride-init flag:
> >
> > arch/arm64/kernel/traps.c:704:26: warning: initialized field overwritten [-Woverride-init]
> >   704 |  [ESR_ELx_EC_UNKNOWN]  = "Unknown/Uncategorized",
> >       |                          ^~~~~~~~~~~~~~~~~~~~~~~
> > arch/arm64/kernel/traps.c:704:26: note: (near initialization for 'esr_class_str[0]')
> > arch/arm64/kernel/traps.c:705:22: warning: initialized field overwritten [-Woverride-init]
> >   705 |  [ESR_ELx_EC_WFx]  = "WFI/WFE",
> >       |                      ^~~~~~~~~
> >
> > This is harmless since they are only informational strings,
> > but it's easy to change the code to ignore missing initialization
> > and instead warn about possible duplicate initializers.
>
> This has come up before, and IMO the warning is more hindrance than
> helpful, given the prevalance of spurious warnings, and the (again IMO)
> the rework needed to avoid those making the code harder to reason about

FWIW in QEMU we turn the clang version of this off with
-Wno-initializer-overrides because we agree that the code is
fine and the compiler is being unhelpful in this case. (There's
a reason gcc doesn't put it in -Wall.)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91688 is a request
for something that would catch bugs without breaking ranged-array
initializer syntax usage, but the gcc devs don't seem to have
responded.

thanks
-- PMM

Re: [PATCH 4/4] arm64: traps: fix -Woverride-init warnings

[Will Deacon]

On Mon, Oct 26, 2020 at 05:13:30PM +0000, Peter Maydell wrote:
> On Mon, 26 Oct 2020 at 16:23, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Mon, Oct 26, 2020 at 05:03:31PM +0100, Arnd Bergmann wrote:
> > > From: Arnd Bergmann <arnd@arndb.de>
> > >
> > > There are many warnings in this file when we re-enable the
> > > Woverride-init flag:
> > >
> > > arch/arm64/kernel/traps.c:704:26: warning: initialized field overwritten [-Woverride-init]
> > >   704 |  [ESR_ELx_EC_UNKNOWN]  = "Unknown/Uncategorized",
> > >       |                          ^~~~~~~~~~~~~~~~~~~~~~~
> > > arch/arm64/kernel/traps.c:704:26: note: (near initialization for 'esr_class_str[0]')
> > > arch/arm64/kernel/traps.c:705:22: warning: initialized field overwritten [-Woverride-init]
> > >   705 |  [ESR_ELx_EC_WFx]  = "WFI/WFE",
> > >       |                      ^~~~~~~~~
> > >
> > > This is harmless since they are only informational strings,
> > > but it's easy to change the code to ignore missing initialization
> > > and instead warn about possible duplicate initializers.
> >
> > This has come up before, and IMO the warning is more hindrance than
> > helpful, given the prevalance of spurious warnings, and the (again IMO)
> > the rework needed to avoid those making the code harder to reason about
> 
> FWIW in QEMU we turn the clang version of this off with
> -Wno-initializer-overrides because we agree that the code is
> fine and the compiler is being unhelpful in this case. (There's
> a reason gcc doesn't put it in -Wall.)
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91688 is a request
> for something that would catch bugs without breaking ranged-array
> initializer syntax usage, but the gcc devs don't seem to have
> responded.

Yes, I'm inclined to agree. The code is fine, and "fixing" it just leads to
churn and the possible introduction of bugs.

Will

Re: [PATCH 4/4] arm64: traps: fix -Woverride-init warnings

[Arnd Bergmann]

On Mon, Oct 26, 2020 at 6:27 PM Will Deacon <will@kernel.org> wrote:
> On Mon, Oct 26, 2020 at 05:13:30PM +0000, Peter Maydell wrote:
> > On Mon, 26 Oct 2020 at 16:23, Mark Rutland <mark.rutland@arm.com> wrote:
> > > On Mon, Oct 26, 2020 at 05:03:31PM +0100, Arnd Bergmann wrote:
> > > > From: Arnd Bergmann <arnd@arndb.de>
> > > >
> > > > There are many warnings in this file when we re-enable the
> > > > Woverride-init flag:
> > > >
> > > > arch/arm64/kernel/traps.c:704:26: warning: initialized field overwritten [-Woverride-init]
> > > >   704 |  [ESR_ELx_EC_UNKNOWN]  = "Unknown/Uncategorized",
> > > >       |                          ^~~~~~~~~~~~~~~~~~~~~~~
> > > > arch/arm64/kernel/traps.c:704:26: note: (near initialization for 'esr_class_str[0]')
> > > > arch/arm64/kernel/traps.c:705:22: warning: initialized field overwritten [-Woverride-init]
> > > >   705 |  [ESR_ELx_EC_WFx]  = "WFI/WFE",
> > > >       |                      ^~~~~~~~~
> > > >
> > > > This is harmless since they are only informational strings,
> > > > but it's easy to change the code to ignore missing initialization
> > > > and instead warn about possible duplicate initializers.
> > >
> > > This has come up before, and IMO the warning is more hindrance than
> > > helpful, given the prevalance of spurious warnings, and the (again IMO)
> > > the rework needed to avoid those making the code harder to reason about
> >
> > FWIW in QEMU we turn the clang version of this off with
> > -Wno-initializer-overrides because we agree that the code is
> > fine and the compiler is being unhelpful in this case. (There's
> > a reason gcc doesn't put it in -Wall.)
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91688 is a request
> > for something that would catch bugs without breaking ranged-array
> > initializer syntax usage, but the gcc devs don't seem to have
> > responded.
>
> Yes, I'm inclined to agree. The code is fine, and "fixing" it just leads to
> churn and the possible introduction of bugs.

Ok, shall we just disable it for all of arch/arm64/kernel then?
The other parts of the kernel that follow the same line of thinking
are drivers/gpu/drm/amd/ and drivers/ata, for which I already
just turn them off. The rest of the kernel is mostly clean for
the warning, or there are actual bugs that it finds.

       Arnd

Re: [PATCH 3/4] arm64: avoid -Woverride-init warning

[Arnd Bergmann]

On Mon, Oct 26, 2020 at 6:01 PM Mark Rutland <mark.rutland@arm.com> wrote:
> On Mon, Oct 26, 2020 at 05:03:30PM +0100, Arnd Bergmann wrote:

> > @@ -335,6 +335,7 @@ static void cpuinfo_detect_icache_policy(struct cpuinfo_arm64 *info)
> >               set_bit(ICACHEF_VPIPT, &__icache_flags);
> >               break;
> >       default:
> > +     case ICACHE_POLICY_RESERVED:
> >       case ICACHE_POLICY_VIPT:
> >               /* Assume aliasing */
> >               set_bit(ICACHEF_ALIASING, &__icache_flags);
> >
> ... but it's a bit weird to have both the default and
> ICACHE_POLICY_RESERVED cases. If we get rid of the default case, does
> any compiler warn? I suspect the masking in CTR_L1IP() might be
> sufficient to let the compiler see we've handled all cases.

It's not an enum, so the compiler doesn't actually know what the
complete set is and doesn't warn without the default. I'll send a v2.

      Arnd

Re: [PATCH 4/4] arm64: traps: fix -Woverride-init warnings

[Will Deacon]

On Mon, Oct 26, 2020 at 08:28:45PM +0100, Arnd Bergmann wrote:
> On Mon, Oct 26, 2020 at 6:27 PM Will Deacon <will@kernel.org> wrote:
> > On Mon, Oct 26, 2020 at 05:13:30PM +0000, Peter Maydell wrote:
> > > On Mon, 26 Oct 2020 at 16:23, Mark Rutland <mark.rutland@arm.com> wrote:
> > > > On Mon, Oct 26, 2020 at 05:03:31PM +0100, Arnd Bergmann wrote:
> > > > > From: Arnd Bergmann <arnd@arndb.de>
> > > > >
> > > > > There are many warnings in this file when we re-enable the
> > > > > Woverride-init flag:
> > > > >
> > > > > arch/arm64/kernel/traps.c:704:26: warning: initialized field overwritten [-Woverride-init]
> > > > >   704 |  [ESR_ELx_EC_UNKNOWN]  = "Unknown/Uncategorized",
> > > > >       |                          ^~~~~~~~~~~~~~~~~~~~~~~
> > > > > arch/arm64/kernel/traps.c:704:26: note: (near initialization for 'esr_class_str[0]')
> > > > > arch/arm64/kernel/traps.c:705:22: warning: initialized field overwritten [-Woverride-init]
> > > > >   705 |  [ESR_ELx_EC_WFx]  = "WFI/WFE",
> > > > >       |                      ^~~~~~~~~
> > > > >
> > > > > This is harmless since they are only informational strings,
> > > > > but it's easy to change the code to ignore missing initialization
> > > > > and instead warn about possible duplicate initializers.
> > > >
> > > > This has come up before, and IMO the warning is more hindrance than
> > > > helpful, given the prevalance of spurious warnings, and the (again IMO)
> > > > the rework needed to avoid those making the code harder to reason about
> > >
> > > FWIW in QEMU we turn the clang version of this off with
> > > -Wno-initializer-overrides because we agree that the code is
> > > fine and the compiler is being unhelpful in this case. (There's
> > > a reason gcc doesn't put it in -Wall.)
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91688 is a request
> > > for something that would catch bugs without breaking ranged-array
> > > initializer syntax usage, but the gcc devs don't seem to have
> > > responded.
> >
> > Yes, I'm inclined to agree. The code is fine, and "fixing" it just leads to
> > churn and the possible introduction of bugs.
> 
> Ok, shall we just disable it for all of arch/arm64/kernel then?
> The other parts of the kernel that follow the same line of thinking
> are drivers/gpu/drm/amd/ and drivers/ata, for which I already
> just turn them off. The rest of the kernel is mostly clean for
> the warning, or there are actual bugs that it finds.

I'm happy to take a patch disabling it, either for arch/arm64/kernel or
arch/arm64 as a whole. Thanks!

Will

Re: [PATCH 2/4] arm64: hide more compat_vdso code

[Arnd Bergmann]

On Mon, Oct 26, 2020 at 5:55 PM Mark Rutland <mark.rutland@arm.com> wrote:
> On Mon, Oct 26, 2020 at 05:03:29PM +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > When CONFIG_COMPAT_VDSO is disabled, we get a warning
> > about a potential out-of-bounds access:
> >
> > arch/arm64/kernel/vdso.c: In function 'aarch32_vdso_mremap':
> > arch/arm64/kernel/vdso.c:86:37: warning: array subscript 1 is above array bounds of 'struct vdso_abi_info[1]' [-Warray-bounds]
> >    86 |  unsigned long vdso_size = vdso_info[abi].vdso_code_end -
> >       |                            ~~~~~~~~~^~~~~
> >
> > This is all in dead code however that the compiler is unable to
> > eliminate by itself.
> >
> > Change the array to individual local variables that can be
> > dropped in dead code elimination to let the compiler understand
> > this better.
> >
> > Fixes: 0cbc2659123e ("arm64: vdso32: Remove a bunch of #ifdef CONFIG_COMPAT_VDSO guards")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> This looks like a nice cleanup to me! I agree we don't need the array
> here.
>
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Thanks!

I see the patch now conflicts with "mm: forbid splitting special mappings"
in -mm, by Dmitry Safonov. I have rebased my patch on top, should
I send it to Andrew for inclusion in -mm then?

      Arnd

Re: [PATCH 2/4] arm64: hide more compat_vdso code

[Dmitry Safonov]

On 10/29/20 1:35 PM, Arnd Bergmann wrote:
> On Mon, Oct 26, 2020 at 5:55 PM Mark Rutland <mark.rutland@arm.com> wrote:
>> On Mon, Oct 26, 2020 at 05:03:29PM +0100, Arnd Bergmann wrote:
>>> From: Arnd Bergmann <arnd@arndb.de>
>>>
>>> When CONFIG_COMPAT_VDSO is disabled, we get a warning
>>> about a potential out-of-bounds access:
>>>
>>> arch/arm64/kernel/vdso.c: In function 'aarch32_vdso_mremap':
>>> arch/arm64/kernel/vdso.c:86:37: warning: array subscript 1 is above array bounds of 'struct vdso_abi_info[1]' [-Warray-bounds]
>>>    86 |  unsigned long vdso_size = vdso_info[abi].vdso_code_end -
>>>       |                            ~~~~~~~~~^~~~~
>>>
>>> This is all in dead code however that the compiler is unable to
>>> eliminate by itself.
>>>
>>> Change the array to individual local variables that can be
>>> dropped in dead code elimination to let the compiler understand
>>> this better.
>>>
>>> Fixes: 0cbc2659123e ("arm64: vdso32: Remove a bunch of #ifdef CONFIG_COMPAT_VDSO guards")
>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>
>> This looks like a nice cleanup to me! I agree we don't need the array
>> here.
>>
>> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> 
> Thanks!
> 
> I see the patch now conflicts with "mm: forbid splitting special mappings"
> in -mm, by Dmitry Safonov. I have rebased my patch on top, should
> I send it to Andrew for inclusion in -mm then?

Makes sense to me.
I plan to add some more patches on top that will make tracking of user
landing (on vdso/sigpage/etc) common between architectures in generic code.
So, I think it's probably good idea to keep it in one place, -mm tree
seems like a proper place for it.

Thanks,
          Dmitry