Re: [Qemu-devel] [PATCH] loader: pad kernel size when loaded from a uImage
On Sun, Aug 1, 2010 at 5:36 AM, Edgar E. Iglesias edgar.igles...@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 hol...@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 -- To unsubscribe from this list: send the line unsubscribe kvm-ppc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH] loader: pad kernel size when loaded from a uImage
On Mon, Aug 2, 2010 at 11:57 AM, Edgar E. Iglesias edgar.igles...@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.igles...@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 hol...@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
Re: [Qemu-devel] [PATCH] loader: pad kernel size when loaded from a uImage
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.igles...@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.igles...@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 hol...@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
Re: [Qemu-devel] [PATCH] loader: pad kernel size when loaded from a uImage
On Mon, Aug 2, 2010 at 12:56 PM, Edgar E. Iglesias edgar.igles...@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); 2233 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, 5277 hw/mcf5208.c mcf5208evb_init kernel_size = load_uimage(kernel_filename, entry, NULL, NULL); 6121 hw/ppc440_bamboo.c bamboo_init kernel_size = load_uimage(kernel...ename, entry, loadaddr, NULL); 7235 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 -- To unsubscribe from this list: send the line unsubscribe kvm-ppc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH] loader: pad kernel size when loaded from a uImage
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 hol...@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 -- To unsubscribe from this list: send the line unsubscribe kvm-ppc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH] loader: pad kernel size when loaded from a uImage
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 hol...@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:av1...@comtv.ru -- To unsubscribe from this list: send the line unsubscribe kvm-ppc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH] loader: pad kernel size when loaded from a uImage
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 hol...@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 -- To unsubscribe from this list: send the line unsubscribe kvm-ppc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
