[PATCH v4 02/11] FWU: Add FWU metadata access driver for GPT partitioned block devices

[Masami Hiramatsu]

2022年2月10日(木) 10:43 Masami Hiramatsu <masami.hiramatsu at linaro.org>:
>
> > > > > Of course as I said in the other thread, I would like to put the
> > > > > dfu_alt_info like information in the devicetree too.
> > > > > (But it maybe different from this discussion)
> > > >
> > > > Just curious, why do you need to define a variable like dfu_alt_info
> > > > in a device tree. If this has already been described earlier, you can
> > > > point me to that discussion. Trying to understand what benefit does
> > > > having the variables defined in a device tree brings. Thanks.
> > >
> > > If we can consolidate the configuration information related to the
> > > firmware layout on the devicetree, it is very easy to understand
> > > and manage the firmware update only by checking the devicetree.
> > > Current design is very fragile from the consistency viewpoint,
> > > because there are 3 different information we are using for FWU,
> > > Kconfig, devicetree and u-boot env-variables. If one of them
> > > sets inconsistent value, FWU may not work as we expected.
> >
> > I get your point. But I think generating the dfu_alt_info at runtime,
> > like how it is done for the ST platforms is in general a better
> > method, as against using a static variable defined in the device tree.
>
> Yeah, the GPT based one is able to store this information on the GPT,
> and it must be a primary definition.
>
> > With runtime generation of the variable, the same code can be used on
> > multiple platforms and can be generated at runtime -- I feel that is
> > better than defining the variable in every platform's device tree.
>
> I don't agree this point at least for non-GPT devices, since the
> firmware storage layout depends on the platform hardware configuration
> statically in such cases.

I changed my mind, it can be solved if we have "uuid" property for
each partition in the devicetree. I will explain later.

> Of course if the device uses GPT to store the firmware, we have to
> follow the GPT layout and FWU Metadata to find the corresponding
> firmware partition.
>
> > Btw, there is also provision to define the variable(or part of it)
> > statically through Kconfig variables. As against your concern about
> > the feature using multiple methods for stating information, it is
> > indeed valid. But I guess we can have documentation explaining how
> > each of that information needs to be defined. Thanks.
>
> Yeah, even using GPT, we need to set correct UUID to the FWU metadata,
> and the metadata depends on Kconfig if we keep putting the #of
> images-per-bank and the #of banks in the Kconfig, and storage

Sorry, I confused. there are "#of images and #of banks per image".

> (controller) is defined in the devicetree.
>
> And I still feel this "chain of definitions" seems a bit fragile. This
> fragile comes from the FWU metadata is not enough self-described (it
> has no the #of images-per-bank and the #of banks, without this
> information we can not decode FWU metadata itself.)
> Anyway, if we can define the #of images-per-bank and the #of banks in
> the devicetree, we don't need to change the binary but just changing
> the devicetree for the different products which has different firmware
> layout. I think that is more flexible.

What I would like to suggest is

/* For GPT BLK backend */
fwu_mdata {
    compatible = "u-boot,fwu-mdata-gpt";
    fwu-mdata-store = <&mmc1>;
    /* No need to specify the mdata partition, because it finds the
mdata by partition type uuid. */
    banks = <2>;
    images-per-bank = <1>;
};

/* For SF backend */
fwu_mdata {
    compatible = "u-boot,fwu-mdata-sf";
    fwu-mdata-store = <&spi-flash0>;
    mdata-offsets = <500000, 520000>; /* Or specified by partition label? */
    banks = <6>;
    images-per-bank = <1>;
};

Note that this is only for the metadata, the real firmware layout issue
still exists. If we can add "uuid" property for the fixed-partitions node
as a additional property, e.g.

spi-flash at 0 {
   partitions {
       compatible = "fixed-partitions";
       ...
       uuid = "aaaaaaaa-bbbb-cccc-dddd-eeeeffffgggg";
       ...
       partition at 600000 {
           label = "Firmware-Bank0";
           reg = <600000, 400000>;
           uuid = "12345678-aaaa-bbbb-cccc-0123456789ab";
       };
       ...
   };
};

Then we can decode the real fwu-mdata and find corresponding
partitions, and able to build dfu_alt_info in runtime.

What would you think?

Thank you,

>
> Thank you,
>
> >
> > -sughosh
> >
> > >
> > > That is my impression felt from porting AB update on the DeveloperBox platform.
> > >
> > > Thank you,
> > >
> > > >
> > > > -sughosh
> > > >
> > > > >
> > > > > Thank you,
> > > > >
> > > > >
> > > > > > +                       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(&mdata_dev);
> > > > > > +       if (ret)
> > > > > > +               return ret;
> > > > > > +
> > > > > > +       dev_set_priv(dev, mdata_dev);
> > > > > > +
> > > > > > +       return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static const struct fwu_mdata_ops fwu_gpt_blk_ops = {
> > > > > > +       .get_image_alt_num = fwu_gpt_get_image_alt_num,
> > > > > > +       .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" },
> > > > >
> > > > > > +       { }
> > > > > > +};
> > > > > > +
> > > > > > +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 5a99c579fc..2c7db2dff9 100644
> > > > > > --- a/include/fwu.h
> > > > > > +++ b/include/fwu.h
> > > > > > @@ -43,6 +43,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 **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.17.1
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Masami Hiramatsu
> > >
> > >
> > >
> > > --
> > > Masami Hiramatsu
>
>
>
> --
> Masami Hiramatsu



--
Masami Hiramatsu