Re: [PATCH 2/2] efi_selftest: add minimal fmp test

["Vincent Stehlé" <vincent.stehle@arm.com>]

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é <vincent.stehle@arm.com> 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é <vincent.stehle@arm.com>
> > Cc: Tom Rini <trini@konsulko.com>
> > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >
> > 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