Change in coreboot[master]: mb/google/dedede: Update the flash ROM layout for RW regions

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46569 )

Change subject: mb/google/dedede: Update the flash ROM layout for RW regions
......................................................................

mb/google/dedede: Update the flash ROM layout for RW regions

* Grab ~512 KiB from each FW_MAIN_A/B regions and allocate them to
  RW_LEGACY region so that it grows to 1 MiB.
* Remove VBLOCK_DEV region which is not used.
* Re-size the ELOG region to 4 KiB since that is the maximum size of the
  ELOG mirror buffer.
* Resize RW_NVRAM, VBLOCK_A/B regions to 8 KiB since no more than that size
  is used in those regions.
* Resize SHARED_DATA region to 4 KiB since no more than that size is
  used in that region.
* Based on the resizing, allocate each FW_MAIN_A/B regions with 72 KiB.

BUG=b:167943992, b:167498108
TEST=Build and boot to OS in Drawlat. Ensure that the firmware test
setup and flash map test are successful. Ensure that the event logs are
synced properly between reboots. Ensure that the suspend/resume sequence
is working fine. Ensure that the ChromeOS firmware update completes
successfully for the boot image with updated flash map and the system boots
fine after the update.

Change-Id: I53ada5ac3bd73bea50f4dd4dd352556f1eda7838
Signed-off-by: Karthikeyan Ramasubramanian 
---
M src/mainboard/google/dedede/chromeos-dedede-16MiB.fmd
1 file changed, 15 insertions(+), 16 deletions(-)

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

