[flashrom] [coreboot] [PATCH] superiotool: libsuperiodetect

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Wed Jun 30 16:16:54 CEST 2010


On 30.06.2010 14:51, Stefan Reinauer wrote:
> On 6/30/10 2:35 PM, Carl-Daniel Hailfinger wrote:
>   
>> Using the superiotool code to detect SuperI/O chips can be really useful
>> even for applications besides superiotool, e.g. flashrom.
>> The biggest hurdle right now is the excessive size of the superiotool
>> binary (>2.2 MB) which is still ~15 kB after lzma compression. That's
>> simply unacceptable for a library.
>>   
>>     
> Why? There is no size constraints on libraries in Unix like systems.
>   

Yes, but if I proposed to increase the size of the flashrom binary by
over 2 MBytes, various people would ask if I'm crazy.


> Assuming we're talking about a static library
> >From my system:
>
> 20M /usr/lib64/libbfd.a
> 20M /usr/lib64/libc.a
> 11M /usr/lib64/libcrypto.a
> 19M /usr/lib64/libdb-4.5.a
> 3.9M /usr/lib64/libfreetype.a
> 5.1M /usr/lib64/libgio-2.0.a
> 3.6M /usr/lib64/libglib-2.0.a
> 3.3M /usr/lib64/libgmp.a
> 2.8M /usr/lib64/libm.a
> 2.8M /usr/lib64/libncurses.a
> 3.2M /usr/lib64/libncursesw.a
> 4.9M /usr/lib64/libopcodes.a
> 4.2M /usr/lib64/libruby-static.a
> 3.0M /usr/lib64/libssl.a
> 6.9M /usr/lib64/libxml2.a
>
> Also, keep in mind that the size of the library in the filesystem is not
> the size increase when linking it.
>   

However, if all objects in the library are used, the size increase of
the binary is correlated closely to the linked static library size.


> 15kb lzma compressed is nothing.
>   

8 kB more or less in a recovery payload of a coreboot image are not
negligible IMHO.

In normal OS environments people don't use compressed binaries, and
there a 2 MB size increase for flashrom (98% of it for dead code which
can't be optimized away by the compiler/linker) is definitely something
people will object to.


>> This minimally invasive patch makes all dumping code optional and
>> reduces binary size for the probe-only case (libsuperiodetect) a lot.
>> Binary sizes:
>> 2268258 (before)
>>   31600 (after) 98.6% reduction
>> LZMA compressed and stripped binary sizes:
>> 14778 (before)
>>  6709 (after) 54.6% reduction
>>   
>>     
> Not worth the additional unreadability of the code in my opinion
>
>   
>> To test compilation of superiotool with probe-only functionality, run
>> make CONFIG_LIB=yes
>>
>> This patch is the first step towards libsuperiodetect, and more will
>> have to follow.
>>   
>>     
> I think the idea for libsuperio is great.
>
> I'll gladly ack this without the very intrusive "we save 6700 bytes"
> part of the patch.
>   

The patch is a pure size reduction. If I remove the size reduction part,
the patch will be empty.
I can reduce the number of #ifdefs, but that won't make the patch
prettier. Will send the patch with less #ifdefs in a separate mail.


>> IMHO the {EOT} markers should be killed in libsuperiodetect, at least
>> for the LDNs. Such an operation would have increased the size of the
>> patch and would have reduced readability as well. Due to that, I skipped
>> it, but I plan to do that in a later operation.
>>   
>>     
> What would you use instead? The array members all have different sizes
> as far as I can tell, so some kind of end marker will be needed.
>   

The libsuperiodetect case doesn't care about LDNs at all. Having exactly
one LDN definition per Super I/O which is always EOT looks like dead
code to me, especially if nothing is looking at the LDN.


>> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
>>   
>>     
>
>   
>>  		/* Init: 0x55, 0x55. Exit: 0xaa. Port: 0x3f0. */
>> @@ -765,7 +803,9 @@
>>  				     uint8_t revreg)
>>  {
>>  	uint8_t id, rev;
>> +#ifndef LIBSUPERIODETECT
>>  	uint16_t runtime_base;
>> +#endif /* ! LIBSUPERIODETECT */
>>   
>>     
> Maybe this could be moved downwards to where it is used, to save an ifndef?
>   

IIRC this would break on some compilers which don't allow variable
declarations in the middle of some code.


>> Index: superiotool_libsuperiodetect/superiotool.h
>> ===================================================================
>> --- superiotool_libsuperiodetect/superiotool.h	(Revision 5651)
>> +++ superiotool_libsuperiodetect/superiotool.h	(Arbeitskopie)
>> @@ -98,12 +98,19 @@
>>  struct superio_registers {
>>  	int32_t superio_id;		/* Signed, as we need EOT. */
>>  	const char *name;		/* Super I/O name */
>> +#ifndef LIBSUPERIODETECT
>>  	struct {
>>  		int8_t ldn;
>>  		const char *name;	/* LDN name */
>>  		int16_t idx[IDXSIZE];
>>  		int16_t def[IDXSIZE];
>>  	} ldn[LDNSIZE];
>> +#else /* LIBSUPERIODETECT */
>> +	/* Ugly hack to avoid messing with EOT markers. */
>> +	struct {
>> +		int8_t dummy;
>> +	} dummy[1];
>> +#endif /* LIBSUPERIODETECT */
>>  }; 
>>   
>>     
> hm ... the comment is very terse.
>   

struct dummy is there to avoid putting all those LDN {EOT} markers
inside #ifndef LIBSUPERIODETECT. It is a hack to allow compilation and
make the patch less invasive than it would have been without struct dummy.


Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/





More information about the flashrom mailing list