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

[Carl-Daniel Hailfinger]

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.
That said, I'll comment anyway so you have something to work with.
> diff --git a/Makefile b/Makefile
> index 6e6e2de..b61031f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -223,7 +223,7 @@ ifeq ($(CONFIG_INTERNAL), yes)
>  FEATURE_CFLAGS += -D'CONFIG_INTERNAL=1'
>  PROGRAMMER_OBJS += processor_enable.o chipset_enable.o board_enable.o cbtable.o dmi.o internal.o
>  # FIXME: The PROGRAMMER_OBJS below should only be included on x86.
> -PROGRAMMER_OBJS += it87spi.o it85spi.o ichspi.o sb600spi.o wbsio_spi.o mcp6x_spi.o
> +PROGRAMMER_OBJS += it87spi.o it85spi.o ichspi.o ich_descriptors.o sb600spi.o wbsio_spi.o mcp6x_spi.o

Maybe break this into multiple lines.
> NEED_PCI := yes
>  endif
>
> diff --git a/ich_descriptors.c b/ich_descriptors.c
> new file mode 100644
> index 0000000..0b03624
> --- /dev/null
> +++ b/ich_descriptors.c
> @@ -0,0 +1,453 @@
> +/* This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License version 2 as
> published by the Free Software Foundation.
> authors:
> (c) 2010  Matthias Wenzel <bios at mazzoo dot de>
> (c) 2011  Stefan Tauner

Please use the same format as in other files, i.e. don't only use "(c)"
but also add the word "Copyright". Rumor has it that even the order of
"(c)" vs. "Copyright" is important for US law. No idea if that is true,
but it can't hurt to do the same as everyone else.
> */
>
> +#include "ich_descriptors.h"
>
> +#if defined(__i386__) || defined(__x86_64__)
>
> +#ifdef ICH_DESCRIPTORS_FROM_MMAP_DUMP
>
> +#define DESCRIPTOR_MODE_MAGIC 0x0ff0a55a
> +#define msg_pdbg printf
> +#include <stdio.h>
>
> +struct flash_strap fisba;
> +struct flash_upper_map flumap;
>
> +#else
>
> +#include "flash.h" /* for msg_* */
>
> +#endif // ICH_DESCRIPTORS_FROM_MMAP_DUMP
>
> +struct flash_descriptor fdbar = { 0 };
> +struct flash_component fcba = { {0} };
> +struct flash_region frba = { {0} };
> +struct flash_master fmba = { {0} };
>
> +uint32_t getFCBA()