diff --git a/src/mainboard/google/dedede/chromeos-dedede-16MiB.fmd b/src/mainboard/google/dedede/chromeos-dedede-16MiB.fmd
index 09b2abc..0e21c4c 100644
--- a/src/mainboard/google/dedede/chromeos-dedede-16MiB.fmd
+++ b/src/mainboard/google/dedede/chromeos-dedede-16MiB.fmd
@@ -4,29 +4,28 @@
    	SI_ME@0x1000 0x380000
    }
    SI_BIOS@0x381000 0xc7f000 {
-		RW_LEGACY(CBFS)@0x0 0x1000
-		RW_SECTION_A@0x1000 0x420000 {
-			VBLOCK_A@0x0 0x10000
-			FW_MAIN_A(CBFS)@0x10000 0x40ffc0
-			RW_FWID_A@0x41ffc0 0x40
+		RW_LEGACY(CBFS)@0x0 0x100000
+		RW_SECTION_A@0x100000 0x3a4800 {
+			VBLOCK_A@0x0 0x2000
+			FW_MAIN_A(CBFS)@0x2000 0x3a27c0
+			RW_FWID_A@0x3a47c0 0x40
    	}
-		RW_SECTION_B@0x421000 0x420000 {
-			VBLOCK_B@0x0 0x10000
-			FW_MAIN_B(CBFS)@0x10000 0x40ffc0
-			RW_FWID_B@0x41ffc0 0x40
+		RW_SECTION_B@0x4a4800 0x3a4800 {
+			VBLOCK_B@0x0 0x2000
+			FW_MAIN_B(CBFS)@0x2000 0x3a27c0
+			RW_FWID_B@0x3a47c0 0x40
    	}
-		RW_MISC@0x841000 0x3e000 {
+		RW_MISC@0x849000 0x36000 {
    		UNIFIED_MRC_CACHE(PRESERVE)@0x0 0x30000 {
    			RECOVERY_MRC_CACHE@0x0 0x10000
    			RW_MRC_CACHE@0x10000 0x20000
    		}
-			RW_ELOG(PRESERVE)@0x30000 0x3000
-			RW_SHARED@0x33000 0x4000 {
-				SHARED_DATA@0x0 0x2000
-				VBLOCK_DEV@0x2000 0x2000
+			RW_ELOG(PRESERVE)@0x30000 0x1000
+			RW_SHARED@0x31000 0x1000 {
+				SHARED_DATA@0x0 0x1000
    		}
-			RW_VPD(PRESERVE)@0x37000 0x2000
-			RW_NVRAM(PRESERVE)@0x39000 0x5000
+			RW_VPD(PRESERVE)@0x32000 0x2000
+			RW_NVRAM(PRESERVE)@0x34000 0x2000
    	}
    	# Make WP_RO region align with SPI vendor
    	# memory protected range specification.

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46569 )

Change subject: mb/google/dedede: Update the flash ROM layout for RW regions
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46569/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46569/1//COMMIT_MSG@26 
PS1, Line 26: fine after the update.
I would like to point out that after this change, each FW_MAIN_A/B have ~1 MiB of space left.

[Furquan Shaikh (Code Review)]

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

Change subject: mb/google/dedede: Update the flash ROM layout for RW regions
......................................................................

Patch Set 1: Code-Review+2

[Furquan Shaikh (Code Review)]

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

Change subject: mb/google/dedede: Update the flash ROM layout for RW regions
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46569/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46569/1//COMMIT_MSG@8 
PS1, Line 8: 
It would be good to capture these as documentation in the form of guidelines on how to best allocate space for different FMAP regions and the reason behind the recommendation. I think that will be very helpful for future projects too.

[Paul Menzel (Code Review)]

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

Change subject: mb/google/dedede: Update the flash ROM layout for RW regions
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46569/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46569/1//COMMIT_MSG@8 
PS1, Line 8: 
Please describe the problems the layout update is addressing.

[Karthik Ramasubramanian (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Justin TerAvest, Tim Wawrzynczak,

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

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

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

Change subject: mb/google/dedede: Update the flash ROM layout for RW regions
......................................................................

mb/google/dedede: Update the flash ROM layout for RW regions

RW_LEGACY region needs to be 1 MiB to accommodate any alternate
firmware. Hence update the flash ROM layout as below:
* Grab ~512 KiB from each FW_MAIN_A/B regions and allocate them to
  RW_LEGACY region so that it grows to 1 MiB.
* Remove VBLOCK_DEV region which is not used.
* Re-size the ELOG region to 4 KiB since that is the maximum size of the
  ELOG mirror buffer.
* Resize RW_NVRAM, VBLOCK_A/B regions to 8 KiB since no more than that size
  is used in those regions.
* Resize SHARED_DATA region to 4 KiB since no more than that size is
  used in that region.
* Based on the resizing, allocate each FW_MAIN_A/B regions with 72 KiB.

BUG=b:167943992, b:167498108
TEST=Build and boot to OS in Drawlat. Ensure that the firmware test
setup and flash map test are successful. Ensure that the event logs are
synced properly between reboots. Ensure that the suspend/resume sequence
is working fine. Ensure that the ChromeOS firmware update completes
successfully for the boot image with updated flash map and the system boots
fine after the update.

Change-Id: I53ada5ac3bd73bea50f4dd4dd352556f1eda7838
Signed-off-by: Karthikeyan Ramasubramanian 
---
M src/mainboard/google/dedede/chromeos-dedede-16MiB.fmd
1 file changed, 15 insertions(+), 16 deletions(-)

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

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46569 )

Change subject: mb/google/dedede: Update the flash ROM layout for RW regions
......................................................................

Patch Set 2:

(3 comments)

https://review.coreboot.org/c/coreboot/+/46569/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46569/1//COMMIT_MSG@8 
PS1, Line 8:
> Please describe the problems the layout update is addressing.

Done

https://review.coreboot.org/c/coreboot/+/46569/1//COMMIT_MSG@8 
PS1, Line 8:
> It would be good to capture these as documentation in the form of guidelines on how to best allocate […]

Ack. I will submit a follow-up CL with a document containing the guidelines. I will limit the scope of the document to ChromeOS. I am envisioning a master document with additional pointers to SoC specific documents.

https://review.coreboot.org/c/coreboot/+/46569/1//COMMIT_MSG@26 
PS1, Line 26: fine after the update.
> I would like to point out that after this change, each FW_MAIN_A/B have ~1 MiB of space left.

Ack

[Furquan Shaikh (Code Review)]

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

Change subject: mb/google/dedede: Update the flash ROM layout for RW regions
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46569/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46569/1//COMMIT_MSG@8 
PS1, Line 8:
> Ack. I will submit a follow-up CL with a document containing the guidelines. […]

Sounds good. Thanks Karthik!

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46569 )

Change subject: mb/google/dedede: Update the flash ROM layout for RW regions
......................................................................

Patch Set 2:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/46569/2//COMMIT_MSG@27 
PS2, Line 27: successfully for the boot image with updated flash map and the system boots
72 chars wide

https://review.coreboot.org/c/coreboot/+/46569/2/src/mainboard/google/dedede... 
File src/mainboard/google/dedede/chromeos-dedede-16MiB.fmd:

https://review.coreboot.org/c/coreboot/+/46569/2/src/mainboard/google/dedede... 
PS2, Line 26: VBLOCK_DEV@0x2000 0x2000
So removing this removes the ability to use keys in developer mode if the right flag is set in the FWMP. Not sure if that is still used anymore?

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46569 )

Change subject: mb/google/dedede: Update the flash ROM layout for RW regions
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46569/2/src/mainboard/google/dedede... 
File src/mainboard/google/dedede/chromeos-dedede-16MiB.fmd:

https://review.coreboot.org/c/coreboot/+/46569/2/src/mainboard/google/dedede... 
PS2, Line 26: VBLOCK_DEV@0x2000 0x2000
> So removing this removes the ability to use keys in developer mode if the right flag is set in the F […]

Are you referring to the kernel key? If so, it seems to be stored at the start of the kernel partition?

Even when the USE_DEV_KEY_HASH flag is set in FWMP, the hash does not seem to come from VBLOCK_DEV. Adding jwerner@ to provide more context here.

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46569 )

Change subject: mb/google/dedede: Update the flash ROM layout for RW regions
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/46569/2/src/mainboard/google/dedede... 
File src/mainboard/google/dedede/chromeos-dedede-16MiB.fmd:

https://review.coreboot.org/c/coreboot/+/46569/2/src/mainboard/google/dedede... 
PS2, Line 26: VBLOCK_DEV@0x2000 0x2000
> Are you referring to the kernel key? If so, it seems to be stored at the start of the kernel partiti […]

Yes, what Karthik said. I think this was added here before the developer hash feature was really scoped, or for a potential future expansion that never happened and nobody is still planning to do atm. The only developer hash feature we currently have compares a hash in the FWMP directly with the keyblock loaded from the kernel partition, see here: https://chromium.googlesource.com/chromiumos/platform/vboot_reference/+/refs...

There has never been a way for vboot to access this FMAP section.

[Karthik Ramasubramanian (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Justin TerAvest, Tim Wawrzynczak, Julius Werner,

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

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

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

Change subject: mb/google/dedede: Update the flash ROM layout for RW regions
......................................................................

mb/google/dedede: Update the flash ROM layout for RW regions

RW_LEGACY region needs to be 1 MiB to accommodate any alternate
firmware. Hence update the flash ROM layout as below:
* Grab ~512 KiB from each FW_MAIN_A/B regions and allocate them to
  RW_LEGACY region so that it grows to 1 MiB.
* Remove VBLOCK_DEV region which is not used.
* Re-size the ELOG region to 4 KiB since that is the maximum size of the
  ELOG mirror buffer.
* Resize RW_NVRAM, VBLOCK_A/B regions to 8 KiB since no more than that
  size is used in those regions.
* Resize SHARED_DATA region to 4 KiB since no more than that size is
  used in that region.
* Based on the resizing, allocate each FW_MAIN_A/B regions with 72 KiB.

BUG=b:167943992, b:167498108
TEST=Build and boot to OS in Drawlat. Ensure that the firmware test
setup and flash map test are successful. Ensure that the event logs are
synced properly between reboots. Ensure that the suspend/resume sequence
is working fine. Ensure that the ChromeOS firmware update completes
successfully for the boot image with updated flash map and the system
boots fine after the update.

Change-Id: I53ada5ac3bd73bea50f4dd4dd352556f1eda7838
Signed-off-by: Karthikeyan Ramasubramanian 
---
M src/mainboard/google/dedede/chromeos-dedede-16MiB.fmd
1 file changed, 15 insertions(+), 16 deletions(-)

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

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46569 )

Change subject: mb/google/dedede: Update the flash ROM layout for RW regions
......................................................................

Patch Set 3:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/46569/2//COMMIT_MSG@27 
PS2, Line 27: successfully for the boot image with updated flash map and the system boots
> 72 chars wide

Done

[Paul Menzel (Code Review)]

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

Change subject: mb/google/dedede: Update the flash ROM layout for RW regions
......................................................................

Patch Set 3: Code-Review+1

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46569 )

Change subject: mb/google/dedede: Update the flash ROM layout for RW regions
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46569/2/src/mainboard/google/dedede... 
File src/mainboard/google/dedede/chromeos-dedede-16MiB.fmd:

https://review.coreboot.org/c/coreboot/+/46569/2/src/mainboard/google/dedede... 
PS2, Line 26: VBLOCK_DEV@0x2000 0x2000
> Yes, what Karthik said. […]

Gotcha, thanks for the info

[Maulik V Vaghela (Code Review)]

Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46569 )

Change subject: mb/google/dedede: Update the flash ROM layout for RW regions
......................................................................

Patch Set 3: Code-Review+2

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46569 )

