Change in coreboot[master]: amdfwtool: Get size and location of BIOSBIN

[Bao Zheng (Code Review)]

Hello Zheng Bao,

I'd like you to do a code review. Please visit

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

to review the following change.

Change subject: amdfwtool: Get size and location of BIOSBIN
......................................................................

amdfwtool: Get size and location of BIOSBIN

BUG=b:154957411
TEST=Build & boot on mandolin

Change-Id: I5a26047726f897c57325387cb304fddbc73f6504
Signed-off-by: Zheng Bao 
---
M src/soc/amd/picasso/Makefile.inc
1 file changed, 2 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/46092/1

diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc
index 1e9ba4a..c6bd657 100644
--- a/src/soc/amd/picasso/Makefile.inc
+++ b/src/soc/amd/picasso/Makefile.inc
@@ -208,10 +208,9 @@
 PSP_ELF_FILE=$(objcbfs)/bootblock.elf
 # TODO(b/154957411): Refactor amdfwtool to extract the address and size from
 # the elf file.
-PSP_BIOSBIN_SIZE=$(CONFIG_C_ENV_BOOTBLOCK_SIZE)
+PSP_BIOSBIN_SIZE=$(shell $(READELF_bootblock) -l $(PSP_ELF_FILE) | grep LOAD | awk '{print $$5}')
 # This address must match the BOOTBLOCK logic in arch/x86/memlayout.ld.
-PSP_BIOSBIN_DEST=$(shell printf "%x" $(call int-subtract, $(call int-add, $(CONFIG_X86_RESET_VECTOR) 0x10) $(PSP_BIOSBIN_SIZE)))
-
+PSP_BIOSBIN_DEST=$(shell $(READELF_bootblock) -l $(PSP_ELF_FILE) | grep LOAD | awk '{print $$3}')
 # type = 0x63 - construct APOB NV base/size from flash map
 # The flashmap section used for this is expected to be named RW_MRC_CACHE
 APOB_NV_SIZE=$(shell grep "FMAP_SECTION_RW_MRC_CACHE_SIZE" $(obj)/fmap_config.h | awk '{print $$(NF)}')

[Justin Frodsham (Code Review)]

Justin Frodsham has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46092 )

Change subject: amdfwtool: Get size and location of BIOSBIN
......................................................................

Patch Set 1: Code-Review+1

[Marshall Dawson (Code Review)]

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46092 )

Change subject: amdfwtool: Get size and location of BIOSBIN
......................................................................

Patch Set 1:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/46092/1//COMMIT_MSG@7 
PS1, Line 7: amdfwtool
Change to
  soc/amd/picasso: Use readelf to find bootblock size and location

https://review.coreboot.org/c/coreboot/+/46092/1/src/soc/amd/picasso/Makefil... 
File src/soc/amd/picasso/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/46092/1/src/soc/amd/picasso/Makefil... 
PS1, Line 212: # This address must match the BOOTBLOCK logic in arch/x86/memlayout.ld.
This comment may be uninteresting now, so maybe remove it.  Since we're getting the info from the elf file, it should be guaranteed to match memlayout.ld.

Change in coreboot[master]: soc/amd/picasso: Use readelf to find bootblock size and location

[Bao Zheng (Code Review)]

Hello Justin Frodsham, build bot (Jenkins), Patrick Georgi, Martin Roth, Marshall Dawson, Dave Frodin, Zheng Bao, Justin Frodsham, Felix Held,

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

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

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

Change subject: soc/amd/picasso: Use readelf to find bootblock size and location
......................................................................

soc/amd/picasso: Use readelf to find bootblock size and location

The output of "readelf -l bootblock.elf" is as below.
------------------
Elf file type is EXEC (Executable file)
Entry point 0x203fff0
There is 1 program header, starting at offset 52

Program Headers:
  Type     Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  LOAD     0x001000 0x02030000 0x02030000 0x10000 0x10000 RWE 0x1000

Section to Segment mapping:
  Segment Sections...
   00     .text .data .bss .reset
------------------
We can extract the information from here.

BUG=b:154957411
TEST=Build & boot on mandolin

Change-Id: I5a26047726f897c57325387cb304fddbc73f6504
Signed-off-by: Zheng Bao 
---
M src/soc/amd/picasso/Makefile.inc
1 file changed, 2 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/46092/2

Change in coreboot[master]: soc/amd/picasso: Use readelf to find bootblock size and location

[Bao Zheng (Code Review)]

Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46092 )

Change subject: soc/amd/picasso: Use readelf to find bootblock size and location
......................................................................

Patch Set 2:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/46092/1//COMMIT_MSG@7 
PS1, Line 7: amdfwtool
> Change to […]

Done

https://review.coreboot.org/c/coreboot/+/46092/1/src/soc/amd/picasso/Makefil... 
File src/soc/amd/picasso/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/46092/1/src/soc/amd/picasso/Makefil... 
PS1, Line 212: # This address must match the BOOTBLOCK logic in arch/x86/memlayout.ld.
> This comment may be uninteresting now, so maybe remove it. […]

