Re: [PATCH v3 1/9] spl: Add generic spl_load function

[Xavier Drudis Ferran <xdrudis@tinet.cat>]

Hello. 

Thank you for your work, simplifying and generalizing code, 
and sorry that I hadn't seen this series before.

I'm new to U-Boot so I'm sorry if I waste your time with silly
questions, but I can't seem to understand some details.

1- Does some info->read implementation ever want its buffer aligned to
  ARCH_DMA_MINALIGN ? I thought so, because of some code using aligned
  buffers, but I can't find it documented. Must be too obvious except
  for me ?

2- What constraints do we expect about the buffer returned by 
   spl_get_load_buffer(0, size)  ?  From what I see it would seem 
   to be often just CONFIG_SYS_TEXT_BASE ? 
   Do we know CONFIG_SYS_TEXT_BASE is DMA aligned ? (I think it will be).
   Does it need to be in "the middle" of RAM, with room before it?
      grep -E 'CONFIG_SYS_TEXT_BASE=0(x0+)?\s*$' configs/*
   returns some 35 boards with CONFIG_SYS_TEXT_BASE=0
   Can we assume we can write before the buffer and after buffer+size?

3- do all implementations of info->read expect the size to be in
   ARCH_DMA_ALIGN  units, not a size in bytes 
   when there's info->filename ?

   spl_fat has filename, bl_len=1 but expects size in bytes, 
   not in blocks of length ARCH_DMA_MINALIGN (which could be >1)

   on the other hand (doesn't seem to be touched by this series yet?)
    spl_imx_romapi has no filename but expects size in bytes, 
                   not in bl_len=pagesize units ? 
     
El Thu, May 05, 2022 at 04:16:47PM -0400, Sean Anderson deia:
> 
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index c9750ee163..f9a1cfc71e 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -399,6 +399,74 @@ int spl_parse_image_header(struct spl_image_info *spl_image,
>  	return 0;
>  }
>  
> +static int spl_simple_read(struct spl_load_info *info, void *buf, size_t size,
> +			   size_t offset)
> +{
> +	size_t bl_len = info->filename ? ARCH_DMA_MINALIGN : info->bl_len;

does it work for spl_fat (and spl_imx_romapi if ever needed)? 
and should this or those be fixed ?

> +	size_t bl_mask = bl_len - 1;
> +	size_t overhead = offset & bl_mask;
> +	size_t bl_shift = fls(bl_mask);
> +	int ret;
> +
> +	debug("%s: buf=%p size=%lx offset=%lx\n", __func__, buf, (long)size,
> +	      (long)offset);
> +	debug("%s: bl_len=%lx bl_mask=%lx bl_shift=%lx\n", __func__, bl_len,
> +	      bl_mask, bl_shift);
> +
> +	buf -= overhead;

buf could be 0 ? 
If buf was aligned on entry can it be unaligned now, 
and does it need to be aligned ? 

> +	size = (size + overhead + bl_mask) >> bl_shift;

ditto for spl_fat (and spl_imx_romapi) ? 

> +	offset = offset >> bl_shift;
> +
> +	debug("info->read(info, %lx, %lx, %p)\n", (ulong)offset, (ulong)size,
> +	      buf);
> +	ret = info->read(info, offset, size, buf);

This could read some bytes before the buf pointer that was given to us
and beyond buf+size values received as arguments.

We were given bytes and read multiples of bl_len (or
ARCH_DMA_MINALIGN) bytes, and then there's overhead.

Is this always ok ? 

> +	return ret == size ? 0 : -EIO;
> +}
> +
> +int spl_load(struct spl_image_info *spl_image,
> +	     const struct spl_boot_device *bootdev, struct spl_load_info *info,
> +	     struct image_header *header, size_t size, size_t sector)
> +{
> +	int ret;
> +	size_t offset = sector * info->bl_len;
> +
> +	if (image_get_magic(header) == FDT_MAGIC) {
> +		if (IS_ENABLED(CONFIG_SPL_LOAD_FIT_FULL)) {
> +			void *buf;
> +
> +			/*
> +			 * In order to support verifying images in the FIT, we
> +			 * need to load the whole FIT into memory. Try and
> +			 * guess how much we need to load by using the total
> +			 * size. This will fail for FITs with external data,
> +			 * but there's not much we can do about that.
> +			 */
> +			if (!size)
> +				size = roundup(fdt_totalsize(header), 4);
> +			buf = spl_get_load_buffer(0, size);

maybe 
   extra = max(info->bl_len, ARCH_DMA_MINALIGN) - 1; /* could be more precise, less wasteful */
   buf = spl_get_load_buffer(extra , size + extra) ;

