Hi Sughosh,

• */

+#define LOG_CATEGORY UCLASS_FWU_MDATA

[...]

+/**

• • fwu_verify_mdata() - Verify the FWU metadata
• • @mdata: FWU metadata structure
• • @pri_part: FWU metadata partition is primary or secondary
• • Verify the FWU metadata by computing the CRC32 for the metadata
• • structure and comparing it against the CRC32 value stored as part
• • of the structure.

+/**

• • fwu_update_active_index() - Update active_index from the FWU metadata
• • @active_idx: active_index value to be updated

+int fwu_update_active_index(u32 active_idx) +{

•   int ret;
  

•   struct fwu_mdata *mdata = NULL;
  

•   if (active_idx > CONFIG_FWU_NUM_BANKS - 1) {
  

•           log_err("Active index value to be updated is incorrect\n");
  

Nit but "Invalid active index value" is makes more sense

•           return -1;
  

•   }
  

[...]

+}

+/**

• • fwu_get_image_alt_num() - Get the dfu alt number to be used for capsule update
• • @image_type_id: pointer to the image guid as passed in the capsule
• • @update_bank: Bank to which the update is to be made
• • @alt_num: The alt_num for the image
• • Based on the guid value passed in the capsule, along with the bank to which the
• • image needs to be updated, get the dfu alt number which will be used for the
• • capsule update
• • Return: 0 if OK, -ve on error
• */

+int fwu_get_image_alt_num(efi_guid_t *image_type_id, u32 update_bank,

•                     int *alt_num)
  

+{

•   int ret, i;
  

•   efi_guid_t *image_guid;
  

•   struct udevice *dev = NULL;
  

•   struct fwu_mdata *mdata = NULL;
  

•   struct fwu_image_entry *img_entry;
  

•   const struct fwu_mdata_ops *ops = NULL;
  

•   struct fwu_image_bank_info *img_bank_info;
  

•   ret = fwu_get_dev_ops(&dev, &ops);
  

•   if (ret)
  

•           return ret;
  

•   ret = fwu_get_mdata(&mdata);
  

•   if (ret) {
  

•           log_err("Unable to get valid FWU metadata\n");
  

•           goto out;
  

•   }
  

•   /*
  

•    * The FWU metadata has been read. Now get the image_uuid for the
  

•    * image with the update_bank.
  

•    */
  

•   for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) {
  

•           if (!guidcmp(image_type_id,
  

•                        &mdata->img_entry[i].image_type_uuid)) {
  

•                   img_entry = &mdata->img_entry[i];
  

•                   img_bank_info = &img_entry->img_bank_info[update_bank];
  

•                   image_guid = &img_bank_info->image_uuid;
  

•                   ret = fwu_plat_get_alt_num(dev, image_guid, alt_num);
  

•                   break;
  

•           }
  

•   }
  

I think if you set ret = -EINVAL on top of the for loop you can mostly get rid of the code below. ret = -EINVAL for () { ..... ret = fwu_plat_get_alt_num(dev, image_guid, alt_num); if (ret) log_err("alt_num not found for partition with GUID %pUs\n", image_guid); goto out; //on success instead of break } log_err("Partition with the image type %pUs not found\n", image_type_id);

out: free(mdata);

return ret;

Isn't that more readable?

•   if (i == CONFIG_FWU_NUM_IMAGES_PER_BANK) {
  

•           log_err("Partition with the image type %pUs not found\n",
  

•                   image_type_id);
  

•           ret = -EINVAL;
  

•           goto out;
  

•   }
  

•   if (!ret) {
  

•           log_debug("alt_num %d for partition %pUs\n",
  

•                     *alt_num, image_guid);
  

•   } else {
  

•           log_err("alt_num not found for partition with GUID %pUs\n",
  

•                   image_guid);
  

•           ret = -EINVAL;
  

•   }
  

+out:

•   free(mdata);
  

•   return ret;
  

+}

+/**

[...]

u32 bank, u8 action)

+{

•   int ret, i;
  

•   u32 nimages;
  

Get rid of nimages when they are only used as CONFIG_FWU_NUM_IMAGES_PER_BANK (there's more than one)

[...]

diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index a432e43871..598a8c10a0 100644 @@ -0,0 +1,67 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/*

• • Copyright (c) 2022, Linaro Limited
• */

+#if !defined _FWU_MDATA_H_ +#define _FWU_MDATA_H_

+#include <efi.h>

+/**

• • struct fwu_image_bank_info - firmware image information
• • @image_uuid: Guid value of the image in this bank
• • @accepted: Acceptance status of the image
• • @reserved: Reserved
• • The structure contains image specific fields which are
• • used to identify the image and to specify the image's
• • acceptance status
• */

+struct fwu_image_bank_info {

•   efi_guid_t  image_uuid;
  

•   uint32_t accepted;
  

•   uint32_t reserved;
  

+} __attribute__((__packed__));

The spec doesn't require those to be packed. If there's a reason we should have it in comments, otherwise just drop the packed attribute

+/**

• • struct fwu_image_entry - information for a particular type of image
• • @image_type_uuid: Guid value for identifying the image type
• • @location_uuid: Guid of the storage volume where the image is located
• • @img_bank_info: Array containing properties of images
• • This structure contains information on various types of updatable
• • firmware images. Each image type then contains an array of image
• • information per bank.
• */

+struct fwu_image_entry {

•   efi_guid_t image_type_uuid;
  

•   efi_guid_t location_uuid;
  

•   struct fwu_image_bank_info img_bank_info[CONFIG_FWU_NUM_BANKS];
  

+} __attribute__((__packed__));

+/**

• • struct fwu_mdata - FWU metadata structure for multi-bank updates
• • @crc32: crc32 value for the FWU metadata
• • @version: FWU metadata version
• • @active_index: Index of the bank currently used for booting images
• • @previous_active_inde: Index of the bank used before the current bank

• •                    being used for booting
    

• • @img_entry: Array of information on various firmware images that can

• •         be updated
    

• • This structure is used to store all the needed information for performing
• • multi bank updates on the platform. This contains info on the bank being
• • used to boot along with the information needed for identification of
• • individual images
• */

+struct fwu_mdata {

•   uint32_t crc32;
  

•   uint32_t version;
  

•   uint32_t active_index;
  

•   uint32_t previous_active_index;
  

•   struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK];
  

+} __attribute__((__packed__));

+#endif /* _FWU_MDATA_H_ */

2.34.1

Thanks! /Ilias

hi Ilias,

On Fri, 15 Jul 2022 at 22:11, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

Hi Sughosh,

On Thu, 14 Jul 2022 at 21:40, Sughosh Ganu sughosh.ganu@linaro.org wrote:

...

Add support for setting OEM flags in the capsule header. As per the UEFI specification, bits 0-15 of the flags member of the capsule header can be defined per capsule GUID.

The oemflags will be used for the FWU Multi Bank update feature, as specified by the Dependable Boot specification[1]. Bit 15 of the flags member will be used to determine if the acceptance/rejection of the updated images is to be done by the firmware or an external component like the OS.

Have we documented bit15 in the documentation? If not please add it.

Yes, the use of bit15 has been added in the documentation for this feature. I will incorporate all your review comments, and in case I hit any issues while incorporating any review comment, I will respond to that. Thanks for the review.

-sughosh

...

