[Qemu-devel] [PATCH] PPC4xx: don't unregister RAM at reset

[Qemu-devel] [PATCH] PPC4xx: don't unregister RAM at reset

[Hollis Blanchard]

The PowerPC 4xx SDRAM controller emulation unregisters RAM in its reset
callback. However, qemu_system_reset() is now called at initialization
time, so RAM is unregistered before starting the guest.

Signed-off-by: Hollis Blanchard <hollis@penguinppc.org>
---
 hw/ppc4xx_devs.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/hw/ppc4xx_devs.c b/hw/ppc4xx_devs.c
index be130c4..7f698b8 100644
--- a/hw/ppc4xx_devs.c
+++ b/hw/ppc4xx_devs.c
@@ -619,7 +619,6 @@ static void sdram_reset (void *opaque)
     /* We pre-initialize RAM banks */
     sdram->status = 0x00000000;
     sdram->cfg = 0x00800000;
-    sdram_unmap_bcr(sdram);
 }
 
 void ppc4xx_sdram_init (CPUState *env, qemu_irq irq, int nbanks,
-- 
1.7.1.1

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

[Hollis Blanchard]

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 <hollis@penguinppc.org>
---
 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;
+
 out:
     if (data)
         qemu_free(data);
-- 
1.7.1.1

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

[malc]

On Thu, 29 Jul 2010, 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 <hollis@penguinppc.org>
> ---
>  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;
> +

This portion uses tabs.

>  out:
>      if (data)
>          qemu_free(data);
> 

-- 
mailto:av1474@comtv.ru

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

[Edgar E. Iglesias]

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 <hollis@penguinppc.org>
> ---
>  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?

Cheers

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

[Edgar E. Iglesias]

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 <hollis@penguinppc.org>
> > ---
> >  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?


Hi Hollis,

Sorry I was a bit in a hurry and short last time. And sorry for the bad
wording, I thought awkward simply meant wrong (english is not my native
languauge).

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.

That's why I think that these kind of decisions should be made higher up.

Thanks and sorry for my clumsy wording last time :)
Edgar

[Qemu-devel] Re: [PATCH] PPC4xx: don't unregister RAM at reset

[Alexander Graf]

On 30.07.2010, at 03:48, Hollis Blanchard wrote:

> The PowerPC 4xx SDRAM controller emulation unregisters RAM in its reset
> callback. However, qemu_system_reset() is now called at initialization
> time, so RAM is unregistered before starting the guest.

So the registration should be moved to reset now, no? How is the reset different from boot? How did a reset work before?


Alex

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

[Hollis Blanchard]

On Sun, Aug 1, 2010 at 5:36 AM, Edgar E. Iglesias
<edgar.iglesias@gmail.com> 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 <hollis@penguinppc.org>
>> > ---
>> >  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).

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
could argue it's not *possible* to do that accurately), that logic
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
tell me: where should I hardcode initrd loading?

Anyways, you don't even use load_uimage() in microblaze, and if you
did, you wouldn't be obligated to use the "size" return value, so
fixing this issue for everybody else doesn't limit you at all.

-Hollis

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

[Edgar E. Iglesias]

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
> <edgar.iglesias@gmail.com> 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 <hollis@penguinppc.org>
> >> > ---
> >> >  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. BTW, qemu-arm
seems to follow a convention to place initrd 8Mb above RAM base, it
doesn't look at the loaded uimage size when deciding where to place
initrd.


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


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

Take a look at how arm does it in hw/arm_boot.c. CRIS doesn't have
uimage support now, but if it had It would probably do whatever magic
was needed in it's dedicated boot loader file, hw/cris-boot.c.
Microblaze has uimage support, look in hw/petalogix_s3adsp1800_mmu.c.

Maybe we should consider adding a ppc-boot.c with boot-loader magics?

Cheers,
Edgar

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
<edgar.iglesias@gmail.com> 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
>> <edgar.iglesias@gmail.com> 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 <hollis@penguinppc.org>
>> >> > ---
>> >> >  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

[Qemu-devel] Re: [PATCH] PPC4xx: don't unregister RAM at reset

[Hollis Blanchard]

On Mon, Aug 2, 2010 at 1:41 AM, Alexander Graf <agraf@suse.de> wrote:
>
> On 30.07.2010, at 03:48, Hollis Blanchard wrote:
>
>> The PowerPC 4xx SDRAM controller emulation unregisters RAM in its reset
>> callback. However, qemu_system_reset() is now called at initialization
>> time, so RAM is unregistered before starting the guest.
>
> So the registration should be moved to reset now, no? How is the reset different from boot? How did a reset work before?

As far as I can tell, no other platform unregisters and re-registers
memory at reset, so that is a difference between reset and boot.

Maybe I don't understand your other question. Before
qemu_system_reset() was called at initialization time, memory was not
unregistered, and therefore the platform had memory and could boot.

-Hollis

[Qemu-devel] Re: [PATCH] PPC4xx: don't unregister RAM at reset

[Alexander Graf]

On 02.08.2010, at 21:37, Hollis Blanchard wrote:

> On Mon, Aug 2, 2010 at 1:41 AM, Alexander Graf <agraf@suse.de> wrote:
>> 
>> On 30.07.2010, at 03:48, Hollis Blanchard wrote:
>> 
>>> The PowerPC 4xx SDRAM controller emulation unregisters RAM in its reset
>>> callback. However, qemu_system_reset() is now called at initialization
>>> time, so RAM is unregistered before starting the guest.
>> 
>> So the registration should be moved to reset now, no? How is the reset different from boot? How did a reset work before?
> 
> As far as I can tell, no other platform unregisters and re-registers
> memory at reset, so that is a difference between reset and boot.
> 
> Maybe I don't understand your other question. Before
> qemu_system_reset() was called at initialization time, memory was not
> unregistered, and therefore the platform had memory and could boot.

Then removal of the unregister is sane and the reset path was broken before. That's good to know :).

Alex

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

[Edgar E. Iglesias]

On Mon, Aug 02, 2010 at 12:33:54PM -0700, Hollis Blanchard wrote:
> On Mon, Aug 2, 2010 at 11:57 AM, Edgar E. Iglesias
> <edgar.iglesias@gmail.com> 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
> >> <edgar.iglesias@gmail.com> 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 <hollis@penguinppc.org>
> >> >> > ---
> >> >> >  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

Are we looking at the same code?

Grep for load_uimage in qemu. 4 archs use it, PPC, ARM, m68k and MB.
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?


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


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

Cheers

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
<edgar.iglesias@gmail.com> 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

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

[Richard Henderson]

On 08/02/2010 01:35 PM, Hollis Blanchard wrote:
>> 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. :)

A 1/16 factor is not "accounting", it's guessing i.e. magic.

I'm not sure why the u-image format doesn't include the size
of the bss, but my guess is that it's expecting the kernel to
zero its bss segment at startup.  File a bug with either the
kernel and/or the u-boot project if you like.


r~