[2/4] fwu: move meta-data management in core

From: Jassi Brar <jaswinder.singh@linaro.org>

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 <jaswinder.singh@linaro.org>
---
 drivers/fwu-mdata/fwu-mdata-uclass.c |  34 +++++++
 include/fwu.h                        |  41 ++++++++
 lib/fwu_updates/fwu.c                | 136 ++++++++++++++++++++++++++-
 3 files changed, 207 insertions(+), 4 deletions(-)

Hello Jassi,

On Fri, 4 Nov 2022 at 03:43, <jassisinghbrar@gmail.com> wrote:
>
> From: Jassi Brar <jaswinder.singh@linaro.org>
>
> 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 <jaswinder.singh@linaro.org>
> ---
>  drivers/fwu-mdata/fwu-mdata-uclass.c |  34 +++++++
>  include/fwu.h                        |  41 ++++++++
>  lib/fwu_updates/fwu.c                | 136 ++++++++++++++++++++++++++-
>  3 files changed, 207 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c b/drivers/fwu-mdata/fwu-mdata-uclass.c
> index b477e9603f..ded9902971 100644
> --- a/drivers/fwu-mdata/fwu-mdata-uclass.c
> +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c
> @@ -16,6 +16,40 @@
>  #include <linux/types.h>
>  #include <u-boot/crc.h>
>
> +/**
> + * 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);

Nitpicking: would you be ok to rename this function to fwu_get_mdata().
When getting fwu mdata, we obviously expect to get reliable data.

> +
>  /**
>   * 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..3d51b91ed5 100644
> --- a/lib/fwu_updates/fwu.c
> +++ b/lib/fwu_updates/fwu.c
> @@ -15,13 +15,13 @@
>  #include <linux/errno.h>
>  #include <linux/types.h>
>
> +#include <u-boot/crc.h>
> +
> +static struct fwu_mdata g_mdata = { 0 };

Can remove "= { 0 };"

> +static struct udevice *g_dev;
>  static u8 in_trial;
>  static u8 boottime_check;
>
> -#include <linux/errno.h>
> -#include <linux/types.h>
> -#include <u-boot/crc.h>
> -
>  enum {
>         IMAGE_ACCEPT_SET = 1,
>         IMAGE_ACCEPT_CLEAR,
> @@ -161,6 +161,134 @@ 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 = &mdata->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;
> +       }
> +
> +       /* update the cached copy of meta-data */
> +       memcpy(&g_mdata, mdata, sizeof(struct fwu_mdata));
> +
> +       return 0;
> +}
> +
> +static inline int mdata_crc_check(struct fwu_mdata *mdata)
> +{
> +       u32 calc_crc32 = crc32(0, &mdata->version, sizeof(*mdata) - sizeof(u32));

Add an empty line below the above definition.

> +       return calc_crc32 == mdata->crc32 ? 0 : -EINVAL;
> +}
> +
> +/**
> + * 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)
> +{
> +       int err;
> +       bool pri_ok, sec_ok;
> +       struct fwu_mdata s, *p_mdata, *s_mdata;
> +
> +       p_mdata = &g_mdata;
> +       s_mdata = &s;
> +
> +       /* 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.

Note here I would pass straight &g_mdata as argument rather than
p_mdata indirection, for clarity.

> +       if (!err) {
> +               if (mdata)
> +                       memcpy(mdata, p_mdata, sizeof(struct fwu_mdata));
> +               return 0;
> +       }
> +       /* 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, true);
> +               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, false);
> +               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. If not,
> +                * populate the secondary partition from the
> +                * primary partition copy.
> +                */
> +               if (memcmp(p_mdata, s_mdata, sizeof(struct fwu_mdata))) {
> +                       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);

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/

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

       if (!pri_ok && !sec_ok) {
             log_err("No reliable fwu mdata found\n");
             return -EINVAL;
       }


> +       if (err)
> +               log_debug("mdata saving... crc32 failed\n");
> +       else if (mdata)
> +               memcpy(mdata, p_mdata, sizeof(struct fwu_mdata));
> +
> +       return err;
> +}
> +
>  /**
>   * fwu_verify_mdata() - Verify the FWU metadata
>   * @mdata: FWU metadata structure
> --
> 2.25.1
>

On Mon, Nov 7, 2022 at 11:24 AM Etienne Carriere
<etienne.carriere@linaro.org> wrote:
> On Fri, 4 Nov 2022 at 03:43, <jassisinghbrar@gmail.com> 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, &mdata->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 &g_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.

Hello Jassi,

On Mon, 7 Nov 2022 at 19:29, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>
> On Mon, Nov 7, 2022 at 11:24 AM Etienne Carriere
> <etienne.carriere@linaro.org> wrote:
> > On Fri, 4 Nov 2022 at 03:43, <jassisinghbrar@gmail.com> 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.

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

Maybe add an inline comment above the global variable definition.
/* Crc32 of a zeroes produces a non 0 value */

>
> .....
> > > +static inline int mdata_crc_check(struct fwu_mdata *mdata)
> > > +{
> > > +       u32 calc_crc32 = crc32(0, &mdata->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 &g_mdata as argument rather than
> > p_mdata indirection, for clarity.
> >
> Hmm... I wanted to keep the g_mdata usage clear and minimal.

It looks strange we need to check p_mdata init value to understand
we're accessing the cached mdata.
p_mdata is expected to be the primary part. Perfer keep it for this usage.

br,
etienne

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

On Mon, Nov 7, 2022 at 2:13 PM Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Hello Jassi,
>
> On Mon, 7 Nov 2022 at 19:29, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> >
> > On Mon, Nov 7, 2022 at 11:24 AM Etienne Carriere
> > <etienne.carriere@linaro.org> wrote:
> > > On Fri, 4 Nov 2022 at 03:43, <jassisinghbrar@gmail.com> 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.