Re: [PATCH 2/3] ARM: i.MX8M: return error if imx_load_image can't honour entry address

[Marco Felsch <m.felsch@pengutronix.de>]

Hi Ahmad,

On 24-01-15, Ahmad Fatoum wrote:
> On i.MX6 and 7, we call imx_load_image with start == true and address
> equal to entry.
> 
> On i.MX8M, we call imx_load_image with start == false and address
> unequal to entry.
> 
> imx_load_image interprets the address being unequal to entry as a
> directive to move the image, so that the barebox entry point without
> the i.MX header starts at exactly the entry address.
> 
> If we were to change this, say on an i.MX8MN, so address is equal
> to entry, the system will no longer boot:
> 
>   - i.MX header is loaded to offset 0 from start of SDRAM (like on i.MX6/7)
>   - barebox PBL entry point is loaded to offset 32K
>   - imx8mX_load_bl33 will copy the running barebox PBL to offset 0
> 
> The result of this is that the TF-A will be able to return to barebox
> (non-executable i.MX header overwritten with executable PBL), but
> uncompressing barebox will fail (remainder of partially overwritten
> PBL is interpreted as start of piggydata).

thanks for taking care of this.

> Add some documentation explaining this complexity, a hint on how we
> could improve it and change the condition, so entry == address results
> in an explicit warning message immediately.
> 
> Reported-by: Marco Felsch <m.felsch@pengutronix.de>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  arch/arm/mach-imx/xload-common.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-imx/xload-common.c b/arch/arm/mach-imx/xload-common.c
> index 03eb2ef109e3..32f12cd7f574 100644
> --- a/arch/arm/mach-imx/xload-common.c
> +++ b/arch/arm/mach-imx/xload-common.c
> @@ -78,6 +78,25 @@ imx_search_header(struct imx_flash_header_v2 **header_pointer,
>  	return 0;
>  }
>  
> +/**
> + * imx_load_image - Load i.MX barebox image from boot medium
> + * @address: Start address of SDRAM where barebox can be loaded into
> + * @entry: Address where barebox entry point should be placed.
> + *         This is ignored unless @start == false
> + * @offset: Start offset for i.MX header search
> + * @ivt_offset: offset between i.MX header and IVT
> + * @start: whether image should be started after loading
> + * @alignment: If nonzero, image size hardcoded in PBL will be aligned up
> + *             to this value
> + * @read: function pointer for reading from the beginning of the boot
> + *        medium onwards
> + * @priv: private data pointer passed to read function
> + *
> + * Return: A negative error code on failure.
> + * On success, if @start == true, the function will not return.
> + * If @start == false, the function will return 0 after placing the
> + * barebox entry point (without header) at @entry.
> + */
>  int imx_load_image(ptrdiff_t address, ptrdiff_t entry, u32 offset,
>  		   u32 ivt_offset, bool start, unsigned int alignment,
>  		   int (*read)(void *dest, size_t len, void *priv),
> @@ -102,7 +121,7 @@ int imx_load_image(ptrdiff_t address, ptrdiff_t entry, u32 offset,
>  
>  	ofs = offset + hdr->entry - hdr->boot_data.start;
>  
> -	if (entry != address) {
> +	if (!start) {
>  		/*
>  		 * Passing entry different from address is interpreted
>  		 * as a request to place the image such that its entry

Can you please adapt the comment here slightly to take the new (!start)
into account?

Regards,
  Marco


> @@ -129,6 +148,17 @@ int imx_load_image(ptrdiff_t address, ptrdiff_t entry, u32 offset,
>  		buf = (void *)(entry - ofs);
>  	}
>  
> +	/*
> +	 * For SD/MMC High-Capacity support (> 2G), the offset for the block
> +	 * read command is in blocks, not bytes. We don't have the information
> +	 * whether we have a SDHC card or not, when we run here though, because
> +	 * card setup was done by BootROM. To workaround this, we just read
> +	 * from offset 0 as 0 blocks == 0 bytes.
> +	 *
> +	 * A result of this is that we will have to read the i.MX header and
> +	 * padding in front of the binary first to arrive at the barebox entry
> +	 * point.
> +	 */
>  	ret = read(buf, ofs + len, priv);
>  	if (ret) {
>  		pr_err("Loading image failed with %d\n", ret);
> -- 
> 2.39.2
> 
>