Hi Xavier,
On 6/3/22 16:58, Xavier Drudis Ferran wrote:
> El Fri, Jun 03, 2022 at 03:17:05PM +0200, Jerome Forissier deia:
>> First, I noticed that spl_load_fit_image() may write memory outside the range
>> given in the image node. For example on RockPi 4B I have the following FIT
>> (irrelevant parts omitted):
>>
>> / {
>> images {
>> atf_2 {
>> /* File size is 8024 bytes */
>> data = /incbin/("bl31_0xff3b0000.bin");
>> compression = "none";
>> load = <0xff3b0000>
>> }
>> }
>> }
>>
>> I expect SPL to load atf_2 into 0xff3b0000 - 0xff3b200b but due to the
>> alignment
>> of the source data in the MMC, spl_load_fit_image() writes one more block and
>> later moves the data in place with memcpy(). What are the guarantees that it
>> is a safe thing to do?
>>
>
> In my case bl31_0xff3b0000.bin is 8020 bytes in size.
>
>> In my case, the image starts at offset 308 in the 512-byte MMC block (that
>> offset is called 'overhead' in spl_load_fit_image()). As a result,
>> (8204 + 308) / 512 + 1 = 17 blocks = 8704 bytes are read.
>
> My overhead is 352, it's reading 17 blocks too.
>
>>
>> For some reason I can't explain, on my board only the first 8K of the 64K
>> SRAM
>> range 0xff3b0000 - 0xff3c0000 (INTMEM1) can be safely written to. Any data
>> written after this point corrupt the lower 8K. I found nothing in the rk3399
>> TRM about this [1].
>>
>
> In my Rock Pi 4B this does not happen. It happily writes the 17 blocks.
> Or it's just that I'm not noticing the problem ? Does it hang or give you
> an error, or do you just find it out by reading the SRAM?
I found out when trying to use signed FIT images. The board would boot fine
otherwise.
I am setting CONFIG_SPL_FIT_SIGNATURE=y and I have modified the script
arch/arm/mach-rockchip/make_fit_atf.py so that it generates the required
hash and signature nodes in u-boot.its. Then I am basically following the
instructions in doc/uImage.FIT/signature.txt -- generating a RSA key pair
with OpenSSL and using 'mkimage -k keys -K -r $(other-args...)'.
Please refer to the patches at:
https://git.codelinaro.org/linaro/dependable-boot/meta-ts/-/merge_requests/7/diffs
It is work in progress really but it should give you an idea of what I am doing,
and possibly help reproduce the issue.
I also confirmed the issue with a small test case. Could you please try the
following patch?
---8<-----------------------------------------------------------------------
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index f6ccd837aa..810d5fa6db 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -401,6 +401,12 @@ static int mmc_read_blocks(struct mmc *mmc, void *dst,
lbaint_t start,
struct mmc_cmd cmd;
struct mmc_data data;
+ printf("== 0x%08x\n", *(u32 *)0xff3b0100);
+ *(u32 *)0xff3b0100 = 0;
+ printf("== 0x%08x\n", *(u32 *)0xff3b0100);
+ *(u32 *)0xff3b2100 = 0xdeadbeef;
+ printf("== 0x%08x\n", *(u32 *)0xff3b0100);
+
if (blkcnt > 1)
cmd.cmdidx = MMC_CMD_READ_MULTIPLE_BLOCK;
else
---8<-----------------------------------------------------------------------
Guess what, this **does** print "0xdeadbeef" on the console on my board! :O
>
> I'm using the patch I sent yesterday:
>
> [PATCH 0/3] arm: rockchip: rk3399: rock-pi-4: power domain driver to boot
> from MMCSD
> https://lists.denx.de/pipermail/u-boot/2022-June/485498.html
>
> (instead of the patch you can simply configure
> CONFIG_ROCKCHIP_SPL_RESERVE_IRAM from make nconfig or so)
>
> Which reserves 0x0 - 0x14fff bytes in DDRAM for bl31. I found that
> otherwise, SPL memory in that area gets overwritten by
> bl31_0x00000000.bin. I don't remember exactly what was there, but in
> my tests, bl31_0x00000000 gets loaded before
> bl31_0xff3b0000.bin. Depending on what data or code is overwritten,
> SPL might be doing something funny.
I don't have bl31_0x00000000.bin. The bl31*.bin files come from bl31.elf
which is split by arch/arm/mach-rockchip/make_fit_atf.py.
It could be that we are using different versions of TF-A or different build
parameters, but that seems unrelated I think.
>> Anyways, I tried using a temporary buffer allocated on the heap to handle
>> the first and last blocks at least (the load address is properly aligned
>> for info->read() so the middle blocks can be read in one go). It works but
>> not reliably. And that is the second problem. mmc_read_blocks() in
>> mmc_bread()
>> sometimes returns an error. If I read blocks one by one in a loop THE read
>> randomly fails after a few blocks only. The error is -110 (-ETIMEDOUT) from
>> dwmci_send_cmd().
>>
>> Am I using the MMC read incorrectly?
>>
>
> It's not that you're using the read incorrectly.
>
> It's that the change shouldn't be needed ?
Ideally, yes, but it doesn't seem right to overflow the specified range. Who
knows what might be immediately after the end of the buffer for a given image?
Perhaps another image has been written there already, or the image might be
right before a reserved address range such is some mapped I/O area?
> Why can one only use the
> first 8K from the 64K INTMEM1 ? Did you find any reason the rest
> shouldn't be writable?
No, I don't understand. As I said the rest seems writable (at least up to some
point) but writing to it corrupts the memory below...
> Could corrupted memory explain what you're seeing ?
I found no evidence of corruption until SPL writes to 0xff3b2000 and above.
Thanks,
--
Jerome
