[PATCH 2/3] efi: ECPT add EBBRv2.0 conformance profile

[Jose Marinho]

Hi Heinrich,

> -----Original Message-----
> From: Heinrich Schuchardt <xypron.glpk at gmx.de>
> Sent: 23 December 2021 18:32
> To: Jose Marinho <Jose.Marinho at arm.com>; u-boot at lists.denx.de
> Cc: ilias.apalodimas at linaro.org; sughosh.ganu at linaro.org;
> takahiro.akashi at linaro.org; agraf at csgraf.de; nd <nd at arm.com>
> Subject: Re: [PATCH 2/3] efi: ECPT add EBBRv2.0 conformance profile
> 
> On 12/23/21 15:57, Jose Marinho wrote:
> > Hi Heinrich,
> >
> > Thank you for your reviews.
> >
> >> -----Original Message-----
> >> From: Heinrich Schuchardt <xypron.glpk at gmx.de>
> >> Sent: 17 December 2021 17:27
> >> To: Jose Marinho <Jose.Marinho at arm.com>; u-boot at lists.denx.de
> >> Cc: ilias.apalodimas at linaro.org; sughosh.ganu at linaro.org;
> >> takahiro.akashi at linaro.org; agraf at csgraf.de; nd <nd at arm.com>
> >> Subject: Re: [PATCH 2/3] efi: ECPT add EBBRv2.0 conformance profile
> >>
> >> On 12/17/21 13:55, Jose Marinho wrote:
> >>> Display the EBBRv2.0 conformance in the ECPT table.
> >>>
> >>> The EBBRv2.0 conformance profile is set in the ECPT if
> >>> CONFIG_EFI_EBBR_2_0_CONFORMANCE=y.
> >>> The config defaults to 'n'.
> >>>
> >>>
> >>> Signed-off-by: Jose Marinho <jose.marinho at arm.com>
> >>> ---
> >>>    include/efi_api.h                | 4 ++++
> >>>    lib/efi_loader/Kconfig           | 6 ++++++
> >>>    lib/efi_loader/efi_conformance.c | 9 +++++++++
> >>>    3 files changed, 19 insertions(+)
> >>>
> >>> diff --git a/include/efi_api.h b/include/efi_api.h index
> >>> 6fd4f04de3..49919caa35 100644
> >>> --- a/include/efi_api.h
> >>> +++ b/include/efi_api.h
> >>> @@ -230,6 +230,10 @@ enum efi_reset_type {
> >>>    	EFI_GUID(0x36122546, 0xf7ef, 0x4c8f, 0xbd, 0x9b, \
> >>>    		 0xeb, 0x85, 0x25, 0xb5, 0x0c, 0x0b)
> >>>
> >>> +#define EFI_CONFORMANCE_PROFILE_EBBR_2_0_GUID \
> >>> +	EFI_GUID(0xcce33c35, 0x74ac, 0x4087, 0xbc, 0xe7, \
> >>> +		 0x8b, 0x29, 0xb0, 0x2e, 0xeb, 0x27)
> >>> +
> >>>    struct efi_conformance_profiles_table {
> >>>    	u16 version;
> >>>    	u16 number_of_profiles;
> >>> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index
> >>> b2398976f4..ab7476f68b 100644
> >>> --- a/lib/efi_loader/Kconfig
> >>> +++ b/lib/efi_loader/Kconfig
> >>> @@ -373,4 +373,10 @@ config EFI_ECPT
> >>>    	help
> >>>    	  Enabling this option created the ECPT UEFI table.
> >>>
> >>> +config EFI_EBBR_2_0_CONFORMANCE
> >>> +	bool "Add the EBBRv2.0 conformance entry to the ECPT table"
> >>> +	depends on EFI_ECPT
> >>
> >> With this dependency the symbol EFI_EBBR_2_0_CONFORMANCE is
> >> superfluous.
> >>
> >> Can we add EFI_EBBR_2_0_CONFORMANCE unconditionally or are there
> >> relevant configuration flags in U-Boot that must be enabled to claim
> >> EBBR 2.0 compliance? E.g.
> >>
> >> * CONFIG_CMD_BOOTEFI_BOOTMGR
> >> * CONFIG_EFI_GET_TIME
> >> * CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT
> >>
> > I've removed the "depends on" in PATCH v2.
> > Ideally the EFI_EBBR_2_0_CONFORMANCE should depend on all the
> CONFIGS required by EBBR 2.0.
> > I'm not sure whether this is feasible, i.e. whether there is a set of
> CONFIGS_* which when enabled make the implementation EBBRv2.0
> compliant.
> > Also, as the u-boot code evolves, these dependencies would need to be
> carefully maintained perhaps.
> >
> > Perhaps the best choice is to let the FW provider to set
> EBBR_2_0_CONFORMANCE in the platform config file once the FW has been
> deemed EBBRv2.0 compliant.
> 
> The firmware provider is the U-Boot project. If we do not know under which
> circumstances we might add the EBBR 2.0 conformance GUID, I prefer not to
> implement the table at all.
> 
The EBBR 2.0 conformance GUID can be an entry in the ECPT when the FW is EBBR v2.0 compliant.
The FW compliance with EBBRv2.0 can be determined by running the EBBR ACS (obtainable from https://github.com/ARM-software/bbr-acs).
Should we state this in the commit message, or perhaps as a comment in Config definition? In any case, the GUID inclusion criteria in the ECPT is not ambiguous. 

If we were to determine the EBBR2.0 GUID inclusion as a function of other u-boot configs, we'd potentially unnecessarily complicate the ECPT implementation in u-boot and also generate a maintenance burden as the codebase evolves. 

> Best regards
> 
> Heinrich
> 
> >>
> >>> +	default n
> >>> +	help
> >>> +	  Enabling this option adds the EBBRv2.0 conformance entry to the
> >> ECPT UEFI table.
> >>>    endif
> >>> diff --git a/lib/efi_loader/efi_conformance.c
> >>> b/lib/efi_loader/efi_conformance.c
> >>> index 86c26d6b79..b490ff3326 100644
> >>> --- a/lib/efi_loader/efi_conformance.c
> >>> +++ b/lib/efi_loader/efi_conformance.c
> >>> @@ -12,6 +12,7 @@
> >>>    #include <malloc.h>
> >>>
> >>>    const efi_guid_t efi_ecpt_guid =
> >>> EFI_CONFORMANCE_PROFILES_TABLE_GUID;
> >>> +const efi_guid_t efi_ebbr_2_0_guid =
> >>> +EFI_CONFORMANCE_PROFILE_EBBR_2_0_GUID;
> >>>
> >>>    #define EFI_CONFORMANCE_PROFILES_TABLE_VERSION 1
> >>>
> >>> @@ -29,6 +30,9 @@ efi_status_t efi_ecpt_register(void)
> >>>
> >>>    	EFI_PRINT("ECPT table creation start\n");
> >>>
> >>> +	if (IS_ENABLED(CONFIG_EFI_EBBR_2_0_CONFORMANCE))
> >>> +		num_entries++;
> >>> +
> >>>    	ecpt_size = num_entries * sizeof(efi_guid_t)
> >>>    		+ sizeof(struct efi_conformance_profiles_table);
> >>>    	ret = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, ecpt_size, @@ -
> >> 44,6
> >>> +48,11 @@ efi_status_t efi_ecpt_register(void)
> >>>    	ecpt->version = EFI_CONFORMANCE_PROFILES_TABLE_VERSION;
> >>>    	ecpt->number_of_profiles = num_entries;
> >>>
> >>> +	if (IS_ENABLED(CONFIG_EFI_EBBR_2_0_CONFORMANCE)) {
> >>> +		num_entries--;
> >>> +		guidcpy(&ecpt->conformance_profiles[num_entries],
> >> &efi_ecpt_guid);
> >>> +	}
> >>> +
> >>>    	if (num_entries)
> >>>    		EFI_PRINT("ECPT check conformance profiles, not all entries
> >>> populated in table\n");
> >>>
> >