All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Vincent Stehlé" <vincent.stehle@arm.com>
To: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>,
	Ilias Apalodimas <ilias.apalodimas@linaro.org>,
	Tom Rini <trini@konsulko.com>,
	Jose Marinho <jose.marinho@arm.com>,
	u-boot@lists.denx.de
Subject: Re: [PATCH] efi_loader: fix ecpt size computation
Date: Wed, 11 Feb 2026 12:47:53 +0100	[thread overview]
Message-ID: <aYxsaXdISybHOQTq@debian> (raw)
In-Reply-To: <a386de9e-1dc9-4b50-a86c-bb8dfd845eef@gmx.de>

On Wed, Feb 11, 2026 at 10:56:30AM +0100, Heinrich Schuchardt wrote:
> On 2/11/26 10:33, Vincent Stehlé wrote:
> > The size of the memory allocated for the EFI Conformance Profiles Table is
> > computed with `num_entries' always equal to zero, which is incorrect when
> > CONFIG_EFI_EBBR_2_1_CONFORMANCE is enabled.
> > 
> > This can be verified by allocating the ECPT memory with malloc() instead of
> > efi_allocate_pool(), building u-boot with sandbox_defconfig and
> > CONFIG_VALGRIND=y, and by finally running the following command:
> > 
> >    valgrind --suppressions=scripts/u-boot.supp \
> >      ./u-boot -T -c 'efidebug tables'
> > 
> > Fix this by using an array of the supported profiles GUIDs instead, which
> > should also be easier to extend in the future.
> 
> Thank you Vincent for this patch.

Hi Heinrich,

Thanks for reviewing.

> Maybe mention in the commit message that U-Boot should publish all supported
> EBBR revisions.

Ok, will do.

> 
> > 
> > Fixes: 6b92c1735205 ("efi: Create ECPT table")
> > Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
> > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > Cc: Tom Rini <trini@konsulko.com>
> > Cc: Jose Marinho <jose.marinho@arm.com>
> > ---
> >   lib/efi_loader/efi_conformance.c | 18 +++++++++---------
> >   1 file changed, 9 insertions(+), 9 deletions(-)
> > 
> > diff --git a/lib/efi_loader/efi_conformance.c b/lib/efi_loader/efi_conformance.c
> > index 2bae93a94bd..9768b5ae824 100644
> > --- a/lib/efi_loader/efi_conformance.c
> > +++ b/lib/efi_loader/efi_conformance.c
> > @@ -13,8 +13,12 @@
> >   #include <malloc.h>
> >   static const efi_guid_t efi_ecpt_guid = EFI_CONFORMANCE_PROFILES_TABLE_GUID;
> > -static const efi_guid_t efi_ebbr_2_1_guid =
> > -	EFI_CONFORMANCE_PROFILE_EBBR_2_1_GUID;
> > +
> > +static const efi_guid_t profiles[] = {
> 
> As profiles[] is not used outside efi_ecpt_register(), there is no need to
> make it a static variable.

I am not sure why a "global" symbol would be better than a static one, but if
you like this solution we could make the profiles[] array extern.

Then I guess we should rename it with a much longer name to avoid namespace
"pollution":

	const efi_guid_t efi_conformance_profiles[] = {
		...

All the sizeof() and ARRAY_SIZE() would have to follow, too.

Alternatively, we could move the array declaration inside the
efi_ecpt_register() function completely:

	efi_status_t efi_ecpt_register(void)
	{
		struct efi_conformance_profiles_table *ecpt;
		efi_status_t ret;
		size_t ecpt_size;

		static const efi_guid_t profiles[] = {
		#if CONFIG_IS_ENABLED(EFI_EBBR_2_1_CONFORMANCE)
			EFI_CONFORMANCE_PROFILE_EBBR_2_1_GUID,
		#endif
		};

		...

This is still static here, but for storage duration not linkage. One advantage
is that the name would not have to change. I am not a big fan of the #if in
function scope, but if you prefer this solution this is fine as well.

Best regards,
Vincent.

> 
> Otherwise looks good to me.
> 
> Best regards
> 
> Heinrich
> 
> 
> > +#if CONFIG_IS_ENABLED(EFI_EBBR_2_1_CONFORMANCE)
> > +	EFI_CONFORMANCE_PROFILE_EBBR_2_1_GUID,
> > +#endif
> > +};
> >   /**
> >    * efi_ecpt_register() - Install the ECPT system table.
> > @@ -23,12 +27,11 @@ static const efi_guid_t efi_ebbr_2_1_guid =
> >    */
> >   efi_status_t efi_ecpt_register(void)
> >   {
> > -	u16 num_entries = 0;
> >   	struct efi_conformance_profiles_table *ecpt;
> >   	efi_status_t ret;
> >   	size_t ecpt_size;
> > -	ecpt_size = num_entries * sizeof(efi_guid_t)
> > +	ecpt_size = sizeof(profiles)
> >   		+ sizeof(struct efi_conformance_profiles_table);
> >   	ret = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, ecpt_size,
> >   				(void **)&ecpt);
> > @@ -39,12 +42,9 @@ efi_status_t efi_ecpt_register(void)
> >   		return ret;
> >   	}
> > -	if (CONFIG_IS_ENABLED(EFI_EBBR_2_1_CONFORMANCE))
> > -		guidcpy(&ecpt->conformance_profiles[num_entries++],
> > -			&efi_ebbr_2_1_guid);
> > -
> > +	memcpy(ecpt->conformance_profiles, profiles, sizeof(profiles));
> >   	ecpt->version = EFI_CONFORMANCE_PROFILES_TABLE_VERSION;
> > -	ecpt->number_of_profiles = num_entries;
> > +	ecpt->number_of_profiles = ARRAY_SIZE(profiles);
> >   	/* Install the ECPT in the system configuration table. */
> >   	ret = efi_install_configuration_table(&efi_ecpt_guid, (void *)ecpt);
> 

  reply	other threads:[~2026-02-11 11:48 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-02-11  9:33 [PATCH] efi_loader: fix ecpt size computation Vincent Stehlé
2026-02-11  9:56 ` Heinrich Schuchardt
2026-02-11 11:47   ` Vincent Stehlé [this message]
2026-02-11 11:50     ` Heinrich Schuchardt
2026-02-11 12:25       ` Vincent Stehlé
2026-02-12 14:40 ` [PATCH v2] " Vincent Stehlé
2026-02-15  7:30   ` Heinrich Schuchardt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aYxsaXdISybHOQTq@debian \
    --to=vincent.stehle@arm.com \
    --cc=heinrich.schuchardt@canonical.com \
    --cc=ilias.apalodimas@linaro.org \
    --cc=jose.marinho@arm.com \
    --cc=trini@konsulko.com \
    --cc=u-boot@lists.denx.de \
    --cc=xypron.glpk@gmx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.