grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Bean
Subject: Re: [PATCH] LZMA support in i386-pc kernel
Date: Wed, 2 Jul 2008 23:11:28 +0800

On Wed, Jul 2, 2008 at 10:42 PM, Robert Millan <address@hidden> wrote:
> 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?

The c encoder first appears in 4.58 beta. Previous version of lzma
only have cpp version.

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

I change the location of some include files.

>
>> +#define FIXED_PROPS
>> [...]
>> +#ifdef FIXED_PROPS
>
> What's this option for?  Some comment would be nice.

lzma have three properties that control its behavior, lc, lp and pb.
We can use these values as variable or constant. The default value, 3,
0, 2 is nice in most case, we hardly need to change them. So I use it
as constant to save some extra space.

>
>> +#if 0
>> +
>> +DbgOut:
>
> This is just for debugging, right?  Maybe "#ifdef DEBUG" or so would be
> better, to make it clear.

Yes, this is used in debugging. Now it's working, it's not needed
anymore. I think it's better to keep it disable like this. The kernel
raw space is critical, if we happen to define DEBUG, the decoder code
would expand and kernel.img would fail to compile any more.

>
>> +#ifndef ASM_FILE
>> +     xorl    %eax, %eax
>> +#endif
>
> Why?  I thought ASM_FILE always ought to be defined for asm files in GRUB.

This is also used in debugging. Previously, I link it with c program
to check the decoder, which require to keep register like %esi, %edi,
%ebx. But inside grub, there is no need to keep them. I use ASM_FILE
to distinguish between these two conditions.

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

Sounds good to me.

-- 
Bean




reply via email to

[Prev in Thread] Current Thread [Next in Thread]