Hi Richard
> NACK. What do you think this does? What it doesn't do is sign-extend from bit 41.
I would like to provide some clarification regarding the patch I submitted for optimizing the virt_to_phys() function. My intention was to rely on the compiler's automatic sign extension by using the long type, rather than manually performing bit shifts. However, I overlooked the fact that on the Alpha architecture, long is a 64-bit type, which resulted in a sign extension from bit 63, not bit 41 as originally intended.
I sincerely apologize for this oversight. In Alpha, the sign extension should begin from bit 41, and I mistakenly assumed that the compiler's automatic sign extension would work as I intended. I now realize the importance of retaining the manual sign extension from bit 41, which is crucial for proper address handling on this platform.
Best regards,
Stephen Eta Zhou
________________________________________
From: Richard Henderson <richard.henderson@xxxxxxxxxx>
Sent: Wednesday, February 12, 2025 12:52 AM
To: Zhou Stephen Eta <stephen.eta.zhou@xxxxxxxxxxx>; mattst88@xxxxxxxxx <mattst88@xxxxxxxxx>; arnd@xxxxxxxx <arnd@xxxxxxxx>; paulmck@xxxxxxxxxx <paulmck@xxxxxxxxxx>; linus.walleij@xxxxxxxxxx <linus.walleij@xxxxxxxxxx>
Cc: linux-alpha@xxxxxxxxxxxxxxx <linux-alpha@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx <linux-kernel@xxxxxxxxxxxxxxx>
Subject: Re: [PATCH] arch/alpha: optimize virt_to_phys with compiler automatic sign extension
On 2/11/25 01:32, Zhou Stephen Eta wrote:
> From 0bf2dd816c8369e2c690869b5f6c671f28c2b196 Mon Sep 17 00:00:00 2001
> From: "stephen.eta.zhou" <stephen.eta.zhou@xxxxxxxxxxx>
> Date: Tue, 11 Feb 2025 16:48:14 +0800
> Subject: [PATCH] arch/alpha: optimize virt_to_phys with compiler automatic
> sign extension
>
> In the `virt_to_phys` function, the following changes have been made:
> 1. **Automatic Sign Extension**:
> - The manual sign extension code has been replaced with the
> compiler's automatic sign extension.
> - This simplifies the code and leverages the
> compiler's optimization.
>
> 2. **Fix for 64-bit Address Overflow**:
> - Previously, when the input was a 64-bit address with a
> negative high bit (sign bit), the sign extension caused an
> overflow, resulting in an incorrect conversion to 0.
> - This issue has been addressed by using the compiler's
> automatic sign extension,
> which ensures proper handling of negative addresses.
>
> 3. **NULL Pointer Check**:
> - A NULL pointer check has been added at the
> beginning of the function.
> - If the address is NULL, the function now
> returns 0 to prevent potential crashes.
>
> Signed-off-by: stephen.eta.zhou <stephen.eta.zhou@xxxxxxxxxxx>
> ---
> arch/alpha/include/asm/io.h | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/arch/alpha/include/asm/io.h b/arch/alpha/include/asm/io.h
> index 65fe1e54c6da..4d7cd7486b7d 100644
> --- a/arch/alpha/include/asm/io.h
> +++ b/arch/alpha/include/asm/io.h
> @@ -70,9 +70,11 @@ static inline unsigned long virt_to_phys(volatile void *address)
> {
> unsigned long phys = (unsigned long)address;
>
> - /* Sign-extend from bit 41. */
> - phys <<= (64 - 41);
> - phys = (long)phys >> (64 - 41);
> + if (!address)
> + return 0;
> +
> + /* Automatic Sign-extend */
> + phys = (long)phys;
NACK. What do you think this does? What it doesn't do is sign-extend from bit 41.
r~
