Introduce Platform Firmware Runtime Update and Telemetry drivers
Subsystems (4)
PATCH v8 1/4 efi: Introduce EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER and corresponding structures
In reply to Chen YuPlatform 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_HEADEREFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADEREFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADEREFI_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 are required to be packed in the uefi specification. Ard has pointed out that, 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.
---
v8: Use efi_guid_t instead of guid_t. (Andy Shevchenko)
v7: Use __packed instead of pragma pack(1). (Greg Kroah-Hartman, Ard Biesheuve)
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 | 46 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)
include/linux/efi.h +46 -0
@@ -148,6 +148,52 @@ typedef struct {u32 imagesize; } efi_capsule_header_t; +/* EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER */ +struct efi_manage_capsule_header { +u32 ver; +u16 emb_drv_cnt; +u16 payload_cnt; +/* +* Variable array indicated by number of +* (emb_drv_cnt + payload_cnt) +*/ +u64 offset_list[]; +} __packed; + +/* EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER */ +struct efi_manage_capsule_image_header { +u32 ver; +efi_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; +} __packed; + +/* WIN_CERTIFICATE */ +struct win_cert { +u32 len; +u16 rev; +u16 cert_type; +}; + +/* WIN_CERTIFICATE_UEFI_GUID */ +struct win_cert_uefi_guid { +struct win_cert hdr; +efi_guid_t cert_type; +u8 cert_data[]; +}; + +/* EFI_FIRMWARE_IMAGE_AUTHENTICATIO */ +struct efi_image_auth { +u64 mon_count; +struct win_cert_uefi_guid auth_info; +}; +/* * EFI capsule flags */
Signature · 1 line
2.25.1
v8 cover Introduce Platform Firmware Runtime Update and Telemetry drivers
The PFRU (Platform Firmware Runtime Update) kernel interface is designed to interact with the platform firmware interface defined in the `Management Mode Firmware Runtime Update
<https://uefi.org/sites/default/files/resources/Intel_MM_OS_Interface_Spec_Rev100.pdf>`
specification. The primary function of PFRU is to carry out runtime updates of the platform firmware, which doesn't require the system to be restarted. It also allows telemetry data to be retrieved from the platform firmware.
=============
- Change from v7 to v8:
- Remove the variable-length array in struct pfru_update_cap_info, and copy the non-variable-length struct pfru_update_cap_info to userspace directly. (Greg Kroah-Hartman)
- Use efi_guid_t instead of guid_t when parsing capsule file. (Andy Shevchenko)
- Change the type of rev_id from int to u32, because this data will be copied between kernel and userspace. (Greg Kroah-Hartman)
- Add a prefix for dev in struct pfru_device to parent_dev, so as to indicate that this filed is the parent of the created miscdev. (Greg Kroah-Hartman)
- Use blank lines between different macro sections. (Greg Kroah-Hartman) Illusatrate the possible errno for each ioctl interface.
(Greg Kroah-Hartman)
- Remove pfru_valid_revid() from uapi header to avoid poluting the global namespace.(Greg Kroah-Hartman)
- Assign the value to the enum type explicitly.(Greg Kroah-Hartman)
- Change the guid_t to efi_guid_t when parsing image header in get_image_type() (Greg Kroah-Hartman)
- Remove the void * to other type casting in valid_version(). (Andy Shevchenko)
- Combined the assignment of variables with definitions. (Andy Shevchenko)
- Define this magic for revision ID. (Andy Shevchenko)
- Make the labeling consistent for error handling. (Andy Shevchenko)
- Replace the UUID_SIZE in uapi with 16 directly. (Andy Shevchenko)
- Add blank line between generic include header and uapi header. (Andy Shevchenko)
- Arrange the order between devm_kzalloc() and normal allocation in acpi_pfru_probe() that, the former should always be ahead of the latter. (Andy Shevchenko)
- Change from v6 to v7:
- Use __packed instead of pragma pack(1).
(Greg Kroah-Hartman, Ard Biesheuve)
- Use ida_alloc() to allocate a ID, and release the ID when
device is removed. (Greg Kroah-Hartman)
- Check the _DSM method at early stage, before allocate or parse anything in acpi_pfru_[log_]probe(). (Greg Kroah-Hartman)
- Set the parent of the misc device. (Greg Kroah-Hartman)
- Use module_platform_driver() instead of platform_driver_register() in module_init(). Separate pfru driver and pfru_telemetry driver to two files. (Greg Kroah-Hartman)
- Change from v5 to v6:
- Use Link: tag to add the specification download address.
(Andy Shevchenko)
- Drop comma for each terminator entry in the enum structure. (Andy Shevchenko)
- Remove redundant 'else' in get_image_type().
(Andy Shevchenko)
- Directly return results from the switch cases in adjust_efi_size() and pfru_ioctl().(Andy Shevchenko)
- Keep comment style consistency by removing the period for
one line comment.
(Andy Shevchenko)
- Remove devm_kfree() if .probe() failed.
(Andy Shevchenko)
- Remove linux/uuid.h and use raw buffers to contain uuid.
(Andy Shevchenko)
- Include types.h in pfru.h. (Andy Shevchenko)
- Use __u8[16] instead of uuid_t. (Andy Shevchenko)
- Replace enum in pfru.h with __u32 as enum size is not the
same size on all possible architectures.
(Andy Shevchenko)
- Simplify the userspace tool to use while loop for getopt_long(). (Andy Shevchenko)
- Change from v4 to v5:
- Remove Documentation/ABI/pfru, and move the content to kernel doc in include/uapi/linux/pfru.h (Greg Kroah-Hartman)
- Shrink the range of ioctl numbers declared in Documentation/userspace-api/ioctl/ioctl-number.rst
from 16 to 8. (Greg Kroah-Hartman)
- Change global variable struct pfru_device *pfru_dev to
per PFRU device. (Greg Kroah-Hartman)
- Unregister the misc device in acpi_pfru_remove().
(Greg Kroah-Hartman)
- Convert the kzalloc() to devm_kzalloc() in the driver so
as to avoid freeing the memory. (Greg Kroah-Hartman)
- Fix the compile warning by declaring the pfru_log_ioctl() as static. (kernel test robot LKP)
- Change to global variable misc_device to per PFRU device.
(Greg Kroah-Hartman)
- Remove the telemetry output in commit log. (Greg Kroah-Hartman)
- Add link for corresponding userspace tool in the commit log. (Greg Kroah-Hartman)
- Replace the telemetry .read() with .mmap() so that the userspace could mmap once, and read multiple times. (Greg Kroah-Hartman)
- Change from v3 to v4:
- Add Documentation/ABI/testing/pfru to document the ABI and remove Documentation/x86/pfru.rst (Rafael J. Wysocki)
- Replace all pr_err() with dev_dbg() (Greg Kroah-Hartman,
Rafael J. Wysocki)
- returns ENOTTY rather than ENOIOCTLCMD if invalid ioctl command is provided. (Greg Kroah-Hartman)
- Remove compat ioctl. (Greg Kroah-Hartman)
- Rename /dev/pfru/pfru_update to /dev/acpi_pfru (Greg Kroah-Hartman)
- Simplify the check for element of the package in query_capability() (Rafael J. Wysocki)
- Remove the loop in query_capability(), query_buffer() and query the package elemenet directly. (Rafael J. Wysocki)
- Check the number of elements in case the number of package elements is too small. (Rafael J. Wysocki)
- Doing the assignment as initialization in get_image_type(). Meanwhile, returns the type or a negative error code in get_image_type(). (Rafael J. Wysocki)
- Put the comments inside the function. (Rafael J. Wysocki)
- Returns the size or a negative error code in adjust_efi_size() (Rafael J. Wysocki)
- Fix the return value from EFAULT to EINVAL if pfru_valid_revid() does not pass. (Rafael J. Wysocki)
- Change the write() to be the code injection/update, the read() to be telemetry retrieval and all of the rest to be ioctl()s under one special device file.(Rafael J. Wysocki)
- Remove redundant parens. (Rafael J. Wysocki)
- Putting empty code lines after an if () statement that is not followed by a block. (Rafael J. Wysocki)
- Remove "goto" tags to make the code more readable. (Rafael J. Wysocki)
- Change from v2 to v3:
- Use valid types for structures that cross the user/kernel boundary in the uapi header. (Greg Kroah-Hartman)
- Rename the structure in uapi to start with a prefix pfru so as to avoid confusing in the global namespace. (Greg Kroah-Hartman)
- Change from v1 to v2:
- Add a spot in index.rst so it becomes part of the docs build (Jonathan Corbet).
- Sticking to the 80-column limit(Jonathan Corbet).
- Underline lengths should match the title text(Jonathan Corbet).
- Use literal blocks for the code samples(Jonathan Corbet).
- Add sanity check for duplicated instance of ACPI device.
- Update the driver to work with allocated pfru_device objects. (Mike Rapoport)
- For each switch case pair, get rid of the magic case numbers
and add a default clause with the error handling.(Mike Rapoport)
- Move the obj->type checks outside the switch to reduce redundancy. (Mike Rapoport)
- Parse the code_inj_id and drv_update_id at driver initialization time to reduce the re-parsing at runtime. (Mike Rapoport)
- Explain in detail how the size needs to be adjusted when doing version check. (Mike Rapoport)
- Rename parse_update_result() to dump_update_result()
(Mike Rapoport)
- Remove redundant return.(Mike Rapoport)
- Do not expose struct capsulate_buf_info to uapi, since it is
not needed in userspace. (Mike Rapoport)
- Do not allow non-root user to run this test.(Shuah Khan)
- Test runs on platform without pfru_telemetry should skip
instead of reporting failure/error.(Shuah Khan)
- Reuse uapi/linux/pfru.h instead of copying it into the test directory. (Mike Rapoport)
Chen Yu (4):
efi: Introduce EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER and
corresponding structures
drivers/acpi: Introduce Platform Firmware Runtime Update device driver drivers/acpi: Introduce Platform Firmware Runtime Update Telemetry
tools: Introduce power/acpi/pfru/pfru
.../userspace-api/ioctl/ioctl-number.rst | 1 + drivers/acpi/Kconfig | 1 + drivers/acpi/Makefile | 1 + drivers/acpi/pfru/Kconfig | 13 + drivers/acpi/pfru/Makefile | 2 + drivers/acpi/pfru/pfru_telemetry.c | 465 ++++++++++++++ drivers/acpi/pfru/pfru_update.c | 608 ++++++++++++++++++ include/linux/efi.h | 46 ++ include/uapi/linux/pfru.h | 262 ++++++++ tools/power/acpi/pfru/Makefile | 25 + tools/power/acpi/pfru/pfru.8 | 137 ++++ tools/power/acpi/pfru/pfru.c | 417 ++++++++++++ 12 files changed, 1978 insertions(+) create mode 100644 drivers/acpi/pfru/Kconfig create mode 100644 drivers/acpi/pfru/Makefile create mode 100644 drivers/acpi/pfru/pfru_telemetry.c create mode 100644 drivers/acpi/pfru/pfru_update.c create mode 100644 include/uapi/linux/pfru.h create mode 100644 tools/power/acpi/pfru/Makefile create mode 100644 tools/power/acpi/pfru/pfru.8 create mode 100644 tools/power/acpi/pfru/pfru.c
Signature · 1 line
2.25.1
PATCH v8 2/4 drivers/acpi: Introduce Platform Firmware Runtime Update device driver
Introduce the pfru_update driver which can be used for Platform Firmware Runtime code injection and driver update [1]. The user is expected to provide the update firmware in the form of capsule file, and pass it to the driver via ioctl. Then the driver would hand this capsule file to the Platform Firmware Runtime Update via the ACPI device _DSM method. At last the low level Management Mode would do the firmware update.
The corresponding userspace tool and man page will be introduced at tools/power/acpi/pfru.
---
v8: Remove the variable-length array in struct pfru_update_cap_info, and
copy the non-variable-length struct pfru_update_cap_info to userspace directly. (Greg Kroah-Hartman)
Change the type of rev_id from int to u32, because this data will be copied between kernel and userspace. (Greg Kroah-Hartman)
Add a prefix for dev in struct pfru_device to parent_dev, so as to indicate that this filed is the parent of the created miscdev. (Greg Kroah-Hartman)
Use blank lines between different macro sections. (Greg Kroah-Hartman) Illusatrate the possible errno for each ioctl interface.
(Greg Kroah-Hartman)
Remove pfru_valid_revid() from uapi header to avoid poluting the global namespace.(Greg Kroah-Hartman)
Assign the value to the enum type explicitly.(Greg Kroah-Hartman) Change the guid_t to efi_guid_t when parsing image header in get_image_type() (Greg Kroah-Hartman)
Remove the void * to other type casting in valid_version(). (Andy Shevchenko) Combined the assignment of variables with definitions. (Andy Shevchenko) Define this magic for revision ID. (Andy Shevchenko)
Make the labeling consistent for error handling. (Andy Shevchenko) Replace the UUID_SIZE in uapi with 16 directly. (Andy Shevchenko) Add blank line between generic include header and uapi header. (Andy Shevchenko)
Arrange the order between devm_kzalloc() and normal allocation in acpi_pfru_probe() that, the former should always be ahead of the latter. (Andy Shevchenko)
Move the UUID from uapi header to the c file. (Andy Shevchenko)
v7: Use ida_alloc() to allocate a ID, and release the ID when
device is removed. (Greg Kroah-Hartman)
Check the _DSM method at early stage, before allocate or parse anything in acpi_pfru_probe(). (Greg Kroah-Hartman)
Set the parent of the misc device. (Greg Kroah-Hartman)
Use module_platform_driver() instead of platform_driver_register() in module_init(). Separate pfru driver and pfru_telemetry driver to two files. (Greg Kroah-Hartman)
v6: Use Link: tag to add the specification download address.
(Andy Shevchenko)
Remove linux/uuid.h and use raw buffers to contain uuid.
(Andy Shevchenko)
Drop comma for each terminator entry in the enum structure. (Andy Shevchenko)
Remove redundant 'else' in get_image_type().
(Andy Shevchenko)
Directly return results from the switch cases in adjust_efi_size() and pfru_ioctl().(Andy Shevchenko)
Keep comment style consistent by removing the period for
one line comment.
(Andy Shevchenko)
Remove devm_kfree() if .probe() failed.
(Andy Shevchenko)
v5: Remove Documentation/ABI/pfru, and move the content to kernel doc
in include/uapi/linux/pfru.h (Greg Kroah-Hartman)
Shrink the range of ioctl numbers declared in Documentation/userspace-api/ioctl/ioctl-number.rst
from 16 to 8. (Greg Kroah-Hartman)
Change global variable struct pfru_device *pfru_dev to
per ACPI device. (Greg Kroah-Hartman)
Unregister the misc device in acpi_pfru_remove().
(Greg Kroah-Hartman)
Convert the kzalloc() to devm_kzalloc() in the driver so
as to avoid freeing the memory. (Greg Kroah-Hartman)
Fix the compile error by declaring the pfru_log_ioctl() as static. (kernel test robot LKP)
Change to global variable misc_device to per ACPI device.
(Greg Kroah-Hartman)
v4: Replace all pr_err() with dev_dbg() (Greg Kroah-Hartman,
Rafael J. Wysocki)
Returns ENOTTY rather than ENOIOCTLCMD if invalid ioctl command is provided. (Greg Kroah-Hartman)
Remove compat ioctl. (Greg Kroah-Hartman)
Rename /dev/pfru/pfru_update to /dev/acpi_pfru (Greg Kroah-Hartman) Simplify the check for element of the package in query_capability() (Rafael J. Wysocki)
Remove the loop in query_capability(), query_buffer() and query the package elemenet directly. (Rafael J. Wysocki)
Check the the number of elements in case the number of package elements is too small. (Rafael J. Wysocki)
Doing the assignment as initialization in get_image_type(). Meanwhile, returns the type or a negative error code in get_image_type(). (Rafael J. Wysocki)
Put the comments inside the function. (Rafael J. Wysocki) Returns the size or a negative error code in adjust_efi_size() (Rafael J. Wysocki)
Fix the return value from EFAULT to EINVAL if pfru_valid_revid() does not pass. (Rafael J. Wysocki)
Change the write() to be the code injection/update, the read() to be telemetry retrieval and all of the rest to be ioctl()s under one special device file.(Rafael J. Wysocki)
Putting empty code lines after an if () statement that is not followed by a block. (Rafael J. Wysocki)
Remove "goto" tags to make the code more readable. (Rafael J. Wysocki)
v3: Use __u32 instead of int and __64 instead of unsigned long
in include/uapi/linux/pfru.h (Greg Kroah-Hartman)
Rename the structure in uapi to start with a prefix pfru so as to avoid confusing in the global namespace. (Greg Kroah-Hartman)
v2: Add sanity check for duplicated instance of ACPI device.
Update the driver to work with allocated pfru_device objects. (Mike Rapoport)
For each switch case pair, get rid of the magic case numbers
and add a default clause with the error handling.
(Mike Rapoport)
Move the obj->type checks outside the switch to reduce redundancy. (Mike Rapoport)
Parse the code_inj_id and drv_update_id at driver initialization time to reduce the re-parsing at runtime.(Mike Rapoport)
Explain in detail how the size needs to be adjusted when doing version check.(Mike Rapoport)
Rename parse_update_result() to dump_update_result()(Mike Rapoport) Remove redundant return.(Mike Rapoport)
Do not expose struct capsulate_buf_info to uapi, since it is
not needed in userspace.(Mike Rapoport)
--- .../userspace-api/ioctl/ioctl-number.rst | 1 + drivers/acpi/Kconfig | 1 + drivers/acpi/Makefile | 1 + drivers/acpi/pfru/Kconfig | 13 + drivers/acpi/pfru/Makefile | 2 + drivers/acpi/pfru/pfru_update.c | 608 ++++++++++++++++++ include/uapi/linux/pfru.h | 174 +++++ 7 files changed, 800 insertions(+) create mode 100644 drivers/acpi/pfru/Kconfig create mode 100644 drivers/acpi/pfru/Makefile create mode 100644 drivers/acpi/pfru/pfru_update.c create mode 100644 include/uapi/linux/pfru.h
Documentation/userspace-api/ioctl/ioctl-number.rst +1 -0
@@ -366,6 +366,7 @@ Code Seq# Include File Comments <mailto:[email protected]> 0xE5 00-3F linux/fuse.h 0xEC 00-01 drivers/platform/chrome/cros_ec_dev.h ChromeOS EC driver +0xEE 00-09 uapi/linux/pfru.h Platform Firmware Runtime Update and Telemetry 0xF3 00-3F drivers/usb/misc/sisusbvga/sisusb.h sisfb (in development) <mailto:[email protected]> 0xF6 all LTTng Linux Trace Toolkit Next Generation
drivers/acpi/Kconfig +1 -0
@@ -482,6 +482,7 @@ source "drivers/acpi/nfit/Kconfig" source "drivers/acpi/numa/Kconfig" source "drivers/acpi/apei/Kconfig" source "drivers/acpi/dptf/Kconfig" +source "drivers/acpi/pfru/Kconfig" config ACPI_WATCHDOG bool
drivers/acpi/Makefile +1 -0
@@ -102,6 +102,7 @@ obj-$(CONFIG_ACPI_CPPC_LIB) += cppc_acpi.o obj-$(CONFIG_ACPI_SPCR_TABLE) += spcr.o obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o obj-$(CONFIG_ACPI_PPTT) += pptt.o +obj-$(CONFIG_ACPI_PFRU) += pfru/ # processor has its own "processor." module_param namespace processor-y := processor_driver.o
drivers/acpi/pfru/Kconfig new file +13 -0
@@ -0,0 +1,13 @@ +# SPDX-License-Identifier: GPL-2.0 +config ACPI_PFRU + tristate "ACPI Platform Firmware Runtime Update (PFRU)" + depends on 64BIT + help + In order to reduce the system reboot times and update the platform firmware + in time, Platform Firmware Runtime Update is leveraged to patch the system + without reboot. This driver supports Platform Firmware Runtime Update, + which is composed of two parts: code injection and driver update. It also + allows telemetry data to be retrieved from the platform firmware. + + To compile this driver as module, choose M here: + the modules will be called pfru_update and pfru_telemetry.
drivers/acpi/pfru/Makefile new file +2 -0
@@ -0,0 +1,2 @@ +# SPDX-License-Identifier: GPL-2.0-only +obj-$(CONFIG_ACPI_PFRU) += pfru_update.o
drivers/acpi/pfru/pfru_update.c new file +608 -0
608 lines not on this page · raw message
include/uapi/linux/pfru.h new file +174 -0
174 lines not on this page · raw message
Signature · 1 line
2.25.1
PATCH v8 3/4 drivers/acpi: Introduce Platform Firmware Runtime Update Telemetry
In reply to Chen YuPlatform Firmware Runtime Update(PFRU) Telemetry Service is part of RoT (Root of Trust), which allows PFRU handler and other PFRU drivers to produce telemetry data to upper layer OS consumer at runtime.
The Linux kernel provides interfaces for the user to query the parameters of telemetry data, and the user could read out the telemetry data accordingly.
PFRU and PFRU Telemetry both invoke _DSM to trigger the low level actions. However the input parameters and ACPI package result as well as the functions are different from each other. It is hard to extract the common code between them, so introduce separated files for them.
The corresponding userspace tool and man page will be introduced at tools/power/acpi/pfru.
---
v8: Add blank line between generic include header and uapi header.
(Andy Shevchenko)
Arrange the order between devm_kzalloc() and normal allocation in acpi_pfru_log_probe() that, the former should always be ahead of the latter. (Andy Shevchenko)
Amend commit message to elaborate the reason why PFRU and PRFU_Telemetry drivers look the same. (Andy Shevchenko)
v7: Use ida_alloc() to allocate a ID, and release the ID when
device is removed. (Greg Kroah-Hartman)
Check the _DSM method at early stage, before allocate or parse anything in acpi_pfru_log_probe(). (Greg Kroah-Hartman)
Set the parent of the misc device. (Greg Kroah-Hartman)
Use module_platform_driver() instead of platform_driver_register() in module_init(). Separate pfru driver and pfru_telemetry driver to two files. (Greg Kroah-Hartman)
v6: Remove linux/uuid.h and use raw buffers to contain uuid.
(Andy Shevchenko)
Include types.h in pfru.h. (Andy Shevchenko)
Use __u8[16] instead of uuid_t. (Andy Shevchenko)
Replace enum in pfru.h with __u32 as enum size is not the same on all possible architectures.
(Andy Shevchenko)
Directly return results from the switch cases in pfru_log_ioctl(). (Andy Shevchenko)
v5: Remove the log output sample in commit log. (Greg Kroah-Hartman)
Add link for corresponding userspace tool in the commit log. (Greg Kroah-Hartman)
Replace the telemetry .read() with .mmap() so that the userspace could mmap once, and read multiple times. (Greg Kroah-Hartman) Fix the compile warning by declaring the pfru_log_ioctl() as static. (kernel test robot LKP)
v4: Change the write() to be the code injection/update, the read() to
be telemetry retrieval and all of the rest to be ioctl()s under one special device file.(Rafael J. Wysocki)
Remove redundant parens. (Rafael J. Wysocki)
Putting empty code lines after an if () statement that is not followed by a block. (Rafael J. Wysocki)
Remove "goto" tags to make the code more readable. (Rafael J. Wysocki)
v3: Use __u32 instead of int and __64 instead of unsigned long
in include/uapi/linux/pfru.h (Greg Kroah-Hartman)
Rename the structure in uapi to start with a prefix pfru so as to avoid confusing in the global namespace. (Greg Kroah-Hartman)
v2: Do similar clean up as pfru_update driver:
Add sanity check for duplicated instance of ACPI device. Update the driver to work with allocated telem_device objects. (Mike Rapoport)
For each switch case pair, get rid of the magic case numbers and add a default clause with the error handling.
(Mike Rapoport)
--- drivers/acpi/pfru/Makefile | 2 +- drivers/acpi/pfru/pfru_telemetry.c | 465 +++++++++++++++++++++++++++++ include/uapi/linux/pfru.h | 88 ++++++ 3 files changed, 554 insertions(+), 1 deletion(-) create mode 100644 drivers/acpi/pfru/pfru_telemetry.c
drivers/acpi/pfru/Makefile +1 -1
@@ -1,2 +1,2 @@ # SPDX-License-Identifier: GPL-2.0-only -obj-$(CONFIG_ACPI_PFRU) += pfru_update.o +obj-$(CONFIG_ACPI_PFRU) += pfru_update.o pfru_telemetry.o
drivers/acpi/pfru/pfru_telemetry.c new file +465 -0
465 lines not on this page · raw message
include/uapi/linux/pfru.h +88 -0
92 lines not on this page · raw message
Signature · 1 line
2.25.1
PATCH v8 4/4 tools: Introduce power/acpi/pfru/pfru
In reply to Chen YuIntroduce a user space tool to make use of the interface exposed by Platform Firmware Runtime Update and Telemetry drivers. The users can use this tool to do firmware code injection, driver update and to retrieve the telemetry data.
---
v8: Print the length of OEM information if requested.
v7: No change since v6.
v6: Simplify the userspace tool to use while loop for getopt_long().
(Andy Shevchenko)
v5: Replace the read() with mmap() so that the userspace
could mmap once, and read multiple times. (Greg Kroah-Hartman)
--- tools/power/acpi/pfru/Makefile | 25 ++ tools/power/acpi/pfru/pfru.8 | 137 +++++++++++ tools/power/acpi/pfru/pfru.c | 417 +++++++++++++++++++++++++++++++++ 3 files changed, 579 insertions(+) create mode 100644 tools/power/acpi/pfru/Makefile create mode 100644 tools/power/acpi/pfru/pfru.8 create mode 100644 tools/power/acpi/pfru/pfru.c
tools/power/acpi/pfru/Makefile new file +25 -0
@@ -0,0 +1,25 @@ +# SPDX-License-Identifier: GPL-2.0+ + +CFLAGS += -Wall -O2 +CFLAGS += -DPFRU_HEADER='"../../../../include/uapi/linux/pfru.h"' +BUILD_OUTPUT := $(CURDIR) + +ifeq ("$(origin O)", "command line") + BUILD_OUTPUT := $(O) +endif + +pfru : pfru.c + +%: %.c + @mkdir -p $(BUILD_OUTPUT) + $(CC) $(CFLAGS) $< -o $(BUILD_OUTPUT)/$@ $(LDFLAGS) -luuid + +.PHONY : clean +clean : + @rm -f $(BUILD_OUTPUT)/pfru + +install : pfru + install -d $(DESTDIR)$(PREFIX)/bin + install $(BUILD_OUTPUT)/pfru $(DESTDIR)$(PREFIX)/bin/pfru + install -d $(DESTDIR)$(PREFIX)/share/man/man8 + install -m 644 pfru.8 $(DESTDIR)$(PREFIX)/share/man/man8
tools/power/acpi/pfru/pfru.8 new file +137 -0
137 lines not on this page · raw message
tools/power/acpi/pfru/pfru.c new file +417 -0
417 lines not on this page · raw message
Signature · 1 line
2.25.1
Re: [PATCH v8 2/4] drivers/acpi: Introduce Platform Firmware Runtime Update device driver
In reply to Chen YuQuoted 9 lines · Chen Yu
Introduce the
pfru_updatedriver which can be used for Platform Firmware Runtime code injection and driver update [1]. The user is expected to provide the update firmware in the form of capsule file, and pass it to the driver via ioctl. Then the driver would hand this capsule file to the Platform Firmware Runtime Update via the ACPI device_DSMmethod. At last the low level Management Mode would do the firmware update.The corresponding userspace tool and man page will be introduced at
tools/power/acpi/pfru.
...
drivers/acpi/pfru/pfru_update.c line 28
Quoted 3 lines
drivers/acpi/pfru/pfru_update.c new file +3
-0+#define PFRU_UUID "ECF9533B-4A3C-4E89-939E-C77112601C6D" +#define PFRU_CODE_INJ_UUID "B2F84B79-7B6E-4E45-885F-3FB9BB185402" +#define PFRU_DRV_UPDATE_UUID "4569DD8C-75F1-429A-A3D6-24DE8097A0DF"
What stops you to have these being binaries?GUID_INIT() / EFI_GUID_INIT()
...
Quoted 4 lines
drivers/acpi/pfru/pfru_update.c new file +4
-0+enum cap_index { +CAP_STATUS_IDX =0 , +CAP_UPDATE_IDX =1 , +CAP_CODE_TYPE_IDX =2 ,
drivers/acpi/pfru/pfru_update.c line 54
Quoted 10 lines
drivers/acpi/pfru/pfru_update.c new file +9
-0+CAP_FW_VER_IDX =3 , +CAP_CODE_RT_VER_IDX =4 , +CAP_DRV_TYPE_IDX =5 , +CAP_DRV_RT_VER_IDX =6 , +CAP_DRV_SVN_IDX =7 , +CAP_PLAT_ID_IDX =8 , +CAP_OEM_ID_IDX =9 , +CAP_OEM_INFO_IDX =10 , +CAP_NR_IDX =11
Assignment here doesn't make any sense (it just adds unneeded churn and burden). Same to the rest of similar cases below.
Quoted 1 line
+};
...
drivers/acpi/pfru/pfru_update.c line 83
Quoted 2 lines
drivers/acpi/pfru/pfru_update.c new file +2
-0+struct pfru_device { +guid_t uuid, code_uuid, drv_uuid;
You don't need these. At least for now.
Quoted 4 lines
drivers/acpi/pfru/pfru_update.c new file +4
-0+u32 rev_id, index; +struct device *parent_dev; +struct miscdevice miscdev; +};
...
drivers/acpi/pfru/pfru_update.c line 298
Quoted 1 line
drivers/acpi/pfru/pfru_update.c new file +1
-0+ m_hdr = (struct efi_manage_capsule_header *)(data + size);
Do you need this casting?
...
drivers/acpi/pfru/pfru_update.c line 305
Quoted 1 line
drivers/acpi/pfru/pfru_update.c new file +1
-0+ m_img_hdr = (struct efi_manage_capsule_image_header *)(data + size);
Ditto.
...
drivers/acpi/pfru/pfru_update.c line 315
Quoted 1 line
drivers/acpi/pfru/pfru_update.c new file +1
-0+ auth = (struct efi_image_auth *)(data + size);
Ditto.
...
Review
Quoted 1 line
(quoted diff) +1
-0+ ACPI_FREE(out_obj);
Recently with Hans we realised that this (ACPI_FREE() API) is mostly for ACPICA use. We may use simple kfree(). Sorry for getting back and forward.
...
Quoted 5 lines
drivers/acpi/pfru/pfru_update.c new file +5
-0+static long pfru_ioctl (struct file *file,unsigned int cmd,unsigned long arg) +{ +struct pfru_update_cap_info cap_hdr; +struct pfru_device *pfru_dev =to_pfru_dev (file); +void __user *p = (void __user *)arg;
drivers/acpi/pfru/pfru_update.c line 426
Quoted 10 lines
drivers/acpi/pfru/pfru_update.c new file +10
-0+u32 rev; +int ret; + +switch (cmd) { +case PFRU_IOC_QUERY_CAP : + ret =query_capability (&cap_hdr, pfru_dev); +if (ret) +return ret; + +if (copy_to_user (p, &cap_hdr,sizeof (cap_hdr)))
I'm wondering what will happen if p has less _real data_ than sizeof(cap_hdr)?
drivers/acpi/pfru/pfru_update.c line 431
Quoted 6 lines
drivers/acpi/pfru/pfru_update.c new file +5
-0+return -EFAULT ; + +return 0 ; +case PFRU_IOC_SET_REV : +if (copy_from_user (&rev, p,sizeof (u32)))
sizeof(rev)
Quoted 18 lines
drivers/acpi/pfru/pfru_update.c new file +18
-0+return -EFAULT ; + +if (!pfru_valid_revid (rev)) +return -EINVAL ; + + pfru_dev->rev_id = rev; + +return 0 ; +case PFRU_IOC_STAGE : +return start_acpi_update (START_STAGE , pfru_dev); +case PFRU_IOC_ACTIVATE : +return start_acpi_update (START_ACTIVATE , pfru_dev); +case PFRU_IOC_STAGE_ACTIVATE : +return start_acpi_update (START_STAGE_ACTIVATE , pfru_dev); +default : +return -ENOTTY ; + } +}
...
drivers/acpi/pfru/pfru_update.c line 475
Quoted 2 lines
drivers/acpi/pfru/pfru_update.c new file +2
-0+/* map the communication buffer */ + phy_addr = (phys_addr_t)(buf_info.addr_lo | (buf_info.addr_hi <<32 ));
It's better to read if you start from MSB part to LSB.
...
drivers/acpi/pfru/pfru_update.c line 549
Quoted 3 lines
drivers/acpi/pfru/pfru_update.c new file +3
-0+ ret =ida_alloc (&pfru_ida,GFP_KERNEL ); +if (ret <0 ) +return ret;
(1)
...
drivers/acpi/pfru/pfru_update.c line 557
Quoted 2 lines
drivers/acpi/pfru/pfru_update.c new file +2
-0+ pfru_dev->miscdev.name =kasprintf (GFP_KERNEL , +"pfru%d" , pfru_dev->index);
devm_kasprinf()
...
drivers/acpi/pfru/pfru_update.c line 564
Quoted 2 lines
drivers/acpi/pfru/pfru_update.c new file +2
-0+ pfru_dev->miscdev.nodename =kasprintf (GFP_KERNEL , +"acpi_pfru%d" , pfru_dev->index);
Ditto.
Yep, I know about (1), but do your homework and see how you can satisfy both comments.
...
drivers/acpi/pfru/pfru_update.c line 592
Quoted 2 lines
drivers/acpi/pfru/pfru_update.c new file +2
-0+static const struct acpi_device_id acpi_pfru_ids[] = { + {"INTC1080" ,0 },
0 is redundant.
Quoted 2 lines
(quoted diff) +2
-0+ {} +};
...
include/uapi/linux/pfru.h line 11
Quoted 2 lines
include/uapi/linux/pfru.h new file +2
-0+#include <linux/types.h> +#include <linux/ioctl.h>
Order?
...
Quoted 1 line
+#define
PFRU_MAGIC0xEE
Perhaps PFRU_MAGIC_FOR_IOCTL.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v8 3/4] drivers/acpi: Introduce Platform Firmware Runtime Update Telemetry
In reply to Chen YuQuoted 8 lines · Chen Yu
Platform Firmware Runtime Update(PFRU) Telemetry Service is part of RoT (Root of Trust), which allows PFRU handler and other PFRU drivers to produce telemetry data to upper layer OS consumer at runtime.
The Linux kernel provides interfaces for the user to query the parameters of telemetry data, and the user could read out the telemetry data accordingly.
Quoted 7 lines
PFRU and PFRU Telemetry both invoke
_DSMto trigger the low level actions. However the input parameters and ACPI package result as well as the functions are different from each other. It is hard to extract the common code between them, so introduce separated files for them.The corresponding userspace tool and man page will be introduced at
tools/power/acpi/pfru.
Comments from previous patch partially apply here.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v8 4/4] tools: Introduce power/acpi/pfru/pfru
In reply to Chen YuQuoted 8 lines · Chen Yu
Introduce a user space tool to make use of the interface exposed by Platform Firmware Runtime Update and Telemetry drivers. The users can use this tool to do firmware code injection, driver update and to retrieve the telemetry data.
tools/power/acpi/pfru/Makefile | 25 ++ tools/power/acpi/pfru/pfru.8 | 137 +++++++++++ tools/power/acpi/pfru/pfru.c | 417 +++++++++++++++++++++++++++++++++
Use Build infrastructure (see, for example, tools/gpio/).
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v8 2/4] drivers/acpi: Introduce Platform Firmware Runtime Update device driver
In reply to Andy ShevchenkoQuoted 11 lines · Andy Shevchenko
Quoted 9 lines · Chen Yu
Introduce the
pfru_updatedriver which can be used for Platform Firmware Runtime code injection and driver update [1]. The user is expected to provide the update firmware in the form of capsule file, and pass it to the driver via ioctl. Then the driver would hand this capsule file to the Platform Firmware Runtime Update via the ACPI device_DSMmethod. At last the low level Management Mode would do the firmware update.The corresponding userspace tool and man page will be introduced at
tools/power/acpi/pfru.
Quoted 9 lines
...
drivers/acpi/pfru/pfru_update.c line 28
Quoted 3 lines
drivers/acpi/pfru/pfru_update.c new file +3
-0+#define PFRU_UUID "ECF9533B-4A3C-4E89-939E-C77112601C6D" +#define PFRU_CODE_INJ_UUID "B2F84B79-7B6E-4E45-885F-3FB9BB185402" +#define PFRU_DRV_UPDATE_UUID "4569DD8C-75F1-429A-A3D6-24DE8097A0DF"What stops you to have these being binaries?
GUID_INIT()/EFI_GUID_INIT()
Ok, will change to GUID_INIT().
Quoted 15 lines
...
Quoted 12 lines
drivers/acpi/pfru/pfru_update.c new file +12
-0+enum cap_index { +CAP_STATUS_IDX =0 , +CAP_UPDATE_IDX =1 , +CAP_CODE_TYPE_IDX =2 , +CAP_FW_VER_IDX =3 , +CAP_CODE_RT_VER_IDX =4 , +CAP_DRV_TYPE_IDX =5 , +CAP_DRV_RT_VER_IDX =6 , +CAP_DRV_SVN_IDX =7 , +CAP_PLAT_ID_IDX =8 , +CAP_OEM_ID_IDX =9 , +CAP_OEM_INFO_IDX =10 ,
Quoted 5 lines
drivers/acpi/pfru/pfru_update.c line 54
Quoted 1 line
drivers/acpi/pfru/pfru_update.c new file +1
-0+CAP_NR_IDX =11 Assignment here doesn't make any sense (it just adds unneeded churn and burden). Same to the rest of similar cases below.
Greg mentioned that, we might need to "explicit about the numbers here, because it is uncerntain this is guaranteed by all C compilers or not."
https://lore.kernel.org/lkml/[email protected]/
My understanding is that, this applys to both uapi headers and the kernel internal headers.
Quoted 9 lines
Quoted 1 line
+};
...
drivers/acpi/pfru/pfru_update.c line 83
Quoted 2 lines
drivers/acpi/pfru/pfru_update.c new file +2
-0+struct pfru_device { +guid_t uuid, code_uuid, drv_uuid;You don't need these. At least for now.
Ok, will drop these.
Quoted 11 lines
Quoted 4 lines
drivers/acpi/pfru/pfru_update.c new file +4
-0+u32 rev_id, index; +struct device *parent_dev; +struct miscdevice miscdev; +};...
drivers/acpi/pfru/pfru_update.c line 298
Quoted 1 line
drivers/acpi/pfru/pfru_update.c new file +1
-0+ m_hdr = (struct efi_manage_capsule_header *)(data + size);Do you need this casting?
Will drop this.
Quoted 14 lines
...
drivers/acpi/pfru/pfru_update.c line 305
Quoted 1 line
drivers/acpi/pfru/pfru_update.c new file +1
-0+ m_img_hdr = (struct efi_manage_capsule_image_header *)(data + size);Ditto.
...
drivers/acpi/pfru/pfru_update.c line 315
Quoted 1 line
drivers/acpi/pfru/pfru_update.c new file +1
-0+ auth = (struct efi_image_auth *)(data + size);Ditto.
...
Quoted 8 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ ACPI_FREE(out_obj);Recently with Hans we realised that this (
ACPI_FREE()API) is mostly for ACPICA use. We may use simplekfree(). Sorry for getting back and forward....
Will change it in next version.
Quoted 9 lines
Quoted 9 lines
drivers/acpi/pfru/pfru_update.c new file +9
-0+static long pfru_ioctl (struct file *file,unsigned int cmd,unsigned long arg) +{ +struct pfru_update_cap_info cap_hdr; +struct pfru_device *pfru_dev =to_pfru_dev (file); +void __user *p = (void __user *)arg; +u32 rev; +int ret; + +switch (cmd) {
Quoted 9 lines
drivers/acpi/pfru/pfru_update.c line 426
Quoted 6 lines
drivers/acpi/pfru/pfru_update.c new file +6
-0+case PFRU_IOC_QUERY_CAP : + ret =query_capability (&cap_hdr, pfru_dev); +if (ret) +return ret; + +if (copy_to_user (p, &cap_hdr,sizeof (cap_hdr)))I'm wondering what will happen if p has less
_realdata_thansizeof(cap_hdr)?
Here is my understanding: if the userspace has provided insufficient space, the userspace might either encounter segfault or data overwrite, and it is up to the userspace to avoid this situation from happening.
for example:
int my_test(void)
{
char *cap_on_heap = malloc(insufficient_size);
char cap_on_stack[insufficient_size];
int victim;
...
}copy_to_user(cap_on_heap) might cause segfault, and copy_to_user(cap_on_stack) might overwrite victim.
Quoted 9 lines
drivers/acpi/pfru/pfru_update.c line 431
Quoted 6 lines
drivers/acpi/pfru/pfru_update.c new file +5
-0+return -EFAULT ; + +return 0 ; +case PFRU_IOC_SET_REV : +if (copy_from_user (&rev, p,sizeof (u32)))sizeof(rev)
Ok.
Quoted 21 lines
Quoted 18 lines
drivers/acpi/pfru/pfru_update.c new file +18
-0+return -EFAULT ; + +if (!pfru_valid_revid (rev)) +return -EINVAL ; + + pfru_dev->rev_id = rev; + +return 0 ; +case PFRU_IOC_STAGE : +return start_acpi_update (START_STAGE , pfru_dev); +case PFRU_IOC_ACTIVATE : +return start_acpi_update (START_ACTIVATE , pfru_dev); +case PFRU_IOC_STAGE_ACTIVATE : +return start_acpi_update (START_STAGE_ACTIVATE , pfru_dev); +default : +return -ENOTTY ; + } +}...
Quoted 5 lines
drivers/acpi/pfru/pfru_update.c line 475
Quoted 2 lines
drivers/acpi/pfru/pfru_update.c new file +2
-0+/* map the communication buffer */ + phy_addr = (phys_addr_t)(buf_info.addr_lo | (buf_info.addr_hi <<32 ));It's better to read if you start from MSB part to LSB.
Ok, will do.
Quoted 17 lines
...
drivers/acpi/pfru/pfru_update.c line 549
Quoted 3 lines
drivers/acpi/pfru/pfru_update.c new file +3
-0+ ret =ida_alloc (&pfru_ida,GFP_KERNEL ); +if (ret <0 ) +return ret;(1)
...
drivers/acpi/pfru/pfru_update.c line 557
Quoted 2 lines
drivers/acpi/pfru/pfru_update.c new file +2
-0+ pfru_dev->miscdev.name =kasprintf (GFP_KERNEL , +"pfru%d" , pfru_dev->index);
devm_kasprinf()...
Quoted 8 lines
drivers/acpi/pfru/pfru_update.c line 564
Quoted 2 lines
drivers/acpi/pfru/pfru_update.c new file +2
-0+ pfru_dev->miscdev.nodename =kasprintf (GFP_KERNEL , +"acpi_pfru%d" , pfru_dev->index);Ditto.
Yep, I know about (1), but do your homework and see how you can satisfy both comments.
I did not realize devm_add_action_or_reset() could be used in (1) to deal with this situation, will do in next version.
Quoted 7 lines
...
drivers/acpi/pfru/pfru_update.c line 592
Quoted 2 lines
drivers/acpi/pfru/pfru_update.c new file +2
-0+static const struct acpi_device_id acpi_pfru_ids[] = { + {"INTC1080" ,0 },0 is redundant.
Ok.
Quoted 10 lines
Quoted 2 lines
(quoted diff) +2
-0+ {} +};...
include/uapi/linux/pfru.h line 11
Quoted 2 lines
include/uapi/linux/pfru.h new file +2
-0+#include <linux/types.h> +#include <linux/ioctl.h> Order?
Will adjust it.
Quoted 6 lines
...
Quoted 1 line
+#define
PFRU_MAGIC0xEEPerhaps
PFRU_MAGIC_FOR_IOCTL.
Ok.
Thanks,
Chenyu
Re: [PATCH v8 2/4] drivers/acpi: Introduce Platform Firmware Runtime Update device driver
Quoted 2 lines · Chen Yu
Quoted 1 line · Andy Shevchenko
On Wed, Nov 03, 2021 at 11:43:50PM +0800, Chen Yu wrote:
...
Quoted 18 lines
Quoted 13 lines
Quoted 12 lines
drivers/acpi/pfru/pfru_update.c new file +12
-0+enum cap_index { +CAP_STATUS_IDX =0 , +CAP_UPDATE_IDX =1 , +CAP_CODE_TYPE_IDX =2 , +CAP_FW_VER_IDX =3 , +CAP_CODE_RT_VER_IDX =4 , +CAP_DRV_TYPE_IDX =5 , +CAP_DRV_RT_VER_IDX =6 , +CAP_DRV_SVN_IDX =7 , +CAP_PLAT_ID_IDX =8 , +CAP_OEM_ID_IDX =9 , +CAP_OEM_INFO_IDX =10 ,Quoted 5 lines
drivers/acpi/pfru/pfru_update.c line 54
Quoted 1 line
drivers/acpi/pfru/pfru_update.c new file +1
-0+CAP_NR_IDX =11 Assignment here doesn't make any sense (it just adds unneeded churn and burden). Same to the rest of similar cases below.
Quoted 5 lines
Greg mentioned that, we might need to "explicit about the numbers here, because it is uncerntain this is guaranteed by all C compilers or not."
https://lore.kernel.org/lkml/[email protected]/
My understanding is that, this applys to both uapi headers and the kernel internal headers.
I was talking only about the last item. This is guaranteed to be LAST+1 by the C standard and any deviation from that is a compiler bug that must be fixed.
The first elements may be explicitly defined so we will be sure they are always stay. Greg is right about it.
Quoted 1 line
Quoted 1 line
Quoted 1 line
+};
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v8 2/4] drivers/acpi: Introduce Platform Firmware Runtime Update device driver
Quoted 30 lines · Andy Shevchenko
Quoted 2 lines · Chen Yu
Quoted 1 line · Andy Shevchenko
On Wed, Nov 03, 2021 at 11:43:50PM +0800, Chen Yu wrote:
...
Quoted 18 lines
Quoted 13 lines
Quoted 12 lines
drivers/acpi/pfru/pfru_update.c new file +12
-0+enum cap_index { +CAP_STATUS_IDX =0 , +CAP_UPDATE_IDX =1 , +CAP_CODE_TYPE_IDX =2 , +CAP_FW_VER_IDX =3 , +CAP_CODE_RT_VER_IDX =4 , +CAP_DRV_TYPE_IDX =5 , +CAP_DRV_RT_VER_IDX =6 , +CAP_DRV_SVN_IDX =7 , +CAP_PLAT_ID_IDX =8 , +CAP_OEM_ID_IDX =9 , +CAP_OEM_INFO_IDX =10 ,Quoted 5 lines
drivers/acpi/pfru/pfru_update.c line 54
Quoted 1 line
drivers/acpi/pfru/pfru_update.c new file +1
-0+CAP_NR_IDX =11 Assignment here doesn't make any sense (it just adds unneeded churn and burden). Same to the rest of similar cases below.
Quoted 5 lines
Greg mentioned that, we might need to "explicit about the numbers here, because it is uncerntain this is guaranteed by all C compilers or not."
https://lore.kernel.org/lkml/[email protected]/
My understanding is that, this applys to both uapi headers and the kernel internal headers.
Quoted 6 lines
I was talking only about the last item. This is guaranteed to be LAST+1 by the C standard and any deviation from that is a compiler bug that must be fixed.
The first elements may be explicitly defined so we will be sure they are always stay. Greg is right about it.
Ok, got it. Previously I've sent a v9 patch set that addressed the issues in v8, except for this enum issue. I'll wait for a day or two, if there is no further comments on v9, I'll send a refreshed v10 with this enum issue fixed.
thanks,
Chenyu
Rafael J. Wysocki
maintainerRe: [PATCH v8 1/4] efi: Introduce EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER and corresponding structures
In reply to Chen YuQuoted 7 lines · Chen Yu
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:
Quoted 8 lines
EFI_CAPSULE_HEADEREFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADEREFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADEREFI_FIRMWARE_IMAGE_AUTHENTICATIONThese 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.
Why does the driver need to do that?
The firmware will reject the update if the version is invalid anyway, won't it?
Quoted 3 lines
The
EFI_CAPSULE_HEADERhas been defined in the
kernel, however the rest are not, thus introduce corresponding UEFI structures accordingly.
I would change the above in the following way:
"EFI_CAPSULE_HEADER has been defined in the kernel, but the other structures have not been defined yet, so do that."
Quoted 9 lines
Besides,
EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER
andEFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADERare required to be packed in the uefi specification.Ard has pointed out that, 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__packedattribute may be appropriate to ensure that the second one is not accessed using misaligned loads and stores.
"For this reason, use the __packed attribute to indicate to the compiler that the entire structure can appear misaligned in memory (as suggested by Ard) in case one of them follows the other directly in a capsule header."
Quoted 18 lines
Signed-off-by Chen Yu <[email protected]>---
v8: Useefi_guid_tinstead ofguid_t. (Andy Shevchenko)
v7: Use__packedinstead of pragma pack(1). (Greg Kroah-Hartman, Ard Biesheuve)
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 | 46 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)include/linux/efi.h +0
-0@@ -148,6 +148,52 @@ typedef struct {u32 imagesize;
include/linux/efi.h typedef struct { · line 158
Quoted 10 lines
include/linux/efi.h +8
-0} efi_capsule_header_t; +/* EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER */ +struct efi_manage_capsule_header { +u32 ver; +u16 emb_drv_cnt; +u16 payload_cnt; +/* +* Variable array indicated by number of +* (emb_drv_cnt + payload_cnt)
* Variable-size array of the size given by the sum of
* emb_drv_cnt and payload_cnt.
include/linux/efi.h typedef struct { · line 171
Quoted 13 lines
include/linux/efi.h +13
-0+*/ +u64 offset_list[]; +} __packed; + +/* EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER */ +struct efi_manage_capsule_image_header { +u32 ver; +efi_guid_t image_type_id; +u8 image_index; +u8 reserved_bytes[3 ]; +u32 image_size; +u32 vendor_code_size; +/* ver = 2. */
What does this mean?
include/linux/efi.h typedef struct { · line 173
Quoted 2 lines
include/linux/efi.h +2
-0+u64 hw_ins; +/* ver = v3. */
And same here?
Quoted 8 lines
include/linux/efi.h +8
-0+u64 capsule_support; +} __packed; + +/* WIN_CERTIFICATE */ +struct win_cert { +u32 len; +u16 rev; +u16 cert_type;
include/linux/efi.h typedef struct { · line 191
Quoted 10 lines
include/linux/efi.h +10
-0+}; + +/* WIN_CERTIFICATE_UEFI_GUID */ +struct win_cert_uefi_guid { +struct win_cert hdr; +efi_guid_t cert_type; +u8 cert_data[]; +}; + +/* EFI_FIRMWARE_IMAGE_AUTHENTICATIO */
The "N" character at the end is missing.
Quoted 9 lines
include/linux/efi.h +5
-1+struct efi_image_auth { +u64 mon_count; +struct win_cert_uefi_guid auth_info; +}; +/* * EFI capsule flags */ --
Re: [PATCH v8 1/4] efi: Introduce EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER and corresponding structures
Hi Rafael,
Quoted 13 lines · Rafael J. Wysocki
Quoted 12 lines · Chen Yu
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_HEADEREFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADEREFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADEREFI_FIRMWARE_IMAGE_AUTHENTICATION
Quoted 8 lines
Quoted 3 lines
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.
Why does the driver need to do that?
The firmware will reject the update if the version is invalid anyway, won't it?
Yes, the firmware will reject the update if the version does not match. The motivation of checking it in kernel before the firmware is mainly to deal with a corner case that, if the user provides an invalid capsule image, the kernel could be used as a guard to reject it, without switching to the MM update mode(which might be costly).
Quoted 9 lines
Quoted 3 lines
The
EFI_CAPSULE_HEADERhas been defined in the
kernel, however the rest are not, thus introduce corresponding UEFI structures accordingly.I would change the above in the following way:
"
EFI_CAPSULE_HEADERhas been defined in the kernel, but the other structures have not been defined yet, so do that."
Ok, will do.
Quoted 10 lines
Quoted 9 lines
Besides,
EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER
andEFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADERare required to be packed in the uefi specification.Ard has pointed out that, 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__packedattribute may be appropriate to ensure that the second one is not accessed using misaligned loads and stores.
Quoted 5 lines
"For this reason, use the
__packedattribute to indicate to the compiler that the entire structure can appear misaligned in memory (as suggested by Ard) in case one of them follows the other directly in a capsule header."
Ok, will do.
Quoted 20 lines
Quoted 20 lines
Signed-off-by Chen Yu <[email protected]>---
v8: Useefi_guid_tinstead ofguid_t. (Andy Shevchenko)
v7: Use__packedinstead of pragma pack(1). (Greg Kroah-Hartman, Ard Biesheuve)
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 | 46 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)include/linux/efi.h +0
-0@@ -148,6 +148,52 @@ typedef struct {u32 imagesize; } efi_capsule_header_t;
Quoted 12 lines
include/linux/efi.h typedef struct { · line 158
Quoted 8 lines
include/linux/efi.h +8
-0+/* EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER */ +struct efi_manage_capsule_header { +u32 ver; +u16 emb_drv_cnt; +u16 payload_cnt; +/* +* Variable array indicated by number of +* (emb_drv_cnt + payload_cnt) * Variable-size array of the size given by the sum of
*emb_drv_cntandpayload_cnt.
Ok, will change it.
Quoted 16 lines
include/linux/efi.h typedef struct { · line 171
Quoted 13 lines
include/linux/efi.h +13
-0+*/ +u64 offset_list[]; +} __packed; + +/* EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER */ +struct efi_manage_capsule_image_header { +u32 ver; +efi_guid_t image_type_id; +u8 image_index; +u8 reserved_bytes[3 ]; +u32 image_size; +u32 vendor_code_size; +/* ver = 2. */ What does this mean?
Quoted 5 lines
include/linux/efi.h typedef struct { · line 173
Quoted 2 lines
include/linux/efi.h +2
-0+u64 hw_ins; +/* ver = v3. */ And same here?
The hw_ins was introduced in version 2, and capsule_support was introduced in version 3 of the capsule image format. I'll revise the comment in next version.
Quoted 12 lines
Quoted 12 lines
include/linux/efi.h +12
-0+u64 capsule_support; +} __packed; + +/* WIN_CERTIFICATE */ +struct win_cert { +u32 len; +u16 rev; +u16 cert_type; +}; + +/* WIN_CERTIFICATE_UEFI_GUID */ +struct win_cert_uefi_guid {
Quoted 9 lines
include/linux/efi.h typedef struct { · line 191
Quoted 6 lines
include/linux/efi.h +6
-0+struct win_cert hdr; +efi_guid_t cert_type; +u8 cert_data[]; +}; + +/* EFI_FIRMWARE_IMAGE_AUTHENTICATIO */ The "N" character at the end is missing.
Ok, will fix it.
thanks,
Chenyu
Rafael J. Wysocki
maintainerRe: [PATCH v8 1/4] efi: Introduce EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER and corresponding structures
Quoted 20 lines · Chen Yu
Hi Rafael,
Quoted 17 lines · Rafael J. Wysocki
Quoted 15 lines · Chen Yu
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_HEADEREFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADEREFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADEREFI_FIRMWARE_IMAGE_AUTHENTICATIONThese 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.
Quoted 8 lines
Quoted 4 lines
Why does the driver need to do that?
The firmware will reject the update if the version is invalid anyway, won't it?
Yes, the firmware will reject the update if the version does not match. The motivation of checking it in kernel before the firmware is mainly to deal with a corner case that, if the user provides an invalid capsule image, the kernel could be used as a guard to reject it, without switching to the MM update mode(which might be costly).
OK, but it would be good to mention this somewhere, preferably in he changelog and maybe also in a comment next to the related code.
Quoted 75 lines
Quoted 9 lines
Quoted 3 lines
The
EFI_CAPSULE_HEADERhas been defined in the
kernel, however the rest are not, thus introduce corresponding UEFI structures accordingly.I would change the above in the following way:
"
EFI_CAPSULE_HEADERhas been defined in the kernel, but the other structures have not been defined yet, so do that."Ok, will do.
Quoted 10 lines
Quoted 9 lines
Besides,
EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER
andEFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADERare required to be packed in the uefi specification.Ard has pointed out that, 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__packedattribute may be appropriate to ensure that the second one is not accessed using misaligned loads and stores.Quoted 5 lines
"For this reason, use the
__packedattribute to indicate to the compiler that the entire structure can appear misaligned in memory (as suggested by Ard) in case one of them follows the other directly in a capsule header."Ok, will do.
Quoted 20 lines
Quoted 20 lines
Signed-off-by Chen Yu <[email protected]>---
v8: Useefi_guid_tinstead ofguid_t. (Andy Shevchenko)
v7: Use__packedinstead of pragma pack(1). (Greg Kroah-Hartman, Ard Biesheuve)
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 | 46 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)include/linux/efi.h +0
-0@@ -148,6 +148,52 @@ typedef struct {u32 imagesize; } efi_capsule_header_t;Quoted 12 lines
include/linux/efi.h typedef struct { · line 158
Quoted 8 lines
include/linux/efi.h +8
-0+/* EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER */ +struct efi_manage_capsule_header { +u32 ver; +u16 emb_drv_cnt; +u16 payload_cnt; +/* +* Variable array indicated by number of +* (emb_drv_cnt + payload_cnt) * Variable-size array of the size given by the sum of
*emb_drv_cntandpayload_cnt.Ok, will change it.
Quoted 16 lines
include/linux/efi.h typedef struct { · line 171
Quoted 13 lines
include/linux/efi.h +13
-0+*/ +u64 offset_list[]; +} __packed; + +/* EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER */ +struct efi_manage_capsule_image_header { +u32 ver; +efi_guid_t image_type_id; +u8 image_index; +u8 reserved_bytes[3 ]; +u32 image_size; +u32 vendor_code_size; +/* ver = 2. */ What does this mean?
Quoted 8 lines
Quoted 5 lines
include/linux/efi.h typedef struct { · line 173
Quoted 2 lines
include/linux/efi.h +2
-0+u64 hw_ins; +/* ver = v3. */ And same here?
The
hw_inswas introduced in version 2, andcapsule_supportwas introduced in version 3 of the capsule image format. I'll revise the comment in next version.
Please do.