[PATCH v6 0/6] FWU: Add support for mtd backed feature on DeveloperBox

[Ilias Apalodimas]

Sorry for being late to the party,

+cc Jose who maintains DEN0118

On Mon, Jun 19, 2023 at 11:16:53AM -0500, Jassi Brar wrote:
> Hi Michal,
>
> On Mon, 19 Jun 2023 at 10:02, Michal Simek <michal.simek at amd.com> wrote:
> >
> > Hi Jassi,
> >
> > On 5/31/23 07:28, jaswinder.singh at linaro.org wrote:
> > > From: Jassi Brar <jaswinder.singh at linaro.org>
> > >
> > > Introduce support for mtd backed storage for FWU feature and enable it on
> > > Synquacer platform based DeveloperBox.
> > >
> > > This revision is rebased onto patchset that trims the FWU api
> > >   https://lore.kernel.org/u-boot/20230306231747.1888513-1-jassisinghbrar@gmail.com/
> > >
> .....
>
> > Firstly I can generate 2 images per one bank which should be pretty much regular
> > capsule update for 2 images. I would expect this should still work.
> >
> > And then I tried 2 banks with 2 images and fwu_gen_alt_info_from_mtd() generated
> > this description for DFU
> >
> > mtd nor0=bank0 raw 2320000 80000;bank1 raw 27a0000 8000&mtd nor0=bank0 raw
> > 23a0000 4000000;bank1 raw 2820000 4000000
> >
> > If you look at size in second entry you will see that it is 8000 instead of
> > 80000 because it is the same image. That's why curious if you have tested any
> > scenario like this.
> >
> I had, and have, strong doubts about the practicality of 2
> images/bank. There aren't enough specification details to explain how
> only 1-out-of-N images could be updated. And if we always update all
> images in a bank together, we might as well have them as one composite
> image. I never got satisfactory clarification from designers and
> implementers. So, sorry, I can't defend that scenario with my limited
> knowledge.

This is intentionally left out, as we consider it a platform policy.
For example you could update a single bank and copy over the remaining
firmware images from a different ban. There's nothing on the spec that
prohibits you from doing so, but I personally think it's a bad idea.
Keep in mind there's a document hosted here[0] which explains the update
flow and documents what we have as code in U-Boot.

As far as individual firmware components go now,  do you have any useful
usecase?  The general guidance of [0] is construct a firmware
image, only update the components you want while leaving the rest on the
same revisions and update the entire firmware.  The reason is that firmware

Updating a single image of another bank is not as easy as it sounds.
Firmware components nowadays, whether we like it or not, depend on each other.
Since firmware updates are not so often and fast,  we didn't see any gains from
over complicating the spec and explicitly describe that case, while dealing
with firmware component compatibility at the same time.

>
> > Next part which I want to take a look is practicality of CONFIG_FWU_NUM_BANKS
> > and CONFIG_FWU_NUM_IMAGES_PER_BANK because it pretty much enforcing number of
> > banks and images for every platform and prevent creating one u-boot which works
> > on different boards and just use information from mdata.
> > DEN0118 doesn't show any field with this information but do you think that would
> > be possible to extract that information from there based on for example reserved
> > or accepted field?
> >
> Unfortunately the DEN0118 spec doesn't leave any 'don't care' bits in
> 'accepted' or 'reserved' fields, all unused bits Must-Be-Zero. If we
> use any bit, we'll be in violation of the spec.

Yes this is unfortunate.  We did have similar concerns on when drafting the
spec,  however we never had a solid usecase to justify this.  Arm and
Linaro and working on a v2 (which we try to make compatible with v1) which
addresses this shortcoming.  Maybe Jose can chime in.

>
> However, we can do CRC32 calculations by varying NUM_IMAGES_PER_BANK
> and NUM_BANKS and find the value pair for which the crc32 field
> matches!
> For limiting check-loops and finding corrupted metadata, the .config
> can carry upper limits on both the options, say
> MAX_NUM_IMAGES_PER_BANK=5 and MAX_NUM_BANKS=10 for the most special
> scenario. If we find the approach acceptable, I can cook a patch as
> proof-of-concept.
>
> cheers.

[0] https://gitlab.com/Linaro/trustedsubstrate/mbfw/uploads/3d0d7d11ca9874dc9115616b418aa330/mbfw.pdf

Thanks
/Ilias