Change in coreboot[master]: cbfs: Stop checking master header

[Julius Werner (Code Review)]

Julius Werner has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36688 )

Change subject: cbfs: Stop checking master header
......................................................................

cbfs: Stop checking master header

The CBFS master header is a legacy structure that just conveys the same
information we already have from the FMAP these days. We're still
including it to support older CBFS implementations in some payloads, but
there's no need for coreboot itself to follow this indirection anymore.
This patch simplifies the default CBFS locator to just return the CBFS
offset and size from the FMAP directly.

Change-Id: I6b00dd7f276364d62fa1f637efbaee0e80607c49
Signed-off-by: Julius Werner 
---
M src/lib/cbfs.c
M src/soc/intel/apollolake/mmap_boot.c
M src/vendorcode/eltan/security/verified_boot/vboot_check.c
3 files changed, 14 insertions(+), 42 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/36688/1

diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c
index 51c3772..f4c0100 100644
--- a/src/lib/cbfs.c
+++ b/src/lib/cbfs.c
@@ -280,44 +280,16 @@
    return 0;
 }

-/* This only supports the "COREBOOT" fmap region. */
-static int cbfs_master_header_props(struct cbfs_props *props)
+/* The default locator find the CBFS in the "COREBOOT" FMAP region. */
+static int cbfs_default_props(struct cbfs_props *props)
 {
-	struct cbfs_header header;
-	const struct region_device *bdev;
-	int32_t rel_offset;
-	size_t offset;
+	struct region region;

-	bdev = boot_device_ro();
-
-	if (bdev == NULL)
+	if (fmap_locate_area("COREBOOT", &region))
    	return -1;

-	struct region fmap_region;
-	if (fmap_locate_area("COREBOOT", &fmap_region))
-		return -1;
-	size_t fmap_top = region_end(&fmap_region);
-
-	/* Find location of header using signed 32-bit offset from
-	 * end of CBFS region. */
-	offset = fmap_top - sizeof(int32_t);
-	if (rdev_readat(bdev, &rel_offset, offset, sizeof(int32_t)) < 0)
-		return -1;
-
-	offset = fmap_top + rel_offset;
-	if (rdev_readat(bdev, &header, offset, sizeof(header)) < 0)
-		return -1;
-
-	header.magic = ntohl(header.magic);
-	header.romsize = ntohl(header.romsize);
-	header.offset = ntohl(header.offset);
-
-	if (header.magic != CBFS_HEADER_MAGIC)
-		return -1;
-
-	props->offset = header.offset;
-	props->size = header.romsize;
-	props->size -= props->offset;
+	props->offset = region_offset(&region);
+	props->size = region_sz(&region);

printk(BIOS_SPEW, "CBFS @ %zx size %zx\n", props->offset, props->size);

@@ -327,9 +299,9 @@
 /* This struct is marked as weak to allow a particular platform to
  * override the master header logic. This implementation should work for most
  * devices. */
-const struct cbfs_locator __weak cbfs_master_header_locator = {
-	.name = "Master Header Locator",
-	.locate = cbfs_master_header_props,
+const struct cbfs_locator __weak cbfs_default_locator = {
+	.name = "COREBOOT Locator",
+	.locate = cbfs_default_props,
 };

extern const struct cbfs_locator vboot_locator;
@@ -343,7 +315,7 @@
     */
    &vboot_locator,
 #endif
-	&cbfs_master_header_locator,
+	&cbfs_default_locator,
 };

int cbfs_boot_region_properties(struct cbfs_props *props)
diff --git a/src/soc/intel/apollolake/mmap_boot.c b/src/soc/intel/apollolake/mmap_boot.c
index 614b603..631a834 100644
--- a/src/soc/intel/apollolake/mmap_boot.c
+++ b/src/soc/intel/apollolake/mmap_boot.c
@@ -128,10 +128,10 @@
 }

/*
- * Named cbfs_master_header_locator so that it overrides the default, but
- * incompatible locator in cbfs.c
+ * Named cbfs_default_locator so that it overrides the default, but incompatible
+ * locator in cbfs.c
  */
-const struct cbfs_locator cbfs_master_header_locator = {
+const struct cbfs_locator cbfs_default_locator = {
    .name = "IAFW Locator",
    .locate = iafw_boot_region_properties,
 };
diff --git a/src/vendorcode/eltan/security/verified_boot/vboot_check.c b/src/vendorcode/eltan/security/verified_boot/vboot_check.c
index e2258b9..1ef9d53 100644
--- a/src/vendorcode/eltan/security/verified_boot/vboot_check.c
+++ b/src/vendorcode/eltan/security/verified_boot/vboot_check.c
@@ -448,7 +448,7 @@
 }
 #endif //__RAMSTAGE__