Done. Removed.

Change in coreboot[master]: soc/amd/picasso: Use readelf to find bootblock size and location

[Paul Menzel (Code Review)]

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

Change subject: soc/amd/picasso: Use readelf to find bootblock size and location
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/46092/2//COMMIT_MSG@8 
PS2, Line 8: 
Please summarize the problem first.

Change in coreboot[master]: soc/amd/picasso: Use readelf to find bootblock size and location

[Marshall Dawson (Code Review)]

Marshall Dawson has uploaded a new patch set (#3) to the change originally created by Bao Zheng. ( https://review.coreboot.org/c/coreboot/+/46092 )

Change subject: soc/amd/picasso: Use readelf to find bootblock size and location
......................................................................

soc/amd/picasso: Use readelf to find bootblock size and location

The Picasso build describes the DRAM region where the PSP places
our bootblock.  Rather than relying on Kconfig values, make the build
more robust by using the actual size and target base address
from the boot block's ELF file.

Sample output of "readelf -l bootblock.elf" is:
------------------
Elf file type is EXEC (Executable file)
Entry point 0x203fff0
There is 1 program header, starting at offset 52

Program Headers:
  Type     Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  LOAD     0x001000 0x02030000 0x02030000 0x10000 0x10000 RWE 0x1000

Section to Segment mapping:
  Segment Sections...
   00     .text .data .bss .reset
------------------
We can extract the information from here.

BUG=b:154957411
TEST=Build & boot on mandolin

Change-Id: I5a26047726f897c57325387cb304fddbc73f6504
Signed-off-by: Zheng Bao 
---
M src/soc/amd/picasso/Makefile.inc
1 file changed, 2 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/46092/3

Change in coreboot[master]: soc/amd/picasso: Use readelf to find bootblock size and location

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46092 )

Change subject: soc/amd/picasso: Use readelf to find bootblock size and location
......................................................................

Patch Set 3: Code-Review+2

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/46092/2//COMMIT_MSG@8 
PS2, Line 8:
> Please summarize the problem first.

Done

Change in coreboot[master]: soc/amd/picasso: Use readelf to find bootblock size and location

[Felix Held (Code Review)]

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46092 )

Change subject: soc/amd/picasso: Use readelf to find bootblock size and location
......................................................................

soc/amd/picasso: Use readelf to find bootblock size and location

The Picasso build describes the DRAM region where the PSP places
our bootblock.  Rather than relying on Kconfig values, make the build
more robust by using the actual size and target base address
from the boot block's ELF file.

Sample output of "readelf -l bootblock.elf" is:
------------------
Elf file type is EXEC (Executable file)
Entry point 0x203fff0
There is 1 program header, starting at offset 52

Program Headers:
  Type     Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  LOAD     0x001000 0x02030000 0x02030000 0x10000 0x10000 RWE 0x1000

Section to Segment mapping:
  Segment Sections...
   00     .text .data .bss .reset
------------------
We can extract the information from here.

BUG=b:154957411
TEST=Build & boot on mandolin

Change-Id: I5a26047726f897c57325387cb304fddbc73f6504
Signed-off-by: Zheng Bao 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/46092
Reviewed-by: Felix Held 
Tested-by: build bot (Jenkins) 
---
M src/soc/amd/picasso/Makefile.inc
1 file changed, 2 insertions(+), 6 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Felix Held: Looks good to me, approved

diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc
index 9757160..514b313 100644
--- a/src/soc/amd/picasso/Makefile.inc
+++ b/src/soc/amd/picasso/Makefile.inc
@@ -209,12 +209,8 @@
 # type = 0x62
 PSP_BIOSBIN_FILE=$(obj)/amd_biospsp.img
 PSP_ELF_FILE=$(objcbfs)/bootblock.elf
-# TODO(b/154957411): Refactor amdfwtool to extract the address and size from
-# the elf file.
-PSP_BIOSBIN_SIZE=$(CONFIG_C_ENV_BOOTBLOCK_SIZE)
-# This address must match the BOOTBLOCK logic in arch/x86/memlayout.ld.
-PSP_BIOSBIN_DEST=$(shell printf "%x" $(call int-subtract, $(call int-add, $(CONFIG_X86_RESET_VECTOR) 0x10) $(PSP_BIOSBIN_SIZE)))
-
+PSP_BIOSBIN_SIZE=$(shell $(READELF_bootblock) -l $(PSP_ELF_FILE) | grep LOAD | awk '{print $$5}')
+PSP_BIOSBIN_DEST=$(shell $(READELF_bootblock) -l $(PSP_ELF_FILE) | grep LOAD | awk '{print $$3}')
 # type = 0x63 - construct APOB NV base/size from flash map
 # The flashmap section used for this is expected to be named RW_MRC_CACHE
 APOB_NV_SIZE=$(shell grep "FMAP_SECTION_RW_MRC_CACHE_SIZE" $(obj)/fmap_config.h | awk '{print $$(NF)}')