Please use foo(void) for function declarations/definitions if no
parameters are used.
Same comment applies for all other functions of the same type.
> +{
> return (fdbar.FLMAP0 <<  4) & 0x00000ff0;
>
> +}
>
> +uint32_t getFRBA()
> +{
> return (fdbar.FLMAP0 >> 12) & 0x00000ff0;
>
> +}
>
> +uint32_t getFMBA()
> +{
> return (fdbar.FLMAP1 <<  4) & 0x00000ff0;
>
> +}
>
> +uint32_t getFMSBA()
> +{
> return (fdbar.FLMAP2 <<  4) & 0x00000ff0;
>
> +}
>
> +uint32_t getFLREG_limit(uint32_t flreg)
> +{
> return (flreg >>  4) & 0x01fff000;
>
> +}
>
> +uint32_t getFLREG_base(uint32_t flreg)
> +{
> return (flreg << 12) & 0x01fff000;
>
> +}
>
> +uint32_t getFISBA()
> +{
> return (fdbar.FLMAP1 >> 12) & 0x00000ff0;
>
> +}
>
> +/** Returns the integer representation of the component density with index
> +comp in bytes or 0 if a correct size can not be determined. */
> +int getFCBA_component_density(uint8_t comp)
> +{
> uint8_t size_enc;
> const int dec_mem[6] = {
>       512 * 1024,
>  1 * 1024 * 1024,
>  2 * 1024 * 1024,
>  4 * 1024 * 1024,
>  8 * 1024 * 1024,
> 16 * 1024 * 1024,
> };
> switch(comp) {
> case 0:
> size_enc = fcba.comp1_density;
> break;
> case 1:
> if (fdbar.NC == 0)
> 	return 0;
> size_enc = fcba.comp2_density;
> break;
> default:
> msg_perr("Only component index 0 or 1 are supported yet.\n");
> return 0;
> }
> if (size_enc > 5) {
> msg_perr("Density of component with index %d illegal or "

Please replace "illegal" with "invalid" everywhere.
> 	 "unsupported. Encoded density is 0x%x.\n",
> 	 comp,

Please kill this line break.
> 	 size_enc);
> return 0;
> }
> return dec_mem[size_enc];
>
> +}
>
> +#ifdef ICH_DESCRIPTORS_FROM_MMAP_DUMP
> +uint32_t getVTBA()
> +{
> return (flumap.FLUMAP1 << 4) & 0x0ff0;
>
> +}
> +#endif
>
> +const struct flash_descriptor_addresses desc_addr = {
> .FCBA = getFCBA,
> .FRBA = getFRBA,
> .FMBA = getFMBA,
> .FMSBA = getFMSBA,
> .FISBA = getFISBA,
> .FLREG_limit = getFLREG_limit,
> .FLREG_base = getFLREG_base,
>
> +#ifdef ICH_DESCRIPTORS_FROM_MMAP_DUMP
> .VTBA = getVTBA,
>
> +#endif
> +};
>
> +void pretty_print_ich_descriptors()
> +{
> pretty_print_ich_descriptor_map();
> pretty_print_ich_descriptor_component();
> pretty_print_ich_descriptor_region();
> pretty_print_ich_descriptor_master();
>
> +#ifdef ICH_DESCRIPTORS_FROM_MMAP_DUMP
> pretty_print_ich_descriptor_upper_map();
> /* FIXME: detect chipset correctly */
> pretty_print_ich_descriptor_straps(CHIPSET_SERIES_5_IBEX_PEAK);
>
> +#endif // ICH_DESCRIPTORS_FROM_MMAP_DUMP
> msg_pdbg("\n");
>
> +}
>
> +void pretty_print_ich_descriptor_map(void)
> +{
> msg_pdbg("=== FDBAR ===\n");
> msg_pdbg("FLVALSIG 0x%8.8x\n", fdbar.FLVALSIG);
> msg_pdbg("FLMAP0   0x%8.8x\n", fdbar.FLMAP0  );
> msg_pdbg("FLMAP1   0x%8.8x\n", fdbar.FLMAP1  );
> msg_pdbg("FLMAP2   0x%8.8x\n", fdbar.FLMAP2  );
> msg_pdbg("\n");
> msg_pdbg("--- FDBAR details ---\n");
> msg_pdbg("0x%2.2x        NR    Number of Regions\n", fdbar.NR   );
> msg_pdbg("0x%8.8x  FRBA  Flash Region Base Address\n", desc_addr.FRBA() );
> msg_pdbg("0x%2.2x        NC    Number of Components\n", fdbar.NC   );
> msg_pdbg("0x%8.8x  FCBA  Flash Component Base Address\n", desc_addr.FCBA() );
> msg_pdbg("\n");
> msg_pdbg("0x%2.2x        ISL   ICH Strap Length\n", fdbar.ISL  );
> msg_pdbg("0x%8.8x  FISBA Flash ICH Strap Base Address\n", desc_addr.FISBA());
> msg_pdbg("0x%2.2x        NM    Number of Masters\n", fdbar.NM   );
> msg_pdbg("0x%8.8x  FMBA  Flash Master Base Address\n", desc_addr.FMBA() );
> msg_pdbg("\n");
> msg_pdbg("0x%2.2x        MSL   MCH Strap Length\n", fdbar.MSL  );
> msg_pdbg("0x%8.8x  FMSBA Flash MCH Strap Base Address\n", desc_addr.FMSBA());
>
> +}
>
> +void pretty_print_ich_descriptor_component(void)
> +{
> const char * str_freq[8] = {

const char * const 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] = {

Same here.
> "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
> msg_pdbg("0x%2.2x    freq_write     %s\n",
> fcba.freq_write   , str_freq[fcba.freq_write   ]);

Same problem here.
> msg_pdbg("0x%2.2x    freq_fastread  %s\n",
> fcba.freq_fastread, str_freq[fcba.freq_fastread]);

And here.
> msg_pdbg("0x%2.2x    fastread       %ssupported\n",
> fcba.fastread, fcba.fastread ? "" : "not ");
> msg_pdbg("0x%2.2x    freq_read      %s\n",
> fcba.freq_read, str_freq[fcba.freq_read    ]);

And here.
> msg_pdbg("0x%2.2x    comp 1 density %s\n",
> fcba.comp1_density, str_mem[fcba.comp1_density]);

And here.
> if (fdbar.NC)
> msg_pdbg("0x%2.2x    comp 2 density %s\n",
> 	fcba.comp2_density, str_mem[fcba.comp2_density]);

And here.
> else
> msg_pdbg("0x%2.2x    comp 2 is not used (FLMAP0.NC=0)\n",
> 	fcba.comp2_density);
> msg_pdbg("\n");
> msg_pdbg("0x%2.2x    invalid instr 0\n", fcba.invalid_instr0);
> msg_pdbg("0x%2.2x    invalid instr 1\n", fcba.invalid_instr1);
> msg_pdbg("0x%2.2x    invalid instr 2\n", fcba.invalid_instr2);
> msg_pdbg("0x%2.2x    invalid instr 3\n", fcba.invalid_instr3);
>
> +}
>
> +void pretty_print_ich_descriptor_region(void)
> +{
> msg_pdbg("\n");
> msg_pdbg("=== FRBA ===\n");
> msg_pdbg("FLREG0   0x%8.8x\n", frba.FLREG0);
> msg_pdbg("FLREG1   0x%8.8x\n", frba.FLREG1);
> msg_pdbg("FLREG2   0x%8.8x\n", frba.FLREG2);
> msg_pdbg("FLREG3   0x%8.8x\n", frba.FLREG3);
> msg_pdbg("\n");
> msg_pdbg("--- FRBA details ---\n");
> msg_pdbg("0x%8.8x  region 0 limit (descr)\n", desc_addr.FLREG_limit(frba.FLREG0));
> msg_pdbg("0x%8.8x  region 0 base  (descr)\n", desc_addr.FLREG_base(frba.FLREG0));
> msg_pdbg("0x%8.8x  region 1 limit ( BIOS)\n", desc_addr.FLREG_limit(frba.FLREG1));
> msg_pdbg("0x%8.8x  region 1 base  ( BIOS)\n", desc_addr.FLREG_base(frba.FLREG1));
> msg_pdbg("0x%8.8x  region 2 limit ( ME  )\n", desc_addr.FLREG_limit(frba.FLREG2));
> msg_pdbg("0x%8.8x  region 2 base  ( ME  )\n", desc_addr.FLREG_base(frba.FLREG2));
> msg_pdbg("0x%8.8x  region 3 limit ( GbE )\n", desc_addr.FLREG_limit(frba.FLREG3));
> msg_pdbg("0x%8.8x  region 3 base  ( GbE )\n", desc_addr.FLREG_base(frba.FLREG3));
>
> +}
>
> +void pretty_print_ich_descriptor_master(void)
> +{
> msg_pdbg("\n");
> msg_pdbg("=== FMBA ===\n");
> msg_pdbg("FLMSTR1  0x%8.8x\n", fmba.FLMSTR1);
> msg_pdbg("FLMSTR2  0x%8.8x\n", fmba.FLMSTR2);
> msg_pdbg("FLMSTR3  0x%8.8x\n", fmba.FLMSTR3);
> msg_pdbg("\n");
> msg_pdbg("--- FMBA details ---\n");
> msg_pdbg("BIOS can %s write GbE\n",   fmba.BIOS_GbE_write   ? "   " : "NOT");
> msg_pdbg("BIOS can %s write ME\n",    fmba.BIOS_ME_write    ? "   " : "NOT");
> msg_pdbg("BIOS can %s write BIOS\n",  fmba.BIOS_BIOS_write  ? "   " : "NOT");
> msg_pdbg("BIOS can %s write descr\n", fmba.BIOS_descr_write ? "   " : "NOT");
> msg_pdbg("BIOS can %s read  GbE\n",   fmba.BIOS_GbE_read    ? "   " : "NOT");
> msg_pdbg("BIOS can %s read  ME\n",    fmba.BIOS_ME_read     ? "   " : "NOT");
> msg_pdbg("BIOS can %s read  BIOS\n",  fmba.BIOS_BIOS_read   ? "   " : "NOT");
> msg_pdbg("BIOS can %s read  descr\n", fmba.BIOS_descr_read  ? "   " : "NOT");
> msg_pdbg("ME   can %s write GbE\n",   fmba.ME_GbE_write     ? "   " : "NOT");
> msg_pdbg("ME   can %s write ME\n",    fmba.ME_ME_write      ? "   " : "NOT");
> msg_pdbg("ME   can %s write BIOS\n",  fmba.ME_BIOS_write    ? "   " : "NOT");
> msg_pdbg("ME   can %s write descr\n", fmba.ME_descr_write   ? "   " : "NOT");
> msg_pdbg("ME   can %s read  GbE\n",   fmba.ME_GbE_read      ? "   " : "NOT");
> msg_pdbg("ME   can %s read  ME\n",    fmba.ME_ME_read       ? "   " : "NOT");
> msg_pdbg("ME   can %s read  BIOS\n",  fmba.ME_BIOS_read     ? "   " : "NOT");
> msg_pdbg("ME   can %s read  descr\n", fmba.ME_descr_read    ? "   " : "NOT");
> msg_pdbg("GbE  can %s write GbE\n",   fmba.GbE_GbE_write    ? "   " : "NOT");
> msg_pdbg("GbE  can %s write ME\n",    fmba.GbE_ME_write     ? "   " : "NOT");
> msg_pdbg("GbE  can %s write BIOS\n",  fmba.GbE_BIOS_write   ? "   " : "NOT");
> msg_pdbg("GbE  can %s write descr\n", fmba.GbE_descr_write  ? "   " : "NOT");
> msg_pdbg("GbE  can %s read  GbE\n",   fmba.GbE_GbE_read     ? "   " : "NOT");
> msg_pdbg("GbE  can %s read  ME\n",    fmba.GbE_ME_read      ? "   " : "NOT");
> msg_pdbg("GbE  can %s read  BIOS\n",  fmba.GbE_BIOS_read    ? "   " : "NOT");
> msg_pdbg("GbE  can %s read  descr\n", fmba.GbE_descr_read   ? "   " : "NOT");
>
> +}
>
> +#ifdef ICH_DESCRIPTORS_FROM_MMAP_DUMP
> +void pretty_print_ich_descriptor_straps_ich8(void)
> +{
> const char * str_GPIO12[4] = {
> "GPIO12",
> "LAN PHY Power Control Function (Native Output)",
> " GLAN_DOCK# (Native Input)",

Is the space before GLAN_DOCK# intentional?
> "invalid configuration",
> };
> msg_pdbg("\n");
> msg_pdbg("=== FISBA ===\n");
> msg_pdbg("STRP0    0x%8.8x\n", fisba.ich8.STRP0);
> msg_pdbg("\n");
> msg_pdbg("--- FISBA details ---\n");
> msg_pdbg("ME SMBus addr2 0x%2.2x\n", fisba.ich8.ASD2);
> msg_pdbg("ME SMBus addr1 0x%2.2x\n", fisba.ich8.ASD);
> msg_pdbg("ME SMBus Controller is connected to %s\n", fisba.ich8.MESM2SEL ? "SMLink pins" : "SMBus pins");
> msg_pdbg("SPI CS1 is used for %s\n", fisba.ich8.SPICS1_LANPHYPC_SEL ? "LAN PHY Power Control Function" : "SPI Chip Select");
> msg_pdbg("GPIO12_SEL is used as %s\n", str_GPIO12[fisba.ich8.GPIO12_SEL]);

Out of bounds?
> msg_pdbg("PCIe Port 6 is used for %s\n", fisba.ich8.GLAN_PCIE_SEL ? "integrated GLAN" : "PCI Express");
> msg_pdbg("Intel AMT SMBus Controller 1 is connected to %s\n",   fisba.ich8.BMCMODE ? "SMLink" : "SMBus");
> msg_pdbg("TCO slave is on %s. Intel AMT SMBus Controller 1 is %sabled\n",
> fisba.ich8.TCOMODE ? "SMBus" : "SMLink", fisba.ich8.TCOMODE ? "en" : "dis");
> msg_pdbg("ME A is %sabled\n", fisba.ich8.ME_DISABLE ? "dis" : "en");
> msg_pdbg("\n");
> msg_pdbg("=== FMSBA ===\n");
> msg_pdbg("STRP1    0x%8.8x\n", fisba.ich8.STRP1);
> msg_pdbg("\n");
> msg_pdbg("--- FMSBA details ---\n");
> msg_pdbg("ME B is %sabled\n", fisba.ich8.ME_disable_B ? "dis" : "en");
>
> +}
>
> +void pretty_print_ich_descriptor_straps_ibex(void)
> +{
> int i;
> msg_pdbg("\n");
> msg_pdbg("=== FPSBA ===\n");
> for(i = 0; i <= 15; i++)
> msg_pdbg("STRP%-2d = 0x%8.8x\n", i, fisba.ibex.STRPs[i]);
>
> +}
>
> +void pretty_print_ich_descriptor_straps(enum chipset cs)
> +{
> switch (cs) {
> case CHIPSET_ICH8:
> pretty_print_ich_descriptor_straps_ich8();
> break;
> case CHIPSET_SERIES_5_IBEX_PEAK:
> pretty_print_ich_descriptor_straps_ibex();
> break;
> default:
> break;
> }
>
> +}
>
> +void pretty_print_ich_descriptor_upper_map(void)
> +{
> int i;
> msg_pdbg("\n");
> msg_pdbg("=== FLUMAP ===\n");
> msg_pdbg("FLUMAP1  0x%8.8x\n", flumap.FLUMAP1);
> msg_pdbg("\n");
> msg_pdbg("--- FLUMAP details ---\n");
> msg_pdbg("VTL   0x%8.8x\n", flumap.VTL);
> msg_pdbg("VTBA  0x%8.8x\n", desc_addr.VTBA());
> msg_pdbg("\n");
> for (i=0; i < flumap.VTL/2; i++)
> {
> msg_pdbg("  JID%d  = 0x%8.8x\n", i, flumap.vscc_table[i].JID );

Out of bounds if flumap.VTL contains garbage?
> msg_pdbg("  VSCC%d = 0x%8.8x\n", i, flumap.vscc_table[i].VSCC);

Out of bounds?
> }
>
> +}
>
> +int read_ich_descriptors_from_dump(uint32_t *dump)
> +{
> int i;
> uint8_t pch_bug_offset = 0;
> if (dump[0] != DESCRIPTOR_MODE_MAGIC) {
> if (dump[4] == DESCRIPTOR_MODE_MAGIC)
> 	pch_bug_offset = 4;
> else
> 	return -1;
> }
> /* map */
> fdbar.FLVALSIG = dump[0 + pch_bug_offset];
> fdbar.FLMAP0   = dump[1 + pch_bug_offset];
> fdbar.FLMAP1   = dump[2 + pch_bug_offset];
> fdbar.FLMAP2   = dump[3 + pch_bug_offset];

Please either align with tabs or kill the alignment. If you think
aligning with spaces is essential, please tell me why.
> /* component */
> fcba.FLCOMP = dump[(desc_addr.FCBA() >> 2) + 0];
> fcba.FLILL  = dump[(desc_addr.FCBA() >> 2) + 1];
> fcba.FLPB  = dump[(desc_addr.FCBA() >> 2) + 2];

Out of bounds?
> /* region */
> frba.FLREG0 = dump[(desc_addr.FRBA() >> 2) + 0];
> frba.FLREG1 = dump[(desc_addr.FRBA() >> 2) + 1];
> frba.FLREG2 = dump[(desc_addr.FRBA() >> 2) + 2];
> frba.FLREG3 = dump[(desc_addr.FRBA() >> 2) + 3];

Out of bounds?
> /* master */
> fmba.FLMSTR1 = dump[(desc_addr.FMBA() >> 2) + 0];
> fmba.FLMSTR2 = dump[(desc_addr.FMBA() >> 2) + 1];
> fmba.FLMSTR3 = dump[(desc_addr.FMBA() >> 2) + 2];

Out of bounds?
> /* upper map */
> flumap.FLUMAP1 = dump[(0x0efc >> 2) + 0];
> for (i=0; i < flumap.VTL; i++)
> {
> flumap.vscc_table[i].JID  = dump[(desc_addr.VTBA() >> 2) + i * 2 + 0];
> flumap.vscc_table[i].VSCC = dump[(desc_addr.VTBA() >> 2) + i * 2 + 1];

Out of bounds?
> }
> /* straps */
> /* FIXME: detect chipset correctly */
> switch (CHIPSET_SERIES_5_IBEX_PEAK) {

WTF?
> case CHIPSET_ICH8:
> 	fisba.ich8.STRP0 = dump[(desc_addr.FISBA() >> 2) + 0];
> 	fisba.ich8.STRP1 = dump[(desc_addr.FMSBA() >> 2) + 0];

Out of bounds?
> 	break;
> case CHIPSET_SERIES_5_IBEX_PEAK:
> 	for(i = 0; i <= 15; i++)
> 		fisba.ibex.STRPs[i] = dump[(desc_addr.FISBA() >> 2) + i];

Out of bounds?
> 	break;
> default:
> 	break;
> }
> return 0;
>
> +}
> +#else // ICH_DESCRIPTORS_FROM_MMAP_DUMP
>
> +static uint32_t read_descriptor_reg(uint8_t section, uint16_t offset, void *spibar)
> +{
> uint32_t control = 0;
> control |= (section << FDOC_FDSS_OFF) & FDOC_FDSS;
> control |= (offset << FDOC_FDSI_OFF) & FDOC_FDSI;
> *(volatile uint32_t *) (spibar + ICH9_REG_FDOC) = control;
> return *(volatile uint32_t *)(spibar + ICH9_REG_FDOD);
>
> +}
>
> +void read_ich_descriptors_from_fdo(void *spibar)
> +{
> msg_pdbg("Reading flash descriptors "
>  "mapped by the chipset via FDOC/FDOD...");
> /* descriptor map section */
> fdbar.FLVALSIG = read_descriptor_reg(0, 0, spibar);
> fdbar.FLMAP0   = read_descriptor_reg(0, 1, spibar);
> fdbar.FLMAP1   = read_descriptor_reg(0, 2, spibar);
> fdbar.FLMAP2   = read_descriptor_reg(0, 3, spibar);

Alignment with tabs...
> /* component section */
> fcba.FLCOMP = read_descriptor_reg(1, 0, spibar);
> fcba.FLILL  = read_descriptor_reg(1, 1, spibar);
> fcba.FLPB  = read_descriptor_reg(1, 2, spibar);
> /* region section */
> frba.FLREG0 = read_descriptor_reg(2, 0, spibar);
> frba.FLREG1 = read_descriptor_reg(2, 1, spibar);
> frba.FLREG2 = read_descriptor_reg(2, 2, spibar);
> frba.FLREG3 = read_descriptor_reg(2, 3, spibar);
> /* master section */
> fmba.FLMSTR1 = read_descriptor_reg(3, 0, spibar);
> fmba.FLMSTR2 = read_descriptor_reg(3, 1, spibar);
> fmba.FLMSTR3 = read_descriptor_reg(3, 2, spibar);
> /* strap section and upper map */
> /* impossible via FDOC/FDOD(?) */
> msg_pdbg(" done\n");
>
> +}
>
> +#endif // ICH_DESCRIPTORS_FROM_MMAP_DUMP
> +#endif // defined(__i386__) || defined(__x86_64__)
> diff --git a/ich_descriptors.h b/ich_descriptors.h
> new file mode 100644
> index 0000000..af9d0ba
> --- /dev/null
> +++ b/ich_descriptors.h
> @@ -0,0 +1,408 @@
> +/* This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License version 2 as
> published by the Free Software Foundation.
> authors:
> (c) 2010  Matthias Wenzel <bios at mazzoo dot de>
> (c) 2011  Stefan Tauner
> */
>
> +#include <stdint.h>
>
> +#if defined(__i386__) || defined(__x86_64__)
> +#ifndef __ICH_DESCRIPTORS_H__
> +#define __ICH_DESCRIPTORS_H__ 1
>
> +/* should probably be in ichspi.h */
> +#define ICH9_REG_FDOC		0xB0	/* 32 Bits Flash Descriptor Observability Control */
> +#define FDOC_FDSI_OFF		2	/* 2-11: Flash Descriptor Section Select */
> +#define FDOC_FDSI		(0x3f << FDOC_FDSI_OFF)
> +#define FDOC_FDSS_OFF		12	/* 12-14: Flash Descriptor Section Select */
> +#define FDOC_FDSS		(0x3 << FDOC_FDSS_OFF)
> +#define ICH9_REG_FDOD		0xB4	/* 32 Bits Flash Descriptor Observability Data */
>
> +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?
> +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.
> +struct flash_component {
> union {			/* 0x00 */
> uint32_t FLCOMP;
> struct {
> 	uint8_t  comp1_density	:3;
> 	uint8_t  comp2_density	:3;
> 	unsigned		:11;
> 	uint8_t  freq_read	:3;
> 	uint8_t  fastread	:1;
> 	uint8_t  freq_fastread	:3;
> 	uint8_t  freq_write	:3;
> 	uint8_t  freq_read_id	:3;
> 	unsigned		:2;
> };
> };
> union {			/* 0x04 */
> uint32_t FLILL;
> struct {
> 	uint8_t invalid_instr0;
> 	uint8_t invalid_instr1;
> 	uint8_t invalid_instr2;
> 	uint8_t invalid_instr3;
> };
> };
> union {			/* 0x08 */
> uint32_t FLPB;
> struct {
> 	uint16_t FPBA	:13;
> 	unsigned	:19;
> };
> };
>
> +};

Same problem.
> +struct flash_region {
> union {
> uint32_t FLREG0; /* Flash Descriptor */
> struct {
> 	uint16_t reg0_base	:13;
> 	unsigned		:3;
> 	uint16_t reg0_limit	:13;
> 	unsigned		:3;
> };
> };
> union {
> uint32_t FLREG1; /* BIOS */
> struct {
> 	uint16_t reg1_base	:13;
> 	unsigned		:3;
> 	uint16_t reg1_limit	:13;
> 	unsigned		:3;
> };
> };
> union {
> uint32_t FLREG2; /* ME */
> struct {
> 	uint16_t reg2_base	:13;
> 	unsigned		:3;
> 	uint16_t reg2_limit	:13;
> 	unsigned		:3;
> };
> };
> union {
> uint32_t FLREG3; /* GbE */
> struct {
> 	uint16_t reg3_base	:13;
> 	unsigned		:3;
> 	uint16_t reg3_limit	:13;
> 	unsigned		:3;
> };
> };
>
> +} frba;

Dito.
> +struct flash_master {
> union {
> uint32_t FLMSTR1;
> struct {
> 	uint16_t BIOS_req_ID		:16;
> 	uint8_t  BIOS_descr_read	:1;
> 	uint8_t  BIOS_BIOS_read		:1;
> 	uint8_t  BIOS_ME_read		:1;
> 	uint8_t  BIOS_GbE_read		:1;
> 	uint8_t  BIOS_plat_read		:1;
> 	unsigned			:3;
> 	uint8_t  BIOS_descr_write	:1;
> 	uint8_t  BIOS_BIOS_write	:1;
> 	uint8_t  BIOS_ME_write		:1;
> 	uint8_t  BIOS_GbE_write		:1;
> 	uint8_t  BIOS_plat_write	:1;
> 	unsigned			:3;
> };
> };
> union {
> uint32_t FLMSTR2;
> struct {
> 	uint16_t ME_req_ID		:16;
> 	uint8_t  ME_descr_read	:1;
> 	uint8_t  ME_BIOS_read		:1;
> 	uint8_t  ME_ME_read		:1;
> 	uint8_t  ME_GbE_read		:1;
> 	uint8_t  ME_plat_read		:1;
> 	unsigned			:3;
> 	uint8_t  ME_descr_write		:1;
> 	uint8_t  ME_BIOS_write		:1;
> 	uint8_t  ME_ME_write		:1;
> 	uint8_t  ME_GbE_write		:1;
> 	uint8_t  ME_plat_write		:1;
> 	unsigned			:3;
> };
> };
> union {
> uint32_t FLMSTR3;
> struct {
> 	uint16_t GbE_req_ID		:16;
> 	uint8_t  GbE_descr_read		:1;
> 	uint8_t  GbE_BIOS_read		:1;
> 	uint8_t  GbE_ME_read		:1;
> 	uint8_t  GbE_GbE_read		:1;
> 	uint8_t  GbE_plat_read		:1;
> 	unsigned			:3;
> 	uint8_t  GbE_descr_write	:1;
> 	uint8_t  GbE_BIOS_write		:1;
> 	uint8_t  GbE_ME_write		:1;
> 	uint8_t  GbE_GbE_write		:1;
> 	uint8_t  GbE_plat_write		:1;
> 	unsigned			:3;
> };
> };
>
> +};

Dito.
> +#ifdef ICH_DESCRIPTORS_FROM_MMAP_DUMP
> +struct flash_strap {
> union {
> struct {
> 	union {
> 		uint32_t STRP0;
> 		struct {
> 			uint8_t  ME_DISABLE		:1;
> 			unsigned			:6;
> 			uint8_t  TCOMODE		:1;
> 			uint8_t  ASD			:7;
> 			uint8_t  BMCMODE		:1;
> 			unsigned			:3;
> 			uint8_t  GLAN_PCIE_SEL		:1;
> 			uint8_t  GPIO12_SEL		:2;
> 			uint8_t  SPICS1_LANPHYPC_SEL	:1;
> 			uint8_t  MESM2SEL		:1;
> 			unsigned			:1;
> 			uint8_t  ASD2			:7;
> 		};
> 	};
> 	union {
> 		uint32_t STRP1;
> 		struct {
> 			uint8_t  ME_disable_B		:1;
> 			unsigned			:31;
> 		};
> 	};
> }ich8;
> 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?
> 		};
> 		union {
> 			uint32_t STRP2;
> 			struct {
> 			};

Dito.
> 		};
> 		union {
> 			uint32_t STRP3;
> 			struct {
> 			};
> 		};

You get the point.
> 		union {
> 			uint32_t STRP4;
> 			struct {
> 			};
> 		};
> 		union {
> 			uint32_t STRP5;
> 			struct {
> 			};
> 		};
> 		union {
> 			uint32_t STRP6;
> 			struct {
> 			};
> 		};
> 		union {
> 			uint32_t STRP7;
> 			struct {
> 			};
> 		};
> 		union {
> 			uint32_t STRP8;
> 			struct {
> 			};
> 		};
> 		union {
> 			uint32_t STRP9;
> 			struct {
> 			};
> 		};
> 		union {
> 			uint32_t STRP10;
> 			struct {
> 			};
> 		};
> 		union {
> 			uint32_t STRP11;
> 			struct {
> 			};
> 		};
> 		union {
> 			uint32_t STRP12;
> 			struct {
> 			};
> 		};
> 		union {
> 			uint32_t STRP13;
> 			struct {
> 			};
> 		};
> 		union {
> 			uint32_t STRP14;
> 			struct {
> 			};
> 		};
> 		union {
> 			uint32_t STRP15;
> 			struct {
> 			};
> 		};
> 	};
> }ibex;
> };
>
> +};

