Hi Etienne,
On Wed, 8 Dec 2021 at 07:18, Etienne Carriere etienne.carriere@linaro.org wrote:
Hi Sughosh, Ilias (and all),
On Thu, 2 Dec 2021 at 08:43, Sughosh Ganu sughosh.ganu@linaro.org wrote:
hi Ilias,
On Wed, 1 Dec 2021 at 17:56, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Sughosh,
On Thu, Nov 25, 2021 at 12:42:56PM +0530, Sughosh Ganu 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 functions 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@linaro.org
lib/fwu_updates/fwu_metadata_gpt_blk.c | 716 +++++++++++++++++++++++++ 1 file changed, 716 insertions(+) create mode 100644 lib/fwu_updates/fwu_metadata_gpt_blk.c
+#define SECONDARY_VALID 0x2
Change those to BIT(0), BIT(1) etc please
Will change.
+#define MDATA_READ (u8)0x1 +#define MDATA_WRITE (u8)0x2
+#define IMAGE_ACCEPT_SET (u8)0x1 +#define IMAGE_ACCEPT_CLEAR (u8)0x2
+static int gpt_verify_metadata(struct fwu_metadata *metadata, bool
pri_part)
+{
u32 calc_crc32;void *buf;buf = &metadata->version;calc_crc32 = crc32(0, buf, sizeof(*metadata) - sizeof(u32));if (calc_crc32 != metadata->crc32) {log_err("crc32 check failed for %s metadata partition\n",pri_part ? "primary" : "secondary");I think we can rid of the 'bool pri_part'. It's only used for printing and you can have more that 2 partitions anyway right?
We will have only two partitions for the metadata. But i think looking at it now, it is a bit fuzzy on which is the primary metadata partition and which is the secondary one. Can we instead print the UniquePartitionGUID of the metadata partition instead. That will help in identifying for which metadata copy the verification failed. Let me know your thoughts on this.
return -1;}return 0;+}
+static int gpt_get_metadata_partitions(struct blk_desc *desc,
Please add a function descriptions (on following functions as well)
I have added the function descriptions in the fwu_metadata.c, where the api's are being defined. Do we need to add the descriptions for the functions in this file as well?
u32 *primary_mpart,u32 *secondary_mpart)u16 maybe? This is the number of gpt partitions with metadata right?
Okay.
+{
int i, ret;u32 nparts, mparts;I think the 2 variables labels are too similar, it's a source of confusion. One is a number of entries, the second is a counter. It would be nice it's a bit more explicit.
gpt_entry *gpt_pte = NULL;const efi_guid_t fwu_metadata_guid = FWU_METADATA_GUID;ALLOC_CACHE_ALIGN_BUFFER_PAD(gpt_header, gpt_head, 1,desc->blksz);ret = get_gpt_hdr_parts(desc, gpt_head, &gpt_pte);if (ret < 0) {log_err("Error getting GPT header and partitions\n");ret = -EIO;goto out;}nparts = gpt_head->num_partition_entries;for (i = 0, mparts = 0; i < nparts; i++) {if (!guidcmp(&fwu_metadata_guid,&gpt_pte[i].partition_type_guid)) {++mparts;if (!*primary_mpart)*primary_mpart = i + 1;else*secondary_mpart = i + 1;}}if (mparts != 2) {log_err("Expect two copies of the metadata instead of%d\n",
mparts);ret = -EINVAL;} else {ret = 0;}+out:
free(gpt_pte);return ret;+}
+static int gpt_get_metadata_disk_part(struct blk_desc *desc,
struct disk_partition *info,u32 part_num)+{
int ret;char *metadata_guid_str = "8a7a84a0-8387-40f6-ab41-a8b9a5a60d23";ret = part_get_info(desc, part_num, info);if (ret < 0) {log_err("Unable to get the partition info for the metadatapart %d",
part_num);return -1;}/* Check that it is indeed the metadata partition */if (!strncmp(info->type_guid, metadata_guid_str, UUID_STR_LEN)) {/* Found the metadata partition */return 0;}return -1;+}
+static int gpt_read_write_metadata(struct blk_desc *desc,
struct fwu_metadata *metadata,u8 access, u32 part_num)+{
int ret;u32 len, blk_start, blkcnt;struct disk_partition info;ALLOC_CACHE_ALIGN_BUFFER_PAD(struct fwu_metadata, mdata, 1,desc->blksz);
ret = gpt_get_metadata_disk_part(desc, &info, part_num);if (ret < 0) {printf("Unable to get the metadata partition\n");return -ENODEV;}len = sizeof(*metadata);blkcnt = BLOCK_CNT(len, desc);if (blkcnt > info.size) {log_err("Block count exceeds metadata partition size\n");return -ERANGE;}blk_start = info.start;if (access == MDATA_READ) {if (blk_dread(desc, blk_start, blkcnt, mdata) != blkcnt) {log_err("Error reading metadata from thedevice\n");
return -EIO;}memcpy(metadata, mdata, sizeof(struct fwu_metadata));} else {if (blk_dwrite(desc, blk_start, blkcnt, metadata) !=blkcnt) {
log_err("Error writing metadata to the device\n");return -EIO;}}return 0;+}
+static int gpt_read_metadata(struct blk_desc *desc,
struct fwu_metadata *metadata, u32 part_num)+{
return gpt_read_write_metadata(desc, metadata, MDATA_READ,part_num);
+}
+static int gpt_write_metadata_partition(struct blk_desc *desc,
struct fwu_metadata *metadata,u32 part_num)+{
return gpt_read_write_metadata(desc, metadata, MDATA_WRITE,part_num);
+}
+static int gpt_update_metadata(struct fwu_metadata *metadata) +{
int ret;struct blk_desc *desc;u32 primary_mpart, secondary_mpart;ret = fwu_plat_get_blk_desc(&desc);if (ret < 0) {log_err("Block device not found\n");return -ENODEV;}primary_mpart = secondary_mpart = 0;ret = gpt_get_metadata_partitions(desc, &primary_mpart,&secondary_mpart);if (ret < 0) {log_err("Error getting the metadata partitions\n");return -ENODEV;}/* First write the primary partition*/ret = gpt_write_metadata_partition(desc, metadata, primary_mpart);if (ret < 0) {log_err("Updating primary metadata partition failed\n");return ret;}/* And now the replica */ret = gpt_write_metadata_partition(desc, metadata,secondary_mpart);
if (ret < 0) {log_err("Updating secondary metadata partition failed\n");return ret;}So shouldn't we do something about this case? The first partition was correctly written and the second failed. Now if the primary GPT somehow gets corrupted the device is now unusable.
The replica is not there to overcome bitflips of the storage media. It's here to allow updates while reliable info a still available in the counter part. The scheme could be to rely on only 1 instance of the fwu-metadata (sorry Simon) image is valid. A first load: load 1st instance, crap the second. At update: find the crapped one: write it with new data. Upon success crapped the alternate one. This is a suggestion. There are many ways to handle that.
For sure, the scheme should be well defined so that the boot stage that read fwu-data complies with the scheme used to write them.
I assume that depending on what happened to the firmware rollback counter, we could either try to rewrite this, or revert the first one as well (assuming rollback counters allow that).
Rollback counters should protect image version management, not metadata updates (imho).
Okay, although this might be indicative of some underlying hardware issue with the device, else this scenario should not play out.
return 0;+}
+static int gpt_get_valid_metadata(struct fwu_metadata **metadata)
Could be renamed gpt_get_metadata(), we don't expect to get invalid data :)
How about gpt_get_mdata() ?
When I read this I thought it was getting the GPT table...'metadata' is just too generic.
Regards, Simon