Author: hailfinger
Date: Thu Feb 16 21:31:25 2012
New Revision: 1496
URL: http://flashrom.org/trac/flashrom/changeset/1496
Log:
Reenable forced read
Forced read functionality was disabled when programmer registration was
merged in r1475.
We now support registering more than one controller at once for each bus
type. This can happen e.g. if one SPI controller has an attached flash
chip and one controller doesn't. In such a case we rely on the probe
mechanism to find exactly one chip, and the probe mechanism will
remember which controller/bus the flash chip is attached to. A forced
read does not have the luxury of knowing which compatible controller to
use, so this case is handled by always picking the first one. That may
or may not be the correct one, but there is no way (yet) to specify
which controller a flash chip is attached to.
Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Acked-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
Modified:
trunk/cli_classic.c
Modified: trunk/cli_classic.c
==============================================================================
--- trunk/cli_classic.c Thu Feb 16 02:43:06 2012 (r1495)
+++ trunk/cli_classic.c Thu Feb 16 21:31:25 2012 (r1496)
@@ -168,7 +168,7 @@
struct flashctx *fill_flash;
const char *name;
int namelen, opt, i, j;
- int startchip = 0, chipcount = 0, option_index = 0, force = 0;
+ int startchip = -1, chipcount = 0, option_index = 0, force = 0;
#if CONFIG_PRINT_WIKI == 1
int list_supported_wiki = 0;
#endif
@@ -456,11 +456,27 @@
printf("Note: flashrom can never write if the flash "
"chip isn't found automatically.\n");
}
-#if 0 // FIXME: What happens for a forced chip read if multiple compatible programmers are registered?
if (force && read_it && chip_to_probe) {
+ struct registered_programmer *pgm;
+ int compatible_programmers = 0;
printf("Force read (-f -r -c) requested, pretending "
"the chip is there:\n");
- startchip = probe_flash(0, &flashes[0], 1);
+ /* This loop just counts compatible controllers. */
+ for (j = 0; j < registered_programmer_count; j++) {
+ pgm = ®istered_programmers[j];
+ if (pgm->buses_supported & flashes[0].bustype)
+ compatible_programmers++;
+ }
+ if (compatible_programmers > 1)
+ printf("More than one compatible controller "
+ "found for the requested flash chip, "
+ "using the first one.\n");
+ for (j = 0; j < registered_programmer_count; j++) {
+ pgm = ®istered_programmers[j];
+ startchip = probe_flash(pgm, 0, &flashes[0], 1);
+ if (startchip != -1)
+ break;
+ }
if (startchip == -1) {
printf("Probing for flash chip '%s' failed.\n",
chip_to_probe);
@@ -471,7 +487,6 @@
"contain garbage.\n");
return read_flash_to_file(&flashes[0], filename);
}
-#endif
ret = 1;
goto out_shutdown;
} else if (!chip_to_probe) {
Am 16.02.2012 02:30 schrieb Idwer Vollering:
> 2012/2/16 Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>:
>> > MinGW uses standard Windows C libraries and those apparently don't
>> > support %hhx for sscanf into a uint8_t. SCNx8 isn't available either.
>> >
>> > Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
> Acked-by: Idwer Vollering <vidwer(a)gmail.com>
Thanks, committed in r1495.
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
Author: hailfinger
Date: Thu Feb 16 02:43:06 2012
New Revision: 1495
URL: http://flashrom.org/trac/flashrom/changeset/1495
Log:
Workaround missing %hhx support in MinGW sscanf
MinGW uses standard Windows C libraries and those apparently don't
support %hhx for sscanf into a uint8_t. SCNx8 isn't available either.
Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Acked-by: Idwer Vollering <vidwer(a)gmail.com>
Modified:
trunk/dummyflasher.c
Modified: trunk/dummyflasher.c
==============================================================================
--- trunk/dummyflasher.c Thu Feb 16 02:13:00 2012 (r1494)
+++ trunk/dummyflasher.c Thu Feb 16 02:43:06 2012 (r1495)
@@ -199,7 +199,12 @@
}
}
for (i = 0; i < spi_blacklist_size; i++) {
- sscanf(tmp + i * 2, "%2hhx", &spi_blacklist[i]);
+ unsigned int tmp2;
+ /* SCNx8 is apparently not supported by MSVC (and thus
+ * MinGW), so work around it with an extra variable
+ */
+ sscanf(tmp + i * 2, "%2x", &tmp2);
+ spi_blacklist[i] = (uint8_t)tmp2;
}
msg_pdbg("SPI blacklist is ");
for (i = 0; i < spi_blacklist_size; i++)
@@ -230,7 +235,12 @@
}
}
for (i = 0; i < spi_ignorelist_size; i++) {
- sscanf(tmp + i * 2, "%2hhx", &spi_ignorelist[i]);
+ unsigned int tmp2;
+ /* SCNx8 is apparently not supported by MSVC (and thus
+ * MinGW), so work around it with an extra variable
+ */
+ sscanf(tmp + i * 2, "%2x", &tmp2);
+ spi_ignorelist[i] = (uint8_t)tmp2;
}
msg_pdbg("SPI ignorelist is ");
for (i = 0; i < spi_ignorelist_size; i++)
MinGW uses standard Windows C libraries and those apparently don't
support %hhx for sscanf into a uint8_t. SCNx8 isn't available either.
Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Index: flashrom-mingw_workaround_broken_sscanf_uint8t/dummyflasher.c
===================================================================
--- flashrom-mingw_workaround_broken_sscanf_uint8t/dummyflasher.c (Revision 1492)
+++ flashrom-mingw_workaround_broken_sscanf_uint8t/dummyflasher.c (Arbeitskopie)
@@ -199,7 +199,12 @@
}
}
for (i = 0; i < spi_blacklist_size; i++) {
- sscanf(tmp + i * 2, "%2hhx", &spi_blacklist[i]);
+ unsigned int tmp2;
+ /* SCNx8 is apparently not supported by MSVC (and thus
+ * MinGW), so work around it with an extra variable
+ */
+ sscanf(tmp + i * 2, "%2x", &tmp2);
+ spi_blacklist[i] = (uint8_t)tmp2;
}
msg_pdbg("SPI blacklist is ");
for (i = 0; i < spi_blacklist_size; i++)
@@ -230,7 +235,12 @@
}
}
for (i = 0; i < spi_ignorelist_size; i++) {
- sscanf(tmp + i * 2, "%2hhx", &spi_ignorelist[i]);
+ unsigned int tmp2;
+ /* SCNx8 is apparently not supported by MSVC (and thus
+ * MinGW), so work around it with an extra variable
+ */
+ sscanf(tmp + i * 2, "%2x", &tmp2);
+ spi_ignorelist[i] = (uint8_t)tmp2;
}
msg_pdbg("SPI ignorelist is ");
for (i = 0; i < spi_ignorelist_size; i++)
--
http://www.hailfinger.org/
Am 16.02.2012 00:58 schrieb Stefan Tauner:
> This includes not only the notorious read-only flash descriptors and locked ME
> regions, but also the more rarely used PRs (Protected Ranges).
> The user can enforce write support by specifying ich_spi_force=yes in the
> programmer options, but we don't tell him the exact syntax interactively. He
> has to read it up in the man page.
>
> Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
> Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
The Intel rant seems to be missing... I thought you only wanted to move it.
> diff --git a/flashrom.8 b/flashrom.8
> index e5f9a29..76aacba 100644
> --- a/flashrom.8
> +++ b/flashrom.8
> @@ -315,6 +315,21 @@ important opcodes are inaccessible due to lockdown; or if more than one flash
> chip is attached). The other options (swseq, hwseq) select the respective mode
> (if possible).
> .sp
> +ICH8 and later southbridges may also have locked address ranges of different
> +kinds if a valid descriptor was written to it. The flash address space is then
> +partitioned in multiple so called "Flash Regions" containing the host firmware,
> +the ME firmware and so on respectively. The flash descriptor can also specify up
> +to 5 so called "Protected Regions", which are freely chosen address ranges
> +independent from the aforementioned "Flash Regions". All of them can be write
> +and/or read protected individually. If flashrom detects such a lock it will
> +disable write support unless the user forces it with the
> +.sp
> +.B " flashrom \-p internal:ich_spi_force=yes"
> +.sp
> +syntax. If this leads to erase or write accesses to the flash it would most
> +probably bring it into an inconsistent and unbootable state and we will not
> +provide any support in such a case.
> +.sp
> If you have an Intel chipset with an ICH6 or later southbridge and if you want
> to set specific IDSEL values for a non-default flash chip or an embedded
> controller (EC), you can use the
> diff --git a/ichspi.c b/ichspi.c
> index 163ecf1..711f46c 100644
> --- a/ichspi.c
> +++ b/ichspi.c
> @@ -1444,11 +1445,16 @@ static void do_ich9_spi_frap(uint32_t frap, int i)
> if (base > limit) {
> /* this FREG is disabled */
> msg_pdbg("%s region is unused.\n", region_names[i]);
> - return;
> + return 0;
> }
>
> - msg_pdbg("0x%08x-0x%08x is %s\n", base, (limit | 0x0fff),
> + msg_pdbg("0x%08x-0x%08x is %s.\n", base, (limit | 0x0fff),
> access_names[rwperms]);
> + if (rwperms == 0x3)
> + return 0;
> +
> + msg_pinfo("WARNING: %s region is not fully accessible.\n", region_names[i]);
Odd. For FRAP, the _pinfo message just says "not fully accessible", but
for PR, the _pinfo message mentions the actual read/write protection.
Not terribly important to fix, just a small inconsistency I noticed.
> + return 1;
> }
>
> /* In contrast to FRAP and the master section of the descriptor the bits
> @@ -1460,21 +1466,25 @@ static void do_ich9_spi_frap(uint32_t frap, int i)
> #define ICH_PR_PERMS(pr) (((~((pr) >> PR_RP_OFF) & 1) << 0) | \
> ((~((pr) >> PR_WP_OFF) & 1) << 1))
>
> -static void prettyprint_ich9_reg_pr(int i)
> +/* returns 0 if range is unused (i.e. r/w) */
> +static int ich9_handle_pr(int i)
> {
> - static const char *const access_names[4] = {
> - "locked", "read-only", "write-only", "read-write"
> + static const char *const access_names[3] = {
> + "locked", "read-only", "write-only"
> };
> uint8_t off = ICH9_REG_PR0 + (i * 4);
> uint32_t pr = mmio_readl(ich_spibar + off);
> - int rwperms = ICH_PR_PERMS(pr);
> + unsigned int rwperms = ICH_PR_PERMS(pr);
>
> - msg_pdbg2("0x%02X: 0x%08x (PR%u", off, pr, i);
> - if (rwperms != 0x3)
> - msg_pdbg2(")\n0x%08x-0x%08x is %s\n", ICH_FREG_BASE(pr),
> - ICH_FREG_LIMIT(pr) | 0x0fff, access_names[rwperms]);
> - else
> - msg_pdbg2(", unused)\n");
> + if (rwperms == 0x3) {
> + msg_pdbg2("0x%02X: 0x%08x (PR%u is unused)\n", off, pr, i);
> + return 0;
> + }
> +
> + msg_pdbg("0x%02X: 0x%08x ", off, pr);
> + msg_pinfo("WARNING: PR%u: 0x%08x-0x%08x is %s.\n", i, ICH_FREG_BASE(pr),
> + ICH_FREG_LIMIT(pr) | 0x0fff, access_names[rwperms]);
> + return 1;
> }
>
> /* Set/Clear the read and write protection enable bits of PR register @i
> @@ -1537,6 +1547,8 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
> uint16_t spibar_offset, tmp2;
> uint32_t tmp;
> char *arg;
> + int ich_spi_force = 0;
> + int ich_spi_has_locks = 0;
Rename to ich_spi_has_region_locks or ich_spi_has_locked_regions or
somesuch. Otherwise there is possibility for confusion between
ich_spi_has_locks and ichspi_lock.
> int desc_valid = 0;
> struct ich_descriptors desc = {{ 0 }};
> enum ich_spi_mode {
> @@ -1665,17 +1693,36 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
> msg_pdbg("BRWA 0x%02x, ", ICH_BRWA(tmp));
> msg_pdbg("BRRA 0x%02x\n", ICH_BRRA(tmp));
>
> - /* Decode and print FREGx and FRAP registers */
> + /* Handle FREGx and FRAP registers */
> for (i = 0; i < 5; i++)
> - do_ich9_spi_frap(tmp, i);
> + ich_spi_has_locks |= ich9_handle_frap(tmp, i);
> }
>
> - /* try to disable PR locks before printing them */
> - if (!ichspi_lock)
> - for (i = 0; i < 5; i++)
> + for (i = 0; i < 5; i++) {
> + /* if not locked down try to disable PR locks first */
> + if (!ichspi_lock)
> ich9_set_pr(i, 0, 0);
> - for (i = 0; i < 5; i++)
> - prettyprint_ich9_reg_pr(i);
> + ich_spi_has_locks |= ich9_handle_pr(i);
> + }
> +
> + if (ich_spi_has_locks) {
>
Thanks for working tirelessly to get this code into an excellent shape!
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
Author: stefanct
Date: Thu Feb 16 02:13:00 2012
New Revision: 1494
URL: http://flashrom.org/trac/flashrom/changeset/1494
Log:
ichspi.c: warn user and disable writes when a protected address range is detected.
This includes not only the notorious read-only flash descriptors and locked ME
regions, but also the more rarely used PRs (Protected Ranges).
The user can enforce write support by specifying ich_spi_force=yes in the
programmer options, but we don't tell him the exact syntax interactively. He
has to read it up in the man page.
Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Modified:
trunk/flashrom.8
trunk/ichspi.c
Modified: trunk/flashrom.8
==============================================================================
--- trunk/flashrom.8 Thu Feb 16 00:40:23 2012 (r1493)
+++ trunk/flashrom.8 Thu Feb 16 02:13:00 2012 (r1494)
@@ -315,6 +315,21 @@
chip is attached). The other options (swseq, hwseq) select the respective mode
(if possible).
.sp
+ICH8 and later southbridges may also have locked address ranges of different
+kinds if a valid descriptor was written to it. The flash address space is then
+partitioned in multiple so called "Flash Regions" containing the host firmware,
+the ME firmware and so on respectively. The flash descriptor can also specify up
+to 5 so called "Protected Regions", which are freely chosen address ranges
+independent from the aforementioned "Flash Regions". All of them can be write
+and/or read protected individually. If flashrom detects such a lock it will
+disable write support unless the user forces it with the
+.sp
+.B " flashrom \-p internal:ich_spi_force=yes"
+.sp
+syntax. If this leads to erase or write accesses to the flash it would most
+probably bring it into an inconsistent and unbootable state and we will not
+provide any support in such a case.
+.sp
If you have an Intel chipset with an ICH6 or later southbridge and if you want
to set specific IDSEL values for a non-default flash chip or an embedded
controller (EC), you can use the
Modified: trunk/ichspi.c
==============================================================================
--- trunk/ichspi.c Thu Feb 16 00:40:23 2012 (r1493)
+++ trunk/ichspi.c Thu Feb 16 02:13:00 2012 (r1494)
@@ -1421,7 +1421,8 @@
#define ICH_BRWA(x) ((x >> 8) & 0xff)
#define ICH_BRRA(x) ((x >> 0) & 0xff)
-static void do_ich9_spi_frap(uint32_t frap, int i)
+/* returns 0 if region is unused or r/w */
+static int ich9_handle_frap(uint32_t frap, int i)
{
static const char *const access_names[4] = {
"locked", "read-only", "write-only", "read-write"
@@ -1436,19 +1437,26 @@
int offset = ICH9_REG_FREG0 + i * 4;
uint32_t freg = mmio_readl(ich_spibar + offset);
- 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 > limit) {
/* this FREG is disabled */
- msg_pdbg("%s region is unused.\n", region_names[i]);
- return;
+ msg_pdbg2("0x%02X: 0x%08x FREG%i: %s region is unused.\n",
+ offset, freg, i, region_names[i]);
+ return 0;
+ }
+ msg_pdbg("0x%02X: 0x%08x ", offset, freg);
+ if (rwperms == 0x3) {
+ msg_pdbg("FREG%i: %s region (0x%08x-0x%08x) is %s.\n", i,
+ region_names[i], base, (limit | 0x0fff),
+ access_names[rwperms]);
+ return 0;
}
- msg_pdbg("0x%08x-0x%08x is %s\n", base, (limit | 0x0fff),
- access_names[rwperms]);
+ msg_pinfo("FREG%i: WARNING: %s region (0x%08x-0x%08x) is %s.\n", i,
+ region_names[i], base, (limit | 0x0fff),
+ access_names[rwperms]);
+ return 1;
}
/* In contrast to FRAP and the master section of the descriptor the bits
@@ -1460,21 +1468,25 @@
#define ICH_PR_PERMS(pr) (((~((pr) >> PR_RP_OFF) & 1) << 0) | \
((~((pr) >> PR_WP_OFF) & 1) << 1))
-static void prettyprint_ich9_reg_pr(int i)
+/* returns 0 if range is unused (i.e. r/w) */
+static int ich9_handle_pr(int i)
{
- static const char *const access_names[4] = {
- "locked", "read-only", "write-only", "read-write"
+ static const char *const access_names[3] = {
+ "locked", "read-only", "write-only"
};
uint8_t off = ICH9_REG_PR0 + (i * 4);
uint32_t pr = mmio_readl(ich_spibar + off);
- int rwperms = ICH_PR_PERMS(pr);
+ unsigned int rwperms = ICH_PR_PERMS(pr);
- msg_pdbg2("0x%02X: 0x%08x (PR%u", off, pr, i);
- if (rwperms != 0x3)
- msg_pdbg2(")\n0x%08x-0x%08x is %s\n", ICH_FREG_BASE(pr),
- ICH_FREG_LIMIT(pr) | 0x0fff, access_names[rwperms]);
- else
- msg_pdbg2(", unused)\n");
+ if (rwperms == 0x3) {
+ msg_pdbg2("0x%02X: 0x%08x (PR%u is unused)\n", off, pr, i);
+ return 0;
+ }
+
+ msg_pdbg("0x%02X: 0x%08x ", off, pr);
+ msg_pinfo("PR%u: WARNING: 0x%08x-0x%08x is %s.\n", i, ICH_FREG_BASE(pr),
+ ICH_FREG_LIMIT(pr) | 0x0fff, access_names[rwperms]);
+ return 1;
}
/* Set/Clear the read and write protection enable bits of PR register @i
@@ -1537,6 +1549,8 @@
uint16_t spibar_offset, tmp2;
uint32_t tmp;
char *arg;
+ int ich_spi_force = 0;
+ int ich_spi_rw_restricted = 0;
int desc_valid = 0;
struct ich_descriptors desc = {{ 0 }};
enum ich_spi_mode {
@@ -1631,6 +1645,22 @@
}
free(arg);
+ arg = extract_programmer_param("ich_spi_force");
+ if (arg && !strcmp(arg, "yes")) {
+ ich_spi_force = 1;
+ msg_pspew("ich_spi_force enabled.\n");
+ } else if (arg && !strlen(arg)) {
+ msg_perr("Missing argument for ich_spi_force.\n");
+ free(arg);
+ return ERROR_FATAL;
+ } else if (arg) {
+ msg_perr("Unknown argument for ich_spi_force: \"%s\" "
+ "(not \"yes\").\n", arg);
+ free(arg);
+ return ERROR_FATAL;
+ }
+ free(arg);
+
tmp2 = mmio_readw(ich_spibar + ICH9_REG_HSFS);
msg_pdbg("0x04: 0x%04x (HSFS)\n", tmp2);
prettyprint_ich9_reg_hsfs(tmp2);
@@ -1665,17 +1695,36 @@
msg_pdbg("BRWA 0x%02x, ", ICH_BRWA(tmp));
msg_pdbg("BRRA 0x%02x\n", ICH_BRRA(tmp));
- /* Decode and print FREGx and FRAP registers */
+ /* Handle FREGx and FRAP registers */
for (i = 0; i < 5; i++)
- do_ich9_spi_frap(tmp, i);
+ ich_spi_rw_restricted |= ich9_handle_frap(tmp, i);
}
- /* try to disable PR locks before printing them */
- if (!ichspi_lock)
- for (i = 0; i < 5; i++)
+ for (i = 0; i < 5; i++) {
+ /* if not locked down try to disable PR locks first */
+ if (!ichspi_lock)
ich9_set_pr(i, 0, 0);
- for (i = 0; i < 5; i++)
- prettyprint_ich9_reg_pr(i);
+ ich_spi_rw_restricted |= ich9_handle_pr(i);
+ }
+
+ if (ich_spi_rw_restricted) {
+ msg_pinfo("Please send a verbose log to "
+ "flashrom(a)flashrom.org if this board is not "
+ "listed on\n"
+ "http://flashrom.org/Supported_hardware#Supported_mainboards "
+ "yet.\n");
+ if (!ich_spi_force)
+ programmer_may_write = 0;
+ msg_pinfo("Writes have been disabled. You can enforce "
+ "write support with the\nich_spi_force "
+ "programmer option, but it will most likely "
+ "harm your hardware!\nIf you force flashrom "
+ "you will get no support if something "
+ "breaks.\n");
+ if (ich_spi_force)
+ msg_pinfo("Continuing with write support "
+ "because the user forced us to!\n");
+ }
tmp = mmio_readl(ich_spibar + ICH9_REG_SSFS);
msg_pdbg("0x90: 0x%02x (SSFS)\n", tmp & 0xff);
Am 14.02.2012 16:39 schrieb Stefan Tauner:
> This includes not only the notorious read-only flash descriptors and locked ME
> regions, but also the more rarely used PRs (Protected Ranges).
> The user can enforce write support by specifying ich_spi_force=yes in the
Can you rename ich_spi_force to ich_ignore_locks or something similar?
The PR stuff is not limited to SPI IIRC.
> programmer options, but we don't tell him the exact syntax interactively. He
> has to read it up in the man page.
> ---
> non-verbose sample output from my laptop (that contains both protection types):
> […]
> Found chipset "Intel QS57". Enabling flash write... WARNING: SPI Configuration Lockdown activated.
> WARNING: Flash Descriptor region is not fully accessible and flashrom can
> not deal with this correctly yet.
> Intel does not provide us the necessary documention to support this.
> Please send a verbose log to flashrom(a)flashrom.org if this board is not listed on
> http://flashrom.org/Supported_hardware#Supported_mainboards yet.
> Writes have been disabled. You can enforce write support with the
> ich_spi_force programmer option, but it will most likely harm your hardware!
> If you force flashrom you will get no support if something breaks.
> WARNING: Management Engine region is not fully accessible and flashrom can
> not deal with this correctly yet.
> WARNING: PR0: 0x007d0000-0x01ffffff is read-only.
> OK.
> […]
Ugh. This is the non-verbose version? I had trouble parsing that message
flood on the first attempt. May I suggest an alternative wording?
WARNING: Flash Descriptor region is not fully accessible.
WARNING: Management Engine region is not fully accessible.
WARNING: PR0: 0x007d0000-0x01ffffff is read-only.
Please send a verbose log to flashrom(a)flashrom.org if this board is not listed on
http://flashrom.org/Supported_hardware#Supported_mainboards yet.
Writes have been disabled. You can force writing with the ich_ignore_locks
programmer option, but it will most likely brick your mainboard.
The idea of my wording is to postpone the explanatory message until all
"not accessible"/"read-only" messages have been printed.
The complaint about Intel belongs to the man page or to intel_mysteries.txt
(if you choose the latter, just mention intel_mysteries.txt at the relevant
location in the man page).
For the override case, just print the message above and additionally
"You have been warned. We will not support you if write/erase bricks your
mainboard. Proceeding anyway because user specified ich_ignore_locks."
or something similar.
> Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
> ---
> flashrom.8 | 15 ++++++++++
> ichspi.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++------------
> 2 files changed, 86 insertions(+), 19 deletions(-)
>
> diff --git a/flashrom.8 b/flashrom.8
> index 2f23cb8..0ca4fdb 100644
> --- a/flashrom.8
> +++ b/flashrom.8
> @@ -315,6 +315,21 @@ important opcodes are inaccessible due to lockdown; or if more than one flash
> chip is attached). The other options (swseq, hwseq) select the respective mode
> (if possible).
> .sp
> +ICH8 and later southbridges may also have locked address ranges of different
> +kinds if a valid descriptor was written to it. The flash address space is then
> +partitioned in multiple so called "Flash Regions" containing the host firmware,
> +the ME firmware and so on respectively. The flash descriptor can also specify up
> +to 5 so called "Protected Regions", which are freely chosen address ranges
> +independent from the aforementioned "Flash Regions". All of them can be write
> +and/or read protected individually. If flashrom detects such a lock it will
> +disable write support unless the user forces it with the
> +.sp
> +.B " flashrom \-p internal:ich_spi_force=yes"
> +.sp
> +syntax. If this leads to erase or write accesses to the flash it would most
> +probably bring it into an inconsistent and unbootable state and we will not
> +provide any support in such a case.
> +.sp
> If you have an Intel chipset with an ICH6 or later southbridge and if you want
> to set specific IDSEL values for a non-default flash chip or an embedded
> controller (EC), you can use the
> diff --git a/ichspi.c b/ichspi.c
> index 163ecf1..f21bb35 100644
> --- a/ichspi.c
> +++ b/ichspi.c
> @@ -1416,12 +1416,36 @@ static int ich_spi_send_multicommand(struct flashctx *flash,
> return ret;
> }
>
> +static void ich9_disable_writes(int force, const char * const msg)
> +{
> + /* Don't scare and spam the user even more if write support was already
> + * disabled */
> + if (!programmer_may_write)
> + return;
> +
> + msg_pinfo("%s", msg);
> + msg_pinfo("Please send a verbose log to flashrom(a)flashrom.org if this "
> + "board is not listed on\n"
> + "http://flashrom.org/Supported_hardware#Supported_mainboards "
> + "yet.\n");
> + if (!force) {
> + programmer_may_write = 0;
> + msg_pinfo("Writes have been disabled. You can enforce write "
> + "support with the\nich_spi_force programmer option, "
> + "but it will most likely harm your hardware!\n");
> + }
> + msg_pinfo("If you force flashrom you will get no support if something "
> + "breaks.\n");
> + if (force)
> + msg_pinfo("Continuing anyway because the user forced us to!\n");
> +}
> +
> #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)
>
> -static void do_ich9_spi_frap(uint32_t frap, int i)
> +static void ich9_handle_frap(uint32_t frap, int i, int force)
> {
> static const char *const access_names[4] = {
> "locked", "read-only", "write-only", "read-write"
> @@ -1447,8 +1471,15 @@ static void do_ich9_spi_frap(uint32_t frap, int i)
> return;
> }
>
> - msg_pdbg("0x%08x-0x%08x is %s\n", base, (limit | 0x0fff),
> + msg_pdbg("0x%08x-0x%08x is %s.\n", base, (limit | 0x0fff),
> access_names[rwperms]);
> + if (rwperms == 0x3)
> + return;
> +
> + msg_pinfo("WARNING: %s region is not fully accessible and flashrom "
> + "can\nnot deal with this correctly yet.\n", region_names[i]);
> + ich9_disable_writes(force, "Intel does not provide us the necessary "
> + "documention to support this.\n");
> }
>
> /* In contrast to FRAP and the master section of the descriptor the bits
> @@ -1460,21 +1491,25 @@ static void do_ich9_spi_frap(uint32_t frap, int i)
> #define ICH_PR_PERMS(pr) (((~((pr) >> PR_RP_OFF) & 1) << 0) | \
> ((~((pr) >> PR_WP_OFF) & 1) << 1))
>
> -static void prettyprint_ich9_reg_pr(int i)
> +static void ich9_handle_pr(int i, int force)
> {
> - static const char *const access_names[4] = {
> - "locked", "read-only", "write-only", "read-write"
> + static const char *const access_names[3] = {
> + "locked", "read-only", "write-only"
> };
> uint8_t off = ICH9_REG_PR0 + (i * 4);
> uint32_t pr = mmio_readl(ich_spibar + off);
> - int rwperms = ICH_PR_PERMS(pr);
> + unsigned int rwperms = ICH_PR_PERMS(pr);
>
> - msg_pdbg2("0x%02X: 0x%08x (PR%u", off, pr, i);
> - if (rwperms != 0x3)
> - msg_pdbg2(")\n0x%08x-0x%08x is %s\n", ICH_FREG_BASE(pr),
> - ICH_FREG_LIMIT(pr) | 0x0fff, access_names[rwperms]);
> - else
> - msg_pdbg2(", unused)\n");
> + if (rwperms >= 0x3) {
Why >= 0x3? AFAICS the ICH_PR_PERMS macro can only have values between
0x0 and 0x3.
> + msg_pdbg2("0x%02X: 0x%08x (PR%u is unused)\n", off, pr, i);
> + return;
> + }
> +
> + msg_pdbg("0x%02X: 0x%08x ", off, pr);
> + msg_pinfo("WARNING: PR%u: 0x%08x-0x%08x is %s.\n", i, ICH_FREG_BASE(pr),
> + ICH_FREG_LIMIT(pr) | 0x0fff, access_names[rwperms]);
> + ich9_disable_writes(force, "There is no way to change this from within "
> + "the system.\n");
> }
>
> /* Set/Clear the read and write protection enable bits of PR register @i
> @@ -1537,6 +1572,7 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
> uint16_t spibar_offset, tmp2;
> uint32_t tmp;
> char *arg;
> + int ich_spi_force = 0;
> int desc_valid = 0;
> struct ich_descriptors desc = {{ 0 }};
> enum ich_spi_mode {
> @@ -1631,6 +1667,22 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
> }
> free(arg);
>
> + arg = extract_programmer_param("ich_spi_force");
> + if (arg && !strcmp(arg, "yes")) {
> + ich_spi_force = 1;
> + msg_pspew("ich_spi_force enabled.\n");
> + } else if (arg && !strlen(arg)) {
> + msg_perr("Missing argument for ich_spi_force.\n");
> + free(arg);
> + return ERROR_FATAL;
> + } else if (arg) {
> + msg_perr("Unknown argument for ich_spi_force: \"%s\" "
> + "(not \"yes\").\n", arg);
> + free(arg);
> + return ERROR_FATAL;
> + }
> + free(arg);
> +
> tmp2 = mmio_readw(ich_spibar + ICH9_REG_HSFS);
> msg_pdbg("0x04: 0x%04x (HSFS)\n", tmp2);
> prettyprint_ich9_reg_hsfs(tmp2);
> @@ -1665,17 +1717,17 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
> msg_pdbg("BRWA 0x%02x, ", ICH_BRWA(tmp));
> msg_pdbg("BRRA 0x%02x\n", ICH_BRRA(tmp));
>
> - /* Decode and print FREGx and FRAP registers */
> + /* Handle FREGx and FRAP registers */
> for (i = 0; i < 5; i++)
> - do_ich9_spi_frap(tmp, i);
> + ich9_handle_frap(tmp, i, ich_spi_force);
> }
>
> - /* try to disable PR locks before printing them */
> - if (!ichspi_lock)
> - for (i = 0; i < 5; i++)
> + for (i = 0; i < 5; i++) {
> + /* if not locked down try to disable PR locks first */
> + if (!ichspi_lock)
> ich9_set_pr(i, 0, 0);
> - for (i = 0; i < 5; i++)
> - prettyprint_ich9_reg_pr(i);
> + ich9_handle_pr(i, ich_spi_force);
> + }
>
> tmp = mmio_readl(ich_spibar + ICH9_REG_SSFS);
> msg_pdbg("0x90: 0x%02x (SSFS)\n", tmp & 0xff);
Looks good otherwise.
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
Hi Stefan,
I have waited very long to comment on this patch, but I simply don't
have a good answer for it.
I do think that OTP/security memory is outside the scope of flashrom
because it behaves totally different (and testing could easily kill a
dozen chips during inital implementation, and a few chips each time
someone wants to retest). Now if something is outside the scope of
flashrom, should flashrom care at all? Some flash chips have GPI pins
(GPIO without output) and those are readable with commands very similar
to those used by flashrom. Should we tell the user about that feature as
well? We had such requests in the past, and I was not really happy about
them.
Am 14.08.2011 05:36 schrieb Stefan Tauner:
> Some flash chips contain OTP memory that we cannot read or write (yet). This
> prohibits us from cloning them, hence warn the user if we detect it. Not all
> variations of the tagged chips contain OTP memory. They are often only
> enabled on request or have there own ordering numbers. There is usually no
> way to distinguish them afaict.
Well, it's worse than that. I've seen datasheets for flash chips with
the following features:
- OTP memory, fully readable, fully writable once in one block (no
partial writes)
- OTP memory, fully readable, fully writable once per byte (partial
writes work just fine)
- OTP memory, fully readable, fully writable once per 2^n-byte block
- OTP memory, fully readable, only half of it writable, the other half
is programmed at the factory
- OTP memory, fully readable, completely pre-written at the factory (so
it's rather ROM than OTP)
- Hidden OTP memory which can only be read if you know the correct
security ID which is part of the OTP
- (The complete flash chip behaves like OTP (well, like ROM) once a
specific write-once status register is set)... not relevant here
And then you have the problem that multiple chip generations often share
the same device ID, so probing can't differentiate between a chip with
OTP and one without unless you're extremely lucky. Do we want
FEATURE_OTP and FEATURE_MAYBE_OTP?
Do we warn if a chip has a readonly serial number? That means the chip
can't be cloned. People who care about OTP for clonability reasons
probably care about other readonly contents as well. OTOH, other people
who don't use the OTP at all (for them, OTP is just an accidental
feature of a cheap flash chip) don't want to be bothered by yet another
line of output from flashrom which has no relevancy for them.
> The manpage is extended to describe the backgrounds a bit. While i touched
> that section, i also reformatted it a bit and removed the reference to the trac
> bug reporting facility.
>
> This patch is based on the idea and code of Daniel Lenski.
> ---
> the reason i removed the trac reference is that it is not used, i have no (working)
> login and it does not really fits our work flow anyway. the only user that used it
> in my active flashrom time ranted about it. would anyone miss it?
To be honest, trac is something I never got along with. Even if I'm
logged in there is no way for me to read the email address of the person
who opened the bug, so communication is pretty difficult. I'm all for
checking all old trac tickets, closing what can be closed, and moving
all other tickets in some way or another to the mailing list. However,
we should mention that it's OK to report bugs via IRC as well as long as
the reporter mentions a way to contact hi/her (i.e. email address).
Removing the trac reference and adding the IRC reference should be a
separate patch, though, which is
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
One comment about the man page formatting: This part of the patch may
conflict with the other man page formatting patch you sent. By the way,
do you know any good reference about man page formatting? I had trouble
finding out what .RE and .RS do.
> the new default verbosity output is:
> This chip may contain one-time programmable memory (see man page for details).
>
> the new dbg1/-V output is:
> This chip may contain one-time programmable memory.
> flashrom cannot read, and may never be able to write it. flashrom may not be
> able to completely clone the contents of this chip (see man page for details).
>
> there are certainly more chips with OTP memories out there. additions are
> welcome!
> i have only tagged chips with user modifiably OTP/security memories. there
> are some chips out there which have factory written IDs, but no OTP memory.
> a similar warning could/should be printed for those, but i did not want to
> abuse the OTP tag for this. also, there is nothing we can do, but read those IDs.
> duplicating chips using them is not possible, so a warning in the man page might
> suffice?
>
> since OTP memory (or any other data outside the linear address range of the
> "main" memory) can not be handled very well in flashrom right now, it is not
> clear how OTP memory access could be developed further. of course one could
> start to implement the basic methods needed to read them, but how they
> should be integrated then is not clear.
> one could start by adding an otp_print field to the struct flashchip and just
> dump the bytes on probing similar to the lock printing (not on default verbosity
> and possibly only if there are bytes different from 0x00 and 0xff).
>
> carl-daniel seems to be quite sceptical regarding handling OTP at all in flashrom.
> he stated "it does not really fit our device model at all", which is right, but could
> be changed.
> he also asked "should we really handle all features present in flash chips?"
> and the answer is probably "no", although i don't see a reason why we should
> not, if the effort is not too much and the architectural changes needed aren't
> obvious NOGOs.
> but OTP regions are somewhat special, they are not just *some* feature.
Indeed.
> one argument is the one stated in the message printed by this patch. flashrom
> is not able to clone chips that have some kind of OTP memory written right now.
> imo "cloning" a flash chip seems to be a valid use case for a tool like flashrom as
> long as it is feasible (which is not for chips with unique, preconfigured IDs).
>
> the other more theoretical argument i have is: OTP memory is just some
> memory in the flash chip. it may need other access patterns, but it is not much
> different from other write protected memories apart from that.
> some chips implement it in a way that it is even possible to erase the OTP
> regions. those regions are just normal flash and are made unwriteable by fuses
> in a register or another addressable byte.
>
> Signed-off-by: Daniel Lenski <dlenski(a)gmail.com>
> Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
Stefan: I don't want to veto this patch, and although I think that OTP
handling is not really a flashrom feature, I think that this
implementation satisfies the quality criteria for merging, so the patch is
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Regards,
Carl-Daniel
--
http://www.hailfinger.org/