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

[Karl Apsite]

On 04/23/2015 01:06 PM, Simon Glass wrote:
> 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?

In each of the existing boot_get_<thing> functions, I saw that U-boot stores
various addresses in the images parameter: bootm_headers_t *images. I am making
an assumption that these addresses are used later for any possible "additional
behaviors."  That could very well be a misunderstanding, but I thought those
addresses are used by u-boot later in the boot process.

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