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

[Samuel Mendoza-Jonas]

On Tue, 2018-12-11 at 15:29 +1100, Stewart Smith wrote:
> 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.

Ah I see, we're kind of pretending it's not ECC at write time.. sounds
about right :)

Reviewed-by: Samuel Mendoza-Jonas <sam at mendozajonas.com>

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