Change in coreboot[master]: util/apcb: Strip SPD manufacturer information

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

Rob Barnes (Code Review)

24 Jul 2020 24 Jul '20

11:34 p.m.

Rob Barnes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43832 )

Change subject: util/apcb: Strip SPD manufacturer information ......................................................................

util/apcb: Strip SPD manufacturer information

Manufacturer information from SPDs before injecting into APCB. This allows more flexibilty around changing DRAM modules in the future.

BUG=b:162098961 TEST=Boot, dump memory info

Signed-off-by: Rob Barnes robbarnes@google.com Change-Id: I1bbc81a858f381f62dbd38bb57b3df0e6707d647 --- M src/soc/amd/picasso/Makefile.inc M util/apcb/apcb_edit.py 2 files changed, 13 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/43832/1

diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc index b3af1fd..6db130e 100644 --- a/src/soc/amd/picasso/Makefile.inc +++ b/src/soc/amd/picasso/Makefile.inc @@ -395,6 +395,7 @@ $(APCB_MAGIC_BLOB) \ $@ \ --hex \ + --strip_manufacturer_information \ --spd_0_0 $< \ --board_id_gpio0 $(APCB_BOARD_ID_GPIO0) \ --board_id_gpio1 $(APCB_BOARD_ID_GPIO1) \ @@ -408,6 +409,7 @@ $(APCB_MAGIC_BLOB) \ $@ \ --hex \ + --strip_manufacturer_information \ --spd_0_0 $< \ --spd_1_0 $< \ --board_id_gpio0 $(APCB_BOARD_ID_GPIO0) \ diff --git a/util/apcb/apcb_edit.py b/util/apcb/apcb_edit.py index e5dd0cd..9d97cd8 100755 --- a/util/apcb/apcb_edit.py +++ b/util/apcb/apcb_edit.py @@ -59,6 +59,10 @@ action='store_true', help='SPD input file is hex encoded, binary otherwise') parser.add_argument( + '--strip_manufacturer_information', + action='store_true', + help='Strip all manufacturer information from SPD') + parser.add_argument( '--board_id_gpio0', type=int, required=True, @@ -155,6 +159,13 @@ assert len(spd) == 512, \ "Expected SPD to be 512 bytes, got %d" % len(spd)

+ if args.strip_manufacturer_information: + print("Stripping manufacturer infromation from SPD") + spd = spd[0:320] + b'\x00'*64 + spd[320+64:] + + assert len(spd) == 512, \ + "Error while stripping SPD manufacurer infromation" + print("Enabling channel %d, dimm %d and injecting SPD\n" % (spd_ssp.ChannelNumber, spd_ssp.DimmNumber)) spd_ssp = spd_ssp._replace(SpdValid=True, DimmPresent=True)

Attachments:

attachment.htm (text/html — 4.9 KB)

Show replies by date

Rob Barnes (Code Review)

24 Jul 24 Jul

11:44 p.m.

Hello Patrick Georgi, Martin Roth,

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

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

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

Change subject: util/apcb: Strip SPD manufacturer information ......................................................................

util/apcb: Strip SPD manufacturer information

Strip manufacturer information from SPDs before injecting into APCB. This allows more flexibility around changing DRAM modules in the future.

BUG=b:162098961 TEST=Boot, dump memory info

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/43832/2

build bot (Jenkins) (Code Review)

11:45 p.m.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43832 )

Change subject: util/apcb: Strip SPD manufacturer information ......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/43832/1/util/apcb/apcb_edit.py File util/apcb/apcb_edit.py:

https://review.coreboot.org/c/coreboot/+/43832/1/util/apcb/apcb_edit.py@163 PS1, Line 163: print("Stripping manufacturer infromation from SPD") 'infromation' may be misspelled - perhaps 'information'?

https://review.coreboot.org/c/coreboot/+/43832/1/util/apcb/apcb_edit.py@167 PS1, Line 167: "Error while stripping SPD manufacurer infromation" 'infromation' may be misspelled - perhaps 'information'?

