Re: [PATCH 32/81] fwu-mdata: Remove and add needed includes
On Wed, May 1, 2024 at 8:45 PM Tom Rini wrote: > > Remove from this driver directory and when needed > add missing include files directly. > > Signed-off-by: Tom Rini > --- > Cc: Tom Rini > Cc: Ilias Apalodimas > Cc: Jassi Brar > Cc: Etienne Carriere > --- Acked-by: Jassi Brar Thanks
Re: [PATCH v2] board: synquacer: developerbox: add myself as maintainer
On Thu, 29 Feb 2024 at 22:38, Masahisa Kojima wrote: > > Add myself as maintainer for SynQuacer Developerbox, > as I'm currently working on it. > This commit also removes Jassi from maintainer since he > no longer has a Developerbox. > Acked-by: Jassi Brar
Re: [PATCH] board: synquacer: developerbox: add myself as co-maintainer
On Thu, Feb 29, 2024 at 6:37 PM Masahisa Kojima wrote: > > Add myself as co-maintainer for SynQuacer Developerbox, > as I'm currently working on it. > > Signed-off-by: Masahisa Kojima > --- > board/socionext/developerbox/MAINTAINERS | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/board/socionext/developerbox/MAINTAINERS > b/board/socionext/developerbox/MAINTAINERS > index c6d4f2749d..d772bc3b4c 100644 > --- a/board/socionext/developerbox/MAINTAINERS > +++ b/board/socionext/developerbox/MAINTAINERS > @@ -1,5 +1,6 @@ > DEVELOPER BOX > M: Jassi Brar > +M: Masahisa Kojima > S: Maintained > F: arch/arm/dts/synquacer-* > F: board/socionext/developerbox/* > Considering that I no longer have a Developerbox, maybe remove me as a maintainer too. Thanks, Jassi
Re: [PATCH v2] FWU: developerbox: read boot index from NOR flash
On Tue, 27 Feb 2024 at 23:53, Masahisa Kojima wrote: > > The FWU Multi Bank Update feature allows the platform to boot the > firmware images from one of the partitions(banks). > On the Developerbox, SCP-firmware running on the SCB(Cortex-M3) > passes the value of the boot index on the NOR flash. > Add a function to read the boot index value from the NOR flash. > > Signed-off-by: Masahisa Kojima Acked-by: Jassi Brar
Re: [PATCH] board: synquacer: set actual gd->ram_top and gd->ram_size
On Mon, 2 Oct 2023 at 21:31, Masahisa Kojima wrote:
>
> Current gd->ram_size and gd->ram_top reflect only the
> first DRAM bank even if the SynQuacer Developerbox could
> have up to three DRAM banks.
> With the commit 06d514d77c37 ("lmb: consider EFI memory map"),
> the first DRAM bank indicates <4GB address, so whole >4GB memory
> is marked as EFI_BOOT_SERVICES_DATA and it results that
> U-Boot can not access >4GB memory.
>
> Since 64-bits DRAM address is fully available on the SynQuacer
> Developerbox, let's set the installed DIMM information to
> gd->ram_top and gd->ram_size.
>
> Signed-off-by: Masahisa Kojima
Acked-by: Jassi Brar
Re: [RFC PATCH 0/5] Allow for removal of DT nodes and properties
Hi Simon, On Thu, Sep 7, 2023 at 3:08 PM Simon Glass wrote: > On Wed, 6 Sept 2023 at 23:20, Ilias Apalodimas > > > > > > I beg to differ. Devicetree is more than just hardware and always has > > > > been. See, for example the /chosen and /options nodes. > > > > > > There are exceptions... > > > > > > > We've been this over and over again and frankly it gets a bit annoying. > > It's called *DEVICE* tree for a reason. As Rob pointed out there are > > exceptions, but those made a lot of sense. Having arbitrary internal ABI > > stuff of various projects in the schema just defeats the definition of a > > spec. > > Our efforts should not just be about internal ABI, but working to > provide a consistent configuration system for all firmware elements. > Sure, programmatically we can pass any data/info via DT, however it is only meant to map hardware features onto data structures. devicetree.org landing page "The devicetree is a data structure for describing hardware." devicetree-specification-v0.3.pdf Chapter-2 Line-1 "DTSpec specifies a construct called a devicetree to describe system hardware." If we want to digress from the spec, we need the majority of developers to switch sides :) which is unlikely to happen and rightly so, imho. regards.
Re: [PATCH] fwu: Initialize global fwu library state during CI test
On Tue, 22 Aug 2023 at 19:17, Marek Vasut wrote: > > The current CI test worked by sheer luck, the g_dev global pointer > in the fwu library was never initialized and the test equally well > failed on sandbox64. Trigger the main loop in sandbox tests too to > initialize that global state, and move the sandbox specific exit > from fwu_boottime_checks after g_dev is initialized. > > Signed-off-by: Marek Vasut Acked=by: Jassi Brar
Re: [PATCH] fwu: Allow code to properly decode trial state
On Thu, 13 Jul 2023 at 09:35, Michal Simek wrote: > > Current code after capsule update (mtd write) is not changing active_index > in mdata to previous_active_index. > On the reboot this is shown but showing message > "Boot idx 1 is not matching active idx 0, changing active_idx" > which is changing active_idx and writing mdata to flash. > > But when this message is visible it is not checking which state that images > are. If they have acceptance bit setup to yes everything is fine and valid > images are booted (doesn't mean the latest one). > But if acceptance bit is no and images are in trial state in_trial variable > is never setup. Which means that from new flashed image stable image can be > rewritten because in_trial is not setup properly. > > Signed-off-by: Michal Simek > --- Acked-by: Jassi Brar
Re: [PATCH] fwu: Show number of attempts in Trial State
On Fri, 14 Jul 2023 at 03:47, Michal Simek wrote: > > It is not visible anywhere in Trial State if this is the first, second, etc > attempt that's why show a message to be aware about status. > > Signed-off-by: Michal Simek > --- Acked-by: Jassi Brar
Re: [PATCH] fwu: mtd: Fix dfu_alt_info generation for 2 images per bank
On Thu, 13 Jul 2023 at 09:36, Michal Simek wrote: > > Code rewrites the last char of size with adding &. It is visible from > dfu_alt_info print before this patch: > > Make dfu_alt_info: 'mtd nor0=bank0 raw 232 8;bank1 raw 27a > 8000 nor0=bank0 raw 23a 400;bank1 raw 282 400' > > And after it: > Make dfu_alt_info: 'mtd nor0=bank0 raw 232 8;bank1 raw 27a > 8 nor0=bank0 raw 23a 400;bank1 raw 282 400' > > Size for bank0 and bank1 must be the same because it is the same image. > > Signed-off-by: Michal Simek Acked-by: Jassi Brar
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
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] configs: synquacer: increase SYS_MALLOC_F_LEN
On Thu, 1 Jun 2023 at 04:14, Masahisa Kojima wrote:
>
> DM_FLAG_PRE_RELOC flag is added into some drivers
> by recent commits such as
> 1bd790bc4b ("firmware: psci: enable DM_FLAG_PRE_RELOC").
> Current SYS_MALLOC_F_LEN of SynQuacer Developerbox platform
> is too small, Developerbox will not boot due to lack of
> heap memory.
>
> This commit increases the size of heap memory.
>
> Signed-off-by: Masahisa Kojima
Acked-by: Jassi Brar
Re: [PATCH] dt/bindings: fwu-mdata-mtd: drop changes outside FWU
On Thu, 4 May 2023 at 11:31, Ilias Apalodimas wrote: > > Replying to both Jassi and Tom here since it makes more sense, > > On Thu, 4 May 2023 at 19:19, Tom Rini wrote: > > > > On Thu, May 04, 2023 at 10:39:06AM -0500, Jassi Brar wrote: > > > On Thu, 4 May 2023 at 10:19, Rob Herring wrote: > > > > On Thu, May 4, 2023 at 9:01 AM Jassi Brar > > > > wrote: > > > > > > > > > I may be wrong, but I see having fwu properties contained within the > > > > > fwu node is cleaner than having them embedded into existing bindings > > > > > (potentially different classes in future). So I moved to the current > > > > > design. > > > > > > > > Having all the information related to a device/node in one place is > > > > cleaner IMO. > > > > > > > > As I said, if u-boot wants private interfaces between the DT and > > > > itself, then fine, but that should remain private and be stripped by > > > > u-boot. A separate node would certainly be easier for doing that. > > > > > > > Seems we are on the same page(?). Current implementation does exactly > > > that -- we have a separate fwu node containing all the properties it > > > needs. > > I think Rob is saying the exact opposite. The way I see it we either > - Keep the bindings as an internal u-boot ABI, in which case the > current format is fine, but we need to strip it from the DT before > handing it over to the OS. > I interpreted "Having all the information related to a device/node in one place is cleaner IMO." to suggest this approach. Though the stripping part remains tbd. Where should that be done in u-boot? > - Alternatively, if we want to submit it upstream, we need to change > where that data lives and ideally have them under existing partition > bindings. > Here we may have to add to bindings of various subsystems (as support widens) and still have to strip the property before handover to the kernel. right? > Both would work, with the latter offering a bit more standardization > if another bootloader tries to implement something similar. > > > > > Well, isn't part of why we're here is that this isn't strictly a U-Boot > > only thing? My question is can, and then is, this also being used in > > other projects yet? > > I am not aware of any other projects currently using it. > Maybe I am overlooking something, but the kernel should not have anything to do with it. EDK2 may use the node as such and BL1/2 would have the meta-data location info hardcoded into them (?) thanks.
Re: [PATCH] dt/bindings: fwu-mdata-mtd: drop changes outside FWU
On Thu, 4 May 2023 at 10:19, Rob Herring wrote: > On Thu, May 4, 2023 at 9:01 AM Jassi Brar wrote: > > > I may be wrong, but I see having fwu properties contained within the > > fwu node is cleaner than having them embedded into existing bindings > > (potentially different classes in future). So I moved to the current > > design. > > Having all the information related to a device/node in one place is cleaner > IMO. > > As I said, if u-boot wants private interfaces between the DT and > itself, then fine, but that should remain private and be stripped by > u-boot. A separate node would certainly be easier for doing that. > Seems we are on the same page(?). Current implementation does exactly that -- we have a separate fwu node containing all the properties it needs. thanks.
Re: [PATCH] dt/bindings: fwu-mdata-mtd: drop changes outside FWU
On Thu, 4 May 2023 at 07:08, Ilias Apalodimas wrote: > [...] > > > > > I'm assuming it's per partition type rather than storage medium (e.g. > > > > SATA, USB, SD, NAND, SPI-NOR)? GPT, 'fixed-partitions', other DT > > > > partition bindings, etc. If so, then I'm really wondering why it's a > > > > standalone tree rather than integrated into those existing partition > > > > bindings. > > > > > > I think it's per medium, unless I misunderstood something. Sughosh? > > > > The bindings would be required as per the metadata access mechanism. > > So for example, the MTD bindings would suffice for all the storage > > mediums which would have MTD based partitioning schemes. Same for all > > GPT partitioned devices. Again, as for the need for a separate node > > with the compatible property, it is as per the driver model design. > > For the other properties, we can indeed have them integrated with the > > corresponding partition bindings. > > Ok, Rob is correct then it really is per partition type. > Originally the fwu related properties were embedded into existing nodes. As Sughosh already explained, we need a compatible string, and hence a node, for the u-boot driver core to call probe(). I may be wrong, but I see having fwu properties contained within the fwu node is cleaner than having them embedded into existing bindings (potentially different classes in future). So I moved to the current design. cheers.
Re: [PATCH v4 3/6] tools: Add mkfwumdata tool for FWU metadata image
On Tue, 18 Apr 2023 at 20:46, Simon Glass wrote: > > Hi, > > On Fri, 14 Apr 2023 at 07:53, Michal Simek wrote: > > > > > > > > On 4/10/23 06:25, Jassi Brar wrote: > > > On Wed, 29 Mar 2023 at 15:02, Simon Glass wrote: > > >> > > >> Hi, > > >> > > >> On Tue, 28 Mar 2023 at 10:16, wrote: > > >>> > > >>> From: Masami Hiramatsu > > >>> > > >>> Add 'mkfwumdata' tool to generate FWU metadata image for the meta-data > > >>> partition to be used in A/B Update imeplementation. > > >>> > > >>> Signed-off-by: Masami Hiramatsu > > >>> Signed-off-by: Sughosh Ganu > > >>> Signed-off-by: Jassi Brar > > >>> --- > > >>> tools/Kconfig | 9 ++ > > >>> tools/Makefile | 4 + > > >>> tools/mkfwumdata.c | 334 + > > >>> 3 files changed, 347 insertions(+) > > >>> create mode 100644 tools/mkfwumdata.c > > >> > > >> Can you please look at putting this in binman instead, since we would > > >> rather not have another tool with no tests. > > >> > > > Must I do that? I have no history with binman and it seems the > > > mkfwumdata.c would need to be rewritten in python? > > > > I think it is about calling this utility from python not about rewriting it > > to > > python. > > Yes that's the question. If this tool is for creating firmware > updates, then how are they created? I would expect binman to handle > this when U-Boot is built. Then you can build in some tests in binman > perhaps? > The FWU meta-data format is specified in a standards document created by ARM and not tied to u-boot. U-Boot may not necessarily be the bootloader using the output of mkfwumdata. The u-boot/tools/ is more like a welcoming host. Ideally mkfwumdata should be a reference implementation, also created by ARM. But it is trivial enough that nobody thought there could be any confusion about the format, I guess. > How does one know what parameters to pass? Is the documentation for > this tool elsewhere? > In the latest submission I also created a man-page for it. > Where are the tests? > I am open to learning what could be tested and how. > It is also unfortunate that this seems to be inventing yet another > format (I recall that FIP was invented at one point also), when it > could use FIT. > Hopefully it won't be that bad of a predicament after my explanation above. cheers.
Re: [PATCH v4 3/6] tools: Add mkfwumdata tool for FWU metadata image
On Mon, 17 Apr 2023 at 09:29, Michal Simek wrote: > > > > On 4/17/23 15:48, Jassi Brar wrote: > > On Mon, 17 Apr 2023 at 01:38, Michal Simek wrote: > >> On 4/14/23 17:02, Jassi Brar wrote: > > > >>>>>>> + > >>>>>>> +/* This will dynamically allocate the fwu_mdata */ > >>>>>>> +#define CONFIG_FWU_NUM_BANKS 0 > >>>>>>> +#define CONFIG_FWU_NUM_IMAGES_PER_BANK 0 > >>>>>> > >>>>>> These two are completely unused. > >>>>>> > >>>>> these are necessary for include/fwu_mdata.h that comes later. > >>>> > >>>> If that's come later, add it later. > >>>> > >>> Can you please actually look at the code to see the underlying constraint? > >> > >> Ok. I misunderstand your comment. > >> > >> You have it there because of #include and using macros there. > >> Can you just allocated that structures at run time instead of statically > >> defined > >> them via array? That would clean that design and also fix checkpatch issue. > >> > > I can't see how dynamically allocating an array of packed structures > > inside an array of packed structures , makes the design any cleaner. > > It is not marked as __packed and based on what you are saying it should be > marked like that. > Those structures already existed before my patchset. And I definitely remember they were suggested to be marked as packed but were not. So now you have to read to code to understand what they are. > > > > I think this is a valid case where we can ignore the checkpatch > > warning because we know what we are doing. > > I will let maintainer, who will merge this code, to decide on this. > I would at least make that comment much bigger to explain it better > that you are doing it because you don't want to redefine structures from > fwu_mdata.h. > you mean redeclare, right? That is what the two define's do effectively. -j
Re: [PATCH v4 3/6] tools: Add mkfwumdata tool for FWU metadata image
On Mon, 17 Apr 2023 at 01:38, Michal Simek wrote: > On 4/14/23 17:02, Jassi Brar wrote: > >>>>> + > >>>>> +/* This will dynamically allocate the fwu_mdata */ > >>>>> +#define CONFIG_FWU_NUM_BANKS 0 > >>>>> +#define CONFIG_FWU_NUM_IMAGES_PER_BANK 0 > >>>> > >>>> These two are completely unused. > >>>> > >>> these are necessary for include/fwu_mdata.h that comes later. > >> > >> If that's come later, add it later. > >> > > Can you please actually look at the code to see the underlying constraint? > > Ok. I misunderstand your comment. > > You have it there because of #include and using macros there. > Can you just allocated that structures at run time instead of statically > defined > them via array? That would clean that design and also fix checkpatch issue. > I can't see how dynamically allocating an array of packed structures inside an array of packed structures , makes the design any cleaner. I think this is a valid case where we can ignore the checkpatch warning because we know what we are doing. -jassi
Re: [PATCH v4 1/6] FWU: Add FWU metadata access driver for MTD storage regions
On Fri, Apr 14, 2023 at 8:56 AM Michal Simek wrote: > On 4/10/23 05:56, Jassi Brar wrote: > > On Wed, 29 Mar 2023 at 07:00, Michal Simek wrote: > >> On 3/27/23 23:15, jassisinghb...@gmail.com wrote: > > > >>> diff --git a/drivers/fwu-mdata/raw_mtd.c b/drivers/fwu-mdata/raw_mtd.c > >>> new file mode 100644 > >>> index 00..4b1a10073a > >>> --- /dev/null > >>> +++ b/drivers/fwu-mdata/raw_mtd.c > >>> @@ -0,0 +1,272 @@ > >>> +// SPDX-License-Identifier: GPL-2.0+ > >> > >> Just a note: Did you choose GPL-2.0+ by purpose? Or it is just c? > >> > > just c though isn't that the same as GPL-2.0-or-later ? > > license choice is up to you. We normally use just gpl-2.0. > I think more than "we", the subsystem dictates licensing. All FWU code is under GPL-2.0-or-later. > >> > >> As I said this DT binding should be approved first to make sure that we > >> don't > >> need to fix DT binding in future. Just simply do it right from the > >> begining. > >> > > Yes, I will cc Rob in the next submission (I only forgot last time). > > However, let us note that fwu-mdata-gpt.yaml isn't blessed either. > > I am not sure if there is any reason for the fwu node to even be in > > the dts for kernel. But sure it is good to have it eyeballed by the DT > > gods. > > It doesn't really go to kernel. > Simon pushed options node directly to dt-schema > https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/options/u-boot.yaml#L96 > And that would be also location for this node too. > Yes, but I have resend the already existisng bindings in uboot. My patch only modified them. Not a big problem. -j
Re: [PATCH v4 3/6] tools: Add mkfwumdata tool for FWU metadata image
On Fri, Apr 14, 2023 at 8:53 AM Michal Simek wrote: > > > > On 4/10/23 06:05, Jassi Brar wrote: > > On Wed, 29 Mar 2023 at 07:29, Michal Simek wrote: > >> On 3/27/23 23:16, jassisinghb...@gmail.com wrote: > > > >>> diff --git a/tools/mkfwumdata.c b/tools/mkfwumdata.c > >>> new file mode 100644 > >>> index 00..43dabf3b72 > >>> --- /dev/null > >>> +++ b/tools/mkfwumdata.c > >>> @@ -0,0 +1,334 @@ > >>> +// SPDX-License-Identifier: GPL-2.0+ > >>> +/* > >>> + * Copyright (c) 2023, Linaro Limited > >>> + */ > >>> + > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> + > >>> +/* This will dynamically allocate the fwu_mdata */ > >>> +#define CONFIG_FWU_NUM_BANKS 0 > >>> +#define CONFIG_FWU_NUM_IMAGES_PER_BANK 0 > >> > >> These two are completely unused. > >> > > these are necessary for include/fwu_mdata.h that comes later. > > If that's come later, add it later. > Can you please actually look at the code to see the underlying constraint? -j
Re: [PATCH v4 3/6] tools: Add mkfwumdata tool for FWU metadata image
On Wed, 29 Mar 2023 at 15:02, Simon Glass wrote: > > Hi, > > On Tue, 28 Mar 2023 at 10:16, wrote: > > > > From: Masami Hiramatsu > > > > Add 'mkfwumdata' tool to generate FWU metadata image for the meta-data > > partition to be used in A/B Update imeplementation. > > > > Signed-off-by: Masami Hiramatsu > > Signed-off-by: Sughosh Ganu > > Signed-off-by: Jassi Brar > > --- > > tools/Kconfig | 9 ++ > > tools/Makefile | 4 + > > tools/mkfwumdata.c | 334 + > > 3 files changed, 347 insertions(+) > > create mode 100644 tools/mkfwumdata.c > > Can you please look at putting this in binman instead, since we would > rather not have another tool with no tests. > Must I do that? I have no history with binman and it seems the mkfwumdata.c would need to be rewritten in python? regards.
Re: [PATCH v4 6/6] fwu: DeveloperBox: add support for FWU
On Wed, 29 Mar 2023 at 08:02, Michal Simek wrote:
> On 3/27/23 23:16, jassisinghb...@gmail.com wrote:
.
> > +
> > +void fwu_plat_get_bootidx(uint *boot_idx)
> > +{
> > + int ret;
> > + u32 active_idx;
> > + u32 *bootidx = boot_idx;
> > +
> > + ret = fwu_get_active_index(_idx);
> > +
>
> nit: remove this newline
>
ok
> > + if (ret < 0)
> > + *bootidx = -1;
> > +
> > + *bootidx = active_idx;
>
> Is this logic here right?
> If fwu_get_active_index fails you setup bootidx to -1
> and right after it you rewrite it to active_idx initialized in
> fwu_get_active_index() to mdata->active_index.
>
> It means why to do *bootidx = -1; at all?
>
yes :) it's a silly remnant of history of changes.
Actually this goes away after implementing the default/weak function.
> > +}
> > diff --git a/configs/synquacer_developerbox_defconfig
> > b/configs/synquacer_developerbox_defconfig
> > index 09e12b739b..d09684153a 100644
> > --- a/configs/synquacer_developerbox_defconfig
> > +++ b/configs/synquacer_developerbox_defconfig
> > @@ -97,3 +97,11 @@ CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> > CONFIG_EFI_CAPSULE_ON_DISK=y
> > CONFIG_EFI_IGNORE_OSINDICATIONS=y
> > CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> > +CONFIG_EFI_SECURE_BOOT=y
> > +CONFIG_FWU_MULTI_BANK_UPDATE=y
> > +CONFIG_FWU_MDATA=y
> > +CONFIG_FWU_MDATA_MTD=y
> > +CONFIG_FWU_NUM_BANKS=2
> > +CONFIG_FWU_NUM_IMAGES_PER_BANK=1
> > +CONFIG_CMD_FWU_METADATA=y
> > +CONFIG_TOOLS_MKFWUMDATA=y
>
> doesn't look like that it was created via savedefconfig.
>
Yes. I had some other config changes too and picked only the relevant
ones together.
> > +And make a FIP image.::
> > +
> > + cp build/synquacer/release/fip.bin SPI_NOR_NEWFIP.fd
> > + tools/fiptool/fiptool update --tb-fw build/synquacer/release/bl2.bin
> > SPI_NOR_NEWFIP.fd
> > +
> > +UUIDs for the FWU Multi Bank Update
> > +---
> > +
> > +FWU multi-bank update requires some UUIDs. The DeveloperBox platform uses
> > +following UUIDs.
> > +
> > + - Location UUID for the FIP image: 17e86d77-41f9-4fd7-87ec-a55df9842de5
>
>
> In past you have it listed at flash node in DT. I see you have removed it
> between v3 and v4 without any note about it.
> Is it still needed? And should it be listed in DT spec again?
>
After the dt change, we no longer require this. But the location_uuid
is a standard member of an fwu_image_entry and cmd/fwu_mdata.c always
print it. So I think this should be seen as just what a platform wants
some unique id to be printed for the image (?).
Thanks Michal for reviewing v4.
cheers.
Re: [PATCH v4 3/6] tools: Add mkfwumdata tool for FWU metadata image
On Thu, 30 Mar 2023 at 01:12, Heinrich Schuchardt wrote:
>
> Am 27. März 2023 23:16:10 MESZ schrieb jassisinghb...@gmail.com:
> >From: Masami Hiramatsu
> >
> >Add 'mkfwumdata' tool to generate FWU metadata image for the meta-data
> >partition to be used in A/B Update imeplementation.
> >
> >Signed-off-by: Masami Hiramatsu
> >Signed-off-by: Sughosh Ganu
> >Signed-off-by: Jassi Brar
> >---
> > tools/Kconfig | 9 ++
> > tools/Makefile | 4 +
> > tools/mkfwumdata.c | 334 +
> > 3 files changed, 347 insertions(+)
> > create mode 100644 tools/mkfwumdata.c
>
> For each command line tool there should be a man-page.
>
ok
.
> >+
> >+/* This will dynamically allocate the fwu_mdata */
> >+#define CONFIG_FWU_NUM_BANKS 0
> >+#define CONFIG_FWU_NUM_IMAGES_PER_BANK0
> >+
> >+/* Since we can not include fwu.h, redefine version here. */
> >+#define FWU_MDATA_VERSION 1
> >+
> >+typedef uint8_t u8;
> >+typedef int16_t s16;
> >+typedef uint16_t u16;
> >+typedef uint32_t u32;
> >+typedef uint64_t u64;
> >+
> >+#include
> >+
> >+/* TODO: Endianness conversion may be required for some arch. */
>
> Does the update work without UEFI? UEFI requires low endianness.
>
Not sure what you mean. This only creates a meta-data image that goes
into some offset in mtd.
> >+static void print_usage(void)
> >+{
> >+ fprintf(stderr, "Usage: mkfwumdata [options] >file>\n");
> >+ fprintf(stderr, "Options:\n"
> >+ "\t-i, --images Number of images\n"
> >+ "\t-b, --banksNumber of banks\n"
> >+ "\t-a, --active-bank Active bank\n"
> >+ "\t-p, --previous-bankPrevious active bank\n"
> >+ "\t-g, --guid Use GUID instead of UUID\n"
>
> I would not know what this means.
>
> Why can't you supply that single GUID in the position of the UUIDs parameter?
>
I guess the original authors wanted to have one explicitly specify
what's going on, rather than 'hacking' around (?)
thanks
Re: [PATCH v4 3/6] tools: Add mkfwumdata tool for FWU metadata image
On Wed, 29 Mar 2023 at 07:29, Michal Simek wrote: > On 3/27/23 23:16, jassisinghb...@gmail.com wrote: > > diff --git a/tools/mkfwumdata.c b/tools/mkfwumdata.c > > new file mode 100644 > > index 00..43dabf3b72 > > --- /dev/null > > +++ b/tools/mkfwumdata.c > > @@ -0,0 +1,334 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Copyright (c) 2023, Linaro Limited > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +/* This will dynamically allocate the fwu_mdata */ > > +#define CONFIG_FWU_NUM_BANKS 0 > > +#define CONFIG_FWU_NUM_IMAGES_PER_BANK 0 > > These two are completely unused. > these are necessary for include/fwu_mdata.h that comes later. cheers.
Re: [PATCH v4 2/6] FWU: mtd: Add helper functions for accessing FWU metadata
On Wed, 29 Mar 2023 at 06:55, Michal Simek wrote:
> On 3/27/23 23:16, jassisinghb...@gmail.com wrote:
> > +/**
> > + * fwu_gen_alt_info_from_mtd() - Parse dfu_alt_info from metadata in mtd
> > + * @buf: Buffer into which the dfu_alt_info is filled
> > + * @len: Maximum characters that can be written in buf
> > + * @mtd: Pointer to underlying MTD device
> > + *
> > + * Parse dfu_alt_info from metadata in mtd. Used for setting the env.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
>
> nit: remove this line above.
>
ok
> > + */
> > +int fwu_gen_alt_info_from_mtd(char *buf, size_t len, struct mtd_info *mtd);
> > +
> > +/**
> > + * fwu_mtd_get_alt_num() - Mapping of fwu_plat_get_alt_num for MTD device
> > + * @image_guid: Image GUID for which DFU alt number needs to be retrieved
>
> it doesn't match with parameter below.
>
ok
> > + * @alt_num: Pointer to the alt_num
> > + * @mtd_dev: Name of mtd device instance
> > + *
> > + * To map fwu_plat_get_alt_num onto mtd based metadata implementation.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
>
> nit: remove this line above
>
ok
> > + */
> > +int fwu_mtd_get_alt_num(efi_guid_t *image_id, u8 *alt_num, const char
> > *mtd_dev);
>
>
> ./scripts/kernel-doc -v -man include/fwu.h 1>/dev/null
>
> include/fwu.h:276: info: Scanning doc for fwu_mtd_get_alt_num
> include/fwu.h:286: warning: Function parameter or member 'image_id' not
> described in 'fwu_mtd_get_alt_num'
> include/fwu.h:286: warning: Excess function parameter 'image_guid' description
> in 'fwu_mtd_get_alt_num'
>
ok
> > +
> > +static struct fwu_mtd_image_info *mtd_img_by_uuid(const char *uuidbuf)
> > +{
> > + int num_images = sizeof(fwu_mtd_images) / sizeof(struct
> > fwu_mtd_image_info);
>
> include/linux/kernel.h:55:#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>
of course. thanks.
> > +int fwu_mtd_get_alt_num(efi_guid_t *image_id, u8 *alt_num,
> > + const char *mtd_dev)
> > +{
> > + int i, nalt;
> > + int ret = -1;
>
> nit: can be on the same line.
>
ok
> > + struct mtd_info *mtd;
> > + struct dfu_entity *dfu;
> > + fdt_addr_t offset, size = 0;
> > + char uuidbuf[UUID_STR_LEN + 1];
> > + struct fwu_mtd_image_info *mtd_img_info;
>
> nit: reverse christmas tree order
>
ok
> > +
> > + mtd_probe_devices();
> > + mtd = get_mtd_device_nm(mtd_dev);
>
> you are getting link but you are not using it anywhere.
> You should check return value or remove this call completely.
>
This should go. thanks.
> > +
> > + uuid_bin_to_str(image_id->b, uuidbuf, UUID_STR_FORMAT_STD);
> > +
> > + mtd_img_info = mtd_img_by_uuid(uuidbuf);
> > + if (!mtd_img_info) {
> > + log_err("%s: Not found partition for image %s\n", __func__,
> > uuidbuf);
> > + return -ENOENT;
> > + }
>
> nit: newline
>
ok
> > + offset = mtd_img_info->start;
> > + size = mtd_img_info->size;
> > +
> > + dfu_init_env_entities(NULL, NULL);
>
> worth to check return value here.
>
ok, though it would also cleanly fail immediately after this call when
it find empty dfu_list.
Thanks.
Re: [PATCH v4 1/6] FWU: Add FWU metadata access driver for MTD storage regions
On Wed, 29 Mar 2023 at 07:00, Michal Simek wrote:
> On 3/27/23 23:15, jassisinghb...@gmail.com wrote:
> > diff --git a/drivers/fwu-mdata/raw_mtd.c b/drivers/fwu-mdata/raw_mtd.c
> > new file mode 100644
> > index 00..4b1a10073a
> > --- /dev/null
> > +++ b/drivers/fwu-mdata/raw_mtd.c
> > @@ -0,0 +1,272 @@
> > +// SPDX-License-Identifier: GPL-2.0+
>
> Just a note: Did you choose GPL-2.0+ by purpose? Or it is just c?
>
just c though isn't that the same as GPL-2.0-or-later ?
...
> > +
> > +extern struct fwu_mtd_image_info fwu_mtd_images[];
>
> if there is a need to share it. It should go to header.
>
include/fwu,h would be the header. Which is supposed to be very
global, and doesn't seem very appropriate to mention private data
shared between a driver and helper library.
If people still insist, I will make the change.
> And fwu_mtd_images[] is not defined when only this patch is applied.
> It means order of patches is not right. 1/6 and 2/6 should be swapped
>
I think 1 and 2 should be merged rather.
..
> > + if (!mtd_priv->mtd) {
> > + log_err("Failed to find mtd device by fwu-mdata-store\n");
> > + return -ENODEV;
> > + }
> > +
> > + /* Get the offset of primary and seconday mdata */
>
>
> typo
>
ok
..
> > +
> > +static const struct udevice_id fwu_mdata_ids[] = {
> > + { .compatible = "u-boot,fwu-mdata-mtd" },
> > + { }
> > +};
>
> As I said this DT binding should be approved first to make sure that we don't
> need to fix DT binding in future. Just simply do it right from the begining.
>
Yes, I will cc Rob in the next submission (I only forgot last time).
However, let us note that fwu-mdata-gpt.yaml isn't blessed either.
I am not sure if there is any reason for the fwu node to even be in
the dts for kernel. But sure it is good to have it eyeballed by the DT
gods.
cheers.
Re: [PATCH] spi: synquacer: Silence unused variable warnings
On Fri, 7 Apr 2023 at 04:13, Ilias Apalodimas wrote: > > When building with clang, the compiler compains with > > drivers/spi/spi-synquacer.c:212:11: warning: variable 'bus_width' is used > uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized] > else if (priv->mode & SPI_TX_OCTAL) > ^ > drivers/spi/spi-synquacer.c:276:11: note: uninitialized use occurs here > val |= ((bus_width >> 1) << BUS_WIDTH); > ^ > drivers/spi/spi-synquacer.c:212:7: note: remove the 'if' if its condition is > always true > else if (priv->mode & SPI_TX_OCTAL) > ^~ > drivers/spi/spi-synquacer.c:189:25: note: initialize the variable 'bus_width' > to silence this warning > > So initialize bus_width to 1 and add a warning if none of the configured > modes matches > > Signed-off-by: Ilias Apalodimas > Acked-by: Jassi Brar
Re: [PATCH v5 0/6] FWU: Handle meta-data in common code
On Mon, Feb 27, 2023 at 7:28 PM Tom Rini wrote: > > On Mon, Feb 27, 2023 at 07:00:10PM -0600, Jassi Brar wrote: > > On Mon, 27 Feb 2023 at 18:58, Tom Rini wrote: > > > > > > On Mon, Feb 27, 2023 at 06:51:35PM -0600, jassisinghb...@gmail.com wrote: > > > > > > > From: Jassi Brar > > > > > > > > The patchset reduces ~400 lines of code, while keeping the > > > > functionality same and making > > > > meta-data operations much faster (by using cached structures). > > > > > > > > Issue: > > > > meta-data copies (primary and secondary) are being handled by the > > > > backend/storage layer > > > > instead of the common core in fwu.c (as also noted by Ilias) that is, > > > > gpt_blk.c manages > > > > meta-data and similarly raw_mtd.c will have to do the same when it > > > > arrives. The code > > > > could by make smaller, cleaner and optimised. > > > > > > > > Basic idea: > > > > Introduce .read_mdata() and .write_mdata() in fwu_mdata_ops that > > > > simply read/write > > > > meta-data copy. The core code takes care of integrity and redundancy of > > > > the meta-data, > > > > as a result we can get rid of every other callback .get_mdata() > > > > .update_mdata() > > > > .get_mdata_part_num() .read_mdata_partition() > > > > .write_mdata_partition() and the > > > > corresponding wrapper functions thereby making the code 100s of LOC > > > > smaller. > > > > > > > > Get rid of fwu_check_mdata_validity() and fwu_mdata_check() which > > > > expected underlying > > > > layer to manage and verify mdata copies. > > > > Implement fwu_get_verified_mdata(struct fwu_mdata *mdata) public > > > > function that reads, > > > > verifies and, if needed, fixes the meta-data copies. > > > > > > > > Verified copy of meta-data is now cached as 'g_mdata' in fwu.c, which > > > > avoids multiple > > > > low-level expensive read and parse calls. > > > > gpt meta-data partition numbers are now cached in gpt_blk.c, so that we > > > > don't have to do expensive part_get_info() and uid ops. > > > > > > > > Changes since v4: > > > > * Change fwu-mdata-mtd bindings to not require external changes > > > > * Handle 'part == BOTH_PARTS' in fwu_sync_mdata > > > > * use parts_ok[] and parts_mdata[] instead of pri/sec_ok and > > > > p/s_mdata > > > > > > Did you run this through CI / build sandbox? This doesn't read like you > > > fixed the problem I reported in CI, when I was trying to merge v4. > > > > > I know that remains to be done. > > The dt-bindings for fwu-mdata is changed in this patchset and I > > thought any testcase may be impacted by it. > > So you're not expecting this iteration to be merged, as CI doesn't pass, > and that's known? OK. > Sorry I got confused. I thought new test cases are expected to be written, whereas you only wanted the sandbox compilation breakage fixed. My replies must have sounded so stupid :( I have a patch to fix sandbox compilation in the v6 revision. thanks.
Re: [PATCH v5 3/6] fwu: move meta-data management in core
On Wed, Mar 1, 2023 at 5:16 AM Etienne Carriere
wrote:
> On Tue, 28 Feb 2023 at 01:52, wrote:
> >
> > From: Jassi Brar
> >
> > Instead of each i/f having to implement their own meta-data verification
> > and storage, move the logic in common code. This simplifies the i/f code
> > much simpler and compact.
> >
...
> > diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c
> > b/drivers/fwu-mdata/fwu-mdata-uclass.c
> > index b477e9603f..e03773c584 100644
> > --- a/drivers/fwu-mdata/fwu-mdata-uclass.c
> > +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c
> > @@ -16,6 +16,40 @@
> > #include
> > #include
> >
> > +/**
> > + * fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata()
> > + *
> > + * Return: 0 if OK, -ve on error
> > + */
>
> Inline description only in header file, or duplicated in source and
> header files?
>
This is the original practice in FWU stack - to have the description
in header as well as source code. I just didn't want to stick out.
> > diff --git a/include/fwu.h b/include/fwu.h
> > index 0919ced812..1a700c9e6a 100644
> > --- a/include/fwu.h
> > +++ b/include/fwu.h
> > @@ -24,6 +24,26 @@ struct fwu_mdata_gpt_blk_priv {
> > * @update_mdata() - Update the FWU metadata copy
> > */
> > struct fwu_mdata_ops {
> > + /**
> > +* read_mdata() - Populate the asked FWU metadata copy
> > +* @dev: FWU metadata device
> > +* @mdata: Copy of the FWU metadata
>
> @mdata: Output FWU mdata read
>
> > +* @primary: If primary or secondary copy of meta-data is to be read
>
> s/meta-data/FWU metadata/
> Ditto in .write_mdata description
>
ok
> > +/**
> > + * fwu_get_verified_mdata() - Read, verify and return the FWU metadata
> > + *
> > + * Read both the metadata copies from the storage media, verify their
> > checksum,
> > + * and ascertain that both copies match. If one of the copies has gone bad,
> > + * restore it from the good copy.
>
> @mdata: Output FWU metadata read or NULL
>
ok
> > +static int fwu_sync_mdata(struct fwu_mdata *mdata, int part)
> > +{
> > + void *buf = >version;
> > + int err;
> > +
> > + if (part == BOTH_PARTS) {
> > + err = fwu_sync_mdata(mdata, SECONDARY_PART);
> > + if (err)
> > + return err;
> > + part = PRIMARY_PART;
> > + }
> > +
> > + /*
> > +* Calculate the crc32 for the updated FWU metadata
> > +* and put the updated value in the FWU metadata crc32
> > +* field
> > +*/
> > + mdata->crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
> > +
> > + err = fwu_write_mdata(g_dev, mdata, part & PRIMARY_PART ? true :
> > false);
>
> Since this expects part is either PRIMARY_PART or SECONDARY_PART, prefer:
> err = fwu_write_mdata(g_dev, mdata, part == PRIMARY_PART);
>
> And ditto below:
> part == PRIMARY_PART ? "primary": "secondary");
>
yes, now that we handle out the BOTH_PARTS case already.
> > +int fwu_get_verified_mdata(struct fwu_mdata *mdata)
> > +{
> > + int err;
> > + bool parts_ok[2] = { false };
> > + struct fwu_mdata s, *parts_mdata[2];
> > +
> > + parts_mdata[0] = _mdata;
> > + parts_mdata[1] =
> > +
> > + /* if mdata already read and ready */
> > + err = mdata_crc_check(parts_mdata[0]);
> > + if (!err)
> > + goto ret_mdata;
> > + /* else read, verify and, if needed, fix mdata */
> > +
> > + for (int i = 0; i < 2; i++) {
>
> Define "int i;" at function block entry?
>
Hmm... I prefer this way - limiting scope of the scratch variables.
thanks.
Re: [PATCH v5 1/6] dt/bindings: fwu-mdata-mtd: drop changes outside FWU
On Tue, Feb 28, 2023 at 4:50 AM Michal Simek wrote: > On 2/28/23 01:52, jassisinghb...@gmail.com wrote: > > From: Jassi Brar > > > > Any requirement of FWU should not require changes to bindings > > of other subsystems. For example, for mtd-backed storage we > > can do without requiring 'fixed-partitions' children to also > > carry 'uuid', a property which is non-standard and not in the > > bindings. > > > > There exists no code yet, so we can change the fwu-mtd bindings > > to contain all properties within the fwu-mdata node. > > > > Signed-off-by: Jassi Brar > > --- > > .../firmware/fwu-mdata-mtd.yaml | 105 +++--- > > 1 file changed, 91 insertions(+), 14 deletions(-) > > > > diff --git a/doc/device-tree-bindings/firmware/fwu-mdata-mtd.yaml > > b/doc/device-tree-bindings/firmware/fwu-mdata-mtd.yaml > > index 4f5404f999..4b87fb8624 100644 > > --- a/doc/device-tree-bindings/firmware/fwu-mdata-mtd.yaml > > +++ b/doc/device-tree-bindings/firmware/fwu-mdata-mtd.yaml > > @@ -1,13 +1,13 @@ > > # SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > %YAML 1.2 > > --- > > -$id: http://devicetree.org/schemas/firmware/u-boot,fwu-mdata-sf.yaml# > > -$schema: http://devicetree.org/meta-schemas/core.yaml# > > +$id: http://devicetree.org/schemas/firmware/u-boot,fwu-mdata-mtd.yaml# > > +$schema: http://devicetree.org/meta-schemas/base.yaml# > > > > title: FWU metadata on MTD device without GPT > > > > maintainers: > > - - Masami Hiramatsu > > + - Jassi Brar > > > > properties: > > compatible: > > @@ -15,24 +15,101 @@ properties: > > - const: u-boot,fwu-mdata-mtd > > > > fwu-mdata-store: > > -maxItems: 1 > > -description: Phandle of the MTD device which contains the FWU medatata. > > +$ref: /schemas/types.yaml#/definitions/phandle > > +description: Phandle of the MTD device which contains the FWU MetaData > > and Banks. > > > > - mdata-offsets: > > + mdata-parts: > > +$ref: /schemas/types.yaml#/definitions/non-unique-string-array > > minItems: 2 > > -description: Offsets of the primary and secondary FWU metadata in the > > NOR flash. > > +maxItems: 2 > > +description: labels of the primary and secondary FWU metadata > > partitions in the 'fixed-partitions' subnode of the 'jedec,spi-nor' flash > > device node. > > + > > + patternProperties: > > +"fwu-bank@[0-9]": > > +type: object > > +description: List of FWU mtd-backed banks. Typically two banks. > > + > > +properties: > > + id: > > +$ref: /schemas/types.yaml#/definitions/uint32 > > +description: Index of the bank. > > + > > + label: > > +$ref: /schemas/types.yaml#/definitions/non-unique-string-array > > +minItems: 1 > > +maxItems: 1 > > +description: label of the partition, in the 'fixed-partitions' > > subnode of the 'jedec,spi-nor' flash device node, that holds this bank. > > + > > + patternProperties: > > +"fwu-image@[0-9]": > > +type: object > > +description: List of images in the FWU mtd-backed bank. > > + > > +properties: > > + id: > > +$ref: /schemas/types.yaml#/definitions/uint32 > > +description: Index of the bank. > > + > > + offset: > > +$ref: /schemas/types.yaml#/definitions/uint32 > > +description: Offset, from start of the bank, where the image > > is located. > > + > > + size: > > +$ref: /schemas/types.yaml#/definitions/uint32 > > +description: Size reserved for the image. > > + > > + uuid: > > +$ref: /schemas/types.yaml#/definitions/non-unique-string-array > > +minItems: 1 > > +maxItems: 1 > > +description: UUID of the image. > > + > > +required: > > + - id > > + - offset > > + - size > > + - uuid > > +additionalProperties: false > > + > > +required: > > + - id > > + - label > > + - fwu-images > > +additionalProperties: false > > > > required: > > - compatible > > - fwu-mdata-store > > - - mdata-offsets > > - > > + - mdata-parts > > + - fwu-banks
Re: [PATCHv4 2/5] fwu: move meta-data management in core
Hi Ilias,
On Thu, Feb 23, 2023 at 2:36 AM Ilias Apalodimas
wrote:
> > +int fwu_get_verified_mdata(struct fwu_mdata *mdata)
> > +{
> > + int err;
> > + bool pri_ok, sec_ok;
> > + struct fwu_mdata s, *p_mdata, *s_mdata;
> > +
> > + p_mdata = _mdata;
> > + s_mdata =
> > +
> > + /* if mdata already read and ready */
> > + err = mdata_crc_check(p_mdata);
> > + if (!err)
> > + goto ret_mdata;
> > + /* else read, verify and, if needed, fix mdata */
> > +
> > + pri_ok = false;
> > + err = fwu_read_mdata(g_dev, p_mdata, true);
> > + if (!err) {
> > + err = mdata_crc_check(p_mdata);
> > + if (!err)
> > + pri_ok = true;
> > + else
> > + log_debug("primary mdata: crc32 failed\n");
> > + }
> > +
> > + sec_ok = false;
> > + err = fwu_read_mdata(g_dev, s_mdata, false);
> > + if (!err) {
> > + err = mdata_crc_check(s_mdata);
> > + if (!err)
> > + sec_ok = true;
> > + else
> > + log_debug("secondary mdata: crc32 failed\n");
> > + }
>
> Isn't it better to define pri_ok, sec_ok and their equivalent mdata as
> arrays ? IOW something along the lines of
>
> bool parts_ok[2] = { false };
> struct fwu_mdata parts_mdata[2];
>
> parts_mdata[0] = _mdata;
> parts_mdata[1] = .
> for (i = 0; i < 2; i++) {
> err = fwu_read_mdata(g_dev, parts_mdata[i], !(i % 2) ? true : false);
> if (!err)
> err = mdata_crc_check(parts_mdata[i]);
> etc
> }
>
> > +
> > + if (pri_ok && sec_ok) {
>
> And then also adjust this part?
>
> > + /*
> > + * Before returning, check that both the
> > + * FWU metadata copies are the same.
> > + */
> > + err = memcmp(p_mdata, s_mdata, sizeof(struct fwu_mdata));
> > + if (!err)
> > + goto ret_mdata;
> > +
> > + /*
> > + * If not, populate the secondary partition from the
> > + * primary partition copy.
> > + */
> > + log_info("Both FWU metadata copies are valid but do not
> > match.");
> > + log_info(" Restoring the secondary partition from the
> > primary\n");
> > + sec_ok = false;
> > + }
> > +
> > + if (!pri_ok) {
> > + memcpy(p_mdata, s_mdata, sizeof(struct fwu_mdata));
> > + err = fwu_sync_mdata(p_mdata, PRIMARY_PART);
> > + if (err) {
> > + log_debug("mdata : primary write failed\n");
> > + return err;
> > + }
> > + }
> > +
> > + if (!sec_ok) {
> > + memcpy(s_mdata, p_mdata, sizeof(struct fwu_mdata));
> > + err = fwu_sync_mdata(s_mdata, SECONDARY_PART);
> > + if (err) {
> > + log_debug("mdata : secondary write failed\n");
> > + return err;
> > + }
> > + }
>
> And this could also be folded into a for loop
>
I have done these modifications and submitted v5.
Thanks.
Re: [PATCH v5 0/6] FWU: Handle meta-data in common code
On Mon, Feb 27, 2023 at 7:28 PM Tom Rini wrote: > > On Mon, Feb 27, 2023 at 07:00:10PM -0600, Jassi Brar wrote: > > On Mon, 27 Feb 2023 at 18:58, Tom Rini wrote: > > > > > > On Mon, Feb 27, 2023 at 06:51:35PM -0600, jassisinghb...@gmail.com wrote: > > > > > > > From: Jassi Brar > > > > > > > > The patchset reduces ~400 lines of code, while keeping the > > > > functionality same and making > > > > meta-data operations much faster (by using cached structures). > > > > > > > > Issue: > > > > meta-data copies (primary and secondary) are being handled by the > > > > backend/storage layer > > > > instead of the common core in fwu.c (as also noted by Ilias) that is, > > > > gpt_blk.c manages > > > > meta-data and similarly raw_mtd.c will have to do the same when it > > > > arrives. The code > > > > could by make smaller, cleaner and optimised. > > > > > > > > Basic idea: > > > > Introduce .read_mdata() and .write_mdata() in fwu_mdata_ops that > > > > simply read/write > > > > meta-data copy. The core code takes care of integrity and redundancy of > > > > the meta-data, > > > > as a result we can get rid of every other callback .get_mdata() > > > > .update_mdata() > > > > .get_mdata_part_num() .read_mdata_partition() > > > > .write_mdata_partition() and the > > > > corresponding wrapper functions thereby making the code 100s of LOC > > > > smaller. > > > > > > > > Get rid of fwu_check_mdata_validity() and fwu_mdata_check() which > > > > expected underlying > > > > layer to manage and verify mdata copies. > > > > Implement fwu_get_verified_mdata(struct fwu_mdata *mdata) public > > > > function that reads, > > > > verifies and, if needed, fixes the meta-data copies. > > > > > > > > Verified copy of meta-data is now cached as 'g_mdata' in fwu.c, which > > > > avoids multiple > > > > low-level expensive read and parse calls. > > > > gpt meta-data partition numbers are now cached in gpt_blk.c, so that we > > > > don't have to do expensive part_get_info() and uid ops. > > > > > > > > Changes since v4: > > > > * Change fwu-mdata-mtd bindings to not require external changes > > > > * Handle 'part == BOTH_PARTS' in fwu_sync_mdata > > > > * use parts_ok[] and parts_mdata[] instead of pri/sec_ok and > > > > p/s_mdata > > > > > > Did you run this through CI / build sandbox? This doesn't read like you > > > fixed the problem I reported in CI, when I was trying to merge v4. > > > > > I know that remains to be done. > > The dt-bindings for fwu-mdata is changed in this patchset and I > > thought any testcase may be impacted by it. > > So you're not expecting this iteration to be merged, as CI doesn't pass, > and that's known? OK. > I am more concerned if the bindings displease someone and I have to drop any code. If I get acks on all patches, the next revision will only add the test cases. thanks
Re: [PATCH v5 0/6] FWU: Handle meta-data in common code
On Mon, 27 Feb 2023 at 18:58, Tom Rini wrote: > > On Mon, Feb 27, 2023 at 06:51:35PM -0600, jassisinghb...@gmail.com wrote: > > > From: Jassi Brar > > > > The patchset reduces ~400 lines of code, while keeping the functionality > > same and making > > meta-data operations much faster (by using cached structures). > > > > Issue: > > meta-data copies (primary and secondary) are being handled by the > > backend/storage layer > > instead of the common core in fwu.c (as also noted by Ilias) that is, > > gpt_blk.c manages > > meta-data and similarly raw_mtd.c will have to do the same when it arrives. > > The code > > could by make smaller, cleaner and optimised. > > > > Basic idea: > > Introduce .read_mdata() and .write_mdata() in fwu_mdata_ops that simply > > read/write > > meta-data copy. The core code takes care of integrity and redundancy of the > > meta-data, > > as a result we can get rid of every other callback .get_mdata() > > .update_mdata() > > .get_mdata_part_num() .read_mdata_partition() .write_mdata_partition() > > and the > > corresponding wrapper functions thereby making the code 100s of LOC smaller. > > > > Get rid of fwu_check_mdata_validity() and fwu_mdata_check() which expected > > underlying > > layer to manage and verify mdata copies. > > Implement fwu_get_verified_mdata(struct fwu_mdata *mdata) public function > > that reads, > > verifies and, if needed, fixes the meta-data copies. > > > > Verified copy of meta-data is now cached as 'g_mdata' in fwu.c, which > > avoids multiple > > low-level expensive read and parse calls. > > gpt meta-data partition numbers are now cached in gpt_blk.c, so that we > > don't have to do expensive part_get_info() and uid ops. > > > > Changes since v4: > > * Change fwu-mdata-mtd bindings to not require external changes > > * Handle 'part == BOTH_PARTS' in fwu_sync_mdata > > * use parts_ok[] and parts_mdata[] instead of pri/sec_ok and > > p/s_mdata > > Did you run this through CI / build sandbox? This doesn't read like you > fixed the problem I reported in CI, when I was trying to merge v4. > I know that remains to be done. The dt-bindings for fwu-mdata is changed in this patchset and I thought any testcase may be impacted by it. thanks.
Re: [PATCHv3 1/5] FWU: Add FWU metadata access driver for MTD storage regions
On Sat, 4 Feb 2023 at 22:09, Jassi Brar wrote:
> >
> > + fwu-mdata {
> > + compatible = "u-boot,fwu-mdata-mtd";
> > + fwu-mdata-store = <_flash>;
> > + mdata-offsets = <0x50 0x53>;
> > + };
> >
> > which is based on DT going to location which is already labelled.
> >
> > 79 partition@50 {
> > 80 label = "Ex-OPTEE";
> > 81 reg = <0x50 0x20>;
> > 82 };
> >
> The 'ex-optee' is actually unused so we never faced any issue. But
> yes, this is inconsistent.
>
> > I don't know what this space is used for but the whole code around is using
> > MTD
> > partitions and it's infrastructure and this is using RAW access without MTD.
> >
> > Why not to create separate partitions just for storing metadata?
> > And also identify them like that.
> >
> > Or just switch it to complete RAW mode without MTD and then offsets can be
> > used
> > (but I expect with different dt description).
> >
> The design predates my involvement in fwu. I guess the reason was to
> have a mechanism similar to GPT based implementation which uses a
> similar fwu-mdata {} node structure. It does make sense to have
> dedicated partitions for primary and secondary meta-data, identified
> by uuid (like Banks) or standard labels. But may be Sughosh/Ilias know
> why the current approach was chosen.
>
I changed the bindings to not require any changes to any non-FWU subsystem.
Now every bit of info is contained in 'fwu-mdata' node.
https://lore.kernel.org/u-boot/20230228005218.1635781-1-jassisinghb...@gmail.com/T/#u
Thanks.
Re: [PATCHv4 2/5] fwu: move meta-data management in core
On Mon, Feb 27, 2023 at 10:30 AM Etienne Carriere
wrote:
>
> Hello Jassi,
>
> On Sun, 5 Feb 2023 at 04:01, wrote:
> >
> > From: Jassi Brar
> >
> > Instead of each i/f having to implement their own meta-data verification
> > and storage, move the logic in common code. This simplifies the i/f code
> > much simpler and compact.
> >
> > Signed-off-by: Jassi Brar
> > ---
> > drivers/fwu-mdata/fwu-mdata-uclass.c | 34 +++
> > include/fwu.h| 41
> > lib/fwu_updates/fwu.c| 135 ++-
> > 3 files changed, 206 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c
> > b/drivers/fwu-mdata/fwu-mdata-uclass.c
> > index b477e9603f..e03773c584 100644
> > --- a/drivers/fwu-mdata/fwu-mdata-uclass.c
> > +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c
> > @@ -16,6 +16,40 @@
> > #include
> > #include
> >
> > +/**
> > + * fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata()
> > + *
> > + * Return: 0 if OK, -ve on error
> > + */
> > +int fwu_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool
> > primary)
> > +{
> > + const struct fwu_mdata_ops *ops = device_get_ops(dev);
> > +
> > + if (!ops->read_mdata) {
> > + log_debug("read_mdata() method not defined\n");
> > + return -ENOSYS;
> > + }
> > +
> > + return ops->read_mdata(dev, mdata, primary);
> > +}
> > +
> > +/**
> > + * fwu_write_mdata() - Wrapper around fwu_mdata_ops.write_mdata()
> > + *
> > + * Return: 0 if OK, -ve on error
> > + */
> > +int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool
> > primary)
> > +{
> > + const struct fwu_mdata_ops *ops = device_get_ops(dev);
> > +
> > + if (!ops->write_mdata) {
> > + log_debug("write_mdata() method not defined\n");
> > + return -ENOSYS;
> > + }
> > +
> > + return ops->write_mdata(dev, mdata, primary);
> > +}
> > +
> > /**
> > * fwu_get_mdata_part_num() - Get the FWU metadata partition numbers
> > * @dev: FWU metadata device
> > diff --git a/include/fwu.h b/include/fwu.h
> > index 0919ced812..1a700c9e6a 100644
> > --- a/include/fwu.h
> > +++ b/include/fwu.h
> > @@ -24,6 +24,26 @@ struct fwu_mdata_gpt_blk_priv {
> > * @update_mdata() - Update the FWU metadata copy
> > */
> > struct fwu_mdata_ops {
> > + /**
> > +* read_mdata() - Populate the asked FWU metadata copy
> > +* @dev: FWU metadata device
> > +* @mdata: Copy of the FWU metadata
> > +* @primary: If primary or secondary copy of meta-data is to be read
> > +*
> > +* Return: 0 if OK, -ve on error
> > +*/
> > + int (*read_mdata)(struct udevice *dev, struct fwu_mdata *mdata,
> > bool primary);
> > +
> > + /**
> > +* write_mdata() - Write the given FWU metadata copy
> > +* @dev: FWU metadata device
> > +* @mdata: Copy of the FWU metadata
> > +* @primary: If primary or secondary copy of meta-data is to be
> > written
> > +*
> > +* Return: 0 if OK, -ve on error
> > +*/
> > + int (*write_mdata)(struct udevice *dev, struct fwu_mdata *mdata,
> > bool primary);
> > +
> > /**
> > * check_mdata() - Check if the FWU metadata is valid
> > * @dev:FWU device
> > @@ -126,6 +146,27 @@ struct fwu_mdata_ops {
> > EFI_GUID(0x0c996046, 0xbcc0, 0x4d04, 0x85, 0xec, \
> > 0xe1, 0xfc, 0xed, 0xf1, 0xc6, 0xf8)
> >
> > +/**
> > + * fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata()
> > + */
> > +int fwu_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool
> > primary);
> > +
> > +/**
> > + * fwu_write_mdata() - Wrapper around fwu_mdata_ops.write_mdata()
> > + */
> > +int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool
> > primary);
> > +
> > +/**
> > + * fwu_get_verified_mdata() - Read, verify and return the FWU metadata
> > + *
> > + * Read both the metadata copies from the storage media, verify their
> > checksum,
> > + * and ascertain that both copies match. If one of the copies has gone bad,
> > + * restore it from the good copy.
> > + *
> >
Re: [PATCHv3 1/5] FWU: Add FWU metadata access driver for MTD storage regions
On Wed, 18 Jan 2023 at 08:24, Michal Simek wrote:
>
>
>
> On 1/9/23 02:06, Jassi Brar wrote:
> > From: Sughosh Ganu
> >
> > In the FWU Multi Bank Update feature, the information about the
> > updatable images is stored as part of the metadata, on a separate
> > region. Add a driver for reading from and writing to the metadata
> > when the updatable images and the metadata are stored on a raw
> > MTD region.
> >
> > Signed-off-by: Sughosh Ganu
> > Signed-off-by: Jassi Brar
> > ---
> > drivers/fwu-mdata/Kconfig | 15 +++
> > drivers/fwu-mdata/Makefile | 1 +
> > drivers/fwu-mdata/raw_mtd.c | 201
> > 3 files changed, 217 insertions(+)
> > create mode 100644 drivers/fwu-mdata/raw_mtd.c
> >
> > diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig
> > index 36c4479a59..42736a5e43 100644
> > --- a/drivers/fwu-mdata/Kconfig
> > +++ b/drivers/fwu-mdata/Kconfig
> > @@ -6,6 +6,11 @@ config FWU_MDATA
> > FWU Metadata partitions reside on the same storage device
> > which contains the other FWU updatable firmware images.
> >
> > +choice
> > + prompt "Storage Layout Scheme"
> > + depends on FWU_MDATA
> > + default FWU_MDATA_GPT_BLK
> > +
> > config FWU_MDATA_GPT_BLK
> > bool "FWU Metadata access for GPT partitioned Block devices"
> > select PARTITION_TYPE_GUID
> > @@ -14,3 +19,13 @@ config FWU_MDATA_GPT_BLK
> > help
> > Enable support for accessing FWU Metadata on GPT partitioned
> > block devices.
> > +
> > +config FWU_MDATA_MTD
> > + bool "Raw MTD devices"
> > + depends on MTD
> > + help
> > + Enable support for accessing FWU Metadata on non-partitioned
> > + (or non-GPT partitioned, e.g. partition nodes in devicetree)
> > + MTD devices.
> > +
> > +endchoice
> > diff --git a/drivers/fwu-mdata/Makefile b/drivers/fwu-mdata/Makefile
> > index 3fee64c10c..06c49747ba 100644
> > --- a/drivers/fwu-mdata/Makefile
> > +++ b/drivers/fwu-mdata/Makefile
> > @@ -6,3 +6,4 @@
> >
> > obj-$(CONFIG_FWU_MDATA) += fwu-mdata-uclass.o
> > obj-$(CONFIG_FWU_MDATA_GPT_BLK) += gpt_blk.o
> > +obj-$(CONFIG_FWU_MDATA_MTD) += raw_mtd.o
> > diff --git a/drivers/fwu-mdata/raw_mtd.c b/drivers/fwu-mdata/raw_mtd.c
> > new file mode 100644
> > index 00..edd28b4525
> > --- /dev/null
> > +++ b/drivers/fwu-mdata/raw_mtd.c
> > @@ -0,0 +1,201 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2023, Linaro Limited
> > + */
> > +
> > +#define LOG_CATEGORY UCLASS_FWU_MDATA
> > +
> > +#include
> > +#include
> > +#include
> > +#include
>
> sort them.
>
actually flash.h is not required.
> > +
> > +#include
> > +#include
> > +
> > +/* Internal helper structure to move data around */
> > +struct fwu_mdata_mtd_priv {
> > + struct mtd_info *mtd;
> > + u32 pri_offset;
> > + u32 sec_offset;
> > +};
> > +
> > +enum fwu_mtd_op {
> > + FWU_MTD_READ,
> > + FWU_MTD_WRITE,
> > +};
> > +
> > +#define FWU_MDATA_PRIMARYtrue
> > +#define FWU_MDATA_SECONDARY false
>
> Completely unused.
>
yes, removed.
> > +
> > +static bool mtd_is_aligned_with_block_size(struct mtd_info *mtd, u64 size)
> > +{
> > + return !do_div(size, mtd->erasesize);
> > +}
> > +
> > +static int mtd_io_data(struct mtd_info *mtd, u32 offs, u32 size, void
> > *data,
> > +enum fwu_mtd_op op)
> > +{
> > + struct mtd_oob_ops io_op ={};
> > + u64 lock_offs, lock_len;
> > + size_t len;
> > + void *buf;
> > + int ret;
> > +
> > + if (!mtd_is_aligned_with_block_size(mtd, offs)) {
> > + log_err("Offset unaligned with a block (0x%x)\n",
> > mtd->erasesize);
> > + return -EINVAL;
> > + }
> > +
> > + lock_offs = offs;
> > + /* This will expand erase size to align with the block size */
> > + lock_len = round_up(size, mtd->erasesize);
> > +
> > + ret = mtd_unlock(mtd, lock_offs, lock_len);
> > + if (ret && ret != -EOPNOTSUPP)
> > + return ret;
> > +
> > + if (op == FWU_MTD_WRITE) {
> > + struct erase_info erase_op = {};
> > +
> > +
Re: [PATCHv3 2/5] fwu: move meta-data management in core
On Mon, 9 Jan 2023 at 06:54, Ilias Apalodimas
wrote:
>
> Hi Jassi,
>
> On Mon, Jan 02, 2023 at 12:26:40PM -0600, Jassi Brar wrote:
> > Instead of each i/f having to implement their own meta-data verification
> > and storage, move the logic in common code. This simplifies the i/f code
> > much simpler and compact.
> >
> > Signed-off-by: Jassi Brar
> > ---
> > drivers/fwu-mdata/fwu-mdata-uclass.c | 34 +++
> > include/fwu.h| 41
> > lib/fwu_updates/fwu.c| 142 ++-
> > 3 files changed, 213 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c
> > b/drivers/fwu-mdata/fwu-mdata-uclass.c
> > index b477e9603f..e03773c584 100644
> > --- a/drivers/fwu-mdata/fwu-mdata-uclass.c
> > +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c
.
> > + */
> > +static int fwu_sync_mdata(struct fwu_mdata *mdata, int part)
> > +{
> > + void *buf = >version;
> > + int err = 0;
> > +
> > + /*
> > + * Calculate the crc32 for the updated FWU metadata
> > + * and put the updated value in the FWU metadata crc32
> > + * field
> > + */
> > + mdata->crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
> > +
> > + if (part & PRIMARY_PART)
> > + err = fwu_write_mdata(g_dev, mdata, true);
> > +
> > + if (err) {
> > + log_err("Unable to write primary mdata\n");
> > + return err;
> > + }
> > +
> > + if (part & SECONDARY_PART)
> > + err = fwu_write_mdata(g_dev, mdata, false);
> > +
> > + if (err) {
> > + log_err("Unable to write secondary mdata\n");
> > + return err;
> > + }
>
> Can we write this
> err = fwu_write_mdata(g_dev, mdata, part & PRIMARY_PART ? true: false);
> if (err)
> log_err("Unable to write %s partition\n", part & PRIMARY_PART ?
> "primary": "secondary" );
>
>
of course :)
> > +int fwu_get_verified_mdata(struct fwu_mdata *mdata)
> > +{
> > + int err;
> > + bool pri_ok, sec_ok;
> > + struct fwu_mdata s, *p_mdata, *s_mdata;
> > +
> > + p_mdata = _mdata;
> > + s_mdata =
>
> Why are we defining it like this? Readability to have pointers for primary
> and secondary metadata?
>
that's the idea.
> > +
> > + /* if mdata already read and ready */
> > + err = mdata_crc_check(p_mdata);
> > + if (!err)
> > + goto ret_mdata;
>
> Shouldn't we check the secondary metadata ? At least that's what the old
> fwu_check_mdata_validity() was doing.
>
During the first run after boot, both copies are checked. Also when we
update the mdata.
Othwise we have a good primary copy, even if the secondary is
corrupted for some mysterious (corrupted in readonly mode) reason
maybe we should let that be fixed after reboot and not add crc
checking cost to every call?
> > + /* else read, verify and, if needed, fix mdata */
> > +
> > + pri_ok = false;
> > + err = fwu_read_mdata(g_dev, p_mdata, true);
> > + if (!err) {
> > + err = mdata_crc_check(p_mdata);
> > + if (!err)
> > + pri_ok = true;
> > + else
> > + log_debug("primary mdata: crc32 failed\n");
> > + }
> > +
> > + sec_ok = false;
> > + err = fwu_read_mdata(g_dev, s_mdata, false);
> > + if (!err) {
> > + err = mdata_crc_check(s_mdata);
> > + if (!err)
> > + sec_ok = true;
> > + else
> > + log_debug("secondary mdata: crc32 failed\n");
> > + }
> > +
> > + if (pri_ok && sec_ok) {
> > + /*
> > + * Before returning, check that both the
> > + * FWU metadata copies are the same.
> > + */
> > + err = memcmp(p_mdata, s_mdata, sizeof(struct fwu_mdata));
> > + if (!err)
> > + goto ret_mdata;
> > +
> > + /*
> > + * If not, populate the secondary partition from the
> > + * primary partition copy.
> > + */
> > + log_info("Both FWU metadata copies are valid but do not
> > match.");
> > + log_info(" Restoring the secondary partition from the
> > primary\n");
> > + sec_ok = false;
> > + }
> > +
> > + if (!pri_ok) {
> > + memcpy(p_mdata, s_mdata, sizeof(struct fwu_mdata));
> > + err = fwu_sync_mdata(p_mdata, PRIMARY_PART);
> > + if (err)
> > + goto ret_mdata;
>
> The error print here is a bit misleading. It's a failed write, not a crc32
> mismatch
>
Fixed.
Thanks.
Re: [PATCHv3 4/5] fwu: DeveloperBox: add support for FWU
On Wed, Jan 18, 2023 at 8:46 AM Michal Simek wrote:
>
>
>
> On 1/9/23 02:07, Jassi Brar wrote:
> > Add code to support FWU_MULTI_BANK_UPDATE.
> > The platform does not have gpt-partition storage for
> > Banks and MetaData, rather it used SPI-NOR backed
> > mtd regions for the purpose.
> >
> > Signed-off-by: Jassi Brar
> > ---
> > board/socionext/developerbox/Makefile | 1 +
> > board/socionext/developerbox/developerbox.c | 8 ++
> > board/socionext/developerbox/fwu_plat.c | 57
> > configs/synquacer_developerbox_defconfig| 13 ++-
> > doc/board/socionext/developerbox.rst| 96 +
> > include/configs/synquacer.h | 10 +++
> > 6 files changed, 183 insertions(+), 2 deletions(-)
> > create mode 100644 board/socionext/developerbox/fwu_plat.c
> >
> > diff --git a/board/socionext/developerbox/Makefile
> > b/board/socionext/developerbox/Makefile
> > index 4a46de995a..9b80ee38e7 100644
> > --- a/board/socionext/developerbox/Makefile
> > +++ b/board/socionext/developerbox/Makefile
> > @@ -7,3 +7,4 @@
> > #
> >
> > obj-y := developerbox.o
> > +obj-$(CONFIG_FWU_MULTI_BANK_UPDATE) += fwu_plat.o
> > diff --git a/board/socionext/developerbox/developerbox.c
> > b/board/socionext/developerbox/developerbox.c
> > index 6415c90c1c..2123914f21 100644
> > --- a/board/socionext/developerbox/developerbox.c
> > +++ b/board/socionext/developerbox/developerbox.c
> > @@ -20,6 +20,13 @@
> >
> > #if CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT)
> > struct efi_fw_image fw_images[] = {
> > +#if defined(CONFIG_FWU_MULTI_BANK_UPDATE)
> > + {
> > + .image_type_id = DEVELOPERBOX_FIP_IMAGE_GUID,
> > + .fw_name = u"DEVELOPERBOX-FIP",
> > + .image_index = 1,
> > + },
> > +#else
> > {
> > .image_type_id = DEVELOPERBOX_UBOOT_IMAGE_GUID,
> > .fw_name = u"DEVELOPERBOX-UBOOT",
> > @@ -35,6 +42,7 @@ struct efi_fw_image fw_images[] = {
> > .fw_name = u"DEVELOPERBOX-OPTEE",
> > .image_index = 3,
> > },
> > +#endif
> > };
> >
> > struct efi_capsule_update_info update_info = {
> > diff --git a/board/socionext/developerbox/fwu_plat.c
> > b/board/socionext/developerbox/fwu_plat.c
> > new file mode 100644
> > index 00..29b258f86c
> > --- /dev/null
> > +++ b/board/socionext/developerbox/fwu_plat.c
> > @@ -0,0 +1,57 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2023, Linaro Limited
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define DFU_ALT_BUF_LEN 256
> > +
> > +/* Generate dfu_alt_info from partitions */
> > +void set_dfu_alt_info(char *interface, char *devstr)
> > +{
> > + int ret;
> > + struct mtd_info *mtd;
> > +
> > + ALLOC_CACHE_ALIGN_BUFFER(char, buf, DFU_ALT_BUF_LEN);
> > + memset(buf, 0, sizeof(buf));
> > +
> > + mtd_probe_devices();
>
>
> When you disable CONFIG_MTD for this board this code will fail.
> You need to cover this dependency via different Kconfig symbol or somewhere
> else.
> Maybe this file should be compiled only when CONFIG_FWU_MDATA_MTD is present.
>
Yes, i agree.
> > +
> > + mtd = get_mtd_device_nm("nor1");
> > + if (IS_ERR_OR_NULL(mtd))
> > + return;
> > +
> > + ret = fwu_gen_alt_info_from_mtd(buf, DFU_ALT_BUF_LEN, mtd);
> > + if (ret < 0) {
> > + log_err("Error: Failed to generate dfu_alt_info. (%d)\n",
> > ret);
> > + return;
> > + }
> > + log_debug("Make dfu_alt_info: '%s'\n", buf);
> > +
> > + env_set("dfu_alt_info", buf);
> > +}
> > +
> > +int fwu_plat_get_alt_num(struct udevice __always_unused *dev,
> > + efi_guid_t *image_id, u8 *alt_num)
> > +{
> > + return fwu_mtd_get_alt_num(image_id, alt_num, "nor1");
> > +}
> > +
> > +void fwu_plat_get_bootidx(uint *boot_idx)
> > +{
> > + int ret;
> > + u32 active_idx;
> > + u32 *bootidx = boot_idx;
> > +
> > + ret = fwu_get_active_index(_idx);
> > +
> > + if (ret < 0)
> > + *bootidx = -1;
> > +
> > + *bootidx = active_idx
Re: [PATCHv3 0/5] FWU: Handle meta-data in common code
On Wed, Jan 18, 2023 at 7:28 AM Michal Simek wrote: > > Hi, > > On 1/2/23 19:25, Jassi Brar wrote: > > The patchset reduces ~400 lines of code, while keeping the functionality > > same and making > > meta-data operations much faster (by using cached structures). > > > > Issue: > > meta-data copies (primary and secondary) are being handled by the > > backend/storage layer > > instead of the common core in fwu.c (as also noted by Ilias) that is, > > gpt_blk.c manages > > meta-data and similarly raw_mtd.c will have to do the same when it arrives. > > The code > > could by make smaller, cleaner and optimised. > > > > Basic idea: > > Introduce .read_mdata() and .write_mdata() in fwu_mdata_ops that simply > > read/write > > meta-data copy. The core code takes care of integrity and redundancy of the > > meta-data, > > as a result we can get rid of every other callback .get_mdata() > > .update_mdata() > > .get_mdata_part_num() .read_mdata_partition() .write_mdata_partition() > > and the > > corresponding wrapper functions thereby making the code 100s of LOC smaller. > > > > Get rid of fwu_check_mdata_validity() and fwu_mdata_check() which expected > > underlying > > layer to manage and verify mdata copies. > > Implement fwu_get_verified_mdata(struct fwu_mdata *mdata) public function > > that reads, > > verifies and, if needed, fixes the meta-data copies. > > > > Verified copy of meta-data is now cached as 'g_mdata' in fwu.c, which > > avoids multiple > > low-level expensive read and parse calls. > > gpt meta-data partition numbers are now cached in gpt_blk.c, so that we > > don't have to do expensive part_get_info() and uid ops. > > First of all I have strong suspicious that this series are pretty much two > series at once. > Yes, I submitted two patchsets. 1) Optimizing the api of current fwu. 2) Introduce support for mtd backed storage (DeveloperBox platform as an instance) using the new api. They appear just fine in my inbox. Do they appear bad to you? > > The second issue is that you are sending patches from > Jassi Brar > but SOB is > Signed-off-by: Jassi Brar > > And Tom said in past that they should match. There is a hook for it to check > it > which everybody should be using. That's why please fix this in the next > series. > I have submitted dozens of patches and pull requests over the last many years. This never occurred to anybody. BTW, the 'Author' and 'Signed-off-by' appear consistent in git log. And there are very recent instances in uboot git log where even they actually differ. But if Tom really wants, I am happy to send-email from my other account. Thanks.
[PATCHv3 5/5] tools: Add mkfwumdata tool for FWU metadata image
From: Masami Hiramatsu
Add 'mkfwumdata' tool to generate FWU metadata image for the meta-data
partition to be used in A/B Update imeplementation.
Signed-off-by: Masami Hiramatsu
Signed-off-by: Sughosh Ganu
Signed-off-by: Jassi Brar
---
tools/Kconfig | 9 ++
tools/Makefile | 4 +
tools/mkfwumdata.c | 326 +
3 files changed, 339 insertions(+)
create mode 100644 tools/mkfwumdata.c
diff --git a/tools/Kconfig b/tools/Kconfig
index 539708f277..6e23f44d55 100644
--- a/tools/Kconfig
+++ b/tools/Kconfig
@@ -157,4 +157,13 @@ config LUT_SEQUENCE
help
Look Up Table Sequence
+config TOOLS_MKFWUMDATA
+ bool "Build mkfwumdata command"
+ default y if FWU_MULTI_BANK_UPDATE
+ help
+ This command allows users to create a raw image of the FWU
+ metadata for initial installation of the FWU multi bank
+ update on the board. The installation method depends on
+ the platform.
+
endmenu
diff --git a/tools/Makefile b/tools/Makefile
index 26be0a7ba2..024d6c8eca 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -255,6 +255,10 @@ HOSTLDLIBS_mkeficapsule += \
$(shell pkg-config --libs uuid 2> /dev/null || echo "-luuid")
hostprogs-$(CONFIG_TOOLS_MKEFICAPSULE) += mkeficapsule
+mkfwumdata-objs := mkfwumdata.o lib/crc32.o
+HOSTLDLIBS_mkfwumdata += -luuid
+hostprogs-$(CONFIG_TOOLS_MKFWUMDATA) += mkfwumdata
+
# We build some files with extra pedantic flags to try to minimize things
# that won't build on some weird host compiler -- though there are lots of
# exceptions for files that aren't complaint.
diff --git a/tools/mkfwumdata.c b/tools/mkfwumdata.c
new file mode 100644
index 00..e0b6702039
--- /dev/null
+++ b/tools/mkfwumdata.c
@@ -0,0 +1,326 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2023, Linaro Limited
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/* This will dynamically allocate the fwu_mdata */
+#define CONFIG_FWU_NUM_BANKS 0
+#define CONFIG_FWU_NUM_IMAGES_PER_BANK 0
+
+/* Since we can not include fwu.h, redefine version here. */
+#define FWU_MDATA_VERSION 1
+
+typedef uint8_t u8;
+typedef int16_t s16;
+typedef uint16_t u16;
+typedef uint32_t u32;
+typedef uint64_t u64;
+
+#include
+
+/* TODO: Endianess conversion may be required for some arch. */
+
+static const char *opts_short = "b:i:a:p:gh";
+
+static struct option options[] = {
+ {"banks", required_argument, NULL, 'b'},
+ {"images", required_argument, NULL, 'i'},
+ {"guid", required_argument, NULL, 'g'},
+ {"active-bank", required_argument, NULL, 'a'},
+ {"previous-bank", required_argument, NULL, 'p'},
+ {"help", no_argument, NULL, 'h'},
+ {NULL, 0, NULL, 0},
+};
+
+static void print_usage(void)
+{
+ fprintf(stderr, "Usage: mkfwumdata [options] \n");
+ fprintf(stderr, "Options:\n"
+ "\t-i, --images Number of images\n"
+ "\t-b, --banksNumber of banks\n"
+ "\t-a, --active-bank Active bank\n"
+ "\t-p, --previous-bankPrevious active bank\n"
+ "\t-g, --guid Use GUID instead of UUID\n"
+ "\t-h, --help print a help message\n"
+ );
+ fprintf(stderr, " UUIDs list syntax:\n"
+ "\t ,,\n"
+ "\timages uuid list syntax:\n"
+ "\t\timg_uuid_00,img_uuid_01...img_uuid_0b,\n"
+ "\t\timg_uuid_10,img_uuid_11...img_uuid_1b,\n"
+ "\t\t...,\n"
+ "\t\timg_uuid_i0,img_uuid_i1...img_uuid_ib,\n"
+ "\t\twhere 'b' and 'i' are number of banks and numbder of
images in a bank respectively.\n"
+ );
+}
+
+static int active_bank = 0;
+static int previous_bank = INT_MAX; /* unset */
+static bool __use_guid = false;
+
+struct fwu_mdata_object {
+ size_t images;
+ size_t banks;
+ size_t size;
+ struct fwu_mdata *mdata;
+};
+
+struct fwu_mdata_object *fwu_alloc_mdata(size_t images, size_t banks)
+{
+ struct fwu_mdata_object *mobj;
+
+ mobj = calloc(1, sizeof(*mobj));
+ if (!mobj)
+ return NULL;
+
+ mobj->size = sizeof(struct fwu_mdata) +
+ (sizeof(struct fwu_image_entry) +
+sizeof(struct fwu_image_bank_info) * banks) * images;
+ mobj->images = images;
+ mobj->banks = banks;
+
+ mobj->mdata = calloc(1, mobj->size);
+ if (!mobj->mdata) {
+ free(mobj);
+ return NULL;
+
[PATCHv3 4/5] fwu: DeveloperBox: add support for FWU
Add code to support FWU_MULTI_BANK_UPDATE.
The platform does not have gpt-partition storage for
Banks and MetaData, rather it used SPI-NOR backed
mtd regions for the purpose.
Signed-off-by: Jassi Brar
---
board/socionext/developerbox/Makefile | 1 +
board/socionext/developerbox/developerbox.c | 8 ++
board/socionext/developerbox/fwu_plat.c | 57
configs/synquacer_developerbox_defconfig| 13 ++-
doc/board/socionext/developerbox.rst| 96 +
include/configs/synquacer.h | 10 +++
6 files changed, 183 insertions(+), 2 deletions(-)
create mode 100644 board/socionext/developerbox/fwu_plat.c
diff --git a/board/socionext/developerbox/Makefile
b/board/socionext/developerbox/Makefile
index 4a46de995a..9b80ee38e7 100644
--- a/board/socionext/developerbox/Makefile
+++ b/board/socionext/developerbox/Makefile
@@ -7,3 +7,4 @@
#
obj-y := developerbox.o
+obj-$(CONFIG_FWU_MULTI_BANK_UPDATE) += fwu_plat.o
diff --git a/board/socionext/developerbox/developerbox.c
b/board/socionext/developerbox/developerbox.c
index 6415c90c1c..2123914f21 100644
--- a/board/socionext/developerbox/developerbox.c
+++ b/board/socionext/developerbox/developerbox.c
@@ -20,6 +20,13 @@
#if CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT)
struct efi_fw_image fw_images[] = {
+#if defined(CONFIG_FWU_MULTI_BANK_UPDATE)
+ {
+ .image_type_id = DEVELOPERBOX_FIP_IMAGE_GUID,
+ .fw_name = u"DEVELOPERBOX-FIP",
+ .image_index = 1,
+ },
+#else
{
.image_type_id = DEVELOPERBOX_UBOOT_IMAGE_GUID,
.fw_name = u"DEVELOPERBOX-UBOOT",
@@ -35,6 +42,7 @@ struct efi_fw_image fw_images[] = {
.fw_name = u"DEVELOPERBOX-OPTEE",
.image_index = 3,
},
+#endif
};
struct efi_capsule_update_info update_info = {
diff --git a/board/socionext/developerbox/fwu_plat.c
b/board/socionext/developerbox/fwu_plat.c
new file mode 100644
index 00..29b258f86c
--- /dev/null
+++ b/board/socionext/developerbox/fwu_plat.c
@@ -0,0 +1,57 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2023, Linaro Limited
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+#define DFU_ALT_BUF_LEN 256
+
+/* Generate dfu_alt_info from partitions */
+void set_dfu_alt_info(char *interface, char *devstr)
+{
+ int ret;
+ struct mtd_info *mtd;
+
+ ALLOC_CACHE_ALIGN_BUFFER(char, buf, DFU_ALT_BUF_LEN);
+ memset(buf, 0, sizeof(buf));
+
+ mtd_probe_devices();
+
+ mtd = get_mtd_device_nm("nor1");
+ if (IS_ERR_OR_NULL(mtd))
+ return;
+
+ ret = fwu_gen_alt_info_from_mtd(buf, DFU_ALT_BUF_LEN, mtd);
+ if (ret < 0) {
+ log_err("Error: Failed to generate dfu_alt_info. (%d)\n", ret);
+ return;
+ }
+ log_debug("Make dfu_alt_info: '%s'\n", buf);
+
+ env_set("dfu_alt_info", buf);
+}
+
+int fwu_plat_get_alt_num(struct udevice __always_unused *dev,
+efi_guid_t *image_id, u8 *alt_num)
+{
+ return fwu_mtd_get_alt_num(image_id, alt_num, "nor1");
+}
+
+void fwu_plat_get_bootidx(uint *boot_idx)
+{
+ int ret;
+ u32 active_idx;
+ u32 *bootidx = boot_idx;
+
+ ret = fwu_get_active_index(_idx);
+
+ if (ret < 0)
+ *bootidx = -1;
+
+ *bootidx = active_idx;
+}
diff --git a/configs/synquacer_developerbox_defconfig
b/configs/synquacer_developerbox_defconfig
index f69b873a36..b1085a388e 100644
--- a/configs/synquacer_developerbox_defconfig
+++ b/configs/synquacer_developerbox_defconfig
@@ -1,10 +1,11 @@
CONFIG_ARM=y
CONFIG_ARCH_SYNQUACER=y
-CONFIG_TEXT_BASE=0x0820
+CONFIG_POSITION_INDEPENDENT=y
+CONFIG_SYS_TEXT_BASE=0
CONFIG_SYS_MALLOC_LEN=0x100
CONFIG_SYS_MALLOC_F_LEN=0x400
CONFIG_ENV_SIZE=0x3
-CONFIG_ENV_OFFSET=0x30
+CONFIG_ENV_OFFSET=0x58
CONFIG_ENV_SECT_SIZE=0x1
CONFIG_DM_GPIO=y
CONFIG_DEFAULT_DEVICE_TREE="synquacer-sc2a11-developerbox"
@@ -96,3 +97,11 @@ CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
CONFIG_EFI_CAPSULE_ON_DISK=y
CONFIG_EFI_IGNORE_OSINDICATIONS=y
CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
+CONFIG_EFI_SECURE_BOOT=y
+CONFIG_FWU_MULTI_BANK_UPDATE=y
+CONFIG_FWU_MDATA=y
+CONFIG_FWU_MDATA_MTD=y
+CONFIG_FWU_NUM_BANKS=2
+CONFIG_FWU_NUM_IMAGES_PER_BANK=1
+CONFIG_CMD_FWU_METADATA=y
+CONFIG_TOOLS_MKFWUMDATA=y
diff --git a/doc/board/socionext/developerbox.rst
b/doc/board/socionext/developerbox.rst
index 2d943c23be..be872aa79d 100644
--- a/doc/board/socionext/developerbox.rst
+++ b/doc/board/socionext/developerbox.rst
@@ -85,3 +85,99 @@ Once the flasher tool is running we are ready flash the UEFI
image::
After transferring the SPI_NOR_UBOOT.fd, turn off the DSW2-7 and reset the
board.
+
+Enable FWU Multi Bank Update
+
+
+DeveloperBox supports the FWU Multi B
[PATCHv3 3/5] dt: fwu: developerbox: enable fwu banks and mdata regions
Specify Bank-0/1 and fwu metadata mtd regions.
Signed-off-by: Jassi Brar
Acked-by: Ilias Apalodimas
---
.../synquacer-sc2a11-developerbox-u-boot.dtsi | 22 ++-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi
b/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi
index 7a56116d6f..62eee0aaf0 100644
--- a/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi
+++ b/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi
@@ -23,7 +23,7 @@
active_clk_edges;
chipselect_num = <1>;
- spi-flash@0 {
+ spi_flash: spi-flash@0 {
#address-cells = <1>;
#size-cells = <1>;
compatible = "jedec,spi-nor";
@@ -36,6 +36,7 @@
compatible = "fixed-partitions";
#address-cells = <1>;
#size-cells = <1>;
+ uuid = "17e86d77-41f9-4fd7-87ec-a55df9842de5";
partition@0 {
label = "BootStrap-BL1";
@@ -79,6 +80,19 @@
label = "Ex-OPTEE";
reg = <0x50 0x20>;
};
+
+ /* FWU Multi bank update partitions */
+ partition@60 {
+ label = "FIP-Bank0";
+ reg = <0x60 0x40>;
+ uuid =
"5a66a702-99fd-4fef-a392-c26e261a2828";
+ };
+
+ partition@a0 {
+ label = "FIP-Bank1";
+ reg = <0xa0 0x40>;
+ uuid =
"a8f868a1-6e5c-4757-878d-ce63375ef2c0";
+ };
};
};
};
@@ -104,6 +118,12 @@
optee {
status = "okay";
};
+
+ fwu-mdata {
+ compatible = "u-boot,fwu-mdata-mtd";
+ fwu-mdata-store = <_flash>;
+ mdata-offsets = <0x50 0x53>;
+ };
};
};
--
2.34.1
[PATCHv3 2/5] FWU: mtd: Add helper functions for accessing FWU metadata
From: Sughosh Ganu
Add helper functions needed for accessing the FWU metadata which
contains information on the updatable images.
Signed-off-by: Sughosh Ganu
Signed-off-by: Jassi Brar
---
include/fwu.h | 27 ++
lib/fwu_updates/Makefile | 1 +
lib/fwu_updates/fwu_mtd.c | 172 ++
3 files changed, 200 insertions(+)
create mode 100644 lib/fwu_updates/fwu_mtd.c
diff --git a/include/fwu.h b/include/fwu.h
index ea25aca2cd..2edfa72caf 100644
--- a/include/fwu.h
+++ b/include/fwu.h
@@ -8,6 +8,7 @@
#include
#include
+#include
#include
@@ -251,4 +252,30 @@ u8 fwu_empty_capsule_checks_pass(void);
*/
int fwu_trial_state_ctr_start(void);
+/**
+ * fwu_gen_alt_info_from_mtd() - Parse dfu_alt_info from metadata in mtd
+ * @buf: Buffer into which the dfu_alt_info is filled
+ * @len: Maximum charaters that can be written in buf
+ * @mtd: Pointer to underlying MTD device
+ *
+ * Parse dfu_alt_info from metadata in mtd. Used for setting the env.
+ *
+ * Return: 0 if OK, -ve on error
+ *
+ */
+int fwu_gen_alt_info_from_mtd(char *buf, size_t len, struct mtd_info *mtd);
+
+/**
+ * fwu_mtd_get_alt_num() - Mapping of fwu_plat_get_alt_num for MTD device
+ * @image_guid: Image GUID for which DFU alt number needs to be retrieved
+ * @alt_num: Pointer to the alt_num
+ * @mtd_dev: Name of mtd device instance
+ *
+ * To map fwu_plat_get_alt_num onto mtd based metadata implementation.
+ *
+ * Return: 0 if OK, -ve on error
+ *
+ */
+int fwu_mtd_get_alt_num(efi_guid_t *image_id, u8 *alt_num, const char
*mtd_dev);
+
#endif /* _FWU_H_ */
diff --git a/lib/fwu_updates/Makefile b/lib/fwu_updates/Makefile
index 1993088e5b..c9e3c06b48 100644
--- a/lib/fwu_updates/Makefile
+++ b/lib/fwu_updates/Makefile
@@ -5,3 +5,4 @@
obj-$(CONFIG_FWU_MULTI_BANK_UPDATE) += fwu.o
obj-$(CONFIG_FWU_MDATA_GPT_BLK) += fwu_gpt.o
+obj-$(CONFIG_FWU_MDATA_MTD) += fwu_mtd.o
diff --git a/lib/fwu_updates/fwu_mtd.c b/lib/fwu_updates/fwu_mtd.c
new file mode 100644
index 00..1895b8fab3
--- /dev/null
+++ b/lib/fwu_updates/fwu_mtd.c
@@ -0,0 +1,172 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2023, Linaro Limited
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+int fwu_mtd_get_alt_num(efi_guid_t *image_id, u8 *alt_num,
+ const char *mtd_dev)
+{
+ int i, nalt;
+ int ret = -1;
+ struct mtd_info *mtd;
+ struct dfu_entity *dfu;
+ ofnode node, parts_node;
+ fdt_addr_t offset, size;
+ char uuidbuf[UUID_STR_LEN + 1];
+
+ mtd_probe_devices();
+ mtd = get_mtd_device_nm(mtd_dev);
+
+ /* Find partition node under given MTD device. */
+ parts_node = ofnode_by_compatible(mtd_get_ofnode(mtd),
+ "fixed-partitions");
+
+ uuid_bin_to_str(image_id->b, uuidbuf, UUID_STR_FORMAT_STD);
+ node = ofnode_by_prop_value(parts_node, "uuid", uuidbuf,
+ sizeof(uuidbuf));
+ if (!ofnode_valid(node)) {
+ log_warning("Warning: Failed to find partition by image
UUID\n");
+ return -ENOENT;
+ }
+
+ offset = ofnode_get_addr_size_index_notrans(node, 0, );
+ if (offset == FDT_ADDR_T_NONE || !size)
+ return -ENOENT;
+
+ dfu_init_env_entities(NULL, NULL);
+
+ nalt = 0;
+ list_for_each_entry(dfu, _list, list)
+ nalt++;
+
+ if (!nalt) {
+ log_warning("No entities in dfu_alt_info\n");
+ dfu_free_entities();
+ return -ENOENT;
+ }
+
+ for (i = 0; i < nalt; i++) {
+ dfu = dfu_get_entity(i);
+
+ /* Only MTD RAW access */
+ if (!dfu || dfu->dev_type != DFU_DEV_MTD ||
+ dfu->layout != DFU_RAW_ADDR ||
+ dfu->data.mtd.start != offset ||
+ dfu->data.mtd.size != size)
+ continue;
+
+ *alt_num = dfu->alt;
+ ret = 0;
+ break;
+ }
+
+ dfu_free_entities();
+
+ return ret;
+}
+
+static int gen_image_alt_info(char *buf, size_t len, int sidx,
+ struct fwu_image_entry *img, struct mtd_info *mtd)
+{
+ int i;
+ const char *suuid;
+ ofnode node, parts_node;
+ char uuidbuf[UUID_STR_LEN + 1];
+ char *p = buf, *end = buf + len;
+
+ /* Find partition node under given MTD device. */
+ parts_node = ofnode_by_compatible(mtd_get_ofnode(mtd),
+ "fixed-partitions");
+ if (!ofnode_valid(parts_node))
+ return -ENOENT;
+
+ /* Check the media UUID if exist. */
+ suuid = ofnode_read_string(parts_node, "uuid");
+ if (suuid) {
+
[PATCHv3 1/5] FWU: Add FWU metadata access driver for MTD storage regions
From: Sughosh Ganu
In the FWU Multi Bank Update feature, the information about the
updatable images is stored as part of the metadata, on a separate
region. Add a driver for reading from and writing to the metadata
when the updatable images and the metadata are stored on a raw
MTD region.
Signed-off-by: Sughosh Ganu
Signed-off-by: Jassi Brar
---
drivers/fwu-mdata/Kconfig | 15 +++
drivers/fwu-mdata/Makefile | 1 +
drivers/fwu-mdata/raw_mtd.c | 201
3 files changed, 217 insertions(+)
create mode 100644 drivers/fwu-mdata/raw_mtd.c
diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig
index 36c4479a59..42736a5e43 100644
--- a/drivers/fwu-mdata/Kconfig
+++ b/drivers/fwu-mdata/Kconfig
@@ -6,6 +6,11 @@ config FWU_MDATA
FWU Metadata partitions reside on the same storage device
which contains the other FWU updatable firmware images.
+choice
+ prompt "Storage Layout Scheme"
+ depends on FWU_MDATA
+ default FWU_MDATA_GPT_BLK
+
config FWU_MDATA_GPT_BLK
bool "FWU Metadata access for GPT partitioned Block devices"
select PARTITION_TYPE_GUID
@@ -14,3 +19,13 @@ config FWU_MDATA_GPT_BLK
help
Enable support for accessing FWU Metadata on GPT partitioned
block devices.
+
+config FWU_MDATA_MTD
+ bool "Raw MTD devices"
+ depends on MTD
+ help
+ Enable support for accessing FWU Metadata on non-partitioned
+ (or non-GPT partitioned, e.g. partition nodes in devicetree)
+ MTD devices.
+
+endchoice
diff --git a/drivers/fwu-mdata/Makefile b/drivers/fwu-mdata/Makefile
index 3fee64c10c..06c49747ba 100644
--- a/drivers/fwu-mdata/Makefile
+++ b/drivers/fwu-mdata/Makefile
@@ -6,3 +6,4 @@
obj-$(CONFIG_FWU_MDATA) += fwu-mdata-uclass.o
obj-$(CONFIG_FWU_MDATA_GPT_BLK) += gpt_blk.o
+obj-$(CONFIG_FWU_MDATA_MTD) += raw_mtd.o
diff --git a/drivers/fwu-mdata/raw_mtd.c b/drivers/fwu-mdata/raw_mtd.c
new file mode 100644
index 00..edd28b4525
--- /dev/null
+++ b/drivers/fwu-mdata/raw_mtd.c
@@ -0,0 +1,201 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2023, Linaro Limited
+ */
+
+#define LOG_CATEGORY UCLASS_FWU_MDATA
+
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+/* Internal helper structure to move data around */
+struct fwu_mdata_mtd_priv {
+ struct mtd_info *mtd;
+ u32 pri_offset;
+ u32 sec_offset;
+};
+
+enum fwu_mtd_op {
+ FWU_MTD_READ,
+ FWU_MTD_WRITE,
+};
+
+#define FWU_MDATA_PRIMARY true
+#define FWU_MDATA_SECONDARYfalse
+
+static bool mtd_is_aligned_with_block_size(struct mtd_info *mtd, u64 size)
+{
+ return !do_div(size, mtd->erasesize);
+}
+
+static int mtd_io_data(struct mtd_info *mtd, u32 offs, u32 size, void *data,
+ enum fwu_mtd_op op)
+{
+ struct mtd_oob_ops io_op ={};
+ u64 lock_offs, lock_len;
+ size_t len;
+ void *buf;
+ int ret;
+
+ if (!mtd_is_aligned_with_block_size(mtd, offs)) {
+ log_err("Offset unaligned with a block (0x%x)\n",
mtd->erasesize);
+ return -EINVAL;
+ }
+
+ lock_offs = offs;
+ /* This will expand erase size to align with the block size */
+ lock_len = round_up(size, mtd->erasesize);
+
+ ret = mtd_unlock(mtd, lock_offs, lock_len);
+ if (ret && ret != -EOPNOTSUPP)
+ return ret;
+
+ if (op == FWU_MTD_WRITE) {
+ struct erase_info erase_op = {};
+
+ erase_op.mtd = mtd;
+ erase_op.addr = lock_offs;
+ erase_op.len = lock_len;
+ erase_op.scrub = 0;
+
+ ret = mtd_erase(mtd, _op);
+ if (ret)
+ goto lock;
+ }
+
+ /* Also, expand the write size to align with the write size */
+ len = round_up(size, mtd->writesize);
+
+ buf = memalign(ARCH_DMA_MINALIGN, len);
+ if (!buf) {
+ ret = -ENOMEM;
+ goto lock;
+ }
+ memset(buf, 0xff, len);
+
+ io_op.mode = MTD_OPS_AUTO_OOB;
+ io_op.len = len;
+ io_op.ooblen = 0;
+ io_op.datbuf = buf;
+ io_op.oobbuf = NULL;
+
+ if (op == FWU_MTD_WRITE) {
+ memcpy(buf, data, size);
+ ret = mtd_write_oob(mtd, offs, _op);
+ } else {
+ ret = mtd_read_oob(mtd, offs, _op);
+ if (!ret)
+ memcpy(data, buf, size);
+ }
+ free(buf);
+
+lock:
+ mtd_lock(mtd, lock_offs, lock_len);
+
+ return ret;
+}
+
+static int fwu_mtd_read_mdata(struct udevice *dev, struct fwu_mdata *mdata,
bool primary)
+{
+ struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
+ struct mtd_info *mtd = mtd_priv->mtd;
+ u32 offs = primary ? mtd_priv->pri_offset : mtd_priv->sec_offset;
+
+ return
[PATCHv3 0/5] FWU: Add support for mtd backed feature on DeveloperBox
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/20230102182532.2411125-1-jaswinder.si...@linaro.org/ Jassi Brar (2): dt: fwu: developerbox: enable fwu banks and mdata regions fwu: DeveloperBox: add support for FWU Masami Hiramatsu (1): tools: Add mkfwumdata tool for FWU metadata image Sughosh Ganu (2): FWU: Add FWU metadata access driver for MTD storage regions FWU: mtd: Add helper functions for accessing FWU metadata .../synquacer-sc2a11-developerbox-u-boot.dtsi | 22 +- board/socionext/developerbox/Makefile | 1 + board/socionext/developerbox/developerbox.c | 8 + board/socionext/developerbox/fwu_plat.c | 57 +++ configs/synquacer_developerbox_defconfig | 13 +- doc/board/socionext/developerbox.rst | 96 ++ drivers/fwu-mdata/Kconfig | 15 + drivers/fwu-mdata/Makefile| 1 + drivers/fwu-mdata/raw_mtd.c | 201 +++ include/configs/synquacer.h | 10 + include/fwu.h | 27 ++ lib/fwu_updates/Makefile | 1 + lib/fwu_updates/fwu_mtd.c | 172 + tools/Kconfig | 9 + tools/Makefile| 4 + tools/mkfwumdata.c| 326 ++ 16 files changed, 960 insertions(+), 3 deletions(-) create mode 100644 board/socionext/developerbox/fwu_plat.c 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
[PATCHv3 5/5] fwu: rename fwu_get_verified_mdata to fwu_get_mdata
fwu_get_mdata() sounds more appropriate than fwu_get_verified_mdata()
Signed-off-by: Jassi Brar
Reviewed-by: Etienne Carriere
Reviewed-by: Ilias Apalodimas
---
cmd/fwu_mdata.c | 2 +-
include/fwu.h | 4 ++--
lib/fwu_updates/fwu.c | 6 +++---
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/cmd/fwu_mdata.c b/cmd/fwu_mdata.c
index 9b70340368..5ecda455df 100644
--- a/cmd/fwu_mdata.c
+++ b/cmd/fwu_mdata.c
@@ -46,7 +46,7 @@ int do_fwu_mdata_read(struct cmd_tbl *cmdtp, int flag,
int ret = CMD_RET_SUCCESS, res;
struct fwu_mdata mdata;
- res = fwu_get_verified_mdata();
+ res = fwu_get_mdata();
if (res < 0) {
log_err("Unable to get valid FWU metadata\n");
ret = CMD_RET_FAILURE;
diff --git a/include/fwu.h b/include/fwu.h
index 23bd97fe86..ea25aca2cd 100644
--- a/include/fwu.h
+++ b/include/fwu.h
@@ -80,7 +80,7 @@ int fwu_read_mdata(struct udevice *dev, struct fwu_mdata
*mdata, bool primary);
int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool
primary);
/**
- * fwu_get_verified_mdata() - Read, verify and return the FWU metadata
+ * fwu_get_mdata() - Read, verify and return the FWU metadata
*
* Read both the metadata copies from the storage media, verify their checksum,
* and ascertain that both copies match. If one of the copies has gone bad,
@@ -88,7 +88,7 @@ int fwu_write_mdata(struct udevice *dev, struct fwu_mdata
*mdata, bool primary);
*
* Return: 0 if OK, -ve on error
*/
-int fwu_get_verified_mdata(struct fwu_mdata *mdata);
+int fwu_get_mdata(struct fwu_mdata *mdata);
/**
* fwu_get_active_index() - Get active_index from the FWU metadata
diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c
index 234882af9a..75bcc01c13 100644
--- a/lib/fwu_updates/fwu.c
+++ b/lib/fwu_updates/fwu.c
@@ -191,7 +191,7 @@ static inline int mdata_crc_check(struct fwu_mdata *mdata)
}
/**
- * fwu_get_verified_mdata() - Read, verify and return the FWU metadata
+ * fwu_get_mdata() - Read, verify and return the FWU metadata
*
* Read both the metadata copies from the storage media, verify their checksum,
* and ascertain that both copies match. If one of the copies has gone bad,
@@ -199,7 +199,7 @@ static inline int mdata_crc_check(struct fwu_mdata *mdata)
*
* Return: 0 if OK, -ve on error
*/
-int fwu_get_verified_mdata(struct fwu_mdata *mdata)
+int fwu_get_mdata(struct fwu_mdata *mdata)
{
int err;
bool pri_ok, sec_ok;
@@ -628,7 +628,7 @@ static int fwu_boottime_checks(void *ctx, struct event
*event)
return ret;
}
- ret = fwu_get_verified_mdata(NULL);
+ ret = fwu_get_mdata(NULL);
if (ret) {
log_debug("Unable to read meta-data\n");
return ret;
--
2.34.1
[PATCHv3 4/5] fwu: meta-data: switch to management by common code
The common code can now read, verify and fix meta-data copies
while exposing one consistent structure to users.
Only the .read_mdata() and .write_mdata() callbacks of fwu_mdata_ops
are needed. Get rid of .get_mdata() .update_mdata() .get_mdata_part_num()
.read_mdata_partition() and .write_mdata_partition() and also the
corresponding wrapper functions.
Signed-off-by: Jassi Brar
---
cmd/fwu_mdata.c | 17 +-
drivers/fwu-mdata/fwu-mdata-uclass.c | 165 ---
drivers/fwu-mdata/gpt_blk.c | 124 +-
include/fwu.h| 199 ---
lib/fwu_updates/fwu.c| 235 ---
5 files changed, 38 insertions(+), 702 deletions(-)
diff --git a/cmd/fwu_mdata.c b/cmd/fwu_mdata.c
index f04af27de6..9b70340368 100644
--- a/cmd/fwu_mdata.c
+++ b/cmd/fwu_mdata.c
@@ -43,23 +43,10 @@ static void print_mdata(struct fwu_mdata *mdata)
int do_fwu_mdata_read(struct cmd_tbl *cmdtp, int flag,
int argc, char * const argv[])
{
- struct udevice *dev;
int ret = CMD_RET_SUCCESS, res;
- struct fwu_mdata mdata = { 0 };
+ struct fwu_mdata mdata;
- if (uclass_get_device(UCLASS_FWU_MDATA, 0, ) || !dev) {
- log_err("Unable to get FWU metadata device\n");
- return CMD_RET_FAILURE;
- }
-
- res = fwu_check_mdata_validity();
- if (res < 0) {
- log_err("FWU Metadata check failed\n");
- ret = CMD_RET_FAILURE;
- goto out;
- }
-
- res = fwu_get_mdata(dev, );
+ res = fwu_get_verified_mdata();
if (res < 0) {
log_err("Unable to get valid FWU metadata\n");
ret = CMD_RET_FAILURE;
diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c
b/drivers/fwu-mdata/fwu-mdata-uclass.c
index e03773c584..0a8edaaa41 100644
--- a/drivers/fwu-mdata/fwu-mdata-uclass.c
+++ b/drivers/fwu-mdata/fwu-mdata-uclass.c
@@ -14,7 +14,6 @@
#include
#include
-#include
/**
* fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata()
@@ -50,170 +49,6 @@ int fwu_write_mdata(struct udevice *dev, struct fwu_mdata
*mdata, bool primary)
return ops->write_mdata(dev, mdata, primary);
}
-/**
- * fwu_get_mdata_part_num() - Get the FWU metadata partition numbers
- * @dev: FWU metadata device
- * @mdata_parts: array for storing the metadata partition numbers
- *
- * Get the partition numbers on the storage device on which the
- * FWU metadata is stored. Two partition numbers will be returned.
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_get_mdata_part_num(struct udevice *dev, uint *mdata_parts)
-{
- const struct fwu_mdata_ops *ops = device_get_ops(dev);
-
- if (!ops->get_mdata_part_num) {
- log_debug("get_mdata_part_num() method not defined\n");
- return -ENOSYS;
- }
-
- return ops->get_mdata_part_num(dev, mdata_parts);
-}
-
-/**
- * fwu_read_mdata_partition() - Read the FWU metadata from a partition
- * @dev: FWU metadata device
- * @mdata: Copy of the FWU metadata
- * @part_num: Partition number from which FWU metadata is to be read
- *
- * Read the FWU metadata from the specified partition number
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_read_mdata_partition(struct udevice *dev, struct fwu_mdata *mdata,
-uint part_num)
-{
- const struct fwu_mdata_ops *ops = device_get_ops(dev);
-
- if (!ops->read_mdata_partition) {
- log_debug("read_mdata_partition() method not defined\n");
- return -ENOSYS;
- }
-
- return ops->read_mdata_partition(dev, mdata, part_num);
-}
-
-/**
- * fwu_write_mdata_partition() - Write the FWU metadata to a partition
- * @dev: FWU metadata device
- * @mdata: Copy of the FWU metadata
- * @part_num: Partition number to which FWU metadata is to be written
- *
- * Write the FWU metadata to the specified partition number
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_write_mdata_partition(struct udevice *dev, struct fwu_mdata *mdata,
- uint part_num)
-{
- const struct fwu_mdata_ops *ops = device_get_ops(dev);
-
- if (!ops->write_mdata_partition) {
- log_debug("write_mdata_partition() method not defined\n");
- return -ENOSYS;
- }
-
- return ops->write_mdata_partition(dev, mdata, part_num);
-}
-
-/**
- * fwu_mdata_check() - Check if the FWU metadata is valid
- * @dev: FWU metadata device
- *
- * Validate both copies of the FWU metadata. If one of the copies
- * has gone bad, restore it from the other copy.
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_mdata_check(struct udevice *dev)
-{
- const struct fwu_mdata_ops *ops = device_get_ops(dev);
-
- if (!ops->check_mdata) {
-
[PATCHv3 3/5] fwu: gpt: implement read_mdata and write_mdata callbacks
Moving towards using common code for meta-data management,
implement the read/write mdata hooks.
Signed-off-by: Jassi Brar
Reviewed-by: Etienne Carriere
Reviewed-by: Ilias Apalodimas
---
drivers/fwu-mdata/gpt_blk.c | 36
1 file changed, 36 insertions(+)
diff --git a/drivers/fwu-mdata/gpt_blk.c b/drivers/fwu-mdata/gpt_blk.c
index 28f5d23e1e..bdaa10cd1d 100644
--- a/drivers/fwu-mdata/gpt_blk.c
+++ b/drivers/fwu-mdata/gpt_blk.c
@@ -272,7 +272,43 @@ static int fwu_mdata_gpt_blk_probe(struct udevice *dev)
return 0;
}
+static int fwu_gpt_read_mdata(struct udevice *dev, struct fwu_mdata *mdata,
+bool primary)
+{
+ struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
+ struct blk_desc *desc = dev_get_uclass_plat(priv->blk_dev);
+ int ret;
+
+ ret = gpt_get_mdata_partitions(desc);
+ if (ret < 0) {
+ log_debug("Error getting the FWU metadata partitions\n");
+ return -ENOENT;
+ }
+
+ return gpt_read_write_mdata(desc, mdata, MDATA_READ,
+primary ? g_mdata_part[0] : g_mdata_part[1]);
+}
+
+static int fwu_gpt_write_mdata(struct udevice *dev, struct fwu_mdata *mdata,
+bool primary)
+{
+ struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
+ struct blk_desc *desc = dev_get_uclass_plat(priv->blk_dev);
+ int ret;
+
+ ret = gpt_get_mdata_partitions(desc);
+ if (ret < 0) {
+ log_debug("Error getting the FWU metadata partitions\n");
+ return -ENOENT;
+ }
+
+ return gpt_read_write_mdata(desc, mdata, MDATA_WRITE,
+primary ? g_mdata_part[0] : g_mdata_part[1]);
+}
+
static const struct fwu_mdata_ops fwu_gpt_blk_ops = {
+ .read_mdata = fwu_gpt_read_mdata,
+ .write_mdata = fwu_gpt_write_mdata,
.get_mdata = fwu_gpt_get_mdata,
.update_mdata = fwu_gpt_update_mdata,
.get_mdata_part_num = fwu_gpt_get_mdata_partitions,
--
2.34.1
[PATCHv3 2/5] fwu: move meta-data management in core
Instead of each i/f having to implement their own meta-data verification
and storage, move the logic in common code. This simplifies the i/f code
much simpler and compact.
Signed-off-by: Jassi Brar
---
drivers/fwu-mdata/fwu-mdata-uclass.c | 34 +++
include/fwu.h| 41
lib/fwu_updates/fwu.c| 142 ++-
3 files changed, 213 insertions(+), 4 deletions(-)
diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c
b/drivers/fwu-mdata/fwu-mdata-uclass.c
index b477e9603f..e03773c584 100644
--- a/drivers/fwu-mdata/fwu-mdata-uclass.c
+++ b/drivers/fwu-mdata/fwu-mdata-uclass.c
@@ -16,6 +16,40 @@
#include
#include
+/**
+ * fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata()
+ *
+ * Return: 0 if OK, -ve on error
+ */
+int fwu_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary)
+{
+ const struct fwu_mdata_ops *ops = device_get_ops(dev);
+
+ if (!ops->read_mdata) {
+ log_debug("read_mdata() method not defined\n");
+ return -ENOSYS;
+ }
+
+ return ops->read_mdata(dev, mdata, primary);
+}
+
+/**
+ * fwu_write_mdata() - Wrapper around fwu_mdata_ops.write_mdata()
+ *
+ * Return: 0 if OK, -ve on error
+ */
+int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary)
+{
+ const struct fwu_mdata_ops *ops = device_get_ops(dev);
+
+ if (!ops->write_mdata) {
+ log_debug("write_mdata() method not defined\n");
+ return -ENOSYS;
+ }
+
+ return ops->write_mdata(dev, mdata, primary);
+}
+
/**
* fwu_get_mdata_part_num() - Get the FWU metadata partition numbers
* @dev: FWU metadata device
diff --git a/include/fwu.h b/include/fwu.h
index 0919ced812..1a700c9e6a 100644
--- a/include/fwu.h
+++ b/include/fwu.h
@@ -24,6 +24,26 @@ struct fwu_mdata_gpt_blk_priv {
* @update_mdata() - Update the FWU metadata copy
*/
struct fwu_mdata_ops {
+ /**
+* read_mdata() - Populate the asked FWU metadata copy
+* @dev: FWU metadata device
+* @mdata: Copy of the FWU metadata
+* @primary: If primary or secondary copy of meta-data is to be read
+*
+* Return: 0 if OK, -ve on error
+*/
+ int (*read_mdata)(struct udevice *dev, struct fwu_mdata *mdata, bool
primary);
+
+ /**
+* write_mdata() - Write the given FWU metadata copy
+* @dev: FWU metadata device
+* @mdata: Copy of the FWU metadata
+* @primary: If primary or secondary copy of meta-data is to be written
+*
+* Return: 0 if OK, -ve on error
+*/
+ int (*write_mdata)(struct udevice *dev, struct fwu_mdata *mdata, bool
primary);
+
/**
* check_mdata() - Check if the FWU metadata is valid
* @dev:FWU device
@@ -126,6 +146,27 @@ struct fwu_mdata_ops {
EFI_GUID(0x0c996046, 0xbcc0, 0x4d04, 0x85, 0xec, \
0xe1, 0xfc, 0xed, 0xf1, 0xc6, 0xf8)
+/**
+ * fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata()
+ */
+int fwu_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
+
+/**
+ * fwu_write_mdata() - Wrapper around fwu_mdata_ops.write_mdata()
+ */
+int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool
primary);
+
+/**
+ * fwu_get_verified_mdata() - Read, verify and return the FWU metadata
+ *
+ * Read both the metadata copies from the storage media, verify their checksum,
+ * and ascertain that both copies match. If one of the copies has gone bad,
+ * restore it from the good copy.
+ *
+ * Return: 0 if OK, -ve on error
+*/
+int fwu_get_verified_mdata(struct fwu_mdata *mdata);
+
/**
* fwu_check_mdata_validity() - Check for validity of the FWU metadata copies
*
diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c
index 5313d07302..4554654727 100644
--- a/lib/fwu_updates/fwu.c
+++ b/lib/fwu_updates/fwu.c
@@ -15,13 +15,13 @@
#include
#include
+#include
+
+static struct fwu_mdata g_mdata; /* = {0} makes uninit crc32 always invalid */
+static struct udevice *g_dev;
static u8 in_trial;
static u8 boottime_check;
-#include
-#include
-#include
-
enum {
IMAGE_ACCEPT_SET = 1,
IMAGE_ACCEPT_CLEAR,
@@ -161,6 +161,140 @@ static int fwu_get_image_type_id(u8 *image_index,
efi_guid_t *image_type_id)
return -ENOENT;
}
+/**
+ * fwu_sync_mdata() - Update given meta-data partition(s) with the copy
provided
+ * @mdata: FWU metadata structure
+ * @part: Bitmask of FWU metadata partitions to be written to
+ *
+ * Return: 0 if OK, -ve on error
+ */
+static int fwu_sync_mdata(struct fwu_mdata *mdata, int part)
+{
+ void *buf = >version;
+ int err = 0;
+
+ /*
+* Calculate the crc32 for the updated FWU metadata
+* and put the updated value in the FWU metadata crc32
+* field
+*/
+ mdata->crc32 = crc32(0, buf, sizeof(*mda
[PATCHv3 1/5] fwu: gpt: use cached meta-data partition numbers
Use cached values and avoid parsing and scanning through partitions
everytime for meta-data partitions because they can't change after bootup.
Acked-by: Etienne Carriere
Signed-off-by: Jassi Brar
---
drivers/fwu-mdata/gpt_blk.c | 43 +
1 file changed, 24 insertions(+), 19 deletions(-)
diff --git a/drivers/fwu-mdata/gpt_blk.c b/drivers/fwu-mdata/gpt_blk.c
index d35ce49c5c..28f5d23e1e 100644
--- a/drivers/fwu-mdata/gpt_blk.c
+++ b/drivers/fwu-mdata/gpt_blk.c
@@ -24,8 +24,9 @@ enum {
MDATA_WRITE,
};
-static int gpt_get_mdata_partitions(struct blk_desc *desc,
- uint mdata_parts[2])
+static uint g_mdata_part[2]; /* = {0, 0} to check against uninit parts */
+
+static int gpt_get_mdata_partitions(struct blk_desc *desc)
{
int i, ret;
u32 nparts;
@@ -33,18 +34,19 @@ static int gpt_get_mdata_partitions(struct blk_desc *desc,
struct disk_partition info;
const efi_guid_t fwu_mdata_guid = FWU_MDATA_GUID;
+ /* if primary and secondary partitions already found */
+ if (g_mdata_part[0] && g_mdata_part[1])
+ return 0;
+
nparts = 0;
- for (i = 1; i < MAX_SEARCH_PARTITIONS; i++) {
+ for (i = 1; i < MAX_SEARCH_PARTITIONS && nparts < 2; i++) {
if (part_get_info(desc, i, ))
continue;
uuid_str_to_bin(info.type_guid, part_type_guid.b,
UUID_STR_FORMAT_GUID);
- if (!guidcmp(_mdata_guid, _type_guid)) {
- if (nparts < 2)
- mdata_parts[nparts] = i;
- ++nparts;
- }
+ if (!guidcmp(_mdata_guid, _type_guid))
+ g_mdata_part[nparts++] = i;
}
if (nparts != 2) {
@@ -127,26 +129,25 @@ static int fwu_gpt_update_mdata(struct udevice *dev,
struct fwu_mdata *mdata)
{
int ret;
struct blk_desc *desc;
- uint mdata_parts[2];
struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
desc = dev_get_uclass_plat(priv->blk_dev);
- ret = gpt_get_mdata_partitions(desc, mdata_parts);
+ ret = gpt_get_mdata_partitions(desc);
if (ret < 0) {
log_debug("Error getting the FWU metadata partitions\n");
return -ENOENT;
}
/* First write the primary partition */
- ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, mdata_parts[0]);
+ ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, g_mdata_part[0]);
if (ret < 0) {
log_debug("Updating primary FWU metadata partition failed\n");
return ret;
}
/* And now the replica */
- ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, mdata_parts[1]);
+ ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, g_mdata_part[1]);
if (ret < 0) {
log_debug("Updating secondary FWU metadata partition failed\n");
return ret;
@@ -158,16 +159,14 @@ static int fwu_gpt_update_mdata(struct udevice *dev,
struct fwu_mdata *mdata)
static int gpt_get_mdata(struct blk_desc *desc, struct fwu_mdata *mdata)
{
int ret;
- uint mdata_parts[2];
-
- ret = gpt_get_mdata_partitions(desc, mdata_parts);
+ ret = gpt_get_mdata_partitions(desc);
if (ret < 0) {
log_debug("Error getting the FWU metadata partitions\n");
return -ENOENT;
}
- ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, mdata_parts[0]);
+ ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, g_mdata_part[0]);
if (ret < 0) {
log_debug("Failed to read the FWU metadata from the device\n");
return -EIO;
@@ -182,7 +181,7 @@ static int gpt_get_mdata(struct blk_desc *desc, struct
fwu_mdata *mdata)
* Try to read the replica.
*/
memset(mdata, '\0', sizeof(struct fwu_mdata));
- ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, mdata_parts[1]);
+ ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, g_mdata_part[1]);
if (ret < 0) {
log_debug("Failed to read the FWU metadata from the device\n");
return -EIO;
@@ -206,9 +205,15 @@ static int fwu_gpt_get_mdata(struct udevice *dev, struct
fwu_mdata *mdata)
static int fwu_gpt_get_mdata_partitions(struct udevice *dev, uint *mdata_parts)
{
struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
+ int err;
+
+ err = gpt_get_mdata_partitions(dev_get_uclass_plat(priv->blk_dev));
+ if (!err) {
+ mdata_parts[0] = g_mdata_part[0];
+ mdata_parts[1] = g_mdata_part[1];
+ }
- return gpt_get_mdata_partitions(dev_get_uclass_plat(priv->blk_dev),
-
[PATCHv3 0/5] FWU: Handle meta-data in common code
The patchset reduces ~400 lines of code, while keeping the functionality same and making meta-data operations much faster (by using cached structures). Issue: meta-data copies (primary and secondary) are being handled by the backend/storage layer instead of the common core in fwu.c (as also noted by Ilias) that is, gpt_blk.c manages meta-data and similarly raw_mtd.c will have to do the same when it arrives. The code could by make smaller, cleaner and optimised. Basic idea: Introduce .read_mdata() and .write_mdata() in fwu_mdata_ops that simply read/write meta-data copy. The core code takes care of integrity and redundancy of the meta-data, as a result we can get rid of every other callback .get_mdata() .update_mdata() .get_mdata_part_num() .read_mdata_partition() .write_mdata_partition() and the corresponding wrapper functions thereby making the code 100s of LOC smaller. Get rid of fwu_check_mdata_validity() and fwu_mdata_check() which expected underlying layer to manage and verify mdata copies. Implement fwu_get_verified_mdata(struct fwu_mdata *mdata) public function that reads, verifies and, if needed, fixes the meta-data copies. Verified copy of meta-data is now cached as 'g_mdata' in fwu.c, which avoids multiple low-level expensive read and parse calls. gpt meta-data partition numbers are now cached in gpt_blk.c, so that we don't have to do expensive part_get_info() and uid ops. Changes since v2: * Drop whitespace changes * Fix missing mdata copy before return Changes since v1: * Fix typos and misc cosmetic changes * Catch error returns Jassi Brar (5): fwu: gpt: use cached meta-data partition numbers fwu: move meta-data management in core fwu: gpt: implement read_mdata and write_mdata callbacks fwu: meta-data: switch to management by common code fwu: rename fwu_get_verified_mdata to fwu_get_mdata cmd/fwu_mdata.c | 17 +- drivers/fwu-mdata/fwu-mdata-uclass.c | 151 +- drivers/fwu-mdata/gpt_blk.c | 175 +--- include/fwu.h| 198 ++ lib/fwu_updates/fwu.c| 301 --- 5 files changed, 214 insertions(+), 628 deletions(-) -- 2.34.1
Re: [PATCHv2 1/4] fwu: gpt: use cached meta-data partition numbers
On Thu, 22 Dec 2022 at 06:45, Ilias Apalodimas
wrote:
>
> On Fri, Dec 02, 2022 at 09:16:51PM -0600, jassisinghb...@gmail.com wrote:
> > From: Jassi Brar
> >
> > Use cached values and avoid parsing and scanning through partitions
> > everytime for meta-data partitions because they can't change after bootup.
> >
> > Acked-by: Etienne Carriere
> > Signed-off-by: Jassi Brar
> > ---
> > drivers/fwu-mdata/gpt_blk.c | 43 +
> > 1 file changed, 24 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/fwu-mdata/gpt_blk.c b/drivers/fwu-mdata/gpt_blk.c
> > index d35ce49c5c..28f5d23e1e 100644
> > --- a/drivers/fwu-mdata/gpt_blk.c
> > +++ b/drivers/fwu-mdata/gpt_blk.c
> > @@ -24,8 +24,9 @@ enum {
> > MDATA_WRITE,
> > };
> >
> > -static int gpt_get_mdata_partitions(struct blk_desc *desc,
> > - uint mdata_parts[2])
> > +static uint g_mdata_part[2]; /* = {0, 0} to check against uninit parts */
> > +
> > +static int gpt_get_mdata_partitions(struct blk_desc *desc)
> > {
> > int i, ret;
> > u32 nparts;
> > @@ -33,18 +34,19 @@ static int gpt_get_mdata_partitions(struct blk_desc
> > *desc,
> > struct disk_partition info;
> > const efi_guid_t fwu_mdata_guid = FWU_MDATA_GUID;
> >
> > + /* if primary and secondary partitions already found */
> > + if (g_mdata_part[0] && g_mdata_part[1])
> > + return 0;
> > +
> > nparts = 0;
> > - for (i = 1; i < MAX_SEARCH_PARTITIONS; i++) {
> > + for (i = 1; i < MAX_SEARCH_PARTITIONS && nparts < 2; i++) {
> > if (part_get_info(desc, i, ))
> > continue;
> > uuid_str_to_bin(info.type_guid, part_type_guid.b,
> > UUID_STR_FORMAT_GUID);
> >
> > - if (!guidcmp(_mdata_guid, _type_guid)) {
> > - if (nparts < 2)
> > - mdata_parts[nparts] = i;
> > - ++nparts;
> > - }
> > + if (!guidcmp(_mdata_guid, _type_guid))
> > + g_mdata_part[nparts++] = i;
>
> The reason the 'if (nparts < 2)' was outside the main loop was to show
> errors in case the user defined more than two partitions. Can we keep it
> like that or am I the only being paranoid here?
>
I am not sure if that is an 'error', because FWU only cares about the
first two partitions it comes across. There is no way other ones could
impact the operations.
And if fwu code is responsible for correctness of the setup, there are
things that we already don't care about. So maybe we keep the code
simple?
cheers.
Re: [PATCHv2 2/4] fwu: move meta-data management in core
On Tue, 13 Dec 2022 at 09:00, Etienne Carriere
wrote:
>
> Hello Jassi,
>
> On Sat, 3 Dec 2022 at 04:17, wrote:
> >
> > From: Jassi Brar
> >
> > Instead of each i/f having to implement their own meta-data verification
> > and storage, move the logic in common code. This simplifies the i/f code
> > much simpler and compact.
> >
> > Signed-off-by: Jassi Brar
> > ---
> > drivers/fwu-mdata/fwu-mdata-uclass.c | 34 +++
> > include/fwu.h| 41
> > lib/fwu_updates/fwu.c| 143 ++-
> > 3 files changed, 214 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c
> > b/drivers/fwu-mdata/fwu-mdata-uclass.c
> > index b477e9603f..e03773c584 100644
> > --- a/drivers/fwu-mdata/fwu-mdata-uclass.c
> > +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c
> > @@ -16,6 +16,40 @@
> > #include
> > #include
> >
> > +/**
> > + * fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata()
> > + *
> > + * Return: 0 if OK, -ve on error
> > + */
> > +int fwu_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool
> > primary)
> > +{
> > + const struct fwu_mdata_ops *ops = device_get_ops(dev);
> > +
> > + if (!ops->read_mdata) {
> > + log_debug("read_mdata() method not defined\n");
> > + return -ENOSYS;
> > + }
> > +
> > + return ops->read_mdata(dev, mdata, primary);
> > +}
> > +
> > +/**
> > + * fwu_write_mdata() - Wrapper around fwu_mdata_ops.write_mdata()
> > + *
> > + * Return: 0 if OK, -ve on error
> > + */
> > +int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool
> > primary)
> > +{
> > + const struct fwu_mdata_ops *ops = device_get_ops(dev);
> > +
> > + if (!ops->write_mdata) {
> > + log_debug("write_mdata() method not defined\n");
> > + return -ENOSYS;
> > + }
> > +
> > + return ops->write_mdata(dev, mdata, primary);
> > +}
> > +
> > /**
> > * fwu_get_mdata_part_num() - Get the FWU metadata partition numbers
> > * @dev: FWU metadata device
> > diff --git a/include/fwu.h b/include/fwu.h
> > index 0919ced812..1a700c9e6a 100644
> > --- a/include/fwu.h
> > +++ b/include/fwu.h
> > @@ -24,6 +24,26 @@ struct fwu_mdata_gpt_blk_priv {
> > * @update_mdata() - Update the FWU metadata copy
> > */
> > struct fwu_mdata_ops {
> > + /**
> > +* read_mdata() - Populate the asked FWU metadata copy
> > +* @dev: FWU metadata device
> > +* @mdata: Copy of the FWU metadata
> > +* @primary: If primary or secondary copy of meta-data is to be read
> > +*
> > +* Return: 0 if OK, -ve on error
> > +*/
> > + int (*read_mdata)(struct udevice *dev, struct fwu_mdata *mdata,
> > bool primary);
> > +
> > + /**
> > +* write_mdata() - Write the given FWU metadata copy
> > +* @dev: FWU metadata device
> > +* @mdata: Copy of the FWU metadata
> > +* @primary: If primary or secondary copy of meta-data is to be
> > written
> > +*
> > +* Return: 0 if OK, -ve on error
> > +*/
> > + int (*write_mdata)(struct udevice *dev, struct fwu_mdata *mdata,
> > bool primary);
> > +
> > /**
> > * check_mdata() - Check if the FWU metadata is valid
> > * @dev:FWU device
> > @@ -126,6 +146,27 @@ struct fwu_mdata_ops {
> > EFI_GUID(0x0c996046, 0xbcc0, 0x4d04, 0x85, 0xec, \
> > 0xe1, 0xfc, 0xed, 0xf1, 0xc6, 0xf8)
> >
> > +/**
> > + * fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata()
> > + */
> > +int fwu_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool
> > primary);
> > +
> > +/**
> > + * fwu_write_mdata() - Wrapper around fwu_mdata_ops.write_mdata()
> > + */
> > +int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool
> > primary);
> > +
> > +/**
> > + * fwu_get_verified_mdata() - Read, verify and return the FWU metadata
> > + *
> > + * Read both the metadata copies from the storage media, verify their
> > checksum,
> > + * and ascertain that both copies match. If one of the copies has gone bad,
> > + * restore it from the good copy.
> > + *
> >
Re: [PATCHv2 3/4] fwu: gpt: implement read_mdata and write_mdata callbacks
On Mon, 2 Jan 2023 at 03:48, Etienne Carriere wrote: > > Hi Jassi, > > On Thu, 22 Dec 2022 at 13:59, Ilias Apalodimas > wrote: > > > > Hi Jassi, > > > > On Fri, Dec 02, 2022 at 09:17:12PM -0600, jassisinghb...@gmail.com wrote: > > > From: Jassi Brar > > > > > > Moving towards using common code for meta-data management, > > > implement the read/write mdata hooks. > > > > > > Signed-off-by: Jassi Brar > > > --- > > > drivers/fwu-mdata/gpt_blk.c | 40 +++-- > > > 1 file changed, 38 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/fwu-mdata/gpt_blk.c b/drivers/fwu-mdata/gpt_blk.c > > > index 28f5d23e1e..35239c0a4f 100644 > > > --- a/drivers/fwu-mdata/gpt_blk.c > > > +++ b/drivers/fwu-mdata/gpt_blk.c > > > @@ -222,7 +222,7 @@ static int fwu_gpt_read_mdata_partition(struct > > > udevice *dev, > > > struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev); > > > > > > return gpt_read_write_mdata(dev_get_uclass_plat(priv->blk_dev), > > > - mdata, MDATA_READ, part_num); > > > +mdata, MDATA_READ, part_num); > > > > I assume this was by mistake? > > During the churn I changed from indentation by tabs to spaces. I will drop the change. > > > > Other than that > > Reviewed-by: Ilias Apalodimas > > > > Reviewed-by: Etienne Carriere with > Ilias' comments addressed. Thanks.
Re: [PATCH 2/2] dts: synquacer: Drop unused and undocumented GPIO 'base' property
On Thu, 8 Dec 2022 at 23:10, Masahisa Kojima wrote: > > + Jassi > > On Fri, 9 Dec 2022 at 11:45, Rob Herring wrote: > > > > The 'base' GPIO controller property is unused in u-boot and Linux. It is > > also not documented in the binding. So drop it. > > > > Signed-off-by: Rob Herring > > --- > > arch/arm/dts/synquacer-sc2a11.dtsi | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/arch/arm/dts/synquacer-sc2a11.dtsi > > b/arch/arm/dts/synquacer-sc2a11.dtsi > > index 0dd2969b5e3c..7ba1cd1bee70 100644 > > --- a/arch/arm/dts/synquacer-sc2a11.dtsi > > +++ b/arch/arm/dts/synquacer-sc2a11.dtsi > > @@ -497,7 +497,6 @@ > > gpio-controller; > > #gpio-cells = <2>; > > clocks = <_apb>; > > -base = <0>; > > Reviewed-by: Masahisa Kojima > Acked-by: Jassi Brar
Re: [PATCH 1/2] dts: synquacer: Drop unused and undocumented SPI properties
On Thu, 8 Dec 2022 at 23:09, Masahisa Kojima wrote: > > + Jassi > > On Fri, 9 Dec 2022 at 11:45, Rob Herring wrote: > > > > 'active_clk_edges' and 'chipselect_num' SPI controller properties are > > unused in u-boot and Linux. They are also not documented in the binding. > > So drop them. > > > > Signed-off-by: Rob Herring > > --- > > arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi > > b/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi > > index 6b85c05458d4..9f9837b33bef 100644 > > --- a/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi > > +++ b/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi > > @@ -20,8 +20,6 @@ > > #address-cells = <1>; > > #size-cells = <0>; > > status = "okay"; > > - active_clk_edges; > > - chipselect_num = <1>; > > Reviewed-by: Masahisa Kojima > Acked-by: Jassi Brar
Re: [PATCH 2/4] fwu: move meta-data management in core
On Mon, Nov 7, 2022 at 2:13 PM Etienne Carriere
wrote:
>
> Hello Jassi,
>
> On Mon, 7 Nov 2022 at 19:29, Jassi Brar wrote:
> >
> > On Mon, Nov 7, 2022 at 11:24 AM Etienne Carriere
> > wrote:
> > > On Fri, 4 Nov 2022 at 03:43, wrote:
> > > >
> > > > +/**
> > > > + * fwu_get_verified_mdata() - Read, verify and return the FWU metadata
> > > > + *
> > > > + * Read both the metadata copies from the storage media, verify their
> > > > checksum,
> > > > + * and ascertain that both copies match. If one of the copies has gone
> > > > bad,
> > > > + * restore it from the good copy.
> > > > + *
> > > > + * Return: 0 if OK, -ve on error
> > > > +*/
> > > > +int fwu_get_verified_mdata(struct fwu_mdata *mdata);
> > >
> > > Nitpicking: would you be ok to rename this function to fwu_get_mdata().
> > > When getting fwu mdata, we obviously expect to get reliable data.
> > >
> > I originally called it fwu_get_mdata() in my local 1-patch change :)
> > But there already is one such function and I had to rename, only to delete
> > that.
>
> Ok, maybe a later commit to rename the function once the old one has
> been removed in patch 4/4.
>
ok.
> >
> > > > +
> > > > +static struct fwu_mdata g_mdata = { 0 };
> > >
> > > Can remove "= { 0 };"
> > >
> > I knew, but it was supposed to imply that we want the first crc check
> > to fail (because we set that to 0 here).
>
> As zero init is implicit, I think maintainers will ask to remove it here.
>
Though there are many such examples already and the compiler would
anyway ignore it.
> Maybe add an inline comment above the global variable definition.
> /* Crc32 of a zeroes produces a non 0 value */
>
ok.
thanks.
Re: [PATCH 1/4] fwu: gpt: use cached meta-data partition numbers
On Mon, Nov 7, 2022 at 11:24 AM Etienne Carriere
wrote:
>
> Hello Jassi,
>
> Acked-by: Etienne Carriere
> with the 2 below comments addressed.
>
> On Fri, 4 Nov 2022 at 03:42, wrote:
> >
> > From: Jassi Brar
> >
> > Use cached values and avoid parsing and scanning through partitions
> > everytime for meta-data partitions because they can't change after bootup.
> >
> > Signed-off-by: Jassi Brar
> > ---
> > drivers/fwu-mdata/gpt_blk.c | 43 +
> > 1 file changed, 24 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/fwu-mdata/gpt_blk.c b/drivers/fwu-mdata/gpt_blk.c
> > index d35ce49c5c..d706e3d4e4 100644
> > --- a/drivers/fwu-mdata/gpt_blk.c
> > +++ b/drivers/fwu-mdata/gpt_blk.c
> > @@ -24,8 +24,9 @@ enum {
> > MDATA_WRITE,
> > };
> >
> > -static int gpt_get_mdata_partitions(struct blk_desc *desc,
> > - uint mdata_parts[2])
> > +static uint g_mdata_part[2] = {0, 0};
>
> Not needed to initialize to 0.
> "static uint g_mdata_part[2];" is enough.
>
I am aware. The 0 is like an explicit poison value that we test
against for un-initialized partition numbers.
> > +
> > +static int gpt_get_mdata_partitions(struct blk_desc *desc)
> > {
> > int i, ret;
> > u32 nparts;
> > @@ -33,18 +34,19 @@ static int gpt_get_mdata_partitions(struct blk_desc
> > *desc,
> > struct disk_partition info;
> > const efi_guid_t fwu_mdata_guid = FWU_MDATA_GUID;
> >
> > + /* if primary and secondary partitons already found */
>
> s/partitons/partitions/
>
ok
Thank you!
Re: [PATCH 4/4] fwu: meta-data: switch to management by common code
On Mon, Nov 7, 2022 at 11:23 AM Etienne Carriere wrote: > > Hello Jassi, > > FYI, I've successfully tested this series on stm32mp1 for FWU, once > the few typos fixed to build it them. > Thanks for testing. I will fix the typos and resubmit. . > > @@ -831,11 +662,7 @@ static int fwu_boottime_checks(void *ctx, struct event > > *event) > > if (efi_init_obj_list() != EFI_SUCCESS) > > return 0; > > > > - ret = fwu_get_dev_mdata(, ); > > - if (ret) > > - return ret; > > - > > - in_trial = in_trial_state(); > > + in_trial = in_trial_state(_mdata); > > Since g_mdata is global, in_trial_state() could access it straight > without needing to pass g_mdata as argument. > I wanted to keep changes to the minimum. Also using global variables is ugly, so I wanted to keep the g_mdata usage to minimum (only for initializing the local variable). And finally the signature in_trial_state(void) hides the fact that it works on external data - g_mdata. Cheers!
Re: [PATCH 3/4] fwu: gpt: implement read_mdata and write_mdata callbacks
On Mon, Nov 7, 2022 at 11:23 AM Etienne Carriere
wrote:
>
> On Fri, 4 Nov 2022 at 03:43, wrote:
> >
> > From: Jassi Brar
> >
> > Moving towards using common code for meta-data management,
> > implement the read/write mdata hooks.
> >
> > Signed-off-by: Jassi Brar
> > ---
> > drivers/fwu-mdata/gpt_blk.c | 36
> > 1 file changed, 36 insertions(+)
> >
> > diff --git a/drivers/fwu-mdata/gpt_blk.c b/drivers/fwu-mdata/gpt_blk.c
> > index d706e3d4e4..7fda041302 100644
> > --- a/drivers/fwu-mdata/gpt_blk.c
> > +++ b/drivers/fwu-mdata/gpt_blk.c
> > @@ -272,7 +272,43 @@ static int fwu_mdata_gpt_blk_probe(struct udevice *dev)
> > return 0;
> > }
> >
> > +static int fwu_gpt_read_mdata(struct udevice *dev, struct fwu_mdata *mdata,
> > +bool primary)
> > +{
> > + struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
> > + struct blk_desc *desc = dev_get_uclass_plat(priv->blk_dev);
> > + int ret;
> > +
> > + ret = gpt_get_mdata_partitions(desc);
> > + if (ret < 0) {
> > + log_debug("Error getting the FWU metadata partitions\n");
> > + return -ENOENT;
>
> Not returning ret value?
>
It is unchanged from original behavior.
> > + }
> > +
> > + return gpt_read_write_mdata(desc, mdata, MDATA_READ,
> > + primary ? g_mdata_part[0] :
> > g_mdata_part[1]);
>
> Fix indentation.
>
>
> > +}
> > +
> > +static int fwu_gpt_write_mdata(struct udevice *dev, struct fwu_mdata
> > *mdata,
> > +bool primary)
> > +{
> > + struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
> > + struct blk_desc *desc = dev_get_uclass_plat(priv->blk_dev);
> > + int ret;
> > +
> > + ret = gpt_get_mdata_partitions(desc);
> > + if (ret < 0) {
> > + log_debug("Error getting the FWU metadata partitions\n");
> > + return -ENOENT;
> > + }
> > +
> > + return gpt_read_write_mdata(desc, mdata, MDATA_WRITE,
> > + primary ? g_mdata_part[0] :
> > g_mdata_part[1]);
>
> Dito
>
OK.
thanks.
Re: [PATCH 2/4] fwu: move meta-data management in core
On Mon, Nov 7, 2022 at 11:24 AM Etienne Carriere
wrote:
> On Fri, 4 Nov 2022 at 03:43, wrote:
> >
> > +/**
> > + * fwu_get_verified_mdata() - Read, verify and return the FWU metadata
> > + *
> > + * Read both the metadata copies from the storage media, verify their
> > checksum,
> > + * and ascertain that both copies match. If one of the copies has gone bad,
> > + * restore it from the good copy.
> > + *
> > + * Return: 0 if OK, -ve on error
> > +*/
> > +int fwu_get_verified_mdata(struct fwu_mdata *mdata);
>
> Nitpicking: would you be ok to rename this function to fwu_get_mdata().
> When getting fwu mdata, we obviously expect to get reliable data.
>
I originally called it fwu_get_mdata() in my local 1-patch change :)
But there already is one such function and I had to rename, only to delete that.
> > +
> > +static struct fwu_mdata g_mdata = { 0 };
>
> Can remove "= { 0 };"
>
I knew, but it was supposed to imply that we want the first crc check
to fail (because we set that to 0 here).
.
> > +static inline int mdata_crc_check(struct fwu_mdata *mdata)
> > +{
> > + u32 calc_crc32 = crc32(0, >version, sizeof(*mdata) -
> > sizeof(u32));
>
> Add an empty line below the above definition.
>
OK.
> > + /* if mdata already read and ready */
> > + err = mdata_crc_check(p_mdata, true);
>
> 2nd argument to be removed. Ditto for the other occurrences of
> mdata_crc_check() calls.
>
Ouch, I 'cleaned' the patch before submission. Will fix that.
> Note here I would pass straight _mdata as argument rather than
> p_mdata indirection, for clarity.
>
Hmm... I wanted to keep the g_mdata usage clear and minimal.
> > +
> > + if (!pri_ok) {
> > + memcpy(p_mdata, s_mdata, sizeof(struct fwu_mdata));
> > + err = fwu_sync_mdata(p_mdata, PRIMARY_PART);
>
> Should test return value.
>
> > + }
> > +
> > + if (!sec_ok) {
> > + memcpy(s_mdata, p_mdata, sizeof(struct fwu_mdata));
> > + err = fwu_sync_mdata(s_mdata, SECONDARY_PART);
>
> Ditto
>
> > + }
> > +
> > + /* make sure atleast one copy is good */
>
> s/atleast/at least/
>
ok
> > + err = mdata_crc_check(p_mdata, true);
>
> This is not needed, it's been already verified unless we want to catch
> the case !pri_ok && !sec_ok. I think this case should be explicitly
> handled above with a nice console trace message/
>
ok
Thanks.
Re: [PATCHv2 1/5] FWU: Add FWU metadata access driver for MTD storage regions
On Fri, Oct 14, 2022 at 2:07 AM Ilias Apalodimas
wrote:
>
> On Sun, Oct 02, 2022 at 06:51:32PM -0500, jassisinghb...@gmail.com wrote:
> > +
> > +static bool mtd_is_aligned_with_block_size(struct mtd_info *mtd, u64 size)
> > +{
> > + return !do_div(size, mtd->erasesize);
> > +}
> > +
>
> Can we please add some sphinx style documentation overall ?
>
I can but I thought that is for public api and not static helper functions?
> > +
> > +static int fwu_mtd_check_mdata(struct udevice *dev)
> > +{
> > + struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
> > + struct fwu_mdata primary, secondary;
> > + bool pri = false, sec = false;
> > + int ret;
> > +
> > + ret = fwu_mtd_load_mdata(mtd_priv->mtd, ,
> > +
> > mtd_priv->pri_offset, FWU_MDATA_PRIMARY);
> > + if (ret < 0)
> > + log_debug("Failed to read the primary mdata: %d\n", ret);
> > + else
> > + pri = true;
> > +
> > + ret = fwu_mtd_load_mdata(mtd_priv->mtd, ,
> > +
> > mtd_priv->sec_offset, FWU_MDATA_SECONDARY);
> > + if (ret < 0)
> > + log_debug("Failed to read the secondary mdata: %d\n", ret);
> > + else
> > + sec = true;
> > +
> > + if (pri && sec) {
> > + if (memcmp(, , sizeof(struct fwu_mdata))) {
> > + log_debug("The primary and the secondary mdata are
> > different\n");
> > + ret = -1;
> > + }
> > + } else if (pri) {
> > + ret = fwu_mtd_save_secondary_mdata(mtd_priv, );
> > + if (ret < 0)
> > + log_debug("Restoring secondary mdata partition
> > failed\n");
> > + } else if (sec) {
> > + ret = fwu_mtd_save_primary_mdata(mtd_priv, );
> > + if (ret < 0)
> > + log_debug("Restoring primary mdata partition
> > failed\n");
> > + }
>
> Same on this one. The requirements here are
> - Read our metadata
> - Compare the 2 partitions
>
> The only thing that's 'hardware' specific here is reading the metadata. We
> should at least unify the comparing part and restoration in case of
> failures, no ?
>
Yes. Since redundant copy of meta-data is a requirement of the spec,
we should maintain that in the fwu core code.
Maybe something like adding 'bool primary' argument to get_mdata() and
update_mdata()
thanks
Re: [PATCHv2 4/5] fwu: DeveloperBox: add support for FWU
On Mon, Oct 3, 2022 at 9:44 PM AKASHI Takahiro wrote: > On Mon, Oct 03, 2022 at 09:00:35PM -0500, Jassi Brar wrote: > > > My question is why you use a single capsule (FIP) in A/B update while you > > > use > > > three separate capsule files in normal case. > > > > > We think it is cleaner to not tie up boot binaries at fixed offsets in > > storage, so all CA53 boot assets are now in one parseable FIP image. > > Secondly, and personally, I think there is no real usecase of more > > than one image per bank - that will be too fragile and complicated to > > manage. > > If so, my point is why not use a single capsule in normal case (a single bank > in another word) as well? > for historical/legacy reasons.
Re: [PATCHv2 4/5] fwu: DeveloperBox: add support for FWU
On Mon, Oct 3, 2022 at 8:06 PM AKASHI Takahiro
wrote:
>
> On Mon, Oct 03, 2022 at 04:51:32PM +0300, Ilias Apalodimas wrote:
> > Hi Jassi,
> >
> > On Mon, 3 Oct 2022 at 16:40, Jassi Brar wrote:
> > >
> > > On Mon, Oct 3, 2022 at 6:04 AM AKASHI Takahiro
> > > wrote:
> > >
> > > > > diff --git a/board/socionext/developerbox/developerbox.c
> > > > > b/board/socionext/developerbox/developerbox.c
> > > > > index f5a5fe0121..a0db26eaf3 100644
> > > > > --- a/board/socionext/developerbox/developerbox.c
> > > > > +++ b/board/socionext/developerbox/developerbox.c
> > > > > @@ -20,6 +20,13 @@
> > > > >
> > > > > #if CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT)
> > > > > struct efi_fw_image fw_images[] = {
> > > > > +#if defined(CONFIG_FWU_MULTI_BANK_UPDATE)
> > > > > + {
> > > > > + .image_type_id = DEVELOPERBOX_FIP_IMAGE_GUID,
> > > > > + .fw_name = u"DEVELOPERBOX-FIP",
> > > > > + .image_index = 1,
> > > > > + },
> > > > > +#else
> > > >
> > > > From curiosity, why do you want to use different capsule formats
> > > > for multi-bank update and normal case?
> > > >
> > > normal/legacy layout has one image for each component - uboot, tfa and
> > > optee, whereas the new layout contains everything in one fip image.
>
> Yes, that is exactly what I understand here.
>
> > > So I thought it would be better to make the image_index consistent by
> > > making the fip's as 1.
> >
> > FWIW this does make a lot of sense. Since the SCP firmware is not
> > included in the capsule and that SCP firmware is needed to transition
> > from old -> new layout, I think we are better off having those in
> > different GUIDs. On top of that those GUIDs can be used in LVFS if we
> > ever decide to upload firmwares there.
> >
> > Not having discrete GUIDs means there's a chance to brick the board on
> > old -> new update, unless the SCP is explicitly updated.
>
> SCP? I don't care.
> My question is why you use a single capsule (FIP) in A/B update while you use
> three separate capsule files in normal case.
>
We think it is cleaner to not tie up boot binaries at fixed offsets in
storage, so all CA53 boot assets are now in one parseable FIP image.
Secondly, and personally, I think there is no real usecase of more
than one image per bank - that will be too fragile and complicated to
manage.
cheers.
Re: [PATCHv2 4/5] fwu: DeveloperBox: add support for FWU
On Mon, Oct 3, 2022 at 6:04 AM AKASHI Takahiro
wrote:
> > diff --git a/board/socionext/developerbox/developerbox.c
> > b/board/socionext/developerbox/developerbox.c
> > index f5a5fe0121..a0db26eaf3 100644
> > --- a/board/socionext/developerbox/developerbox.c
> > +++ b/board/socionext/developerbox/developerbox.c
> > @@ -20,6 +20,13 @@
> >
> > #if CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT)
> > struct efi_fw_image fw_images[] = {
> > +#if defined(CONFIG_FWU_MULTI_BANK_UPDATE)
> > + {
> > + .image_type_id = DEVELOPERBOX_FIP_IMAGE_GUID,
> > + .fw_name = u"DEVELOPERBOX-FIP",
> > + .image_index = 1,
> > + },
> > +#else
>
> From curiosity, why do you want to use different capsule formats
> for multi-bank update and normal case?
>
normal/legacy layout has one image for each component - uboot, tfa and
optee, whereas the new layout contains everything in one fip image.
So I thought it would be better to make the image_index consistent by
making the fip's as 1.
cheers.
Re: [PATCH v10 10/15] FWU: Add support for the FWU Multi Bank Update feature
Hi Ilias, On Mon, Oct 3, 2022 at 7:21 AM Ilias Apalodimas wrote: > > Hi Jassi, > > On Wed, Sep 28, 2022 at 10:16:53AM -0500, Jassi Brar wrote: > > Hi Etienne, > > > > On Wed, Sep 28, 2022 at 2:30 AM Etienne Carriere > > wrote: > > > Hello Jassi, Sughosh and all, > > > > > > >>> But a malicious user may force some old vulnerable image back into > > > use > > > >>> by updating all but that image. > > > > > > When the system boots with accepted images (referring to fwu-mdata > > > regular/trial state), the platform monotonic counter is updated > > > against booted image version number if needed, preventing older images > > > to be booted when an accepted image has been deployed. > > > @Jassi, does this answer your question? > > > > > As I said in my earlier post, I know we can employ security+integrity > > techniques to prevent such misuse. > > My point is FWU should still be implemented assuming no such technique > > might be available due to any reason, and we do the best we can. Just > > as we don't say lets not care about buffer-overflow vulnerabilities > > because the system can implement secure boot and other such > > techniques. > > > > For example, the spec warns : "The metadata can be maliciously > > crafted, it should be treated as an insecure information source." So > > clearly the spec doesn't count on rollback and authentication > > mechanisms to be always available - and that is how it should be. > > We've discussed this extensively during drafting the spec. You are right > that we would be better off trying to protect the fwu metadata somehow. In > fact Heinrich had similar concerns when the original RFC was posted. i > Actually I never said we should protect the metadata. If you read the whole thread, the point was that we should try to protect against partial bank updates - accidental or malicious. We can not assume a user updating only partially, knows what they are doing. cheers.
Re: [PATCH v10 02/15] FWU: Add FWU metadata structure and driver for accessing metadata
On Wed, Sep 28, 2022 at 1:00 AM Sughosh Ganu wrote:
>
> On Tue, 27 Sept 2022 at 21:55, Jassi Brar wrote:
> >
> > On Tue, Sep 27, 2022 at 2:14 AM Sughosh Ganu
> > wrote:
> > >
> > > On Mon, 26 Sept 2022 at 20:12, Jassi Brar
> > > wrote:
> > > >
> > > > On Mon, Sep 26, 2022 at 5:00 AM Sughosh Ganu
> > > > wrote:
> > > > >
> > > > > On Mon, 26 Sept 2022 at 08:28, Jassi Brar
> > > > > 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(, );
> > > > > > > + 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_ac
Re: [PATCH v10 10/15] FWU: Add support for the FWU Multi Bank Update feature
Hi Etienne, On Wed, Sep 28, 2022 at 2:30 AM Etienne Carriere wrote: > Hello Jassi, Sughosh and all, > > >>> But a malicious user may force some old vulnerable image back into use > >>> by updating all but that image. > > When the system boots with accepted images (referring to fwu-mdata > regular/trial state), the platform monotonic counter is updated > against booted image version number if needed, preventing older images > to be booted when an accepted image has been deployed. > @Jassi, does this answer your question? > As I said in my earlier post, I know we can employ security+integrity techniques to prevent such misuse. My point is FWU should still be implemented assuming no such technique might be available due to any reason, and we do the best we can. Just as we don't say lets not care about buffer-overflow vulnerabilities because the system can implement secure boot and other such techniques. For example, the spec warns : "The metadata can be maliciously crafted, it should be treated as an insecure information source." So clearly the spec doesn't count on rollback and authentication mechanisms to be always available - and that is how it should be. cheers.
Re: [PATCH v10 10/15] FWU: Add support for the FWU Multi Bank Update feature
On Tue, Sep 27, 2022 at 2:22 AM Sughosh Ganu wrote:
>
> On Mon, 26 Sept 2022 at 20:24, Jassi Brar wrote:
> >
> > On Mon, Sep 26, 2022 at 4:01 AM Sughosh Ganu
> > wrote:
> > > On Mon, 26 Sept 2022 at 08:25, Jassi Brar
> > > wrote:
> >
> > > > .
> > > > >
> > > > > +static __maybe_unused efi_status_t fwu_post_update_process(bool
> > > > > fw_accept_os)
> > > > > +{
> > > > > + int status;
> > > > > + u32 update_index;
> > > > > + efi_status_t ret;
> > > > > +
> > > > > + status = fwu_plat_get_update_index(_index);
> > > > > + if (status < 0) {
> > > > > + log_err("Failed to get the FWU update_index value\n");
> > > > > + return EFI_DEVICE_ERROR;
> > > > > + }
> > > > > +
> > > > > + /*
> > > > > +* All the capsules have been updated successfully,
> > > > > +* update the FWU metadata.
> > > > > +*/
> > > > > + log_debug("Update Complete. Now updating active_index to
> > > > > %u\n",
> > > > > + update_index);
> > > > > + status = fwu_update_active_index(update_index);
> > > > >
> > > > Do we want to check if all images in the bank are updated via capsules
> > > > before switching the bank?
> > >
> > > This function does get called only when the update status for every
> > > capsule is a success. Even if one of the capsules does not get
> > > updated, the active index will not get updated.
> > >
> > but we don't check if the capsule for each image in the bank is
> > provided for update.
>
> Yes, we have had this discussion earlier. Neither the FWU spec, nor
> the capsule update spec in UEFI puts that restriction that all images
> on the platform need to be updated. If a user wants to ensure such a
> behaviour, they may choose some kind of image packaging like FIP or
> FIT which would mean that all the images on the platform are being
> updated. But this is not something to be ensured by the FWU update
> code.
>
> >
> > > >
> > > > A developer will make sure all images are provided in one go, so that
> > > > the switch is successful.
> > > > But a malicious user may force some old vulnerable image back into use
> > > > by updating all but that image.
> > >
> > > That I believe is to be handled through a combination of implementing
> > > a rollback protection mechanism, along with capsule authentication.
> > > These are separate to the implementation of the multi bank updates
> > > that these patches are aiming for.
> > >
> > This sounds like : we don't worry about buffer-overflow
> > vulnerabilities because the system will be secured and hardened by
> > other mechanisms.
>
> Not sure how this is related. The aim of the FWU spec is for providing
> a means for a platform to maintain multiple partitions of images and
> to specify the metadata structure for the bookkeeping of the different
> partitions and images on those partitions. If we need a more secure
> and hardened system, we do have the Dependable Boot spec, which is
> talking precisely about these things. Those are indeed separate
> features or aspects from what the FWU spec is talking about. And this
> patchset is implementing the FWU spec.
>
I am out of ways to explain. Best of luck.
Re: [PATCH v10 02/15] FWU: Add FWU metadata structure and driver for accessing metadata
On Tue, Sep 27, 2022 at 2:14 AM Sughosh Ganu wrote:
>
> On Mon, 26 Sept 2022 at 20:12, Jassi Brar wrote:
> >
> > On Mon, Sep 26, 2022 at 5:00 AM Sughosh Ganu
> > wrote:
> > >
> > > On Mon, 26 Sept 2022 at 08:28, Jassi Brar
> > > 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(, );
> > > > > + 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
Re: [PATCH 5/5] fwu: DeveloperBox: add support for FWU
On Thu, 1 Sept 2022 at 02:28, Heinrich Schuchardt wrote:
> > On 7/22/22 19:43, jassisinghb...@gmail.com wrote:
> >> diff --git a/board/socionext/developerbox/developerbox.c
> >> b/board/socionext/developerbox/developerbox.c
> >> index f5a5fe0121..ad2260e3d7 100644
> >> --- a/board/socionext/developerbox/developerbox.c
> >> +++ b/board/socionext/developerbox/developerbox.c
> >> @@ -20,6 +20,13 @@
> >>
> >> #if CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT)
> >> struct efi_fw_image fw_images[] = {
> >> +#if defined(CONFIG_FWU_MULTI_BANK_UPDATE)
> >> +{
> >> +.image_type_id = DEVELOPERBOX_FIP_IMAGE_GUID,
> >> +.fw_name = u"DEVELOPERBOX-FIP",
> >
> > The design is flawed. These fields should be moved to the device-tree.
>
> Currently we are changing C files for each board were we enable firmware
> updates. Probably an even better place then the device-tree would be a
> Kconfig file. The only problem with Kconfig is that it does not easily
> allow to edit arrays. But we could use a string like:
>
> GUID,name,index,GUID,name,index,...
>
Probably. But there already exists the structure that this patch only
adds an entry to. Moving that structure into dt or kconfig should be a
separate task of different context.
Also right now I don't want to diverge from gpt based STM's
implementation which does the same thing.
thanks
Re: [PATCH v10 03/15] FWU: Add FWU metadata access driver for GPT partitioned block devices
On Mon, Sep 26, 2022 at 3:48 AM Sughosh Ganu wrote:
>
> On Mon, 26 Sept 2022 at 08:22, Jassi Brar wrote:
> >
> > On Thu, Sep 15, 2022 at 3:16 AM Sughosh Ganu
> > wrote:
> >
> > > diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig
> > > new file mode 100644
> > > index 00..7322da48b1
> > > --- /dev/null
> > > +++ b/drivers/fwu-mdata/Kconfig
> > > @@ -0,0 +1,16 @@
> > > +config FWU_MDATA
> > > + bool "Driver support for accessing FWU Metadata"
> > > + depends on DM
> > > + help
> > > + Enable support for accessing FWU Metadata partitions. The
> > > + FWU Metadata partitions reside on the same storage device
> > > + which contains the other FWU updatable firmware images.
> > > +
> > > +config FWU_MDATA_GPT_BLK
> > > + bool "FWU Metadata access for GPT partitioned Block devices"
> > > + select PARTITION_TYPE_GUID
> > > + select PARTITION_UUIDS
> > > + depends on DM && HAVE_BLOCK_DEVICE && EFI_PARTITION
> > >
> > It should depend on FWU_MDATA instead of DM.
> > Though, eventually it will be a choice between GPT and MTD.
>
> Yes, this should depend on FWU_MDATA. Will change.
>
> >
> > ...
> > > +static int fwu_gpt_mdata_check(struct udevice *dev)
> > > +{
> > > + /*
> > > +* Check if both the copies of the FWU
> > > +* metadata are valid. If one has gone
> > > +* bad, restore it from the other good
> > > +* copy.
> > > +*/
> > >
> > This comment is repeated 3 more times in the patchset. Maybe get rid
> > of this and some other.
>
> Sorry, I could not find any other comment like this in the file. Can
> you point me out where this comment has been repeated.
>
I said patchset, not file. grep'ing "restore it from the other" in
the patches show
drivers/fwu-mdata/gpt_blk.c, drivers/fwu-mdata/fwu-mdata-uclass.c
and twice in include/fwu.h
Btw, the other three times it says "restore it from the other bad copy" :D
-j
Re: [PATCH v10 10/15] FWU: Add support for the FWU Multi Bank Update feature
On Mon, Sep 26, 2022 at 4:01 AM Sughosh Ganu wrote:
> On Mon, 26 Sept 2022 at 08:25, Jassi Brar wrote:
> > .
> > >
> > > +static __maybe_unused efi_status_t fwu_post_update_process(bool
> > > fw_accept_os)
> > > +{
> > > + int status;
> > > + u32 update_index;
> > > + efi_status_t ret;
> > > +
> > > + status = fwu_plat_get_update_index(_index);
> > > + if (status < 0) {
> > > + log_err("Failed to get the FWU update_index value\n");
> > > + return EFI_DEVICE_ERROR;
> > > + }
> > > +
> > > + /*
> > > +* All the capsules have been updated successfully,
> > > +* update the FWU metadata.
> > > +*/
> > > + log_debug("Update Complete. Now updating active_index to %u\n",
> > > + update_index);
> > > + status = fwu_update_active_index(update_index);
> > >
> > Do we want to check if all images in the bank are updated via capsules
> > before switching the bank?
>
> This function does get called only when the update status for every
> capsule is a success. Even if one of the capsules does not get
> updated, the active index will not get updated.
>
but we don't check if the capsule for each image in the bank is
provided for update.
> >
> > A developer will make sure all images are provided in one go, so that
> > the switch is successful.
> > But a malicious user may force some old vulnerable image back into use
> > by updating all but that image.
>
> That I believe is to be handled through a combination of implementing
> a rollback protection mechanism, along with capsule authentication.
> These are separate to the implementation of the multi bank updates
> that these patches are aiming for.
>
This sounds like : we don't worry about buffer-overflow
vulnerabilities because the system will be secured and hardened by
other mechanisms.
A/B update does not _require_ rollback-protection or
capsure-authentication. A platform may rely on some other technology
for tamper-proofing.
-j
Re: [PATCH v10 02/15] FWU: Add FWU metadata structure and driver for accessing metadata
On Mon, Sep 26, 2022 at 5:00 AM Sughosh Ganu wrote:
>
> On Mon, 26 Sept 2022 at 08:28, Jassi Brar 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(, );
> > > + 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.
Re: [PATCH v10 09/15] FWU: Add boot time checks as highlighted by the FWU specification
On Mon, Sep 26, 2022 at 5:08 AM Sughosh Ganu wrote:
>
> On Mon, 26 Sept 2022 at 08:29, Jassi Brar wrote:
> > .
> > > +static int fwu_boottime_checks(void *ctx, struct event *event)
> > > +{
> > > + int ret;
> > > + struct udevice *dev;
> > > + u32 boot_idx, active_idx;
> > > +
> > > + ret = fwu_get_dev_mdata(, NULL);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = fwu_mdata_check(dev);
> > > + if (ret) {
> > > + return 0;
> > > + }
> > > +
> > > + /*
> > > +* Get the Boot Index, i.e. the bank from
> > > +* which the platform has booted. This value
> > > +* gets passed from the ealier stage bootloader
> > > +* which booted u-boot, e.g. tf-a. If the
> > > +* boot index is not the same as the
> > > +* active_index read from the FWU metadata,
> > > +* update the active_index.
> > > +*/
> > > + fwu_plat_get_bootidx(_idx);
> > > + if (boot_idx >= CONFIG_FWU_NUM_BANKS) {
> > > + log_err("Received incorrect value of boot_index\n");
> > > + return 0;
> > > + }
> > > +
> > > + ret = fwu_get_active_index(_idx);
> > > + if (ret) {
> > > + log_err("Unable to read active_index\n");
> > > + return 0;
> > > + }
> > > +
> > > + if (boot_idx != active_idx) {
> > > + log_info("Boot idx %u is not matching active idx %u,
> > > changing active_idx\n",
> > > +boot_idx, active_idx);
> > > + ret = fwu_update_active_index(boot_idx);
> > > + if (!ret)
> > > + boottime_check = 1;
> > >
> > We may not want to do anything FWU (accept, reject, modify mdata)
> > until we reboot, if we are recovering from last bad upgrade. So maybe
> > not set boottime_check
>
> Actually, the difference between the boot bank and active bank will
> happen when there is some kind of corruption on the media due to which
> the platform could not boot from the active bank(could also be due to
> repeated wd timeouts).
>
... which may have been caused by the last upgrade attempt, among other reasons.
fwu_trial_state_check() will never be called in this case and any
subsequent fwu_update_checks_pass() will pass even if we are in trial
state.
-j
Re: [PATCH v10 09/15] FWU: Add boot time checks as highlighted by the FWU specification
On Thu, Sep 15, 2022 at 3:17 AM Sughosh Ganu wrote:
> diff --git a/include/fwu.h b/include/fwu.h
> index 484289ed4f..d5f77ce83c 100644
> --- a/include/fwu.h
> +++ b/include/fwu.h
> @@ -256,4 +256,17 @@ int fwu_plat_get_update_index(uint *update_idx);
> *
> */
> void fwu_plat_get_bootidx(uint *boot_idx);
>
Or simply return the boot_idx instead of modifying the pointed variable
.
> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> index c633fcd91e..557e97de4a 100644
> --- a/lib/efi_loader/efi_setup.c
> +++ b/lib/efi_loader/efi_setup.c
> @@ -199,6 +199,7 @@ static efi_status_t __efi_init_early(void)
> goto out;
>
> ret = efi_disk_init();
> +
> out:
> return ret;
> }
We can do without this change in this patchset :)
.
> +static int fwu_trial_state_check(struct udevice *dev)
> +{
> + int ret;
> + efi_status_t status;
> + efi_uintn_t var_size;
> + u16 trial_state_ctr;
> + u32 var_attributes;
> + struct fwu_mdata mdata = { 0 };
> +
> + ret = fwu_get_mdata(dev, );
> + if (ret)
> + return ret;
> +
> + if ((trial_state = in_trial_state())) {
>
This may raise warnings on code checkers. So maybe move the assignment
out of the check.
.
> +static int fwu_boottime_checks(void *ctx, struct event *event)
> +{
> + int ret;
> + struct udevice *dev;
> + u32 boot_idx, active_idx;
> +
> + ret = fwu_get_dev_mdata(, NULL);
> + if (ret)
> + return ret;
> +
> + ret = fwu_mdata_check(dev);
> + if (ret) {
> + return 0;
> + }
> +
> + /*
> +* Get the Boot Index, i.e. the bank from
> +* which the platform has booted. This value
> +* gets passed from the ealier stage bootloader
> +* which booted u-boot, e.g. tf-a. If the
> +* boot index is not the same as the
> +* active_index read from the FWU metadata,
> +* update the active_index.
> +*/
> + fwu_plat_get_bootidx(_idx);
> + if (boot_idx >= CONFIG_FWU_NUM_BANKS) {
> + log_err("Received incorrect value of boot_index\n");
> + return 0;
> + }
> +
> + ret = fwu_get_active_index(_idx);
> + if (ret) {
> + log_err("Unable to read active_index\n");
> + return 0;
> + }
> +
> + if (boot_idx != active_idx) {
> + log_info("Boot idx %u is not matching active idx %u, changing
> active_idx\n",
> +boot_idx, active_idx);
> + ret = fwu_update_active_index(boot_idx);
> + if (!ret)
> + boottime_check = 1;
>
We may not want to do anything FWU (accept, reject, modify mdata)
until we reboot, if we are recovering from last bad upgrade. So maybe
not set boottime_check
cheers
Re: [PATCH v10 02/15] FWU: Add FWU metadata structure and driver for accessing metadata
On Thu, Sep 15, 2022 at 3:15 AM Sughosh Ganu 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(, );
> + 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.
Re: [PATCH v10 10/15] FWU: Add support for the FWU Multi Bank Update feature
On Thu, Sep 15, 2022 at 3:17 AM Sughosh Ganu wrote:
> +
> +static __maybe_unused void fwu_post_update_checks(
> + struct efi_capsule_header *capsule,
> + bool *fw_accept_os, bool *capsule_update)
> +{
> + if (fwu_empty_capsule(capsule))
> + *capsule_update = false;
> + else
> + if (!*fw_accept_os)
> + *fw_accept_os =
> + capsule->flags & FW_ACCEPT_OS ? 0x1 : 0x0;
>
True and False, instead of 1 and 0 for consistency.
.
>
> +static __maybe_unused efi_status_t fwu_post_update_process(bool fw_accept_os)
> +{
> + int status;
> + u32 update_index;
> + efi_status_t ret;
> +
> + status = fwu_plat_get_update_index(_index);
> + if (status < 0) {
> + log_err("Failed to get the FWU update_index value\n");
> + return EFI_DEVICE_ERROR;
> + }
> +
> + /*
> +* All the capsules have been updated successfully,
> +* update the FWU metadata.
> +*/
> + log_debug("Update Complete. Now updating active_index to %u\n",
> + update_index);
> + status = fwu_update_active_index(update_index);
>
Do we want to check if all images in the bank are updated via capsules
before switching the bank?
A developer will make sure all images are provided in one go, so that
the switch is successful.
But a malicious user may force some old vulnerable image back into use
by updating all but that image.
> @@ -410,7 +544,35 @@ static efi_status_t efi_capsule_update_firmware(
> int item;
> struct efi_firmware_management_protocol *fmp;
> u16 *abort_reason;
> + efi_guid_t image_type_id;
> efi_status_t ret = EFI_SUCCESS;
> + int status;
> + u8 image_index;
> + u32 update_index;
> + bool fw_accept_os, image_index_check;
> +
> + if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {
> + if (!fwu_empty_capsule(capsule_data) &&
> + !fwu_update_checks_pass()) {
> + log_err("FWU checks failed. Cannot start update\n");
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + if (fwu_empty_capsule(capsule_data))
> + return fwu_empty_capsule_process(capsule_data);
> +
This could be simplified as
if (fwu_empty_capsule(capsule_data))
return fwu_empty_capsule_process(capsule_data);
if (!fwu_update_checks_pass()) {
log_err("FWU checks failed. Cannot start update\n");
return EFI_INVALID_PARAMETER;
}
> @@ -1151,7 +1374,15 @@ efi_status_t efi_launch_capsules(void)
> log_err("Deleting capsule %ls failed\n",
> files[i]);
> }
> +
> efi_capsule_scan_done();
> + if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {
>
missing newline before the if
> + if (update_status == true && capsule_update == true) {
> + ret = fwu_post_update_process(fw_accept_os);
> + } else if (capsule_update == true && update_status == false) {
> + log_err("All capsules were not updated. Not updating
> FWU metadata\n");
> + }
nit: maybe keep the order of capsule_update and update_status same in
both clauses.
cheers.
Re: [PATCH v10 03/15] FWU: Add FWU metadata access driver for GPT partitioned block devices
On Thu, Sep 15, 2022 at 3:16 AM Sughosh Ganu wrote:
> diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig
> new file mode 100644
> index 00..7322da48b1
> --- /dev/null
> +++ b/drivers/fwu-mdata/Kconfig
> @@ -0,0 +1,16 @@
> +config FWU_MDATA
> + bool "Driver support for accessing FWU Metadata"
> + depends on DM
> + help
> + Enable support for accessing FWU Metadata partitions. The
> + FWU Metadata partitions reside on the same storage device
> + which contains the other FWU updatable firmware images.
> +
> +config FWU_MDATA_GPT_BLK
> + bool "FWU Metadata access for GPT partitioned Block devices"
> + select PARTITION_TYPE_GUID
> + select PARTITION_UUIDS
> + depends on DM && HAVE_BLOCK_DEVICE && EFI_PARTITION
>
It should depend on FWU_MDATA instead of DM.
Though, eventually it will be a choice between GPT and MTD.
...
> +static int fwu_gpt_mdata_check(struct udevice *dev)
> +{
> + /*
> +* Check if both the copies of the FWU
> +* metadata are valid. If one has gone
> +* bad, restore it from the other good
> +* copy.
> +*/
>
This comment is repeated 3 more times in the patchset. Maybe get rid
of this and some other.
cheers.
Re: [PATCH v10 15/15] FWU: doc: Add documentation for the FWU feature
On Thu, 15 Sept 2022 at 03:16, Sughosh Ganu wrote: > diff --git a/doc/develop/uefi/fwu_updates.rst > b/doc/develop/uefi/fwu_updates.rst > new file mode 100644 > index 00..fad3fbb3a8 > --- /dev/null > +++ b/doc/develop/uefi/fwu_updates.rst > @@ -0,0 +1,165 @@ > +.. SPDX-License-Identifier: GPL-2.0+ > +.. Copyright (c) 2022 Linaro Limited > + > +FWU Multi Bank Updates in U-Boot > + > + > +The FWU Multi Bank Update feature implements the firmware update > +mechanism described in the PSA Firmware Update for A-profile Arm > +Architecture specification [1]. Certain aspects of the Dependable > +Boot specification [2] are also implemented. The feature provides a > +mechanism to have multiple banks of updatable firmware images and for > +updating the firmware images on the non-booted bank. On a successful > +update, the platform boots from the updated bank on subsequent > +boot. The UEFI capsule-on-disk update feature is used for performing > +the actual updates of the updatable firmware images. > + > +The bookkeeping of the updatable images is done through a structure > +called metadata. Currently, the FWU metadata supports identification > +of images based on image GUIDs stored on a GPT partitioned storage > +media. There are plans to extend the metadata structure for non GPT > +partitioned devices as well. > + > +Accessing the FWU metadata is done through generic API's which are > +defined in a driver which complies with the U-Boot's driver model. A > +new uclass UCLASS_FWU_MDATA has been added for accessing the FWU > +metadata. Individual drivers can be added based on the type of storage > +media, and it's partitioning method. Details of the storage device > +containing the FWU metadata partitions are specified through a U-Boot > +specific device tree property `fwu-mdata-store`. Please refer to > +U-Boot `doc `__ > +for the device tree bindings. > + > +Enabling the FWU Multi Bank Update feature > +-- > + > +The feature can be enabled by specifying the following configs:: > + > +CONFIG_EFI_CAPSULE_ON_DISK=y > +CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT=y > +CONFIG_EFI_CAPSULE_FIRMWARE=y > +CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y > + > +CONFIG_FWU_MULTI_BANK_UPDATE=y > +CONFIG_CMD_FWU_METADATA=y > +CONFIG_DM_FWU_MDATA=y > s/CONFIG_DM_FWU_MDATA/CONFIG_FWU_MDATA
Re: [PATCH v10 02/15] FWU: Add FWU metadata structure and driver for accessing metadata
On Thu, 15 Sept 2022 at 03:15, Sughosh Ganu 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.
Re: [PATCH v8 02/13] FWU: Add FWU metadata structure and driver for accessing metadata
On Fri, Aug 19, 2022 at 10:25 AM Simon Glass wrote:
>
> Hi Jassi,
>
> On Fri, 19 Aug 2022 at 08:59, Jassi Brar wrote:
> >
> > Hi Simon,
> >
> > On Thu, Aug 18, 2022 at 12:50 PM Simon Glass wrote:
> >
> > > > > > +/**
> > > > > > + * 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;
> > > > > > +} __attribute__((__packed__));
> > > > >
> > > > > Why is this packed?
> > > >
> > > > This was based on a review comment from Masami [1], as he wanted to
> > > > use the same structure in the low level bootloader as well.
> > >
> > > It doesn't actually make any sense though. The packed struct is the same
> > > as
> > > the normal struct, so far as I can tell. What am I missing?
> > >
> > I think because we want the structure to be read/written onto
> > persistent storage, and possibly manipulated by entities other than
> > uboot.
>
> But specifically, how does __packed help here? What are the other
> entities doing that changes the format and what is __packed doing to
> update that? This is actually standard C code.
>
It is just to make sure there is no padding between structure members,
and the size of structure is exactly as in the specification.
Though in this case it likely won't make any difference because the
members are already 16, 4 and 4 bytes.
Cheers.
Re: [PATCH v8 02/13] FWU: Add FWU metadata structure and driver for accessing metadata
Hi Simon,
On Thu, Aug 18, 2022 at 12:50 PM Simon Glass wrote:
> > > > +/**
> > > > + * 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;
> > > > +} __attribute__((__packed__));
> > >
> > > Why is this packed?
> >
> > This was based on a review comment from Masami [1], as he wanted to
> > use the same structure in the low level bootloader as well.
>
> It doesn't actually make any sense though. The packed struct is the same as
> the normal struct, so far as I can tell. What am I missing?
>
I think because we want the structure to be read/written onto
persistent storage, and possibly manipulated by entities other than
uboot.
I totally agree with all the rest of your comments on the patchset.
cheers!
Re: [PATCH v8 06/13] FWU: Add helper functions for accessing FWU metadata
On Wed, 17 Aug 2022 at 07:44, Sughosh Ganu wrote:
.
> diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c
> new file mode 100644
> index 00..9808036eec
> --- /dev/null
> +++ b/lib/fwu_updates/fwu.c
> @@ -0,0 +1,22 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2022, Linaro Limited
> + */
> +
> +#include
> +#include
> +
> +__weak int fwu_plat_get_update_index(u32 *update_idx)
> +{
> + int ret;
> + u32 active_idx;
> +
> + ret = fwu_get_active_index(_idx);
> +
> + if (ret < 0)
> + return -1;
> +
> + *update_idx = active_idx ^= 0x1;
> +
It shoud be
*update_idx = (active_idx + 1) % CONFIG_FWU_NUM_BANKS;
cheers.
Re: [PATCH 0/5] FWU: Add support for mtd backed feature on DeveloperBox
Hi Sughosh, On Mon, 25 Jul 2022 at 02:18, Sughosh Ganu wrote: > > hi Jassi, > > On Fri, 22 Jul 2022 at 23:12, wrote: > > > > From: Jassi Brar > > > > The mtd and synquacer (developerbox) support was dropped from v6[1] > > This patchset re-introduces the support over last v7[2] submission of the > > patchset. > > > > All the comments on this code over v5 submission have been addressed. > > Moving forward > > a changelog will be maintained. > > > > [1] > > https://lore.kernel.org/all/20220704051658.1085442-1-sughosh.g...@linaro.org/ > > [2] > > https://lore.kernel.org/all/20220714183913.118505-1-sughosh.g...@linaro.org/ > > > > Jassi Brar (2): > > dt: fwu: developerbox: enable fwu banks and mdata regions > > fwu: DeveloperBox: add support for FWU > > > > Sughosh Ganu (3): > > dt/bindings: Add bindings for FWU Metadata mtd storage > > FWU: Add FWU metadata access driver for MTD storage regions > > FWU: mtd: Add helper functions for accessing FWU metadata > > All the above patches have been authored by Masami Hiramatsu. I think > you should change the author for these patches. > oops... I used the '-c' option to carry over the log from your patches and forgot to update the author. Will do. thanks. > Also, there were a few > more patches from Masami, like the documentation for the feature on > the Synquacer platform, and addition of the tool, mkfwumdata, for > generating the FWU metadata which I do not see in this series. Do you > plan to send those separately? > yes.
Re: [PATCH v5 19/23] FWU: synquacer: Add FWU Multi bank update support for DeveloperBox
On Fri, Jul 22, 2022 at 3:37 AM Ilias Apalodimas
wrote:
>
> Hi Jassi
>
> On Wed, 20 Jul 2022 at 17:30, Jassi Brar wrote:
> >
> > On Wed, Jul 20, 2022 at 2:54 AM Ilias Apalodimas
> > wrote:
> > >
> > > Hi Jassi,
> > >
> > > On Tue, 19 Jul 2022 at 18:27, Jassi Brar
> > > wrote:
> > > >
> > > > On Mon, 18 Jul 2022 at 16:00, Tom Rini wrote:
> > > > > On Mon, Jul 18, 2022 at 10:31:56AM -0500, Jassi Brar wrote:
> > > >
> > > > > > > > > >
> > > > > > > > > > > > +
> > > > > > > > > > > > +#define PLAT_METADATA_OFFSET 0x51
> > > > > > > > > > > > +#define PLAT_METADATA_SIZE (sizeof(struct
> > > > > > > > > > > > devbox_metadata))
> > > > > > > > > > > > +
> > > > > > > > > > > > +struct __packed devbox_metadata {
> > > > > > > > > > > > + u32 boot_index;
> > > > > > > > > > > > + u32 boot_count;
> > > > > > > > > > >
> > > > > > > > > > > There is the whole bootcount infrastructure for this. I
> > > > > > > > > > > think it would be much
> > > > > > > > > > > better to use that framework instead of creating parallel
> > > > > > > > > > > one.
> > > > > > > > > > >
> > > > > > > > > > Yes, this goes too.
> > > > > > > > >
> > > > > > > > > Is bootcount really suited for this case?
> > > > > > > > > AFAIK bootcount either requires device specific registers
> > > > > > > > > (which won't
> > > > > > > > > reset on reboots), or an environment you can write data to.
> > > > > > > > > But what if a user wants to disable writing the env variables
> > > > > > > > > and the
> > > > > > > > > device doesn't have a set of registers we can use?
> > > > > > > > >
> > > > > > > > Maybe it should be moved in 'struct fwu_mdata' ?
> > > > > > >
> > > > > > > I was mostly thinking on moving this count as another 'bootcount'
> > > > > > > method. So in case the user has disabled writing evn variables
> > > > > > > but he
> > > > > > > is booting with EFI he can use that.
> > > > > >
> > > > > > Sorry, not sure I understand IIUIC there has to be some
> > > > > > persistent storage.
> > > > >
> > > > > No, there just has to be "somewhere" to do the counting. We've got a
> > > > > DDR backed driver, for example. So yes, I think we should try and use
> > > > > the bootcount framework here.
> > > > >
> > > > OK, for platforms that can preserve ram across reboot, using
> > > > non-persistent storage can work.
> > > > My platform neither preserves ram, nor has any warmreset-proof
> > > > registers. So I have to choose between saving the bootcount in efi-env
> > > > or in vendor specific structure next to the metadata. I prefer
> > > > metadata because it is common to all stages of boot. Any corrections
> > > > to this approach?
> > >
> > > The metadata is defined by a spec and they don't have a field for
> > > bootcounting. Once Sughosh resends his patches he'll include a
> > > bootcount backend that reuses EFI variables. Can't we just use that?
> > >
> > Yes, I am aware metadata spec has no provision of vendor data. But
> > there is nothing illegal in appending vendor-data to metadata and that
> > is trivial to implement ... basically use sizeof(struct fwu_mdata) +
> > sizeof(struct sni_vendor_mdata) while read/write meta-data. That will
> > also be zero extra-overhead.
> >
> > fwu-mdata {
> >compatible = "u-boot,fwu-mdata-mtd";
> >fwu-mdata-store = <_flash>;
> >mdata-offsets = <0x50 0x53>;
> >vendor-data-size = <0x100>; // optional
> > };
> >
> > Sure we can use an efi variable, but I see more uses of vendor-data
> > :- shared among BL1/BL2/BL3x/OS so we can emulate reset-syndrome,
> > crash-logging, per-image bootcount etc when the h/w doesn't support
> > these features.
> >
> > Ofcourse, please feel free to implement efi-variables still.
>
> Ok, in that case, you'll still have to implement this as a 'special'
> bootcount method since the A/B updates code will use that API to
> get/set the values.
>
I thought the bootcount mechanism would always be platform specific?
But ok.
thanks.
Re: [PATCH v7 06/13] FWU: stm32mp1: Add helper functions for accessing FWU metadata
On Thu, 14 Jul 2022 at 13:40, Sughosh Ganu wrote: > > Add helper functions needed for accessing the FWU metadata which > contains information on the updatable images. These functions have > been added for the STM32MP157C-DK2 board which has the updatable > images on the uSD card, formatted as GPT partitions. > > Signed-off-by: Sughosh Ganu > Reviewed-by: Patrick Delaunay > --- > Changes since V6: None > > board/st/stm32mp1/stm32mp1.c | 40 > include/fwu.h| 3 ++ > lib/fwu_updates/Makefile | 6 +++ > lib/fwu_updates/fwu_gpt.c| 88 > 4 files changed, 137 insertions(+) > create mode 100644 lib/fwu_updates/Makefile > create mode 100644 lib/fwu_updates/fwu_gpt.c > fwu_gpt is to be used by all platforms that have the gpt-scheme, not just STM. So do you want to break this patch into two ? One generic that introduces the helper functions and another for stm using that api Also, you may want to specify GPL-2.0-or-later everywhere. thanks.
Re: [PATCH v5 19/23] FWU: synquacer: Add FWU Multi bank update support for DeveloperBox
On Wed, Jul 20, 2022 at 2:54 AM Ilias Apalodimas
wrote:
>
> Hi Jassi,
>
> On Tue, 19 Jul 2022 at 18:27, Jassi Brar wrote:
> >
> > On Mon, 18 Jul 2022 at 16:00, Tom Rini wrote:
> > > On Mon, Jul 18, 2022 at 10:31:56AM -0500, Jassi Brar wrote:
> >
> > > > > > > >
> > > > > > > > > > +
> > > > > > > > > > +#define PLAT_METADATA_OFFSET 0x51
> > > > > > > > > > +#define PLAT_METADATA_SIZE (sizeof(struct
> > > > > > > > > > devbox_metadata))
> > > > > > > > > > +
> > > > > > > > > > +struct __packed devbox_metadata {
> > > > > > > > > > + u32 boot_index;
> > > > > > > > > > + u32 boot_count;
> > > > > > > > >
> > > > > > > > > There is the whole bootcount infrastructure for this. I think
> > > > > > > > > it would be much
> > > > > > > > > better to use that framework instead of creating parallel one.
> > > > > > > > >
> > > > > > > > Yes, this goes too.
> > > > > > >
> > > > > > > Is bootcount really suited for this case?
> > > > > > > AFAIK bootcount either requires device specific registers (which
> > > > > > > won't
> > > > > > > reset on reboots), or an environment you can write data to.
> > > > > > > But what if a user wants to disable writing the env variables and
> > > > > > > the
> > > > > > > device doesn't have a set of registers we can use?
> > > > > > >
> > > > > > Maybe it should be moved in 'struct fwu_mdata' ?
> > > > >
> > > > > I was mostly thinking on moving this count as another 'bootcount'
> > > > > method. So in case the user has disabled writing evn variables but he
> > > > > is booting with EFI he can use that.
> > > >
> > > > Sorry, not sure I understand IIUIC there has to be some persistent
> > > > storage.
> > >
> > > No, there just has to be "somewhere" to do the counting. We've got a
> > > DDR backed driver, for example. So yes, I think we should try and use
> > > the bootcount framework here.
> > >
> > OK, for platforms that can preserve ram across reboot, using
> > non-persistent storage can work.
> > My platform neither preserves ram, nor has any warmreset-proof
> > registers. So I have to choose between saving the bootcount in efi-env
> > or in vendor specific structure next to the metadata. I prefer
> > metadata because it is common to all stages of boot. Any corrections
> > to this approach?
>
> The metadata is defined by a spec and they don't have a field for
> bootcounting. Once Sughosh resends his patches he'll include a
> bootcount backend that reuses EFI variables. Can't we just use that?
>
Yes, I am aware metadata spec has no provision of vendor data. But
there is nothing illegal in appending vendor-data to metadata and that
is trivial to implement ... basically use sizeof(struct fwu_mdata) +
sizeof(struct sni_vendor_mdata) while read/write meta-data. That will
also be zero extra-overhead.
fwu-mdata {
compatible = "u-boot,fwu-mdata-mtd";
fwu-mdata-store = <_flash>;
mdata-offsets = <0x50 0x53>;
vendor-data-size = <0x100>; // optional
};
Sure we can use an efi variable, but I see more uses of vendor-data
:- shared among BL1/BL2/BL3x/OS so we can emulate reset-syndrome,
crash-logging, per-image bootcount etc when the h/w doesn't support
these features.
Ofcourse, please feel free to implement efi-variables still.
thanks.
Re: [PATCH v5 20/23] FWU: synquacer: Generate dfu_alt_info from devicetree partition
On Tue, 19 Jul 2022 at 20:13, Takahiro Akashi
wrote:
>
> On Mon, Jul 18, 2022 at 09:49:56AM -0500, Jassi Brar wrote:
> > On Fri, 17 Jun 2022 at 09:02, Michal Simek wrote:
> > > On 6/9/22 14:30, Sughosh Ganu wrote:
> > > > From: Masami Hiramatsu
> > .
> >
> > > >
> > > > @@ -188,6 +178,9 @@ int board_late_init(void)
> > > > {
> > > > int ret;
> > > >
> > > > + /* Make mmc available for EFI */
> > > > + run_command("mmc dev 0", 0);
> > > > +
> > >
> > > What is this for?
> > >
> > > And I can't see any single note about in commit message.
> > >
> > For some reason, we get "No EFI system partition" during bootup and
> > the mmc does not show up in 'efidebug devices' unless we manually run
> > this (or mmc part) command.
>
> As far as UEFI is concerned, any U-Boot block device will be
> recognized as a UEFI disk(block_io) object *only* after device_probe()
> is called.
>
OK, thanks for the clarification. And I shouldn't feel too bad about
the hack then :)
thanks.
Re: [PATCH v5 19/23] FWU: synquacer: Add FWU Multi bank update support for DeveloperBox
On Mon, Jul 18, 2022 at 4:00 PM Tom Rini wrote:
> On Mon, Jul 18, 2022 at 10:31:56AM -0500, Jassi Brar wrote:
> > > > > > > > +
> > > > > > > > +#define PLAT_METADATA_OFFSET 0x51
> > > > > > > > +#define PLAT_METADATA_SIZE (sizeof(struct devbox_metadata))
> > > > > > > > +
> > > > > > > > +struct __packed devbox_metadata {
> > > > > > > > + u32 boot_index;
> > > > > > > > + u32 boot_count;
> > > > > > >
> > > > > > > There is the whole bootcount infrastructure for this. I think it
> > > > > > > would be much
> > > > > > > better to use that framework instead of creating parallel one.
> > > > > > >
> > > > > > Yes, this goes too.
> > > > >
> > > > > Is bootcount really suited for this case?
> > > > > AFAIK bootcount either requires device specific registers (which won't
> > > > > reset on reboots), or an environment you can write data to.
> > > > > But what if a user wants to disable writing the env variables and the
> > > > > device doesn't have a set of registers we can use?
> > > > >
> > > > Maybe it should be moved in 'struct fwu_mdata' ?
> > >
> > > I was mostly thinking on moving this count as another 'bootcount'
> > > method. So in case the user has disabled writing evn variables but he
> > > is booting with EFI he can use that.
> >
> > Sorry, not sure I understand IIUIC there has to be some persistent
> > storage.
>
> No, there just has to be "somewhere" to do the counting. We've got a
> DDR backed driver, for example. So yes, I think we should try and use
> the bootcount framework here.
>
OK, for platforms that can preserve ram across reboot, using
non-persistent storage can work.
My platform neither preserves ram, nor has any warmreset-proof
registers. So I have to choose between saving the bootcount in efi-env
or in vendor specific structure next to the metadata. I prefer
metadata because it is common to all stages of boot. Any corrections
to this approach?
Thanks
Re: [PATCH v5 19/23] FWU: synquacer: Add FWU Multi bank update support for DeveloperBox
On Mon, 18 Jul 2022 at 16:00, Tom Rini wrote:
> On Mon, Jul 18, 2022 at 10:31:56AM -0500, Jassi Brar wrote:
> > > > > >
> > > > > > > > +
> > > > > > > > +#define PLAT_METADATA_OFFSET 0x51
> > > > > > > > +#define PLAT_METADATA_SIZE (sizeof(struct devbox_metadata))
> > > > > > > > +
> > > > > > > > +struct __packed devbox_metadata {
> > > > > > > > + u32 boot_index;
> > > > > > > > + u32 boot_count;
> > > > > > >
> > > > > > > There is the whole bootcount infrastructure for this. I think it
> > > > > > > would be much
> > > > > > > better to use that framework instead of creating parallel one.
> > > > > > >
> > > > > > Yes, this goes too.
> > > > >
> > > > > Is bootcount really suited for this case?
> > > > > AFAIK bootcount either requires device specific registers (which won't
> > > > > reset on reboots), or an environment you can write data to.
> > > > > But what if a user wants to disable writing the env variables and the
> > > > > device doesn't have a set of registers we can use?
> > > > >
> > > > Maybe it should be moved in 'struct fwu_mdata' ?
> > >
> > > I was mostly thinking on moving this count as another 'bootcount'
> > > method. So in case the user has disabled writing evn variables but he
> > > is booting with EFI he can use that.
> >
> > Sorry, not sure I understand IIUIC there has to be some persistent
> > storage.
>
> No, there just has to be "somewhere" to do the counting. We've got a
> DDR backed driver, for example. So yes, I think we should try and use
> the bootcount framework here.
>
OK, for platforms that can preserve ram across reboot, using
non-persistent storage can work.
My platform neither preserves ram, nor has any warmreset-proof
registers. So I have to choose between saving the bootcount in efi-env
or in vendor specific structure next to the metadata. I prefer
metadata because it is common to all stages of boot. Any corrections
to this approach?
Thanks
Re: [PATCH v5 19/23] FWU: synquacer: Add FWU Multi bank update support for DeveloperBox
On Mon, 18 Jul 2022 at 10:31, Jassi Brar wrote:
>
> Hi Ilias,
>
> On Mon, 18 Jul 2022 at 10:16, Ilias Apalodimas
> wrote:
> >
> > Hi Jassi
> >
> > On Mon, 18 Jul 2022 at 18:08, Jassi Brar wrote:
> > >
> > > On Mon, 18 Jul 2022 at 09:47, Ilias Apalodimas
> > > wrote:
> > > >
> > > > Hi all,
> > > >
> > > > On Mon, 18 Jul 2022 at 17:43, Jassi Brar
> > > > wrote:
> > > > >
> > > > > On Mon, 20 Jun 2022 at 03:23, Michal Simek wrote:
> > > > > > On 6/9/22 14:30, Sughosh Ganu wrote:
> > > > > > > From: Masami Hiramatsu
> > > > > > >
> > > > >
> > > > >
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int plat_sf_get_flash(struct spi_flash **flash)
> > > > > > > +{
> > > > > > > + int ret = 0;
> > > > > > > +
> > > > > > > + if (!plat_spi_flash)
> > > > > > > + ret = __plat_sf_get_flash();
> > > > > > > +
> > > > > > > + *flash = plat_spi_flash;
> > > > > > > +
> > > > > > > + return ret;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int sf_load_data(u32 offs, u32 size, void **data)
> > > > > > > +{
> > > > > > > + struct spi_flash *flash;
> > > > > > > + int ret;
> > > > > > > +
> > > > > > > + ret = plat_sf_get_flash();
> > > > > > > + if (ret < 0)
> > > > > > > + return ret;
> > > > > > > +
> > > > > > > + *data = memalign(ARCH_DMA_MINALIGN, size);
> > > > > > > + if (!*data)
> > > > > > > + return -ENOMEM;
> > > > > > > +
> > > > > > > + ret = spi_flash_read(flash, offs, size, *data);
> > > > > > > + if (ret < 0) {
> > > > > > > + free(*data);
> > > > > > > + *data = NULL;
> > > > > > > + }
> > > > > > > +
> > > > > > > + return ret;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int sf_save_data(u32 offs, u32 size, void *data)
> > > > > > > +{
> > > > > > > + struct spi_flash *flash;
> > > > > > > + u32 sect_size, nsect;
> > > > > > > + void *buf;
> > > > > > > + int ret;
> > > > > > > +
> > > > > > > + ret = plat_sf_get_flash();
> > > > > > > + if (ret < 0)
> > > > > > > + return ret;
> > > > > > > +
> > > > > > > + sect_size = flash->mtd.erasesize;
> > > > > > > + nsect = DIV_ROUND_UP(size, sect_size);
> > > > > > > + ret = spi_flash_erase(flash, offs, nsect * sect_size);
> > > > > >
> > > > > > What it is interesting here that framework itself is using mtd
> > > > > > infrastructure
> > > > > > but this platform driver is calling spi functions directly.
> > > > > > It looks a little bit nonstandard way. What's the reason for it?
> > > > > >
> > > > > Yup, this whole sf shebang is unnecessary, and removed for next
> > > > > revision.
> > > > >
> > > > > > > +
> > > > > > > +#define PLAT_METADATA_OFFSET 0x51
> > > > > > > +#define PLAT_METADATA_SIZE (sizeof(struct devbox_metadata))
> > > > > > > +
> > > > > > > +struct __packed devbox_metadata {
> > > > > > > + u32 boot_index;
> > > > > > > + u32 boot_count;
> > > > > >
> > > > > > There is the whole bootcount infrastructure for this. I think it
> > > > > > would be much
> > > > > > better to use that framework instead of creating parallel one.
> > > > > >
> > > > > Yes, this goes too.
> > > >
> > > > Is bootcount really suited for this case?
> > > > AFAIK bootcount either requires device specific registers (which won't
> > > > reset on reboots), or an environment you can write data to.
> > > > But what if a user wants to disable writing the env variables and the
> > > > device doesn't have a set of registers we can use?
> > > >
> > > Maybe it should be moved in 'struct fwu_mdata' ?
> >
> > I was mostly thinking on moving this count as another 'bootcount'
> > method. So in case the user has disabled writing evn variables but he
> > is booting with EFI he can use that.
> >
> Sorry, not sure I understand IIUIC there has to be some persistent
> storage.
>
> Of the three options - registers, efi-env and mdata, I think the last
> one is more robust.
> For ex, if BL33 isn't reached after an update. We want BL2 (which may
> not have access to efi variables)
> to be able to revert the active index.
>
and which requires a bootcount for each stage. hmm...
probably I am overlooking something.
Re: [PATCH v5 19/23] FWU: synquacer: Add FWU Multi bank update support for DeveloperBox
Hi Ilias,
On Mon, 18 Jul 2022 at 10:16, Ilias Apalodimas
wrote:
>
> Hi Jassi
>
> On Mon, 18 Jul 2022 at 18:08, Jassi Brar wrote:
> >
> > On Mon, 18 Jul 2022 at 09:47, Ilias Apalodimas
> > wrote:
> > >
> > > Hi all,
> > >
> > > On Mon, 18 Jul 2022 at 17:43, Jassi Brar
> > > wrote:
> > > >
> > > > On Mon, 20 Jun 2022 at 03:23, Michal Simek wrote:
> > > > > On 6/9/22 14:30, Sughosh Ganu wrote:
> > > > > > From: Masami Hiramatsu
> > > > > >
> > > >
> > > >
> > > > > > +}
> > > > > > +
> > > > > > +static int plat_sf_get_flash(struct spi_flash **flash)
> > > > > > +{
> > > > > > + int ret = 0;
> > > > > > +
> > > > > > + if (!plat_spi_flash)
> > > > > > + ret = __plat_sf_get_flash();
> > > > > > +
> > > > > > + *flash = plat_spi_flash;
> > > > > > +
> > > > > > + return ret;
> > > > > > +}
> > > > > > +
> > > > > > +static int sf_load_data(u32 offs, u32 size, void **data)
> > > > > > +{
> > > > > > + struct spi_flash *flash;
> > > > > > + int ret;
> > > > > > +
> > > > > > + ret = plat_sf_get_flash();
> > > > > > + if (ret < 0)
> > > > > > + return ret;
> > > > > > +
> > > > > > + *data = memalign(ARCH_DMA_MINALIGN, size);
> > > > > > + if (!*data)
> > > > > > + return -ENOMEM;
> > > > > > +
> > > > > > + ret = spi_flash_read(flash, offs, size, *data);
> > > > > > + if (ret < 0) {
> > > > > > + free(*data);
> > > > > > + *data = NULL;
> > > > > > + }
> > > > > > +
> > > > > > + return ret;
> > > > > > +}
> > > > > > +
> > > > > > +static int sf_save_data(u32 offs, u32 size, void *data)
> > > > > > +{
> > > > > > + struct spi_flash *flash;
> > > > > > + u32 sect_size, nsect;
> > > > > > + void *buf;
> > > > > > + int ret;
> > > > > > +
> > > > > > + ret = plat_sf_get_flash();
> > > > > > + if (ret < 0)
> > > > > > + return ret;
> > > > > > +
> > > > > > + sect_size = flash->mtd.erasesize;
> > > > > > + nsect = DIV_ROUND_UP(size, sect_size);
> > > > > > + ret = spi_flash_erase(flash, offs, nsect * sect_size);
> > > > >
> > > > > What it is interesting here that framework itself is using mtd
> > > > > infrastructure
> > > > > but this platform driver is calling spi functions directly.
> > > > > It looks a little bit nonstandard way. What's the reason for it?
> > > > >
> > > > Yup, this whole sf shebang is unnecessary, and removed for next
> > > > revision.
> > > >
> > > > > > +
> > > > > > +#define PLAT_METADATA_OFFSET 0x51
> > > > > > +#define PLAT_METADATA_SIZE (sizeof(struct devbox_metadata))
> > > > > > +
> > > > > > +struct __packed devbox_metadata {
> > > > > > + u32 boot_index;
> > > > > > + u32 boot_count;
> > > > >
> > > > > There is the whole bootcount infrastructure for this. I think it
> > > > > would be much
> > > > > better to use that framework instead of creating parallel one.
> > > > >
> > > > Yes, this goes too.
> > >
> > > Is bootcount really suited for this case?
> > > AFAIK bootcount either requires device specific registers (which won't
> > > reset on reboots), or an environment you can write data to.
> > > But what if a user wants to disable writing the env variables and the
> > > device doesn't have a set of registers we can use?
> > >
> > Maybe it should be moved in 'struct fwu_mdata' ?
>
> I was mostly thinking on moving this count as another 'bootcount'
> method. So in case the user has disabled writing evn variables but he
> is booting with EFI he can use that.
>
Sorry, not sure I understand IIUIC there has to be some persistent storage.
Of the three options - registers, efi-env and mdata, I think the last
one is more robust.
For ex, if BL33 isn't reached after an update. We want BL2 (which may
not have access to efi variables)
to be able to revert the active index.
thanks.
Re: [PATCH v6 00/13] FWU: Add FWU Multi Bank Update feature support
On Mon, 4 Jul 2022 at 00:17, Sughosh Ganu wrote: > > The earlier patchset contained patches for both the DK2 and the > Synquacer platforms. The handling of review comments for the Synquacer > platform is to be taken up by a different engineer, and > That would be me. > has not been > done yet. After discussion with Tom Rini and Heinrich, it was decided > to send the patches for the DK2 platform separately for review. > I see the whole not-gpt/mtd support has been dissected out of this revision. I need to reintroduce those patches and then Synquacer support on top of that. Is anybody already working on the first part? thanks
Re: [PATCH v5 19/23] FWU: synquacer: Add FWU Multi bank update support for DeveloperBox
On Mon, 18 Jul 2022 at 09:47, Ilias Apalodimas
wrote:
>
> Hi all,
>
> On Mon, 18 Jul 2022 at 17:43, Jassi Brar wrote:
> >
> > On Mon, 20 Jun 2022 at 03:23, Michal Simek wrote:
> > > On 6/9/22 14:30, Sughosh Ganu wrote:
> > > > From: Masami Hiramatsu
> > > >
> >
> >
> > > > +}
> > > > +
> > > > +static int plat_sf_get_flash(struct spi_flash **flash)
> > > > +{
> > > > + int ret = 0;
> > > > +
> > > > + if (!plat_spi_flash)
> > > > + ret = __plat_sf_get_flash();
> > > > +
> > > > + *flash = plat_spi_flash;
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int sf_load_data(u32 offs, u32 size, void **data)
> > > > +{
> > > > + struct spi_flash *flash;
> > > > + int ret;
> > > > +
> > > > + ret = plat_sf_get_flash();
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + *data = memalign(ARCH_DMA_MINALIGN, size);
> > > > + if (!*data)
> > > > + return -ENOMEM;
> > > > +
> > > > + ret = spi_flash_read(flash, offs, size, *data);
> > > > + if (ret < 0) {
> > > > + free(*data);
> > > > + *data = NULL;
> > > > + }
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int sf_save_data(u32 offs, u32 size, void *data)
> > > > +{
> > > > + struct spi_flash *flash;
> > > > + u32 sect_size, nsect;
> > > > + void *buf;
> > > > + int ret;
> > > > +
> > > > + ret = plat_sf_get_flash();
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + sect_size = flash->mtd.erasesize;
> > > > + nsect = DIV_ROUND_UP(size, sect_size);
> > > > + ret = spi_flash_erase(flash, offs, nsect * sect_size);
> > >
> > > What it is interesting here that framework itself is using mtd
> > > infrastructure
> > > but this platform driver is calling spi functions directly.
> > > It looks a little bit nonstandard way. What's the reason for it?
> > >
> > Yup, this whole sf shebang is unnecessary, and removed for next revision.
> >
> > > > +
> > > > +#define PLAT_METADATA_OFFSET 0x51
> > > > +#define PLAT_METADATA_SIZE (sizeof(struct devbox_metadata))
> > > > +
> > > > +struct __packed devbox_metadata {
> > > > + u32 boot_index;
> > > > + u32 boot_count;
> > >
> > > There is the whole bootcount infrastructure for this. I think it would be
> > > much
> > > better to use that framework instead of creating parallel one.
> > >
> > Yes, this goes too.
>
> Is bootcount really suited for this case?
> AFAIK bootcount either requires device specific registers (which won't
> reset on reboots), or an environment you can write data to.
> But what if a user wants to disable writing the env variables and the
> device doesn't have a set of registers we can use?
>
Maybe it should be moved in 'struct fwu_mdata' ?
thnx
Re: [PATCH v5 20/23] FWU: synquacer: Generate dfu_alt_info from devicetree partition
On Fri, 17 Jun 2022 at 09:02, Michal Simek wrote:
> On 6/9/22 14:30, Sughosh Ganu wrote:
> > From: Masami Hiramatsu
.
> >
> > @@ -188,6 +178,9 @@ int board_late_init(void)
> > {
> > int ret;
> >
> > + /* Make mmc available for EFI */
> > + run_command("mmc dev 0", 0);
> > +
>
> What is this for?
>
> And I can't see any single note about in commit message.
>
For some reason, we get "No EFI system partition" during bootup and
the mmc does not show up in 'efidebug devices' unless we manually run
this (or mmc part) command.
Though not elegant, I found similar being done by some other
platforms grep run_command -rw board/*
I am happy to learn the proper way of doing it.
Thanks.
Re: [PATCH v5 19/23] FWU: synquacer: Add FWU Multi bank update support for DeveloperBox
On Mon, 20 Jun 2022 at 03:23, Michal Simek wrote:
> On 6/9/22 14:30, Sughosh Ganu wrote:
> > From: Masami Hiramatsu
> >
> > +}
> > +
> > +static int plat_sf_get_flash(struct spi_flash **flash)
> > +{
> > + int ret = 0;
> > +
> > + if (!plat_spi_flash)
> > + ret = __plat_sf_get_flash();
> > +
> > + *flash = plat_spi_flash;
> > +
> > + return ret;
> > +}
> > +
> > +static int sf_load_data(u32 offs, u32 size, void **data)
> > +{
> > + struct spi_flash *flash;
> > + int ret;
> > +
> > + ret = plat_sf_get_flash();
> > + if (ret < 0)
> > + return ret;
> > +
> > + *data = memalign(ARCH_DMA_MINALIGN, size);
> > + if (!*data)
> > + return -ENOMEM;
> > +
> > + ret = spi_flash_read(flash, offs, size, *data);
> > + if (ret < 0) {
> > + free(*data);
> > + *data = NULL;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int sf_save_data(u32 offs, u32 size, void *data)
> > +{
> > + struct spi_flash *flash;
> > + u32 sect_size, nsect;
> > + void *buf;
> > + int ret;
> > +
> > + ret = plat_sf_get_flash();
> > + if (ret < 0)
> > + return ret;
> > +
> > + sect_size = flash->mtd.erasesize;
> > + nsect = DIV_ROUND_UP(size, sect_size);
> > + ret = spi_flash_erase(flash, offs, nsect * sect_size);
>
> What it is interesting here that framework itself is using mtd infrastructure
> but this platform driver is calling spi functions directly.
> It looks a little bit nonstandard way. What's the reason for it?
>
Yup, this whole sf shebang is unnecessary, and removed for next revision.
> > +
> > +#define PLAT_METADATA_OFFSET 0x51
> > +#define PLAT_METADATA_SIZE (sizeof(struct devbox_metadata))
> > +
> > +struct __packed devbox_metadata {
> > + u32 boot_index;
> > + u32 boot_count;
>
> There is the whole bootcount infrastructure for this. I think it would be much
> better to use that framework instead of creating parallel one.
>
Yes, this goes too.
Thanks.
Re: [PATCH v7 01/13] dt/bindings: Add bindings for FWU Metadata storage device
On Thu, 14 Jul 2022 at 13:39, Sughosh Ganu wrote: > > Add bindings needed for accessing the FWU metadata partitions. These > include the compatible string which point to the access method and the > actual device which stores the FWU metadata. > > The current patch adds basic bindings needed for accessing the > metadata structure on GPT partitioned block devices. > > Signed-off-by: Sughosh Ganu > --- > Changes since V6: None > > .../firmware/fwu-mdata.yaml | 32 +++ > 1 file changed, 32 insertions(+) > create mode 100644 doc/device-tree-bindings/firmware/fwu-mdata.yaml > > diff --git a/doc/device-tree-bindings/firmware/fwu-mdata.yaml > b/doc/device-tree-bindings/firmware/fwu-mdata.yaml > new file mode 100644 > index 00..97d30bd1c1 > --- /dev/null > +++ b/doc/device-tree-bindings/firmware/fwu-mdata.yaml > I think we want to call it fwu-mdata-gpt.yaml ? thanks.
Re: [PATCH 4/4] spi: synquacer: simplify tx completion checking
On Tue, 17 May 2022 at 03:41, Masahisa Kojima wrote: > > There is a TX-FIFO and Shift Register empty(TFES) status > bit in spi controller. This commit checks the TFES bit > to wait the TX transfer completes. > > Signed-off-by: Masahisa Kojima > Signed-off-by: Satoru Okamoto > --- Acked-by: Jassi Brar
