powerpc: Split PAGE_SHIFT/SIZE into vdso/page.h

The VDSO needs PAGE_SHIFT/SIZE defined, so it includes asm/page.h.

However when COMPAT=y the VDSO is built 32-bit, even though the kernel
is 64-bit. That can lead to odd warnings because some kernel constants
are 64-bit, but unsigned long is 32-bit, for example:

    VDSO32C arch/powerpc/kernel/vdso/vgettimeofday-32.o
  In file included from <built-in>:4:
  In file included from /home/michael/linux/lib/vdso/gettimeofday.c:5:
  In file included from ../include/vdso/datapage.h:137:
  In file included from ../arch/powerpc/include/asm/vdso/gettimeofday.h:7:
  ../arch/powerpc/include/asm/page.h:230:9: warning: result of comparison of constant 13835058055282163712 with expression of type 'unsigned long' is always true
    230 |         return __pa(kaddr) >> PAGE_SHIFT;
        |                ^~~~~~~~~~~

Fix it by moving the PAGE_SHIFT/SIZE constants into a separate header,
which can be included by the VDSO, and also by the existing kernel
headers. That avoids exposing the rest of the header to the non-standard
build environment of the compat VDSO.

The particular warning above was introduced by commit 58b6fed89ab0
("powerpc: Make virt_to_pfn() a static inline"), though it is not at
fault, it just exposed the fact that the VDSO was including parts of
page.h that weren't needed or appropriate for the VDSO.

Don't copy the comment about page sizes, it just risks becoming
outdated, that information is better available in the Kconfig
dependencies and help text.

Fixes: 58b6fed89ab0 ("powerpc: Make virt_to_pfn() a static inline")
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202311061940.4pBrm44u-lkp@intel.com/
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/include/asm/page.h              | 10 +---------
 arch/powerpc/include/asm/vdso/gettimeofday.h |  2 +-
 arch/powerpc/include/asm/vdso/page.h         | 10 ++++++++++
 arch/powerpc/kernel/vdso/vdso32.lds.S        |  2 +-
 arch/powerpc/kernel/vdso/vdso64.lds.S        |  2 +-
 arch/powerpc/kernel/vdso32_wrapper.S         |  2 +-
 arch/powerpc/kernel/vdso64_wrapper.S         |  2 +-
 7 files changed, 16 insertions(+), 14 deletions(-)
 create mode 100644 arch/powerpc/include/asm/vdso/page.h

On Thu, Dec 21, 2023 at 1:04 PM Michael Ellerman <mpe@ellerman.id.au> wrote:

> The VDSO needs PAGE_SHIFT/SIZE defined, so it includes asm/page.h.
>
> However when COMPAT=y the VDSO is built 32-bit, even though the kernel
> is 64-bit. That can lead to odd warnings because some kernel constants
> are 64-bit, but unsigned long is 32-bit, for example:
>
>     VDSO32C arch/powerpc/kernel/vdso/vgettimeofday-32.o
>   In file included from <built-in>:4:
>   In file included from /home/michael/linux/lib/vdso/gettimeofday.c:5:
>   In file included from ../include/vdso/datapage.h:137:
>   In file included from ../arch/powerpc/include/asm/vdso/gettimeofday.h:7:
>   ../arch/powerpc/include/asm/page.h:230:9: warning: result of comparison of constant 13835058055282163712 with expression of type 'unsigned long' is always true
>     230 |         return __pa(kaddr) >> PAGE_SHIFT;
>         |                ^~~~~~~~~~~
>
> Fix it by moving the PAGE_SHIFT/SIZE constants into a separate header,
> which can be included by the VDSO, and also by the existing kernel
> headers. That avoids exposing the rest of the header to the non-standard
> build environment of the compat VDSO.
>
> The particular warning above was introduced by commit 58b6fed89ab0
> ("powerpc: Make virt_to_pfn() a static inline"), though it is not at
> fault, it just exposed the fact that the VDSO was including parts of
> page.h that weren't needed or appropriate for the VDSO.
>
> Don't copy the comment about page sizes, it just risks becoming
> outdated, that information is better available in the Kconfig
> dependencies and help text.
>
> Fixes: 58b6fed89ab0 ("powerpc: Make virt_to_pfn() a static inline")
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202311061940.4pBrm44u-lkp@intel.com/
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

