Am 10.07.2013 21:17 schrieb Stefan Tauner:
Detect and temporarily disable the IMC while accessing the flash. Disable writes on default, but allow the user to enforce it.
Signed-off-by: Rudolf Marek r.marek@assembler.cz Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
imc.c | 171 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
I already mentioned the suboptimal file name and public function names on IRC. You said: "[...] best would be to name it amd_imc.c and name the one external visible function amd_imc_shutdown [...]" That change would be appreciated.
diff --git a/chipset_enable.c b/chipset_enable.c index 3979347..c884c80 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -922,6 +922,10 @@ static int enable_flash_sb600(struct pci_dev *dev, const char *name) uint8_t reg; int ret;
- if (imc_shutdown(dev) != 0) {
Hm. Can we avoid calling this on SB600 (if SB600 doesn't support IMC at all)?
return ERROR_FATAL;- }
- /* Clear ROM protect 0-3. */ for (reg = 0x50; reg < 0x60; reg += 4) { prot = pci_read_long(dev, reg);
diff --git a/sb600spi.c b/sb600spi.c index a5c00d8..7043aec 100644 --- a/sb600spi.c +++ b/sb600spi.c @@ -210,10 +211,26 @@ int sb600_probe_spi(struct pci_dev *dev) struct pci_dev *smbus_dev; uint32_t tmp; uint8_t reg;
- bool amd_imc_force = false;
Ah yes, the joy of having a bool type in C99. It makes sense here.
From a quick glance the code looks sane otherwise. I'll try to evaluate
it in light of the recent discussions about AMD IMC and associated problems.
Regards, Carl-Daniel
On Sun, 14 Jul 2013 01:41:51 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
I already mentioned the suboptimal file name and public function names on IRC. You said: "[...] best would be to name it amd_imc.c and name the one external visible function amd_imc_shutdown [...]" That change would be appreciated.
already done locally and on github ;)
diff --git a/chipset_enable.c b/chipset_enable.c index 3979347..c884c80 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -922,6 +922,10 @@ static int enable_flash_sb600(struct pci_dev *dev, const char *name) uint8_t reg; int ret;
- if (imc_shutdown(dev) != 0) {
Hm. Can we avoid calling this on SB600 (if SB600 doesn't support IMC at all)?
i would rather postpone this for sb600spi.c refactoring where i would like to see some differentiation like in ichspi.c. there are many small differences which we pretty much ignored so far... but i wondered a few times if this is the right place to call it. why not in sb600_probe_spi() for example?
Detect and temporarily disable the IMC while accessing the flash. Disable writes on default, but allow the user to enforce it.
Signed-off-by: Rudolf Marek r.marek@assembler.cz Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at --- Makefile | 2 +- amd_imc.c | 159 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ flashrom.8 | 17 +++++++ programmer.h | 3 ++ sb600spi.c | 75 ++++++++++++++++++++-------- 5 files changed, 235 insertions(+), 21 deletions(-) create mode 100644 amd_imc.c
diff --git a/Makefile b/Makefile index 0cd4e75..c165ef9 100644 --- a/Makefile +++ b/Makefile @@ -433,7 +433,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 ifeq ($(ARCH), x86) -PROGRAMMER_OBJS += it87spi.o it85spi.o sb600spi.o wbsio_spi.o mcp6x_spi.o +PROGRAMMER_OBJS += it87spi.o it85spi.o sb600spi.o amd_imc.o wbsio_spi.o mcp6x_spi.o PROGRAMMER_OBJS += ichspi.o ich_descriptors.o else endif diff --git a/amd_imc.c b/amd_imc.c new file mode 100644 index 0000000..b05390c --- /dev/null +++ b/amd_imc.c @@ -0,0 +1,159 @@ +/* + * This file is part of the flashrom project. + * + * Copyright (C) 2013 Rudolf Marek r.marek@assembler.cz + * Copyright (C) 2013 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 + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#if defined(__i386__) || defined(__x86_64__) + +#include "flash.h" +#include "programmer.h" +#include "hwaccess.h" +#include "spi.h" + +/* same as serverengines */ +static void enter_conf_mode_ec(uint16_t port) +{ + OUTB(0x5a, port); +} + +static void exit_conf_mode_ec(uint16_t port) +{ + OUTB(0xa5, port); +} + +static uint16_t get_sio_port(struct pci_dev *dev) +{ + uint16_t ec_port; + + if (!dev) { + return 0; + } + + ec_port = pci_read_word(dev, 0xa4); + + /* EcPortActive? */ + if (!(ec_port & 0x1)) + return 0; + + ec_port &= ~0x1; + + return ec_port; +} + +/* Wait for up to 10 ms for a response. */ +static int mbox_wait_ack(uint16_t mbox_port) +{ + int i = 10; + while (sio_read(mbox_port, 0x82) != 0xfa) { + if (--i == 0) { + msg_pwarn("IMC MBOX: Timeout!\n"); + return 1; + } + programmer_delay(1000); + } + return 0; +} + +static uint16_t mbox_get_port(uint16_t sio_port) +{ + uint16_t mbox_port; + + enter_conf_mode_ec(sio_port); + + /* Go to LDN 9, mailbox */ + sio_write(sio_port, 7, 9); + + /* MBOX inactive? */ + if ((sio_read(sio_port, 0x30) & 1) == 0) { + exit_conf_mode_ec(sio_port); + return 0; + } + + mbox_port = sio_read(sio_port, 0x60) << 8; + mbox_port |= sio_read(sio_port, 0x61); + + exit_conf_mode_ec(sio_port); + return mbox_port; +} + +/* Returns negative values when IMC is inactive, positive values on errors */ +static int imc_send_cmd(struct pci_dev *dev, uint8_t cmd) +{ + uint16_t sio_port; + uint16_t mbox_port; + + /* IntegratedEcPresent? */ + if (!(pci_read_byte(dev, 0x40) & (1 << 7))) + return -1; + + sio_port = get_sio_port(dev); + if (!sio_port) + return -1; + + msg_pdbg2("IMC SIO is at 0x%x.\n", sio_port); + mbox_port = mbox_get_port(sio_port); + if (!mbox_port) + return -1; + msg_pdbg2("IMC MBOX is at 0x%x.\n", mbox_port); + + sio_write(mbox_port, 0x82, 0x0); + sio_write(mbox_port, 0x83, cmd); + sio_write(mbox_port, 0x84, 0x0); + /* trigger transfer 0x96 with subcommand cmd */ + sio_write(mbox_port, 0x80, 0x96); + + return mbox_wait_ack(mbox_port); +} + +static int imc_resume(void *data) +{ + struct pci_dev *dev = data; + int ret = imc_send_cmd(dev, 0xb5); + + if (ret != 0) + msg_pinfo("Resuming IMC failed)\n"); + else + msg_pdbg2("IMC resumed.\n"); + return ret; +} + +int amd_imc_shutdown(struct pci_dev *dev) +{ + /* Try to put IMC to sleep */ + int ret = imc_send_cmd(dev, 0xb4); + + /* No IMC activity detectable, assume we are fine */ + if (ret < 0) { + msg_pdbg2("No IMC found.\n"); + return 0; + } + + if (ret != 0) { + msg_perr("Shutting down IMC failed.\n"); + return ret; + } + msg_pdbg2("Shutting down IMC successful.\n"); + + if (register_shutdown(imc_resume, dev)) + return 1; + + return ret; +} + +#endif diff --git a/flashrom.8 b/flashrom.8 index dfca222..67acef4 100644 --- a/flashrom.8 +++ b/flashrom.8 @@ -329,6 +329,23 @@ flashrom doesn't detect an active IT87 LPC<->SPI bridge, please send a bug report so we can diagnose the problem. .sp .TP +.B AMD chipsets +.sp +Beginning with the SB700 chipset there is an integrated microcontroller (IMC) based on the 8051 embedded in +every AMD southbridge. Its firmware resides in the same flash chip as the host's which makes writing to the +flash risky if the IMC is active. Flashrom tries to temporarily disable the IMC but even then changing the +contents of the flash can have unwanted effects: when the IMC continues (at the latest after a reboot) it will +continue executing code from the flash. If the code was removed or changed in an unfortunate way it is +unpredictable what the IMC will do. Therefore, if flashrom detects an active IMC it will disable write support +unless the user forces it with the +.sp +.B " flashrom -p internal:amd_imc_force=yes" +.sp +syntax. The user is responsible for supplying a suitable image or leaving out the IMC region with the help of +a layout file. This limitation might be removed in the future when we understand the details better and have +received enough feedback from users. Please report the outcome if you had to use this option to write a chip. +.sp +.TP .B Intel chipsets .sp If you have an Intel chipset with an ICH8 or later southbridge with SPI flash diff --git a/programmer.h b/programmer.h index db914cb..f03eac3 100644 --- a/programmer.h +++ b/programmer.h @@ -572,6 +572,9 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb, enum ich_chipset ich_generation); int via_init_spi(struct pci_dev *dev, uint32_t mmio_base);
+/* imc.c */ +int amd_imc_shutdown(struct pci_dev *dev); + /* it85spi.c */ int it85xx_spi_init(struct superio s);
diff --git a/sb600spi.c b/sb600spi.c index a5c00d8..e76c04a 100644 --- a/sb600spi.c +++ b/sb600spi.c @@ -23,6 +23,8 @@
#if defined(__i386__) || defined(__x86_64__)
+#include <string.h> +#include <stdlib.h> #include "flash.h" #include "programmer.h" #include "hwaccess.h" @@ -47,7 +49,7 @@ static void reset_internal_fifo_pointer(void) { mmio_writeb(mmio_readb(sb600_spibar + 2) | 0x10, sb600_spibar + 2);
- /* FIXME: This loop makes no sense at all. */ + /* FIXME: This loop needs a timeout and a clearer message. */ while (mmio_readb(sb600_spibar + 0xD) & 0x7) msg_pspew("reset\n"); } @@ -59,8 +61,7 @@ static int compare_internal_fifo_pointer(uint8_t want) tmp = mmio_readb(sb600_spibar + 0xd) & 0x07; want &= 0x7; if (want != tmp) { - msg_perr("SB600 FIFO pointer corruption! Pointer is %d, wanted " - "%d\n", tmp, want); + msg_perr("FIFO pointer corruption! Pointer is %d, wanted %d\n", tmp, want); msg_perr("Something else is accessing the flash chip and " "causes random corruption.\nPlease stop all " "applications and drivers and IPMI which access the " @@ -194,6 +195,39 @@ static int sb600_spi_send_command(struct flashctx *flash, unsigned int writecnt, return 0; }
+static int sb600_handle_imc(struct pci_dev *dev, bool amd_imc_force) +{ + /* Handle IMC everywhere but sb600 which does not have one. */ + if (dev->device_id == 0x438d) + return 0; + + /* TODO: we should not only look at IntegratedImcPresent (LPC Dev 20, Func 3, 40h) but also at + * IMCEnable(Strap) and Override EcEnable(Strap) (sb8xx, sb9xx?, a50: Misc_Reg: 80h-87h; + * sb7xx, sp5100: PM_Reg: B0h-B1h) etc. */ + uint8_t reg = pci_read_byte(dev, 0x40); + if ((reg & (1 << 7)) == 0) { + msg_pdbg("IMC is not active.\n"); + return 0; + } + + if (!amd_imc_force) + programmer_may_write = 0; + msg_pinfo("Writes have been disabled for safety reasons because the IMC is active\n" + "and it could interfere with accessing flash memory. Flashrom will try\n" + "to disable it temporarily but even then this might not be safe:\n" + "when it is reenabled and after a reboot it expects to find working code\n" + "in the flash and it is unpredictable what happens if there is none.\n" + "\n" + "To be safe make sure that there is a working IMC firmware at the right\n" + "location in the image you intend to write and do not attempt to erase.\n" + "\n" + "You can enforce write support with the amd_imc_force programmer option.\n"); + if (amd_imc_force) + msg_pinfo("Continuing with write support because the user forced us to!\n"); + + return amd_imc_shutdown(dev); +} + static const struct spi_programmer spi_programmer_sb600 = { .type = SPI_CONTROLLER_SB600, .max_data_read = 8, @@ -210,10 +244,26 @@ int sb600_probe_spi(struct pci_dev *dev) struct pci_dev *smbus_dev; uint32_t tmp; uint8_t reg; + bool amd_imc_force = false; static const char *const speed_names[4] = { "66/reserved", "33", "22", "16.5" };
+ char *arg = extract_programmer_param("amd_imc_force"); + if (arg && !strcmp(arg, "yes")) { + amd_imc_force = true; + msg_pspew("amd_imc_force enabled.\n"); + } else if (arg && !strlen(arg)) { + msg_perr("Missing argument for amd_imc_force.\n"); + free(arg); + return ERROR_FATAL; + } else if (arg) { + msg_perr("Unknown argument for amd_imc_force: "%s" (not "yes").\n", arg); + free(arg); + return ERROR_FATAL; + } + free(arg); + /* Read SPI_BaseAddr */ tmp = pci_read_long(dev, 0xa0); tmp &= 0xffffffe0; /* remove bits 4-0 (reserved) */ @@ -300,23 +350,8 @@ int sb600_probe_spi(struct pci_dev *dev) return 0; }
- reg = pci_read_byte(dev, 0x40); - msg_pdbg("SB700 IMC is %sactive.\n", (reg & (1 << 7)) ? "" : "not "); - if (reg & (1 << 7)) { - /* If we touch any region used by the IMC, the IMC and the SPI - * interface will lock up, and the only way to recover is a - * hard reset, but that is a bad choice for a half-erased or - * half-written flash chip. - * There appears to be an undocumented register which can freeze - * or disable the IMC, but for now we want to play it safe. - */ - msg_perr("The SB700 IMC is active and may interfere with SPI " - "commands. Disabling write.\n"); - /* FIXME: Should we only disable SPI writes, or will the lockup - * affect LPC/FWH chips as well? - */ - programmer_may_write = 0; - } + if (sb600_handle_imc(dev, amd_imc_force) != 0) + return ERROR_FATAL;
/* Bring the FIFO to a clean state. */ reset_internal_fifo_pointer();
-
Carl-Daniel Hailfinger -
Stefan Tauner