Change subject: mb/google/dedede: Update the flash ROM layout for RW regions
......................................................................

mb/google/dedede: Update the flash ROM layout for RW regions

RW_LEGACY region needs to be 1 MiB to accommodate any alternate
firmware. Hence update the flash ROM layout as below:
* Grab ~512 KiB from each FW_MAIN_A/B regions and allocate them to
  RW_LEGACY region so that it grows to 1 MiB.
* Remove VBLOCK_DEV region which is not used.
* Re-size the ELOG region to 4 KiB since that is the maximum size of the
  ELOG mirror buffer.
* Resize RW_NVRAM, VBLOCK_A/B regions to 8 KiB since no more than that
  size is used in those regions.
* Resize SHARED_DATA region to 4 KiB since no more than that size is
  used in that region.
* Based on the resizing, allocate each FW_MAIN_A/B regions with 72 KiB.

BUG=b:167943992, b:167498108
TEST=Build and boot to OS in Drawlat. Ensure that the firmware test
setup and flash map test are successful. Ensure that the event logs are
synced properly between reboots. Ensure that the suspend/resume sequence
is working fine. Ensure that the ChromeOS firmware update completes
successfully for the boot image with updated flash map and the system
boots fine after the update.

Change-Id: I53ada5ac3bd73bea50f4dd4dd352556f1eda7838
Signed-off-by: Karthikeyan Ramasubramanian 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/46569
Reviewed-by: Paul Menzel 
Reviewed-by: Maulik V Vaghela 
Reviewed-by: Julius Werner 
Reviewed-by: Furquan Shaikh 
Tested-by: build bot (Jenkins) 
---
M src/mainboard/google/dedede/chromeos-dedede-16MiB.fmd
1 file changed, 15 insertions(+), 16 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Paul Menzel: Looks good to me, but someone else must approve
  Furquan Shaikh: Looks good to me, approved
  Julius Werner: Looks good to me, but someone else must approve
  Maulik V Vaghela: Looks good to me, approved

