| Message ID | 20221018114337.439816-3-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | FWU: Add FWU Multi Bank Update feature support | expand |
Hi Sughosh [...] > + * > + * Read both the metadata copies from the storage media, verify their checksum, > + * and ascertain that both copies match. If one of the copies has gone bad, > + * restore it from the good copy. > + * > + * Return: 0 if OK, -ve on error > + * > + */ > +int fwu_check_mdata_validity(void) > +{ > + int ret; > + struct udevice *dev; > + struct fwu_mdata pri_mdata; > + struct fwu_mdata secondary_mdata; > + uint mdata_parts[2]; > + uint valid_partitions, invalid_partitions; > + > + ret = fwu_get_dev_mdata(&dev, NULL); > + if (ret) > + return ret; > + > + /* > + * Check if the platform has defined its own > + * function to check the metadata partitions' > + * validity. If so, that takes precedence. > + */ > + ret = fwu_mdata_check(dev); Isn't this a bit dangerous? Let's say a device defines it's own check function but for some reason returns -ENOSYS. I am wondering if we should just return 0 if the platform defined functions aren't defined. > + if (!ret || ret != -ENOSYS) > + return ret; > + > + /* > + * Two FWU metadata partitions are expected. > + * If we don't have two, user needs to create > + * them first > + */ > + valid_partitions = 0; > + ret = fwu_get_mdata_part_num(dev, mdata_parts); > + if (ret < 0) { > + log_debug("Error getting the FWU metadata partitions\n"); > + return -ENOENT; > + } > + > + ret = fwu_read_mdata_partition(dev, &pri_mdata, mdata_parts[0]); > + if (!ret) { > + ret = fwu_verify_mdata(&pri_mdata, 1); > + if (!ret) > + valid_partitions |= PRIMARY_PART; > + } > + > + ret = fwu_read_mdata_partition(dev, &secondary_mdata, mdata_parts[1]); > + if (!ret) { > + ret = fwu_verify_mdata(&secondary_mdata, 0); > + if (!ret) > + valid_partitions |= SECONDARY_PART; > + } > + > + if (valid_partitions == (PRIMARY_PART | SECONDARY_PART)) { > + /* > + * Before returning, check that both the > + * FWU metadata copies are the same. If not, > + * the FWU metadata copies need to be > + * re-populated. > + */ > + if (!memcmp(&pri_mdata, &secondary_mdata, > + sizeof(struct fwu_mdata))) { > + ret = 0; > + } else { > + log_info("Both FWU metadata copies are valid but do not match. Please check!\n"); Check what ? Just remove that part please > + ret = -1; > + } > + goto out; > + } > + > + if (!(valid_partitions & BOTH_PARTS)) { > + ret = -1; In general we should try to avoid returning -1 etc. Is there an errno that would make sense? > + goto out; > + } > + > + invalid_partitions = valid_partitions ^ BOTH_PARTS; > + ret = fwu_write_mdata_partition(dev, > + (invalid_partitions == PRIMARY_PART) ? > + &secondary_mdata : &pri_mdata, > + (invalid_partitions == PRIMARY_PART) ? > + mdata_parts[0] : mdata_parts[1]); > + > + if (ret < 0) > + log_info("Restoring %s FWU metadata partition failed\n", > + (invalid_partitions == PRIMARY_PART) ? > + "primary" : "secondary"); > + > +out: > + return ret; > +} > + > +/** > + * fwu_get_active_index() - Get active_index from the FWU metadata > + * @active_idx: active_index value to be read > + * > + * Read the active_index field from the FWU metadata and place it in > + * the variable pointed to be the function argument. > + * > + * Return: 0 if OK, -ve on error > + * > + */ > +int fwu_get_active_index(uint *active_idx) > +{ > + int ret; > + struct udevice *dev; > + struct fwu_mdata mdata = { 0 }; > + > + ret = fwu_get_dev_mdata(&dev, &mdata); > + if (ret) > + return ret; > + > + /* > + * Found the FWU metadata partition, now read the active_index > + * value > + */ > + *active_idx = mdata.active_index; > + if (*active_idx >= CONFIG_FWU_NUM_BANKS) { > + log_debug("Active index value read is incorrect\n"); > + ret = -EINVAL; > + } > + > + return ret; > +} > + > +/** > + * fwu_set_active_index() - Set active_index in the FWU metadata > + * @active_idx: active_index value to be set > + * > + * Update the active_index field in the FWU metadata > + * > + * Return: 0 if OK, -ve on error > + * > + */ > +int fwu_set_active_index(uint active_idx) > +{ > + int ret; > + struct udevice *dev; > + struct fwu_mdata mdata = { 0 }; > + > + if (active_idx >= CONFIG_FWU_NUM_BANKS) { > + log_debug("Invalid active index value\n"); > + return -EINVAL; > + } > + > + ret = fwu_get_dev_mdata(&dev, &mdata); > + if (ret) > + return ret; > + > + /* > + * Update the active index and previous_active_index fields > + * in the FWU metadata > + */ > + mdata.previous_active_index = mdata.active_index; > + mdata.active_index = active_idx; > + > + /* > + * Now write this updated FWU metadata to both the > + * FWU metadata partitions > + */ > + ret = fwu_update_mdata(dev, &mdata); > + if (ret) { > + log_debug("Failed to update FWU metadata partitions\n"); > + ret = -EIO; > + } > + > + return ret; > +} > + > +/** > + * fwu_get_image_index() - Get the Image Index to be used for capsule update > + * @image_index: The Image Index for the image > + * > + * The FWU multi bank update feature computes the value of image_index at > + * runtime, based on the bank to which the image needs to be written to. > + * Derive the image_index value for the image. > + * > + * Currently, the capsule update driver uses the DFU framework for > + * the updates. This function gets the DFU alt number which is to > + * be used as the Image Index > + * > + * Return: 0 if OK, -ve on error > + * > + */ > +int fwu_get_image_index(u8 *image_index) > +{ > + int ret, i; > + u8 alt_num; > + uint update_bank; > + efi_guid_t *image_guid, image_type_id; > + struct udevice *dev; > + struct fwu_mdata mdata = { 0 }; > + struct fwu_image_entry *img_entry; > + struct fwu_image_bank_info *img_bank_info; > + > + ret = fwu_get_dev_mdata(&dev, &mdata); > + if (ret) > + return ret; > + > + ret = fwu_plat_get_update_index(&update_bank); > + if (ret) { > + log_debug("Failed to get the FWU update bank\n"); > + goto out; > + } > + > + ret = fwu_get_image_type_id(image_index, &image_type_id); > + if (ret) { > + log_debug("Unable to get image_type_id for image_index %u\n", > + *image_index); > + goto out; > + } > + > + ret = -EINVAL; > + /* > + * The FWU metadata has been read. Now get the image_uuid for the > + * image with the update_bank. > + */ > + for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) { > + if (!guidcmp(&image_type_id, > + &mdata.img_entry[i].image_type_uuid)) { can we invert the check with a 'continue' here? Will save us one level of indentation > + img_entry = &mdata.img_entry[i]; > + img_bank_info = &img_entry->img_bank_info[update_bank]; > + image_guid = &img_bank_info->image_uuid; > + ret = fwu_plat_get_alt_num(dev, image_guid, &alt_num); > + if (ret) { > + log_debug("alt_num not found for partition with GUID %pUs\n", > + image_guid); > + } else { > + log_debug("alt_num %d for partition %pUs\n", > + alt_num, image_guid); > + *image_index = alt_num + 1; > + } > + > + goto out; > + } > + } > + > + log_debug("Partition with the image type %pUs not found\n", > + &image_type_id); > + > +out: > + return ret; > +} > + > +/** [...] > +static int fwu_clrset_image_accept(efi_guid_t *img_type_id, u32 bank, u8 action) > +{ > + int ret, i; > + struct udevice *dev; > + struct fwu_mdata mdata = { 0 }; > + struct fwu_image_entry *img_entry; > + struct fwu_image_bank_info *img_bank_info; > + > + ret = fwu_get_dev_mdata(&dev, &mdata); > + if (ret) > + return ret; > + > + img_entry = &mdata.img_entry[0]; > + for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) { > + if (!guidcmp(&img_entry[i].image_type_uuid, img_type_id)) { > + img_bank_info = &img_entry[i].img_bank_info[bank]; > + if (action == IMAGE_ACCEPT_SET) > + img_bank_info->accepted |= FWU_IMAGE_ACCEPTED; > + else > + img_bank_info->accepted = 0; > + > + ret = fwu_update_mdata(dev, &mdata); > + goto out; ditto > + } > + } > + > + /* Image not found */ > + ret = -ENOENT; > + > +out: > + return ret; > +} > + > +/** > + * fwu_accept_image() - Set the Acceptance bit for the image > + * @img_type_id: GUID of the image type for which the accepted bit is to be > + * cleared > + * @bank: Bank of which the image's Accept bit is to be set > + * > + * Set the accepted bit for the image specified by the img_guid parameter. This > + * indicates acceptance of image for subsequent boots by some governing component > + * like OS(or firmware). > + * > + * Return: 0 if OK, -ve on error > + * > + */ > +int fwu_accept_image(efi_guid_t *img_type_id, u32 bank) > +{ > + return fwu_clrset_image_accept(img_type_id, bank, > + IMAGE_ACCEPT_SET); > +} > + > +/** > + * fwu_clear_accept_image() - Clear the Acceptance bit for the image > + * @img_type_id: GUID of the image type for which the accepted bit is to be > + * cleared > + * @bank: Bank of which the image's Accept bit is to be cleared > + * > + * Clear the accepted bit for the image type specified by the img_type_id parameter. > + * This function is called after the image has been updated. The accepted bit is > + * cleared to be set subsequently after passing the image acceptance criteria, by > + * either the OS(or firmware) > + * > + * Return: 0 if OK, -ve on error > + * > + */ > +int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank) > +{ > + return fwu_clrset_image_accept(img_type_id, bank, > + IMAGE_ACCEPT_CLEAR); > +} > -- > 2.34.1 > Thanks /Ilias
Hi Sughosh, Ilias, On Wed, 19 Oct 2022 at 21:56, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Sughosh > > > [...] > > > + * > > + * Read both the metadata copies from the storage media, verify their checksum, > > + * and ascertain that both copies match. If one of the copies has gone bad, > > + * restore it from the good copy. > > + * > > + * Return: 0 if OK, -ve on error > > + * > > + */ > > +int fwu_check_mdata_validity(void) > > +{ > > + int ret; > > + struct udevice *dev; > > + struct fwu_mdata pri_mdata; > > + struct fwu_mdata secondary_mdata; > > + uint mdata_parts[2]; > > + uint valid_partitions, invalid_partitions; > > + > > + ret = fwu_get_dev_mdata(&dev, NULL); > > + if (ret) > > + return ret; > > + > > + /* > > + * Check if the platform has defined its own > > + * function to check the metadata partitions' > > + * validity. If so, that takes precedence. > > + */ > > + ret = fwu_mdata_check(dev); > > Isn't this a bit dangerous? Let's say a device defines it's own check > function but for some reason returns -ENOSYS. I am wondering if we should > just return 0 if the platform defined functions aren't defined. > > > + if (!ret || ret != -ENOSYS) > > + return ret; > > + > > + /* > > + * Two FWU metadata partitions are expected. > > + * If we don't have two, user needs to create > > + * them first > > + */ > > + valid_partitions = 0; > > + ret = fwu_get_mdata_part_num(dev, mdata_parts); > > + if (ret < 0) { > > + log_debug("Error getting the FWU metadata partitions\n"); > > + return -ENOENT; > > + } > > + > > + ret = fwu_read_mdata_partition(dev, &pri_mdata, mdata_parts[0]); > > + if (!ret) { > > + ret = fwu_verify_mdata(&pri_mdata, 1); > > + if (!ret) > > + valid_partitions |= PRIMARY_PART; > > + } > > + > > + ret = fwu_read_mdata_partition(dev, &secondary_mdata, mdata_parts[1]); > > + if (!ret) { > > + ret = fwu_verify_mdata(&secondary_mdata, 0); > > + if (!ret) > > + valid_partitions |= SECONDARY_PART; > > + } > > + > > + if (valid_partitions == (PRIMARY_PART | SECONDARY_PART)) { > > + /* > > + * Before returning, check that both the > > + * FWU metadata copies are the same. If not, > > + * the FWU metadata copies need to be > > + * re-populated. > > + */ > > + if (!memcmp(&pri_mdata, &secondary_mdata, > > + sizeof(struct fwu_mdata))) { > > + ret = 0; > > + } else { > > + log_info("Both FWU metadata copies are valid but do not match. Please check!\n"); > > Check what ? Just remove that part please Discussing with Sughosh, here we could update secondary partition with primary content. This would prevent us from bricking an update if a power failure happens before backup (secondary) mdata image is updated. The code that updates mdata content (patch fwu_gpt_update_mdata() in patch 03/15) first updates primary partition, then the secondary. It should be the mandated policy: 1/ ensure secondary content primary content, 2/ update primary 3/ update secondary. So here, we can safely use the primary if content looks consistent. If secondary differs from primary, it means the former was not successfully updated after primary was loaded with new content. br, etienne > > > + ret = -1; > > + } > > + goto out; > > + } > > + > > + if (!(valid_partitions & BOTH_PARTS)) { > > + ret = -1; > > In general we should try to avoid returning -1 etc. Is there an errno that > would make sense? > > > + goto out; > > + } > > + > > + invalid_partitions = valid_partitions ^ BOTH_PARTS; > > + ret = fwu_write_mdata_partition(dev, > > + (invalid_partitions == PRIMARY_PART) ? > > + &secondary_mdata : &pri_mdata, > > + (invalid_partitions == PRIMARY_PART) ? > > + mdata_parts[0] : mdata_parts[1]); > > + > > + if (ret < 0) > > + log_info("Restoring %s FWU metadata partition failed\n", > > + (invalid_partitions == PRIMARY_PART) ? > > + "primary" : "secondary"); > > + > > +out: > > + return ret; > > +} > > + > > +/** > > + * fwu_get_active_index() - Get active_index from the FWU metadata > > + * @active_idx: active_index value to be read > > + * > > + * Read the active_index field from the FWU metadata and place it in > > + * the variable pointed to be the function argument. > > + * > > + * Return: 0 if OK, -ve on error > > + * > > + */ > > +int fwu_get_active_index(uint *active_idx) > > +{ > > + int ret; > > + struct udevice *dev; > > + struct fwu_mdata mdata = { 0 }; > > + > > + ret = fwu_get_dev_mdata(&dev, &mdata); > > + if (ret) > > + return ret; > > + > > + /* > > + * Found the FWU metadata partition, now read the active_index > > + * value > > + */ > > + *active_idx = mdata.active_index; > > + if (*active_idx >= CONFIG_FWU_NUM_BANKS) { > > + log_debug("Active index value read is incorrect\n"); > > + ret = -EINVAL; > > + } > > + > > + return ret; > > +} > > + > > +/** > > + * fwu_set_active_index() - Set active_index in the FWU metadata > > + * @active_idx: active_index value to be set > > + * > > + * Update the active_index field in the FWU metadata > > + * > > + * Return: 0 if OK, -ve on error > > + * > > + */ > > +int fwu_set_active_index(uint active_idx) > > +{ > > + int ret; > > + struct udevice *dev; > > + struct fwu_mdata mdata = { 0 }; > > + > > + if (active_idx >= CONFIG_FWU_NUM_BANKS) { > > + log_debug("Invalid active index value\n"); > > + return -EINVAL; > > + } > > + > > + ret = fwu_get_dev_mdata(&dev, &mdata); > > + if (ret) > > + return ret; > > + > > + /* > > + * Update the active index and previous_active_index fields > > + * in the FWU metadata > > + */ > > + mdata.previous_active_index = mdata.active_index; > > + mdata.active_index = active_idx; > > + > > + /* > > + * Now write this updated FWU metadata to both the > > + * FWU metadata partitions > > + */ > > + ret = fwu_update_mdata(dev, &mdata); > > + if (ret) { > > + log_debug("Failed to update FWU metadata partitions\n"); > > + ret = -EIO; > > + } > > + > > + return ret; > > +} > > + > > +/** > > + * fwu_get_image_index() - Get the Image Index to be used for capsule update > > + * @image_index: The Image Index for the image > > + * > > + * The FWU multi bank update feature computes the value of image_index at > > + * runtime, based on the bank to which the image needs to be written to. > > + * Derive the image_index value for the image. > > + * > > + * Currently, the capsule update driver uses the DFU framework for > > + * the updates. This function gets the DFU alt number which is to > > + * be used as the Image Index > > + * > > + * Return: 0 if OK, -ve on error > > + * > > + */ > > +int fwu_get_image_index(u8 *image_index) > > +{ > > + int ret, i; > > + u8 alt_num; > > + uint update_bank; > > + efi_guid_t *image_guid, image_type_id; > > + struct udevice *dev; > > + struct fwu_mdata mdata = { 0 }; > > + struct fwu_image_entry *img_entry; > > + struct fwu_image_bank_info *img_bank_info; > > + > > + ret = fwu_get_dev_mdata(&dev, &mdata); > > + if (ret) > > + return ret; > > + > > + ret = fwu_plat_get_update_index(&update_bank); > > + if (ret) { > > + log_debug("Failed to get the FWU update bank\n"); > > + goto out; > > + } > > + > > + ret = fwu_get_image_type_id(image_index, &image_type_id); > > + if (ret) { > > + log_debug("Unable to get image_type_id for image_index %u\n", > > + *image_index); > > + goto out; > > + } > > + > > + ret = -EINVAL; > > + /* > > + * The FWU metadata has been read. Now get the image_uuid for the > > + * image with the update_bank. > > + */ > > + for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) { > > + if (!guidcmp(&image_type_id, > > + &mdata.img_entry[i].image_type_uuid)) { > > can we invert the check with a 'continue' here? Will save us one level of > indentation > > > + img_entry = &mdata.img_entry[i]; > > + img_bank_info = &img_entry->img_bank_info[update_bank]; > > + image_guid = &img_bank_info->image_uuid; > > + ret = fwu_plat_get_alt_num(dev, image_guid, &alt_num); > > + if (ret) { > > + log_debug("alt_num not found for partition with GUID %pUs\n", > > + image_guid); > > + } else { > > + log_debug("alt_num %d for partition %pUs\n", > > + alt_num, image_guid); > > + *image_index = alt_num + 1; > > + } > > + > > + goto out; > > + } > > + } > > + > > + log_debug("Partition with the image type %pUs not found\n", > > + &image_type_id); > > + > > +out: > > + return ret; > > +} > > + > > +/** > > [...] > > > +static int fwu_clrset_image_accept(efi_guid_t *img_type_id, u32 bank, u8 action) > > +{ > > + int ret, i; > > + struct udevice *dev; > > + struct fwu_mdata mdata = { 0 }; > > + struct fwu_image_entry *img_entry; > > + struct fwu_image_bank_info *img_bank_info; > > + > > + ret = fwu_get_dev_mdata(&dev, &mdata); > > + if (ret) > > + return ret; > > + > > + img_entry = &mdata.img_entry[0]; > > + for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) { > > + if (!guidcmp(&img_entry[i].image_type_uuid, img_type_id)) { > > + img_bank_info = &img_entry[i].img_bank_info[bank]; > > + if (action == IMAGE_ACCEPT_SET) > > + img_bank_info->accepted |= FWU_IMAGE_ACCEPTED; > > + else > > + img_bank_info->accepted = 0; > > + > > + ret = fwu_update_mdata(dev, &mdata); > > + goto out; > > ditto > > > + } > > + } > > + > > + /* Image not found */ > > + ret = -ENOENT; > > + > > +out: > > + return ret; > > +} > > + > > +/** > > + * fwu_accept_image() - Set the Acceptance bit for the image > > + * @img_type_id: GUID of the image type for which the accepted bit is to be > > + * cleared > > + * @bank: Bank of which the image's Accept bit is to be set > > + * > > + * Set the accepted bit for the image specified by the img_guid parameter. This > > + * indicates acceptance of image for subsequent boots by some governing component > > + * like OS(or firmware). > > + * > > + * Return: 0 if OK, -ve on error > > + * > > + */ > > +int fwu_accept_image(efi_guid_t *img_type_id, u32 bank) > > +{ > > + return fwu_clrset_image_accept(img_type_id, bank, > > + IMAGE_ACCEPT_SET); > > +} > > + > > +/** > > + * fwu_clear_accept_image() - Clear the Acceptance bit for the image > > + * @img_type_id: GUID of the image type for which the accepted bit is to be > > + * cleared > > + * @bank: Bank of which the image's Accept bit is to be cleared > > + * > > + * Clear the accepted bit for the image type specified by the img_type_id parameter. > > + * This function is called after the image has been updated. The accepted bit is > > + * cleared to be set subsequently after passing the image acceptance criteria, by > > + * either the OS(or firmware) > > + * > > + * Return: 0 if OK, -ve on error > > + * > > + */ > > +int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank) > > +{ > > + return fwu_clrset_image_accept(img_type_id, bank, > > + IMAGE_ACCEPT_CLEAR); > > +} > > -- > > 2.34.1 > > > > Thanks > /Ilias
hi Ilias, On Thu, 20 Oct 2022 at 01:26, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Sughosh > > > [...] > > > + * > > + * Read both the metadata copies from the storage media, verify their checksum, > > + * and ascertain that both copies match. If one of the copies has gone bad, > > + * restore it from the good copy. > > + * > > + * Return: 0 if OK, -ve on error > > + * > > + */ > > +int fwu_check_mdata_validity(void) > > +{ > > + int ret; > > + struct udevice *dev; > > + struct fwu_mdata pri_mdata; > > + struct fwu_mdata secondary_mdata; > > + uint mdata_parts[2]; > > + uint valid_partitions, invalid_partitions; > > + > > + ret = fwu_get_dev_mdata(&dev, NULL); > > + if (ret) > > + return ret; > > + > > + /* > > + * Check if the platform has defined its own > > + * function to check the metadata partitions' > > + * validity. If so, that takes precedence. > > + */ > > + ret = fwu_mdata_check(dev); > > Isn't this a bit dangerous? Let's say a device defines it's own check > function but for some reason returns -ENOSYS. I am wondering if we should > just return 0 if the platform defined functions aren't defined. A driver should return ENOSYS only if a driver method has not been defined. That is what I see being used in other drivers as well. So I think that the current implementation of returning ENOSYS for a non defined method is correct. If a driver is returning ENOSYS for any other purpose I feel that should be fixed instead. > > > + if (!ret || ret != -ENOSYS) > > + return ret; > > + > > + /* > > + * Two FWU metadata partitions are expected. > > + * If we don't have two, user needs to create > > + * them first > > + */ > > + valid_partitions = 0; > > + ret = fwu_get_mdata_part_num(dev, mdata_parts); > > + if (ret < 0) { > > + log_debug("Error getting the FWU metadata partitions\n"); > > + return -ENOENT; > > + } > > + > > + ret = fwu_read_mdata_partition(dev, &pri_mdata, mdata_parts[0]); > > + if (!ret) { > > + ret = fwu_verify_mdata(&pri_mdata, 1); > > + if (!ret) > > + valid_partitions |= PRIMARY_PART; > > + } > > + > > + ret = fwu_read_mdata_partition(dev, &secondary_mdata, mdata_parts[1]); > > + if (!ret) { > > + ret = fwu_verify_mdata(&secondary_mdata, 0); > > + if (!ret) > > + valid_partitions |= SECONDARY_PART; > > + } > > + > > + if (valid_partitions == (PRIMARY_PART | SECONDARY_PART)) { > > + /* > > + * Before returning, check that both the > > + * FWU metadata copies are the same. If not, > > + * the FWU metadata copies need to be > > + * re-populated. > > + */ > > + if (!memcmp(&pri_mdata, &secondary_mdata, > > + sizeof(struct fwu_mdata))) { > > + ret = 0; > > + } else { > > + log_info("Both FWU metadata copies are valid but do not match. Please check!\n"); > > Check what ? Just remove that part please Okay. I will restore the secondary partition from the primary partition as Etienne has suggested. > > > + ret = -1; > > + } > > + goto out; > > + } > > + > > + if (!(valid_partitions & BOTH_PARTS)) { > > + ret = -1; > > In general we should try to avoid returning -1 etc. Is there an errno that > would make sense? Even I am not sure what is relevant here(EINVAL?). I think I can add a log_info mentioning the error case? > > > + goto out; > > + } > > + > > + invalid_partitions = valid_partitions ^ BOTH_PARTS; > > + ret = fwu_write_mdata_partition(dev, > > + (invalid_partitions == PRIMARY_PART) ? > > + &secondary_mdata : &pri_mdata, > > + (invalid_partitions == PRIMARY_PART) ? > > + mdata_parts[0] : mdata_parts[1]); > > + > > + if (ret < 0) > > + log_info("Restoring %s FWU metadata partition failed\n", > > + (invalid_partitions == PRIMARY_PART) ? > > + "primary" : "secondary"); > > + > > +out: > > + return ret; > > +} > > + > > +/** > > + * fwu_get_active_index() - Get active_index from the FWU metadata > > + * @active_idx: active_index value to be read > > + * > > + * Read the active_index field from the FWU metadata and place it in > > + * the variable pointed to be the function argument. > > + * > > + * Return: 0 if OK, -ve on error > > + * > > + */ > > +int fwu_get_active_index(uint *active_idx) > > +{ > > + int ret; > > + struct udevice *dev; > > + struct fwu_mdata mdata = { 0 }; > > + > > + ret = fwu_get_dev_mdata(&dev, &mdata); > > + if (ret) > > + return ret; > > + > > + /* > > + * Found the FWU metadata partition, now read the active_index > > + * value > > + */ > > + *active_idx = mdata.active_index; > > + if (*active_idx >= CONFIG_FWU_NUM_BANKS) { > > + log_debug("Active index value read is incorrect\n"); > > + ret = -EINVAL; > > + } > > + > > + return ret; > > +} > > + > > +/** > > + * fwu_set_active_index() - Set active_index in the FWU metadata > > + * @active_idx: active_index value to be set > > + * > > + * Update the active_index field in the FWU metadata > > + * > > + * Return: 0 if OK, -ve on error > > + * > > + */ > > +int fwu_set_active_index(uint active_idx) > > +{ > > + int ret; > > + struct udevice *dev; > > + struct fwu_mdata mdata = { 0 }; > > + > > + if (active_idx >= CONFIG_FWU_NUM_BANKS) { > > + log_debug("Invalid active index value\n"); > > + return -EINVAL; > > + } > > + > > + ret = fwu_get_dev_mdata(&dev, &mdata); > > + if (ret) > > + return ret; > > + > > + /* > > + * Update the active index and previous_active_index fields > > + * in the FWU metadata > > + */ > > + mdata.previous_active_index = mdata.active_index; > > + mdata.active_index = active_idx; > > + > > + /* > > + * Now write this updated FWU metadata to both the > > + * FWU metadata partitions > > + */ > > + ret = fwu_update_mdata(dev, &mdata); > > + if (ret) { > > + log_debug("Failed to update FWU metadata partitions\n"); > > + ret = -EIO; > > + } > > + > > + return ret; > > +} > > + > > +/** > > + * fwu_get_image_index() - Get the Image Index to be used for capsule update > > + * @image_index: The Image Index for the image > > + * > > + * The FWU multi bank update feature computes the value of image_index at > > + * runtime, based on the bank to which the image needs to be written to. > > + * Derive the image_index value for the image. > > + * > > + * Currently, the capsule update driver uses the DFU framework for > > + * the updates. This function gets the DFU alt number which is to > > + * be used as the Image Index > > + * > > + * Return: 0 if OK, -ve on error > > + * > > + */ > > +int fwu_get_image_index(u8 *image_index) > > +{ > > + int ret, i; > > + u8 alt_num; > > + uint update_bank; > > + efi_guid_t *image_guid, image_type_id; > > + struct udevice *dev; > > + struct fwu_mdata mdata = { 0 }; > > + struct fwu_image_entry *img_entry; > > + struct fwu_image_bank_info *img_bank_info; > > + > > + ret = fwu_get_dev_mdata(&dev, &mdata); > > + if (ret) > > + return ret; > > + > > + ret = fwu_plat_get_update_index(&update_bank); > > + if (ret) { > > + log_debug("Failed to get the FWU update bank\n"); > > + goto out; > > + } > > + > > + ret = fwu_get_image_type_id(image_index, &image_type_id); > > + if (ret) { > > + log_debug("Unable to get image_type_id for image_index %u\n", > > + *image_index); > > + goto out; > > + } > > + > > + ret = -EINVAL; > > + /* > > + * The FWU metadata has been read. Now get the image_uuid for the > > + * image with the update_bank. > > + */ > > + for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) { > > + if (!guidcmp(&image_type_id, > > + &mdata.img_entry[i].image_type_uuid)) { > > can we invert the check with a 'continue' here? Will save us one level of > indentation Hmm, yeah. I know it's subjective, but I personally find the current code flow easier to follow. Moreover, the current code is not breaking the 80 column rule. If you don't have a strong opinion about this, I will prefer to keep it this way. > > > + img_entry = &mdata.img_entry[i]; > > + img_bank_info = &img_entry->img_bank_info[update_bank]; > > + image_guid = &img_bank_info->image_uuid; > > + ret = fwu_plat_get_alt_num(dev, image_guid, &alt_num); > > + if (ret) { > > + log_debug("alt_num not found for partition with GUID %pUs\n", > > + image_guid); > > + } else { > > + log_debug("alt_num %d for partition %pUs\n", > > + alt_num, image_guid); > > + *image_index = alt_num + 1; > > + } > > + > > + goto out; > > + } > > + } > > + > > + log_debug("Partition with the image type %pUs not found\n", > > + &image_type_id); > > + > > +out: > > + return ret; > > +} > > + > > +/** > > [...] > > > +static int fwu_clrset_image_accept(efi_guid_t *img_type_id, u32 bank, u8 action) > > +{ > > + int ret, i; > > + struct udevice *dev; > > + struct fwu_mdata mdata = { 0 }; > > + struct fwu_image_entry *img_entry; > > + struct fwu_image_bank_info *img_bank_info; > > + > > + ret = fwu_get_dev_mdata(&dev, &mdata); > > + if (ret) > > + return ret; > > + > > + img_entry = &mdata.img_entry[0]; > > + for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) { > > + if (!guidcmp(&img_entry[i].image_type_uuid, img_type_id)) { > > + img_bank_info = &img_entry[i].img_bank_info[bank]; > > + if (action == IMAGE_ACCEPT_SET) > > + img_bank_info->accepted |= FWU_IMAGE_ACCEPTED; > > + else > > + img_bank_info->accepted = 0; > > + > > + ret = fwu_update_mdata(dev, &mdata); > > + goto out; > > ditto Same as above. Thanks. -sughosh > > > + } > > + } > > + > > + /* Image not found */ > > + ret = -ENOENT; > > + > > +out: > > + return ret; > > +} > > + > > +/** > > + * fwu_accept_image() - Set the Acceptance bit for the image > > + * @img_type_id: GUID of the image type for which the accepted bit is to be > > + * cleared > > + * @bank: Bank of which the image's Accept bit is to be set > > + * > > + * Set the accepted bit for the image specified by the img_guid parameter. This > > + * indicates acceptance of image for subsequent boots by some governing component > > + * like OS(or firmware). > > + * > > + * Return: 0 if OK, -ve on error > > + * > > + */ > > +int fwu_accept_image(efi_guid_t *img_type_id, u32 bank) > > +{ > > + return fwu_clrset_image_accept(img_type_id, bank, > > + IMAGE_ACCEPT_SET); > > +} > > + > > +/** > > + * fwu_clear_accept_image() - Clear the Acceptance bit for the image > > + * @img_type_id: GUID of the image type for which the accepted bit is to be > > + * cleared > > + * @bank: Bank of which the image's Accept bit is to be cleared > > + * > > + * Clear the accepted bit for the image type specified by the img_type_id parameter. > > + * This function is called after the image has been updated. The accepted bit is > > + * cleared to be set subsequently after passing the image acceptance criteria, by > > + * either the OS(or firmware) > > + * > > + * Return: 0 if OK, -ve on error > > + * > > + */ > > +int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank) > > +{ > > + return fwu_clrset_image_accept(img_type_id, bank, > > + IMAGE_ACCEPT_CLEAR); > > +} > > -- > > 2.34.1 > > > > Thanks > /Ilias
Hi Sughosh, Etienne [...] > > > + * Check if the platform has defined its own > > > + * function to check the metadata partitions' > > > + * validity. If so, that takes precedence. > > > + */ > > > + ret = fwu_mdata_check(dev); > > > > Isn't this a bit dangerous? Let's say a device defines it's own check > > function but for some reason returns -ENOSYS. I am wondering if we should > > just return 0 if the platform defined functions aren't defined. > > A driver should return ENOSYS only if a driver method has not been > defined. That is what I see being used in other drivers as well. So I > think that the current implementation of returning ENOSYS for a non > defined method is correct. If a driver is returning ENOSYS for any > other purpose I feel that should be fixed instead. > Fair enough > > > > > + if (!ret || ret != -ENOSYS) > > > + return ret; > > > + > > > + /* > > > + * Two FWU metadata partitions are expected. > > > + * If we don't have two, user needs to create > > > + * them first > > > + */ > > > + valid_partitions = 0; > > > + ret = fwu_get_mdata_part_num(dev, mdata_parts); > > > + if (ret < 0) { > > > + log_debug("Error getting the FWU metadata partitions\n"); > > > + return -ENOENT; > > > + } > > > + > > > + ret = fwu_read_mdata_partition(dev, &pri_mdata, mdata_parts[0]); > > > + if (!ret) { > > > + ret = fwu_verify_mdata(&pri_mdata, 1); > > > + if (!ret) > > > + valid_partitions |= PRIMARY_PART; > > > + } > > > + > > > + ret = fwu_read_mdata_partition(dev, &secondary_mdata, mdata_parts[1]); > > > + if (!ret) { > > > + ret = fwu_verify_mdata(&secondary_mdata, 0); > > > + if (!ret) > > > + valid_partitions |= SECONDARY_PART; > > > + } > > > + > > > + if (valid_partitions == (PRIMARY_PART | SECONDARY_PART)) { > > > + /* > > > + * Before returning, check that both the > > > + * FWU metadata copies are the same. If not, > > > + * the FWU metadata copies need to be > > > + * re-populated. > > > + */ > > > + if (!memcmp(&pri_mdata, &secondary_mdata, > > > + sizeof(struct fwu_mdata))) { > > > + ret = 0; > > > + } else { > > > + log_info("Both FWU metadata copies are valid but do not match. Please check!\n"); > > > > Check what ? Just remove that part please > > Okay. I will restore the secondary partition from the primary > partition as Etienne has suggested. Yep, I agree on that. I vaguely remember discussing the same thing a few versions ago, but that somehow slipped away. Good catch! > > > > > > + ret = -1; > > > + } > > > + goto out; > > > + } > > > + > > > + if (!(valid_partitions & BOTH_PARTS)) { > > > + ret = -1; > > > > In general we should try to avoid returning -1 etc. Is there an errno that > > would make sense? > > Even I am not sure what is relevant here(EINVAL?). I think I can add a > log_info mentioning the error case? EBADMSG could also be an option here I guess, but there's not POSIX errnor I am aware of that means "data corrupted" > > > > > > + goto out; > > > + } > > > + > > > + invalid_partitions = valid_partitions ^ BOTH_PARTS; > > > + ret = fwu_write_mdata_partition(dev, > > > + (invalid_partitions == PRIMARY_PART) ? > > > + &secondary_mdata : &pri_mdata, > > > + (invalid_partitions == PRIMARY_PART) ? > > > + mdata_parts[0] : mdata_parts[1]); > > > + > > > + if (ret < 0) > > > + log_info("Restoring %s FWU metadata partition failed\n", > > > + (invalid_partitions == PRIMARY_PART) ? > > > + "primary" : "secondary"); > > > + > > > +out: > > > + return ret; > > > +} > > > + > > > +/** > > > + * fwu_get_active_index() - Get active_index from the FWU metadata > > > + * @active_idx: active_index value to be read > > > + * > > > + * Read the active_index field from the FWU metadata and place it in > > > + * the variable pointed to be the function argument. > > > + * > > > + * Return: 0 if OK, -ve on error > > > + * > > > + */ > > > +int fwu_get_active_index(uint *active_idx) > > > +{ > > > + int ret; > > > + struct udevice *dev; > > > + struct fwu_mdata mdata = { 0 }; > > > + > > > + ret = fwu_get_dev_mdata(&dev, &mdata); > > > + if (ret) > > > + return ret; > > > + > > > + /* > > > + * Found the FWU metadata partition, now read the active_index > > > + * value > > > + */ > > > + *active_idx = mdata.active_index; > > > + if (*active_idx >= CONFIG_FWU_NUM_BANKS) { > > > + log_debug("Active index value read is incorrect\n"); > > > + ret = -EINVAL; > > > + } > > > + > > > + return ret; > > > +} > > > + > > > +/** > > > + * fwu_set_active_index() - Set active_index in the FWU metadata > > > + * @active_idx: active_index value to be set > > > + * > > > + * Update the active_index field in the FWU metadata > > > + * > > > + * Return: 0 if OK, -ve on error > > > + * > > > + */ > > > +int fwu_set_active_index(uint active_idx) > > > +{ > > > + int ret; > > > + struct udevice *dev; > > > + struct fwu_mdata mdata = { 0 }; > > > + > > > + if (active_idx >= CONFIG_FWU_NUM_BANKS) { > > > + log_debug("Invalid active index value\n"); > > > + return -EINVAL; > > > + } > > > + > > > + ret = fwu_get_dev_mdata(&dev, &mdata); > > > + if (ret) > > > + return ret; > > > + > > > + /* > > > + * Update the active index and previous_active_index fields > > > + * in the FWU metadata > > > + */ > > > + mdata.previous_active_index = mdata.active_index; > > > + mdata.active_index = active_idx; > > > + > > > + /* > > > + * Now write this updated FWU metadata to both the > > > + * FWU metadata partitions > > > + */ > > > + ret = fwu_update_mdata(dev, &mdata); > > > + if (ret) { > > > + log_debug("Failed to update FWU metadata partitions\n"); > > > + ret = -EIO; > > > + } > > > + > > > + return ret; > > > +} > > > + > > > +/** > > > + * fwu_get_image_index() - Get the Image Index to be used for capsule update > > > + * @image_index: The Image Index for the image > > > + * > > > + * The FWU multi bank update feature computes the value of image_index at > > > + * runtime, based on the bank to which the image needs to be written to. > > > + * Derive the image_index value for the image. > > > + * > > > + * Currently, the capsule update driver uses the DFU framework for > > > + * the updates. This function gets the DFU alt number which is to > > > + * be used as the Image Index > > > + * > > > + * Return: 0 if OK, -ve on error > > > + * > > > + */ > > > +int fwu_get_image_index(u8 *image_index) > > > +{ > > > + int ret, i; > > > + u8 alt_num; > > > + uint update_bank; > > > + efi_guid_t *image_guid, image_type_id; > > > + struct udevice *dev; > > > + struct fwu_mdata mdata = { 0 }; > > > + struct fwu_image_entry *img_entry; > > > + struct fwu_image_bank_info *img_bank_info; > > > + > > > + ret = fwu_get_dev_mdata(&dev, &mdata); > > > + if (ret) > > > + return ret; > > > + > > > + ret = fwu_plat_get_update_index(&update_bank); > > > + if (ret) { > > > + log_debug("Failed to get the FWU update bank\n"); > > > + goto out; > > > + } > > > + > > > + ret = fwu_get_image_type_id(image_index, &image_type_id); > > > + if (ret) { > > > + log_debug("Unable to get image_type_id for image_index %u\n", > > > + *image_index); > > > + goto out; > > > + } > > > + > > > + ret = -EINVAL; > > > + /* > > > + * The FWU metadata has been read. Now get the image_uuid for the > > > + * image with the update_bank. > > > + */ > > > + for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) { > > > + if (!guidcmp(&image_type_id, > > > + &mdata.img_entry[i].image_type_uuid)) { > > > > can we invert the check with a 'continue' here? Will save us one level of > > indentation > > Hmm, yeah. I know it's subjective, but I personally find the current > code flow easier to follow. Moreover, the current code is not breaking > the 80 column rule. If you don't have a strong opinion about this, I > will prefer to keep it this way. > > > > > > + img_entry = &mdata.img_entry[i]; > > > + img_bank_info = &img_entry->img_bank_info[update_bank]; [...] Thanks /Ilias
diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c b/drivers/fwu-mdata/fwu-mdata-uclass.c new file mode 100644 index 0000000000..fb412b2a9b --- /dev/null +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c @@ -0,0 +1,178 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2022, Linaro Limited + */ + +#define LOG_CATEGORY UCLASS_FWU_MDATA + +#include <common.h> +#include <dm.h> +#include <efi_loader.h> +#include <fwu.h> +#include <fwu_mdata.h> +#include <log.h> + +#include <linux/errno.h> +#include <linux/types.h> +#include <u-boot/crc.h> + +/** + * fwu_get_mdata_part_num() - Get the FWU metadata partition numbers + * @dev: FWU metadata device + * @mdata_parts: array for storing the metadata partition numbers + * + * Get the partition numbers on the storage device on which the + * FWU metadata is stored. Two partition numbers will be returned. + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_get_mdata_part_num(struct udevice *dev, uint *mdata_parts) +{ + const struct fwu_mdata_ops *ops = device_get_ops(dev); + + if (!ops->get_mdata_part_num) { + log_debug("get_mdata_part_num() method not defined\n"); + return -ENOSYS; + } + + return ops->get_mdata_part_num(dev, mdata_parts); +} + +/** + * fwu_read_mdata_partition() - Read the FWU metadata from a partition + * @dev: FWU metadata device + * @mdata: Copy of the FWU metadata + * @part_num: Partition number from which FWU metadata is to be read + * + * Read the FWU metadata from the specified partition number + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_read_mdata_partition(struct udevice *dev, struct fwu_mdata *mdata, + uint part_num) +{ + const struct fwu_mdata_ops *ops = device_get_ops(dev); + + if (!ops->read_mdata_partition) { + log_debug("read_mdata_partition() method not defined\n"); + return -ENOSYS; + } + + return ops->read_mdata_partition(dev, mdata, part_num); +} + +/** + * fwu_write_mdata_partition() - Write the FWU metadata to a partition + * @dev: FWU metadata device + * @mdata: Copy of the FWU metadata + * @part_num: Partition number to which FWU metadata is to be written + * + * Write the FWU metadata to the specified partition number + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_write_mdata_partition(struct udevice *dev, struct fwu_mdata *mdata, + uint part_num) +{ + const struct fwu_mdata_ops *ops = device_get_ops(dev); + + if (!ops->write_mdata_partition) { + log_debug("write_mdata_partition() method not defined\n"); + return -ENOSYS; + } + + return ops->write_mdata_partition(dev, mdata, part_num); +} + +/** + * fwu_mdata_check() - Check if the FWU metadata is valid + * @dev: FWU metadata device + * + * Validate both copies of the FWU metadata. If one of the copies + * has gone bad, restore it from the other copy. + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_mdata_check(struct udevice *dev) +{ + const struct fwu_mdata_ops *ops = device_get_ops(dev); + + if (!ops->check_mdata) { + log_debug("check_mdata() method not defined\n"); + return -ENOSYS; + } + + return ops->check_mdata(dev); +} + +/** + * fwu_get_mdata() - Get a FWU metadata copy + * @dev: FWU metadata device + * @mdata: Copy of the FWU metadata + * + * Get a valid copy of the FWU metadata. + * + * Note: This function is to be called first when modifying any fields + * in the metadata. The sequence of calls to modify any field in the + * metadata would be 1) fwu_get_mdata 2) Modify metadata, followed by + * 3) fwu_update_mdata + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_get_mdata(struct udevice *dev, struct fwu_mdata *mdata) +{ + const struct fwu_mdata_ops *ops = device_get_ops(dev); + + if (!ops->get_mdata) { + log_debug("get_mdata() method not defined\n"); + return -ENOSYS; + } + + return ops->get_mdata(dev, mdata); +} + +/** + * fwu_update_mdata() - Update the FWU metadata + * @dev: FWU metadata device + * @mdata: Copy of the FWU metadata + * + * Update the FWU metadata structure by writing to the + * FWU metadata partitions. + * + * Note: This function is not to be called directly to update the + * metadata fields. The sequence of function calls should be + * 1) fwu_get_mdata() 2) Modify the medata fields 3) fwu_update_mdata() + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_update_mdata(struct udevice *dev, struct fwu_mdata *mdata) +{ + void *buf; + const struct fwu_mdata_ops *ops = device_get_ops(dev); + + if (!ops->update_mdata) { + log_debug("get_mdata() method not defined\n"); + return -ENOSYS; + } + + /* + * Calculate the crc32 for the updated FWU metadata + * and put the updated value in the FWU metadata crc32 + * field + */ + buf = &mdata->version; + mdata->crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32)); + + return ops->update_mdata(dev, mdata); +} + +UCLASS_DRIVER(fwu_mdata) = { + .id = UCLASS_FWU_MDATA, + .name = "fwu-mdata", +}; diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index c2b15881ba..85fda04f2a 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -59,6 +59,7 @@ enum uclass_id { UCLASS_FPGA, /* FPGA device */ UCLASS_FUZZING_ENGINE, /* Fuzzing engine */ UCLASS_FS_FIRMWARE_LOADER, /* Generic loader */ + UCLASS_FWU_MDATA, /* FWU Metadata Access */ UCLASS_GPIO, /* Bank of general-purpose I/O pins */ UCLASS_HASH, /* Hash device */ UCLASS_HWSPINLOCK, /* Hardware semaphores */ diff --git a/include/fwu.h b/include/fwu.h new file mode 100644 index 0000000000..84c4ffacee --- /dev/null +++ b/include/fwu.h @@ -0,0 +1,303 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (c) 2022, Linaro Limited + */ + +#if !defined _FWU_H_ +#define _FWU_H_ + +#include <blk.h> +#include <efi.h> + +#include <linux/types.h> + +struct fwu_mdata; +struct udevice; + +/** + * @mdata_check: check the validity of the FWU metadata partitions + * @get_mdata() - Get a FWU metadata copy + * @update_mdata() - Update the FWU metadata copy + */ +struct fwu_mdata_ops { + /** + * check_mdata() - Check if the FWU metadata is valid + * @dev: FWU device + * + * Validate both copies of the FWU metadata. If one of the copies + * has gone bad, restore it from the other copy. + * + * Return: 0 if OK, -ve on error + */ + int (*check_mdata)(struct udevice *dev); + + /** + * get_mdata() - Get a FWU metadata copy + * @dev: FWU device + * @mdata: Pointer to FWU metadata + * + * Get a valid copy of the FWU metadata. + * + * Return: 0 if OK, -ve on error + */ + int (*get_mdata)(struct udevice *dev, struct fwu_mdata *mdata); + + /** + * update_mdata() - Update the FWU metadata + * @dev: FWU device + * @mdata: Copy of the FWU metadata + * + * Update the FWU metadata structure by writing to the + * FWU metadata partitions. + * + * Return: 0 if OK, -ve on error + */ + int (*update_mdata)(struct udevice *dev, struct fwu_mdata *mdata); + + /** + * get_mdata_part_num() - Get the FWU metadata partition numbers + * @dev: FWU metadata device + * @mdata_parts: array for storing the metadata partition numbers + * + * Get the partition numbers on the storage device on which the + * FWU metadata is stored. Two partition numbers will be returned. + * + * Return: 0 if OK, -ve on error + */ + int (*get_mdata_part_num)(struct udevice *dev, uint *mdata_parts); + + /** + * read_mdata_partition() - Read the FWU metadata from a partition + * @dev: FWU metadata device + * @mdata: Copy of the FWU metadata + * @part_num: Partition number from which FWU metadata is to be read + * + * Read the FWU metadata from the specified partition number + * + * Return: 0 if OK, -ve on error + */ + int (*read_mdata_partition)(struct udevice *dev, + struct fwu_mdata *mdata, uint part_num); + + /** + * write_mdata_partition() - Write the FWU metadata to a partition + * @dev: FWU metadata device + * @mdata: Copy of the FWU metadata + * @part_num: Partition number to which FWU metadata is to be written + * + * Write the FWU metadata to the specified partition number + * + * Return: 0 if OK, -ve on error + */ + int (*write_mdata_partition)(struct udevice *dev, + struct fwu_mdata *mdata, uint part_num); +}; + +#define FWU_MDATA_VERSION 0x1 + +/* +* GUID value defined in the FWU specification for identification +* of the FWU metadata partition. +*/ +#define FWU_MDATA_GUID \ + EFI_GUID(0x8a7a84a0, 0x8387, 0x40f6, 0xab, 0x41, \ + 0xa8, 0xb9, 0xa5, 0xa6, 0x0d, 0x23) + +/** + * fwu_check_mdata_validity() - Check for validity of the FWU metadata copies + * + * Read both the metadata copies from the storage media, verify their + * checksum, and ascertain that both copies match. If one of the copies + * has gone bad, restore it from the good copy. + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_check_mdata_validity(void); + +/** + * fwu_get_mdata_part_num() - Get the FWU metadata partition numbers + * @dev: FWU metadata device + * @mdata_parts: array for storing the metadata partition numbers + * + * Get the partition numbers on the storage device on which the + * FWU metadata is stored. Two partition numbers will be returned + * through the array. + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_get_mdata_part_num(struct udevice *dev, uint *mdata_parts); + +/** + * fwu_read_mdata_partition() - Read the FWU metadata from a partition + * @dev: FWU metadata device + * @mdata: Copy of the FWU metadata + * @part_num: Partition number from which FWU metadata is to be read + * + * Read the FWU metadata from the specified partition number + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_read_mdata_partition(struct udevice *dev, struct fwu_mdata *mdata, + uint part_num); + +/** + * fwu_write_mdata_partition() - Write the FWU metadata to a partition + * @dev: FWU metadata device + * @mdata: Copy of the FWU metadata + * @part_num: Partition number to which FWU metadata is to be written + * + * Write the FWU metadata to the specified partition number + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_write_mdata_partition(struct udevice *dev, struct fwu_mdata *mdata, + uint part_num); + +/** + * fwu_get_mdata() - Get a FWU metadata copy + * @dev: FWU metadata device + * @mdata: Copy of the FWU metadata + * + * Get a valid copy of the FWU metadata. + * + * Note: This function is to be called first when modifying any fields + * in the metadata. The sequence of calls to modify any field in the + * metadata would be 1) fwu_get_mdata 2) Modify metadata, followed by + * 3) fwu_update_mdata + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_get_mdata(struct udevice *dev, struct fwu_mdata *mdata); + +/** + * fwu_update_mdata() - Update the FWU metadata + * @dev: FWU metadata device + * @mdata: Copy of the FWU metadata + * + * Update the FWU metadata structure by writing to the + * FWU metadata partitions. + * + * Note: This function is not to be called directly to update the + * metadata fields. The sequence of function calls should be + * 1) fwu_get_mdata() 2) Modify the medata fields 3) fwu_update_mdata() + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_update_mdata(struct udevice *dev, struct fwu_mdata *mdata); + +/** + * fwu_get_active_index() - Get active_index from the FWU metadata + * @active_idxp: active_index value to be read + * + * Read the active_index field from the FWU metadata and place it in + * the variable pointed to be the function argument. + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_get_active_index(uint *active_idxp); + +/** + * fwu_set_active_index() - Set active_index in the FWU metadata + * @active_idx: active_index value to be set + * + * Update the active_index field in the FWU metadata + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_set_active_index(uint active_idx); + +/** + * fwu_get_image_index() - Get the Image Index to be used for capsule update + * @image_index: The Image Index for the image + * + * The FWU multi bank update feature computes the value of image_index at + * runtime, based on the bank to which the image needs to be written to. + * Derive the image_index value for the image. + * + * Currently, the capsule update driver uses the DFU framework for + * the updates. This function gets the DFU alt number which is to + * be used as the Image Index + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_get_image_index(u8 *image_index); + +/** + * fwu_mdata_check() - Check if the FWU metadata is valid + * @dev: FWU metadata device + * + * Validate both copies of the FWU metadata. If one of the copies + * has gone bad, restore it from the other copy. + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_mdata_check(struct udevice *dev); + +/** + * fwu_revert_boot_index() - Revert the active index in the FWU metadata + * + * Revert the active_index value in the FWU metadata, by swapping the values + * of active_index and previous_active_index in both copies of the + * FWU metadata. + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_revert_boot_index(void); + +/** + * fwu_verify_mdata() - Verify the FWU metadata + * @mdata: FWU metadata structure + * @pri_part: FWU metadata partition is primary or secondary + * + * Verify the FWU metadata by computing the CRC32 for the metadata + * structure and comparing it against the CRC32 value stored as part + * of the structure. + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part); + +/** + * fwu_accept_image() - Set the Acceptance bit for the image + * @img_type_id: GUID of the image type for which the accepted bit is to be + * cleared + * @bank: Bank of which the image's Accept bit is to be set + * + * Set the accepted bit for the image specified by the img_guid parameter. This + * indicates acceptance of image for subsequent boots by some governing component + * like OS(or firmware). + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_accept_image(efi_guid_t *img_type_id, u32 bank); + +/** + * fwu_clear_accept_image() - Clear the Acceptance bit for the image + * @img_type_id: GUID of the image type for which the accepted bit is to be + * cleared + * @bank: Bank of which the image's Accept bit is to be cleared + * + * Clear the accepted bit for the image type specified by the img_type_id parameter. + * This function is called after the image has been updated. The accepted bit is + * cleared to be set subsequently after passing the image acceptance criteria, by + * either the OS(or firmware) + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank); + +#endif /* _FWU_H_ */ diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h new file mode 100644 index 0000000000..8fda4f4ac2 --- /dev/null +++ b/include/fwu_mdata.h @@ -0,0 +1,67 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (c) 2022, Linaro Limited + */ + +#if !defined _FWU_MDATA_H_ +#define _FWU_MDATA_H_ + +#include <efi.h> + +/** + * struct fwu_image_bank_info - firmware image information + * @image_uuid: Guid value of the image in this bank + * @accepted: Acceptance status of the image + * @reserved: Reserved + * + * The structure contains image specific fields which are + * used to identify the image and to specify the image's + * acceptance status + */ +struct fwu_image_bank_info { + efi_guid_t image_uuid; + uint32_t accepted; + uint32_t reserved; +}; + +/** + * struct fwu_image_entry - information for a particular type of image + * @image_type_uuid: Guid value for identifying the image type + * @location_uuid: Guid of the storage volume where the image is located + * @img_bank_info: Array containing properties of images + * + * This structure contains information on various types of updatable + * firmware images. Each image type then contains an array of image + * information per bank. + */ +struct fwu_image_entry { + efi_guid_t image_type_uuid; + efi_guid_t location_uuid; + struct fwu_image_bank_info img_bank_info[CONFIG_FWU_NUM_BANKS]; +}; + +/** + * struct fwu_mdata - FWU metadata structure for multi-bank updates + * @crc32: crc32 value for the FWU metadata + * @version: FWU metadata version + * @active_index: Index of the bank currently used for booting images + * @previous_active_inde: Index of the bank used before the current bank + * being used for booting + * @img_entry: Array of information on various firmware images that can + * be updated + * + * This structure is used to store all the needed information for performing + * multi bank updates on the platform. This contains info on the bank being + * used to boot along with the information needed for identification of + * individual images + */ +struct fwu_mdata { + uint32_t crc32; + uint32_t version; + uint32_t active_index; + uint32_t previous_active_index; + + struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK]; +}; + +#endif /* _FWU_MDATA_H_ */ diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c new file mode 100644 index 0000000000..b43b406355 --- /dev/null +++ b/lib/fwu_updates/fwu.c @@ -0,0 +1,469 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2022, Linaro Limited + */ + +#include <dm.h> +#include <efi_loader.h> +#include <fwu.h> +#include <fwu_mdata.h> +#include <log.h> + +#include <linux/errno.h> +#include <linux/types.h> +#include <u-boot/crc.h> + +enum { + IMAGE_ACCEPT_SET = 1, + IMAGE_ACCEPT_CLEAR, +}; + +enum { + PRIMARY_PART = 1, + SECONDARY_PART, + BOTH_PARTS, +}; + +static int fwu_get_dev_mdata(struct udevice **dev, struct fwu_mdata *mdata) +{ + int ret; + + ret = uclass_first_device_err(UCLASS_FWU_MDATA, dev); + if (ret) { + log_debug("Cannot find fwu device\n"); + return ret; + } + + if (!mdata) + return 0; + + ret = fwu_get_mdata(*dev, mdata); + if (ret < 0) + log_debug("Unable to get valid FWU metadata\n"); + + return ret; +} + +static int fwu_get_image_type_id(u8 *image_index, efi_guid_t *image_type_id) +{ + u8 index; + int i; + struct efi_fw_image *image; + + index = *image_index; + image = update_info.images; + for (i = 0; i < num_image_type_guids; i++) { + if (index == image[i].image_index) { + guidcpy(image_type_id, &image[i].image_type_id); + return 0; + } + } + + return -ENOENT; +} + +/** + * fwu_verify_mdata() - Verify the FWU metadata + * @mdata: FWU metadata structure + * @pri_part: FWU metadata partition is primary or secondary + * + * Verify the FWU metadata by computing the CRC32 for the metadata + * structure and comparing it against the CRC32 value stored as part + * of the structure. + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part) +{ + u32 calc_crc32; + void *buf; + + buf = &mdata->version; + calc_crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32)); + + if (calc_crc32 != mdata->crc32) { + log_debug("crc32 check failed for %s FWU metadata partition\n", + pri_part ? "primary" : "secondary"); + return -EINVAL; + } + + return 0; +} + +/** + * fwu_check_mdata_validity() - Check for validity of the FWU metadata copies + * + * Read both the metadata copies from the storage media, verify their checksum, + * and ascertain that both copies match. If one of the copies has gone bad, + * restore it from the good copy. + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_check_mdata_validity(void) +{ + int ret; + struct udevice *dev; + struct fwu_mdata pri_mdata; + struct fwu_mdata secondary_mdata; + uint mdata_parts[2]; + uint valid_partitions, invalid_partitions; + + ret = fwu_get_dev_mdata(&dev, NULL); + if (ret) + return ret; + + /* + * Check if the platform has defined its own + * function to check the metadata partitions' + * validity. If so, that takes precedence. + */ + ret = fwu_mdata_check(dev); + if (!ret || ret != -ENOSYS) + return ret; + + /* + * Two FWU metadata partitions are expected. + * If we don't have two, user needs to create + * them first + */ + valid_partitions = 0; + ret = fwu_get_mdata_part_num(dev, mdata_parts); + if (ret < 0) { + log_debug("Error getting the FWU metadata partitions\n"); + return -ENOENT; + } + + ret = fwu_read_mdata_partition(dev, &pri_mdata, mdata_parts[0]); + if (!ret) { + ret = fwu_verify_mdata(&pri_mdata, 1); + if (!ret) + valid_partitions |= PRIMARY_PART; + } + + ret = fwu_read_mdata_partition(dev, &secondary_mdata, mdata_parts[1]); + if (!ret) { + ret = fwu_verify_mdata(&secondary_mdata, 0); + if (!ret) + valid_partitions |= SECONDARY_PART; + } + + if (valid_partitions == (PRIMARY_PART | SECONDARY_PART)) { + /* + * Before returning, check that both the + * FWU metadata copies are the same. If not, + * the FWU metadata copies need to be + * re-populated. + */ + if (!memcmp(&pri_mdata, &secondary_mdata, + sizeof(struct fwu_mdata))) { + ret = 0; + } else { + log_info("Both FWU metadata copies are valid but do not match. Please check!\n"); + ret = -1; + } + goto out; + } + + if (!(valid_partitions & BOTH_PARTS)) { + ret = -1; + goto out; + } + + invalid_partitions = valid_partitions ^ BOTH_PARTS; + ret = fwu_write_mdata_partition(dev, + (invalid_partitions == PRIMARY_PART) ? + &secondary_mdata : &pri_mdata, + (invalid_partitions == PRIMARY_PART) ? + mdata_parts[0] : mdata_parts[1]); + + if (ret < 0) + log_info("Restoring %s FWU metadata partition failed\n", + (invalid_partitions == PRIMARY_PART) ? + "primary" : "secondary"); + +out: + return ret; +} + +/** + * fwu_get_active_index() - Get active_index from the FWU metadata + * @active_idx: active_index value to be read + * + * Read the active_index field from the FWU metadata and place it in + * the variable pointed to be the function argument. + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_get_active_index(uint *active_idx) +{ + int ret; + struct udevice *dev; + struct fwu_mdata mdata = { 0 }; + + ret = fwu_get_dev_mdata(&dev, &mdata); + if (ret) + return ret; + + /* + * Found the FWU metadata partition, now read the active_index + * value + */ + *active_idx = mdata.active_index; + if (*active_idx >= CONFIG_FWU_NUM_BANKS) { + log_debug("Active index value read is incorrect\n"); + ret = -EINVAL; + } + + return ret; +} + +/** + * fwu_set_active_index() - Set active_index in the FWU metadata + * @active_idx: active_index value to be set + * + * Update the active_index field in the FWU metadata + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_set_active_index(uint active_idx) +{ + int ret; + struct udevice *dev; + struct fwu_mdata mdata = { 0 }; + + if (active_idx >= CONFIG_FWU_NUM_BANKS) { + log_debug("Invalid active index value\n"); + return -EINVAL; + } + + ret = fwu_get_dev_mdata(&dev, &mdata); + if (ret) + return ret; + + /* + * Update the active index and previous_active_index fields + * in the FWU metadata + */ + mdata.previous_active_index = mdata.active_index; + mdata.active_index = active_idx; + + /* + * Now write this updated FWU metadata to both the + * FWU metadata partitions + */ + ret = fwu_update_mdata(dev, &mdata); + if (ret) { + log_debug("Failed to update FWU metadata partitions\n"); + ret = -EIO; + } + + return ret; +} + +/** + * fwu_get_image_index() - Get the Image Index to be used for capsule update + * @image_index: The Image Index for the image + * + * The FWU multi bank update feature computes the value of image_index at + * runtime, based on the bank to which the image needs to be written to. + * Derive the image_index value for the image. + * + * Currently, the capsule update driver uses the DFU framework for + * the updates. This function gets the DFU alt number which is to + * be used as the Image Index + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_get_image_index(u8 *image_index) +{ + int ret, i; + u8 alt_num; + uint update_bank; + efi_guid_t *image_guid, image_type_id; + struct udevice *dev; + struct fwu_mdata mdata = { 0 }; + struct fwu_image_entry *img_entry; + struct fwu_image_bank_info *img_bank_info; + + ret = fwu_get_dev_mdata(&dev, &mdata); + if (ret) + return ret; + + ret = fwu_plat_get_update_index(&update_bank); + if (ret) { + log_debug("Failed to get the FWU update bank\n"); + goto out; + } + + ret = fwu_get_image_type_id(image_index, &image_type_id); + if (ret) { + log_debug("Unable to get image_type_id for image_index %u\n", + *image_index); + goto out; + } + + ret = -EINVAL; + /* + * The FWU metadata has been read. Now get the image_uuid for the + * image with the update_bank. + */ + for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) { + if (!guidcmp(&image_type_id, + &mdata.img_entry[i].image_type_uuid)) { + img_entry = &mdata.img_entry[i]; + img_bank_info = &img_entry->img_bank_info[update_bank]; + image_guid = &img_bank_info->image_uuid; + ret = fwu_plat_get_alt_num(dev, image_guid, &alt_num); + if (ret) { + log_debug("alt_num not found for partition with GUID %pUs\n", + image_guid); + } else { + log_debug("alt_num %d for partition %pUs\n", + alt_num, image_guid); + *image_index = alt_num + 1; + } + + goto out; + } + } + + log_debug("Partition with the image type %pUs not found\n", + &image_type_id); + +out: + return ret; +} + +/** + * fwu_revert_boot_index() - Revert the active index in the FWU metadata + * + * Revert the active_index value in the FWU metadata, by swapping the values + * of active_index and previous_active_index in both copies of the + * FWU metadata. + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_revert_boot_index(void) +{ + int ret; + u32 cur_active_index; + struct udevice *dev; + struct fwu_mdata mdata = { 0 }; + + ret = fwu_get_dev_mdata(&dev, &mdata); + if (ret) + return ret; + + /* + * Swap the active index and previous_active_index fields + * in the FWU metadata + */ + cur_active_index = mdata.active_index; + mdata.active_index = mdata.previous_active_index; + mdata.previous_active_index = cur_active_index; + + /* + * Now write this updated FWU metadata to both the + * FWU metadata partitions + */ + ret = fwu_update_mdata(dev, &mdata); + if (ret) { + log_debug("Failed to update FWU metadata partitions\n"); + ret = -EIO; + } + + return ret; +} + +/** + * fwu_clrset_image_accept() - Set or Clear the Acceptance bit for the image + * @img_type_id: GUID of the image type for which the accepted bit is to be + * set or cleared + * @bank: Bank of which the image's Accept bit is to be set or cleared + * @action: Action which specifies whether image's Accept bit is to be set or + * cleared + * + * Set/Clear the accepted bit for the image specified by the img_guid parameter. + * This indicates acceptance or rejection of image for subsequent boots by some + * governing component like OS(or firmware). + * + * Return: 0 if OK, -ve on error + * + */ +static int fwu_clrset_image_accept(efi_guid_t *img_type_id, u32 bank, u8 action) +{ + int ret, i; + struct udevice *dev; + struct fwu_mdata mdata = { 0 }; + struct fwu_image_entry *img_entry; + struct fwu_image_bank_info *img_bank_info; + + ret = fwu_get_dev_mdata(&dev, &mdata); + if (ret) + return ret; + + img_entry = &mdata.img_entry[0]; + for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) { + if (!guidcmp(&img_entry[i].image_type_uuid, img_type_id)) { + img_bank_info = &img_entry[i].img_bank_info[bank]; + if (action == IMAGE_ACCEPT_SET) + img_bank_info->accepted |= FWU_IMAGE_ACCEPTED; + else + img_bank_info->accepted = 0; + + ret = fwu_update_mdata(dev, &mdata); + goto out; + } + } + + /* Image not found */ + ret = -ENOENT; + +out: + return ret; +} + +/** + * fwu_accept_image() - Set the Acceptance bit for the image + * @img_type_id: GUID of the image type for which the accepted bit is to be + * cleared + * @bank: Bank of which the image's Accept bit is to be set + * + * Set the accepted bit for the image specified by the img_guid parameter. This + * indicates acceptance of image for subsequent boots by some governing component + * like OS(or firmware). + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_accept_image(efi_guid_t *img_type_id, u32 bank) +{ + return fwu_clrset_image_accept(img_type_id, bank, + IMAGE_ACCEPT_SET); +} + +/** + * fwu_clear_accept_image() - Clear the Acceptance bit for the image + * @img_type_id: GUID of the image type for which the accepted bit is to be + * cleared + * @bank: Bank of which the image's Accept bit is to be cleared + * + * Clear the accepted bit for the image type specified by the img_type_id parameter. + * This function is called after the image has been updated. The accepted bit is + * cleared to be set subsequently after passing the image acceptance criteria, by + * either the OS(or firmware) + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank) +{ + return fwu_clrset_image_accept(img_type_id, bank, + IMAGE_ACCEPT_CLEAR); +}