From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id BF4F5C54EE9 for ; Wed, 7 Sep 2022 11:02:57 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id EDE828494A; Wed, 7 Sep 2022 13:02:54 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="NeyJI5Hl"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id E544D84A4C; Wed, 7 Sep 2022 13:02:52 +0200 (CEST) Received: from mail-vs1-xe2f.google.com (mail-vs1-xe2f.google.com [IPv6:2607:f8b0:4864:20::e2f]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 55B21848DC for ; Wed, 7 Sep 2022 13:02:49 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=sughosh.ganu@linaro.org Received: by mail-vs1-xe2f.google.com with SMTP id a129so4376693vsc.0 for ; Wed, 07 Sep 2022 04:02:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date; bh=P9OtjA7k2P8Ehap5VkD0Bvw9ZoIkkYzeP6UMSMh/sp8=; b=NeyJI5HlDPBRshwJmKBHL2Wv8f5gRj+fxm6sAqi6+XCQkik7HDzxvAYixUYfaHLECo 4qCjhjIpM1pn140WQqkXP68LiPLZu5dgZwV3It1cAwEqhghynLquzp3b4nJ6WH9yIFha SCifbQTRut1qDNGYV1y4EGwhLhv06VSdAgLP+7qQJLB7h/fSWgRXjJPbf9KSOQIa/FTJ UBd9wtnZMDOe0qrjxsohiYL3UlERvGx+fkR2oY9PKhoDvUCp6W0ho06G5kHq1wEuYZpv RkjZjzM0K+xCrhJ5qJ8YVSQwgQRdwVftRu4tLMOMyl+0HSFNexTs5FllREQbNWY7gPL0 aNeQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date; bh=P9OtjA7k2P8Ehap5VkD0Bvw9ZoIkkYzeP6UMSMh/sp8=; b=l893FG44g1Vhu7vA5+BUkzb05SobTSfRhs2vvQ/rDZ/wwsZAlGLPT7JJCFCLOFdTXE sftnu/gXugeZvB5Wz6kVNMR6nbG4SO3O+o/c2D/5OHiAEG6jkpTeXwAKVU4sXlVKc7I/ Z8hSauyU+SUCGC7y88mnfjcKEWgYNFuFToez6iGP8BuVFFZ+/sqJj8Xle6C/4ELm1ZzM 97Td/eMKV50C1eQSZdUK0xBUEzOh9A0kPH0ELrPoad7z0/dIX9Q5pE0uwNi3W3y5xjuZ uqIIyoqeGqOQjSEnWFnPbpCKXJJm1U8yubmC00g9/sr1dHKXjNi6jMz74UUERv9ozxO6 uVHA== X-Gm-Message-State: ACgBeo1HxeIoBIT05NWkodb1uK+EvrH9zM1VsVnmVBK049Ci87/m55yx /8obbRBjiVnyS45qJq8VZLbZgZbE8MeCXqGax+iEXw== X-Google-Smtp-Source: AA6agR4KkP6o2K7FasV0/AnCpMOGyIXVZRuDYGi2lTWSHNX2Y13gBGeZz+n/yJEID2pUU+7zt8cwbTF8zpQLgJV8B/k= X-Received: by 2002:a67:d59d:0:b0:398:211f:9a45 with SMTP id m29-20020a67d59d000000b00398211f9a45mr338586vsj.78.1662548567849; Wed, 07 Sep 2022 04:02:47 -0700 (PDT) MIME-Version: 1.0 References: <20220826095716.1676150-1-sughosh.ganu@linaro.org> <20220826095716.1676150-3-sughosh.ganu@linaro.org> In-Reply-To: From: Sughosh Ganu Date: Wed, 7 Sep 2022 16:32:37 +0530 Message-ID: Subject: Re: [PATCH v9 02/15] FWU: Add FWU metadata structure and driver for accessing metadata To: Ilias Apalodimas Cc: u-boot@lists.denx.de, Heinrich Schuchardt , Takahiro Akashi , Patrick Delaunay , Patrice Chotard , Simon Glass , Bin Meng , Tom Rini , Etienne Carriere , Michal Simek , Jassi Brar Content-Type: text/plain; charset="UTF-8" X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean hi Ilias, On Wed, 7 Sept 2022 at 12:15, Ilias Apalodimas wrote: > > Hi Sughosh, > > On Fri, Aug 26, 2022 at 03:27:03PM +0530, Sughosh Ganu wrote: > > In the FWU Multi Bank Update feature, the information about the > > updatable images is stored as part of the metadata, which is stored on > > a dedicated partition. Add the metadata structure, and a driver model > > uclass which provides functions to access the metadata. These are > > generic API's, and implementations can be added based on parameters > > like how the metadata partition is accessed and what type of storage > > device houses the metadata. > > > > Signed-off-by: Sughosh Ganu > > Reviewed-by: Patrick Delaunay > > --- > > Changes since V8: > > * Declare the metadata struct on the stack rather than heap > > * Move the API's to fwu.c and only keep the DM ops in the uclass > > driver > > * Fix return codes as suggested by Simon > > * Change log_err to log_debug as suggested by Simon > > * Remove the __packed attribute on the metadata structures as > > suggested by Simon > > * Add comments to the API functions > > > > drivers/fwu-mdata/fwu-mdata-uclass.c | 107 ++++++++ > > include/dm/uclass-id.h | 1 + > > include/fwu.h | 210 ++++++++++++++++ > > include/fwu_mdata.h | 67 +++++ > > lib/fwu_updates/fwu.c | 357 +++++++++++++++++++++++++++ > > 5 files changed, 742 insertions(+) > > create mode 100644 drivers/fwu-mdata/fwu-mdata-uclass.c > > create mode 100644 include/fwu.h > > create mode 100644 include/fwu_mdata.h > > create mode 100644 lib/fwu_updates/fwu.c > > diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c > > new file mode 100644 > > index 0000000000..a871d77b4c > > --- /dev/null > > +++ b/lib/fwu_updates/fwu.c > > @@ -0,0 +1,357 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * Copyright (c) 2022, Linaro Limited > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include > > +#include > > +#include > > + > > +#define IMAGE_ACCEPT_SET BIT(0) > > +#define IMAGE_ACCEPT_CLEAR BIT(1) > > + > > +static int fwu_get_dev(struct udevice **dev) > > + > > +{ > > + int ret; > > + > > + ret = uclass_first_device(UCLASS_FWU_MDATA, dev); > > + if (ret) { > > + log_debug("Cannot find fwu device\n"); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +/** > > + * 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. > > + * > > + * Return: 0 if OK, -ve on error > > + * > > + */ > > +int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part) > > +{ > > + u32 calc_crc32; > > + void *buf; > > + > > + buf = &mdata->version; > > + calc_crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32)); > > + > > + if (calc_crc32 != mdata->crc32) { > > + log_debug("crc32 check failed for %s FWU metadata partition\n", > > + pri_part ? "primary" : "secondary"); > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + > > +/** > > + * fwu_get_active_index() - Get active_index from the FWU metadata > > + * @active_idx: 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_idx) > > +{ > > + int ret; > > + struct udevice *dev; > > + struct fwu_mdata mdata = { 0 }; > > + > > + ret = fwu_get_dev(&dev); > > + if (ret) > > + return ret; > > + > > + ret = fwu_get_mdata(dev, &mdata); > > + if (ret < 0) { > > + log_debug("Unable to get valid FWU metadata\n"); > > + goto out; > > + } > > > This pattern is repeated on the entire patch. Moreover the struct udevice > *dev is seems only needed for calling fwu_get_mdata(). So is there any > reason why can we can't change fwu_get_mdata() and fold in the fwu_get_dev()? Sure, I will move the fetching of the device to fwu_get_mdata(). Will also remove the braces as per your other review comment. Thanks. -sughosh > > > + > > + /* > > + * Found the FWU metadata partition, now read the active_index > > + * value > > + */ > > + *active_idx = mdata.active_index; > > + if (*active_idx >= CONFIG_FWU_NUM_BANKS) { > > + log_debug("Active index value read is incorrect\n"); > > + ret = -EINVAL; > > + } > > + > > +out: > > + return ret; > > +} > > + > > +/** > > + * 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) > > +{ > > + int ret; > > + struct udevice *dev; > > + struct fwu_mdata mdata = { 0 }; > > + > > + if (active_idx >= CONFIG_FWU_NUM_BANKS) { > > + log_debug("Invalid active index value\n"); > > + return -EINVAL; > > + } > > + > > + ret = fwu_get_dev(&dev); > > + if (ret) > > + return ret; > > + > > + ret = fwu_get_mdata(dev, &mdata); > > + if (ret < 0) { > > + log_debug("Unable to get valid FWU metadata\n"); > > + goto out; > > + } > > + > > + /* > > + * Update the active index and previous_active_index fields > > + * in the FWU metadata > > + */ > > + mdata.previous_active_index = mdata.active_index; > > + mdata.active_index = active_idx; > > + > > + /* > > + * Now write this updated FWU metadata to both the > > + * FWU metadata partitions > > + */ > > + ret = fwu_update_mdata(dev, &mdata); > > + if (ret < 0) { > > + log_debug("Failed to update FWU metadata partitions\n"); > > + ret = -EIO; > > + } > > + > > +out: > > + return ret; > > +} > > + > > +/** > > + * 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; > > + struct fwu_mdata mdata = { 0 }; > > + struct fwu_image_entry *img_entry; > > + struct fwu_image_bank_info *img_bank_info; > > + > > + ret = fwu_get_dev(&dev); > > + if (ret) > > + return ret; > > + > > + ret = fwu_get_mdata(dev, &mdata); > > + if (ret) { > > + log_debug("Unable to get valid FWU metadata\n"); > > + goto out; > > + } > > + > > + ret = -EINVAL; > > + /* > > + * 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); > > + if (ret) { > > + log_debug("alt_num not found for partition with GUID %pUs\n", > > + image_guid); > > + } else { > > + log_debug("alt_num %d for partition %pUs\n", > > + *alt_num, image_guid); > > + } > > Nit you don't need {} > > > + > > + goto out; > > + } > > + } > > + > > + log_debug("Partition with the image type %pUs not found\n", > > + image_type_id); > > + > > +out: > > + return ret; > > +} > > + > > +/** > > + * 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(&dev); > > + if (ret) > > + return ret; > > + > > + ret = fwu_get_mdata(dev, &mdata); > > + if (ret < 0) { > > + log_debug("Unable to get valid FWU metadata\n"); > > + goto out; > > + } > > + > > + /* > > + * 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; > > + > > + /* > > + * Now write this updated FWU metadata to both the > > + * FWU metadata partitions > > + */ > > + ret = fwu_update_mdata(dev, &mdata); > > + if (ret < 0) { > > + log_debug("Failed to update FWU metadata partitions\n"); > > + ret = -EIO; > > + } > > + > > +out: > > + return ret; > > +} > > + > > +/** > > + * fwu_clrset_image_accept() - Set or Clear the Acceptance bit for the image > > + * @img_type_id: GUID of the image type for which the accepted bit is to be > > + * set or cleared > > + * @bank: Bank of which the image's Accept bit is to be set or cleared > > + * @action: Action which specifies whether image's Accept bit is to be set or > > + * cleared > > + * > > + * Set/Clear the accepted bit for the image specified by the img_guid parameter. > > + * This indicates acceptance or rejection of image for subsequent boots by some > > + * governing component like OS(or firmware). > > + * > > + * Return: 0 if OK, -ve on error > > + * > > + */ > > +static int fwu_clrset_image_accept(efi_guid_t *img_type_id, u32 bank, u8 action) > > +{ > > + int ret, i; > > + struct udevice *dev; > > + struct fwu_mdata mdata = { 0 }; > > + struct fwu_image_entry *img_entry; > > + struct fwu_image_bank_info *img_bank_info; > > + > > + ret = fwu_get_dev(&dev); > > + if (ret) > > + return ret; > > + > > + ret = fwu_get_mdata(dev, &mdata); > > + if (ret < 0) { > > + log_debug("Unable to get valid FWU metadata\n"); > > + goto out; > > + } > > + > > + img_entry = &mdata.img_entry[0]; > > + for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) { > > + if (!guidcmp(&img_entry[i].image_type_uuid, img_type_id)) { > > + img_bank_info = &img_entry[i].img_bank_info[bank]; > > + if (action == IMAGE_ACCEPT_SET) > > + img_bank_info->accepted |= FWU_IMAGE_ACCEPTED; > > + else > > + img_bank_info->accepted = 0; > > + > > + ret = fwu_update_mdata(dev, &mdata); > > + goto out; > > + } > > + } > > + > > + /* Image not found */ > > + ret = -ENOENT; > > + > > +out: > > + return ret; > > +} > > + > > +/** > > + * 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 > > + * @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 cleared > > + * > > + * Clear the accepted bit for the image type specified by the img_type_id parameter. > > + * This function is called after the image has been updated. The accepted bit is > > + * cleared to be set subsequently after passing the image acceptance criteria, by > > + * either the OS(or firmware) > > + * > > + * Return: 0 if OK, -ve on error > > + * > > + */ > > +int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank) > > +{ > > + return fwu_clrset_image_accept(img_type_id, bank, > > + IMAGE_ACCEPT_CLEAR); > > +} > > -- > > 2.34.1 > > > > > Thanks > /Ilias