Repository: oreboot/oreboot (language: rust)

Issue:
broken src/lib/uefi parsing and tests
1. The crate `src/lib/uefi` broke in `5beeb5641124c240a06bb889605e2f44485cbf26`, where the loops were removed that iterate over the FV (firmware volume).
2. Its tests broke in `b8f0e5100abbdbd396ac884e827bce5e2cabb4c0` where the `fsp` submodule was updated, but the test cases depend on the binaries in it (12 of them).

The first issue is easy to fix, but the test setup is fragile.

Question: What should we prefer, a second fsp submodule side by side with the uefi lib, pinned to its version for testing, or checking the respective files (some MB involved) into the repo (maybe reduce them? 12 feels like it's not really providing value), or... any other ideas?


Discussion / hints:
We forked https://github.com/oreboot/FSP and will add the revision we use for testing as a submodule in the uefi crate.

Fix patch:
diff --git a/.gitmodules b/.gitmodules
index f99da18fa9..1c377eba63 100644
--- a/.gitmodules
+++ b/.gitmodules
@@ -11,3 +11,6 @@
 	url = https://github.com/universalpayload/fspsdk
 	branch = qemu_fsp_x64
 	ignore = dirty
+[submodule "src/lib/uefi/tests/fsp-bin"]
+	path = src/lib/uefi/tests/fsp-bin
+	url = https://github.com/oreboot/fsp
diff --git a/src/lib/uefi/Makefile b/src/lib/uefi/Makefile
index a7aad55756..088e3c5a0f 100644
--- a/src/lib/uefi/Makefile
+++ b/src/lib/uefi/Makefile
@@ -1,5 +1,4 @@
 OREBOOT=$(abspath $(CURDIR)/../../../)
 include ../../../Makefile.inc
 ciclippy: clippy
-#todo bring back tests
-citest: skiptest
+citest: coverage
diff --git a/src/lib/uefi/src/lib.rs b/src/lib/uefi/src/lib.rs
index 711a9c6b01..9b2c936686 100644
--- a/src/lib/uefi/src/lib.rs
+++ b/src/lib/uefi/src/lib.rs
@@ -209,11 +209,10 @@ where
         }
 
         // Iterate through files.
-        let res = {
+        while {
             index = (index + 7) & !7; // align to 8 bytes
             index < fv_end
-        };
-        if res {
+        } {
             let ffs_base = index;
 
             // Check if the header is empty.
@@ -301,11 +300,10 @@ where
             index += ffs_header_size;
 
             // Iterate through sections.
-            let res = {
+            while {
                 index = (index + 3) & !3; // align to 4 bytes
                 index < ffs_end
-            };
-            if res {
+            } {
                 let sec_base = index;
                 let sec_common: EFI_COMMON_SECTION_HEADER =
                     unsafe { core::ptr::read(data[sec_base..].as_ptr() as *const _) };
@@ -337,7 +335,13 @@ where
                     },
                     slice_data(sec_base + sec_header_size, sec_end, sec_end)?,
                 );
+
+                // Skip to next section.
+                index = sec_end;
             }
+
+            // Skip to next file.
+            index = ffs_end;
         }
 
         // Skip to next FV.


Test patch:
diff --git a/src/lib/uefi/tests/fsp-bin b/src/lib/uefi/tests/fsp-bin
new file mode 160000
index 0000000000..4445df2daf
--- /dev/null
+++ b/src/lib/uefi/tests/fsp-bin
@@ -0,0 +1,1 @@
+Subproject commit 4445df2daffc88aa32bd9c396d0185d2ee5f237d
diff --git a/src/lib/uefi/tests/tests.rs b/src/lib/uefi/tests/tests.rs
index 279c0d7be6..e1abbdaad4 100644
--- a/src/lib/uefi/tests/tests.rs
+++ b/src/lib/uefi/tests/tests.rs
@@ -3,7 +3,8 @@ use std::{fs, io};
 extern crate uefi;
 use uefi::*;
 
-/// This test parses every fsp image from https://github.com/intel/FSP.
+/// This test parses every fsp image from https://github.com/oreboot/FSP.
+/// NOTE: Paths are relative to the _crate_ directory, not this file.
 #[test]
 fn parse_fsp() -> io::Result<()> {
     use std::path::Path;
@@ -15,7 +16,7 @@ fn parse_fsp() -> io::Result<()> {
 
     let test_cases = &[
         TestCase {
-            path: Path::new("../../../3rdparty/fsp/AmberLakeFspBinPkg/Fsp.fd"),
+            path: Path::new("./tests/fsp-bin/AmberLakeFspBinPkg/Fsp.fd"),
             expected_sections_and_sizes: &[
                 (
                     SectionInfo {
@@ -1028,7 +1029,7 @@ fn parse_fsp() -> io::Result<()> {
             ],
         },
         TestCase {
-            path: Path::new("../../../3rdparty/fsp/ApolloLakeFspBinPkg/FspBin/Fsp.fd"),
+            path: Path::new("./tests/fsp-bin/ApolloLakeFspBinPkg/FspBin/Fsp.fd"),
             expected_sections_and_sizes: &[
                 (
                     SectionInfo {
@@ -1657,7 +1658,7 @@ fn parse_fsp() -> io::Result<()> {
             ],
         },
         TestCase {
-            path: Path::new("../../../3rdparty/fsp/BraswellFspBinPkg/FspBin/BSWFSP.fd"),
+            path: Path::new("./tests/fsp-bin/BraswellFspBinPkg/FspBin/BSWFSP.fd"),
             expected_sections_and_sizes: &[
                 (
                     SectionInfo {
@@ -2878,7 +2879,7 @@ fn parse_fsp() -> io::Result<()> {
             ],
         },
         TestCase {
-            path: Path::new("../../../3rdparty/fsp/CoffeeLakeFspBinPkg/Fsp.fd"),
+            path: Path::new("./tests/fsp-bin/CoffeeLakeFspBinPkg/Fsp.fd"),
             expected_sections_and_sizes: &[
                 (
                     SectionInfo {
@@ -3635,7 +3636,7 @@ fn parse_fsp() -> io::Result<()> {
             ],
         },
         TestCase {
-            path: Path::new("../../../3rdparty/fsp/CometLakeFspBinPkg/CometLake1/Fsp.fd"),
+            path: Path::new("./tests/fsp-bin/CometLakeFspBinPkg/CometLake1/Fsp.fd"),
             expected_sections_and_sizes: &[
                 (
                     SectionInfo {
@@ -4712,7 +4713,7 @@ fn parse_fsp() -> io::Result<()> {
             ],
         },
         TestCase {
-            path: Path::new("../../../3rdparty/fsp/CometLakeFspBinPkg/CometLake2/Fsp.fd"),
+            path: Path::new("./tests/fsp-bin/CometLakeFspBinPkg/CometLake2/Fsp.fd"),
             expected_sections_and_sizes: &[
                 (
                     SectionInfo {
@@ -5789,7 +5790,7 @@ fn parse_fsp() -> io::Result<()> {
             ],
         },
         TestCase {
-            path: Path::new("../../../3rdparty/fsp/CometLakeFspBinPkg/CometLakeS/Fsp.fd"),
+            path: Path::new("./tests/fsp-bin/CometLakeFspBinPkg/CometLakeS/Fsp.fd"),
             expected_sections_and_sizes: &[
                 (
                     SectionInfo {
@@ -6866,7 +6867,7 @@ fn parse_fsp() -> io::Result<()> {
             ],
         },
         TestCase {
-            path: Path::new("../../../3rdparty/fsp/CometLakeFspBinPkg/CometLakeV/Fsp.fd"),
+            path: Path::new("./tests/fsp-bin/CometLakeFspBinPkg/CometLakeV/Fsp.fd"),
             expected_sections_and_sizes: &[
                 (
                     SectionInfo {
@@ -7943,7 +7944,7 @@ fn parse_fsp() -> io::Result<()> {
             ],
         },
         TestCase {
-            path: Path::new("../../../3rdparty/fsp/DenvertonNSFspBinPkg/FspBin/DenvertonNSFsp.fd"),
+            path: Path::new("./tests/fsp-bin/DenvertonNSFspBinPkg/FspBin/DenvertonNSFsp.fd"),
             expected_sections_and_sizes: &[
                 (
                     SectionInfo {
@@ -8940,7 +8941,7 @@ fn parse_fsp() -> io::Result<()> {
             ],
         },
         TestCase {
-            path: Path::new("../../../3rdparty/fsp/IceLakeFspBinPkg/Fsp.fd"),
+            path: Path::new("./tests/fsp-bin/IceLakeFspBinPkg/Fsp.fd"),
             expected_sections_and_sizes: &[
                 (
                     SectionInfo {
@@ -10033,7 +10034,7 @@ fn parse_fsp() -> io::Result<()> {
             ],
         },
         TestCase {
-            path: Path::new("../../../3rdparty/fsp/KabylakeFspBinPkg/Fsp.fd"),
+            path: Path::new("./tests/fsp-bin/KabylakeFspBinPkg/Fsp.fd"),
             expected_sections_and_sizes: &[
                 (
                     SectionInfo {
@@ -10790,7 +10791,7 @@ fn parse_fsp() -> io::Result<()> {
             ],
         },
         TestCase {
-            path: Path::new("../../../3rdparty/fsp/SkylakeFspBinPkg/FspBin/SkylakeFsp.fd"),
+            path: Path::new("./tests/fsp-bin/SkylakeFspBinPkg/FspBin/SkylakeFsp.fd"),
             expected_sections_and_sizes: &[
                 (
                     SectionInfo {