Hi Simon,
On 3/28/22 2:35 AM, Simon Glass wrote:
> Hi Sean,
>
> On Thu, 24 Mar 2022 at 12:23, Sean Anderson <sean.ander...@seco.com> wrote:
>>
>> Several different firmware users have repetitive code to extract the
>> firmware data from a FIT. Add some helper functions to reduce the amount
>> of repetition. fit_conf_get_prop_node (eventually) calls
>> fdt_check_node_offset_, so we can avoid an explicit if. In general, this
>> version avoids printing on error because the callers are typically
>> library functions, and because the FIT code generally has (debug)
>> prints of its own. One difference in these helpers is that they use
>> fit_image_get_data_and_size instead of fit_image_get_data, as the former
>> handles external data correctly.
>>
>> Signed-off-by: Sean Anderson <sean.ander...@seco.com>
>> ---
>>
>> arch/arm/cpu/armv8/sec_firmware.c | 39 ++---------------------------
>> boot/image-fit.c | 37 +++++++++++++++++++++++++++
>> cmd/fpga.c | 24 +++++-------------
>> drivers/net/fsl-mc/mc.c | 30 +++-------------------
>> drivers/net/pfe_eth/pfe_firmware.c | 40 +-----------------------------
>> include/image.h | 4 +++
>> 6 files changed, 53 insertions(+), 121 deletions(-)
>
> It feels like this patch should be split up into generic / armv8 / fsp things.
OK, I will do that for V2.
(Actually, I think this series can be split into fs-loader and verified-boot).
>>
>> diff --git a/arch/arm/cpu/armv8/sec_firmware.c
>> b/arch/arm/cpu/armv8/sec_firmware.c
>> index 41525a10d5..879eb6ec81 100644
>> --- a/arch/arm/cpu/armv8/sec_firmware.c
>> +++ b/arch/arm/cpu/armv8/sec_firmware.c
>> @@ -42,43 +42,8 @@ phys_addr_t sec_firmware_addr;
>> static int sec_firmware_get_data(const void *sec_firmware_img,
>> const void **data, size_t *size)
>> {
>> - int conf_node_off, fw_node_off;
>> - char *desc;
>> - int ret;
>> -
>> - conf_node_off = fit_conf_get_node(sec_firmware_img, NULL);
>> - if (conf_node_off < 0) {
>> - puts("SEC Firmware: no config\n");
>> - return -ENOENT;
>> - }
>> -
>> - fw_node_off = fit_conf_get_prop_node(sec_firmware_img, conf_node_off,
>> - SEC_FIRMWARE_FIT_IMAGE);
>> - if (fw_node_off < 0) {
>> - printf("SEC Firmware: No '%s' in config\n",
>> - SEC_FIRMWARE_FIT_IMAGE);
>> - return -ENOLINK;
>> - }
>> -
>> - /* Verify secure firmware image */
>> - if (!(fit_image_verify(sec_firmware_img, fw_node_off))) {
>> - printf("SEC Firmware: Bad firmware image (bad CRC)\n");
>> - return -EINVAL;
>> - }
>> -
>> - if (fit_image_get_data(sec_firmware_img, fw_node_off, data, size)) {
>> - printf("SEC Firmware: Can't get %s subimage data/size",
>> - SEC_FIRMWARE_FIT_IMAGE);
>> - return -ENOENT;
>> - }
>> -
>> - ret = fit_get_desc(sec_firmware_img, fw_node_off, &desc);
>> - if (ret)
>> - printf("SEC Firmware: Can't get description\n");
>> - else
>> - printf("%s\n", desc);
>> -
>> - return ret;
>> + return fit_get_data_conf_prop(sec_firmware_img,
>> SEC_FIRMWARE_FIT_IMAGE,
>> + data, size);
>> }
>>
>> /*
>> diff --git a/boot/image-fit.c b/boot/image-fit.c
>> index 6610035d0a..b87378cbf6 100644
>> --- a/boot/image-fit.c
>> +++ b/boot/image-fit.c
>> @@ -1919,6 +1919,43 @@ int fit_conf_get_prop_node(const void *fit, int
>> noffset,
>> return fit_conf_get_prop_node_index(fit, noffset, prop_name, 0);
>> }
>>
>> +static int fit_get_data_tail(const void *fit, int noffset,
>> + const void **data, size_t *size)
>> +{
>> + char *desc;
>> +
>> + if (noffset < 0)
>> + return noffset;
>> +
>> + if (!fit_image_verify(fit, noffset))
>> + return -EINVAL;
>> +
>> + if (fit_image_get_data_and_size(fit, noffset, data, size))
>> + return -ENOENT;
>> +
>> + if (!fit_get_desc(fit, noffset, &desc))
>> + printf("%s\n", desc);
>> +
>> + return 0;
>> +}
>> +
>> +int fit_get_data_node(const void *fit, const char *image_uname,
>> + const void **data, size_t *size)
>> +{
>> + int noffset = fit_image_get_node(fit, image_uname);
>> +
>> + return fit_get_data_tail(fit, noffset, data, size);
>> +}
>> +
>> +int fit_get_data_conf_prop(const void *fit, const char *prop_name,
>> + const void **data, size_t *size)
>> +{
>> + int noffset = fit_conf_get_node(fit, NULL);
>> +
>> + noffset = fit_conf_get_prop_node(fit, noffset, prop_name);
>> + return fit_get_data_tail(fit, noffset, data, size);
>> +}
>> +
>> static int fit_image_select(const void *fit, int rd_noffset, int verify)
>> {
>> fit_image_print(fit, rd_noffset, " ");
>> diff --git a/cmd/fpga.c b/cmd/fpga.c
>> index 3fdd0b35e8..1102a84d76 100644
>> --- a/cmd/fpga.c
>> +++ b/cmd/fpga.c
>> @@ -322,7 +322,7 @@ static int do_fpga_loadmk(struct cmd_tbl *cmdtp, int
>> flag, int argc,
>> case IMAGE_FORMAT_FIT:
>> {
>> const void *fit_hdr = (const void *)fpga_data;
>> - int noffset;
>> + int err;
>> const void *fit_data;
>>
>> if (!fit_uname) {
>> @@ -335,23 +335,11 @@ static int do_fpga_loadmk(struct cmd_tbl *cmdtp, int
>> flag, int argc,
>> return CMD_RET_FAILURE;
>> }
>>
>> - /* get fpga component image node offset */
>> - noffset = fit_image_get_node(fit_hdr, fit_uname);
>> - if (noffset < 0) {
>> - printf("Can't find '%s' FIT subimage\n", fit_uname);
>> - return CMD_RET_FAILURE;
>> - }
>> -
>> - /* verify integrity */
>> - if (!fit_image_verify(fit_hdr, noffset)) {
>> - puts("Bad Data Hash\n");
>> - return CMD_RET_FAILURE;
>> - }
>> -
>> - /* get fpga subimage/external data address and length */
>> - if (fit_image_get_data_and_size(fit_hdr, noffset,
>> - &fit_data, &data_size)) {
>> - puts("Fpga subimage data not found\n");
>> + err = fit_get_data_node(fit_hdr, fit_uname, &fit_data,
>> + &data_size);
>> + if (err) {
>> + printf("Could not load '%s' subimage (err %d)\n",
>> + fit_uname, err);
>> return CMD_RET_FAILURE;
>> }
>>
>> diff --git a/drivers/net/fsl-mc/mc.c b/drivers/net/fsl-mc/mc.c
>> index bc1c31d467..68833f9ddd 100644
>> --- a/drivers/net/fsl-mc/mc.c
>> +++ b/drivers/net/fsl-mc/mc.c
>> @@ -137,13 +137,7 @@ int parse_mc_firmware_fit_image(u64 mc_fw_addr,
>> size_t *raw_image_size)
>> {
>> int format;
>> - void *fit_hdr;
>> - int node_offset;
>> - const void *data;
>> - size_t size;
>> - const char *uname = "firmware";
>> -
>> - fit_hdr = (void *)mc_fw_addr;
>> + void *fit_hdr = (void *)mc_fw_addr;
>>
>> /* Check if Image is in FIT format */
>> format = genimg_get_format(fit_hdr);
>> @@ -158,26 +152,8 @@ int parse_mc_firmware_fit_image(u64 mc_fw_addr,
>> return -EINVAL;
>> }
>>
>> - node_offset = fit_image_get_node(fit_hdr, uname);
>> -
>> - if (node_offset < 0) {
>> - printf("fsl-mc: ERR: Bad firmware image (missing
>> subimage)\n");
>> - return -ENOENT;
>> - }
>> -
>> - /* Verify MC firmware image */
>> - if (!(fit_image_verify(fit_hdr, node_offset))) {
>> - printf("fsl-mc: ERR: Bad firmware image (bad CRC)\n");
>> - return -EINVAL;
>> - }
>> -
>> - /* Get address and size of raw image */
>> - fit_image_get_data(fit_hdr, node_offset, &data, &size);
>> -
>> - *raw_image_addr = data;
>> - *raw_image_size = size;
>> -
>> - return 0;
>> + return fit_get_data_node(fit_hdr, "firmware", raw_image_addr,
>> + raw_image_size);
>> }
>> #endif
>>
>> diff --git a/drivers/net/pfe_eth/pfe_firmware.c
>> b/drivers/net/pfe_eth/pfe_firmware.c
>> index 6669048181..adaa139219 100644
>> --- a/drivers/net/pfe_eth/pfe_firmware.c
>> +++ b/drivers/net/pfe_eth/pfe_firmware.c
>> @@ -104,45 +104,7 @@ err:
>> static int pfe_get_fw(const void **data,
>> size_t *size, char *fw_name)
>> {
>> - int conf_node_off, fw_node_off;
>> - char *conf_node_name = NULL;
>> - char *desc;
>> - int ret = 0;
>> -
>> - conf_node_name = PFE_FIRMWARE_FIT_CNF_NAME;
>> -
>> - conf_node_off = fit_conf_get_node(pfe_fit_addr, conf_node_name);
>> - if (conf_node_off < 0) {
>> - printf("PFE Firmware: %s: no such config\n", conf_node_name);
>> - return -ENOENT;
>> - }
>> -
>> - fw_node_off = fit_conf_get_prop_node(pfe_fit_addr, conf_node_off,
>> - fw_name);
>> - if (fw_node_off < 0) {
>> - printf("PFE Firmware: No '%s' in config\n",
>> - fw_name);
>> - return -ENOLINK;
>> - }
>> -
>> - if (!(fit_image_verify(pfe_fit_addr, fw_node_off))) {
>> - printf("PFE Firmware: Bad firmware image (bad CRC)\n");
>> - return -EINVAL;
>> - }
>> -
>> - if (fit_image_get_data(pfe_fit_addr, fw_node_off, data, size)) {
>> - printf("PFE Firmware: Can't get %s subimage data/size",
>> - fw_name);
>> - return -ENOENT;
>> - }
>> -
>> - ret = fit_get_desc(pfe_fit_addr, fw_node_off, &desc);
>> - if (ret)
>> - printf("PFE Firmware: Can't get description\n");
>> - else
>> - printf("%s\n", desc);
>> -
>> - return ret;
>> + return fit_get_data_conf_prop(pfe_fit_addr, fw_name, data, size);
>> }
>>
>> /*
>> diff --git a/include/image.h b/include/image.h
>> index 97e5f2eb24..ae1f015896 100644
>> --- a/include/image.h
>> +++ b/include/image.h
>> @@ -1010,6 +1010,10 @@ int fit_image_get_data_size_unciphered(const void
>> *fit, int noffset,
>> size_t *data_size);
>> int fit_image_get_data_and_size(const void *fit, int noffset,
>> const void **data, size_t *size);
>> +int fit_get_data_node(const void *fit, const char *image_uname,
>> + const void **data, size_t *size);
>> +int fit_get_data_conf_prop(const void *fit, const char *prop_name,
>> + const void **data, size_t *size);
>
> Please comment the functions.
Sure.
>>
>> int fit_image_hash_get_algo(const void *fit, int noffset, const char
>> **algo);
>> int fit_image_hash_get_value(const void *fit, int noffset, uint8_t **value,
>> --
>> 2.25.1
>>
>
> Regards,
> SImon
>