[1] - https://git.codelinaro.org/linaro/dependable-boot/mbfw/uploads/6f7ddfe3be24e...

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

Changes since V6: None

doc/mkeficapsule.1 | 4 ++++ tools/mkeficapsule.c | 17 ++++++++++++++--- 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/doc/mkeficapsule.1 b/doc/mkeficapsule.1 index 77ca061efd..6fb2dd0810 100644 --- a/doc/mkeficapsule.1 +++ b/doc/mkeficapsule.1 @@ -72,6 +72,10 @@ Generate a firmware acceptance empty capsule .BI "-R\fR,\fB --fw-revert " Generate a firmware revert empty capsule

+.TP +.BI "-o\fR,\fB --capoemflag " +Capsule OEM flag, value between 0x0000 to 0xffff

.TP .BR -h ", " --help Print a help message diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c index 244c80e1f7..237c1218fd 100644 --- a/tools/mkeficapsule.c +++ b/tools/mkeficapsule.c @@ -29,7 +29,7 @@ static const char *tool_name = "mkeficapsule"; efi_guid_t efi_guid_fm_capsule = EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;

-static const char *opts_short = "g:i:I:v:p:c:m:dhAR"; +static const char *opts_short = "g:i:I:v:p:c:m:o:dhAR";

enum { CAPSULE_NORMAL_BLOB = 0, @@ -47,6 +47,7 @@ static struct option options[] = { {"dump-sig", no_argument, NULL, 'd'}, {"fw-accept", no_argument, NULL, 'A'}, {"fw-revert", no_argument, NULL, 'R'},

•   {"capoemflag", required_argument, NULL, 'o'},
    {"help", no_argument, NULL, 'h'},
    {NULL, 0, NULL, 0},
  

}; @@ -65,6 +66,7 @@ static void print_usage(void) "\t-d, --dump_sig dump signature (*.p7)\n" "\t-A, --fw-accept firmware accept capsule, requires GUID, no image blob\n" "\t-R, --fw-revert firmware revert capsule, takes no GUID, no image blob\n"

•           "\t-o, --capoemflag Capsule OEM Flag, an integer between 0x0000 and 0xffff\n"
            "\t-h, --help                  print a help message\n",
            tool_name);
  

} @@ -387,6 +389,7 @@ static void free_sig_data(struct auth_context *ctx)

• @mcount: Monotonic count in authentication information
• @private_file: Path to a private key file
• @cert_file: Path to a certificate file

• • @oemflags: Capsule OEM Flags, bits 0-15
  • This function actually does the job of creating an uefi capsule file.
  • All the arguments must be supplied.

@@ -399,7 +402,8 @@ static void free_sig_data(struct auth_context *ctx) */ static int create_fwbin(char *path, char *bin, efi_guid_t *guid, unsigned long index, unsigned long instance,

•                   uint64_t mcount, char *privkey_file, char *cert_file)
  

•                   uint64_t mcount, char *privkey_file, char *cert_file,
  

•                   uint16_t oemflags)
  

{ struct efi_capsule_header header; struct efi_firmware_management_capsule_header capsule; @@ -464,6 +468,8 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid, header.header_size = sizeof(header); /* TODO: The current implementation ignores flags */ header.flags = CAPSULE_FLAGS_PERSIST_ACROSS_RESET;

•   if (oemflags)
  

•           header.flags |= oemflags;
    header.capsule_image_size = sizeof(header)
                                    + sizeof(capsule) + sizeof(uint64_t)
                                    + sizeof(image)
  

@@ -635,6 +641,7 @@ int main(int argc, char **argv) unsigned char uuid_buf[16]; unsigned long index, instance; uint64_t mcount;

•   uint16_t oemflags;
    char *privkey_file, *cert_file;
    int c, idx;
  
  

@@ -646,6 +653,7 @@ int main(int argc, char **argv) cert_file = NULL; dump_sig = 0; capsule_type = CAPSULE_NORMAL_BLOB;

•   oemflags = 0;
    for (;;) {
            c = getopt_long(argc, argv, opts_short, options, &idx);
            if (c == -1)
  

@@ -699,6 +707,9 @@ int main(int argc, char **argv) case 'R': capsule_type |= CAPSULE_REVERT; break;

•           case 'o':
  

•                   oemflags = strtoul(optarg, NULL, 0);
  

•                   break;
            default:
                    print_usage();
                    exit(EXIT_SUCCESS);
  

@@ -732,7 +743,7 @@ int main(int argc, char **argv) } } else if (create_fwbin(argv[argc - 1], argv[argc - 2], guid, index, instance, mcount, privkey_file,

•                            cert_file) < 0) {
  

•                            cert_file, oemflags) < 0) {
            fprintf(stderr, "Creating firmware capsule failed\n");
            exit(EXIT_FAILURE);
    }
  

-- 2.34.1

Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

On 7/14/22 20:39, Sughosh Ganu wrote:

Add documentattion for the FWU Multi Bank Update feature. The document describes the steps needed for setting up the platform for the feature, as well as steps for enabling the feature on the platform.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

Changes since V6: None

doc/develop/uefi/fwu_updates.rst | 156 +++++++++++++++++++++++++++++++ doc/develop/uefi/index.rst | 1 + doc/develop/uefi/uefi.rst | 2 + 3 files changed, 159 insertions(+) create mode 100644 doc/develop/uefi/fwu_updates.rst

diff --git a/doc/develop/uefi/fwu_updates.rst b/doc/develop/uefi/fwu_updates.rst new file mode 100644 index 0000000000..57d1b5b703 --- /dev/null +++ b/doc/develop/uefi/fwu_updates.rst @@ -0,0 +1,156 @@ +.. 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

Thanks a lot for the extensive documentation.

%s/[1]/ [1]/

+Boot specification[2] are also implemented. The feature provides a

ditto