or maybe better

   buf = spl_get_load_buffer(0, size + 2 * extra) + extra ;

or something, to prevent buf being 0 and make sure we have almost 1 buffer
before and one buffer after the image size to cater for images unaligned in media ? 

Also any consideration needed for the (DMA?) alignment of buf ?

> +			ret = spl_simple_read(info, buf, size, offset);
> +			if (ret)
> +				return ret;
> +
> +			return spl_parse_image_header(spl_image, bootdev, buf);
> +		}
> +
> +		if (IS_ENABLED(CONFIG_SPL_LOAD_FIT))
> +			return spl_load_simple_fit(spl_image, info, sector,
> +						   header);
> +	}
> +
> +	if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER))
> +		return spl_load_imx_container(spl_image, info, sector);
> +
> +	ret = spl_parse_image_header(spl_image, bootdev, header);
> +	if (ret)
> +		return ret;
> +
> +	return spl_simple_read(info, (void *)spl_image->load_addr,
> +			       spl_image->size, offset + spl_image->offset);

This looks like maybe it could run into the same problem as spl_load_fit_image

http://patchwork.ozlabs.org/project/uboot/patch/20220609152414.1518919-1-jerome.forissier@linaro.org/

Namely that the extra data in the media blocks before and after the
image can get written outside [load_addr, load_addr+length) and give
trouble (in the case Jerome Forissier found, writes to 0xff3b2XYZ were
overwriting 0xff3b0XYZ on rk3399 because INTMEM1 was only a 8 KiB
SRAM).  That was solved by optionally reading into an aligned buffer
and copying from there to load_addr without overflow (in chunks, but
it could be at once if there's room).

But I'm no longer sure in which case I am, and when can we reach this. 
Non FIT image that is not inside a FIT image? I don't think current 
Rock Pi 4 would get here, but maybe loading from something else or 
in the future going to binman, or something ? 

> +}
> +
>  __weak void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
>  {
>  	typedef void __noreturn (*image_entry_noargs_t)(void);
> diff --git a/include/spl.h b/include/spl.h
> index 6134aba857..025fffb895 100644
> --- a/include/spl.h
> +++ b/include/spl.h
> @@ -237,7 +237,7 @@ struct spl_image_info {
>   *
>   * @dev: Pointer to the device, e.g. struct mmc *
>   * @priv: Private data for the device
> - * @bl_len: Block length for reading in bytes
> + * @bl_len: Block length for reading in bytes; must be a power of 2
>   * @filename: Name of the fit image file.
>   * @read: Function to call to read from the device
>   */
> @@ -609,6 +609,34 @@ int spl_load_image_ext_os(struct spl_image_info *spl_image,
>  			  struct spl_boot_device *bootdev,
>  			  struct blk_desc *block_dev, int partition);
>  
> +/**
> + * spl_load() - Parse a header and load the image
> + * @spl_image: Image data which will be filled in by this function
> + * @bootdev: The device to load from
> + * @info: Describes how to load additional information from @bootdev. At the
> + *        minimum, read() and bl_len must be populated.

declare whether read must be able to read from unaligned buffers ? 

> + * @header: The image header. This should already have been loaded. It may be
> + *          clobbered by the load process (if e.g. the load address overlaps).
> + * @size: The size of the image, if it is known in advance. Some boot devices
> + *        (such as filesystems) know how big an image is before parsing the
> + *        header. If this information is unknown, then the size will be
> + *        determined from the header.

The size in bytes.
If 0 , then the size will be

> + * @sectors: The offset from the start if @bootdev, in units of @info->bl_len.

if -> of 
maybe (nitpick)
in units of @info->bl_len. -> in units of @info->bl_len bytes.

> + *           This should have the offset @header was loaded from. It will be
> + *           added to any offsets passed to @info->read().
> + *
> + * This function determines the image type (FIT, legacy, i.MX, raw, etc), calls
> + * the appropriate parsing function, determines the load address, and the loads
> + * the image from storage. It is designed to replace ad-hoc image loading which
> + * may not support all image types (especially when config options are
> + * involved).
> + *
> + * Return: 0 on success, or a negative error on failure
> + */
> +int spl_load(struct spl_image_info *spl_image,
> +	     const struct spl_boot_device *bootdev, struct spl_load_info *info,
> +	     struct image_header *header, size_t size, size_t sector);
> +
>  /**
>   * spl_early_init() - Set up device tree and driver model in SPL if enabled
>   *

Thanks for your patience, on top of for your code.