[U-Boot] [RFC] Booting Xen from a FIT - Additional discussion about a refactor

[Simon Glass]

Hi Karl,

On 23 April 2015 at 07:15, Karl Apsite <Karl.Apsite at dornerworks.com> wrote:
>
> On 04/22/2015 09:55 PM, Simon Glass wrote:
>> +Tom
>>
>> Hi Karl,
>>
>> On 22 April 2015 at 13:05, Karl Apsite <Karl.Apsite at dornerworks.com> wrote:
>>> Hi!
>>>
>>> I work at DornerWorks with the Xen Hypervisor.  We work with a variety of
>>> embedded systems, and we wanted to facilitate Xen's boot procedure through
>>> U-boot's Flattened Image Tree (FIT) format.  I've already prototyped some of the
>>> functionality we were hoping to see, so we thought it'd be prudent to begin a
>>> discussion with denx to get your opinion on the matter,
>>>
>>> First Objective: (Summary of what was prototyped)
>>> A Flattened Image Tree is capable of holding all of the necessary binaries
>>> already, so we only need to make a quick change to allow u-boot to load an extra
>>> binary (in this case, a linux kernel) so that Xen can boot and load the kernel
>>> when it's ready.  I started by simply adding a line in the configuration of my
>>> tree-source (.its) to look like:
>>>
>>> config at 1 {
>>>     description = "Xen 4.6.0-unstable configuration";
>>>     kernel = "xen_kernel at 1";
>>>     fdt = "fdt at 1";
>>>     gen_bin0 = "linux_kernel at 1";
>>> };
>>>
>>> I investigated what effort would be needed to load the additional binary.
>>>
>>> Booting Xen is easy (only a kernel and fdt are required), but Xen will look at a
>>> hard-coded memory address to try a load a linux kernel.  This has to be placed
>>> in memory by u-boot.  The only major addition I needed, was to make u-boot care
>>> about a config option named "generic-binary" and load it, no questions asked.  I
>>> chose the name "generic binary" as I simply needed u-boot to load a [thing]
>>> without any additional behavior.  I'm using it to specifically load a linux
>>> kernel at a specific memory address in preparation for xen, but there could be
>>> potential future uses, hence the ambiguous name.
>>
>> I wonder whether you should add a new type for the target kernel?
>> General binary seems a bit vague. Just a thought.
>
> I do agree, I don't really like the term "generic binary" either.
>
> When preparing to boot Xen, u-boot needs to take a binary, and simply put it in
> place.  Unlike the other images/objects (kernel, fdt, ramdisk, etc) u-boot's
> role is very simple in this regard: "Take these bits, and make sure they go over
> here."
>
> In this scenario, the action taken by u-boot should be agnostic to what the
> image actually is.  U-boot should simply move a binary, without any additional
> behavior.  This led me to choose a name just as generic.

What is this additional behaviour you are referring to?

>
> Maybe changing the name to "Loadable(s)" would sound a bit better, but going so
> far as to name it "kernel" could be misleading.
>
>>
>>>
>>> The hack is pretty simple, as most everything was in place to boot xen, and it
>>> took a little extra legwork to make u-boot care about my gen_bin.  I added the
>>> necessary structure to load another object using ramdisk functions as examples.
>>>  By loading a separate binary, Xen was able to boot, and successively boot Linux
>>> as expected.  If you have any thoughts on this process overall, we'd like to
>>> take your concerns into consideration.
>>>
>>> For a little more fun, I extended out the configuration to be able to load N
>>> number of generics.
>
> This plan was part of the reason we were trying to keep the name more generic.
> Keeping it generic allows for people other than xen users to take advantage of
> the new feature in various ways.
>
>>>
>>> Affected Files:
>>> common/bootm.c
>>> common/image-fit.c
>>> common/image.c
>>> doc/uImage.FIT/source_file_format.txt
>>> include/configs/xilinx_zynqmp.h
>>> include/image.h
>>>
>>> Second Objective:
>>> While I was there, I noticed that u-boot's binary loading logic isn't as modular
>>> as I first expected.  Most objects eventually boil down to a boot_get_<thing>().
>>> Example: boot_get_ramdisk() or boot_get_kernel().  These functions are all very
>>> similar as they handle the various u-boot image types and load their specific
>>> binary.  The functions appear to have grown similar in structure and operation
>>> overtime.  I don't think it is too much to reduce the duplicated structure of
>>> the functions into a common boot_get_object() function, while replacing some of
>>> the extra function wrappings.
>>>
>>> Some quick diagrams to explain what I mean
>>> Initial:  http://i.imgur.com/H44dXmN.png (29KB)
>>> Refactor: http://i.imgur.com/apEpyWz.png (24KB)
>>
>> SGTM.
>>
>>>
>>> The refactor will likely effect the following files, several of which contain
>>> trivial name changes, or small changes in a variable type.
>>> arch/arm/lib/bootm.c
>>> arch/avr32/lib/bootm.c
>>> arch/microblaze/lib/bootm.c
>>> arch/mips/lib/bootm.c
>>> arch/nds32/lib/bootm.c
>>> arch/nios2/lib/bootm.c
>>> arch/openrisc/lib/bootm.c
>>> arch/powerpc/lib/bootm.c
>>> arch/sh/lib/bootm.c
>>> arch/sparc/lib/bootm.c
>>> arch/x86/lib/bootm.c
>>> common/bootm.c
>>> common/bootm_os.c
>>> common/cmd_bootm.c
>>> common/image-fdt.c
>>> common/image.c
>>> include/bootm.h
>>> include/image.h
>>>
>>> Summary
>>> 1.) RFC: What is the opinion on implementing a FIT configuration option to
>>> permit a FIT to boot Xen
>>
>> Seems OK to me.
>>
>>> 2.) RFC: What is the opinion on a potential refactor concerning the
>>> image-loading functions (boot_get_ramdisk, boot_get_kernel, etc) in order to
>>> unify them into one common boot_get_object() function.
>>
>> That would be good. I spent a bit of time refactoring the code to
>> reduce duplication - fit_image_load() was one of the results of that.
>> If you think the code is duplicated now you should have seen it before
>> :-)
>>
>> Probably 'boot_get_image()' is better than 'boot_get_object()' given
>> the current naming.
>>
>> Please do add tests for the new functionality - see test/image for
>> some existing tests. Python is preferred if the test is non-trivial.
>
> Absolutely, I'll be sure to include some tests in the patch(es).

Great. This area of U-Boot has had a lot of undocumented or untested
behaviour. It has got a bit better but your boot function sounds like
another step in the right direction.

Regards,
Simon