RFC: enable flashrom to write images of arbitrary size

Message ID 1478552439-16843-1-git-send-email-pmamonov@gmail.com
State New
Headers show

Commit Message

Peter Mamonov Nov. 7, 2016, 9 p.m.
Flashrom restricts an image size to be equal to a ROM capacity. This is
inconvenient in case of large and slow ROM chips, when only part of the ROM
should be updated. This patch removes this restriction in a quick-and-dirty
manner.

Signed-off-by: Peter Mamonov <pmamonov@gmail.com>
---
 flashrom.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

Comments

David Hendricks via flashrom Nov. 8, 2016, 7:22 p.m. | #1
Hi Peter,
Thanks for the patch! However, I suspect this will not be accepted
since the size check is an important safety measure for the general
use case.

In general layout files should be used for this sort of thing. The way
we've gone about this in the chromium.org branch is to augment the -i
syntax to allow the user to specify a region and corresponding file,
and we also added a "--fast-verify" mode to only verify regions which
were targeted. So for example you could do "flashrom -p <programmer>
-l <layout_file> -i region:filename.bin --fast-verify --ignore-fmap
-w" which will write filename.bin to the targeted region and verify
only that region.

Details here: https://www.chromium.org/chromium-os/packages/cros-flashrom#TOC-Partial-Reads-and-Writes
. LMK if this is any help.



