Change in coreboot[master]: fmap: Add get_fmap_flash_offset()

[Furquan Shaikh (Code Review)]

Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35639 )

Change subject: fmap: Add get_fmap_flash_offset()
......................................................................

fmap: Add get_fmap_flash_offset()

CB:35377 changed the behavior of find_fmap_directory() to return
pointer to CBMEM_ID_FMAP if fmap is cached in
cbmem. lb_boot_media_params() calls find_fmap_directory to add offset
of fmap in flash to coreboot table. However, because of the change in
behavior of find_fmap_directory(), it ended up adding 0 as the offset.

This change adds a new function get_fmap_flash_offset() which returns
the offset of fmap in flash. Ideally, all payloads should move to
using the FMAP from CBMEM. However, in order to maintain compatibility
with payloads which are not updated, ensure that fmap_offset is
updated correctly.

In a follow up patch, we need to push a change to libpayload to expose
the fmap cache pointer to lib_sysinfo.

BUG=b:141723751

Change-Id: I7ff6e8199143d1a992a83d7de1e3b44813b733f4
Signed-off-by: Furquan Shaikh 
---
M src/include/fmap.h
M src/lib/coreboot_table.c
M src/lib/fmap.c
3 files changed, 10 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/35639/1

diff --git a/src/include/fmap.h b/src/include/fmap.h
index ab7e5ab..f989e5d 100644
--- a/src/include/fmap.h
+++ b/src/include/fmap.h
@@ -48,4 +48,8 @@
 /* Write provided buffer into fmap area.
  * Return size written on success, < 0 on error. */
 ssize_t fmap_overwrite_area(const char *name, const void *buffer, size_t size);
+
+/* Get offset of FMAP in flash. */
+uint64_t get_fmap_flash_offset(void);
+
 #endif
diff --git a/src/lib/coreboot_table.c b/src/lib/coreboot_table.c
index 9c5942f..d3576e6 100644
--- a/src/lib/coreboot_table.c
+++ b/src/lib/coreboot_table.c
@@ -256,7 +256,6 @@
    struct lb_boot_media_params *bmp;
    struct cbfs_props props;
    const struct region_device *boot_dev;
-	struct region_device fmrd;

boot_device_init();

@@ -275,9 +274,7 @@
    bmp->cbfs_size = props.size;
    bmp->boot_media_size = region_device_sz(boot_dev);

-	bmp->fmap_offset = ~(uint64_t)0;
-	if (find_fmap_directory(&fmrd) == 0)
-		bmp->fmap_offset = region_device_offset(&fmrd);
+	bmp->fmap_offset = get_fmap_flash_offset();
 }

static void lb_ram_code(struct lb_header *header)
diff --git a/src/lib/fmap.c b/src/lib/fmap.c
index f007418..027d6ac 100644
--- a/src/lib/fmap.c
+++ b/src/lib/fmap.c
@@ -31,6 +31,11 @@
 static int fmap_print_once CAR_GLOBAL;
 static struct mem_region_device fmap_cache CAR_GLOBAL;

+uint64_t get_fmap_flash_offset(void)
+{
+	return FMAP_OFFSET;
+}
+
 int find_fmap_directory(struct region_device *fmrd)
 {
    const struct region_device *boot;

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35639 )

Change subject: fmap: Add get_fmap_flash_offset()
......................................................................

Patch Set 1: Code-Review+1

[Patrick Rudolph (Code Review)]

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

Change subject: fmap: Add get_fmap_flash_offset()
......................................................................

Patch Set 1: Code-Review+1

(2 comments)

Godd catch and thanks for fixing that!

https://review.coreboot.org/c/coreboot/+/35639/1/src/include/fmap.h 
File src/include/fmap.h:

https://review.coreboot.org/c/coreboot/+/35639/1/src/include/fmap.h@23 
PS1, Line 23: int find_fmap_directory(struct region_device *fmrd);
should be unexported, now that there are no more external users.

https://review.coreboot.org/c/coreboot/+/35639/1/src/include/fmap.h@53 
PS1, Line 53: uint64_t get_fmap_flash_offset(void);
can you use fmap_locate_area_as_rdev("FMAP", ...) instead?

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35639 )

Change subject: fmap: Add get_fmap_flash_offset()
......................................................................

