Change in coreboot[master]: nb/intel/sandybridge: Read spds only once if measured boot is enabled

[Bill XIE (Code Review)]

Bill XIE has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39969 )

Change subject: nb/intel/sandybridge: Read spds only once if measured boot is enabled
......................................................................

nb/intel/sandybridge: Read spds only once if measured boot is enabled

Without considering s3 resume, spd may be used various times depending
on various condition. If spd is stored in CBFS and read various times,
PCR value may become inconsistent.

As mentioned in CB:39906, in order to avoid this, we could read spd
exactly once, and use the data read out various times, when measured
boot is enabled.

Change-Id: I02cad7e85d5e66fd9efb674e4dc9868233f6c233
Signed-off-by: Bill XIE 
---
M src/northbridge/intel/sandybridge/raminit.c
1 file changed, 27 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/39969/1

diff --git a/src/northbridge/intel/sandybridge/raminit.c b/src/northbridge/intel/sandybridge/raminit.c
index b096a11..f3c81a6 100644
--- a/src/northbridge/intel/sandybridge/raminit.c
+++ b/src/northbridge/intel/sandybridge/raminit.c
@@ -98,6 +98,17 @@
    }
 }

+static bool spd_is_available(const spd_raw_data spds[], size_t num_spd)
+{
+	/* An available spd should at least have an non-zero id */
+	size_t i, j, sum = 0;
+	for (i = 0; i < num_spd; i++) {
+		for (j = 117; j < 128; j++)
+			sum += spds[i][j];
+	}
+	return (sum > 0);
+}
+
 static void dram_find_spds_ddr3(spd_raw_data *spd, ramctr_timing *ctrl)
 {
    int dimms = 0, ch_dimms;
@@ -222,6 +233,8 @@
    struct region_device rdev;
    ramctr_timing *ctrl_cached = NULL;

+	if (CONFIG(TPM_MEASURED_BOOT))
+		memset(spds, 0, sizeof(spds));
    MCHBAR32(SAPMCTL) |= 1;

/* Wait for ME to be ready */
@@ -271,8 +284,14 @@
    /* Verify MRC cache for fast boot */
    if (!s3resume && ctrl_cached) {
    	/* Load SPD unique information data. */
-		memset(spds, 0, sizeof(spds));
-		mainboard_get_spd(spds, 1);
+		if (CONFIG(TPM_MEASURED_BOOT)) {
+			/* if CONFIG(TPM_MEASURED_BOOT),
+			   we manage to get spds only ONCE */
+			mainboard_get_spd(spds, 0);
+		} else {
+			memset(spds, 0, sizeof(spds));
+			mainboard_get_spd(spds, 1);
+		}

/* check SPD CRC16 to make sure the DIMMs haven't been replaced */
    	fast_boot = verify_crc16_spds_ddr3(spds, ctrl_cached);
@@ -307,8 +326,12 @@
    	ctrl.cpu = cpuid;

/* Get DDR3 SPD data */
-		memset(spds, 0, sizeof(spds));
-		mainboard_get_spd(spds, 0);
+		if (!CONFIG(TPM_MEASURED_BOOT) || !spd_is_available(spds, 4)) {
+			/* without CONFIG(TPM_MEASURED_BOOT), the previous read may
+			   only contains id, so read it again */
+			memset(spds, 0, sizeof(spds));
+			mainboard_get_spd(spds, 0);
+		}
    	dram_find_spds_ddr3(spds, &ctrl);

err = try_init_dram_ddr3(&ctrl, fast_boot, s3resume, me_uma_size);

[Bill XIE (Code Review)]

Hello build bot (Jenkins), Patrick Rudolph,

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

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

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

Change subject: nb/intel/sandybridge: Read spds only once if measured boot is enabled
......................................................................

nb/intel/sandybridge: Read spds only once if measured boot is enabled

Without considering s3 resume, spd may be used various times depending
on various condition. If spd is stored in CBFS and read various times,
PCR value may become inconsistent.

As mentioned in CB:39906, in order to avoid this, we could read spd
exactly once, and use the data read out various times, when measured
boot is enabled.

Change-Id: I02cad7e85d5e66fd9efb674e4dc9868233f6c233
Signed-off-by: Bill XIE 
---
M src/northbridge/intel/sandybridge/raminit.c
1 file changed, 29 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/39969/2

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39969 )

Change subject: nb/intel/sandybridge: Read spds only once if measured boot is enabled
......................................................................

Patch Set 2:

(9 comments)

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

https://review.coreboot.org/c/coreboot/+/39969/2//COMMIT_MSG@7 
PS2, Line 7: spds
SPDs

https://review.coreboot.org/c/coreboot/+/39969/2//COMMIT_MSG@9 
PS2, Line 9: spd
SPD

https://review.coreboot.org/c/coreboot/+/39969/2//COMMIT_MSG@10 
PS2, Line 10: spd
SPD

https://review.coreboot.org/c/coreboot/+/39969/2//COMMIT_MSG@10 
PS2, Line 10: condition
conditions

https://review.coreboot.org/c/coreboot/+/39969/2//COMMIT_MSG@13 
PS2, Line 13: spd
SPD

https://review.coreboot.org/c/coreboot/+/39969/2//COMMIT_MSG@13 
PS2, Line 13: CB:39906
Do you mean the commit or review comments? Please add the URL for people using `git log`.

https://review.coreboot.org/c/coreboot/+/39969/2/src/northbridge/intel/sandy... 
File src/northbridge/intel/sandybridge/raminit.c:

https://review.coreboot.org/c/coreboot/+/39969/2/src/northbridge/intel/sandy... 
PS2, Line 103: spd
SPD

https://review.coreboot.org/c/coreboot/+/39969/2/src/northbridge/intel/sandy... 
PS2, Line 103: an
a

https://review.coreboot.org/c/coreboot/+/39969/2/src/northbridge/intel/sandy... 
PS2, Line 107: 			sum += spds[i][j];
Can’t you just return, if one element is greater 0, that means the first time you find that element?

Change in coreboot[master]: nb/intel/sandybridge: Read SPDs only once if measured boot is enabled

[Bill XIE (Code Review)]

Hello build bot (Jenkins), Patrick Rudolph,

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

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

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

Change subject: nb/intel/sandybridge: Read SPDs only once if measured boot is enabled
......................................................................

nb/intel/sandybridge: Read SPDs only once if measured boot is enabled

Without considering s3 resume, SPD may be used various times depending
on various conditions. If SPD is stored in CBFS and read various times,
PCR value may become inconsistent.

As mentioned in https://review.coreboot.org/c/coreboot/+/39906/ , in
order to avoid this, we could read SPD exactly once, and use the data
read out various times, when measured boot is enabled.

Change-Id: I02cad7e85d5e66fd9efb674e4dc9868233f6c233
Signed-off-by: Bill XIE 
---
M src/northbridge/intel/sandybridge/raminit.c
1 file changed, 30 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/39969/3

Change in coreboot[master]: nb/intel/sandybridge: Read SPDs only once if measured boot is enabled

[Bill XIE (Code Review)]

Bill XIE has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39969 )