On Mon, Nov 7, 2016 at 1:00 PM, Peter Mamonov <pmamonov@gmail.com> wrote:
> Flashrom restricts an image size to be equal to a ROM capacity. This is
> inconvenient in case of large and slow ROM chips, when only part of the ROM
> should be updated. This patch removes this restriction in a quick-and-dirty
> manner.
>
> Signed-off-by: Peter Mamonov <pmamonov@gmail.com>
> ---
>  flashrom.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/flashrom.c b/flashrom.c
> index d51a44c..f805e00 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -1255,30 +1255,30 @@ int read_buf_from_file(unsigned char *buf, unsigned long size,
>
>         if ((image = fopen(filename, "rb")) == NULL) {
>                 msg_gerr("Error: opening file \"%s\" failed: %s\n", filename, strerror(errno));
> -               return 1;
> +               return -1;
>         }
>         if (fstat(fileno(image), &image_stat) != 0) {
>                 msg_gerr("Error: getting metadata of file \"%s\" failed: %s\n", filename, strerror(errno));
>                 fclose(image);
> -               return 1;
> +               return -1;
>         }
> -       if (image_stat.st_size != size) {
> +       if (image_stat.st_size > size) {
>                 msg_gerr("Error: Image size (%jd B) doesn't match the flash chip's size (%lu B)!\n",
>                          (intmax_t)image_stat.st_size, size);
> -               fclose(image);
> -               return 1;
> +               return -1;
>         }
> +       size = image_stat.st_size;
>         numbytes = fread(buf, 1, size, image);
>         if (fclose(image)) {
>                 msg_gerr("Error: closing file \"%s\" failed: %s\n", filename, strerror(errno));
> -               return 1;
> +               return -1;
>         }
>         if (numbytes != size) {
>                 msg_gerr("Error: Failed to read complete file. Got %ld bytes, "
>                          "wanted %ld!\n", numbytes, size);
> -               return 1;
> +               return -1;
>         }
> -       return 0;
> +       return size;
>  #endif
>  }
>
> @@ -1481,7 +1481,10 @@ static int walk_eraseregions(struct flashctx *flash, int erasefunction,
>                  * members so the loop below won't be executed for them.
>                  */
>                 len = eraser.eraseblocks[i].size;
> -               for (j = 0; j < eraser.eraseblocks[i].count; j++) {
> +               for (j = 0;
> +                    j < eraser.eraseblocks[i].count &&
> +                    start + len <= flash->chip->total_size * 1024;
> +                    j++) {
>                         /* Print this for every block except the first one. */
>                         if (i || j)
>                                 msg_cdbg(", ");
> @@ -1988,11 +1991,12 @@ int doit(struct flashctx *flash, int force, const char *filename, int read_it,
>         }
>
>         if (write_it || verify_it) {
> -               if (read_buf_from_file(newcontents, size, filename)) {
> +               size = read_buf_from_file(newcontents, size, filename);
> +               if (size < 0) {
>                         ret = 1;
>                         goto out;
>                 }
> -
> +               flash->chip->total_size = size / 1024; /* FIXME */
>  #if CONFIG_INTERNAL == 1
>                 if (programmer == PROGRAMMER_INTERNAL && cb_check_image(newcontents, size) < 0) {
>                         if (force_boardmismatch) {
> --
> 2.1.4
>
>
> _______________________________________________
> flashrom mailing list
> flashrom@flashrom.org
> https://www.flashrom.org/mailman/listinfo/flashrom
Charlotte Plusplus Nov. 8, 2016, 11:20 p.m. | #2
Hello

That is very interesting. Do you have a way to automate that?

I am testing ram stuff with coreboot, I don't need to flash a full 8MB
image. I could just pass a -i of the cbfs content to only change the
ramstage, and do a manual md5 of the various parts contained to only target
the difference, but it seems a bit tedious.

It would be nice to do have a way to do kind of diff between the currenttly
flashed image and the desired image, and only flash the required changes.

Charlotte

On Tue, Nov 8, 2016 at 2:22 PM, David Hendricks via flashrom <
flashrom@flashrom.org> wrote:

> Hi Peter,
> Thanks for the patch! However, I suspect this will not be accepted
> since the size check is an important safety measure for the general
> use case.
>
> In general layout files should be used for this sort of thing. The way
> we've gone about this in the chromium.org branch is to augment the -i
> syntax to allow the user to specify a region and corresponding file,
> and we also added a "--fast-verify" mode to only verify regions which
> were targeted. So for example you could do "flashrom -p <programmer>
> -l <layout_file> -i region:filename.bin --fast-verify --ignore-fmap
> -w" which will write filename.bin to the targeted region and verify
> only that region.
>
> Details here: https://www.chromium.org/chromium-os/packages/cros-
> flashrom#TOC-Partial-Reads-and-Writes
> . LMK if this is any help.
>
>
>
> On Mon, Nov 7, 2016 at 1:00 PM, Peter Mamonov <pmamonov@gmail.com> wrote:
> > Flashrom restricts an image size to be equal to a ROM capacity. This is
> > inconvenient in case of large and slow ROM chips, when only part of the
> ROM
> > should be updated. This patch removes this restriction in a
> quick-and-dirty
> > manner.
> >
> > Signed-off-by: Peter Mamonov <pmamonov@gmail.com>
> > ---
> >  flashrom.c | 26 +++++++++++++++-----------
> >  1 file changed, 15 insertions(+), 11 deletions(-)
> >
> > diff --git a/flashrom.c b/flashrom.c
> > index d51a44c..f805e00 100644
> > --- a/flashrom.c
> > +++ b/flashrom.c
> > @@ -1255,30 +1255,30 @@ int read_buf_from_file(unsigned char *buf,
> unsigned long size,
> >
> >         if ((image = fopen(filename, "rb")) == NULL) {
> >                 msg_gerr("Error: opening file \"%s\" failed: %s\n",
> filename, strerror(errno));
> > -               return 1;
> > +               return -1;
> >         }
> >         if (fstat(fileno(image), &image_stat) != 0) {
> >                 msg_gerr("Error: getting metadata of file \"%s\" failed:
> %s\n", filename, strerror(errno));
> >                 fclose(image);
> > -               return 1;
> > +               return -1;
> >         }
> > -       if (image_stat.st_size != size) {
> > +       if (image_stat.st_size > size) {
> >                 msg_gerr("Error: Image size (%jd B) doesn't match the
> flash chip's size (%lu B)!\n",
> >                          (intmax_t)image_stat.st_size, size);
> > -               fclose(image);
> > -               return 1;
> > +               return -1;
> >         }
> > +       size = image_stat.st_size;
> >         numbytes = fread(buf, 1, size, image);
> >         if (fclose(image)) {
> >                 msg_gerr("Error: closing file \"%s\" failed: %s\n",
> filename, strerror(errno));
> > -               return 1;
> > +               return -1;
> >         }
> >         if (numbytes != size) {
> >                 msg_gerr("Error: Failed to read complete file. Got %ld
> bytes, "
> >                          "wanted %ld!\n", numbytes, size);
> > -               return 1;
> > +               return -1;
> >         }
> > -       return 0;
> > +       return size;
> >  #endif
> >  }
> >
> > @@ -1481,7 +1481,10 @@ static int walk_eraseregions(struct flashctx
> *flash, int erasefunction,
> >                  * members so the loop below won't be executed for them.
> >                  */
> >                 len = eraser.eraseblocks[i].size;
> > -               for (j = 0; j < eraser.eraseblocks[i].count; j++) {
> > +               for (j = 0;
> > +                    j < eraser.eraseblocks[i].count &&
> > +                    start + len <= flash->chip->total_size * 1024;
> > +                    j++) {
> >                         /* Print this for every block except the first
> one. */
> >                         if (i || j)
> >                                 msg_cdbg(", ");
> > @@ -1988,11 +1991,12 @@ int doit(struct flashctx *flash, int force,
> const char *filename, int read_it,
> >         }
> >
> >         if (write_it || verify_it) {
> > -               if (read_buf_from_file(newcontents, size, filename)) {
> > +               size = read_buf_from_file(newcontents, size, filename);
> > +               if (size < 0) {
> >                         ret = 1;
> >                         goto out;
> >                 }
> > -
> > +               flash->chip->total_size = size / 1024; /* FIXME */
> >  #if CONFIG_INTERNAL == 1
> >                 if (programmer == PROGRAMMER_INTERNAL &&
> cb_check_image(newcontents, size) < 0) {
> >                         if (force_boardmismatch) {
> > --
> > 2.1.4
> >
> >
> > _______________________________________________
> > flashrom mailing list
> > flashrom@flashrom.org
> > https://www.flashrom.org/mailman/listinfo/flashrom
>
>
>
> --
> David Hendricks (dhendrix)
> Systems Software Engineer, Google Inc.
>
> _______________________________________________
> flashrom mailing list
> flashrom@flashrom.org
> https://www.flashrom.org/mailman/listinfo/flashrom
>
Nico Huber Nov. 8, 2016, 11:48 p.m. | #3
Hi all,

On 09.11.2016 00:20, Charlotte Plusplus wrote:
> Hello
> 
> That is very interesting. Do you have a way to automate that?
> 
> I am testing ram stuff with coreboot, I don't need to flash a full 8MB
> image. I could just pass a -i of the cbfs content to only change the
> ramstage, and do a manual md5 of the various parts contained to only target
> the difference, but it seems a bit tedious.
> 
> It would be nice to do have a way to do kind of diff between the currenttly
> flashed image and the desired image, and only flash the required changes.

that is exactly what stock flashrom already does. The only thing that's
not optimized is the reading. It currently always reads the whole flash
twice, once before the flashing to create the diff, once for verifica-
tion (no matter what the layout says).

I did something to improve that for stock flashrom, see
https://www.flashrom.org/pipermail/flashrom/2016-May/014629.html
and the fourteen patches surrounding it. The magic there would be the
-A option (don't read All).

Nico

> 
> Charlotte
> 
> On Tue, Nov 8, 2016 at 2:22 PM, David Hendricks via flashrom <
> flashrom@flashrom.org> wrote:
> 
>> Hi Peter,
>> Thanks for the patch! However, I suspect this will not be accepted
>> since the size check is an important safety measure for the general
>> use case.
>>
>> In general layout files should be used for this sort of thing. The way
>> we've gone about this in the chromium.org branch is to augment the -i
>> syntax to allow the user to specify a region and corresponding file,
>> and we also added a "--fast-verify" mode to only verify regions which
>> were targeted. So for example you could do "flashrom -p <programmer>
>> -l <layout_file> -i region:filename.bin --fast-verify --ignore-fmap
>> -w" which will write filename.bin to the targeted region and verify
>> only that region.
>>
>> Details here: https://www.chromium.org/chromium-os/packages/cros-
>> flashrom#TOC-Partial-Reads-and-Writes
>> . LMK if this is any help.
>>
>>
>>
>> On Mon, Nov 7, 2016 at 1:00 PM, Peter Mamonov <pmamonov@gmail.com> wrote:
>>> Flashrom restricts an image size to be equal to a ROM capacity. This is
>>> inconvenient in case of large and slow ROM chips, when only part of the
>> ROM
>>> should be updated. This patch removes this restriction in a
>> quick-and-dirty
>>> manner.
>>>
>>> Signed-off-by: Peter Mamonov <pmamonov@gmail.com>
>>> ---
>>>  flashrom.c | 26 +++++++++++++++-----------
>>>  1 file changed, 15 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/flashrom.c b/flashrom.c
>>> index d51a44c..f805e00 100644
>>> --- a/flashrom.c
>>> +++ b/flashrom.c
>>> @@ -1255,30 +1255,30 @@ int read_buf_from_file(unsigned char *buf,
>> unsigned long size,
>>>
>>>         if ((image = fopen(filename, "rb")) == NULL) {
>>>                 msg_gerr("Error: opening file \"%s\" failed: %s\n",
>> filename, strerror(errno));
>>> -               return 1;
>>> +               return -1;
>>>         }
>>>         if (fstat(fileno(image), &image_stat) != 0) {
>>>                 msg_gerr("Error: getting metadata of file \"%s\" failed:
>> %s\n", filename, strerror(errno));
>>>                 fclose(image);
>>> -               return 1;
>>> +               return -1;
>>>         }
>>> -       if (image_stat.st_size != size) {
>>> +       if (image_stat.st_size > size) {
>>>                 msg_gerr("Error: Image size (%jd B) doesn't match the
>> flash chip's size (%lu B)!\n",
>>>                          (intmax_t)image_stat.st_size, size);
>>> -               fclose(image);
>>> -               return 1;
>>> +               return -1;
>>>         }
>>> +       size = image_stat.st_size;
>>>         numbytes = fread(buf, 1, size, image);
>>>         if (fclose(image)) {
>>>                 msg_gerr("Error: closing file \"%s\" failed: %s\n",
>> filename, strerror(errno));
>>> -               return 1;
>>> +               return -1;
>>>         }
>>>         if (numbytes != size) {
>>>                 msg_gerr("Error: Failed to read complete file. Got %ld
>> bytes, "
>>>                          "wanted %ld!\n", numbytes, size);
>>> -               return 1;
>>> +               return -1;
>>>         }
>>> -       return 0;
>>> +       return size;
>>>  #endif
>>>  }
>>>
>>> @@ -1481,7 +1481,10 @@ static int walk_eraseregions(struct flashctx
>> *flash, int erasefunction,
>>>                  * members so the loop below won't be executed for them.
>>>                  */
>>>                 len = eraser.eraseblocks[i].size;
>>> -               for (j = 0; j < eraser.eraseblocks[i].count; j++) {
>>> +               for (j = 0;
>>> +                    j < eraser.eraseblocks[i].count &&
>>> +                    start + len <= flash->chip->total_size * 1024;
>>> +                    j++) {
>>>                         /* Print this for every block except the first
>> one. */
>>>                         if (i || j)
>>>                                 msg_cdbg(", ");
>>> @@ -1988,11 +1991,12 @@ int doit(struct flashctx *flash, int force,
>> const char *filename, int read_it,
>>>         }
>>>
>>>         if (write_it || verify_it) {
>>> -               if (read_buf_from_file(newcontents, size, filename)) {
>>> +               size = read_buf_from_file(newcontents, size, filename);
>>> +               if (size < 0) {
>>>                         ret = 1;
>>>                         goto out;
>>>                 }
>>> -
>>> +               flash->chip->total_size = size / 1024; /* FIXME */
>>>  #if CONFIG_INTERNAL == 1
>>>                 if (programmer == PROGRAMMER_INTERNAL &&
>> cb_check_image(newcontents, size) < 0) {
>>>                         if (force_boardmismatch) {
>>> --
>>> 2.1.4
>>>
>>>
>>> _______________________________________________
>>> flashrom mailing list
>>> flashrom@flashrom.org
>>> https://www.flashrom.org/mailman/listinfo/flashrom
>>
>>
>>
>> --
>> David Hendricks (dhendrix)
>> Systems Software Engineer, Google Inc.
>>
>> _______________________________________________
>> flashrom mailing list
>> flashrom@flashrom.org
>> https://www.flashrom.org/mailman/listinfo/flashrom
>>
> 
> 
> 
> _______________________________________________
> flashrom mailing list
> flashrom@flashrom.org
> https://www.flashrom.org/mailman/listinfo/flashrom
>
Peter Mamonov Nov. 9, 2016, 10:44 a.m. | #4
Hi David,

On Tue, Nov 08, 2016 at 11:22:56AM -0800, David Hendricks wrote:
> Hi Peter,
> Thanks for the patch! However, I suspect this will not be accepted
> since the size check is an important safety measure for the general
> use case.
> 
> In general layout files should be used for this sort of thing. The way
> we've gone about this in the chromium.org branch is to augment the -i
> syntax to allow the user to specify a region and corresponding file,
> and we also added a "--fast-verify" mode to only verify regions which
> were targeted. So for example you could do "flashrom -p <programmer>
> -l <layout_file> -i region:filename.bin --fast-verify --ignore-fmap
> -w" which will write filename.bin to the targeted region and verify
> only that region.
> 
> Details here: https://www.chromium.org/chromium-os/packages/cros-flashrom#TOC-Partial-Reads-and-Writes
> . LMK if this is any help.

Thanks for the tip! Looks like an ultimate solution. I'll try it and report the
results later.

Regards,
Peter

> 
> 
> 
> On Mon, Nov 7, 2016 at 1:00 PM, Peter Mamonov <pmamonov@gmail.com> wrote:
> > Flashrom restricts an image size to be equal to a ROM capacity. This is
> > inconvenient in case of large and slow ROM chips, when only part of the ROM
> > should be updated. This patch removes this restriction in a quick-and-dirty
> > manner.
> >
> > Signed-off-by: Peter Mamonov <pmamonov@gmail.com>
> > ---
> >  flashrom.c | 26 +++++++++++++++-----------
> >  1 file changed, 15 insertions(+), 11 deletions(-)
> >
> > diff --git a/flashrom.c b/flashrom.c
> > index d51a44c..f805e00 100644
> > --- a/flashrom.c
> > +++ b/flashrom.c
> > @@ -1255,30 +1255,30 @@ int read_buf_from_file(unsigned char *buf, unsigned long size,
> >
> >         if ((image = fopen(filename, "rb")) == NULL) {
> >                 msg_gerr("Error: opening file \"%s\" failed: %s\n", filename, strerror(errno));
> > -               return 1;
> > +               return -1;
> >         }
> >         if (fstat(fileno(image), &image_stat) != 0) {
> >                 msg_gerr("Error: getting metadata of file \"%s\" failed: %s\n", filename, strerror(errno));
> >                 fclose(image);
> > -               return 1;
> > +               return -1;
> >         }
> > -       if (image_stat.st_size != size) {
> > +       if (image_stat.st_size > size) {
> >                 msg_gerr("Error: Image size (%jd B) doesn't match the flash chip's size (%lu B)!\n",
> >                          (intmax_t)image_stat.st_size, size);
> > -               fclose(image);
> > -               return 1;
> > +               return -1;
> >         }
> > +       size = image_stat.st_size;
> >         numbytes = fread(buf, 1, size, image);
> >         if (fclose(image)) {
> >                 msg_gerr("Error: closing file \"%s\" failed: %s\n", filename, strerror(errno));
> > -               return 1;
> > +               return -1;
> >         }
> >         if (numbytes != size) {
> >                 msg_gerr("Error: Failed to read complete file. Got %ld bytes, "
> >                          "wanted %ld!\n", numbytes, size);
> > -               return 1;
> > +               return -1;
> >         }
> > -       return 0;
> > +       return size;
> >  #endif
> >  }
> >
> > @@ -1481,7 +1481,10 @@ static int walk_eraseregions(struct flashctx *flash, int erasefunction,
> >                  * members so the loop below won't be executed for them.
> >                  */
> >                 len = eraser.eraseblocks[i].size;
> > -               for (j = 0; j < eraser.eraseblocks[i].count; j++) {
> > +               for (j = 0;
> > +                    j < eraser.eraseblocks[i].count &&
> > +                    start + len <= flash->chip->total_size * 1024;
> > +                    j++) {
> >                         /* Print this for every block except the first one. */
> >                         if (i || j)
> >                                 msg_cdbg(", ");
> > @@ -1988,11 +1991,12 @@ int doit(struct flashctx *flash, int force, const char *filename, int read_it,
> >         }
> >
> >         if (write_it || verify_it) {
> > -               if (read_buf_from_file(newcontents, size, filename)) {
> > +               size = read_buf_from_file(newcontents, size, filename);
> > +               if (size < 0) {
> >                         ret = 1;
> >                         goto out;
> >                 }
> > -
> > +               flash->chip->total_size = size / 1024; /* FIXME */
> >  #if CONFIG_INTERNAL == 1
> >                 if (programmer == PROGRAMMER_INTERNAL && cb_check_image(newcontents, size) < 0) {
> >                         if (force_boardmismatch) {
> > --
> > 2.1.4
> >
> >
> > _______________________________________________
> > flashrom mailing list
> > flashrom@flashrom.org
> > https://www.flashrom.org/mailman/listinfo/flashrom
> 
> 
> 
> -- 
> David Hendricks (dhendrix)
> Systems Software Engineer, Google Inc.

Patch

diff --git a/flashrom.c b/flashrom.c
index d51a44c..f805e00 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -1255,30 +1255,30 @@  int read_buf_from_file(unsigned char *buf, unsigned long size,
 
 	if ((image = fopen(filename, "rb")) == NULL) {
 		msg_gerr("Error: opening file \"%s\" failed: %s\n", filename, strerror(errno));
-		return 1;
+		return -1;
 	}
 	if (fstat(fileno(image), &image_stat) != 0) {
 		msg_gerr("Error: getting metadata of file \"%s\" failed: %s\n", filename, strerror(errno));
 		fclose(image);
-		return 1;
+		return -1;
 	}
-	if (image_stat.st_size != size) {
+	if (image_stat.st_size > size) {
 		msg_gerr("Error: Image size (%jd B) doesn't match the flash chip's size (%lu B)!\n",
 			 (intmax_t)image_stat.st_size, size);
-		fclose(image);
-		return 1;
+		return -1;
 	}
+	size = image_stat.st_size;
 	numbytes = fread(buf, 1, size, image);
 	if (fclose(image)) {
 		msg_gerr("Error: closing file \"%s\" failed: %s\n", filename, strerror(errno));
-		return 1;
+		return -1;
 	}
 	if (numbytes != size) {
 		msg_gerr("Error: Failed to read complete file. Got %ld bytes, "
 			 "wanted %ld!\n", numbytes, size);
-		return 1;
+		return -1;
 	}
-	return 0;
+	return size;
 #endif
 }
 
@@ -1481,7 +1481,10 @@  static int walk_eraseregions(struct flashctx *flash, int erasefunction,
 		 * members so the loop below won't be executed for them.
 		 */
 		len = eraser.eraseblocks[i].size;
-		for (j = 0; j < eraser.eraseblocks[i].count; j++) {
+		for (j = 0;
+		     j < eraser.eraseblocks[i].count &&
+		     start + len <= flash->chip->total_size * 1024;
+		     j++) {
 			/* Print this for every block except the first one. */
 			if (i || j)
 				msg_cdbg(", ");
@@ -1988,11 +1991,12 @@  int doit(struct flashctx *flash, int force, const char *filename, int read_it,
 	}
 
 	if (write_it || verify_it) {
-		if (read_buf_from_file(newcontents, size, filename)) {
+		size = read_buf_from_file(newcontents, size, filename);
+		if (size < 0) {
 			ret = 1;
 			goto out;
 		}
-
+		flash->chip->total_size = size / 1024; /* FIXME */
 #if CONFIG_INTERNAL == 1
 		if (programmer == PROGRAMMER_INTERNAL && cb_check_image(newcontents, size) < 0) {
 			if (force_boardmismatch) {