[flashrom] [PATCH 07/12] add ICH/PCH flash descriptor decoding

[Stefan Tauner]

On Thu, 14 Jul 2011 01:17:50 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Am 12.06.2011 09:41 schrieb Stefan Tauner:
> > On Sun, 12 Jun 2011 02:15:34 +0200
> > Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:
> >
> >   
> >> Am 08.06.2011 04:55 schrieb Stefan Tauner:
> >> 
> >>> +void pretty_print_ich_descriptor_component(void)
> >>> +{
> >>> +	const char * str_freq[8] = {
> >>> +		"20 MHz",		/* 000 */
> >>> +		"33 MHz",		/* 001 */
> >>> +		"reserved/illegal",	/* 010 */
> >>> +		"reserved/illegal",	/* 011 */
> >>> +		"50 MHz",		/* 100 */
> >>> +		"reserved/illegal",	/* 101 */
> >>> +		"reserved/illegal",	/* 110 */
> >>> +		"reserved/illegal"	/* 111 */
> >>> +	};
> >>> +	const char * str_mem[8] = {
> >>> +		"512kB",
> >>> +		"1 MB",
> >>> +		"2 MB",
> >>> +		"4 MB",
> >>> +		"8 MB",
> >>> +		"16 MB",
> >>> +		"undocumented/illegal",
> >>> +		"reserved/illegal"
> >>> +	};
> >>> +
> >>> +	msg_pdbg("\n");
> >>> +	msg_pdbg("=== FCBA ===\n");
> >>> +	msg_pdbg("FLCOMP   0x%8.8x\n", fcba.FLCOMP);
> >>> +	msg_pdbg("FLILL    0x%8.8x\n", fcba.FLILL );
> >>> +	msg_pdbg("\n");
> >>> +	msg_pdbg("--- FCBA details ---\n");
> >>> +	msg_pdbg("0x%2.2x    freq_read_id   %s\n",
> >>> +		fcba.freq_read_id , str_freq[fcba.freq_read_id ]);
> >>>   
> >>>       
> >> Out-of-bounds access if fcba.freq_read_id>=8
> >>     
> > hm. well if that happens then the compiler is REALLY doing something
> > unexpected because .freq_read_id is defined to be 3 (unsigned) bits
> > wide.
> >   
> 
> Hm yes. A comment (/* freq_read_id has 3 bits */) or something to that
> effect would be nice and improve code auditability (no need to look up
> stuff elsewhere).

after the changes i have done i am no longer sure this is a good idea.
they dont change anything directly related, but commenting this seems
very redundant if you look at the patch as a whole. it would be
required in quite some places although the definition is only one click
away. this is quite easier to review than for example do_ich9_spi_frap,
which uses an argument to index into similar arrays and does no check
whatsoever.
if you insist though, i can add them anyway.
 
> Can we add some selftest during flashrom startup which checks for one or
> two special hand-made similar structs if it worked and complains about a
> compiler problem otherwise? That would allow using structs with all
> benefits and without the risk.
> 
> To be honest, I had hoped some C expert would chime in and lecture us
> about how to do this correctly.
> Michael?

i could use the structs we introduce with this patch in the startup
checking code.
i would do two tests:
writing to big fields and verify that reading out the matching smaller
fields in the same range are correct, and vice-versa.

we could also add such a check to the makefile tests, which is probably
better because it is a compile-time/compiler check not a runtime check?

side note: doesnt that apply to other startup checks too?
 
> 
> > in some cases we should certainly check the values and this is in the
> > case of redirection; i.e. when we look up an offset to another field in
> > some descriptor register which could potentially be garbage.
> >   
> 
> Indeed.

done. this was quite some work to get the checked boundaries correct
and tied. now i know why there are so many overflow errors out there...
not just because everyone is lazy. :)

> >>> +	}
> >>> +	/* straps */
> >>> +	/* FIXME: detect chipset correctly */
> >>> +	switch (CHIPSET_SERIES_5_IBEX_PEAK) {
> >>>   
> >>>       
> >> WTF?
> >>     
> > yes. :)
> > there is afaik no way to distinguish between the chipsets from the
> > descriptor alone. this is not a big problem for flashrom. it can deduce
> > this from the pci ids of the system. but for the stand alone utility it
> > is a problem.
> > we could add pci id detection directly here but this would make it
> > necessary to either include flashrom's pci stuff (which - from a quick
> > look - is not as good modularized as i would like it be for this case,
> > i have not tried yet to use it though) or to force the user to supply
> > the chipset to use (maybe with a safe - if there is one - default).
> >   
> 
> Parsing a file should be hardware-independent. Heck, if someone wants to
> use a non-PCI ARM machine that should still work fine. Requiring a
> command line argument is absolutely fine. Use the enum below, and inside
> flashrom map PCI IDs to that enum, and in the separate tool map command
> line parameters to that enum.

inside flashrom we don't need to know (yet), so there is no problem...
i have added a parameter to the tool, that selects the chipset.

> > regarding packing:
> >   
> >> If enough space remains, a bit-field that immediately follows another
> >> bit-field in a structure shall be packed into adjacent bits of the same 
> >> unit.
> >> If insufficient space remains, whether a bit-field that does not fit is
> >> put into the next unit or overlaps adjacent units is
> >> implementation-defined.
> >>     
> > so i think if i use uint32_t for all (overlapping) fields it should be
> > safe?
> >   
> 
> As long as a field does not span two different uint32_t, it should be
> fine, yes.

fixed.
     
> >> Why an empty struct?
> >>     
> > not done yet. i wanted feedback first ;)
> > this information is not available publicly btw.
> > apart from what mazzoo has supplied, i do only have the data for ibex
> > peak/5 series.
> >   
> 
> Oh, that's unfortunate. I should ask my Intel contacts if there is
> anything they might be able to share legally. Can you ping me about that
> in a few days (I hope to find the right people by then)?

ping! :)

also what about the authors of the MEI kernel module? i have not
messaged them because i presume they won't be allowed to help us anyway
and did not want to bother them. but since i have written that
letter/blog thingy now i could just spam them anyway. i would like to
hear your opinion on this before i do so because you brought it up
lately yourself...

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner