| Message ID | 1cd3161bf51de99990fd5ee2dc896b4defef4f38.1635140590.git.yu.c.chen@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Introduce Platform Firmware Runtime Update and Telemetry drivers | expand |
On Mon, Oct 25, 2021 at 02:25:04PM +0800, Chen Yu wrote: > Platform Firmware Runtime Update image starts with UEFI headers, and the > headers are defined in UEFI specification, but some of them have not been > defined in the kernel yet. > > For example, the header layout of a capsule file looks like this: > > EFI_CAPSULE_HEADER > EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER > EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER > EFI_FIRMWARE_IMAGE_AUTHENTICATION > > These structures would be used by the Platform Firmware Runtime Update > driver to parse the format of capsule file to verify if the corresponding > version number is valid. The EFI_CAPSULE_HEADER has been defined in the > kernel, however the rest are not, thus introduce corresponding UEFI > structures accordingly. Besides, EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER > and EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER need not be aligned and > so the corresponding data types should be packed. > > Signed-off-by: Chen Yu <yu.c.chen@intel.com> > --- > v6: No change since v5. > v5: No change since v4. > v4: Revise the commit log to make it more clear. (Rafael J. Wysocki) > --- > include/linux/efi.h | 50 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 50 insertions(+) > > diff --git a/include/linux/efi.h b/include/linux/efi.h > index 6b5d36babfcc..19ff834e1388 100644 > --- a/include/linux/efi.h > +++ b/include/linux/efi.h > @@ -148,6 +148,56 @@ typedef struct { > u32 imagesize; > } efi_capsule_header_t; > > +#pragma pack(1) Why is this pragma suddenly needed now in this file? If you really need this for a specific structure, use the "__packed" attribute please. thanks, greg k-h
On Mon, Oct 25, 2021 at 08:44:00AM +0200, Greg Kroah-Hartman wrote: > On Mon, Oct 25, 2021 at 02:25:04PM +0800, Chen Yu wrote: > > Platform Firmware Runtime Update image starts with UEFI headers, and the > > headers are defined in UEFI specification, but some of them have not been > > defined in the kernel yet. > > > > For example, the header layout of a capsule file looks like this: > > > > EFI_CAPSULE_HEADER > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER > > EFI_FIRMWARE_IMAGE_AUTHENTICATION > > > > These structures would be used by the Platform Firmware Runtime Update > > driver to parse the format of capsule file to verify if the corresponding > > version number is valid. The EFI_CAPSULE_HEADER has been defined in the > > kernel, however the rest are not, thus introduce corresponding UEFI > > structures accordingly. Besides, EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER > > and EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER need not be aligned and > > so the corresponding data types should be packed. > > > > Signed-off-by: Chen Yu <yu.c.chen@intel.com> > > --- > > v6: No change since v5. > > v5: No change since v4. > > v4: Revise the commit log to make it more clear. (Rafael J. Wysocki) > > --- > > include/linux/efi.h | 50 +++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 50 insertions(+) > > > > diff --git a/include/linux/efi.h b/include/linux/efi.h > > index 6b5d36babfcc..19ff834e1388 100644 > > --- a/include/linux/efi.h > > +++ b/include/linux/efi.h > > @@ -148,6 +148,56 @@ typedef struct { > > u32 imagesize; > > } efi_capsule_header_t; > > > > +#pragma pack(1) > > Why is this pragma suddenly needed now in this file? > > If you really need this for a specific structure, use the "__packed" > attribute please. > These two structures are required to be packed in the uefi spec, I'll change them to "__packed". thanks, Chenyu > thanks, > > greg k-h
On Mon, Oct 25, 2021 at 07:45:19PM +0800, Chen Yu wrote: > On Mon, Oct 25, 2021 at 08:44:00AM +0200, Greg Kroah-Hartman wrote: > > On Mon, Oct 25, 2021 at 02:25:04PM +0800, Chen Yu wrote: > > > Platform Firmware Runtime Update image starts with UEFI headers, and the > > > headers are defined in UEFI specification, but some of them have not been > > > defined in the kernel yet. > > > > > > For example, the header layout of a capsule file looks like this: > > > > > > EFI_CAPSULE_HEADER > > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER > > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER > > > EFI_FIRMWARE_IMAGE_AUTHENTICATION > > > > > > These structures would be used by the Platform Firmware Runtime Update > > > driver to parse the format of capsule file to verify if the corresponding > > > version number is valid. The EFI_CAPSULE_HEADER has been defined in the > > > kernel, however the rest are not, thus introduce corresponding UEFI > > > structures accordingly. Besides, EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER > > > and EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER need not be aligned and > > > so the corresponding data types should be packed. > > > > > > Signed-off-by: Chen Yu <yu.c.chen@intel.com> > > > --- > > > v6: No change since v5. > > > v5: No change since v4. > > > v4: Revise the commit log to make it more clear. (Rafael J. Wysocki) > > > --- > > > include/linux/efi.h | 50 +++++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 50 insertions(+) > > > > > > diff --git a/include/linux/efi.h b/include/linux/efi.h > > > index 6b5d36babfcc..19ff834e1388 100644 > > > --- a/include/linux/efi.h > > > +++ b/include/linux/efi.h > > > @@ -148,6 +148,56 @@ typedef struct { > > > u32 imagesize; > > > } efi_capsule_header_t; > > > > > > +#pragma pack(1) > > > > Why is this pragma suddenly needed now in this file? > > > > If you really need this for a specific structure, use the "__packed" > > attribute please. > > > These two structures are required to be packed in the uefi spec, I'll change > them to "__packed". And they are the _only_ ones in this .h file that require this? I would think that they all require this. thanks, greg k-h
On Mon, Oct 25, 2021 at 02:02:24PM +0200, Greg Kroah-Hartman wrote: > On Mon, Oct 25, 2021 at 07:45:19PM +0800, Chen Yu wrote: > > On Mon, Oct 25, 2021 at 08:44:00AM +0200, Greg Kroah-Hartman wrote: > > > On Mon, Oct 25, 2021 at 02:25:04PM +0800, Chen Yu wrote: > > > > Platform Firmware Runtime Update image starts with UEFI headers, and the > > > > headers are defined in UEFI specification, but some of them have not been > > > > defined in the kernel yet. > > > > > > > > For example, the header layout of a capsule file looks like this: > > > > > > > > EFI_CAPSULE_HEADER > > > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER > > > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER > > > > EFI_FIRMWARE_IMAGE_AUTHENTICATION > > > > > > > > These structures would be used by the Platform Firmware Runtime Update > > > > driver to parse the format of capsule file to verify if the corresponding > > > > version number is valid. The EFI_CAPSULE_HEADER has been defined in the > > > > kernel, however the rest are not, thus introduce corresponding UEFI > > > > structures accordingly. Besides, EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER > > > > and EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER need not be aligned and > > > > so the corresponding data types should be packed. > > > > > > > > Signed-off-by: Chen Yu <yu.c.chen@intel.com> > > > > --- > > > > v6: No change since v5. > > > > v5: No change since v4. > > > > v4: Revise the commit log to make it more clear. (Rafael J. Wysocki) > > > > --- > > > > include/linux/efi.h | 50 +++++++++++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 50 insertions(+) > > > > > > > > diff --git a/include/linux/efi.h b/include/linux/efi.h > > > > index 6b5d36babfcc..19ff834e1388 100644 > > > > --- a/include/linux/efi.h > > > > +++ b/include/linux/efi.h > > > > @@ -148,6 +148,56 @@ typedef struct { > > > > u32 imagesize; > > > > } efi_capsule_header_t; > > > > > > > > +#pragma pack(1) > > > > > > Why is this pragma suddenly needed now in this file? > > > > > > If you really need this for a specific structure, use the "__packed" > > > attribute please. > > > > > These two structures are required to be packed in the uefi spec, I'll change > > them to "__packed". > > And they are the _only_ ones in this .h file that require this? I would > think that they all require this. > I did a search in the uefi specification, and found 42 pack(1) structures, while the other structures do not have pack(1) attribute. It seems to me that whether the structures are required to be strictly packed depends on the use case. Here's my understanding and I might be wrong: In this patch, according to the skeleton of capsule file described in [Figure 23-6 Firmware Management and Firmware Image Management headers] in the uefi spec [1], the two structures are located at the beginning of the capsule file, and followed by real payload. If these structure are packed then the the adjacent binary payload could start on byte boundary without padding, which might save space for capsule file. For those structures that do not have strict pack requirement, the uefi spec does not force to pack them. Link: https://uefi.org/sites/default/files/resources/UEFI_Spec_2_9_2021_03_18.pdf # [1] thanks, Chenyu > thanks, > > greg k-h
On Mon, 25 Oct 2021 at 14:47, Chen Yu <yu.c.chen@intel.com> wrote: > > On Mon, Oct 25, 2021 at 02:02:24PM +0200, Greg Kroah-Hartman wrote: > > On Mon, Oct 25, 2021 at 07:45:19PM +0800, Chen Yu wrote: > > > On Mon, Oct 25, 2021 at 08:44:00AM +0200, Greg Kroah-Hartman wrote: > > > > On Mon, Oct 25, 2021 at 02:25:04PM +0800, Chen Yu wrote: > > > > > Platform Firmware Runtime Update image starts with UEFI headers, and the > > > > > headers are defined in UEFI specification, but some of them have not been > > > > > defined in the kernel yet. > > > > > > > > > > For example, the header layout of a capsule file looks like this: > > > > > > > > > > EFI_CAPSULE_HEADER > > > > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER > > > > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER > > > > > EFI_FIRMWARE_IMAGE_AUTHENTICATION > > > > > > > > > > These structures would be used by the Platform Firmware Runtime Update > > > > > driver to parse the format of capsule file to verify if the corresponding > > > > > version number is valid. The EFI_CAPSULE_HEADER has been defined in the > > > > > kernel, however the rest are not, thus introduce corresponding UEFI > > > > > structures accordingly. Besides, EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER > > > > > and EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER need not be aligned and > > > > > so the corresponding data types should be packed. > > > > > > > > > > Signed-off-by: Chen Yu <yu.c.chen@intel.com> > > > > > --- > > > > > v6: No change since v5. > > > > > v5: No change since v4. > > > > > v4: Revise the commit log to make it more clear. (Rafael J. Wysocki) > > > > > --- > > > > > include/linux/efi.h | 50 +++++++++++++++++++++++++++++++++++++++++++++ > > > > > 1 file changed, 50 insertions(+) > > > > > > > > > > diff --git a/include/linux/efi.h b/include/linux/efi.h > > > > > index 6b5d36babfcc..19ff834e1388 100644 > > > > > --- a/include/linux/efi.h > > > > > +++ b/include/linux/efi.h > > > > > @@ -148,6 +148,56 @@ typedef struct { > > > > > u32 imagesize; > > > > > } efi_capsule_header_t; > > > > > > > > > > +#pragma pack(1) > > > > > > > > Why is this pragma suddenly needed now in this file? > > > > > > > > If you really need this for a specific structure, use the "__packed" > > > > attribute please. > > > > > > > These two structures are required to be packed in the uefi spec, I'll change > > > them to "__packed". > > > > And they are the _only_ ones in this .h file that require this? I would > > think that they all require this. > > > I did a search in the uefi specification, and found 42 pack(1) structures, > while the other structures do not have pack(1) attribute. > > It seems to me that whether the structures are required to be strictly packed > depends on the use case. Here's my understanding and I might be wrong: In this > patch, according to the skeleton of capsule file described in > [Figure 23-6 Firmware Management and Firmware Image Management headers] > in the uefi spec [1], the two structures are located at the beginning of > the capsule file, and followed by real payload. If these structure are packed > then the the adjacent binary payload could start on byte boundary without > padding, which might save space for capsule file. > Packing only affects internal padding, and a struct's size is never padded to be a multiple of its alignment (which equals the largest alignment of all its members). This of course assumes that you don't abuse array indexing as a sizeof() operator. However, the __packed attribute does indicate to the compiler that the entire thing can appear misaligned in memory. So if one follows the other in the capsule header, the __packed attribute may be appropriate to ensure that the second one is not accessed using misaligned loads and stores. And then there is of course the ambiguity in alignment of uint64_t on x86, which could be either 4 or 8 bytes depending on the context (and UEFI targets all of them). So __packed may be used to disambiguate between those if a uint64_t field appears on a boundary whose offset % 8 == 4. So please use __packed rather than the pragma(), and apply it wherever it is applied in the spec. > For those structures that do not have strict pack requirement, the uefi spec > does not force to pack them. > > Link: https://uefi.org/sites/default/files/resources/UEFI_Spec_2_9_2021_03_18.pdf # [1] > > thanks, > Chenyu > > > > thanks, > > > > greg k-h
On Mon, Oct 25, 2021 at 03:11:57PM +0200, Ard Biesheuvel wrote: > On Mon, 25 Oct 2021 at 14:47, Chen Yu <yu.c.chen@intel.com> wrote: > > > > On Mon, Oct 25, 2021 at 02:02:24PM +0200, Greg Kroah-Hartman wrote: > > > On Mon, Oct 25, 2021 at 07:45:19PM +0800, Chen Yu wrote: > > > > On Mon, Oct 25, 2021 at 08:44:00AM +0200, Greg Kroah-Hartman wrote: > > > > > On Mon, Oct 25, 2021 at 02:25:04PM +0800, Chen Yu wrote: > > > > > > Platform Firmware Runtime Update image starts with UEFI headers, and the > > > > > > headers are defined in UEFI specification, but some of them have not been > > > > > > defined in the kernel yet. > > > > > > > > > > > > For example, the header layout of a capsule file looks like this: > > > > > > > > > > > > EFI_CAPSULE_HEADER > > > > > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER > > > > > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER > > > > > > EFI_FIRMWARE_IMAGE_AUTHENTICATION > > > > > > > > > > > > These structures would be used by the Platform Firmware Runtime Update > > > > > > driver to parse the format of capsule file to verify if the corresponding > > > > > > version number is valid. The EFI_CAPSULE_HEADER has been defined in the > > > > > > kernel, however the rest are not, thus introduce corresponding UEFI > > > > > > structures accordingly. Besides, EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER > > > > > > and EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER need not be aligned and > > > > > > so the corresponding data types should be packed. > > > > > > > > > > > > Signed-off-by: Chen Yu <yu.c.chen@intel.com> > > > > > > --- > > > > > > v6: No change since v5. > > > > > > v5: No change since v4. > > > > > > v4: Revise the commit log to make it more clear. (Rafael J. Wysocki) > > > > > > --- > > > > > > include/linux/efi.h | 50 +++++++++++++++++++++++++++++++++++++++++++++ > > > > > > 1 file changed, 50 insertions(+) > > > > > > > > > > > > diff --git a/include/linux/efi.h b/include/linux/efi.h > > > > > > index 6b5d36babfcc..19ff834e1388 100644 > > > > > > --- a/include/linux/efi.h > > > > > > +++ b/include/linux/efi.h > > > > > > @@ -148,6 +148,56 @@ typedef struct { > > > > > > u32 imagesize; > > > > > > } efi_capsule_header_t; > > > > > > > > > > > > +#pragma pack(1) > > > > > > > > > > Why is this pragma suddenly needed now in this file? > > > > > > > > > > If you really need this for a specific structure, use the "__packed" > > > > > attribute please. > > > > > > > > > These two structures are required to be packed in the uefi spec, I'll change > > > > them to "__packed". > > > > > > And they are the _only_ ones in this .h file that require this? I would > > > think that they all require this. > > > > > I did a search in the uefi specification, and found 42 pack(1) structures, > > while the other structures do not have pack(1) attribute. > > > > It seems to me that whether the structures are required to be strictly packed > > depends on the use case. Here's my understanding and I might be wrong: In this > > patch, according to the skeleton of capsule file described in > > [Figure 23-6 Firmware Management and Firmware Image Management headers] > > in the uefi spec [1], the two structures are located at the beginning of > > the capsule file, and followed by real payload. If these structure are packed > > then the the adjacent binary payload could start on byte boundary without > > padding, which might save space for capsule file. > > > > Packing only affects internal padding, and a struct's size is never > padded to be a multiple of its alignment (which equals the largest > alignment of all its members). This of course assumes that you don't > abuse array indexing as a sizeof() operator. > > However, the __packed attribute does indicate to the compiler that the > entire thing can appear misaligned in memory. So if one follows the > other in the capsule header, the __packed attribute may be appropriate > to ensure that the second one is not accessed using misaligned loads > and stores. > > And then there is of course the ambiguity in alignment of uint64_t on > x86, which could be either 4 or 8 bytes depending on the context (and > UEFI targets all of them). So __packed may be used to disambiguate > between those if a uint64_t field appears on a boundary whose offset % > 8 == 4. > > So please use __packed rather than the pragma(), and apply it wherever > it is applied in the spec. > Thanks for the explaination in detail! Ard. Will do in next version. Thanks, Chenyu
diff --git a/include/linux/efi.h b/include/linux/efi.h index 6b5d36babfcc..19ff834e1388 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -148,6 +148,56 @@ typedef struct { u32 imagesize; } efi_capsule_header_t; +#pragma pack(1) + +/* EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER */ +typedef struct { + u32 ver; + u16 emb_drv_cnt; + u16 payload_cnt; + /* + * Variable array indicated by number of + * (emb_drv_cnt + payload_cnt) + */ + u64 offset_list[]; +} efi_manage_capsule_header_t; + +/* EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER */ +typedef struct { + u32 ver; + guid_t image_type_id; + u8 image_index; + u8 reserved_bytes[3]; + u32 image_size; + u32 vendor_code_size; + /* ver = 2. */ + u64 hw_ins; + /* ver = v3. */ + u64 capsule_support; +} efi_manage_capsule_image_header_t; + +#pragma pack() + +/* WIN_CERTIFICATE */ +typedef struct { + u32 len; + u16 rev; + u16 cert_type; +} win_cert_t; + +/* WIN_CERTIFICATE_UEFI_GUID */ +typedef struct { + win_cert_t hdr; + guid_t cert_type; + u8 cert_data[]; +} win_cert_uefi_guid_t; + +/* EFI_FIRMWARE_IMAGE_AUTHENTICATIO */ +typedef struct { + u64 mon_count; + win_cert_uefi_guid_t auth_info; +} efi_image_auth_t; + /* * EFI capsule flags */
Platform Firmware Runtime Update image starts with UEFI headers, and the headers are defined in UEFI specification, but some of them have not been defined in the kernel yet. For example, the header layout of a capsule file looks like this: EFI_CAPSULE_HEADER EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER EFI_FIRMWARE_IMAGE_AUTHENTICATION These structures would be used by the Platform Firmware Runtime Update driver to parse the format of capsule file to verify if the corresponding version number is valid. The EFI_CAPSULE_HEADER has been defined in the kernel, however the rest are not, thus introduce corresponding UEFI structures accordingly. Besides, EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER and EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER need not be aligned and so the corresponding data types should be packed. Signed-off-by: Chen Yu <yu.c.chen@intel.com> --- v6: No change since v5. v5: No change since v4. v4: Revise the commit log to make it more clear. (Rafael J. Wysocki) --- include/linux/efi.h | 50 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)