[Skiboot] [PATCH v2] ffspart: Support flashing already ECC protected images

[Stewart Smith]

Samuel Mendoza-Jonas <sam at mendozajonas.com> writes:
> On Mon, 2018-12-10 at 17:48 +1100, Stewart Smith wrote:
>> We do this by assuming filenames with '.ecc' in them are already ECC
>> protected.
>> 
>> This solves a practical problem in transitioning op-build to use ffspart
>> for pnor assembly rather than three perl scripts and a lot of XML.
>> 
>> We also update the ffspart tests to take into account ECC requirements.
>> 
>> Signed-off-by: Stewart Smith <stewart at linux.ibm.com>
>> ---
>> Changes since v1:
>>   - make the test suite pass
>> ---
>>  external/ffspart/ffspart.c                    |  16 ++++++++++++++--
>>  external/ffspart/test/files/04-tiny-pnor2.out | Bin 2560 -> 2560 bytes
>>  .../ffspart/test/results/07-big-files.err     |   2 +-
>>  external/ffspart/test/tests/04-tiny-pnor2     |   2 +-
>>  external/ffspart/test/tests/08-small-files    |   2 +-
>>  5 files changed, 17 insertions(+), 5 deletions(-)
>> 
>> diff --git a/external/ffspart/ffspart.c b/external/ffspart/ffspart.c
>> index eeee0d4d657b..bb46a9eaf311 100644
>> --- a/external/ffspart/ffspart.c
>> +++ b/external/ffspart/ffspart.c
>> @@ -31,6 +31,7 @@
>>  #include <libflash/libflash.h>
>>  #include <libflash/libffs.h>
>>  #include <libflash/blocklevel.h>
>> +#include <libflash/ecc.h>
>>  #include <common/arch_flash.h>
>>  
>>  /*
>> @@ -250,6 +251,14 @@ static int parse_entry(struct blocklevel_device *bl,
>>  
>>  	if (*line != '\0' && *(line + 1) != '\0') {
>>  		filename = line + 1;
>> +
>> +		/*
>> +		 * Support flashing already ecc'd data as this is the case
>> +		 * for POWER8 SBE image binary.
>> +		 */
>> +		if (has_ecc(new_entry) && !strstr(filename, ".ecc"))
>> +			blocklevel_ecc_protect(bl, pbase, psize);
>
> If the entry has the ECC bit shouldn't we mark it protected regardless of
> the contents of the file?

The blocklevel_write() call will add ECC based on what's been
blocklevel_ecc_protect()ed, and we don't have a
blocklevel_write_but_i_did_ecc_already() call to bypass it... so this is
a bit of a hack I guess. I do want to remove it eventually though.

>> +
>>  		data_fd = open(filename, O_RDONLY);
>>  		if (data_fd == -1) {
>>  			fprintf(stderr, "Couldn't open file '%s' for '%s' partition "
>> @@ -269,9 +278,12 @@ static int parse_entry(struct blocklevel_device *bl,
>>  		 * Sanity check that the file isn't too large for
>>  		 * partition
>>  		 */
>> +		if (has_ecc(new_entry) && !strstr(filename, ".ecc"))
>> +			psize = ecc_buffer_size_minus_ecc(psize);
>>  		if (pactual > psize) {
>
> I needed to stare at this for a while but I think I'm happy with it -
> checking if the size of the non-ECC file is less than the size of the
> ECC-partition less the ECC bits?

yes.

-- 
Stewart Smith
OPAL Architect, IBM.