Re: [PATCH v2 4/8] vdso: Introduce vdso/page.h

["Arnd Bergmann" <arnd@arndb.de>]

On Mon, Sep 23, 2024, at 14:19, Vincenzo Frascino wrote:
> The VDSO implementation includes headers from outside of the
> vdso/ namespace.
>
> Introduce vdso/page.h to make sure that the generic library
> uses only the allowed namespace.
>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jason A. Donenfeld <Jason@zx2c4.com>
> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>

Thanks for the new version. This looks all good, just some
very minor ideas for how to possibly improve the new version:

> +/* PAGE_SHIFT determines the page size */
> +#define PAGE_SHIFT      CONFIG_PAGE_SHIFT
> +
> +#define PAGE_SIZE	(_AC(1,UL) << PAGE_SHIFT)
> +
> +#if defined(CONFIG_PHYS_ADDR_T_64BIT) && !defined(CONFIG_X86_64)
> +#define PAGE_MASK	(~((1 << PAGE_SHIFT) - 1))
> +#else
> +#define PAGE_MASK	(~(PAGE_SIZE-1))
> +#endif

I would open-code the CONFIG_PAGE_SHIFT in PAGE_SIZE
and PAGE_MASK, just to avoid the extra indirection in the
preprocessor. This mainly has the benefit of slightly
shorter compiler warnings when all the macros get
traced back but can also slightly improve compile speed
in case this is used in deeply nested macros.

Without a comment, the special case for CONFIG_X86_64
not very clear, and probably not needed. If you are
worried about introducing an architecture specific
regression, I would suggest instead explaining the
possible issue in the patch description but using the
more generic and simpler #ifdef check.

      Arnd