[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] loader: pad kernel size when loaded from a uIm
From: |
Hollis Blanchard |
Subject: |
Re: [Qemu-devel] [PATCH] loader: pad kernel size when loaded from a uImage |
Date: |
Mon, 2 Aug 2010 13:35:23 -0700 |
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