diff --git a/src/mainboard/google/dedede/chromeos-dedede-16MiB.fmd b/src/mainboard/google/dedede/chromeos-dedede-16MiB.fmd
index 09b2abc..0e21c4c 100644
--- a/src/mainboard/google/dedede/chromeos-dedede-16MiB.fmd
+++ b/src/mainboard/google/dedede/chromeos-dedede-16MiB.fmd
@@ -4,29 +4,28 @@
    	SI_ME@0x1000 0x380000
    }
    SI_BIOS@0x381000 0xc7f000 {
-		RW_LEGACY(CBFS)@0x0 0x1000
-		RW_SECTION_A@0x1000 0x420000 {
-			VBLOCK_A@0x0 0x10000
-			FW_MAIN_A(CBFS)@0x10000 0x40ffc0
-			RW_FWID_A@0x41ffc0 0x40
+		RW_LEGACY(CBFS)@0x0 0x100000
+		RW_SECTION_A@0x100000 0x3a4800 {
+			VBLOCK_A@0x0 0x2000
+			FW_MAIN_A(CBFS)@0x2000 0x3a27c0
+			RW_FWID_A@0x3a47c0 0x40
    	}
-		RW_SECTION_B@0x421000 0x420000 {
-			VBLOCK_B@0x0 0x10000
-			FW_MAIN_B(CBFS)@0x10000 0x40ffc0
-			RW_FWID_B@0x41ffc0 0x40
+		RW_SECTION_B@0x4a4800 0x3a4800 {
+			VBLOCK_B@0x0 0x2000
+			FW_MAIN_B(CBFS)@0x2000 0x3a27c0
+			RW_FWID_B@0x3a47c0 0x40
    	}
-		RW_MISC@0x841000 0x3e000 {
+		RW_MISC@0x849000 0x36000 {
    		UNIFIED_MRC_CACHE(PRESERVE)@0x0 0x30000 {
    			RECOVERY_MRC_CACHE@0x0 0x10000
    			RW_MRC_CACHE@0x10000 0x20000
    		}
-			RW_ELOG(PRESERVE)@0x30000 0x3000
-			RW_SHARED@0x33000 0x4000 {
-				SHARED_DATA@0x0 0x2000
-				VBLOCK_DEV@0x2000 0x2000
+			RW_ELOG(PRESERVE)@0x30000 0x1000
+			RW_SHARED@0x31000 0x1000 {
+				SHARED_DATA@0x0 0x1000
    		}
-			RW_VPD(PRESERVE)@0x37000 0x2000
-			RW_NVRAM(PRESERVE)@0x39000 0x5000
+			RW_VPD(PRESERVE)@0x32000 0x2000
+			RW_NVRAM(PRESERVE)@0x34000 0x2000
    	}
    	# Make WP_RO region align with SPI vendor
    	# memory protected range specification.