Change in coreboot[master]: [WIP] intel/fsp2_0: Restrict references to CAR

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35289 )

Change subject: [WIP] intel/fsp2_0: Restrict references to CAR
......................................................................

[WIP] intel/fsp2_0: Restrict references to CAR

Change-Id: Ifb1e33d67722db086d9f9feff0e1d289f40d4ef5
Signed-off-by: Kyösti Mälkki 
---
M src/drivers/intel/fsp2_0/memory_init.c
1 file changed, 21 insertions(+), 23 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/35289/1

diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c
index a916c9d..a4c7c07 100644
--- a/src/drivers/intel/fsp2_0/memory_init.c
+++ b/src/drivers/intel/fsp2_0/memory_init.c
@@ -185,24 +185,9 @@
    return CB_SUCCESS;
 }

-static enum cb_err fsp_fill_common_arch_params(FSPM_ARCH_UPD *arch_upd,
-					bool s3wake, uint32_t fsp_version,
-					const struct memranges *memmap)
+static void cb_err fsp_fill_common_arch_params(FSPM_ARCH_UPD *arch_upd,
+					bool s3wake, uint32_t fsp_version)
 {
-	/*
-	 * FSP 2.1 version would use same stack as coreboot instead of
-	 * setting up separate stack frame. FSP 2.1 would not relocate stack
-	 * top and does not reinitialize stack pointer. The parameters passed
-	 * as StackBase and StackSize are actually for temporary RAM and HOBs
-	 * and are not related to FSP stack at all.
-	 */
-	if (CONFIG(FSP_USES_CB_STACK)) {
-		arch_upd->StackBase = temp_ram;
-		arch_upd->StackSize = sizeof(temp_ram);
-	} else if (setup_fsp_stack_frame(arch_upd, memmap)) {
-		return CB_ERR;
-	}
-
    fsp_fill_mrc_cache(arch_upd, fsp_version);

/* Configure bootmode */
@@ -226,8 +211,6 @@
    }

printk(BIOS_SPEW, "bootmode is set to :%d\n", arch_upd->BootMode);
-
-	return CB_SUCCESS;
 }

__weak
@@ -292,11 +275,23 @@
    /* Reserve enough memory under TOLUD to save CBMEM header */
    arch_upd->BootLoaderTolumSize = cbmem_overhead_size();

-	/* Fill common settings on behalf of chipset. */
-	if (fsp_fill_common_arch_params(arch_upd, s3wake, fsp_version,
-					memmap) != CB_SUCCESS)
+	/*
+	 * FSP 2.1 version would use same stack as coreboot instead of
+	 * setting up separate stack frame. FSP 2.1 would not relocate stack
+	 * top and does not reinitialize stack pointer. The parameters passed
+	 * as StackBase and StackSize are actually for temporary RAM and HOBs
+	 * and are not related to FSP stack at all.
+	 */
+	if (CONFIG(FSP_USES_CB_STACK)) {
+		arch_upd->StackBase = temp_ram;
+		arch_upd->StackSize = sizeof(temp_ram);
+	} else if (setup_fsp_stack_frame(arch_upd, memmap)) {
    	die_with_post_code(POST_INVALID_VENDOR_BINARY,
    		"FSPM_ARCH_UPD not found!\n");
+	}
+
+	/* Fill common settings on behalf of chipset. */
+	fsp_fill_common_arch_params(arch_upd, s3wake, fsp_version);

/* Give SoC and mainboard a chance to update the UPD */
    platform_fsp_memory_init_params_cb(&fspm_upd, fsp_version);
@@ -421,7 +416,10 @@
    /* Signal that FSP component has been loaded. */
    prog_segment_loaded(hdr.image_base, hdr.image_size, SEG_FINAL);

-	do_fsp_memory_init(&hdr, s3wake, &memmap);
+	if (CONFIG(FSP_USES_CB_STACK))
+		do_fsp_memory_init(&hdr, s3wake, NULL);
+	else
+		do_fsp_memory_init(&hdr, s3wake, &memmap);

timestamp_add_now(TS_AFTER_INITRAM);
 }

[Kyösti Mälkki (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins),

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

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

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

Change subject: [WIP] intel/fsp2_0: Restrict references to CAR
......................................................................

[WIP] intel/fsp2_0: Restrict references to CAR

Change-Id: Ifb1e33d67722db086d9f9feff0e1d289f40d4ef5
Signed-off-by: Kyösti Mälkki 
---
M src/drivers/intel/fsp2_0/memory_init.c
1 file changed, 31 insertions(+), 31 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/35289/2

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35289 )

Change subject: [WIP] intel/fsp2_0: Restrict references to CAR
......................................................................

Patch Set 2:

Subrata; the motivation is that AMD Picasso will use some of the FSP2.x infrastructure for the UEFI abstractions/API it provides. Not optimal, but apparently the most straight-forward choice given the layout of AMD vendorcode.

For this change, the motivation is to be able to build the file such that _car_xxx symbol references will hit garbage collection. Implementation of amd/picasso runs with romstage already in DRAM.

My understanding is Picasso needs FSP-M loader but without the actual raminit call here. Marshall or Martin should comment on that further. The attempt is to reorganise code here such that it is not necessary to initialize memranges at all.

[Subrata Banik (Code Review)]

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

Change subject: [WIP] intel/fsp2_0: Restrict references to CAR
......................................................................

