Re: [PATCH v1 4/9] powerpc/vdso: Remove unnecessary ifdefs in vdso_pagelist initialization
Le 28/08/2020 à 07:40, Christophe Leroy a écrit :
Le 27/08/2020 à 15:19, Michael Ellerman a écrit :
Christophe Leroy writes:
On 08/26/2020 02:58 PM, Michael Ellerman wrote:
Christophe Leroy writes:
diff --git a/arch/powerpc/kernel/vdso.c b/arch/powerpc/kernel/vdso.c
index daef14a284a3..bbb69832fd46 100644
--- a/arch/powerpc/kernel/vdso.c
+++ b/arch/powerpc/kernel/vdso.c
@@ -718,16 +710,14 @@ static int __init vdso_init(void)
...
-
-#ifdef CONFIG_VDSO32
vdso32_kbase = _start;
/*
@@ -735,8 +725,6 @@ static int __init vdso_init(void)
*/
vdso32_pages = (_end - _start) >> PAGE_SHIFT;
DBG("vdso32_kbase: %p, 0x%x pages\n", vdso32_kbase,
vdso32_pages);
-#endif
This didn't build for ppc64le:
/opt/cross/gcc-8.20_binutils-2.32/powerpc64-unknown-linux-gnu/bin/powerpc64-unknown-linux-gnu-ld:
arch/powerpc/kernel/vdso.o:(.toc+0x0): undefined reference to
`vdso32_end'
/opt/cross/gcc-8.20_binutils-2.32/powerpc64-unknown-linux-gnu/bin/powerpc64-unknown-linux-gnu-ld:
arch/powerpc/kernel/vdso.o:(.toc+0x8): undefined reference to
`vdso32_start'
make[1]: *** [/scratch/michael/build/maint/Makefile:1166:
vmlinux] Error 1
make: *** [Makefile:185: __sub-make] Error 2
So I just put that ifdef back.
The problem is because is_32bit() can still return true even when
CONFIG_VDSO32 is not set.
Hmm, you're right. My config had CONFIG_COMPAT enabled.
But that seems like a bug, if someone enables COMPAT on ppc64le they are
almost certainly going to want VDSO32 as well.
So I think I'll do a lead up patch as below.
Ah yes, and with that then no need to consider the case where
is_32bit_task() is true and CONFIG_VDSO32 is not selected.
I'll update my leading series accordingly.
I meant follow up series.
Christophe
Christophe
cheers
diff --git a/arch/powerpc/platforms/Kconfig.cputype
b/arch/powerpc/platforms/Kconfig.cputype
index d4fd109f177e..cf2da1e401ef 100644
--- a/arch/powerpc/platforms/Kconfig.cputype
+++ b/arch/powerpc/platforms/Kconfig.cputype
@@ -501,13 +501,12 @@ endmenu
config VDSO32
def_bool y
- depends on PPC32 || CPU_BIG_ENDIAN
+ depends on PPC32 || COMPAT
help
This symbol controls whether we build the 32-bit VDSO. We
obviously
want to do that if we're building a 32-bit kernel. If we're
building
- a 64-bit kernel then we only want a 32-bit VDSO if we're
building for
- big endian. That is because the only little endian
configuration we
- support is ppc64le which is 64-bit only.
+ a 64-bit kernel then we only want a 32-bit VDSO if we're also
enabling
+ COMPAT.
choice
prompt "Endianness selection"
Re: [PATCH v1 4/9] powerpc/vdso: Remove unnecessary ifdefs in vdso_pagelist initialization
Le 27/08/2020 à 15:19, Michael Ellerman a écrit :
Christophe Leroy writes:
On 08/26/2020 02:58 PM, Michael Ellerman wrote:
Christophe Leroy writes:
diff --git a/arch/powerpc/kernel/vdso.c b/arch/powerpc/kernel/vdso.c
index daef14a284a3..bbb69832fd46 100644
--- a/arch/powerpc/kernel/vdso.c
+++ b/arch/powerpc/kernel/vdso.c
@@ -718,16 +710,14 @@ static int __init vdso_init(void)
...
-
-#ifdef CONFIG_VDSO32
vdso32_kbase = _start;
/*
@@ -735,8 +725,6 @@ static int __init vdso_init(void)
*/
vdso32_pages = (_end - _start) >> PAGE_SHIFT;
DBG("vdso32_kbase: %p, 0x%x pages\n", vdso32_kbase, vdso32_pages);
-#endif
This didn't build for ppc64le:
/opt/cross/gcc-8.20_binutils-2.32/powerpc64-unknown-linux-gnu/bin/powerpc64-unknown-linux-gnu-ld:
arch/powerpc/kernel/vdso.o:(.toc+0x0): undefined reference to `vdso32_end'
/opt/cross/gcc-8.20_binutils-2.32/powerpc64-unknown-linux-gnu/bin/powerpc64-unknown-linux-gnu-ld:
arch/powerpc/kernel/vdso.o:(.toc+0x8): undefined reference to `vdso32_start'
make[1]: *** [/scratch/michael/build/maint/Makefile:1166: vmlinux] Error 1
make: *** [Makefile:185: __sub-make] Error 2
So I just put that ifdef back.
The problem is because is_32bit() can still return true even when
CONFIG_VDSO32 is not set.
Hmm, you're right. My config had CONFIG_COMPAT enabled.
But that seems like a bug, if someone enables COMPAT on ppc64le they are
almost certainly going to want VDSO32 as well.
So I think I'll do a lead up patch as below.
Ah yes, and with that then no need to consider the case where
is_32bit_task() is true and CONFIG_VDSO32 is not selected.
I'll update my leading series accordingly.
Christophe
cheers
diff --git a/arch/powerpc/platforms/Kconfig.cputype
b/arch/powerpc/platforms/Kconfig.cputype
index d4fd109f177e..cf2da1e401ef 100644
--- a/arch/powerpc/platforms/Kconfig.cputype
+++ b/arch/powerpc/platforms/Kconfig.cputype
@@ -501,13 +501,12 @@ endmenu
config VDSO32
def_bool y
- depends on PPC32 || CPU_BIG_ENDIAN
+ depends on PPC32 || COMPAT
help
This symbol controls whether we build the 32-bit VDSO. We obviously
want to do that if we're building a 32-bit kernel. If we're building
- a 64-bit kernel then we only want a 32-bit VDSO if we're building for
- big endian. That is because the only little endian configuration we
- support is ppc64le which is 64-bit only.
+ a 64-bit kernel then we only want a 32-bit VDSO if we're also enabling
+ COMPAT.
choice
prompt "Endianness selection"
Re: [PATCH v1 4/9] powerpc/vdso: Remove unnecessary ifdefs in vdso_pagelist initialization
Christophe Leroy writes:
> On 08/26/2020 02:58 PM, Michael Ellerman wrote:
>> Christophe Leroy writes:
>>> diff --git a/arch/powerpc/kernel/vdso.c b/arch/powerpc/kernel/vdso.c
>>> index daef14a284a3..bbb69832fd46 100644
>>> --- a/arch/powerpc/kernel/vdso.c
>>> +++ b/arch/powerpc/kernel/vdso.c
>>> @@ -718,16 +710,14 @@ static int __init vdso_init(void)
>> ...
>>>
>>> -
>>> -#ifdef CONFIG_VDSO32
>>> vdso32_kbase = _start;
>>>
>>> /*
>>> @@ -735,8 +725,6 @@ static int __init vdso_init(void)
>>> */
>>> vdso32_pages = (_end - _start) >> PAGE_SHIFT;
>>> DBG("vdso32_kbase: %p, 0x%x pages\n", vdso32_kbase, vdso32_pages);
>>> -#endif
>>
>> This didn't build for ppc64le:
>>
>>
>> /opt/cross/gcc-8.20_binutils-2.32/powerpc64-unknown-linux-gnu/bin/powerpc64-unknown-linux-gnu-ld:
>> arch/powerpc/kernel/vdso.o:(.toc+0x0): undefined reference to `vdso32_end'
>>
>> /opt/cross/gcc-8.20_binutils-2.32/powerpc64-unknown-linux-gnu/bin/powerpc64-unknown-linux-gnu-ld:
>> arch/powerpc/kernel/vdso.o:(.toc+0x8): undefined reference to `vdso32_start'
>>make[1]: *** [/scratch/michael/build/maint/Makefile:1166: vmlinux] Error 1
>>make: *** [Makefile:185: __sub-make] Error 2
>>
>> So I just put that ifdef back.
>>
>
> The problem is because is_32bit() can still return true even when
> CONFIG_VDSO32 is not set.
Hmm, you're right. My config had CONFIG_COMPAT enabled.
But that seems like a bug, if someone enables COMPAT on ppc64le they are
almost certainly going to want VDSO32 as well.
So I think I'll do a lead up patch as below.
cheers
diff --git a/arch/powerpc/platforms/Kconfig.cputype
b/arch/powerpc/platforms/Kconfig.cputype
index d4fd109f177e..cf2da1e401ef 100644
--- a/arch/powerpc/platforms/Kconfig.cputype
+++ b/arch/powerpc/platforms/Kconfig.cputype
@@ -501,13 +501,12 @@ endmenu
config VDSO32
def_bool y
- depends on PPC32 || CPU_BIG_ENDIAN
+ depends on PPC32 || COMPAT
help
This symbol controls whether we build the 32-bit VDSO. We obviously
want to do that if we're building a 32-bit kernel. If we're building
- a 64-bit kernel then we only want a 32-bit VDSO if we're building for
- big endian. That is because the only little endian configuration we
- support is ppc64le which is 64-bit only.
+ a 64-bit kernel then we only want a 32-bit VDSO if we're also enabling
+ COMPAT.
choice
prompt "Endianness selection"
Re: [PATCH v1 4/9] powerpc/vdso: Remove unnecessary ifdefs in vdso_pagelist initialization
On 08/26/2020 02:58 PM, Michael Ellerman wrote:
Christophe Leroy writes:
diff --git a/arch/powerpc/kernel/vdso.c b/arch/powerpc/kernel/vdso.c
index daef14a284a3..bbb69832fd46 100644
--- a/arch/powerpc/kernel/vdso.c
+++ b/arch/powerpc/kernel/vdso.c
@@ -718,16 +710,14 @@ static int __init vdso_init(void)
...
-
-#ifdef CONFIG_VDSO32
vdso32_kbase = _start;
/*
@@ -735,8 +725,6 @@ static int __init vdso_init(void)
*/
vdso32_pages = (_end - _start) >> PAGE_SHIFT;
DBG("vdso32_kbase: %p, 0x%x pages\n", vdso32_kbase, vdso32_pages);
-#endif
This didn't build for ppc64le:
/opt/cross/gcc-8.20_binutils-2.32/powerpc64-unknown-linux-gnu/bin/powerpc64-unknown-linux-gnu-ld:
arch/powerpc/kernel/vdso.o:(.toc+0x0): undefined reference to `vdso32_end'
/opt/cross/gcc-8.20_binutils-2.32/powerpc64-unknown-linux-gnu/bin/powerpc64-unknown-linux-gnu-ld:
arch/powerpc/kernel/vdso.o:(.toc+0x8): undefined reference to `vdso32_start'
make[1]: *** [/scratch/michael/build/maint/Makefile:1166: vmlinux] Error 1
make: *** [Makefile:185: __sub-make] Error 2
So I just put that ifdef back.
The problem is because is_32bit() can still return true even when
CONFIG_VDSO32 is not set.
The change below fixes the problem:
diff --git a/arch/powerpc/kernel/vdso.c b/arch/powerpc/kernel/vdso.c
index bbb69832fd46..38abff60cbe2 100644
--- a/arch/powerpc/kernel/vdso.c
+++ b/arch/powerpc/kernel/vdso.c
@@ -132,11 +132,7 @@ int arch_setup_additional_pages(struct linux_binprm
*bprm, int uses_interp)
if (!vdso_ready)
return 0;
- if (is_32bit_task()) {
- vdso_pagelist = vdso32_pagelist;
- vdso_pages = vdso32_pages;
- vdso_base = VDSO32_MBASE;
- } else {
+ if (!is_32bit_task()) {
vdso_pagelist = vdso64_pagelist;
vdso_pages = vdso64_pages;
/*
@@ -145,6 +141,12 @@ int arch_setup_additional_pages(struct linux_binprm
*bprm, int uses_interp)
* and most likely share a SLB entry.
*/
vdso_base = 0;
+ } else if (IS_ENABLED(CONFIG_VDSO32)) {
+ vdso_pagelist = vdso32_pagelist;
+ vdso_pages = vdso32_pages;
+ vdso_base = VDSO32_MBASE;
+ } else {
+ vdso_pages = 0;
}
current->mm->context.vdso_base = 0;
With this change all vdso32 static objects (functions and vars) go away
as expected.
We get a simple conflict with the following patch.
Do you prefer an updated series or a follow up patch, or you take the
above change yourself ?
Christophe
Re: [PATCH v1 4/9] powerpc/vdso: Remove unnecessary ifdefs in vdso_pagelist initialization
Le 26/08/2020 à 16:58, Michael Ellerman a écrit :
Christophe Leroy writes:
diff --git a/arch/powerpc/kernel/vdso.c b/arch/powerpc/kernel/vdso.c
index daef14a284a3..bbb69832fd46 100644
--- a/arch/powerpc/kernel/vdso.c
+++ b/arch/powerpc/kernel/vdso.c
@@ -718,16 +710,14 @@ static int __init vdso_init(void)
...
-
-#ifdef CONFIG_VDSO32
vdso32_kbase = _start;
/*
@@ -735,8 +725,6 @@ static int __init vdso_init(void)
*/
vdso32_pages = (_end - _start) >> PAGE_SHIFT;
DBG("vdso32_kbase: %p, 0x%x pages\n", vdso32_kbase, vdso32_pages);
-#endif
This didn't build for ppc64le:
/opt/cross/gcc-8.20_binutils-2.32/powerpc64-unknown-linux-gnu/bin/powerpc64-unknown-linux-gnu-ld:
arch/powerpc/kernel/vdso.o:(.toc+0x0): undefined reference to `vdso32_end'
/opt/cross/gcc-8.20_binutils-2.32/powerpc64-unknown-linux-gnu/bin/powerpc64-unknown-linux-gnu-ld:
arch/powerpc/kernel/vdso.o:(.toc+0x8): undefined reference to `vdso32_start'
make[1]: *** [/scratch/michael/build/maint/Makefile:1166: vmlinux] Error 1
make: *** [Makefile:185: __sub-make] Error 2
So I just put that ifdef back.
Argh. I guess that's the DBG() that hurts. I'll think about it.
Christophe
Re: [PATCH v1 4/9] powerpc/vdso: Remove unnecessary ifdefs in vdso_pagelist initialization
Christophe Leroy writes:
> diff --git a/arch/powerpc/kernel/vdso.c b/arch/powerpc/kernel/vdso.c
> index daef14a284a3..bbb69832fd46 100644
> --- a/arch/powerpc/kernel/vdso.c
> +++ b/arch/powerpc/kernel/vdso.c
> @@ -718,16 +710,14 @@ static int __init vdso_init(void)
...
>
> -
> -#ifdef CONFIG_VDSO32
> vdso32_kbase = _start;
>
> /*
> @@ -735,8 +725,6 @@ static int __init vdso_init(void)
>*/
> vdso32_pages = (_end - _start) >> PAGE_SHIFT;
> DBG("vdso32_kbase: %p, 0x%x pages\n", vdso32_kbase, vdso32_pages);
> -#endif
This didn't build for ppc64le:
/opt/cross/gcc-8.20_binutils-2.32/powerpc64-unknown-linux-gnu/bin/powerpc64-unknown-linux-gnu-ld:
arch/powerpc/kernel/vdso.o:(.toc+0x0): undefined reference to `vdso32_end'
/opt/cross/gcc-8.20_binutils-2.32/powerpc64-unknown-linux-gnu/bin/powerpc64-unknown-linux-gnu-ld:
arch/powerpc/kernel/vdso.o:(.toc+0x8): undefined reference to `vdso32_start'
make[1]: *** [/scratch/michael/build/maint/Makefile:1166: vmlinux] Error 1
make: *** [Makefile:185: __sub-make] Error 2
So I just put that ifdef back.
cheers
[PATCH v1 4/9] powerpc/vdso: Remove unnecessary ifdefs in vdso_pagelist initialization
No need of all those #ifdefs around the pagelist initialisation,
use IS_ENABLED(), GCC will kick out unused static variables.
Signed-off-by: Christophe Leroy
---
arch/powerpc/kernel/vdso.c | 57 +++---
1 file changed, 22 insertions(+), 35 deletions(-)
diff --git a/arch/powerpc/kernel/vdso.c b/arch/powerpc/kernel/vdso.c
index daef14a284a3..bbb69832fd46 100644
--- a/arch/powerpc/kernel/vdso.c
+++ b/arch/powerpc/kernel/vdso.c
@@ -51,15 +51,13 @@ static struct page **vdso32_pagelist;
unsigned long vdso32_sigtramp;
unsigned long vdso32_rt_sigtramp;
-#ifdef CONFIG_VDSO32
extern char vdso32_start, vdso32_end;
-#endif
-#ifdef CONFIG_PPC64
extern char vdso64_start, vdso64_end;
static void *vdso64_kbase = _start;
static unsigned int vdso64_pages;
static struct page **vdso64_pagelist;
+#ifdef CONFIG_PPC64
unsigned long vdso64_rt_sigtramp;
#endif /* CONFIG_PPC64 */
@@ -134,7 +132,6 @@ int arch_setup_additional_pages(struct linux_binprm *bprm,
int uses_interp)
if (!vdso_ready)
return 0;
-#ifdef CONFIG_PPC64
if (is_32bit_task()) {
vdso_pagelist = vdso32_pagelist;
vdso_pages = vdso32_pages;
@@ -149,11 +146,6 @@ int arch_setup_additional_pages(struct linux_binprm *bprm,
int uses_interp)
*/
vdso_base = 0;
}
-#else
- vdso_pagelist = vdso32_pagelist;
- vdso_pages = vdso32_pages;
- vdso_base = VDSO32_MBASE;
-#endif
current->mm->context.vdso_base = 0;
@@ -718,16 +710,14 @@ static int __init vdso_init(void)
vdso_data->icache_block_size = ppc64_caches.l1i.block_size;
vdso_data->dcache_log_block_size = ppc64_caches.l1d.log_block_size;
vdso_data->icache_log_block_size = ppc64_caches.l1i.log_block_size;
+#endif /* CONFIG_PPC64 */
/*
* Calculate the size of the 64 bits vDSO
*/
vdso64_pages = (_end - _start) >> PAGE_SHIFT;
DBG("vdso64_kbase: %p, 0x%x pages\n", vdso64_kbase, vdso64_pages);
-#endif /* CONFIG_PPC64 */
-
-#ifdef CONFIG_VDSO32
vdso32_kbase = _start;
/*
@@ -735,8 +725,6 @@ static int __init vdso_init(void)
*/
vdso32_pages = (_end - _start) >> PAGE_SHIFT;
DBG("vdso32_kbase: %p, 0x%x pages\n", vdso32_kbase, vdso32_pages);
-#endif
-
/*
* Setup the syscall map in the vDOS
@@ -750,30 +738,30 @@ static int __init vdso_init(void)
if (vdso_setup())
goto setup_failed;
-#ifdef CONFIG_VDSO32
- /* Make sure pages are in the correct state */
- vdso32_pagelist = kcalloc(vdso32_pages + 1, sizeof(struct page *),
- GFP_KERNEL);
- if (!vdso32_pagelist)
- goto alloc_failed;
+ if (IS_ENABLED(CONFIG_VDSO32)) {
+ /* Make sure pages are in the correct state */
+ vdso32_pagelist = kcalloc(vdso32_pages + 1, sizeof(struct page
*),
+ GFP_KERNEL);
+ if (!vdso32_pagelist)
+ goto alloc_failed;
- for (i = 0; i < vdso32_pages; i++)
- vdso32_pagelist[i] = virt_to_page(vdso32_kbase + i * PAGE_SIZE);
+ for (i = 0; i < vdso32_pages; i++)
+ vdso32_pagelist[i] = virt_to_page(vdso32_kbase + i *
PAGE_SIZE);
- vdso32_pagelist[i] = virt_to_page(vdso_data);
-#endif
+ vdso32_pagelist[i] = virt_to_page(vdso_data);
+ }
-#ifdef CONFIG_PPC64
- vdso64_pagelist = kcalloc(vdso64_pages + 1, sizeof(struct page *),
- GFP_KERNEL);
- if (!vdso64_pagelist)
- goto alloc_failed;
+ if (IS_ENABLED(CONFIG_PPC64)) {
+ vdso64_pagelist = kcalloc(vdso64_pages + 1, sizeof(struct page
*),
+ GFP_KERNEL);
+ if (!vdso64_pagelist)
+ goto alloc_failed;
- for (i = 0; i < vdso64_pages; i++)
- vdso64_pagelist[i] = virt_to_page(vdso64_kbase + i * PAGE_SIZE);
+ for (i = 0; i < vdso64_pages; i++)
+ vdso64_pagelist[i] = virt_to_page(vdso64_kbase + i *
PAGE_SIZE);
- vdso64_pagelist[i] = virt_to_page(vdso_data);
-#endif /* CONFIG_PPC64 */
+ vdso64_pagelist[i] = virt_to_page(vdso_data);
+ }
smp_wmb();
vdso_ready = 1;
@@ -784,9 +772,8 @@ static int __init vdso_init(void)
pr_err("vDSO setup failure, not enabled !\n");
alloc_failed:
vdso32_pages = 0;
-#ifdef CONFIG_PPC64
vdso64_pages = 0;
-#endif
+
return 0;
}
arch_initcall(vdso_init);
--
2.25.0
