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

[Joshua Roys]

Hello,

I'm glad it seems useful!  Some questions inline:

On 06/29/2010 08:12 AM, Carl-Daniel Hailfinger wrote:
>> +static void ich_spi_frap(uint32_t frap, int i)
>>   
> 
> Can you change the name to print_ich9_spi_frap?
> 

I was considering making this function twiddle some bits eventually (if
it could).  The BM{W,R}AG fields can apparently override the FRAP access
controls to the "BIOS Descriptor" region.  Unfortunately on my board,
the regions were labeled as a flash descriptor and a ME section...
do_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.
> 

OK.

>> +	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.
> 

OK.

>> +
>> +	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?
> 

If I understand the datasheet correctly it simply means that there is no
region in the flash chip that is considered as this (ME, GbE, etc).
Maybe something like msg_pdbg("%s region is unused.", region_names[i]);

> 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
> 

Thanks for the review,

Josh