RE: [PATCH v6 0/6] FWU: Add support for mtd backed feature on DeveloperBox
Hi, > -Original Message- > From: Michal Simek > Sent: Tuesday, June 20, 2023 3:20 PM > To: Jassi Brar ; Ilias Apalodimas > > Cc: Jose Marinho ; u-boot@lists.denx.de; > etienne.carri...@linaro.org; tr...@konsulko.com; s...@chromium.org; > sughosh.g...@linaro.org; xypron.g...@gmx.de; takahiro.aka...@linaro.org > Subject: Re: [PATCH v6 0/6] FWU: Add support for mtd backed feature on > DeveloperBox > > > > On 6/20/23 16:14, Jassi Brar wrote: > > On Tue, 20 Jun 2023 at 05:05, Ilias Apalodimas > > wrote: > >> > >> 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 > wrote: > >>>> > >>>> Hi Jassi, > >>>> > >>>> On 5/31/23 07:28, jaswinder.si...@linaro.org wrote: > >>>>> From: Jassi Brar > >>>>> > >>>>> 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- > jassisingh > >>>>> b...@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 232 8;bank1 raw 27a 8000 > >>>> nor0=bank0 raw > >>>> 23a 400;bank1 raw 282 400 > >>>> > >>>> If you look at size in second entry you will see that it is 8000 > >>>> instead of > >>>> 8 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. > >> > > Me too. > > > >> 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? > >> > > No. And I don't think current A/B update implementation in u-boot > > even has helpers for platforms to implement multiple images per bank. > > Which is fine, except we pretend we do by having NUM_IMAGES_PER_BANK > > config -- which will always be set to 1 I predict ;) > > Actually support is there but it requires one thing to patch. I will send that > patch when I clean my repo. > And I have tested it on our platform. > > > > >> 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. > >> > > exactly. > > > >> 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
Re: [PATCH v6 0/6] FWU: Add support for mtd backed feature on DeveloperBox
On 6/20/23 16:14, Jassi Brar wrote: On Tue, 20 Jun 2023 at 05:05, Ilias Apalodimas wrote: 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 wrote: Hi Jassi, On 5/31/23 07:28, jaswinder.si...@linaro.org wrote: From: Jassi Brar 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-jassisinghb...@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 232 8;bank1 raw 27a 8000 nor0=bank0 raw 23a 400;bank1 raw 282 400 If you look at size in second entry you will see that it is 8000 instead of 8 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. Me too. 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? No. And I don't think current A/B update implementation in u-boot even has helpers for platforms to implement multiple images per bank. Which is fine, except we pretend we do by having NUM_IMAGES_PER_BANK config -- which will always be set to 1 I predict ;) Actually support is there but it requires one thing to patch. I will send that patch when I clean my repo. And I have tested it on our platform. 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. exactly. 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. Totally. As I said, I don't believe in the practicality of more than 1 image/bank. So we are on the same page. 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. Another way would be to simply decode this information directly from DT fwu-mdata node. Because from that node it is clear how many banks and how many images that particular bank has. Thanks, Michal
Re: [PATCH v6 0/6] FWU: Add support for mtd backed feature on DeveloperBox
On Tue, 20 Jun 2023 at 05:05, Ilias Apalodimas wrote: > > 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 wrote: > > > > > > Hi Jassi, > > > > > > On 5/31/23 07:28, jaswinder.si...@linaro.org wrote: > > > > From: Jassi Brar > > > > > > > > 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-jassisinghb...@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 232 8;bank1 raw 27a 8000 nor0=bank0 raw > > > 23a 400;bank1 raw 282 400 > > > > > > If you look at size in second entry you will see that it is 8000 instead > > > of > > > 8 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. > Me too. > 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? > No. And I don't think current A/B update implementation in u-boot even has helpers for platforms to implement multiple images per bank. Which is fine, except we pretend we do by having NUM_IMAGES_PER_BANK config -- which will always be set to 1 I predict ;) > 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. > exactly. > 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. > Totally. As I said, I don't believe in the practicality of more than 1 image/bank. So we are on the same page. > > > > > 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. > OK. If we have, say, reserved[0] as last Image and reserved[1] as last Bank, we could get rid of the two compile-time configs. Thanks.
Re: [PATCH v6 0/6] FWU: Add support for mtd backed feature on DeveloperBox
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 wrote: > > > > Hi Jassi, > > > > On 5/31/23 07:28, jaswinder.si...@linaro.org wrote: > > > From: Jassi Brar > > > > > > 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-jassisinghb...@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 232 8;bank1 raw 27a 8000 nor0=bank0 raw > > 23a 400;bank1 raw 282 400 > > > > If you look at size in second entry you will see that it is 8000 instead of > > 8 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
Re: [PATCH v6 0/6] FWU: Add support for mtd backed feature on DeveloperBox
Hi Michal, On Mon, 19 Jun 2023 at 10:02, Michal Simek wrote: > > Hi Jassi, > > On 5/31/23 07:28, jaswinder.si...@linaro.org wrote: > > From: Jassi Brar > > > > 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-jassisinghb...@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 232 8;bank1 raw 27a 8000 nor0=bank0 raw > 23a 400;bank1 raw 282 400 > > If you look at size in second entry you will see that it is 8000 instead of > 8 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. > 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. 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.
Re: [PATCH v6 0/6] FWU: Add support for mtd backed feature on DeveloperBox
Hi Jassi, On 5/31/23 07:28, jaswinder.si...@linaro.org wrote: From: Jassi Brar 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-jassisinghb...@gmail.com/ Changes since v5: * Some of the typo fixes and cosmetic changes suggested by Etienne Changes since v4: * Provide default/weak implementations of fwu_plat_get_alt_num and fwu_plat_get_bootidx * Provide man page for mkfwumdata * Misc typo fixes and cosmetic changes Changes since v3: * Fix and Update documentation to also build optee for FWU FIP image. * Fixed checkpatch warnings * Made local functions static. * Split config changes to a separate patch * Fix authorship of three patches. Jassi Brar (4): dt: fwu: developerbox: enable fwu banks and mdata regions configs: move to new flash layout and boot flow fwu: DeveloperBox: add support for FWU fwu: provide default fwu_plat_get_bootidx Masami Hiramatsu (2): FWU: Add FWU metadata access driver for MTD storage regions tools: Add mkfwumdata tool for FWU metadata image .../synquacer-sc2a11-developerbox-u-boot.dtsi | 49 ++- board/socionext/developerbox/Makefile | 1 + board/socionext/developerbox/developerbox.c | 8 + board/socionext/developerbox/fwu_plat.c | 37 ++ configs/synquacer_developerbox_defconfig | 12 +- doc/board/socionext/developerbox.rst | 154 +++- doc/mkfwumdata.1 | 89 + drivers/fwu-mdata/Kconfig | 15 + drivers/fwu-mdata/Makefile| 1 + drivers/fwu-mdata/raw_mtd.c | 269 ++ include/configs/synquacer.h | 10 + include/fwu.h | 32 ++ lib/fwu_updates/Makefile | 1 + lib/fwu_updates/fwu.c | 18 + lib/fwu_updates/fwu_mtd.c | 185 ++ tools/Kconfig | 9 + tools/Makefile| 4 + tools/mkfwumdata.c| 334 ++ 18 files changed, 1217 insertions(+), 11 deletions(-) create mode 100644 board/socionext/developerbox/fwu_plat.c create mode 100644 doc/mkfwumdata.1 create mode 100644 drivers/fwu-mdata/raw_mtd.c create mode 100644 lib/fwu_updates/fwu_mtd.c create mode 100644 tools/mkfwumdata.c I know that Tom has applied this already but I still think there is a work to do here. 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 232 8;bank1 raw 27a 8000 nor0=bank0 raw 23a 400;bank1 raw 282 400 If you look at size in second entry you will see that it is 8000 instead of 8 because it is the same image. That's why curious if you have tested any scenario like this. 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? Thanks, Michal
Re: [PATCH v6 0/6] FWU: Add support for mtd backed feature on DeveloperBox
On Wed, 31 May 2023 00:28:04 -0500, jaswinder.si...@linaro.org wrote: > From: Jassi Brar > > 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-jassisinghb...@gmail.com/ > > [...] Applied to u-boot/next, thanks! -- Tom
[PATCH v6 0/6] FWU: Add support for mtd backed feature on DeveloperBox
From: Jassi Brar 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-jassisinghb...@gmail.com/ Changes since v5: * Some of the typo fixes and cosmetic changes suggested by Etienne Changes since v4: * Provide default/weak implementations of fwu_plat_get_alt_num and fwu_plat_get_bootidx * Provide man page for mkfwumdata * Misc typo fixes and cosmetic changes Changes since v3: * Fix and Update documentation to also build optee for FWU FIP image. * Fixed checkpatch warnings * Made local functions static. * Split config changes to a separate patch * Fix authorship of three patches. Jassi Brar (4): dt: fwu: developerbox: enable fwu banks and mdata regions configs: move to new flash layout and boot flow fwu: DeveloperBox: add support for FWU fwu: provide default fwu_plat_get_bootidx Masami Hiramatsu (2): FWU: Add FWU metadata access driver for MTD storage regions tools: Add mkfwumdata tool for FWU metadata image .../synquacer-sc2a11-developerbox-u-boot.dtsi | 49 ++- board/socionext/developerbox/Makefile | 1 + board/socionext/developerbox/developerbox.c | 8 + board/socionext/developerbox/fwu_plat.c | 37 ++ configs/synquacer_developerbox_defconfig | 12 +- doc/board/socionext/developerbox.rst | 154 +++- doc/mkfwumdata.1 | 89 + drivers/fwu-mdata/Kconfig | 15 + drivers/fwu-mdata/Makefile| 1 + drivers/fwu-mdata/raw_mtd.c | 269 ++ include/configs/synquacer.h | 10 + include/fwu.h | 32 ++ lib/fwu_updates/Makefile | 1 + lib/fwu_updates/fwu.c | 18 + lib/fwu_updates/fwu_mtd.c | 185 ++ tools/Kconfig | 9 + tools/Makefile| 4 + tools/mkfwumdata.c| 334 ++ 18 files changed, 1217 insertions(+), 11 deletions(-) create mode 100644 board/socionext/developerbox/fwu_plat.c create mode 100644 doc/mkfwumdata.1 create mode 100644 drivers/fwu-mdata/raw_mtd.c create mode 100644 lib/fwu_updates/fwu_mtd.c create mode 100644 tools/mkfwumdata.c -- 2.34.1
