Change in coreboot[master]: [WIP]payloads/tianocore: Enable UEFIPayload

[Lance Zhao (Code Review)]

Hello Lijian Zhao,

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

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

to review the following change.

Change subject: [WIP]payloads/tianocore: Enable UEFIPayload
......................................................................

[WIP]payloads/tianocore: Enable UEFIPayload

Corebootpayload package in upstream tianocore had been replaced with
UEFIPayload, make the changes in coreboot side as well.

BUG=N/A
TEST=Able to build with tianocore debug and release build.

Signed-off-by: Lijian Zhao 
Change-Id: I0b7785fde9f4113b2cd91323ac0358b229c5a6e6
---
M payloads/external/tianocore/Makefile
1 file changed, 6 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/34459/1

diff --git a/payloads/external/tianocore/Makefile b/payloads/external/tianocore/Makefile
index f208e59..727aa22 100644
--- a/payloads/external/tianocore/Makefile
+++ b/payloads/external/tianocore/Makefile
@@ -39,9 +39,9 @@
 endif

ifeq ($(CONFIG_TIANOCORE_TARGET_IA32), y)
-	BUILD_STR=-a IA32 -t COREBOOT -p CorebootPayloadPkg/CorebootPayloadPkgIa32.dsc -b $(BUILD_TYPE) $(TIMER)
+	BUILD_STR=-a IA32 -t COREBOOT -p UefiPayloadPkg/UefiPayloadPkgIa32.dsc -b $(BUILD_TYPE) $(TIMER) -D BOOTLOADER=COREBOOT
 else
-	BUILD_STR=-a IA32 -a X64 -t COREBOOT -p CorebootPayloadPkg/CorebootPayloadPkgIa32X64.dsc -b $(BUILD_TYPE) $(TIMER)
+	BUILD_STR=-a IA32 -a X64 -t COREBOOT -p UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc -b $(BUILD_TYPE) $(TIMER) -D BOOTLOADER=COREBOOT
 endif

