From mboxrd@z Thu Jan 1 00:00:00 1970 From: Karl Apsite Date: Thu, 23 Apr 2015 17:39:36 -0400 Subject: [U-Boot] [RFC] Booting Xen from a FIT - Additional discussion about a refactor In-Reply-To: References: <5537F10A.90800@dornerworks.com> <5538F069.9090108@dornerworks.com> Message-ID: <55396698.9030002@dornerworks.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 04/23/2015 01:06 PM, Simon Glass wrote: > Hi Karl, > > On 23 April 2015 at 07:15, Karl Apsite wrote: >> >> On 04/22/2015 09:55 PM, Simon Glass wrote: >>> +Tom >>> >>> Hi Karl, >>> >>> On 22 April 2015 at 13:05, Karl Apsite 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_ 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_(). >>>> 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 >