+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 <doc/device-tree-bindings/firmware/fwu-mdata.txt>`__ 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
• CONFIG_FWU_MDATA_GPT_BLK=y
• CONFIG_FWU_NUM_BANKS=<val>
• CONFIG_FWU_NUM_IMAGES_PER_BANK=<val>

+in the .config file

+The first group of configs enable the UEFI capsule-on-disk update

%s/configs/configuration settings/

+functionality. The second group of configs enable the FWU Multi Bank +Update functionality. Please refer to the section +:ref:`uefi_capsule_update_ref` for more details on generation of the +UEFI capsule.

+Setting up the device for GPT partitioned storage +-------------------------------------------------

+Before enabling the functionality in U-Boot, certain changes are +required to be done on the storage device. Assuming a GPT partitioned

A GPT partitioned storage device is required.

+storage device, the storage media needs to be partitioned with the +correct number of partitions, given the number of banks and number of +images per bank that the platform is going to support. Each updatable +firmware image will be stored on an separate partition. In addition, +the two copies of the FWU metadata will be stored on two separate +partitions.

+As an example, a platform supporting two banks with each bank +containing three images would need to have 2 * 3 = 6 parititions plus

%s/parititions/partitions/

Please, provide an example:

* meta data 1 * U-Boot 1 * ESP 1 * Operating system image 1 * meta data 2 * U-Boot 2 * ESP 2 * Operating system image 2

+the two metadata partitions, or 8 partitions. In addition the storage +media can have additional partitions of non-updatable images, like the +EFI System Partition(ESP), a partition for the root file system etc.

Couldn't the ESP and the root file system also be updated by FWU for instance to switch to a new GRUB version or a new kernel?

+When generating the partitions, a few aspects need to be taken care +of. Each GPT partition entry in the GPT header has two GUIDs::

• *PartitionTypeGUID*
• *UniquePartitionGUID*

Please, use a bullet list here.

+The PartitionTypeGUID value should correspond to the *image_type_uuid*

You could use double backquotes to show variable names as code.

E.g. this is a ``variable``.

+field of the FWU metadata. This field is used to identify a given type +of updatable firmware image, e.g. u-boot, op-tee, FIP etc. This GUID

%s/u-boot/U-Boot/ $s/op-tee/OP-TEE/

Best regards

Heinrich

+should also be used for specifying the `--guid` parameter when +generating the capsule.

+The UniquePartitionGUID value should correspond to the *image_uuid* +field in the FWU metadata. This GUID is used to identify images of a +given image type in different banks.

+Similarly, the FWU specifications defines the GUID value to be used +for the metadata partitions. This would be the PartitionTypeGUID for +the metadata partitions.

+When generating the metadata, the *image_type_uuid* and the +*image_uuid* values should match the *PartitionTypeGUID* and the +*UniquePartitionGUID* values respectively.

+Performing the Update +---------------------

+Once the storage media has been partitioned and populated with the +metadata partitions, the UEFI capsule-on-disk update functionality can +be used for performing the update. Refer to the section +:ref:`uefi_capsule_update_ref` for details on how the update can be +invoked.

+On a successful update, the FWU metadata gets updated to reflect the +bank from which the platform would be booting on subsequent boot.

+Based on the value of bit15 of the Flags member of the capsule header, +the updated images would either be accepted by the u-boot's UEFI +implementation, or by the Operating System. If the Operating System is +accepting the firmware images, it does so by generating an empty +*accept* capsule. The Operating System can also reject the updated +firmware by generating a *revert* capsule. The empty capsule can be +applied by using the exact same procedure used for performing the +capsule-on-disk update.

+The task of accepting the different firmware images, post an update +may be done by multiple, separate components in the Operating +System. To help identify the firmware image that is being accepted, +the accept capsule passes the image GUID of the firmware image being +accepted. The relevant code in u-boot then sets the Accept bit of the +corresponding firmware image for which the accept capsule was +found. Only when all the firmware components in a bank have been +accepted does the platform transition to the regular state from trial +state.

+The revert capsule on the other hand does not pass any image GUID, +since reverting any image of the bank has the same result of the +platform booting from the other bank on subsequent boot.

+Generating an empty capsule +---------------------------

+The empty capsule can be generated using the mkeficapsule utility. To +build the tool, enable::

• CONFIG_TOOLS_MKEFICAPSULE=y

+Run the following commands to generate the accept/revert capsules::

+.. code-block:: bash

• $ ./tools/mkeficapsule \

•  [--fw-accept --guid <image guid>] | \
  

•  [--fw-revert] \
  

•  <capsule_file_name>
  

+Links +-----

+* [1] https://developer.arm.com/documentation/den0118/a/ - FWU Specification +* [2] https://git.codelinaro.org/linaro/dependable-boot/mbfw/uploads/6f7ddfe3be24e... - Dependable Boot Specification diff --git a/doc/develop/uefi/index.rst b/doc/develop/uefi/index.rst index 7e65dbc5d5..e26b1fbe05 100644 --- a/doc/develop/uefi/index.rst +++ b/doc/develop/uefi/index.rst @@ -13,3 +13,4 @@ can be run an UEFI payload. uefi.rst u-boot_on_efi.rst iscsi.rst

• fwu_updates.rst

diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst index 941e427093..536b278dd9 100644 --- a/doc/develop/uefi/uefi.rst +++ b/doc/develop/uefi/uefi.rst @@ -277,6 +277,8 @@ Enable ``CONFIG_OPTEE``, ``CONFIG_CMD_OPTEE_RPMB`` and ``CONFIG_EFI_MM_COMM_TEE`

[1] https://optee.readthedocs.io/en/latest/building/efi_vars/stmm.html

+.. _uefi_capsule_update_ref:

• Enabling UEFI Capsule Update feature

On Fri, 22 Jul 2022 at 23:13, jassisinghbrar@gmail.com wrote:

From: Sughosh Ganu sughosh.ganu@linaro.org

Add bindings needed for accessing the FWU metadata regions. These include the compatible string which point to the access method, the actual device which stores the FWU metadata and the offsets for both metadata regions.

The current patch adds basic bindings needed for accessing the metadata structure on non-GPT mtd regions.

Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org Signed-off-by: Jassi Brar jaswinder.singh@linaro.org

This patch is authored by Masami Hiramatsu, and should reflect that.

-sughosh

---

.../firmware/fwu-mdata-mtd.yaml | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 doc/device-tree-bindings/firmware/fwu-mdata-mtd.yaml

diff --git a/doc/device-tree-bindings/firmware/fwu-mdata-mtd.yaml b/doc/device-tree-bindings/firmware/fwu-mdata-mtd.yaml new file mode 100644 index 0000000000..4f5404f999 --- /dev/null +++ b/doc/device-tree-bindings/firmware/fwu-mdata-mtd.yaml @@ -0,0 +1,38 @@ +# 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#

+title: FWU metadata on MTD device without GPT

+maintainers:

• • Masami Hiramatsu masami.hiramatsu@linaro.org

+properties:

• compatible:
• items:

•  - const: u-boot,fwu-mdata-mtd
  

• fwu-mdata-store:
• maxItems: 1
• description: Phandle of the MTD device which contains the FWU medatata.
• mdata-offsets:
• minItems: 2
• description: Offsets of the primary and secondary FWU metadata in the NOR flash.

+required:

• • compatible
• • fwu-mdata-store
• • mdata-offsets

+additionalProperties: false

+examples:

• • |
• fwu-mdata {

•    compatible = "u-boot,fwu-mdata-mtd";
  

•    fwu-mdata-store = <&spi-flash>;
  

•    mdata-offsets = <0x500000 0x530000>;
  

• };

-- 2.25.1

On 7/22/22 19:43, jassisinghbrar@gmail.com wrote:

From: Sughosh Ganu sughosh.ganu@linaro.org

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 sughosh.ganu@linaro.org Signed-off-by: Jassi Brar jaswinder.singh@linaro.org

---

drivers/fwu-mdata/Kconfig | 8 + drivers/fwu-mdata/Makefile | 1 + drivers/fwu-mdata/fwu_mdata_mtd.c | 308 ++++++++++++++++++++++++++++++ 3 files changed, 317 insertions(+) create mode 100644 drivers/fwu-mdata/fwu_mdata_mtd.c

diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig index d5edef19d6..a8fa9ad783 100644 --- a/drivers/fwu-mdata/Kconfig +++ b/drivers/fwu-mdata/Kconfig @@ -14,3 +14,11 @@ config FWU_MDATA_GPT_BLK help Enable support for accessing FWU Metadata on GPT partitioned block devices.

+config FWU_MDATA_MTD

• bool "FWU Metadata access for non-GPT MTD devices"
• depends on DM_FWU_MDATA && MTD
• help

•  Enable support for accessing FWU Metadata on non-partitioned
  

•  (or non-GPT partitioned, e.g. partition nodes in devicetree)
  

•  MTD devices.
  

