Change in coreboot[master]: util/ifdtool: Support modification of single Flash Descriptor

[Marcello Sylvester Bauer (Code Review)]

Marcello Sylvester Bauer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38711 )

Change subject: util/ifdtool: Support modification of single Flash Descriptor
......................................................................

util/ifdtool: Support modification of single Flash Descriptor

Add the capability to update the Flash Descriptor module directly
instead of raising a Segmentation Fault.
In this way it will be possible to add a Kconfig options to modify the
ifd descriptor at build-time.

Change-Id: Id3db09291af2bd2e759c283e316afd5da1fb4ca7
Signed-off-by: Marcello Sylvester Bauer 
---
M util/ifdtool/ifdtool.c
1 file changed, 23 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/38711/1

diff --git a/util/ifdtool/ifdtool.c b/util/ifdtool/ifdtool.c
index 0b6b210..fa2a564 100644
--- a/util/ifdtool/ifdtool.c
+++ b/util/ifdtool/ifdtool.c
@@ -1263,6 +1263,7 @@
    region_t new_regions[MAX_REGIONS];
    int new_extent = 0;
    char *new_image;
+	bool descriptor_only;

/* load current descriptor map and regions */
    frba_t *frba = find_frba(image, size);
@@ -1336,8 +1337,17 @@
    		new_extent = new_regions[i].limit;
    }

+	/* check if the image is actually a Flash Descriptor module */
+	descriptor_only = false;
+	/* compare image size to descriptor */
+	if (size == new_regions[0].size) {
+		printf("The image is a single Flash Descriptor module:\n");
+		printf("    Only the module will be modified\n");
+		descriptor_only = true;
+	}
+
    new_extent = next_pow2(new_extent - 1);
-	if (new_extent != size) {
+	if (!descriptor_only && new_extent != size) {
    	printf("The image has changed in size.\n");
    	printf("The old image is %d bytes.\n", size);
    	printf("The new image is %d bytes.\n", new_extent);
@@ -1367,6 +1377,12 @@
    		offset_current = current->size - new->size;
    	}

+		if (size < current->base + offset_current + copy_size) {
+			printf("Skip Descriptor %d (%s) (region missing in the old image)\n", i,
+				region_name(i));
+			continue;
+		};
+
    	printf("Copy Descriptor %d (%s) (%d bytes)\n", i,
    			region_name(i), copy_size);
    	printf("   from %08x+%08x:%08x (%10d)\n", current->base,
@@ -1384,10 +1400,15 @@
    if (!frba)
    	exit(EXIT_FAILURE);

+	printf("Modify Flash Descriptor regions\n");
    for (i = 1; i < max_regions; i++)
    	set_region(frba, i, &new_regions[i]);

-	write_image(filename, new_image, new_extent);
+	if (descriptor_only) {
+		write_image(filename, new_image, size);
+	} else {
+		write_image(filename, new_image, new_extent);
+	};
    free(new_image);
 }

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38711 )

Change subject: util/ifdtool: Support modification of single Flash Descriptor
......................................................................

Patch Set 1: Code-Review+1

(4 comments)

https://review.coreboot.org/c/coreboot/+/38711/1/util/ifdtool/ifdtool.c 
File util/ifdtool/ifdtool.c:

https://review.coreboot.org/c/coreboot/+/38711/1/util/ifdtool/ifdtool.c@1340 
PS1, Line 1340: module
module isn't used in this context. Please use 'region'

https://review.coreboot.org/c/coreboot/+/38711/1/util/ifdtool/ifdtool.c@1344 
PS1, Line 1344: module
Remove

https://review.coreboot.org/c/coreboot/+/38711/1/util/ifdtool/ifdtool.c@1346 
PS1, Line 1346: descriptor_only
Setting new_extent = size here should make descriptor_only obsolescent

https://review.coreboot.org/c/coreboot/+/38711/1/util/ifdtool/ifdtool.c@1381 
PS1, Line 1381: Descriptor
Skipping descriptor

