Change in coreboot[master]: cbfs: store header offset as absolute value BE from fmap_top

[Marty E. Plummer (Code Review)]

Marty E. Plummer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36346 )

Change subject: cbfs: store header offset as absolute value BE from fmap_top
......................................................................

cbfs: store header offset as absolute value BE from fmap_top

Change-Id: Idc99b0c133faa2aa15d06f998e7371d332ffa490
Signed-off-by: Marty E. Plummer 
---
M src/lib/cbfs.c
M util/cbfstool/cbfstool.c
2 files changed, 9 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/36346/1

diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c
index 91368fb..8fb747f 100644
--- a/src/lib/cbfs.c
+++ b/src/lib/cbfs.c
@@ -296,13 +296,14 @@

size_t fmap_top = ___FMAP__COREBOOT_BASE + ___FMAP__COREBOOT_SIZE;

-	/* Find location of header using signed 32-bit offset from
+	/* Find location of header using unsigned 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;
+	/* Subtract after converting to the native byte ordering */
+	offset = fmap_top - ntohl(rel_offset);
    if (rdev_readat(bdev, &header, offset, sizeof(header)) < 0)
    	return -1;

diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c
index 65c5e08..c829005 100644
--- a/util/cbfstool/cbfstool.c
+++ b/util/cbfstool/cbfstool.c
@@ -285,8 +285,8 @@

static void fill_header_offset(void *location, uint32_t offset)
 {
-	// TODO: When we have a BE target, we'll need to store this as BE
-	write_le32(location, offset);
+	// Store as BE as all other cbfs data structures are
+	write_be32(location, offset);
 }

static int update_master_header_loc_topswap(struct cbfs_image *image,
@@ -380,7 +380,10 @@

uint32_t header_offset = CBFS_SUBHEADER(entry) -
    	buffer_get(&image.buffer);
-	header_offset = -(buffer_size(&image.buffer) - header_offset);
+	/* Store as absolute value (positive) and subtract
+	 * it in src/lib/cbfs.c:cbfs_master_header_props
+	 */
+	header_offset = buffer_size(&image.buffer) - header_offset;

h_loc = (void *)(buffer_get(&image.buffer) +
    			buffer_size(&image.buffer) - 4);

[Julius Werner (Code Review)]

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

Change subject: cbfs: store header offset as absolute value BE from fmap_top
......................................................................

Patch Set 1: Code-Review-1

I... would rather we do not do this. I understand that it's inconsistent with the rest of CBFS, but then again we don't really support any big-endian platforms anyway so I don't know what the whole well-defined endianness thing in CBFS is good for other than annoying people who have to work with it.

There are a ton of CBFS parsers out there: in SeaBIOS, in GRUB, in some random old Chrome OS utilities that we don't even know have them anymore... I don't think we could find them all if we tried. A change like this would break all of them. If we ever wanna do a full backwards-incompatible overhaul of CBFS we can add this too (although then I think this pointer would go away entirely in favor of the FMAP instead), but until then I think we should try to stay compatible with what we have.

Also, I think(?) this is still used as a jump offset by the 16-bit bootblock entry code on x86, so you would have to do byte swapping in assembly there as well (and I think you only have 16 bytes so that may not be possible).

[Angel Pons (Code Review)]

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

Change subject: cbfs: store header offset as absolute value BE from fmap_top
......................................................................

Patch Set 1:
> Patch Set 1: Code-Review-1
>
> but then again we don't really support any big-endian platforms anyway

Yet. Marty is working on POWER stuff.

[Julius Werner (Code Review)]

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

Change subject: cbfs: store header offset as absolute value BE from fmap_top
......................................................................

Patch Set 1:

Hmm... well, if you really need this to be well-defined, can we define it to always be little-endian? That wouldn't break existing readers.

[Marty E. Plummer (Code Review)]

Marty E. Plummer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36346 )

Change subject: cbfs: store header offset as absolute value BE from fmap_top
......................................................................

Patch Set 1:
> Patch Set 1:
>
> Hmm... well, if you really need this to be well-defined, can we define it to always be little-endian? That wouldn't break existing readers.

Well, as you mentioned, there is the consistency issue, and to be frank I am
not aware of any things other than src/lib/cbfs.c which look at this particular
offset. I could be wrong but I think the others just look at the header it
points to.

[Marty E. Plummer (Code Review)]

Marty E. Plummer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36346 )

