ICH SPI can enforces address restrictions for all accesses which take an address (well, it could if the chipset implementation was not broken). Since exploiting the broken implementation is harder than conforming to the address restrictions wherever possible, conform to the address restrictions instead. This patch will eliminate a lot of transaction errors people were seeing (for probe, anyway).
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-ichspi_avoid_transaction_error_use_bbar/flash.h =================================================================== --- flashrom-ichspi_avoid_transaction_error_use_bbar/flash.h (Revision 1015) +++ flashrom-ichspi_avoid_transaction_error_use_bbar/flash.h (Arbeitskopie) @@ -663,6 +663,8 @@ uint32_t spi_get_valid_read_addr(void);
/* ichspi.c */ +extern int ichspi_lock; +extern uint32_t ichspi_bbar; int ich_init_opcodes(void); int ich_spi_send_command(unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr); Index: flashrom-ichspi_avoid_transaction_error_use_bbar/spi.c =================================================================== --- flashrom-ichspi_avoid_transaction_error_use_bbar/spi.c (Revision 1015) +++ flashrom-ichspi_avoid_transaction_error_use_bbar/spi.c (Arbeitskopie) @@ -207,8 +207,22 @@ return spi_programmer[spi_controller].write_256(flash, buf); }
+/* + * Get the lowest allowed address for read accesses. This often happens to + * be the lowest allowed address for all commands which take an address. + * This is a programmer limitation. + */ uint32_t spi_get_valid_read_addr(void) { - /* Need to return BBAR for ICH chipsets. */ - return 0; + switch (spi_controller) { +#if INTERNAL_SUPPORT == 1 +#if defined(__i386__) || defined(__x86_64__) + case SPI_CONTROLLER_ICH7: + /* Return BBAR for ICH chipsets. */ + return ichspi_bbar; +#endif +#endif + default: + return 0; + } } Index: flashrom-ichspi_avoid_transaction_error_use_bbar/chipset_enable.c =================================================================== --- flashrom-ichspi_avoid_transaction_error_use_bbar/chipset_enable.c (Revision 1015) +++ flashrom-ichspi_avoid_transaction_error_use_bbar/chipset_enable.c (Arbeitskopie) @@ -36,8 +36,6 @@
#if defined(__i386__) || defined(__x86_64__)
-extern int ichspi_lock; - static int enable_flash_ali_m1533(struct pci_dev *dev, const char *name) { uint8_t tmp; @@ -515,8 +513,9 @@ msg_pdbg("0x%02x: 0x%08x (SPID%d+4)\n", offs + 4, mmio_readl(spibar + offs + 4), i); } + ichspi_bbar = mmio_readl(spibar + 0x50); msg_pdbg("0x50: 0x%08x (BBAR)\n", - mmio_readl(spibar + 0x50)); + ichspi_bbar); msg_pdbg("0x54: 0x%04x (PREOP)\n", mmio_readw(spibar + 0x54)); msg_pdbg("0x56: 0x%04x (OPTYPE)\n", @@ -587,8 +586,9 @@ mmio_readl(spibar + 0x98)); msg_pdbg("0x9C: 0x%08x (OPMENU+4)\n", mmio_readl(spibar + 0x9C)); + ichspi_bbar = mmio_readl(spibar + 0xA0); msg_pdbg("0xA0: 0x%08x (BBAR)\n", - mmio_readl(spibar + 0xA0)); + ichspi_bbar); msg_pdbg("0xB0: 0x%08x (FDOC)\n", mmio_readl(spibar + 0xB0)); if (tmp2 & (1 << 15)) { Index: flashrom-ichspi_avoid_transaction_error_use_bbar/ichspi.c =================================================================== --- flashrom-ichspi_avoid_transaction_error_use_bbar/ichspi.c (Revision 1015) +++ flashrom-ichspi_avoid_transaction_error_use_bbar/ichspi.c (Arbeitskopie) @@ -103,6 +103,8 @@ /* ICH SPI configuration lock-down. May be set during chipset enabling. */ int ichspi_lock = 0;
+uint32_t ichspi_bbar = 0; + typedef struct _OPCODE { uint8_t opcode; //This commands spi opcode uint8_t spi_type; //This commands spi type @@ -327,6 +329,35 @@ return 0; }
+/* + * Try to set BBAR (BIOS Base Address Register), but read back the value in case + * it didn't stick. + */ +void ich_set_bbar(uint32_t minaddr) +{ + switch (spi_controller) { + case SPI_CONTROLLER_ICH7: + mmio_writel(minaddr, spibar + 0x50); + ichspi_bbar = mmio_readl(spibar + 0x50); + /* We don't have any option except complaining. */ + if (ichspi_bbar != minaddr) + msg_perr("Setting BBAR failed!\n"); + break; + case SPI_CONTROLLER_ICH9: + mmio_writel(minaddr, spibar + 0xA0); + ichspi_bbar = mmio_readl(spibar + 0xA0); + /* We don't have any option except complaining. */ + if (ichspi_bbar != minaddr) + msg_perr("Setting BBAR failed!\n"); + break; + default: + /* Not sure if BBAR actually exists on VIA. */ + msg_pdbg("Setting BBAR is not implemented for VIA yet.\n"); + break; + } + +} + /* This function generates OPCODES from or programs OPCODES to ICH according to * the chipset's SPI configuration lock. * @@ -341,13 +372,18 @@ return 0;
if (ichspi_lock) { - msg_pdbg("Generating OPCODES... "); + msg_pdbg("Reading OPCODES... "); curopcodes_done = &O_EXISTING; rc = generate_opcodes(curopcodes_done); } else { msg_pdbg("Programming OPCODES... "); curopcodes_done = &O_ST_M25P; rc = program_opcodes(curopcodes_done); + /* Technically not part of opcode init, but it allows opcodes + * to run without transaction errors by setting the lowest + * allowed address to zero. + */ + ich_set_bbar(0); }
if (rc) {
On 28.05.2010 16:32, Carl-Daniel Hailfinger wrote:
ICH SPI can enforces address restrictions for all accesses which take an address (well, it could if the chipset implementation was not broken). Since exploiting the broken implementation is harder than conforming to the address restrictions wherever possible, conform to the address restrictions instead. This patch will eliminate a lot of transaction errors people were seeing (for probe, anyway).
New patch. Added feature: Explicitly check for invalid addresses in the ichspi driver and abort early with SPI_INVALID_ADDRESS.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-ichspi_avoid_transaction_error_use_bbar/flash.h =================================================================== --- flashrom-ichspi_avoid_transaction_error_use_bbar/flash.h (Revision 1015) +++ flashrom-ichspi_avoid_transaction_error_use_bbar/flash.h (Arbeitskopie) @@ -663,6 +663,8 @@ uint32_t spi_get_valid_read_addr(void);
/* ichspi.c */ +extern int ichspi_lock; +extern uint32_t ichspi_bbar; int ich_init_opcodes(void); int ich_spi_send_command(unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr); Index: flashrom-ichspi_avoid_transaction_error_use_bbar/spi.c =================================================================== --- flashrom-ichspi_avoid_transaction_error_use_bbar/spi.c (Revision 1015) +++ flashrom-ichspi_avoid_transaction_error_use_bbar/spi.c (Arbeitskopie) @@ -207,8 +207,22 @@ return spi_programmer[spi_controller].write_256(flash, buf); }
+/* + * Get the lowest allowed address for read accesses. This often happens to + * be the lowest allowed address for all commands which take an address. + * This is a programmer limitation. + */ uint32_t spi_get_valid_read_addr(void) { - /* Need to return BBAR for ICH chipsets. */ - return 0; + switch (spi_controller) { +#if INTERNAL_SUPPORT == 1 +#if defined(__i386__) || defined(__x86_64__) + case SPI_CONTROLLER_ICH7: + /* Return BBAR for ICH chipsets. */ + return ichspi_bbar; +#endif +#endif + default: + return 0; + } } Index: flashrom-ichspi_avoid_transaction_error_use_bbar/chipset_enable.c =================================================================== --- flashrom-ichspi_avoid_transaction_error_use_bbar/chipset_enable.c (Revision 1015) +++ flashrom-ichspi_avoid_transaction_error_use_bbar/chipset_enable.c (Arbeitskopie) @@ -36,8 +36,6 @@
#if defined(__i386__) || defined(__x86_64__)
-extern int ichspi_lock; - static int enable_flash_ali_m1533(struct pci_dev *dev, const char *name) { uint8_t tmp; @@ -515,8 +513,9 @@ msg_pdbg("0x%02x: 0x%08x (SPID%d+4)\n", offs + 4, mmio_readl(spibar + offs + 4), i); } + ichspi_bbar = mmio_readl(spibar + 0x50); msg_pdbg("0x50: 0x%08x (BBAR)\n", - mmio_readl(spibar + 0x50)); + ichspi_bbar); msg_pdbg("0x54: 0x%04x (PREOP)\n", mmio_readw(spibar + 0x54)); msg_pdbg("0x56: 0x%04x (OPTYPE)\n", @@ -587,8 +586,9 @@ mmio_readl(spibar + 0x98)); msg_pdbg("0x9C: 0x%08x (OPMENU+4)\n", mmio_readl(spibar + 0x9C)); + ichspi_bbar = mmio_readl(spibar + 0xA0); msg_pdbg("0xA0: 0x%08x (BBAR)\n", - mmio_readl(spibar + 0xA0)); + ichspi_bbar); msg_pdbg("0xB0: 0x%08x (FDOC)\n", mmio_readl(spibar + 0xB0)); if (tmp2 & (1 << 15)) { Index: flashrom-ichspi_avoid_transaction_error_use_bbar/ichspi.c =================================================================== --- flashrom-ichspi_avoid_transaction_error_use_bbar/ichspi.c (Revision 1015) +++ flashrom-ichspi_avoid_transaction_error_use_bbar/ichspi.c (Arbeitskopie) @@ -103,6 +103,8 @@ /* ICH SPI configuration lock-down. May be set during chipset enabling. */ int ichspi_lock = 0;
+uint32_t ichspi_bbar = 0; + typedef struct _OPCODE { uint8_t opcode; //This commands spi opcode uint8_t spi_type; //This commands spi type @@ -327,6 +329,34 @@ return 0; }
+/* + * Try to set BBAR (BIOS Base Address Register), but read back the value in case + * it didn't stick. + */ +void ich_set_bbar(uint32_t minaddr) +{ + switch (spi_controller) { + case SPI_CONTROLLER_ICH7: + mmio_writel(minaddr, spibar + 0x50); + ichspi_bbar = mmio_readl(spibar + 0x50); + /* We don't have any option except complaining. */ + if (ichspi_bbar != minaddr) + msg_perr("Setting BBAR failed!\n"); + break; + case SPI_CONTROLLER_ICH9: + mmio_writel(minaddr, spibar + 0xA0); + ichspi_bbar = mmio_readl(spibar + 0xA0); + /* We don't have any option except complaining. */ + if (ichspi_bbar != minaddr) + msg_perr("Setting BBAR failed!\n"); + break; + default: + /* Not sure if BBAR actually exists on VIA. */ + msg_pdbg("Setting BBAR is not implemented for VIA yet.\n"); + break; + } +} + /* This function generates OPCODES from or programs OPCODES to ICH according to * the chipset's SPI configuration lock. * @@ -341,13 +371,18 @@ return 0;
if (ichspi_lock) { - msg_pdbg("Generating OPCODES... "); + msg_pdbg("Reading OPCODES... "); curopcodes_done = &O_EXISTING; rc = generate_opcodes(curopcodes_done); } else { msg_pdbg("Programming OPCODES... "); curopcodes_done = &O_ST_M25P; rc = program_opcodes(curopcodes_done); + /* Technically not part of opcode init, but it allows opcodes + * to run without transaction errors by setting the lowest + * allowed address to zero. + */ + ich_set_bbar(0); }
if (rc) { @@ -743,8 +778,22 @@ opcode->spi_type == SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS) { addr = (writearr[1] << 16) | (writearr[2] << 8) | (writearr[3] << 0); + switch (spi_controller) { + case SPI_CONTROLLER_ICH7: + case SPI_CONTROLLER_ICH9: + if (addr < ichspi_bbar) { + msg_perr("%s: Address 0x%06x below allowed " + "range 0x%06x-0xffffff\n", __func__, + addr, ichspi_bbar); + return SPI_INVALID_ADDRESS; + } + break; + default: + break; + } }
+ /* translate read/write array/count */ if (opcode->spi_type == SPI_OPCODE_TYPE_WRITE_NO_ADDRESS) { data = (uint8_t *) (writearr + 1);
On 5/28/10 5:04 PM, Carl-Daniel Hailfinger wrote:
On 28.05.2010 16:32, Carl-Daniel Hailfinger wrote:
ICH SPI can enforces address restrictions for all accesses which take an address (well, it could if the chipset implementation was not broken). Since exploiting the broken implementation is harder than conforming to the address restrictions wherever possible, conform to the address restrictions instead. This patch will eliminate a lot of transaction errors people were seeing (for probe, anyway).
New patch. Added feature: Explicitly check for invalid addresses in the ichspi driver and abort early with SPI_INVALID_ADDRESS.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Acked-by: Stefan Reinauer stepan@coresystems.de
On 28.05.2010 17:43, Stefan Reinauer wrote:
On 5/28/10 5:04 PM, Carl-Daniel Hailfinger wrote:
On 28.05.2010 16:32, Carl-Daniel Hailfinger wrote:
ICH SPI can enforces address restrictions for all accesses which take an address (well, it could if the chipset implementation was not broken). Since exploiting the broken implementation is harder than conforming to the address restrictions wherever possible, conform to the address restrictions instead.
Explicitly check for invalid addresses in the ichspi driver and abort early with SPI_INVALID_ADDRESS.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Acked-by: Stefan Reinauer stepan@coresystems.de
Thanks, committed in r1016 with one minor whitespace change.
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Stefan Reinauer