Change in coreboot[master]: rmodule: Add R_386_16 to valid reloc

[Marshall Dawson (Code Review)]

Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34912 )

Change subject: rmodule: Add R_386_16 to valid reloc
......................................................................

rmodule: Add R_386_16 to valid reloc

Prepare for a relocatable module that will rely on the x86 sequenced
files of prologue.inc, entry16.inc, reset.inc, entry32.inc.  This
generates type 20 R_386_16.

Change-Id: I56df52cb5626b99b39fa170d65920e71cafff569
Signed-off-by: Marshall Dawson 
---
M util/cbfstool/rmodule.c
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/34912/1

diff --git a/util/cbfstool/rmodule.c b/util/cbfstool/rmodule.c
index 80e8911..c79029d 100644
--- a/util/cbfstool/rmodule.c
+++ b/util/cbfstool/rmodule.c
@@ -31,7 +31,7 @@
    type = ELF64_R_TYPE(rel->r_info);

/* Only these 2 relocations are expected to be found. */
-	return (type == R_386_32 || type == R_386_PC32);
+	return (type == R_386_32 || type == R_386_PC32 || type == R_386_16);
 }

static int should_emit_386(Elf64_Rela *rel)

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34912 )

Change subject: rmodule: Add R_386_16 to valid reloc
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34912/2/util/cbfstool/rmodule.c 
File util/cbfstool/rmodule.c:

https://review.coreboot.org/c/coreboot/+/34912/2/util/cbfstool/rmodule.c@33 
PS2, Line 33: 2 relocations
Update the comment?

[Marshall Dawson (Code Review)]

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

Change subject: rmodule: Add R_386_16 to valid reloc
......................................................................

rmodule: Add R_386_16 to valid reloc

Prepare for a relocatable module that will rely on the x86 sequenced
files of prologue.inc, entry16.inc, reset.inc, entry32.inc.  This
generates type 20 R_386_16.

Change-Id: I56df52cb5626b99b39fa170d65920e71cafff569
Signed-off-by: Marshall Dawson 
---
M util/cbfstool/rmodule.c
1 file changed, 2 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/34912/3

[Marshall Dawson (Code Review)]

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

Change subject: rmodule: Add R_386_16 to valid reloc
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34912/2/util/cbfstool/rmodule.c 
File util/cbfstool/rmodule.c:

https://review.coreboot.org/c/coreboot/+/34912/2/util/cbfstool/rmodule.c@33 
PS2, Line 33: 2 relocations
> Update the comment?

Thanks

[Aaron Durbin (Code Review)]

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

Change subject: rmodule: Add R_386_16 to valid reloc
......................................................................

Patch Set 3: Code-Review+1

Why is this one actually needed? Aren't you linking your romstage statically?

[Marshall Dawson (Code Review)]

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

Change subject: rmodule: Add R_386_16 to valid reloc
......................................................................

Patch Set 3:
> Why is this one actually needed? Aren't you linking your romstage statically?

Yes.  However this is an example of all the little things that turn out to be different enough to cause small problems.  This is due to passing --xip to cbfstool for romstage.  If I try to use NO_XIP_EARLY_STAGES, I hit cbfs-mkstage.c "Conflicting sections in segment".

So in hindsight, this is the wrong tool for the job.  I actually prefer to keep romstage out of the image altogether, as it's burning space.  And I don't care that it's made into a stage; I only need the elf in order to create the compressed image for the PSP.  Will reorient this patch shortly.

[Aaron Durbin (Code Review)]

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

Change subject: rmodule: Add R_386_16 to valid reloc
......................................................................

Patch Set 3:
> Patch Set 3:
> > Why is this one actually needed? Aren't you linking your romstage statically?
>
> Yes.  However this is an example of all the little things that turn out to be different enough to cause small problems.  This is due to passing --xip to cbfstool for romstage.  If I try to use NO_XIP_EARLY_STAGES, I hit cbfs-mkstage.c "Conflicting sections in segment".

Why would the program header overlap the section header? Do you have readelf output showing the issue?
> So in hindsight, this is the wrong tool for the job.  I actually prefer to keep romstage out of the image altogether, as it's burning space.  And I don't care that it's made into a stage; I only need the elf in order to create the compressed image for the PSP.  Will reorient this patch shortly.

I see.

[Marshall Dawson (Code Review)]

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

Change subject: rmodule: Add R_386_16 to valid reloc
......................................................................

Patch Set 3:

Aaron, I decided to squash everything into https://review.coreboot.org/c/coreboot/+/32414 "x86: Introduce RESET_VECTOR_IN_DRAM option" and am going to abandon this change.

Let me know if the "conflicting segments" message is interesting enough that you'd like more info.  Otherwise I'll assume it wasn't interesting enough in light of skipping making a stage and excluding from cbfs.

[Marshall Dawson (Code Review)]

Marshall Dawson has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/34912 )

