All, I found an issue with seabios when it is attempting to download and execute a payload that has been added to a coreboot rom image img/ directory. The cbfs header has a destination address of "u64 load_addr". The code that reads the destination address out of the header is using ntohl which only works on u32, so the address that the cbfstool puts in the header, 0x00000000000100000 gets converted to 0x00000000. I couldn't find where there was 64 bit version of the ntohl ftn/macro so I copied one out of coreboot.
Feel free to change where/how the ntohll gets implemented.
Thanks, Dave Frodin
P.S. Thanks to whomever came up with the img/payload method of adding payloads. It's slick.
diff --git a/src/coreboot.c b/src/coreboot.c index e116a14..4efec9c 100644 --- a/src/coreboot.c +++ b/src/coreboot.c @@ -470,6 +470,17 @@ struct cbfs_payload { struct cbfs_payload_segment segments[1]; };
+#define ntohll(x) \ + ((u64)( \ + (((u64)(x) & (u64)0x00000000000000ffULL) << 56) | \ + (((u64)(x) & (u64)0x000000000000ff00ULL) << 40) | \ + (((u64)(x) & (u64)0x0000000000ff0000ULL) << 24) | \ + (((u64)(x) & (u64)0x00000000ff000000ULL) << 8) | \ + (((u64)(x) & (u64)0x000000ff00000000ULL) >> 8) | \ + (((u64)(x) & (u64)0x0000ff0000000000ULL) >> 24) | \ + (((u64)(x) & (u64)0x00ff000000000000ULL) >> 40) | \ + (((u64)(x) & (u64)0xff00000000000000ULL) >> 56) )) + void cbfs_run_payload(struct cbfs_file *file) { @@ -480,7 +491,7 @@ cbfs_run_payload(struct cbfs_file *file) struct cbfs_payload_segment *seg = pay->segments; for (;;) { void *src = (void*)pay + ntohl(seg->offset); - void *dest = (void*)ntohl((u32)seg->load_addr); + void *dest = (void*)ntohll((u64)seg->load_addr); u32 src_len = ntohl(seg->len); u32 dest_len = ntohl(seg->mem_len); switch (seg->type) {
Dave Frodin wrote:
@@ -480,7 +491,7 @@ cbfs_run_payload(struct cbfs_file *file) struct cbfs_payload_segment *seg = pay->segments; for (;;) { void *src = (void*)pay + ntohl(seg->offset);
void *dest = (void*)ntohl((u32)seg->load_addr);
void *dest = (void*)ntohll((u64)seg->load_addr);
Is the (u64) cast still needed? The less casts the better.
//Peter
Peter,
You're right, that u64 cast wasn't needed, but I needed to cast the result to u32. The (void*) is evidently expecting a u32. Is that correct? Would SeaBIOS always be expecting a u32 for an address to execute from?
I also cleaned up the macro to use the existing ntohl. The new patch is below. I also tested it.
dave
commit 66c82fdbf283340067a8531ef6e3afae82102396 Author: Dave Frodin dave.frodin@se-eng.com Date: Tue Aug 7 17:01:08 2012 -0600
Seabios: This fixes reading of CBFS 64 bit destination addresses from payload headers. This allows img/payloads to run.
diff --git a/src/coreboot.c b/src/coreboot.c index e116a14..b989517 100644 --- a/src/coreboot.c +++ b/src/coreboot.c @@ -470,6 +470,8 @@ struct cbfs_payload { struct cbfs_payload_segment segments[1]; };
+#define ntohll(in) (((u64) ntohl( (in) & 0xFFFFFFFF) << 32) | ((u64) ntohl( (in) >> 32))) + void cbfs_run_payload(struct cbfs_file *file) { @@ -480,7 +482,7 @@ cbfs_run_payload(struct cbfs_file *file) struct cbfs_payload_segment *seg = pay->segments; for (;;) { void *src = (void*)pay + ntohl(seg->offset); - void *dest = (void*)ntohl((u32)seg->load_addr); + void *dest = (void*)(u32)ntohll(seg->load_addr); u32 src_len = ntohl(seg->len); u32 dest_len = ntohl(seg->mem_len); switch (seg->type) {
----- Original Message -----
From: "Peter Stuge" peter@stuge.se To: seabios@seabios.org Sent: Wednesday, August 8, 2012 5:29:57 AM Subject: Re: [SeaBIOS] The cbfs header for a payloads dest addr is a u64, use ntohll instead of ntohl
Dave Frodin wrote:
@@ -480,7 +491,7 @@ cbfs_run_payload(struct cbfs_file *file) struct cbfs_payload_segment *seg = pay->segments; for (;;) { void *src = (void*)pay + ntohl(seg->offset);
void *dest = (void*)ntohl((u32)seg->load_addr);
void *dest = (void*)ntohll((u64)seg->load_addr);Is the (u64) cast still needed? The less casts the better.
//Peter
SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
Peter, Have you had a chance to review my last patch (below)?
thanks, dave
----- Original Message -----
From: "Dave Frodin" dave.frodin@se-eng.com To: "Peter Stuge" peter@stuge.se Cc: seabios@seabios.org Sent: Wednesday, August 8, 2012 12:07:50 PM Subject: Re: [SeaBIOS] The cbfs header for a payloads dest addr is a u64, use ntohll instead of ntohl
Peter,
You're right, that u64 cast wasn't needed, but I needed to cast the result to u32. The (void*) is evidently expecting a u32. Is that correct? Would SeaBIOS always be expecting a u32 for an address to execute from?
I also cleaned up the macro to use the existing ntohl. The new patch is below. I also tested it.
dave
commit 66c82fdbf283340067a8531ef6e3afae82102396 Author: Dave Frodin dave.frodin@se-eng.com Date: Tue Aug 7 17:01:08 2012 -0600
Seabios: This fixes reading of CBFS 64 bit destination addresses from payload headers. This allows img/payloads to run.diff --git a/src/coreboot.c b/src/coreboot.c index e116a14..b989517 100644 --- a/src/coreboot.c +++ b/src/coreboot.c @@ -470,6 +470,8 @@ struct cbfs_payload { struct cbfs_payload_segment segments[1]; };
+#define ntohll(in) (((u64) ntohl( (in) & 0xFFFFFFFF) << 32) | ((u64) ntohl( (in) >> 32)))
void cbfs_run_payload(struct cbfs_file *file) { @@ -480,7 +482,7 @@ cbfs_run_payload(struct cbfs_file *file) struct cbfs_payload_segment *seg = pay->segments; for (;;) { void *src = (void*)pay + ntohl(seg->offset);
void *dest = (void*)ntohl((u32)seg->load_addr);
void *dest = (void*)(u32)ntohll(seg->load_addr); u32 src_len = ntohl(seg->len); u32 dest_len = ntohl(seg->mem_len); switch (seg->type) {----- Original Message -----
From: "Peter Stuge" peter@stuge.se To: seabios@seabios.org Sent: Wednesday, August 8, 2012 5:29:57 AM Subject: Re: [SeaBIOS] The cbfs header for a payloads dest addr is a u64, use ntohll instead of ntohl
Dave Frodin wrote:
@@ -480,7 +491,7 @@ cbfs_run_payload(struct cbfs_file *file) struct cbfs_payload_segment *seg = pay->segments; for (;;) { void *src = (void*)pay + ntohl(seg->offset);
void *dest = (void*)ntohl((u32)seg->load_addr);
void *dest = (void*)ntohll((u64)seg->load_addr);Is the (u64) cast still needed? The less casts the better.
//Peter
SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
On Wed, Aug 08, 2012 at 12:07:50PM -0600, Dave Frodin wrote:
Peter,
You're right, that u64 cast wasn't needed, but I needed to cast the result to u32. The (void*) is evidently expecting a u32. Is that correct? Would SeaBIOS always be expecting a u32 for an address to execute from?
I also cleaned up the macro to use the existing ntohl. The new patch is below. I also tested it.
Thanks for catching this. I think it would be worthwhile to clean up the byteorder calls - see a patch I've sent in a separate email.
-Kevin
Kevin,
Thanks, your patch works for me.
Dave
----- Original Message -----
From: "Kevin O'Connor" kevin@koconnor.net To: "Dave Frodin" dave.frodin@se-eng.com Cc: "Peter Stuge" peter@stuge.se, seabios@seabios.org Sent: Tuesday, August 14, 2012 7:20:01 PM Subject: Re: [SeaBIOS] The cbfs header for a payloads dest addr is a u64, use ntohll instead of ntohl
On Wed, Aug 08, 2012 at 12:07:50PM -0600, Dave Frodin wrote:
Peter,
You're right, that u64 cast wasn't needed, but I needed to cast the result to u32. The (void*) is evidently expecting a u32. Is that correct? Would SeaBIOS always be expecting a u32 for an address to execute from?
I also cleaned up the macro to use the existing ntohl. The new patch is below. I also tested it.
Thanks for catching this. I think it would be worthwhile to clean up the byteorder calls - see a patch I've sent in a separate email.
-Kevin
-
Dave Frodin -
Kevin O'Connor -
Peter Stuge