-- To view, visit https://review.coreboot.org/c/coreboot/+/43832 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1bbc81a858f381f62dbd38bb57b3df0e6707d647 Gerrit-Change-Number: 43832 Gerrit-PatchSet: 1 Gerrit-Owner: Rob Barnes robbarnes@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-CC: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Fri, 24 Jul 2020 21:45:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Angel Pons (Code Review)

25 Jul 25 Jul

1:40 a.m.

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43832 )

Change subject: util/apcb: Strip SPD manufacturer information ......................................................................

Patch Set 2: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/43832 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1bbc81a858f381f62dbd38bb57b3df0e6707d647 Gerrit-Change-Number: 43832 Gerrit-PatchSet: 2 Gerrit-Owner: Rob Barnes robbarnes@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Fri, 24 Jul 2020 23:40:26 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Rob Barnes (Code Review)

2:25 a.m.

Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Angel Pons, Aaron Durbin,

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

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

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

Change subject: util/apcb: Strip SPD manufacturer information ......................................................................

util/apcb: Strip SPD manufacturer information

Strip manufacturer information from SPDs before injecting into APCB. This allows more flexibility around changing DRAM modules in the future.

BUG=b:162098961 TEST=Boot, dump memory info

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/43832/3

-- To view, visit https://review.coreboot.org/c/coreboot/+/43832 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1bbc81a858f381f62dbd38bb57b3df0e6707d647 Gerrit-Change-Number: 43832 Gerrit-PatchSet: 3 Gerrit-Owner: Rob Barnes robbarnes@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: newpatchset

Furquan Shaikh (Code Review)

2:26 a.m.

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43832 )

Change subject: util/apcb: Strip SPD manufacturer information ......................................................................

Patch Set 3: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/43832/3/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/43832/3/src/soc/amd/picasso/Makefil... PS3, Line 398: --strip_manufacturer_information This is okay for now, but probably we would want this to be a mainboard controlled option eventually?

-- To view, visit https://review.coreboot.org/c/coreboot/+/43832 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1bbc81a858f381f62dbd38bb57b3df0e6707d647 Gerrit-Change-Number: 43832 Gerrit-PatchSet: 3 Gerrit-Owner: Rob Barnes robbarnes@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Sat, 25 Jul 2020 00:26:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Rob Barnes (Code Review)

2:43 a.m.

Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43832 )

Change subject: util/apcb: Strip SPD manufacturer information ......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43832/3/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/43832/3/src/soc/amd/picasso/Makefil... PS3, Line 398: --strip_manufacturer_information

...

This is okay for now, but probably we would want this to be a mainboard controlled option eventually […]

I have a short shell script that I can substitute for this and call from the mainboard makefile. But that's more temporary code to maintain. I assume this won't be needed after integration with the gen_spd tool? https://source.chromium.org/chromiumos/chromiumos/codesearch/+/master:src/th...

-- To view, visit https://review.coreboot.org/c/coreboot/+/43832 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1bbc81a858f381f62dbd38bb57b3df0e6707d647 Gerrit-Change-Number: 43832 Gerrit-PatchSet: 3 Gerrit-Owner: Rob Barnes robbarnes@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Sat, 25 Jul 2020 00:43:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

Rob Barnes (Code Review)

27 Jul 27 Jul

5:29 p.m.

Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Angel Pons, Aaron Durbin,

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

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

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

Change subject: util/apcb: Strip SPD manufacturer information ......................................................................

util/apcb: Strip SPD manufacturer information

Strip manufacturer information from SPDs before injecting into APCB. This allows more flexibility around changing DRAM modules in the future.

BUG=b:162098961 TEST=Boot, dump memory info

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/43832/4

-- To view, visit https://review.coreboot.org/c/coreboot/+/43832 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1bbc81a858f381f62dbd38bb57b3df0e6707d647 Gerrit-Change-Number: 43832 Gerrit-PatchSet: 4 Gerrit-Owner: Rob Barnes robbarnes@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: newpatchset

Furquan Shaikh (Code Review)

28 Jul 28 Jul

5:31 a.m.

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43832 )

Change subject: util/apcb: Strip SPD manufacturer information ......................................................................

