Re: [RFC PATCH v2 5/8] EFI: FMP: Add provision to update image's ImageTypeId in image descriptor
hi Takahiro,
On Tue, 21 Dec 2021 at 10:23, AKASHI Takahiro
wrote:
> Sughosh,
>
> On Sun, Dec 19, 2021 at 12:36:02PM +0530, Sughosh Ganu wrote:
> > The FWU Multi Banks Update feature allows updating different types of
> > updatable firmware images on the platform. These image types are
> > identified using the ImageTypeId GUID value. Add support in the
> > GetImageInfo function of the FMP protocol to get the GUID values for
> > the individual images and populate these in the image descriptor for
> > the corresponding images.
> >
> > Signed-off-by: Sughosh Ganu
> > ---
> > Changes since V1:
> > * Define a new function fwu_plat_get_alt_num for filling up
> > all the dfu partitions with a preset ImageTypeId guid
> > * Remove the distinction made in the earlier version for
> > setting image_type_id as suggested by Heinrich
> >
> > lib/efi_loader/efi_firmware.c | 90 ---
> > 1 file changed, 83 insertions(+), 7 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_firmware.c
> b/lib/efi_loader/efi_firmware.c
> > index a1b88dbfc2..648342ae72 100644
> > --- a/lib/efi_loader/efi_firmware.c
> > +++ b/lib/efi_loader/efi_firmware.c
> > @@ -10,6 +10,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> >
> > @@ -96,6 +97,46 @@ efi_status_t EFIAPI
> efi_firmware_set_package_info_unsupported(
> > return EFI_EXIT(EFI_UNSUPPORTED);
> > }
> >
> > +static efi_status_t fill_part_guid_array(const efi_guid_t *guid,
> > + efi_guid_t **part_guid_arr)
> > +{
> > + int i;
> > + int dfu_num = 0;
> > + efi_guid_t *guid_arr;
> > + struct dfu_entity *dfu;
> > + efi_status_t ret = EFI_SUCCESS;
> > +
> > + dfu_init_env_entities(NULL, NULL);
> > +
> > + dfu_num = 0;
> > + list_for_each_entry(dfu, _list, list) {
> > + dfu_num++;
> > + }
> > +
> > + if (!dfu_num) {
> > + log_warning("Probably dfu_alt_info not defined\n");
> > + ret = EFI_NOT_READY;
> > + goto out;
> > + }
> > +
> > + *part_guid_arr = malloc(sizeof(efi_guid_t) * dfu_num);
> > + if (!*part_guid_arr) {
> > + ret = EFI_OUT_OF_RESOURCES;
> > + goto out;
> > + }
> > +
> > + guid_arr = *part_guid_arr;
> > + for (i = 0; i < dfu_num; i++) {
> > + guidcpy(guid_arr, guid);
> > + ++guid_arr;
> > + }
> > +
> > +out:
> > + dfu_free_entities();
> > +
> > + return ret;
> > +}
> > +
> > /**
> > * efi_get_dfu_info - return information about the current firmware
> image
> > * @this:Protocol instance
> > @@ -104,9 +145,9 @@ efi_status_t EFIAPI
> efi_firmware_set_package_info_unsupported(
> > * @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
> > - * image_type: Image type GUID
> > + * @package_version: Package version
> > + * @package_version_name:Package version's name
> > + * @guid_array: Image type GUID array
>
> If I understand your code correctly, a guid in descriptor for any image
> (I mean any part defined by dfu_alto_info) will have the same value
> even with this change.
> If so, why do we have to take an array of GUIDs as a parameter?
>
One of Heinrich's comments in my previous version was not to distinguish
between the FWU case and the normal case when setting the image_type_id.
The existing code sets the same GUID value(image_type) for all images. I
have kept this consistent. With the FWU feature enabled,
fwu_plat_fill_partition_guids is called and this function fills in the
array with the relevant values of partition GUIDs.
fwu_plat_fill_partition_guids has been defined for the ST board in patch
3/8.
> On your previous version, Heinrich made a comment like:
>
> ! The sequence of GUIDs in a capsule should not influence the update. The
> ! design lacks a configuration defining which GUID maps to which DFU part.
> !
> ! Please, get rid of all DFU environment variables and describe the update
> ! in a configuration file that you add to the capsule.
>
> Have you addressed this issue in this series?
> (To do that, we may have to introduce a new format of capsule file.)
>
No, Ilias and I had replied back that detaching capsule update from DFU is
a separate effort, not related to the addition of the FWU functionality and
should be taken up as a separate activity.
-sughosh
>
> -Takahiro Akashi
>
>
>
> > *
> > * Return information bout the current firmware image in @image_info.
> > * @image_info will consist of a number of descriptors.
> > @@ -122,7 +163,7 @@ static efi_status_t efi_get_dfu_info(
> > efi_uintn_t *descriptor_size,
> > u32
Re: [RFC PATCH v2 5/8] EFI: FMP: Add provision to update image's ImageTypeId in image descriptor
Sughosh,
On Sun, Dec 19, 2021 at 12:36:02PM +0530, Sughosh Ganu wrote:
> The FWU Multi Banks Update feature allows updating different types of
> updatable firmware images on the platform. These image types are
> identified using the ImageTypeId GUID value. Add support in the
> GetImageInfo function of the FMP protocol to get the GUID values for
> the individual images and populate these in the image descriptor for
> the corresponding images.
>
> Signed-off-by: Sughosh Ganu
> ---
> Changes since V1:
> * Define a new function fwu_plat_get_alt_num for filling up
> all the dfu partitions with a preset ImageTypeId guid
> * Remove the distinction made in the earlier version for
> setting image_type_id as suggested by Heinrich
>
> lib/efi_loader/efi_firmware.c | 90 ---
> 1 file changed, 83 insertions(+), 7 deletions(-)
>
> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> index a1b88dbfc2..648342ae72 100644
> --- a/lib/efi_loader/efi_firmware.c
> +++ b/lib/efi_loader/efi_firmware.c
> @@ -10,6 +10,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -96,6 +97,46 @@ efi_status_t EFIAPI
> efi_firmware_set_package_info_unsupported(
> return EFI_EXIT(EFI_UNSUPPORTED);
> }
>
> +static efi_status_t fill_part_guid_array(const efi_guid_t *guid,
> + efi_guid_t **part_guid_arr)
> +{
> + int i;
> + int dfu_num = 0;
> + efi_guid_t *guid_arr;
> + struct dfu_entity *dfu;
> + efi_status_t ret = EFI_SUCCESS;
> +
> + dfu_init_env_entities(NULL, NULL);
> +
> + dfu_num = 0;
> + list_for_each_entry(dfu, _list, list) {
> + dfu_num++;
> + }
> +
> + if (!dfu_num) {
> + log_warning("Probably dfu_alt_info not defined\n");
> + ret = EFI_NOT_READY;
> + goto out;
> + }
> +
> + *part_guid_arr = malloc(sizeof(efi_guid_t) * dfu_num);
> + if (!*part_guid_arr) {
> + ret = EFI_OUT_OF_RESOURCES;
> + goto out;
> + }
> +
> + guid_arr = *part_guid_arr;
> + for (i = 0; i < dfu_num; i++) {
> + guidcpy(guid_arr, guid);
> + ++guid_arr;
> + }
> +
> +out:
> + dfu_free_entities();
> +
> + return ret;
> +}
> +
> /**
> * efi_get_dfu_info - return information about the current firmware image
> * @this:Protocol instance
> @@ -104,9 +145,9 @@ efi_status_t EFIAPI
> efi_firmware_set_package_info_unsupported(
> * @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
> - * image_type: Image type GUID
> + * @package_version: Package version
> + * @package_version_name:Package version's name
> + * @guid_array: Image type GUID array
If I understand your code correctly, a guid in descriptor for any image
(I mean any part defined by dfu_alto_info) will have the same value
even with this change.
If so, why do we have to take an array of GUIDs as a parameter?
On your previous version, Heinrich made a comment like:
! The sequence of GUIDs in a capsule should not influence the update. The
! design lacks a configuration defining which GUID maps to which DFU part.
!
! Please, get rid of all DFU environment variables and describe the update
! in a configuration file that you add to the capsule.
Have you addressed this issue in this series?
(To do that, we may have to introduce a new format of capsule file.)
-Takahiro Akashi
> *
> * Return information bout the current firmware image in @image_info.
> * @image_info will consist of a number of descriptors.
> @@ -122,7 +163,7 @@ static efi_status_t efi_get_dfu_info(
> efi_uintn_t *descriptor_size,
> u32 *package_version,
> u16 **package_version_name,
> - const efi_guid_t *image_type)
> + const efi_guid_t *guid_array)
> {
> struct dfu_entity *dfu;
> size_t names_len, total_size;
> @@ -172,7 +213,7 @@ static efi_status_t efi_get_dfu_info(
> next = name;
> list_for_each_entry(dfu, _list, list) {
> image_info[i].image_index = dfu->alt + 1;
> - image_info[i].image_type_id = *image_type;
> + image_info[i].image_type_id = guid_array[i];
> image_info[i].image_id = dfu->alt;
>
> /* copy the DFU entity name */
> @@ -250,6 +291,7 @@ efi_status_t EFIAPI efi_firmware_fit_get_image_info(
> u16 **package_version_name)
> {
> efi_status_t ret;
> + efi_guid_t *part_guid_arr = NULL;
>
> EFI_ENTRY("%p %p %p %p %p %p %p %p\n", this,
> image_info_size, image_info,
> @@ -264,12 +306,19 @@ efi_status_t EFIAPI efi_firmware_fit_get_image_info(
>
