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

[Robert Millan]

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