Re: [flashrom] [PATCH] ichspi.c: warn user and disable writes when a protected address range is detected.

[Carl-Daniel Hailfinger]

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 [e-mail] 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 [e-mail] 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
>
> 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 [e-mail] 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

Regards,
Carl-Daniel

[PATCH] ichspi.c: warn user and disable writes when a protected address range is detected.

[Stefan Tauner]

On Wed, 15 Feb 2012 03:14:19 +0100
Carl-Daniel Hailfinger  wrote:
> 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 idea behind naming ich_spi_force not too specifically was that we
may need another force switch in the future and i dont want to add yet
another parameter (or change the old one, because it is a (small) UI
change), but it is not that important to me...
> The PR stuff is not limited to SPI IIRC.

i dont think so. there is a table in the respective datasheets named
"Flash Protection Mechanism Summary" it names the "Equivalent Function
on FWH" for the "BIOS Range Write Protection": "FWH Sector Protection"

My interpretation is that the "BIOS Range Write Protection" is the one
related to PRx and that "FWH Sector Protection" refers to the chip
feature of lock bits. Also, the PRx registers are located in the SPIBAR
range and the documentation of the PRx addresses refer to FLA, which is
only used to define the SPI flash address (for hwseq and swseq).

maybe name it ich_force?
NB: the other parameter for hw/swseq selection is ich_spi_mode
> > 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 [e-mail] 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?

the good thing is.. it wont get much worse in verbose mode ;)
> 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 [e-mail] 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.

complicates the logic... this order would be my favorite too (of
course).
> 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).

ok... i am certainly a bit biased on and obsessed with this :)
> 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.

i will look into it.
> > Signed-off-by: Stefan Tauner
> >
> > 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 [e-mail] 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.

it does, but OTOH one has to direct the execution flow for the
impossible values anyway and since the else path would try to access
the array out of bounds, i have done it in that way.
note that ich9_handle_frap is different because there we access the
array before the if, so it does matter even less there. if you think
that reduces readability i can change it - for me it does not.
> > 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

ill try to make the output more readable, please notify me about the
other decisions.

[PATCH] ichspi.c: warn user and disable writes when a protected address range is detected.

[Carl-Daniel Hailfinger]

Am 15.02.2012 13:18 schrieb Stefan Tauner:
> On Wed, 15 Feb 2012 03:14:19 +0100
> Carl-Daniel Hailfinger  wrote:
> > 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 idea behind naming ich_spi_force not too specifically was that we
> may need another force switch in the future and i dont want to add yet
> another parameter (or change the old one, because it is a (small) UI
> change), but it is not that important to me...

Right. Regardless of which name we pick, it won't be optimal. You know
that code better than I do, so the naming choice is yours.
> > The PR stuff is not limited to SPI IIRC.
>
> i dont think so. there is a table in the respective datasheets named
> "Flash Protection Mechanism Summary" it names the "Equivalent Function
> on FWH" for the "BIOS Range Write Protection": "FWH Sector Protection"
>
> My interpretation is that the "BIOS Range Write Protection" is the one
> related to PRx and that "FWH Sector Protection" refers to the chip
> feature of lock bits. Also, the PRx registers are located in the SPIBAR
> range and the documentation of the PRx addresses refer to FLA, which is
> only used to define the SPI flash address (for hwseq and swseq).
>
> maybe name it ich_force?

Sounds a bit odd (what are we forcing?). Your original naming was better
than ich_force. To be honest, I think some of the ICH specific
programmer parameters will go away anyway once we have better documentation.
> NB: the other parameter for hw/swseq selection is ich_spi_mode
> > > 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 [e-mail] 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?
>
> the good thing is.. it wont get much worse in verbose mode ;)

Heh.
> > 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 [e-mail] 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.
>
> complicates the logic... this order would be my favorite too (of
> course).

Should be possible if the function doing the check (ich9_handle_frap and
ich9_handle_pr) returns int instead of void.
> > 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).
>
> ok... i am certainly a bit biased on and obsessed with this :)

And I think you're right.
> > 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.
>
> i will look into it.
> > > Signed-off-by: Stefan Tauner
> > >
> > > --- a/ichspi.c
> > > +++ b/ichspi.c
> > > @@ -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.
>
> it does, but OTOH one has to direct the execution flow for the
> impossible values anyway

Really? In this case, the compiler even has the chance to know which
possible values rwperms may have.
> and since the else path would try to access
> the array out of bounds, i have done it in that way.

Assuming there is no memory corruption, we can be totally sure that
regardless of the value of pr, rwperms can only be one of
0x0,0x1,0x2,0x3. This means out-of-bounds is impossible, and a good
compiler should realize that.
> note that ich9_handle_frap is different because there we access the
> array before the if, so it does matter even less there. if you think
> that reduces readability i can change it - for me it does not.

You know the datasheets very well, and you also know the code very well.
For me, the >= operation implied that values >0x3 were possible and
that's why I complained. My goal is to have code which can be read
mostly in isolation without chasing nonexisting corner cases. Then
again, this is my personal impression, not some scientifically
measurable hard fact.
> > > 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
> >
> > Looks good otherwise.
> > Acked-by: Carl-Daniel Hailfinger
>
> ill try to make the output more readable, please notify me about the
> other decisions.

Regards,
Carl-Daniel

[PATCH] ichspi.c: warn user and disable writes when a protected address range is detected.

[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 
Acked-by: Carl-Daniel Hailfinger 
---
 flashrom.8 |   15 ++++++++++
 ichspi.c   |   87 ++++++++++++++++++++++++++++++++++++++++++++++--------------
 2 files changed, 82 insertions(+), 20 deletions(-)

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
@@ -1421,7 +1421,8 @@ static int ich_spi_send_multicommand(struct flashctx *flash,
 #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"
@@ -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]);
+	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;
    int desc_valid = 0;
    struct ich_descriptors desc = {{ 0 }};
    enum ich_spi_mode {
@@ -1631,6 +1643,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 +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) {
+			msg_pinfo("Please send a verbose log to "
+				  "[e-mail] 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);