[PATCH v2 13/17] efi_loader: add firmware management protocol for raw image
Sughosh Ganu
sughosh.ganu at linaro.org
Mon Jun 22 09:53:51 CEST 2020
On Mon, 22 Jun 2020 at 06:51, AKASHI Takahiro <takahiro.akashi at linaro.org>
wrote:
> On Sun, Jun 21, 2020 at 12:27:07AM +0530, Sughosh Ganu wrote:
> > On Wed, 17 Jun 2020 at 08:26, AKASHI Takahiro <
> takahiro.akashi at linaro.org>
> > wrote:
> >
> > > In this commit, a very simple firmware management protocol driver
> > > is implemented. It will take a binary image in a capsule file and
> > > apply the data using dfu backend storage drivers via dfu_write_by_alt()
> > > interface.
> > >
> > > So "dfu_alt_info" variable should be properly set to specify a device
> > > and location to be updated. Please read README.dfu.
> > >
> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> > > ---
> > > include/efi_api.h | 4 +
> > > include/efi_loader.h | 1 +
> > > lib/efi_loader/Kconfig | 16 ++
> > > lib/efi_loader/Makefile | 2 +-
> > > lib/efi_loader/efi_capsule.c | 8 +
> > > lib/efi_loader/efi_firmware.c | 273 ++++++++++++++++++++++++++++------
> > > 6 files changed, 256 insertions(+), 48 deletions(-)
> > >
> >
> > <snip>
> >
> >
> > > diff --git a/lib/efi_loader/efi_firmware.c
> b/lib/efi_loader/efi_firmware.c
> > > index 28ce5647a2cc..db8fdf30ace0 100644
> > > --- a/lib/efi_loader/efi_firmware.c
> > > +++ b/lib/efi_loader/efi_firmware.c
> > > @@ -13,6 +13,68 @@
> > > #include <image.h>
> > > #include <linux/list.h>
> > >
> > >
> > <snip>
> >
> >
> > > +#ifdef CONFIG_EFI_CAPSULE_FIRMWARE_RAW
> > > +/*
> > > + * This FIRMWARE_MANAGEMENT_PROTOCOL driver provides a firmware update
> > > + * method with raw data.
> > > + */
> > > +const efi_guid_t efi_firmware_image_type_uboot_raw =
> > > + EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID;
> > >
> > > -/* Place holder; not supported */
> > > +/**
> > > + * efi_firmware_raw_get_image_info - return information about the
> current
> > > + firmware image
> > > + * @this: Protocol instance
> > > + * @image_info_size: Size of @image_info
> > > + * @image_info: Image information
> > > + * @descriptor_version: Pointer to version number
> > > + * @descriptor_count: Pointer to number of descriptors
> > > + * @descriptor_size: Pointer to descriptor size
> > > + * package_version: Package version
> > > + * package_version_name: Package version's name
> > > + *
> > > + * Return information bout the current firmware image in @image_info.
> > > + * @image_info will consist of a number of descriptors.
> > > + * Each descriptor will be created based on "dfu_alt_info" variable.
> > > + *
> > > + * Return status code
> > > + */
> > > static
> > > -efi_status_t EFIAPI efi_firmware_get_package_info_unsupported(
> > > +efi_status_t EFIAPI efi_firmware_raw_get_image_info(
> > > struct efi_firmware_management_protocol *this,
> > > + efi_uintn_t *image_info_size,
> > > + struct efi_firmware_image_descriptor *image_info,
> > > + u32 *descriptor_version,
> > > + u8 *descriptor_count,
> > > + efi_uintn_t *descriptor_size,
> > > u32 *package_version,
> > > - u16 **package_version_name,
> > > - u32 *package_version_name_maxlen,
> > > - u64 *attributes_supported,
> > > - u64 *attributes_setting)
> > > + u16 **package_version_name)
> > > {
> > > - EFI_ENTRY("%p %p %p %p %p %p\n", this, package_version,
> > > - package_version_name, package_version_name_maxlen,
> > > - attributes_supported, attributes_setting);
> > > + struct dfu_entity *dfu;
> > > + size_t names_len, total_size;
> > > + int dfu_num, i;
> > > + u16 *name, *next;
> > > + efi_status_t ret = EFI_SUCCESS;
> > >
> > > - return EFI_EXIT(EFI_UNSUPPORTED);
> > > + EFI_ENTRY("%p %p %p %p %p %p %p %p\n", this,
> > > + image_info_size, image_info,
> > > + descriptor_version, descriptor_count,
> descriptor_size,
> > > + package_version, package_version_name);
> > > +
> > > + if (!image_info_size || (*image_info_size && !image_info))
> > > + return EFI_EXIT(EFI_INVALID_PARAMETER);
> > > +
> > > + dfu_init_env_entities(NULL, NULL);
> > > +
> > > + names_len = 0;
> > > + dfu_num = 0;
> > > + list_for_each_entry(dfu, &dfu_list, list) {
> > > + names_len += (utf8_utf16_strlen(dfu->name) + 1) * 2;
> > > + dfu_num++;
> > > + }
> > > + if (!dfu_num) {
> > > + EFI_PRINT("Probably dfu_alt_info not defined\n");
> > > + *image_info_size = 0;
> > > + dfu_free_entities();
> > > +
> > > + return EFI_EXIT(EFI_SUCCESS);
> > > + }
> > > +
> > > + total_size = sizeof(*image_info) * dfu_num + names_len;
> > > + /*
> > > + * we will assume that sizeof(*image_info) * dfu_name
> > > + * is, at least, a multiple of 2. So the start address for
> > > + * image_id_name would be aligned with 2 bytes.
> > > + */
> > > + if (*image_info_size < total_size) {
> > > + *image_info_size = total_size;
> > > + dfu_free_entities();
> > > +
> > > + return EFI_EXIT(EFI_BUFFER_TOO_SMALL);
> > > + }
> > > + *image_info_size = total_size;
> > > +
> > > + if (descriptor_version)
> > > + *descriptor_version =
> > > EFI_FIRMWARE_IMAGE_DESCRIPTOR_VERSION;
> > > + if (descriptor_count)
> > > + *descriptor_count = dfu_num;
> > > + if (descriptor_size)
> > > + *descriptor_size = sizeof(*image_info);
> > > + if (package_version)
> > > + *package_version = 0xffffffff; /* not supported */
> > > + if (package_version_name)
> > > + *package_version_name = NULL; /* not supported */
> > > +
> > > + /* DFU alt number should correspond to image_index */
> > > + i = 0;
> > > + /* Name area starts just after descriptors */
> > > + name = (u16 *)((u8 *)image_info + sizeof(*image_info) *
> dfu_num);
> > > + next = name;
> > > + list_for_each_entry(dfu, &dfu_list, list) {
> > > + image_info[i].image_index = dfu->alt + 1;
> > > + image_info[i].image_type_id =
> > > efi_firmware_image_type_uboot_raw;
> > > + image_info[i].image_id = dfu->alt;
> > > +
> > > + /* copy the DFU entity name */
> > > + utf8_utf16_strcpy(&next, dfu->name);
> > > + image_info[i].image_id_name = name;
> > > + name = ++next;
> > > +
> > > + image_info[i].version = 0; /* not supported */
> > > + image_info[i].version_name = NULL; /* not supported */
> > > + image_info[i].size = 0;
> > > + image_info[i].attributes_supported =
> > > + EFI_IMAGE_ATTRIBUTE_IMAGE_UPDATABLE;
> > > + image_info[i].attributes_setting =
> > > + EFI_IMAGE_ATTRIBUTE_IMAGE_UPDATABLE;
> > > + image_info[i].lowest_supported_image_version = 0;
> > > + image_info[i].last_attempt_version = 0;
> > > + image_info[i].last_attempt_status =
> > > LAST_ATTEMPT_STATUS_SUCCESS;
> > > + image_info[i].hardware_instance = 1;
> > > + image_info[i].dependencies = NULL;
> > > +
> > > + i++;
> > > + }
> > > +
> > > + dfu_free_entities();
> > > +
> > > + return EFI_EXIT(ret);
> > > }
> > >
> >
> > I think this function is the same as that used for the FIT image, except
> > for the image_type_id. I guess the common part can be refactored into a
> > common function.
>
> If you don't need to add anything here for your patch,
> I will extract a common function here.
>
Yes, please do.
>
> >
> > >
> > > -/* Place holder; not supported */
> > > +/**
> > > + * efi_firmware_raw_set_image - update the firmware image
> > > + * @this: Protocol instance
> > > + * @image_index: Image index number
> > > + * @image: New image
> > > + * @image_size: Size of new image
> > > + * @vendor_code: Vendor-specific update policy
> > > + * @progress: Function to report the progress of update
> > > + * @abort_reason: Pointer to string of abort reason
> > > + *
> > > + * Update the firmware to new image, using dfu. The new image should
> > > + * be a single raw image.
> > > + * @vendor_code, @progress and @abort_reason are not supported.
> > > + *
> > > + * Return: status code
> > > + */
> > > static
> > > -efi_status_t EFIAPI efi_firmware_set_package_info_unsupported(
> > > +efi_status_t EFIAPI efi_firmware_raw_set_image(
> > > struct efi_firmware_management_protocol *this,
> > > + u8 image_index,
> > > const void *image,
> > > - efi_uintn_t *image_size,
> > > + efi_uintn_t image_size,
> > > const void *vendor_code,
> > > - u32 package_version,
> > > - const u16 *package_version_name)
> > > + efi_status_t (*progress)(efi_uintn_t completion),
> > > + u16 **abort_reason)
> > > {
> > > - EFI_ENTRY("%p %p %p %p %x %p\n", this, image, image_size,
> > > vendor_code,
> > > - package_version, package_version_name);
> > > + EFI_ENTRY("%p %d %p %ld %p %p %p\n", this, image_index, image,
> > > + image_size, vendor_code, progress, abort_reason);
> > >
> > > - return EFI_EXIT(EFI_UNSUPPORTED);
> > > + if (!image)
> > > + return EFI_EXIT(EFI_INVALID_PARAMETER);
> > > +
> > > + if (dfu_write_by_alt(image_index - 1, (uintptr_t)image,
> image_size,
> > > + NULL, NULL))
> > >
> >
> > As stated in an earlier patch, we need to pass the image parameter as a
> > void pointer.
> >
> > + return EFI_EXIT(EFI_DEVICE_ERROR);
> > > +
> > > + return EFI_EXIT(EFI_SUCCESS);
> > > }
> > >
> >
> > A print, or at least a debug message stating the status of the capsule
> > update would be helpful. Same applies for the set_image implementation
> for
> > the FIT images as well.
>
> Well, we already have error messages in efi_capsule_update_firmware(),
> while another message can be placed specifically in efi_fmp_find().
>
I think that it would be good to have a debug message about the final
status of the update. A debug message would not hurt.
-sughosh
More information about the U-Boot
mailing list