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

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

On 1/2/22 00:56, Heinrich Schuchardt wrote:
>
>
> On 12/31/21 15:36, Jose Marinho wrote:
>> Hi Heinrich,
>>
>>> -----Original Message-----
>>> From: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> Sent: 23 December 2021 18:32
>>> To: Jose Marinho <Jose.Marinho@arm.com>; u-boot@lists.denx.de
>>> Cc: ilias.apalodimas@linaro.org; sughosh.ganu@linaro.org;
>>> takahiro.akashi@linaro.org; agraf@csgraf.de; nd <nd@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@gmx.de>
>>>>> Sent: 17 December 2021 17:27
>>>>> To: Jose Marinho <Jose.Marinho@arm.com>; u-boot@lists.denx.de
>>>>> Cc: ilias.apalodimas@linaro.org; sughosh.ganu@linaro.org;
>>>>> takahiro.akashi@linaro.org; agraf@csgraf.de; nd <nd@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@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.

UEFI 2.10 now has defined the EFI_CONFORMANCE_PROFILE_TABLE. But it does
not provide a GUID for EBBR. EBBR does not define one either. I have
created https://github.com/ARM-software/ebbr/issues/95. Once the EBBR
community agrees on a GUID we can add the table.

Best regards

Heinrich

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