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

[Carl-Daniel Hailfinger]

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
> > /* 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