Change in coreboot[master]: util/amdfwtool: If no APOB_NV is specified, don't error out

List overview All Threads
Download

newer

older

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

Change in coreboot[master]:...

Martin Roth (Code Review)

29 Jul 2020 29 Jul '20

5:16 p.m.

Martin Roth has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44045 )

Change subject: util/amdfwtool: If no APOB_NV is specified, don't error out ......................................................................

util/amdfwtool: If no APOB_NV is specified, don't error out

amdfwtool currently assumes that we MUST have an apob_nv area if we have an aopb. This is not required, so if neither the apob_nv size or base are specified, just move on.

BUG=b:158363448 TEST=Build an image with no APOB_NV region. Dump regions to show that it's not there.

Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: Ibaeacd3dcdfd73f690df61c2a19d39bbb9dcc838 --- M util/amdfwtool/amdfwtool.c 1 file changed, 4 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/44045/1

diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c index 14ffdb3..26d8d04 100644 --- a/util/amdfwtool/amdfwtool.c +++ b/util/amdfwtool/amdfwtool.c @@ -930,7 +930,10 @@ if (fw_table[i].type == AMD_BIOS_APOB_NV && !fw_table[i].size) { /* Attempt to determine whether this is an error */ apob_idx = find_bios_entry(AMD_BIOS_APOB); - if (apob_idx < 0 || !fw_table[apob_idx].dest) { + if (!fw_table[i].src) { + /* With no source or size specified, don't use the APOV NV */ + continue; + } else if (apob_idx < 0 || !fw_table[apob_idx].dest) { /* APOV NV not expected to be used */ continue; } else {

Attachments:

attachment.htm (text/html — 3.0 KB)

Show replies by date

Martin Roth (Code Review)

29 Jul 29 Jul

5:26 p.m.

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

Change subject: util/amdfwtool: If no APOB_NV is specified, don't error out ......................................................................

Patch Set 1:

(3 comments)

https://review.coreboot.org/c/coreboot/+/44045/1/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:

https://review.coreboot.org/c/coreboot/+/44045/1/util/amdfwtool/amdfwtool.c@... PS1, Line 922: if (fw_table[i].type == AMD_BIOS_APOB_NV && fw_table[i].src) { : if (!fw_table[i].size) { We check for src without size here.

https://review.coreboot.org/c/coreboot/+/44045/1/util/amdfwtool/amdfwtool.c@... PS1, Line 930: !fw_table[i].size) { If we enter this loop, we already know that there's no size specified.

https://review.coreboot.org/c/coreboot/+/44045/1/util/amdfwtool/amdfwtool.c@... PS1, Line 933: if (!fw_table[i].src) { so here we only need to check to see if there's also no source specified.

-- To view, visit https://review.coreboot.org/c/coreboot/+/44045 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibaeacd3dcdfd73f690df61c2a19d39bbb9dcc838 Gerrit-Change-Number: 44045 Gerrit-PatchSet: 1 Gerrit-Owner: Martin Roth martinroth@google.com Gerrit-Reviewer: Eric Peers epeers@google.com Gerrit-Reviewer: Felix Held felix-coreboot@felixheld.de Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: Rob Barnes robbarnes@google.com Gerrit-Comment-Date: Thu, 30 Jul 2020 00:26:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Aaron Durbin (Code Review)

30 Jul 30 Jul

7:32 a.m.

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44045 )

Change subject: util/amdfwtool: If no APOB_NV is specified, don't error out ......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/44045/1/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:

https://review.coreboot.org/c/coreboot/+/44045/1/util/amdfwtool/amdfwtool.c@... PS1, Line 932: apob_idx = find_bios_entry(AMD_BIOS_APOB); Why do we perform this lookup prior to checking src field?

https://review.coreboot.org/c/coreboot/+/44045/1/util/amdfwtool/amdfwtool.c@... PS1, Line 933: if (!fw_table[i].src) {

...

so here we only need to check to see if there's also no source specified.

I'm not sure what scenarios this existing code is trying to cover, but I feel like this code is not really very clear. Aren't the scenarios the following?

1. error if src and !size 2. !dest in existing apob? -> apob not used 3. And you want to add !src && !size -> apob not used

if (fw_table[i].type == AMD_BIOS_APOB_NV) { if (!fw_table[i].size && !fw_table[i].src) continue; // not used if (fw_table[i].src && !fw_table[i].size) error apob_idx = find_bios_entry(); if (apob_idx < 0 || !fw_table[apob_idx].dest) continue; // not used // fall through to the baseline error }

-- To view, visit https://review.coreboot.org/c/coreboot/+/44045 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibaeacd3dcdfd73f690df61c2a19d39bbb9dcc838 Gerrit-Change-Number: 44045 Gerrit-PatchSet: 1 Gerrit-Owner: Martin Roth martinroth@google.com Gerrit-Reviewer: Eric Peers epeers@google.com Gerrit-Reviewer: Felix Held felix-coreboot@felixheld.de Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: Rob Barnes robbarnes@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-Comment-Date: Thu, 30 Jul 2020 14:32:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Martin Roth martinroth@google.com Gerrit-MessageType: comment

Martin Roth (Code Review)

17 Aug 17 Aug

7:35 p.m.

New subject: Change in coreboot[master]: util/amdfwtool: Refactor APOB_NV requirements

Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Marshall Dawson, Eric Peers, Rob Barnes, Felix Held,

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

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

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

Change subject: util/amdfwtool: Refactor APOB_NV requirements ......................................................................

util/amdfwtool: Refactor APOB_NV requirements

amdfwtool currently assumes that we MUST have an apob_nv area if we have an aopb. This is not required, so if neither the apob_nv size or base are specified, just move on.

BUG=b:158363448 TEST=Build an image with no APOB_NV region. Dump regions to show that it's not there.

Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: Ibaeacd3dcdfd73f690df61c2a19d39bbb9dcc838 --- M util/amdfwtool/amdfwtool.c 1 file changed, 8 insertions(+), 15 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/44045/2

-- To view, visit https://review.coreboot.org/c/coreboot/+/44045 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibaeacd3dcdfd73f690df61c2a19d39bbb9dcc838 Gerrit-Change-Number: 44045 Gerrit-PatchSet: 2 Gerrit-Owner: Martin Roth martinroth@google.com Gerrit-Reviewer: Eric Peers epeers@google.com Gerrit-Reviewer: Felix Held felix-coreboot@felixheld.de Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: Rob Barnes robbarnes@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-MessageType: newpatchset

Martin Roth (Code Review)

18 Aug 18 Aug

1:25 p.m.

New subject: Change in coreboot[master]: util/amdfwtool: Refactor APOB_NV requirements

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

Change subject: util/amdfwtool: Refactor APOB_NV requirements ......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/44045/1/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:

https://review.coreboot.org/c/coreboot/+/44045/1/util/amdfwtool/amdfwtool.c@... PS1, Line 932: apob_idx = find_bios_entry(AMD_BIOS_APOB);

...

Why do we perform this lookup prior to checking src field?

I have no idea why this was initially written the way it was. My guess is that it was just added a piece at a time and this was a convenient way for the author to do it.

https://review.coreboot.org/c/coreboot/+/44045/1/util/amdfwtool/amdfwtool.c@... PS1, Line 933: if (!fw_table[i].src) {

...

I'm not sure what scenarios this existing code is trying to cover, but I feel like this code is not […]

I was trying to just make a minor modification since I believe Zheng Bao is refactoring the file, but since that's not done yet, I'll go ahead and update it.

-- To view, visit https://review.coreboot.org/c/coreboot/+/44045 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibaeacd3dcdfd73f690df61c2a19d39bbb9dcc838 Gerrit-Change-Number: 44045 Gerrit-PatchSet: 2 Gerrit-Owner: Martin Roth martinroth@google.com Gerrit-Reviewer: Eric Peers epeers@google.com Gerrit-Reviewer: Felix Held felix-coreboot@felixheld.de Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: Rob Barnes robbarnes@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-Comment-Date: Tue, 18 Aug 2020 20:25:13 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Martin Roth martinroth@google.com Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Gerrit-MessageType: comment

Martin Roth (Code Review)

20 Aug 20 Aug

4:27 p.m.

New subject: Change in coreboot[master]: util/amdfwtool: Refactor APOB_NV requirements

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

Change subject: util/amdfwtool: Refactor APOB_NV requirements ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44045/1/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:

https://review.coreboot.org/c/coreboot/+/44045/1/util/amdfwtool/amdfwtool.c@... PS1, Line 932: apob_idx = find_bios_entry(AMD_BIOS_APOB);

...

I have no idea why this was initially written the way it was. […]

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/44045 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibaeacd3dcdfd73f690df61c2a19d39bbb9dcc838 Gerrit-Change-Number: 44045 Gerrit-PatchSet: 2 Gerrit-Owner: Martin Roth martinroth@google.com Gerrit-Reviewer: Eric Peers epeers@google.com Gerrit-Reviewer: Felix Held felix-coreboot@felixheld.de Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: Rob Barnes robbarnes@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-Comment-Date: Thu, 20 Aug 2020 23:27:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Martin Roth martinroth@google.com Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Gerrit-MessageType: comment

Aaron Durbin (Code Review)

21 Aug 21 Aug

11:46 a.m.

New subject: Change in coreboot[master]: util/amdfwtool: Refactor APOB_NV requirements

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44045 )

Change subject: util/amdfwtool: Refactor APOB_NV requirements ......................................................................

Patch Set 3: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/44045 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibaeacd3dcdfd73f690df61c2a19d39bbb9dcc838 Gerrit-Change-Number: 44045 Gerrit-PatchSet: 3 Gerrit-Owner: Martin Roth martinroth@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Eric Peers epeers@google.com Gerrit-Reviewer: Felix Held felix-coreboot@felixheld.de Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: Rob Barnes robbarnes@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Fri, 21 Aug 2020 18:46:45 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Aaron Durbin (Code Review)

11:46 a.m.

New subject: Change in coreboot[master]: util/amdfwtool: Refactor APOB_NV requirements

Aaron Durbin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44045 )

Change subject: util/amdfwtool: Refactor APOB_NV requirements ......................................................................

util/amdfwtool: Refactor APOB_NV requirements

amdfwtool currently assumes that we MUST have an apob_nv area if we have an aopb. This is not required, so if neither the apob_nv size or base are specified, just move on.

BUG=b:158363448 TEST=Build an image with no APOB_NV region. Dump regions to show that it's not there.

Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: Ibaeacd3dcdfd73f690df61c2a19d39bbb9dcc838 Reviewed-on: https://review.coreboot.org/c/coreboot/+/44045 Reviewed-by: Aaron Durbin adurbin@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M util/amdfwtool/amdfwtool.c 1 file changed, 8 insertions(+), 15 deletions(-)

Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved

diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c index 14ffdb3..499a1bd 100644 --- a/util/amdfwtool/amdfwtool.c +++ b/util/amdfwtool/amdfwtool.c @@ -918,26 +918,19 @@

/* BIOS Directory items may have additional requirements */

- /* APOB_NV must have a size if it has a source */ - if (fw_table[i].type == AMD_BIOS_APOB_NV && fw_table[i].src) { - if (!fw_table[i].size) { + /* Check APOB_NV requirements */ + if (fw_table[i].type == AMD_BIOS_APOB_NV) { + if (!fw_table[i].size && !fw_table[i].src) + continue; /* APOB_NV not used */ + if (fw_table[i].src && !fw_table[i].size) { printf("Error: APOB NV address provided, but no size\n"); free(ctx->rom); exit(1); } - } - /* APOB_NV needs a size, else no choice but to skip the item */ - if (fw_table[i].type == AMD_BIOS_APOB_NV && !fw_table[i].size) { - /* Attempt to determine whether this is an error */ + /* If the APOB isn't used, APOB_NV isn't used either */ apob_idx = find_bios_entry(AMD_BIOS_APOB); - if (apob_idx < 0 || !fw_table[apob_idx].dest) { - /* APOV NV not expected to be used */ - continue; - } else { - printf("Error: APOB NV must have a size\n"); - free(ctx->rom); - exit(1); - } + if (apob_idx < 0 || !fw_table[apob_idx].dest) + continue; /* APOV NV not supported */ }

/* APOB_DATA needs destination */

-- To view, visit https://review.coreboot.org/c/coreboot/+/44045 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibaeacd3dcdfd73f690df61c2a19d39bbb9dcc838 Gerrit-Change-Number: 44045 Gerrit-PatchSet: 4 Gerrit-Owner: Martin Roth martinroth@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Eric Peers epeers@google.com Gerrit-Reviewer: Felix Held felix-coreboot@felixheld.de Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: Rob Barnes robbarnes@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: merged

1549

days inactive

1571

days old

coreboot-gerrit@coreboot.org

7 comments

2 participants

tags (0)

participants (2)

Aaron Durbin (Code Review)
Martin Roth (Code Review)