[U-Boot] [PATCH 0/3] imx: bootaux elf firmware support

[Stefan Agner]

On 2017-04-04 13:17, Marek Vasut wrote:
> On 04/04/2017 09:45 PM, Stefan Agner wrote:
>> On 2017-04-04 11:38, Marek Vasut wrote:
>>> On 04/04/2017 07:57 PM, Stefan Agner wrote:
>>>> On 2017-04-04 02:22, Marek Vasut wrote:
>>>>> On 04/04/2017 02:02 AM, Stefan Agner wrote:
>>>>> [...]
>>>>>>>>> Admitedly, I didn't look at the patch, but if you want to boot ad-hoc
>>>>>>>>> cores, you can very well also boot secondary cores on the current CPU
>>>>>>>>> complex with the same command. Why not ?
>>>>>>>>
>>>>>>>> Sure, it could be done. I just feel it is not the right design.
>>>>>>>>
>>>>>>>> Auxiliary cores have usually a different view to memory, this is why I
>>>>>>>> had to add the get_host_mapping callback in the elf loader code to let
>>>>>>>> architecture dependent code translate to host addresses. SMP systems
>>>>>>>> don't need that.
>>>>>>>>
>>>>>>>> Also flush caches is not necessary on some cache coherent CPU's
>>>>>>>> (depending on how your cache coherence between I and D cache looks
>>>>>>>> like).
>>>>>>>
>>>>>>> So SMP is just a reduced special-case of this , yes ?
>>>>>>
>>>>>> Yeah, I guess you can get away with dummy callback implementation and a
>>>>>> performance hit due to cash flushes.
>>>>>
>>>>> Or you can abstract things out ?
>>>>>
>>>>
>>>> There is one callback to arch for translation and one for cache flush,
>>>> what more can I abstract out?
>>>
>>> Well then I don't think I understand your concerns about cache flushing
>>> in SMP .
>>>
>>
>> It makes things unnecessary slower.
> 
> You always have to flush cache if you expect the peripheral to read
> cached memory , so I don't think I understand your remark ...
> 

Which peripheral? The cache coherent CPU is going to read that memory...

>>>>>>>> Creating a new command like bootaux comes with very few overhead.
>>>>>>>
>>>>>>> The overhead is the new command, we already have many ad-hoc commands.
>>>>>>>
>>>>>>
>>>>>> Agreed, and I really wished that this got discussed when that command
>>>>>> initially got added. I brought it up back then...
>>>>>> https://lists.denx.de/pipermail/u-boot/2016-January/240323.html
>>>>>>
>>>>>> It seemed to be acceptable to just add this ad hoc command, with some
>>>>>> "random binary format" support back then...
>>>>>
>>>>> Based on that discussion, I only see that noone opposed, but I don't see
>>>>> any agreement.
>>>>>
>>>>
>>>> Maybe I need to word my emails a bit stronger, but with that email I
>>>> actually tried to oppose:
>>>> https://lists.denx.de/pipermail/u-boot/2016-January/240989.html
>>>
>>> Well, I do not like adding ad-hoc commands as it's not managable in the
>>> long run.
>>>
>>
>> I argue that a remote core loading command is _much_ more manageable
>> than making the bootm command even more complex by support
>> SMP/remoteproc and whatnot usecases... I would even argue that a bunch
>> of those commands are more manageable than a single ifdef/if hell....
> 
> I guess a boot* option to start remote core would do ?
> 
>> That said, I still would push for keeping the image processing code
>> generic, whenever it makes sense.
> 
> Agreed
> 
>>>>>> Ok, it is not entirely
>>>>>> random, since it is the format of a binary how it ends up in
>>>>>> microcontrollers execute in place flash (stack pointer and reset vector
>>>>>> are the two first words)....
>>>>>
>>>>> I thought this command was starting the core by loading data to RAM ,
>>>>> not flash ?
>>>>>
>>>>
>>>> Ok, maybe I am a bit unclear here:
>>>>
>>>> bootaux currently supports a Cortex-M specific binary format only. The
>>>> binary format starts with a Cortex-M class vector table. The vector
>>>> tables first vector is the "reset vector".
>>>>
>>>> In a regular microcontroller, that binary gets flashed on a NOR flash
>>>> which is mapped at 0x0 for the CPU. The CPU has no "boot ROM", the CPU
>>>> starts by calling the reset vector. So when NXP defined how the bootaux
>>>> command should look like, they just took that binary format which was
>>>> laying around and implemented a U-Boot command around it.
>>>>
>>>> So this is the history of the binary format. And my point here is, that
>>>> the binary format supported by bootaux is _very_ Cortex-M class
>>>> specific.
>>>
>>> Aha, so I now totally don't understand why this command cannot be
>>> fixed/extended to support other/generic cores or SMP systems etc.
>>> But looking at the initial proposal, I think maybe the intention of this
>>> patchset was not to add that support, but to fix the command to
>>> support loading ELF files ? We already have bootelf for that though ...
>>>
>>
>> Yes, that is pretty much it. I would like to teach that command a more
>> generic format, which would be at least a step towards something more
>> generic/standardized.
>>
>> bootelf is really meant for the primary CPU. That would be an entirely
>> different direction: Make all common boot commands "aux core" capable.
>>
>> But I would strongly vote against that. First, those commands have
>> already complex arguments and argument handling (e.g. bootm), and their
>> implementation supports use cases which we hardly would ever use on aux
>> cores (initramfs..).
> 
> That might need some further thinking/consolidation . IMO out of scope
> of this discussion ...
> 

