Re: [PATCH 1/3] efi: Create ECPT table

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

On 12/17/21 13:55, Jose Marinho wrote:
> The ECPT table will be included in the UEFI specification 2.9+.
> The ECPT table was introduced in UEFI following the code-first path. The
> acceptance ticket can be viewed at:
> 	https://bugzilla.tianocore.org/show_bug.cgi?id=3591
>
> The Conformance Profiles table is a UEFI configuration table that contains
> GUID of the UEFI profiles that the UEFI implementation conforms with.
>
> The ECPT table is created when CONFIG_EFI_ECPT=y.
> The config is set by default.
>
> Signed-off-by: Jose Marinho <jose.marinho@arm.com>
> ---
>   cmd/efidebug.c                   |  4 ++
>   include/efi_api.h                | 10 +++++
>   include/efi_loader.h             |  7 ++++
>   lib/efi_loader/Kconfig           |  6 +++
>   lib/efi_loader/Makefile          |  1 +
>   lib/efi_loader/efi_conformance.c | 66 ++++++++++++++++++++++++++++++++
>   lib/efi_loader/efi_setup.c       |  6 +++
>   7 files changed, 100 insertions(+)
>   create mode 100644 lib/efi_loader/efi_conformance.c
>
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> index a977ca9c72..a53a5029fa 100644
> --- a/cmd/efidebug.c
> +++ b/cmd/efidebug.c
> @@ -619,6 +619,10 @@ static const struct {
>   		"TCG2 Final Events Table",
>   		EFI_TCG2_FINAL_EVENTS_TABLE_GUID,
>   	},
> +	{
> +		"EFI Conformance Profiles Table",
> +		EFI_CONFORMANCE_PROFILES_TABLE_GUID,

Just as side note. Consider sending a patch for GRUB to amend:

grub-core/commands/efi/lsefisystab.c
include/grub/efi/api.h

> +	},
>   };
>
>   /**
> diff --git a/include/efi_api.h b/include/efi_api.h
> index 80109f012b..6fd4f04de3 100644
> --- a/include/efi_api.h
> +++ b/include/efi_api.h
> @@ -226,6 +226,16 @@ enum efi_reset_type {
>   	EFI_GUID(0x6dcbd5ed, 0xe82d, 0x4c44, 0xbd, 0xa1, \
>   		 0x71, 0x94, 0x19, 0x9a, 0xd9, 0x2a)
>
> +#define EFI_CONFORMANCE_PROFILES_TABLE_GUID \
> +	EFI_GUID(0x36122546, 0xf7ef, 0x4c8f, 0xbd, 0x9b, \
> +		 0xeb, 0x85, 0x25, 0xb5, 0x0c, 0x0b)
> +
> +struct efi_conformance_profiles_table {
> +	u16 version;
> +	u16 number_of_profiles;
> +	efi_guid_t	conformance_profiles[];
> +} __packed;
> +
>   struct efi_capsule_header {
>   	efi_guid_t capsule_guid;
>   	u32 header_size;
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index d52e399841..d20ff396d0 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -976,6 +976,13 @@ efi_status_t efi_capsule_authenticate(const void *capsule,
>    */
>   efi_status_t efi_esrt_register(void);
>
> +/**
> + * efi_ecpt_register() - Install the ECPT system table.
> + *
> + * Return: status code
> + */
> +efi_status_t efi_ecpt_register(void);
> +
>   /**
>    * efi_esrt_populate() - Populates the ESRT entries from the FMP instances
>    * present in the system.
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index 700dc838dd..b2398976f4 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -367,4 +367,10 @@ config EFI_ESRT
>   	help
>   	  Enabling this option creates the ESRT UEFI system table.
>
> +config EFI_ECPT
> +	bool "Enable the UEFI ECPT generation"
> +	default y
> +	help
> +	  Enabling this option created the ECPT UEFI table.
> +
>   endif
> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> index fd344cea29..9f5a0cebd1 100644
> --- a/lib/efi_loader/Makefile
> +++ b/lib/efi_loader/Makefile
> @@ -64,6 +64,7 @@ obj-$(CONFIG_EFI_RNG_PROTOCOL) += efi_rng.o
>   obj-$(CONFIG_EFI_TCG2_PROTOCOL) += efi_tcg2.o
>   obj-$(CONFIG_EFI_LOAD_FILE2_INITRD) += efi_load_initrd.o
>   obj-$(CONFIG_EFI_SIGNATURE_SUPPORT) += efi_signature.o
> +obj-$(CONFIG_EFI_ECPT) += efi_conformance.o
>
>   EFI_VAR_SEED_FILE := $(subst $\",,$(CONFIG_EFI_VAR_SEED_FILE))
>   $(obj)/efi_var_seed.o: $(srctree)/$(EFI_VAR_SEED_FILE)
> diff --git a/lib/efi_loader/efi_conformance.c b/lib/efi_loader/efi_conformance.c
> new file mode 100644
> index 0000000000..86c26d6b79
> --- /dev/null
> +++ b/lib/efi_loader/efi_conformance.c
> @@ -0,0 +1,66 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + *  EFI conformance profile table
> + *
> + *  Copyright (C) 2022 Arm Ltd.
> + */
> +
> +#include <common.h>
> +#include <efi_loader.h>
> +#include <log.h>
> +#include <efi_api.h>
> +#include <malloc.h>
> +
> +const efi_guid_t efi_ecpt_guid = EFI_CONFORMANCE_PROFILES_TABLE_GUID;
> +
> +#define EFI_CONFORMANCE_PROFILES_TABLE_VERSION 1
> +
> +/**
> + * efi_ecpt_register() - Install the ECPT system table.
> + *
> + * Return: status code
> + */
> +efi_status_t efi_ecpt_register(void)
> +{
> +	int num_entries = 0;

Shouldn't we add EFI_CONFORMANCE_PROFILES_EBBR_2_0_GUID as entry?
What would be the point of adding an empty ECPT table?

> +	struct efi_conformance_profiles_table *ecpt;
> +	efi_status_t ret;
> +	size_t ecpt_size = 0;
> +
> +	EFI_PRINT("ECPT table creation start\n");

This function is not called via the EFI API. Please, use log_debug().

Best regards

Heinrich

> +
> +	ecpt_size = num_entries * sizeof(efi_guid_t)
> +		+ sizeof(struct efi_conformance_profiles_table);
> +	ret = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, ecpt_size,
> +				(void **)&ecpt);
> +
> +	if (ret != EFI_SUCCESS) {
> +		EFI_PRINT("ECPT cannot allocate memory for %u entries (%zu bytes)\n",
> +			  num_entries, ecpt_size);
> +
> +		return ret;
> +	}
> +
> +	ecpt->version = EFI_CONFORMANCE_PROFILES_TABLE_VERSION;
> +	ecpt->number_of_profiles = num_entries;
> +
> +	if (num_entries)
> +		EFI_PRINT("ECPT check conformance profiles, not all entries populated in table\n");
> +
> +	/* Install the ECPT in the system configuration table. */
> +	ret = efi_install_configuration_table(&efi_ecpt_guid, (void *)ecpt);
> +	if (ret != EFI_SUCCESS) {
> +		EFI_PRINT("ECPT failed to install the ECPT in the system table\n");
> +		goto error;
> +	}
> +
> +	EFI_PRINT("ECPT table successfully created\n");
> +
> +	return ret;
> +
> +error:
> +
> +	ret = efi_free_pool(ecpt);
> +
> +	return ret;
> +}
> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> index 1aba71cd96..fa5ad13500 100644
> --- a/lib/efi_loader/efi_setup.c
> +++ b/lib/efi_loader/efi_setup.c
> @@ -231,6 +231,12 @@ efi_status_t efi_init_obj_list(void)
>   	if (ret != EFI_SUCCESS)
>   		goto out;
>
> +	if (IS_ENABLED(CONFIG_EFI_ECPT)) {
> +		ret = efi_ecpt_register();
> +		if (ret != EFI_SUCCESS)
> +			goto out;
> +	}
> +
>   	if (IS_ENABLED(CONFIG_EFI_ESRT)) {
>   		ret = efi_esrt_register();
>   		if (ret != EFI_SUCCESS)