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@gmx.net 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 stefan.tauner@student.tuwien.ac.at
A slightly more verbose explanation would be nice.
regarding the topic "flash descriptors" in general or the implementation, or both?
Good question. Can you add a few sentences to the commit message about which of the flash descriptors are only available if you have read access to the flash image, and which of them are purely available via FDOC/FDOD (if any)? And how should we handle the case where the descriptor region of the flash chip is not readlocked, so that information would be available?
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?
Sure. I think I once saw some Intel datasheets which mentioned some settings as forbidden (not reserved), e.g. if two settings in different registers conflicted. Not sure if that is the case here. Anyway, unless Intel explicitly calls a setting illegal/invalid, just call it reserved. Side note: Some invalid/reserved settings get new meanings in later hardware revisions, so "reserved" is indeed a good choice if possible.
+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).
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).
I knew that llvm blog series, and that's why I'm really cautious about bitfield and union fun.
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.
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?
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.
- }
- /* 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.
+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_.
Group them into series/whatever with identical descriptor interpretations and add a comment explaining that.
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.
Yes,
+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. ;)
Comments from a C expert please! I don't know enough to speak ex cathedra.
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.
+#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.
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)?
Regards, Carl-Daniel