Outside of scope, but you need to take it in consideration when
desciding which way to go with remote proc boot commands...

>>>>>> However, making this ad hoc command now
>>>>>> generic really feels weird to me, since we would end up supporting that
>>>>>> format for A class CPUs etc... bootaux is really suited for auxiliary
>>>>>> M-class cores on ARM, as it is right now. Maybe we should have named it
>>>>>> bootm ;-)
>>>>>
>>>>> We always try to avoid one-off hacks, so it's not weird. I still don't
>>>>> quite understand how it is a problem to abstract things out . I am not
>>>>> asking you to support CA, but to avoid CM specific implementation which
>>>>> cannot be extended to support CA or even MIPS/Nios2/etc .
>>>>>
>>>>
>>>> Again, why would you support a Cortex-M class specific file format for
>>>> MIPS/Nios2 etc...?
>>>>
>>>> bootaux is M4 specific. We can through it away, and start over, but then
>>>> we should call the command differently.
>>>
>>> Unless you look very carefully, it is not clear that it is cortex M4
>>> specific at all based on this discussion. I was under the impression
>>> that the goal here was to support all sorts of secondary cores ...
>>>
>>
>> Yeah it's also only that binary format handling which is M4 specific.
>> And frankly, that is just 2 lines of code, basically dereferencing the
>> first two words:
>>
>> +    if (valid_elf_image(addr)) {
>> +        stack = 0x0;
>> +        pc = load_elf_image_phdr(addr);
>> +        if (!pc)
>> +            return CMD_RET_FAILURE;
>> +
>> +    } else {
>> +        /*
>> +         * Assume binary file with vector table at the beginning.
>> +         * Cortex-M4 vector tables start with the stack pointer (SP)
>> +         * and reset vector (initial PC).
>> +         */
>> +        stack = *(u32 *)addr;
>> +        pc = *(u32 *)(addr + 4);
>> +    }
>>
>>>>>>>> This are the reasons why I feel creating a new command for a SMP boot
>>>>>>>> case makes more sense. We can still reuse functions which are very
>>>>>>>> similar by moving them into some common location, where it makes sense.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Also, I think this might come useful when booting stuff like "Altera
>>>>>>>>> Sparrow" ...
>>>>>>>>
>>>>>>>> I am not familiar with that architecture, what kind of core does it
>>>>>>>> provide which needs to be booted by U-Boot?
>>>>>>>
>>>>>>> The secondary ARM core in the SoCFPGA C-A9 complex or Nios2 core in the
>>>>>>> FPGA.
>>>>>>
>>>>>> In my thinking, the Nios2 core seems like such a remote processor well
>>>>>> suited for the bootaux command. For the secondary A9, I would create a
>>>>>> new command.
>>>>>
>>>>> Uh, why, that does not make any sense to me. Both the random core in
>>>>> FPGA and the secondary CA9 core have almost the same bringup sequence.
>>>>> What is so different about this stuff ?
>>>>>
>>>>
>>>> A cache coherent core and a non-cache coherent core remote core
>>>> somewhere in the SoC is very much different IMHO.
>>>>
>>>> Lets compare how you bring up a A class CPU, or cache coherent SMP style
>>>> CPU in general:
>>>>
>>>> 1. Load code into RAM
>>>> 2. Some cache flushing might be necessary due to I/D-cache incoherency
>>>> 3. Write entry point into some architecture specific register
>>>> 4. kick off the CPU by writing another architecture specific register
>>>>
>>>> M class/remote CPU
>>>>
>>>> 1. Load code in RAM, depending on image format translate core specific
>>>> memory map to host memory map
>>>> 2. Flush D-cache
>>>> 3. (Potentially set up IOMMU, not in the NXP case though)
>>>> 4. Write entry point into some architecture specific register
>>>> 5. kick off the CPU by writing another architecture specific register
>>>>
>>>> From what I can see, everything is different other than "Load code in
>>>> RAM" which is image specific. If the image format is complex, we
>>>> certainly should factor that out to avoid code duplication.
>>>
>>> Image format is usually binary blob or elf file, we support both.
>>> The rest is "load file to RAM, do magic setup (can be CPU specific),
>>> unreset CPU".
>>>
>>
>> Loading files to RAM is already generic since it is handled by separate
>> commands.
>>
>> It is really the image handling which has the potential of reusability.
> 
> OK
> 
>>>>>> If we want to support the two with the same command, we already have a
>>>>>> first problem: How do we address them? Of course, we could add just a
>>>>>> index or something, but this would already break backward compatibility
>>>>>> of the bootaux command.
>>>>>
>>>>> So we already accepted a command which has shit commandline semantics
>>>>> and we're stuck with it , hrm. You can always specify a default core
>>>>
>>>> Add shitty binary file format to the list.
>>>
>>> Why do we care about this ? The CM just needs a vectoring table, so set
>>> it up (in the elf file or whatever) and point the core at the reset vector.
>>>
>>
>> Not sure what you want to say here, but I guess we need to keep support
>> for this file format for backward compatibility?
> 
> I guess so, I didn't look into the requirements for the CM4 core on MX6.
> 

