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

[Sean Anderson]

Hi Xavier,

On 6/16/22 5:42 AM, Xavier Drudis Ferran wrote:
> [You don't often get email from xdrudis at tinet.cat. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> 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 ?

I believe that some storage drivers expect the buffer to be DMA aligned.
See e.g. spl_simple_fit_read which is what I based spl_simple_read off of.

> 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).

It should 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

Yes. Presumably these boards did not support legacy images before. So this
will not break anything, but it will not work either.

>    Can we assume we can write before the buffer and after buffer+size

We can assume that we can write from buffer+offset to buffer+offset+size.
It's up to spl_get_load_buffer to ensure this is the case (but as you can
see it doesn't always do a good job).

For some context, AIUI the idea of using a negative offset is that for
memory-mapped storage this will ensure that the kernel is aligned to some
known base address (which is typically the load address). 

> 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 ?

>From the docs for struct spl_load_info, info->read uses units of
info->bl_len. For many types of storage this is 1 (byte aligned). For
others, it is the block size (typically 512 bytes, but maybe more for
flash). Whether or not there is a filename just controls whether we need
to be aligned to some other value.

>    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

Yes (on my system). TBH I'm not really sure about why the DMA align was
added in the first place. I think you need to ask Simon about that.

> (and spl_imx_romapi if ever needed)?

No clue.

> and should this or those be fixed ?

Maybe? I think it might be better to add a flag or a field to specify
whether we need to be aligned.

>> +     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 ?

Yeah, maybe. The problem is that generally we don't control the address
where the final image will be loaded. Something like
spl_parse_image_header is going to determine where we load the image,
and we have to work around that. We could perhaps warn here with DEBUG
enabled, but at this point everything is already laid out. The only way
to do this properly would be to load everything to an aligned location,
and then memmove it so the load_addr is correct. For a completely correct
load function, that's probably the only way to do it. I'll look into how
that affects the size.

spl_simple_fit_read gets around this because it gets to choose the load
address.

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

It will be unchanged by this 

>> +     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.

Correct. 

> We were given bytes and read multiples of bl_len (or
> ARCH_DMA_MINALIGN) bytes, and then there's overhead.
> 
> Is this always ok ?

No. But all the existing code is broken in this same way too.

>> +     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 ?

Yes, probably.

>> +                     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 ?

This code in particular is effectively translating old semantics to new.
The negative (unaligned) offset stuff is done by all loaders supporting
legacy images. So it will not always work, but it *will* work for things
where it did before.

>> +}
>> +
>>  __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

Will update.

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

Will fix.

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

I like the original wording here.

>> + *           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.

Sure. I suppose the real question is how badly we really need things to be DMA aligned...

I think Simon really needs to weigh in on this, but he's been pretty quiet recently.

--Sean