Re: [PATCH] LZMA support in i386-pc kernel

[Robert Millan <rmh@aybabtu.com>]

On Wed, Jul 02, 2008 at 09:39:52PM +0800, Bean wrote:
> Hi,
> 
> This patch add support for lzma decompression. The assembly code
> lzma_decode.S is manually optimized to reduce size. The result decoder
> is tiny, only 416 bytes longer than the lzo version.
> 
> I also include lzma encode from the LZMA SDK. grub needs to use the
> ANSI-C version of encoder, which is only present in the latest 4.58
> beta. I can't find ready to use shared library in most distro.
> Including the encoder/decoder has advantages as well. We don't need to
> worry about the host os, and lzma encoder/decoder can be used in other
> place, like font compression.

Lenny's most likely going to ship with lzma 4.43.  Is that good?

> Result of
> ./grub-mkimage -o core.img biosdisk pc ext2 lvm raid
> 
> lzma version: 27,776 bytes
> lzo version: 32,768 bytes

Amazing.  I'd really like this to make it in time for lenny;  although we're
really pressed for time now.

> diff --git a/include/grub/lib/LzFind.h b/include/grub/lib/LzFind.h
> new file mode 100644
> index 0000000..3d74d0c
> --- /dev/null
> +++ b/include/grub/lib/LzFind.h
> @@ -0,0 +1,116 @@
> +/* LzFind.h  -- Match finder for LZ algorithms
> +2008-04-04
> +Copyright (c) 1999-2008 Igor Pavlov

Have you modified the files from LZMA SDK?  Please add our license boilerplate
to them if you did (e.g. like in kern/i386/pc/lzo1x.S).

> +#define FIXED_PROPS
> [...]
> +#ifdef FIXED_PROPS

What's this option for?  Some comment would be nice.

> +#if 0
> +
> +DbgOut:

This is just for debugging, right?  Maybe "#ifdef DEBUG" or so would be
better, to make it clear.

> +#ifndef ASM_FILE
> +	xorl	%eax, %eax
> +#endif

Why?  I thought ASM_FILE always ought to be defined for asm files in GRUB.

> diff --git a/kern/i386/pc/startup.S b/kern/i386/pc/startup.S
> index 9542978..964643a 100644
> --- a/kern/i386/pc/startup.S
> +++ b/kern/i386/pc/startup.S
> @@ -200,6 +200,7 @@ codestart:
>  	incl	%eax
>  	call	EXT_C(grub_gate_a20)
>  	
> +#if defined(ENABLE_LZO)
>  	/* decompress the compressed part and put the result at 1MB */
>  	movl	$GRUB_MEMORY_MACHINE_DECOMPRESSION_ADDR, %esi
>  	movl	$(START_SYMBOL + GRUB_KERNEL_MACHINE_RAW_SIZE), %edi
> @@ -213,6 +214,23 @@ codestart:
>  	/* copy back the decompressed part */
>  	movl	%eax, %ecx
>  	cld
> +#else
> +	movl	$GRUB_MEMORY_MACHINE_DECOMPRESSION_ADDR, %edi
> +	movl	$(START_SYMBOL + GRUB_KERNEL_MACHINE_RAW_SIZE), %esi
> +	pushl	%edi
> +	pushl	%esi
> +	movl	EXT_C(grub_kernel_image_size), %ecx
> +	addl	EXT_C(grub_total_module_size), %ecx
> +	addl	EXT_C(grub_memdisk_image_size), %ecx
> +	subl	$GRUB_KERNEL_MACHINE_RAW_SIZE, %ecx
> +	pushl	%ecx
> +	leal	(%edi, %ecx), %ebx
> +	call	_LzmaDecodeA
> +	popl	%ecx
> +	popl	%edi
> +	popl	%esi
> +#endif

I suggest having this explicit, like

#elif defined(ENABLE_LZMA)
[...]
#else
#error blah
#endif

> +#if defined(ENABLE_LZO)
>  #include "lzo1x.S"
> +#else
> +#include "lzma_decode.S"
> +#endif

Same here (and in grub-mkimage).


Nice work!

 
-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)