Patch Set 1: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/35639/1/src/lib/coreboot_table.c 
File src/lib/coreboot_table.c:

https://review.coreboot.org/c/coreboot/+/35639/1/src/lib/coreboot_table.c@a2... 
PS1, Line 279: if (find_fmap_directory(&fmrd) == 0)
             : 		bmp->fmap_offset = region_device_offset(&fmrd);
Would changing this to

/* offsets are relative to start of boot media */
if (fmap_locate_area_as_rdev("FMAP", &fmrd) == 0)
    bmp->fmap_offset = region_device_offset(&fmrd);

Not be a better solution?

[Furquan Shaikh (Code Review)]

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

Change subject: fmap: Add get_fmap_flash_offset()
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/35639/1/src/include/fmap.h 
File src/include/fmap.h:

https://review.coreboot.org/c/coreboot/+/35639/1/src/include/fmap.h@23 
PS1, Line 23: int find_fmap_directory(struct region_device *fmrd);
> should be unexported, now that there are no more external users.

Ah good point. I missed that this was the only user of find_fmap_directory() outside of fmap.c

https://review.coreboot.org/c/coreboot/+/35639/1/src/include/fmap.h@53 
PS1, Line 53: uint64_t get_fmap_flash_offset(void);
> can you use fmap_locate_area_as_rdev("FMAP", ... […]

I thought of doing that initially, but skipped it looking at all the steps it would take to parse through the map to get to the same offset which was used in the first place to get to it. But, if it seems cleaner, I can push a patch to do that.

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35639 )

Change subject: fmap: Add get_fmap_flash_offset()
......................................................................

Patch Set 1: Code-Review+2

[V Sowmya (Code Review)]

V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35639 )

Change subject: fmap: Add get_fmap_flash_offset()
......................................................................

Patch Set 1: Code-Review+2

[Aaron Durbin (Code Review)]

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

Change subject: fmap: Add get_fmap_flash_offset()
......................................................................

Patch Set 1: Code-Review+1

Change in coreboot[master]: coreboot_table: Use fmap_locate_area_as_rdev() for fmap_offset

[Furquan Shaikh (Code Review)]

Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Julius Werner, Arthur Heymans, Tim Wawrzynczak, V Sowmya, Shelley Chen, build bot (Jenkins),

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

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

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

Change subject: coreboot_table: Use fmap_locate_area_as_rdev() for fmap_offset
......................................................................

coreboot_table: Use fmap_locate_area_as_rdev() for fmap_offset

CB:35377 changed the behavior of find_fmap_directory() to return
pointer to CBMEM_ID_FMAP if fmap is cached in
cbmem. lb_boot_media_params() calls find_fmap_directory() to add
offset of fmap in flash to coreboot table. However, because of the
change in behavior of find_fmap_directory(), it ended up adding 0 as
the offset.

This change uses fmap_locate_area_as_rdev() instead of
find_fmap_directory() to get to the offset of FMAP in flash. Ideally,
all payloads should move to using the FMAP from CBMEM. However, in
order to maintain compatibility with payloads which are not updated,
ensure that fmap_offset is updated correctly.

Since find_fmap_directory() is no longer used outside fmap.c, this
change also removes it from fmap.h and limits scope to fmap.c.

In a follow up patch, we need to push a change to libpayload to expose
the fmap cache pointer to lib_sysinfo.

BUG=b:141723751

Change-Id: I7ff6e8199143d1a992a83d7de1e3b44813b733f4
Signed-off-by: Furquan Shaikh 
---
M src/include/fmap.h
M src/lib/coreboot_table.c
M src/lib/fmap.c
3 files changed, 3 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/35639/2

Change in coreboot[master]: coreboot_table: Use fmap_locate_area_as_rdev() for fmap_offset

[Furquan Shaikh (Code Review)]

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

Change subject: coreboot_table: Use fmap_locate_area_as_rdev() for fmap_offset
......................................................................

Patch Set 2:

(3 comments)

https://review.coreboot.org/c/coreboot/+/35639/1/src/include/fmap.h 
File src/include/fmap.h:

https://review.coreboot.org/c/coreboot/+/35639/1/src/include/fmap.h@23 
PS1, Line 23: int find_fmap_directory(struct region_device *fmrd);
> Ah good point. I missed that this was the only user of find_fmap_directory() outside of fmap. […]

