Re: [Qemu-devel] [PATCH] loader: pad kernel size when loaded from a uImage

[Hollis Blanchard]

On Mon, Aug 2, 2010 at 12:56 PM, Edgar E. Iglesias
<address@hidden> wrote:
> On Mon, Aug 02, 2010 at 12:33:54PM -0700, Hollis Blanchard wrote:
>>
>> You mean the one architecture, which by the way doesn't even use this
>> API? That doesn't seem like a strong argument to me. Anyways, it's
>
> Are we looking at the same code?

I don't know.

> Grep for load_uimage in qemu. 4 archs use it, PPC, ARM, m68k and MB.

I see the following:

   1     75  hw/an5206.c <<an5206_init>>
             kernel_size = load_uimage(kernel_filename, &entry, NULL, NULL);
   2    233  hw/arm_boot.c <<arm_load_kernel>>
             kernel_size = load_uimage(info->kernel_filename, &entry, NULL,
   3     50  hw/dummy_m68k.c <<dummy_m68k_init>>
             kernel_size = load_uimage(kernel_filename, &entry, NULL, NULL);
   4     14  hw/loader.h <<uint64_t>>
             int load_uimage(const char *filename, target_phys_addr_t *ep,
   5    277  hw/mcf5208.c <<mcf5208evb_init>>
             kernel_size = load_uimage(kernel_filename, &entry, NULL, NULL);
   6    121  hw/ppc440_bamboo.c <<bamboo_init>>
             kernel_size = load_uimage(kernel...ename, &entry, &loadaddr, NULL);
   7    235  hw/ppce500_mpc8544ds.c <<mpc8544ds_init>>
             kernel_size = load_uimage(kernel...ename, &entry, &loadaddr, NULL);

That makes 2x ColdFire, ARM, M68K, 2x PowerPC.
hw/petalogix_s3adsp1800_mmu.c is the only MicroBlaze I can see, and it
only loads ELF and binary kernels, not uImages.

> Of those archs, only 2 actually use the return value of load_uimage
> to decide where to place blobs. PPC and MB. MB doesn't want any
> magic applied to the return value. That leaves us with _ONE_ single
> arch that needs magic that IMO is broken. You are trying to guess the
> size of the loaded image's .bss area by adding a 16th of the uimage size?

Accounting for BSS hardly counts as "magic", I think. :)

>> just as much work to relocate an initrd from a padded address as it is
>> from a closer address, so there is no downside.
>>
>> The fact remains that the current API is broken by design, or to be
>> charitable "violates the principle of least surprise." With the
>> exception of microblaze, everybody who calls load_uimage() must
>> understand the nuances of the return value and adjust it (or ignore
>> it) accordingly. Why wouldn't we consolidate that logic?
>
> I don't see how guessing that the .bss area is a 16th of the loaded
> image makes this call any less confusing.

I agree it's arbitrary, but it's only arbitrary in one place. It's
also well-commented (IMHO), which is more than can be said for the
current code.

>> >> tell me: where should I hardcode initrd loading?
>> >
>> > Not sure, but I'd guess somewhere close to where you are calling
>> > load_uimage from (it wasn't clear to me where that was).
>>
>> Sorry, let me rephrase. At what address should I hardcode my initrd?
>> What about my device tree? As a followup, why not lower, or higher?
>
> You should be putting them at the same addresses as uboot puts them.

Fine. It's arbitrary in u-boot too, but at least it will be consistent.

-Hollis