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

[Hollis Blanchard]

On Mon, Aug 2, 2010 at 11:57 AM, Edgar E. Iglesias
<address@hidden> wrote:
> On Mon, Aug 02, 2010 at 10:59:11AM -0700, Hollis Blanchard wrote:
>> On Sun, Aug 1, 2010 at 5:36 AM, Edgar E. Iglesias
>> <address@hidden> wrote:
>> > On Sat, Jul 31, 2010 at 12:56:42AM +0200, Edgar E. Iglesias wrote:
>> >> On Thu, Jul 29, 2010 at 06:48:24PM -0700, Hollis Blanchard wrote:
>> >> > The kernel's BSS size is lost by mkimage, which only considers file
>> >> > size. As a result, loading other blobs (e.g. device tree, initrd)
>> >> > immediately after the kernel location can result in them being zeroed by
>> >> > the kernel's BSS initialization code.
>> >> >
>> >> > Signed-off-by: Hollis Blanchard <address@hidden>
>> >> > ---
>> >> >  hw/loader.c |    7 +++++++
>> >> >  1 files changed, 7 insertions(+), 0 deletions(-)
>> >> >
>> >> > diff --git a/hw/loader.c b/hw/loader.c
>> >> > index 79a6f95..35bc25a 100644
>> >> > --- a/hw/loader.c
>> >> > +++ b/hw/loader.c
>> >> > @@ -507,6 +507,13 @@ int load_uimage(const char *filename, 
>> >> > target_phys_addr_t *ep,
>> >> >
>> >> >      ret = hdr->ih_size;
>> >> >
>> >> > +   /* The kernel's BSS size is lost by mkimage, which only considers 
>> >> > file
>> >> > +    * size. We don't know how big it is, but we do know we can't place
>> >> > +    * anything immediately after the kernel. The padding seems like it 
>> >> > should
>> >> > +    * be proportional to overall file size, but we also make sure it's 
>> >> > at
>> >> > +    * least 4-byte aligned. */
>> >> > +   ret += (hdr->ih_size / 16) & ~0x3;
>> >>
>> >> Maybe it's only me, but it feels a bit akward to push down this kind of
>> >> knowledge down the abstraction layers. Does it work for you to have your
>> >> caller of load_uimage apply whatever resizing magic needed for your kernel
>> >> and arch?
>> >
>> > Ayway, IMO the conventions of where to pass blobs from the bootloader to 
>> > the
>> > loaded image are an agreement between the bootloader and the loaded code. 
>> > The
>> > formats or mechanisms to load the image should need to be involved that 
>> > much.
>> >
>> > For example in this particular case, other archs (e.g, MicroBlaze) might 
>> > not
>> > need any magic. The MicroBlaze linux kernel moves cmdline and device tree 
>> > blobs
>> > into safe areas prior to .bss initialization.
>>
>> Are you claiming that's the common case? FWIW, PowerPC and ARM don't
>> seem to. I wouldn't expect such logic except in reaction to a specific
>> bug (i.e. a qemu/firmware loader bug).
>
> I'm not trying to claim it's the common case, but it exists.

It exists and will remain unaffected by this patch, while the common
case will be improved.

>> The load_uimage() interface claims to report the size of the kernel it
>> loaded. If you argue that it shouldn't try to do that (and indeed you
>
> The way I understand it, it reports the size of what got loaded.

The difference between "what got loaded" and "the size of the loaded
file in memory" is a subtlety that is not at all clear from the code,
and that is precisely why I propose centralizing the logic to handle
it.

> It would be very difficult for load_uimage to figure out what memory
> areas are beeing touched prior to .bss init (or the point where the passed
> blob is used).
>
>> could argue it's not *possible* to do that accurately), that logic
>
> Right, its very hard for it to guess what memory areas are safe.
>
>> should be completely removed. The current behavior is worse than not
>> knowing at all: callers *think* they know, but it's guaranteed to be
>> wrong.
>>
>> Of course, if you do want to remove the size, then callers are left
>> with even less information than they had before. In that case, you
>
> I think returning the size of the loaded image has a value, for example
> for archs that move away the blobs before touching any memory outside
> their image. Bootloaders for those archs can put some blobs right after
> the loaded image.

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

>> 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?
Also, how can I know in the code if I chose wrong, what will the
user-visible failure be, and how difficult will that be to debug?

In summary, this patch protects users and developers. If I move it to
be PowerPC-specific, it will protect only PowerPC users and
developers, which is clearly a much smaller number. Debating whether
theoretically *all* users and developers would benefit from protection
seems odd.

-Hollis