[Marcello Sylvester Bauer (Code Review)]

Hello Patrick Rudolph, Stefan Reinauer, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/38711

to look at the new patch set (#2).

Change subject: util/ifdtool: Support modification of single Flash Descriptor
......................................................................

util/ifdtool: Support modification of single Flash Descriptor

Add the capability to update the Flash Descriptor module directly
instead of raising a Segmentation Fault.
In this way it will be possible to add a Kconfig options to modify the
ifd descriptor at build-time.

Change-Id: Id3db09291af2bd2e759c283e316afd5da1fb4ca7
Signed-off-by: Marcello Sylvester Bauer 
---
M util/ifdtool/ifdtool.c
1 file changed, 19 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/38711/2

[Marcello Sylvester Bauer (Code Review)]

Marcello Sylvester Bauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38711 )

Change subject: util/ifdtool: Support modification of single Flash Descriptor
......................................................................

Patch Set 2:

(4 comments)

https://review.coreboot.org/c/coreboot/+/38711/1/util/ifdtool/ifdtool.c 
File util/ifdtool/ifdtool.c:

https://review.coreboot.org/c/coreboot/+/38711/1/util/ifdtool/ifdtool.c@1340 
PS1, Line 1340: module
> module isn't used in this context. […]

Done

https://review.coreboot.org/c/coreboot/+/38711/1/util/ifdtool/ifdtool.c@1344 
PS1, Line 1344: module
> Remove

Done

https://review.coreboot.org/c/coreboot/+/38711/1/util/ifdtool/ifdtool.c@1346 
PS1, Line 1346: descriptor_only
> Setting new_extent = size here should make descriptor_only obsolescent

Done

https://review.coreboot.org/c/coreboot/+/38711/1/util/ifdtool/ifdtool.c@1381 
PS1, Line 1381: Descriptor
> Skipping descriptor

Done

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38711 )

Change subject: util/ifdtool: Support modification of single Flash Descriptor
......................................................................

Patch Set 2: Code-Review+1

(3 comments)

https://review.coreboot.org/c/coreboot/+/38711/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38711/2//COMMIT_MSG@9 
PS2, Line 9: module
drop `module`?

https://review.coreboot.org/c/coreboot/+/38711/2//COMMIT_MSG@10 
PS2, Line 10: instead of raising a Segmentation Fault.
Please separate paragraphs with an empty line.

https://review.coreboot.org/c/coreboot/+/38711/2/util/ifdtool/ifdtool.c 
File util/ifdtool/ifdtool.c:

https://review.coreboot.org/c/coreboot/+/38711/2/util/ifdtool/ifdtool.c@1342 
PS2, Line 1342: module
descriptor?

[Marcello Sylvester Bauer (Code Review)]

Hello Patrick Rudolph, Stefan Reinauer, build bot (Jenkins), Nico Huber,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/38711

