Hello, 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. Thanks, Josh
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 -- http://www.hailfinger.org/
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
Hi, On 29.06.2010 17:34, Joshua Roys wrote:
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?
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]);
But your output says:
0x54: 0x00010000 (FREG0: Flash Descriptor) 0x00000000-0x00001fff is Read-only 0x58: 0x00001fff (FREG1: BIOS Descriptor) 0x5C: 0x007f0002 (FREG2: ME) 0x00002000-0x0007ffff is Locked 0x60: 0x00001fff (FREG3: GbE) 0x64: 0x00001fff (FREG4: Platform Data)
That would imply no region of the flash chip is considered to be BIOS/GbE/Platformdata. Maybe we need to dump the flash descriptor region and analyze it. I think Matthias Wenzel wrote a tool for that: http://github.com/mazzoo/descriptormode Such an analysis would be best done in a separate patch, though.
Thanks for the review,
You're welcome. I'm confident we can commit the next iteration of your patch. Regards, Carl-Daniel -- http://www.hailfinger.org/
Hello, Here is version 2. Let me know if there are any more bits that need fixing! Thanks, Josh
Hi, your patch looks good, but I have some cosmetic nitpicks. On 29.06.2010 18:29, Joshua Roys wrote:
diff --git a/chipset_enable.c b/chipset_enable.c index d124291..ce80a58 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -430,6 +430,44 @@ 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 do_ich9_spi_frap(uint32_t frap, int i) +{ + const char *access_names[4] = { + "locked", "read-only", "write-only", "read-write" + }; + const char *region_names[5] = { + "Flash Descriptor", "BIOS Descriptor",
"BIOS", not "BIOS Descriptor"
+ "ME", "GbE", "Platform Data"
Not sure if "Management Engine" and "Gigabit Ethernet" would be better.
+ }; + int rwperms = ((ICH_BRWA(frap) & (1 << i)) << 1) | + ((ICH_BRRA(frap) & (1 << i)) << 0); + int offset = 0x54 + i * 4; + uint32_t freg = mmio_readl(spibar + offset), base, limit; + + 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 */ + msg_pdbg("%s region is unused.\n", region_names[i]);
"... unused or has no access restrictions.\n" would be better. I don't believe your machine has no BIOS region.
+ return; + } + + 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 +592,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++) + do_ich9_spi_frap(tmp, i); + msg_pdbg("0x74: 0x%08x (PR0)\n", mmio_readl(spibar + 0x74)); msg_pdbg("0x78: 0x%08x (PR1)\n",
Rest is fine, and will be acked and committed by me. Regards, Carl-Daniel -- http://www.hailfinger.org/
On 07/01/2010 12:35 PM, Carl-Daniel Hailfinger wrote:
+ base = ICH_FREG_BASE(freg); + limit = ICH_FREG_LIMIT(freg); + if (base == 0x1fff && limit == 0) { + /* this FREG is disabled */ + msg_pdbg("%s region is unused.\n", region_names[i]);
"... unused or has no access restrictions.\n" would be better. I don't believe your machine has no BIOS region.
I would prefer to leave this as-is, per the ich10 datasheet around page 805: "NOTE: If the BIOS region is not used, the Region Base must be programmed to 1FFFh and the Region Limit to 0000h to disable the region."
Rest is fine, and will be acked and committed by me.
Regards, Carl-Daniel
The other suggestions were incorporated. Thanks! Josh
On 01.07.2010 18:50, Joshua Roys wrote:
On 07/01/2010 12:35 PM, Carl-Daniel Hailfinger wrote:
+ base = ICH_FREG_BASE(freg); + limit = ICH_FREG_LIMIT(freg); + if (base == 0x1fff && limit == 0) { + /* this FREG is disabled */ + msg_pdbg("%s region is unused.\n", region_names[i]);
"... unused or has no access restrictions.\n" would be better. I don't believe your machine has no BIOS region.
I would prefer to leave this as-is, per the ich10 datasheet around page 805:
"NOTE: If the BIOS region is not used, the Region Base must be programmed to 1FFFh and the Region Limit to 0000h to disable the region."
Interesting. So it is indeed possible to have flash in descriptor mode and still have no designated BIOS region? It will be very interesting to dump the descriptors in the descriptor region.
ICH9/10: display FRAP/FREGx access controls
Signed-off-by: Joshua Roys <roysjosh@gmail.com>
Thanks for reworking the patch. Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> and committed in r1066. Regards, Carl-Daniel -- http://www.hailfinger.org/
participants (2)
-
Carl-Daniel Hailfinger -
Joshua Roys