[PATCH v5 4/4] efi: add EFI_DEBUG_IMAGE_INFO for debug

[Ilias Apalodimas]

[...]


>         }
> +
> +       if (IS_ENABLED(CONFIG_EFI_DEBUG_SUPPORT)) {
> +               if (*image_handle) {

Can you fold this in a single line if?

> +                       efi_core_new_debug_image_info_entry(EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL,
> +                                                           info,
> +                                                           *image_handle);
> +               }
> +       }
>  error:
>         return EFI_EXIT(ret);
>  }
> @@ -3359,6 +3367,9 @@ efi_status_t EFIAPI efi_unload_image(efi_handle_t image_handle)
>                 ret = EFI_INVALID_PARAMETER;
>                 goto out;
>         }
> +       if (IS_ENABLED(CONFIG_EFI_DEBUG_SUPPORT)) {
> +               efi_core_remove_debug_image_info_entry(image_handle);
> +       }

Drop the {}

>         switch (efiobj->type) {
>         case EFI_OBJECT_TYPE_STARTED_IMAGE:
>                 /* Call the unload function */
> diff --git a/lib/efi_loader/efi_debug_support.c b/lib/efi_loader/efi_debug_support.c
> index a6a672bc0e4..468e9e1cc83 100644
> --- a/lib/efi_loader/efi_debug_support.c
> +++ b/lib/efi_loader/efi_debug_support.c
> @@ -16,6 +16,13 @@ struct efi_debug_image_info_table_header efi_m_debug_info_table_header = {
>         NULL
>  };
>
> +/* efi_m_max_table_entries is the maximum entries allocated for
> + * the efi_m_debug_info_table_header.efi_debug_image_info_table.
> + */
> +static u32 efi_m_max_table_entries;
> +
> +#define EFI_DEBUG_TABLE_ENTRY_SIZE  (sizeof(union efi_debug_image_info *))
> +
>  /**
>   * efi_initialize_system_table_pointer() - Initialize system table pointer
>   *
> @@ -50,3 +57,125 @@ efi_status_t efi_initialize_system_table_pointer(void)
>
>         return EFI_SUCCESS;
>  }
> +
> +/**
> + * efi_core_new_debug_image_info_entry() - Add a new efi_loaded_image structure to the
> + *                                         efi_debug_image_info Table.
> + *
> + * @image_info_type: type of debug image information
> + * @loaded_image:    pointer to the loaded image protocol for the image
> + *                   being loaded
> + * @image_handle:    image handle for the image being loaded
> + *
> + * Re-Allocates the table if it's not large enough to accomidate another

s/accomidate/accomodate

> + * entry.
> + *
> + * Return: status code
> + **/
> +efi_status_t efi_core_new_debug_image_info_entry(u32 image_info_type,
> +                                                struct efi_loaded_image *loaded_image,
> +                                                efi_handle_t image_handle)
> +{
> +       union efi_debug_image_info **table;
> +       u32 index;
> +       u32 table_size;
> +       efi_status_t ret;
> +
> +       /* Set the flag indicating that we're in the process of updating
> +        * the table.
> +        */
> +       efi_m_debug_info_table_header.update_status |=
> +               EFI_DEBUG_IMAGE_INFO_UPDATE_IN_PROGRESS;
> +
> +       table = &efi_m_debug_info_table_header.efi_debug_image_info_table;
> +
> +       if (efi_m_debug_info_table_header.table_size >= efi_m_max_table_entries) {
> +               /* table is full, re-allocate the buffer increasing the size
> +                * by 4 KiB.
> +                */
> +               table_size = efi_m_max_table_entries * EFI_DEBUG_TABLE_ENTRY_SIZE;
> +
> +               ret = efi_realloc((void **)table, table_size + EFI_PAGE_SIZE);
> +
> +               if (ret != EFI_SUCCESS) {
> +                       efi_m_debug_info_table_header.update_status &=
> +                               ~EFI_DEBUG_IMAGE_INFO_UPDATE_IN_PROGRESS;
> +                       return ret;
> +               }
> +
> +               /* Enlarge the max table entries and set the first empty
> +                * entry index to be the original max table entries.
> +                */
> +               efi_m_max_table_entries +=
> +                       EFI_PAGE_SIZE / EFI_DEBUG_TABLE_ENTRY_SIZE;
> +       }
> +
> +       /* We always put the next entry at the end of the currently consumed
> +        * table (i.e. first free entry)
> +        */
> +       index = efi_m_debug_info_table_header.table_size;
> +
> +       /* Allocate data for new entry. */
> +       efi_allocate_pool(EFI_BOOT_SERVICES_DATA,
> +                         sizeof(union efi_debug_image_info),
> +                         (void **)(&(*table)[index].normal_image));
> +       if ((*table)[index].normal_image) {
> +               /* Update the entry. */
> +               (*table)[index].normal_image->image_info_type = image_info_type;
> +               (*table)[index].normal_image->loaded_image_protocol_instance =
> +                       loaded_image;
> +               (*table)[index].normal_image->image_handle = image_handle;
> +
> +               /* Increase the number of EFI_DEBUG_IMAGE_INFO elements and
> +                * set the efi_m_debug_info_table_header in modified status.
> +                */
> +               efi_m_debug_info_table_header.table_size++;
> +               efi_m_debug_info_table_header.update_status |=
> +                       EFI_DEBUG_IMAGE_INFO_TABLE_MODIFIED;
> +       }

Since it's a debug protocol I am ok with not unwinding all the
reallocations here, but don't you at least have to somehow warn the
consumer that updating the entry failed?

> +
> +       efi_m_debug_info_table_header.update_status &=
> +               ~EFI_DEBUG_IMAGE_INFO_UPDATE_IN_PROGRESS;
> +
> +       return EFI_SUCCESS;
> +}
> +

[...]

Thanks
/Ilias