| Message ID | 20181210064849.7784-1-stewart@linux.ibm.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] ffspart: Support flashing already ECC protected images | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | master/apply_patch Successfully applied |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | success | Test snowpatch/job/snowpatch-skiboot on branch master |
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@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? > + > 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? > - fprintf(stderr, "File '%s' for partition '%s' is too large\n", > - filename, name); > + fprintf(stderr, "File '%s' for partition '%s' is too large," > + " %u > %u\n", > + filename, name, pactual, psize); > close(data_fd); > return -1; > } > diff --git a/external/ffspart/test/files/04-tiny-pnor2.out b/external/ffspart/test/files/04-tiny-pnor2.out > index 394edf0411267503a24caf3bb25d4b4b0a0327e6..617c4ef444dee89012155a2540ed973988a3c5c5 100644 > GIT binary patch > literal 2560 > zcmWG=3<_ajU|<AdW*}|=Vpa&3feXk+0RfJ|HwzMrN>BtL9OnN(0A#|<0n#Ajs0;e% > z^>zFExuP2ivWO9CKNE-zQ2?b-R5yS<h$2J+$6%xNzmRZ$bR8@}0hs+@w?GtNx1T`( > zT|4po)jJG-gm?tGqJ;o3zTozQ-9jt--!0eK?dBgEgyv#4B>TZ`p_TnR7ig~o#WOX5 > rAr$-{fecVM8bQHGWPto{3<V4$kOA_)2^5S(2FU-TU^E0qatHtbT!f8< > > literal 2560 > zcmWG=3<_ajU|<AdW*}|=Vpa&3feXk+0RfJ|HwzMrN>BtL9OnN(0A#|<0n#Ajs0;e% > z^>zFExuP2ivWO9CKNE-zQ2?b-R5yS<h$2J+$6%xNzmRZ$bR8@}0hs+@w?GtNx1T`( > zT|4po)jJG-gm?tGqJ;o3zTozQ-9jt--!0eK?dBgEgyv#4B>TZ`p_TnR7ig~o#WOX5 > c;RpvtBODlya9}dR0X55#QL{!vV5o%v0EntXDF6Tf > > diff --git a/external/ffspart/test/results/07-big-files.err b/external/ffspart/test/results/07-big-files.err > index 083bad20ea84..bfb6fa0220da 100644 > --- a/external/ffspart/test/results/07-big-files.err > +++ b/external/ffspart/test/results/07-big-files.err > @@ -1,4 +1,4 @@ > WARNING: Attempting to parse a partition line without any TOCs created. > Generating a default TOC at zero > -File 'FILE_ONE' for partition 'ONE' is too large > +File 'FILE_ONE' for partition 'ONE' is too large, 257 > 227 > Failed to parse input file 'FILE' at line 1 > diff --git a/external/ffspart/test/tests/04-tiny-pnor2 b/external/ffspart/test/tests/04-tiny-pnor2 > index a7e79ab5e1f6..3db5f1e29c6e 100644 > --- a/external/ffspart/test/tests/04-tiny-pnor2 > +++ b/external/ffspart/test/tests/04-tiny-pnor2 > @@ -4,7 +4,7 @@ touch $DATA_DIR/$CUR_TEST.gen > i=1; > while [ $i -lt 5 ] ; do > j=0; > - while [ $j -lt $((0x100)) ] ; do > + while [ $j -lt $((0xe0)) ] ; do > echo -n "$i" >> $DATA_DIR/$CUR_TEST.$i; > j=$(expr $j + 1); > done > diff --git a/external/ffspart/test/tests/08-small-files b/external/ffspart/test/tests/08-small-files > index fb2a98b3dedb..1e4f3b3beba4 100644 > --- a/external/ffspart/test/tests/08-small-files > +++ b/external/ffspart/test/tests/08-small-files > @@ -4,7 +4,7 @@ touch $DATA_DIR/$CUR_TEST.gen > i=1; > while [ $i -lt 5 ] ; do > j=0; > - while [ $j -lt $((0xff)) ] ; do > + while [ $j -lt $((0xe0)) ] ; do > echo -n "$i" >> $DATA_DIR/$CUR_TEST.$i; > j=$(expr $j + 1); > done
Samuel Mendoza-Jonas <sam@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@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.
On Tue, 2018-12-11 at 15:29 +1100, Stewart Smith wrote: > Samuel Mendoza-Jonas <sam@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@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@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. >
Samuel Mendoza-Jonas <sam@mendozajonas.com> writes: > On Tue, 2018-12-11 at 15:29 +1100, Stewart Smith wrote: >> Samuel Mendoza-Jonas <sam@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@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 :) Yeah - and that's the *least* horrible part of this entire endeavour :)
Stewart Smith <stewart@linux.ibm.com> writes: > 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@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(-) Merged to master as of f12bdee4d0b1409533a3c0dd12d867ad4acae8b7
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); + 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) { - fprintf(stderr, "File '%s' for partition '%s' is too large\n", - filename, name); + fprintf(stderr, "File '%s' for partition '%s' is too large," + " %u > %u\n", + filename, name, pactual, psize); close(data_fd); return -1; } diff --git a/external/ffspart/test/files/04-tiny-pnor2.out b/external/ffspart/test/files/04-tiny-pnor2.out index 394edf0411267503a24caf3bb25d4b4b0a0327e6..617c4ef444dee89012155a2540ed973988a3c5c5 100644 GIT binary patch literal 2560 zcmWG=3<_ajU|<AdW*}|=Vpa&3feXk+0RfJ|HwzMrN>BtL9OnN(0A#|<0n#Ajs0;e% z^>zFExuP2ivWO9CKNE-zQ2?b-R5yS<h$2J+$6%xNzmRZ$bR8@}0hs+@w?GtNx1T`( zT|4po)jJG-gm?tGqJ;o3zTozQ-9jt--!0eK?dBgEgyv#4B>TZ`p_TnR7ig~o#WOX5 rAr$-{fecVM8bQHGWPto{3<V4$kOA_)2^5S(2FU-TU^E0qatHtbT!f8< literal 2560 zcmWG=3<_ajU|<AdW*}|=Vpa&3feXk+0RfJ|HwzMrN>BtL9OnN(0A#|<0n#Ajs0;e% z^>zFExuP2ivWO9CKNE-zQ2?b-R5yS<h$2J+$6%xNzmRZ$bR8@}0hs+@w?GtNx1T`( zT|4po)jJG-gm?tGqJ;o3zTozQ-9jt--!0eK?dBgEgyv#4B>TZ`p_TnR7ig~o#WOX5 c;RpvtBODlya9}dR0X55#QL{!vV5o%v0EntXDF6Tf diff --git a/external/ffspart/test/results/07-big-files.err b/external/ffspart/test/results/07-big-files.err index 083bad20ea84..bfb6fa0220da 100644 --- a/external/ffspart/test/results/07-big-files.err +++ b/external/ffspart/test/results/07-big-files.err @@ -1,4 +1,4 @@ WARNING: Attempting to parse a partition line without any TOCs created. Generating a default TOC at zero -File 'FILE_ONE' for partition 'ONE' is too large +File 'FILE_ONE' for partition 'ONE' is too large, 257 > 227 Failed to parse input file 'FILE' at line 1 diff --git a/external/ffspart/test/tests/04-tiny-pnor2 b/external/ffspart/test/tests/04-tiny-pnor2 index a7e79ab5e1f6..3db5f1e29c6e 100644 --- a/external/ffspart/test/tests/04-tiny-pnor2 +++ b/external/ffspart/test/tests/04-tiny-pnor2 @@ -4,7 +4,7 @@ touch $DATA_DIR/$CUR_TEST.gen i=1; while [ $i -lt 5 ] ; do j=0; - while [ $j -lt $((0x100)) ] ; do + while [ $j -lt $((0xe0)) ] ; do echo -n "$i" >> $DATA_DIR/$CUR_TEST.$i; j=$(expr $j + 1); done diff --git a/external/ffspart/test/tests/08-small-files b/external/ffspart/test/tests/08-small-files index fb2a98b3dedb..1e4f3b3beba4 100644 --- a/external/ffspart/test/tests/08-small-files +++ b/external/ffspart/test/tests/08-small-files @@ -4,7 +4,7 @@ touch $DATA_DIR/$CUR_TEST.gen i=1; while [ $i -lt 5 ] ; do j=0; - while [ $j -lt $((0xff)) ] ; do + while [ $j -lt $((0xe0)) ] ; do echo -n "$i" >> $DATA_DIR/$CUR_TEST.$i; j=$(expr $j + 1); done
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@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(-)