| Message ID | 20170829062506.8317-1-cyril.bur@au1.ibm.com |
|---|---|
| Headers | show |
| Series | Rework flash TOC generation | expand |
Cyril Bur <cyril.bur@au1.ibm.com> writes: > Currently the ability within libffs to prove an api to generate TOCs > is a bit experimental/developmental and it needs work. The original > version tried to abstract too much which mean needless complexity > within the library. > > I've ripped quite a lot of the complexity out of the libffs in favour > of some added (but overall less) complexity in ffspart. > > Unfortunately this does change libffs just a tad, hopefully none of > the functions have been used by anyone outside of skiboot, and the > input file to ffspart had to change a bit too, but I haven't heard of > anyone actually using it. > > Feedback very much welcome, how close is this to being able to produce a binary identical FFS image to the existing tooling? We have a different CSV format for FFS in phosphor-mboxd too, perhaps Adriana can chime in if it's possible/plausible to move to just one representation, as this could help simplify our PNOR building process. I'd *really* like to rip out openpower-ffs and other related crap from the op-build process and just replace it with this, largely because this has a remote chance of being maintained and a remote chance of not bringing in 13,000 lines of useless junk with it. In theory at least, ffspart ond the openbmc image building utility could just take the same input and output a different image.
On Mon, 2017-12-04 at 13:53 +1100, Stewart Smith wrote: > Cyril Bur <cyril.bur@au1.ibm.com> writes: > > Currently the ability within libffs to prove an api to generate TOCs > > is a bit experimental/developmental and it needs work. The original > > version tried to abstract too much which mean needless complexity > > within the library. > > > > I've ripped quite a lot of the complexity out of the libffs in favour > > of some added (but overall less) complexity in ffspart. > > > > Unfortunately this does change libffs just a tad, hopefully none of > > the functions have been used by anyone outside of skiboot, and the > > input file to ffspart had to change a bit too, but I haven't heard of > > anyone actually using it. > > > > Feedback very much welcome, > > how close is this to being able to produce a binary identical FFS image > to the existing tooling? > I'm pretty sure that at time of posting, it was identical to what I could wget from openpower.xyz as build pnors. In previous iterations of this patch it was also true, and things magically changed... Definitely worth a recheck - perhaps I could add it as part of a test. Bit of a wget and diff situation, that might be fragile because the XML would need to be converted > We have a different CSV format for FFS in phosphor-mboxd too, perhaps > Adriana can chime in if it's possible/plausible to move to just one > representation, as this could help simplify our PNOR building process. > Hmm I remember thinking that the phosphor-mbox CSV was pretty close. Of course its possible that the one here grew a bit in complexity. > I'd *really* like to rip out openpower-ffs and other related crap from > the op-build process and just replace it with this, largely because this > has a remote chance of being maintained and a remote chance of not > bringing in 13,000 lines of useless junk with it. > Strong ack! > In theory at least, ffspart ond the openbmc image building utility could > just take the same input and output a different image. >
Cyril Bur <cyril.bur@au1.ibm.com> writes: >> how close is this to being able to produce a binary identical FFS image >> to the existing tooling? >> > > I'm pretty sure that at time of posting, it was identical to what I > could wget from openpower.xyz as build pnors. In previous iterations of > this patch it was also true, and things magically changed... > > Definitely worth a recheck - perhaps I could add it as part of a test. > Bit of a wget and diff situation, that might be fragile because the XML > would need to be converted Sounds fragile :) If we just did it once and added a test with that layout and checking the bits matched, I'd be happy. >> We have a different CSV format for FFS in phosphor-mboxd too, perhaps >> Adriana can chime in if it's possible/plausible to move to just one >> representation, as this could help simplify our PNOR building process. >> > > Hmm I remember thinking that the phosphor-mbox CSV was pretty close. Of > course its possible that the one here grew a bit in complexity. Yeah, the multiple flash sides and all that probably made things horrific :) >> I'd *really* like to rip out openpower-ffs and other related crap from >> the op-build process and just replace it with this, largely because this >> has a remote chance of being maintained and a remote chance of not >> bringing in 13,000 lines of useless junk with it. >> > > Strong ack! > >> In theory at least, ffspart ond the openbmc image building utility could >> just take the same input and output a different image. >> >
Stewart Smith <stewart@linux.vnet.ibm.com> wrote on 12/03/2017 11:34:25 PM: > >> We have a different CSV format for FFS in phosphor-mboxd too, perhaps > >> Adriana can chime in if it's possible/plausible to move to just one > >> representation, as this could help simplify our PNOR building process. > >> > > > > Hmm I remember thinking that the phosphor-mbox CSV was pretty close. Of > > course its possible that the one here grew a bit in complexity. > > Yeah, the multiple flash sides and all that probably made things > horrific :) > The script that creates the toc in phosphor-mboxd is using output from pflash to recreate the part partition. Probably not the best plan... Maybe this is a good time to talk about changing what has now become an "ABI" :) > >> I'd *really* like to rip out openpower-ffs and other related crap from > >> the op-build process and just replace it with this, largely because this > >> has a remote chance of being maintained and a remote chance of not > >> bringing in 13,000 lines of useless junk with it. > >> > > > > Strong ack! > > > >> In theory at least, ffspart ond the openbmc image building utility could > >> just take the same input and output a different image. > >> > > > > -- > Stewart Smith > OPAL Architect, IBM. <br><tt><font size=2>Stewart Smith <stewart@linux.vnet.ibm.com> wrote on 12/03/2017 11:34:25 PM:<br><br>> >> We have a different CSV format for FFS in phosphor-mboxd too, perhaps<br>> >> Adriana can chime in if it's possible/plausible to move to just one<br>> >> representation, as this could help simplify our PNOR building process.<br>> >> <br>> ><br>> > Hmm I remember thinking that the phosphor-mbox CSV was pretty close. Of<br>> > course its possible that the one here grew a bit in complexity.<br>> <br>> Yeah, the multiple flash sides and all that probably made things<br>> horrific :)<br>> </font></tt><br><tt><font size=2>The script that creates the toc in phosphor-mboxd is using output from pflash</font></tt><br><tt><font size=2>to recreate the part partition. Probably not the best plan...</font></tt><br><tt><font size=2>Maybe this is a good time to talk about changing what has now become an "ABI" :)</font></tt><br><tt><font size=2><br>> >> I'd *really* like to rip out openpower-ffs and other related crap from<br>> >> the op-build process and just replace it with this, largely because this<br>> >> has a remote chance of being maintained and a remote chance of not<br>> >> bringing in 13,000 lines of useless junk with it.<br>> >> <br>> ><br>> > Strong ack!<br>> ><br>> >> In theory at least, ffspart ond the openbmc image building utility could<br>> >> just take the same input and output a different image.<br>> >> <br>> ><br>> <br>> -- <br>> Stewart Smith<br>> OPAL Architect, IBM.<br></font></tt><BR>
On Mon, 2017-12-04 at 16:34 +1100, Stewart Smith wrote: > Cyril Bur <cyril.bur@au1.ibm.com> writes: > > > how close is this to being able to produce a binary identical FFS image > > > to the existing tooling? > > > > > > > I'm pretty sure that at time of posting, it was identical to what I > > could wget from openpower.xyz as build pnors. In previous iterations of > > this patch it was also true, and things magically changed... > > > > Definitely worth a recheck - perhaps I could add it as part of a test. > > Bit of a wget and diff situation, that might be fragile because the XML > > would need to be converted > > > Sounds fragile :) > > If we just did it once and added a test with that layout and checking > the bits matched, I'd be happy. > Ah but need all the intermediary build files - arg that test is becoming a bit of a nightmare to write. Maybe get op-build to use both methods for a while and always compare the results, would be much less of a headache. > > > We have a different CSV format for FFS in phosphor-mboxd too, perhaps > > > Adriana can chime in if it's possible/plausible to move to just one > > > representation, as this could help simplify our PNOR building process. > > > > > > > Hmm I remember thinking that the phosphor-mbox CSV was pretty close. Of > > course its possible that the one here grew a bit in complexity. > > Yeah, the multiple flash sides and all that probably made things > horrific :) > > > > I'd *really* like to rip out openpower-ffs and other related crap from > > > the op-build process and just replace it with this, largely because this > > > has a remote chance of being maintained and a remote chance of not > > > bringing in 13,000 lines of useless junk with it. > > > > > > > Strong ack! > > > > > In theory at least, ffspart ond the openbmc image building utility could > > > just take the same input and output a different image. > > > > >
Cyril Bur <cyril.bur@au1.ibm.com> writes: > On Mon, 2017-12-04 at 16:34 +1100, Stewart Smith wrote: >> Cyril Bur <cyril.bur@au1.ibm.com> writes: >> > > how close is this to being able to produce a binary identical FFS image >> > > to the existing tooling? >> > > >> > >> > I'm pretty sure that at time of posting, it was identical to what I >> > could wget from openpower.xyz as build pnors. In previous iterations of >> > this patch it was also true, and things magically changed... >> > >> > Definitely worth a recheck - perhaps I could add it as part of a test. >> > Bit of a wget and diff situation, that might be fragile because the XML >> > would need to be converted >> >> >> Sounds fragile :) >> >> If we just did it once and added a test with that layout and checking >> the bits matched, I'd be happy. >> > > Ah but need all the intermediary build files - arg that test is > becoming a bit of a nightmare to write. > > Maybe get op-build to use both methods for a while and always compare > the results, would be much less of a headache. Yeah, I wouldn't be opposed to that as an interim solution. Of course, that still means we have to work out WTF it's doing and replicate it :)
Adriana Kobylak <anoo@us.ibm.com> writes: > Stewart Smith <stewart@linux.vnet.ibm.com> wrote on 12/03/2017 11:34:25 > PM: > >> >> We have a different CSV format for FFS in phosphor-mboxd too, perhaps >> >> Adriana can chime in if it's possible/plausible to move to just one >> >> representation, as this could help simplify our PNOR building > process. >> >> >> > >> > Hmm I remember thinking that the phosphor-mbox CSV was pretty close. > Of >> > course its possible that the one here grew a bit in complexity. >> >> Yeah, the multiple flash sides and all that probably made things >> horrific :) >> > The script that creates the toc in phosphor-mboxd is using output from > pflash > to recreate the part partition. Probably not the best plan... > Maybe this is a good time to talk about changing what has now become an > "ABI" :) Yeah, I think part of it is going to be starting to pull apart what op-build does TBH. There's just way too many random large perl scripts and random binaries there. My guess is doing that is probably about a week's worth of work :/
On Tue, 2017-12-05 at 16:54 +1100, Stewart Smith wrote: > Adriana Kobylak <anoo@us.ibm.com> writes: > > Stewart Smith <stewart@linux.vnet.ibm.com> wrote on 12/03/2017 11:34:25 > > PM: > > > > > > > We have a different CSV format for FFS in phosphor-mboxd too, perhaps > > > > > Adriana can chime in if it's possible/plausible to move to just one > > > > > representation, as this could help simplify our PNOR building > > > > process. > > > > > > > > > > > > > Hmm I remember thinking that the phosphor-mbox CSV was pretty close. > > > > Of > > > > course its possible that the one here grew a bit in complexity. > > > > > > Yeah, the multiple flash sides and all that probably made things > > > horrific :) > > > > > > > The script that creates the toc in phosphor-mboxd is using output from > > pflash > > to recreate the part partition. Probably not the best plan... > > Maybe this is a good time to talk about changing what has now become an > > "ABI" :) > > Yeah, I think part of it is going to be starting to pull apart what > op-build does TBH. There's just way too many random large perl scripts > and random binaries there. > > My guess is doing that is probably about a week's worth of work :/ I found my random script to convert the xml to csv, it has bitrotted a bit so I'll fix it and probably just add it to the series. I'll keep plodding along. Cyril >
Hi, Currently the ability within libffs to prove an api to generate TOCs is a bit experimental/developmental and it needs work. The original version tried to abstract too much which mean needless complexity within the library. I've ripped quite a lot of the complexity out of the libffs in favour of some added (but overall less) complexity in ffspart. Unfortunately this does change libffs just a tad, hopefully none of the functions have been used by anyone outside of skiboot, and the input file to ffspart had to change a bit too, but I haven't heard of anyone actually using it. Feedback very much welcome, Thanks Cyril Bur (13): external/ffspart: Allow # comments in input file libflash/libffs: Add helpers for the treatment of partition flags external/pflash: Use ffs_entry_user_to_string() to standardise flag strings libflash/libffs: Add setter for a partitions actual size libflash/libffs: ffs_close() should use ffs_hdr_free() external/ffspart: Remove side, order and backup options libflash/libffs: Always add entries to the end of the TOC libflash/libffs: Remove the 'sides' from the FFS TOC generation code libflash/libffs: Remove backup partition from TOC generation code libflash/libffs: Switch to storing header entries in an array libflash/libffs: Refcount ffs entries libflash/libffs: Allow caller to specifiy header partition external/ffspart: Use new interface external/ffspart/ffspart.c | 604 +++++++++++---------- external/ffspart/test/files/03-tiny-pnor.in | 8 +- .../ffspart/test/files/03.1-tiny-pnor-backup.in | 4 - .../ffspart/test/files/03.1-tiny-pnor-backup.out | Bin 3840 -> 0 bytes external/ffspart/test/files/04-tiny-pnor2.in | 8 +- external/ffspart/test/files/05-hdr-overlap.in | 8 +- .../ffspart/test/files/05.1-hdr-overlap-backup.in | 3 - external/ffspart/test/files/06-small-flash.in | 10 +- external/ffspart/test/files/07-big-files.in | 8 +- external/ffspart/test/files/08-small-files.in | 8 +- external/ffspart/test/files/10-bad-input.in | 8 +- external/ffspart/test/files/11-long-name.in | 8 +- external/ffspart/test/files/12-bad-numbers-base.in | 8 +- external/ffspart/test/files/13-bad-numbers-size.in | 8 +- external/ffspart/test/files/14-bad-input-flags.in | 8 +- .../test/files/15-overlapping-partitions.in | 8 +- external/ffspart/test/results/00-usage.out | 5 - external/ffspart/test/results/01-param-sanity.err | 1 - external/ffspart/test/results/01-param-sanity.out | 5 - .../ffspart/test/results/01.1-param-sanity.err | 0 .../{02-param-sides.out => 01.1-param-sanity.out} | 5 - external/ffspart/test/results/02-param-sides.err | 1 - external/ffspart/test/results/05-hdr-overlap.err | 5 +- external/ffspart/test/results/05-hdr-overlap.out | 4 - .../test/results/05.1-hdr-overlap-backup.err | 2 - .../test/results/05.1-hdr-overlap-backup.out | 4 - external/ffspart/test/results/06-small-flash.err | 5 +- external/ffspart/test/results/06-small-flash.out | 3 - external/ffspart/test/results/07-big-files.err | 5 +- external/ffspart/test/results/07-big-files.out | 2 - external/ffspart/test/results/08-small-files.err | 2 + external/ffspart/test/results/08-small-files.out | 5 - external/ffspart/test/results/10-bad-input.err | 5 +- external/ffspart/test/results/10-bad-input.out | 1 - external/ffspart/test/results/11-long-name.err | 6 +- external/ffspart/test/results/11-long-name.out | 5 - .../ffspart/test/results/12-bad-numbers-base.err | 5 +- .../ffspart/test/results/12-bad-numbers-base.out | 1 - .../ffspart/test/results/13-bad-numbers-size.err | 5 +- .../ffspart/test/results/13-bad-numbers-size.out | 1 - .../ffspart/test/results/14-bad-input-flags.err | 5 +- .../ffspart/test/results/14-bad-input-flags.out | 1 - .../test/results/15-overlapping-partitions.err | 5 +- .../test/results/15-overlapping-partitions.out | 3 - external/ffspart/test/tests/01-param-sanity | 2 +- .../tests/{02-param-sides => 01.1-param-sanity} | 2 +- external/ffspart/test/tests/03.1-tiny-pnor-backup | 15 - external/ffspart/test/tests/05-hdr-overlap | 7 +- .../ffspart/test/tests/05.1-hdr-overlap-backup | 15 - external/ffspart/test/tests/06-small-flash | 5 +- external/ffspart/test/tests/07-big-files | 4 + external/ffspart/test/tests/10-bad-input | 2 + external/ffspart/test/tests/12-bad-numbers-base | 2 + external/ffspart/test/tests/13-bad-numbers-size | 2 + external/ffspart/test/tests/14-bad-input-flags | 2 + .../ffspart/test/tests/15-overlapping-partitions | 4 +- external/pflash/pflash.c | 16 +- external/pflash/test/files/01-info.ffs | 8 +- external/pflash/test/files/02-erase.ffs | 8 +- external/pflash/test/files/03-erase-parts.ffs | 8 +- external/pflash/test/files/04-program-rand.ffs | 8 +- external/pflash/test/files/05-bad-numbers.ffs | 8 +- external/pflash/test/files/06-miscprint.ffs | 8 +- external/pflash/test/results/01-info.out | 10 +- external/pflash/test/results/06-miscprint.out | 2 +- libflash/ffs.h | 16 +- libflash/libffs.c | 455 ++++++++-------- libflash/libffs.h | 18 +- 68 files changed, 731 insertions(+), 702 deletions(-) delete mode 100644 external/ffspart/test/files/03.1-tiny-pnor-backup.in delete mode 100644 external/ffspart/test/files/03.1-tiny-pnor-backup.out delete mode 100644 external/ffspart/test/files/05.1-hdr-overlap-backup.in create mode 100644 external/ffspart/test/results/01.1-param-sanity.err rename external/ffspart/test/results/{02-param-sides.out => 01.1-param-sanity.out} (67%) delete mode 100644 external/ffspart/test/results/02-param-sides.err delete mode 100644 external/ffspart/test/results/05.1-hdr-overlap-backup.err delete mode 100644 external/ffspart/test/results/05.1-hdr-overlap-backup.out rename external/ffspart/test/tests/{02-param-sides => 01.1-param-sanity} (63%) delete mode 100644 external/ffspart/test/tests/03.1-tiny-pnor-backup delete mode 100644 external/ffspart/test/tests/05.1-hdr-overlap-backup