Change subject: nb/intel/sandybridge: Read SPDs only once if measured boot is enabled
......................................................................

Patch Set 3:

(7 comments)

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

https://review.coreboot.org/c/coreboot/+/39969/2//COMMIT_MSG@7 
PS2, Line 7: spds
> SPDs

Done

https://review.coreboot.org/c/coreboot/+/39969/2//COMMIT_MSG@9 
PS2, Line 9: spd
> SPD

Done

https://review.coreboot.org/c/coreboot/+/39969/2//COMMIT_MSG@10 
PS2, Line 10: condition
> conditions

Done

https://review.coreboot.org/c/coreboot/+/39969/2//COMMIT_MSG@10 
PS2, Line 10: spd
> SPD

Done

https://review.coreboot.org/c/coreboot/+/39969/2//COMMIT_MSG@13 
PS2, Line 13: spd
> SPD

Done

https://review.coreboot.org/c/coreboot/+/39969/2//COMMIT_MSG@13 
PS2, Line 13: CB:39906
> Do you mean the commit or review comments? Please add the URL for people using `git log`.

Done

https://review.coreboot.org/c/coreboot/+/39969/2/src/northbridge/intel/sandy... 
File src/northbridge/intel/sandybridge/raminit.c:

https://review.coreboot.org/c/coreboot/+/39969/2/src/northbridge/intel/sandy... 
PS2, Line 107: 			sum += spds[i][j];
> Can’t you just return, if one element is greater 0, that means the first time you find that element?

Done

Change in coreboot[master]: nb/intel/sandybridge: Read SPDs only once if measured boot is enabled

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39969 )

Change subject: nb/intel/sandybridge: Read SPDs only once if measured boot is enabled
......................................................................

Patch Set 4: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/39969/4/src/northbridge/intel/sandy... 
File src/northbridge/intel/sandybridge/raminit.c:

https://review.coreboot.org/c/coreboot/+/39969/4/src/northbridge/intel/sandy... 
PS4, Line 318: 			   we manage to get spds only ONCE */
1.  SPDs
2.  Fits on one line?

Change in coreboot[master]: nb/intel/sandybridge: Read SPDs only once if measured boot is enabled

[Bill XIE (Code Review)]

Hello build bot (Jenkins), Paul Menzel, Patrick Rudolph,

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

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

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

Change subject: nb/intel/sandybridge: Read SPDs only once if measured boot is enabled
......................................................................

nb/intel/sandybridge: Read SPDs only once if measured boot is enabled