Change subject: rmodule: Add R_386_16 to valid reloc
......................................................................

Abandoned

OBE with https://review.coreboot.org/c/coreboot/+/32414/12 "x86: Introduce RESET_VECTOR_IN_DRAM option"

[Aaron Durbin (Code Review)]

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

Change subject: rmodule: Add R_386_16 to valid reloc
......................................................................

Patch Set 3:
> Patch Set 3:
> > Patch Set 3:
> > > Why is this one actually needed? Aren't you linking your romstage statically?
> >
> > Yes.  However this is an example of all the little things that turn out to be different enough to cause small problems.  This is due to passing --xip to cbfstool for romstage.  If I try to use NO_XIP_EARLY_STAGES, I hit cbfs-mkstage.c "Conflicting sections in segment".
>
> Why would the program header overlap the section header? Do you have readelf output showing the issue?
> > So in hindsight, this is the wrong tool for the job.  I actually prefer to keep romstage out of the image altogether, as it's burning space.  And I don't care that it's made into a stage; I only need the elf in order to create the compressed image for the PSP.  Will reorient this patch shortly.
>
> I see.

I was just thinking about this. We do need romstage in the image somewhere. vboot will steer psp to load from the correct location, but the multiple copies needs to reside somewhere. What were your plans on this front?

[Marshall Dawson (Code Review)]

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

Change subject: rmodule: Add R_386_16 to valid reloc
......................................................................

Patch Set 3:
> I was just thinking about this. We do need romstage in the image somewhere. vboot will steer psp to load from the correct location, but the multiple copies needs to reside somewhere. What were your plans on this front?

As I understand it in b:127795737, vboot will return a pointer to the PSP for a PSP Directory Table and not to a romstage image in cbfs.  I infer that each table resides in a separate FMAP region.  So the decision is made ahead of this code.  However this seems like we have two holes to plug:

(1) An image that finds itself running in DRAM will have no idea which table was chosen by vboot, yet will need to load ramstage from the appropriate RO/A/B region in the flash.  I'm not sure if there have been discussions for whether/how to get that knowledge from a PSP-based vboot.

(2) Rereading the precise language in the bug, a PSP Directory Table is not the same as the PSP's BIOS Directory Table.  I will reopen that discussion, because the BIOS image is described by the BIOS Directory Table, as are microcode patches and PMU firmware.  If I could have a separate hybrid-romstage for each RO/A/B, then #1 becomes easier.  However if not, the hybrid-romstage should arguably be in RO and then not updateable.  That feels like a problem without a current solution.

FWIW the PSP supports 2 (only 2?) directory table levels so that the secondary may be updated without risking bricking the system.  But to use this I would still need to place the 2nd level inside an A/B to ensure rogue code can't be run.  And I don't know whether it's possible to prevent the PSP from using a 2nd level if it's healthy, i.e. no way to force the primary RO table to be used.

BTW (3) Since we'll need three tables, amdfwtool may still require an update, maybe we want to call it multiple times, or we may be able to duplicate the amdfw.rom images and fixup pointers.

[Aaron Durbin (Code Review)]

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

Change subject: rmodule: Add R_386_16 to valid reloc
......................................................................

Patch Set 3:
> Patch Set 3:
> > I was just thinking about this. We do need romstage in the image somewhere. vboot will steer psp to load from the correct location, but the multiple copies needs to reside somewhere. What were your plans on this front?
>
> As I understand it in b:127795737, vboot will return a pointer to the PSP for a PSP Directory Table and not to a romstage image in cbfs.  I infer that each table resides in a separate FMAP region.  So the decision is made ahead of this code.  However this seems like we have two holes to plug:
>
> (1) An image that finds itself running in DRAM will have no idea which table was chosen by vboot, yet will need to load ramstage from the appropriate RO/A/B region in the flash.  I'm not sure if there have been discussions for whether/how to get that knowledge from a PSP-based vboot.
>
> (2) Rereading the precise language in the bug, a PSP Directory Table is not the same as the PSP's BIOS Directory Table.  I will reopen that discussion, because the BIOS image is described by the BIOS Directory Table, as are microcode patches and PMU firmware.  If I could have a separate hybrid-romstage for each RO/A/B, then #1 becomes easier.  However if not, the hybrid-romstage should arguably be in RO and then not updateable.  That feels like a problem without a current solution.
>
> FWIW the PSP supports 2 (only 2?) directory table levels so that the secondary may be updated without risking bricking the system.  But to use this I would still need to place the 2nd level inside an A/B to ensure rogue code can't be run.  And I don't know whether it's possible to prevent the PSP from using a 2nd level if it's healthy, i.e. no way to force the primary RO table to be used.
>
> BTW (3) Since we'll need three tables, amdfwtool may still require an update, maybe we want to call it multiple times, or we may be able to duplicate the amdfw.rom images and fixup pointers.

Thanks for the update. There are definitely opens that need to be cleared and sorted through.