| Message ID | 20220207182001.31270-6-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | FWU: Add support for FWU Multi Bank Update feature | expand |
Hi Sughosh, On Mon, Feb 07, 2022 at 11:49:55PM +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. After re-thinking of your approach here, I would have to say NAK. You use ImageTypeId to identify a particular firmware object. (By "object," I mean one of firmware instances represented by "dfu_alto_info". Please don't confuse it with the binary blob embedded in a capsule file.) But ImageTypeId is not for that purpose, at least, as my intention in initially implementing capsule framework and FMP drivers. 1) ImageTypeId is used to uniquely identify a corresponding FMP driver, either FIT FMP driver or Raw FMP driver. 2) Each firmware object handled by a given FMP driver can further be identified by ImageIndex. My implementation of efi_fmp_find() does (1) and Raw FMP driver does (2) in efi_firmware_raw_set_image() which takes "image_index" as a parameter. Using ImageTypeId as an identifier is simply wrong in my opinion and doesn't meet the UEFI specification. -Takahiro Akashi > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > --- > > Changes since V3: > * Define a weak function fill_image_type_guid_array for populating the > image descriptor array with u-boot's raw and fit image GUIDs > > include/efi_loader.h | 2 + > lib/efi_loader/efi_firmware.c | 71 +++++++++++++++++++++++++++++++---- > 2 files changed, 66 insertions(+), 7 deletions(-) > > diff --git a/include/efi_loader.h b/include/efi_loader.h > index f4860e87fc..ae60de0be5 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -992,4 +992,6 @@ efi_status_t efi_esrt_populate(void); > efi_status_t efi_load_capsule_drivers(void); > > efi_status_t platform_get_eventlog(struct udevice *dev, u64 *addr, u32 *sz); > +efi_status_t fill_image_type_guid_array(const efi_guid_t *default_guid, > + efi_guid_t **part_guid_arr); > #endif /* _EFI_LOADER_H */ > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c > index a1b88dbfc2..5642be9f9a 100644 > --- a/lib/efi_loader/efi_firmware.c > +++ b/lib/efi_loader/efi_firmware.c > @@ -96,6 +96,46 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported( > return EFI_EXIT(EFI_UNSUPPORTED); > } > > +efi_status_t __weak fill_image_type_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, &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 +144,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 > * > * Return information bout the current firmware image in @image_info. > * @image_info will consist of a number of descriptors. > @@ -122,7 +162,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 +212,7 @@ static efi_status_t efi_get_dfu_info( > next = name; > list_for_each_entry(dfu, &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 +290,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 +305,19 @@ efi_status_t EFIAPI efi_firmware_fit_get_image_info( > !descriptor_size || !package_version || !package_version_name)) > return EFI_EXIT(EFI_INVALID_PARAMETER); > > + ret = fill_image_type_guid_array(&efi_firmware_image_type_uboot_fit, > + &part_guid_arr); > + if (ret != EFI_SUCCESS) > + goto out; > + > ret = efi_get_dfu_info(image_info_size, image_info, > descriptor_version, descriptor_count, > descriptor_size, > package_version, package_version_name, > - &efi_firmware_image_type_uboot_fit); > + part_guid_arr); > > +out: > + free(part_guid_arr); > return EFI_EXIT(ret); > } > > @@ -359,6 +407,7 @@ efi_status_t EFIAPI efi_firmware_raw_get_image_info( > u16 **package_version_name) > { > efi_status_t ret = EFI_SUCCESS; > + efi_guid_t *part_guid_arr = NULL; > > EFI_ENTRY("%p %p %p %p %p %p %p %p\n", this, > image_info_size, image_info, > @@ -373,12 +422,20 @@ efi_status_t EFIAPI efi_firmware_raw_get_image_info( > !descriptor_size || !package_version || !package_version_name)) > return EFI_EXIT(EFI_INVALID_PARAMETER); > > + ret = fill_image_type_guid_array( > + &efi_firmware_image_type_uboot_raw, > + &part_guid_arr); > + if (ret != EFI_SUCCESS) > + goto out; > + > ret = efi_get_dfu_info(image_info_size, image_info, > descriptor_version, descriptor_count, > descriptor_size, > package_version, package_version_name, > - &efi_firmware_image_type_uboot_raw); > + part_guid_arr); > > +out: > + free(part_guid_arr); > return EFI_EXIT(ret); > } > > -- > 2.17.1 >
hi Takahiro, On Thu, 10 Feb 2022 at 08:18, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > Hi Sughosh, > > On Mon, Feb 07, 2022 at 11:49:55PM +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. > > After re-thinking of your approach here, I would have to say NAK. > > You use ImageTypeId to identify a particular firmware object. > (By "object," I mean one of firmware instances represented by "dfu_alto_info". > Please don't confuse it with the binary blob embedded in a capsule file.) > But ImageTypeId is not for that purpose, at least, as my intention > in initially implementing capsule framework and FMP drivers. > > 1) ImageTypeId is used to uniquely identify a corresponding FMP driver, > either FIT FMP driver or Raw FMP driver. I believe the identification of an FMP protocol should be done by the FMP GUID, which is what is done in efi_fmp_find. The ImageTypeId is nowhere involved in this identification. > 2) Each firmware object handled by a given FMP driver can further be > identified by ImageIndex. > > My implementation of efi_fmp_find() does (1) and Raw FMP driver does > (2) in efi_firmware_raw_set_image() which takes "image_index" as > a parameter. > > Using ImageTypeId as an identifier is simply wrong in my opinion and > doesn't meet the UEFI specification. So, as per what you are stating, all payloads under a given EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER should have the same ImageTypeId, either EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID. Same applies for all images in the EFI_FIRMWARE_IMAGE_DESCRIPTOR. Because, without one of the two values, the check in efi_fmp_find to compare the UpdateImageTypeId with the ImageTypeId retrieved from the image descriptor would simply fail. I think this interpretation of the UEFI spec is incorrect, since the spec states that the ImageTypeId and the UpdateImageTypeId are fields used to identify the firmware component targeted for the update. If all values in the image descriptor array and the UpdateImageTypeId are the same, why have this field in the first place for individual images. -sughosh > > -Takahiro Akashi > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > --- > > > > Changes since V3: > > * Define a weak function fill_image_type_guid_array for populating the > > image descriptor array with u-boot's raw and fit image GUIDs > > > > include/efi_loader.h | 2 + > > lib/efi_loader/efi_firmware.c | 71 +++++++++++++++++++++++++++++++---- > > 2 files changed, 66 insertions(+), 7 deletions(-) > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > index f4860e87fc..ae60de0be5 100644 > > --- a/include/efi_loader.h > > +++ b/include/efi_loader.h > > @@ -992,4 +992,6 @@ efi_status_t efi_esrt_populate(void); > > efi_status_t efi_load_capsule_drivers(void); > > > > efi_status_t platform_get_eventlog(struct udevice *dev, u64 *addr, u32 *sz); > > +efi_status_t fill_image_type_guid_array(const efi_guid_t *default_guid, > > + efi_guid_t **part_guid_arr); > > #endif /* _EFI_LOADER_H */ > > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c > > index a1b88dbfc2..5642be9f9a 100644 > > --- a/lib/efi_loader/efi_firmware.c > > +++ b/lib/efi_loader/efi_firmware.c > > @@ -96,6 +96,46 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported( > > return EFI_EXIT(EFI_UNSUPPORTED); > > } > > > > +efi_status_t __weak fill_image_type_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, &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 +144,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 > > * > > * Return information bout the current firmware image in @image_info. > > * @image_info will consist of a number of descriptors. > > @@ -122,7 +162,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 +212,7 @@ static efi_status_t efi_get_dfu_info( > > next = name; > > list_for_each_entry(dfu, &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 +290,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 +305,19 @@ efi_status_t EFIAPI efi_firmware_fit_get_image_info( > > !descriptor_size || !package_version || !package_version_name)) > > return EFI_EXIT(EFI_INVALID_PARAMETER); > > > > + ret = fill_image_type_guid_array(&efi_firmware_image_type_uboot_fit, > > + &part_guid_arr); > > + if (ret != EFI_SUCCESS) > > + goto out; > > + > > ret = efi_get_dfu_info(image_info_size, image_info, > > descriptor_version, descriptor_count, > > descriptor_size, > > package_version, package_version_name, > > - &efi_firmware_image_type_uboot_fit); > > + part_guid_arr); > > > > +out: > > + free(part_guid_arr); > > return EFI_EXIT(ret); > > } > > > > @@ -359,6 +407,7 @@ efi_status_t EFIAPI efi_firmware_raw_get_image_info( > > u16 **package_version_name) > > { > > efi_status_t ret = EFI_SUCCESS; > > + efi_guid_t *part_guid_arr = NULL; > > > > EFI_ENTRY("%p %p %p %p %p %p %p %p\n", this, > > image_info_size, image_info, > > @@ -373,12 +422,20 @@ efi_status_t EFIAPI efi_firmware_raw_get_image_info( > > !descriptor_size || !package_version || !package_version_name)) > > return EFI_EXIT(EFI_INVALID_PARAMETER); > > > > + ret = fill_image_type_guid_array( > > + &efi_firmware_image_type_uboot_raw, > > + &part_guid_arr); > > + if (ret != EFI_SUCCESS) > > + goto out; > > + > > ret = efi_get_dfu_info(image_info_size, image_info, > > descriptor_version, descriptor_count, > > descriptor_size, > > package_version, package_version_name, > > - &efi_firmware_image_type_uboot_raw); > > + part_guid_arr); > > > > +out: > > + free(part_guid_arr); > > return EFI_EXIT(ret); > > } > > > > -- > > 2.17.1 > >
On Thu, Feb 10, 2022 at 12:48:13PM +0530, Sughosh Ganu wrote: > hi Takahiro, > > On Thu, 10 Feb 2022 at 08:18, AKASHI Takahiro > <takahiro.akashi@linaro.org> wrote: > > > > Hi Sughosh, > > > > On Mon, Feb 07, 2022 at 11:49:55PM +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. > > > > After re-thinking of your approach here, I would have to say NAK. > > > > You use ImageTypeId to identify a particular firmware object. > > (By "object," I mean one of firmware instances represented by "dfu_alto_info". > > Please don't confuse it with the binary blob embedded in a capsule file.) > > But ImageTypeId is not for that purpose, at least, as my intention > > in initially implementing capsule framework and FMP drivers. > > > > 1) ImageTypeId is used to uniquely identify a corresponding FMP driver, > > either FIT FMP driver or Raw FMP driver. > > I believe the identification of an FMP protocol should be done by the > FMP GUID, What does FMP GUID stand for? > which is what is done in efi_fmp_find. The ImageTypeId is > nowhere involved in this identification. Please take a look at efi_capsule_update_firmware() carefully. efi_find_fmp() is called with the image's update_image_type_id which is to be set to EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID by mkeficapsule (see create_fwbin()). > > 2) Each firmware object handled by a given FMP driver can further be > > identified by ImageIndex. > > > > My implementation of efi_fmp_find() does (1) and Raw FMP driver does > > (2) in efi_firmware_raw_set_image() which takes "image_index" as > > a parameter. > > > > Using ImageTypeId as an identifier is simply wrong in my opinion and > > doesn't meet the UEFI specification. > > So, as per what you are stating, all payloads under a given > EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER should have the same > ImageTypeId, either EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID. Same applies for all images in > the EFI_FIRMWARE_IMAGE_DESCRIPTOR. Because, without one of the two > values, > the check in efi_fmp_find to compare the UpdateImageTypeId > with the ImageTypeId retrieved from the image descriptor would simply > fail. I don't follow your point. Please elaborate a bit more. > I think this interpretation of the UEFI spec is incorrect, since the > spec states that the ImageTypeId and the UpdateImageTypeId are fields > used to identify the firmware component targeted for the update. If > all values in the image descriptor array and the UpdateImageTypeId are > the same, why have this field in the first place for individual > images. As I said, ImageIndex is for that purpose. -Takahiro Akashi > > -sughosh > > > > > -Takahiro Akashi > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > --- > > > > > > Changes since V3: > > > * Define a weak function fill_image_type_guid_array for populating the > > > image descriptor array with u-boot's raw and fit image GUIDs > > > > > > include/efi_loader.h | 2 + > > > lib/efi_loader/efi_firmware.c | 71 +++++++++++++++++++++++++++++++---- > > > 2 files changed, 66 insertions(+), 7 deletions(-) > > > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > > index f4860e87fc..ae60de0be5 100644 > > > --- a/include/efi_loader.h > > > +++ b/include/efi_loader.h > > > @@ -992,4 +992,6 @@ efi_status_t efi_esrt_populate(void); > > > efi_status_t efi_load_capsule_drivers(void); > > > > > > efi_status_t platform_get_eventlog(struct udevice *dev, u64 *addr, u32 *sz); > > > +efi_status_t fill_image_type_guid_array(const efi_guid_t *default_guid, > > > + efi_guid_t **part_guid_arr); > > > #endif /* _EFI_LOADER_H */ > > > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c > > > index a1b88dbfc2..5642be9f9a 100644 > > > --- a/lib/efi_loader/efi_firmware.c > > > +++ b/lib/efi_loader/efi_firmware.c > > > @@ -96,6 +96,46 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported( > > > return EFI_EXIT(EFI_UNSUPPORTED); > > > } > > > > > > +efi_status_t __weak fill_image_type_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, &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 +144,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 > > > * > > > * Return information bout the current firmware image in @image_info. > > > * @image_info will consist of a number of descriptors. > > > @@ -122,7 +162,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 +212,7 @@ static efi_status_t efi_get_dfu_info( > > > next = name; > > > list_for_each_entry(dfu, &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 +290,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 +305,19 @@ efi_status_t EFIAPI efi_firmware_fit_get_image_info( > > > !descriptor_size || !package_version || !package_version_name)) > > > return EFI_EXIT(EFI_INVALID_PARAMETER); > > > > > > + ret = fill_image_type_guid_array(&efi_firmware_image_type_uboot_fit, > > > + &part_guid_arr); > > > + if (ret != EFI_SUCCESS) > > > + goto out; > > > + > > > ret = efi_get_dfu_info(image_info_size, image_info, > > > descriptor_version, descriptor_count, > > > descriptor_size, > > > package_version, package_version_name, > > > - &efi_firmware_image_type_uboot_fit); > > > + part_guid_arr); > > > > > > +out: > > > + free(part_guid_arr); > > > return EFI_EXIT(ret); > > > } > > > > > > @@ -359,6 +407,7 @@ efi_status_t EFIAPI efi_firmware_raw_get_image_info( > > > u16 **package_version_name) > > > { > > > efi_status_t ret = EFI_SUCCESS; > > > + efi_guid_t *part_guid_arr = NULL; > > > > > > EFI_ENTRY("%p %p %p %p %p %p %p %p\n", this, > > > image_info_size, image_info, > > > @@ -373,12 +422,20 @@ efi_status_t EFIAPI efi_firmware_raw_get_image_info( > > > !descriptor_size || !package_version || !package_version_name)) > > > return EFI_EXIT(EFI_INVALID_PARAMETER); > > > > > > + ret = fill_image_type_guid_array( > > > + &efi_firmware_image_type_uboot_raw, > > > + &part_guid_arr); > > > + if (ret != EFI_SUCCESS) > > > + goto out; > > > + > > > ret = efi_get_dfu_info(image_info_size, image_info, > > > descriptor_version, descriptor_count, > > > descriptor_size, > > > package_version, package_version_name, > > > - &efi_firmware_image_type_uboot_raw); > > > + part_guid_arr); > > > > > > +out: > > > + free(part_guid_arr); > > > return EFI_EXIT(ret); > > > } > > > > > > -- > > > 2.17.1 > > >
On Thu, 10 Feb 2022 at 13:28, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > On Thu, Feb 10, 2022 at 12:48:13PM +0530, Sughosh Ganu wrote: > > hi Takahiro, > > > > On Thu, 10 Feb 2022 at 08:18, AKASHI Takahiro > > <takahiro.akashi@linaro.org> wrote: > > > > > > Hi Sughosh, > > > > > > On Mon, Feb 07, 2022 at 11:49:55PM +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. > > > > > > After re-thinking of your approach here, I would have to say NAK. > > > > > > You use ImageTypeId to identify a particular firmware object. > > > (By "object," I mean one of firmware instances represented by "dfu_alto_info". > > > Please don't confuse it with the binary blob embedded in a capsule file.) > > > But ImageTypeId is not for that purpose, at least, as my intention > > > in initially implementing capsule framework and FMP drivers. > > > > > > 1) ImageTypeId is used to uniquely identify a corresponding FMP driver, > > > either FIT FMP driver or Raw FMP driver. > > > > I believe the identification of an FMP protocol should be done by the > > FMP GUID, > > What does FMP GUID stand for? EFI_FIRMWARE_MANAGEMENT_PROTOCOL_GUID, defined in include/efi_api.h. What I mean is that even when installing the FMP protocol, the call to efi_install_multiple_protocol_interfaces takes the above FMP GUID as an argument -- nowhere is the ImageTypeId considered when installing the protocol. > > > which is what is done in efi_fmp_find. The ImageTypeId is > > nowhere involved in this identification. > > Please take a look at efi_capsule_update_firmware() carefully. > efi_find_fmp() is called with the image's update_image_type_id > which is to be set to EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID by mkeficapsule > (see create_fwbin()). I think you are thinking from the point of view of the '--guid' value that is being passed to the capsule generation tool. But the thing is that it is the current design(or limitation) of the tool that it takes only a single guid parameter. So the mkeficapsule tool currently can generate only a single payload capsule. Please check the GenerateCapsule script in EDK2. In case of a multi payload based capsule, individual parameters like the UpdateImageTypeId are passed through the json file, where each of the UpdateImageTypeId has a different value per payload. > > > > 2) Each firmware object handled by a given FMP driver can further be > > > identified by ImageIndex. > > > > > > My implementation of efi_fmp_find() does (1) and Raw FMP driver does > > > (2) in efi_firmware_raw_set_image() which takes "image_index" as > > > a parameter. > > > > > > Using ImageTypeId as an identifier is simply wrong in my opinion and > > > doesn't meet the UEFI specification. > > > > So, as per what you are stating, all payloads under a given > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER should have the same > > ImageTypeId, either EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID. Same applies for all images in > > the EFI_FIRMWARE_IMAGE_DESCRIPTOR. Because, without one of the two > > values, > the check in efi_fmp_find to compare the UpdateImageTypeId > > with the ImageTypeId retrieved from the image descriptor would simply > > fail. > > I don't follow your point. > Please elaborate a bit more. The current implementation of GetImageInfo, passes either of EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID for all the images of the image descriptor array. So, in case the capsule is generated with a '--guid' value which is different from these two values, the check in efi_fmp_find on line 204 will fail. This means that unless the --guid value passed to the capsule generation is either of u-boot FIT or u-boot raw, the current FMP protocol for raw devices cannot be used. Why do we need that restriction. It should be possible to use the raw FMP protocol for any other type of image types as well. > > > I think this interpretation of the UEFI spec is incorrect, since the > > spec states that the ImageTypeId and the UpdateImageTypeId are fields > > used to identify the firmware component targeted for the update. If > > all values in the image descriptor array and the UpdateImageTypeId are > > the same, why have this field in the first place for individual > > images. > > As I said, ImageIndex is for that purpose. Yes, that is one possible way in the scenario where the ImageIndex is determined at the capsule generation time. But, for the A/B update scenario, we do not know the ImageIndex at build time -- this is determined only at runtime, and is based on the bank to which the image is to be updated. Which is why I am finding out the alt_num at runtime in case the FWU Multi Bank feature is enabled. Like I said above, I do not see a reason why the current FMP protocols should be restricted to only the u-boot FIT and u-boot raw image types. It is being extended, without affecting the default non FWU behaviour. -sughosh > > -Takahiro Akashi > > > > > -sughosh > > > > > > > > -Takahiro Akashi > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > --- > > > > > > > > Changes since V3: > > > > * Define a weak function fill_image_type_guid_array for populating the > > > > image descriptor array with u-boot's raw and fit image GUIDs > > > > > > > > include/efi_loader.h | 2 + > > > > lib/efi_loader/efi_firmware.c | 71 +++++++++++++++++++++++++++++++---- > > > > 2 files changed, 66 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > > > index f4860e87fc..ae60de0be5 100644 > > > > --- a/include/efi_loader.h > > > > +++ b/include/efi_loader.h > > > > @@ -992,4 +992,6 @@ efi_status_t efi_esrt_populate(void); > > > > efi_status_t efi_load_capsule_drivers(void); > > > > > > > > efi_status_t platform_get_eventlog(struct udevice *dev, u64 *addr, u32 *sz); > > > > +efi_status_t fill_image_type_guid_array(const efi_guid_t *default_guid, > > > > + efi_guid_t **part_guid_arr); > > > > #endif /* _EFI_LOADER_H */ > > > > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c > > > > index a1b88dbfc2..5642be9f9a 100644 > > > > --- a/lib/efi_loader/efi_firmware.c > > > > +++ b/lib/efi_loader/efi_firmware.c > > > > @@ -96,6 +96,46 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported( > > > > return EFI_EXIT(EFI_UNSUPPORTED); > > > > } > > > > > > > > +efi_status_t __weak fill_image_type_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, &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 +144,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 > > > > * > > > > * Return information bout the current firmware image in @image_info. > > > > * @image_info will consist of a number of descriptors. > > > > @@ -122,7 +162,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 +212,7 @@ static efi_status_t efi_get_dfu_info( > > > > next = name; > > > > list_for_each_entry(dfu, &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 +290,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 +305,19 @@ efi_status_t EFIAPI efi_firmware_fit_get_image_info( > > > > !descriptor_size || !package_version || !package_version_name)) > > > > return EFI_EXIT(EFI_INVALID_PARAMETER); > > > > > > > > + ret = fill_image_type_guid_array(&efi_firmware_image_type_uboot_fit, > > > > + &part_guid_arr); > > > > + if (ret != EFI_SUCCESS) > > > > + goto out; > > > > + > > > > ret = efi_get_dfu_info(image_info_size, image_info, > > > > descriptor_version, descriptor_count, > > > > descriptor_size, > > > > package_version, package_version_name, > > > > - &efi_firmware_image_type_uboot_fit); > > > > + part_guid_arr); > > > > > > > > +out: > > > > + free(part_guid_arr); > > > > return EFI_EXIT(ret); > > > > } > > > > > > > > @@ -359,6 +407,7 @@ efi_status_t EFIAPI efi_firmware_raw_get_image_info( > > > > u16 **package_version_name) > > > > { > > > > efi_status_t ret = EFI_SUCCESS; > > > > + efi_guid_t *part_guid_arr = NULL; > > > > > > > > EFI_ENTRY("%p %p %p %p %p %p %p %p\n", this, > > > > image_info_size, image_info, > > > > @@ -373,12 +422,20 @@ efi_status_t EFIAPI efi_firmware_raw_get_image_info( > > > > !descriptor_size || !package_version || !package_version_name)) > > > > return EFI_EXIT(EFI_INVALID_PARAMETER); > > > > > > > > + ret = fill_image_type_guid_array( > > > > + &efi_firmware_image_type_uboot_raw, > > > > + &part_guid_arr); > > > > + if (ret != EFI_SUCCESS) > > > > + goto out; > > > > + > > > > ret = efi_get_dfu_info(image_info_size, image_info, > > > > descriptor_version, descriptor_count, > > > > descriptor_size, > > > > package_version, package_version_name, > > > > - &efi_firmware_image_type_uboot_raw); > > > > + part_guid_arr); > > > > > > > > +out: > > > > + free(part_guid_arr); > > > > return EFI_EXIT(ret); > > > > } > > > > > > > > -- > > > > 2.17.1 > > > >
Sughosh, On Thu, Feb 10, 2022 at 03:40:00PM +0530, Sughosh Ganu wrote: > On Thu, 10 Feb 2022 at 13:28, AKASHI Takahiro > <takahiro.akashi@linaro.org> wrote: > > > > On Thu, Feb 10, 2022 at 12:48:13PM +0530, Sughosh Ganu wrote: > > > hi Takahiro, > > > > > > On Thu, 10 Feb 2022 at 08:18, AKASHI Takahiro > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > Hi Sughosh, > > > > > > > > On Mon, Feb 07, 2022 at 11:49:55PM +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. > > > > > > > > After re-thinking of your approach here, I would have to say NAK. > > > > > > > > You use ImageTypeId to identify a particular firmware object. > > > > (By "object," I mean one of firmware instances represented by "dfu_alto_info". > > > > Please don't confuse it with the binary blob embedded in a capsule file.) > > > > But ImageTypeId is not for that purpose, at least, as my intention > > > > in initially implementing capsule framework and FMP drivers. > > > > > > > > 1) ImageTypeId is used to uniquely identify a corresponding FMP driver, > > > > either FIT FMP driver or Raw FMP driver. > > > > > > I believe the identification of an FMP protocol should be done by the > > > FMP GUID, > > > > What does FMP GUID stand for? > > EFI_FIRMWARE_MANAGEMENT_PROTOCOL_GUID, defined in include/efi_api.h. > What I mean is that even when installing the FMP protocol, the call to > efi_install_multiple_protocol_interfaces takes the above FMP GUID as > an argument -- nowhere is the ImageTypeId considered when installing > the protocol. Okay. > > > > > which is what is done in efi_fmp_find. The ImageTypeId is > > > nowhere involved in this identification. > > > > Please take a look at efi_capsule_update_firmware() carefully. > > efi_find_fmp() is called with the image's update_image_type_id > > which is to be set to EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID by mkeficapsule > > (see create_fwbin()). > > I think you are thinking from the point of view of the '--guid' value > that is being passed to the capsule generation tool. But the thing is > that it is the current design(or limitation) of the tool that it takes > only a single guid parameter. So the mkeficapsule tool currently can > generate only a single payload capsule. That is exactly what I intended to do here. We have only one FMP driver (either FIT or RAW) which is based on U-Boot's DFU framework and we need only one payload since, for multiple objects of firmware, we can use FIT format as a payload. That is what FIT is aimed for. Or you can use multiple RAW capsule files with different indexes ("--index" exists for this purpose). > Please check the > GenerateCapsule script in EDK2. In case of a multi payload based > capsule, individual parameters like the UpdateImageTypeId are passed > through the json file, where each of the UpdateImageTypeId has a > different value per payload. > > > > > > > 2) Each firmware object handled by a given FMP driver can further be > > > > identified by ImageIndex. > > > > > > > > My implementation of efi_fmp_find() does (1) and Raw FMP driver does > > > > (2) in efi_firmware_raw_set_image() which takes "image_index" as > > > > a parameter. > > > > > > > > Using ImageTypeId as an identifier is simply wrong in my opinion and > > > > doesn't meet the UEFI specification. > > > > > > So, as per what you are stating, all payloads under a given > > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER should have the same > > > ImageTypeId, either EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID. Same applies for all images in > > > the EFI_FIRMWARE_IMAGE_DESCRIPTOR. Because, without one of the two > > > values, > the check in efi_fmp_find to compare the UpdateImageTypeId > > > with the ImageTypeId retrieved from the image descriptor would simply > > > fail. > > > > I don't follow your point. > > Please elaborate a bit more. > > The current implementation of GetImageInfo, passes either of > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID for all the images of the image > descriptor array. So, in case the capsule is generated with a '--guid' > value which is different from these two values, the check in > efi_fmp_find on line 204 will fail. That is an expected behavior, isn't it? If you want to use a different FMP driver (with another GUID), you naturally need to add your own FMP driver. > This means that unless the --guid > value passed to the capsule generation is either of u-boot FIT or > u-boot raw, the current FMP protocol for raw devices cannot be used. > Why do we need that restriction. It should be possible to use the raw > FMP protocol for any other type of image types as well. > > > > > > > I think this interpretation of the UEFI spec is incorrect, since the > > > spec states that the ImageTypeId and the UpdateImageTypeId are fields > > > used to identify the firmware component targeted for the update. If > > > all values in the image descriptor array and the UpdateImageTypeId are > > > the same, why have this field in the first place for individual > > > images. > > > > As I said, ImageIndex is for that purpose. > > Yes, that is one possible way in the scenario where the ImageIndex is > determined at the capsule generation time. But, for the A/B update > scenario, we do not know the ImageIndex at build time "Build time" of what? I think that users should know how "dfu_alt_info" is defined (in other words, where the firmware be located on the target system) when capsule files are created. -Takahiro Akashi > -- this is > determined only at runtime, and is based on the bank to which the > image is to be updated. Which is why I am finding out the alt_num at > runtime in case the FWU Multi Bank feature is enabled. Like I said > above, I do not see a reason why the current FMP protocols should be > restricted to only the u-boot FIT and u-boot raw image types. It is > being extended, without affecting the default non FWU behaviour. > > -sughosh > > > > > -Takahiro Akashi > > > > > > > > -sughosh > > > > > > > > > > > -Takahiro Akashi > > > > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > --- > > > > > > > > > > Changes since V3: > > > > > * Define a weak function fill_image_type_guid_array for populating the > > > > > image descriptor array with u-boot's raw and fit image GUIDs > > > > > > > > > > include/efi_loader.h | 2 + > > > > > lib/efi_loader/efi_firmware.c | 71 +++++++++++++++++++++++++++++++---- > > > > > 2 files changed, 66 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > > > > index f4860e87fc..ae60de0be5 100644 > > > > > --- a/include/efi_loader.h > > > > > +++ b/include/efi_loader.h > > > > > @@ -992,4 +992,6 @@ efi_status_t efi_esrt_populate(void); > > > > > efi_status_t efi_load_capsule_drivers(void); > > > > > > > > > > efi_status_t platform_get_eventlog(struct udevice *dev, u64 *addr, u32 *sz); > > > > > +efi_status_t fill_image_type_guid_array(const efi_guid_t *default_guid, > > > > > + efi_guid_t **part_guid_arr); > > > > > #endif /* _EFI_LOADER_H */ > > > > > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c > > > > > index a1b88dbfc2..5642be9f9a 100644 > > > > > --- a/lib/efi_loader/efi_firmware.c > > > > > +++ b/lib/efi_loader/efi_firmware.c > > > > > @@ -96,6 +96,46 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported( > > > > > return EFI_EXIT(EFI_UNSUPPORTED); > > > > > } > > > > > > > > > > +efi_status_t __weak fill_image_type_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, &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 +144,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 > > > > > * > > > > > * Return information bout the current firmware image in @image_info. > > > > > * @image_info will consist of a number of descriptors. > > > > > @@ -122,7 +162,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 +212,7 @@ static efi_status_t efi_get_dfu_info( > > > > > next = name; > > > > > list_for_each_entry(dfu, &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 +290,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 +305,19 @@ efi_status_t EFIAPI efi_firmware_fit_get_image_info( > > > > > !descriptor_size || !package_version || !package_version_name)) > > > > > return EFI_EXIT(EFI_INVALID_PARAMETER); > > > > > > > > > > + ret = fill_image_type_guid_array(&efi_firmware_image_type_uboot_fit, > > > > > + &part_guid_arr); > > > > > + if (ret != EFI_SUCCESS) > > > > > + goto out; > > > > > + > > > > > ret = efi_get_dfu_info(image_info_size, image_info, > > > > > descriptor_version, descriptor_count, > > > > > descriptor_size, > > > > > package_version, package_version_name, > > > > > - &efi_firmware_image_type_uboot_fit); > > > > > + part_guid_arr); > > > > > > > > > > +out: > > > > > + free(part_guid_arr); > > > > > return EFI_EXIT(ret); > > > > > } > > > > > > > > > > @@ -359,6 +407,7 @@ efi_status_t EFIAPI efi_firmware_raw_get_image_info( > > > > > u16 **package_version_name) > > > > > { > > > > > efi_status_t ret = EFI_SUCCESS; > > > > > + efi_guid_t *part_guid_arr = NULL; > > > > > > > > > > EFI_ENTRY("%p %p %p %p %p %p %p %p\n", this, > > > > > image_info_size, image_info, > > > > > @@ -373,12 +422,20 @@ efi_status_t EFIAPI efi_firmware_raw_get_image_info( > > > > > !descriptor_size || !package_version || !package_version_name)) > > > > > return EFI_EXIT(EFI_INVALID_PARAMETER); > > > > > > > > > > + ret = fill_image_type_guid_array( > > > > > + &efi_firmware_image_type_uboot_raw, > > > > > + &part_guid_arr); > > > > > + if (ret != EFI_SUCCESS) > > > > > + goto out; > > > > > + > > > > > ret = efi_get_dfu_info(image_info_size, image_info, > > > > > descriptor_version, descriptor_count, > > > > > descriptor_size, > > > > > package_version, package_version_name, > > > > > - &efi_firmware_image_type_uboot_raw); > > > > > + part_guid_arr); > > > > > > > > > > +out: > > > > > + free(part_guid_arr); > > > > > return EFI_EXIT(ret); > > > > > } > > > > > > > > > > -- > > > > > 2.17.1 > > > > >
hi Takahiro, On Mon, 14 Feb 2022 at 08:54, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > Sughosh, > > On Thu, Feb 10, 2022 at 03:40:00PM +0530, Sughosh Ganu wrote: > > On Thu, 10 Feb 2022 at 13:28, AKASHI Takahiro > > <takahiro.akashi@linaro.org> wrote: > > > > > > On Thu, Feb 10, 2022 at 12:48:13PM +0530, Sughosh Ganu wrote: > > > > hi Takahiro, > > > > > > > > On Thu, 10 Feb 2022 at 08:18, AKASHI Takahiro > > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > Hi Sughosh, > > > > > > > > > > On Mon, Feb 07, 2022 at 11:49:55PM +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. > > > > > > > > > > After re-thinking of your approach here, I would have to say NAK. > > > > > > > > > > You use ImageTypeId to identify a particular firmware object. > > > > > (By "object," I mean one of firmware instances represented by "dfu_alto_info". > > > > > Please don't confuse it with the binary blob embedded in a capsule file.) > > > > > But ImageTypeId is not for that purpose, at least, as my intention > > > > > in initially implementing capsule framework and FMP drivers. > > > > > > > > > > 1) ImageTypeId is used to uniquely identify a corresponding FMP driver, > > > > > either FIT FMP driver or Raw FMP driver. > > > > > > > > I believe the identification of an FMP protocol should be done by the > > > > FMP GUID, > > > > > > What does FMP GUID stand for? > > > > EFI_FIRMWARE_MANAGEMENT_PROTOCOL_GUID, defined in include/efi_api.h. > > What I mean is that even when installing the FMP protocol, the call to > > efi_install_multiple_protocol_interfaces takes the above FMP GUID as > > an argument -- nowhere is the ImageTypeId considered when installing > > the protocol. > > Okay. > > > > > > > > which is what is done in efi_fmp_find. The ImageTypeId is > > > > nowhere involved in this identification. > > > > > > Please take a look at efi_capsule_update_firmware() carefully. > > > efi_find_fmp() is called with the image's update_image_type_id > > > which is to be set to EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID by mkeficapsule > > > (see create_fwbin()). > > > > I think you are thinking from the point of view of the '--guid' value > > that is being passed to the capsule generation tool. But the thing is > > that it is the current design(or limitation) of the tool that it takes > > only a single guid parameter. So the mkeficapsule tool currently can > > generate only a single payload capsule. > > That is exactly what I intended to do here. > We have only one FMP driver (either FIT or RAW) which is based on > U-Boot's DFU framework and we need only one payload since, for > multiple objects of firmware, we can use FIT format as a payload. > That is what FIT is aimed for. > Or you can use multiple RAW capsule files with different indexes > ("--index" exists for this purpose). Yes, we can use --index when we know the index value corresponding to the firmware image that we need to update. But like I mentioned in my earlier reply, for A/B updates, we do not know what the index value is going to be. That is going to be determined at runtime. Also, the point I was making is that we can have a capsule which is consumed by an FMP protocol which has more than one image, and those images have different ImageTypeId/UpdateImageTypeId. > > > Please check the > > GenerateCapsule script in EDK2. In case of a multi payload based > > capsule, individual parameters like the UpdateImageTypeId are passed > > through the json file, where each of the UpdateImageTypeId has a > > different value per payload. > > > > > > > > > > 2) Each firmware object handled by a given FMP driver can further be > > > > > identified by ImageIndex. > > > > > > > > > > My implementation of efi_fmp_find() does (1) and Raw FMP driver does > > > > > (2) in efi_firmware_raw_set_image() which takes "image_index" as > > > > > a parameter. > > > > > > > > > > Using ImageTypeId as an identifier is simply wrong in my opinion and > > > > > doesn't meet the UEFI specification. > > > > > > > > So, as per what you are stating, all payloads under a given > > > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER should have the same > > > > ImageTypeId, either EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID. Same applies for all images in > > > > the EFI_FIRMWARE_IMAGE_DESCRIPTOR. Because, without one of the two > > > > values, > the check in efi_fmp_find to compare the UpdateImageTypeId > > > > with the ImageTypeId retrieved from the image descriptor would simply > > > > fail. > > > > > > I don't follow your point. > > > Please elaborate a bit more. > > > > The current implementation of GetImageInfo, passes either of > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID for all the images of the image > > descriptor array. So, in case the capsule is generated with a '--guid' > > value which is different from these two values, the check in > > efi_fmp_find on line 204 will fail. > > That is an expected behavior, isn't it? Yes it is. Do not contest that. > If you want to use a different FMP driver (with another GUID), > you naturally need to add your own FMP driver. This is where I differ. We can use the same FMP protocol instance for any type of ImageTypeId. I do not see why we need to define a different FMP protocol instance for a GUID value other than what has been defined for u-boot raw and u-boot FIT GUIDs. The platform can give us the image descriptor array, and with that, the same FMP instance can be used for any type of image(ImageTypeId). > > > > This means that unless the --guid > > value passed to the capsule generation is either of u-boot FIT or > > u-boot raw, the current FMP protocol for raw devices cannot be used. > > Why do we need that restriction. It should be possible to use the raw > > FMP protocol for any other type of image types as well. > > > > > > > > > > > I think this interpretation of the UEFI spec is incorrect, since the > > > > spec states that the ImageTypeId and the UpdateImageTypeId are fields > > > > used to identify the firmware component targeted for the update. If > > > > all values in the image descriptor array and the UpdateImageTypeId are > > > > the same, why have this field in the first place for individual > > > > images. > > > > > > As I said, ImageIndex is for that purpose. > > > > Yes, that is one possible way in the scenario where the ImageIndex is > > determined at the capsule generation time. But, for the A/B update > > scenario, we do not know the ImageIndex at build time > > "Build time" of what? Of the capsule. > I think that users should know how "dfu_alt_info" is defined > (in other words, where the firmware be located on the target system) > when capsule files are created. That is true for a non A/B scenario. And that is how it works in the non A/B updates case. But for A/B updates, since the determination of the "location" where the firmware image has to be written will be done only at runtime, we cannot use the --index to differentiate. Like I mentioned earlier, this is not breaking the existing behaviour -- for the non A/B updates, the update procedure remains exactly the same, of using the index value to determine the location of the update. I have only extended the behaviour to use the same FMP instance for the A/B update(FWU) feature using ImageTypeId's. -sughosh > > -Takahiro Akashi > > > > -- this is > > determined only at runtime, and is based on the bank to which the > > image is to be updated. Which is why I am finding out the alt_num at > > runtime in case the FWU Multi Bank feature is enabled. Like I said > > above, I do not see a reason why the current FMP protocols should be > > restricted to only the u-boot FIT and u-boot raw image types. It is > > being extended, without affecting the default non FWU behaviour. > > > > -sughosh > > > > > > > > -Takahiro Akashi > > > > > > > > > > > -sughosh > > > > > > > > > > > > > > -Takahiro Akashi > > > > > > > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > > --- > > > > > > > > > > > > Changes since V3: > > > > > > * Define a weak function fill_image_type_guid_array for populating the > > > > > > image descriptor array with u-boot's raw and fit image GUIDs > > > > > > > > > > > > include/efi_loader.h | 2 + > > > > > > lib/efi_loader/efi_firmware.c | 71 +++++++++++++++++++++++++++++++---- > > > > > > 2 files changed, 66 insertions(+), 7 deletions(-) > > > > > > > > > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > > > > > index f4860e87fc..ae60de0be5 100644 > > > > > > --- a/include/efi_loader.h > > > > > > +++ b/include/efi_loader.h > > > > > > @@ -992,4 +992,6 @@ efi_status_t efi_esrt_populate(void); > > > > > > efi_status_t efi_load_capsule_drivers(void); > > > > > > > > > > > > efi_status_t platform_get_eventlog(struct udevice *dev, u64 *addr, u32 *sz); > > > > > > +efi_status_t fill_image_type_guid_array(const efi_guid_t *default_guid, > > > > > > + efi_guid_t **part_guid_arr); > > > > > > #endif /* _EFI_LOADER_H */ > > > > > > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c > > > > > > index a1b88dbfc2..5642be9f9a 100644 > > > > > > --- a/lib/efi_loader/efi_firmware.c > > > > > > +++ b/lib/efi_loader/efi_firmware.c > > > > > > @@ -96,6 +96,46 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported( > > > > > > return EFI_EXIT(EFI_UNSUPPORTED); > > > > > > } > > > > > > > > > > > > +efi_status_t __weak fill_image_type_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, &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 +144,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 > > > > > > * > > > > > > * Return information bout the current firmware image in @image_info. > > > > > > * @image_info will consist of a number of descriptors. > > > > > > @@ -122,7 +162,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 +212,7 @@ static efi_status_t efi_get_dfu_info( > > > > > > next = name; > > > > > > list_for_each_entry(dfu, &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 +290,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 +305,19 @@ efi_status_t EFIAPI efi_firmware_fit_get_image_info( > > > > > > !descriptor_size || !package_version || !package_version_name)) > > > > > > return EFI_EXIT(EFI_INVALID_PARAMETER); > > > > > > > > > > > > + ret = fill_image_type_guid_array(&efi_firmware_image_type_uboot_fit, > > > > > > + &part_guid_arr); > > > > > > + if (ret != EFI_SUCCESS) > > > > > > + goto out; > > > > > > + > > > > > > ret = efi_get_dfu_info(image_info_size, image_info, > > > > > > descriptor_version, descriptor_count, > > > > > > descriptor_size, > > > > > > package_version, package_version_name, > > > > > > - &efi_firmware_image_type_uboot_fit); > > > > > > + part_guid_arr); > > > > > > > > > > > > +out: > > > > > > + free(part_guid_arr); > > > > > > return EFI_EXIT(ret); > > > > > > } > > > > > > > > > > > > @@ -359,6 +407,7 @@ efi_status_t EFIAPI efi_firmware_raw_get_image_info( > > > > > > u16 **package_version_name) > > > > > > { > > > > > > efi_status_t ret = EFI_SUCCESS; > > > > > > + efi_guid_t *part_guid_arr = NULL; > > > > > > > > > > > > EFI_ENTRY("%p %p %p %p %p %p %p %p\n", this, > > > > > > image_info_size, image_info, > > > > > > @@ -373,12 +422,20 @@ efi_status_t EFIAPI efi_firmware_raw_get_image_info( > > > > > > !descriptor_size || !package_version || !package_version_name)) > > > > > > return EFI_EXIT(EFI_INVALID_PARAMETER); > > > > > > > > > > > > + ret = fill_image_type_guid_array( > > > > > > + &efi_firmware_image_type_uboot_raw, > > > > > > + &part_guid_arr); > > > > > > + if (ret != EFI_SUCCESS) > > > > > > + goto out; > > > > > > + > > > > > > ret = efi_get_dfu_info(image_info_size, image_info, > > > > > > descriptor_version, descriptor_count, > > > > > > descriptor_size, > > > > > > package_version, package_version_name, > > > > > > - &efi_firmware_image_type_uboot_raw); > > > > > > + part_guid_arr); > > > > > > > > > > > > +out: > > > > > > + free(part_guid_arr); > > > > > > return EFI_EXIT(ret); > > > > > > } > > > > > > > > > > > > -- > > > > > > 2.17.1 > > > > > >
Sughosh, On Mon, Feb 14, 2022 at 11:12:22AM +0530, Sughosh Ganu wrote: > hi Takahiro, > > On Mon, 14 Feb 2022 at 08:54, AKASHI Takahiro > <takahiro.akashi@linaro.org> wrote: > > > > Sughosh, > > > > On Thu, Feb 10, 2022 at 03:40:00PM +0530, Sughosh Ganu wrote: > > > On Thu, 10 Feb 2022 at 13:28, AKASHI Takahiro > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > On Thu, Feb 10, 2022 at 12:48:13PM +0530, Sughosh Ganu wrote: > > > > > hi Takahiro, > > > > > > > > > > On Thu, 10 Feb 2022 at 08:18, AKASHI Takahiro > > > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > > > Hi Sughosh, > > > > > > > > > > > > On Mon, Feb 07, 2022 at 11:49:55PM +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. > > > > > > > > > > > > After re-thinking of your approach here, I would have to say NAK. > > > > > > > > > > > > You use ImageTypeId to identify a particular firmware object. > > > > > > (By "object," I mean one of firmware instances represented by "dfu_alto_info". > > > > > > Please don't confuse it with the binary blob embedded in a capsule file.) > > > > > > But ImageTypeId is not for that purpose, at least, as my intention > > > > > > in initially implementing capsule framework and FMP drivers. > > > > > > > > > > > > 1) ImageTypeId is used to uniquely identify a corresponding FMP driver, > > > > > > either FIT FMP driver or Raw FMP driver. > > > > > > > > > > I believe the identification of an FMP protocol should be done by the > > > > > FMP GUID, > > > > > > > > What does FMP GUID stand for? > > > > > > EFI_FIRMWARE_MANAGEMENT_PROTOCOL_GUID, defined in include/efi_api.h. > > > What I mean is that even when installing the FMP protocol, the call to > > > efi_install_multiple_protocol_interfaces takes the above FMP GUID as > > > an argument -- nowhere is the ImageTypeId considered when installing > > > the protocol. > > > > Okay. > > > > > > > > > > > which is what is done in efi_fmp_find. The ImageTypeId is > > > > > nowhere involved in this identification. > > > > > > > > Please take a look at efi_capsule_update_firmware() carefully. > > > > efi_find_fmp() is called with the image's update_image_type_id > > > > which is to be set to EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID by mkeficapsule > > > > (see create_fwbin()). > > > > > > I think you are thinking from the point of view of the '--guid' value > > > that is being passed to the capsule generation tool. But the thing is > > > that it is the current design(or limitation) of the tool that it takes > > > only a single guid parameter. So the mkeficapsule tool currently can > > > generate only a single payload capsule. > > > > That is exactly what I intended to do here. > > We have only one FMP driver (either FIT or RAW) which is based on > > U-Boot's DFU framework and we need only one payload since, for > > multiple objects of firmware, we can use FIT format as a payload. > > That is what FIT is aimed for. > > Or you can use multiple RAW capsule files with different indexes > > ("--index" exists for this purpose). > > Yes, we can use --index when we know the index value corresponding to > the firmware image that we need to update. But like I mentioned in my > earlier reply, for A/B updates, we do not know what the index value is > going to be. That is going to be determined at runtime. I don't think so. See below for alternative approach. > Also, the point I was making is that we can have a capsule which is > consumed by an FMP protocol which has more than one image, and those > images have different ImageTypeId/UpdateImageTypeId. Yes, but it is a design choice in my first implementation. I didn't think that we need to "have a capsule which is consumed by an FMP protocol which has more than one image" as long as we use DFU framework (and FIT as standard format of aggregation on U-Boot). > > > > > Please check the > > > GenerateCapsule script in EDK2. In case of a multi payload based > > > capsule, individual parameters like the UpdateImageTypeId are passed > > > through the json file, where each of the UpdateImageTypeId has a > > > different value per payload. > > > > > > > > > > > > > 2) Each firmware object handled by a given FMP driver can further be > > > > > > identified by ImageIndex. > > > > > > > > > > > > My implementation of efi_fmp_find() does (1) and Raw FMP driver does > > > > > > (2) in efi_firmware_raw_set_image() which takes "image_index" as > > > > > > a parameter. > > > > > > > > > > > > Using ImageTypeId as an identifier is simply wrong in my opinion and > > > > > > doesn't meet the UEFI specification. > > > > > > > > > > So, as per what you are stating, all payloads under a given > > > > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER should have the same > > > > > ImageTypeId, either EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > > > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID. Same applies for all images in > > > > > the EFI_FIRMWARE_IMAGE_DESCRIPTOR. Because, without one of the two > > > > > values, > the check in efi_fmp_find to compare the UpdateImageTypeId > > > > > with the ImageTypeId retrieved from the image descriptor would simply > > > > > fail. > > > > > > > > I don't follow your point. > > > > Please elaborate a bit more. > > > > > > The current implementation of GetImageInfo, passes either of > > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID for all the images of the image > > > descriptor array. So, in case the capsule is generated with a '--guid' > > > value which is different from these two values, the check in > > > efi_fmp_find on line 204 will fail. > > > > That is an expected behavior, isn't it? > > Yes it is. Do not contest that. > > > If you want to use a different FMP driver (with another GUID), > > you naturally need to add your own FMP driver. > > This is where I differ. We can use the same FMP protocol instance for > any type of ImageTypeId. I do not see why we need to define a > different FMP protocol instance for a GUID value other than what has > been defined for u-boot raw and u-boot FIT GUIDs. I do understand part of your concern a bit. I thought of using the same ImageType GUID, say IMAGE_TYPE_DFU_GUID, at first but we needed different GUIDs here simply because we need to determine the format of payload, FIT format or raw binary. > The platform can give us the image descriptor array, and with that, > the same FMP instance can be used for any type of image(ImageTypeId). "any type of image"? Really? > > > > > > > > This means that unless the --guid > > > value passed to the capsule generation is either of u-boot FIT or > > > u-boot raw, the current FMP protocol for raw devices cannot be used. > > > Why do we need that restriction. It should be possible to use the raw > > > FMP protocol for any other type of image types as well. > > > > > > > > > > > > > > > I think this interpretation of the UEFI spec is incorrect, since the > > > > > spec states that the ImageTypeId and the UpdateImageTypeId are fields > > > > > used to identify the firmware component targeted for the update. If > > > > > all values in the image descriptor array and the UpdateImageTypeId are > > > > > the same, why have this field in the first place for individual > > > > > images. > > > > > > > > As I said, ImageIndex is for that purpose. > > > > > > Yes, that is one possible way in the scenario where the ImageIndex is > > > determined at the capsule generation time. But, for the A/B update > > > scenario, we do not know the ImageIndex at build time > > > > "Build time" of what? > > Of the capsule. > > > I think that users should know how "dfu_alt_info" is defined > > (in other words, where the firmware be located on the target system) > > when capsule files are created. > > That is true for a non A/B scenario. And that is how it works in the > non A/B updates case. But for A/B updates, since the determination of > the "location" where the firmware image has to be written will be done > only at runtime, we cannot use the --index to differentiate. Yes, we can :) First of all, my essential assumption in either FIT or RAW FMP driver is that U-Boot has (somehow conceptually) single firmware blob represented by DFU or dfu_alt_info. As I said, each object or location in dfu_alt_info can be further identified by index or "UpdateImageIndex". Let's assume that we have two locations of firmware, fw1 and fw2, and that we have two bank A and B. Then we will define dfu_alt_info as follows: <loc of fw1 for A>;<loc of fw2 for A>;<loc of fw1 for B>;<loc of fw2 for B>; |<--- 1st set --->|<--- 2nd set --->| When you want to update bank A, we can use the first set of dfu_alt_info, and use the second set of dfu_alt_info for bank B. At runtime, you should know which bank you're working on, and therefore you should know the exact physical location from dfu_alt_info. Please note that you don't have to change the syntax of dfu_alt_info at all. Simply offset the location with 0 for bank A and with 2 for bank B. I don't think we need to introduce extra GUIDs. -Takahiro Akashi > Like I mentioned earlier, this is not breaking the existing behaviour > -- for the non A/B updates, the update procedure remains exactly the > same, of using the index value to determine the location of the > update. I have only extended the behaviour to use the same FMP > instance for the A/B update(FWU) feature using ImageTypeId's. > > -sughosh > > > > > -Takahiro Akashi > > > > > > > -- this is > > > determined only at runtime, and is based on the bank to which the > > > image is to be updated. Which is why I am finding out the alt_num at > > > runtime in case the FWU Multi Bank feature is enabled. Like I said > > > above, I do not see a reason why the current FMP protocols should be > > > restricted to only the u-boot FIT and u-boot raw image types. It is > > > being extended, without affecting the default non FWU behaviour. > > > > > > -sughosh > > > > > > > > > > > -Takahiro Akashi > > > > > > > > > > > > > > -sughosh > > > > > > > > > > > > > > > > > -Takahiro Akashi > > > > > > > > > > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > > > --- > > > > > > > > > > > > > > Changes since V3: > > > > > > > * Define a weak function fill_image_type_guid_array for populating the > > > > > > > image descriptor array with u-boot's raw and fit image GUIDs > > > > > > > > > > > > > > include/efi_loader.h | 2 + > > > > > > > lib/efi_loader/efi_firmware.c | 71 +++++++++++++++++++++++++++++++---- > > > > > > > 2 files changed, 66 insertions(+), 7 deletions(-) > > > > > > > > > > > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > > > > > > index f4860e87fc..ae60de0be5 100644 > > > > > > > --- a/include/efi_loader.h > > > > > > > +++ b/include/efi_loader.h > > > > > > > @@ -992,4 +992,6 @@ efi_status_t efi_esrt_populate(void); > > > > > > > efi_status_t efi_load_capsule_drivers(void); > > > > > > > > > > > > > > efi_status_t platform_get_eventlog(struct udevice *dev, u64 *addr, u32 *sz); > > > > > > > +efi_status_t fill_image_type_guid_array(const efi_guid_t *default_guid, > > > > > > > + efi_guid_t **part_guid_arr); > > > > > > > #endif /* _EFI_LOADER_H */ > > > > > > > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c > > > > > > > index a1b88dbfc2..5642be9f9a 100644 > > > > > > > --- a/lib/efi_loader/efi_firmware.c > > > > > > > +++ b/lib/efi_loader/efi_firmware.c > > > > > > > @@ -96,6 +96,46 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported( > > > > > > > return EFI_EXIT(EFI_UNSUPPORTED); > > > > > > > } > > > > > > > > > > > > > > +efi_status_t __weak fill_image_type_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, &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 +144,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 > > > > > > > * > > > > > > > * Return information bout the current firmware image in @image_info. > > > > > > > * @image_info will consist of a number of descriptors. > > > > > > > @@ -122,7 +162,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 +212,7 @@ static efi_status_t efi_get_dfu_info( > > > > > > > next = name; > > > > > > > list_for_each_entry(dfu, &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 +290,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 +305,19 @@ efi_status_t EFIAPI efi_firmware_fit_get_image_info( > > > > > > > !descriptor_size || !package_version || !package_version_name)) > > > > > > > return EFI_EXIT(EFI_INVALID_PARAMETER); > > > > > > > > > > > > > > + ret = fill_image_type_guid_array(&efi_firmware_image_type_uboot_fit, > > > > > > > + &part_guid_arr); > > > > > > > + if (ret != EFI_SUCCESS) > > > > > > > + goto out; > > > > > > > + > > > > > > > ret = efi_get_dfu_info(image_info_size, image_info, > > > > > > > descriptor_version, descriptor_count, > > > > > > > descriptor_size, > > > > > > > package_version, package_version_name, > > > > > > > - &efi_firmware_image_type_uboot_fit); > > > > > > > + part_guid_arr); > > > > > > > > > > > > > > +out: > > > > > > > + free(part_guid_arr); > > > > > > > return EFI_EXIT(ret); > > > > > > > } > > > > > > > > > > > > > > @@ -359,6 +407,7 @@ efi_status_t EFIAPI efi_firmware_raw_get_image_info( > > > > > > > u16 **package_version_name) > > > > > > > { > > > > > > > efi_status_t ret = EFI_SUCCESS; > > > > > > > + efi_guid_t *part_guid_arr = NULL; > > > > > > > > > > > > > > EFI_ENTRY("%p %p %p %p %p %p %p %p\n", this, > > > > > > > image_info_size, image_info, > > > > > > > @@ -373,12 +422,20 @@ efi_status_t EFIAPI efi_firmware_raw_get_image_info( > > > > > > > !descriptor_size || !package_version || !package_version_name)) > > > > > > > return EFI_EXIT(EFI_INVALID_PARAMETER); > > > > > > > > > > > > > > + ret = fill_image_type_guid_array( > > > > > > > + &efi_firmware_image_type_uboot_raw, > > > > > > > + &part_guid_arr); > > > > > > > + if (ret != EFI_SUCCESS) > > > > > > > + goto out; > > > > > > > + > > > > > > > ret = efi_get_dfu_info(image_info_size, image_info, > > > > > > > descriptor_version, descriptor_count, > > > > > > > descriptor_size, > > > > > > > package_version, package_version_name, > > > > > > > - &efi_firmware_image_type_uboot_raw); > > > > > > > + part_guid_arr); > > > > > > > > > > > > > > +out: > > > > > > > + free(part_guid_arr); > > > > > > > return EFI_EXIT(ret); > > > > > > > } > > > > > > > > > > > > > > -- > > > > > > > 2.17.1 > > > > > > >
On Tue, 15 Feb 2022 at 07:21, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > Sughosh, > > On Mon, Feb 14, 2022 at 11:12:22AM +0530, Sughosh Ganu wrote: > > hi Takahiro, > > > > On Mon, 14 Feb 2022 at 08:54, AKASHI Takahiro > > <takahiro.akashi@linaro.org> wrote: > > > > > > Sughosh, > > > > > > On Thu, Feb 10, 2022 at 03:40:00PM +0530, Sughosh Ganu wrote: > > > > On Thu, 10 Feb 2022 at 13:28, AKASHI Takahiro > > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > On Thu, Feb 10, 2022 at 12:48:13PM +0530, Sughosh Ganu wrote: > > > > > > hi Takahiro, > > > > > > > > > > > > On Thu, 10 Feb 2022 at 08:18, AKASHI Takahiro > > > > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > > > > > Hi Sughosh, > > > > > > > > > > > > > > On Mon, Feb 07, 2022 at 11:49:55PM +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. > > > > > > > > > > > > > > After re-thinking of your approach here, I would have to say NAK. > > > > > > > > > > > > > > You use ImageTypeId to identify a particular firmware object. > > > > > > > (By "object," I mean one of firmware instances represented by "dfu_alto_info". > > > > > > > Please don't confuse it with the binary blob embedded in a capsule file.) > > > > > > > But ImageTypeId is not for that purpose, at least, as my intention > > > > > > > in initially implementing capsule framework and FMP drivers. > > > > > > > > > > > > > > 1) ImageTypeId is used to uniquely identify a corresponding FMP driver, > > > > > > > either FIT FMP driver or Raw FMP driver. > > > > > > > > > > > > I believe the identification of an FMP protocol should be done by the > > > > > > FMP GUID, > > > > > > > > > > What does FMP GUID stand for? > > > > > > > > EFI_FIRMWARE_MANAGEMENT_PROTOCOL_GUID, defined in include/efi_api.h. > > > > What I mean is that even when installing the FMP protocol, the call to > > > > efi_install_multiple_protocol_interfaces takes the above FMP GUID as > > > > an argument -- nowhere is the ImageTypeId considered when installing > > > > the protocol. > > > > > > Okay. > > > > > > > > > > > > > > which is what is done in efi_fmp_find. The ImageTypeId is > > > > > > nowhere involved in this identification. > > > > > > > > > > Please take a look at efi_capsule_update_firmware() carefully. > > > > > efi_find_fmp() is called with the image's update_image_type_id > > > > > which is to be set to EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > > > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID by mkeficapsule > > > > > (see create_fwbin()). > > > > > > > > I think you are thinking from the point of view of the '--guid' value > > > > that is being passed to the capsule generation tool. But the thing is > > > > that it is the current design(or limitation) of the tool that it takes > > > > only a single guid parameter. So the mkeficapsule tool currently can > > > > generate only a single payload capsule. > > > > > > That is exactly what I intended to do here. > > > We have only one FMP driver (either FIT or RAW) which is based on > > > U-Boot's DFU framework and we need only one payload since, for > > > multiple objects of firmware, we can use FIT format as a payload. > > > That is what FIT is aimed for. > > > Or you can use multiple RAW capsule files with different indexes > > > ("--index" exists for this purpose). > > > > Yes, we can use --index when we know the index value corresponding to > > the firmware image that we need to update. But like I mentioned in my > > earlier reply, for A/B updates, we do not know what the index value is > > going to be. That is going to be determined at runtime. > > I don't think so. See below for alternative approach. > > > Also, the point I was making is that we can have a capsule which is > > consumed by an FMP protocol which has more than one image, and those > > images have different ImageTypeId/UpdateImageTypeId. > > Yes, but it is a design choice in my first implementation. > I didn't think that we need to "have a capsule which is consumed > by an FMP protocol which has more than one image" as long as we > use DFU framework (and FIT as standard format of aggregation on U-Boot). But this design can be extended without any hassle, and more importantly without any regression, no? What kind of a problem does it create if the FMP can handle more than one image type. Even as per the UEFI spec, we have the EFI_FIRMWARE_MANAGEMENT_CAPSULE header for all images to be managed by the FMP protocol which has multiple images with different UpdateImageTypeId. > > > > > > > > Please check the > > > > GenerateCapsule script in EDK2. In case of a multi payload based > > > > capsule, individual parameters like the UpdateImageTypeId are passed > > > > through the json file, where each of the UpdateImageTypeId has a > > > > different value per payload. > > > > > > > > > > > > > > > > 2) Each firmware object handled by a given FMP driver can further be > > > > > > > identified by ImageIndex. > > > > > > > > > > > > > > My implementation of efi_fmp_find() does (1) and Raw FMP driver does > > > > > > > (2) in efi_firmware_raw_set_image() which takes "image_index" as > > > > > > > a parameter. > > > > > > > > > > > > > > Using ImageTypeId as an identifier is simply wrong in my opinion and > > > > > > > doesn't meet the UEFI specification. > > > > > > > > > > > > So, as per what you are stating, all payloads under a given > > > > > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER should have the same > > > > > > ImageTypeId, either EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > > > > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID. Same applies for all images in > > > > > > the EFI_FIRMWARE_IMAGE_DESCRIPTOR. Because, without one of the two > > > > > > values, > the check in efi_fmp_find to compare the UpdateImageTypeId > > > > > > with the ImageTypeId retrieved from the image descriptor would simply > > > > > > fail. > > > > > > > > > > I don't follow your point. > > > > > Please elaborate a bit more. > > > > > > > > The current implementation of GetImageInfo, passes either of > > > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID for all the images of the image > > > > descriptor array. So, in case the capsule is generated with a '--guid' > > > > value which is different from these two values, the check in > > > > efi_fmp_find on line 204 will fail. > > > > > > That is an expected behavior, isn't it? > > > > Yes it is. Do not contest that. > > > > > If you want to use a different FMP driver (with another GUID), > > > you naturally need to add your own FMP driver. > > > > This is where I differ. We can use the same FMP protocol instance for > > any type of ImageTypeId. I do not see why we need to define a > > different FMP protocol instance for a GUID value other than what has > > been defined for u-boot raw and u-boot FIT GUIDs. > > I do understand part of your concern a bit. > I thought of using the same ImageType GUID, say IMAGE_TYPE_DFU_GUID, at first > but we needed different GUIDs here simply because we need to determine > the format of payload, FIT format or raw binary. > > > The platform can give us the image descriptor array, and with that, > > the same FMP instance can be used for any type of image(ImageTypeId). > > "any type of image"? Really? The raw FMP instance can certainly handle any type of binary payloads right. There is no restriction on what type of payload it is as long as it is all going as a single entity to a given dfu partition. > > > > > > > > > > > > This means that unless the --guid > > > > value passed to the capsule generation is either of u-boot FIT or > > > > u-boot raw, the current FMP protocol for raw devices cannot be used. > > > > Why do we need that restriction. It should be possible to use the raw > > > > FMP protocol for any other type of image types as well. > > > > > > > > > > > > > > > > > > > I think this interpretation of the UEFI spec is incorrect, since the > > > > > > spec states that the ImageTypeId and the UpdateImageTypeId are fields > > > > > > used to identify the firmware component targeted for the update. If > > > > > > all values in the image descriptor array and the UpdateImageTypeId are > > > > > > the same, why have this field in the first place for individual > > > > > > images. > > > > > > > > > > As I said, ImageIndex is for that purpose. > > > > > > > > Yes, that is one possible way in the scenario where the ImageIndex is > > > > determined at the capsule generation time. But, for the A/B update > > > > scenario, we do not know the ImageIndex at build time > > > > > > "Build time" of what? > > > > Of the capsule. > > > > > I think that users should know how "dfu_alt_info" is defined > > > (in other words, where the firmware be located on the target system) > > > when capsule files are created. > > > > That is true for a non A/B scenario. And that is how it works in the > > non A/B updates case. But for A/B updates, since the determination of > > the "location" where the firmware image has to be written will be done > > only at runtime, we cannot use the --index to differentiate. > > Yes, we can :) You know what I mean -- if we could use the same logic, I would not have added all that code :) > > First of all, my essential assumption in either FIT or RAW FMP driver > is that U-Boot has (somehow conceptually) single firmware blob represented > by DFU or dfu_alt_info. As I said, each object or location in > dfu_alt_info can be further identified by index or "UpdateImageIndex". > > Let's assume that we have two locations of firmware, fw1 and fw2, and > that we have two bank A and B. > Then we will define dfu_alt_info as follows: > <loc of fw1 for A>;<loc of fw2 for A>;<loc of fw1 for B>;<loc of fw2 for B>; > |<--- 1st set --->|<--- 2nd set --->| > > When you want to update bank A, we can use the first set of dfu_alt_info, > and use the second set of dfu_alt_info for bank B. > At runtime, you should know which bank you're working on, and therefore > you should know the exact physical location from dfu_alt_info. > > Please note that you don't have to change the syntax of dfu_alt_info > at all. Simply offset the location with 0 for bank A and with 2 for bank B. We can use this logic yes. But please note, that with this, we need to a) Keep a definite order of the images in all the banks, and b) Know the order of the images. With the way the FWU metadata is designed, we do not have these restrictions -- the firmware images can be placed on the device in any order, irrespective of the bank that they belong to. One might prefer clubbing images of a given bank together, but that is not the restriction put by the FWU spec. That is because the images are being identified using image GUIDs. I really don't get your opposition to extending the current design. I would like to hear from Heinrich or Ilias on what their thoughts are on this. -sughosh > > I don't think we need to introduce extra GUIDs. > > -Takahiro Akashi > > > Like I mentioned earlier, this is not breaking the existing behaviour > > -- for the non A/B updates, the update procedure remains exactly the > > same, of using the index value to determine the location of the > > update. I have only extended the behaviour to use the same FMP > > instance for the A/B update(FWU) feature using ImageTypeId's. > > > > -sughosh > > > > > > > > -Takahiro Akashi > > > > > > > > > > -- this is > > > > determined only at runtime, and is based on the bank to which the > > > > image is to be updated. Which is why I am finding out the alt_num at > > > > runtime in case the FWU Multi Bank feature is enabled. Like I said > > > > above, I do not see a reason why the current FMP protocols should be > > > > restricted to only the u-boot FIT and u-boot raw image types. It is > > > > being extended, without affecting the default non FWU behaviour. > > > > > > > > -sughosh > > > > > > > > > > > > > > -Takahiro Akashi > > > > > > > > > > > > > > > > > -sughosh > > > > > > > > > > > > > > > > > > > > -Takahiro Akashi > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > > > > --- > > > > > > > > > > > > > > > > Changes since V3: > > > > > > > > * Define a weak function fill_image_type_guid_array for populating the > > > > > > > > image descriptor array with u-boot's raw and fit image GUIDs > > > > > > > > > > > > > > > > include/efi_loader.h | 2 + > > > > > > > > lib/efi_loader/efi_firmware.c | 71 +++++++++++++++++++++++++++++++---- > > > > > > > > 2 files changed, 66 insertions(+), 7 deletions(-) > > > > > > > > > > > > > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > > > > > > > index f4860e87fc..ae60de0be5 100644 > > > > > > > > --- a/include/efi_loader.h > > > > > > > > +++ b/include/efi_loader.h > > > > > > > > @@ -992,4 +992,6 @@ efi_status_t efi_esrt_populate(void); > > > > > > > > efi_status_t efi_load_capsule_drivers(void); > > > > > > > > > > > > > > > > efi_status_t platform_get_eventlog(struct udevice *dev, u64 *addr, u32 *sz); > > > > > > > > +efi_status_t fill_image_type_guid_array(const efi_guid_t *default_guid, > > > > > > > > + efi_guid_t **part_guid_arr); > > > > > > > > #endif /* _EFI_LOADER_H */ > > > > > > > > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c > > > > > > > > index a1b88dbfc2..5642be9f9a 100644 > > > > > > > > --- a/lib/efi_loader/efi_firmware.c > > > > > > > > +++ b/lib/efi_loader/efi_firmware.c > > > > > > > > @@ -96,6 +96,46 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported( > > > > > > > > return EFI_EXIT(EFI_UNSUPPORTED); > > > > > > > > } > > > > > > > > > > > > > > > > +efi_status_t __weak fill_image_type_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, &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 +144,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 > > > > > > > > * > > > > > > > > * Return information bout the current firmware image in @image_info. > > > > > > > > * @image_info will consist of a number of descriptors. > > > > > > > > @@ -122,7 +162,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 +212,7 @@ static efi_status_t efi_get_dfu_info( > > > > > > > > next = name; > > > > > > > > list_for_each_entry(dfu, &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 +290,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 +305,19 @@ efi_status_t EFIAPI efi_firmware_fit_get_image_info( > > > > > > > > !descriptor_size || !package_version || !package_version_name)) > > > > > > > > return EFI_EXIT(EFI_INVALID_PARAMETER); > > > > > > > > > > > > > > > > + ret = fill_image_type_guid_array(&efi_firmware_image_type_uboot_fit, > > > > > > > > + &part_guid_arr); > > > > > > > > + if (ret != EFI_SUCCESS) > > > > > > > > + goto out; > > > > > > > > + > > > > > > > > ret = efi_get_dfu_info(image_info_size, image_info, > > > > > > > > descriptor_version, descriptor_count, > > > > > > > > descriptor_size, > > > > > > > > package_version, package_version_name, > > > > > > > > - &efi_firmware_image_type_uboot_fit); > > > > > > > > + part_guid_arr); > > > > > > > > > > > > > > > > +out: > > > > > > > > + free(part_guid_arr); > > > > > > > > return EFI_EXIT(ret); > > > > > > > > } > > > > > > > > > > > > > > > > @@ -359,6 +407,7 @@ efi_status_t EFIAPI efi_firmware_raw_get_image_info( > > > > > > > > u16 **package_version_name) > > > > > > > > { > > > > > > > > efi_status_t ret = EFI_SUCCESS; > > > > > > > > + efi_guid_t *part_guid_arr = NULL; > > > > > > > > > > > > > > > > EFI_ENTRY("%p %p %p %p %p %p %p %p\n", this, > > > > > > > > image_info_size, image_info, > > > > > > > > @@ -373,12 +422,20 @@ efi_status_t EFIAPI efi_firmware_raw_get_image_info( > > > > > > > > !descriptor_size || !package_version || !package_version_name)) > > > > > > > > return EFI_EXIT(EFI_INVALID_PARAMETER); > > > > > > > > > > > > > > > > + ret = fill_image_type_guid_array( > > > > > > > > + &efi_firmware_image_type_uboot_raw, > > > > > > > > + &part_guid_arr); > > > > > > > > + if (ret != EFI_SUCCESS) > > > > > > > > + goto out; > > > > > > > > + > > > > > > > > ret = efi_get_dfu_info(image_info_size, image_info, > > > > > > > > descriptor_version, descriptor_count, > > > > > > > > descriptor_size, > > > > > > > > package_version, package_version_name, > > > > > > > > - &efi_firmware_image_type_uboot_raw); > > > > > > > > + part_guid_arr); > > > > > > > > > > > > > > > > +out: > > > > > > > > + free(part_guid_arr); > > > > > > > > return EFI_EXIT(ret); > > > > > > > > } > > > > > > > > > > > > > > > > -- > > > > > > > > 2.17.1 > > > > > > > >
Hi, Took me some time to go through the whole thread, but here it goes. On Tue, Feb 15, 2022 at 12:08:30PM +0530, Sughosh Ganu wrote: > On Tue, 15 Feb 2022 at 07:21, AKASHI Takahiro > <takahiro.akashi@linaro.org> wrote: > > > > Sughosh, > > > > On Mon, Feb 14, 2022 at 11:12:22AM +0530, Sughosh Ganu wrote: > > > hi Takahiro, > > > > > > On Mon, 14 Feb 2022 at 08:54, AKASHI Takahiro > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > Sughosh, > > > > > > > > On Thu, Feb 10, 2022 at 03:40:00PM +0530, Sughosh Ganu wrote: > > > > > On Thu, 10 Feb 2022 at 13:28, AKASHI Takahiro > > > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > > > On Thu, Feb 10, 2022 at 12:48:13PM +0530, Sughosh Ganu wrote: > > > > > > > hi Takahiro, > > > > > > > > > > > > > > On Thu, 10 Feb 2022 at 08:18, AKASHI Takahiro > > > > > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > > > > > > > Hi Sughosh, > > > > > > > > > > > > > > > > On Mon, Feb 07, 2022 at 11:49:55PM +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. > > > > > > > > > > > > > > > > After re-thinking of your approach here, I would have to say NAK. > > > > > > > > > > > > > > > > You use ImageTypeId to identify a particular firmware object. > > > > > > > > (By "object," I mean one of firmware instances represented by "dfu_alto_info". > > > > > > > > Please don't confuse it with the binary blob embedded in a capsule file.) > > > > > > > > But ImageTypeId is not for that purpose, at least, as my intention > > > > > > > > in initially implementing capsule framework and FMP drivers. > > > > > > > > > > > > > > > > 1) ImageTypeId is used to uniquely identify a corresponding FMP driver, > > > > > > > > either FIT FMP driver or Raw FMP driver. > > > > > > > > > > > > > > I believe the identification of an FMP protocol should be done by the > > > > > > > FMP GUID, > > > > > > > > > > > > What does FMP GUID stand for? > > > > > > > > > > EFI_FIRMWARE_MANAGEMENT_PROTOCOL_GUID, defined in include/efi_api.h. > > > > > What I mean is that even when installing the FMP protocol, the call to > > > > > efi_install_multiple_protocol_interfaces takes the above FMP GUID as > > > > > an argument -- nowhere is the ImageTypeId considered when installing > > > > > the protocol. > > > > > > > > Okay. > > > > > > > > > > > > > > > > > which is what is done in efi_fmp_find. The ImageTypeId is > > > > > > > nowhere involved in this identification. > > > > > > > > > > > > Please take a look at efi_capsule_update_firmware() carefully. > > > > > > efi_find_fmp() is called with the image's update_image_type_id > > > > > > which is to be set to EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > > > > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID by mkeficapsule > > > > > > (see create_fwbin()). > > > > > > > > > > I think you are thinking from the point of view of the '--guid' value > > > > > that is being passed to the capsule generation tool. But the thing is > > > > > that it is the current design(or limitation) of the tool that it takes > > > > > only a single guid parameter. So the mkeficapsule tool currently can > > > > > generate only a single payload capsule. > > > > > > > > That is exactly what I intended to do here. > > > > We have only one FMP driver (either FIT or RAW) which is based on > > > > U-Boot's DFU framework and we need only one payload since, for > > > > multiple objects of firmware, we can use FIT format as a payload. > > > > That is what FIT is aimed for. > > > > Or you can use multiple RAW capsule files with different indexes > > > > ("--index" exists for this purpose). > > > > > > Yes, we can use --index when we know the index value corresponding to > > > the firmware image that we need to update. But like I mentioned in my > > > earlier reply, for A/B updates, we do not know what the index value is > > > going to be. That is going to be determined at runtime. > > > > I don't think so. See below for alternative approach. > > > > > Also, the point I was making is that we can have a capsule which is > > > consumed by an FMP protocol which has more than one image, and those > > > images have different ImageTypeId/UpdateImageTypeId. > > > > Yes, but it is a design choice in my first implementation. > > I didn't think that we need to "have a capsule which is consumed > > by an FMP protocol which has more than one image" as long as we > > use DFU framework (and FIT as standard format of aggregation on U-Boot). > > But this design can be extended without any hassle, and more > importantly without any regression, no? What kind of a problem does it > create if the FMP can handle more than one image type. > > Even as per the UEFI spec, we have the EFI_FIRMWARE_MANAGEMENT_CAPSULE > header for all images to be managed by the FMP protocol which has > multiple images with different UpdateImageTypeId. > > > > > > > > > > > > Please check the > > > > > GenerateCapsule script in EDK2. In case of a multi payload based > > > > > capsule, individual parameters like the UpdateImageTypeId are passed > > > > > through the json file, where each of the UpdateImageTypeId has a > > > > > different value per payload. > > > > > > > > > > > > > > > > > > > 2) Each firmware object handled by a given FMP driver can further be > > > > > > > > identified by ImageIndex. > > > > > > > > > > > > > > > > My implementation of efi_fmp_find() does (1) and Raw FMP driver does > > > > > > > > (2) in efi_firmware_raw_set_image() which takes "image_index" as > > > > > > > > a parameter. > > > > > > > > > > > > > > > > Using ImageTypeId as an identifier is simply wrong in my opinion and > > > > > > > > doesn't meet the UEFI specification. > > > > > > > > > > > > > > So, as per what you are stating, all payloads under a given > > > > > > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER should have the same > > > > > > > ImageTypeId, either EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > > > > > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID. Same applies for all images in > > > > > > > the EFI_FIRMWARE_IMAGE_DESCRIPTOR. Because, without one of the two > > > > > > > values, > the check in efi_fmp_find to compare the UpdateImageTypeId > > > > > > > with the ImageTypeId retrieved from the image descriptor would simply > > > > > > > fail. > > > > > > > > > > > > I don't follow your point. > > > > > > Please elaborate a bit more. > > > > > > > > > > The current implementation of GetImageInfo, passes either of > > > > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > > > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID for all the images of the image > > > > > descriptor array. So, in case the capsule is generated with a '--guid' > > > > > value which is different from these two values, the check in > > > > > efi_fmp_find on line 204 will fail. > > > > > > > > That is an expected behavior, isn't it? > > > > > > Yes it is. Do not contest that. > > > > > > > If you want to use a different FMP driver (with another GUID), > > > > you naturally need to add your own FMP driver. > > > > > > This is where I differ. We can use the same FMP protocol instance for > > > any type of ImageTypeId. I do not see why we need to define a > > > different FMP protocol instance for a GUID value other than what has > > > been defined for u-boot raw and u-boot FIT GUIDs. > > > > I do understand part of your concern a bit. > > I thought of using the same ImageType GUID, say IMAGE_TYPE_DFU_GUID, at first > > but we needed different GUIDs here simply because we need to determine > > the format of payload, FIT format or raw binary. > > > > > The platform can give us the image descriptor array, and with that, > > > the same FMP instance can be used for any type of image(ImageTypeId). > > > > "any type of image"? Really? > > The raw FMP instance can certainly handle any type of binary payloads > right. There is no restriction on what type of payload it is as long > as it is all going as a single entity to a given dfu partition. > > > > > > > > > > > > > > > > > This means that unless the --guid > > > > > value passed to the capsule generation is either of u-boot FIT or > > > > > u-boot raw, the current FMP protocol for raw devices cannot be used. > > > > > Why do we need that restriction. It should be possible to use the raw > > > > > FMP protocol for any other type of image types as well. > > > > > > > > > > > > > > > > > > > > > > > I think this interpretation of the UEFI spec is incorrect, since the > > > > > > > spec states that the ImageTypeId and the UpdateImageTypeId are fields > > > > > > > used to identify the firmware component targeted for the update. If > > > > > > > all values in the image descriptor array and the UpdateImageTypeId are > > > > > > > the same, why have this field in the first place for individual > > > > > > > images. > > > > > > > > > > > > As I said, ImageIndex is for that purpose. > > > > > > > > > > Yes, that is one possible way in the scenario where the ImageIndex is > > > > > determined at the capsule generation time. But, for the A/B update > > > > > scenario, we do not know the ImageIndex at build time > > > > > > > > "Build time" of what? > > > > > > Of the capsule. > > > > > > > I think that users should know how "dfu_alt_info" is defined > > > > (in other words, where the firmware be located on the target system) > > > > when capsule files are created. > > > > > > That is true for a non A/B scenario. And that is how it works in the > > > non A/B updates case. But for A/B updates, since the determination of > > > the "location" where the firmware image has to be written will be done > > > only at runtime, we cannot use the --index to differentiate. > > > > Yes, we can :) > > You know what I mean -- if we could use the same logic, I would not > have added all that code :) > > > > > First of all, my essential assumption in either FIT or RAW FMP driver > > is that U-Boot has (somehow conceptually) single firmware blob represented > > by DFU or dfu_alt_info. As I said, each object or location in > > dfu_alt_info can be further identified by index or "UpdateImageIndex". > > > > Let's assume that we have two locations of firmware, fw1 and fw2, and > > that we have two bank A and B. > > Then we will define dfu_alt_info as follows: > > <loc of fw1 for A>;<loc of fw2 for A>;<loc of fw1 for B>;<loc of fw2 for B>; > > |<--- 1st set --->|<--- 2nd set --->| > > > > When you want to update bank A, we can use the first set of dfu_alt_info, > > and use the second set of dfu_alt_info for bank B. > > At runtime, you should know which bank you're working on, and therefore > > you should know the exact physical location from dfu_alt_info. > > > > Please note that you don't have to change the syntax of dfu_alt_info > > at all. Simply offset the location with 0 for bank A and with 2 for bank B. I'll try digging a bit more, but I think the current approach is not working as it was intended wrt to the EFI spec. My reading of the spec and specifically section 23.3.2 is that a Capsule consists of an EFI capsule header and a payload. The payload now has an EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER which in it's turn can host multiple firmware images of different type described in EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER. An FMP implementation should read the UpdateImageTypeId's used to identify the image you are updating and from that derive the UpdateImageIndex which SetImage will use. That would give you the ability to update the all the firmware components with a single capsule. Sughosh what about the ESRT table generation? If you use different UpdateImageTypeId those should be reflected on the ESRT tables from the OS Regards /Ilias > > We can use this logic yes. But please note, that with this, we need to > a) Keep a definite order of the images in all the banks, and b) Know > the order of the images. > > With the way the FWU metadata is designed, we do not have these > restrictions -- the firmware images can be placed on the device in any > order, irrespective of the bank that they belong to. One might prefer > clubbing images of a given bank together, but that is not the > restriction put by the FWU spec. That is because the images are being > identified using image GUIDs. > > I really don't get your opposition to extending the current design. I > would like to hear from Heinrich or Ilias on what their thoughts are > on this. > > -sughosh > > > > > I don't think we need to introduce extra GUIDs. > > > > -Takahiro Akashi > > > > > Like I mentioned earlier, this is not breaking the existing behaviour > > > -- for the non A/B updates, the update procedure remains exactly the > > > same, of using the index value to determine the location of the > > > update. I have only extended the behaviour to use the same FMP > > > instance for the A/B update(FWU) feature using ImageTypeId's. > > > > > > -sughosh > > > > > > > > > > > -Takahiro Akashi > > > > > > > > > > > > > -- this is > > > > > determined only at runtime, and is based on the bank to which the > > > > > image is to be updated. Which is why I am finding out the alt_num at > > > > > runtime in case the FWU Multi Bank feature is enabled. Like I said > > > > > above, I do not see a reason why the current FMP protocols should be > > > > > restricted to only the u-boot FIT and u-boot raw image types. It is > > > > > being extended, without affecting the default non FWU behaviour. > > > > > > > > > > -sughosh > > > > > > > > > > > > > > > > > -Takahiro Akashi > > > > > > > > > > > > > > > > > > > > -sughosh > > > > > > > > > > > > > > > > > > > > > > > -Takahiro Akashi > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > > > > > --- > > > > > > > > > > > > > > > > > > Changes since V3: > > > > > > > > > * Define a weak function fill_image_type_guid_array for populating the > > > > > > > > > image descriptor array with u-boot's raw and fit image GUIDs > > > > > > > > > > > > > > > > > > include/efi_loader.h | 2 + > > > > > > > > > lib/efi_loader/efi_firmware.c | 71 +++++++++++++++++++++++++++++++---- > > > > > > > > > 2 files changed, 66 insertions(+), 7 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > > > > > > > > index f4860e87fc..ae60de0be5 100644 > > > > > > > > > --- a/include/efi_loader.h > > > > > > > > > +++ b/include/efi_loader.h > > > > > > > > > @@ -992,4 +992,6 @@ efi_status_t efi_esrt_populate(void); > > > > > > > > > efi_status_t efi_load_capsule_drivers(void); > > > > > > > > > > > > > > > > > > efi_status_t platform_get_eventlog(struct udevice *dev, u64 *addr, u32 *sz); > > > > > > > > > +efi_status_t fill_image_type_guid_array(const efi_guid_t *default_guid, > > > > > > > > > + efi_guid_t **part_guid_arr); > > > > > > > > > #endif /* _EFI_LOADER_H */ > > > > > > > > > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c > > > > > > > > > index a1b88dbfc2..5642be9f9a 100644 > > > > > > > > > --- a/lib/efi_loader/efi_firmware.c > > > > > > > > > +++ b/lib/efi_loader/efi_firmware.c > > > > > > > > > @@ -96,6 +96,46 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported( > > > > > > > > > return EFI_EXIT(EFI_UNSUPPORTED); > > > > > > > > > } > > > > > > > > > > > > > > > > > > +efi_status_t __weak fill_image_type_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, &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 +144,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 > > > > > > > > > * > > > > > > > > > * Return information bout the current firmware image in @image_info. > > > > > > > > > * @image_info will consist of a number of descriptors. > > > > > > > > > @@ -122,7 +162,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 +212,7 @@ static efi_status_t efi_get_dfu_info( > > > > > > > > > next = name; > > > > > > > > > list_for_each_entry(dfu, &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 +290,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 +305,19 @@ efi_status_t EFIAPI efi_firmware_fit_get_image_info( > > > > > > > > > !descriptor_size || !package_version || !package_version_name)) > > > > > > > > > return EFI_EXIT(EFI_INVALID_PARAMETER); > > > > > > > > > > > > > > > > > > + ret = fill_image_type_guid_array(&efi_firmware_image_type_uboot_fit, > > > > > > > > > + &part_guid_arr); > > > > > > > > > + if (ret != EFI_SUCCESS) > > > > > > > > > + goto out; > > > > > > > > > + > > > > > > > > > ret = efi_get_dfu_info(image_info_size, image_info, > > > > > > > > > descriptor_version, descriptor_count, > > > > > > > > > descriptor_size, > > > > > > > > > package_version, package_version_name, > > > > > > > > > - &efi_firmware_image_type_uboot_fit); > > > > > > > > > + part_guid_arr); > > > > > > > > > > > > > > > > > > +out: > > > > > > > > > + free(part_guid_arr); > > > > > > > > > return EFI_EXIT(ret); > > > > > > > > > } > > > > > > > > > > > > > > > > > > @@ -359,6 +407,7 @@ efi_status_t EFIAPI efi_firmware_raw_get_image_info( > > > > > > > > > u16 **package_version_name) > > > > > > > > > { > > > > > > > > > efi_status_t ret = EFI_SUCCESS; > > > > > > > > > + efi_guid_t *part_guid_arr = NULL; > > > > > > > > > > > > > > > > > > EFI_ENTRY("%p %p %p %p %p %p %p %p\n", this, > > > > > > > > > image_info_size, image_info, > > > > > > > > > @@ -373,12 +422,20 @@ efi_status_t EFIAPI efi_firmware_raw_get_image_info( > > > > > > > > > !descriptor_size || !package_version || !package_version_name)) > > > > > > > > > return EFI_EXIT(EFI_INVALID_PARAMETER); > > > > > > > > > > > > > > > > > > + ret = fill_image_type_guid_array( > > > > > > > > > + &efi_firmware_image_type_uboot_raw, > > > > > > > > > + &part_guid_arr); > > > > > > > > > + if (ret != EFI_SUCCESS) > > > > > > > > > + goto out; > > > > > > > > > + > > > > > > > > > ret = efi_get_dfu_info(image_info_size, image_info, > > > > > > > > > descriptor_version, descriptor_count, > > > > > > > > > descriptor_size, > > > > > > > > > package_version, package_version_name, > > > > > > > > > - &efi_firmware_image_type_uboot_raw); > > > > > > > > > + part_guid_arr); > > > > > > > > > > > > > > > > > > +out: > > > > > > > > > + free(part_guid_arr); > > > > > > > > > return EFI_EXIT(ret); > > > > > > > > > } > > > > > > > > > > > > > > > > > > -- > > > > > > > > > 2.17.1 > > > > > > > > >
hi Ilias, On Tue, 15 Feb 2022 at 20:10, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi, > > Took me some time to go through the whole thread, but here it goes. > > On Tue, Feb 15, 2022 at 12:08:30PM +0530, Sughosh Ganu wrote: > > On Tue, 15 Feb 2022 at 07:21, AKASHI Takahiro > > <takahiro.akashi@linaro.org> wrote: > > > > > > Sughosh, > > > > > > On Mon, Feb 14, 2022 at 11:12:22AM +0530, Sughosh Ganu wrote: > > > > hi Takahiro, > > > > > > > > On Mon, 14 Feb 2022 at 08:54, AKASHI Takahiro > > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > Sughosh, > > > > > > > > > > On Thu, Feb 10, 2022 at 03:40:00PM +0530, Sughosh Ganu wrote: > > > > > > On Thu, 10 Feb 2022 at 13:28, AKASHI Takahiro > > > > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > > > > > On Thu, Feb 10, 2022 at 12:48:13PM +0530, Sughosh Ganu wrote: > > > > > > > > hi Takahiro, > > > > > > > > > > > > > > > > On Thu, 10 Feb 2022 at 08:18, AKASHI Takahiro > > > > > > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > > > > > > > > > Hi Sughosh, > > > > > > > > > > > > > > > > > > On Mon, Feb 07, 2022 at 11:49:55PM +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. > > > > > > > > > > > > > > > > > > After re-thinking of your approach here, I would have to say NAK. > > > > > > > > > > > > > > > > > > You use ImageTypeId to identify a particular firmware object. > > > > > > > > > (By "object," I mean one of firmware instances represented by "dfu_alto_info". > > > > > > > > > Please don't confuse it with the binary blob embedded in a capsule file.) > > > > > > > > > But ImageTypeId is not for that purpose, at least, as my intention > > > > > > > > > in initially implementing capsule framework and FMP drivers. > > > > > > > > > > > > > > > > > > 1) ImageTypeId is used to uniquely identify a corresponding FMP driver, > > > > > > > > > either FIT FMP driver or Raw FMP driver. > > > > > > > > > > > > > > > > I believe the identification of an FMP protocol should be done by the > > > > > > > > FMP GUID, > > > > > > > > > > > > > > What does FMP GUID stand for? > > > > > > > > > > > > EFI_FIRMWARE_MANAGEMENT_PROTOCOL_GUID, defined in include/efi_api.h. > > > > > > What I mean is that even when installing the FMP protocol, the call to > > > > > > efi_install_multiple_protocol_interfaces takes the above FMP GUID as > > > > > > an argument -- nowhere is the ImageTypeId considered when installing > > > > > > the protocol. > > > > > > > > > > Okay. > > > > > > > > > > > > > > > > > > > > which is what is done in efi_fmp_find. The ImageTypeId is > > > > > > > > nowhere involved in this identification. > > > > > > > > > > > > > > Please take a look at efi_capsule_update_firmware() carefully. > > > > > > > efi_find_fmp() is called with the image's update_image_type_id > > > > > > > which is to be set to EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > > > > > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID by mkeficapsule > > > > > > > (see create_fwbin()). > > > > > > > > > > > > I think you are thinking from the point of view of the '--guid' value > > > > > > that is being passed to the capsule generation tool. But the thing is > > > > > > that it is the current design(or limitation) of the tool that it takes > > > > > > only a single guid parameter. So the mkeficapsule tool currently can > > > > > > generate only a single payload capsule. > > > > > > > > > > That is exactly what I intended to do here. > > > > > We have only one FMP driver (either FIT or RAW) which is based on > > > > > U-Boot's DFU framework and we need only one payload since, for > > > > > multiple objects of firmware, we can use FIT format as a payload. > > > > > That is what FIT is aimed for. > > > > > Or you can use multiple RAW capsule files with different indexes > > > > > ("--index" exists for this purpose). > > > > > > > > Yes, we can use --index when we know the index value corresponding to > > > > the firmware image that we need to update. But like I mentioned in my > > > > earlier reply, for A/B updates, we do not know what the index value is > > > > going to be. That is going to be determined at runtime. > > > > > > I don't think so. See below for alternative approach. > > > > > > > Also, the point I was making is that we can have a capsule which is > > > > consumed by an FMP protocol which has more than one image, and those > > > > images have different ImageTypeId/UpdateImageTypeId. > > > > > > Yes, but it is a design choice in my first implementation. > > > I didn't think that we need to "have a capsule which is consumed > > > by an FMP protocol which has more than one image" as long as we > > > use DFU framework (and FIT as standard format of aggregation on U-Boot). > > > > But this design can be extended without any hassle, and more > > importantly without any regression, no? What kind of a problem does it > > create if the FMP can handle more than one image type. > > > > Even as per the UEFI spec, we have the EFI_FIRMWARE_MANAGEMENT_CAPSULE > > header for all images to be managed by the FMP protocol which has > > multiple images with different UpdateImageTypeId. > > > > > > > > > > > > > > > > Please check the > > > > > > GenerateCapsule script in EDK2. In case of a multi payload based > > > > > > capsule, individual parameters like the UpdateImageTypeId are passed > > > > > > through the json file, where each of the UpdateImageTypeId has a > > > > > > different value per payload. > > > > > > > > > > > > > > > > > > > > > > 2) Each firmware object handled by a given FMP driver can further be > > > > > > > > > identified by ImageIndex. > > > > > > > > > > > > > > > > > > My implementation of efi_fmp_find() does (1) and Raw FMP driver does > > > > > > > > > (2) in efi_firmware_raw_set_image() which takes "image_index" as > > > > > > > > > a parameter. > > > > > > > > > > > > > > > > > > Using ImageTypeId as an identifier is simply wrong in my opinion and > > > > > > > > > doesn't meet the UEFI specification. > > > > > > > > > > > > > > > > So, as per what you are stating, all payloads under a given > > > > > > > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER should have the same > > > > > > > > ImageTypeId, either EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > > > > > > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID. Same applies for all images in > > > > > > > > the EFI_FIRMWARE_IMAGE_DESCRIPTOR. Because, without one of the two > > > > > > > > values, > the check in efi_fmp_find to compare the UpdateImageTypeId > > > > > > > > with the ImageTypeId retrieved from the image descriptor would simply > > > > > > > > fail. > > > > > > > > > > > > > > I don't follow your point. > > > > > > > Please elaborate a bit more. > > > > > > > > > > > > The current implementation of GetImageInfo, passes either of > > > > > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > > > > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID for all the images of the image > > > > > > descriptor array. So, in case the capsule is generated with a '--guid' > > > > > > value which is different from these two values, the check in > > > > > > efi_fmp_find on line 204 will fail. > > > > > > > > > > That is an expected behavior, isn't it? > > > > > > > > Yes it is. Do not contest that. > > > > > > > > > If you want to use a different FMP driver (with another GUID), > > > > > you naturally need to add your own FMP driver. > > > > > > > > This is where I differ. We can use the same FMP protocol instance for > > > > any type of ImageTypeId. I do not see why we need to define a > > > > different FMP protocol instance for a GUID value other than what has > > > > been defined for u-boot raw and u-boot FIT GUIDs. > > > > > > I do understand part of your concern a bit. > > > I thought of using the same ImageType GUID, say IMAGE_TYPE_DFU_GUID, at first > > > but we needed different GUIDs here simply because we need to determine > > > the format of payload, FIT format or raw binary. > > > > > > > The platform can give us the image descriptor array, and with that, > > > > the same FMP instance can be used for any type of image(ImageTypeId). > > > > > > "any type of image"? Really? > > > > The raw FMP instance can certainly handle any type of binary payloads > > right. There is no restriction on what type of payload it is as long > > as it is all going as a single entity to a given dfu partition. > > > > > > > > > > > > > > > > > > > > > > This means that unless the --guid > > > > > > value passed to the capsule generation is either of u-boot FIT or > > > > > > u-boot raw, the current FMP protocol for raw devices cannot be used. > > > > > > Why do we need that restriction. It should be possible to use the raw > > > > > > FMP protocol for any other type of image types as well. > > > > > > > > > > > > > > > > > > > > > > > > > > > I think this interpretation of the UEFI spec is incorrect, since the > > > > > > > > spec states that the ImageTypeId and the UpdateImageTypeId are fields > > > > > > > > used to identify the firmware component targeted for the update. If > > > > > > > > all values in the image descriptor array and the UpdateImageTypeId are > > > > > > > > the same, why have this field in the first place for individual > > > > > > > > images. > > > > > > > > > > > > > > As I said, ImageIndex is for that purpose. > > > > > > > > > > > > Yes, that is one possible way in the scenario where the ImageIndex is > > > > > > determined at the capsule generation time. But, for the A/B update > > > > > > scenario, we do not know the ImageIndex at build time > > > > > > > > > > "Build time" of what? > > > > > > > > Of the capsule. > > > > > > > > > I think that users should know how "dfu_alt_info" is defined > > > > > (in other words, where the firmware be located on the target system) > > > > > when capsule files are created. > > > > > > > > That is true for a non A/B scenario. And that is how it works in the > > > > non A/B updates case. But for A/B updates, since the determination of > > > > the "location" where the firmware image has to be written will be done > > > > only at runtime, we cannot use the --index to differentiate. > > > > > > Yes, we can :) > > > > You know what I mean -- if we could use the same logic, I would not > > have added all that code :) > > > > > > > > First of all, my essential assumption in either FIT or RAW FMP driver > > > is that U-Boot has (somehow conceptually) single firmware blob represented > > > by DFU or dfu_alt_info. As I said, each object or location in > > > dfu_alt_info can be further identified by index or "UpdateImageIndex". > > > > > > Let's assume that we have two locations of firmware, fw1 and fw2, and > > > that we have two bank A and B. > > > Then we will define dfu_alt_info as follows: > > > <loc of fw1 for A>;<loc of fw2 for A>;<loc of fw1 for B>;<loc of fw2 for B>; > > > |<--- 1st set --->|<--- 2nd set --->| > > > > > > When you want to update bank A, we can use the first set of dfu_alt_info, > > > and use the second set of dfu_alt_info for bank B. > > > At runtime, you should know which bank you're working on, and therefore > > > you should know the exact physical location from dfu_alt_info. > > > > > > Please note that you don't have to change the syntax of dfu_alt_info > > > at all. Simply offset the location with 0 for bank A and with 2 for bank B. > > I'll try digging a bit more, but I think the current approach is not > working as it was intended wrt to the EFI spec. My reading of the spec > and specifically section 23.3.2 is that a Capsule consists of an > EFI capsule header and a payload. The payload now has an > EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER which in it's turn can host multiple > firmware images of different type described in EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER. > > An FMP implementation should read the UpdateImageTypeId's used to identify > the image you are updating and from that derive the UpdateImageIndex > which SetImage will use. That would give you the ability to update the > all the firmware components with a single capsule. > > Sughosh what about the ESRT table generation? If you use different UpdateImageTypeId > those should be reflected on the ESRT tables from the OS That would depend on the values populated in the EFI_FIRMWARE_IMAGE_DESCRIPTOR array by the GetImageInfo function. The image descriptor structure has an ImageTypeId field. The value of ImageTypeId is what will be reflected in the ESRT table. In the current implementation, all the images in the ESRT table will show the same ImageTypeId value, either EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID. The UpdateImageTypeId value from the capsule is used to compare with the ImageTypeId values returned by the GetImageInfo function to check if the given FMP protocol can be used for the update. -sughosh > > Regards > /Ilias > > > > > We can use this logic yes. But please note, that with this, we need to > > a) Keep a definite order of the images in all the banks, and b) Know > > the order of the images. > > > > With the way the FWU metadata is designed, we do not have these > > restrictions -- the firmware images can be placed on the device in any > > order, irrespective of the bank that they belong to. One might prefer > > clubbing images of a given bank together, but that is not the > > restriction put by the FWU spec. That is because the images are being > > identified using image GUIDs. > > > > I really don't get your opposition to extending the current design. I > > would like to hear from Heinrich or Ilias on what their thoughts are > > on this. > > > > -sughosh > > > > > > > > I don't think we need to introduce extra GUIDs. > > > > > > -Takahiro Akashi > > > > > > > Like I mentioned earlier, this is not breaking the existing behaviour > > > > -- for the non A/B updates, the update procedure remains exactly the > > > > same, of using the index value to determine the location of the > > > > update. I have only extended the behaviour to use the same FMP > > > > instance for the A/B update(FWU) feature using ImageTypeId's. > > > > > > > > -sughosh > > > > > > > > > > > > > > -Takahiro Akashi > > > > > > > > > > > > > > > > -- this is > > > > > > determined only at runtime, and is based on the bank to which the > > > > > > image is to be updated. Which is why I am finding out the alt_num at > > > > > > runtime in case the FWU Multi Bank feature is enabled. Like I said > > > > > > above, I do not see a reason why the current FMP protocols should be > > > > > > restricted to only the u-boot FIT and u-boot raw image types. It is > > > > > > being extended, without affecting the default non FWU behaviour. > > > > > > > > > > > > -sughosh > > > > > > > > > > > > > > > > > > > > -Takahiro Akashi > > > > > > > > > > > > > > > > > > > > > > > -sughosh > > > > > > > > > > > > > > > > > > > > > > > > > > -Takahiro Akashi > > > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > > > > > > --- > > > > > > > > > > > > > > > > > > > > Changes since V3: > > > > > > > > > > * Define a weak function fill_image_type_guid_array for populating the > > > > > > > > > > image descriptor array with u-boot's raw and fit image GUIDs > > > > > > > > > > > > > > > > > > > > include/efi_loader.h | 2 + > > > > > > > > > > lib/efi_loader/efi_firmware.c | 71 +++++++++++++++++++++++++++++++---- > > > > > > > > > > 2 files changed, 66 insertions(+), 7 deletions(-) > > > > > > > > > > > > > > > > > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > > > > > > > > > index f4860e87fc..ae60de0be5 100644 > > > > > > > > > > --- a/include/efi_loader.h > > > > > > > > > > +++ b/include/efi_loader.h > > > > > > > > > > @@ -992,4 +992,6 @@ efi_status_t efi_esrt_populate(void); > > > > > > > > > > efi_status_t efi_load_capsule_drivers(void); > > > > > > > > > > > > > > > > > > > > efi_status_t platform_get_eventlog(struct udevice *dev, u64 *addr, u32 *sz); > > > > > > > > > > +efi_status_t fill_image_type_guid_array(const efi_guid_t *default_guid, > > > > > > > > > > + efi_guid_t **part_guid_arr); > > > > > > > > > > #endif /* _EFI_LOADER_H */ > > > > > > > > > > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c > > > > > > > > > > index a1b88dbfc2..5642be9f9a 100644 > > > > > > > > > > --- a/lib/efi_loader/efi_firmware.c > > > > > > > > > > +++ b/lib/efi_loader/efi_firmware.c > > > > > > > > > > @@ -96,6 +96,46 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported( > > > > > > > > > > return EFI_EXIT(EFI_UNSUPPORTED); > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > +efi_status_t __weak fill_image_type_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, &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 +144,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 > > > > > > > > > > * > > > > > > > > > > * Return information bout the current firmware image in @image_info. > > > > > > > > > > * @image_info will consist of a number of descriptors. > > > > > > > > > > @@ -122,7 +162,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 +212,7 @@ static efi_status_t efi_get_dfu_info( > > > > > > > > > > next = name; > > > > > > > > > > list_for_each_entry(dfu, &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 +290,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 +305,19 @@ efi_status_t EFIAPI efi_firmware_fit_get_image_info( > > > > > > > > > > !descriptor_size || !package_version || !package_version_name)) > > > > > > > > > > return EFI_EXIT(EFI_INVALID_PARAMETER); > > > > > > > > > > > > > > > > > > > > + ret = fill_image_type_guid_array(&efi_firmware_image_type_uboot_fit, > > > > > > > > > > + &part_guid_arr); > > > > > > > > > > + if (ret != EFI_SUCCESS) > > > > > > > > > > + goto out; > > > > > > > > > > + > > > > > > > > > > ret = efi_get_dfu_info(image_info_size, image_info, > > > > > > > > > > descriptor_version, descriptor_count, > > > > > > > > > > descriptor_size, > > > > > > > > > > package_version, package_version_name, > > > > > > > > > > - &efi_firmware_image_type_uboot_fit); > > > > > > > > > > + part_guid_arr); > > > > > > > > > > > > > > > > > > > > +out: > > > > > > > > > > + free(part_guid_arr); > > > > > > > > > > return EFI_EXIT(ret); > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > @@ -359,6 +407,7 @@ efi_status_t EFIAPI efi_firmware_raw_get_image_info( > > > > > > > > > > u16 **package_version_name) > > > > > > > > > > { > > > > > > > > > > efi_status_t ret = EFI_SUCCESS; > > > > > > > > > > + efi_guid_t *part_guid_arr = NULL; > > > > > > > > > > > > > > > > > > > > EFI_ENTRY("%p %p %p %p %p %p %p %p\n", this, > > > > > > > > > > image_info_size, image_info, > > > > > > > > > > @@ -373,12 +422,20 @@ efi_status_t EFIAPI efi_firmware_raw_get_image_info( > > > > > > > > > > !descriptor_size || !package_version || !package_version_name)) > > > > > > > > > > return EFI_EXIT(EFI_INVALID_PARAMETER); > > > > > > > > > > > > > > > > > > > > + ret = fill_image_type_guid_array( > > > > > > > > > > + &efi_firmware_image_type_uboot_raw, > > > > > > > > > > + &part_guid_arr); > > > > > > > > > > + if (ret != EFI_SUCCESS) > > > > > > > > > > + goto out; > > > > > > > > > > + > > > > > > > > > > ret = efi_get_dfu_info(image_info_size, image_info, > > > > > > > > > > descriptor_version, descriptor_count, > > > > > > > > > > descriptor_size, > > > > > > > > > > package_version, package_version_name, > > > > > > > > > > - &efi_firmware_image_type_uboot_raw); > > > > > > > > > > + part_guid_arr); > > > > > > > > > > > > > > > > > > > > +out: > > > > > > > > > > + free(part_guid_arr); > > > > > > > > > > return EFI_EXIT(ret); > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > 2.17.1 > > > > > > > > > >
> > > > > [...] > > > > > Yes, we can use --index when we know the index value corresponding to > > > > > the firmware image that we need to update. But like I mentioned in my > > > > > earlier reply, for A/B updates, we do not know what the index value is > > > > > going to be. That is going to be determined at runtime. > > > > > > > > I don't think so. See below for alternative approach. > > > > > > > > > Also, the point I was making is that we can have a capsule which is > > > > > consumed by an FMP protocol which has more than one image, and those > > > > > images have different ImageTypeId/UpdateImageTypeId. > > > > > > > > Yes, but it is a design choice in my first implementation. > > > > I didn't think that we need to "have a capsule which is consumed > > > > by an FMP protocol which has more than one image" as long as we > > > > use DFU framework (and FIT as standard format of aggregation on U-Boot). > > > > > > But this design can be extended without any hassle, and more > > > importantly without any regression, no? What kind of a problem does it > > > create if the FMP can handle more than one image type. > > > > > > Even as per the UEFI spec, we have the EFI_FIRMWARE_MANAGEMENT_CAPSULE > > > header for all images to be managed by the FMP protocol which has > > > multiple images with different UpdateImageTypeId. > > > > > > > > > > > > > > > > > > > > Please check the > > > > > > > GenerateCapsule script in EDK2. In case of a multi payload based > > > > > > > capsule, individual parameters like the UpdateImageTypeId are passed > > > > > > > through the json file, where each of the UpdateImageTypeId has a > > > > > > > different value per payload. > > > > > > > > > > > > > > > > > > > > > > > > > 2) Each firmware object handled by a given FMP driver can further be > > > > > > > > > > identified by ImageIndex. > > > > > > > > > > > > > > > > > > > > My implementation of efi_fmp_find() does (1) and Raw FMP driver does > > > > > > > > > > (2) in efi_firmware_raw_set_image() which takes "image_index" as > > > > > > > > > > a parameter. > > > > > > > > > > > > > > > > > > > > Using ImageTypeId as an identifier is simply wrong in my opinion and > > > > > > > > > > doesn't meet the UEFI specification. > > > > > > > > > > > > > > > > > > So, as per what you are stating, all payloads under a given > > > > > > > > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER should have the same > > > > > > > > > ImageTypeId, either EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > > > > > > > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID. Same applies for all images in > > > > > > > > > the EFI_FIRMWARE_IMAGE_DESCRIPTOR. Because, without one of the two > > > > > > > > > values, > the check in efi_fmp_find to compare the UpdateImageTypeId > > > > > > > > > with the ImageTypeId retrieved from the image descriptor would simply > > > > > > > > > fail. > > > > > > > > > > > > > > > > I don't follow your point. > > > > > > > > Please elaborate a bit more. > > > > > > > > > > > > > > The current implementation of GetImageInfo, passes either of > > > > > > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > > > > > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID for all the images of the image > > > > > > > descriptor array. So, in case the capsule is generated with a '--guid' > > > > > > > value which is different from these two values, the check in > > > > > > > efi_fmp_find on line 204 will fail. > > > > > > > > > > > > That is an expected behavior, isn't it? > > > > > > > > > > Yes it is. Do not contest that. > > > > > > > > > > > If you want to use a different FMP driver (with another GUID), > > > > > > you naturally need to add your own FMP driver. > > > > > > > > > > This is where I differ. We can use the same FMP protocol instance for > > > > > any type of ImageTypeId. I do not see why we need to define a > > > > > different FMP protocol instance for a GUID value other than what has > > > > > been defined for u-boot raw and u-boot FIT GUIDs. > > > > > > > > I do understand part of your concern a bit. > > > > I thought of using the same ImageType GUID, say IMAGE_TYPE_DFU_GUID, at first > > > > but we needed different GUIDs here simply because we need to determine > > > > the format of payload, FIT format or raw binary. > > > > > > > > > The platform can give us the image descriptor array, and with that, > > > > > the same FMP instance can be used for any type of image(ImageTypeId). > > > > > > > > "any type of image"? Really? > > > > > > The raw FMP instance can certainly handle any type of binary payloads > > > right. There is no restriction on what type of payload it is as long > > > as it is all going as a single entity to a given dfu partition. > > > > > > > > > > > > > > > > > > > > > > > > > > > This means that unless the --guid > > > > > > > value passed to the capsule generation is either of u-boot FIT or > > > > > > > u-boot raw, the current FMP protocol for raw devices cannot be used. > > > > > > > Why do we need that restriction. It should be possible to use the raw > > > > > > > FMP protocol for any other type of image types as well. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I think this interpretation of the UEFI spec is incorrect, since the > > > > > > > > > spec states that the ImageTypeId and the UpdateImageTypeId are fields > > > > > > > > > used to identify the firmware component targeted for the update. If > > > > > > > > > all values in the image descriptor array and the UpdateImageTypeId are > > > > > > > > > the same, why have this field in the first place for individual > > > > > > > > > images. > > > > > > > > > > > > > > > > As I said, ImageIndex is for that purpose. > > > > > > > > > > > > > > Yes, that is one possible way in the scenario where the ImageIndex is > > > > > > > determined at the capsule generation time. But, for the A/B update > > > > > > > scenario, we do not know the ImageIndex at build time > > > > > > > > > > > > "Build time" of what? > > > > > > > > > > Of the capsule. > > > > > > > > > > > I think that users should know how "dfu_alt_info" is defined > > > > > > (in other words, where the firmware be located on the target system) > > > > > > when capsule files are created. > > > > > > > > > > That is true for a non A/B scenario. And that is how it works in the > > > > > non A/B updates case. But for A/B updates, since the determination of > > > > > the "location" where the firmware image has to be written will be done > > > > > only at runtime, we cannot use the --index to differentiate. > > > > > > > > Yes, we can :) > > > > > > You know what I mean -- if we could use the same logic, I would not > > > have added all that code :) > > > > > > > > > > > First of all, my essential assumption in either FIT or RAW FMP driver > > > > is that U-Boot has (somehow conceptually) single firmware blob represented > > > > by DFU or dfu_alt_info. As I said, each object or location in > > > > dfu_alt_info can be further identified by index or "UpdateImageIndex". > > > > > > > > Let's assume that we have two locations of firmware, fw1 and fw2, and > > > > that we have two bank A and B. > > > > Then we will define dfu_alt_info as follows: > > > > <loc of fw1 for A>;<loc of fw2 for A>;<loc of fw1 for B>;<loc of fw2 for B>; > > > > |<--- 1st set --->|<--- 2nd set --->| > > > > > > > > When you want to update bank A, we can use the first set of dfu_alt_info, > > > > and use the second set of dfu_alt_info for bank B. > > > > At runtime, you should know which bank you're working on, and therefore > > > > you should know the exact physical location from dfu_alt_info. > > > > > > > > Please note that you don't have to change the syntax of dfu_alt_info > > > > at all. Simply offset the location with 0 for bank A and with 2 for bank B. > > > > I'll try digging a bit more, but I think the current approach is not > > working as it was intended wrt to the EFI spec. My reading of the spec > > and specifically section 23.3.2 is that a Capsule consists of an > > EFI capsule header and a payload. The payload now has an > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER which in it's turn can host multiple > > firmware images of different type described in EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER. > > > > An FMP implementation should read the UpdateImageTypeId's used to identify > > the image you are updating and from that derive the UpdateImageIndex > > which SetImage will use. That would give you the ability to update the > > all the firmware components with a single capsule. > > > > Sughosh what about the ESRT table generation? If you use different UpdateImageTypeId > > those should be reflected on the ESRT tables from the OS > > That would depend on the values populated in the > EFI_FIRMWARE_IMAGE_DESCRIPTOR array by the GetImageInfo function. The > image descriptor structure has an ImageTypeId field. The value of > ImageTypeId is what will be reflected in the ESRT table. > > In the current implementation, all the images in the ESRT table will > show the same ImageTypeId value, either > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID. Yea I was mostly asking wrt to A/B updates. Would the correct UUID be shown in the ESRT instead of EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID? > > The UpdateImageTypeId value from the capsule is used to compare with > the ImageTypeId values returned by the GetImageInfo function to check > if the given FMP protocol can be used for the update. > > -sughosh > [...] Regards /Ilias
On Thu, 17 Feb 2022 at 13:52, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > [...] > > > > > > > Yes, we can use --index when we know the index value corresponding to > > > > > > the firmware image that we need to update. But like I mentioned in my > > > > > > earlier reply, for A/B updates, we do not know what the index value is > > > > > > going to be. That is going to be determined at runtime. > > > > > > > > > > I don't think so. See below for alternative approach. > > > > > > > > > > > Also, the point I was making is that we can have a capsule which is > > > > > > consumed by an FMP protocol which has more than one image, and those > > > > > > images have different ImageTypeId/UpdateImageTypeId. > > > > > > > > > > Yes, but it is a design choice in my first implementation. > > > > > I didn't think that we need to "have a capsule which is consumed > > > > > by an FMP protocol which has more than one image" as long as we > > > > > use DFU framework (and FIT as standard format of aggregation on U-Boot). > > > > > > > > But this design can be extended without any hassle, and more > > > > importantly without any regression, no? What kind of a problem does it > > > > create if the FMP can handle more than one image type. > > > > > > > > Even as per the UEFI spec, we have the EFI_FIRMWARE_MANAGEMENT_CAPSULE > > > > header for all images to be managed by the FMP protocol which has > > > > multiple images with different UpdateImageTypeId. > > > > > > > > > > > > > > > > > > > > > > > > Please check the > > > > > > > > GenerateCapsule script in EDK2. In case of a multi payload based > > > > > > > > capsule, individual parameters like the UpdateImageTypeId are passed > > > > > > > > through the json file, where each of the UpdateImageTypeId has a > > > > > > > > different value per payload. > > > > > > > > > > > > > > > > > > > > > > > > > > > > 2) Each firmware object handled by a given FMP driver can further be > > > > > > > > > > > identified by ImageIndex. > > > > > > > > > > > > > > > > > > > > > > My implementation of efi_fmp_find() does (1) and Raw FMP driver does > > > > > > > > > > > (2) in efi_firmware_raw_set_image() which takes "image_index" as > > > > > > > > > > > a parameter. > > > > > > > > > > > > > > > > > > > > > > Using ImageTypeId as an identifier is simply wrong in my opinion and > > > > > > > > > > > doesn't meet the UEFI specification. > > > > > > > > > > > > > > > > > > > > So, as per what you are stating, all payloads under a given > > > > > > > > > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER should have the same > > > > > > > > > > ImageTypeId, either EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > > > > > > > > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID. Same applies for all images in > > > > > > > > > > the EFI_FIRMWARE_IMAGE_DESCRIPTOR. Because, without one of the two > > > > > > > > > > values, > the check in efi_fmp_find to compare the UpdateImageTypeId > > > > > > > > > > with the ImageTypeId retrieved from the image descriptor would simply > > > > > > > > > > fail. > > > > > > > > > > > > > > > > > > I don't follow your point. > > > > > > > > > Please elaborate a bit more. > > > > > > > > > > > > > > > > The current implementation of GetImageInfo, passes either of > > > > > > > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > > > > > > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID for all the images of the image > > > > > > > > descriptor array. So, in case the capsule is generated with a '--guid' > > > > > > > > value which is different from these two values, the check in > > > > > > > > efi_fmp_find on line 204 will fail. > > > > > > > > > > > > > > That is an expected behavior, isn't it? > > > > > > > > > > > > Yes it is. Do not contest that. > > > > > > > > > > > > > If you want to use a different FMP driver (with another GUID), > > > > > > > you naturally need to add your own FMP driver. > > > > > > > > > > > > This is where I differ. We can use the same FMP protocol instance for > > > > > > any type of ImageTypeId. I do not see why we need to define a > > > > > > different FMP protocol instance for a GUID value other than what has > > > > > > been defined for u-boot raw and u-boot FIT GUIDs. > > > > > > > > > > I do understand part of your concern a bit. > > > > > I thought of using the same ImageType GUID, say IMAGE_TYPE_DFU_GUID, at first > > > > > but we needed different GUIDs here simply because we need to determine > > > > > the format of payload, FIT format or raw binary. > > > > > > > > > > > The platform can give us the image descriptor array, and with that, > > > > > > the same FMP instance can be used for any type of image(ImageTypeId). > > > > > > > > > > "any type of image"? Really? > > > > > > > > The raw FMP instance can certainly handle any type of binary payloads > > > > right. There is no restriction on what type of payload it is as long > > > > as it is all going as a single entity to a given dfu partition. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This means that unless the --guid > > > > > > > > value passed to the capsule generation is either of u-boot FIT or > > > > > > > > u-boot raw, the current FMP protocol for raw devices cannot be used. > > > > > > > > Why do we need that restriction. It should be possible to use the raw > > > > > > > > FMP protocol for any other type of image types as well. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I think this interpretation of the UEFI spec is incorrect, since the > > > > > > > > > > spec states that the ImageTypeId and the UpdateImageTypeId are fields > > > > > > > > > > used to identify the firmware component targeted for the update. If > > > > > > > > > > all values in the image descriptor array and the UpdateImageTypeId are > > > > > > > > > > the same, why have this field in the first place for individual > > > > > > > > > > images. > > > > > > > > > > > > > > > > > > As I said, ImageIndex is for that purpose. > > > > > > > > > > > > > > > > Yes, that is one possible way in the scenario where the ImageIndex is > > > > > > > > determined at the capsule generation time. But, for the A/B update > > > > > > > > scenario, we do not know the ImageIndex at build time > > > > > > > > > > > > > > "Build time" of what? > > > > > > > > > > > > Of the capsule. > > > > > > > > > > > > > I think that users should know how "dfu_alt_info" is defined > > > > > > > (in other words, where the firmware be located on the target system) > > > > > > > when capsule files are created. > > > > > > > > > > > > That is true for a non A/B scenario. And that is how it works in the > > > > > > non A/B updates case. But for A/B updates, since the determination of > > > > > > the "location" where the firmware image has to be written will be done > > > > > > only at runtime, we cannot use the --index to differentiate. > > > > > > > > > > Yes, we can :) > > > > > > > > You know what I mean -- if we could use the same logic, I would not > > > > have added all that code :) > > > > > > > > > > > > > > First of all, my essential assumption in either FIT or RAW FMP driver > > > > > is that U-Boot has (somehow conceptually) single firmware blob represented > > > > > by DFU or dfu_alt_info. As I said, each object or location in > > > > > dfu_alt_info can be further identified by index or "UpdateImageIndex". > > > > > > > > > > Let's assume that we have two locations of firmware, fw1 and fw2, and > > > > > that we have two bank A and B. > > > > > Then we will define dfu_alt_info as follows: > > > > > <loc of fw1 for A>;<loc of fw2 for A>;<loc of fw1 for B>;<loc of fw2 for B>; > > > > > |<--- 1st set --->|<--- 2nd set --->| > > > > > > > > > > When you want to update bank A, we can use the first set of dfu_alt_info, > > > > > and use the second set of dfu_alt_info for bank B. > > > > > At runtime, you should know which bank you're working on, and therefore > > > > > you should know the exact physical location from dfu_alt_info. > > > > > > > > > > Please note that you don't have to change the syntax of dfu_alt_info > > > > > at all. Simply offset the location with 0 for bank A and with 2 for bank B. > > > > > > I'll try digging a bit more, but I think the current approach is not > > > working as it was intended wrt to the EFI spec. My reading of the spec > > > and specifically section 23.3.2 is that a Capsule consists of an > > > EFI capsule header and a payload. The payload now has an > > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER which in it's turn can host multiple > > > firmware images of different type described in EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER. > > > > > > An FMP implementation should read the UpdateImageTypeId's used to identify > > > the image you are updating and from that derive the UpdateImageIndex > > > which SetImage will use. That would give you the ability to update the > > > all the firmware components with a single capsule. > > > > > > Sughosh what about the ESRT table generation? If you use different UpdateImageTypeId > > > those should be reflected on the ESRT tables from the OS > > > > That would depend on the values populated in the > > EFI_FIRMWARE_IMAGE_DESCRIPTOR array by the GetImageInfo function. The > > image descriptor structure has an ImageTypeId field. The value of > > ImageTypeId is what will be reflected in the ESRT table. > > > > In the current implementation, all the images in the ESRT table will > > show the same ImageTypeId value, either > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID. > > Yea I was mostly asking wrt to A/B updates. Would the correct UUID be > shown in the ESRT instead of EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID? Yes, the platform would have to define the fill_image_type_guid_array function which would populate the ImageTypeId values in the EFI_FIRMWARE_IMAGE_DESCRIPTOR array, and the image GUID values would then show up as part of the ESRT table. As part of this patchset, I have added this function for the STM32MP1 DK2 board. -sughosh > > > > > The UpdateImageTypeId value from the capsule is used to compare with > > the ImageTypeId values returned by the GetImageInfo function to check > > if the given FMP protocol can be used for the update. > > > > -sughosh > > > [...] > > > Regards > /Ilias
On 2/17/22 11:10, Sughosh Ganu wrote: > On Thu, 17 Feb 2022 at 13:52, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: >> >>>>>>> >> >> [...] >> >>>>>>> Yes, we can use --index when we know the index value corresponding to >>>>>>> the firmware image that we need to update. But like I mentioned in my >>>>>>> earlier reply, for A/B updates, we do not know what the index value is >>>>>>> going to be. That is going to be determined at runtime. >>>>>> >>>>>> I don't think so. See below for alternative approach. >>>>>> >>>>>>> Also, the point I was making is that we can have a capsule which is >>>>>>> consumed by an FMP protocol which has more than one image, and those >>>>>>> images have different ImageTypeId/UpdateImageTypeId. >>>>>> >>>>>> Yes, but it is a design choice in my first implementation. >>>>>> I didn't think that we need to "have a capsule which is consumed >>>>>> by an FMP protocol which has more than one image" as long as we >>>>>> use DFU framework (and FIT as standard format of aggregation on U-Boot). >>>>> >>>>> But this design can be extended without any hassle, and more >>>>> importantly without any regression, no? What kind of a problem does it >>>>> create if the FMP can handle more than one image type. >>>>> >>>>> Even as per the UEFI spec, we have the EFI_FIRMWARE_MANAGEMENT_CAPSULE >>>>> header for all images to be managed by the FMP protocol which has >>>>> multiple images with different UpdateImageTypeId. >>>>> >>>>>> >>>>>>>> >>>>>>>>> Please check the >>>>>>>>> GenerateCapsule script in EDK2. In case of a multi payload based >>>>>>>>> capsule, individual parameters like the UpdateImageTypeId are passed >>>>>>>>> through the json file, where each of the UpdateImageTypeId has a >>>>>>>>> different value per payload. >>>>>>>>> >>>>>>>>>> >>>>>>>>>>>> 2) Each firmware object handled by a given FMP driver can further be >>>>>>>>>>>> identified by ImageIndex. >>>>>>>>>>>> >>>>>>>>>>>> My implementation of efi_fmp_find() does (1) and Raw FMP driver does >>>>>>>>>>>> (2) in efi_firmware_raw_set_image() which takes "image_index" as >>>>>>>>>>>> a parameter. >>>>>>>>>>>> >>>>>>>>>>>> Using ImageTypeId as an identifier is simply wrong in my opinion and >>>>>>>>>>>> doesn't meet the UEFI specification. >>>>>>>>>>> >>>>>>>>>>> So, as per what you are stating, all payloads under a given >>>>>>>>>>> EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER should have the same >>>>>>>>>>> ImageTypeId, either EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or >>>>>>>>>>> EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID. Same applies for all images in >>>>>>>>>>> the EFI_FIRMWARE_IMAGE_DESCRIPTOR. Because, without one of the two >>>>>>>>>>> values, > the check in efi_fmp_find to compare the UpdateImageTypeId >>>>>>>>>>> with the ImageTypeId retrieved from the image descriptor would simply >>>>>>>>>>> fail. >>>>>>>>>> >>>>>>>>>> I don't follow your point. >>>>>>>>>> Please elaborate a bit more. >>>>>>>>> >>>>>>>>> The current implementation of GetImageInfo, passes either of >>>>>>>>> EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or >>>>>>>>> EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID for all the images of the image >>>>>>>>> descriptor array. So, in case the capsule is generated with a '--guid' >>>>>>>>> value which is different from these two values, the check in >>>>>>>>> efi_fmp_find on line 204 will fail. >>>>>>>> >>>>>>>> That is an expected behavior, isn't it? >>>>>>> >>>>>>> Yes it is. Do not contest that. >>>>>>> >>>>>>>> If you want to use a different FMP driver (with another GUID), >>>>>>>> you naturally need to add your own FMP driver. >>>>>>> >>>>>>> This is where I differ. We can use the same FMP protocol instance for >>>>>>> any type of ImageTypeId. I do not see why we need to define a >>>>>>> different FMP protocol instance for a GUID value other than what has >>>>>>> been defined for u-boot raw and u-boot FIT GUIDs. >>>>>> >>>>>> I do understand part of your concern a bit. >>>>>> I thought of using the same ImageType GUID, say IMAGE_TYPE_DFU_GUID, at first >>>>>> but we needed different GUIDs here simply because we need to determine >>>>>> the format of payload, FIT format or raw binary. >>>>>> >>>>>>> The platform can give us the image descriptor array, and with that, >>>>>>> the same FMP instance can be used for any type of image(ImageTypeId). >>>>>> >>>>>> "any type of image"? Really? >>>>> >>>>> The raw FMP instance can certainly handle any type of binary payloads >>>>> right. There is no restriction on what type of payload it is as long >>>>> as it is all going as a single entity to a given dfu partition. >>>>> >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>>> This means that unless the --guid >>>>>>>>> value passed to the capsule generation is either of u-boot FIT or >>>>>>>>> u-boot raw, the current FMP protocol for raw devices cannot be used. >>>>>>>>> Why do we need that restriction. It should be possible to use the raw >>>>>>>>> FMP protocol for any other type of image types as well. >>>>>>>>> >>>>>>>>> >>>>>>>>>> >>>>>>>>>>> I think this interpretation of the UEFI spec is incorrect, since the >>>>>>>>>>> spec states that the ImageTypeId and the UpdateImageTypeId are fields >>>>>>>>>>> used to identify the firmware component targeted for the update. If >>>>>>>>>>> all values in the image descriptor array and the UpdateImageTypeId are >>>>>>>>>>> the same, why have this field in the first place for individual >>>>>>>>>>> images. >>>>>>>>>> >>>>>>>>>> As I said, ImageIndex is for that purpose. >>>>>>>>> >>>>>>>>> Yes, that is one possible way in the scenario where the ImageIndex is >>>>>>>>> determined at the capsule generation time. But, for the A/B update >>>>>>>>> scenario, we do not know the ImageIndex at build time >>>>>>>> >>>>>>>> "Build time" of what? >>>>>>> >>>>>>> Of the capsule. >>>>>>> >>>>>>>> I think that users should know how "dfu_alt_info" is defined >>>>>>>> (in other words, where the firmware be located on the target system) >>>>>>>> when capsule files are created. >>>>>>> >>>>>>> That is true for a non A/B scenario. And that is how it works in the >>>>>>> non A/B updates case. But for A/B updates, since the determination of >>>>>>> the "location" where the firmware image has to be written will be done >>>>>>> only at runtime, we cannot use the --index to differentiate. >>>>>> >>>>>> Yes, we can :) >>>>> >>>>> You know what I mean -- if we could use the same logic, I would not >>>>> have added all that code :) >>>>> >>>>>> >>>>>> First of all, my essential assumption in either FIT or RAW FMP driver >>>>>> is that U-Boot has (somehow conceptually) single firmware blob represented >>>>>> by DFU or dfu_alt_info. As I said, each object or location in >>>>>> dfu_alt_info can be further identified by index or "UpdateImageIndex". >>>>>> >>>>>> Let's assume that we have two locations of firmware, fw1 and fw2, and >>>>>> that we have two bank A and B. >>>>>> Then we will define dfu_alt_info as follows: >>>>>> <loc of fw1 for A>;<loc of fw2 for A>;<loc of fw1 for B>;<loc of fw2 for B>; >>>>>> |<--- 1st set --->|<--- 2nd set --->| >>>>>> >>>>>> When you want to update bank A, we can use the first set of dfu_alt_info, >>>>>> and use the second set of dfu_alt_info for bank B. >>>>>> At runtime, you should know which bank you're working on, and therefore >>>>>> you should know the exact physical location from dfu_alt_info. >>>>>> >>>>>> Please note that you don't have to change the syntax of dfu_alt_info >>>>>> at all. Simply offset the location with 0 for bank A and with 2 for bank B. >>>> >>>> I'll try digging a bit more, but I think the current approach is not >>>> working as it was intended wrt to the EFI spec. My reading of the spec >>>> and specifically section 23.3.2 is that a Capsule consists of an >>>> EFI capsule header and a payload. The payload now has an >>>> EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER which in it's turn can host multiple >>>> firmware images of different type described in EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER. >>>> >>>> An FMP implementation should read the UpdateImageTypeId's used to identify >>>> the image you are updating and from that derive the UpdateImageIndex >>>> which SetImage will use. That would give you the ability to update the >>>> all the firmware components with a single capsule. >>>> >>>> Sughosh what about the ESRT table generation? If you use different UpdateImageTypeId >>>> those should be reflected on the ESRT tables from the OS >>> >>> That would depend on the values populated in the >>> EFI_FIRMWARE_IMAGE_DESCRIPTOR array by the GetImageInfo function. The >>> image descriptor structure has an ImageTypeId field. The value of >>> ImageTypeId is what will be reflected in the ESRT table. >>> >>> In the current implementation, all the images in the ESRT table will >>> show the same ImageTypeId value, either >>> EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or >>> EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID. This is wrong. According to the the UEFI 2.9 specification the UpdateImageTypeId is used to "identify (the) device firmware targeted by this update". It does not identify the format in which the firmware is delivered. So this needs to be fixed in the next revision of this patch series. For each firmware part that can be updated provide a unique GUID. Best regards Heinrich >> >> Yea I was mostly asking wrt to A/B updates. Would the correct UUID be >> shown in the ESRT instead of EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID? > > Yes, the platform would have to define the fill_image_type_guid_array > function which would populate the ImageTypeId values in the > EFI_FIRMWARE_IMAGE_DESCRIPTOR array, and the image GUID values would > then show up as part of the ESRT table. > > As part of this patchset, I have added this function for the STM32MP1 DK2 board. > > -sughosh > >> >>> >>> The UpdateImageTypeId value from the capsule is used to compare with >>> the ImageTypeId values returned by the GetImageInfo function to check >>> if the given FMP protocol can be used for the update. >>> >>> -sughosh >>> >> [...] >> >> >> Regards >> /Ilias
hello Heinrich, On Sat, 26 Feb 2022 at 12:24, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 2/17/22 11:10, Sughosh Ganu wrote: > > On Thu, 17 Feb 2022 at 13:52, Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > >> > >>>>>>> > >> > >> [...] > >> > >>>>>>> Yes, we can use --index when we know the index value corresponding to > >>>>>>> the firmware image that we need to update. But like I mentioned in my > >>>>>>> earlier reply, for A/B updates, we do not know what the index value is > >>>>>>> going to be. That is going to be determined at runtime. > >>>>>> > >>>>>> I don't think so. See below for alternative approach. > >>>>>> > >>>>>>> Also, the point I was making is that we can have a capsule which is > >>>>>>> consumed by an FMP protocol which has more than one image, and those > >>>>>>> images have different ImageTypeId/UpdateImageTypeId. > >>>>>> > >>>>>> Yes, but it is a design choice in my first implementation. > >>>>>> I didn't think that we need to "have a capsule which is consumed > >>>>>> by an FMP protocol which has more than one image" as long as we > >>>>>> use DFU framework (and FIT as standard format of aggregation on U-Boot). > >>>>> > >>>>> But this design can be extended without any hassle, and more > >>>>> importantly without any regression, no? What kind of a problem does it > >>>>> create if the FMP can handle more than one image type. > >>>>> > >>>>> Even as per the UEFI spec, we have the EFI_FIRMWARE_MANAGEMENT_CAPSULE > >>>>> header for all images to be managed by the FMP protocol which has > >>>>> multiple images with different UpdateImageTypeId. > >>>>> > >>>>>> > >>>>>>>> > >>>>>>>>> Please check the > >>>>>>>>> GenerateCapsule script in EDK2. In case of a multi payload based > >>>>>>>>> capsule, individual parameters like the UpdateImageTypeId are passed > >>>>>>>>> through the json file, where each of the UpdateImageTypeId has a > >>>>>>>>> different value per payload. > >>>>>>>>> > >>>>>>>>>> > >>>>>>>>>>>> 2) Each firmware object handled by a given FMP driver can further be > >>>>>>>>>>>> identified by ImageIndex. > >>>>>>>>>>>> > >>>>>>>>>>>> My implementation of efi_fmp_find() does (1) and Raw FMP driver does > >>>>>>>>>>>> (2) in efi_firmware_raw_set_image() which takes "image_index" as > >>>>>>>>>>>> a parameter. > >>>>>>>>>>>> > >>>>>>>>>>>> Using ImageTypeId as an identifier is simply wrong in my opinion and > >>>>>>>>>>>> doesn't meet the UEFI specification. > >>>>>>>>>>> > >>>>>>>>>>> So, as per what you are stating, all payloads under a given > >>>>>>>>>>> EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER should have the same > >>>>>>>>>>> ImageTypeId, either EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > >>>>>>>>>>> EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID. Same applies for all images in > >>>>>>>>>>> the EFI_FIRMWARE_IMAGE_DESCRIPTOR. Because, without one of the two > >>>>>>>>>>> values, > the check in efi_fmp_find to compare the UpdateImageTypeId > >>>>>>>>>>> with the ImageTypeId retrieved from the image descriptor would simply > >>>>>>>>>>> fail. > >>>>>>>>>> > >>>>>>>>>> I don't follow your point. > >>>>>>>>>> Please elaborate a bit more. > >>>>>>>>> > >>>>>>>>> The current implementation of GetImageInfo, passes either of > >>>>>>>>> EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > >>>>>>>>> EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID for all the images of the image > >>>>>>>>> descriptor array. So, in case the capsule is generated with a '--guid' > >>>>>>>>> value which is different from these two values, the check in > >>>>>>>>> efi_fmp_find on line 204 will fail. > >>>>>>>> > >>>>>>>> That is an expected behavior, isn't it? > >>>>>>> > >>>>>>> Yes it is. Do not contest that. > >>>>>>> > >>>>>>>> If you want to use a different FMP driver (with another GUID), > >>>>>>>> you naturally need to add your own FMP driver. > >>>>>>> > >>>>>>> This is where I differ. We can use the same FMP protocol instance for > >>>>>>> any type of ImageTypeId. I do not see why we need to define a > >>>>>>> different FMP protocol instance for a GUID value other than what has > >>>>>>> been defined for u-boot raw and u-boot FIT GUIDs. > >>>>>> > >>>>>> I do understand part of your concern a bit. > >>>>>> I thought of using the same ImageType GUID, say IMAGE_TYPE_DFU_GUID, at first > >>>>>> but we needed different GUIDs here simply because we need to determine > >>>>>> the format of payload, FIT format or raw binary. > >>>>>> > >>>>>>> The platform can give us the image descriptor array, and with that, > >>>>>>> the same FMP instance can be used for any type of image(ImageTypeId). > >>>>>> > >>>>>> "any type of image"? Really? > >>>>> > >>>>> The raw FMP instance can certainly handle any type of binary payloads > >>>>> right. There is no restriction on what type of payload it is as long > >>>>> as it is all going as a single entity to a given dfu partition. > >>>>> > >>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>>>> This means that unless the --guid > >>>>>>>>> value passed to the capsule generation is either of u-boot FIT or > >>>>>>>>> u-boot raw, the current FMP protocol for raw devices cannot be used. > >>>>>>>>> Why do we need that restriction. It should be possible to use the raw > >>>>>>>>> FMP protocol for any other type of image types as well. > >>>>>>>>> > >>>>>>>>> > >>>>>>>>>> > >>>>>>>>>>> I think this interpretation of the UEFI spec is incorrect, since the > >>>>>>>>>>> spec states that the ImageTypeId and the UpdateImageTypeId are fields > >>>>>>>>>>> used to identify the firmware component targeted for the update. If > >>>>>>>>>>> all values in the image descriptor array and the UpdateImageTypeId are > >>>>>>>>>>> the same, why have this field in the first place for individual > >>>>>>>>>>> images. > >>>>>>>>>> > >>>>>>>>>> As I said, ImageIndex is for that purpose. > >>>>>>>>> > >>>>>>>>> Yes, that is one possible way in the scenario where the ImageIndex is > >>>>>>>>> determined at the capsule generation time. But, for the A/B update > >>>>>>>>> scenario, we do not know the ImageIndex at build time > >>>>>>>> > >>>>>>>> "Build time" of what? > >>>>>>> > >>>>>>> Of the capsule. > >>>>>>> > >>>>>>>> I think that users should know how "dfu_alt_info" is defined > >>>>>>>> (in other words, where the firmware be located on the target system) > >>>>>>>> when capsule files are created. > >>>>>>> > >>>>>>> That is true for a non A/B scenario. And that is how it works in the > >>>>>>> non A/B updates case. But for A/B updates, since the determination of > >>>>>>> the "location" where the firmware image has to be written will be done > >>>>>>> only at runtime, we cannot use the --index to differentiate. > >>>>>> > >>>>>> Yes, we can :) > >>>>> > >>>>> You know what I mean -- if we could use the same logic, I would not > >>>>> have added all that code :) > >>>>> > >>>>>> > >>>>>> First of all, my essential assumption in either FIT or RAW FMP driver > >>>>>> is that U-Boot has (somehow conceptually) single firmware blob represented > >>>>>> by DFU or dfu_alt_info. As I said, each object or location in > >>>>>> dfu_alt_info can be further identified by index or "UpdateImageIndex". > >>>>>> > >>>>>> Let's assume that we have two locations of firmware, fw1 and fw2, and > >>>>>> that we have two bank A and B. > >>>>>> Then we will define dfu_alt_info as follows: > >>>>>> <loc of fw1 for A>;<loc of fw2 for A>;<loc of fw1 for B>;<loc of fw2 for B>; > >>>>>> |<--- 1st set --->|<--- 2nd set --->| > >>>>>> > >>>>>> When you want to update bank A, we can use the first set of dfu_alt_info, > >>>>>> and use the second set of dfu_alt_info for bank B. > >>>>>> At runtime, you should know which bank you're working on, and therefore > >>>>>> you should know the exact physical location from dfu_alt_info. > >>>>>> > >>>>>> Please note that you don't have to change the syntax of dfu_alt_info > >>>>>> at all. Simply offset the location with 0 for bank A and with 2 for bank B. > >>>> > >>>> I'll try digging a bit more, but I think the current approach is not > >>>> working as it was intended wrt to the EFI spec. My reading of the spec > >>>> and specifically section 23.3.2 is that a Capsule consists of an > >>>> EFI capsule header and a payload. The payload now has an > >>>> EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER which in it's turn can host multiple > >>>> firmware images of different type described in EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER. > >>>> > >>>> An FMP implementation should read the UpdateImageTypeId's used to identify > >>>> the image you are updating and from that derive the UpdateImageIndex > >>>> which SetImage will use. That would give you the ability to update the > >>>> all the firmware components with a single capsule. > >>>> > >>>> Sughosh what about the ESRT table generation? If you use different UpdateImageTypeId > >>>> those should be reflected on the ESRT tables from the OS > >>> > >>> That would depend on the values populated in the > >>> EFI_FIRMWARE_IMAGE_DESCRIPTOR array by the GetImageInfo function. The > >>> image descriptor structure has an ImageTypeId field. The value of > >>> ImageTypeId is what will be reflected in the ESRT table. > >>> > >>> In the current implementation, all the images in the ESRT table will > >>> show the same ImageTypeId value, either > >>> EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > >>> EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID. > > This is wrong. According to the the UEFI 2.9 specification the > UpdateImageTypeId is used to "identify (the) device firmware targeted by > this update". It does not identify the format in which the firmware is > delivered. > > So this needs to be fixed in the next revision of this patch series. This patch series is actually adding that platform function which populates the image descriptor array with the image GUID's -- patch 6 of this series[1] actually does that for the ST DK2 platform. This discussion was because Takahiro wanted to use the same image GUID(u-boot raw/FIT) for all the images, and use the image index for identifying where the image is to be written. I guess with what you are stating, along with Ilias's opinion on this, I will send the next version with the same approach, i.e. using a platform function to populate the image GUIDs in the firmware image descriptor array. With this, each firmware image will have a different GUID which can be used to identify the image. -sughosh [1] - https://lists.denx.de/pipermail/u-boot/2022-February/474434.html > > For each firmware part that can be updated provide a unique GUID. > > Best regards > > Heinrich > > >> > >> Yea I was mostly asking wrt to A/B updates. Would the correct UUID be > >> shown in the ESRT instead of EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID? > > > > Yes, the platform would have to define the fill_image_type_guid_array > > function which would populate the ImageTypeId values in the > > EFI_FIRMWARE_IMAGE_DESCRIPTOR array, and the image GUID values would > > then show up as part of the ESRT table. > > > > As part of this patchset, I have added this function for the STM32MP1 DK2 board. > > > > -sughosh > > > >> > >>> > >>> The UpdateImageTypeId value from the capsule is used to compare with > >>> the ImageTypeId values returned by the GetImageInfo function to check > >>> if the given FMP protocol can be used for the update. > >>> > >>> -sughosh > >>> > >> [...] > >> > >> > >> Regards > >> /Ilias >
On Sat, Feb 26, 2022 at 03:24:10PM +0530, Sughosh Ganu wrote: > hello Heinrich, > > On Sat, 26 Feb 2022 at 12:24, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > On 2/17/22 11:10, Sughosh Ganu wrote: > > > On Thu, 17 Feb 2022 at 13:52, Ilias Apalodimas > > > <ilias.apalodimas@linaro.org> wrote: > > >> > > >>>>>>> > > >> > > >> [...] > > >> > > >>>>>>> Yes, we can use --index when we know the index value corresponding to > > >>>>>>> the firmware image that we need to update. But like I mentioned in my > > >>>>>>> earlier reply, for A/B updates, we do not know what the index value is > > >>>>>>> going to be. That is going to be determined at runtime. > > >>>>>> > > >>>>>> I don't think so. See below for alternative approach. > > >>>>>> > > >>>>>>> Also, the point I was making is that we can have a capsule which is > > >>>>>>> consumed by an FMP protocol which has more than one image, and those > > >>>>>>> images have different ImageTypeId/UpdateImageTypeId. > > >>>>>> > > >>>>>> Yes, but it is a design choice in my first implementation. > > >>>>>> I didn't think that we need to "have a capsule which is consumed > > >>>>>> by an FMP protocol which has more than one image" as long as we > > >>>>>> use DFU framework (and FIT as standard format of aggregation on U-Boot). > > >>>>> > > >>>>> But this design can be extended without any hassle, and more > > >>>>> importantly without any regression, no? What kind of a problem does it > > >>>>> create if the FMP can handle more than one image type. > > >>>>> > > >>>>> Even as per the UEFI spec, we have the EFI_FIRMWARE_MANAGEMENT_CAPSULE > > >>>>> header for all images to be managed by the FMP protocol which has > > >>>>> multiple images with different UpdateImageTypeId. > > >>>>> > > >>>>>> > > >>>>>>>> > > >>>>>>>>> Please check the > > >>>>>>>>> GenerateCapsule script in EDK2. In case of a multi payload based > > >>>>>>>>> capsule, individual parameters like the UpdateImageTypeId are passed > > >>>>>>>>> through the json file, where each of the UpdateImageTypeId has a > > >>>>>>>>> different value per payload. > > >>>>>>>>> > > >>>>>>>>>> > > >>>>>>>>>>>> 2) Each firmware object handled by a given FMP driver can further be > > >>>>>>>>>>>> identified by ImageIndex. > > >>>>>>>>>>>> > > >>>>>>>>>>>> My implementation of efi_fmp_find() does (1) and Raw FMP driver does > > >>>>>>>>>>>> (2) in efi_firmware_raw_set_image() which takes "image_index" as > > >>>>>>>>>>>> a parameter. > > >>>>>>>>>>>> > > >>>>>>>>>>>> Using ImageTypeId as an identifier is simply wrong in my opinion and > > >>>>>>>>>>>> doesn't meet the UEFI specification. > > >>>>>>>>>>> > > >>>>>>>>>>> So, as per what you are stating, all payloads under a given > > >>>>>>>>>>> EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER should have the same > > >>>>>>>>>>> ImageTypeId, either EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > >>>>>>>>>>> EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID. Same applies for all images in > > >>>>>>>>>>> the EFI_FIRMWARE_IMAGE_DESCRIPTOR. Because, without one of the two > > >>>>>>>>>>> values, > the check in efi_fmp_find to compare the UpdateImageTypeId > > >>>>>>>>>>> with the ImageTypeId retrieved from the image descriptor would simply > > >>>>>>>>>>> fail. > > >>>>>>>>>> > > >>>>>>>>>> I don't follow your point. > > >>>>>>>>>> Please elaborate a bit more. > > >>>>>>>>> > > >>>>>>>>> The current implementation of GetImageInfo, passes either of > > >>>>>>>>> EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > >>>>>>>>> EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID for all the images of the image > > >>>>>>>>> descriptor array. So, in case the capsule is generated with a '--guid' > > >>>>>>>>> value which is different from these two values, the check in > > >>>>>>>>> efi_fmp_find on line 204 will fail. > > >>>>>>>> > > >>>>>>>> That is an expected behavior, isn't it? > > >>>>>>> > > >>>>>>> Yes it is. Do not contest that. > > >>>>>>> > > >>>>>>>> If you want to use a different FMP driver (with another GUID), > > >>>>>>>> you naturally need to add your own FMP driver. > > >>>>>>> > > >>>>>>> This is where I differ. We can use the same FMP protocol instance for > > >>>>>>> any type of ImageTypeId. I do not see why we need to define a > > >>>>>>> different FMP protocol instance for a GUID value other than what has > > >>>>>>> been defined for u-boot raw and u-boot FIT GUIDs. > > >>>>>> > > >>>>>> I do understand part of your concern a bit. > > >>>>>> I thought of using the same ImageType GUID, say IMAGE_TYPE_DFU_GUID, at first > > >>>>>> but we needed different GUIDs here simply because we need to determine > > >>>>>> the format of payload, FIT format or raw binary. > > >>>>>> > > >>>>>>> The platform can give us the image descriptor array, and with that, > > >>>>>>> the same FMP instance can be used for any type of image(ImageTypeId). > > >>>>>> > > >>>>>> "any type of image"? Really? > > >>>>> > > >>>>> The raw FMP instance can certainly handle any type of binary payloads > > >>>>> right. There is no restriction on what type of payload it is as long > > >>>>> as it is all going as a single entity to a given dfu partition. > > >>>>> > > >>>>>>> > > >>>>>>>> > > >>>>>>>> > > >>>>>>>>> This means that unless the --guid > > >>>>>>>>> value passed to the capsule generation is either of u-boot FIT or > > >>>>>>>>> u-boot raw, the current FMP protocol for raw devices cannot be used. > > >>>>>>>>> Why do we need that restriction. It should be possible to use the raw > > >>>>>>>>> FMP protocol for any other type of image types as well. > > >>>>>>>>> > > >>>>>>>>> > > >>>>>>>>>> > > >>>>>>>>>>> I think this interpretation of the UEFI spec is incorrect, since the > > >>>>>>>>>>> spec states that the ImageTypeId and the UpdateImageTypeId are fields > > >>>>>>>>>>> used to identify the firmware component targeted for the update. If > > >>>>>>>>>>> all values in the image descriptor array and the UpdateImageTypeId are > > >>>>>>>>>>> the same, why have this field in the first place for individual > > >>>>>>>>>>> images. > > >>>>>>>>>> > > >>>>>>>>>> As I said, ImageIndex is for that purpose. > > >>>>>>>>> > > >>>>>>>>> Yes, that is one possible way in the scenario where the ImageIndex is > > >>>>>>>>> determined at the capsule generation time. But, for the A/B update > > >>>>>>>>> scenario, we do not know the ImageIndex at build time > > >>>>>>>> > > >>>>>>>> "Build time" of what? > > >>>>>>> > > >>>>>>> Of the capsule. > > >>>>>>> > > >>>>>>>> I think that users should know how "dfu_alt_info" is defined > > >>>>>>>> (in other words, where the firmware be located on the target system) > > >>>>>>>> when capsule files are created. > > >>>>>>> > > >>>>>>> That is true for a non A/B scenario. And that is how it works in the > > >>>>>>> non A/B updates case. But for A/B updates, since the determination of > > >>>>>>> the "location" where the firmware image has to be written will be done > > >>>>>>> only at runtime, we cannot use the --index to differentiate. > > >>>>>> > > >>>>>> Yes, we can :) > > >>>>> > > >>>>> You know what I mean -- if we could use the same logic, I would not > > >>>>> have added all that code :) > > >>>>> > > >>>>>> > > >>>>>> First of all, my essential assumption in either FIT or RAW FMP driver > > >>>>>> is that U-Boot has (somehow conceptually) single firmware blob represented > > >>>>>> by DFU or dfu_alt_info. As I said, each object or location in > > >>>>>> dfu_alt_info can be further identified by index or "UpdateImageIndex". > > >>>>>> > > >>>>>> Let's assume that we have two locations of firmware, fw1 and fw2, and > > >>>>>> that we have two bank A and B. > > >>>>>> Then we will define dfu_alt_info as follows: > > >>>>>> <loc of fw1 for A>;<loc of fw2 for A>;<loc of fw1 for B>;<loc of fw2 for B>; > > >>>>>> |<--- 1st set --->|<--- 2nd set --->| > > >>>>>> > > >>>>>> When you want to update bank A, we can use the first set of dfu_alt_info, > > >>>>>> and use the second set of dfu_alt_info for bank B. > > >>>>>> At runtime, you should know which bank you're working on, and therefore > > >>>>>> you should know the exact physical location from dfu_alt_info. > > >>>>>> > > >>>>>> Please note that you don't have to change the syntax of dfu_alt_info > > >>>>>> at all. Simply offset the location with 0 for bank A and with 2 for bank B. > > >>>> > > >>>> I'll try digging a bit more, but I think the current approach is not > > >>>> working as it was intended wrt to the EFI spec. My reading of the spec > > >>>> and specifically section 23.3.2 is that a Capsule consists of an > > >>>> EFI capsule header and a payload. The payload now has an > > >>>> EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER which in it's turn can host multiple > > >>>> firmware images of different type described in EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER. > > >>>> > > >>>> An FMP implementation should read the UpdateImageTypeId's used to identify > > >>>> the image you are updating and from that derive the UpdateImageIndex > > >>>> which SetImage will use. That would give you the ability to update the > > >>>> all the firmware components with a single capsule. > > >>>> > > >>>> Sughosh what about the ESRT table generation? If you use different UpdateImageTypeId > > >>>> those should be reflected on the ESRT tables from the OS > > >>> > > >>> That would depend on the values populated in the > > >>> EFI_FIRMWARE_IMAGE_DESCRIPTOR array by the GetImageInfo function. The > > >>> image descriptor structure has an ImageTypeId field. The value of > > >>> ImageTypeId is what will be reflected in the ESRT table. > > >>> > > >>> In the current implementation, all the images in the ESRT table will > > >>> show the same ImageTypeId value, either > > >>> EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > >>> EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID. > > > > This is wrong. According to the the UEFI 2.9 specification the > > UpdateImageTypeId is used to "identify (the) device firmware targeted by > > this update". It does not identify the format in which the firmware is > > delivered. > > > > So this needs to be fixed in the next revision of this patch series. > > This patch series is actually adding that platform function which > populates the image descriptor array with the image GUID's -- patch 6 > of this series[1] actually does that for the ST DK2 platform. This > discussion was because Takahiro wanted to use the same image > GUID(u-boot raw/FIT) for all the images, and use the image index for > identifying where the image is to be written. The discussion depends on what the *firmware* means. With my FMP drivers (either FIT or raw), I intended a *set of firmware* managed with a *single* "dfu_alt_info", which may include various *images* for different target *components* as the original DFU framework does. -Takahiro Akashi > I guess with what you are stating, along with Ilias's opinion on this, > I will send the next version with the same approach, i.e. using a > platform function to populate the image GUIDs in the firmware image > descriptor array. With this, each firmware image will have a different > GUID which can be used to identify the image. > > -sughosh > > [1] - https://lists.denx.de/pipermail/u-boot/2022-February/474434.html > > > > > For each firmware part that can be updated provide a unique GUID. > > > > Best regards > > > > Heinrich > > > > >> > > >> Yea I was mostly asking wrt to A/B updates. Would the correct UUID be > > >> shown in the ESRT instead of EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID? > > > > > > Yes, the platform would have to define the fill_image_type_guid_array > > > function which would populate the ImageTypeId values in the > > > EFI_FIRMWARE_IMAGE_DESCRIPTOR array, and the image GUID values would > > > then show up as part of the ESRT table. > > > > > > As part of this patchset, I have added this function for the STM32MP1 DK2 board. > > > > > > -sughosh > > > > > >> > > >>> > > >>> The UpdateImageTypeId value from the capsule is used to compare with > > >>> the ImageTypeId values returned by the GetImageInfo function to check > > >>> if the given FMP protocol can be used for the update. > > >>> > > >>> -sughosh > > >>> > > >> [...] > > >> > > >> > > >> Regards > > >> /Ilias > >
Akashi-san [...] > > > >>>>>>>> I think that users should know how "dfu_alt_info" is defined > > > >>>>>>>> (in other words, where the firmware be located on the target system) > > > >>>>>>>> when capsule files are created. > > > >>>>>>> > > > >>>>>>> That is true for a non A/B scenario. And that is how it works in the > > > >>>>>>> non A/B updates case. But for A/B updates, since the determination of > > > >>>>>>> the "location" where the firmware image has to be written will be done > > > >>>>>>> only at runtime, we cannot use the --index to differentiate. > > > >>>>>> > > > >>>>>> Yes, we can :) > > > >>>>> > > > >>>>> You know what I mean -- if we could use the same logic, I would not > > > >>>>> have added all that code :) > > > >>>>> > > > >>>>>> > > > >>>>>> First of all, my essential assumption in either FIT or RAW FMP driver > > > >>>>>> is that U-Boot has (somehow conceptually) single firmware blob represented > > > >>>>>> by DFU or dfu_alt_info. As I said, each object or location in > > > >>>>>> dfu_alt_info can be further identified by index or "UpdateImageIndex". > > > >>>>>> > > > >>>>>> Let's assume that we have two locations of firmware, fw1 and fw2, and > > > >>>>>> that we have two bank A and B. > > > >>>>>> Then we will define dfu_alt_info as follows: > > > >>>>>> <loc of fw1 for A>;<loc of fw2 for A>;<loc of fw1 for B>;<loc of fw2 for B>; > > > >>>>>> |<--- 1st set --->|<--- 2nd set --->| > > > >>>>>> > > > >>>>>> When you want to update bank A, we can use the first set of dfu_alt_info, > > > >>>>>> and use the second set of dfu_alt_info for bank B. > > > >>>>>> At runtime, you should know which bank you're working on, and therefore > > > >>>>>> you should know the exact physical location from dfu_alt_info. > > > >>>>>> > > > >>>>>> Please note that you don't have to change the syntax of dfu_alt_info > > > >>>>>> at all. Simply offset the location with 0 for bank A and with 2 for bank B. > > > >>>> > > > >>>> I'll try digging a bit more, but I think the current approach is not > > > >>>> working as it was intended wrt to the EFI spec. My reading of the spec > > > >>>> and specifically section 23.3.2 is that a Capsule consists of an > > > >>>> EFI capsule header and a payload. The payload now has an > > > >>>> EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER which in it's turn can host multiple > > > >>>> firmware images of different type described in EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER. > > > >>>> > > > >>>> An FMP implementation should read the UpdateImageTypeId's used to identify > > > >>>> the image you are updating and from that derive the UpdateImageIndex > > > >>>> which SetImage will use. That would give you the ability to update the > > > >>>> all the firmware components with a single capsule. > > > >>>> > > > >>>> Sughosh what about the ESRT table generation? If you use different UpdateImageTypeId > > > >>>> those should be reflected on the ESRT tables from the OS > > > >>> > > > >>> That would depend on the values populated in the > > > >>> EFI_FIRMWARE_IMAGE_DESCRIPTOR array by the GetImageInfo function. The > > > >>> image descriptor structure has an ImageTypeId field. The value of > > > >>> ImageTypeId is what will be reflected in the ESRT table. > > > >>> > > > >>> In the current implementation, all the images in the ESRT table will > > > >>> show the same ImageTypeId value, either > > > >>> EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or > > > >>> EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID. > > > > > > This is wrong. According to the the UEFI 2.9 specification the > > > UpdateImageTypeId is used to "identify (the) device firmware targeted by > > > this update". It does not identify the format in which the firmware is > > > delivered. > > > > > > So this needs to be fixed in the next revision of this patch series. > > > > This patch series is actually adding that platform function which > > populates the image descriptor array with the image GUID's -- patch 6 > > of this series[1] actually does that for the ST DK2 platform. This > > discussion was because Takahiro wanted to use the same image > > GUID(u-boot raw/FIT) for all the images, and use the image index for > > identifying where the image is to be written. > > The discussion depends on what the *firmware* means. > With my FMP drivers (either FIT or raw), I intended a *set of firmware* > managed with a *single* "dfu_alt_info", which may include various *images* > for different target *components* as the original DFU framework does. I still think we should fix that. The current code is working, but it's not what the EFI spec describes. I think we should do it the other way around. IOW the board defines the UUID's for the firmware partitions it needs to update. The FMP driver should then populate those properly in an ESRT table and also generate an appropriate dfu_alt_info (or similar) on the fly. Regards /Ilias > > -Takahiro Akashi > > > > I guess with what you are stating, along with Ilias's opinion on this, > > I will send the next version with the same approach, i.e. using a > > platform function to populate the image GUIDs in the firmware image > > descriptor array. With this, each firmware image will have a different > > GUID which can be used to identify the image. > > > > -sughosh > > > > [1] - https://lists.denx.de/pipermail/u-boot/2022-February/474434.html > > > > > > > > For each firmware part that can be updated provide a unique GUID. > > > > > > Best regards > > > > > > Heinrich > > > > > > >> > > > >> Yea I was mostly asking wrt to A/B updates. Would the correct UUID be > > > >> shown in the ESRT instead of EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID? > > > > > > > > Yes, the platform would have to define the fill_image_type_guid_array > > > > function which would populate the ImageTypeId values in the > > > > EFI_FIRMWARE_IMAGE_DESCRIPTOR array, and the image GUID values would > > > > then show up as part of the ESRT table. > > > > > > > > As part of this patchset, I have added this function for the STM32MP1 DK2 board. > > > > > > > > -sughosh > > > > > > > >> > > > >>> > > > >>> The UpdateImageTypeId value from the capsule is used to compare with > > > >>> the ImageTypeId values returned by the GetImageInfo function to check > > > >>> if the given FMP protocol can be used for the update. > > > >>> > > > >>> -sughosh > > > >>> > > > >> [...] > > > >> > > > >> > > > >> Regards > > > >> /Ilias > > >
diff --git a/include/efi_loader.h b/include/efi_loader.h index f4860e87fc..ae60de0be5 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -992,4 +992,6 @@ efi_status_t efi_esrt_populate(void); efi_status_t efi_load_capsule_drivers(void); efi_status_t platform_get_eventlog(struct udevice *dev, u64 *addr, u32 *sz); +efi_status_t fill_image_type_guid_array(const efi_guid_t *default_guid, + efi_guid_t **part_guid_arr); #endif /* _EFI_LOADER_H */ diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c index a1b88dbfc2..5642be9f9a 100644 --- a/lib/efi_loader/efi_firmware.c +++ b/lib/efi_loader/efi_firmware.c @@ -96,6 +96,46 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported( return EFI_EXIT(EFI_UNSUPPORTED); } +efi_status_t __weak fill_image_type_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, &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 +144,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 * * Return information bout the current firmware image in @image_info. * @image_info will consist of a number of descriptors. @@ -122,7 +162,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 +212,7 @@ static efi_status_t efi_get_dfu_info( next = name; list_for_each_entry(dfu, &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 +290,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 +305,19 @@ efi_status_t EFIAPI efi_firmware_fit_get_image_info( !descriptor_size || !package_version || !package_version_name)) return EFI_EXIT(EFI_INVALID_PARAMETER); + ret = fill_image_type_guid_array(&efi_firmware_image_type_uboot_fit, + &part_guid_arr); + if (ret != EFI_SUCCESS) + goto out; + ret = efi_get_dfu_info(image_info_size, image_info, descriptor_version, descriptor_count, descriptor_size, package_version, package_version_name, - &efi_firmware_image_type_uboot_fit); + part_guid_arr); +out: + free(part_guid_arr); return EFI_EXIT(ret); } @@ -359,6 +407,7 @@ efi_status_t EFIAPI efi_firmware_raw_get_image_info( u16 **package_version_name) { efi_status_t ret = EFI_SUCCESS; + efi_guid_t *part_guid_arr = NULL; EFI_ENTRY("%p %p %p %p %p %p %p %p\n", this, image_info_size, image_info, @@ -373,12 +422,20 @@ efi_status_t EFIAPI efi_firmware_raw_get_image_info( !descriptor_size || !package_version || !package_version_name)) return EFI_EXIT(EFI_INVALID_PARAMETER); + ret = fill_image_type_guid_array( + &efi_firmware_image_type_uboot_raw, + &part_guid_arr); + if (ret != EFI_SUCCESS) + goto out; + ret = efi_get_dfu_info(image_info_size, image_info, descriptor_version, descriptor_count, descriptor_size, package_version, package_version_name, - &efi_firmware_image_type_uboot_raw); + part_guid_arr); +out: + free(part_guid_arr); return EFI_EXIT(ret); }
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 <sughosh.ganu@linaro.org> --- Changes since V3: * Define a weak function fill_image_type_guid_array for populating the image descriptor array with u-boot's raw and fit image GUIDs include/efi_loader.h | 2 + lib/efi_loader/efi_firmware.c | 71 +++++++++++++++++++++++++++++++---- 2 files changed, 66 insertions(+), 7 deletions(-)