| Message ID | 1303272904-31392-3-git-send-email-nicolas.pitre@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
* Nicolas Pitre <nicolas.pitre@linaro.org> [110420 07:12]: > The return value for decompress_kernel() is no longer used. Furthermore, > this was obtained and stored in a variable called output_ptr which is > a complete misnomer for what is actually the size of the decompressed > kernel image. Let's get rid of it. > > Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org> Works for me: Tested-by: Tony Lindgren <tony@atomide.com> > BTW, I've yet to understand why, but this patch makes CONFIG_KERNEL_LZMA > work for me. Maybe the compressed image size changes slightly so the compressed image end no longer goes past the uncompressed kernel end and relocate works properly? Maybe see if the patch I posted solves your LZMA issue even without this patch? Tony
On Wed, Apr 20, 2011 at 12:15:03AM -0400, Nicolas Pitre wrote: > The return value for decompress_kernel() is no longer used. Furthermore, > this was obtained and stored in a variable called output_ptr which is > a complete misnomer for what is actually the size of the decompressed > kernel image. Let's get rid of it. > > Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org> > --- > > BTW, I've yet to understand why, but this patch makes CONFIG_KERNEL_LZMA > work for me. Hmm. I've switched almost entirely to LZMA since it was introduced and it works for me on all my platforms. I haven't tested kernels rigorously since your patches to the decompressor, so maybe there's a regression in there somewhere. Can you try LZMA with your various patches over the last four months reverted?
On Wed, 27 Apr 2011, Russell King - ARM Linux wrote: > On Wed, Apr 20, 2011 at 12:15:03AM -0400, Nicolas Pitre wrote: > > The return value for decompress_kernel() is no longer used. Furthermore, > > this was obtained and stored in a variable called output_ptr which is > > a complete misnomer for what is actually the size of the decompressed > > kernel image. Let's get rid of it. > > > > Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org> > > --- > > > > BTW, I've yet to understand why, but this patch makes CONFIG_KERNEL_LZMA > > work for me. > > Hmm. I've switched almost entirely to LZMA since it was introduced and it > works for me on all my platforms. I haven't tested kernels rigorously since > your patches to the decompressor, so maybe there's a regression in there > somewhere. > > Can you try LZMA with your various patches over the last four months > reverted? Yes I did, and the issue turned up to be about the stack alignment wich was not enforced to a 64 bit boundary. Depending on the size of the code and compressed kernel data, the stack would end up properly aligned with a 50% probability. And this was a problem only if your gcc version emited LDRD/STRD type accesses to the stack. I'm about to send you a pull request with all those fixes. Nicolas
On Wed, Apr 27, 2011 at 10:48:07AM -0400, Nicolas Pitre wrote: > On Wed, 27 Apr 2011, Russell King - ARM Linux wrote: > > > On Wed, Apr 20, 2011 at 12:15:03AM -0400, Nicolas Pitre wrote: > > > The return value for decompress_kernel() is no longer used. Furthermore, > > > this was obtained and stored in a variable called output_ptr which is > > > a complete misnomer for what is actually the size of the decompressed > > > kernel image. Let's get rid of it. > > > > > > Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org> > > > --- > > > > > > BTW, I've yet to understand why, but this patch makes CONFIG_KERNEL_LZMA > > > work for me. > > > > Hmm. I've switched almost entirely to LZMA since it was introduced and it > > works for me on all my platforms. I haven't tested kernels rigorously since > > your patches to the decompressor, so maybe there's a regression in there > > somewhere. > > > > Can you try LZMA with your various patches over the last four months > > reverted? > > Yes I did, and the issue turned up to be about the stack alignment wich > was not enforced to a 64 bit boundary. Depending on the size of the > code and compressed kernel data, the stack would end up properly aligned > with a 50% probability. And this was a problem only if your gcc version > emited LDRD/STRD type accesses to the stack. > > I'm about to send you a pull request with all those fixes. Please first resend the entire set of patches for review - I've no idea what the set of patches contain having read through most of the emails on the decompressor which were sent over Easter. I really don't like the idea of getting rid of the 'sp' for the cache flush and stamping over a load of registers instead. I think that's storing up problems for the future.
* Nicolas Pitre <nicolas.pitre@linaro.org> [110427 07:44]: > On Wed, 27 Apr 2011, Russell King - ARM Linux wrote: > > > On Wed, Apr 20, 2011 at 12:15:03AM -0400, Nicolas Pitre wrote: > > > The return value for decompress_kernel() is no longer used. Furthermore, > > > this was obtained and stored in a variable called output_ptr which is > > > a complete misnomer for what is actually the size of the decompressed > > > kernel image. Let's get rid of it. > > > > > > Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org> > > > --- > > > > > > BTW, I've yet to understand why, but this patch makes CONFIG_KERNEL_LZMA > > > work for me. > > > > Hmm. I've switched almost entirely to LZMA since it was introduced and it > > works for me on all my platforms. I haven't tested kernels rigorously since > > your patches to the decompressor, so maybe there's a regression in there > > somewhere. > > > > Can you try LZMA with your various patches over the last four months > > reverted? > > Yes I did, and the issue turned up to be about the stack alignment wich > was not enforced to a 64 bit boundary. Depending on the size of the > code and compressed kernel data, the stack would end up properly aligned > with a 50% probability. And this was a problem only if your gcc version > emited LDRD/STRD type accesses to the stack. > > I'm about to send you a pull request with all those fixes. Just to summarize, looks like there are three regressions: 1. The stack alignment mentioned above 2. Stack overwriting relocated kernel with cache flush in some cases on ARMv7 3. Reloction overwriting running code when the relocate offset is small These fixes would be nice to get merged in the -rc cycle. Then the other patches like the static usage in uncompress.h can probably wait until the merge window. Regards, Tony
On Thu, 28 Apr 2011, Tony Lindgren wrote: > Just to summarize, looks like there are three regressions: > > 1. The stack alignment mentioned above This one has been there forever. I marked it for inclusion into the stable tree. > 2. Stack overwriting relocated kernel with cache flush in some cases on ARMv7 Yes, and I reverted to your original patch. I think I agree with RMK about the fact that having a stack available is more sane than trying to squeeze everything into registers (including sp as my patch did). > 3. Reloction overwriting running code when the relocate offset is small Yep. However I agree with you that having a constant like my patch did is prone to failure if the code grows big enough (unlikely but still). However your usage of the GOT end is overshooting as only the copy loop and cache flush code has to be protected. I have an alternative patch using an explicit symbol to mark critical code. > These fixes would be nice to get merged in the -rc cycle. Then the > other patches like the static usage in uncompress.h can probably wait > until the merge window. Right. I have sorted them so Russell can send a pointer to the first part to Linus and keep the rest for later. I'll repost the whole set soon anyway. Nicolas
* Nicolas Pitre <nicolas.pitre@linaro.org> [110428 11:17]: > On Thu, 28 Apr 2011, Tony Lindgren wrote: > > > Just to summarize, looks like there are three regressions: > > > > 1. The stack alignment mentioned above > > This one has been there forever. I marked it for inclusion into > the stable tree. Right, not a regression, but a nasty bug. > > 2. Stack overwriting relocated kernel with cache flush in some cases on ARMv7 > > Yes, and I reverted to your original patch. I think I agree with RMK > about the fact that having a stack available is more sane than trying to > squeeze everything into registers (including sp as my patch did). OK > > 3. Reloction overwriting running code when the relocate offset is small > > Yep. However I agree with you that having a constant like my patch did > is prone to failure if the code grows big enough (unlikely but still). > However your usage of the GOT end is overshooting as only the copy loop > and cache flush code has to be protected. I have an alternative patch > using an explicit symbol to mark critical code. Cool saw that, yeah that's a nice way of fixing it. > > These fixes would be nice to get merged in the -rc cycle. Then the > > other patches like the static usage in uncompress.h can probably wait > > until the merge window. > > Right. I have sorted them so Russell can send a pointer to the first > part to Linus and keep the rest for later. I'll repost the whole set > soon anyway. Great & thanks for tracking down these issues. Tony
diff --git a/arch/arm/boot/compressed/misc.c b/arch/arm/boot/compressed/misc.c index 2df3826..a565853 100644 --- a/arch/arm/boot/compressed/misc.c +++ b/arch/arm/boot/compressed/misc.c @@ -26,8 +26,6 @@ unsigned int __machine_arch_type; #include <linux/linkage.h> #include <asm/string.h> -#include <asm/unaligned.h> - static void putstr(const char *ptr); extern void error(char *x); @@ -139,13 +137,12 @@ void *memcpy(void *__dest, __const void *__src, size_t __n) } /* - * gzip delarations + * gzip declarations */ extern char input_data[]; extern char input_data_end[]; unsigned char *output_data; -unsigned long output_ptr; unsigned long free_mem_ptr; unsigned long free_mem_end_ptr; @@ -173,13 +170,11 @@ asmlinkage void __div0(void) extern void do_decompress(u8 *input, int len, u8 *output, void (*error)(char *x)); -unsigned long +void decompress_kernel(unsigned long output_start, unsigned long free_mem_ptr_p, unsigned long free_mem_ptr_end_p, int arch_id) { - unsigned char *tmp; - output_data = (unsigned char *)output_start; free_mem_ptr = free_mem_ptr_p; free_mem_end_ptr = free_mem_ptr_end_p; @@ -187,12 +182,8 @@ decompress_kernel(unsigned long output_start, unsigned long free_mem_ptr_p, arch_decomp_setup(); - tmp = (unsigned char *) (((unsigned long)input_data_end) - 4); - output_ptr = get_unaligned_le32(tmp); - putstr("Uncompressing Linux..."); do_decompress(input_data, input_data_end - input_data, output_data, error); putstr(" done, booting the kernel.\n"); - return output_ptr; }
The return value for decompress_kernel() is no longer used. Furthermore, this was obtained and stored in a variable called output_ptr which is a complete misnomer for what is actually the size of the decompressed kernel image. Let's get rid of it. Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org> --- BTW, I've yet to understand why, but this patch makes CONFIG_KERNEL_LZMA work for me. arch/arm/boot/compressed/misc.c | 13 ++----------- 1 files changed, 2 insertions(+), 11 deletions(-)