From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 212B2C43334 for ; Thu, 16 Jun 2022 09:42:34 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id A958A83EF9; Thu, 16 Jun 2022 11:42:31 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=tinet.cat Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: by phobos.denx.de (Postfix, from userid 109) id 2426B84328; Thu, 16 Jun 2022 11:42:30 +0200 (CEST) Received: from mx1.tinet.cat (mx1.tinet.cat [195.77.216.146]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id D678C83EF9 for ; Thu, 16 Jun 2022 11:42:25 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=tinet.cat Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=xdrudis@tinet.cat X-ASG-Debug-ID: 1655372544-163e7b4553920590001-4l7tJC Received: from smtp01.tinet.cat (smtp.tinet.cat [195.77.216.131]) by mx1.tinet.cat with ESMTP id GOaTPxxipxyBMgJZ; Thu, 16 Jun 2022 11:42:24 +0200 (CEST) X-Barracuda-Envelope-From: xdrudis@tinet.cat X-Barracuda-Effective-Source-IP: smtp.tinet.cat[195.77.216.131] X-Barracuda-Apparent-Source-IP: 195.77.216.131 Received: from begut (50.red-79-152-182.dynamicip.rima-tde.net [79.152.182.50]) by smtp01.tinet.cat (Postfix) with ESMTPSA id 19D9E605D0AB; Thu, 16 Jun 2022 11:42:24 +0200 (CEST) Date: Thu, 16 Jun 2022 11:42:13 +0200 From: Xavier Drudis Ferran To: Sean Anderson Cc: Tom Rini , u-boot@lists.denx.de, Stefan Roese , Marek Vasut , Simon Glass , Pali =?utf-8?B?Um9ow6Fy?= , Marek =?utf-8?B?QmVow7pu?= Subject: Re: [PATCH v3 1/9] spl: Add generic spl_load function Message-ID: <20220616094213.GA1930@begut> X-ASG-Orig-Subj: Re: [PATCH v3 1/9] spl: Add generic spl_load function References: <20220505201655.645692-1-sean.anderson@seco.com> <20220505201655.645692-2-sean.anderson@seco.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20220505201655.645692-2-sean.anderson@seco.com> User-Agent: Mutt/1.10.1 (2018-07-13) X-Barracuda-Connect: smtp.tinet.cat[195.77.216.131] X-Barracuda-Start-Time: 1655372544 X-Barracuda-URL: https://webmail.tinet.cat:443/cgi-mod/mark.cgi X-Barracuda-Scan-Msg-Size: 8827 X-Barracuda-BRTS-Status: 1 X-Barracuda-Bayes: INNOCENT GLOBAL 0.5017 1.0000 0.7500 X-Barracuda-Spam-Score: 4.75 X-Barracuda-Spam-Status: No, SCORE=4.75 using global scores of TAG_LEVEL=1000.0 QUARANTINE_LEVEL=6.0 KILL_LEVEL=8.0 tests=BSF_RULE7568M, BSF_SC0_MV0249, BSF_SC0_SA085b, MARKETING_SUBJECT X-Barracuda-Spam-Report: Code version 3.2, rules version 3.2.3.98754 Rule breakdown below pts rule name description ---- ---------------------- -------------------------------------------------- 0.60 MARKETING_SUBJECT Subject contains popular marketing words 0.50 BSF_RULE7568M Custom Rule 7568M 0.40 BSF_SC0_SA085b Custom Rule SA085b 2.50 BSF_SC0_MV0249 Custom rule MV0249 X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.5 at phobos.denx.de X-Virus-Status: Clean 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.