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

[Heinrich Schuchardt]

On 12/31/21 15:36, Jose Marinho wrote:
> 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.

As the implementation of the EFI_HII_DATABASE_PROTOCOL in U-Boot does
not pass the SCT U-Boot does not comply to EBBR 2.0.

I see no use case for the ECPT table.

Best regards

Heinrich

>
>> 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");
>>>>>
>>>
>