| Message ID | 20230502131200.2551513-2-rui.silva@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | corstone1000: fwu metadata and GPT | expand |
On Tue, 2 May 2023 at 16:12, Rui Miguel Silva <rui.silva@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.silva@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; > > /** > * 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; > > /** > * 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; > > #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.apalodimas@linaro.org>
On 5/3/23 10:06, Ilias Apalodimas wrote: > On Tue, 2 May 2023 at 16:12, Rui Miguel Silva <rui.silva@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.silva@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? >> >> /** >> * 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 >> >> /** >> * 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. 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.apalodimas@linaro.org>
Hi Heinrich, thanks for the review, Heinrich Schuchardt <xypron.glpk@gmx.de> writes: > On 5/3/23 10:06, Ilias Apalodimas wrote: >> On Tue, 2 May 2023 at 16:12, Rui Miguel Silva <rui.silva@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.silva@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? 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. 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.apalodimas@linaro.org>
Hi, On Mon, 29 May 2023 at 16:12, Rui Miguel Silva <rui.silva@linaro.org> wrote: > > Hi Heinrich, > thanks for the review, > Heinrich Schuchardt <xypron.glpk@gmx.de> writes: > > > On 5/3/23 10:06, Ilias Apalodimas wrote: > >> On Tue, 2 May 2023 at 16:12, Rui Miguel Silva <rui.silva@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.silva@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.apalodimas@linaro.org>
Ilias Apalodimas <ilias.apalodimas@linaro.org> writes: > Hi, > > On Mon, 29 May 2023 at 16:12, Rui Miguel Silva <rui.silva@linaro.org> wrote: >> >> Hi Heinrich, >> thanks for the review, >> Heinrich Schuchardt <xypron.glpk@gmx.de> writes: >> >> > On 5/3/23 10:06, Ilias Apalodimas wrote: >> >> On Tue, 2 May 2023 at 16:12, Rui Miguel Silva <rui.silva@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.silva@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, Sorry, was looking at an relative old branch. (only with this: 1dd705cf99 efi: use 32-bit alignment for efi_guid_t in v2023.04 it changed to 4 aligned) and, please note, that there is a definition of efi_guid_t as aligned 8 in eficapsule tool: https://elixir.bootlin.com/u-boot/latest/source/tools/eficapsule.h#L27 > >> 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. Agree. Thanks for the feedback. Cheers, Rui > > 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.apalodimas@linaro.org>
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; /** * 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; /** * 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; #endif /* _FWU_MDATA_H_ */
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.silva@linaro.org> --- include/fwu_mdata.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)