[PATCH v5 03/23] FWU: Add FWU metadata access driver for GPT partitioned block devices
Sughosh Ganu
sughosh.ganu at linaro.org
Tue Jun 28 12:11:51 CEST 2022
hi Etienne,
On Tue, 21 Jun 2022 at 16:26, Etienne Carriere
<etienne.carriere at linaro.org> wrote:
>
> Hello Sughosh,
>
> On Thu, 9 Jun 2022 at 14:30, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> >
> > In the FWU Multi Bank Update feature, the information about the
> > updatable images is stored as part of the metadata, on a separate
> > partition. Add a driver for reading from and writing to the metadata
> > when the updatable images and the metadata are stored on a block
> > device which is formated with GPT based partition scheme.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > ---
> > drivers/fwu-mdata/Kconfig | 9 +
> > drivers/fwu-mdata/Makefile | 1 +
> > drivers/fwu-mdata/fwu_mdata_gpt_blk.c | 404 ++++++++++++++++++++++++++
> > include/fwu.h | 2 +
> > 4 files changed, 416 insertions(+)
> > create mode 100644 drivers/fwu-mdata/fwu_mdata_gpt_blk.c
> >
> > diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig
> > index d6a21c8e19..d5edef19d6 100644
> > --- a/drivers/fwu-mdata/Kconfig
> > +++ b/drivers/fwu-mdata/Kconfig
> > @@ -5,3 +5,12 @@ config DM_FWU_MDATA
> > 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
> > + help
> > + Enable support for accessing FWU Metadata on GPT partitioned
> > + block devices.
> > diff --git a/drivers/fwu-mdata/Makefile b/drivers/fwu-mdata/Makefile
> > index 7fec7171f4..12a5b4fe04 100644
> > --- a/drivers/fwu-mdata/Makefile
> > +++ b/drivers/fwu-mdata/Makefile
> > @@ -4,3 +4,4 @@
> > #
> >
> > obj-$(CONFIG_DM_FWU_MDATA) += fwu-mdata-uclass.o
> > +obj-$(CONFIG_FWU_MDATA_GPT_BLK) += fwu_mdata_gpt_blk.o
> > diff --git a/drivers/fwu-mdata/fwu_mdata_gpt_blk.c b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c
> > new file mode 100644
> > index 0000000000..329bd3779b
> > --- /dev/null
> > +++ b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c
> > @@ -0,0 +1,404 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2022, Linaro Limited
> > + */
> > +
> > +#include <blk.h>
> > +#include <dm.h>
> > +#include <efi_loader.h>
> > +#include <fwu.h>
> > +#include <fwu_mdata.h>
> > +#include <log.h>
> > +#include <malloc.h>
> > +#include <memalign.h>
> > +#include <part.h>
> > +#include <part_efi.h>
> > +
> > +#include <dm/device-internal.h>
> > +#include <linux/errno.h>
> > +#include <linux/types.h>
> > +#include <u-boot/crc.h>
> > +
> > +#define PRIMARY_PART BIT(0)
> > +#define SECONDARY_PART BIT(1)
> > +#define BOTH_PARTS (PRIMARY_PART | SECONDARY_PART)
> > +
> > +#define MDATA_READ BIT(0)
> > +#define MDATA_WRITE BIT(1)
> > +
<snip>
> > +
> > +static int gpt_get_mdata(struct blk_desc *desc, struct fwu_mdata **mdata)
> > +{
> > + int ret;
> > + u16 primary_mpart = 0, secondary_mpart = 0;
> > +
> > + ret = gpt_get_mdata_partitions(desc, &primary_mpart,
> > + &secondary_mpart);
> > +
> > + if (ret < 0) {
> > + log_err("Error getting the FWU metadata partitions\n");
> > + return -ENODEV;
> > + }
> > +
> > + *mdata = malloc(sizeof(struct fwu_mdata));
> > + if (!*mdata) {
> > + log_err("Unable to allocate memory for reading FWU metadata\n");
> > + return -ENOMEM;
> > + }
> > +
> > + ret = gpt_read_mdata(desc, *mdata, primary_mpart);
> > + if (ret < 0) {
> > + log_err("Failed to read the FWU metadata from the device\n");
> > + return -EIO;
> > + }
> > +
> > + ret = fwu_verify_mdata(*mdata, 1);
> > + if (!ret)
>
> Upon success, I think this function should also either ensure
> secondary_part contains a valid copy of primary part,
> Maybe this function should call gpt_check_mdata_validity() and then
> read mdata content.
Okay
>
> > + return 0;
> > +
> > + /*
> > + * Verification of the primary FWU metadata copy failed.
> > + * Try to read the replica.
> > + */
> > + memset(*mdata, 0, sizeof(struct fwu_mdata));
> > + ret = gpt_read_mdata(desc, *mdata, secondary_mpart);
> > + if (ret < 0) {
> > + log_err("Failed to read the FWU metadata from the device\n");
> > + return -EIO;
> > + }
> > +
> > + ret = fwu_verify_mdata(*mdata, 0);
> > + if (!ret)
> > + return 0;
> > +
> > + /* Both the FWU metadata copies are corrupted. */
> > + return -1;
> > +}
> > +
> > +static int gpt_check_mdata_validity(struct udevice *dev)
> > +{
> > + int ret;
> > + struct blk_desc *desc;
> > + struct fwu_mdata pri_mdata;
> > + struct fwu_mdata secondary_mdata;
> > + u16 primary_mpart = 0, secondary_mpart = 0;
> > + u16 valid_partitions, invalid_partitions;
> > +
> > + desc = dev_get_uclass_plat(dev_get_priv(dev));
> > + if (!desc) {
> > + log_err("Block device not found\n");
> > + return -ENODEV;
> > + }
> > +
> > + /*
> > + * Two FWU metadata partitions are expected.
> > + * If we don't have two, user needs to create
> > + * them first
> > + */
> > + valid_partitions = 0;
> > + ret = gpt_get_mdata_partitions(desc, &primary_mpart,
> > + &secondary_mpart);
> > +
> > + if (ret < 0) {
> > + log_err("Error getting the FWU metadata partitions\n");
> > + return -ENODEV;
> > + }
> > +
> > + ret = gpt_read_mdata(desc, &pri_mdata, primary_mpart);
> > + if (ret < 0) {
> > + log_err("Failed to read the FWU metadata from the device\n");
> > + goto secondary_read;
> > + }
> > +
> > + ret = fwu_verify_mdata(&pri_mdata, 1);
> > + if (!ret)
> > + valid_partitions |= PRIMARY_PART;
> > +
> > +secondary_read:
> > + /* Now check the secondary partition */
> > + ret = gpt_read_mdata(desc, &secondary_mdata, secondary_mpart);
> > + if (ret < 0) {
> > + log_err("Failed to read the FWU metadata from the device\n");
> > + goto mdata_restore;
> > + }
> > +
> > + ret = fwu_verify_mdata(&secondary_mdata, 0);
> > + if (!ret)
> > + valid_partitions |= SECONDARY_PART;
> > +
> > +mdata_restore:
> > + if (valid_partitions == (PRIMARY_PART | SECONDARY_PART)) {
> > + ret = -1;
> > + /*
> > + * Before returning, check that both the
> > + * FWU metadata copies are the same. If not,
> > + * the FWU metadata copies need to be
> > + * re-populated.
> > + */
> > + if (!memcmp(&pri_mdata, &secondary_mdata,
> > + sizeof(struct fwu_mdata))) {
> > + ret = 0;
> > + } else {
> > + log_err("Both FWU metadata copies are valid but do not match. Please check!\n");
>
> I think this function should select one of the part and copies to the
> other, e.g. assume primary is fine, copy primary to secondary and
> return upon success.
In this case, I think there is some kind of an unexpected scenario
which has resulted in both the metadata partitions being valid but not
being the same. Should this not be flagged as an error for the user to
handle? Moreover, which metadata partition do we assume to be the
correct one . We are just calling one partition as primary, and
another one secondary. But the spec does not give any more importance
to one over the other -- the spec just says that both the copies
should be the same.
>
> > + }
> > + goto out;
> > + }
> > +
> > + ret = -1;
> > + if (!(valid_partitions & BOTH_PARTS))
> > + goto out;
> > +
> > + invalid_partitions = valid_partitions ^ BOTH_PARTS;
> > + ret = gpt_write_mdata_partition(desc,
> > + (invalid_partitions == PRIMARY_PART) ?
> > + &secondary_mdata : &pri_mdata,
> > + (invalid_partitions == PRIMARY_PART) ?
> > + primary_mpart : secondary_mpart);
> > +
> > + if (ret < 0)
> > + log_err("Restoring %s FWU metadata partition failed\n",
> > + (invalid_partitions == PRIMARY_PART) ?
> > + "primary" : "secondary");
> > +
> > +out:
> > + return ret;
> > +}
> > +
> > +int fwu_gpt_mdata_check(struct udevice *dev)
>
> Function should be static.
Will change all the relevant functions as per your comment.
>
> > +{
> > + /*
> > + * Check if both the copies of the FWU metadata are
> > + * valid. If one has gone bad, restore it from the
> > + * other good copy.
> > + */
> > + return gpt_check_mdata_validity(dev);
> > +}
> > +
> > +int fwu_gpt_get_mdata(struct udevice *dev, struct fwu_mdata **mdata)
>
> Function should be static.
>
> > +{
> > + struct blk_desc *desc;
> > +
> > + desc = dev_get_uclass_plat(dev_get_priv(dev));
> > + if (!desc) {
> > + log_err("Block device not found\n");
> > + return -ENODEV;
> > + }
> > +
> > + return gpt_get_mdata(desc, mdata);
> > +}
> > +
> > +int fwu_get_mdata_device(struct udevice *dev, struct udevice **mdata_dev)
>
> Function could be static (and declaration removed from fwu.h).
Okay
>
> > +{
> > + u32 phandle;
> > + int ret, size;
> > + struct udevice *parent, *child;
> > + const fdt32_t *phandle_p = NULL;
> > +
> > + phandle_p = ofnode_get_property(dev_ofnode(dev), "fwu-mdata-store",
> > + &size);
>
> Should this be retrieved from driver's ::of_to_plat method?
Patrick has suggested another API for getting the phandle. Thanks.
-sughosh
>
> Br,
> etienne
>
>
> > + if (!phandle_p) {
> > + log_err("fwu-mdata-store property not found\n");
> > + return -ENOENT;
> > + }
> > +
> > + phandle = fdt32_to_cpu(*phandle_p);
> > +
> > + ret = device_get_global_by_ofnode(ofnode_get_by_phandle(phandle),
> > + &parent);
> > + if (ret)
> > + return ret;
> > +
> > + ret = -ENODEV;
> > + for (device_find_first_child(parent, &child); child;
> > + device_find_next_child(&child)) {
> > + if (device_get_uclass_id(child) == UCLASS_BLK) {
> > + *mdata_dev = child;
> > + ret = 0;
> > + }
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int fwu_mdata_gpt_blk_probe(struct udevice *dev)
> > +{
> > + int ret;
> > + struct udevice *mdata_dev = NULL;
> > +
> > + ret = fwu_get_mdata_device(dev, &mdata_dev);
> > + if (ret)
> > + return ret;
> > +
> > + dev_set_priv(dev, mdata_dev);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct fwu_mdata_ops fwu_gpt_blk_ops = {
> > + .mdata_check = fwu_gpt_mdata_check,
> > + .get_mdata = fwu_gpt_get_mdata,
> > + .update_mdata = fwu_gpt_update_mdata,
> > +};
> > +
> > +static const struct udevice_id fwu_mdata_ids[] = {
> > + { .compatible = "u-boot,fwu-mdata-gpt" },
> > + { }
> > +};
> > +
> > +U_BOOT_DRIVER(fwu_mdata_gpt_blk) = {
> > + .name = "fwu-mdata-gpt-blk",
> > + .id = UCLASS_FWU_MDATA,
> > + .of_match = fwu_mdata_ids,
> > + .ops = &fwu_gpt_blk_ops,
> > + .probe = fwu_mdata_gpt_blk_probe,
> > +};
> > diff --git a/include/fwu.h b/include/fwu.h
> > index f9e44e7b39..3b1ee4e83e 100644
> > --- a/include/fwu.h
> > +++ b/include/fwu.h
> > @@ -39,6 +39,8 @@ int fwu_get_active_index(u32 *active_idx);
> > int fwu_update_active_index(u32 active_idx);
> > int fwu_get_image_alt_num(efi_guid_t *image_type_id, u32 update_bank,
> > int *alt_num);
> > +int fwu_get_mdata_device(struct udevice *dev, struct udevice **mdata_dev);
> > +int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part);
> > int fwu_mdata_check(void);
> > int fwu_revert_boot_index(void);
> > int fwu_accept_image(efi_guid_t *img_type_id, u32 bank);
> > --
> > 2.25.1
> >
More information about the U-Boot
mailing list