Change subject: cbfs: store header offset as absolute value BE from fmap_top
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> >
> > Hmm... well, if you really need this to be well-defined, can we define it to always be little-endian? That wouldn't break existing readers.
>
> Well, as you mentioned, there is the consistency issue, and to be frank I am
> not aware of any things other than src/lib/cbfs.c which look at this particular
> offset. I could be wrong but I think the others just look at the header it
> points to.

Also there's the probem of folks who may be building on a big-endian system
(corebooted or not).

[Marty E. Plummer (Code Review)]

Marty E. Plummer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36346 )

Change subject: cbfs: store header offset as absolute value BE from fmap_top
......................................................................

Patch Set 1:

Hell, I'm even up for the idea of a bigish overhaul of cbfs,
in that it will store these header and other similar values
in be/le depending on the build arch (eg, for most everything
the values will be stored as LE and for big-endian ppc64 it
would store them as be). Would take out a lot of the ntohl
and so on in the coreboot code itself and reduce some of the
overhead (granted, byte swizzling is pretty low cost in terms
of instructions).

[Julius Werner (Code Review)]

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

Change subject: cbfs: store header offset as absolute value BE from fmap_top
......................................................................

Patch Set 1:
> Well, as you mentioned, there is the consistency issue, and to be frank I am
> not aware of any things other than src/lib/cbfs.c which look at this particular
> offset. I could be wrong but I think the others just look at the header it
> points to.

Here are just two other CBFS parsers I'm aware of that are using this pointer:

https://github.com/coreboot/seabios/blob/master/src/fw/coreboot.c#L432
https://github.com/coreos/grub/blob/2.02-coreos/grub-core/fs/cbfs.c#L345

I know there are more and I'm pretty sure we couldn't find all of them if we tried.

This has worked this way since basically forever and I don't think we should break it just for some minor design cleanliness concern. I understand the problem of supporting big-endian systems if we want that, bug big-endian systems can still explicitly convert a value from little-endian to CPU byte order, so I think that's how we should handle this one to avoid breaking older parsers.

[Julius Werner (Code Review)]

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

Change subject: cbfs: store header offset as absolute value BE from fmap_top
......................................................................

Patch Set 1:
> Hell, I'm even up for the idea of a bigish overhaul of cbfs,
> in that it will store these header and other similar values
> in be/le depending on the build arch (eg, for most everything
> the values will be stored as LE and for big-endian ppc64 it
> would store them as be).

I hate the endian conversions too but I think this fails with the same problem... CBFS is just too much of a standard and has been reimplemented in too many places to make this worthwhile. I think most coreboot users would probably prefer if we keep the slightly gnarly but downwards-compatible thing we have. (If you really want to make such a big change you should probably bring it up on the mailing list first and see what others think.)

[Marty E. Plummer (Code Review)]

Marty E. Plummer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36346 )

Change subject: cbfs: store header offset as absolute value BE from fmap_top
......................................................................

Patch Set 1:
> Patch Set 1:
> > Well, as you mentioned, there is the consistency issue, and to be frank I am
> > not aware of any things other than src/lib/cbfs.c which look at this particular
> > offset. I could be wrong but I think the others just look at the header it
> > points to.
>
> Here are just two other CBFS parsers I'm aware of that are using this pointer:
>
> https://github.com/coreboot/seabios/blob/master/src/fw/coreboot.c#L432
> https://github.com/coreos/grub/blob/2.02-coreos/grub-core/fs/cbfs.c#L345
>
> I know there are more and I'm pretty sure we couldn't find all of them if we tried.
>
> This has worked this way since basically forever and I don't think we should break it just for some minor design cleanliness concern. I understand the problem of supporting big-endian systems if we want that, bug big-endian systems can still explicitly convert a value from little-endian to CPU byte order, so I think that's how we should handle this one to avoid breaking older parsers.

True, it is fairly simple (in c-land, at least) to convert be to le.
Problem is, a coreboot.rom (as it currently stands) built on a big
endian system would store this pointer as be, so conversion in cbfs.c
would need even more logic to determine whether or not to leave it
alone.

[Marty E. Plummer (Code Review)]

Marty E. Plummer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36346 )

Change subject: cbfs: store header offset as absolute value BE from fmap_top
......................................................................

