[flashrom] [PATCH] ICH9/10: display FRAP/FREGx access controls

[Carl-Daniel Hailfinger]

Hi Joshua,

On 28.06.2010 22:55, Joshua Roys wrote:
> Attached is a patch to display the FRAP access controls on the FREGx
> regions of the flash chip.  Also attached is the output from `flashrom
> -Vr foo.bin' on an ICH10 machine.
>
> Signed-off-by: Joshua Roys <roysjosh at gmail.com>
>   

Thanks for your patch.
Overall, it looks nice, but i'd like to see a few minor changes before I
commit.


> diff --git a/chipset_enable.c b/chipset_enable.c
> index d124291..ba802af 100644
> --- a/chipset_enable.c
> +++ b/chipset_enable.c
> @@ -430,6 +430,40 @@ static int enable_flash_vt8237s_spi(struct pci_dev *dev, const char *name)
>  	return 0;
>  }
>  
> +#define ICH_BMWAG(x) ((x >> 24) & 0xff)
> +#define ICH_BMRAG(x) ((x >> 16) & 0xff)
> +#define ICH_BRWA(x)  ((x >>  8) & 0xff)
> +#define ICH_BRRA(x)  ((x >>  0) & 0xff)
> +
> +#define ICH_FREG_BASE(x)  ((x >> 0) & 0x1fff)
> +#define ICH_FREG_LIMIT(x) ((x >> 16) & 0x1fff)
> +
> +static void ich_spi_frap(uint32_t frap, int i)
>   

Can you change the name to print_ich9_spi_frap?


> +{
> +	char *access_names[4] = { "Locked", "Read-only", "Write-only", "Read-Write" };
> +	char *region_names[5] = {
> +		"Flash Descriptor", "BIOS Descriptor", "ME", "GbE", "Platform Data" };
>   

Those two string arrays should be const, and it would be appreciated if
you could fit them into 80 column width. I'd prefer to have access_names
in lowercase.


> +	int rwperms =
> +		((ICH_BRWA(frap) & (1 << i)) << 1) |
> +		((ICH_BRRA(frap) & (1 << i)) << 0);
>   

Odd indentation. The first two lines should be mergeable, and then
ICH_BRRA should be aligned with ICH_BRWA.


> +	int offset = 0x54 + i * 4;
> +	uint32_t freg = mmio_readl(spibar + offset), base, limit;
>   

Side note: spibar is a horrible global name. It should be ich_spibar,
but that's not your fault, and needs to be fixed in a separate patch.


> +
> +	msg_pdbg("0x%02X: 0x%08x (FREG%i: %s)\n",
> +		     offset, freg, i, region_names[i]);
> +
> +	base  = ICH_FREG_BASE(freg);
> +	limit = ICH_FREG_LIMIT(freg);
> +	if (base == 0x1fff && limit == 0) {
> +		/* this FREG is disabled */
> +		return;
>   

This is a bit irritating in the output. Can you print a message that the
FREG is disabled? OTOH, "disabled" implies that the region is
inaccessible or does not exist. Looking at your log, what does it mean
that the BIOS FREG is disabled? No access? Full access? Should the
message be "unknown location, full access", or is that incorrect?


> +	}
> +
> +	msg_pdbg("0x%08x-0x%08x is %s\n",
> +		    (base << 12), (limit << 12) | 0x0fff,
> +		    access_names[rwperms]);
> +}
> +
>  static int enable_flash_ich_dc_spi(struct pci_dev *dev, const char *name,
>  				   int ich_generation)
>  {
> @@ -554,21 +588,15 @@ static int enable_flash_ich_dc_spi(struct pci_dev *dev, const char *name,
>  
>  		tmp = mmio_readl(spibar + 0x50);
>  		msg_pdbg("0x50: 0x%08x (FRAP)\n", tmp);
> -		msg_pdbg("BMWAG %i, ", (tmp >> 24) & 0xff);
> -		msg_pdbg("BMRAG %i, ", (tmp >> 16) & 0xff);
> -		msg_pdbg("BRWA %i, ", (tmp >> 8) & 0xff);
> -		msg_pdbg("BRRA %i\n", (tmp >> 0) & 0xff);
> -
> -		msg_pdbg("0x54: 0x%08x (FREG0)\n",
> -			     mmio_readl(spibar + 0x54));
> -		msg_pdbg("0x58: 0x%08x (FREG1)\n",
> -			     mmio_readl(spibar + 0x58));
> -		msg_pdbg("0x5C: 0x%08x (FREG2)\n",
> -			     mmio_readl(spibar + 0x5C));
> -		msg_pdbg("0x60: 0x%08x (FREG3)\n",
> -			     mmio_readl(spibar + 0x60));
> -		msg_pdbg("0x64: 0x%08x (FREG4)\n",
> -			     mmio_readl(spibar + 0x64));
> +		msg_pdbg("BMWAG 0x%02x, ", ICH_BMWAG(tmp));
> +		msg_pdbg("BMRAG 0x%02x, ", ICH_BMRAG(tmp));
> +		msg_pdbg("BRWA 0x%02x, ", ICH_BRWA(tmp));
> +		msg_pdbg("BRRA 0x%02x\n", ICH_BRRA(tmp));
> +
> +		/* print out the FREGx registers along with FRAP access bits */
> +		for(i = 0; i < 5; i++)
> +			ich_spi_frap(tmp, i);
> +
>  		msg_pdbg("0x74: 0x%08x (PR0)\n",
>  			     mmio_readl(spibar + 0x74));
>  		msg_pdbg("0x78: 0x%08x (PR1)\n",
>   

This patch is definitely a good way to improve the readability of logs.
Now we just have to add some generic infrastructure which refuses to
read the chip if unreadable regions exist, and refuses to erase/write if
unwritable regions exist. This would either be some global flag
can_read/can_write set by the programmer, or some detailed region list.
The global flags would also be useful for programmers where we only
support reading.

Side note: Most BIOS implementers are using those ICH region
restrictions in an insecure way, and that may allow us to execute a
two-stage hack where the first stage tricks the chipset into allowing
write access to the descriptor region, and the second stage uses this
access to remove all restrictions from all regions.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/