Change in ...coreboot[master]: util/amdfwtool: Align PSP NVRAM

List overview All Threads
Download

newer

older

Change in ...coreboot[master]:...

Marshall Dawson (Code Review)

11 Jun 2019 11 Jun '19

6 p.m.

Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33397

Change subject: util/amdfwtool: Align PSP NVRAM ......................................................................

util/amdfwtool: Align PSP NVRAM

Align the PSP's NVRAM item since it's intended to be updateable in the flash device.

Change-Id: I6b28525624b95b411cc82de0cbe430ea7871149d Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M util/amdfwtool/amdfwtool.c 1 file changed, 26 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/33397/1

diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c index 51e913c..f2ce34c 100644 --- a/util/amdfwtool/amdfwtool.c +++ b/util/amdfwtool/amdfwtool.c @@ -79,6 +79,7 @@ #define ALIGN(val, by) (((val) + (by) - 1) & ~((by) - 1)) #define TABLE_ALIGNMENT 0x1000U #define BLOB_ALIGNMENT 0x100U +#define ERASE_ALIGNMENT 0x1000U

#define EMBEDDED_FW_SIGNATURE 0x55aa55aa #define PSP_COOKIE 0x50535024 /* 'PSP$' */ @@ -457,6 +458,31 @@ else pspdir->entries[count].addr = 1; count++; + } else if (fw_table[i].type == AMD_FW_PSP_NVRAM) { + if (fw_table[i].filename == NULL) + continue; + /* TODO: Add a way to reserve for NVRAM without + * requiring a filename. This isn't a feature used + * by coreboot systems, so priority is very low. + */ + ctx->current = ALIGN(ctx->current, ERASE_ALIGNMENT); + bytes = copy_blob(BUFF_CURRENT(*ctx), + fw_table[i].filename, BUFF_ROOM(*ctx)); + if (bytes <= 0) { + free(ctx->rom); + exit(1); + } + + pspdir->entries[count].type = fw_table[i].type; + pspdir->entries[count].subprog = fw_table[i].subprog; + pspdir->entries[count].rsvd = 0; + pspdir->entries[count].size = (uint32_t)bytes; + pspdir->entries[count].addr = RUN_CURRENT(*ctx); + + ctx->current = ALIGN(ctx->current + bytes, + BLOB_ALIGNMENT); + ctx->current = ALIGN(ctx->current, ERASE_ALIGNMENT); + count++; } else if (fw_table[i].filename != NULL) { bytes = copy_blob(BUFF_CURRENT(*ctx), fw_table[i].filename, BUFF_ROOM(*ctx));

Attachments:

attachment.htm (text/html — 4.9 KB)

Show replies by date

Martin Roth (Code Review)

11 Jun 11 Jun

8 p.m.

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33397 )

Change subject: util/amdfwtool: Align PSP NVRAM ......................................................................

Patch Set 1: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/33397 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6b28525624b95b411cc82de0cbe430ea7871149d Gerrit-Change-Number: 33397 Gerrit-PatchSet: 1 Gerrit-Owner: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 11 Jun 2019 18:00:36 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Martin Roth (Code Review)

8:06 p.m.

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33397 )

Change subject: util/amdfwtool: Align PSP NVRAM ......................................................................

Patch Set 1: Code-Review+1

(1 comment)

https://review.coreboot.org/#/c/33397/1/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:

https://review.coreboot.org/#/c/33397/1/util/amdfwtool/amdfwtool.c@479 PS1, Line 479: (uint32_t)bytes; Should this be rounded up to fill the entire 4k flash sector?

-- To view, visit https://review.coreboot.org/c/coreboot/+/33397 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6b28525624b95b411cc82de0cbe430ea7871149d Gerrit-Change-Number: 33397 Gerrit-PatchSet: 1 Gerrit-Owner: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 11 Jun 2019 18:06:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Kyösti Mälkki (Code Review)

10:29 p.m.

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33397 )

Change subject: util/amdfwtool: Align PSP NVRAM ......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/#/c/33397/1/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:

https://review.coreboot.org/#/c/33397/1/util/amdfwtool/amdfwtool.c@484 PS1, Line 484: ctx->current = ALIGN(ctx->current, ERASE_ALIGNMENT); Why the intermediate value with BLOB_ALIGNMENT?

https://review.coreboot.org/#/c/33397/1/util/amdfwtool/amdfwtool.c@508 PS1, Line 508: if (count > MAX_PSP_ENTRIES) { Unrelated, but you really should test for invalid indexing of arrays before the access happens.

-- To view, visit https://review.coreboot.org/c/coreboot/+/33397 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6b28525624b95b411cc82de0cbe430ea7871149d Gerrit-Change-Number: 33397 Gerrit-PatchSet: 1 Gerrit-Owner: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Comment-Date: Tue, 11 Jun 2019 20:29:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Marshall Dawson (Code Review)

12 Jun 12 Jun

12:47 a.m.

Hello build bot (Jenkins), Martin Roth,

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

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

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

Change subject: util/amdfwtool: Align PSP NVRAM ......................................................................

util/amdfwtool: Align PSP NVRAM

Align the PSP's NVRAM item since it's intended to be updateable in the flash device.

Change-Id: I6b28525624b95b411cc82de0cbe430ea7871149d Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M util/amdfwtool/amdfwtool.c 1 file changed, 28 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/33397/2

Marshall Dawson (Code Review)

