Sughosh, On Tue, Nov 24, 2020 at 01:07:53PM +0530, Sughosh Ganu wrote: > Takahiro, > > On Tue, 24 Nov 2020 at 11:21, AKASHI Takahiro <takahiro.aka...@linaro.org> > wrote: > > > Sughosh, > > > > On Sat, Nov 21, 2020 at 11:32:43PM +0530, Sughosh Ganu wrote: > > > hi Takahiro, > > > > > > On Tue, 17 Nov 2020 at 05:58, AKASHI Takahiro < > > takahiro.aka...@linaro.org> > > > wrote: > > > > > > > A capsule tagged with the guid, > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID, > > > > is handled as a firmware update object. > > > > What efi_update_capsule() basically does is to load any firmware > > management > > > > protocol (or fmp) drivers contained in a capsule, find out an > > appropriate > > > > fmp driver and then invoke its set_image() interface against each > > binary > > > > in a capsule. > > > > In this commit, however, loading drivers is not supported. > > > > > > > > The result of applying a capsule is set to be stored in "CapsuleXXXX" > > > > variable, but its implementation is deferred to a fmp driver. > > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.aka...@linaro.org> > > > > --- > > > > include/efi_api.h | 129 +++++++++++++++++++ > > > > include/efi_loader.h | 2 + > > > > lib/efi_loader/Kconfig | 8 ++ > > > > lib/efi_loader/efi_capsule.c | 238 ++++++++++++++++++++++++++++++++++- > > > > lib/efi_loader/efi_setup.c | 4 + > > > > 5 files changed, 380 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/include/efi_api.h b/include/efi_api.h > > > > index 7a2a087c60ed..966bc6e590bf 100644 > > > > --- a/include/efi_api.h > > > > +++ b/include/efi_api.h > > > > @@ -217,6 +217,9 @@ enum efi_reset_type { > > > > #define CAPSULE_FLAGS_POPULATE_SYSTEM_TABLE 0x00020000 > > > > #define CAPSULE_FLAGS_INITIATE_RESET 0x00040000 > > > > > > > > +#define CAPSULE_SUPPORT_AUTHENTICATION 0x0000000000000001 > > > > +#define CAPSULE_SUPPORT_DEPENDENCY 0x0000000000000002 > > > > + > > > > #define EFI_CAPSULE_REPORT_GUID \ > > > > EFI_GUID(0x39b68c46, 0xf7fb, 0x441b, 0xb6, 0xec, \ > > > > 0x16, 0xb0, 0xf6, 0x98, 0x21, 0xf3) > > > > @@ -225,6 +228,10 @@ enum efi_reset_type { > > > > EFI_GUID(0xde9f0ec, 0x88b6, 0x428f, 0x97, 0x7a, \ > > > > 0x25, 0x8f, 0x1d, 0xe, 0x5e, 0x72) > > > > > > > > +#define EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID \ > > > > + EFI_GUID(0x6dcbd5ed, 0xe82d, 0x4c44, 0xbd, 0xa1, \ > > > > + 0x71, 0x94, 0x19, 0x9a, 0xd9, 0x2a) > > > > + > > > > struct efi_capsule_header { > > > > efi_guid_t capsule_guid; > > > > u32 header_size; > > > > @@ -253,6 +260,33 @@ struct efi_memory_range_capsule { > > > > struct efi_memory_range memory_ranges[]; > > > > } __packed; > > > > > > > > +struct efi_firmware_management_capsule_header { > > > > + u32 version; > > > > + u16 embedded_driver_count; > > > > + u16 payload_item_count; > > > > + u64 item_offset_list[]; > > > > +} __packed; > > > > + > > > > +struct efi_firmware_management_capsule_image_header { > > > > + u32 version; > > > > + efi_guid_t update_image_type_id; > > > > + u8 update_image_index; > > > > + u8 reserved[3]; > > > > + u32 update_image_size; > > > > + u32 update_vendor_code_size; > > > > + u64 update_hardware_instance; > > > > + u64 image_capsule_support; > > > > +} __packed; > > > > + > > > > +struct efi_capsule_result_variable_fmp { > > > > + u16 version; > > > > + u8 payload_index; > > > > + u8 update_image_index; > > > > + efi_guid_t update_image_type_id; > > > > + // u16 capsule_file_name[]; > > > > + // u16 capsule_target[]; > > > > +} __packed; > > > > + > > > > > > > > > > <snip> > > > > > > > > > > +/** > > > > + * efi_capsule_update_firmware - update firmware from capsule > > > > + * @capsule_data: Capsule > > > > + * > > > > + * Update firmware, using a capsule, @capsule_data. Loading any FMP > > > > + * drivers embedded in a capsule is not supported. > > > > + * > > > > + * Return: status code > > > > + */ > > > > +static efi_status_t efi_capsule_update_firmware( > > > > + struct efi_capsule_header *capsule_data) > > > > +{ > > > > + struct efi_firmware_management_capsule_header *capsule; > > > > + struct efi_firmware_management_capsule_image_header *image; > > > > + size_t capsule_size; > > > > + void *image_binary, *vendor_code; > > > > + efi_handle_t *handles; > > > > + efi_uintn_t no_handles; > > > > + int item; > > > > + struct efi_firmware_management_protocol *fmp; > > > > + u16 *abort_reason; > > > > + efi_status_t ret = EFI_SUCCESS; > > > > + > > > > + /* sanity check */ > > > > + if (capsule_data->header_size < sizeof(*capsule) || > > > > + capsule_data->header_size >= > > capsule_data->capsule_image_size) > > > > + return EFI_INVALID_PARAMETER; > > > > + > > > > + capsule = (void *)capsule_data + capsule_data->header_size; > > > > + capsule_size = capsule_data->capsule_image_size > > > > + - capsule_data->header_size; > > > > + > > > > + if (capsule->version != 0x00000001) > > > > + return EFI_INVALID_PARAMETER; > > > > + > > > > + /* Drivers */ > > > > + /* TODO: support loading drivers */ > > > > + > > > > + handles = NULL; > > > > + ret = EFI_CALL(efi_locate_handle_buffer( > > > > + BY_PROTOCOL, > > > > + &efi_guid_firmware_management_protocol, > > > > + NULL, &no_handles, (efi_handle_t **)&handles)); > > > > + if (ret != EFI_SUCCESS) > > > > + return EFI_UNSUPPORTED; > > > > + > > > > + /* Payload */ > > > > + for (item = capsule->embedded_driver_count; > > > > + item < capsule->embedded_driver_count > > > > + + capsule->payload_item_count; item++) { > > > > + /* sanity check */ > > > > + if ((capsule->item_offset_list[item] + sizeof(*image) > > > > + >= capsule_size)) { > > > > + printf("EFI: A capsule has not enough size of > > > > data\n"); > > > > + ret = EFI_INVALID_PARAMETER; > > > > + goto out; > > > > + } > > > > + > > > > + image = (void *)capsule + > > capsule->item_offset_list[item]; > > > > + > > > > + if (image->version != 0x00000001 && > > > > + image->version != 0x00000002 && > > > > + image->version != 0x00000003) { > > > > + ret = EFI_INVALID_PARAMETER; > > > > + goto out; > > > > + } > > > > + > > > > + /* find a device for update firmware */ > > > > + /* TODO: should we pass index as well, or nothing but > > > > type? */ > > > > + fmp = efi_fmp_find(&image->update_image_type_id, > > > > + image->version == 0x1 ? 0 : > > > > + > > image->update_hardware_instance, > > > > + handles, no_handles); > > > > + if (!fmp) { > > > > + printf("EFI Capsule: driver not found for > > firmware > > > > type: %pUl, hardware instance: %lld\n", > > > > + &image->update_image_type_id, > > > > + image->version == 0x1 ? 0 : > > > > + > > image->update_hardware_instance); > > > > + ret = EFI_UNSUPPORTED; > > > > + goto out; > > > > + } > > > > + > > > > + /* do it */ > > > > + image_binary = (void *)image + sizeof(*image); > > > > > > > > > > Sorry for not having spotted this earlier. But since we are supporting > > the > > > first three versions of the struct > > > efi_firmware_management_capsule_image_header, the above logic needs to be > > > changed to accomodate versions 1 and 2. So in case of a version 1 > > > structure, the fields update_hardware_instance and image_capsule_support > > > need to be discarded. Similarly for a version 2 structure, the > > > image_capsule_support field needs to be discarded while calculating the > > > image_binary pointer. > > > > Thank you for catching this issue, you're right. > > > > Given, however, that the firmware update is quite firmware-specific > > and that we don't have any "backward compatibility" requirement, > > it would be better to simply drop the support for previous versions > > and to support the current/latest one, in this case, 3. > > > > The edk2 scripts which i use for generating the capsules are currently > using version 2.
Personally, I think edk2's script should be updated to catch up with the latest specification. > So if it's not too much of a hassle, i will prefer having > support for version 2 at least. However, if you don't want to respin the > series again, I can take care of this in my series for adding support for > capsule update on the qemu arm platform. I will have to respin the code to drop the current "half-baked" support for previous versions if Heinrich anticipates it. After that, it's up to you to implement additional support as a follow-up patch. -Takahiro Akashi > -sughosh > > > > We have a similar issue on 'struct efi_firmware_image_descriptor'. > > I hope that imposing a version check/restriction won't bring any > > inconvenience to anybody. > > > > -Takahiro Akashi > > > > > -sughosh > >