| Message ID | 20220915081451.633983-3-sughosh.ganu@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | FWU: Add FWU Multi Bank Update feature support | expand |
On Thu, 15 Sept 2022 at 03:15, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: .... > +/** > + * 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(u32 *active_idxp); > + Bank index is u32 here and uint in fwu_update_active_index(), so s/u32/uint ? cheers.
On Mon, 19 Sept 2022 at 06:03, Jassi Brar <jaswinder.singh@linaro.org> wrote: > > On Thu, 15 Sept 2022 at 03:15, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > .... > > +/** > > + * 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(u32 *active_idxp); > > + > Bank index is u32 here and uint in fwu_update_active_index(), so s/u32/uint ? Will change. Thanks. -sughosh
On Thu, Sep 15, 2022 at 3:15 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote: .... > +/** > + * @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 { > + /** > + * mdata_check() - 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 bad copy. > + * > + * Return: 0 if OK, -ve on error > + */ > + int (*mdata_check)(struct udevice *dev); > Like get_mdata and update_mdata, maybe check_mdata too ? ..... > +/** > + * 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(u32 *active_idxp); > + > +/** > + * fwu_update_active_index() - Update active_index from the FWU metadata > + * @active_idx: active_index value to be updated > + * > + * Update the active_index field in the FWU metadata > + * > + * Return: 0 if OK, -ve on error > + * > + */ > +int fwu_update_active_index(uint active_idx); > maybe fwu_set_active_index ? just like fwu_get_active_index ..... > +/** > + * 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; > This may cause problems. We are reverting because active_index does not work, and here we set it to previous_active_index which is supposed to mean "last good index". Also this logic assumes a 2-banks setup, and is obviously incorrect for >2 banks where the previous_active_index should point to "boot_index minus 2" bank (but of course there is no guarantee that that bank is preserved still). So either previous_active_index be left changed OR we also copy the previous bank to active bank before the swap. ..... > +/** > + * 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 > Something more consistent like fwu_image_accepted_clear() and fwu_image_accepted_set() ? cheers.
On Mon, 26 Sept 2022 at 08:28, Jassi Brar <jassisinghbrar@gmail.com> wrote: > > On Thu, Sep 15, 2022 at 3:15 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > .... > > +/** > > + * @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 { > > + /** > > + * mdata_check() - 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 bad copy. > > + * > > + * Return: 0 if OK, -ve on error > > + */ > > + int (*mdata_check)(struct udevice *dev); > > > Like get_mdata and update_mdata, maybe check_mdata too ? Okay > > ..... > > +/** > > + * 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(u32 *active_idxp); > > + > > +/** > > + * fwu_update_active_index() - Update active_index from the FWU metadata > > + * @active_idx: active_index value to be updated > > + * > > + * Update the active_index field in the FWU metadata > > + * > > + * Return: 0 if OK, -ve on error > > + * > > + */ > > +int fwu_update_active_index(uint active_idx); > > > maybe fwu_set_active_index ? just like fwu_get_active_index Okay > > ..... > > +/** > > + * 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; > > > This may cause problems. > We are reverting because active_index does not work, and here we set > it to previous_active_index which is supposed to mean "last good > index". > Also this logic assumes a 2-banks setup, and is obviously incorrect > for >2 banks where the previous_active_index should point to > "boot_index minus 2" bank (but of course there is no guarantee that > that bank is preserved still). > So either previous_active_index be left changed OR we also copy the > previous bank to active bank before the swap. Sorry, but I don't understand the review comment here. Even in the case of num_banks > 2, this function is simply using the previous_active_index value. It does not care what the previous_active_index value is. If you remember, the setting of the update bank is really a platform function(fwu_plat_get_update_index()). A platform can set any bank number as the update bank. So we cannot tell what the value of the previous_active_index will be. All that this function does is use the previous_active_index as the partition/bank to boot from in the subsequent boot cycle. > > ..... > > +/** > > + * 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 > > > Something more consistent like fwu_image_accepted_clear() and > fwu_image_accepted_set() ? Umm, the other related API is fwu_accept_image, and this is clearing the accept bit, hence the name. If you don't feel strongly about this, I would prefer the current name. -sughosh
On Mon, Sep 26, 2022 at 5:00 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Mon, 26 Sept 2022 at 08:28, Jassi Brar <jassisinghbrar@gmail.com> wrote: > > > > > > ..... > > > +/** > > > + * 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; > > > > > This may cause problems. > > We are reverting because active_index does not work, and here we set > > it to previous_active_index which is supposed to mean "last good > > index". > > Also this logic assumes a 2-banks setup, and is obviously incorrect > > for >2 banks where the previous_active_index should point to > > "boot_index minus 2" bank (but of course there is no guarantee that > > that bank is preserved still). > > So either previous_active_index be left changed OR we also copy the > > previous bank to active bank before the swap. > > Sorry, but I don't understand the review comment here. Even in the > case of num_banks > 2, this function is simply using the > previous_active_index value. It does not care what the > previous_active_index value is. If you remember, the setting of the > update bank is really a platform > function(fwu_plat_get_update_index()). A platform can set any bank > number as the update bank. So we cannot tell what the value of the > previous_active_index will be. > Do you remember you pick update_bank in a circular-buffer manner in fwu_plat_get_update_index() ? But don't even bother the >2 banks. Consider the simple 2-banks platform.... Initially: active_index = 1 previous_active_index = 0 After update and before reboot active_index = 0 <<<< updated bank 0 previous_active_index = 1 After reboot, for some reason update fails (reject bank0) and we call fwu_revert_boot_index() active_index = 1 <<< good previous_active_index = 0 <<<< points to unbootable bank Which may be seen as inconsistency if we assume previous_bank to always contain a bootable set of images. So we also need to copy bank1 into bank0 as part of the revert (at least as a backup for reasons other than a/b update failure). > All that this function does is use the > previous_active_index as the partition/bank to boot from in the > subsequent boot cycle. > That is, you assume the previous_active_index bank contains working images. > > ..... > > > +/** > > > + * 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 > > > > > Something more consistent like fwu_image_accepted_clear() and > > fwu_image_accepted_set() ? > > Umm, the other related API is fwu_accept_image, and this is clearing > the accept bit, hence the name. If you don't feel strongly about this, > I would prefer the current name. > fwu_accept_image() and fwu_clear_accept_image() don't seem like a pair.... is all I say. cheers.
On Mon, 26 Sept 2022 at 20:12, Jassi Brar <jassisinghbrar@gmail.com> wrote: > > On Mon, Sep 26, 2022 at 5:00 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > On Mon, 26 Sept 2022 at 08:28, Jassi Brar <jassisinghbrar@gmail.com> wrote: > > > > > > > > > > ..... > > > > +/** > > > > + * 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; > > > > > > > This may cause problems. > > > We are reverting because active_index does not work, and here we set > > > it to previous_active_index which is supposed to mean "last good > > > index". > > > Also this logic assumes a 2-banks setup, and is obviously incorrect > > > for >2 banks where the previous_active_index should point to > > > "boot_index minus 2" bank (but of course there is no guarantee that > > > that bank is preserved still). > > > So either previous_active_index be left changed OR we also copy the > > > previous bank to active bank before the swap. > > > > Sorry, but I don't understand the review comment here. Even in the > > case of num_banks > 2, this function is simply using the > > previous_active_index value. It does not care what the > > previous_active_index value is. If you remember, the setting of the > > update bank is really a platform > > function(fwu_plat_get_update_index()). A platform can set any bank > > number as the update bank. So we cannot tell what the value of the > > previous_active_index will be. > > > Do you remember you pick update_bank in a circular-buffer manner in > fwu_plat_get_update_index() ? But don't even bother the >2 banks. > > Consider the simple 2-banks platform.... > Initially: > active_index = 1 > previous_active_index = 0 > > After update and before reboot > active_index = 0 <<<< updated bank 0 > previous_active_index = 1 > > After reboot, for some reason update fails (reject bank0) and we call > fwu_revert_boot_index() > active_index = 1 <<< good > previous_active_index = 0 <<<< points to unbootable bank > > Which may be seen as inconsistency if we assume previous_bank to > always contain a bootable set of images. > So we also need to copy bank1 into bank0 as part of the revert (at > least as a backup for reasons other than a/b update failure). If the platform owner wants to restore a particular bank with good images, the procedure to update that bank needs to be followed just like it was any other update. If an updated bank fails the image acceptance test, the following boot would be from the previous_active_bank. In that case, the other bank needs to be updated by explicitly putting capsules in the ESP and initiating the update. > > > All that this function does is use the > > previous_active_index as the partition/bank to boot from in the > > subsequent boot cycle. > > > That is, you assume the previous_active_index bank contains working images. It is the responsibility of the platform owner to ensure that all partitions have valid images. For e.g. a platform which implements rollback protection would have this scenario in the case where, after updating a bank with images, the platform's rollback counter is incremented. This would mean that the previous_active_index(all other banks) no longer contains valid/bootable images. In such a scenario, the platform owner would have to ensure that all banks are updated with valid images. It is not the role to be played by the update code to identify this situation. The FWU code is simply for updating the images which are part of capsules to the update bank. -sughosh > > > > ..... > > > > +/** > > > > + * 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 > > > > > > > Something more consistent like fwu_image_accepted_clear() and > > > fwu_image_accepted_set() ? > > > > Umm, the other related API is fwu_accept_image, and this is clearing > > the accept bit, hence the name. If you don't feel strongly about this, > > I would prefer the current name. > > > fwu_accept_image() and fwu_clear_accept_image() don't seem like a > pair.... is all I say. > > cheers.
On Tue, Sep 27, 2022 at 2:14 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Mon, 26 Sept 2022 at 20:12, Jassi Brar <jassisinghbrar@gmail.com> wrote: > > > > On Mon, Sep 26, 2022 at 5:00 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > On Mon, 26 Sept 2022 at 08:28, Jassi Brar <jassisinghbrar@gmail.com> wrote: > > > > > > > > > > > > > > ..... > > > > > +/** > > > > > + * 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; > > > > > > > > > This may cause problems. > > > > We are reverting because active_index does not work, and here we set > > > > it to previous_active_index which is supposed to mean "last good > > > > index". > > > > Also this logic assumes a 2-banks setup, and is obviously incorrect > > > > for >2 banks where the previous_active_index should point to > > > > "boot_index minus 2" bank (but of course there is no guarantee that > > > > that bank is preserved still). > > > > So either previous_active_index be left changed OR we also copy the > > > > previous bank to active bank before the swap. > > > > > > Sorry, but I don't understand the review comment here. Even in the > > > case of num_banks > 2, this function is simply using the > > > previous_active_index value. It does not care what the > > > previous_active_index value is. If you remember, the setting of the > > > update bank is really a platform > > > function(fwu_plat_get_update_index()). A platform can set any bank > > > number as the update bank. So we cannot tell what the value of the > > > previous_active_index will be. > > > > > Do you remember you pick update_bank in a circular-buffer manner in > > fwu_plat_get_update_index() ? But don't even bother the >2 banks. > > > > Consider the simple 2-banks platform.... > > Initially: > > active_index = 1 > > previous_active_index = 0 > > > > After update and before reboot > > active_index = 0 <<<< updated bank 0 > > previous_active_index = 1 > > > > After reboot, for some reason update fails (reject bank0) and we call > > fwu_revert_boot_index() > > active_index = 1 <<< good > > previous_active_index = 0 <<<< points to unbootable bank > > > > Which may be seen as inconsistency if we assume previous_bank to > > always contain a bootable set of images. > > So we also need to copy bank1 into bank0 as part of the revert (at > > least as a backup for reasons other than a/b update failure). > > If the platform owner wants to restore a particular bank with good > images, the procedure to update that bank needs to be followed just > like it was any other update. > The banks are under FWU and the platform has (should have) no control over which bank the image goes in. > If an updated bank fails the image > acceptance test, the following boot would be from the > previous_active_bank. In that case, the other bank needs to be updated > by explicitly putting capsules in the ESP and initiating the update. > Which capsule - the one that just failed or the previous one (which may not be available/provided)? Doesn't simply copying over the bank make more sense? > > > > All that this function does is use the > > > previous_active_index as the partition/bank to boot from in the > > > subsequent boot cycle. > > > > > That is, you assume the previous_active_index bank contains working images. > > It is the responsibility of the platform owner to ensure that all > partitions have valid images. > I differ. The platform should not be modifying banks and meta-data beneath the fwu framework. The specification says "A Client can only read from or write to images in the update bank" -j
On Tue, 27 Sept 2022 at 21:55, Jassi Brar <jassisinghbrar@gmail.com> wrote: > > On Tue, Sep 27, 2022 at 2:14 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > On Mon, 26 Sept 2022 at 20:12, Jassi Brar <jassisinghbrar@gmail.com> wrote: > > > > > > On Mon, Sep 26, 2022 at 5:00 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > On Mon, 26 Sept 2022 at 08:28, Jassi Brar <jassisinghbrar@gmail.com> wrote: > > > > > > > > > > > > > > > > > > ..... > > > > > > +/** > > > > > > + * 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; > > > > > > > > > > > This may cause problems. > > > > > We are reverting because active_index does not work, and here we set > > > > > it to previous_active_index which is supposed to mean "last good > > > > > index". > > > > > Also this logic assumes a 2-banks setup, and is obviously incorrect > > > > > for >2 banks where the previous_active_index should point to > > > > > "boot_index minus 2" bank (but of course there is no guarantee that > > > > > that bank is preserved still). > > > > > So either previous_active_index be left changed OR we also copy the > > > > > previous bank to active bank before the swap. > > > > > > > > Sorry, but I don't understand the review comment here. Even in the > > > > case of num_banks > 2, this function is simply using the > > > > previous_active_index value. It does not care what the > > > > previous_active_index value is. If you remember, the setting of the > > > > update bank is really a platform > > > > function(fwu_plat_get_update_index()). A platform can set any bank > > > > number as the update bank. So we cannot tell what the value of the > > > > previous_active_index will be. > > > > > > > Do you remember you pick update_bank in a circular-buffer manner in > > > fwu_plat_get_update_index() ? But don't even bother the >2 banks. > > > > > > Consider the simple 2-banks platform.... > > > Initially: > > > active_index = 1 > > > previous_active_index = 0 > > > > > > After update and before reboot > > > active_index = 0 <<<< updated bank 0 > > > previous_active_index = 1 > > > > > > After reboot, for some reason update fails (reject bank0) and we call > > > fwu_revert_boot_index() > > > active_index = 1 <<< good > > > previous_active_index = 0 <<<< points to unbootable bank > > > > > > Which may be seen as inconsistency if we assume previous_bank to > > > always contain a bootable set of images. > > > So we also need to copy bank1 into bank0 as part of the revert (at > > > least as a backup for reasons other than a/b update failure). > > > > If the platform owner wants to restore a particular bank with good > > images, the procedure to update that bank needs to be followed just > > like it was any other update. > > > The banks are under FWU and the platform has (should have) no control > over which bank the image goes in. It is the platform code that can decide on how the update banks are selected. Even in the case of num_banks > 2, the platform can have a round robin selection of the update banks through which all the other banks can be restored/updated. And currently, the default function for selection of the update bank does use round robin selection of the banks. > > > > If an updated bank fails the image > > acceptance test, the following boot would be from the > > previous_active_bank. In that case, the other bank needs to be updated > > by explicitly putting capsules in the ESP and initiating the update. > > > Which capsule - the one that just failed or the previous one (which > may not be available/provided)? One with working images. Please note that in the normal flow, a user would have tested the images locally before getting them deployed. > Doesn't simply copying over the bank make more sense? When we are talking about "simply copying", I hope you appreciate that 1) this(restoration of the banks) is not under the purview of the FWU spec and 2) it will need some thought on how to have a generic implementation which fetches images from the good bank and then writes them over to the other banks. The implementation of the FWU spec requires using the UEFI capsule update interface for the updates. This would require some thought on how to use the FMP functions to copy over the images to other banks in a generic way. But please note that even in case of num_banks > 2, this can be done through the normal capsule update flow if the platform implements a round robin update bank selection. > > > > > > > All that this function does is use the > > > > previous_active_index as the partition/bank to boot from in the > > > > subsequent boot cycle. > > > > > > > That is, you assume the previous_active_index bank contains working images. > > > > It is the responsibility of the platform owner to ensure that all > > partitions have valid images. > > > I differ. The platform should not be modifying banks and meta-data > beneath the fwu framework. > The specification says "A Client can only read from or write to images > in the update bank" I did not say modifying the banks through a parallel mechanism. Like I mentioned above, this can be done through the normal update flow. -sughosh
On Wed, Sep 28, 2022 at 1:00 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Tue, 27 Sept 2022 at 21:55, Jassi Brar <jassisinghbrar@gmail.com> wrote: > > > > On Tue, Sep 27, 2022 at 2:14 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > On Mon, 26 Sept 2022 at 20:12, Jassi Brar <jassisinghbrar@gmail.com> wrote: > > > > > > > > On Mon, Sep 26, 2022 at 5:00 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > On Mon, 26 Sept 2022 at 08:28, Jassi Brar <jassisinghbrar@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > > > ..... > > > > > > > +/** > > > > > > > + * 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; > > > > > > > > > > > > > This may cause problems. > > > > > > We are reverting because active_index does not work, and here we set > > > > > > it to previous_active_index which is supposed to mean "last good > > > > > > index". > > > > > > Also this logic assumes a 2-banks setup, and is obviously incorrect > > > > > > for >2 banks where the previous_active_index should point to > > > > > > "boot_index minus 2" bank (but of course there is no guarantee that > > > > > > that bank is preserved still). > > > > > > So either previous_active_index be left changed OR we also copy the > > > > > > previous bank to active bank before the swap. > > > > > > > > > > Sorry, but I don't understand the review comment here. Even in the > > > > > case of num_banks > 2, this function is simply using the > > > > > previous_active_index value. It does not care what the > > > > > previous_active_index value is. If you remember, the setting of the > > > > > update bank is really a platform > > > > > function(fwu_plat_get_update_index()). A platform can set any bank > > > > > number as the update bank. So we cannot tell what the value of the > > > > > previous_active_index will be. > > > > > > > > > Do you remember you pick update_bank in a circular-buffer manner in > > > > fwu_plat_get_update_index() ? But don't even bother the >2 banks. > > > > > > > > Consider the simple 2-banks platform.... > > > > Initially: > > > > active_index = 1 > > > > previous_active_index = 0 > > > > > > > > After update and before reboot > > > > active_index = 0 <<<< updated bank 0 > > > > previous_active_index = 1 > > > > > > > > After reboot, for some reason update fails (reject bank0) and we call > > > > fwu_revert_boot_index() > > > > active_index = 1 <<< good > > > > previous_active_index = 0 <<<< points to unbootable bank > > > > > > > > Which may be seen as inconsistency if we assume previous_bank to > > > > always contain a bootable set of images. > > > > So we also need to copy bank1 into bank0 as part of the revert (at > > > > least as a backup for reasons other than a/b update failure). > > > > > > If the platform owner wants to restore a particular bank with good > > > images, the procedure to update that bank needs to be followed just > > > like it was any other update. > > > > > The banks are under FWU and the platform has (should have) no control > > over which bank the image goes in. > > It is the platform code that can decide on how the update banks are > selected. Even in the case of num_banks > 2, the platform can have a > round robin selection of the update banks through which all the other > banks can be restored/updated. And currently, the default function for > selection of the update bank does use round robin selection of the > banks. > Platform only reads from and writes to a bank number given by the fwu. Platform has no means to sense if a bank is accepted or rejected. Or is there? > > > > > If an updated bank fails the image > > > acceptance test, the following boot would be from the > > > previous_active_bank. In that case, the other bank needs to be updated > > > by explicitly putting capsules in the ESP and initiating the update. > > > > > Which capsule - the one that just failed or the previous one (which > > may not be available/provided)? > > One with working images. Please note that in the normal flow, a user > would have tested the images locally before getting them deployed. > In the "normal flow" of the world many industries shouldn't exist - security, anti-virus, customer-support, repair etc :) > > Doesn't simply copying over the bank make more sense? > > When we are talking about "simply copying", I hope you appreciate that > 1) this(restoration of the banks) is not under the purview of the FWU > spec > I think it is. The restoration is part of the management of the banks... just like FWU makes sure metadata replicas are always in sync. 2) it will need some thought on how to have a generic > implementation which fetches images from the good bank and then writes > them over to the other banks. > Not from "good bank" to "other banks". FWU only needs to copy from previous_bank to active_bank before swapping their pointers in fwu_revert_boot_index() maybe by a platform specific callback copy_bank(int from, int to) ? > The implementation of the FWU spec > requires using the UEFI capsule update interface for the updates. > The spec actually expects FWU to reside in BL32, but here we are implementing it in BL33 (I know we had to). IMO, always keeping a good previous_bank is not even a violation of the spec, it is internal to the implementation. But ok. I don't want to be the one to slow down the uboot's fwu progress. We can always implement it later when more people see the need. -jassi
On Thu, 29 Sept 2022 at 00:59, Jassi Brar <jassisinghbrar@gmail.com> wrote: > > On Wed, Sep 28, 2022 at 1:00 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > On Tue, 27 Sept 2022 at 21:55, Jassi Brar <jassisinghbrar@gmail.com> wrote: > > > > > > On Tue, Sep 27, 2022 at 2:14 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > On Mon, 26 Sept 2022 at 20:12, Jassi Brar <jassisinghbrar@gmail.com> wrote: > > > > > > > > > > On Mon, Sep 26, 2022 at 5:00 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > > > On Mon, 26 Sept 2022 at 08:28, Jassi Brar <jassisinghbrar@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > ..... > > > > > > > > +/** > > > > > > > > + * 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; > > > > > > > > > > > > > > > This may cause problems. > > > > > > > We are reverting because active_index does not work, and here we set > > > > > > > it to previous_active_index which is supposed to mean "last good > > > > > > > index". > > > > > > > Also this logic assumes a 2-banks setup, and is obviously incorrect > > > > > > > for >2 banks where the previous_active_index should point to > > > > > > > "boot_index minus 2" bank (but of course there is no guarantee that > > > > > > > that bank is preserved still). > > > > > > > So either previous_active_index be left changed OR we also copy the > > > > > > > previous bank to active bank before the swap. > > > > > > > > > > > > Sorry, but I don't understand the review comment here. Even in the > > > > > > case of num_banks > 2, this function is simply using the > > > > > > previous_active_index value. It does not care what the > > > > > > previous_active_index value is. If you remember, the setting of the > > > > > > update bank is really a platform > > > > > > function(fwu_plat_get_update_index()). A platform can set any bank > > > > > > number as the update bank. So we cannot tell what the value of the > > > > > > previous_active_index will be. > > > > > > > > > > > Do you remember you pick update_bank in a circular-buffer manner in > > > > > fwu_plat_get_update_index() ? But don't even bother the >2 banks. > > > > > > > > > > Consider the simple 2-banks platform.... > > > > > Initially: > > > > > active_index = 1 > > > > > previous_active_index = 0 > > > > > > > > > > After update and before reboot > > > > > active_index = 0 <<<< updated bank 0 > > > > > previous_active_index = 1 > > > > > > > > > > After reboot, for some reason update fails (reject bank0) and we call > > > > > fwu_revert_boot_index() > > > > > active_index = 1 <<< good > > > > > previous_active_index = 0 <<<< points to unbootable bank > > > > > > > > > > Which may be seen as inconsistency if we assume previous_bank to > > > > > always contain a bootable set of images. > > > > > So we also need to copy bank1 into bank0 as part of the revert (at > > > > > least as a backup for reasons other than a/b update failure). > > > > > > > > If the platform owner wants to restore a particular bank with good > > > > images, the procedure to update that bank needs to be followed just > > > > like it was any other update. > > > > > > > The banks are under FWU and the platform has (should have) no control > > > over which bank the image goes in. > > > > It is the platform code that can decide on how the update banks are > > selected. Even in the case of num_banks > 2, the platform can have a > > round robin selection of the update banks through which all the other > > banks can be restored/updated. And currently, the default function for > > selection of the update bank does use round robin selection of the > > banks. > > > Platform only reads from and writes to a bank number given by the fwu. > Platform has no means to sense if a bank is accepted or rejected. Or > is there? What I meant is that the bank to which the update will be written to is governed by the function fwu_plat_get_update_index(), which can be provided by every platform. And the platform can compute the update bank in a round robin fashion. This is another way to restore a bank from which the images are not booting for any reason(like rollback protection). Unless there is a hardware issue in the storage device, which will require manual intervention. So what I am trying to say here is that there is a way the rest of the banks can be restored without having to copy the banks. > > > > > > > > > If an updated bank fails the image > > > > acceptance test, the following boot would be from the > > > > previous_active_bank. In that case, the other bank needs to be updated > > > > by explicitly putting capsules in the ESP and initiating the update. > > > > > > > Which capsule - the one that just failed or the previous one (which > > > may not be available/provided)? > > > > One with working images. Please note that in the normal flow, a user > > would have tested the images locally before getting them deployed. > > > In the "normal flow" of the world many industries shouldn't exist - > security, anti-virus, customer-support, repair etc :) > > > > Doesn't simply copying over the bank make more sense? > > > > When we are talking about "simply copying", I hope you appreciate that > > 1) this(restoration of the banks) is not under the purview of the FWU > > spec > > > I think it is. The restoration is part of the management of the > banks... just like FWU makes sure metadata replicas are always in > sync. Okay. I did not find mention of this in the FWU specification. Can you point me to this. > > 2) it will need some thought on how to have a generic > > implementation which fetches images from the good bank and then writes > > them over to the other banks. > > > Not from "good bank" to "other banks". > FWU only needs to copy from previous_bank to active_bank before > swapping their pointers in fwu_revert_boot_index() maybe by a platform > specific callback copy_bank(int from, int to) ? Yes, but you might have a situation where all the other banks need to be updated, say due to rollback protection kicking in. I was talking from that point of view. > > > > The implementation of the FWU spec > > requires using the UEFI capsule update interface for the updates. > > > The spec actually expects FWU to reside in BL32, but here we are > implementing it in BL33 (I know we had to). Not really. The spec does actually mention this scenario of the Update Agent and Client both residing in the normal world. This is a valid scenario that is described by the FWU spec, pg 39, "Normal World controlled FW store". > IMO, always keeping a good previous_bank is not even a violation of > the spec, it is internal to the implementation. Yes, I take your point. Only thing I was trying to highlight is that it can be achieved even without the copy. -sughosh > > But ok. I don't want to be the one to slow down the uboot's fwu > progress. We can always implement it later when more people see the > need. > > -jassi
diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c b/drivers/fwu-mdata/fwu-mdata-uclass.c new file mode 100644 index 0000000000..65ae93c21f --- /dev/null +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c @@ -0,0 +1,107 @@ +// 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_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 bad 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->mdata_check) { + log_debug("mdata_check() method not defined\n"); + return -ENOSYS; + } + + return ops->mdata_check(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 a432e43871..598a8c10a0 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -58,6 +58,7 @@ enum uclass_id { UCLASS_FIRMWARE, /* Firmware */ 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..745f6225d0 --- /dev/null +++ b/include/fwu.h @@ -0,0 +1,214 @@ +/* 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 { + /** + * mdata_check() - 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 bad copy. + * + * Return: 0 if OK, -ve on error + */ + int (*mdata_check)(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); + + /** + * fwu_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); +}; + +#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_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(u32 *active_idxp); + +/** + * fwu_update_active_index() - Update active_index from the FWU metadata + * @active_idx: active_index value to be updated + * + * Update the active_index field in the FWU metadata + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_update_active_index(uint active_idx); + +/** + * fwu_get_image_index() - Get the Image Index to be used for capsule update + * @image_type_id: pointer to the image GUID as passed in the capsule + * @update_bank: Bank to which the update is to be made + * @image_index: The Image Index for the image + * + * Based on the GUID value passed in the capsule, along with the bank to which the + * image needs to be updated, get the Image Index value which will be used for the + * capsule update. + * + * 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(efi_guid_t *image_type_id, u32 update_bank, + 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 bad 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..fb9dbca307 --- /dev/null +++ b/lib/fwu_updates/fwu.c @@ -0,0 +1,333 @@ +// 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> + +#define IMAGE_ACCEPT_SET BIT(0) +#define IMAGE_ACCEPT_CLEAR BIT(1) + +static int fwu_get_dev_mdata(struct udevice **dev, struct fwu_mdata *mdata) + +{ + int ret; + + ret = uclass_first_device(UCLASS_FWU_MDATA, dev); + if (ret) { + log_debug("Cannot find fwu device\n"); + return ret; + } + + ret = fwu_get_mdata(*dev, mdata); + if (ret < 0) + log_debug("Unable to get valid FWU metadata\n"); + + return ret; +} + +/** + * 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_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(u32 *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_update_active_index() - Update active_index from the FWU metadata + * @active_idx: active_index value to be updated + * + * Update the active_index field in the FWU metadata + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_update_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 < 0) { + 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_type_id: pointer to the image GUID as passed in the capsule + * @update_bank: Bank to which the update is to be made + * @image_index: The Image Index for the image + * + * Based on the GUID value passed in the capsule, along with the bank to which the + * image needs to be updated, get the Image Index value which will be used for the + * capsule update. + * + * 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(efi_guid_t *image_type_id, u32 update_bank, + u8 *image_index) +{ + int ret, i; + efi_guid_t *image_guid; + 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 = -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, image_index); + 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", + *image_index, image_guid); + ++*image_index; + } + + 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 < 0) { + 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); +}