Clearly a working solution!
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Just a note:

> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
(...)
> +#include <asm/vdso/page.h>

(...)
> +++ b/arch/powerpc/include/asm/vdso/page.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef _ASM_POWERPC_VDSO_PAGE_H
> +#define _ASM_POWERPC_VDSO_PAGE_H
> +
> +#include <vdso/const.h>

Now the whole kernel includes <vdso/const.h>, is this necessary?

Yours,
Linus Walleij

Linus Walleij <linus.walleij@linaro.org> writes:
> On Thu, Dec 21, 2023 at 1:04 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
>> The VDSO needs PAGE_SHIFT/SIZE defined, so it includes asm/page.h.
>>
>> However when COMPAT=y the VDSO is built 32-bit, even though the kernel
>> is 64-bit. That can lead to odd warnings because some kernel constants
>> are 64-bit, but unsigned long is 32-bit, for example:
>>
>>     VDSO32C arch/powerpc/kernel/vdso/vgettimeofday-32.o
>>   In file included from <built-in>:4:
>>   In file included from /home/michael/linux/lib/vdso/gettimeofday.c:5:
>>   In file included from ../include/vdso/datapage.h:137:
>>   In file included from ../arch/powerpc/include/asm/vdso/gettimeofday.h:7:
>>   ../arch/powerpc/include/asm/page.h:230:9: warning: result of comparison of constant 13835058055282163712 with expression of type 'unsigned long' is always true
>>     230 |         return __pa(kaddr) >> PAGE_SHIFT;
>>         |                ^~~~~~~~~~~
>>
>> Fix it by moving the PAGE_SHIFT/SIZE constants into a separate header,
>> which can be included by the VDSO, and also by the existing kernel
>> headers. That avoids exposing the rest of the header to the non-standard
>> build environment of the compat VDSO.
>>
>> The particular warning above was introduced by commit 58b6fed89ab0
>> ("powerpc: Make virt_to_pfn() a static inline"), though it is not at
>> fault, it just exposed the fact that the VDSO was including parts of
>> page.h that weren't needed or appropriate for the VDSO.
>>
>> Don't copy the comment about page sizes, it just risks becoming
>> outdated, that information is better available in the Kconfig
>> dependencies and help text.
>>
>> Fixes: 58b6fed89ab0 ("powerpc: Make virt_to_pfn() a static inline")
>> Reported-by: kernel test robot <lkp@intel.com>
>> Closes: https://lore.kernel.org/oe-kbuild-all/202311061940.4pBrm44u-lkp@intel.com/
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>
> Clearly a working solution!
> Acked-by: Linus Walleij <linus.walleij@linaro.org>

Thanks.

> Just a note:
>
>> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
> (...)
>> +#include <asm/vdso/page.h>
>
> (...)
>> +++ b/arch/powerpc/include/asm/vdso/page.h
>> @@ -0,0 +1,10 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +#ifndef _ASM_POWERPC_VDSO_PAGE_H
>> +#define _ASM_POWERPC_VDSO_PAGE_H
>> +
>> +#include <vdso/const.h>
>
> Now the whole kernel includes <vdso/const.h>, is this necessary?

It's already included by approximately the whole kernel via:
  include/linux/kernel.h
   - include/uapi/linux/kernel.h
     - include/linux/const.h
       - include/vdso/const.h

And arch/powerpc/include/asm/page.h already includes linux/kernel.h, so
includers of page.h should not see any new headers other than
asm/vdso/page.h.

cheers

Michael Ellerman <mpe@ellerman.id.au> writes:
> The VDSO needs PAGE_SHIFT/SIZE defined, so it includes asm/page.h.

For the archives, this was superseeded by Arnd's rework:

  https://lore.kernel.org/all/20240320180228.136371-1-arnd@kernel.org/

cheers