__attribute__((packed))
> +struct flash_upper_map {
> union {
> uint32_t FLUMAP1;
> struct {
> 	uint8_t  VTBA	:8;
> 	uint8_t  VTL	:8;
> 	unsigned	:16;
> };
> };
> struct {
> union {
> 	uint32_t JID;
> 	struct {
> 		uint8_t vid	:8;
> 		uint8_t cid0	:8;
> 		uint8_t cid1	:8;
> 		unsigned	:8;
> 	};
> };
> union {
> 	uint32_t VSCC;
> 	struct {
> 		uint8_t  ubes	:2;
> 		uint8_t  uwg	:1;
> 		uint8_t  uwsr	:1;
> 		uint8_t  uwews	:1;
> 		unsigned	:3;
> 		uint8_t  ueo	:8;
> 		uint8_t  lbes	:2;
> 		uint8_t  lwg	:1;
> 		uint8_t  lwsr	:1;
> 		uint8_t  lwews	:1;
> 		unsigned	:3;
> 		uint16_t leo	:16;
> 	};
> };
> }vscc_table[128];
>
> +};

Dito.
> +#endif // ICH_DESCRIPTORS_FROM_MMAP_DUMP
>
> +void pretty_print_ich_descriptors(void);
>
> +void pretty_print_ich_descriptor_map(void);
> +void pretty_print_ich_descriptor_component(void);
> +void pretty_print_ich_descriptor_region(void);
> +void pretty_print_ich_descriptor_master(void);
>
> +int getFCBA_component_density(uint8_t comp);
>
> +#ifdef ICH_DESCRIPTORS_FROM_MMAP_DUMP
>
> +void pretty_print_ich_descriptor_upper_map(void);
> +void pretty_print_ich_descriptor_straps(enum chipset cs);
> +int read_ich_descriptors_from_dump(uint32_t *dump);
>
> +#else // ICH_DESCRIPTORS_FROM_MMAP_DUMP
>
> +void read_ich_descriptors_from_fdo(void *spibar);
>
> +#endif // ICH_DESCRIPTORS_FROM_MMAP_DUMP
>
> +#endif // __ICH_DESCRIPTORS_H__
> +#endif // defined(__i386__) || defined(__x86_64__)
> diff --git a/ichspi.c b/ichspi.c
> index 584cab4..d8df898 100644
> --- a/ichspi.c
> +++ b/ichspi.c
> @@ -6,6 +6,7 @@
> Copyright (C) 2008 Dominik Geyer
> Copyright (C) 2008 coresystems GmbH
> Copyright (C) 2009, 2010 Carl-Daniel Hailfinger
> Copyright (C) 2011 Stefan Tauner
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
>
> @@ -41,6 +42,7 @@
>  #include "chipdrivers.h"
>  #include "programmer.h"
>  #include "spi.h"
> +#include "ich_descriptors.h"
>
> /* ICH9 controller register definition */
>  #define ICH9_REG_HSFS		0x04	/* 16 Bits Hardware Sequencing Flash Status */
> @@ -1197,6 +1199,7 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
>    uint8_t old, new;
>    uint16_t spibar_offset, tmp2;
>    uint32_t tmp;
>
> int ichspi_desc = 0;
>
> switch (ich_generation) {
> case 7:
>
> @@ -1268,6 +1271,8 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
>    		msg_pinfo("WARNING: SPI Configuration Lockdown activated.\n");
>    		ichspi_lock = 1;
>    	}
> if (tmp2 & HSFS_FDV)
> 	ichspi_desc = 1;
>
> tmp2 = mmio_readw(ich_spibar + ICH9_REG_HSFC);
> msg_pdbg("0x06: 0x%04x (HSFC)\n", tmp2);
>
> @@ -1316,6 +1321,11 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
>    	ichspi_bbar = mmio_readl(ich_spibar + ICH9_REG_BBAR);
>    	msg_pdbg("0xA0: 0x%08x (BBAR)\n",
>    		     ichspi_bbar);
> msg_pdbg("\n");
> if (ichspi_desc) {
> 	read_ich_descriptors_from_fdo(ich_spibar);
> 	pretty_print_ich_descriptors();
> }
> ich_init_opcodes();
> break;
> default:
>
> diff --git a/util/ich_descriptors_tool/Makefile b/util/ich_descriptors_tool/Makefile
> new file mode 100644
> index 0000000..9b1d096
> --- /dev/null
> +++ b/util/ich_descriptors_tool/Makefile
> @@ -0,0 +1,42 @@
> +CC ?= gcc
>
> +PROGRAM=ich_descriptors_tool
> +EXTRAINCDIRS = ../../ .
> +DEPPATH = .dep
> +OBJATH = .obj
> +SHAREDSRC = ich_descriptors.c
> +SHAREDSRCDIR = ../..
>
> +SRC = $(wildcard *.c)
>
> +CFLAGS=-Wall
> +CFLAGS += -MMD -MP -MF $(DEPPATH)/$(@F).d
> +# enables functions that populate the descriptor structs from plain dump arrays
> +CFLAGS += -D ICH_DESCRIPTORS_FROM_MMAP_DUMP
> +CFLAGS += $(patsubst %,-I%,$(EXTRAINCDIRS))
>
> +OBJ = $(OBJATH)/$(SRC:%.c=%.o)
>
> +SHAREDOBJ = $(OBJATH)/$(notdir $(SHAREDSRC:%.c=%.o))
>
> +all:$(PROGRAM)
>
> +$(OBJ): $(OBJATH)/%.o : %.c
> $(CC) $(CFLAGS) -o $@ -c $<
>
> +# this enables us to NOT share object files with flashrom,
> +# which would lead to unexpected results otherwise (w/o running make clean)
> +$(SHAREDOBJ): $(OBJATH)/%.o : $(SHAREDSRCDIR)/%.c
> $(CC) $(CFLAGS) -o $@ -c $<
>
> +$(PROGRAM): $(OBJ) $(SHAREDOBJ)
> $(CC) -o $(PROGRAM) $(OBJ) $(SHAREDOBJ)
>
> +clean:
> rm -f $(PROGRAM)
> rm -rf $(DEPPATH) $(OBJATH)
>
> +# Include the dependency files.
> +-include $(shell mkdir -p $(DEPPATH) $(OBJATH) 2>/dev/null) $(wildcard $(DEPPATH)/*)
>
> +.PHONY: all clean
> \ No newline at end of file

A newline at the end of the file would be nice.
> diff --git a/util/ich_descriptors_tool/TODO b/util/ich_descriptors_tool/TODO
> new file mode 100644
> index 0000000..e3933ee
> --- /dev/null
> +++ b/util/ich_descriptors_tool/TODO
> @@ -0,0 +1,5 @@
> +- reverse the path, as in assemble a descriptormode image from various
> blobs (BIOS, GbE, ME, OEM) and a description (xml? custom config?
> sane defaults and cmd-line switches?)
>
> +- dump 256 OEM bytes
> +- handle descriptors of various chipsets correctly
> \ No newline at end of file
> diff --git a/util/ich_descriptors_tool/ich_descriptors_tool.c b/util/ich_descriptors_tool/ich_descriptors_tool.c
> new file mode 100644
> index 0000000..8ce7677
> --- /dev/null
> +++ b/util/ich_descriptors_tool/ich_descriptors_tool.c
> @@ -0,0 +1,226 @@
> +/*
> dump information and binaries from BIOS images that are in descriptor mode/soft-strap
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License version 2 as
> published by the Free Software Foundation.
> authors:
> (c) 2010  Matthias Wenzel <bios at mazzoo dot de>
> (c) 2011  Stefan Tauner
> */
>
> +#include <stdio.h>
> +#include <stdint.h>
> +#include <stdlib.h>
> +#include <sys/mman.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +#include <unistd.h>
> +#include <string.h>
> +#include <errno.h>
> +#include "flash.h"
> +#include "ich_descriptors.h"
>
> +extern struct flash_descriptor fdbar;
> +extern struct flash_component fcba;
> +extern struct flash_region frba;
> +extern struct flash_master fmba;
> +extern struct flash_strap fisba;
> +extern struct flash_upper_map flumap;
> +extern struct flash_descriptor_addresses desc_addr;
>
> +static int f;         /* file descriptor to flash file */
> +static int fs;        /* file size */
> +static uint32_t *fm; /* mmap'd file */
>
> +void dump_file_descriptor(char * fn)
> +{
> char * n = malloc(strlen(fn) + 11);
> snprintf(n, strlen(fn) + 11, "%s.descr.bin", fn);
> printf("\n");
> printf("+++ dumping %s ... ", n);
> int fh = open(n, O_WRONLY | O_CREAT, S_IRUSR | S_IWUSR);
> free(n);
> if (fh < 0)
> {
> printf("ERROR: couldn't open(%s): %s\n", n, strerror(errno));
> exit(1);
> }
> int ret;
> ret = write(fh, &fm[frba.reg0_base >> 2], frba.reg0_limit);
> if (ret != frba.reg0_limit)
> {
> printf("FAILED.\n");
> exit(1);
> }
> printf("done.\n");
> close(fh);
>
> +}
>
> +void dump_file_BIOS(char * fn)
> +{
> char * n = malloc(strlen(fn) + 10);
> snprintf(n, strlen(fn) + 10, "%s.BIOS.bin", fn);
> printf("\n");
> printf("+++ dumping %s ... ", n);
> int fh = open(n, O_WRONLY | O_CREAT, S_IRUSR | S_IWUSR);
> free(n);
> if (fh < 0)
> {
> printf("ERROR: couldn't open(%s): %s\n", n, strerror(errno));
> exit(1);
> }
> int ret;
> ret = write(fh, &fm[frba.reg1_base >> 2], frba.reg1_limit);
> if (ret != frba.reg1_limit)
> {
> printf("FAILED.\n");
> exit(1);
> }
> printf("done.\n");
> close(fh);
>
> +}
>
> +void dump_file_ME(char * fn)
> +{
> char * n = malloc(strlen(fn) + 8);
> snprintf(n, strlen(fn) + 8, "%s.ME.bin", fn);
> printf("\n");
> printf("+++ dumping %s ... ", n);
> int fh = open(n, O_WRONLY | O_CREAT, S_IRUSR | S_IWUSR);
> free(n);
> if (fh < 0)
> {
> printf("ERROR: couldn't open(%s): %s\n", n, strerror(errno));
> exit(1);
> }
> int ret;
> ret = write(fh, &fm[frba.reg2_base >> 2], frba.reg2_limit);
> if (ret != frba.reg2_limit)
> {
> printf("FAILED.\n");
> exit(1);
> }
> printf("done.\n");
> close(fh);
>
> +}
>
> +void dump_file_GbE(char * fn)
> +{
> char * n = malloc(strlen(fn) + 9);
> snprintf(n, strlen(fn) + 9, "%s.GbE.bin", fn);
> printf("\n");
> printf("+++ dumping %s ... ", n);
> int fh = open(n, O_WRONLY | O_CREAT, S_IRUSR | S_IWUSR);
> free(n);
> if (fh < 0)
> {
> printf("ERROR: couldn't open(%s): %s\n", n, strerror(errno));
> exit(1);
> }
> int ret;
> ret = write(fh, &fm[frba.reg3_base >> 2], frba.reg3_limit);
> if (ret != frba.reg3_limit)
> {
> printf("FAILED.\n");
> exit(1);
> }
> printf("done.\n");
> uint8_t * pMAC = (uint8_t *) &fm[frba.reg3_base >> 2];
> printf("the MAC-address might be: %2.2x:%2.2x:%2.2x:%2.2x:%2.2x:%2.2x\n",
> pMAC[0],
> pMAC[1],
> pMAC[2],
> pMAC[3],
> pMAC[4],
> pMAC[5]
> );
> close(fh);
>
> +}
>
> +void dump_files(char * n)
> +{
> printf("=== dumping section files ===\n");
> if (frba.reg0_limit)
> dump_file_descriptor(n);
> if (frba.reg1_limit)
> dump_file_BIOS(n);
> if (frba.reg2_limit)
> dump_file_ME(n);
> if (frba.reg3_limit)
> dump_file_GbE(n);
>
> +}
>
> +void usage(char ** argv, char* error)
> +{
> printf("%s\n", error);
> printf("\n");
> printf("usage: '%s <bios image file> [-d]'\n", argv[0]);
> printf("\n");
> printf("\twhere <bios image file> isn't actually a BIOS image, but the SPI flash\n");
> printf("\tcontents of the SPI connected to your intel chipset.\n");
> printf("\tIn case that image is really in descriptor mode (a.k.a soft-strap)\n");
> printf("\t%s will pretty print all relevant information.\n", argv[0]);
> printf("\tIf '-d' is specified as the second parameter some sections such as\n");
> printf("\tthe real BIOS image as seen by the CPU or a GbE blob that is\n");
> printf("\trequired to initialize your ICH GbE are also dumped to files.\n");
> exit(1);
>
> +}
>
> +int main(int argc, char **argv)
> +{
> if (argc < 2)
> usage(argv, "Need at least one parameter");
> f = open(argv[1], O_RDONLY);
> if (f < 0)
> usage(argv, "No such file");
> if (argc >= 3 && (strcmp("-d", argv[2]) == 0))
> usage(argv, "Bogus second argument");
> fs = lseek(f, 0, SEEK_END);
> if (fs < 0)
> usage(argv, "Seeking to the end of the file failed");
> fm = mmap(NULL, fs, PROT_READ, MAP_PRIVATE, f, 0);
> if (fm == (void *) -1)
> {
> /* fallback for stupid OSes like cygwin */
> int ret;
> fm = malloc(fs);
> if (!fm)
> 	usage(argv, "Could not allocate memory");
> lseek(f, 0, SEEK_SET);
> ret = read(f, fm, fs);
> if (ret != fs)
> 	usage(argv, "Seeking to the end of the file failed");
> }
> printf("flash image is %d [0x%x] bytes\n", fs, fs);
> close(f);
> if(read_ich_descriptors_from_dump(fm)){
> printf("not in descriptor mode\n");
> exit(1);
> }
> pretty_print_ich_descriptors();
> if (argc >= 3)
> dump_files(argv[1]);
> return 0;
>
> +}

Regards,
Carl-Daniel

[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.

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

[Carl-Daniel Hailfinger]

Am 12.06.2011 09:41 schrieb 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?

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

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

[Stefan Tauner]

On Thu, 14 Jul 2011 01:17:50 +0200
Carl-Daniel Hailfinger  wrote:
> Am 12.06.2011 09:41 schrieb Stefan Tauner:
> > On Sun, 12 Jun 2011 02:15:34 +0200
> > Carl-Daniel Hailfinger  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...

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

[Andrew Goodbody]

Carl-Daniel Hailfinger wrote:
> Am 08.06.2011 04:55 schrieb Stefan Tauner:
> > (c) 2010  Matthias Wenzel <bios at mazzoo dot de>
> > (c) 2011  Stefan Tauner
>
> Please use the same format as in other files, i.e. don't only use "(c)"
> but also add the word "Copyright". Rumor has it that even the order of
> "(c)" vs. "Copyright" is important for US law. No idea if that is true,
> but it can't hurt to do the same as everyone else.

My understanding is that there is no recognition in law for the use of 
(c). The law only recognises the actual copyright symbol ie 'c' inside a 
circle. So the use of (c) can actually be harmful as it can interfere 
with legally defined phrases.

The limitations of ASCII are not the concern of the law.

Andrew