Re: [PATCH v4 0/7] elf: add better bootm support

["Clément Leger" <cleger@kalray.eu>]

Hi Oleksij,

----- On 6 Jun, 2020, at 12:11, Oleksij Rempel linux@rempel-privat.de wrote:

> Hi Clement,
> 
> This changes are needed to make your patch partially work on my system:
> https://github.com/olerem/barebox/commit/90ad4f3cd8cbcfd361ff780bcfce1573ae73053d

Thanks for investigating and sorry for the remainign errors.
Some xzalloc checks can probably be omitted or I think I should replace
the existing xzalloc by calloc to allow failing on elf loading
(which seems more sane).

> 
> I found following issues:
> - the elf->list was not initiated

My bad, I missed it while moving elf init to a specific function...
I started to wonder what I had tested side but It worked because
the list is only used in elf_release_regions (which wasn't called).

> - my system has only 64MiB RAM and only 8MiB is available for barebox
> alloc. Since you patch is reading complete file in to alloc, i was not
> able to start kernel which was already copied to alloc by:
> cp /mnt/tftp/kernel .

Indeed, strangely, mips code used to do a read_file before loading the
elf file in the mips bootm code. There is probably something different
but I can't quite get it. Anyway, it will be way better by not reading
the whole file.

> This is still not fixed, but with some sanity checks the user is
> notified about it.
> - lseek(fd, 0, SEEK_SET); is not working on tftp mount point. This was
> the reason why elf header actually passed check, but had a trash in
> elf->buf later.

Arg, and I did not checked lseek return value... I will use open/close
as you did.

> 
> On systems with limited amount of RAM, it would be better to read the
> header and request RAM regions on first stand, and then directly read
> them from file in to the requested regions.

Ok, I will modify the elf loader to do so. Apparently, tftp supports
forward lseek so it should be possible.

> 
> And there is one more thing, by using "dryrun" function (boot(m) -d), i
> would expect to be able to see reserved regions with "iomem" command.
> Currently it is not the case.

I can modify that but I guess this is the default behavior no ?
In the bootm command:

	ret = handler->bootm(data);
	if (data->dryrun)
		printf("Dryrun. Aborted\n");

err_out:
	if (data->os_res)
		release_sdram_region(data->os_res);
	if (data->initrd_res)
		release_sdram_region(data->initrd_res);

So if the dryrun argument was given, the complete err_out path is taken
and lead to releasing all regions. But I can do it if needed.

Thanks again for testing and sorry for letting such things pass in
the patches.

Clément

> 
> Am 10.05.20 um 13:31 schrieb Clément Leger:
>> Hi Oleksij,
>> 
>> ----- On 10 May, 2020, at 06:31, Oleksij Rempel o.rempel@pengutronix.de wrote:
>> 
>>> Hi Clement,
>>>
>>> just in case it makes a difference. I load this file over tftp (boot
>>> net)
>> 
>> I manage to load your elf file on kvx using bootm (I do not have net boot)
>> and here is what I get:
>> 
>> --------------------------------------------
>> Board: KONIC 200 (K200)
>> malloc space: 0x110306050 -> 0x200000000 (size 3.7 GiB)
>> export: No such file or directory
>> 
>> Hit any to stop autoboot:    3
>> barebox:/ bootm env/ore-linux-dpt-module
>> Entry Point:  80980000
>> 
>> Loading ELF 'env/ore-linux-dpt-module'
>> Loading phdr to 0x0000000080980000 (2740416 bytes)
>> ...
>> --------------------------------------------
>> 
>> So one segment is loaded which is correct according to readelf.
>> elf file is correct and can be loaded using bootm.
>> 
>> Could you try loading it using bootm directly ?
>> This will confirm if it's related to net boot or not (although I don't
>> know what are the underlying differences).
>> 
>> Thanks,
>> 
>> Clément
>> 
>>>
>>> On Sat, May 09, 2020 at 09:24:55PM +0200, Clément Leger wrote:
>>>> Hi Oleksij,
>>>>
>>>> ----- On 9 May, 2020, at 18:51, Oleksij Rempel o.rempel@pengutronix.de wrote:
>>>>
>>>>> On Sat, May 09, 2020 at 04:51:42PM +0200, Oleksij Rempel wrote:
>>>>>> Hi Clement,
>>>>>>
>>>>>> suddenly it is still exploding. I'll try to investigate the reason.
>>>>>
>>>>> common/elf.c:
>>>>>  load_elf_image_phdr()
>>>>>    elf_hdr_e_phnum() <-- returns 0
>>>>>    for (i = 0; i < elf_hdr_e_phnum(elf, buf) ; ++i) {
>>>>>      ... so we will never get here.
>>>>>
>>>>> Probably we should add a sanity check here and be more verbose in this
>>>>> case.
>>>>
>>>> There is something odd because your elf is correct and has 2 program
>>>> headers. So this should not be a problem (but a sanity check makes
>>>> sense !).
>>>>
>>>> I suspect that the elf size computation is wrong with your elf file
>>>> and leads to incompletely loaded elf file but I tried to compute
>>>> it using the sequence of code used in barebox and everything seems ok.
>>>> Maybe there is something with the endianess/elfclass going wrong.
>>>> I tried again on kvx to be sure I did not made a mistake but it works
>>>> and the elf entry address is correctly used.
>>>> I'll try to add handling for different endianess and load your elf
>>>> file on kvx to debug that.
>>>>
>>>>>
>>>>> here is my elf image:
>>>>> https://github.com/olerem/barebox/blob/new-elf/ore-linux-dpt-module
>>>>
>>>> I succeeded in building Distrokit images but I can't get it under barebox.
>>>> There is only 4M oof RAM and each time I tries to modify it, it won't boot
>>>> anymore.
>>>> I tried integrating the kernel (and your eore-linux-dpt-module elf) in
>>>> the environment but it also won't boot. Do you have any idea on how to
>>>> do it ?
>>>> Even if the kernel won't start under qemu , at least I'll be able to
>>>> understand what is wrong.
>>>>
>>>> Thanks for testing,
>>>>
>>>> Clément
>>>>
>>>>>
>>>>>> On Fri, May 08, 2020 at 07:04:04PM +0200, Clement Leger wrote:
>>>>>>> Currently, when booting an elf file using "bootm /dev/mtdx", bootm will
>>>>>>> simply pass the file to the bootm and the read done on it will read the
>>>>>>> entire flash partition. This series starts by some cleanup and then add an
>>>>>>> elf_open function to load the elf file size only based on the elf header.
>>>>>>> A special handling for the elf file is also added in bootm data to allow
>>>>>>> using directly the elf file structure. Finally the mips bootm is modified
>>>>>>> to use bootm_load_os directly instead of manual elf loading.
>>>>>>>
>>>>>>> Compilation for both mips and arm has been tested but run on qemu-malta was not
>>>>>>> possible. Changing the MALLOC_SIZE to allow loading a kernel always lead to a
>>>>>>> non-bootable system. Changes have been tested on kvx architecture for which
>>>>>>> bootm support has been added and will be submitted.
>>>>>>>
>>>>>>> Changes v3 -> v4
>>>>>>>  - Fix init of elf entry address to be used by bootm_load_elf
>>>>>>>
>>>>>>> Changes v2 -> v3
>>>>>>>  - Integrate elf loading in bootm_load_os
>>>>>>>  - Add patch to remove now unused elf_load_image/elf_release_image
>>>>>>>  - Use malloc instead of xmalloc and check return value
>>>>>>>
>>>>>>> Changes v1 -> v2
>>>>>>>  - Add BOOTM_ELF config to select elf support and add checks in code
>>>>>>>  - Add an elf_get_mem_size function to avoid computing elf size in bootm.c
>>>>>>>  - Use xmalloc and read_full in elf_open instead of xzalloc/read
>>>>>>>  - Fix data->elf NULL reset
>>>>>>>  - Remove elf struct entirely from mips bootm code
>>>>>>>
>>>>>>> Clement Leger (7):
>>>>>>>   common: elf: add computation of elf boundaries
>>>>>>>   common: elf: fix warning on 32 bits architectures
>>>>>>>   common: elf: split init to be reused from other function
>>>>>>>   common: elf: add elf_open, elf_close and elf_load
>>>>>>>   common: bootm: add support for elf file loading
>>>>>>>   mips: lib: bootm: use bootm elf loading capabilities
>>>>>>>   common: elf: remove elf_load_image/elf_release_image
>>>>>>>
>>>>>>>  arch/mips/lib/bootm.c |  31 +++++-------
>>>>>>>  common/Kconfig        |   8 +++
>>>>>>>  common/bootm.c        |  33 +++++++++++++
>>>>>>>  common/elf.c          | 111 +++++++++++++++++++++++++++++++++++-------
>>>>>>>  include/bootm.h       |   3 ++
>>>>>>>  include/elf.h         |  16 +++++-
>>>>>>>  6 files changed, 163 insertions(+), 39 deletions(-)
>>>>>>>
>>>>>>> --
>>>>>>> 2.17.1
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> barebox mailing list
>>>>>>> barebox@lists.infradead.org
>>>>>>> http://lists.infradead.org/mailman/listinfo/barebox
>>>>>>>
>>>>>>
>>>>>> --
>>>>>> Pengutronix e.K.                           |                             |
>>>>>> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
>>>>>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>>>>>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Pengutronix e.K.                           |                             |
>>>>> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
>>>>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>>>>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>>>>
>>>> _______________________________________________
>>>> barebox mailing list
>>>> barebox@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/barebox
>>>
>>> --
>>> Pengutronix e.K.                           |                             |
>>> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
>>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>> 
>> _______________________________________________
>> barebox mailing list
>> barebox@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/barebox
>> 
> 
> 
> --
> Regards,
> Oleksij

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox