| Message ID | 1540320449-41061-1-git-send-email-anoo@linux.ibm.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | pflash: Add --skip option for reading | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | master/apply_patch Successfully applied |
| snowpatch_ozlabs/make_check | success | Test make_check on branch master |
On Tue, 2018-10-23 at 13:47 -0500, Adriana Kobylak wrote: > Add a --skip=N option to pflash to skip N number of bytes when reading. > This would allow users to print the VERSION partition without the STB > header by specifying the --skip=4096 argument, and it's a more generic > solution rather than making pflash depend on secure/trusted boot code. > > Signed-off-by: Adriana Kobylak <anoo@linux.ibm.com> Reviewed-by: Samuel Mendoza-Jonas <sam@mendozajonas.com> > --- > external/pflash/pflash.c | 26 +++++++++++++++++++++++--- > 1 file changed, 23 insertions(+), 3 deletions(-) > > diff --git a/external/pflash/pflash.c b/external/pflash/pflash.c > index 72b90fc8..c81520cb 100644 > --- a/external/pflash/pflash.c > +++ b/external/pflash/pflash.c > @@ -471,7 +471,7 @@ out: > } > > static int do_read_file(struct blocklevel_device *bl, const char *file, > - uint32_t start, uint32_t size) > + uint32_t start, uint32_t size, uint32_t skip_size) > { > int fd, rc = 0; > uint32_t done = 0; > @@ -481,6 +481,9 @@ static int do_read_file(struct blocklevel_device *bl, const char *file, > perror("Failed to open file"); > return 1; > } > + start += skip_size; > + size -= skip_size; > + > printf("Reading to \"%s\" from 0x%08x..0x%08x !\n", > file, start, start + size); > > @@ -644,6 +647,8 @@ static void print_help(const char *pname) > printf("\t\tTarget filename instead of actual flash.\n\n"); > printf("\t-S, --side\n"); > printf("\t\tSide of the flash on which to operate, 0 (default) or 1\n\n"); > + printf("\t--skip=N\n"); > + printf("\t\tSkip N number of bytes from the start when reading\n\n"); > printf("\t-T, --toc\n"); > printf("\t\tlibffs TOC on which to operate, defaults to 0.\n"); > printf("\t\tleading 0x is required for interpretation of a hex value\n\n"); > @@ -709,7 +714,7 @@ int main(int argc, char *argv[]) > bool no_action = false, tune = false; > char *write_file = NULL, *read_file = NULL, *part_name = NULL; > bool ffs_toc_seen = false, direct = false, print_detail = false; > - int flash_side = 0; > + int flash_side = 0, skip_size = 0; > int rc = 0; > > while(1) { > @@ -735,6 +740,7 @@ int main(int argc, char *argv[]) > {"version", no_argument, NULL, 'v'}, > {"debug", no_argument, NULL, 'g'}, > {"side", required_argument, NULL, 'S'}, > + {"skip", required_argument, NULL, 'k'}, > {"toc", required_argument, NULL, 'T'}, > {"clear", no_argument, NULL, 'c'}, > {NULL, 0, NULL, 0 } > @@ -826,6 +832,13 @@ int main(int argc, char *argv[]) > case 'S': > flash_side = atoi(optarg); > break; > + case 'k': > + skip_size = strtoul(optarg, &endptr, 0); > + if (*endptr != '\0') { > + rc = 1; > + no_action = true; > + } > + break; > case 'T': > if (!optarg) > break; > @@ -935,6 +948,13 @@ int main(int argc, char *argv[]) > goto out; > } > > + /* Skip only supported on read */ > + if (skip_size && !do_read) { > + fprintf(stderr, "--skip requires a --read command !\n"); > + rc = 1; > + goto out; > + } > + > /* Program command should always come with a file */ > if (program && !write_file) { > fprintf(stderr, "Program with no file specified !\n"); > @@ -1142,7 +1162,7 @@ int main(int argc, char *argv[]) > } > rc = 0; > if (do_read) > - rc = do_read_file(flash.bl, read_file, address, read_size); > + rc = do_read_file(flash.bl, read_file, address, read_size, skip_size); > if (!rc && erase_all) > rc = erase_chip(&flash); > else if (!rc && erase)
Adriana Kobylak <anoo@linux.ibm.com> writes: > Add a --skip=N option to pflash to skip N number of bytes when reading. > This would allow users to print the VERSION partition without the STB > header by specifying the --skip=4096 argument, and it's a more generic > solution rather than making pflash depend on secure/trusted boot code. > > Signed-off-by: Adriana Kobylak <anoo@linux.ibm.com> > --- > external/pflash/pflash.c | 26 +++++++++++++++++++++++--- > 1 file changed, 23 insertions(+), 3 deletions(-) Thanks, merged to master as of b6ebee077d915916989aa3057239a985981edb07. Does it make sense to also have a --skip-stb-header option? I don't like us relying on the stb header just being 4096 bytes, but IIRC everywhere else hardcodes it too... I'm mostly wondering about not building too much knowledge of host firmware into OpenBMC.
> > Thanks, merged to master as of > b6ebee077d915916989aa3057239a985981edb07. > Thanks > Does it make sense to also have a --skip-stb-header option? > > I don't like us relying on the stb header just being 4096 bytes, but > IIRC everywhere else hardcodes it too... I'm mostly wondering about not > building too much knowledge of host firmware into OpenBMC. I started that way, but seems we'd need additional changes when building pflash to find the libstb/container.h, and since this new option is intended to be used by users and not built into any code, this generic option is probably ok for now. If there are more features related to secure boot that we need to add to pflash or requests from people we can revisit then.
Adriana Kobylak <anoo@linux.ibm.com> writes: >> Thanks, merged to master as of >> b6ebee077d915916989aa3057239a985981edb07. >> > > Thanks > >> Does it make sense to also have a --skip-stb-header option? >> >> I don't like us relying on the stb header just being 4096 bytes, but >> IIRC everywhere else hardcodes it too... I'm mostly wondering about not >> building too much knowledge of host firmware into OpenBMC. > > I started that way, but seems we'd need additional changes when building > pflash to find the libstb/container.h, and since this new option is > intended > to be used by users and not built into any code, this generic option is > probably ok for now. > If there are more features related to secure boot that we need to add to > pflash or requests from people we can revisit then. Yeah, I figured as much - and skipping into a partition is a valid enough thing to do anyway (dd has it for example).
diff --git a/external/pflash/pflash.c b/external/pflash/pflash.c index 72b90fc8..c81520cb 100644 --- a/external/pflash/pflash.c +++ b/external/pflash/pflash.c @@ -471,7 +471,7 @@ out: } static int do_read_file(struct blocklevel_device *bl, const char *file, - uint32_t start, uint32_t size) + uint32_t start, uint32_t size, uint32_t skip_size) { int fd, rc = 0; uint32_t done = 0; @@ -481,6 +481,9 @@ static int do_read_file(struct blocklevel_device *bl, const char *file, perror("Failed to open file"); return 1; } + start += skip_size; + size -= skip_size; + printf("Reading to \"%s\" from 0x%08x..0x%08x !\n", file, start, start + size); @@ -644,6 +647,8 @@ static void print_help(const char *pname) printf("\t\tTarget filename instead of actual flash.\n\n"); printf("\t-S, --side\n"); printf("\t\tSide of the flash on which to operate, 0 (default) or 1\n\n"); + printf("\t--skip=N\n"); + printf("\t\tSkip N number of bytes from the start when reading\n\n"); printf("\t-T, --toc\n"); printf("\t\tlibffs TOC on which to operate, defaults to 0.\n"); printf("\t\tleading 0x is required for interpretation of a hex value\n\n"); @@ -709,7 +714,7 @@ int main(int argc, char *argv[]) bool no_action = false, tune = false; char *write_file = NULL, *read_file = NULL, *part_name = NULL; bool ffs_toc_seen = false, direct = false, print_detail = false; - int flash_side = 0; + int flash_side = 0, skip_size = 0; int rc = 0; while(1) { @@ -735,6 +740,7 @@ int main(int argc, char *argv[]) {"version", no_argument, NULL, 'v'}, {"debug", no_argument, NULL, 'g'}, {"side", required_argument, NULL, 'S'}, + {"skip", required_argument, NULL, 'k'}, {"toc", required_argument, NULL, 'T'}, {"clear", no_argument, NULL, 'c'}, {NULL, 0, NULL, 0 } @@ -826,6 +832,13 @@ int main(int argc, char *argv[]) case 'S': flash_side = atoi(optarg); break; + case 'k': + skip_size = strtoul(optarg, &endptr, 0); + if (*endptr != '\0') { + rc = 1; + no_action = true; + } + break; case 'T': if (!optarg) break; @@ -935,6 +948,13 @@ int main(int argc, char *argv[]) goto out; } + /* Skip only supported on read */ + if (skip_size && !do_read) { + fprintf(stderr, "--skip requires a --read command !\n"); + rc = 1; + goto out; + } + /* Program command should always come with a file */ if (program && !write_file) { fprintf(stderr, "Program with no file specified !\n"); @@ -1142,7 +1162,7 @@ int main(int argc, char *argv[]) } rc = 0; if (do_read) - rc = do_read_file(flash.bl, read_file, address, read_size); + rc = do_read_file(flash.bl, read_file, address, read_size, skip_size); if (!rc && erase_all) rc = erase_chip(&flash); else if (!rc && erase)
Add a --skip=N option to pflash to skip N number of bytes when reading. This would allow users to print the VERSION partition without the STB header by specifying the --skip=4096 argument, and it's a more generic solution rather than making pflash depend on secure/trusted boot code. Signed-off-by: Adriana Kobylak <anoo@linux.ibm.com> --- external/pflash/pflash.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-)