diff --git a/drivers/fwu-mdata/Makefile b/drivers/fwu-mdata/Makefile index 313049f67a..58f8023f16 100644 --- a/drivers/fwu-mdata/Makefile +++ b/drivers/fwu-mdata/Makefile @@ -5,3 +5,4 @@

obj-$(CONFIG_DM_FWU_MDATA) += fwu-mdata-uclass.o obj-$(CONFIG_FWU_MDATA_GPT_BLK) += fwu_mdata_gpt_blk.o +obj-$(CONFIG_FWU_MDATA_MTD) += fwu_mdata_mtd.o diff --git a/drivers/fwu-mdata/fwu_mdata_mtd.c b/drivers/fwu-mdata/fwu_mdata_mtd.c new file mode 100644 index 0000000000..d543a419fd --- /dev/null +++ b/drivers/fwu-mdata/fwu_mdata_mtd.c @@ -0,0 +1,308 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/*

• • Copyright (c) 2022, Linaro Limited
• */

+#include <efi_loader.h> +#include <fwu.h> +#include <fwu_mdata.h> +#include <malloc.h> +#include <memalign.h> +#include <spi.h> +#include <spi_flash.h> +#include <flash.h>

+#include <linux/errno.h> +#include <linux/types.h> +#include <u-boot/crc.h>

How should a reader know what pri_offset and sec_offset refer to? Please, provide Sphix style comments describing the structure.

+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,

+};

Please, document all functions.

+static bool /(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)
  

Most of the functionality of this function should be in the mtd uclass. It is not FWU specific.

+{

• 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))

• return -EINVAL;
  

I think this is the only place where you could write an error message indicating that misplacement is the reason for the error.

• lock_offs = offs;
• 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 = {};
  

• /* This will expand erase size to align with the block size */
  

This comment is misplaced. It should be above the round_up() call.

• erase_op.mtd = mtd;
  

• erase_op.addr = lock_offs;
  

• erase_op.len = lock_len;
  

• erase_op.scrub = 0;
  

• ret = mtd_erase(mtd, &erase_op);
  

• if (ret)
  

• 	goto lock_out;
  

• }
• /* 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_out;
  

• }
• 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);
  

Don't copy random bytes to the flash. You have to zero out the padding bytes.

• ret = mtd_write_oob(mtd, offs, &io_op);
  

• } else {

• ret = mtd_read_oob(mtd, offs, &io_op);
  

• if (!ret)
  

• 	memcpy(data, buf, size);
  

• }
• free(buf);

+lock_out:

This label sound like you want to lock something out. Please use lock: instead.

• mtd_lock(mtd, lock_offs, lock_len);
• return ret;

+}

+static int fwu_mtd_load_mdata(struct mtd_info *mtd, struct fwu_mdata **mdata,

• 	      u32 offs, bool primary)
  

+{

• size_t size = sizeof(struct fwu_mdata);
• int ret;
• *mdata = malloc(size);
• if (!*mdata)

• return -ENOMEM;
  

• ret = mtd_io_data(mtd, offs, size, (void *)*mdata, FWU_MTD_READ);

The conversion to (void *) is superfluous as the expected parameter is of type void *.

• if (ret >= 0) {

• ret = fwu_verify_mdata(*mdata, primary);
  

• if (ret < 0) {
  

• 	free(*mdata);
  

• 	*mdata = NULL;
  

• }
  

• }
• return ret;

+}

+static int fwu_mtd_load_primary_mdata(struct fwu_mdata_mtd_priv *mtd_priv,

• 		     struct fwu_mdata **mdata)
  

+{

• return fwu_mtd_load_mdata(mtd_priv->mtd, mdata, mtd_priv->pri_offset, true);

Instead of true and false use an enum like

enum fwu_sec_prim { FWU_META_PRIMARY, FWU_META_SECONDARY, }

and get rid of these functions. That will give you the same readability with less of complexity.

+}

+static int fwu_mtd_load_secondary_mdata(struct fwu_mdata_mtd_priv *mtd_priv,

• 		       struct fwu_mdata **mdata)
  

+{

• return fwu_mtd_load_mdata(mtd_priv->mtd, mdata, mtd_priv->sec_offset, false);

+}

+static int fwu_mtd_save_primary_mdata(struct fwu_mdata_mtd_priv *mtd_priv,

• 		     struct fwu_mdata *mdata)
  

+{

• return mtd_io_data(mtd_priv->mtd, mtd_priv->pri_offset,

• 	   sizeof(struct fwu_mdata), mdata, FWU_MTD_WRITE);
  

+}

+static int fwu_mtd_save_secondary_mdata(struct fwu_mdata_mtd_priv *mtd_priv,

• 		       struct fwu_mdata *mdata)
  

+{

• return mtd_io_data(mtd_priv->mtd, mtd_priv->sec_offset,

• 	   sizeof(struct fwu_mdata), mdata, FWU_MTD_WRITE);
  

+}

+static int fwu_mtd_get_valid_mdata(struct fwu_mdata_mtd_priv *mtd_priv,

• 		  struct fwu_mdata **mdata)
  

+{

• if (fwu_mtd_load_primary_mdata(mtd_priv, mdata) == 0)

Please, don't use == 0. But "if (!".

• return 0;
  

• log_err("Failed to load/verify primary mdata. Try secondary.\n");
• if (fwu_mtd_load_secondary_mdata(mtd_priv, mdata) == 0)

ditto

• return 0;
  

• log_err("Failed to load/verify secondary mdata.\n");
• return -1;

+}

+static int fwu_mtd_update_mdata(struct udevice *dev, struct fwu_mdata *mdata) +{

• struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
• int ret;
• /* Update mdata crc32 field */
• mdata->crc32 = crc32(0, (void *)&mdata->version,

Avoid superfluous conversions. There is already a conversion in the defintion of the crc32 macro.

• 	     sizeof(*mdata) - sizeof(u32));
  

• /* First write the primary mdata */
• ret = fwu_mtd_save_primary_mdata(mtd_priv, mdata);
• if (ret < 0) {

• log_err("Failed to update the primary mdata.\n");
  

• return ret;
  

• }
• /* And now the replica */
• ret = fwu_mtd_save_secondary_mdata(mtd_priv, mdata);
• if (ret < 0) {

• log_err("Failed to update the secondary mdata.\n");
  

• return ret;
  

• }
• return 0;

+}

+static int fwu_mtd_mdata_check(struct udevice *dev) +{

• struct fwu_mdata *primary = NULL, *secondary = NULL;
• struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
• int ret;
• ret = fwu_mtd_load_primary_mdata(mtd_priv, &primary);
• if (ret < 0)

• log_err("Failed to read the primary mdata: %d\n", ret);
  

• ret = fwu_mtd_load_secondary_mdata(mtd_priv, &secondary);
• if (ret < 0)

• log_err("Failed to read the secondary mdata: %d\n", ret);
  

• if (primary && secondary) {

• if (memcmp(primary, secondary, sizeof(struct fwu_mdata))) {
  

• 	log_err("The primary and the secondary mdata are different\n");
  

• 	ret = -1;
  

• }
  

• } else if (primary) {

• ret = fwu_mtd_save_secondary_mdata(mtd_priv, primary);
  

• if (ret < 0)
  

• 	log_err("Restoring secondary mdata partition failed\n");
  

• } else if (secondary) {

• ret = fwu_mtd_save_primary_mdata(mtd_priv, secondary);
  

• if (ret < 0)
  

• 	log_err("Restoring primary mdata partition failed\n");
  

• }

If neither primary nor secondary data is available, you are happy???

• free(primary);
• free(secondary);
• return ret;

+}

