Received-SPF: pass (google.com: domain of linux-kernel+bounces-111976-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <a4d340f0-960f-47bc-b45c-f89053526a22@rivosinc.com>
Date: Fri, 22 Mar 2024 11:57:49 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] riscv: Improve sbi_ecall() code generation by reordering
 arguments
Content-Language: en-US
To: Alexandre Ghiti <alexghiti@rivosinc.com>,
 Paul Walmsley <paul.walmsley@sifive.com>, Palmer Dabbelt
 <palmer@dabbelt.com>, Albert Ou <aou@eecs.berkeley.edu>,
 Samuel Holland <samuel.holland@sifive.com>, linux-riscv@lists.infradead.org,
 linux-kernel@vger.kernel.org
References: <20240322112629.68170-1-alexghiti@rivosinc.com>
From: Atish Patra <atishp@rivosinc.com>
In-Reply-To: <20240322112629.68170-1-alexghiti@rivosinc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 3/22/24 04:26, Alexandre Ghiti wrote:
> The sbi_ecall() function arguments are not in the same order as the
> ecall arguments, so we end up re-ordering the registers before the
> ecall which is useless and costly.
> 
> So simply reorder the arguments in the same way as expected by ecall.
> Instead of reordering directly the arguments of sbi_ecall(), use a proxy
> macro since the current ordering is more natural.
> 
> Before:
> 
> Dump of assembler code for function sbi_ecall:
>     0xffffffff800085e0 <+0>: add sp,sp,-32
>     0xffffffff800085e2 <+2>: sd s0,24(sp)
>     0xffffffff800085e4 <+4>: mv t1,a0
>     0xffffffff800085e6 <+6>: add s0,sp,32
>     0xffffffff800085e8 <+8>: mv t3,a1
>     0xffffffff800085ea <+10>: mv a0,a2
>     0xffffffff800085ec <+12>: mv a1,a3
>     0xffffffff800085ee <+14>: mv a2,a4
>     0xffffffff800085f0 <+16>: mv a3,a5
>     0xffffffff800085f2 <+18>: mv a4,a6
>     0xffffffff800085f4 <+20>: mv a5,a7
>     0xffffffff800085f6 <+22>: mv a6,t3
>     0xffffffff800085f8 <+24>: mv a7,t1
>     0xffffffff800085fa <+26>: ecall
>     0xffffffff800085fe <+30>: ld s0,24(sp)
>     0xffffffff80008600 <+32>: add sp,sp,32
>     0xffffffff80008602 <+34>: ret
> 
> After:
> 
> Dump of assembler code for function __sbi_ecall:
>     0xffffffff8000b6b2 <+0>:	add	sp,sp,-32
>     0xffffffff8000b6b4 <+2>:	sd	s0,24(sp)
>     0xffffffff8000b6b6 <+4>:	add	s0,sp,32
>     0xffffffff8000b6b8 <+6>:	ecall
>     0xffffffff8000b6bc <+10>:	ld	s0,24(sp)
>     0xffffffff8000b6be <+12>:	add	sp,sp,32
>     0xffffffff8000b6c0 <+14>:	ret
> 

Nice!

> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> ---
>   arch/riscv/include/asm/sbi.h | 10 ++++++----
>   arch/riscv/kernel/sbi.c      | 10 +++++-----
>   2 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> index 6e68f8dff76b..9041b009d3b5 100644
> --- a/arch/riscv/include/asm/sbi.h
> +++ b/arch/riscv/include/asm/sbi.h
> @@ -292,10 +292,12 @@ struct sbiret {
>   };
>   
>   void sbi_init(void);
> -struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
> -			unsigned long arg1, unsigned long arg2,
> -			unsigned long arg3, unsigned long arg4,
> -			unsigned long arg5);
> +struct sbiret __sbi_ecall(unsigned long arg0, unsigned long arg1,
> +			  unsigned long arg2, unsigned long arg3,
> +			  unsigned long arg4, unsigned long arg5,
> +			  int fid, int ext);
> +#define sbi_ecall(e, f, a0, a1, a2, a3, a4, a5)	\
> +		__sbi_ecall(a0, a1, a2, a3, a4, a5, f, e)
>   
>   #ifdef CONFIG_RISCV_SBI_V01
>   void sbi_console_putchar(int ch);
> diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> index e66e0999a800..5719fa03c3d1 100644
> --- a/arch/riscv/kernel/sbi.c
> +++ b/arch/riscv/kernel/sbi.c
> @@ -24,10 +24,10 @@ static int (*__sbi_rfence)(int fid, const struct cpumask *cpu_mask,
>   			   unsigned long start, unsigned long size,
>   			   unsigned long arg4, unsigned long arg5) __ro_after_init;
>   
> -struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
> -			unsigned long arg1, unsigned long arg2,
> -			unsigned long arg3, unsigned long arg4,
> -			unsigned long arg5)
> +struct sbiret __sbi_ecall(unsigned long arg0, unsigned long arg1,
> +			  unsigned long arg2, unsigned long arg3,
> +			  unsigned long arg4, unsigned long arg5,
> +			  int fid, int ext)
>   {
>   	struct sbiret ret;
>   
> @@ -48,7 +48,7 @@ struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
>   
>   	return ret;
>   }
> -EXPORT_SYMBOL(sbi_ecall);
> +EXPORT_SYMBOL(__sbi_ecall);
>   
>   int sbi_err_map_linux_errno(int err)
>   {

Reviewed-by: Atish Patra <atishp@rivosinc.com>