Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] powerpc: Fix kernel-jump address for ppc64 wrapper boot
To:     He Ying <heying24@huawei.com>, mpe@ellerman.id.au,
        benh@kernel.crashing.org, paulus@samba.org, nathan@kernel.org
Cc:     linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
References: <20210604092228.199588-1-heying24@huawei.com>
From:   Christophe Leroy <christophe.leroy@csgroup.eu>
Message-ID: <c80f69d0-44b0-24a6-ce4f-ed5a40514597@csgroup.eu>
Date:   Tue, 8 Jun 2021 06:55:35 +0200
User-Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <20210604092228.199588-1-heying24@huawei.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: fr
Content-Transfer-Encoding: 8bit
Precedence: bulk


Le 04/06/2021 à 11:22, He Ying a écrit :
>  From "64-bit PowerPC ELF Application Binary Interface Supplement 1.9",
> we know that the value of a function pointer in a language like C is
> the address of the function descriptor and the first doubleword
> of the function descriptor contains the address of the entry point
> of the function.
> 
> So, when we want to jump to an address (e.g. addr) to execute for
> PPC-elf64abi, we should assign the address of addr *NOT* addr itself
> to the function pointer or system will jump to the wrong address.
> 
> Link: https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html#FUNC-DES
> Signed-off-by: He Ying <heying24@huawei.com>
> ---
>   arch/powerpc/boot/main.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/arch/powerpc/boot/main.c b/arch/powerpc/boot/main.c
> index cae31a6e8f02..50fd7f11b642 100644
> --- a/arch/powerpc/boot/main.c
> +++ b/arch/powerpc/boot/main.c
> @@ -268,7 +268,16 @@ void start(void)
>   	if (console_ops.close)
>   		console_ops.close();
>   
> +#ifdef CONFIG_PPC64_BOOT_WRAPPER

This kind of need doesn't desserve a #ifdef, see 
https://www.kernel.org/doc/html/latest/process/coding-style.html#conditional-compilation

You can do:


	kentry = (kernel_entry_t)(IS_ENABLED(CONFIG_PPC64_BOOT_WRAPPER) ? &vmlinux.addr : vmlinux.addr);


Or, if you prefer something less compact:


	if (IS_ENABLED(CONFIG_PPC64_BOOT_WRAPPER))
		kentry = (kernel_entry_t) &vmlinux.addr;
	else
		kentry = (kernel_entry_t) vmlinux.addr;


> +	/*
> +	 * For PPC-elf64abi, the value of a function pointer is the address
> +	 * of the function descriptor. And the first doubleword of a function
> +	 * descriptor contains the address of the entry point of the function.
> +	 */
> +	kentry = (kernel_entry_t) &vmlinux.addr;
> +#else
>   	kentry = (kernel_entry_t) vmlinux.addr;
> +#endif
>   	if (ft_addr) {
>   		if(platform_ops.kentry)
>   			platform_ops.kentry(ft_addr, vmlinux.addr);
>