+static int fwu_mtd_get_mdata(struct udevice *dev, struct fwu_mdata **mdata) +{

• struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
• return fwu_mtd_get_valid_mdata(mtd_priv, mdata);

+}

+/**

• • fwu_mdata_mtd_of_to_plat() - Translate from DT to fwu mdata device
• */

+static int fwu_mdata_mtd_of_to_plat(struct udevice *dev) +{

• struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
• const fdt32_t *phandle_p = NULL;
• struct udevice *mtd_dev;
• struct mtd_info *mtd;
• int ret, size;
• u32 phandle;
• /* Find the FWU mdata storage device */
• phandle_p = ofnode_get_property(dev_ofnode(dev),

• 			"fwu-mdata-store", &size);
  

• if (!phandle_p) {

• log_err("fwu-mdata-store property not found\n");
  

A user needs to know that the problem is in the device-tree, e.g. "FWU meta data store not defined in device-tree".

• return -ENOENT;
  

• }
• phandle = fdt32_to_cpu(*phandle_p);
• ret = device_get_global_by_ofnode(

• ofnode_get_by_phandle(phandle),
  

• &mtd_dev);
  

• if (ret)

• return ret;
  

No log message?

• mtd_probe_devices();
• mtd_for_each_device(mtd) {

• if (mtd->dev == mtd_dev) {
  

• 	mtd_priv->mtd = mtd;
  

• 	log_debug("Found the FWU mdata mtd device %s\n", mtd->name);
  

• 	break;
  

• }
  

• }
• if (!mtd_priv->mtd) {

• log_err("Failed to find mtd device by fwu-mdata-store\n");
  

• return -ENOENT;
  

• }
• /* Get the offset of primary and seconday mdata */
• ret = ofnode_read_u32_index(dev_ofnode(dev), "mdata-offsets", 0,

• 		    &mtd_priv->pri_offset);
  

• if (ret)

• return ret;
  

• ret = ofnode_read_u32_index(dev_ofnode(dev), "mdata-offsets", 1,

• 		    &mtd_priv->sec_offset);
  

• if (ret)

• return ret;
  

• return 0;

+}

+static int fwu_mdata_mtd_probe(struct udevice *dev) +{

• /* Ensure the metadata can be read. */
• return fwu_mtd_mdata_check(dev);

If you don't like the name of fwu_mtd_mdata_check(), change it instead of wrapping it in another function.

Best regards

Heinrich

+}

+static struct fwu_mdata_ops fwu_mtd_ops = {

• .mdata_check = fwu_mtd_mdata_check,
• .get_mdata = fwu_mtd_get_mdata,
• .update_mdata = fwu_mtd_update_mdata,

+};

+static const struct udevice_id fwu_mdata_ids[] = {

• { .compatible = "u-boot,fwu-mdata-mtd" },
• { }

+};

+U_BOOT_DRIVER(fwu_mdata_mtd) = {

• .name = "fwu-mdata-mtd",
• .id = UCLASS_FWU_MDATA,
• .of_match = fwu_mdata_ids,
• .ops = &fwu_mtd_ops,
• .probe = fwu_mdata_mtd_probe,
• .of_to_plat = fwu_mdata_mtd_of_to_plat,
• .priv_auto = sizeof(struct fwu_mdata_mtd_priv),

+};

On 7/22/22 19:43, jassisinghbrar@gmail.com wrote:

From: Sughosh Ganu sughosh.ganu@linaro.org

Add helper functions needed for accessing the FWU metadata which contains information on the updatable images.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org Signed-off-by: Jassi Brar jaswinder.singh@linaro.org

---

lib/fwu_updates/Makefile | 1 + lib/fwu_updates/fwu_mtd.c | 173 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 174 insertions(+) create mode 100644 lib/fwu_updates/fwu_mtd.c

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 0000000000..f13cb149a2 --- /dev/null +++ b/lib/fwu_updates/fwu_mtd.c @@ -0,0 +1,173 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/*

• • Copyright (c) 2022, Linaro Limited
• */

+#include <dfu.h> +#include <fwu.h> +#include <fwu_mdata.h> +#include <log.h> +#include <malloc.h> +#include <mtd.h> +#include <uuid.h> +#include <vsprintf.h>

+#include <dm/ofnode.h>

Please, document functions.

You missed adding the function to an include in the series. The include changes should be in the same patch as the library functions.

+int mtd_plat_get_alt_num(efi_guid_t *image_id, int *alt_num,

• 	const char *mtd_dev, bool guid)
  

+{

• 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,

• 	guid ? UUID_STR_FORMAT_GUID : 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;
  

• }

Isn't finding an mtd partition by UUID something that should be a function in the mtd uclass because it may find usages outside FWU?

• offset = ofnode_get_addr_size_index_notrans(node, 0, &size);
• if (offset == FDT_ADDR_T_NONE || !size)

• return -ENOENT;
  

• dfu_init_env_entities(NULL, NULL);
• nalt = 0;
• list_for_each_entry(dfu, &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);
  

• if (!dfu)
  

• 	continue;
  

• if (dfu->dev_type != DFU_DEV_MTD)
  

• 	continue;
  

• if (dfu->layout == DFU_RAW_ADDR &&
  

•     dfu->data.mtd.start == offset &&
  

•     dfu->data.mtd.size == size) {
  

• 	*alt_num = dfu->alt;
  

• 	ret = 0;
  

• 	break;
  

• }
  

• }
• dfu_free_entities();
• return ret;

+}

+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) {

• log_debug("Get location uuid %s\n", suuid);
  

• uuid_bin_to_str(img->location_uuid.b, uuidbuf,
  

• 		UUID_STR_FORMAT_STD);
  

• if (strcasecmp(suuid, uuidbuf))
  

• 	log_warning("Warning: Location UUID does not match!\n");
  

• }
• p += snprintf(p, end - p, "mtd %s", mtd->name);
• if (end < p)

err_log() needed?

• return -E2BIG;
  

• /*

• * List up the image banks in the FWU mdata and search the corresponding
  

%s/up //

• * partition based on partition's uuid.
  

• */
  

• for (i = 0; i < CONFIG_FWU_NUM_BANKS; i++) {

• struct fwu_image_bank_info *bank;
  

• fdt_addr_t offset, size;
  

• /* Query a partition by image UUID */
  

• bank = &img->img_bank_info[i];
  

• uuid_bin_to_str(bank->image_uuid.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");
  

• 	break;
  

• }
  

• offset = ofnode_get_addr_size_index_notrans(node, 0, &size);
  

• if (offset == FDT_ADDR_T_NONE || !size)
  

• 	break;
  

• p += snprintf(p, end - p, "%sbank%d raw %lx %lx",
  

• 	      i == 0 ? "=" : ";", i, (unsigned long)offset,
  

• 	      (unsigned long)size);
  

• if (end < p)
  

• 	return -E2BIG;
  

• }
• return i != CONFIG_FWU_NUM_BANKS ? -ENOENT : 0;

This is easier to read:

if (i != CONFIG_FWU_NUM_BANKS) return -ENOENT; return 0;

Best regards

Heinrich

+}