all: clean build
@@ -85,9 +85,9 @@
    	echo "    Copying custom bootsplash image"; \
    	case "$(CONFIG_TIANOCORE_BOOTSPLASH_FILE)" in \
    		/*) cp $(CONFIG_TIANOCORE_BOOTSPLASH_FILE) \
-				$(project_dir)/CorebootPayloadPkg/Logo/Logo.bmp;; \
+				$(project_dir)/UefiPayloadPkg/Logo/Logo.bmp;; \
    		*) cp $(top)/$(CONFIG_TIANOCORE_BOOTSPLASH_FILE) \
-				$(project_dir)/CorebootPayloadPkg/Logo/Logo.bmp;; \
+				$(project_dir)/UefiPayloadPkg/Logo/Logo.bmp;; \
    	esac \
    fi; \
    cd $(project_dir); \
@@ -99,8 +99,8 @@
    		cat ../tools_def.txt >> $(project_dir)/Conf/tools_def.txt; \
    	fi; \
    	build $(BUILD_STR); \
-		mv $(project_dir)/Build/CorebootPayloadPkg*/*/FV/UEFIPAYLOAD.fd $(project_dir)/Build/UEFIPAYLOAD.fd; \
-		git checkout CorebootPayloadPkg/Logo/Logo.bmp > /dev/null 2>&1 || true
+		mv $(project_dir)/Build/UefiPayloadPkg*/*/FV/UEFIPAYLOAD.fd $(project_dir)/Build/UEFIPAYLOAD.fd; \
+		git checkout UefiPayloadPkg/Logo/Logo.bmp > /dev/null 2>&1 || true

clean:
    test -d $(project_dir) && (cd $(project_dir); rm -rf Build; rm -f Conf/tools_def.txt) || exit 0

[Patrick Rudolph (Code Review)]

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

Change subject: [WIP]payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 1: Code-Review-2

This is not upstream tianocode you are modifying.

[Lance Zhao (Code Review)]

Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34459 )

Change subject: [WIP]payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 1:
> Patch Set 1: Code-Review-2
>
> This is not upstream tianocode you are modifying.

Yes, will make the change base on upstream tianocore or the other one.

[Angel Pons (Code Review)]

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

Change subject: [WIP]payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 1: Code-Review-1

(1 comment)

I recall reading that the UEFIPayload package failed to boot with coreboot, so I would like this to be boot-tested.

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

https://review.coreboot.org/c/coreboot/+/34459/1//COMMIT_MSG@13 
PS1, Line 13: TEST=Able to build with tianocore debug and release build.
What about boot-testing tianocore? I don't think build-testing is enough testing for this change.

[Patrick Rudolph (Code Review)]

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

Change subject: [WIP]payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 1:

Please sync with Matt. We should test the new payload package on every platform we support, before merging it. Build testing isn't sufficient.

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34459 )

Change subject: [WIP]payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 1:

I've not had a chance to boot-test this patch yet, but it is insufficient as-is with a default config due to the default being my repo/my coreboot_fb branch. One would need to change the git revision to upstream/master for this to build even.

Until UefiPayloadPkg reaches feature/functional parity with my existing fork, what might make sense is to add a Kconfig option to select CorebootPayloadPkg (using my repo/branch) or UefiPayloadPkg (using upstream/master) and leave the default as-is

[Arthur Heymans (Code Review)]

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

Change subject: [WIP]payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 1:
> Patch Set 1:
>
> I've not had a chance to boot-test this patch yet, but it is insufficient as-is with a default config due to the default being my repo/my coreboot_fb branch. One would need to change the git revision to upstream/master for this to build even.
>
> Until UefiPayloadPkg reaches feature/functional parity with my existing fork, what might make sense is to add a Kconfig option to select CorebootPayloadPkg (using my repo/branch) or UefiPayloadPkg (using upstream/master) and leave the default as-is

I tested a separately build UefiPayloadPkg (qemu). Specifying the PCIE_BASE was needed and PS2 keyboard did not work. Other than that, it does boot.

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34459 )

Change subject: [WIP]payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 1:
> I tested a separately build UefiPayloadPkg (qemu). Specifying the PCIE_BASE was needed and PS2 keyboard did not work. Other than that, it does boot.

sounds like making it a secondary option would be best then. And maybe we need a way to automatically pass in the PCIE_BASE

[Lance Zhao (Code Review)]

Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34459 )

Change subject: [WIP]payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> >
> > I've not had a chance to boot-test this patch yet, but it is insufficient as-is with a default config due to the default being my repo/my coreboot_fb branch. One would need to change the git revision to upstream/master for this to build even.
> >
> > Until UefiPayloadPkg reaches feature/functional parity with my existing fork, what might make sense is to add a Kconfig option to select CorebootPayloadPkg (using my repo/branch) or UefiPayloadPkg (using upstream/master) and leave the default as-is
>
> I tested a separately build UefiPayloadPkg (qemu). Specifying the PCIE_BASE was needed and PS2 keyboard did not work. Other than that, it does boot.

Okay, PCIE_BASE is needed. Did you get a chance to try on real platform, last time I tried and that can't boot up as well. May I know what you have done to add PCIE_BASE? Thanks.

[Lance Zhao (Code Review)]

Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34459 )

Change subject: [WIP]payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 1:
> Patch Set 1:
>
> I've not had a chance to boot-test this patch yet, but it is insufficient as-is with a default config due to the default being my repo/my coreboot_fb branch. One would need to change the git revision to upstream/master for this to build even.
>
> Until UefiPayloadPkg reaches feature/functional parity with my existing fork, what might make sense is to add a Kconfig option to select CorebootPayloadPkg (using my repo/branch) or UefiPayloadPkg (using upstream/master) and leave the default as-is

https://edk2.groups.io/g/devel/message/39126%EF%BC%8C we can still access CorebootPayloadPkg through EDK2 stable release edk2-stable201903. Earlier I am considering build UefiPayloadPkg only for master branch, though that maybe a problem if people select a git revision that's also newer enough without CorebootPayloadPkg.

[Lance Zhao (Code Review)]

Hello Patrick Rudolph, Angel Pons, Arthur Heymans, Matt DeVillier, build bot (Jenkins), Lijian Zhao, Nico Huber, Patrick Georgi,

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

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

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

Change subject: [WIP]payloads/tianocore: Enable UEFIPayload
......................................................................

[WIP]payloads/tianocore: Enable UEFIPayload

Corebootpayload package in upstream tianocore had been replaced with
UEFIPayload, make the changes in coreboot side as well.

BUG=N/A
TEST=Able to build with tianocore debug and release build.

Signed-off-by: Lijian Zhao 
Change-Id: I0b7785fde9f4113b2cd91323ac0358b229c5a6e6
---
M payloads/external/tianocore/Kconfig
M payloads/external/tianocore/Makefile
2 files changed, 20 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/34459/2

[Lance Zhao (Code Review)]

Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34459 )

Change subject: [WIP]payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 4:

This change is ready for review.

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Lance Zhao (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Matt DeVillier, build bot (Jenkins), Lijian Zhao, Maurice Ma, Nico Huber, Martin Roth, Patrick Georgi,

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

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

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

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

payloads/tianocore: Enable UEFIPayload

corebootpayload package in upstream TianoCore was replaced with
UEFIPayload, add external payload build option for UEFIPayload.

BUG=N/A
TEST=Select TianoCore payload and UEFIPayload Bootloader, set git
revision to origin/master. Then build and able to boot up on QEMU q35
after PCIE_BASE had been set.

Change-Id: I0b7785fde9f4113b2cd91323ac0358b229c5a6e6
Signed-off-by: Lijian Zhao 
---
M payloads/external/Makefile.inc
M payloads/external/tianocore/Kconfig
M payloads/external/tianocore/Makefile
3 files changed, 20 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/34459/5

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Patrick Rudolph (Code Review)]

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

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 5:

(1 comment)
> Patch Set 4:
> > Patch Set 4:
> >
> > Who is going to maintain UEFIpayloadPkg?
> > Why is this even selectable?
>
> I believe the UEFIpayloadPkg will be maintained by tianocore community just like the previously corebootpayloadpkg time.

The tianocore community will maintain this change within coreboot?

https://review.coreboot.org/c/coreboot/+/34459/5//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34459/5//COMMIT_MSG@14 
PS5, Line 14: revision to origin/master. Then build and able to boot up on QEMU q35
UEFIPayload Pkg isn't part of origin/master.

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Lance Zhao (Code Review)]

Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34459 )

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 5:

(1 comment)
> Patch Set 5:
>
> (1 comment)
> > Patch Set 4:
> > > Patch Set 4:
> > >
> > > Who is going to maintain UEFIpayloadPkg?
> > > Why is this even selectable?
> >
> > I believe the UEFIpayloadPkg will be maintained by tianocore community just like the previously corebootpayloadpkg time.
>
> The tianocore community will maintain this change within coreboot?

Nope, just the payload itself but not the implementation. If there's more confusion we can discuss that in slack. Anyway I submit the changes since Zaolin asked, I am totally fine if the change won't be merged as that's "WIP".

https://review.coreboot.org/c/coreboot/+/34459/5//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34459/5//COMMIT_MSG@14 
PS5, Line 14: revision to origin/master. Then build and able to boot up on QEMU q35
> UEFIPayload Pkg isn't part of origin/master.

https://github.com/tianocore/edk2/tree/master/UefiPayloadPkg? Is that the one?

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Paul Menzel (Code Review)]

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

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34459/5//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34459/5//COMMIT_MSG@14 
PS5, Line 14: revision to origin/master. Then build and able to boot up on QEMU q35
> https://github. […]

I guess because currently MrChromebox’ repository is used? https://github.com/mrchromebox/edk2

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Lance Zhao (Code Review)]

Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34459 )

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 5:
> Patch Set 5:
>
> (1 comment)

echo "    Fetching new commits from the $(project_name) repo"; \
    	git fetch --multiple origin upstream 2>/dev/null; \
I think current step will fetch some newer commits as well.

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34459 )

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 5:

(2 comments)

https://review.coreboot.org/c/coreboot/+/34459/5//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34459/5//COMMIT_MSG@14 
PS5, Line 14: origin/master
> origin/master

upstream/master

https://review.coreboot.org/c/coreboot/+/34459/5/payloads/external/tianocore... 
File payloads/external/tianocore/Kconfig:

https://review.coreboot.org/c/coreboot/+/34459/5/payloads/external/tianocore... 
PS5, Line 90: config TIANOCORE_BOOTLOADER
            : 	bool "TianoCore UEFI Payload"
            : 	help
            : 	  UEFIPayload supports coreboot and slimboot.
            : 
IMO, would make more sense to move this to the top and make it a choice between CorebootPayloadPkg and UefiPayloadPkg, replacing the current choice between stable and git revision.

Selecting the former would set the git revision (which could be renamed) to my repo/branch, and expose the Kconfig options below (like the logo) which are specific to CorebootPayloadPkg.

Selecting the latter would set the git revision to upstream/master and hide options not relevant to UefiPayloadPkg.

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Patrick Rudolph (Code Review)]

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

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34459/5/payloads/external/tianocore... 
File payloads/external/tianocore/Kconfig:

https://review.coreboot.org/c/coreboot/+/34459/5/payloads/external/tianocore... 
PS5, Line 90: config TIANOCORE_BOOTLOADER
            : 	bool "TianoCore UEFI Payload"
            : 	help
            : 	  UEFIPayload supports coreboot and slimboot.
            :
> IMO, would make more sense to move this to the top and make it a choice between CorebootPayloadPkg a […]

I still don't see why we should add an unmaintained bootloader-selection that only works with upstream. Upstream was broken quite often, that's why we switched to a stable branch.
The best decision would be to integrate UefiPayloadPkg into origin and once stable and we'll tested it can be merged into origin.

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34459 )

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34459/5/payloads/external/tianocore... 
File payloads/external/tianocore/Kconfig:

https://review.coreboot.org/c/coreboot/+/34459/5/payloads/external/tianocore... 
PS5, Line 90: config TIANOCORE_BOOTLOADER
            : 	bool "TianoCore UEFI Payload"
            : 	help
            : 	  UEFIPayload supports coreboot and slimboot.
            :
> I still don't see why we should add an unmaintained bootloader-selection that only works with upstream. Upstream was broken quite often, that's why we switched to a stable branch.

I think the argument could be made that if it's not easy to test UefiPayloadPkg via coreboot's build system, then the likelihood of it being properly maintained drops significantly.

If we have two payload options then it makes it easier to test, and we can switch the default to UefiPayloadPkg once it reaches the point to where it's a better option

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Lance Zhao (Code Review)]

Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34459 )

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 5:

(1 comment)
> Patch Set 5:
>
> (1 comment)

https://review.coreboot.org/c/coreboot/+/34459/5/payloads/external/tianocore... 
File payloads/external/tianocore/Kconfig:

https://review.coreboot.org/c/coreboot/+/34459/5/payloads/external/tianocore... 
PS5, Line 90: config TIANOCORE_BOOTLOADER
            : 	bool "TianoCore UEFI Payload"
            : 	help
            : 	  UEFIPayload supports coreboot and slimboot.
            :
> > I still don't see why we should add an unmaintained bootloader-selection that only works with upst […]

Ack

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Patrick Rudolph (Code Review)]

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

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 5: Code-Review-1

(1 comment)

https://review.coreboot.org/c/coreboot/+/34459/5/payloads/external/tianocore... 
File payloads/external/tianocore/Kconfig:

https://review.coreboot.org/c/coreboot/+/34459/5/payloads/external/tianocore... 
PS5, Line 90: config TIANOCORE_BOOTLOADER
            : 	bool "TianoCore UEFI Payload"
            : 	help
            : 	  UEFIPayload supports coreboot and slimboot.
            :
> Ack

That makes sense.
Please update the help message explaining how to select the right bootloader, depending on the selected repo/commit.

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Lance Zhao (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Matt DeVillier, build bot (Jenkins), Lijian Zhao, Maurice Ma, Nico Huber, Martin Roth, Patrick Georgi,

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

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

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

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

payloads/tianocore: Enable UEFIPayload

corebootpayload package in upstream TianoCore was replaced with
UEFIPayload, add external payload build option for UEFIPayload.

BUG=N/A
TEST=Select TianoCore payload as UEFIPayload, build and able to boot up on
QEMU q35 after PCIE_BASE set.

Change-Id: I0b7785fde9f4113b2cd91323ac0358b229c5a6e6
Signed-off-by: Lijian Zhao 
---
M payloads/external/Makefile.inc
M payloads/external/tianocore/Kconfig
M payloads/external/tianocore/Makefile
3 files changed, 39 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/34459/6

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Lance Zhao (Code Review)]

Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34459 )

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34459/5/payloads/external/tianocore... 
File payloads/external/tianocore/Kconfig:

https://review.coreboot.org/c/coreboot/+/34459/5/payloads/external/tianocore... 
PS5, Line 90: config TIANOCORE_BOOTLOADER
            : 	bool "TianoCore UEFI Payload"
            : 	help
            : 	  UEFIPayload supports coreboot and slimboot.
            :
> That makes sense. […]

Done

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34459 )

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 6:

(2 comments)

https://review.coreboot.org/c/coreboot/+/34459/6/payloads/external/tianocore... 
File payloads/external/tianocore/Kconfig:

https://review.coreboot.org/c/coreboot/+/34459/6/payloads/external/tianocore... 
PS6, Line 32: payload
why change this from 'version' to 'payload,' duplicating the choice above?

https://review.coreboot.org/c/coreboot/+/34459/6/payloads/external/tianocore... 
PS6, Line 34: TIANOCORE_COREBOOTPAYLOAD
would it not be useful to be able to test a specific commit for UefiPayload as well?

Maybe we should rework this to have two options: master and revision, that apply for both Tianocore payloads - just point to different branches?

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Lance Zhao (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Matt DeVillier, build bot (Jenkins), Lijian Zhao, Maurice Ma, Nico Huber, Martin Roth, Patrick Georgi,

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

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

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

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

payloads/tianocore: Enable UEFIPayload

corebootpayload package in upstream TianoCore was replaced with
UEFIPayload, add external payload build option for UEFIPayload.

BUG=N/A
TEST=Select TianoCore payload as UEFIPayload, build and able to boot up on
QEMU q35 after PCIE_BASE set.

Change-Id: I0b7785fde9f4113b2cd91323ac0358b229c5a6e6
Signed-off-by: Lijian Zhao 
---
M payloads/external/Makefile.inc
M payloads/external/tianocore/Kconfig
M payloads/external/tianocore/Kconfig.name
M payloads/external/tianocore/Makefile
4 files changed, 40 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/34459/7

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Lance Zhao (Code Review)]

Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34459 )

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 7:

(2 comments)

https://review.coreboot.org/c/coreboot/+/34459/6/payloads/external/tianocore... 
File payloads/external/tianocore/Kconfig:

https://review.coreboot.org/c/coreboot/+/34459/6/payloads/external/tianocore... 
PS6, Line 32: payload
> why change this from 'version' to 'payload,' duplicating the choice above?

Done

https://review.coreboot.org/c/coreboot/+/34459/6/payloads/external/tianocore... 
PS6, Line 34: TIANOCORE_COREBOOTPAYLOAD
> would it not be useful to be able to test a specific commit for UefiPayload as well? […]

Then there's risk if people choose revision earlier than retirement of corebootpayload package will cause a build break.

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34459 )

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34459/6/payloads/external/tianocore... 
File payloads/external/tianocore/Kconfig:

https://review.coreboot.org/c/coreboot/+/34459/6/payloads/external/tianocore... 
PS6, Line 34: TIANOCORE_COREBOOTPAYLOAD
> Then there's risk if people choose revision earlier than retirement of corebootpayload package will  […]

AFAIK, picking a random revision of Tianocore most often results in breakage
(build or runtime) no matter if the code is there. So this is rather a question
if we should offer an option to choose the revision at all (IMHO, no. but this
should be discussed separately, I guess).

Btw. the same argument can be made for CorebootPayloadPkg: if people choose a
revision after retirement, it will cause a build break.

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Lance Zhao (Code Review)]

Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34459 )

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34459/6/payloads/external/tianocore... 
File payloads/external/tianocore/Kconfig:

https://review.coreboot.org/c/coreboot/+/34459/6/payloads/external/tianocore... 
PS6, Line 34: TIANOCORE_COREBOOTPAYLOAD
> AFAIK, picking a random revision of Tianocore most often results in breakage […]

That's true, if people choose other than coreboot_fb branch (like origin/master), then build will break because there's no gop driver.

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34459 )

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 7:

(2 comments)

https://review.coreboot.org/c/coreboot/+/34459/7//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34459/7//COMMIT_MSG@14 
PS7, Line 14: after PCIE_BASE set
any reason not to pass this in as a command line param since it's required for UefiPayloadPkg to boot?

https://review.coreboot.org/c/coreboot/+/34459/6/payloads/external/tianocore... 
File payloads/external/tianocore/Kconfig:

https://review.coreboot.org/c/coreboot/+/34459/6/payloads/external/tianocore... 
PS6, Line 34: TIANOCORE_COREBOOTPAYLOAD
> That's true, if people choose other than coreboot_fb branch (like origin/master), then build will br […]

for UefiPayloadPkg, offering the option to use a specific commit provides the user a workaround when upstream master inevitably breaks.

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Lance Zhao (Code Review)]

Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34459 )

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34459/6/payloads/external/tianocore... 
File payloads/external/tianocore/Kconfig:

https://review.coreboot.org/c/coreboot/+/34459/6/payloads/external/tianocore... 
PS6, Line 34: TIANOCORE_COREBOOTPAYLOAD
> for UefiPayloadPkg, offering the option to use a specific commit provides the user a workaround when […]

So that's not needed for current corebootpayloadpkg?

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Lance Zhao (Code Review)]

Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34459 )

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34459/7//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34459/7//COMMIT_MSG@14 
PS7, Line 14: after PCIE_BASE set
> any reason not to pass this in as a command line param since it's required for UefiPayloadPkg to boo […]

Because MMCONF_BASE_ADDRESS is not always set

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Lance Zhao (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Matt DeVillier, build bot (Jenkins), Lijian Zhao, Nico Huber, Maurice Ma, Martin Roth, Patrick Georgi,

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

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

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

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

payloads/tianocore: Enable UEFIPayload

corebootpayload package in upstream TianoCore was replaced with
UEFIPayload, add external payload build option for UEFIPayload.

BUG=N/A
TEST=Select TianoCore payload as UEFIPayload, build and able to boot up on
QEMU q35 after PCIE_BASE set.

Change-Id: I0b7785fde9f4113b2cd91323ac0358b229c5a6e6
Signed-off-by: Lijian Zhao 
---
M payloads/external/Makefile.inc
M payloads/external/tianocore/Kconfig
M payloads/external/tianocore/Kconfig.name
M payloads/external/tianocore/Makefile
4 files changed, 43 insertions(+), 15 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/34459/8

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Lance Zhao (Code Review)]

Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34459 )

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 8:

(2 comments)

https://review.coreboot.org/c/coreboot/+/34459/7//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34459/7//COMMIT_MSG@14 
PS7, Line 14: after PCIE_BASE set
> Because MMCONF_BASE_ADDRESS is not always set

Done

https://review.coreboot.org/c/coreboot/+/34459/6/payloads/external/tianocore... 
File payloads/external/tianocore/Kconfig:

https://review.coreboot.org/c/coreboot/+/34459/6/payloads/external/tianocore... 
PS6, Line 34: TIANOCORE_COREBOOTPAYLOAD
> So that's not needed for current corebootpayloadpkg?

Done

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34459 )

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 9:

(5 comments)

https://review.coreboot.org/c/coreboot/+/34459/9//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34459/9//COMMIT_MSG@14 
PS9, Line 14:  PCIE_BASE set.
> Because MMCONF_BASE_ADDRESS is not always set

but if it is, we need to pass it in/set it for UefiPayloadPkg to work, no?
we can't have the default cause be non-working for the majority of devices likely to use the payload

https://review.coreboot.org/c/coreboot/+/34459/9/payloads/external/tianocore... 
File payloads/external/tianocore/Kconfig:

https://review.coreboot.org/c/coreboot/+/34459/9/payloads/external/tianocore... 
PS9, Line 27: select TIANOCORE_REVISION
change to newly-added TIANOCORE_MASTER per below?

https://review.coreboot.org/c/coreboot/+/34459/9/payloads/external/tianocore... 
PS9, Line 39: 
I'd add another option here:

config TIANOCORE_MASTER
    bool "master"
    depends on TIANOCORE_UEFIPAYLOAD

https://review.coreboot.org/c/coreboot/+/34459/9/payloads/external/tianocore... 
PS9, Line 42: depends on TIANOCORE_UEFIPAYLOAD
this should be removed, both payload options should be able to set a specific revision

https://review.coreboot.org/c/coreboot/+/34459/9/payloads/external/tianocore... 
PS9, Line 45: from MrChromebox's tree
remove this since either tree/any branch can be used

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Lance Zhao (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Matt DeVillier, build bot (Jenkins), Lijian Zhao, Nico Huber, Maurice Ma, Martin Roth, Patrick Georgi,

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

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

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

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

payloads/tianocore: Enable UEFIPayload

corebootpayload package in upstream TianoCore was replaced with
UEFIPayload, add external payload build option for UEFIPayload.

BUG=N/A
TEST=Select TianoCore payload as UEFIPayload, build and able to boot up on
QEMU q35 after PCIE_BASE set.

Change-Id: I0b7785fde9f4113b2cd91323ac0358b229c5a6e6
Signed-off-by: Lijian Zhao 
---
M payloads/external/Makefile.inc
M payloads/external/tianocore/Kconfig
M payloads/external/tianocore/Kconfig.name
M payloads/external/tianocore/Makefile
4 files changed, 46 insertions(+), 20 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/34459/10

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Lance Zhao (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Matt DeVillier, build bot (Jenkins), Lijian Zhao, Nico Huber, Maurice Ma, Martin Roth, Patrick Georgi,

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

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

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

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

payloads/tianocore: Enable UEFIPayload

corebootpayload package in upstream TianoCore was replaced with
UEFIPayload, add external payload build option for UEFIPayload.

BUG=N/A
TEST=Select TianoCore payload as UEFIPayload, build and able to boot up on
QEMU q35 after PCIE_BASE set.

Change-Id: I0b7785fde9f4113b2cd91323ac0358b229c5a6e6
Signed-off-by: Lijian Zhao 
---
M payloads/external/Makefile.inc
M payloads/external/tianocore/Kconfig
M payloads/external/tianocore/Kconfig.name
M payloads/external/tianocore/Makefile
4 files changed, 48 insertions(+), 21 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/34459/11

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Lance Zhao (Code Review)]

Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34459 )

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 11:

(4 comments)

https://review.coreboot.org/c/coreboot/+/34459/9//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34459/9//COMMIT_MSG@14 
PS9, Line 14:  PCIE_BASE set.
> > Because MMCONF_BASE_ADDRESS is not always set […]

Done

https://review.coreboot.org/c/coreboot/+/34459/9/payloads/external/tianocore... 
File payloads/external/tianocore/Kconfig:

https://review.coreboot.org/c/coreboot/+/34459/9/payloads/external/tianocore... 
PS9, Line 27: select TIANOCORE_REVISION
> change to newly-added TIANOCORE_MASTER per below?

Done

https://review.coreboot.org/c/coreboot/+/34459/9/payloads/external/tianocore... 
PS9, Line 39:
> I'd add another option here: […]

Done

https://review.coreboot.org/c/coreboot/+/34459/9/payloads/external/tianocore... 
PS9, Line 45: from MrChromebox's tree
> remove this since either tree/any branch can be used

Done

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Lance Zhao (Code Review)]

Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34459 )

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 11:

(4 comments)

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

https://review.coreboot.org/c/coreboot/+/34459/1//COMMIT_MSG@13 
PS1, Line 13: TEST=Able to build with tianocore debug and release build.
> What about boot-testing tianocore? I don't think build-testing is enough testing for this change.

Done

https://review.coreboot.org/c/coreboot/+/34459/5//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34459/5//COMMIT_MSG@14 
PS5, Line 14: origin/master
> > origin/master […]

Done

https://review.coreboot.org/c/coreboot/+/34459/5//COMMIT_MSG@14 
PS5, Line 14: revision to origin/master. Then build and able to boot up on QEMU q35
> I guess because currently MrChromebox’ repository is used? https://github. […]

Done

https://review.coreboot.org/c/coreboot/+/34459/9/payloads/external/tianocore... 
File payloads/external/tianocore/Kconfig:

https://review.coreboot.org/c/coreboot/+/34459/9/payloads/external/tianocore... 
PS9, Line 42: depends on TIANOCORE_UEFIPAYLOAD
> this should be removed, both payload options should be able to set a specific revision

Done

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34459 )

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 11:

(5 comments)

https://review.coreboot.org/c/coreboot/+/34459/11/payloads/external/Makefile... 
File payloads/external/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/34459/11/payloads/external/Makefile... 
PS11, Line 131: CONFIG_TIANOCORE_MASTER=$(CONFIG_TIANOCORE_MASTER) \
              : 		CONFIG_TIANOCORE_STABLE=$(CONFIG_TIANOCORE_STABLE) \
              : 		CONFIG_TIANOCORE_REVISION=$(CONFIG_TIANOCORE_REVISION) \
drop these

https://review.coreboot.org/c/coreboot/+/34459/11/payloads/external/tianocor... 
File payloads/external/tianocore/Kconfig:

https://review.coreboot.org/c/coreboot/+/34459/11/payloads/external/tianocor... 
PS11, Line 33: config TIANOCORE_STABLE
             : 	bool "stable"
             : 	depends on TIANOCORE_COREBOOTPAYLOAD
             : 	help
             : 	  Select this option to build using MrChromebox's custom Tianocore tree
             : 	  i.e. a version of Tianocore that builds without any errors and just works.
             : 
             : config TIANOCORE_MASTER
             : 	bool "master"
             : 	depends on TIANOCORE_UEFIPAYLOAD
             : 	help
             : 	  Select this option if you have a specific commit or branch
             : 	  that you want to build Tianocore.
             : 
             : 	  You will be able to specify the name of a branch or a commit id
             : 	  later.
we can actually drop these, since not being used in a choice, and default branches are set in the makefile based on payload selection

https://review.coreboot.org/c/coreboot/+/34459/11/payloads/external/tianocor... 
PS11, Line 105: depends on TIANOCORE_STABLE
drop here too

https://review.coreboot.org/c/coreboot/+/34459/11/payloads/external/tianocor... 
File payloads/external/tianocore/Makefile:

https://review.coreboot.org/c/coreboot/+/34459/11/payloads/external/tianocor... 
PS11, Line 26: bootloader=UefiPayloadPkg
             : build_flavor=-D BOOTLOADER=COREBOOT -D PCIE_BASE=$(CONFIG_MMCONF_BASE_ADDRESS)
add:
tag-y=upstream/master

https://review.coreboot.org/c/coreboot/+/34459/11/payloads/external/tianocor... 
PS11, Line 31: TAG-$(CONFIG_TIANOCORE_STABLE)=origin/$(project_git_branch)
change to tag-y

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Lance Zhao (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, Matt DeVillier, build bot (Jenkins), Lijian Zhao, Nico Huber, Maurice Ma, Martin Roth, Patrick Georgi,

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

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

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

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

payloads/tianocore: Enable UEFIPayload

corebootpayload package in upstream TianoCore was replaced with
UEFIPayload, add external payload build option for UEFIPayload.

BUG=N/A
TEST=Select TianoCore payload as UEFIPayload, build and able to boot up on
QEMU q35 after PCIE_BASE set.

Change-Id: I0b7785fde9f4113b2cd91323ac0358b229c5a6e6
Signed-off-by: Lijian Zhao 
---
M payloads/external/Makefile.inc
M payloads/external/tianocore/Kconfig
M payloads/external/tianocore/Kconfig.name
M payloads/external/tianocore/Makefile
4 files changed, 38 insertions(+), 33 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/34459/12

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Lance Zhao (Code Review)]

Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34459 )

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 12:

(5 comments)

https://review.coreboot.org/c/coreboot/+/34459/11/payloads/external/Makefile... 
File payloads/external/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/34459/11/payloads/external/Makefile... 
PS11, Line 131: CONFIG_TIANOCORE_MASTER=$(CONFIG_TIANOCORE_MASTER) \
              : 		CONFIG_TIANOCORE_STABLE=$(CONFIG_TIANOCORE_STABLE) \
              : 		CONFIG_TIANOCORE_REVISION=$(CONFIG_TIANOCORE_REVISION) \
> drop these

Done

https://review.coreboot.org/c/coreboot/+/34459/11/payloads/external/tianocor... 
File payloads/external/tianocore/Kconfig:

https://review.coreboot.org/c/coreboot/+/34459/11/payloads/external/tianocor... 
PS11, Line 33: config TIANOCORE_STABLE
             : 	bool "stable"
             : 	depends on TIANOCORE_COREBOOTPAYLOAD
             : 	help
             : 	  Select this option to build using MrChromebox's custom Tianocore tree
             : 	  i.e. a version of Tianocore that builds without any errors and just works.
             : 
             : config TIANOCORE_MASTER
             : 	bool "master"
             : 	depends on TIANOCORE_UEFIPAYLOAD
             : 	help
             : 	  Select this option if you have a specific commit or branch
             : 	  that you want to build Tianocore.
             : 
             : 	  You will be able to specify the name of a branch or a commit id
             : 	  later.
> we can actually drop these, since not being used in a choice, and default branches are set in the ma […]

Done

https://review.coreboot.org/c/coreboot/+/34459/11/payloads/external/tianocor... 
PS11, Line 105: depends on TIANOCORE_STABLE
> drop here too

Done

https://review.coreboot.org/c/coreboot/+/34459/11/payloads/external/tianocor... 
File payloads/external/tianocore/Makefile:

https://review.coreboot.org/c/coreboot/+/34459/11/payloads/external/tianocor... 
PS11, Line 26: bootloader=UefiPayloadPkg
             : build_flavor=-D BOOTLOADER=COREBOOT -D PCIE_BASE=$(CONFIG_MMCONF_BASE_ADDRESS)
> add: […]

Done

https://review.coreboot.org/c/coreboot/+/34459/11/payloads/external/tianocor... 
PS11, Line 31: TAG-$(CONFIG_TIANOCORE_STABLE)=origin/$(project_git_branch)
> change to tag-y

I changed to tag directly, seems there's no more need for -y anymore as that's always selected.

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34459 )

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 12: Code-Review+1

(1 comment)

boot tested on google/cyan
(though payload itself has quite a few issues, unrelated to this patch)

https://review.coreboot.org/c/coreboot/+/34459/11/payloads/external/tianocor... 
File payloads/external/tianocore/Makefile:

https://review.coreboot.org/c/coreboot/+/34459/11/payloads/external/tianocor... 
PS11, Line 31: TAG-$(CONFIG_TIANOCORE_STABLE)=origin/$(project_git_branch)
> I changed to tag directly, seems there's no more need for -y anymore as that's always selected.

Ack

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Philipp Deppenwiese (Code Review)]

Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34459 )

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

Patch Set 13: Code-Review+2

Change in coreboot[master]: payloads/tianocore: Enable UEFIPayload

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34459 )

Change subject: payloads/tianocore: Enable UEFIPayload
......................................................................

payloads/tianocore: Enable UEFIPayload

corebootpayload package in upstream TianoCore was replaced with
UEFIPayload, add external payload build option for UEFIPayload.

BUG=N/A
TEST=Select TianoCore payload as UEFIPayload, build and able to boot up on
QEMU q35 after PCIE_BASE set.

Change-Id: I0b7785fde9f4113b2cd91323ac0358b229c5a6e6
Signed-off-by: Lijian Zhao 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/34459
Reviewed-by: Philipp Deppenwiese 
Reviewed-by: Matt DeVillier 
Tested-by: build bot (Jenkins) 
---
M payloads/external/Makefile.inc
M payloads/external/tianocore/Kconfig
M payloads/external/tianocore/Kconfig.name
M payloads/external/tianocore/Makefile
4 files changed, 38 insertions(+), 33 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Philipp Deppenwiese: Looks good to me, approved
  Matt DeVillier: Looks good to me, but someone else must approve

diff --git a/payloads/external/Makefile.inc b/payloads/external/Makefile.inc
index df168f2..d5c4e7f 100644
--- a/payloads/external/Makefile.inc
+++ b/payloads/external/Makefile.inc
@@ -128,14 +128,14 @@
    $(MAKE) -C payloads/external/tianocore all \
    	HOSTCC="$(HOSTCC)" \
    	CC="$(HOSTCC)" \
-		CONFIG_TIANOCORE_MASTER=$(CONFIG_TIANOCORE_MASTER) \
-		CONFIG_TIANOCORE_STABLE=$(CONFIG_TIANOCORE_STABLE) \
-		CONFIG_TIANOCORE_REVISION=$(CONFIG_TIANOCORE_REVISION) \
    	CONFIG_TIANOCORE_REVISION_ID=$(CONFIG_TIANOCORE_REVISION_ID) \
    	CONFIG_TIANOCORE_DEBUG=$(CONFIG_TIANOCORE_DEBUG) \
    	CONFIG_TIANOCORE_TARGET_IA32=$(CONFIG_TIANOCORE_TARGET_IA32) \
    	CONFIG_TIANOCORE_USE_8254_TIMER=$(CONFIG_TIANOCORE_USE_8254_TIMER) \
    	CONFIG_TIANOCORE_BOOTSPLASH_FILE=$(CONFIG_TIANOCORE_BOOTSPLASH_FILE) \
+		CONFIG_TIANOCORE_UEFIPAYLOAD=$(CONFIG_TIANOCORE_UEFIPAYLOAD) \
+		CONFIG_TIANOCORE_COREBOOTPAYLOAD=$(CONFIG_TIANOCORE_COREBOOTPAYLOAD) \
+		CONFIG_MMCONF_BASE_ADDRESS=$(CONFIG_MMCONF_BASE_ADDRESS) \
    	GCC_CC_x86_32=$(GCC_CC_x86_32) \
    	GCC_CC_x86_64=$(GCC_CC_x86_64) \
    	GCC_CC_arm=$(GCC_CC_arm) \
diff --git a/payloads/external/tianocore/Kconfig b/payloads/external/tianocore/Kconfig
index d8b7d92..7717917 100644
--- a/payloads/external/tianocore/Kconfig
+++ b/payloads/external/tianocore/Kconfig
@@ -7,38 +7,32 @@
      The result of a corebootPkg build

choice
-	prompt "Tianocore version"
-	default TIANOCORE_STABLE
+	prompt "Tianocore payload"
+	default TIANOCORE_COREBOOTPAYLOAD
    help
-	  Select which version of Tianocore to build (default is to build stable)
-	  stable: MrChromebox's customized version of Tianocore which works on most
+	  Select which type of payload Tianocore will build (default is CorebootPayload)
+	  CorebootPayload: MrChromebox's customized version of Tianocore which works on most
      (all?) x86_64 devices
-	  revision: use specific commit or branch to build Tianocore (specified by user)
+	  UEFIPayload: Use upstream Tianocore payload from https://github.com/tianocore/edk2

-config TIANOCORE_STABLE
-	bool "stable"
+config TIANOCORE_COREBOOTPAYLOAD
+	bool "CorebootPayload"
    help
      Select this option to build using MrChromebox's custom Tianocore tree
      i.e. a version of Tianocore that builds without any errors and just works.

-config TIANOCORE_REVISION
-	bool "git revision"
+config TIANOCORE_UEFIPAYLOAD
+	bool "UEFIPayload"
    help
-	  Select this option if you have a specific commit or branch
-	  that you want to use from either MrChromebox's tree or upstream
-	  EDK2 from which to build Tianocore.
-
-	  You will be able to specify the name of a branch or a commit id
-	  later.
+	  Select this option if you want to use upstream EDK2 to build Tianocore.

endchoice

config TIANOCORE_REVISION_ID
    string "Insert a commit's SHA-1 or a branch name"
-	depends on TIANOCORE_REVISION
-	default "upstream/master"
    help
-	   The commit's SHA-1 or branch name of the revision to use.
+	   The commit's SHA-1 or branch name of the revision to use. Choose "upstream/master"
+	   for master branch of Tianocore release on github.

choice
    prompt "Target architecture"
@@ -89,7 +83,7 @@

config TIANOCORE_BOOTSPLASH_IMAGE
    bool "Use a custom bootsplash image"
-	depends on TIANOCORE_STABLE
+	depends on TIANOCORE_COREBOOTPAYLOAD
    help
      Select this option if you have a bootsplash image that you would
      like to be used. If this option is not selected, the default
@@ -98,6 +92,7 @@
 config TIANOCORE_BOOTSPLASH_FILE
    string "Tianocore Bootsplash path and filename"
    depends on TIANOCORE_BOOTSPLASH_IMAGE
+	depends on TIANOCORE_COREBOOTPAYLOAD
    default "bootsplash.bmp"
    help
      The path and filename of the file to use as graphical bootsplash
diff --git a/payloads/external/tianocore/Kconfig.name b/payloads/external/tianocore/Kconfig.name
index a56d248..7b0b300 100644
--- a/payloads/external/tianocore/Kconfig.name
+++ b/payloads/external/tianocore/Kconfig.name
@@ -1,5 +1,5 @@
 config PAYLOAD_TIANOCORE
-	bool "Tianocore coreboot payload package"
+	bool "Tianocore payload"
    depends on ARCH_X86
    help
      Select this option if you want to build a coreboot image
diff --git a/payloads/external/tianocore/Makefile b/payloads/external/tianocore/Makefile
index f208e59..7adb700 100644
--- a/payloads/external/tianocore/Makefile
+++ b/payloads/external/tianocore/Makefile
@@ -22,9 +22,19 @@
 project_git_branch=coreboot_fb
 upstream_git_repo=https://github.com/tianocore/edk2

+ifeq ($(CONFIG_TIANOCORE_UEFIPAYLOAD),y)
+bootloader=UefiPayloadPkg
+build_flavor=-D BOOTLOADER=COREBOOT -D PCIE_BASE=$(CONFIG_MMCONF_BASE_ADDRESS)
+TAG=upstream/master
+else
+bootloader=CorebootPayloadPkg
 # STABLE revision is MrChromebox's coreboot framebuffer (coreboot_fb) branch
-TAG-$(CONFIG_TIANOCORE_STABLE)=origin/$(project_git_branch)
-TAG-$(CONFIG_TIANOCORE_REVISION)=$(CONFIG_TIANOCORE_REVISION_ID)
+TAG=origin/$(project_git_branch)
+endif
+
+ifneq ($(CONFIG_TIANOCORE_REVISION_ID),)
+TAG=$(CONFIG_TIANOCORE_REVISION_ID)
+endif

export EDK_TOOLS_PATH=$(project_dir)/BaseTools

@@ -39,9 +49,9 @@
 endif

ifeq ($(CONFIG_TIANOCORE_TARGET_IA32), y)
-	BUILD_STR=-a IA32 -t COREBOOT -p CorebootPayloadPkg/CorebootPayloadPkgIa32.dsc -b $(BUILD_TYPE) $(TIMER)
+	BUILD_STR=-a IA32 -t COREBOOT -p $(bootloader)/$(bootloader)Ia32.dsc -b $(BUILD_TYPE) $(TIMER) $(build_flavor)
 else
-	BUILD_STR=-a IA32 -a X64 -t COREBOOT -p CorebootPayloadPkg/CorebootPayloadPkgIa32X64.dsc -b $(BUILD_TYPE) $(TIMER)
+	BUILD_STR=-a IA32 -a X64 -t COREBOOT -p $(bootloader)/$(bootloader)Ia32X64.dsc -b $(BUILD_TYPE) $(TIMER) $(build_flavor)
 endif

all: clean build
@@ -56,13 +66,13 @@
    cd  $(project_dir); \
    	echo "    Fetching new commits from the $(project_name) repo"; \
    	git fetch --multiple origin upstream 2>/dev/null; \
-		if ! git rev-parse --verify -q $(TAG-y) >/dev/null; then \
-			echo "    $(TAG-y) is not a valid git reference"; \
+		if ! git rev-parse --verify -q $(TAG) >/dev/null; then \
+			echo "    $(TAG) is not a valid git reference"; \
    		exit 1; \
    	fi; \
    	if git describe --all --dirty | grep -qv dirty; then \
-			echo "    Checking out $(project_name) revision $(TAG-y)"; \
-			git checkout --detach $(TAG-y); \
+			echo "    Checking out $(project_name) revision $(TAG)"; \
+			git checkout --detach $(TAG); \
    	else \
    		echo "    Working directory not clean; will not overwrite"; \
    	fi
@@ -80,7 +90,7 @@

build: update checktools
    unset CC; $(MAKE) -C $(project_dir)/BaseTools
-	echo " build $(project_name) $(TAG-y)"
+	echo " build $(project_name) $(TAG)"
    if [ -n $(CONFIG_TIANOCORE_BOOTSPLASH_FILE) ]; then \
    	echo "    Copying custom bootsplash image"; \
    	case "$(CONFIG_TIANOCORE_BOOTSPLASH_FILE)" in \
@@ -99,7 +109,7 @@
    		cat ../tools_def.txt >> $(project_dir)/Conf/tools_def.txt; \
    	fi; \
    	build $(BUILD_STR); \
-		mv $(project_dir)/Build/CorebootPayloadPkg*/*/FV/UEFIPAYLOAD.fd $(project_dir)/Build/UEFIPAYLOAD.fd; \
+		mv $(project_dir)/Build/$(bootloader)*/*/FV/UEFIPAYLOAD.fd $(project_dir)/Build/UEFIPAYLOAD.fd; \
    	git checkout CorebootPayloadPkg/Logo/Logo.bmp > /dev/null 2>&1 || true

clean: