[PATCH 2/6] image: fit: Add some helpers for getting data

Sean Anderson sean.anderson at seco.com
Mon Mar 28 17:33:46 CEST 2022


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.anderson at 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.anderson at 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
> 


More information about the U-Boot mailing list