-const struct cbfs_locator cbfs_master_header_locator = {
+const struct cbfs_locator cbfs_default_locator = {
    .name = "Vendorcode Header Locator",
    .prepare = vendor_secure_prepare,
    .locate = vendor_secure_locate

[Julius Werner (Code Review)]

Hello Aaron Durbin, Patrick Rudolph, build bot (Jenkins), Patrick Georgi, Furquan Shaikh,

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

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

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

Change subject: cbfs: Stop checking master header
......................................................................

cbfs: Stop checking master header

The CBFS master header is a legacy structure that just conveys the same
information we already have from the FMAP these days. We're still
including it to support older CBFS implementations in some payloads, but
there's no need for coreboot itself to follow this indirection anymore.
This patch simplifies the default CBFS locator to just return the CBFS
offset and size from the FMAP directly.

Change-Id: I6b00dd7f276364d62fa1f637efbaee0e80607c49
Signed-off-by: Julius Werner 
---
M src/lib/cbfs.c
M src/soc/intel/apollolake/mmap_boot.c
M src/vendorcode/eltan/security/verified_boot/vboot_check.c
3 files changed, 14 insertions(+), 42 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/36688/2

[Joel Kitching (Code Review)]

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36688 )

Change subject: cbfs: Stop checking master header
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/36688/2/src/lib/cbfs.c 
File src/lib/cbfs.c:

https://review.coreboot.org/c/coreboot/+/36688/2/src/lib/cbfs.c@299 
PS2, Line 299: find
finds

[Frans Hendriks (Code Review)]

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36688 )

Change subject: cbfs: Stop checking master header
......................................................................

Patch Set 2:

Will look into alternative support for Facebook FBG1701 (solving Jenkins build error)

[Aaron Durbin (Code Review)]

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

Change subject: cbfs: Stop checking master header
......................................................................

Patch Set 2: Code-Review+2

[Julius Werner (Code Review)]

Hello Aaron Durbin, Patrick Rudolph, Wim Vervoorn, Frans Hendriks, build bot (Jenkins), Joel Kitching, Patrick Georgi, Furquan Shaikh,

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

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

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

Change subject: cbfs: Stop checking master header
......................................................................

cbfs: Stop checking master header

The CBFS master header is a legacy structure that just conveys the same
information we already have from the FMAP these days. We're still
including it to support older CBFS implementations in some payloads, but
there's no need for coreboot itself to follow this indirection anymore.
This patch simplifies the default CBFS locator to just return the CBFS
offset and size from the FMAP directly.

Change-Id: I6b00dd7f276364d62fa1f637efbaee0e80607c49
Signed-off-by: Julius Werner 
---
M src/include/cbfs.h
M src/lib/cbfs.c
M src/soc/intel/apollolake/mmap_boot.c
M src/vendorcode/eltan/security/verified_boot/vboot_check.c
4 files changed, 16 insertions(+), 44 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/36688/3

[Aaron Durbin (Code Review)]

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

Change subject: cbfs: Stop checking master header
......................................................................

Patch Set 3: Code-Review+2

[Wim Vervoorn (Code Review)]

Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36688 )

Change subject: cbfs: Stop checking master header
......................................................................

Patch Set 4: Code-Review+1

This is fine with us. We are making the changes now to remove the requirement on the cbfs prepare and locate completely. Hopefully this will be completed by the end of the week.

[Julius Werner (Code Review)]

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

Change subject: cbfs: Stop checking master header
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36688/2/src/lib/cbfs.c 
File src/lib/cbfs.c:

https://review.coreboot.org/c/coreboot/+/36688/2/src/lib/cbfs.c@299 
PS2, Line 299: find
> finds

Done

[Julius Werner (Code Review)]

Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36688 )

Change subject: cbfs: Stop checking master header
......................................................................

cbfs: Stop checking master header

The CBFS master header is a legacy structure that just conveys the same
information we already have from the FMAP these days. We're still
including it to support older CBFS implementations in some payloads, but
there's no need for coreboot itself to follow this indirection anymore.
This patch simplifies the default CBFS locator to just return the CBFS
offset and size from the FMAP directly.

Change-Id: I6b00dd7f276364d62fa1f637efbaee0e80607c49
Signed-off-by: Julius Werner 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/36688
Reviewed-by: Wim Vervoorn 
Reviewed-by: Aaron Durbin 
Tested-by: build bot (Jenkins) 
---
M src/include/cbfs.h
M src/lib/cbfs.c
M src/soc/intel/apollolake/mmap_boot.c
M src/vendorcode/eltan/security/verified_boot/vboot_check.c
4 files changed, 16 insertions(+), 44 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Aaron Durbin: Looks good to me, approved
  Wim Vervoorn: Looks good to me, but someone else must approve