to look at the new patch set (#3).

Change subject: util/ifdtool: Support modification of single Flash Descriptor
......................................................................

util/ifdtool: Support modification of single Flash Descriptor

Add the capability to update the Flash Descriptor module directly instead
of raising a Segmentation Fault. In this way it will be possible to add a
Kconfig options to modify the ifd descriptor at build-time.

Change-Id: Id3db09291af2bd2e759c283e316afd5da1fb4ca7
Signed-off-by: Marcello Sylvester Bauer 
---
M util/ifdtool/ifdtool.c
1 file changed, 19 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/38711/3

[Marcello Sylvester Bauer (Code Review)]

Hello Patrick Rudolph, Stefan Reinauer, build bot (Jenkins), Nico Huber,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/38711

to look at the new patch set (#4).

Change subject: util/ifdtool: Support modification of single Flash Descriptor
......................................................................

util/ifdtool: Support modification of single Flash Descriptor

Add the capability to update the Flash Descriptor directly instead
of raising a Segmentation Fault. In this way it will be possible to
add a Kconfig options to modify the ifd descriptor at build-time.

Change-Id: Id3db09291af2bd2e759c283e316afd5da1fb4ca7
Signed-off-by: Marcello Sylvester Bauer 
---
M util/ifdtool/ifdtool.c
1 file changed, 19 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/38711/4

[Marcello Sylvester Bauer (Code Review)]

Marcello Sylvester Bauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38711 )

Change subject: util/ifdtool: Support modification of single Flash Descriptor
......................................................................

Patch Set 4:

(3 comments)

https://review.coreboot.org/c/coreboot/+/38711/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38711/2//COMMIT_MSG@9 
PS2, Line 9: module
> drop `module`?

Done

https://review.coreboot.org/c/coreboot/+/38711/2//COMMIT_MSG@10 
PS2, Line 10: instead of raising a Segmentation Fault.
> Please separate paragraphs with an empty line.

Done

https://review.coreboot.org/c/coreboot/+/38711/2/util/ifdtool/ifdtool.c 
File util/ifdtool/ifdtool.c:

https://review.coreboot.org/c/coreboot/+/38711/2/util/ifdtool/ifdtool.c@1342 
PS2, Line 1342: module
> descriptor?

Done

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38711 )

Change subject: util/ifdtool: Support modification of single Flash Descriptor
......................................................................

Patch Set 4: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38711 )

Change subject: util/ifdtool: Support modification of single Flash Descriptor
......................................................................

util/ifdtool: Support modification of single Flash Descriptor

Add the capability to update the Flash Descriptor directly instead
of raising a Segmentation Fault. In this way it will be possible to
add a Kconfig options to modify the ifd descriptor at build-time.

Change-Id: Id3db09291af2bd2e759c283e316afd5da1fb4ca7
Signed-off-by: Marcello Sylvester Bauer 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/38711
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
---
M util/ifdtool/ifdtool.c
1 file changed, 19 insertions(+), 5 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved

diff --git a/util/ifdtool/ifdtool.c b/util/ifdtool/ifdtool.c
index 0b6b210..2bf2f4d 100644
--- a/util/ifdtool/ifdtool.c
+++ b/util/ifdtool/ifdtool.c
@@ -1336,11 +1336,18 @@
    		new_extent = new_regions[i].limit;
    }

-	new_extent = next_pow2(new_extent - 1);
-	if (new_extent != size) {
-		printf("The image has changed in size.\n");
-		printf("The old image is %d bytes.\n", size);
-		printf("The new image is %d bytes.\n", new_extent);
+	/* check if the image is actually a Flash Descriptor region */
+	if (size == new_regions[0].size) {
+		printf("The image is a single Flash Descriptor:\n");
+		printf("    Only the descriptor will be modified\n");
+		new_extent = size;
+	} else {
+		new_extent = next_pow2(new_extent - 1);
+		if (new_extent != size) {
+			printf("The image has changed in size.\n");
+			printf("The old image is %d bytes.\n", size);
+			printf("The new image is %d bytes.\n", new_extent);
+		}
    }

/* copy regions to a new image */
@@ -1367,6 +1374,12 @@
    		offset_current = current->size - new->size;
    	}

+		if (size < current->base + offset_current + copy_size) {
+			printf("Skip descriptor %d (%s) (region missing in the old image)\n", i,
+				region_name(i));
+			continue;
+		};
+
    	printf("Copy Descriptor %d (%s) (%d bytes)\n", i,
    			region_name(i), copy_size);
    	printf("   from %08x+%08x:%08x (%10d)\n", current->base,
@@ -1384,6 +1397,7 @@
    if (!frba)
    	exit(EXIT_FAILURE);

+	printf("Modify Flash Descriptor regions\n");
    for (i = 1; i < max_regions; i++)
    	set_region(frba, i, &new_regions[i]);