pkg/amd/firmware: Wrap PSP and BIOS directories in arrays #367
Conversation
This allows for returning multiple directories. Up until now, only the first directories would be returned, while many firmware images contain multiple directories for various SoC families and models, e.g., for desktop boards where SoCs can be swapped. Signed-off-by: Daniel Maslowski <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #367 +/- ##
==========================================
- Coverage 41.74% 41.72% -0.03%
==========================================
Files 110 110
Lines 7649 7653 +4
==========================================
Hits 3193 3193
- Misses 3877 3881 +4
Partials 579 579
Continue to review full report at Codecov.
|
|
A slice doesn't provide information about the origin of the element. |
Yea, that is also currently the problem: I don't know what I got, I only got a single directory, and I am missing the other directories. From the code I could understand that I got the first directory that is found. And it could be either from the EmbeddedFirmwareStruct or it could have been found by scanning. When the latter is the case, how would we know the processor family/model range? To expost that information when we have it, I would suggest adding metadata to the respective directory, such as processor family / model range based on the offset. However, that is a different concern; this PR here is to enable multiple directories for a start. WDYT? |
There was a problem hiding this comment.
I do think this needs to be cleaned up long term, descriptors need to be added to differentiate the PSPDirectories, but it's possible this could be placed in the PSPDir struct instead. I'm ok with merging this right now as it doesn't make anything worse, and looking at the code like this actually warns me that there can be multiple PSP directories.
Perhaps a TODO or block comment is in order warning us that only the first one is handled right now?
Sorry, for a very long delay. There is a documentation problem, each "directory" has its own purpose. The future code should clearly specify the purpose of each directory. (which we currently can't do) I agree that the current code assumes, that there could be no more than a single directory in firmware, which could be incorrect. I propose to leave everything as is, but move "scanning" into a separate function |
There was a problem hiding this comment.
There is a documentation problem, each "directory" has its own purpose. The future code should clearly specify the purpose of each directory. (which we currently can't do)
I agree that the current code assumes, that there could be no more than a single directory in firmware, which could be incorrect.
I propose to leave everything as is, but move "scanning" into a separate function, this will make the behaviour of what directory do we have clear.
|
Okay I will create another implementation then. We discussed this in the Fiano sync call, and the proposal is to rework the directory structs, so that they
|
| } | ||
| } | ||
| break | ||
| } | ||
| } | ||
|
|
||
| result.BIOSDirectories = []BIOSDir{} |
There was a problem hiding this comment.
not sure I understand this. you can append to a nil slice. But 112 or 113 seem redundant. But you wanted two elements or ...?
There was a problem hiding this comment.
I don't know enough Go, will see about your suggestion.
|
I've been working on this again recently, but in a Rust implementation, based on System76/Jeremey Soller's romulan: The current objective is diffing two given images, allowing me to somewhat understand them more. Addendum 1: coreboot refers to NDA docs, so they are not publicly accessible/verifiable. |
This allows for returning multiple directories. Up until now, only the first directories
would be returned, while many firmware images contain multiple directories for various
SoC families and models, e.g., for desktop boards where SoCs can be swapped.
Signed-off-by: Daniel Maslowski [email protected]