Done

https://review.coreboot.org/c/coreboot/+/35639/1/src/include/fmap.h@53 
PS1, Line 53: uint64_t get_fmap_flash_offset(void);
> I thought of doing that initially, but skipped it looking at all the steps it would take to parse th […]

Done

https://review.coreboot.org/c/coreboot/+/35639/1/src/lib/coreboot_table.c 
File src/lib/coreboot_table.c:

https://review.coreboot.org/c/coreboot/+/35639/1/src/lib/coreboot_table.c@a2... 
PS1, Line 279: if (find_fmap_directory(&fmrd) == 0)
             : 		bmp->fmap_offset = region_device_offset(&fmrd);
> Would changing this to […]

Done

Change in coreboot[master]: coreboot_table: Use fmap_locate_area_as_rdev() for fmap_offset

[Furquan Shaikh (Code Review)]

Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Julius Werner, Arthur Heymans, Tim Wawrzynczak, V Sowmya, Shelley Chen, build bot (Jenkins),

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

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

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

Change subject: coreboot_table: Use fmap_locate_area_as_rdev() for fmap_offset
......................................................................

coreboot_table: Use fmap_locate_area_as_rdev() for fmap_offset

CB:35377 changed the behavior of find_fmap_directory() to return
pointer to CBMEM_ID_FMAP if fmap is cached in
cbmem. lb_boot_media_params() calls find_fmap_directory() to add
offset of fmap in flash to coreboot table. However, because of the
change in behavior of find_fmap_directory(), it ended up adding 0 as
the offset.

This change uses fmap_locate_area_as_rdev() instead of
find_fmap_directory() to get to the offset of FMAP in flash. Ideally,
all payloads should move to using the FMAP from CBMEM. However, in
order to maintain compatibility with payloads which are not updated,
ensure that fmap_offset is updated correctly.

Since find_fmap_directory() is no longer used outside fmap.c, this
change also removes it from fmap.h and limits scope to fmap.c.

In a follow up patch, we need to push a change to libpayload to expose
the fmap cache pointer to lib_sysinfo.

BUG=b:141723751

Change-Id: I7ff6e8199143d1a992a83d7de1e3b44813b733f4
Signed-off-by: Furquan Shaikh 
---
M src/include/fmap.h
M src/lib/coreboot_table.c
M src/lib/fmap.c
3 files changed, 2 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/35639/3

Change in coreboot[master]: coreboot_table: Use fmap_locate_area_as_rdev() for fmap_offset

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35639 )

Change subject: coreboot_table: Use fmap_locate_area_as_rdev() for fmap_offset
......................................................................

Patch Set 3: Code-Review+2

Change in coreboot[master]: coreboot_table: Use fmap_locate_area_as_rdev() for fmap_offset

[Aaron Durbin (Code Review)]

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

Change subject: coreboot_table: Use fmap_locate_area_as_rdev() for fmap_offset
......................................................................

Patch Set 3: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/35639/3/src/lib/coreboot_table.c 
File src/lib/coreboot_table.c:

https://review.coreboot.org/c/coreboot/+/35639/3/src/lib/coreboot_table.c@27... 
PS3, Line 279: "FMAP",
Does anyone else find this sort of silly? Using fmap to find fmap. I think this is fine, but it certainly makes me chuckle.

Change in coreboot[master]: coreboot_table: Use fmap_locate_area_as_rdev() for fmap_offset

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35639 )

Change subject: coreboot_table: Use fmap_locate_area_as_rdev() for fmap_offset
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35639/3/src/lib/coreboot_table.c 
File src/lib/coreboot_table.c:

https://review.coreboot.org/c/coreboot/+/35639/3/src/lib/coreboot_table.c@27... 
PS3, Line 279: "FMAP",
> Does anyone else find this sort of silly? Using fmap to find fmap. I think this is fine, but it certainly makes me chuckle.

Makes me wonder: what payload or application is using this parameter? It could be deprecated in the future, I suppose?

Change in coreboot[master]: coreboot_table: Use fmap_locate_area_as_rdev() for fmap_offset

[Aaron Durbin (Code Review)]

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

Change subject: coreboot_table: Use fmap_locate_area_as_rdev() for fmap_offset
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35639/3/src/lib/coreboot_table.c 
File src/lib/coreboot_table.c:

