Re: [PATCH] vdso: change PAGE_MASK to signed on all 32-bit architectures

[Vincenzo Frascino <vincenzo.frascino@arm.com>]

On 24/10/2024 14:34, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> With the introduction of an architecture-independent defintion of
> PAGE_MASK, we had to make a choice between defining it as 'unsigned
> long' as on 64-bit architectures, or as signed 'long' as required for
> architectures with a 64-bit phys_addr_t.
> 
> To reduce the risk for regressions and minimize the changes in behavior,
> the result was using the signed value only when CONFIG_PHYS_ADDR_T_64BIT
> is set, but that ended up causing a regression after all in the
> early_init_dt_add_memory_arch() function that uses 64-bit integers for
> address calculation.
> 
> Presumably the same regression also affects mips32 and powerpc32 when
> dealing with large amounts of memory on DT platforms: like arm32, they
> were using the signed version unconditionally.
> 
> The two most sensible options that I see for addressing the regiression
> are either to go back to an architecture specific definition, using a
> signed constant on arm/powerpc/mips and unsigned on the others, or to
> use the same definition everywhere.
> 
> Use the simpler of those two and change them all to the signed version,
> in the hope that this does not cause a different type of bug. Most
> of the other 32-bit architectures have no large physical address
> support and are rarely used, so it seems more likely that using the
> same definition helps than hurts here.
> 
> In particular, x86-32 does have physical addressing extensions, so it
> already changed to the signed version after the previous patch,
> so it makes sense to use the same version on non-PAE as well.
> 
> Fixes: efe8419ae78d ("vdso: Introduce vdso/page.h")
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Tested-by: Anders Roxell <anders.roxell@linaro.org>
> Cc: Alex Bennee <alex.bennee@linaro.org>,
> Link: https://lore.kernel.org/lkml/CA+G9fYt86bUAu_v5dXPWnDUwQNVipj+Wq3Djir1KUSKdr9QLNg@mail.gmail.com/

Thanks Arnd, I tested it this morning and seems working correctly with qemu 9.1.1.

With this:

Reviewed-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
Tested-by: Vincenzo Frascino <vincenzo.frascino@arm.com>

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  include/vdso/page.h | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/include/vdso/page.h b/include/vdso/page.h
> index 4ada1ba6bd1f..710ae2414e68 100644
> --- a/include/vdso/page.h
> +++ b/include/vdso/page.h
> @@ -14,13 +14,14 @@
>  
>  #define PAGE_SIZE	(_AC(1,UL) << CONFIG_PAGE_SHIFT)
>  
> -#if defined(CONFIG_PHYS_ADDR_T_64BIT) && !defined(CONFIG_64BIT)
> +#if !defined(CONFIG_64BIT)
>  /*
> - * Applies only to 32-bit architectures with a 64-bit phys_addr_t.
> + * Applies only to 32-bit architectures.
>   *
>   * Subtle: (1 << CONFIG_PAGE_SHIFT) is an int, not an unsigned long.
>   * So if we assign PAGE_MASK to a larger type it gets extended the
> - * way we want (i.e. with 1s in the high bits)
> + * way we want (i.e. with 1s in the high bits) while masking a
> + * 64-bit value such as phys_addr_t.
>   */
>  #define PAGE_MASK	(~((1 << CONFIG_PAGE_SHIFT) - 1))
>  #else

-- 
Regards,
Vincenzo