+int fwu_gen_alt_info_from_mtd(char *buf, size_t len, struct mtd_info *mtd) +{

• struct fwu_mdata *mdata = NULL;
• int i, l, ret;
• ret = fwu_get_mdata(&mdata);
• if (ret < 0) {

• log_debug("Failed to get the FWU mdata.\n");
  

• return ret;
  

• }
• for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) {

• ret = gen_image_alt_info(buf, len, i * CONFIG_FWU_NUM_BANKS,
  

• 			 &mdata->img_entry[i], mtd);
  

• if (ret)
  

• 	break;
  

• l = strlen(buf);
  

• /* Replace the last ';' with '&' if there is another image. */
  

• if (i != CONFIG_FWU_NUM_IMAGES_PER_BANK - 1 && l)
  

• 	buf[l - 1] = '&';
  

• len -= l;
  

• buf += l;
  

• }
• free(mdata);
• return ret;

+}

From: Jassi Brar jaswinder.singh@linaro.org

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: Masami Hiramatsu masami.hiramatsu@linaro.org Signed-off-by: Jassi Brar jaswinder.singh@linaro.org --- board/socionext/developerbox/Makefile | 1 + board/socionext/developerbox/developerbox.c | 13 +++ board/socionext/developerbox/fwu_plat.c | 95 ++++++++++++++++++++ configs/synquacer_developerbox_defconfig | 13 ++- doc/board/socionext/developerbox.rst | 96 +++++++++++++++++++++ include/configs/synquacer.h | 10 +++ 6 files changed, 226 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 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", + .image_index = 1, + }, +#else { .image_type_id = DEVELOPERBOX_UBOOT_IMAGE_GUID, .fw_name = u"DEVELOPERBOX-UBOOT", @@ -35,12 +42,18 @@ struct efi_fw_image fw_images[] = { .fw_name = u"DEVELOPERBOX-OPTEE", .image_index = 3, }, +#endif };

struct efi_capsule_update_info update_info = { +#if defined(CONFIG_FWU_MULTI_BANK_UPDATE) + .dfu_string = "mtd nor1=bank0 raw 600000 400000;" + "bank1 raw a00000 400000;", +#else .dfu_string = "mtd nor1=u-boot.bin raw 200000 100000;" "fip.bin raw 180000 78000;" "optee.bin raw 500000 100000", +#endif .images = fw_images, };

diff --git a/board/socionext/developerbox/fwu_plat.c b/board/socionext/developerbox/fwu_plat.c new file mode 100644 index 0000000000..9fb5cb28b3 --- /dev/null +++ b/board/socionext/developerbox/fwu_plat.c @@ -0,0 +1,95 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2021, Linaro Limited + */ + +#include <dfu.h> +#include <efi_loader.h> +#include <flash.h> +#include <fwu.h> +#include <fwu_mdata.h> +#include <malloc.h> +#include <memalign.h> +#include <mtd.h> +#include <spi.h> +#include <spi_flash.h> +#include <uuid.h> + +#include <linux/errno.h> +#include <linux/types.h> +#include <u-boot/crc.h> + +#define DFU_ALT_BUF_LEN 256 +#define DFU_ALT_NUM_MAX (CONFIG_FWU_NUM_IMAGES_PER_BANK * CONFIG_FWU_NUM_BANKS) + +/* Generate dfu_alt_info from partitions */ +void set_dfu_alt_info(char *interface, char *devstr) +{ + int ret; + struct mtd_info *mtd; + static char *buf = NULL; + + if (!buf) { + buf = malloc_cache_aligned(DFU_ALT_BUF_LEN); + memset(buf, 0, DFU_ALT_BUF_LEN); + + 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, int *alt_num) +{ + return mtd_plat_get_alt_num(image_id, alt_num, "nor1", 0); +} + +int fwu_plat_get_update_index(u32 *update_idx) +{ + int ret; + u32 active_idx; + + ret = fwu_get_active_index(&active_idx); + + if (ret < 0) + return ret; + + *update_idx = 1 - active_idx; + + return ret; +} + +void fwu_plat_get_bootidx(void *boot_idx) +{ + int ret; + u32 active_idx; + u32 *bootidx = boot_idx; + + ret = fwu_get_active_index(&active_idx); + + if (ret < 0) + *bootidx = -1; + + *bootidx = active_idx; +} + +int board_late_init(void) +{ + /* Make mmc available for EFI, otherwise efi subsystem + * complains "No EFI system partition" during bootup. + */ + run_command("mmc dev 0", 0); + + return 0; +} diff --git a/configs/synquacer_developerbox_defconfig b/configs/synquacer_developerbox_defconfig index add6041e27..ded31ada6e 100644 --- a/configs/synquacer_developerbox_defconfig +++ b/configs/synquacer_developerbox_defconfig @@ -1,10 +1,11 @@ CONFIG_ARM=y CONFIG_ARCH_SYNQUACER=y -CONFIG_SYS_TEXT_BASE=0x08200000 +CONFIG_POSITION_INDEPENDENT=y +CONFIG_SYS_TEXT_BASE=0 CONFIG_SYS_MALLOC_LEN=0x1000000 CONFIG_SYS_MALLOC_F_LEN=0x400 CONFIG_ENV_SIZE=0x30000 -CONFIG_ENV_OFFSET=0x300000 +CONFIG_ENV_OFFSET=0x580000 CONFIG_ENV_SECT_SIZE=0x10000 CONFIG_DM_GPIO=y CONFIG_DEFAULT_DEVICE_TREE="synquacer-sc2a11-developerbox" @@ -93,3 +94,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_BOARD_LATE_INIT=y +CONFIG_FWU_MULTI_BANK_UPDATE=y +CONFIG_DM_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 diff --git a/doc/board/socionext/developerbox.rst b/doc/board/socionext/developerbox.rst index 2d943c23be..f52820c2b0 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 Bank Update. You *MUST* update both *SCP firmware* and *TF-A* for this feature. This will change the layout and the boot process but you can switch back to the normal one by changing the DSW 1-4 off. + +Configure U-Boot +---------------- + +To enable the FWU Multi Bank Update on the DeveloperBox, you need to add following configurations to configs/synquacer_developerbox_defconfig :: + + CONFIG_FWU_MULTI_BANK_UPDATE=y + CONFIG_DM_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 + +And build it:: + + cd u-boot/ + export ARCH=arm64 + export CROSS_COMPILE=aarch64-linux-gnu- + make synqucer_developerbox_defconfig + make -j `noproc` + cd ../ + +By default, the CONFIG_FWU_NUM_BANKS and COFNIG_FWU_NUM_IMAGES_PER_BANKS are set to 2 and 1 respectively. This uses FIP (Firmware Image Package) type image which contains TF-A, U-Boot and OP-TEE (the OP-TEE is optional.) +You can use fiptool to compose the FIP image from those firmware images. + +Rebuild SCP firmware +-------------------- + +Rebuild SCP firmware which supports FWU Multi Bank Update as below:: + + cd SCP-firmware/ + OUT=./build/product/synquacer + ROMFW_FILE=$OUT/scp_romfw/$SCP_BUILD_MODE/bin/scp_romfw.bin + RAMFW_FILE=$OUT/scp_ramfw/$SCP_BUILD_MODE/bin/scp_ramfw.bin + ROMRAMFW_FILE=scp_romramfw_release.bin + + make CC=$ARM_EMB_GCC PRODUCT=synquacer MODE=release + tr "\000" "\377" < /dev/zero | dd of=${ROMRAMFW_FILE} bs=1 count=196608 + dd if=${ROMFW_FILE} of=${ROMRAMFW_FILE} bs=1 conv=notrunc seek=0 + dd if=${RAMFW_FILE} of=${ROMRAMFW_FILE} bs=1 seek=65536 + cd ../ + +And you can get the `scp_romramfw_release.bin` file + +Rebuild TF-A and FIP +-------------------- + +Rebuild TF-A which supports FWU Multi Bank Update as below:: + + cd arm-trusted-firmware/ + make CROSS_COMPILE=aarch64-linux-gnu- -j`nproc` PLAT=synquacer \ + SPD=opteed SQ_RESET_TO_BL2=1 GENERATE_COT=1 MBEDTLS_DIR=../mbedtls \ + BL33=../u-boot/u-boot.bin all fip fiptool + +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 + - Image type UUID for the FIP image: 10c36d7d-ca52-b843-b7b9-f9d6c501d108 + - Image UUID for Bank0 : 5a66a702-99fd-4fef-a392-c26e261a2828 + - Image UUID for Bank1 : a8f868a1-6e5c-4757-878d-ce63375ef2c0 + +These UUIDs are used for making a FWU metadata image. + +Install via flash writer +------------------------ + +As explained in above section, the new FIP image and the FWU metadata image can be installed via NOR flash writer. Note that the installation offsets for the FWU multi bank update supported firmware. + +Once the flasher tool is running we are ready flash the images.:: +Write the FIP image to the 0x600000 offset.:: + + flash rawwrite 600000 180000 + >> Send SPI_NOR_NEWFIP.fd via XMODEM (Control-A S in minicom) << + +And write the new SCP firmware.:: + + flash write cm3 + >> Send scp_romramfw_release.bin via XMODEM (Control-A S in minicom) << + +At last, turn on the DSW 3-4 on the board, and reboot. +Note that if DSW 3-4 is turned off, the DeveloperBox will boot from +the original EDK2 firmware (or non-FWU U-Boot if you already installed.) diff --git a/include/configs/synquacer.h b/include/configs/synquacer.h index 5686a5b910..7995be852d 100644 --- a/include/configs/synquacer.h +++ b/include/configs/synquacer.h @@ -46,19 +46,29 @@

/* Since U-Boot 64bit PCIe support is limited, disable 64bit MMIO support */

+#ifdef CONFIG_FWU_MULTI_BANK_UPDATE +#define DEFAULT_DFU_ALT_INFO +#else #define DEFAULT_DFU_ALT_INFO "dfu_alt_info=" \ "mtd nor1=u-boot.bin raw 200000 100000;" \ "fip.bin raw 180000 78000;" \ "optee.bin raw 500000 100000\0" +#endif

/* GUIDs for capsule updatable firmware images */ #define DEVELOPERBOX_UBOOT_IMAGE_GUID \ EFI_GUID(0x53a92e83, 0x4ef4, 0x473a, 0x8b, 0x0d, \ 0xb5, 0xd8, 0xc7, 0xb2, 0xd6, 0x00)

+#ifdef CONFIG_FWU_MULTI_BANK_UPDATE +#define DEVELOPERBOX_FIP_IMAGE_GUID \ + EFI_GUID(0x7d6dc310, 0x52ca, 0x43b8, 0xb7, 0xb9, \ + 0xf9, 0xd6, 0xc5, 0x01, 0xd1, 0x08) +#else #define DEVELOPERBOX_FIP_IMAGE_GUID \ EFI_GUID(0x880866e9, 0x84ba, 0x4793, 0xa9, 0x08, \ 0x33, 0xe0, 0xb9, 0x16, 0xf3, 0x98) +#endif

#define DEVELOPERBOX_OPTEE_IMAGE_GUID \ EFI_GUID(0xc1b629f1, 0xce0e, 0x4894, 0x82, 0xbf, \

On 9/1/22 09:07, Heinrich Schuchardt wrote:

On 7/22/22 19:43, jassisinghbrar@gmail.com wrote:

...

From: Jassi Brar jaswinder.singh@linaro.org

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: Masami Hiramatsu masami.hiramatsu@linaro.org Signed-off-by: Jassi Brar jaswinder.singh@linaro.org

---

board/socionext/developerbox/Makefile       |  1 +   board/socionext/developerbox/developerbox.c | 13 +++   board/socionext/developerbox/fwu_plat.c     | 95 ++++++++++++++++++++   configs/synquacer_developerbox_defconfig    | 13 ++-   doc/board/socionext/developerbox.rst        | 96 +++++++++++++++++++++   include/configs/synquacer.h                 | 10 +++   6 files changed, 226 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 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,...

Best regards

Heinrich>

...

+        .image_index = 1, +    }, +#else       {           .image_type_id = DEVELOPERBOX_UBOOT_IMAGE_GUID,           .fw_name = u"DEVELOPERBOX-UBOOT", @@ -35,12 +42,18 @@ struct efi_fw_image fw_images[] = {           .fw_name = u"DEVELOPERBOX-OPTEE",           .image_index = 3,       }, +#endif   };

