Hello,
I'm glad it seems useful! Some questions inline:
On 06/29/2010 08:12 AM, Carl-Daniel Hailfinger wrote:
+static void ich_spi_frap(uint32_t frap, int i)
Can you change the name to print_ich9_spi_frap?
I was considering making this function twiddle some bits eventually (if it could). The BM{W,R}AG fields can apparently override the FRAP access controls to the "BIOS Descriptor" region. Unfortunately on my board, the regions were labeled as a flash descriptor and a ME section... do_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.
OK.
- 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.
OK.
- 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?
If I understand the datasheet correctly it simply means that there is no region in the flash chip that is considered as this (ME, GbE, etc). Maybe something like msg_pdbg("%s region is unused.", region_names[i]);
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
Thanks for the review,
Josh