12:51 a.m.

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33397 )

Change subject: util/amdfwtool: Align PSP NVRAM ......................................................................

Patch Set 2:

(3 comments)

https://review.coreboot.org/#/c/33397/1/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:

https://review.coreboot.org/#/c/33397/1/util/amdfwtool/amdfwtool.c@479 PS1, Line 479: (uint32_t)bytes;

...

Should this be rounded up to fill the entire 4k flash sector?

Yes, done

https://review.coreboot.org/#/c/33397/1/util/amdfwtool/amdfwtool.c@484 PS1, Line 484: ctx->current = ALIGN(ctx->current, ERASE_ALIGNMENT);

...

Why the intermediate value with BLOB_ALIGNMENT?

I didn't want this code to need to know which was the larger granularity. Will change it to use a max() value instead.

https://review.coreboot.org/#/c/33397/1/util/amdfwtool/amdfwtool.c@508 PS1, Line 508: if (count > MAX_PSP_ENTRIES) {

...

Unrelated, but you really should test for invalid indexing of arrays before the access happens.

Maybe I'll consider that for a follow-on.

Since the utility allocates the size of the rom, I don't think it's an emergency, and in fact count should never be greater than the number of items in the fw_table. We allocate the amount of memory equal to the flash's size. The intent here was to not write a higher count into the header than the header was meant to describe.

-- To view, visit https://review.coreboot.org/c/coreboot/+/33397 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6b28525624b95b411cc82de0cbe430ea7871149d Gerrit-Change-Number: 33397 Gerrit-PatchSet: 2 Gerrit-Owner: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Comment-Date: Tue, 11 Jun 2019 22:51:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Kyösti Mälkki kyosti.malkki@gmail.com Comment-In-Reply-To: Martin Roth martinroth@google.com Gerrit-MessageType: comment

Martin Roth (Code Review)

10:53 p.m.

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33397 )

Change subject: util/amdfwtool: Align PSP NVRAM ......................................................................

Patch Set 2: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/33397 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6b28525624b95b411cc82de0cbe430ea7871149d Gerrit-Change-Number: 33397 Gerrit-PatchSet: 2 Gerrit-Owner: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Wed, 12 Jun 2019 20:53:11 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Martin Roth (Code Review)

13 Jun 13 Jun

8 p.m.

Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33397 )

Change subject: util/amdfwtool: Align PSP NVRAM ......................................................................

util/amdfwtool: Align PSP NVRAM

Align the PSP's NVRAM item since it's intended to be updateable in the flash device.

Change-Id: I6b28525624b95b411cc82de0cbe430ea7871149d Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/33397 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Martin Roth martinroth@google.com --- M util/amdfwtool/amdfwtool.c 1 file changed, 28 insertions(+), 0 deletions(-)

Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved

diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c index a3a692d..4a05dfe 100644 --- a/util/amdfwtool/amdfwtool.c +++ b/util/amdfwtool/amdfwtool.c @@ -77,8 +77,11 @@ #define MIN_ROM_KB 256

#define ALIGN(val, by) (((val) + (by) - 1) & ~((by) - 1)) +#define _MAX(A, B) (((A) > (B)) ? (A) : (B)) +#define ERASE_ALIGNMENT 0x1000U #define TABLE_ALIGNMENT 0x1000U #define BLOB_ALIGNMENT 0x100U +#define BLOB_ERASE_ALIGNMENT _MAX(BLOB_ALIGNMENT, ERASE_ALIGNMENT)

#define DEFAULT_SOFT_FUSE_CHAIN "0x1"

@@ -456,6 +459,31 @@ pspdir->entries[count].size = 0xFFFFFFFF; pspdir->entries[count].addr = fw_table[i].other; count++; + } else if (fw_table[i].type == AMD_FW_PSP_NVRAM) { + if (fw_table[i].filename == NULL) + continue; + /* TODO: Add a way to reserve for NVRAM without + * requiring a filename. This isn't a feature used + * by coreboot systems, so priority is very low. + */ + ctx->current = ALIGN(ctx->current, ERASE_ALIGNMENT); + bytes = copy_blob(BUFF_CURRENT(*ctx), + fw_table[i].filename, BUFF_ROOM(*ctx)); + if (bytes <= 0) { + free(ctx->rom); + exit(1); + } + + pspdir->entries[count].type = fw_table[i].type; + pspdir->entries[count].subprog = fw_table[i].subprog; + pspdir->entries[count].rsvd = 0; + pspdir->entries[count].size = ALIGN(bytes, + ERASE_ALIGNMENT); + pspdir->entries[count].addr = RUN_CURRENT(*ctx); + + ctx->current = ALIGN(ctx->current + bytes, + BLOB_ERASE_ALIGNMENT); + count++; } else if (fw_table[i].filename != NULL) { bytes = copy_blob(BUFF_CURRENT(*ctx), fw_table[i].filename, BUFF_ROOM(*ctx));

-- To view, visit https://review.coreboot.org/c/coreboot/+/33397 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6b28525624b95b411cc82de0cbe430ea7871149d Gerrit-Change-Number: 33397 Gerrit-PatchSet: 3 Gerrit-Owner: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: merged

1981

days inactive

1983

days old

coreboot-gerrit@coreboot.org

7 comments

3 participants

tags (0)

participants (3)

Kyösti Mälkki (Code Review)
Marshall Dawson (Code Review)
Martin Roth (Code Review)