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

[Stefan Tauner]

On Sun, 12 Jun 2011 02:15:34 +0200
Carl-Daniel Hailfinger  wrote:
> Am 08.06.2011 04:55 schrieb Stefan Tauner:
> > based on the work of Matthias 'mazzoo' Wenzel this patch adds...
> > pretty printing of those ICH/PCH flash descriptors that are cached/mapped by the chipset
> > (and which are therefore reachable via FDOC/FDOD registers) and
> > a new tool to the util directory that is able to decode all flash descriptors stored in a flash
> > dump file.
> >
> > Signed-off-by: Stefan Tauner
>
> A slightly more verbose explanation would be nice.

regarding the topic "flash descriptors" in general or the
implementation, or both?
> That said, I'll comment anyway so you have something to work with.

thanks a lot, this is exactly what (even more than) i was hoping for in
a first review. for such big patches it would be most favorable to
discuss early and often imho.
> Please replace "illegal" with "invalid" everywhere.

hm. intel uses "reserved" only afaics. sometimes they leave out a few
possible permutations of bits though. if pressed i would replace
everything with semantic of reserved/invalid/illegal to "reserved". is
that also ok for you?
> > +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.
i am not saying that this would be against any standard though... c has
undefined behavior lurking behind every corner ;)
(a good read for example is:
http://blog.llvm.org/2011/05/what-every-c-programmer-should-know.html).

same applies probably to most other bounds, but i have not checked this
yet, due to the open decision about using structs for decoding at all.

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.
> > }
> > /* 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).
> > +enum chipset {
> > CHIPSET_ICH8,
> > CHIPSET_ICH9,
> > CHIPSET_ICH10,
> > CHIPSET_SERIES_5_IBEX_PEAK,
> > CHIPSET_SERIES_6_COUGAR_POINT,
> > CHIPSET_SERIES_7_PANTHER_POINT
> >
> > +};
>
> Are those all possible chipsets?

i think these are all possible chipset _series_. as you know intel
splits them in many versions with different pci ids, but this is not
important for the descriptors and so i used this for now. depending on
the issue of how to decide which one we use, there will probably be an
additional pci ids, parameter name, enum chipset array or something
similar + decoding functions... let's see how the rest turns out.
> > +struct flash_descriptor_addresses {
> > uint32_t (*FCBA)(void);
> > uint32_t (*FRBA)(void);
> > uint32_t (*FMBA)(void);
> > uint32_t (*FMSBA)(void);
> > uint32_t (*FLREG_limit)(uint32_t flreg);
> > uint32_t (*FLREG_base)(uint32_t flreg);
> > uint32_t (*FISBA)(void);
> >
> > +#ifdef ICH_DESCRIPTORS_FROM_MMAP_DUMP
> > uint32_t (*VTBA)(void);
> >
> > +#endif // ICH_DESCRIPTORS_FROM_MMAP_DUMP
> > +};
> >
> > +struct flash_descriptor {
> > uint32_t FLVALSIG;	/* 0x00 */
> > union {			/* 0x04 */
> > uint32_t FLMAP0;
> > struct {
> > 	uint8_t FCBA	:8;
> > 	uint8_t NC	:2;
> > 	unsigned	:6;
> > 	uint8_t FRBA	:8;
> > 	uint8_t NR	:3;
> > 	unsigned	:5;
> > };
> > };
> > union {			/* 0x08 */
> > uint32_t FLMAP1;
> > struct {
> > 	uint8_t FMBA	:8;
> > 	uint8_t NM	:3;
> > 	unsigned	:5;
> > 	union {
> > 		uint8_t FISBA	:8;
> > 		uint8_t FPSBA	:8;
> > 	};
> > 	uint8_t ISL	:8;
> > };
> > };
> > union {			/* 0x0c */
> > uint32_t FLMAP2;
> > struct {
> > 	uint8_t  FMSBA	:8;
> > 	uint8_t  MSL	:8;
> > 	unsigned	:16;
> > };
> > };
> >
> > +};
>
> This won't fly. At the very least, it needs __attribute__((packed)), but
> even then I see pretty explosions because the bitfield ordering is
> implementation defined and I expect this to do the wrong thing if anyone
> ever runs that code on bigendian hosts (looking at a file is possible
> even if you don't have ICH SPI hardware). Heck, each gcc version could
> do something else.
> I might have misunderstood C99, so feel free to correct me.

you are totally right that this will explode on big endian machines.
i think this is not really a problem because of widespread use of x86
and based on the assumption that one would not be interested in Intel
flash descriptors without having more than one x86 around... but if
that is an issue, it has to be changed.
this could still be done with structs, but with additional ifdefs.
the reason i chose this method is that it seems to be fairly well
supported by compilers and is much easier to read and write.
the only correct way according to the standard to do this kind of stuff
(as far as i could find out) is with bitmasks and lots of shifting, but
it seemed structs are recommended by many (in the case only one
endianess is used) over the use of bitmasks and shifting.
i would not use this for anything that should be platform independent,
but in this case i think it is ok. i am ready for being crucified
though. ;)

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?
> > +#ifdef ICH_DESCRIPTORS_FROM_MMAP_DUMP
> > +struct flash_strap {
> > union {
> >
> > […]
> > union {
> > 	uint32_t STRPs[15];
> > 	struct {
> > 		union {
> > 			uint32_t STRP0;
> > 			struct {
> > 				unsigned			:1;
> > 				uint8_t  cs_ss2			:1;
> > 				unsigned			:5;
> > 				uint8_t  SMB_EN			:1;
> > 				uint8_t  SML0_EN		:1;
> > 				uint8_t  SML1_EN		:1;
> > 				uint8_t  SML1FRQ		:2;
> > 				uint8_t  SMB0FRQ		:2;
> > 				uint8_t  SML0FRQ		:2;
> > 				unsigned			:4;
> > 				uint8_t  LANPHYPC_GP12_SEL	:1;
> > 				uint8_t  cs_ss1			:1;
> > 				unsigned			:2;
> > 				uint8_t  DMI_REQID_DIS		:1;
> > 				unsigned			:4;
> > 				uint8_t  BBBS			:2;
> > 				unsigned			:1;
> > 			};
> > 		};
> > 		union {
> > 			uint32_t STRP1;
> > 			struct {
> > 			};
>
> 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.