Patch Set 2:
> Patch Set 2:
>
> Subrata; the motivation is that AMD Picasso will use some of the FSP2.x infrastructure for the UEFI abstractions/API it provides. Not optimal, but apparently the most straight-forward choice given the layout of AMD vendorcode.
>
> For this change, the motivation is to be able to build the file such that _car_xxx symbol references will hit garbage collection. Implementation of amd/picasso runs with romstage already in DRAM.
>
> My understanding is Picasso needs FSP-M loader but without the actual raminit call here. Marshall or Martin should comment on that further. The attempt is to reorganise code here such that it is not necessary to initialize memranges at all.

Thanks for adding me, let me talk a look as well

[Subrata Banik (Code Review)]

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

Change subject: [WIP] intel/fsp2_0: Restrict references to CAR
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35289/2/src/drivers/intel/fsp2_0/me... 
File src/drivers/intel/fsp2_0/memory_init.c:

https://review.coreboot.org/c/coreboot/+/35289/2/src/drivers/intel/fsp2_0/me... 
PS2, Line 402: 	if (!(CONFIG(FSP_M_XIP) && CONFIG(FSP_USES_CB_STACK))) {
!CONFIG(FSP_USES_CB_STACK) ?

[Furquan Shaikh (Code Review)]

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

Change subject: [WIP] intel/fsp2_0: Restrict references to CAR
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35289/2/src/drivers/intel/fsp2_0/me... 
File src/drivers/intel/fsp2_0/memory_init.c:

https://review.coreboot.org/c/coreboot/+/35289/2/src/drivers/intel/fsp2_0/me... 
PS2, Line 402: 	if (!(CONFIG(FSP_M_XIP) && CONFIG(FSP_USES_CB_STACK))) {
> !CONFIG(FSP_USES_CB_STACK) ?

Why is FSP_USES_CB_STACK added at all?

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35289 )

Change subject: [WIP] intel/fsp2_0: Restrict references to CAR
......................................................................

Patch Set 2: Code-Review-2

(3 comments)

Also, this will be not-for-merge for some time.

https://review.coreboot.org/c/coreboot/+/35289/2/src/drivers/intel/fsp2_0/me... 
File src/drivers/intel/fsp2_0/memory_init.c:

https://review.coreboot.org/c/coreboot/+/35289/2/src/drivers/intel/fsp2_0/me... 
PS2, Line 402: 	if (!(CONFIG(FSP_M_XIP) && CONFIG(FSP_USES_CB_STACK))) {
> Why is FSP_USES_CB_STACK added at all?

I could write this '!FSP_M_XIP || !FSP_USES_CB_STACK'  if that is clearer.

I believe there will be request to split this entire function further, so this if very much WIP now.

https://review.coreboot.org/c/coreboot/+/35289/2/src/drivers/intel/fsp2_0/me... 
PS2, Line 413: 		status = load_fspm_mem(&hdr, &file_data, &memmap);
memmap reference here

https://review.coreboot.org/c/coreboot/+/35289/2/src/drivers/intel/fsp2_0/me... 
PS2, Line 424: 		do_fsp_memory_init(&hdr, s3wake, &memmap);
Another memmap reference here. But I believe amd/picasso, with DRAM already up, would not want to make this call. That patchtrain actually already has some comments about avoidind fsp_memory_init() call, but they still want FSP-M loader (afaics!).

[Aaron Durbin (Code Review)]

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

Change subject: [WIP] intel/fsp2_0: Restrict references to CAR
......................................................................

Patch Set 2:

(5 comments)

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

https://review.coreboot.org/c/coreboot/+/35289/2//COMMIT_MSG@8 
PS2, Line 8: 
I know this is a WIP patch, but please fill out the description with the motivation behind this patch.

https://review.coreboot.org/c/coreboot/+/35289/2/src/drivers/intel/fsp2_0/me... 
File src/drivers/intel/fsp2_0/memory_init.c:

https://review.coreboot.org/c/coreboot/+/35289/2/src/drivers/intel/fsp2_0/me... 
PS2, Line 252: const struct memranges *memmap
This should be removed and all the StackBase/StackSize handling for the various scenarios should be in one function and called from fsp_memory_init(). This function likely needs to be split as well for some initial validation of header prior to doing other things. That said, I don't think the change looks too too bad.

https://review.coreboot.org/c/coreboot/+/35289/2/src/drivers/intel/fsp2_0/me... 
PS2, Line 335: 					const struct memranges *memmap)
Or pass NULL for memmap here.

Or, which I think is more appropriate, provide an FSP-S-like loading path for FSP-M. It can be relocated or one can attempt to statically link things, but the latter problematic in the S3 case.

I suggest we add a Kconfig which is FSPM_RUNS_FROM_DRAM which would handle all the complex conditionals and loading, tbh.

https://review.coreboot.org/c/coreboot/+/35289/2/src/drivers/intel/fsp2_0/me... 
PS2, Line 404: 		memranges_init_empty(&memmap, &freeranges[0], ARRAY_SIZE(freeranges));
this line needs to be unconditional even if memmap isn't used.

https://review.coreboot.org/c/coreboot/+/35289/2/src/drivers/intel/fsp2_0/me... 
PS2, Line 405: _car_region_start
Don't we have a ENV_CAR to use to guard all of this appropriately? I don't think the Kconfigs above are accurate. FSP_USES_CB_STACK is completely orthogonal to CAR. !FSP_M_XIP along with ENV_CAR would be the cleanest. However, I think my suggestion above is cleaner.

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35289 )

Change subject: [WIP] intel/fsp2_0: Restrict references to CAR
......................................................................

Patch Set 2:

Marshall; For the time being I won't be updating this commit so go ahead and modify as needed/requested.

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35289 )

Change subject: [WIP] intel/fsp2_0: Restrict references to CAR
......................................................................

Abandoned