On 11/13/18 10:47 PM, Simon Goldschmidt wrote:
Am Di., 13. Nov. 2018, 22:37 hat Heinrich Schuchardt <xypron.glpk@gmx.de mailto:xypron.glpk@gmx.de> geschrieben:
On 11/13/18 9:01 PM, Simon Goldschmidt wrote: > On 13.11.2018 20:42, Heinrich Schuchardt wrote: >> On 11/13/18 6:47 AM, Simon Goldschmidt wrote: >>> On Tue, Nov 13, 2018 at 3:23 AM Fabio Estevam <festevam@gmail.com <mailto:festevam@gmail.com>> >>> wrote: >>>> Hi Simon, >>>> >>>> On Mon, Nov 12, 2018 at 7:25 PM Simon Goldschmidt >>>> <simon.k.r.goldschmidt@gmail.com <mailto:simon.k.r.goldschmidt@gmail.com>> wrote: >>>> >>>>> diff --git a/fs/fs.c b/fs/fs.c >>>>> index adae98d021..4baf6b1c39 100644 >>>>> --- a/fs/fs.c >>>>> +++ b/fs/fs.c >>>>> @@ -428,13 +428,57 @@ int fs_size(const char *filename, loff_t *size) >>>>> return ret; >>>>> } >>>>> >>>>> -int fs_read(const char *filename, ulong addr, loff_t offset, >>>>> loff_t len, >>>>> - loff_t *actread) >>>>> +#ifdef CONFIG_LMB >>>> Unrelated to your series, but I was wondering if we could get rid of >>>> the CONFIG_LMB option. >>>> >>>> As far as I can see all the architectures define it, the only >>>> exception being arch/sh. >>>> >>>> If you agree I can send a patch after your series gets applied that >>>> removes CONFIG_LMB. >>> Sure, that would clean things up. >>> >>> Simon >>> >> NAK >> >> This patch-series does not provide what is needed. With >> odroid-c2_defconfig I get >> >> fdt list /reserved-memory/secmon@10000000 >> reserved-memory { >> secmon@10000000 { >> reg = <0x00000000 0x10000000 0x00000000 0x00200000>; >> no-map; >> }; >> }; >> >> => load mmc 0:1 0x10000000 dtb >> 22925 bytes read in 8 ms (2.7 MiB/s) >> >> So now I have successfully overwritten the secure monitor. Urrgh. >> >> As you have observed load is still writing into a memory area that is >> reserved by the device-tree. >> >> Please, iterate over the device tree to ensure that nothing is loaded >> into a reserved memory area. Do not expect board files to do anything >> but create the reserve-memory entry in the device tree. > > First of all, thanks for testing. I must admit I haven't tested this > case, I just had the impression the existing function > 'boot_fdt_add_mem_rsv_regions()' (in image-fdt.c) would do that. I'll > have to dig into why it doesn't. > > Or do you have CONFIG_OF_LIBFDT disabled? `make odroid-c2_defconfig` sets CONFIG_OF_LIBFDT=y CONFIG_CMD_FDT depends on it. So without I would not have had the fdt command used above. The device-tree I was looking at was the one provided by U-Boot at ${fdtcontroladdr}.That might be an explanation. I used 'gd->fdt_blob' only in my patch.
For the Odroid C2 the relevant memory reservations are created in arch/arm/mach-meson/board.c:61: void meson_gx_init_reserved_memory(void *fdt)
According to /README ${fdtcontroladdr} and gd->fdt_blob should be the same:
lib/fdtdec.c:1255: gd->fdt_blob = (void *)env_get_ulong("fdtcontroladdr", 16, (uintptr_t)gd->fdt_blob);
The boards with CONFIG_OF_PRIOR_STAGE=y set fdtcontroladdr in the board file (board/broadcom/bcmstb/bcmstb.c).
You should use gd->fdt_blob. Just make sure it is already set.
Best regards
Heinrich
Are there any more device tree locations to care about?
We should also think about making this testable. I would be happy if we had a test on a QEMU board.I tried to build the tests but I only got build errors. Is there any documentation about what I could be missing? I think my Ubuntu should be up to date, so maybe I am missing some dependencies...?
Simon
Best regards Heinrich > > In any case, it *was* my intention to include the dts memory > reservation! I'll make sure I test that for a v2. > > Simon >