https://review.coreboot.org/c/coreboot/+/35639/3/src/lib/coreboot_table.c@27... 
PS3, Line 279: "FMAP",
> > Does anyone else find this sort of silly? Using fmap to find fmap. I think this is fine, but it certainly makes me chuckle.
>
> Makes me wonder: what payload or application is using this parameter? It could be deprecated in the future, I suppose?

depthcharge on our end was using to obtain the FMAP offset instead of needing to maintain that static value in multiple places.

Change in coreboot[master]: coreboot_table: Use fmap_locate_area_as_rdev() for fmap_offset

[Furquan Shaikh (Code Review)]

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

Change subject: coreboot_table: Use fmap_locate_area_as_rdev() for fmap_offset
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35639/3/src/lib/coreboot_table.c 
File src/lib/coreboot_table.c:

https://review.coreboot.org/c/coreboot/+/35639/3/src/lib/coreboot_table.c@27... 
PS3, Line 279: "FMAP",
> Does anyone else find this sort of silly? Using fmap to find fmap. I think this is fine, but it certainly makes me chuckle.

Yes!! That is the reason I used FMAP_OFFSET in the first iteration of this patch. But since I got 2 comments asking to switch to fmap_locate_area_as_rdev(), I updated it. I can go back to FMAP_OFFSET since I liked it more :P.

(https://review.coreboot.org/c/coreboot/+/35639/1/src/include/fmap.h#53)
> Makes me wonder: what payload or application is using this parameter? It could be deprecated in the future, I suppose?

Like Aaron said, on Chrome OS it was depthcharge. I am not sure if any other payload cares about it. We can eventually deprecate it if no one cares about knowing the fmap offset in flash.

Change in coreboot[master]: coreboot_table: Use fmap_locate_area_as_rdev() for fmap_offset

[Shelley Chen (Code Review)]

Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35639 )

Change subject: coreboot_table: Use fmap_locate_area_as_rdev() for fmap_offset
......................................................................

Patch Set 3: Code-Review+2

[Furquan Shaikh (Code Review)]

Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Julius Werner, Arthur Heymans, Tim Wawrzynczak, Shelley Chen, V Sowmya, build bot (Jenkins),

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

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

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

Change subject: fmap: Add get_fmap_flash_offset()
......................................................................

fmap: Add get_fmap_flash_offset()

CB:35377 changed the behavior of find_fmap_directory() to return
pointer to CBMEM_ID_FMAP if fmap is cached in
cbmem. lb_boot_media_params() calls find_fmap_directory to add offset
of fmap in flash to coreboot table. However, because of the change in
behavior of find_fmap_directory(), it ended up adding 0 as the offset.

This change adds a new function get_fmap_flash_offset() which returns
the offset of fmap in flash. Ideally, all payloads should move to
using the FMAP from CBMEM. However, in order to maintain compatibility
with payloads which are not updated, ensure that fmap_offset is
updated correctly.

Since find_fmap_directory() is no longer used outside fmap.c, this
change also removes it from fmap.h and limits scope to fmap.c.

In a follow up patch, we need to push a change to libpayload to expose
the fmap cache pointer to lib_sysinfo.

BUG=b:141723751

Change-Id: I7ff6e8199143d1a992a83d7de1e3b44813b733f4
Signed-off-by: Furquan Shaikh 
---
M src/include/fmap.h
M src/lib/coreboot_table.c
M src/lib/fmap.c
3 files changed, 11 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/35639/4

[Furquan Shaikh (Code Review)]

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

Change subject: fmap: Add get_fmap_flash_offset()
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35639/3/src/lib/coreboot_table.c 
File src/lib/coreboot_table.c:

https://review.coreboot.org/c/coreboot/+/35639/3/src/lib/coreboot_table.c@27... 
PS3, Line 279: "FMAP",
> > Does anyone else find this sort of silly? Using fmap to find fmap. […]

Okay. I have gone back to patch set 1. It really seems silly to have to use fmap to get to fmap.

[Patrick Rudolph (Code Review)]

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

Change subject: fmap: Add get_fmap_flash_offset()
......................................................................

Patch Set 4: Code-Review+2

[Aaron Durbin (Code Review)]

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

Change subject: fmap: Add get_fmap_flash_offset()
......................................................................

