From: Arnd Bergmann <[email protected]>
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 <[email protected]>
---
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
From: Arnd Bergmann <[email protected]>
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 <[email protected]>
---
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
From: Arnd Bergmann <[email protected]>
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 <[email protected]>
---
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
From: Arnd Bergmann <[email protected]>
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 <[email protected]>
---
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
Hi Arnd,
On Mon, Oct 26, 2020 at 05:03:31PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
>
On Mon, 26 Oct 2020 at 16:23, Mark Rutland <[email protected]> wrote:
>
> Hi Arnd,
>
> On Mon, Oct 26, 2020 at 05:03:31PM +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann <[email protected]>
> >
> > 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
On Mon, Oct 26, 2020 at 05:03:29PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> 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 <[email protected]>
This looks like a nice cleanup to me! I agree we don't need the array
here.
Reviewed-by: Mark Rutland <[email protected]>
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
>
On Mon, Oct 26, 2020 at 05:03:30PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> 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 <[email protected]>
> ---
> 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.
On Mon, Oct 26, 2020 at 6:01 PM Mark Rutland <[email protected]> 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
On Mon, Oct 26, 2020 at 5:55 PM Mark Rutland <[email protected]> wrote:
> On Mon, Oct 26, 2020 at 05:03:29PM +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann <[email protected]>
> >
> > 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 <[email protected]>
>
> This looks like a nice cleanup to me! I agree we don't need the array
> here.
>
> Reviewed-by: Mark Rutland <[email protected]>
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
On 10/29/20 1:35 PM, Arnd Bergmann wrote:
> On Mon, Oct 26, 2020 at 5:55 PM Mark Rutland <[email protected]> wrote:
>> On Mon, Oct 26, 2020 at 05:03:29PM +0100, Arnd Bergmann wrote:
>>> From: Arnd Bergmann <[email protected]>
>>>
>>> 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 <[email protected]>
>>
>> This looks like a nice cleanup to me! I agree we don't need the array
>> here.
>>
>> Reviewed-by: Mark Rutland <[email protected]>
>
> 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