[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <c4e1b5c8-4830-4835-86b5-22171450f1b2@arm.com>
Date: Mon, 28 Oct 2024 13:29:06 +0000
From: Vincenzo Frascino <vincenzo.frascino@....com>
To: Arnd Bergmann <arnd@...nel.org>, Andy Lutomirski <luto@...nel.org>,
Thomas Gleixner <tglx@...utronix.de>
Cc: Arnd Bergmann <arnd@...db.de>, Naresh Kamboju
<naresh.kamboju@...aro.org>, Anders Roxell <anders.roxell@...aro.org>,
Alex Bennee <alex.bennee@...aro.org>,
Geert Uytterhoeven <geert@...ux-m68k.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] vdso: change PAGE_MASK to signed on all 32-bit
architectures
On 24/10/2024 14:34, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@...db.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@...aro.org>
> Tested-by: Anders Roxell <anders.roxell@...aro.org>
> Cc: Alex Bennee <alex.bennee@...aro.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@....com>
Tested-by: Vincenzo Frascino <vincenzo.frascino@....com>
> Signed-off-by: Arnd Bergmann <arnd@...db.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
Powered by blists - more mailing lists