Patch Set 4: Code-Review+2

[Shelley Chen (Code Review)]

Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35639 )

Change subject: fmap: Add get_fmap_flash_offset()
......................................................................

Patch Set 4: Code-Review+2

[Shelley Chen (Code Review)]

Shelley Chen has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35639 )

Change subject: fmap: Add get_fmap_flash_offset()
......................................................................

fmap: Add get_fmap_flash_offset()

CB:35377 changed the behavior of find_fmap_directory() to return
pointer to CBMEM_ID_FMAP if fmap is cached in
cbmem. lb_boot_media_params() calls find_fmap_directory to add offset
of fmap in flash to coreboot table. However, because of the change in
behavior of find_fmap_directory(), it ended up adding 0 as the offset.

This change adds a new function get_fmap_flash_offset() which returns
the offset of fmap in flash. Ideally, all payloads should move to
using the FMAP from CBMEM. However, in order to maintain compatibility
with payloads which are not updated, ensure that fmap_offset is
updated correctly.

Since find_fmap_directory() is no longer used outside fmap.c, this
change also removes it from fmap.h and limits scope to fmap.c.

In a follow up patch, we need to push a change to libpayload to expose
the fmap cache pointer to lib_sysinfo.

BUG=b:141723751

Change-Id: I7ff6e8199143d1a992a83d7de1e3b44813b733f4
Signed-off-by: Furquan Shaikh 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/35639
Tested-by: build bot (Jenkins) 
Reviewed-by: Patrick Rudolph 
Reviewed-by: Aaron Durbin 
Reviewed-by: Shelley Chen 
---
M src/include/fmap.h
M src/lib/coreboot_table.c
M src/lib/fmap.c
3 files changed, 11 insertions(+), 8 deletions(-)

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

diff --git a/src/include/fmap.h b/src/include/fmap.h
index ab7e5ab..649ecc0 100644
--- a/src/include/fmap.h
+++ b/src/include/fmap.h
@@ -19,9 +19,6 @@
 #include <commonlib/region.h>
 #include <commonlib/fmap_serialized.h>

-/* Locate the fmap directory. Return 0 on success, < 0 on error. */
-int find_fmap_directory(struct region_device *fmrd);
-
 /* Locate the named area in the fmap and fill in a region device representing
  * that area. The region is a sub-region of the readonly boot media. Return
  * 0 on success, < 0 on error. */
@@ -48,4 +45,8 @@
 /* Write provided buffer into fmap area.
  * Return size written on success, < 0 on error. */
 ssize_t fmap_overwrite_area(const char *name, const void *buffer, size_t size);
+
+/* Get offset of FMAP in flash. */
+uint64_t get_fmap_flash_offset(void);
+
 #endif
diff --git a/src/lib/coreboot_table.c b/src/lib/coreboot_table.c
index 9c5942f..d3576e6 100644
--- a/src/lib/coreboot_table.c
+++ b/src/lib/coreboot_table.c
@@ -256,7 +256,6 @@
    struct lb_boot_media_params *bmp;
    struct cbfs_props props;
    const struct region_device *boot_dev;
-	struct region_device fmrd;

boot_device_init();

@@ -275,9 +274,7 @@
    bmp->cbfs_size = props.size;
    bmp->boot_media_size = region_device_sz(boot_dev);

-	bmp->fmap_offset = ~(uint64_t)0;
-	if (find_fmap_directory(&fmrd) == 0)
-		bmp->fmap_offset = region_device_offset(&fmrd);
+	bmp->fmap_offset = get_fmap_flash_offset();
 }

static void lb_ram_code(struct lb_header *header)
diff --git a/src/lib/fmap.c b/src/lib/fmap.c
index f007418..a427102 100644
--- a/src/lib/fmap.c
+++ b/src/lib/fmap.c
@@ -31,7 +31,12 @@
 static int fmap_print_once CAR_GLOBAL;
 static struct mem_region_device fmap_cache CAR_GLOBAL;

-int find_fmap_directory(struct region_device *fmrd)
+uint64_t get_fmap_flash_offset(void)
+{
+	return FMAP_OFFSET;
+}
+
+static int find_fmap_directory(struct region_device *fmrd)
 {
    const struct region_device *boot;
    struct fmap *fmap;