Re: [PATCH] powerpc: Split PAGE_SHIFT/SIZE into vdso/page.h

[Michael Ellerman <mpe@ellerman.id.au>]

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