diff --git a/src/include/cbfs.h b/src/include/cbfs.h
index f012441..d76fdd3 100644
--- a/src/include/cbfs.h
+++ b/src/include/cbfs.h
@@ -74,7 +74,7 @@
 };

/* Default CBFS locator .locate() callback that locates "COREBOOT" region. */
-int cbfs_master_header_props(struct cbfs_props *props);
+int cbfs_default_props(struct cbfs_props *props);

/* Return < 0 on error otherwise props are filled out accordingly. */
 int cbfs_boot_region_properties(struct cbfs_props *props);
diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c
index 27a1592..408e685 100644
--- a/src/lib/cbfs.c
+++ b/src/lib/cbfs.c
@@ -296,44 +296,16 @@
    return 0;
 }

-/* This only supports the "COREBOOT" fmap region. */
-int cbfs_master_header_props(struct cbfs_props *props)
+/* The default locator to find the CBFS in the "COREBOOT" FMAP region. */
+int cbfs_default_props(struct cbfs_props *props)
 {
-	struct cbfs_header header;
-	const struct region_device *bdev;
-	int32_t rel_offset;
-	size_t offset;
+	struct region region;

-	bdev = boot_device_ro();
-
-	if (bdev == NULL)
+	if (fmap_locate_area("COREBOOT", &region))
    	return -1;

-	struct region fmap_region;
-	if (fmap_locate_area("COREBOOT", &fmap_region))
-		return -1;
-	size_t fmap_top = region_end(&fmap_region);
-
-	/* Find location of header using signed 32-bit offset from
-	 * end of CBFS region. */
-	offset = fmap_top - sizeof(int32_t);
-	if (rdev_readat(bdev, &rel_offset, offset, sizeof(int32_t)) < 0)
-		return -1;
-
-	offset = fmap_top + (int32_t)le32_to_cpu(rel_offset);
-	if (rdev_readat(bdev, &header, offset, sizeof(header)) < 0)
-		return -1;
-
-	header.magic = ntohl(header.magic);
-	header.romsize = ntohl(header.romsize);
-	header.offset = ntohl(header.offset);
-
-	if (header.magic != CBFS_HEADER_MAGIC)
-		return -1;
-
-	props->offset = header.offset;
-	props->size = header.romsize;
-	props->size -= props->offset;
+	props->offset = region_offset(&region);
+	props->size = region_sz(&region);

printk(BIOS_SPEW, "CBFS @ %zx size %zx\n", props->offset, props->size);

@@ -343,9 +315,9 @@
 /* This struct is marked as weak to allow a particular platform to
  * override the master header logic. This implementation should work for most
  * devices. */
-const struct cbfs_locator __weak cbfs_master_header_locator = {
-	.name = "Master Header Locator",
-	.locate = cbfs_master_header_props,
+const struct cbfs_locator __weak cbfs_default_locator = {
+	.name = "COREBOOT Locator",
+	.locate = cbfs_default_props,
 };

extern const struct cbfs_locator vboot_locator;
@@ -359,7 +331,7 @@
     */
    &vboot_locator,
 #endif
-	&cbfs_master_header_locator,
+	&cbfs_default_locator,
 };

int cbfs_boot_region_properties(struct cbfs_props *props)
diff --git a/src/soc/intel/apollolake/mmap_boot.c b/src/soc/intel/apollolake/mmap_boot.c
index 614b603..631a834 100644
--- a/src/soc/intel/apollolake/mmap_boot.c
+++ b/src/soc/intel/apollolake/mmap_boot.c
@@ -128,10 +128,10 @@
 }

/*
- * Named cbfs_master_header_locator so that it overrides the default, but
- * incompatible locator in cbfs.c
+ * Named cbfs_default_locator so that it overrides the default, but incompatible
+ * locator in cbfs.c
  */
-const struct cbfs_locator cbfs_master_header_locator = {
+const struct cbfs_locator cbfs_default_locator = {
    .name = "IAFW Locator",
    .locate = iafw_boot_region_properties,
 };
diff --git a/src/vendorcode/eltan/security/verified_boot/vboot_check.c b/src/vendorcode/eltan/security/verified_boot/vboot_check.c
index 823c2de..c58ace1 100644
--- a/src/vendorcode/eltan/security/verified_boot/vboot_check.c
+++ b/src/vendorcode/eltan/security/verified_boot/vboot_check.c
@@ -355,8 +355,8 @@
    }
 }

-const struct cbfs_locator cbfs_master_header_locator = {
+const struct cbfs_locator cbfs_default_locator = {
    .name = "Vendorcode Header Locator",
    .prepare = vendor_secure_prepare,
-	.locate = cbfs_master_header_props
+	.locate = cbfs_default_props
 };