[PATCHv2] firmware: zynqmp: add handling of the NULL return payload pointer in xilinx_pm_request.

[Michal Simek]

First of all subject is quite long. Please make it shorter and remove 
dot at the end.


On 10/13/21 18:21, Adrian Fiergolski wrote:
> When a caller is not interested in the returned message, the ret_payload pointer is set to NULL in the u-boot-sources.
> In this case, under EL3, the memory from address 0x0 would be overwritten by xilinx_pm_request with the returned IPI message,
> damaging the original data under this address. The patch, in case ret_payload is NULL, assigns the pointer to the array
> holding the IPI message being sent.

There is any limit around 80 chars on the line that's why please use it.

> 
> Signed-off-by: Adrian Fiergolski <adrian.fiergolski at fastree3d.com>
> ---
>   drivers/firmware/firmware-zynqmp.c | 20 ++++++++++++++------
>   1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/firmware/firmware-zynqmp.c b/drivers/firmware/firmware-zynqmp.c
> index d4dc856baf..7517a84f0e 100644
> --- a/drivers/firmware/firmware-zynqmp.c
> +++ b/drivers/firmware/firmware-zynqmp.c
> @@ -154,16 +154,24 @@ U_BOOT_DRIVER(zynqmp_power) = {
>   int __maybe_unused xilinx_pm_request(u32 api_id, u32 arg0, u32 arg1, u32 arg2,
>   				     u32 arg3, u32 *ret_payload)
>   {
> +#if defined(CONFIG_ZYNQMP_IPI)
> +	/*
> +	 * Use fixed payload and arg size as the EL2 call. The firmware
> +	 * is capable to handle PMUFW_PAYLOAD_ARG_CNT bytes but the
> +	 * firmware API is limited by the SMC call size
> +	 */
> +	u32 regs[] = {api_id, arg0, arg1, arg2, arg3};
> +
> +	/*
> +	 * Use regs array in case ret_payload is NULL
> +	 */
> +	if (ret_payload == NULL)
> +		ret_payload = regs;
> +#endif
>   	debug("%s at EL%d, API ID: 0x%0x\n", __func__, current_el(), api_id);
>   
>   	if (IS_ENABLED(CONFIG_SPL_BUILD) || current_el() == 3) {
>   #if defined(CONFIG_ZYNQMP_IPI)
> -		/*
> -		 * Use fixed payload and arg size as the EL2 call. The firmware
> -		 * is capable to handle PMUFW_PAYLOAD_ARG_CNT bytes but the
> -		 * firmware API is limited by the SMC call size
> -		 */
> -		u32 regs[] = {api_id, arg0, arg1, arg2, arg3};
>   

Based on your description the only code affected by this is here and 
there is no reason to move it out of there.
It should be enough just to define ret_payload here as:

	/* Use regs array in case ret_payload is NULL */
	if (!ret_payload)
		ret_payload = regs;


Or is there any other reason why you have moved code from here?
In else part regs are defined as pt_regs that's why that regs won't be 
used anyway. And reg_payload is checked already.

Also using regs should be fine because ret_payload[0] is return value 
anyway.

Thanks,
Michal