Hi Joshua,
On 28.06.2010 22:55, Joshua Roys wrote:
Attached is a patch to display the FRAP access controls on the FREGx regions of the flash chip. Also attached is the output from `flashrom -Vr foo.bin' on an ICH10 machine.
Signed-off-by: Joshua Roys roysjosh@gmail.com
Thanks for your patch. Overall, it looks nice, but i'd like to see a few minor changes before I commit.
diff --git a/chipset_enable.c b/chipset_enable.c index d124291..ba802af 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -430,6 +430,40 @@ static int enable_flash_vt8237s_spi(struct pci_dev *dev, const char *name) return 0; }
+#define ICH_BMWAG(x) ((x >> 24) & 0xff) +#define ICH_BMRAG(x) ((x >> 16) & 0xff) +#define ICH_BRWA(x) ((x >> 8) & 0xff) +#define ICH_BRRA(x) ((x >> 0) & 0xff)
+#define ICH_FREG_BASE(x) ((x >> 0) & 0x1fff) +#define ICH_FREG_LIMIT(x) ((x >> 16) & 0x1fff)
+static void ich_spi_frap(uint32_t frap, int i)
Can you change the name to print_ich9_spi_frap?
+{
- char *access_names[4] = { "Locked", "Read-only", "Write-only", "Read-Write" };
- char *region_names[5] = {
"Flash Descriptor", "BIOS Descriptor", "ME", "GbE", "Platform Data" };
Those two string arrays should be const, and it would be appreciated if you could fit them into 80 column width. I'd prefer to have access_names in lowercase.
- int rwperms =
((ICH_BRWA(frap) & (1 << i)) << 1) |((ICH_BRRA(frap) & (1 << i)) << 0);
Odd indentation. The first two lines should be mergeable, and then ICH_BRRA should be aligned with ICH_BRWA.
- int offset = 0x54 + i * 4;
- uint32_t freg = mmio_readl(spibar + offset), base, limit;
Side note: spibar is a horrible global name. It should be ich_spibar, but that's not your fault, and needs to be fixed in a separate patch.
- msg_pdbg("0x%02X: 0x%08x (FREG%i: %s)\n",
offset, freg, i, region_names[i]);- base = ICH_FREG_BASE(freg);
- limit = ICH_FREG_LIMIT(freg);
- if (base == 0x1fff && limit == 0) {
/* this FREG is disabled */return;
This is a bit irritating in the output. Can you print a message that the FREG is disabled? OTOH, "disabled" implies that the region is inaccessible or does not exist. Looking at your log, what does it mean that the BIOS FREG is disabled? No access? Full access? Should the message be "unknown location, full access", or is that incorrect?
- }
- msg_pdbg("0x%08x-0x%08x is %s\n",
(base << 12), (limit << 12) | 0x0fff,access_names[rwperms]);+}
static int enable_flash_ich_dc_spi(struct pci_dev *dev, const char *name, int ich_generation) { @@ -554,21 +588,15 @@ static int enable_flash_ich_dc_spi(struct pci_dev *dev, const char *name,
tmp = mmio_readl(spibar + 0x50); msg_pdbg("0x50: 0x%08x (FRAP)\n", tmp);
msg_pdbg("BMWAG %i, ", (tmp >> 24) & 0xff);msg_pdbg("BMRAG %i, ", (tmp >> 16) & 0xff);msg_pdbg("BRWA %i, ", (tmp >> 8) & 0xff);msg_pdbg("BRRA %i\n", (tmp >> 0) & 0xff);msg_pdbg("0x54: 0x%08x (FREG0)\n",mmio_readl(spibar + 0x54));msg_pdbg("0x58: 0x%08x (FREG1)\n",mmio_readl(spibar + 0x58));msg_pdbg("0x5C: 0x%08x (FREG2)\n",mmio_readl(spibar + 0x5C));msg_pdbg("0x60: 0x%08x (FREG3)\n",mmio_readl(spibar + 0x60));msg_pdbg("0x64: 0x%08x (FREG4)\n",mmio_readl(spibar + 0x64));
msg_pdbg("BMWAG 0x%02x, ", ICH_BMWAG(tmp));msg_pdbg("BMRAG 0x%02x, ", ICH_BMRAG(tmp));msg_pdbg("BRWA 0x%02x, ", ICH_BRWA(tmp));msg_pdbg("BRRA 0x%02x\n", ICH_BRRA(tmp));/* print out the FREGx registers along with FRAP access bits */for(i = 0; i < 5; i++)ich_spi_frap(tmp, i);- msg_pdbg("0x74: 0x%08x (PR0)\n", mmio_readl(spibar + 0x74)); msg_pdbg("0x78: 0x%08x (PR1)\n",
This patch is definitely a good way to improve the readability of logs. Now we just have to add some generic infrastructure which refuses to read the chip if unreadable regions exist, and refuses to erase/write if unwritable regions exist. This would either be some global flag can_read/can_write set by the programmer, or some detailed region list. The global flags would also be useful for programmers where we only support reading.
Side note: Most BIOS implementers are using those ICH region restrictions in an insecure way, and that may allow us to execute a two-stage hack where the first stage tricks the chipset into allowing write access to the descriptor region, and the second stage uses this access to remove all restrictions from all regions.
Regards, Carl-Daniel