Without considering s3 resume, SPD may be used various times depending
on various conditions. If SPD is stored in CBFS and read various times,
PCR value may become inconsistent.

As mentioned in https://review.coreboot.org/c/coreboot/+/39906/ , in
order to avoid this, we could read SPD exactly once, and use the data
read out various times, when measured boot is enabled.

Change-Id: I02cad7e85d5e66fd9efb674e4dc9868233f6c233
Signed-off-by: Bill XIE 
---
M src/northbridge/intel/sandybridge/raminit.c
1 file changed, 29 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/39969/5

Change in coreboot[master]: nb/intel/sandybridge: Read SPDs only once if measured boot is enabled

[Bill XIE (Code Review)]

Bill XIE has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39969 )

Change subject: nb/intel/sandybridge: Read SPDs only once if measured boot is enabled
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39969/4/src/northbridge/intel/sandy... 
File src/northbridge/intel/sandybridge/raminit.c:

https://review.coreboot.org/c/coreboot/+/39969/4/src/northbridge/intel/sandy... 
PS4, Line 318: 			   we manage to get spds only ONCE */
> SPDs […]

Done

Change in coreboot[master]: nb/intel/sandybridge: Read SPDs only once if measured boot is enabled

[Patrick Rudolph (Code Review)]

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

Change subject: nb/intel/sandybridge: Read SPDs only once if measured boot is enabled
......................................................................

Patch Set 6:

(1 comment)

Looking at the code I cannot easily understand what you are trying to achieve.

https://review.coreboot.org/c/coreboot/+/39969/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/39969/6//COMMIT_MSG@10 
PS6, Line 10: CBFS
What do you measure? SPD read over smbus? MRC cache contents?
How do you handle first boot ( with no mrc cache) vs following boots?

Change in coreboot[master]: nb/intel/sandybridge: Read SPDs only once if measured boot is enabled

[Bill XIE (Code Review)]

Bill XIE has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39969 )

Change subject: nb/intel/sandybridge: Read SPDs only once if measured boot is enabled
......................................................................

Patch Set 6:

(1 comment)
> Patch Set 6:
>
> (1 comment)
>
> Looking at the code I cannot easily understand what you are trying to achieve.

https://review.coreboot.org/c/coreboot/+/39969/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/39969/6//COMMIT_MSG@10 
PS6, Line 10: CBFS
> What do you measure?

The SPD file stored in CBFS for memory chips soldered on the mainboard. If present, it will be loaded with mainboard_get_spd(), along with SPD for DIMMs read over smbus.
> How do you handle first boot ( with no mrc cache) vs following boots?

The SPD file does not changes as MRC cache does, so if it is "loaded once while used many time", measurement for it will not change.

Change in coreboot[master]: nb/intel/sandybridge: Read SPDs only once if measured boot is enabled

[Bill XIE (Code Review)]

Hello build bot (Jenkins), Paul Menzel, Patrick Rudolph,

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

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

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

Change subject: nb/intel/sandybridge: Read SPDs only once if measured boot is enabled
......................................................................

nb/intel/sandybridge: Read SPDs only once if measured boot is enabled

Without considering s3 resume, SPD may be used various times depending
on various conditions. If there is an SPD file (for memory chips
soldered on the mainboard) stored in CBFS and loaded various times,
PCR value may become inconsistent.

As mentioned in https://review.coreboot.org/c/coreboot/+/39906/ , in
order to avoid this, we could read SPD exactly once, and use the data
read out various times, when measured boot is enabled.

Change-Id: I02cad7e85d5e66fd9efb674e4dc9868233f6c233
Signed-off-by: Bill XIE 
---
M src/northbridge/intel/sandybridge/raminit.c
1 file changed, 29 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/39969/7

Change in coreboot[master]: nb/intel/sandybridge: Read SPDs only once if measured boot is enabled

[Bill XIE (Code Review)]

Bill XIE has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39969 )

Change subject: nb/intel/sandybridge: Read SPDs only once if measured boot is enabled
......................................................................

Patch Set 7:

(2 comments)
> Patch Set 6:
>
> (1 comment)
>
> Looking at the code I cannot easily understand what you are trying to achieve.

https://review.coreboot.org/c/coreboot/+/39969/2/src/northbridge/intel/sandy... 
File src/northbridge/intel/sandybridge/raminit.c:

https://review.coreboot.org/c/coreboot/+/39969/2/src/northbridge/intel/sandy... 
PS2, Line 103: an
> a

Done

https://review.coreboot.org/c/coreboot/+/39969/2/src/northbridge/intel/sandy... 
PS2, Line 103: spd
> SPD

Done

[S] Change in coreboot[master]: nb/intel/sandybridge: Read SPDs only once if measured boot is enabled

[Stefan Reinauer (Code Review)]

Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/39969?usp=email )

Change subject: nb/intel/sandybridge: Read SPDs only once if measured boot is enabled
......................................................................

Abandoned