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