On Thu, Oct 01, 2026 at 02:31:48AM +0100, Simon Glass wrote: > Hi Vincent,
Hi Simon, Thank you very much for your review. I will send a v2 addressing all your remarks. Best regards, Vincent. > > On 2026-09-22T14:58:39, Vincent Stehlé <[email protected]> wrote: > > efi_selftest: add minimal fmp test > > > > Add a minimal test for the UEFI Firmware Management Protocol. > > At this point the test only does the following: > > > > - Get current firmware images info. > > - For each image, verify consistency of the authentication flags. > > > > This can be tested on the sandbox with the following command: > > > > ./u-boot -T -c "setenv efi_selftest firmware management protocol; \ > > bootefi selftest" > > > > Signed-off-by: Vincent Stehlé <[email protected]> > > Cc: Tom Rini <[email protected]> > > Cc: Heinrich Schuchardt <[email protected]> > > Cc: Ilias Apalodimas <[email protected]> > > > > lib/efi_selftest/Makefile | 2 + > > lib/efi_selftest/efi_selftest_fmp.c | 135 > > ++++++++++++++++++++++++++++++++++++ > > 2 files changed, 137 insertions(+) > > > diff --git a/lib/efi_selftest/efi_selftest_fmp.c > > b/lib/efi_selftest/efi_selftest_fmp.c > > @@ -0,0 +1,135 @@ > > + /* Verify each image info */ > > + for (i = 0; i < desc_cnt; i++) { > > + const struct efi_firmware_image_descriptor *inf = > > &img_info[i]; > > The spec says callers must step through the array using > DescriptorSize, since firmware may return a larger (newer) descriptor > than the one we know about. U-Boot happens to use sizeof(), but since > this is a protocol test, please can you use desc_size as the stride, > e.g. (void *)img_info + i * desc_size, and perhaps check that > desc_size >= sizeof(*inf)? > > > diff --git a/lib/efi_selftest/efi_selftest_fmp.c > > b/lib/efi_selftest/efi_selftest_fmp.c > > @@ -0,0 +1,135 @@ > > + /* Verify authentication */ > > + if (CONFIG_IS_ENABLED(EFI_CAPSULE_AUTHENTICATE) && > > + (attr_supp & IMAGE_ATTRIBUTE_IMAGE_UPDATABLE) && > > + !(attr_supp & IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED)) { > > + efi_st_error("Bad AttributesSupported %u\n", > > + (unsigned int)attr_supp); > > + goto out; > > + } > > efi_firmware.c uses IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE), so > please match that. There is no SPL variant of this option, so > CONFIG_IS_ENABLED() just adds confusion. > > This only checks one direction. You could also check that any bit set > in AttributesSetting is also set in AttributesSupported, and that > AUTHENTICATION_REQUIRED is not set in AttributesSetting when > authentication is disabled. That would also catch regressions on > boards without CONFIG_EFI_CAPSULE_AUTHENTICATE. What do you think? > > > diff --git a/lib/efi_selftest/efi_selftest_fmp.c > > b/lib/efi_selftest/efi_selftest_fmp.c > > @@ -0,0 +1,135 @@ > > + ret = boottime->locate_protocol(&firmware_management_protocol_guid, > > + NULL, (void **)&fmp); > > This only tests the first FMP instance. With > CONFIG_EFI_CAPSULE_FIRMWARE_FIT and _RAW both enabled on sandbox there > can be two, so using locate_handle_buffer() and checking each one > would make the test more useful as it grows. I'm fine with leaving > that for later though, since the patch says 'minimal'. > > Regards, > Simon