Patch Set 4: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/43832/3/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/43832/3/src/soc/amd/picasso/Makefil... PS3, Line 398: --strip_manufacturer_information

...

I have a short shell script that I can substitute for this and call from the mainboard makefile. […]

Makes sense. Let's go ahead with this until we move to the gen_spd tool.

-- To view, visit https://review.coreboot.org/c/coreboot/+/43832 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1bbc81a858f381f62dbd38bb57b3df0e6707d647 Gerrit-Change-Number: 43832 Gerrit-PatchSet: 4 Gerrit-Owner: Rob Barnes robbarnes@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Tue, 28 Jul 2020 03:31:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Furquan Shaikh furquan@google.com Comment-In-Reply-To: Rob Barnes robbarnes@google.com Gerrit-MessageType: comment

Patrick Georgi (Code Review)

29 Jul 29 Jul

11:35 a.m.

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

Change subject: util/apcb: Strip SPD manufacturer information ......................................................................

util/apcb: Strip SPD manufacturer information

Strip manufacturer information from SPDs before injecting into APCB. This allows more flexibility around changing DRAM modules in the future.

BUG=b:162098961 TEST=Boot, dump memory info

Signed-off-by: Rob Barnes robbarnes@google.com Change-Id: I1bbc81a858f381f62dbd38bb57b3df0e6707d647 Reviewed-on: https://review.coreboot.org/c/coreboot/+/43832 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/soc/amd/picasso/Makefile.inc M util/apcb/apcb_edit.py 2 files changed, 13 insertions(+), 0 deletions(-)

Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved

diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc index aa9d8c9..4c1d726 100644 --- a/src/soc/amd/picasso/Makefile.inc +++ b/src/soc/amd/picasso/Makefile.inc @@ -395,6 +395,7 @@ $(APCB_MAGIC_BLOB) \ $@ \ --hex \ + --strip_manufacturer_information \ --spd_0_0 $< \ --board_id_gpio0 $(APCB_BOARD_ID_GPIO0) \ --board_id_gpio1 $(APCB_BOARD_ID_GPIO1) \ @@ -408,6 +409,7 @@ $(APCB_MAGIC_BLOB) \ $@ \ --hex \ + --strip_manufacturer_information \ --spd_0_0 $< \ --spd_1_0 $< \ --board_id_gpio0 $(APCB_BOARD_ID_GPIO0) \ diff --git a/util/apcb/apcb_edit.py b/util/apcb/apcb_edit.py index 54d59d6..388b18a 100755 --- a/util/apcb/apcb_edit.py +++ b/util/apcb/apcb_edit.py @@ -59,6 +59,10 @@ action='store_true', help='SPD input file is hex encoded, binary otherwise') parser.add_argument( + '--strip_manufacturer_information', + action='store_true', + help='Strip all manufacturer information from SPD') + parser.add_argument( '--board_id_gpio0', type=int, required=True, @@ -155,6 +159,13 @@ assert len(spd) == 512, \ "Expected SPD to be 512 bytes, got %d" % len(spd)

+ if args.strip_manufacturer_information: + print("Stripping manufacturer information from SPD") + spd = spd[0:320] + b'\x00'*64 + spd[320+64:] + + assert len(spd) == 512, \ + "Error while stripping SPD manufacurer information" + print("Enabling channel %d, dimm %d and injecting SPD" % (spd_ssp.ChannelNumber, spd_ssp.DimmNumber)) spd_ssp = spd_ssp._replace(SpdValid=True, DimmPresent=True)

-- To view, visit https://review.coreboot.org/c/coreboot/+/43832 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1bbc81a858f381f62dbd38bb57b3df0e6707d647 Gerrit-Change-Number: 43832 Gerrit-PatchSet: 5 Gerrit-Owner: Rob Barnes robbarnes@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: merged

1561

days inactive

1566

days old

coreboot-gerrit@coreboot.org

9 comments

5 participants

tags (0)

participants (5)

Angel Pons (Code Review)
build bot (Jenkins) (Code Review)
Furquan Shaikh (Code Review)
Patrick Georgi (Code Review)
Rob Barnes (Code Review)