| Message ID | 20230410090732.1676-2-masahisa.kojima@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | FMP versioning support | expand |
On 4/10/23 11:07, Masahisa Kojima wrote: > Current FMP->GetImageInfo() always return 0 for the firmware > version, user can not identify which firmware version is currently > running through the EFI interface. > > This commit gets the version information from device tree, > then fills the firmware version, lowest supported version > in FMP->GetImageInfo(). > > Now FMP->GetImageInfo() and ESRT have the meaningful version number. > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > --- > Changes in v5: > - newly implement a device tree based versioning > > .../firmware/firmware-version.txt | 25 ++++++++ > lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++-- > 2 files changed, 84 insertions(+), 4 deletions(-) > create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt > > diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt > new file mode 100644 > index 0000000000..6112de4a1d > --- /dev/null > +++ b/doc/device-tree-bindings/firmware/firmware-version.txt > @@ -0,0 +1,25 @@ > +firmware-version bindings > +------------------------------- > + > +Required properties: > +- image-type-id : guid for image blob type > +- image-index : image index > +- fw-version : firmware version > +- lowest-supported-version : lowest supported version > + > +Example: > + > + firmware-version { > + image1 { > + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; > + image-index = <1>; > + fw-version = <5>; > + lowest-supported-version = <3>; > + }; > + image2 { > + image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0"; > + image-index = <2>; > + fw-version = <10>; > + lowest-supported-version = <7>; > + }; > + }; > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c > index 93e2b01c07..1c6ef468bf 100644 > --- a/lib/efi_loader/efi_firmware.c > +++ b/lib/efi_loader/efi_firmware.c > @@ -102,6 +102,56 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported( > return EFI_EXIT(EFI_UNSUPPORTED); > } > > +/** > + * efi_firmware_get_firmware_version - get firmware version from dtb > + * @image_index: Image index > + * @image_type_id: Image type id > + * @fw_version: Pointer to store the version number > + * @lsv: Pointer to store the lowest supported version > + * > + * Authenticate the capsule if authentication is enabled. > + * The image pointer and the image size are updated in case of success. > + */ > +void efi_firmware_get_firmware_version(u8 image_index, > + efi_guid_t *image_type_id, > + u32 *fw_version, u32 *lsv) > +{ > + const void *fdt = gd->fdt_blob; > + const fdt32_t *val; > + const char *guid_str; > + int len, offset, index; > + int parent; > + > + parent = fdt_subnode_offset(fdt, 0, "firmware-version"); > + if (parent < 0) > + return; > + > + fdt_for_each_subnode(offset, fdt, parent) { > + efi_guid_t guid; > + > + guid_str = fdt_getprop(fdt, offset, "image-type-id", &len); > + if (!guid_str) > + continue; > + uuid_str_to_bin(guid_str, guid.b, UUID_STR_FORMAT_GUID); > + > + val = fdt_getprop(fdt, offset, "image-index", &len); > + if (!val) > + continue; > + index = fdt32_to_cpu(*val); > + > + if (!guidcmp(&guid, image_type_id) && index == image_index) { > + val = fdt_getprop(fdt, offset, "fw-version", &len); > + if (val) > + *fw_version = fdt32_to_cpu(*val); > + > + val = fdt_getprop(fdt, offset, > + "lowest-supported-version", &len); > + if (val) > + *lsv = fdt32_to_cpu(*val); > + } > + } > +} > + > /** > * efi_fill_image_desc_array - populate image descriptor array > * @image_info_size: Size of @image_info > @@ -148,13 +198,19 @@ static efi_status_t efi_fill_image_desc_array( > *package_version_name = NULL; /* not supported */ > > for (i = 0; i < num_image_type_guids; i++) { Currently we define num_image_type_guids per board in a C file. Using the same line of code once per board makes no sense to me. Please, move the definition of that variable to lib/efi_loader/efi_firmware.c. > + u32 fw_version = 0; > + u32 lowest_supported_version = 0; These assignments should be in efi_firmware_get_firmware_version. > + > image_info[i].image_index = fw_array[i].image_index; Why did we ever introduce the field image_index? Please, eliminate it it as the GUID is always sufficient to identify an image. > image_info[i].image_type_id = fw_array[i].image_type_id; > image_info[i].image_id = fw_array[i].image_index; > > image_info[i].image_id_name = fw_array[i].fw_name; > - > - image_info[i].version = 0; /* not supported */ > + efi_firmware_get_firmware_version(fw_array[i].image_index, > + &fw_array[i].image_type_id, > + &fw_version, > + &lowest_supported_version); This interface makes no sense to me. We expect images with specific GUIDs and should not care about images with other GUIDs that may additionally exist in the capsule. So you must pass the expected GUID as input variable here. > + image_info[i].version = fw_version; > image_info[i].version_name = NULL; /* not supported */ Please, add the missing functionality to efi_firmware_get_firmware_version(). Please, pass *image_info[i] to efi_firmware_get_firmware_version. That will simplify the code. Best regards Heinrich > image_info[i].size = 0; > image_info[i].attributes_supported = > @@ -168,7 +224,7 @@ static efi_status_t efi_fill_image_desc_array( > image_info[0].attributes_setting |= > IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED; > > - image_info[i].lowest_supported_image_version = 0; > + image_info[i].lowest_supported_image_version = lowest_supported_version; > image_info[i].last_attempt_version = 0; > image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS; > image_info[i].hardware_instance = 1; > @@ -290,7 +346,6 @@ efi_status_t EFIAPI efi_firmware_get_image_info( > descriptor_version, descriptor_count, > descriptor_size, package_version, > package_version_name); > - > return EFI_EXIT(ret); > } >
Hi Masahisa, On Mon, 10 Apr 2023 at 03:07, Masahisa Kojima <masahisa.kojima@linaro.org> wrote: > > Current FMP->GetImageInfo() always return 0 for the firmware > version, user can not identify which firmware version is currently > running through the EFI interface. > > This commit gets the version information from device tree, > then fills the firmware version, lowest supported version > in FMP->GetImageInfo(). > > Now FMP->GetImageInfo() and ESRT have the meaningful version number. > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > --- > Changes in v5: > - newly implement a device tree based versioning > > .../firmware/firmware-version.txt | 25 ++++++++ > lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++-- > 2 files changed, 84 insertions(+), 4 deletions(-) > create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt > > diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt > new file mode 100644 > index 0000000000..6112de4a1d > --- /dev/null > +++ b/doc/device-tree-bindings/firmware/firmware-version.txt > @@ -0,0 +1,25 @@ > +firmware-version bindings > +------------------------------- > + > +Required properties: > +- image-type-id : guid for image blob type > +- image-index : image index > +- fw-version : firmware version > +- lowest-supported-version : lowest supported version > + > +Example: > + > + firmware-version { > + image1 { > + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; Nit: please use lower-case hex and add a decoder to uuid.c so we can look it up when debugging. > + image-index = <1>; > + fw-version = <5>; > + lowest-supported-version = <3>; > + }; > + image2 { > + image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0"; > + image-index = <2>; > + fw-version = <10>; > + lowest-supported-version = <7>; > + }; > + }; [..] Regards, Simon
Am 28. April 2023 01:35:04 MESZ schrieb Simon Glass <sjg@chromium.org>: >Hi Masahisa, > >On Mon, 10 Apr 2023 at 03:07, Masahisa Kojima ><masahisa.kojima@linaro.org> wrote: >> >> Current FMP->GetImageInfo() always return 0 for the firmware >> version, user can not identify which firmware version is currently >> running through the EFI interface. >> >> This commit gets the version information from device tree, >> then fills the firmware version, lowest supported version >> in FMP->GetImageInfo(). >> >> Now FMP->GetImageInfo() and ESRT have the meaningful version number. >> >> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> >> --- >> Changes in v5: >> - newly implement a device tree based versioning >> >> .../firmware/firmware-version.txt | 25 ++++++++ >> lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++-- >> 2 files changed, 84 insertions(+), 4 deletions(-) >> create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt >> >> diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt >> new file mode 100644 >> index 0000000000..6112de4a1d >> --- /dev/null >> +++ b/doc/device-tree-bindings/firmware/firmware-version.txt >> @@ -0,0 +1,25 @@ >> +firmware-version bindings >> +------------------------------- >> + >> +Required properties: >> +- image-type-id : guid for image blob type >> +- image-index : image index >> +- fw-version : firmware version >> +- lowest-supported-version : lowest supported version >> + >> +Example: >> + >> + firmware-version { >> + image1 { >> + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; > >Nit: please use lower-case hex and add a decoder to uuid.c so we can >look it up when debugging. The GUIDs are board specific. No, we should not clutter uuid.c with strings for dozens of boards. Our development aim is to keep U-Boot small and these GUIDs are not printed anywhere. Instead we should define a string per firmware image. We already have a field for it: fw_array[i].fw_name Best regards Heinrich > >> + image-index = <1>; >> + fw-version = <5>; >> + lowest-supported-version = <3>; >> + }; >> + image2 { >> + image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0"; >> + image-index = <2>; >> + fw-version = <10>; >> + lowest-supported-version = <7>; >> + }; >> + }; >[..] > >Regards, >Simon
Hi Heinrich, On Thu, 27 Apr 2023 at 15:09, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 4/10/23 11:07, Masahisa Kojima wrote: > > Current FMP->GetImageInfo() always return 0 for the firmware > > version, user can not identify which firmware version is currently > > running through the EFI interface. > > > > This commit gets the version information from device tree, > > then fills the firmware version, lowest supported version > > in FMP->GetImageInfo(). > > > > Now FMP->GetImageInfo() and ESRT have the meaningful version number. > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > > --- > > Changes in v5: > > - newly implement a device tree based versioning > > > > .../firmware/firmware-version.txt | 25 ++++++++ > > lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++-- > > 2 files changed, 84 insertions(+), 4 deletions(-) > > create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt > > > > diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt > > new file mode 100644 > > index 0000000000..6112de4a1d > > --- /dev/null > > +++ b/doc/device-tree-bindings/firmware/firmware-version.txt > > @@ -0,0 +1,25 @@ > > +firmware-version bindings > > +------------------------------- > > + > > +Required properties: > > +- image-type-id : guid for image blob type > > +- image-index : image index > > +- fw-version : firmware version > > +- lowest-supported-version : lowest supported version > > + > > +Example: > > + > > + firmware-version { > > + image1 { > > + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; > > + image-index = <1>; > > + fw-version = <5>; > > + lowest-supported-version = <3>; > > + }; > > + image2 { > > + image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0"; > > + image-index = <2>; > > + fw-version = <10>; > > + lowest-supported-version = <7>; > > + }; > > + }; > > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c > > index 93e2b01c07..1c6ef468bf 100644 > > --- a/lib/efi_loader/efi_firmware.c > > +++ b/lib/efi_loader/efi_firmware.c > > @@ -102,6 +102,56 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported( > > return EFI_EXIT(EFI_UNSUPPORTED); > > } > > > > +/** > > + * efi_firmware_get_firmware_version - get firmware version from dtb > > + * @image_index: Image index > > + * @image_type_id: Image type id > > + * @fw_version: Pointer to store the version number > > + * @lsv: Pointer to store the lowest supported version > > + * > > + * Authenticate the capsule if authentication is enabled. > > + * The image pointer and the image size are updated in case of success. > > + */ > > +void efi_firmware_get_firmware_version(u8 image_index, > > + efi_guid_t *image_type_id, > > + u32 *fw_version, u32 *lsv) > > +{ > > + const void *fdt = gd->fdt_blob; > > + const fdt32_t *val; > > + const char *guid_str; > > + int len, offset, index; > > + int parent; > > + > > + parent = fdt_subnode_offset(fdt, 0, "firmware-version"); > > + if (parent < 0) > > + return; > > + > > + fdt_for_each_subnode(offset, fdt, parent) { > > + efi_guid_t guid; > > + > > + guid_str = fdt_getprop(fdt, offset, "image-type-id", &len); > > + if (!guid_str) > > + continue; > > + uuid_str_to_bin(guid_str, guid.b, UUID_STR_FORMAT_GUID); > > + > > + val = fdt_getprop(fdt, offset, "image-index", &len); > > + if (!val) > > + continue; > > + index = fdt32_to_cpu(*val); > > + > > + if (!guidcmp(&guid, image_type_id) && index == image_index) { > > + val = fdt_getprop(fdt, offset, "fw-version", &len); > > + if (val) > > + *fw_version = fdt32_to_cpu(*val); > > + > > + val = fdt_getprop(fdt, offset, > > + "lowest-supported-version", &len); > > + if (val) > > + *lsv = fdt32_to_cpu(*val); > > + } > > + } > > +} > > + > > /** > > * efi_fill_image_desc_array - populate image descriptor array > > * @image_info_size: Size of @image_info > > @@ -148,13 +198,19 @@ static efi_status_t efi_fill_image_desc_array( > > *package_version_name = NULL; /* not supported */ > > > > for (i = 0; i < num_image_type_guids; i++) { > > Currently we define num_image_type_guids per board in a C file. Using > the same line of code once per board makes no sense to me. Please, move > the definition of that variable to lib/efi_loader/efi_firmware.c. Sorry for the late reply. num_image_type_guids is calculated with "ARRAY_SIZE(fw_images)", fw_images[] array is also defined in each board file, so we can not simply move num_image_type_guids into lib/efi_loader/efi_firmware.c. And fw_images[] array is abstracted by struct efi_capsule_update_info, so I think we should not extract the fw_images[] array. > > > + u32 fw_version = 0; > > + u32 lowest_supported_version = 0; > > These assignments should be in efi_firmware_get_firmware_version. OK. > > > + > > image_info[i].image_index = fw_array[i].image_index; > > Why did we ever introduce the field image_index? Please, eliminate it it > as the GUID is always sufficient to identify an image. This is derived from the UEFI specification. UEFI specification "23.1.2 EFI_FIRMWARE_MANAGEMENT_PROTOCOL.GetImageInfo()" requires ImageIndex and ImageTypeId(guid). ImageIndex: A unique number identifying the firmware image within the device. The number is between 1 and DescriptorCount. > > > image_info[i].image_type_id = fw_array[i].image_type_id; > > image_info[i].image_id = fw_array[i].image_index; > > > > image_info[i].image_id_name = fw_array[i].fw_name; > > - > > - image_info[i].version = 0; /* not supported */ > > + efi_firmware_get_firmware_version(fw_array[i].image_index, > > + &fw_array[i].image_type_id, > > + &fw_version, > > + &lowest_supported_version); > > This interface makes no sense to me. We expect images with specific > GUIDs and should not care about images with other GUIDs that may > additionally exist in the capsule. > > So you must pass the expected GUID as input variable here. I don't clearly understand this comment, but the expected GUID is fw_array[i].image_type_id. > > > + image_info[i].version = fw_version; > > image_info[i].version_name = NULL; /* not supported */ > > Please, add the missing functionality to > efi_firmware_get_firmware_version(). Does it mean we need to support version_name? I can add a version_name in dtb. > > Please, pass *image_info[i] to efi_firmware_get_firmware_version. That > will simplify the code. OK. Thank you for your review. Regards, Masahisa Kojima > > Best regards > > Heinrich > > > image_info[i].size = 0; > > image_info[i].attributes_supported = > > @@ -168,7 +224,7 @@ static efi_status_t efi_fill_image_desc_array( > > image_info[0].attributes_setting |= > > IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED; > > > > - image_info[i].lowest_supported_image_version = 0; > > + image_info[i].lowest_supported_image_version = lowest_supported_version; > > image_info[i].last_attempt_version = 0; > > image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS; > > image_info[i].hardware_instance = 1; > > @@ -290,7 +346,6 @@ efi_status_t EFIAPI efi_firmware_get_image_info( > > descriptor_version, descriptor_count, > > descriptor_size, package_version, > > package_version_name); > > - > > return EFI_EXIT(ret); > > } > > >
On 5/8/23 10:15, Masahisa Kojima wrote: > Hi Heinrich, > > On Thu, 27 Apr 2023 at 15:09, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> >> On 4/10/23 11:07, Masahisa Kojima wrote: >>> Current FMP->GetImageInfo() always return 0 for the firmware >>> version, user can not identify which firmware version is currently >>> running through the EFI interface. >>> >>> This commit gets the version information from device tree, >>> then fills the firmware version, lowest supported version >>> in FMP->GetImageInfo(). >>> >>> Now FMP->GetImageInfo() and ESRT have the meaningful version number. >>> >>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> >>> --- >>> Changes in v5: >>> - newly implement a device tree based versioning >>> >>> .../firmware/firmware-version.txt | 25 ++++++++ >>> lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++-- >>> 2 files changed, 84 insertions(+), 4 deletions(-) >>> create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt >>> >>> diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt >>> new file mode 100644 >>> index 0000000000..6112de4a1d >>> --- /dev/null >>> +++ b/doc/device-tree-bindings/firmware/firmware-version.txt >>> @@ -0,0 +1,25 @@ >>> +firmware-version bindings >>> +------------------------------- >>> + >>> +Required properties: >>> +- image-type-id : guid for image blob type >>> +- image-index : image index >>> +- fw-version : firmware version >>> +- lowest-supported-version : lowest supported version >>> + >>> +Example: >>> + >>> + firmware-version { >>> + image1 { >>> + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; >>> + image-index = <1>; >>> + fw-version = <5>; >>> + lowest-supported-version = <3>; >>> + }; >>> + image2 { >>> + image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0"; >>> + image-index = <2>; >>> + fw-version = <10>; >>> + lowest-supported-version = <7>; >>> + }; >>> + }; >>> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c >>> index 93e2b01c07..1c6ef468bf 100644 >>> --- a/lib/efi_loader/efi_firmware.c >>> +++ b/lib/efi_loader/efi_firmware.c >>> @@ -102,6 +102,56 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported( >>> return EFI_EXIT(EFI_UNSUPPORTED); >>> } >>> >>> +/** >>> + * efi_firmware_get_firmware_version - get firmware version from dtb >>> + * @image_index: Image index >>> + * @image_type_id: Image type id >>> + * @fw_version: Pointer to store the version number >>> + * @lsv: Pointer to store the lowest supported version >>> + * >>> + * Authenticate the capsule if authentication is enabled. >>> + * The image pointer and the image size are updated in case of success. >>> + */ >>> +void efi_firmware_get_firmware_version(u8 image_index, >>> + efi_guid_t *image_type_id, >>> + u32 *fw_version, u32 *lsv) >>> +{ >>> + const void *fdt = gd->fdt_blob; >>> + const fdt32_t *val; >>> + const char *guid_str; >>> + int len, offset, index; >>> + int parent; >>> + >>> + parent = fdt_subnode_offset(fdt, 0, "firmware-version"); >>> + if (parent < 0) >>> + return; >>> + >>> + fdt_for_each_subnode(offset, fdt, parent) { >>> + efi_guid_t guid; >>> + >>> + guid_str = fdt_getprop(fdt, offset, "image-type-id", &len); >>> + if (!guid_str) >>> + continue; >>> + uuid_str_to_bin(guid_str, guid.b, UUID_STR_FORMAT_GUID); >>> + >>> + val = fdt_getprop(fdt, offset, "image-index", &len); >>> + if (!val) >>> + continue; >>> + index = fdt32_to_cpu(*val); >>> + >>> + if (!guidcmp(&guid, image_type_id) && index == image_index) { >>> + val = fdt_getprop(fdt, offset, "fw-version", &len); >>> + if (val) >>> + *fw_version = fdt32_to_cpu(*val); >>> + >>> + val = fdt_getprop(fdt, offset, >>> + "lowest-supported-version", &len); >>> + if (val) >>> + *lsv = fdt32_to_cpu(*val); >>> + } >>> + } >>> +} >>> + >>> /** >>> * efi_fill_image_desc_array - populate image descriptor array >>> * @image_info_size: Size of @image_info >>> @@ -148,13 +198,19 @@ static efi_status_t efi_fill_image_desc_array( >>> *package_version_name = NULL; /* not supported */ >>> >>> for (i = 0; i < num_image_type_guids; i++) { >> >> Currently we define num_image_type_guids per board in a C file. Using >> the same line of code once per board makes no sense to me. Please, move >> the definition of that variable to lib/efi_loader/efi_firmware.c. > > Sorry for the late reply. > > num_image_type_guids is calculated with "ARRAY_SIZE(fw_images)", > fw_images[] array is also defined in each board file, > so we can not simply move num_image_type_guids into > lib/efi_loader/efi_firmware.c. Why can't we have int num_image_type_guids = ARRAY_SIZE(fw_images); in lib/efi_loader/efi_firmware.c? Best regards Heinrich > > And fw_images[] array is abstracted by struct efi_capsule_update_info, > so I think > we should not extract the fw_images[] array. > >> >>> + u32 fw_version = 0; >>> + u32 lowest_supported_version = 0; >> >> These assignments should be in efi_firmware_get_firmware_version. > > OK. > >> >>> + >>> image_info[i].image_index = fw_array[i].image_index; >> >> Why did we ever introduce the field image_index? Please, eliminate it it >> as the GUID is always sufficient to identify an image. > > This is derived from the UEFI specification. > UEFI specification "23.1.2 > EFI_FIRMWARE_MANAGEMENT_PROTOCOL.GetImageInfo()" requires ImageIndex > and ImageTypeId(guid). > > ImageIndex: A unique number identifying the firmware image within the > device. The number is between 1 and > DescriptorCount. > >> >>> image_info[i].image_type_id = fw_array[i].image_type_id; >>> image_info[i].image_id = fw_array[i].image_index; >>> >>> image_info[i].image_id_name = fw_array[i].fw_name; >>> - >>> - image_info[i].version = 0; /* not supported */ >>> + efi_firmware_get_firmware_version(fw_array[i].image_index, >>> + &fw_array[i].image_type_id, >>> + &fw_version, >>> + &lowest_supported_version); >> >> This interface makes no sense to me. We expect images with specific >> GUIDs and should not care about images with other GUIDs that may >> additionally exist in the capsule. >> >> So you must pass the expected GUID as input variable here. > > I don't clearly understand this comment, but the expected GUID is > fw_array[i].image_type_id. > >> >>> + image_info[i].version = fw_version; >>> image_info[i].version_name = NULL; /* not supported */ >> >> Please, add the missing functionality to >> efi_firmware_get_firmware_version(). > > Does it mean we need to support version_name? > I can add a version_name in dtb. > >> >> Please, pass *image_info[i] to efi_firmware_get_firmware_version. That >> will simplify the code. > > OK. > > Thank you for your review. > > Regards, > Masahisa Kojima > >> >> Best regards >> >> Heinrich >> >>> image_info[i].size = 0; >>> image_info[i].attributes_supported = >>> @@ -168,7 +224,7 @@ static efi_status_t efi_fill_image_desc_array( >>> image_info[0].attributes_setting |= >>> IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED; >>> >>> - image_info[i].lowest_supported_image_version = 0; >>> + image_info[i].lowest_supported_image_version = lowest_supported_version; >>> image_info[i].last_attempt_version = 0; >>> image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS; >>> image_info[i].hardware_instance = 1; >>> @@ -290,7 +346,6 @@ efi_status_t EFIAPI efi_firmware_get_image_info( >>> descriptor_version, descriptor_count, >>> descriptor_size, package_version, >>> package_version_name); >>> - >>> return EFI_EXIT(ret); >>> } >>> >>
On Mon, 8 May 2023 at 18:44, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 5/8/23 10:15, Masahisa Kojima wrote: > > Hi Heinrich, > > > > On Thu, 27 Apr 2023 at 15:09, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > >> > >> On 4/10/23 11:07, Masahisa Kojima wrote: > >>> Current FMP->GetImageInfo() always return 0 for the firmware > >>> version, user can not identify which firmware version is currently > >>> running through the EFI interface. > >>> > >>> This commit gets the version information from device tree, > >>> then fills the firmware version, lowest supported version > >>> in FMP->GetImageInfo(). > >>> > >>> Now FMP->GetImageInfo() and ESRT have the meaningful version number. > >>> > >>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > >>> --- > >>> Changes in v5: > >>> - newly implement a device tree based versioning > >>> > >>> .../firmware/firmware-version.txt | 25 ++++++++ > >>> lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++-- > >>> 2 files changed, 84 insertions(+), 4 deletions(-) > >>> create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt > >>> > >>> diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt > >>> new file mode 100644 > >>> index 0000000000..6112de4a1d > >>> --- /dev/null > >>> +++ b/doc/device-tree-bindings/firmware/firmware-version.txt > >>> @@ -0,0 +1,25 @@ > >>> +firmware-version bindings > >>> +------------------------------- > >>> + > >>> +Required properties: > >>> +- image-type-id : guid for image blob type > >>> +- image-index : image index > >>> +- fw-version : firmware version > >>> +- lowest-supported-version : lowest supported version > >>> + > >>> +Example: > >>> + > >>> + firmware-version { > >>> + image1 { > >>> + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; > >>> + image-index = <1>; > >>> + fw-version = <5>; > >>> + lowest-supported-version = <3>; > >>> + }; > >>> + image2 { > >>> + image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0"; > >>> + image-index = <2>; > >>> + fw-version = <10>; > >>> + lowest-supported-version = <7>; > >>> + }; > >>> + }; > >>> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c > >>> index 93e2b01c07..1c6ef468bf 100644 > >>> --- a/lib/efi_loader/efi_firmware.c > >>> +++ b/lib/efi_loader/efi_firmware.c > >>> @@ -102,6 +102,56 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported( > >>> return EFI_EXIT(EFI_UNSUPPORTED); > >>> } > >>> > >>> +/** > >>> + * efi_firmware_get_firmware_version - get firmware version from dtb > >>> + * @image_index: Image index > >>> + * @image_type_id: Image type id > >>> + * @fw_version: Pointer to store the version number > >>> + * @lsv: Pointer to store the lowest supported version > >>> + * > >>> + * Authenticate the capsule if authentication is enabled. > >>> + * The image pointer and the image size are updated in case of success. > >>> + */ > >>> +void efi_firmware_get_firmware_version(u8 image_index, > >>> + efi_guid_t *image_type_id, > >>> + u32 *fw_version, u32 *lsv) > >>> +{ > >>> + const void *fdt = gd->fdt_blob; > >>> + const fdt32_t *val; > >>> + const char *guid_str; > >>> + int len, offset, index; > >>> + int parent; > >>> + > >>> + parent = fdt_subnode_offset(fdt, 0, "firmware-version"); > >>> + if (parent < 0) > >>> + return; > >>> + > >>> + fdt_for_each_subnode(offset, fdt, parent) { > >>> + efi_guid_t guid; > >>> + > >>> + guid_str = fdt_getprop(fdt, offset, "image-type-id", &len); > >>> + if (!guid_str) > >>> + continue; > >>> + uuid_str_to_bin(guid_str, guid.b, UUID_STR_FORMAT_GUID); > >>> + > >>> + val = fdt_getprop(fdt, offset, "image-index", &len); > >>> + if (!val) > >>> + continue; > >>> + index = fdt32_to_cpu(*val); > >>> + > >>> + if (!guidcmp(&guid, image_type_id) && index == image_index) { > >>> + val = fdt_getprop(fdt, offset, "fw-version", &len); > >>> + if (val) > >>> + *fw_version = fdt32_to_cpu(*val); > >>> + > >>> + val = fdt_getprop(fdt, offset, > >>> + "lowest-supported-version", &len); > >>> + if (val) > >>> + *lsv = fdt32_to_cpu(*val); > >>> + } > >>> + } > >>> +} > >>> + > >>> /** > >>> * efi_fill_image_desc_array - populate image descriptor array > >>> * @image_info_size: Size of @image_info > >>> @@ -148,13 +198,19 @@ static efi_status_t efi_fill_image_desc_array( > >>> *package_version_name = NULL; /* not supported */ > >>> > >>> for (i = 0; i < num_image_type_guids; i++) { > >> > >> Currently we define num_image_type_guids per board in a C file. Using > >> the same line of code once per board makes no sense to me. Please, move > >> the definition of that variable to lib/efi_loader/efi_firmware.c. > > > > Sorry for the late reply. > > > > num_image_type_guids is calculated with "ARRAY_SIZE(fw_images)", > > fw_images[] array is also defined in each board file, > > so we can not simply move num_image_type_guids into > > lib/efi_loader/efi_firmware.c. > > Why can't we have > > int num_image_type_guids = ARRAY_SIZE(fw_images); > > in lib/efi_loader/efi_firmware.c? At first thought, I thought it was a matter of abstraction. But there is a compilation error when we expose fw_images[]. fw_images[] array is initialized in each board file, and sizeof() of the external fw_images[] array in lib/efi_loader/efi_firmware.c will cause compilation failure. We need to specify the array size when fw_images is exposed, for example: extern struct efi_fw_image fw_images[2]; but currently there is no method to pre-define the fw_images[] array size, it is board specific. We can define the macro to indicate the array size or having sentinel in the fw_images[] array, but I think the current implementation is simpler, I would like to keep the current implementation. Correct me if I'm wrong. Thanks, Masahisa Kojima > > Best regards > > Heinrich > > > > > And fw_images[] array is abstracted by struct efi_capsule_update_info, > > so I think > > we should not extract the fw_images[] array. > > > >> > >>> + u32 fw_version = 0; > >>> + u32 lowest_supported_version = 0; > >> > >> These assignments should be in efi_firmware_get_firmware_version. > > > > OK. > > > >> > >>> + > >>> image_info[i].image_index = fw_array[i].image_index; > >> > >> Why did we ever introduce the field image_index? Please, eliminate it it > >> as the GUID is always sufficient to identify an image. > > > > This is derived from the UEFI specification. > > UEFI specification "23.1.2 > > EFI_FIRMWARE_MANAGEMENT_PROTOCOL.GetImageInfo()" requires ImageIndex > > and ImageTypeId(guid). > > > > ImageIndex: A unique number identifying the firmware image within the > > device. The number is between 1 and > > DescriptorCount. > > > >> > >>> image_info[i].image_type_id = fw_array[i].image_type_id; > >>> image_info[i].image_id = fw_array[i].image_index; > >>> > >>> image_info[i].image_id_name = fw_array[i].fw_name; > >>> - > >>> - image_info[i].version = 0; /* not supported */ > >>> + efi_firmware_get_firmware_version(fw_array[i].image_index, > >>> + &fw_array[i].image_type_id, > >>> + &fw_version, > >>> + &lowest_supported_version); > >> > >> This interface makes no sense to me. We expect images with specific > >> GUIDs and should not care about images with other GUIDs that may > >> additionally exist in the capsule. > >> > >> So you must pass the expected GUID as input variable here. > > > > I don't clearly understand this comment, but the expected GUID is > > fw_array[i].image_type_id. > > > >> > >>> + image_info[i].version = fw_version; > >>> image_info[i].version_name = NULL; /* not supported */ > >> > >> Please, add the missing functionality to > >> efi_firmware_get_firmware_version(). > > > > Does it mean we need to support version_name? > > I can add a version_name in dtb. > > > >> > >> Please, pass *image_info[i] to efi_firmware_get_firmware_version. That > >> will simplify the code. > > > > OK. > > > > Thank you for your review. > > > > Regards, > > Masahisa Kojima > > > >> > >> Best regards > >> > >> Heinrich > >> > >>> image_info[i].size = 0; > >>> image_info[i].attributes_supported = > >>> @@ -168,7 +224,7 @@ static efi_status_t efi_fill_image_desc_array( > >>> image_info[0].attributes_setting |= > >>> IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED; > >>> > >>> - image_info[i].lowest_supported_image_version = 0; > >>> + image_info[i].lowest_supported_image_version = lowest_supported_version; > >>> image_info[i].last_attempt_version = 0; > >>> image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS; > >>> image_info[i].hardware_instance = 1; > >>> @@ -290,7 +346,6 @@ efi_status_t EFIAPI efi_firmware_get_image_info( > >>> descriptor_version, descriptor_count, > >>> descriptor_size, package_version, > >>> package_version_name); > >>> - > >>> return EFI_EXIT(ret); > >>> } > >>> > >> >
On Tue, May 09, 2023 at 06:57:19PM +0900, Masahisa Kojima wrote: > On Mon, 8 May 2023 at 18:44, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > On 5/8/23 10:15, Masahisa Kojima wrote: > > > Hi Heinrich, > > > > > > On Thu, 27 Apr 2023 at 15:09, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > >> > > >> On 4/10/23 11:07, Masahisa Kojima wrote: > > >>> Current FMP->GetImageInfo() always return 0 for the firmware > > >>> version, user can not identify which firmware version is currently > > >>> running through the EFI interface. > > >>> > > >>> This commit gets the version information from device tree, > > >>> then fills the firmware version, lowest supported version > > >>> in FMP->GetImageInfo(). > > >>> > > >>> Now FMP->GetImageInfo() and ESRT have the meaningful version number. > > >>> > > >>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > > >>> --- > > >>> Changes in v5: > > >>> - newly implement a device tree based versioning > > >>> > > >>> .../firmware/firmware-version.txt | 25 ++++++++ > > >>> lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++-- > > >>> 2 files changed, 84 insertions(+), 4 deletions(-) > > >>> create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt > > >>> > > >>> diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt > > >>> new file mode 100644 > > >>> index 0000000000..6112de4a1d > > >>> --- /dev/null > > >>> +++ b/doc/device-tree-bindings/firmware/firmware-version.txt > > >>> @@ -0,0 +1,25 @@ > > >>> +firmware-version bindings > > >>> +------------------------------- > > >>> + > > >>> +Required properties: > > >>> +- image-type-id : guid for image blob type > > >>> +- image-index : image index > > >>> +- fw-version : firmware version > > >>> +- lowest-supported-version : lowest supported version > > >>> + > > >>> +Example: > > >>> + > > >>> + firmware-version { > > >>> + image1 { > > >>> + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; > > >>> + image-index = <1>; > > >>> + fw-version = <5>; > > >>> + lowest-supported-version = <3>; > > >>> + }; > > >>> + image2 { > > >>> + image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0"; > > >>> + image-index = <2>; > > >>> + fw-version = <10>; > > >>> + lowest-supported-version = <7>; > > >>> + }; > > >>> + }; > > >>> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c > > >>> index 93e2b01c07..1c6ef468bf 100644 > > >>> --- a/lib/efi_loader/efi_firmware.c > > >>> +++ b/lib/efi_loader/efi_firmware.c > > >>> @@ -102,6 +102,56 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported( > > >>> return EFI_EXIT(EFI_UNSUPPORTED); > > >>> } > > >>> > > >>> +/** > > >>> + * efi_firmware_get_firmware_version - get firmware version from dtb > > >>> + * @image_index: Image index > > >>> + * @image_type_id: Image type id > > >>> + * @fw_version: Pointer to store the version number > > >>> + * @lsv: Pointer to store the lowest supported version > > >>> + * > > >>> + * Authenticate the capsule if authentication is enabled. > > >>> + * The image pointer and the image size are updated in case of success. > > >>> + */ > > >>> +void efi_firmware_get_firmware_version(u8 image_index, > > >>> + efi_guid_t *image_type_id, > > >>> + u32 *fw_version, u32 *lsv) > > >>> +{ > > >>> + const void *fdt = gd->fdt_blob; > > >>> + const fdt32_t *val; > > >>> + const char *guid_str; > > >>> + int len, offset, index; > > >>> + int parent; > > >>> + > > >>> + parent = fdt_subnode_offset(fdt, 0, "firmware-version"); > > >>> + if (parent < 0) > > >>> + return; > > >>> + > > >>> + fdt_for_each_subnode(offset, fdt, parent) { > > >>> + efi_guid_t guid; > > >>> + > > >>> + guid_str = fdt_getprop(fdt, offset, "image-type-id", &len); > > >>> + if (!guid_str) > > >>> + continue; > > >>> + uuid_str_to_bin(guid_str, guid.b, UUID_STR_FORMAT_GUID); > > >>> + > > >>> + val = fdt_getprop(fdt, offset, "image-index", &len); > > >>> + if (!val) > > >>> + continue; > > >>> + index = fdt32_to_cpu(*val); > > >>> + > > >>> + if (!guidcmp(&guid, image_type_id) && index == image_index) { > > >>> + val = fdt_getprop(fdt, offset, "fw-version", &len); > > >>> + if (val) > > >>> + *fw_version = fdt32_to_cpu(*val); > > >>> + > > >>> + val = fdt_getprop(fdt, offset, > > >>> + "lowest-supported-version", &len); > > >>> + if (val) > > >>> + *lsv = fdt32_to_cpu(*val); > > >>> + } > > >>> + } > > >>> +} > > >>> + > > >>> /** > > >>> * efi_fill_image_desc_array - populate image descriptor array > > >>> * @image_info_size: Size of @image_info > > >>> @@ -148,13 +198,19 @@ static efi_status_t efi_fill_image_desc_array( > > >>> *package_version_name = NULL; /* not supported */ > > >>> > > >>> for (i = 0; i < num_image_type_guids; i++) { > > >> > > >> Currently we define num_image_type_guids per board in a C file. Using > > >> the same line of code once per board makes no sense to me. Please, move > > >> the definition of that variable to lib/efi_loader/efi_firmware.c. > > > > > > Sorry for the late reply. > > > > > > num_image_type_guids is calculated with "ARRAY_SIZE(fw_images)", > > > fw_images[] array is also defined in each board file, > > > so we can not simply move num_image_type_guids into > > > lib/efi_loader/efi_firmware.c. > > > > Why can't we have > > > > int num_image_type_guids = ARRAY_SIZE(fw_images); > > > > in lib/efi_loader/efi_firmware.c? > > At first thought, I thought it was a matter of abstraction. > > But there is a compilation error when we expose fw_images[]. > fw_images[] array is initialized in each board file, > and sizeof() of the external fw_images[] array in lib/efi_loader/efi_firmware.c > will cause compilation failure. > We need to specify the array size when fw_images is exposed, > for example: > extern struct efi_fw_image fw_images[2]; > > but currently there is no method to pre-define the fw_images[] array size, > it is board specific. > > We can define the macro to indicate the array size or having > sentinel in the fw_images[] array, but I think the current I simply wonder if the value should be embedded in "struct efi_capsule_update_info". struct efi_capsule_update_info { const char *dfu_string; int num_images; <- added struct efi_fw_image *images; }; This is the best place because the value must match not only "images" but also (entries in) "dfu_string". Even now, efi_fill_image_desc_array() tries to access "fw_images[]" via the exposed update_info variable. Beautiful, isn't it? One more comment: uefi.rst doesn't mention anything about num_image_type_guids. -Takahiro Akashi > implementation is simpler, > I would like to keep the current implementation. > Correct me if I'm wrong. > > Thanks, > Masahisa Kojima > > > > > Best regards > > > > Heinrich > > > > > > > > And fw_images[] array is abstracted by struct efi_capsule_update_info, > > > so I think > > > we should not extract the fw_images[] array. > > > > > >> > > >>> + u32 fw_version = 0; > > >>> + u32 lowest_supported_version = 0; > > >> > > >> These assignments should be in efi_firmware_get_firmware_version. > > > > > > OK. > > > > > >> > > >>> + > > >>> image_info[i].image_index = fw_array[i].image_index; > > >> > > >> Why did we ever introduce the field image_index? Please, eliminate it it > > >> as the GUID is always sufficient to identify an image. > > > > > > This is derived from the UEFI specification. > > > UEFI specification "23.1.2 > > > EFI_FIRMWARE_MANAGEMENT_PROTOCOL.GetImageInfo()" requires ImageIndex > > > and ImageTypeId(guid). > > > > > > ImageIndex: A unique number identifying the firmware image within the > > > device. The number is between 1 and > > > DescriptorCount. > > > > > >> > > >>> image_info[i].image_type_id = fw_array[i].image_type_id; > > >>> image_info[i].image_id = fw_array[i].image_index; > > >>> > > >>> image_info[i].image_id_name = fw_array[i].fw_name; > > >>> - > > >>> - image_info[i].version = 0; /* not supported */ > > >>> + efi_firmware_get_firmware_version(fw_array[i].image_index, > > >>> + &fw_array[i].image_type_id, > > >>> + &fw_version, > > >>> + &lowest_supported_version); > > >> > > >> This interface makes no sense to me. We expect images with specific > > >> GUIDs and should not care about images with other GUIDs that may > > >> additionally exist in the capsule. > > >> > > >> So you must pass the expected GUID as input variable here. > > > > > > I don't clearly understand this comment, but the expected GUID is > > > fw_array[i].image_type_id. > > > > > >> > > >>> + image_info[i].version = fw_version; > > >>> image_info[i].version_name = NULL; /* not supported */ > > >> > > >> Please, add the missing functionality to > > >> efi_firmware_get_firmware_version(). > > > > > > Does it mean we need to support version_name? > > > I can add a version_name in dtb. > > > > > >> > > >> Please, pass *image_info[i] to efi_firmware_get_firmware_version. That > > >> will simplify the code. > > > > > > OK. > > > > > > Thank you for your review. > > > > > > Regards, > > > Masahisa Kojima > > > > > >> > > >> Best regards > > >> > > >> Heinrich > > >> > > >>> image_info[i].size = 0; > > >>> image_info[i].attributes_supported = > > >>> @@ -168,7 +224,7 @@ static efi_status_t efi_fill_image_desc_array( > > >>> image_info[0].attributes_setting |= > > >>> IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED; > > >>> > > >>> - image_info[i].lowest_supported_image_version = 0; > > >>> + image_info[i].lowest_supported_image_version = lowest_supported_version; > > >>> image_info[i].last_attempt_version = 0; > > >>> image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS; > > >>> image_info[i].hardware_instance = 1; > > >>> @@ -290,7 +346,6 @@ efi_status_t EFIAPI efi_firmware_get_image_info( > > >>> descriptor_version, descriptor_count, > > >>> descriptor_size, package_version, > > >>> package_version_name); > > >>> - > > >>> return EFI_EXIT(ret); > > >>> } > > >>> > > >> > >
Hi Akashi-san, On Wed, 10 May 2023 at 09:28, Takahiro Akashi <takahiro.akashi@linaro.org> wrote: > > On Tue, May 09, 2023 at 06:57:19PM +0900, Masahisa Kojima wrote: > > On Mon, 8 May 2023 at 18:44, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > On 5/8/23 10:15, Masahisa Kojima wrote: > > > > Hi Heinrich, > > > > > > > > On Thu, 27 Apr 2023 at 15:09, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > >> > > > >> On 4/10/23 11:07, Masahisa Kojima wrote: > > > >>> Current FMP->GetImageInfo() always return 0 for the firmware > > > >>> version, user can not identify which firmware version is currently > > > >>> running through the EFI interface. > > > >>> > > > >>> This commit gets the version information from device tree, > > > >>> then fills the firmware version, lowest supported version > > > >>> in FMP->GetImageInfo(). > > > >>> > > > >>> Now FMP->GetImageInfo() and ESRT have the meaningful version number. > > > >>> > > > >>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > > > >>> --- > > > >>> Changes in v5: > > > >>> - newly implement a device tree based versioning > > > >>> > > > >>> .../firmware/firmware-version.txt | 25 ++++++++ > > > >>> lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++-- > > > >>> 2 files changed, 84 insertions(+), 4 deletions(-) > > > >>> create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt > > > >>> > > > >>> diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt > > > >>> new file mode 100644 > > > >>> index 0000000000..6112de4a1d > > > >>> --- /dev/null > > > >>> +++ b/doc/device-tree-bindings/firmware/firmware-version.txt > > > >>> @@ -0,0 +1,25 @@ > > > >>> +firmware-version bindings > > > >>> +------------------------------- > > > >>> + > > > >>> +Required properties: > > > >>> +- image-type-id : guid for image blob type > > > >>> +- image-index : image index > > > >>> +- fw-version : firmware version > > > >>> +- lowest-supported-version : lowest supported version > > > >>> + > > > >>> +Example: > > > >>> + > > > >>> + firmware-version { > > > >>> + image1 { > > > >>> + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; > > > >>> + image-index = <1>; > > > >>> + fw-version = <5>; > > > >>> + lowest-supported-version = <3>; > > > >>> + }; > > > >>> + image2 { > > > >>> + image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0"; > > > >>> + image-index = <2>; > > > >>> + fw-version = <10>; > > > >>> + lowest-supported-version = <7>; > > > >>> + }; > > > >>> + }; > > > >>> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c > > > >>> index 93e2b01c07..1c6ef468bf 100644 > > > >>> --- a/lib/efi_loader/efi_firmware.c > > > >>> +++ b/lib/efi_loader/efi_firmware.c > > > >>> @@ -102,6 +102,56 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported( > > > >>> return EFI_EXIT(EFI_UNSUPPORTED); > > > >>> } > > > >>> > > > >>> +/** > > > >>> + * efi_firmware_get_firmware_version - get firmware version from dtb > > > >>> + * @image_index: Image index > > > >>> + * @image_type_id: Image type id > > > >>> + * @fw_version: Pointer to store the version number > > > >>> + * @lsv: Pointer to store the lowest supported version > > > >>> + * > > > >>> + * Authenticate the capsule if authentication is enabled. > > > >>> + * The image pointer and the image size are updated in case of success. > > > >>> + */ > > > >>> +void efi_firmware_get_firmware_version(u8 image_index, > > > >>> + efi_guid_t *image_type_id, > > > >>> + u32 *fw_version, u32 *lsv) > > > >>> +{ > > > >>> + const void *fdt = gd->fdt_blob; > > > >>> + const fdt32_t *val; > > > >>> + const char *guid_str; > > > >>> + int len, offset, index; > > > >>> + int parent; > > > >>> + > > > >>> + parent = fdt_subnode_offset(fdt, 0, "firmware-version"); > > > >>> + if (parent < 0) > > > >>> + return; > > > >>> + > > > >>> + fdt_for_each_subnode(offset, fdt, parent) { > > > >>> + efi_guid_t guid; > > > >>> + > > > >>> + guid_str = fdt_getprop(fdt, offset, "image-type-id", &len); > > > >>> + if (!guid_str) > > > >>> + continue; > > > >>> + uuid_str_to_bin(guid_str, guid.b, UUID_STR_FORMAT_GUID); > > > >>> + > > > >>> + val = fdt_getprop(fdt, offset, "image-index", &len); > > > >>> + if (!val) > > > >>> + continue; > > > >>> + index = fdt32_to_cpu(*val); > > > >>> + > > > >>> + if (!guidcmp(&guid, image_type_id) && index == image_index) { > > > >>> + val = fdt_getprop(fdt, offset, "fw-version", &len); > > > >>> + if (val) > > > >>> + *fw_version = fdt32_to_cpu(*val); > > > >>> + > > > >>> + val = fdt_getprop(fdt, offset, > > > >>> + "lowest-supported-version", &len); > > > >>> + if (val) > > > >>> + *lsv = fdt32_to_cpu(*val); > > > >>> + } > > > >>> + } > > > >>> +} > > > >>> + > > > >>> /** > > > >>> * efi_fill_image_desc_array - populate image descriptor array > > > >>> * @image_info_size: Size of @image_info > > > >>> @@ -148,13 +198,19 @@ static efi_status_t efi_fill_image_desc_array( > > > >>> *package_version_name = NULL; /* not supported */ > > > >>> > > > >>> for (i = 0; i < num_image_type_guids; i++) { > > > >> > > > >> Currently we define num_image_type_guids per board in a C file. Using > > > >> the same line of code once per board makes no sense to me. Please, move > > > >> the definition of that variable to lib/efi_loader/efi_firmware.c. > > > > > > > > Sorry for the late reply. > > > > > > > > num_image_type_guids is calculated with "ARRAY_SIZE(fw_images)", > > > > fw_images[] array is also defined in each board file, > > > > so we can not simply move num_image_type_guids into > > > > lib/efi_loader/efi_firmware.c. > > > > > > Why can't we have > > > > > > int num_image_type_guids = ARRAY_SIZE(fw_images); > > > > > > in lib/efi_loader/efi_firmware.c? > > > > At first thought, I thought it was a matter of abstraction. > > > > But there is a compilation error when we expose fw_images[]. > > fw_images[] array is initialized in each board file, > > and sizeof() of the external fw_images[] array in lib/efi_loader/efi_firmware.c > > will cause compilation failure. > > We need to specify the array size when fw_images is exposed, > > for example: > > extern struct efi_fw_image fw_images[2]; > > > > but currently there is no method to pre-define the fw_images[] array size, > > it is board specific. > > > > We can define the macro to indicate the array size or having > > sentinel in the fw_images[] array, but I think the current > > I simply wonder if the value should be embedded in "struct efi_capsule_update_info". > struct efi_capsule_update_info { > const char *dfu_string; > int num_images; <- added > struct efi_fw_image *images; > }; Thank you, I agree with this. > This is the best place because the value must match not only "images" > but also (entries in) "dfu_string". > > Even now, efi_fill_image_desc_array() tries to access "fw_images[]" > via the exposed update_info variable. Beautiful, isn't it? > > One more comment: > uefi.rst doesn't mention anything about num_image_type_guids. I will also update uefi.rst. Regards, Masahisa Kojima > > -Takahiro Akashi > > > implementation is simpler, > > I would like to keep the current implementation. > > Correct me if I'm wrong. > > > > Thanks, > > Masahisa Kojima > > > > > > > > Best regards > > > > > > Heinrich > > > > > > > > > > > And fw_images[] array is abstracted by struct efi_capsule_update_info, > > > > so I think > > > > we should not extract the fw_images[] array. > > > > > > > >> > > > >>> + u32 fw_version = 0; > > > >>> + u32 lowest_supported_version = 0; > > > >> > > > >> These assignments should be in efi_firmware_get_firmware_version. > > > > > > > > OK. > > > > > > > >> > > > >>> + > > > >>> image_info[i].image_index = fw_array[i].image_index; > > > >> > > > >> Why did we ever introduce the field image_index? Please, eliminate it it > > > >> as the GUID is always sufficient to identify an image. > > > > > > > > This is derived from the UEFI specification. > > > > UEFI specification "23.1.2 > > > > EFI_FIRMWARE_MANAGEMENT_PROTOCOL.GetImageInfo()" requires ImageIndex > > > > and ImageTypeId(guid). > > > > > > > > ImageIndex: A unique number identifying the firmware image within the > > > > device. The number is between 1 and > > > > DescriptorCount. > > > > > > > >> > > > >>> image_info[i].image_type_id = fw_array[i].image_type_id; > > > >>> image_info[i].image_id = fw_array[i].image_index; > > > >>> > > > >>> image_info[i].image_id_name = fw_array[i].fw_name; > > > >>> - > > > >>> - image_info[i].version = 0; /* not supported */ > > > >>> + efi_firmware_get_firmware_version(fw_array[i].image_index, > > > >>> + &fw_array[i].image_type_id, > > > >>> + &fw_version, > > > >>> + &lowest_supported_version); > > > >> > > > >> This interface makes no sense to me. We expect images with specific > > > >> GUIDs and should not care about images with other GUIDs that may > > > >> additionally exist in the capsule. > > > >> > > > >> So you must pass the expected GUID as input variable here. > > > > > > > > I don't clearly understand this comment, but the expected GUID is > > > > fw_array[i].image_type_id. > > > > > > > >> > > > >>> + image_info[i].version = fw_version; > > > >>> image_info[i].version_name = NULL; /* not supported */ > > > >> > > > >> Please, add the missing functionality to > > > >> efi_firmware_get_firmware_version(). > > > > > > > > Does it mean we need to support version_name? > > > > I can add a version_name in dtb. > > > > > > > >> > > > >> Please, pass *image_info[i] to efi_firmware_get_firmware_version. That > > > >> will simplify the code. > > > > > > > > OK. > > > > > > > > Thank you for your review. > > > > > > > > Regards, > > > > Masahisa Kojima > > > > > > > >> > > > >> Best regards > > > >> > > > >> Heinrich > > > >> > > > >>> image_info[i].size = 0; > > > >>> image_info[i].attributes_supported = > > > >>> @@ -168,7 +224,7 @@ static efi_status_t efi_fill_image_desc_array( > > > >>> image_info[0].attributes_setting |= > > > >>> IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED; > > > >>> > > > >>> - image_info[i].lowest_supported_image_version = 0; > > > >>> + image_info[i].lowest_supported_image_version = lowest_supported_version; > > > >>> image_info[i].last_attempt_version = 0; > > > >>> image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS; > > > >>> image_info[i].hardware_instance = 1; > > > >>> @@ -290,7 +346,6 @@ efi_status_t EFIAPI efi_firmware_get_image_info( > > > >>> descriptor_version, descriptor_count, > > > >>> descriptor_size, package_version, > > > >>> package_version_name); > > > >>> - > > > >>> return EFI_EXIT(ret); > > > >>> } > > > >>> > > > >> > > >
Hi Heinrich, On Thu, 27 Apr 2023 at 22:12, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > Am 28. April 2023 01:35:04 MESZ schrieb Simon Glass <sjg@chromium.org>: > >Hi Masahisa, > > > >On Mon, 10 Apr 2023 at 03:07, Masahisa Kojima > ><masahisa.kojima@linaro.org> wrote: > >> > >> Current FMP->GetImageInfo() always return 0 for the firmware > >> version, user can not identify which firmware version is currently > >> running through the EFI interface. > >> > >> This commit gets the version information from device tree, > >> then fills the firmware version, lowest supported version > >> in FMP->GetImageInfo(). > >> > >> Now FMP->GetImageInfo() and ESRT have the meaningful version number. > >> > >> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > >> --- > >> Changes in v5: > >> - newly implement a device tree based versioning > >> > >> .../firmware/firmware-version.txt | 25 ++++++++ > >> lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++-- > >> 2 files changed, 84 insertions(+), 4 deletions(-) > >> create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt > >> > >> diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt > >> new file mode 100644 > >> index 0000000000..6112de4a1d > >> --- /dev/null > >> +++ b/doc/device-tree-bindings/firmware/firmware-version.txt > >> @@ -0,0 +1,25 @@ > >> +firmware-version bindings > >> +------------------------------- > >> + > >> +Required properties: > >> +- image-type-id : guid for image blob type > >> +- image-index : image index > >> +- fw-version : firmware version > >> +- lowest-supported-version : lowest supported version > >> + > >> +Example: > >> + > >> + firmware-version { > >> + image1 { > >> + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; > > > >Nit: please use lower-case hex and add a decoder to uuid.c so we can > >look it up when debugging. > > The GUIDs are board specific. No, we should not clutter uuid.c with strings for dozens of boards. Our development aim is to keep U-Boot small and these GUIDs are not printed anywhere. > > Instead we should define a string per firmware image. We already have a field for it: > > fw_array[i].fw_name This is of course madness. NAK to any UUIDs in U-Boot that don't have a decoder ring. Let's use a compatible string (vendor,board) and drop these silly UUIDs. Regards, Simon
On 5/10/23 22:46, Simon Glass wrote: > Hi Heinrich, > > On Thu, 27 Apr 2023 at 22:12, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> >> >> >> Am 28. April 2023 01:35:04 MESZ schrieb Simon Glass <sjg@chromium.org>: >>> Hi Masahisa, >>> >>> On Mon, 10 Apr 2023 at 03:07, Masahisa Kojima >>> <masahisa.kojima@linaro.org> wrote: >>>> >>>> Current FMP->GetImageInfo() always return 0 for the firmware >>>> version, user can not identify which firmware version is currently >>>> running through the EFI interface. >>>> >>>> This commit gets the version information from device tree, >>>> then fills the firmware version, lowest supported version >>>> in FMP->GetImageInfo(). >>>> >>>> Now FMP->GetImageInfo() and ESRT have the meaningful version number. >>>> >>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> >>>> --- >>>> Changes in v5: >>>> - newly implement a device tree based versioning >>>> >>>> .../firmware/firmware-version.txt | 25 ++++++++ >>>> lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++-- >>>> 2 files changed, 84 insertions(+), 4 deletions(-) >>>> create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt >>>> >>>> diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt >>>> new file mode 100644 >>>> index 0000000000..6112de4a1d >>>> --- /dev/null >>>> +++ b/doc/device-tree-bindings/firmware/firmware-version.txt >>>> @@ -0,0 +1,25 @@ >>>> +firmware-version bindings >>>> +------------------------------- >>>> + >>>> +Required properties: >>>> +- image-type-id : guid for image blob type >>>> +- image-index : image index >>>> +- fw-version : firmware version >>>> +- lowest-supported-version : lowest supported version >>>> + >>>> +Example: >>>> + >>>> + firmware-version { >>>> + image1 { >>>> + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; >>> >>> Nit: please use lower-case hex and add a decoder to uuid.c so we can >>> look it up when debugging. >> >> The GUIDs are board specific. No, we should not clutter uuid.c with strings for dozens of boards. Our development aim is to keep U-Boot small and these GUIDs are not printed anywhere. >> >> Instead we should define a string per firmware image. We already have a field for it: >> >> fw_array[i].fw_name > > This is of course madness. NAK to any UUIDs in U-Boot that don't have > a decoder ring. I never have heard of a "decoder ring" for UUIDs in U-Boot. git grep -A2 -ni decoder | grep -i ring does not find such a term. Our development target is to keep the code size of U-Boot small. Hence, we should not clutter uuid.c with board specific strings. These should be kept in board specific files. > > Let's use a compatible string (vendor,board) and drop these silly UUIDs. Please, avoid derogatory language like "silly". The UEFI specification defines EFI_CAPSULE_HEADER. This structure contains field CapusuleGuid (A GUID that defines the contents of a capsule). Best regards Heinrich
diff --git a/doc/device-tree-bindings/firmware/firmware-version.txt b/doc/device-tree-bindings/firmware/firmware-version.txt new file mode 100644 index 0000000000..6112de4a1d --- /dev/null +++ b/doc/device-tree-bindings/firmware/firmware-version.txt @@ -0,0 +1,25 @@ +firmware-version bindings +------------------------------- + +Required properties: +- image-type-id : guid for image blob type +- image-index : image index +- fw-version : firmware version +- lowest-supported-version : lowest supported version + +Example: + + firmware-version { + image1 { + image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8"; + image-index = <1>; + fw-version = <5>; + lowest-supported-version = <3>; + }; + image2 { + image-type-id = "5A7021F5-FEF2-48B4-AABA-832E777418C0"; + image-index = <2>; + fw-version = <10>; + lowest-supported-version = <7>; + }; + }; diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c index 93e2b01c07..1c6ef468bf 100644 --- a/lib/efi_loader/efi_firmware.c +++ b/lib/efi_loader/efi_firmware.c @@ -102,6 +102,56 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported( return EFI_EXIT(EFI_UNSUPPORTED); } +/** + * efi_firmware_get_firmware_version - get firmware version from dtb + * @image_index: Image index + * @image_type_id: Image type id + * @fw_version: Pointer to store the version number + * @lsv: Pointer to store the lowest supported version + * + * Authenticate the capsule if authentication is enabled. + * The image pointer and the image size are updated in case of success. + */ +void efi_firmware_get_firmware_version(u8 image_index, + efi_guid_t *image_type_id, + u32 *fw_version, u32 *lsv) +{ + const void *fdt = gd->fdt_blob; + const fdt32_t *val; + const char *guid_str; + int len, offset, index; + int parent; + + parent = fdt_subnode_offset(fdt, 0, "firmware-version"); + if (parent < 0) + return; + + fdt_for_each_subnode(offset, fdt, parent) { + efi_guid_t guid; + + guid_str = fdt_getprop(fdt, offset, "image-type-id", &len); + if (!guid_str) + continue; + uuid_str_to_bin(guid_str, guid.b, UUID_STR_FORMAT_GUID); + + val = fdt_getprop(fdt, offset, "image-index", &len); + if (!val) + continue; + index = fdt32_to_cpu(*val); + + if (!guidcmp(&guid, image_type_id) && index == image_index) { + val = fdt_getprop(fdt, offset, "fw-version", &len); + if (val) + *fw_version = fdt32_to_cpu(*val); + + val = fdt_getprop(fdt, offset, + "lowest-supported-version", &len); + if (val) + *lsv = fdt32_to_cpu(*val); + } + } +} + /** * efi_fill_image_desc_array - populate image descriptor array * @image_info_size: Size of @image_info @@ -148,13 +198,19 @@ static efi_status_t efi_fill_image_desc_array( *package_version_name = NULL; /* not supported */ for (i = 0; i < num_image_type_guids; i++) { + u32 fw_version = 0; + u32 lowest_supported_version = 0; + image_info[i].image_index = fw_array[i].image_index; image_info[i].image_type_id = fw_array[i].image_type_id; image_info[i].image_id = fw_array[i].image_index; image_info[i].image_id_name = fw_array[i].fw_name; - - image_info[i].version = 0; /* not supported */ + efi_firmware_get_firmware_version(fw_array[i].image_index, + &fw_array[i].image_type_id, + &fw_version, + &lowest_supported_version); + image_info[i].version = fw_version; image_info[i].version_name = NULL; /* not supported */ image_info[i].size = 0; image_info[i].attributes_supported = @@ -168,7 +224,7 @@ static efi_status_t efi_fill_image_desc_array( image_info[0].attributes_setting |= IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED; - image_info[i].lowest_supported_image_version = 0; + image_info[i].lowest_supported_image_version = lowest_supported_version; image_info[i].last_attempt_version = 0; image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS; image_info[i].hardware_instance = 1; @@ -290,7 +346,6 @@ efi_status_t EFIAPI efi_firmware_get_image_info( descriptor_version, descriptor_count, descriptor_size, package_version, package_version_name); - return EFI_EXIT(ret); }
Current FMP->GetImageInfo() always return 0 for the firmware version, user can not identify which firmware version is currently running through the EFI interface. This commit gets the version information from device tree, then fills the firmware version, lowest supported version in FMP->GetImageInfo(). Now FMP->GetImageInfo() and ESRT have the meaningful version number. Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> --- Changes in v5: - newly implement a device tree based versioning .../firmware/firmware-version.txt | 25 ++++++++ lib/efi_loader/efi_firmware.c | 63 +++++++++++++++++-- 2 files changed, 84 insertions(+), 4 deletions(-) create mode 100644 doc/device-tree-bindings/firmware/firmware-version.txt