struct efi_capsule_update_info update_info = { +#if defined(CONFIG_FWU_MULTI_BANK_UPDATE) +    .dfu_string = "mtd nor1=bank0 raw 600000 400000;" +            "bank1 raw a00000 400000;", +#else       .dfu_string = "mtd nor1=u-boot.bin raw 200000 100000;"               "fip.bin raw 180000 78000;"               "optee.bin raw 500000 100000", +#endif       .images = fw_images,   };

diff --git a/board/socionext/developerbox/fwu_plat.c b/board/socionext/developerbox/fwu_plat.c new file mode 100644 index 0000000000..9fb5cb28b3 --- /dev/null +++ b/board/socionext/developerbox/fwu_plat.c @@ -0,0 +1,95 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/*

• • Copyright (c) 2021, Linaro Limited
• */

+#include <dfu.h> +#include <efi_loader.h> +#include <flash.h> +#include <fwu.h> +#include <fwu_mdata.h> +#include <malloc.h> +#include <memalign.h> +#include <mtd.h> +#include <spi.h> +#include <spi_flash.h> +#include <uuid.h>

+#include <linux/errno.h> +#include <linux/types.h> +#include <u-boot/crc.h>

+#define DFU_ALT_BUF_LEN 256 +#define DFU_ALT_NUM_MAX (CONFIG_FWU_NUM_IMAGES_PER_BANK * CONFIG_FWU_NUM_BANKS)

+/* Generate dfu_alt_info from partitions */ +void set_dfu_alt_info(char *interface, char *devstr) +{ +    int ret; +    struct mtd_info *mtd; +    static char *buf = NULL;

+    if (!buf) { +        buf = malloc_cache_aligned(DFU_ALT_BUF_LEN); +        memset(buf, 0, DFU_ALT_BUF_LEN);

+        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, int *alt_num) +{ +    return mtd_plat_get_alt_num(image_id, alt_num, "nor1", 0); +}

+int fwu_plat_get_update_index(u32 *update_idx) +{ +    int ret; +    u32 active_idx;

+    ret = fwu_get_active_index(&active_idx);

+    if (ret < 0) +        return ret;

+    *update_idx = 1 - active_idx;

+    return ret; +}

+void fwu_plat_get_bootidx(void *boot_idx) +{ +    int ret; +    u32 active_idx; +    u32 *bootidx = boot_idx;

+    ret = fwu_get_active_index(&active_idx);

+    if (ret < 0) +        *bootidx = -1;

+    *bootidx = active_idx; +}

+int board_late_init(void) +{ +    /* Make mmc available for EFI, otherwise efi subsystem +     * complains "No EFI system partition" during bootup. +     */ +    run_command("mmc dev 0", 0);

+    return 0; +} diff --git a/configs/synquacer_developerbox_defconfig b/configs/synquacer_developerbox_defconfig index add6041e27..ded31ada6e 100644 --- a/configs/synquacer_developerbox_defconfig +++ b/configs/synquacer_developerbox_defconfig @@ -1,10 +1,11 @@   CONFIG_ARM=y   CONFIG_ARCH_SYNQUACER=y -CONFIG_SYS_TEXT_BASE=0x08200000 +CONFIG_POSITION_INDEPENDENT=y +CONFIG_SYS_TEXT_BASE=0   CONFIG_SYS_MALLOC_LEN=0x1000000   CONFIG_SYS_MALLOC_F_LEN=0x400   CONFIG_ENV_SIZE=0x30000 -CONFIG_ENV_OFFSET=0x300000 +CONFIG_ENV_OFFSET=0x580000   CONFIG_ENV_SECT_SIZE=0x10000   CONFIG_DM_GPIO=y   CONFIG_DEFAULT_DEVICE_TREE="synquacer-sc2a11-developerbox" @@ -93,3 +94,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_BOARD_LATE_INIT=y +CONFIG_FWU_MULTI_BANK_UPDATE=y +CONFIG_DM_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 diff --git a/doc/board/socionext/developerbox.rst b/doc/board/socionext/developerbox.rst index 2d943c23be..f52820c2b0 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 Bank Update. You *MUST* update both *SCP firmware* and *TF-A* for this feature. This will change the layout and the boot process but you can switch back to the normal one by changing the DSW 1-4 off.

+Configure U-Boot +----------------

+To enable the FWU Multi Bank Update on the DeveloperBox, you need to add following configurations to configs/synquacer_developerbox_defconfig ::

• CONFIG_FWU_MULTI_BANK_UPDATE=y
• CONFIG_DM_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

+And build it::

+  cd u-boot/ +  export ARCH=arm64 +  export CROSS_COMPILE=aarch64-linux-gnu- +  make synqucer_developerbox_defconfig +  make -j `noproc` +  cd ../

+By default, the CONFIG_FWU_NUM_BANKS and COFNIG_FWU_NUM_IMAGES_PER_BANKS are set to 2 and 1 respectively. This uses FIP (Firmware Image Package) type image which contains TF-A, U-Boot and OP-TEE (the OP-TEE is optional.) +You can use fiptool to compose the FIP image from those firmware images.

+Rebuild SCP firmware +--------------------

+Rebuild SCP firmware which supports FWU Multi Bank Update as below::

+  cd SCP-firmware/ +  OUT=./build/product/synquacer +  ROMFW_FILE=$OUT/scp_romfw/$SCP_BUILD_MODE/bin/scp_romfw.bin +  RAMFW_FILE=$OUT/scp_ramfw/$SCP_BUILD_MODE/bin/scp_ramfw.bin +  ROMRAMFW_FILE=scp_romramfw_release.bin

+  make CC=$ARM_EMB_GCC PRODUCT=synquacer MODE=release +  tr "\000" "\377" < /dev/zero | dd of=${ROMRAMFW_FILE} bs=1 count=196608 +  dd if=${ROMFW_FILE} of=${ROMRAMFW_FILE} bs=1 conv=notrunc seek=0 +  dd if=${RAMFW_FILE} of=${ROMRAMFW_FILE} bs=1 seek=65536 +  cd ../

+And you can get the `scp_romramfw_release.bin` file

+Rebuild TF-A and FIP +--------------------

+Rebuild TF-A which supports FWU Multi Bank Update as below::

+  cd arm-trusted-firmware/ +  make CROSS_COMPILE=aarch64-linux-gnu- -j`nproc` PLAT=synquacer \ +     SPD=opteed SQ_RESET_TO_BL2=1 GENERATE_COT=1 MBEDTLS_DIR=../mbedtls \ +     BL33=../u-boot/u-boot.bin all fip fiptool

+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
• • Image type UUID for the FIP image:

10c36d7d-ca52-b843-b7b9-f9d6c501d108

• • Image UUID for Bank0 : 5a66a702-99fd-4fef-a392-c26e261a2828
• • Image UUID for Bank1 : a8f868a1-6e5c-4757-878d-ce63375ef2c0

+These UUIDs are used for making a FWU metadata image.

+Install via flash writer +------------------------

+As explained in above section, the new FIP image and the FWU metadata image can be installed via NOR flash writer. Note that the installation offsets for the FWU multi bank update supported firmware.

+Once the flasher tool is running we are ready flash the images.:: +Write the FIP image to the 0x600000 offset.::

+  flash rawwrite 600000 180000 +  >> Send SPI_NOR_NEWFIP.fd via XMODEM (Control-A S in minicom) <<

+And write the new SCP firmware.::

+  flash write cm3 +  >> Send scp_romramfw_release.bin via XMODEM (Control-A S in minicom) <<

+At last, turn on the DSW 3-4 on the board, and reboot. +Note that if DSW 3-4 is turned off, the DeveloperBox will boot from +the original EDK2 firmware (or non-FWU U-Boot if you already installed.) diff --git a/include/configs/synquacer.h b/include/configs/synquacer.h index 5686a5b910..7995be852d 100644 --- a/include/configs/synquacer.h +++ b/include/configs/synquacer.h @@ -46,19 +46,29 @@

/* Since U-Boot 64bit PCIe support is limited, disable 64bit MMIO support */

+#ifdef CONFIG_FWU_MULTI_BANK_UPDATE +#define DEFAULT_DFU_ALT_INFO +#else   #define DEFAULT_DFU_ALT_INFO "dfu_alt_info="                \               "mtd nor1=u-boot.bin raw 200000 100000;"    \               "fip.bin raw 180000 78000;"            \               "optee.bin raw 500000 100000\0" +#endif

/* GUIDs for capsule updatable firmware images */   #define DEVELOPERBOX_UBOOT_IMAGE_GUID \       EFI_GUID(0x53a92e83, 0x4ef4, 0x473a, 0x8b, 0x0d, \            0xb5, 0xd8, 0xc7, 0xb2, 0xd6, 0x00)

+#ifdef CONFIG_FWU_MULTI_BANK_UPDATE +#define DEVELOPERBOX_FIP_IMAGE_GUID \ +    EFI_GUID(0x7d6dc310, 0x52ca, 0x43b8, 0xb7, 0xb9, \ +         0xf9, 0xd6, 0xc5, 0x01, 0xd1, 0x08) +#else   #define DEVELOPERBOX_FIP_IMAGE_GUID \       EFI_GUID(0x880866e9, 0x84ba, 0x4793, 0xa9, 0x08, \            0x33, 0xe0, 0xb9, 0x16, 0xf3, 0x98) +#endif

#define DEVELOPERBOX_OPTEE_IMAGE_GUID \       EFI_GUID(0xc1b629f1, 0xce0e, 0x4894, 0x82, 0xbf, \