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

[Marek Vasut]

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 ...

>>>>>>> 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 ...

>>>>> 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.

>>>> 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?
> 
> 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 ?

> 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
> 
> 


-- 
Best regards,
Marek Vasut