Hi, On Mon, 29 May 2023 at 16:12, Rui Miguel Silva <rui.si...@linaro.org> wrote: > > Hi Heinrich, > thanks for the review, > Heinrich Schuchardt <xypron.g...@gmx.de> writes: > > > On 5/3/23 10:06, Ilias Apalodimas wrote: > >> On Tue, 2 May 2023 at 16:12, Rui Miguel Silva <rui.si...@linaro.org> wrote: > >> > >> Hi Rui, > >>> > >>> The fwu metadata in the metadata partitions > >>> should/are packed to guarantee that the info is > >>> correct in all platforms. Also the size of them > >>> are used to calculate the crc32 and that is important > >>> to get it right. > >>> > >>> Signed-off-by: Rui Miguel Silva <rui.si...@linaro.org> > >>> --- > >>> include/fwu_mdata.h | 6 +++--- > >>> 1 file changed, 3 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h > >>> index 8fda4f4ac225..c61221a91735 100644 > >>> --- a/include/fwu_mdata.h > >>> +++ b/include/fwu_mdata.h > >>> @@ -22,7 +22,7 @@ struct fwu_image_bank_info { > >>> efi_guid_t image_uuid; > >>> uint32_t accepted; > >>> uint32_t reserved; > >>> -}; > >>> +} __packed; > > > > This structure is naturally packed. Why should adding a __packed > > attribute make a difference? > > Agree. > > > > >>> > >>> /** > >>> * struct fwu_image_entry - information for a particular type of image > >>> @@ -38,7 +38,7 @@ struct fwu_image_entry { > >>> efi_guid_t image_type_uuid; > >>> efi_guid_t location_uuid; > >>> struct fwu_image_bank_info img_bank_info[CONFIG_FWU_NUM_BANKS]; > >>> -}; > >>> +} __packed; > > > > ditto > > Agree. > > > > >>> > >>> /** > >>> * struct fwu_mdata - FWU metadata structure for multi-bank updates > >>> @@ -62,6 +62,6 @@ struct fwu_mdata { > >>> uint32_t previous_active_index; > >>> > >>> struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK]; > >>> -}; > >>> +} __packed; > > > > Depending on the alignment of efi_guid_t __packed might make a > > difference here. But as efi_guid_t is defined as 4 aligned I can't see > > an impact here either. > > but efi_guid_t is defined as 8 aligned, right?
No, we have it as 4byte aligned, > Even though, I think that this type of data written to disk/flash > without guaranteeing (trusting natural picketing) packed and endianness > is never a good idea. I think we should overall define these as packed regardless. The spec might change at some point and add a few fields ane even though it doesn't explicitly requires packing it only mentions offsets. This is going to make a difference in code generation, but it's in a path noone cares about speed. Thanks /Ilias > > Cheers, > Rui > > > > > Best regards > > > > Heinrich > > > >>> > >>> #endif /* _FWU_MDATA_H_ */ > >>> -- > >>> 2.40.0 > >>> > >> > >> Yep, that's a good catch. The spec defines 'just' the offsets and not > >> the C representation, so those should be indeed packed. > >> > >> Reviewed-by: Ilias Apalodimas <ilias.apalodi...@linaro.org>