The only hard requirement the CPU has it needs a valid entry point, and
of course, code to run. The binary format is otherwise pretty much
independent of the hardware.

So the only reason to keep backward compatibility is because there are
already users using the command. There is documentation etc...

That said, if your firmware relies on stack pointer being setup
correctly, then your binary format should also be able to provide a
valid stack pointer. The binary format has that since that is the way
the initial reset vector on M class cores work. The elf and FIT image do
not have these capabilities. However, setting up the SP as part of the
firmware is rather easy, and the FreeRTOS BSP as offered by NXP does not
make use of the preinitialized stack pointer and sets it up anyway...


>>>>> index for those two iMX boards and it's done. Or just fix the semantics
>>>>> in their default environment, which is not awesome, but I doubt this is
>>>>> wildly used yet.
>>>>
>>>> I guess my general point is, bootaux is already broken in two ways:
>>>> Command line interface does not allow for extensibility, the only format
>>>> supported right now is a binary format which is not generic.
>>>>
>>>> My patchset was an attempt to improve the situation to give it at least
>>>> a decent elf format support.
>>>
>>> Maybe this information was lost ... Lukasz ?!
>>>
>>>> Note that there is also bootelf, where the elf headers have been
>>>> introduced. Reusing that command seems impractical since it is meant to
>>>> boot on the primary CPU. My first attempt was reusing at least the load
>>>> function, but it seemed impractical due to memory map translation and
>>>> since the load function is rather short. If it helps for the acceptance
>>>> of this patchset, I still could try to make that happen.
>>>
>>> Extending bootelf might indeed make more sense IMO. But that needs
>>> discussion.
>>>
>>
>> There are two variants of reuse here:
>> 1. Reuse the elf parsing and loading function (function
>> load_elf_image_phdr in this case).
>> 2. Reuse the bootelf command. This would need extending the command with
>> some parameter to tell it to not boot the primary CPU but boot some
>> other auxiliary processor.
>>
>>
>> Let's get a bit more concrete here, take bootelf as a case study:
>>
>> 1 would introduce the architecture specific translation callback
>> load_elf_image_phdr (see my load_elf_image_phdr implementation in this
>> patchset). For regular/primary boot we can implement a dummy 1:1
>> translation.
>>
>>
>>
>> 2 would need 1, and changes to do_bootelf. Since the command has already
>> optional arguments adding core selection as an optional argument is not
>> possible. But we could add a parameter, e.g. -c (core):
>>
>> bootelf -c aux
>>
>> or
>>
>> bootelf -c 1
> 
> Works for me ...
> 
>> The exact enumeration would have to be discussed. I guess this would
>> also be architecture specific.
>>
>> Adding the new parameter handling plus callbacks into architecture
>> dependent code probably makes do_bootelf double in length. Also, half of
>> the existing code cannot be reused: The call do_bootelf_exec is primary
>> CPU specific, passing the arguments (this is likely not supported by
>> remote cores) and the return code handling (since a remote core would
>> not return), autostart handling, not sure?