Patch Set 1:
> Patch Set 1:
> > Hell, I'm even up for the idea of a bigish overhaul of cbfs,
> > in that it will store these header and other similar values
> > in be/le depending on the build arch (eg, for most everything
> > the values will be stored as LE and for big-endian ppc64 it
> > would store them as be).
>
> I hate the endian conversions too but I think this fails with the same problem... CBFS is just too much of a standard and has been reimplemented in too many places to make this worthwhile. I think most coreboot users would probably prefer if we keep the slightly gnarly but downwards-compatible thing we have. (If you really want to make such a big change you should probably bring it up on the mailing list first and see what others think.)

Honestly I don't care about endian conversions; its just part of
the job. I honestly don't even care if this approach is what is
taken, but the conversation has to get started as to be frank this
right here is my current stopping point for the power9 port (the
jump from bootblock to ramstage fails because of this) and any
solution which will let me get past this stupid issue and on to
actually interesting problems is fine by me.

Change in coreboot[master]: cbfs: read header offset as explicitly LE.

[Marty E. Plummer (Code Review)]

Hello Julius Werner, build bot (Jenkins),

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

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

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

Change subject: cbfs: read header offset as explicitly LE.
......................................................................

cbfs: read header offset as explicitly LE.

le32_to_cpu spits out uint32_t on BE targets, cast it.

Change-Id: Idc99b0c133faa2aa15d06f998e7371d332ffa490
Signed-off-by: Marty E. Plummer 
---
M src/lib/cbfs.c
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/36346/2

Change in coreboot[master]: cbfs: read header offset as explicitly LE.

[Julius Werner (Code Review)]

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

Change subject: cbfs: read header offset as explicitly LE.
......................................................................

Patch Set 2: Code-Review+2

You'll probably want to add explicit conversion to LE in cbfstool as well.

Change in coreboot[master]: cbfs: read header offset as explicitly LE.

[Angel Pons (Code Review)]

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

Change subject: cbfs: read header offset as explicitly LE.
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/36346/2//COMMIT_MSG@7 
PS2, Line 7: .
Nit: please do not use a period at the end of commit summaries

Change in coreboot[master]: cbfs: read header offset as explicitly LE

[Marty E. Plummer (Code Review)]

Hello Julius Werner, build bot (Jenkins),

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

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

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

Change subject: cbfs: read header offset as explicitly LE
......................................................................

cbfs: read header offset as explicitly LE

le32_to_cpu spits out uint32_t on BE targets, cast it.

Change-Id: Idc99b0c133faa2aa15d06f998e7371d332ffa490
Signed-off-by: Marty E. Plummer 
---
M src/lib/cbfs.c
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/36346/3

Change in coreboot[master]: cbfs: read header offset as explicitly LE

[Marty E. Plummer (Code Review)]

Marty E. Plummer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36346 )

Change subject: cbfs: read header offset as explicitly LE
......................................................................

Patch Set 3:
> Patch Set 2: Code-Review+2
>
> You'll probably want to add explicit conversion to LE in cbfstool as well.

Unless I'm mistaken, util/cbfstool/cbfstool.c:286 or so already does so,
calling write_le32 for this pointer.

Change in coreboot[master]: cbfs: read header offset as explicitly LE

[Patrick Georgi (Code Review)]

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

Change subject: cbfs: read header offset as explicitly LE
......................................................................

Patch Set 3:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/36346/2//COMMIT_MSG@7 
PS2, Line 7: .
> Nit: please do not use a period at the end of commit summaries

Done

Change in coreboot[master]: cbfs: read header offset as explicitly LE

[Patrick Georgi (Code Review)]

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

Change subject: cbfs: read header offset as explicitly LE
......................................................................

cbfs: read header offset as explicitly LE

le32_to_cpu spits out uint32_t on BE targets, cast it.

Change-Id: Idc99b0c133faa2aa15d06f998e7371d332ffa490
Signed-off-by: Marty E. Plummer 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/36346
Tested-by: build bot (Jenkins) 
Reviewed-by: Julius Werner 
---
M src/lib/cbfs.c
1 file changed, 1 insertion(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Julius Werner: Looks good to me, approved

diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c
index 1e8a93f..9ac1bc0 100644
--- a/src/lib/cbfs.c
+++ b/src/lib/cbfs.c
@@ -302,7 +302,7 @@
    if (rdev_readat(bdev, &rel_offset, offset, sizeof(int32_t)) < 0)
    	return -1;

-	offset = fmap_top + rel_offset;
+	offset = fmap_top + (int32_t)le32_to_cpu(rel_offset);
    if (rdev_readat(bdev, &header, offset, sizeof(header)) < 0)
    	return -1;