On 11/16/18 7:48 AM, Simon Goldschmidt wrote: > On 14.11.2018 00:03, Heinrich Schuchardt wrote: >> On 11/13/18 10:47 PM, Simon Goldschmidt wrote: >>> >>> Am Di., 13. Nov. 2018, 22:37 hat Heinrich Schuchardt <xypron.g...@gmx.de >>> <mailto:xypron.g...@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 >>> <feste...@gmail.com <mailto:feste...@gmail.com>> >>> >>> wrote: >>> >>>> Hi Simon, >>> >>>> >>> >>>> On Mon, Nov 12, 2018 at 7:25 PM Simon Goldschmidt >>> >>>> <simon.k.r.goldschm...@gmail.com >>> <mailto:simon.k.r.goldschm...@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. > > OK, so it seems U-Boot just cannot handle the "reserved-memory" node > with its subnodes but only the "memreserve" thing on top level? > I have the rest of the patches in a state I would submit, but I'll need > some more time to dig into the dts reserved memory reservation... > > Simon >
Linux Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt defines /reserved-memory. Documentation/arm64/booting.txt defines /memreserve The device tree specification v0.2 fails to provide the label used for memory reservations. https://patchwork.kernel.org/patch/5143721/ sheds some light: "Device tree regions described by /memreserve/ entries are not available in /proc/device-tree, and hence are not available to user space utilities that use /proc/device-tree. In order for these regions to be available, convert any arm64 DTS files using /memreserve/ entries to use reserved-memory nodes." So /memreserve and /reserved-memory have different semantics but neither region should be used by the load command. Best regards Heinrich >> >> 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 >>> > >>> > > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