Here...  see below

>>
>> 2 would also need to either disable the section header functionality for
>> the remote core, or we add another if in do_bootelf and avoid parsing
>> section header even if requested...
>>
>>
>> Again, I strongly disfavor solution 2. That makes boot commands with
>> image types we want to support even more complex. bootelf already starts
>> to look ugly when I start implementing that, and bootelf is really one
>> of the cleanest and simplest boot commands I have seen... I probably
>> would give it a shot if this is the only way to bring elf format support
>> for aux cores upstream... Good luck for the guy implementing FIT image
>> support (bootm) for auxiliary cores :-D
> 
> So what is the problem with this ? Load the payload somewhere and ungate
> the secondary core , why is that so hard ?
> 

Are you ignoring my arguments above on purpose? As I clearly showed,
even bootelf has differences between a primary and a secondary/remote
core... I don't expect that for bootm to be any different.

In fact, almost certainly there will be a lot more primary core/remote
core distinctions to be made during the whole bootm command. bootm also
supports cases which probably never make sense for a remote core
(Android images, initramfs). Sure, hacking something up which works in a
certain use-case is probably not a big deal, but to do it properly, you
would have to check every image format, every corner case, and properly
disambiguate between primary/auxilary core scenarios (otherwise you end
up with "for me, bootm -c aux my.fit works, I don't know why your bootm
-c your.uimg does not work!", sure you can just ignore that, but IMHO
that is broken design, and just not user-friendly).


But then, I really couldn't care less since I am not interested in FIT
image support for remote processors. My point is just that in bootm will
be ugly and hard to properly support auxiliary cores. And I feel when
bootm does not work well, and bootelf also is a bit bumpy, we probably
should just not mix up primary core boot commands and auxilary core boot
commands...

--
Stefan


>> 1 we could do, and I would be willing to do it. I personally feel that
>> the small code duplication is cleaner than reusing that function from
>> cmd/elf.c and implementing the dummy 1:1 translation, but both works for
>> me....
>>
>> IMHO, commands are really lightweight and nice in U-Boot, I think we
>> should make use of them, even when it means some namespace pollution.
>>
>> --
>> Stefan
>>
>>