* [PATCH v11 0/4] Introduce Platform Firmware Runtime Update and Telemetry drivers
@ 2021-11-21 15:15 Chen Yu
2021-11-21 15:16 ` [PATCH v11 1/4] efi: Introduce EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER and corresponding structures Chen Yu
` (3 more replies)
0 siblings, 4 replies; 17+ messages in thread
From: Chen Yu @ 2021-11-21 15:15 UTC (permalink / raw)
To: linux-acpi
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Ard Biesheuvel, Len Brown,
Ashok Raj, Andy Shevchenko, Mike Rapoport, Chen Yu, linux-kernel
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 v10 to v11:
- Revise the commit log to explain why version check is introduced
in kernel rather than letting Management Mode to do it.
(Rafael J. Wysocki)
Revise the commit log to better describe the pack attribute.
(Rafael J. Wysocki)
Refine the comment for hw_ins and capsule_support.
(Rafael J. Wysocki)
- Change from v9 to v10:
- Remove the explicit assignment of the last item of enum.
(Andy Shevchenko)
- Change from v8 to v9:
- Use GUID_INIT() instead of guid_parse() during boot up.
(Andy Shevchenko)
- Drop uuid, code_uuid, drv_uuid in struct pfru_device as they
are not needed. (Andy Shevchenko)
- Drop type casting from void * in valid_version().
(Andy Shevchenko)
- Use kfree() instead of ACPI_FREE() in non-ACPICA usage.
(Andy Shevchenko)
- Use sizeof(rev) instead of sizeof(u32) in copy_from_user().
(Andy Shevchenko)
- Generate physical address from MSB part to LSB.
(Andy Shevchenko)
- Use devm_add_action_or_reset() to add ida release into dev resource
management. (Andy Shevchenko)
- Use devm_kasprintf() instead of kasprintf() to format the
pfru_dev name.(Andy Shevchenko)
- Remove redundant 0 in acpi_pfru_ids. (Andy Shevchenko)
- Adjust the order of included headers in pfru.h.
(Andy Shevchenko)
- Replace PFRU_MAGIC with PFRU_MAGIC_FOR_IOCTL in uapi file.
(Andy Shevchenko)
Use devm_kasprintf() instead of kasprintf() to format the
pfru_log_dev name.(Andy Shevchenko)
Remove redundant 0 in acpi_pfru_log_ids. (Andy Shevchenko)
- 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/tools/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 | 457 +++++++++++++
drivers/acpi/pfru/pfru_update.c | 601 ++++++++++++++++++
include/linux/efi.h | 46 ++
include/uapi/linux/pfru.h | 262 ++++++++
tools/power/acpi/.gitignore | 1 +
tools/power/acpi/Makefile | 16 +-
tools/power/acpi/Makefile.rules | 2 +-
tools/power/acpi/man/pfru.8 | 137 ++++
tools/power/acpi/tools/pfru/Makefile | 23 +
tools/power/acpi/tools/pfru/pfru.c | 417 ++++++++++++
15 files changed, 1971 insertions(+), 9 deletions(-)
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/man/pfru.8
create mode 100644 tools/power/acpi/tools/pfru/Makefile
create mode 100644 tools/power/acpi/tools/pfru/pfru.c
--
2.25.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v11 1/4] efi: Introduce EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER and corresponding structures
2021-11-21 15:15 [PATCH v11 0/4] Introduce Platform Firmware Runtime Update and Telemetry drivers Chen Yu
@ 2021-11-21 15:16 ` Chen Yu
2021-12-03 18:20 ` Rafael J. Wysocki
2021-11-21 15:16 ` [PATCH v11 2/4] drivers/acpi: Introduce Platform Firmware Runtime Update device driver Chen Yu
` (2 subsequent siblings)
3 siblings, 1 reply; 17+ messages in thread
From: Chen Yu @ 2021-11-21 15:16 UTC (permalink / raw)
To: linux-acpi
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Ard Biesheuvel, Len Brown,
Ashok Raj, Andy Shevchenko, Mike Rapoport, Chen Yu, linux-kernel,
linux-efi
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. In this way, if the user provides an invalid
capsule image, the kernel could be used as a guard to reject it, without
switching to the Management Mode (which might be costly).
EFI_CAPSULE_HEADER has been defined in the kernel, but the other
structures have not been defined yet, so do that. Besides,
EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER and
EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER are required to be packed
in the uefi specification. 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.
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
v11:Add explanation on why version check is introduced
in kernel rather than letting Management Mode to do it.
(Rafael J. Wysocki)
Revise the commit log to better describe the pack attribute.
(Rafael J. Wysocki)
Refine the comment for hw_ins and capsule_support.
(Rafael J. Wysocki)
v10:Remove the explicit assignment of the last item of enum.
(Andy Shevchenko)
v9: Use GUID_INIT() instead of guid_parse() during boot up.
(Andy Shevchenko)
Drop uuid, code_uuid, drv_uuid in struct pfru_device as they
are not needed. (Andy Shevchenko)
Drop type casting from void * in valid_version().
(Andy Shevchenko)
Use kfree() instead of ACPI_FREE() in non-ACPICA usage.
(Andy Shevchenko)
Use sizeof(rev) instead of sizeof(u32) in copy_from_user().
(Andy Shevchenko)
Generate physical address from MSB part to LSB.
(Andy Shevchenko)
Use devm_add_action_or_reset() to add ida release into dev resource
management. (Andy Shevchenko)
Use devm_kasprintf() instead of kasprintf() to format the
pfru_dev name.(Andy Shevchenko)
Remove redundant 0 in acpi_pfru_ids. (Andy Shevchenko)
Adjust the order of included headers in pfru.h.
(Andy Shevchenko)
Replace PFRU_MAGIC with PFRU_MAGIC_FOR_IOCTL in uapi file.
(Andy Shevchenko)
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)
---
include/linux/efi.h | 46 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 46 insertions(+)
diff --git a/include/linux/efi.h b/include/linux/efi.h
index dbd39b20e034..80e970f7e6f8 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -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-size array of the size given by the sum of
+ * emb_drv_cnt and 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;
+ /* hw_ins was introduced in version 2 */
+ u64 hw_ins;
+ /* capsule_support was introduced in version 3 */
+ 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_AUTHENTICATION */
+struct efi_image_auth {
+ u64 mon_count;
+ struct win_cert_uefi_guid auth_info;
+};
+
/*
* EFI capsule flags
*/
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v11 2/4] drivers/acpi: Introduce Platform Firmware Runtime Update device driver
2021-11-21 15:15 [PATCH v11 0/4] Introduce Platform Firmware Runtime Update and Telemetry drivers Chen Yu
2021-11-21 15:16 ` [PATCH v11 1/4] efi: Introduce EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER and corresponding structures Chen Yu
@ 2021-11-21 15:16 ` Chen Yu
2021-12-03 19:37 ` Rafael J. Wysocki
2021-11-21 15:16 ` [PATCH v11 3/4] drivers/acpi: Introduce Platform Firmware Runtime Update Telemetry Chen Yu
2021-11-21 15:17 ` [PATCH v11 4/4] tools: Introduce power/acpi/tools/pfru Chen Yu
3 siblings, 1 reply; 17+ messages in thread
From: Chen Yu @ 2021-11-21 15:16 UTC (permalink / raw)
To: linux-acpi
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Ard Biesheuvel, Len Brown,
Ashok Raj, Andy Shevchenko, Mike Rapoport, Chen Yu, linux-kernel
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/tools/pfru.
Link: https://uefi.org/sites/default/files/resources/Intel_MM_OS_Interface_Spec_Rev100.pdf # [1]
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
v11:Revise the comment to explain why version check is introduced
in kernel rather than letting Management Mode to do it.
(Rafael J. Wysocki)
v10:Remove the explicit assignment of the last item of enum.
(Andy Shevchenko)
v9: Use GUID_INIT() instead of guid_parse() during boot up.
(Andy Shevchenko)
Drop uuid, code_uuid, drv_uuid in struct pfru_device as they
are not needed. (Andy Shevchenko)
Drop type casting from void * in valid_version().
(Andy Shevchenko)
Use kfree() instead of ACPI_FREE() in non-ACPICA usage.
(Andy Shevchenko)
Use sizeof(rev) instead of sizeof(u32) in copy_from_user().
(Andy Shevchenko)
Generate physical address from MSB part to LSB.
(Andy Shevchenko)
Use devm_add_action_or_reset() to add ida release into dev resource
management. (Andy Shevchenko)
Use devm_kasprintf() instead of kasprintf() to format the
pfru_dev name.(Andy Shevchenko)
Remove redundant 0 in acpi_pfru_ids. (Andy Shevchenko)
Adjust the order of included headers in pfru.h.
(Andy Shevchenko)
Replace PFRU_MAGIC with PFRU_MAGIC_FOR_IOCTL in uapi file.
(Andy Shevchenko)
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 | 601 ++++++++++++++++++
include/uapi/linux/pfru.h | 174 +++++
7 files changed, 793 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
diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
index cfe6cccf0f44..adf830410e0d 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -367,6 +367,7 @@ Code Seq# Include File Comments
<mailto:aherrman@de.ibm.com>
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:thomas@winischhofer.net>
0xF6 all LTTng Linux Trace Toolkit Next Generation
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index cdbdf68bd98f..1cf26e404b63 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -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
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 3018714e87d9..9c2c5ddff6ec 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -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
diff --git a/drivers/acpi/pfru/Kconfig b/drivers/acpi/pfru/Kconfig
new file mode 100644
index 000000000000..fd0a41b88fac
--- /dev/null
+++ b/drivers/acpi/pfru/Kconfig
@@ -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.
diff --git a/drivers/acpi/pfru/Makefile b/drivers/acpi/pfru/Makefile
new file mode 100644
index 000000000000..098cbe80cf3d
--- /dev/null
+++ b/drivers/acpi/pfru/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_ACPI_PFRU) += pfru_update.o
diff --git a/drivers/acpi/pfru/pfru_update.c b/drivers/acpi/pfru/pfru_update.c
new file mode 100644
index 000000000000..cf670d052cc9
--- /dev/null
+++ b/drivers/acpi/pfru/pfru_update.c
@@ -0,0 +1,601 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ACPI Platform Firmware Runtime Update Device Driver
+ *
+ * Copyright (C) 2021 Intel Corporation
+ * Author: Chen Yu <yu.c.chen@intel.com>
+ */
+#include <linux/acpi.h>
+#include <linux/device.h>
+#include <linux/efi.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/file.h>
+#include <linux/fs.h>
+#include <linux/idr.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/string.h>
+#include <linux/uaccess.h>
+#include <linux/uio.h>
+#include <linux/uuid.h>
+
+#include <uapi/linux/pfru.h>
+
+#define PFRU_FUNC_STANDARD_QUERY 0
+#define PFRU_FUNC_QUERY_UPDATE_CAP 1
+#define PFRU_FUNC_QUERY_BUF 2
+#define PFRU_FUNC_START 3
+
+#define PFRU_CODE_INJECT_TYPE 1
+#define PFRU_DRIVER_UPDATE_TYPE 2
+
+#define PFRU_REVID_1 1
+#define PFRU_REVID_2 2
+#define PFRU_DEFAULT_REV_ID PFRU_REVID_1
+
+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,
+ CAP_NR_IDX
+};
+
+enum buf_index {
+ BUF_STATUS_IDX = 0,
+ BUF_EXT_STATUS_IDX = 1,
+ BUF_ADDR_LOW_IDX = 2,
+ BUF_ADDR_HI_IDX = 3,
+ BUF_SIZE_IDX = 4,
+ BUF_NR_IDX
+};
+
+enum update_index {
+ UPDATE_STATUS_IDX = 0,
+ UPDATE_EXT_STATUS_IDX = 1,
+ UPDATE_AUTH_TIME_LOW_IDX = 2,
+ UPDATE_AUTH_TIME_HI_IDX = 3,
+ UPDATE_EXEC_TIME_LOW_IDX = 4,
+ UPDATE_EXEC_TIME_HI_IDX = 5,
+ UPDATE_NR_IDX
+};
+
+enum pfru_start_action {
+ START_STAGE = 0,
+ START_ACTIVATE = 1,
+ START_STAGE_ACTIVATE = 2,
+};
+
+struct pfru_device {
+ u32 rev_id, index;
+ struct device *parent_dev;
+ struct miscdevice miscdev;
+};
+
+static DEFINE_IDA(pfru_ida);
+
+static const guid_t pfru_guid =
+ GUID_INIT(0xECF9533B, 0x4A3C, 0x4E89, 0x93, 0x9E, 0xC7, 0x71,
+ 0x12, 0x60, 0x1C, 0x6D);
+
+static const guid_t pfru_code_inj_guid =
+ GUID_INIT(0xB2F84B79, 0x7B6E, 0x4E45, 0x88, 0x5F, 0x3F, 0xB9,
+ 0xBB, 0x18, 0x54, 0x02);
+
+static const guid_t pfru_drv_update_guid =
+ GUID_INIT(0x4569DD8C, 0x75F1, 0x429A, 0xA3, 0xD6, 0x24, 0xDE,
+ 0x80, 0x97, 0xA0, 0xDF);
+
+static inline int pfru_valid_revid(u32 id)
+{
+ return id == PFRU_REVID_1 || id == PFRU_REVID_2;
+}
+
+static inline struct pfru_device *to_pfru_dev(struct file *file)
+{
+ return container_of(file->private_data, struct pfru_device, miscdev);
+}
+
+static int query_capability(struct pfru_update_cap_info *cap_hdr,
+ struct pfru_device *pfru_dev)
+{
+ acpi_handle handle = ACPI_HANDLE(pfru_dev->parent_dev);
+ union acpi_object *out_obj;
+ int ret = -EINVAL;
+
+ out_obj = acpi_evaluate_dsm_typed(handle, &pfru_guid,
+ pfru_dev->rev_id,
+ PFRU_FUNC_QUERY_UPDATE_CAP,
+ NULL, ACPI_TYPE_PACKAGE);
+ if (!out_obj)
+ return ret;
+
+ if (out_obj->package.count < CAP_NR_IDX)
+ goto free_acpi_buffer;
+
+ if (out_obj->package.elements[CAP_STATUS_IDX].type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ cap_hdr->status = out_obj->package.elements[CAP_STATUS_IDX].integer.value;
+
+ if (out_obj->package.elements[CAP_UPDATE_IDX].type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ cap_hdr->update_cap = out_obj->package.elements[CAP_UPDATE_IDX].integer.value;
+
+ if (out_obj->package.elements[CAP_CODE_TYPE_IDX].type != ACPI_TYPE_BUFFER)
+ goto free_acpi_buffer;
+
+ memcpy(&cap_hdr->code_type,
+ out_obj->package.elements[CAP_CODE_TYPE_IDX].buffer.pointer,
+ out_obj->package.elements[CAP_CODE_TYPE_IDX].buffer.length);
+
+ if (out_obj->package.elements[CAP_FW_VER_IDX].type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ cap_hdr->fw_version =
+ out_obj->package.elements[CAP_FW_VER_IDX].integer.value;
+
+ if (out_obj->package.elements[CAP_CODE_RT_VER_IDX].type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ cap_hdr->code_rt_version =
+ out_obj->package.elements[CAP_CODE_RT_VER_IDX].integer.value;
+
+ if (out_obj->package.elements[CAP_DRV_TYPE_IDX].type != ACPI_TYPE_BUFFER)
+ goto free_acpi_buffer;
+
+ memcpy(&cap_hdr->drv_type,
+ out_obj->package.elements[CAP_DRV_TYPE_IDX].buffer.pointer,
+ out_obj->package.elements[CAP_DRV_TYPE_IDX].buffer.length);
+
+ if (out_obj->package.elements[CAP_DRV_RT_VER_IDX].type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ cap_hdr->drv_rt_version =
+ out_obj->package.elements[CAP_DRV_RT_VER_IDX].integer.value;
+
+ if (out_obj->package.elements[CAP_DRV_SVN_IDX].type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ cap_hdr->drv_svn =
+ out_obj->package.elements[CAP_DRV_SVN_IDX].integer.value;
+
+ if (out_obj->package.elements[CAP_PLAT_ID_IDX].type != ACPI_TYPE_BUFFER)
+ goto free_acpi_buffer;
+
+ memcpy(&cap_hdr->platform_id,
+ out_obj->package.elements[CAP_PLAT_ID_IDX].buffer.pointer,
+ out_obj->package.elements[CAP_PLAT_ID_IDX].buffer.length);
+
+ if (out_obj->package.elements[CAP_OEM_ID_IDX].type != ACPI_TYPE_BUFFER)
+ goto free_acpi_buffer;
+
+ memcpy(&cap_hdr->oem_id,
+ out_obj->package.elements[CAP_OEM_ID_IDX].buffer.pointer,
+ out_obj->package.elements[CAP_OEM_ID_IDX].buffer.length);
+
+ if (out_obj->package.elements[CAP_OEM_INFO_IDX].type != ACPI_TYPE_BUFFER)
+ goto free_acpi_buffer;
+
+ cap_hdr->oem_info_len =
+ out_obj->package.elements[CAP_OEM_INFO_IDX].buffer.length;
+
+ ret = 0;
+
+free_acpi_buffer:
+ kfree(out_obj);
+
+ return ret;
+}
+
+static int query_buffer(struct pfru_com_buf_info *info,
+ struct pfru_device *pfru_dev)
+{
+ acpi_handle handle = ACPI_HANDLE(pfru_dev->parent_dev);
+ union acpi_object *out_obj;
+ int ret = -EINVAL;
+
+ out_obj = acpi_evaluate_dsm_typed(handle, &pfru_guid,
+ pfru_dev->rev_id, PFRU_FUNC_QUERY_BUF,
+ NULL, ACPI_TYPE_PACKAGE);
+ if (!out_obj)
+ return ret;
+
+ if (out_obj->package.count < BUF_NR_IDX)
+ goto free_acpi_buffer;
+
+ if (out_obj->package.elements[BUF_STATUS_IDX].type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ info->status = out_obj->package.elements[BUF_STATUS_IDX].integer.value;
+
+ if (out_obj->package.elements[BUF_EXT_STATUS_IDX].type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ info->ext_status =
+ out_obj->package.elements[BUF_EXT_STATUS_IDX].integer.value;
+
+ if (out_obj->package.elements[BUF_ADDR_LOW_IDX].type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ info->addr_lo =
+ out_obj->package.elements[BUF_ADDR_LOW_IDX].integer.value;
+
+ if (out_obj->package.elements[BUF_ADDR_HI_IDX].type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ info->addr_hi =
+ out_obj->package.elements[BUF_ADDR_HI_IDX].integer.value;
+
+ if (out_obj->package.elements[BUF_SIZE_IDX].type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ info->buf_size = out_obj->package.elements[BUF_SIZE_IDX].integer.value;
+
+ ret = 0;
+
+free_acpi_buffer:
+ kfree(out_obj);
+
+ return ret;
+}
+
+static int get_image_type(const struct efi_manage_capsule_image_header *img_hdr,
+ struct pfru_device *pfru_dev)
+{
+ const efi_guid_t *image_type_id = &img_hdr->image_type_id;
+
+ /* check whether this is a code injection or driver update */
+ if (guid_equal(image_type_id, &pfru_code_inj_guid))
+ return PFRU_CODE_INJECT_TYPE;
+
+ if (guid_equal(image_type_id, &pfru_drv_update_guid))
+ return PFRU_DRIVER_UPDATE_TYPE;
+
+ return -EINVAL;
+}
+
+static int adjust_efi_size(const struct efi_manage_capsule_image_header *img_hdr,
+ int size)
+{
+ /*
+ * The (u64 hw_ins) was introduced in UEFI spec version 2,
+ * and (u64 capsule_support) was introduced in version 3.
+ * The size needs to be adjusted accordingly. That is to
+ * say, version 1 should subtract the size of hw_ins+capsule_support,
+ * and version 2 should sbstract the size of capsule_support.
+ */
+ size += sizeof(struct efi_manage_capsule_image_header);
+ switch (img_hdr->ver) {
+ case 1:
+ return size - 2 * sizeof(u64);
+ case 2:
+ return size - sizeof(u64);
+ default:
+ /* only support version 1 and 2 */
+ return -EINVAL;
+ }
+}
+
+static bool valid_version(const void *data, struct pfru_update_cap_info *cap,
+ struct pfru_device *pfru_dev)
+{
+ struct pfru_payload_hdr *payload_hdr;
+ const efi_capsule_header_t *cap_hdr = data;
+ const struct efi_manage_capsule_header *m_hdr;
+ const struct efi_manage_capsule_image_header *m_img_hdr;
+ const struct efi_image_auth *auth;
+ int type, size;
+
+ /*
+ * Sanity check if the capsule image has a newer version
+ * than current one. In this way, 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)
+ */
+ size = cap_hdr->headersize;
+ m_hdr = data + size;
+ /*
+ * Current data structure size plus variable array indicated
+ * by number of (emb_drv_cnt + payload_cnt)
+ */
+ size += offsetof(struct efi_manage_capsule_header, offset_list) +
+ (m_hdr->emb_drv_cnt + m_hdr->payload_cnt) * sizeof(u64);
+ m_img_hdr = data + size;
+
+ type = get_image_type(m_img_hdr, pfru_dev);
+ if (type < 0)
+ return false;
+
+ size = adjust_efi_size(m_img_hdr, size);
+ if (size < 0)
+ return false;
+
+ auth = data + size;
+ size += sizeof(u64) + auth->auth_info.hdr.len;
+ payload_hdr = (struct pfru_payload_hdr *)(data + size);
+
+ /* finally compare the version */
+ if (type == PFRU_CODE_INJECT_TYPE)
+ return payload_hdr->rt_ver >= cap->code_rt_version;
+ else
+ return payload_hdr->rt_ver >= cap->drv_rt_version;
+}
+
+static void dump_update_result(struct pfru_updated_result *result,
+ struct pfru_device *pfru_dev)
+{
+ dev_dbg(pfru_dev->parent_dev, "Update result:\n");
+ dev_dbg(pfru_dev->parent_dev, "Status:%d\n", result->status);
+ dev_dbg(pfru_dev->parent_dev, "Extended Status:%d\n", result->ext_status);
+ dev_dbg(pfru_dev->parent_dev, "Authentication Time Low:%lld\n",
+ result->low_auth_time);
+ dev_dbg(pfru_dev->parent_dev, "Authentication Time High:%lld\n",
+ result->high_auth_time);
+ dev_dbg(pfru_dev->parent_dev, "Execution Time Low:%lld\n",
+ result->low_exec_time);
+ dev_dbg(pfru_dev->parent_dev, "Execution Time High:%lld\n",
+ result->high_exec_time);
+}
+
+static int start_acpi_update(int action, struct pfru_device *pfru_dev)
+{
+ union acpi_object *out_obj, in_obj, in_buf;
+ struct pfru_updated_result update_result;
+ acpi_handle handle;
+ int ret = -EINVAL;
+
+ memset(&in_obj, 0, sizeof(in_obj));
+ memset(&in_buf, 0, sizeof(in_buf));
+ in_obj.type = ACPI_TYPE_PACKAGE;
+ in_obj.package.count = 1;
+ in_obj.package.elements = &in_buf;
+ in_buf.type = ACPI_TYPE_INTEGER;
+ in_buf.integer.value = action;
+
+ handle = ACPI_HANDLE(pfru_dev->parent_dev);
+ out_obj = acpi_evaluate_dsm_typed(handle, &pfru_guid,
+ pfru_dev->rev_id, PFRU_FUNC_START,
+ &in_obj, ACPI_TYPE_PACKAGE);
+ if (!out_obj)
+ return ret;
+
+ if (out_obj->package.count < UPDATE_NR_IDX)
+ goto free_acpi_buffer;
+
+ if (out_obj->package.elements[UPDATE_STATUS_IDX].type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ update_result.status =
+ out_obj->package.elements[UPDATE_STATUS_IDX].integer.value;
+
+ if (out_obj->package.elements[UPDATE_EXT_STATUS_IDX].type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ update_result.ext_status =
+ out_obj->package.elements[UPDATE_EXT_STATUS_IDX].integer.value;
+
+ if (out_obj->package.elements[UPDATE_AUTH_TIME_LOW_IDX].type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ update_result.low_auth_time =
+ out_obj->package.elements[UPDATE_AUTH_TIME_LOW_IDX].integer.value;
+
+ if (out_obj->package.elements[UPDATE_AUTH_TIME_HI_IDX].type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ update_result.high_auth_time =
+ out_obj->package.elements[UPDATE_AUTH_TIME_HI_IDX].integer.value;
+
+ if (out_obj->package.elements[UPDATE_EXEC_TIME_LOW_IDX].type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ update_result.low_exec_time =
+ out_obj->package.elements[UPDATE_EXEC_TIME_LOW_IDX].integer.value;
+
+ if (out_obj->package.elements[UPDATE_EXEC_TIME_HI_IDX].type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ update_result.high_exec_time =
+ out_obj->package.elements[UPDATE_EXEC_TIME_HI_IDX].integer.value;
+
+ dump_update_result(&update_result, pfru_dev);
+ ret = 0;
+
+free_acpi_buffer:
+ kfree(out_obj);
+
+ return ret;
+}
+
+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) {
+ 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)))
+ return -EFAULT;
+
+ return 0;
+ case PFRU_IOC_SET_REV:
+ if (copy_from_user(&rev, p, sizeof(rev)))
+ 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;
+ }
+}
+
+static ssize_t pfru_write(struct file *file, const char __user *buf,
+ size_t len, loff_t *ppos)
+{
+ struct pfru_device *pfru_dev = to_pfru_dev(file);
+ struct pfru_update_cap_info cap;
+ struct pfru_com_buf_info buf_info;
+ phys_addr_t phy_addr;
+ struct iov_iter iter;
+ struct iovec iov;
+ char *buf_ptr;
+ int ret;
+
+ ret = query_buffer(&buf_info, pfru_dev);
+ if (ret)
+ return ret;
+
+ if (len > buf_info.buf_size)
+ return -EINVAL;
+
+ iov.iov_base = (void __user *)buf;
+ iov.iov_len = len;
+ iov_iter_init(&iter, WRITE, &iov, 1, len);
+
+ /* map the communication buffer */
+ phy_addr = (phys_addr_t)((buf_info.addr_hi << 32) | buf_info.addr_lo);
+ buf_ptr = memremap(phy_addr, buf_info.buf_size, MEMREMAP_WB);
+ if (IS_ERR(buf_ptr))
+ return PTR_ERR(buf_ptr);
+
+ if (!copy_from_iter_full(buf_ptr, len, &iter)) {
+ ret = -EINVAL;
+ goto unmap;
+ }
+
+ /* check if the capsule header has a valid version number */
+ ret = query_capability(&cap, pfru_dev);
+ if (ret)
+ goto unmap;
+
+ if (cap.status != DSM_SUCCEED)
+ ret = -EBUSY;
+ else if (!valid_version(buf_ptr, &cap, pfru_dev))
+ ret = -EINVAL;
+
+unmap:
+ memunmap(buf_ptr);
+
+ return ret ?: len;
+}
+
+static const struct file_operations acpi_pfru_fops = {
+ .owner = THIS_MODULE,
+ .write = pfru_write,
+ .unlocked_ioctl = pfru_ioctl,
+ .llseek = noop_llseek,
+};
+
+static int acpi_pfru_remove(struct platform_device *pdev)
+{
+ struct pfru_device *pfru_dev = platform_get_drvdata(pdev);
+
+ misc_deregister(&pfru_dev->miscdev);
+
+ return 0;
+}
+
+static void pfru_put_idx(void *data)
+{
+ struct pfru_device *pfru_dev = data;
+
+ ida_free(&pfru_ida, pfru_dev->index);
+}
+
+static int acpi_pfru_probe(struct platform_device *pdev)
+{
+ acpi_handle handle = ACPI_HANDLE(&pdev->dev);
+ struct pfru_device *pfru_dev;
+ int ret;
+
+ if (!acpi_has_method(handle, "_DSM")) {
+ dev_dbg(&pdev->dev, "Missing _DSM\n");
+ return -ENODEV;
+ }
+
+ pfru_dev = devm_kzalloc(&pdev->dev, sizeof(*pfru_dev), GFP_KERNEL);
+ if (!pfru_dev)
+ return -ENOMEM;
+
+ ret = ida_alloc(&pfru_ida, GFP_KERNEL);
+ if (ret < 0)
+ return ret;
+
+ pfru_dev->index = ret;
+ ret = devm_add_action_or_reset(&pdev->dev, pfru_put_idx, pfru_dev);
+ if (ret)
+ return ret;
+
+ pfru_dev->rev_id = PFRU_DEFAULT_REV_ID;
+ pfru_dev->parent_dev = &pdev->dev;
+
+ pfru_dev->miscdev.minor = MISC_DYNAMIC_MINOR;
+ pfru_dev->miscdev.name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
+ "pfru%d", pfru_dev->index);
+ if (!pfru_dev->miscdev.name)
+ return -ENOMEM;
+
+ pfru_dev->miscdev.nodename = devm_kasprintf(&pdev->dev, GFP_KERNEL,
+ "acpi_pfru%d", pfru_dev->index);
+ if (!pfru_dev->miscdev.nodename)
+ return -ENOMEM;
+
+ pfru_dev->miscdev.fops = &acpi_pfru_fops;
+ pfru_dev->miscdev.parent = &pdev->dev;
+
+ ret = misc_register(&pfru_dev->miscdev);
+ if (ret)
+ return ret;
+
+ platform_set_drvdata(pdev, pfru_dev);
+
+ return 0;
+}
+
+static const struct acpi_device_id acpi_pfru_ids[] = {
+ {"INTC1080"},
+ {}
+};
+MODULE_DEVICE_TABLE(acpi, acpi_pfru_ids);
+
+static struct platform_driver acpi_pfru_driver = {
+ .driver = {
+ .name = "pfru_update",
+ .acpi_match_table = acpi_pfru_ids,
+ },
+ .probe = acpi_pfru_probe,
+ .remove = acpi_pfru_remove,
+};
+module_platform_driver(acpi_pfru_driver);
+
+MODULE_DESCRIPTION("Platform Firmware Runtime Update device driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/uapi/linux/pfru.h b/include/uapi/linux/pfru.h
new file mode 100644
index 000000000000..fed50cb01309
--- /dev/null
+++ b/include/uapi/linux/pfru.h
@@ -0,0 +1,174 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Platform Firmware Runtime Update header
+ *
+ * Copyright(c) 2021 Intel Corporation. All rights reserved.
+ */
+#ifndef __PFRU_H__
+#define __PFRU_H__
+
+#include <linux/ioctl.h>
+#include <linux/types.h>
+
+#define PFRU_MAGIC_FOR_IOCTL 0xEE
+
+/**
+ * PFRU_IOC_SET_REV - _IOW(PFRU_MAGIC_FOR_IOCTL, 0x01, unsigned int)
+ *
+ * Return:
+ * * 0 - success
+ * * -EFAULT - fail to read the revision id
+ * * -EINVAL - user provides an invalid revision id
+ *
+ * Set the Revision ID for PFRU Runtime Update.
+ */
+#define PFRU_IOC_SET_REV _IOW(PFRU_MAGIC_FOR_IOCTL, 0x01, unsigned int)
+
+/**
+ * PFRU_IOC_STAGE - _IOW(PFRU_MAGIC_FOR_IOCTL, 0x02, unsigned int)
+ *
+ * Return:
+ * * 0 - success
+ * * -EINVAL - stage phase returns invalid result
+ *
+ * Stage a capsule image from communication buffer and perform authentication.
+ */
+#define PFRU_IOC_STAGE _IOW(PFRU_MAGIC_FOR_IOCTL, 0x02, unsigned int)
+
+/**
+ * PFRU_IOC_ACTIVATE - _IOW(PFRU_MAGIC_FOR_IOCTL, 0x03, unsigned int)
+ *
+ * Return:
+ * * 0 - success
+ * * -EINVAL - activate phase returns invalid result
+ *
+ * Activate a previous staged capsule image.
+ */
+#define PFRU_IOC_ACTIVATE _IOW(PFRU_MAGIC_FOR_IOCTL, 0x03, unsigned int)
+
+/**
+ * PFRU_IOC_STAGE_ACTIVATE - _IOW(PFRU_MAGIC_FOR_IOCTL, 0x04, unsigned int)
+ *
+ * Return:
+ * * 0 - success
+ * * -EINVAL - stage/activate phase returns invalid result.
+ *
+ * Perform both stage and activation action.
+ */
+#define PFRU_IOC_STAGE_ACTIVATE _IOW(PFRU_MAGIC_FOR_IOCTL, 0x04, unsigned int)
+
+/**
+ * PFRU_IOC_QUERY_CAP - _IOR(PFRU_MAGIC_FOR_IOCTL, 0x05,
+ * struct pfru_update_cap_info)
+ *
+ * Return:
+ * * 0 - success
+ * * -EINVAL - query phase returns invalid result
+ * * -EFAULT - the result fails to be copied to userspace
+ *
+ * Retrieve information about the PFRU Runtime Update capability.
+ * The information is a struct pfru_update_cap_info.
+ */
+#define PFRU_IOC_QUERY_CAP _IOR(PFRU_MAGIC_FOR_IOCTL, 0x05, struct pfru_update_cap_info)
+
+/**
+ * struct pfru_payload_hdr - Capsule file payload header.
+ *
+ * @sig: Signature of this capsule file.
+ * @hdr_version: Revision of this header structure.
+ * @hdr_size: Size of this header, including the OemHeader bytes.
+ * @hw_ver: The supported firmware version.
+ * @rt_ver: Version of the code injection image.
+ * @platform_id: A platform specific GUID to specify the platform what
+ * this capsule image support.
+ */
+struct pfru_payload_hdr {
+ __u32 sig;
+ __u32 hdr_version;
+ __u32 hdr_size;
+ __u32 hw_ver;
+ __u32 rt_ver;
+ __u8 platform_id[16];
+};
+
+enum pfru_dsm_status {
+ DSM_SUCCEED = 0,
+ DSM_FUNC_NOT_SUPPORT = 1,
+ DSM_INVAL_INPUT = 2,
+ DSM_HARDWARE_ERR = 3,
+ DSM_RETRY_SUGGESTED = 4,
+ DSM_UNKNOWN = 5,
+ DSM_FUNC_SPEC_ERR = 6,
+};
+
+/**
+ * struct pfru_update_cap_info - Runtime update capability information.
+ *
+ * @status: Indicator of whether this query succeed.
+ * @update_cap: Bitmap to indicate whether the feature is supported.
+ * @code_type: A buffer containing an image type GUID.
+ * @fw_version: Platform firmware version.
+ * @code_rt_version: Code injection runtime version for anti-rollback.
+ * @drv_type: A buffer containing an image type GUID.
+ * @drv_rt_version: The version of the driver update runtime code.
+ * @drv_svn: The secure version number(SVN) of the driver update runtime code.
+ * @platform_id: A buffer containing a platform ID GUID.
+ * @oem_id: A buffer containing an OEM ID GUID.
+ * @oem_info_len: Length of the buffer containing the vendor specific information.
+ */
+struct pfru_update_cap_info {
+ __u32 status;
+ __u32 update_cap;
+
+ __u8 code_type[16];
+ __u32 fw_version;
+ __u32 code_rt_version;
+
+ __u8 drv_type[16];
+ __u32 drv_rt_version;
+ __u32 drv_svn;
+
+ __u8 platform_id[16];
+ __u8 oem_id[16];
+
+ __u32 oem_info_len;
+};
+
+/**
+ * struct pfru_com_buf_info - Communication buffer information.
+ *
+ * @status: Indicator of whether this query succeed.
+ * @ext_status: Implementation specific query result.
+ * @addr_lo: Low 32bit physical address of the communication buffer to hold
+ * a runtime update package.
+ * @addr_hi: High 32bit physical address of the communication buffer to hold
+ * a runtime update package.
+ * @buf_size: Maximum size in bytes of the communication buffer.
+ */
+struct pfru_com_buf_info {
+ __u32 status;
+ __u32 ext_status;
+ __u64 addr_lo;
+ __u64 addr_hi;
+ __u32 buf_size;
+};
+
+/**
+ * struct pfru_updated_result - Platform firmware runtime update result information.
+ * @status: Indicator of whether this update succeed.
+ * @ext_status: Implementation specific update result.
+ * @low_auth_time: Low 32bit value of image authentication time in nanosecond.
+ * @high_auth_time: High 32bit value of image authentication time in nanosecond.
+ * @low_exec_time: Low 32bit value of image execution time in nanosecond.
+ * @high_exec_time: High 32bit value of image execution time in nanosecond.
+ */
+struct pfru_updated_result {
+ __u32 status;
+ __u32 ext_status;
+ __u64 low_auth_time;
+ __u64 high_auth_time;
+ __u64 low_exec_time;
+ __u64 high_exec_time;
+};
+
+#endif /* __PFRU_H__ */
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v11 3/4] drivers/acpi: Introduce Platform Firmware Runtime Update Telemetry
2021-11-21 15:15 [PATCH v11 0/4] Introduce Platform Firmware Runtime Update and Telemetry drivers Chen Yu
2021-11-21 15:16 ` [PATCH v11 1/4] efi: Introduce EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER and corresponding structures Chen Yu
2021-11-21 15:16 ` [PATCH v11 2/4] drivers/acpi: Introduce Platform Firmware Runtime Update device driver Chen Yu
@ 2021-11-21 15:16 ` Chen Yu
2021-12-07 19:44 ` Rafael J. Wysocki
2021-11-21 15:17 ` [PATCH v11 4/4] tools: Introduce power/acpi/tools/pfru Chen Yu
3 siblings, 1 reply; 17+ messages in thread
From: Chen Yu @ 2021-11-21 15:16 UTC (permalink / raw)
To: linux-acpi
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Ard Biesheuvel, Len Brown,
Ashok Raj, Andy Shevchenko, Mike Rapoport, Chen Yu, linux-kernel
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.
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.
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
v11:No change since v10.
v10:Remove the explicit assignment of the last item of enum.
(Andy Shevchenko)
v9: Use GUID_INIT() instead of guid_parse() during boot up.
(Andy Shevchenko)
Drop uuid from struct pfru_log_device.(Andy Shevchenko)
Use kfree() instead of ACPI_FREE() in non-ACPICA usage.
(Andy Shevchenko)
Use devm_add_action_or_reset() to add ida release into dev resource
management. (Andy Shevchenko)
Use devm_kasprintf() instead of kasprintf() to format the
pfru_log_dev name.(Andy Shevchenko)
Remove redundant 0 in acpi_pfru_log_ids. (Andy Shevchenko)
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 | 457 +++++++++++++++++++++++++++++
include/uapi/linux/pfru.h | 88 ++++++
3 files changed, 546 insertions(+), 1 deletion(-)
create mode 100644 drivers/acpi/pfru/pfru_telemetry.c
diff --git a/drivers/acpi/pfru/Makefile b/drivers/acpi/pfru/Makefile
index 098cbe80cf3d..9ba11efe10b3 100644
--- a/drivers/acpi/pfru/Makefile
+++ b/drivers/acpi/pfru/Makefile
@@ -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
diff --git a/drivers/acpi/pfru/pfru_telemetry.c b/drivers/acpi/pfru/pfru_telemetry.c
new file mode 100644
index 000000000000..5140a4591c9e
--- /dev/null
+++ b/drivers/acpi/pfru/pfru_telemetry.c
@@ -0,0 +1,457 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ACPI Platform Firmware Runtime Update Telemetry Driver
+ *
+ * Copyright (C) 2021 Intel Corporation
+ * Author: Chen Yu <yu.c.chen@intel.com>
+ */
+#include <linux/acpi.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/file.h>
+#include <linux/fs.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/mm.h>
+#include <linux/platform_device.h>
+#include <linux/string.h>
+#include <linux/uaccess.h>
+#include <linux/uio.h>
+#include <linux/uuid.h>
+
+#include <uapi/linux/pfru.h>
+
+#define PFRU_LOG_EXEC_IDX 0
+#define PFRU_LOG_HISTORY_IDX 1
+
+#define PFRU_LOG_ERR 0
+#define PFRU_LOG_WARN 1
+#define PFRU_LOG_INFO 2
+#define PFRU_LOG_VERB 4
+
+#define PFRU_FUNC_SET_LEV 1
+#define PFRU_FUNC_GET_LEV 2
+#define PFRU_FUNC_GET_DATA 3
+
+#define PFRU_REVID_1 1
+#define PFRU_REVID_2 2
+#define PFRU_DEFAULT_REV_ID PFRU_REVID_1
+
+enum log_index {
+ LOG_STATUS_IDX = 0,
+ LOG_EXT_STATUS_IDX = 1,
+ LOG_MAX_SZ_IDX = 2,
+ LOG_CHUNK1_LO_IDX = 3,
+ LOG_CHUNK1_HI_IDX = 4,
+ LOG_CHUNK1_SZ_IDX = 5,
+ LOG_CHUNK2_LO_IDX = 6,
+ LOG_CHUNK2_HI_IDX = 7,
+ LOG_CHUNK2_SZ_IDX = 8,
+ LOG_ROLLOVER_CNT_IDX = 9,
+ LOG_RESET_CNT_IDX = 10,
+ LOG_NR_IDX
+};
+
+struct pfru_log_device {
+ int index;
+ struct pfru_log_info info;
+ struct device *parent_dev;
+ struct miscdevice miscdev;
+};
+
+static const guid_t pfru_log_guid =
+ GUID_INIT(0x75191659, 0x8178, 0x4D9D, 0xB8, 0x8F, 0xAC, 0x5E,
+ 0x5E, 0x93, 0xE8, 0xBF);
+
+static DEFINE_IDA(pfru_log_ida);
+
+static inline struct pfru_log_device *to_pfru_log_dev(struct file *file)
+{
+ return container_of(file->private_data, struct pfru_log_device, miscdev);
+}
+
+static int get_pfru_log_data_info(struct pfru_log_data_info *data_info,
+ struct pfru_log_device *pfru_log_dev)
+{
+ acpi_handle handle = ACPI_HANDLE(pfru_log_dev->parent_dev);
+ union acpi_object *out_obj, in_obj, in_buf;
+ int ret = -EINVAL;
+
+ memset(&in_obj, 0, sizeof(in_obj));
+ memset(&in_buf, 0, sizeof(in_buf));
+ in_obj.type = ACPI_TYPE_PACKAGE;
+ in_obj.package.count = 1;
+ in_obj.package.elements = &in_buf;
+ in_buf.type = ACPI_TYPE_INTEGER;
+ in_buf.integer.value = pfru_log_dev->info.log_type;
+
+ out_obj = acpi_evaluate_dsm_typed(handle, &pfru_log_guid,
+ pfru_log_dev->info.log_revid, PFRU_FUNC_GET_DATA,
+ &in_obj, ACPI_TYPE_PACKAGE);
+ if (!out_obj)
+ return ret;
+
+ if (out_obj->package.count < LOG_NR_IDX)
+ goto free_acpi_buffer;
+
+ if (out_obj->package.elements[LOG_STATUS_IDX].type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ data_info->status = out_obj->package.elements[LOG_STATUS_IDX].integer.value;
+
+ if (out_obj->package.elements[LOG_EXT_STATUS_IDX].type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ data_info->ext_status =
+ out_obj->package.elements[LOG_EXT_STATUS_IDX].integer.value;
+
+ if (out_obj->package.elements[LOG_MAX_SZ_IDX].type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ data_info->max_data_size =
+ out_obj->package.elements[LOG_MAX_SZ_IDX].integer.value;
+
+ if (out_obj->package.elements[LOG_CHUNK1_LO_IDX].type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ data_info->chunk1_addr_lo =
+ out_obj->package.elements[LOG_CHUNK1_LO_IDX].integer.value;
+
+ if (out_obj->package.elements[LOG_CHUNK1_HI_IDX].type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ data_info->chunk1_addr_hi =
+ out_obj->package.elements[LOG_CHUNK1_HI_IDX].integer.value;
+
+ if (out_obj->package.elements[LOG_CHUNK1_SZ_IDX].type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ data_info->chunk1_size =
+ out_obj->package.elements[LOG_CHUNK1_SZ_IDX].integer.value;
+
+ if (out_obj->package.elements[LOG_CHUNK2_LO_IDX].type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ data_info->chunk2_addr_lo =
+ out_obj->package.elements[LOG_CHUNK2_LO_IDX].integer.value;
+
+ if (out_obj->package.elements[LOG_CHUNK2_HI_IDX].type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ data_info->chunk2_addr_hi =
+ out_obj->package.elements[LOG_CHUNK2_HI_IDX].integer.value;
+
+ if (out_obj->package.elements[LOG_CHUNK2_SZ_IDX].type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ data_info->chunk2_size =
+ out_obj->package.elements[LOG_CHUNK2_SZ_IDX].integer.value;
+
+ if (out_obj->package.elements[LOG_ROLLOVER_CNT_IDX].type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ data_info->rollover_cnt =
+ out_obj->package.elements[LOG_ROLLOVER_CNT_IDX].integer.value;
+
+ if (out_obj->package.elements[LOG_RESET_CNT_IDX].type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ data_info->reset_cnt =
+ out_obj->package.elements[LOG_RESET_CNT_IDX].integer.value;
+
+ ret = 0;
+
+free_acpi_buffer:
+ kfree(out_obj);
+
+ return ret;
+}
+
+static int set_pfru_log_level(int level, struct pfru_log_device *pfru_log_dev)
+{
+ acpi_handle handle = ACPI_HANDLE(pfru_log_dev->parent_dev);
+ union acpi_object *out_obj, *obj, in_obj, in_buf;
+ enum pfru_dsm_status status;
+ int ret = -EINVAL;
+
+ memset(&in_obj, 0, sizeof(in_obj));
+ memset(&in_buf, 0, sizeof(in_buf));
+ in_obj.type = ACPI_TYPE_PACKAGE;
+ in_obj.package.count = 1;
+ in_obj.package.elements = &in_buf;
+ in_buf.type = ACPI_TYPE_INTEGER;
+ in_buf.integer.value = level;
+
+ out_obj = acpi_evaluate_dsm_typed(handle, &pfru_log_guid,
+ pfru_log_dev->info.log_revid, PFRU_FUNC_SET_LEV,
+ &in_obj, ACPI_TYPE_PACKAGE);
+ if (!out_obj)
+ return -EINVAL;
+
+ obj = &out_obj->package.elements[0];
+ status = obj->integer.value;
+ if (status)
+ goto free_acpi_buffer;
+
+ obj = &out_obj->package.elements[1];
+ status = obj->integer.value;
+ if (status)
+ goto free_acpi_buffer;
+
+ ret = 0;
+
+free_acpi_buffer:
+ kfree(out_obj);
+
+ return ret;
+}
+
+static int get_pfru_log_level(struct pfru_log_device *pfru_log_dev)
+{
+ acpi_handle handle = ACPI_HANDLE(pfru_log_dev->parent_dev);
+ union acpi_object *out_obj, *obj;
+ enum pfru_dsm_status status;
+ int ret = -EINVAL;
+
+ out_obj = acpi_evaluate_dsm_typed(handle, &pfru_log_guid,
+ pfru_log_dev->info.log_revid, PFRU_FUNC_GET_LEV,
+ NULL, ACPI_TYPE_PACKAGE);
+ if (!out_obj)
+ return -EINVAL;
+
+ obj = &out_obj->package.elements[0];
+ if (obj->type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ status = obj->integer.value;
+ if (status)
+ goto free_acpi_buffer;
+
+ obj = &out_obj->package.elements[1];
+ if (obj->type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ status = obj->integer.value;
+ if (status)
+ goto free_acpi_buffer;
+
+ obj = &out_obj->package.elements[2];
+ if (obj->type != ACPI_TYPE_INTEGER)
+ goto free_acpi_buffer;
+
+ ret = obj->integer.value;
+
+free_acpi_buffer:
+ kfree(out_obj);
+
+ return ret;
+}
+
+static int valid_log_level(u32 level)
+{
+ return level == PFRU_LOG_ERR || level == PFRU_LOG_WARN ||
+ level == PFRU_LOG_INFO || level == PFRU_LOG_VERB;
+}
+
+static int valid_log_type(u32 type)
+{
+ return type == PFRU_LOG_EXEC_IDX || type == PFRU_LOG_HISTORY_IDX;
+}
+
+static inline int valid_log_revid(u32 id)
+{
+ return id == PFRU_REVID_1 || id == PFRU_REVID_2;
+}
+
+static long pfru_log_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ struct pfru_log_device *pfru_log_dev = to_pfru_log_dev(file);
+ struct pfru_log_data_info data_info;
+ struct pfru_log_info info;
+ void __user *p;
+ int ret = 0;
+
+ p = (void __user *)arg;
+
+ switch (cmd) {
+ case PFRU_LOG_IOC_SET_INFO:
+ if (copy_from_user(&info, p, sizeof(info)))
+ return -EFAULT;
+
+ if (valid_log_revid(info.log_revid))
+ pfru_log_dev->info.log_revid = info.log_revid;
+
+ if (valid_log_level(info.log_level)) {
+ ret = set_pfru_log_level(info.log_level, pfru_log_dev);
+ if (ret < 0)
+ return ret;
+
+ pfru_log_dev->info.log_level = info.log_level;
+ }
+
+ if (valid_log_type(info.log_type))
+ pfru_log_dev->info.log_type = info.log_type;
+
+ return 0;
+ case PFRU_LOG_IOC_GET_INFO:
+ info.log_level = get_pfru_log_level(pfru_log_dev);
+ if (ret < 0)
+ return ret;
+
+ info.log_type = pfru_log_dev->info.log_type;
+ info.log_revid = pfru_log_dev->info.log_revid;
+ if (copy_to_user(p, &info, sizeof(info)))
+ return -EFAULT;
+
+ return 0;
+ case PFRU_LOG_IOC_GET_DATA_INFO:
+ ret = get_pfru_log_data_info(&data_info, pfru_log_dev);
+ if (ret)
+ return ret;
+
+ if (copy_to_user(p, &data_info, sizeof(struct pfru_log_data_info)))
+ return -EFAULT;
+
+ return 0;
+ default:
+ return -ENOTTY;
+ }
+}
+
+static int
+pfru_log_mmap(struct file *file, struct vm_area_struct *vma)
+{
+ struct pfru_log_device *pfru_log_dev;
+ struct pfru_log_data_info info;
+ unsigned long psize, vsize;
+ phys_addr_t base_addr;
+ int ret;
+
+ if (vma->vm_flags & VM_WRITE)
+ return -EROFS;
+
+ /* changing from read to write with mprotect is not allowed */
+ vma->vm_flags &= ~VM_MAYWRITE;
+
+ pfru_log_dev = to_pfru_log_dev(file);
+
+ ret = get_pfru_log_data_info(&info, pfru_log_dev);
+ if (ret)
+ return ret;
+
+ base_addr = (phys_addr_t)((info.chunk2_addr_hi << 32) | info.chunk2_addr_lo);
+ /* pfru update has not been launched yet */
+ if (!base_addr)
+ return -ENODEV;
+
+ psize = info.max_data_size;
+ /* base address and total buffer size must be page aligned */
+ if (!PAGE_ALIGNED(base_addr) || !PAGE_ALIGNED(psize))
+ return -ENODEV;
+
+ vsize = vma->vm_end - vma->vm_start;
+ if (vsize > psize)
+ return -EINVAL;
+
+ vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+ if (io_remap_pfn_range(vma, vma->vm_start, PFN_DOWN(base_addr),
+ vsize, vma->vm_page_prot))
+ return -EAGAIN;
+
+ return 0;
+}
+
+static const struct file_operations acpi_pfru_log_fops = {
+ .owner = THIS_MODULE,
+ .mmap = pfru_log_mmap,
+ .unlocked_ioctl = pfru_log_ioctl,
+ .llseek = noop_llseek,
+};
+
+static int acpi_pfru_log_remove(struct platform_device *pdev)
+{
+ struct pfru_log_device *pfru_log_dev = platform_get_drvdata(pdev);
+
+ misc_deregister(&pfru_log_dev->miscdev);
+
+ return 0;
+}
+
+static void pfru_log_put_idx(void *data)
+{
+ struct pfru_log_device *pfru_log_dev = data;
+
+ ida_free(&pfru_log_ida, pfru_log_dev->index);
+}
+
+static int acpi_pfru_log_probe(struct platform_device *pdev)
+{
+ acpi_handle handle = ACPI_HANDLE(&pdev->dev);
+ struct pfru_log_device *pfru_log_dev;
+ int ret;
+
+ if (!acpi_has_method(handle, "_DSM")) {
+ dev_dbg(&pdev->dev, "Missing _DSM\n");
+ return -ENODEV;
+ }
+
+ pfru_log_dev = devm_kzalloc(&pdev->dev, sizeof(*pfru_log_dev), GFP_KERNEL);
+ if (!pfru_log_dev)
+ return -ENOMEM;
+
+ ret = ida_alloc(&pfru_log_ida, GFP_KERNEL);
+ if (ret < 0)
+ return ret;
+
+ pfru_log_dev->index = ret;
+ ret = devm_add_action_or_reset(&pdev->dev, pfru_log_put_idx, pfru_log_dev);
+ if (ret)
+ return ret;
+
+ pfru_log_dev->info.log_revid = PFRU_DEFAULT_REV_ID;
+ pfru_log_dev->parent_dev = &pdev->dev;
+
+ pfru_log_dev->miscdev.minor = MISC_DYNAMIC_MINOR;
+ pfru_log_dev->miscdev.name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
+ "pfru_telemetry%d",
+ pfru_log_dev->index);
+ if (!pfru_log_dev->miscdev.name)
+ return -ENOMEM;
+
+ pfru_log_dev->miscdev.nodename = devm_kasprintf(&pdev->dev, GFP_KERNEL,
+ "acpi_pfru_telemetry%d",
+ pfru_log_dev->index);
+ if (!pfru_log_dev->miscdev.nodename)
+ return -ENOMEM;
+
+ pfru_log_dev->miscdev.fops = &acpi_pfru_log_fops;
+ pfru_log_dev->miscdev.parent = &pdev->dev;
+
+ ret = misc_register(&pfru_log_dev->miscdev);
+ if (ret)
+ return ret;
+
+ platform_set_drvdata(pdev, pfru_log_dev);
+
+ return 0;
+}
+
+static const struct acpi_device_id acpi_pfru_log_ids[] = {
+ {"INTC1081"},
+ {}
+};
+MODULE_DEVICE_TABLE(acpi, acpi_pfru_log_ids);
+
+static struct platform_driver acpi_pfru_log_driver = {
+ .driver = {
+ .name = "pfru_telemetry",
+ .acpi_match_table = acpi_pfru_log_ids,
+ },
+ .probe = acpi_pfru_log_probe,
+ .remove = acpi_pfru_log_remove,
+};
+module_platform_driver(acpi_pfru_log_driver);
+
+MODULE_DESCRIPTION("Platform Firmware Runtime Update Telemetry driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/uapi/linux/pfru.h b/include/uapi/linux/pfru.h
index fed50cb01309..2f17714e05b7 100644
--- a/include/uapi/linux/pfru.h
+++ b/include/uapi/linux/pfru.h
@@ -171,4 +171,92 @@ struct pfru_updated_result {
__u64 high_exec_time;
};
+/**
+ * struct pfru_log_data_info - Log Data from telemetry service.
+ * @status: Indicator of whether this update succeed.
+ * @ext_status: Implementation specific update result.
+ * @chunk1_addr_lo: Low 32bit physical address of the telemetry data chunk1
+ * starting address.
+ * @chunk1_addr_hi: High 32bit physical address of the telemetry data chunk1
+ * starting address.
+ * @chunk2_addr_lo: Low 32bit physical address of the telemetry data chunk2
+ * starting address.
+ * @chunk2_addr_hi: High 32bit physical address of the telemetry data chunk2
+ * starting address.
+ * @max_data_size: Maximum supported size of data of all data chunks combined.
+ * @chunk1_size: Data size in bytes of the telemetry data chunk1 buffer.
+ * @chunk2_size: Data size in bytes of the telemetry data chunk2 buffer.
+ * @rollover_cnt: Number of times telemetry data buffer is overwritten
+ * since telemetry buffer reset.
+ * @reset_cnt: Number of times telemetry services resets that results in
+ * rollover count and data chunk buffers are reset.
+ */
+struct pfru_log_data_info {
+ __u32 status;
+ __u32 ext_status;
+ __u64 chunk1_addr_lo;
+ __u64 chunk1_addr_hi;
+ __u64 chunk2_addr_lo;
+ __u64 chunk2_addr_hi;
+ __u32 max_data_size;
+ __u32 chunk1_size;
+ __u32 chunk2_size;
+ __u32 rollover_cnt;
+ __u32 reset_cnt;
+};
+
+/**
+ * struct pfru_log_info - Telemetry log information.
+ * @log_level: The telemetry log level.
+ * @log_type: The telemetry log type(history and execution).
+ * @log_revid: The telemetry log revision id.
+ */
+struct pfru_log_info {
+ __u32 log_level;
+ __u32 log_type;
+ __u32 log_revid;
+};
+
+/**
+ * PFRU_LOG_IOC_SET_INFO - _IOW(PFRU_MAGIC_FOR_IOCTL, 0x06,
+ * struct pfru_log_info)
+ *
+ * Return:
+ * * 0 - success
+ * * -EFAULT - fail to get the setting parameter
+ * * -EINVAL - fail to set the log level
+ *
+ * Set the PFRU log level and log type. The input information is
+ * a struct pfru_log_info.
+ */
+#define PFRU_LOG_IOC_SET_INFO _IOW(PFRU_MAGIC_FOR_IOCTL, 0x06, struct pfru_log_info)
+
+/**
+ * PFRU_LOG_IOC_GET_INFO - _IOR(PFRU_MAGIC_FOR_IOCTL, 0x07,
+ * struct pfru_log_info)
+ *
+ * Return:
+ * * 0 - success
+ * * -EINVAL - fail to get the log level
+ * * -EFAULT - fail to copy the result back to userspace
+ *
+ * Retrieve log level and log type of the PFRU telemetry. The information is
+ * a struct pfru_log_info.
+ */
+#define PFRU_LOG_IOC_GET_INFO _IOR(PFRU_MAGIC_FOR_IOCTL, 0x07, struct pfru_log_info)
+
+/**
+ * PFRU_LOG_IOC_GET_DATA_INFO - _IOR(PFRU_MAGIC_FOR_IOCTL, 0x08,
+ * struct pfru_log_data_info)
+ *
+ * Return:
+ * * 0 - success
+ * * -EINVAL - fail to get the log buffer information
+ * * -EFAULT - fail to copy the log buffer information to userspace
+ *
+ * Retrieve data information about the PFRU telemetry. The information
+ * is a struct pfru_log_data_info.
+ */
+#define PFRU_LOG_IOC_GET_DATA_INFO _IOR(PFRU_MAGIC_FOR_IOCTL, 0x08, struct pfru_log_data_info)
+
#endif /* __PFRU_H__ */
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v11 4/4] tools: Introduce power/acpi/tools/pfru
2021-11-21 15:15 [PATCH v11 0/4] Introduce Platform Firmware Runtime Update and Telemetry drivers Chen Yu
` (2 preceding siblings ...)
2021-11-21 15:16 ` [PATCH v11 3/4] drivers/acpi: Introduce Platform Firmware Runtime Update Telemetry Chen Yu
@ 2021-11-21 15:17 ` Chen Yu
2021-11-22 11:26 ` Andy Shevchenko
2021-12-07 19:52 ` Rafael J. Wysocki
3 siblings, 2 replies; 17+ messages in thread
From: Chen Yu @ 2021-11-21 15:17 UTC (permalink / raw)
To: linux-acpi
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Ard Biesheuvel, Len Brown,
Ashok Raj, Andy Shevchenko, Mike Rapoport, Chen Yu, linux-kernel,
Robert Moore
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.
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
v11:No change since v10.
v10:No change since v9.
v9: Add this tool into tools/power/acpi build infrastructure.
(Andy Shevchenko)
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/.gitignore | 1 +
tools/power/acpi/Makefile | 16 +-
tools/power/acpi/Makefile.rules | 2 +-
tools/power/acpi/man/pfru.8 | 137 +++++++++
tools/power/acpi/tools/pfru/Makefile | 23 ++
tools/power/acpi/tools/pfru/pfru.c | 417 +++++++++++++++++++++++++++
6 files changed, 587 insertions(+), 9 deletions(-)
create mode 100644 tools/power/acpi/man/pfru.8
create mode 100644 tools/power/acpi/tools/pfru/Makefile
create mode 100644 tools/power/acpi/tools/pfru/pfru.c
diff --git a/tools/power/acpi/.gitignore b/tools/power/acpi/.gitignore
index 0b319fc8bb17..a6f196912999 100644
--- a/tools/power/acpi/.gitignore
+++ b/tools/power/acpi/.gitignore
@@ -2,4 +2,5 @@
/acpidbg
/acpidump
/ec
+/pfru
/include/
diff --git a/tools/power/acpi/Makefile b/tools/power/acpi/Makefile
index a249c50ebf55..2fcf09d3add3 100644
--- a/tools/power/acpi/Makefile
+++ b/tools/power/acpi/Makefile
@@ -9,18 +9,18 @@ include ../../scripts/Makefile.include
.NOTPARALLEL:
-all: acpidbg acpidump ec
-clean: acpidbg_clean acpidump_clean ec_clean
-install: acpidbg_install acpidump_install ec_install
-uninstall: acpidbg_uninstall acpidump_uninstall ec_uninstall
+all: acpidbg acpidump ec pfru
+clean: acpidbg_clean acpidump_clean ec_clean pfru_clean
+install: acpidbg_install acpidump_install ec_install pfru_install
+uninstall: acpidbg_uninstall acpidump_uninstall ec_uninstall pfru_uninstall
-acpidbg acpidump ec: FORCE
+acpidbg acpidump ec pfru: FORCE
$(call descend,tools/$@,all)
-acpidbg_clean acpidump_clean ec_clean:
+acpidbg_clean acpidump_clean ec_clean pfru_clean:
$(call descend,tools/$(@:_clean=),clean)
-acpidbg_install acpidump_install ec_install:
+acpidbg_install acpidump_install ec_install pfru_install:
$(call descend,tools/$(@:_install=),install)
-acpidbg_uninstall acpidump_uninstall ec_uninstall:
+acpidbg_uninstall acpidump_uninstall ec_uninstall pfru_uninstall:
$(call descend,tools/$(@:_uninstall=),uninstall)
.PHONY: FORCE
diff --git a/tools/power/acpi/Makefile.rules b/tools/power/acpi/Makefile.rules
index 2a6c170b57cd..68aa7e92d554 100644
--- a/tools/power/acpi/Makefile.rules
+++ b/tools/power/acpi/Makefile.rules
@@ -9,7 +9,7 @@ objdir := $(OUTPUT)tools/$(TOOL)/
toolobjs := $(addprefix $(objdir),$(TOOL_OBJS))
$(OUTPUT)$(TOOL): $(toolobjs) FORCE
$(ECHO) " LD " $(subst $(OUTPUT),,$@)
- $(QUIET) $(LD) $(CFLAGS) $(LDFLAGS) $(toolobjs) -L$(OUTPUT) -o $@
+ $(QUIET) $(LD) $(CFLAGS) $(toolobjs) $(LDFLAGS) -L$(OUTPUT) -o $@
$(ECHO) " STRIP " $(subst $(OUTPUT),,$@)
$(QUIET) $(STRIPCMD) $@
diff --git a/tools/power/acpi/man/pfru.8 b/tools/power/acpi/man/pfru.8
new file mode 100644
index 000000000000..d9cda7beaa3c
--- /dev/null
+++ b/tools/power/acpi/man/pfru.8
@@ -0,0 +1,137 @@
+.TH "PFRU" "8" "October 2021" "pfru 1.0" ""
+.hy
+.SH Name
+.PP
+pfru \- Platform Firmware Runtime Update tool
+.SH SYNOPSIS
+.PP
+\f[B]pfru\f[R] [\f[I]Options\f[R]]
+.SH DESCRIPTION
+.PP
+The PFRU(Platform Firmware Runtime Update) kernel interface is designed
+to
+.PD 0
+.P
+.PD
+interact with the platform firmware interface defined in the
+.PD 0
+.P
+.PD
+Management Mode Firmware Runtime
+Update (https://uefi.org/sites/default/files/resources/Intel_MM_OS_Interface_Spec_Rev100.pdf)
+.PD 0
+.P
+.PD
+\f[B]pfru\f[R] is the tool to interact with the kernel interface.
+.PD 0
+.P
+.PD
+.SH OPTIONS
+.TP
+.B \f[B]\-h\f[R], \f[B]\-\-help\f[R]
+Display helper information.
+.TP
+.B \f[B]\-l\f[R], \f[B]\-\-load\f[R]
+Load the capsule file into the system.
+To be more specific, the capsule file will be copied to the
+communication buffer.
+.TP
+.B \f[B]\-s\f[R], \f[B]\-\-stage\f[R]
+Stage the capsule image from communication buffer into Management Mode
+and perform authentication.
+.TP
+.B \f[B]\-a\f[R], \f[B]\-\-activate\f[R]
+Activate a previous staged capsule image.
+.TP
+.B \f[B]\-u\f[R], \f[B]\-\-update\f[R]
+Perform both stage and activation actions.
+.TP
+.B \f[B]\-q\f[R], \f[B]\-\-query\f[R]
+Query the update capability.
+.TP
+.B \f[B]\-d\f[R], \f[B]\-\-setrev\f[R]
+Set the revision ID of code injection/driver update.
+.TP
+.B \f[B]\-D\f[R], \f[B]\-\-setrevlog\f[R]
+Set the revision ID of telemetry.
+.TP
+.B \f[B]\-G\f[R], \f[B]\-\-getloginfo\f[R]
+Get telemetry log information and print it out.
+.TP
+.B \f[B]\-T\f[R], \f[B]\-\-type\f[R]
+Set the telemetry log data type.
+.TP
+.B \f[B]\-L\f[R], \f[B]\-\-level\f[R]
+Set the telemetry log level.
+.TP
+.B \f[B]\-R\f[R], \f[B]\-\-read\f[R]
+Read all the telemetry data and print it out.
+.SH EXAMPLES
+.PP
+\f[B]pfru \-G\f[R]
+.PP
+log_level:4
+.PD 0
+.P
+.PD
+log_type:0
+.PD 0
+.P
+.PD
+log_revid:2
+.PD 0
+.P
+.PD
+max_data_size:65536
+.PD 0
+.P
+.PD
+chunk1_size:0
+.PD 0
+.P
+.PD
+chunk2_size:1401
+.PD 0
+.P
+.PD
+rollover_cnt:0
+.PD 0
+.P
+.PD
+reset_cnt:4
+.PP
+\f[B]pfru \-q\f[R]
+.PP
+code injection image type:794bf8b2\-6e7b\-454e\-885f\-3fb9bb185402
+.PD 0
+.P
+.PD
+fw_version:0
+.PD 0
+.P
+.PD
+code_rt_version:1
+.PD 0
+.P
+.PD
+driver update image type:0e5f0b14\-f849\-7945\-ad81\-bc7b6d2bb245
+.PD 0
+.P
+.PD
+drv_rt_version:0
+.PD 0
+.P
+.PD
+drv_svn:0
+.PD 0
+.P
+.PD
+platform id:39214663\-b1a8\-4eaa\-9024\-f2bb53ea4723
+.PD 0
+.P
+.PD
+oem id:a36db54f\-ea2a\-e14e\-b7c4\-b5780e51ba3d
+.PP
+\f[B]pfru \-l yours.cap \-u \-T 1 \-L 4\f[R]
+.SH AUTHORS
+Chen Yu.
diff --git a/tools/power/acpi/tools/pfru/Makefile b/tools/power/acpi/tools/pfru/Makefile
new file mode 100644
index 000000000000..09002a81e10c
--- /dev/null
+++ b/tools/power/acpi/tools/pfru/Makefile
@@ -0,0 +1,23 @@
+# SPDX-License-Identifier: GPL-2.0+
+
+include ../../Makefile.config
+
+TOOL = pfru
+EXTRA_INSTALL = install-man
+EXTRA_UNINSTALL = uninstall-man
+
+CFLAGS += -Wall -O2
+CFLAGS += -DPFRU_HEADER='"../../../../../include/uapi/linux/pfru.h"'
+LDFLAGS += -luuid
+
+TOOL_OBJS = \
+ pfru.o
+
+include ../../Makefile.rules
+
+install-man: $(srctree)/man/pfru.8
+ $(ECHO) " INST " pfru.8
+ $(QUIET) $(INSTALL_DATA) -D $< $(DESTDIR)$(mandir)/man8/pfru.8
+uninstall-man:
+ $(ECHO) " UNINST " pfru.8
+ $(QUIET) rm -f $(DESTDIR)$(mandir)/man8/pfru.8
diff --git a/tools/power/acpi/tools/pfru/pfru.c b/tools/power/acpi/tools/pfru/pfru.c
new file mode 100644
index 000000000000..eeec5043f825
--- /dev/null
+++ b/tools/power/acpi/tools/pfru/pfru.c
@@ -0,0 +1,417 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Platform Firmware Runtime Update tool to do Management
+ * Mode code injection/driver update and telemetry retrieval.
+ */
+#define _GNU_SOURCE
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <getopt.h>
+#include <sys/ioctl.h>
+#include <sys/mman.h>
+#include <uuid/uuid.h>
+#include PFRU_HEADER
+
+char *capsule_name;
+int action, query_cap, log_type, log_level, log_read, log_getinfo,
+ revid, log_revid;
+int set_log_level, set_log_type,
+ set_revid, set_log_revid;
+
+char *progname;
+
+#define LOG_ERR 0
+#define LOG_WARN 1
+#define LOG_INFO 2
+#define LOG_VERB 4
+#define LOG_EXEC_IDX 0
+#define LOG_HISTORY_IDX 1
+#define REVID_1 1
+#define REVID_2 2
+
+static int valid_log_level(int level)
+{
+ return level == LOG_ERR || level == LOG_WARN ||
+ level == LOG_INFO || level == LOG_VERB;
+}
+
+static int valid_log_type(int type)
+{
+ return type == LOG_EXEC_IDX || type == LOG_HISTORY_IDX;
+}
+
+static inline int valid_log_revid(int id)
+{
+ return id == REVID_1 || id == REVID_2;
+}
+
+static void help(void)
+{
+ fprintf(stderr,
+ "usage: %s [OPTIONS]\n"
+ " code injection:\n"
+ " -l, --load\n"
+ " -s, --stage\n"
+ " -a, --activate\n"
+ " -u, --update [stage and activate]\n"
+ " -q, --query\n"
+ " -d, --revid update\n"
+ " telemetry:\n"
+ " -G, --getloginfo\n"
+ " -T, --type(0:execution, 1:history)\n"
+ " -L, --level(0, 1, 2, 4)\n"
+ " -R, --read\n"
+ " -D, --revid log\n",
+ progname);
+}
+
+char *option_string = "l:sauqd:GT:L:RD:h";
+static struct option long_options[] = {
+ {"load", required_argument, 0, 'l'},
+ {"stage", no_argument, 0, 's'},
+ {"activate", no_argument, 0, 'a'},
+ {"update", no_argument, 0, 'u'},
+ {"query", no_argument, 0, 'q'},
+ {"getloginfo", no_argument, 0, 'G'},
+ {"type", required_argument, 0, 'T'},
+ {"level", required_argument, 0, 'L'},
+ {"read", no_argument, 0, 'R'},
+ {"setrev", required_argument, 0, 'd'},
+ {"setrevlog", required_argument, 0, 'D'},
+ {"help", no_argument, 0, 'h'},
+ {}
+};
+
+static void parse_options(int argc, char **argv)
+{
+ int option_index = 0;
+ char *pathname;
+ int opt;
+
+ pathname = strdup(argv[0]);
+ progname = basename(pathname);
+
+ while ((opt = getopt_long_only(argc, argv, option_string,
+ long_options, &option_index)) != -1) {
+ switch (opt) {
+ case 'l':
+ capsule_name = optarg;
+ break;
+ case 's':
+ action = 1;
+ break;
+ case 'a':
+ action = 2;
+ break;
+ case 'u':
+ action = 3;
+ break;
+ case 'q':
+ query_cap = 1;
+ break;
+ case 'G':
+ log_getinfo = 1;
+ break;
+ case 'T':
+ log_type = atoi(optarg);
+ set_log_type = 1;
+ break;
+ case 'L':
+ log_level = atoi(optarg);
+ set_log_level = 1;
+ break;
+ case 'R':
+ log_read = 1;
+ break;
+ case 'd':
+ revid = atoi(optarg);
+ set_revid = 1;
+ break;
+ case 'D':
+ log_revid = atoi(optarg);
+ set_log_revid = 1;
+ break;
+ case 'h':
+ help();
+ exit(0);
+ default:
+ break;
+ }
+ }
+}
+
+void print_cap(struct pfru_update_cap_info *cap)
+{
+ char *uuid;
+
+ uuid = malloc(37);
+ if (!uuid) {
+ perror("Can not allocate uuid buffer\n");
+ exit(1);
+ }
+
+ uuid_unparse(cap->code_type, uuid);
+ printf("code injection image type:%s\n", uuid);
+ printf("fw_version:%d\n", cap->fw_version);
+ printf("code_rt_version:%d\n", cap->code_rt_version);
+
+ uuid_unparse(cap->drv_type, uuid);
+ printf("driver update image type:%s\n", uuid);
+ printf("drv_rt_version:%d\n", cap->drv_rt_version);
+ printf("drv_svn:%d\n", cap->drv_svn);
+
+ uuid_unparse(cap->platform_id, uuid);
+ printf("platform id:%s\n", uuid);
+ uuid_unparse(cap->oem_id, uuid);
+ printf("oem id:%s\n", uuid);
+ printf("oem information length:%d\n", cap->oem_info_len);
+
+ free(uuid);
+}
+
+int main(int argc, char *argv[])
+{
+ int fd_update, fd_update_log, fd_capsule;
+ struct pfru_log_data_info data_info;
+ struct pfru_log_info info;
+ struct pfru_update_cap_info cap;
+ void *addr_map_capsule;
+ struct stat st;
+ char *log_buf;
+ int ret = 0;
+
+ if (getuid() != 0) {
+ printf("Please run the tool as root - Exiting.\n");
+ return 1;
+ }
+
+ parse_options(argc, argv);
+
+ fd_update = open("/dev/acpi_pfru0", O_RDWR);
+ if (fd_update < 0) {
+ printf("PFRU device not supported - Quit...\n");
+ return 1;
+ }
+
+ fd_update_log = open("/dev/acpi_pfru_telemetry0", O_RDWR);
+ if (fd_update_log < 0) {
+ printf("PFRU telemetry device not supported - Quit...\n");
+ return 1;
+ }
+
+ if (query_cap) {
+ ret = ioctl(fd_update, PFRU_IOC_QUERY_CAP, &cap);
+ if (ret)
+ perror("Query Update Capability info failed.");
+ else
+ print_cap(&cap);
+
+ close(fd_update);
+ close(fd_update_log);
+
+ return ret;
+ }
+
+ if (log_getinfo) {
+ ret = ioctl(fd_update_log, PFRU_LOG_IOC_GET_DATA_INFO, &data_info);
+ if (ret) {
+ perror("Get telemetry data info failed.");
+ close(fd_update);
+ close(fd_update_log);
+
+ return 1;
+ }
+
+ ret = ioctl(fd_update_log, PFRU_LOG_IOC_GET_INFO, &info);
+ if (ret) {
+ perror("Get telemetry info failed.");
+ close(fd_update);
+ close(fd_update_log);
+
+ return 1;
+ }
+
+ printf("log_level:%d\n", info.log_level);
+ printf("log_type:%d\n", info.log_type);
+ printf("log_revid:%d\n", info.log_revid);
+ printf("max_data_size:%d\n", data_info.max_data_size);
+ printf("chunk1_size:%d\n", data_info.chunk1_size);
+ printf("chunk2_size:%d\n", data_info.chunk2_size);
+ printf("rollover_cnt:%d\n", data_info.rollover_cnt);
+ printf("reset_cnt:%d\n", data_info.reset_cnt);
+
+ return 0;
+ }
+
+ info.log_level = -1;
+ info.log_type = -1;
+ info.log_revid = -1;
+
+ if (set_log_level) {
+ if (!valid_log_level(log_level)) {
+ printf("Invalid log level %d\n",
+ log_level);
+ } else {
+ info.log_level = log_level;
+ }
+ }
+
+ if (set_log_type) {
+ if (!valid_log_type(log_type)) {
+ printf("Invalid log type %d\n",
+ log_type);
+ } else {
+ info.log_type = log_type;
+ }
+ }
+
+ if (set_log_revid) {
+ if (!valid_log_revid(log_revid)) {
+ printf("Invalid log revid %d, unchanged.\n",
+ log_revid);
+ } else {
+ info.log_revid = log_revid;
+ }
+ }
+
+ ret = ioctl(fd_update_log, PFRU_LOG_IOC_SET_INFO, &info);
+ if (ret) {
+ perror("Log information set failed.(log_level, log_type, log_revid)");
+ close(fd_update);
+ close(fd_update_log);
+
+ return 1;
+ }
+
+ if (set_revid) {
+ ret = ioctl(fd_update, PFRU_IOC_SET_REV, &revid);
+ if (ret) {
+ perror("pfru update revid set failed");
+ close(fd_update);
+ close(fd_update_log);
+
+ return 1;
+ }
+
+ printf("pfru update revid set to %d\n", revid);
+ }
+
+ if (capsule_name) {
+ fd_capsule = open(capsule_name, O_RDONLY);
+ if (fd_capsule < 0) {
+ perror("Can not open capsule file...");
+ close(fd_update);
+ close(fd_update_log);
+
+ return 1;
+ }
+
+ if (fstat(fd_capsule, &st) < 0) {
+ perror("Can not fstat capsule file...");
+ close(fd_capsule);
+ close(fd_update);
+ close(fd_update_log);
+
+ return 1;
+ }
+
+ addr_map_capsule = mmap(NULL, st.st_size, PROT_READ, MAP_SHARED,
+ fd_capsule, 0);
+ if (addr_map_capsule == MAP_FAILED) {
+ perror("Failed to mmap capsule file.");
+ close(fd_capsule);
+ close(fd_update);
+ close(fd_update_log);
+
+ return 1;
+ }
+
+ ret = write(fd_update, (char *)addr_map_capsule, st.st_size);
+ printf("Load %d bytes of capsule file into the system\n",
+ ret);
+
+ if (ret == -1) {
+ perror("Failed to load capsule file");
+ close(fd_capsule);
+ close(fd_update);
+ close(fd_update_log);
+
+ return 1;
+ }
+
+ munmap(addr_map_capsule, st.st_size);
+ close(fd_capsule);
+ printf("Load done.\n");
+ }
+
+ if (action) {
+ if (action == 1) {
+ ret = ioctl(fd_update, PFRU_IOC_STAGE, NULL);
+ } else if (action == 2) {
+ ret = ioctl(fd_update, PFRU_IOC_ACTIVATE, NULL);
+ } else if (action == 3) {
+ ret = ioctl(fd_update, PFRU_IOC_STAGE_ACTIVATE, NULL);
+ } else {
+ close(fd_update);
+ close(fd_update_log);
+
+ return 1;
+ }
+ printf("Update finished, return %d\n", ret);
+ }
+
+ close(fd_update);
+
+ if (log_read) {
+ void *p_mmap;
+ int max_data_sz;
+
+ ret = ioctl(fd_update_log, PFRU_LOG_IOC_GET_DATA_INFO, &data_info);
+ if (ret) {
+ perror("Get telemetry data info failed.");
+ close(fd_update_log);
+
+ return 1;
+ }
+
+ max_data_sz = data_info.max_data_size;
+ if (!max_data_sz) {
+ printf("No telemetry data available.\n");
+ close(fd_update_log);
+
+ return 1;
+ }
+
+ log_buf = malloc(max_data_sz + 1);
+ if (!log_buf) {
+ perror("log_buf allocate failed.");
+ close(fd_update_log);
+
+ return 1;
+ }
+
+ p_mmap = mmap(NULL, max_data_sz, PROT_READ, MAP_SHARED, fd_update_log, 0);
+ if (p_mmap == MAP_FAILED) {
+ perror("mmap error.");
+ close(fd_update_log);
+
+ return 1;
+ }
+
+ memcpy(log_buf, p_mmap, max_data_sz);
+ log_buf[max_data_sz] = '\0';
+ printf("%s\n", log_buf);
+ free(log_buf);
+
+ munmap(p_mmap, max_data_sz);
+ }
+
+ close(fd_update_log);
+
+ return 0;
+}
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v11 4/4] tools: Introduce power/acpi/tools/pfru
2021-11-21 15:17 ` [PATCH v11 4/4] tools: Introduce power/acpi/tools/pfru Chen Yu
@ 2021-11-22 11:26 ` Andy Shevchenko
2021-11-22 15:48 ` Chen Yu
2021-12-07 19:52 ` Rafael J. Wysocki
1 sibling, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2021-11-22 11:26 UTC (permalink / raw)
To: Chen Yu
Cc: linux-acpi, Greg Kroah-Hartman, Rafael J. Wysocki,
Ard Biesheuvel, Len Brown, Ashok Raj, Mike Rapoport,
linux-kernel, Robert Moore
On Sun, Nov 21, 2021 at 11:17:29PM +0800, Chen Yu wrote:
> 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.
Have you tried to build tools with `make O=/my/tmp/folder/for/kernel/build ...`
which previously has been used for kernel builds?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v11 4/4] tools: Introduce power/acpi/tools/pfru
2021-11-22 11:26 ` Andy Shevchenko
@ 2021-11-22 15:48 ` Chen Yu
2021-11-22 17:18 ` Andy Shevchenko
0 siblings, 1 reply; 17+ messages in thread
From: Chen Yu @ 2021-11-22 15:48 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-acpi, Greg Kroah-Hartman, Rafael J. Wysocki,
Ard Biesheuvel, Len Brown, Ashok Raj, Mike Rapoport,
linux-kernel, Robert Moore
Hi Andy,
On Mon, Nov 22, 2021 at 01:26:12PM +0200, Andy Shevchenko wrote:
> On Sun, Nov 21, 2021 at 11:17:29PM +0800, Chen Yu wrote:
> > 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.
>
> Have you tried to build tools with `make O=/my/tmp/folder/for/kernel/build ...`
> which previously has been used for kernel builds?
>
I was not aware of that and just had a try. It seems that there is an issue in
tools/power/acpi that, only with the following patch appiled, the make O=xxx
would work:
diff --git a/tools/power/acpi/Makefile.config b/tools/power/acpi/Makefile.config
index 331f6d30f472..cd7106876a5f 100644
--- a/tools/power/acpi/Makefile.config
+++ b/tools/power/acpi/Makefile.config
@@ -69,6 +69,7 @@ KERNEL_INCLUDE := $(OUTPUT)include
ACPICA_INCLUDE := $(srctree)/../../../drivers/acpi/acpica
CFLAGS += -D_LINUX -I$(KERNEL_INCLUDE) -I$(ACPICA_INCLUDE)
CFLAGS += $(WARNINGS)
+MKDIR = mkdir
ifeq ($(strip $(V)),false)
QUIET=@
diff --git a/tools/power/acpi/Makefile.rules b/tools/power/acpi/Makefile.rules
index 2a6c170b57cd..1d7616f5d0ae 100644
--- a/tools/power/acpi/Makefile.rules
+++ b/tools/power/acpi/Makefile.rules
@@ -21,6 +21,7 @@ $(KERNEL_INCLUDE):
$(objdir)%.o: %.c $(KERNEL_INCLUDE)
$(ECHO) " CC " $(subst $(OUTPUT),,$@)
+ $(QUIET) $(MKDIR) -p $(objdir) 2>/dev/null
$(QUIET) $(CC) -c $(CFLAGS) -o $@ $<
all: $(OUTPUT)$(TOOL)
--
2.25.1
Should above patch be a separate one or folded in [4/4]?
thanks,
Chenyu
> --
> With Best Regards,
> Andy Shevchenko
>
>
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v11 4/4] tools: Introduce power/acpi/tools/pfru
2021-11-22 15:48 ` Chen Yu
@ 2021-11-22 17:18 ` Andy Shevchenko
2021-11-23 1:15 ` Chen Yu
0 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2021-11-22 17:18 UTC (permalink / raw)
To: Chen Yu
Cc: linux-acpi, Greg Kroah-Hartman, Rafael J. Wysocki,
Ard Biesheuvel, Len Brown, Ashok Raj, Mike Rapoport,
linux-kernel, Robert Moore
On Mon, Nov 22, 2021 at 11:48:42PM +0800, Chen Yu wrote:
> On Mon, Nov 22, 2021 at 01:26:12PM +0200, Andy Shevchenko wrote:
> > On Sun, Nov 21, 2021 at 11:17:29PM +0800, Chen Yu wrote:
> > > 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.
> >
> > Have you tried to build tools with `make O=/my/tmp/folder/for/kernel/build ...`
> > which previously has been used for kernel builds?
> >
> I was not aware of that and just had a try. It seems that there is an issue in
> tools/power/acpi that, only with the following patch appiled, the make O=xxx
> would work:
Cool!
Care to send a separate fix for this, please?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v11 4/4] tools: Introduce power/acpi/tools/pfru
2021-11-22 17:18 ` Andy Shevchenko
@ 2021-11-23 1:15 ` Chen Yu
0 siblings, 0 replies; 17+ messages in thread
From: Chen Yu @ 2021-11-23 1:15 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-acpi, Greg Kroah-Hartman, Rafael J. Wysocki,
Ard Biesheuvel, Len Brown, Ashok Raj, Mike Rapoport,
linux-kernel, Robert Moore
On Mon, Nov 22, 2021 at 07:18:38PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 22, 2021 at 11:48:42PM +0800, Chen Yu wrote:
> > On Mon, Nov 22, 2021 at 01:26:12PM +0200, Andy Shevchenko wrote:
> > > On Sun, Nov 21, 2021 at 11:17:29PM +0800, Chen Yu wrote:
> > > > 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.
> > >
> > > Have you tried to build tools with `make O=/my/tmp/folder/for/kernel/build ...`
> > > which previously has been used for kernel builds?
> > >
> > I was not aware of that and just had a try. It seems that there is an issue in
> > tools/power/acpi that, only with the following patch appiled, the make O=xxx
> > would work:
>
> Cool!
> Care to send a separate fix for this, please?
>
Ok, will do.
thanks,
Chenyu
> --
> With Best Regards,
> Andy Shevchenko
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v11 1/4] efi: Introduce EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER and corresponding structures
2021-11-21 15:16 ` [PATCH v11 1/4] efi: Introduce EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER and corresponding structures Chen Yu
@ 2021-12-03 18:20 ` Rafael J. Wysocki
2021-12-03 18:22 ` Ard Biesheuvel
0 siblings, 1 reply; 17+ messages in thread
From: Rafael J. Wysocki @ 2021-12-03 18:20 UTC (permalink / raw)
To: Chen Yu, Ard Biesheuvel
Cc: ACPI Devel Maling List, Greg Kroah-Hartman, Rafael J. Wysocki,
Len Brown, Ashok Raj, Andy Shevchenko, Mike Rapoport,
Linux Kernel Mailing List, linux-efi
On Sun, Nov 21, 2021 at 4:17 PM Chen Yu <yu.c.chen@intel.com> 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. In this way, if the user provides an invalid
> capsule image, the kernel could be used as a guard to reject it, without
> switching to the Management Mode (which might be costly).
>
> EFI_CAPSULE_HEADER has been defined in the kernel, but the other
> structures have not been defined yet, so do that. Besides,
> EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER and
> EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER are required to be packed
> in the uefi specification. 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.
>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---
> v11:Add explanation on why version check is introduced
> in kernel rather than letting Management Mode to do it.
> (Rafael J. Wysocki)
> Revise the commit log to better describe the pack attribute.
> (Rafael J. Wysocki)
> Refine the comment for hw_ins and capsule_support.
> (Rafael J. Wysocki)
> v10:Remove the explicit assignment of the last item of enum.
> (Andy Shevchenko)
> v9: Use GUID_INIT() instead of guid_parse() during boot up.
> (Andy Shevchenko)
> Drop uuid, code_uuid, drv_uuid in struct pfru_device as they
> are not needed. (Andy Shevchenko)
> Drop type casting from void * in valid_version().
> (Andy Shevchenko)
> Use kfree() instead of ACPI_FREE() in non-ACPICA usage.
> (Andy Shevchenko)
> Use sizeof(rev) instead of sizeof(u32) in copy_from_user().
> (Andy Shevchenko)
> Generate physical address from MSB part to LSB.
> (Andy Shevchenko)
> Use devm_add_action_or_reset() to add ida release into dev resource
> management. (Andy Shevchenko)
> Use devm_kasprintf() instead of kasprintf() to format the
> pfru_dev name.(Andy Shevchenko)
> Remove redundant 0 in acpi_pfru_ids. (Andy Shevchenko)
> Adjust the order of included headers in pfru.h.
> (Andy Shevchenko)
> Replace PFRU_MAGIC with PFRU_MAGIC_FOR_IOCTL in uapi file.
> (Andy Shevchenko)
> 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)
> ---
OK, this is simple enough.
Ard, any concerns?
> include/linux/efi.h | 46 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 46 insertions(+)
>
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index dbd39b20e034..80e970f7e6f8 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -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-size array of the size given by the sum of
> + * emb_drv_cnt and 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;
> + /* hw_ins was introduced in version 2 */
> + u64 hw_ins;
> + /* capsule_support was introduced in version 3 */
> + 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_AUTHENTICATION */
> +struct efi_image_auth {
> + u64 mon_count;
> + struct win_cert_uefi_guid auth_info;
> +};
> +
> /*
> * EFI capsule flags
> */
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v11 1/4] efi: Introduce EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER and corresponding structures
2021-12-03 18:20 ` Rafael J. Wysocki
@ 2021-12-03 18:22 ` Ard Biesheuvel
0 siblings, 0 replies; 17+ messages in thread
From: Ard Biesheuvel @ 2021-12-03 18:22 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Chen Yu, ACPI Devel Maling List, Greg Kroah-Hartman, Len Brown,
Ashok Raj, Andy Shevchenko, Mike Rapoport,
Linux Kernel Mailing List, linux-efi
On Fri, 3 Dec 2021 at 19:21, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Sun, Nov 21, 2021 at 4:17 PM Chen Yu <yu.c.chen@intel.com> 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. In this way, if the user provides an invalid
> > capsule image, the kernel could be used as a guard to reject it, without
> > switching to the Management Mode (which might be costly).
> >
> > EFI_CAPSULE_HEADER has been defined in the kernel, but the other
> > structures have not been defined yet, so do that. Besides,
> > EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER and
> > EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER are required to be packed
> > in the uefi specification. 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.
> >
> > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > ---
> > v11:Add explanation on why version check is introduced
> > in kernel rather than letting Management Mode to do it.
> > (Rafael J. Wysocki)
> > Revise the commit log to better describe the pack attribute.
> > (Rafael J. Wysocki)
> > Refine the comment for hw_ins and capsule_support.
> > (Rafael J. Wysocki)
> > v10:Remove the explicit assignment of the last item of enum.
> > (Andy Shevchenko)
> > v9: Use GUID_INIT() instead of guid_parse() during boot up.
> > (Andy Shevchenko)
> > Drop uuid, code_uuid, drv_uuid in struct pfru_device as they
> > are not needed. (Andy Shevchenko)
> > Drop type casting from void * in valid_version().
> > (Andy Shevchenko)
> > Use kfree() instead of ACPI_FREE() in non-ACPICA usage.
> > (Andy Shevchenko)
> > Use sizeof(rev) instead of sizeof(u32) in copy_from_user().
> > (Andy Shevchenko)
> > Generate physical address from MSB part to LSB.
> > (Andy Shevchenko)
> > Use devm_add_action_or_reset() to add ida release into dev resource
> > management. (Andy Shevchenko)
> > Use devm_kasprintf() instead of kasprintf() to format the
> > pfru_dev name.(Andy Shevchenko)
> > Remove redundant 0 in acpi_pfru_ids. (Andy Shevchenko)
> > Adjust the order of included headers in pfru.h.
> > (Andy Shevchenko)
> > Replace PFRU_MAGIC with PFRU_MAGIC_FOR_IOCTL in uapi file.
> > (Andy Shevchenko)
> > 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)
> > ---
>
> OK, this is simple enough.
>
> Ard, any concerns?
>
Looks fine to me
Acked-by: Ard Biesheuvel <ardb@kernel.org>
> > include/linux/efi.h | 46 +++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 46 insertions(+)
> >
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index dbd39b20e034..80e970f7e6f8 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -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-size array of the size given by the sum of
> > + * emb_drv_cnt and 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;
> > + /* hw_ins was introduced in version 2 */
> > + u64 hw_ins;
> > + /* capsule_support was introduced in version 3 */
> > + 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_AUTHENTICATION */
> > +struct efi_image_auth {
> > + u64 mon_count;
> > + struct win_cert_uefi_guid auth_info;
> > +};
> > +
> > /*
> > * EFI capsule flags
> > */
> > --
> > 2.25.1
> >
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v11 2/4] drivers/acpi: Introduce Platform Firmware Runtime Update device driver
2021-11-21 15:16 ` [PATCH v11 2/4] drivers/acpi: Introduce Platform Firmware Runtime Update device driver Chen Yu
@ 2021-12-03 19:37 ` Rafael J. Wysocki
2021-12-04 8:07 ` Chen Yu
0 siblings, 1 reply; 17+ messages in thread
From: Rafael J. Wysocki @ 2021-12-03 19:37 UTC (permalink / raw)
To: Chen Yu
Cc: ACPI Devel Maling List, Greg Kroah-Hartman, Rafael J. Wysocki,
Ard Biesheuvel, Len Brown, Ashok Raj, Andy Shevchenko,
Mike Rapoport, Linux Kernel Mailing List
On Sun, Nov 21, 2021 at 4:17 PM Chen Yu <yu.c.chen@intel.com> wrote:
>
> Introduce the pfru_update driver which can be used for Platform Firmware
> Runtime code injection and driver update [1].
The PFRU TLA already covers the "update" part. so the naming is a bit unclean.
IMO any of pfrut_update, or pfr_update, or even pfru alone would be better.
My favorite is pfr_update, and the telemetry part can be called
pfr_telementry for symmetry.
> The user is expected to
> provide the update firmware in the form of capsule file, and pass it to
This is an EFI capsule and not necessarily in a separate file, so why
don't you just say "EFI capsule"?
> the driver via ioctl.
It is actually passed by writing the capsule to a device special file, isn't it?
The ioctl() interface is necessary for setup/cleanup.
> Then the driver would hand this capsule file to the
> Platform Firmware Runtime Update via the ACPI device _DSM method. At last
"The capsule is transferred by the driver to the platform firmware
with the help of an ACPI _DSM method under the special ACPI Platform
Firmware Runtime Update device (INTC1080)"
> the low level Management Mode would do the firmware update.
"and the actual firmware update is carried out by the low-level
Management Mode code in the platform firmware."
> The corresponding userspace tool and man page will be introduced at
> tools/power/acpi/tools/pfru.
The above is irrelevant for this particular patch IMO.
What is relevant, and missing, is a bit of justification, like why it
is a good idea to add this driver to the kernel. There are some
pieces of it in the Kconfig help text and elsewhere, but it should be
there in the patch changelog too.
> Link: https://uefi.org/sites/default/files/resources/Intel_MM_OS_Interface_Spec_Rev100.pdf # [1]
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---
[cut the history]
> ---
> .../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 | 601 ++++++++++++++++++
> include/uapi/linux/pfru.h | 174 +++++
> 7 files changed, 793 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
>
> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> index cfe6cccf0f44..adf830410e0d 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -367,6 +367,7 @@ Code Seq# Include File Comments
> <mailto:aherrman@de.ibm.com>
> 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
If this is called pfrut.h, it would be more consistent with the description.
> 0xF3 00-3F drivers/usb/misc/sisusbvga/sisusb.h sisfb (in development)
> <mailto:thomas@winischhofer.net>
> 0xF6 all LTTng Linux Trace Toolkit Next Generation
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index cdbdf68bd98f..1cf26e404b63 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -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
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 3018714e87d9..9c2c5ddff6ec 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -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
> diff --git a/drivers/acpi/pfru/Kconfig b/drivers/acpi/pfru/Kconfig
> new file mode 100644
> index 000000000000..fd0a41b88fac
> --- /dev/null
> +++ b/drivers/acpi/pfru/Kconfig
> @@ -0,0 +1,13 @@
> +# SPDX-License-Identifier: GPL-2.0
> +config ACPI_PFRU
The telemetry will depend on the same config option, so I would call
it ACPI_PFRUT.
> + tristate "ACPI Platform Firmware Runtime Update (PFRU)"
And I would simply say "ACPI Platform Firmware Runtime Update and
Telemetry" here.
> + 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.
The above needs to be edited. What about using something like this instead:
"This mechanism allows certain pieces of the platform firmware to be
updated on the fly while the system is running (runtime) without the
need to restart it, which is key in the cases when the system needs to
be available 100% of the time and it cannot afford the downtime
related to restarting it, or when the work carried out by the system
is particularly important, so it cannot be interrupted, and it is not
practical to wait until it is complete.
The existing firmware code can be modified (driver update) or extended
by adding new code to the firmware (code injection)."
The telemetry part will be added separately later, so it need not be
mentioned here just yet.
> +
> + To compile this driver as module, choose M here:
> + the modules will be called pfru_update and pfru_telemetry.
> diff --git a/drivers/acpi/pfru/Makefile b/drivers/acpi/pfru/Makefile
> new file mode 100644
> index 000000000000..098cbe80cf3d
> --- /dev/null
> +++ b/drivers/acpi/pfru/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_ACPI_PFRU) += pfru_update.o
> diff --git a/drivers/acpi/pfru/pfru_update.c b/drivers/acpi/pfru/pfru_update.c
> new file mode 100644
> index 000000000000..cf670d052cc9
> --- /dev/null
> +++ b/drivers/acpi/pfru/pfru_update.c
So why don't you call this file pfr_update.c?
> @@ -0,0 +1,601 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ACPI Platform Firmware Runtime Update Device Driver
> + *
> + * Copyright (C) 2021 Intel Corporation
> + * Author: Chen Yu <yu.c.chen@intel.com>
Please describe what the driver is for here at least briefly.
> + */
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/efi.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/file.h>
> +#include <linux/fs.h>
> +#include <linux/idr.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/string.h>
> +#include <linux/uaccess.h>
> +#include <linux/uio.h>
> +#include <linux/uuid.h>
> +
> +#include <uapi/linux/pfru.h>
> +
> +#define PFRU_FUNC_STANDARD_QUERY 0
> +#define PFRU_FUNC_QUERY_UPDATE_CAP 1
> +#define PFRU_FUNC_QUERY_BUF 2
> +#define PFRU_FUNC_START 3
> +
> +#define PFRU_CODE_INJECT_TYPE 1
> +#define PFRU_DRIVER_UPDATE_TYPE 2
> +
> +#define PFRU_REVID_1 1
> +#define PFRU_REVID_2 2
> +#define PFRU_DEFAULT_REV_ID PFRU_REVID_1
> +
> +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,
> + CAP_NR_IDX
> +};
> +
> +enum buf_index {
> + BUF_STATUS_IDX = 0,
> + BUF_EXT_STATUS_IDX = 1,
> + BUF_ADDR_LOW_IDX = 2,
> + BUF_ADDR_HI_IDX = 3,
> + BUF_SIZE_IDX = 4,
> + BUF_NR_IDX
> +};
> +
> +enum update_index {
> + UPDATE_STATUS_IDX = 0,
> + UPDATE_EXT_STATUS_IDX = 1,
> + UPDATE_AUTH_TIME_LOW_IDX = 2,
> + UPDATE_AUTH_TIME_HI_IDX = 3,
> + UPDATE_EXEC_TIME_LOW_IDX = 4,
> + UPDATE_EXEC_TIME_HI_IDX = 5,
> + UPDATE_NR_IDX
> +};
> +
> +enum pfru_start_action {
The pfru prefix here (and below) is OK.
> + START_STAGE = 0,
> + START_ACTIVATE = 1,
> + START_STAGE_ACTIVATE = 2,
> +};
> +
> +struct pfru_device {
> + u32 rev_id, index;
> + struct device *parent_dev;
> + struct miscdevice miscdev;
> +};
> +
> +static DEFINE_IDA(pfru_ida);
> +
> +static const guid_t pfru_guid =
> + GUID_INIT(0xECF9533B, 0x4A3C, 0x4E89, 0x93, 0x9E, 0xC7, 0x71,
> + 0x12, 0x60, 0x1C, 0x6D);
> +
> +static const guid_t pfru_code_inj_guid =
> + GUID_INIT(0xB2F84B79, 0x7B6E, 0x4E45, 0x88, 0x5F, 0x3F, 0xB9,
> + 0xBB, 0x18, 0x54, 0x02);
> +
> +static const guid_t pfru_drv_update_guid =
> + GUID_INIT(0x4569DD8C, 0x75F1, 0x429A, 0xA3, 0xD6, 0x24, 0xDE,
> + 0x80, 0x97, 0xA0, 0xDF);
The above are _DSM UUIDs, right?
If so, please add a comment pointing to their definitions and
explaining briefly what they are for.
> +
> +static inline int pfru_valid_revid(u32 id)
> +{
> + return id == PFRU_REVID_1 || id == PFRU_REVID_2;
> +}
> +
> +static inline struct pfru_device *to_pfru_dev(struct file *file)
> +{
> + return container_of(file->private_data, struct pfru_device, miscdev);
> +}
> +
> +static int query_capability(struct pfru_update_cap_info *cap_hdr,
> + struct pfru_device *pfru_dev)
> +{
> + acpi_handle handle = ACPI_HANDLE(pfru_dev->parent_dev);
> + union acpi_object *out_obj;
> + int ret = -EINVAL;
> +
> + out_obj = acpi_evaluate_dsm_typed(handle, &pfru_guid,
> + pfru_dev->rev_id,
> + PFRU_FUNC_QUERY_UPDATE_CAP,
> + NULL, ACPI_TYPE_PACKAGE);
> + if (!out_obj)
> + return ret;
> +
> + if (out_obj->package.count < CAP_NR_IDX)
> + goto free_acpi_buffer;
> +
> + if (out_obj->package.elements[CAP_STATUS_IDX].type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + cap_hdr->status = out_obj->package.elements[CAP_STATUS_IDX].integer.value;
> +
> + if (out_obj->package.elements[CAP_UPDATE_IDX].type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + cap_hdr->update_cap = out_obj->package.elements[CAP_UPDATE_IDX].integer.value;
> +
> + if (out_obj->package.elements[CAP_CODE_TYPE_IDX].type != ACPI_TYPE_BUFFER)
> + goto free_acpi_buffer;
> +
> + memcpy(&cap_hdr->code_type,
> + out_obj->package.elements[CAP_CODE_TYPE_IDX].buffer.pointer,
> + out_obj->package.elements[CAP_CODE_TYPE_IDX].buffer.length);
> +
> + if (out_obj->package.elements[CAP_FW_VER_IDX].type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + cap_hdr->fw_version =
> + out_obj->package.elements[CAP_FW_VER_IDX].integer.value;
> +
> + if (out_obj->package.elements[CAP_CODE_RT_VER_IDX].type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + cap_hdr->code_rt_version =
> + out_obj->package.elements[CAP_CODE_RT_VER_IDX].integer.value;
> +
> + if (out_obj->package.elements[CAP_DRV_TYPE_IDX].type != ACPI_TYPE_BUFFER)
> + goto free_acpi_buffer;
> +
> + memcpy(&cap_hdr->drv_type,
> + out_obj->package.elements[CAP_DRV_TYPE_IDX].buffer.pointer,
> + out_obj->package.elements[CAP_DRV_TYPE_IDX].buffer.length);
> +
> + if (out_obj->package.elements[CAP_DRV_RT_VER_IDX].type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + cap_hdr->drv_rt_version =
> + out_obj->package.elements[CAP_DRV_RT_VER_IDX].integer.value;
> +
> + if (out_obj->package.elements[CAP_DRV_SVN_IDX].type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + cap_hdr->drv_svn =
> + out_obj->package.elements[CAP_DRV_SVN_IDX].integer.value;
> +
> + if (out_obj->package.elements[CAP_PLAT_ID_IDX].type != ACPI_TYPE_BUFFER)
> + goto free_acpi_buffer;
> +
> + memcpy(&cap_hdr->platform_id,
> + out_obj->package.elements[CAP_PLAT_ID_IDX].buffer.pointer,
> + out_obj->package.elements[CAP_PLAT_ID_IDX].buffer.length);
> +
> + if (out_obj->package.elements[CAP_OEM_ID_IDX].type != ACPI_TYPE_BUFFER)
> + goto free_acpi_buffer;
> +
> + memcpy(&cap_hdr->oem_id,
> + out_obj->package.elements[CAP_OEM_ID_IDX].buffer.pointer,
> + out_obj->package.elements[CAP_OEM_ID_IDX].buffer.length);
> +
> + if (out_obj->package.elements[CAP_OEM_INFO_IDX].type != ACPI_TYPE_BUFFER)
> + goto free_acpi_buffer;
> +
> + cap_hdr->oem_info_len =
> + out_obj->package.elements[CAP_OEM_INFO_IDX].buffer.length;
> +
> + ret = 0;
The number of goto statements and possibly redundant memory updates
can be reduced by doing all of the checks upfront (possibly in one big
if () statement even). And there would be fewer LOC then.
> +
> +free_acpi_buffer:
> + kfree(out_obj);
> +
> + return ret;
> +}
> +
> +static int query_buffer(struct pfru_com_buf_info *info,
> + struct pfru_device *pfru_dev)
> +{
> + acpi_handle handle = ACPI_HANDLE(pfru_dev->parent_dev);
> + union acpi_object *out_obj;
> + int ret = -EINVAL;
> +
> + out_obj = acpi_evaluate_dsm_typed(handle, &pfru_guid,
> + pfru_dev->rev_id, PFRU_FUNC_QUERY_BUF,
> + NULL, ACPI_TYPE_PACKAGE);
> + if (!out_obj)
> + return ret;
> +
> + if (out_obj->package.count < BUF_NR_IDX)
> + goto free_acpi_buffer;
> +
> + if (out_obj->package.elements[BUF_STATUS_IDX].type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + info->status = out_obj->package.elements[BUF_STATUS_IDX].integer.value;
> +
> + if (out_obj->package.elements[BUF_EXT_STATUS_IDX].type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + info->ext_status =
> + out_obj->package.elements[BUF_EXT_STATUS_IDX].integer.value;
> +
> + if (out_obj->package.elements[BUF_ADDR_LOW_IDX].type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + info->addr_lo =
> + out_obj->package.elements[BUF_ADDR_LOW_IDX].integer.value;
> +
> + if (out_obj->package.elements[BUF_ADDR_HI_IDX].type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + info->addr_hi =
> + out_obj->package.elements[BUF_ADDR_HI_IDX].integer.value;
> +
> + if (out_obj->package.elements[BUF_SIZE_IDX].type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + info->buf_size = out_obj->package.elements[BUF_SIZE_IDX].integer.value;
The same comment applies to this function.
> +
> + ret = 0;
> +
> +free_acpi_buffer:
> + kfree(out_obj);
> +
> + return ret;
> +}
> +
> +static int get_image_type(const struct efi_manage_capsule_image_header *img_hdr,
> + struct pfru_device *pfru_dev)
> +{
> + const efi_guid_t *image_type_id = &img_hdr->image_type_id;
> +
> + /* check whether this is a code injection or driver update */
> + if (guid_equal(image_type_id, &pfru_code_inj_guid))
> + return PFRU_CODE_INJECT_TYPE;
> +
> + if (guid_equal(image_type_id, &pfru_drv_update_guid))
> + return PFRU_DRIVER_UPDATE_TYPE;
> +
> + return -EINVAL;
> +}
> +
> +static int adjust_efi_size(const struct efi_manage_capsule_image_header *img_hdr,
> + int size)
> +{
> + /*
> + * The (u64 hw_ins) was introduced in UEFI spec version 2,
> + * and (u64 capsule_support) was introduced in version 3.
> + * The size needs to be adjusted accordingly. That is to
> + * say, version 1 should subtract the size of hw_ins+capsule_support,
> + * and version 2 should sbstract the size of capsule_support.
> + */
> + size += sizeof(struct efi_manage_capsule_image_header);
> + switch (img_hdr->ver) {
> + case 1:
> + return size - 2 * sizeof(u64);
> + case 2:
> + return size - sizeof(u64);
> + default:
> + /* only support version 1 and 2 */
> + return -EINVAL;
> + }
> +}
> +
> +static bool valid_version(const void *data, struct pfru_update_cap_info *cap,
> + struct pfru_device *pfru_dev)
> +{
> + struct pfru_payload_hdr *payload_hdr;
> + const efi_capsule_header_t *cap_hdr = data;
> + const struct efi_manage_capsule_header *m_hdr;
> + const struct efi_manage_capsule_image_header *m_img_hdr;
> + const struct efi_image_auth *auth;
> + int type, size;
> +
> + /*
> + * Sanity check if the capsule image has a newer version
> + * than current one. In this way, 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)
So this really isn't "valid", but "applicable", so I would call the
function applicable_image().
And I would use a different comment text, like the following:
"If the code in the capsule is older than the current firmware code,
the update will be rejected by the firmware, so check the version of
it upfront without engaging the Management Mode update mechanism which
may be costly."
> + */
> + size = cap_hdr->headersize;
> + m_hdr = data + size;
> + /*
> + * Current data structure size plus variable array indicated
> + * by number of (emb_drv_cnt + payload_cnt)
> + */
> + size += offsetof(struct efi_manage_capsule_header, offset_list) +
> + (m_hdr->emb_drv_cnt + m_hdr->payload_cnt) * sizeof(u64);
> + m_img_hdr = data + size;
> +
> + type = get_image_type(m_img_hdr, pfru_dev);
> + if (type < 0)
> + return false;
> +
> + size = adjust_efi_size(m_img_hdr, size);
> + if (size < 0)
> + return false;
> +
> + auth = data + size;
> + size += sizeof(u64) + auth->auth_info.hdr.len;
> + payload_hdr = (struct pfru_payload_hdr *)(data + size);
> +
> + /* finally compare the version */
> + if (type == PFRU_CODE_INJECT_TYPE)
> + return payload_hdr->rt_ver >= cap->code_rt_version;
> + else
Redundant else.
> + return payload_hdr->rt_ver >= cap->drv_rt_version;
> +}
> +
> +static void dump_update_result(struct pfru_updated_result *result,
> + struct pfru_device *pfru_dev)
What about calling this print_ipdate_debug_info()?
> +{
> + dev_dbg(pfru_dev->parent_dev, "Update result:\n");
> + dev_dbg(pfru_dev->parent_dev, "Status:%d\n", result->status);
> + dev_dbg(pfru_dev->parent_dev, "Extended Status:%d\n", result->ext_status);
> + dev_dbg(pfru_dev->parent_dev, "Authentication Time Low:%lld\n",
> + result->low_auth_time);
> + dev_dbg(pfru_dev->parent_dev, "Authentication Time High:%lld\n",
> + result->high_auth_time);
> + dev_dbg(pfru_dev->parent_dev, "Execution Time Low:%lld\n",
> + result->low_exec_time);
> + dev_dbg(pfru_dev->parent_dev, "Execution Time High:%lld\n",
> + result->high_exec_time);
> +}
> +
> +static int start_acpi_update(int action, struct pfru_device *pfru_dev)
Why not just start_update()?
> +{
> + union acpi_object *out_obj, in_obj, in_buf;
> + struct pfru_updated_result update_result;
> + acpi_handle handle;
> + int ret = -EINVAL;
> +
> + memset(&in_obj, 0, sizeof(in_obj));
> + memset(&in_buf, 0, sizeof(in_buf));
> + in_obj.type = ACPI_TYPE_PACKAGE;
> + in_obj.package.count = 1;
> + in_obj.package.elements = &in_buf;
> + in_buf.type = ACPI_TYPE_INTEGER;
> + in_buf.integer.value = action;
> +
> + handle = ACPI_HANDLE(pfru_dev->parent_dev);
> + out_obj = acpi_evaluate_dsm_typed(handle, &pfru_guid,
> + pfru_dev->rev_id, PFRU_FUNC_START,
> + &in_obj, ACPI_TYPE_PACKAGE);
> + if (!out_obj)
> + return ret;
> +
> + if (out_obj->package.count < UPDATE_NR_IDX)
> + goto free_acpi_buffer;
> +
> + if (out_obj->package.elements[UPDATE_STATUS_IDX].type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + update_result.status =
> + out_obj->package.elements[UPDATE_STATUS_IDX].integer.value;
> +
> + if (out_obj->package.elements[UPDATE_EXT_STATUS_IDX].type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + update_result.ext_status =
> + out_obj->package.elements[UPDATE_EXT_STATUS_IDX].integer.value;
> +
> + if (out_obj->package.elements[UPDATE_AUTH_TIME_LOW_IDX].type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + update_result.low_auth_time =
> + out_obj->package.elements[UPDATE_AUTH_TIME_LOW_IDX].integer.value;
> +
> + if (out_obj->package.elements[UPDATE_AUTH_TIME_HI_IDX].type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + update_result.high_auth_time =
> + out_obj->package.elements[UPDATE_AUTH_TIME_HI_IDX].integer.value;
> +
> + if (out_obj->package.elements[UPDATE_EXEC_TIME_LOW_IDX].type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + update_result.low_exec_time =
> + out_obj->package.elements[UPDATE_EXEC_TIME_LOW_IDX].integer.value;
> +
> + if (out_obj->package.elements[UPDATE_EXEC_TIME_HI_IDX].type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + update_result.high_exec_time =
> + out_obj->package.elements[UPDATE_EXEC_TIME_HI_IDX].integer.value;
Again, checks can be combined to reduce the number of goto statements etc.
> +
> + dump_update_result(&update_result, pfru_dev);
> + ret = 0;
> +
> +free_acpi_buffer:
> + kfree(out_obj);
> +
> + return ret;
> +}
> +
> +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) {
> + 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)))
> + return -EFAULT;
> +
> + return 0;
I would add a blank line before each "case xxx:" line to visually
separate the cases from each other.
> + case PFRU_IOC_SET_REV:
> + if (copy_from_user(&rev, p, sizeof(rev)))
> + 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;
> + }
> +}
> +
> +static ssize_t pfru_write(struct file *file, const char __user *buf,
> + size_t len, loff_t *ppos)
> +{
> + struct pfru_device *pfru_dev = to_pfru_dev(file);
> + struct pfru_update_cap_info cap;
> + struct pfru_com_buf_info buf_info;
> + phys_addr_t phy_addr;
> + struct iov_iter iter;
> + struct iovec iov;
> + char *buf_ptr;
> + int ret;
> +
> + ret = query_buffer(&buf_info, pfru_dev);
> + if (ret)
> + return ret;
> +
> + if (len > buf_info.buf_size)
> + return -EINVAL;
> +
> + iov.iov_base = (void __user *)buf;
> + iov.iov_len = len;
> + iov_iter_init(&iter, WRITE, &iov, 1, len);
> +
> + /* map the communication buffer */
> + phy_addr = (phys_addr_t)((buf_info.addr_hi << 32) | buf_info.addr_lo);
> + buf_ptr = memremap(phy_addr, buf_info.buf_size, MEMREMAP_WB);
> + if (IS_ERR(buf_ptr))
> + return PTR_ERR(buf_ptr);
> +
> + if (!copy_from_iter_full(buf_ptr, len, &iter)) {
> + ret = -EINVAL;
> + goto unmap;
> + }
> +
> + /* check if the capsule header has a valid version number */
> + ret = query_capability(&cap, pfru_dev);
> + if (ret)
> + goto unmap;
> +
> + if (cap.status != DSM_SUCCEED)
> + ret = -EBUSY;
Can this check be made by query_capability() and -EBUSY returned by it
if the check triggers?
> + else if (!valid_version(buf_ptr, &cap, pfru_dev))
> + ret = -EINVAL;
> +
> +unmap:
> + memunmap(buf_ptr);
> +
> + return ret ?: len;
> +}
> +
> +static const struct file_operations acpi_pfru_fops = {
> + .owner = THIS_MODULE,
> + .write = pfru_write,
> + .unlocked_ioctl = pfru_ioctl,
> + .llseek = noop_llseek,
> +};
> +
> +static int acpi_pfru_remove(struct platform_device *pdev)
> +{
> + struct pfru_device *pfru_dev = platform_get_drvdata(pdev);
> +
> + misc_deregister(&pfru_dev->miscdev);
> +
> + return 0;
> +}
> +
> +static void pfru_put_idx(void *data)
> +{
> + struct pfru_device *pfru_dev = data;
> +
> + ida_free(&pfru_ida, pfru_dev->index);
> +}
> +
> +static int acpi_pfru_probe(struct platform_device *pdev)
> +{
> + acpi_handle handle = ACPI_HANDLE(&pdev->dev);
> + struct pfru_device *pfru_dev;
> + int ret;
> +
> + if (!acpi_has_method(handle, "_DSM")) {
> + dev_dbg(&pdev->dev, "Missing _DSM\n");
> + return -ENODEV;
> + }
> +
> + pfru_dev = devm_kzalloc(&pdev->dev, sizeof(*pfru_dev), GFP_KERNEL);
> + if (!pfru_dev)
> + return -ENOMEM;
> +
> + ret = ida_alloc(&pfru_ida, GFP_KERNEL);
> + if (ret < 0)
> + return ret;
> +
> + pfru_dev->index = ret;
> + ret = devm_add_action_or_reset(&pdev->dev, pfru_put_idx, pfru_dev);
> + if (ret)
> + return ret;
> +
> + pfru_dev->rev_id = PFRU_DEFAULT_REV_ID;
> + pfru_dev->parent_dev = &pdev->dev;
> +
> + pfru_dev->miscdev.minor = MISC_DYNAMIC_MINOR;
> + pfru_dev->miscdev.name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> + "pfru%d", pfru_dev->index);
> + if (!pfru_dev->miscdev.name)
> + return -ENOMEM;
> +
> + pfru_dev->miscdev.nodename = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> + "acpi_pfru%d", pfru_dev->index);
So in accordance with the naming considerations above, I would call
this devis "acpi_pfr_update".
> + if (!pfru_dev->miscdev.nodename)
> + return -ENOMEM;
> +
> + pfru_dev->miscdev.fops = &acpi_pfru_fops;
> + pfru_dev->miscdev.parent = &pdev->dev;
> +
> + ret = misc_register(&pfru_dev->miscdev);
> + if (ret)
> + return ret;
> +
> + platform_set_drvdata(pdev, pfru_dev);
> +
> + return 0;
> +}
> +
> +static const struct acpi_device_id acpi_pfru_ids[] = {
> + {"INTC1080"},
> + {}
> +};
> +MODULE_DEVICE_TABLE(acpi, acpi_pfru_ids);
> +
> +static struct platform_driver acpi_pfru_driver = {
> + .driver = {
> + .name = "pfru_update",
"pfr_update" ?
> + .acpi_match_table = acpi_pfru_ids,
> + },
> + .probe = acpi_pfru_probe,
> + .remove = acpi_pfru_remove,
> +};
> +module_platform_driver(acpi_pfru_driver);
> +
> +MODULE_DESCRIPTION("Platform Firmware Runtime Update device driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/uapi/linux/pfru.h b/include/uapi/linux/pfru.h
> new file mode 100644
> index 000000000000..fed50cb01309
> --- /dev/null
> +++ b/include/uapi/linux/pfru.h
> @@ -0,0 +1,174 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Platform Firmware Runtime Update header
> + *
> + * Copyright(c) 2021 Intel Corporation. All rights reserved.
> + */
> +#ifndef __PFRU_H__
> +#define __PFRU_H__
> +
> +#include <linux/ioctl.h>
> +#include <linux/types.h>
> +
> +#define PFRU_MAGIC_FOR_IOCTL 0xEE
If you follow my comments regarding the naming, this should be
PFRUT_MAGIC_FOR_IOCTL, or even PFRUT_IOCTL_MAGIC for brevity.
> +
> +/**
> + * PFRU_IOC_SET_REV - _IOW(PFRU_MAGIC_FOR_IOCTL, 0x01, unsigned int)
> + *
> + * Return:
> + * * 0 - success
> + * * -EFAULT - fail to read the revision id
> + * * -EINVAL - user provides an invalid revision id
> + *
> + * Set the Revision ID for PFRU Runtime Update.
> + */
> +#define PFRU_IOC_SET_REV _IOW(PFRU_MAGIC_FOR_IOCTL, 0x01, unsigned int)
> +
> +/**
> + * PFRU_IOC_STAGE - _IOW(PFRU_MAGIC_FOR_IOCTL, 0x02, unsigned int)
> + *
> + * Return:
> + * * 0 - success
> + * * -EINVAL - stage phase returns invalid result
> + *
> + * Stage a capsule image from communication buffer and perform authentication.
> + */
> +#define PFRU_IOC_STAGE _IOW(PFRU_MAGIC_FOR_IOCTL, 0x02, unsigned int)
> +
> +/**
> + * PFRU_IOC_ACTIVATE - _IOW(PFRU_MAGIC_FOR_IOCTL, 0x03, unsigned int)
> + *
> + * Return:
> + * * 0 - success
> + * * -EINVAL - activate phase returns invalid result
> + *
> + * Activate a previous staged capsule image.
"previously"
> + */
> +#define PFRU_IOC_ACTIVATE _IOW(PFRU_MAGIC_FOR_IOCTL, 0x03, unsigned int)
> +
> +/**
> + * PFRU_IOC_STAGE_ACTIVATE - _IOW(PFRU_MAGIC_FOR_IOCTL, 0x04, unsigned int)
> + *
> + * Return:
> + * * 0 - success
> + * * -EINVAL - stage/activate phase returns invalid result.
> + *
> + * Perform both stage and activation action.
> + */
> +#define PFRU_IOC_STAGE_ACTIVATE _IOW(PFRU_MAGIC_FOR_IOCTL, 0x04, unsigned int)
> +
> +/**
> + * PFRU_IOC_QUERY_CAP - _IOR(PFRU_MAGIC_FOR_IOCTL, 0x05,
> + * struct pfru_update_cap_info)
> + *
> + * Return:
> + * * 0 - success
> + * * -EINVAL - query phase returns invalid result
> + * * -EFAULT - the result fails to be copied to userspace
> + *
> + * Retrieve information about the PFRU Runtime Update capability.
"information on"
> + * The information is a struct pfru_update_cap_info.
> + */
> +#define PFRU_IOC_QUERY_CAP _IOR(PFRU_MAGIC_FOR_IOCTL, 0x05, struct pfru_update_cap_info)
> +
> +/**
> + * struct pfru_payload_hdr - Capsule file payload header.
> + *
> + * @sig: Signature of this capsule file.
> + * @hdr_version: Revision of this header structure.
> + * @hdr_size: Size of this header, including the OemHeader bytes.
> + * @hw_ver: The supported firmware version.
> + * @rt_ver: Version of the code injection image.
> + * @platform_id: A platform specific GUID to specify the platform what
> + * this capsule image support.
> + */
> +struct pfru_payload_hdr {
> + __u32 sig;
> + __u32 hdr_version;
> + __u32 hdr_size;
> + __u32 hw_ver;
> + __u32 rt_ver;
> + __u8 platform_id[16];
> +};
> +
> +enum pfru_dsm_status {
> + DSM_SUCCEED = 0,
> + DSM_FUNC_NOT_SUPPORT = 1,
> + DSM_INVAL_INPUT = 2,
> + DSM_HARDWARE_ERR = 3,
> + DSM_RETRY_SUGGESTED = 4,
> + DSM_UNKNOWN = 5,
> + DSM_FUNC_SPEC_ERR = 6,
> +};
> +
> +/**
> + * struct pfru_update_cap_info - Runtime update capability information.
> + *
> + * @status: Indicator of whether this query succeed.
> + * @update_cap: Bitmap to indicate whether the feature is supported.
> + * @code_type: A buffer containing an image type GUID.
> + * @fw_version: Platform firmware version.
> + * @code_rt_version: Code injection runtime version for anti-rollback.
> + * @drv_type: A buffer containing an image type GUID.
> + * @drv_rt_version: The version of the driver update runtime code.
> + * @drv_svn: The secure version number(SVN) of the driver update runtime code.
> + * @platform_id: A buffer containing a platform ID GUID.
> + * @oem_id: A buffer containing an OEM ID GUID.
> + * @oem_info_len: Length of the buffer containing the vendor specific information.
> + */
> +struct pfru_update_cap_info {
> + __u32 status;
> + __u32 update_cap;
> +
> + __u8 code_type[16];
> + __u32 fw_version;
> + __u32 code_rt_version;
> +
> + __u8 drv_type[16];
> + __u32 drv_rt_version;
> + __u32 drv_svn;
> +
> + __u8 platform_id[16];
> + __u8 oem_id[16];
> +
> + __u32 oem_info_len;
> +};
> +
> +/**
> + * struct pfru_com_buf_info - Communication buffer information.
> + *
> + * @status: Indicator of whether this query succeed.
> + * @ext_status: Implementation specific query result.
> + * @addr_lo: Low 32bit physical address of the communication buffer to hold
> + * a runtime update package.
> + * @addr_hi: High 32bit physical address of the communication buffer to hold
> + * a runtime update package.
> + * @buf_size: Maximum size in bytes of the communication buffer.
> + */
> +struct pfru_com_buf_info {
> + __u32 status;
> + __u32 ext_status;
> + __u64 addr_lo;
> + __u64 addr_hi;
> + __u32 buf_size;
> +};
> +
> +/**
> + * struct pfru_updated_result - Platform firmware runtime update result information.
> + * @status: Indicator of whether this update succeed.
> + * @ext_status: Implementation specific update result.
> + * @low_auth_time: Low 32bit value of image authentication time in nanosecond.
> + * @high_auth_time: High 32bit value of image authentication time in nanosecond.
> + * @low_exec_time: Low 32bit value of image execution time in nanosecond.
> + * @high_exec_time: High 32bit value of image execution time in nanosecond.
> + */
> +struct pfru_updated_result {
> + __u32 status;
> + __u32 ext_status;
> + __u64 low_auth_time;
> + __u64 high_auth_time;
> + __u64 low_exec_time;
> + __u64 high_exec_time;
> +};
> +
> +#endif /* __PFRU_H__ */
> --
Overall, I think that the design is fine now, it just needs to be made nicer.
[Please wait for my comments on the next patch before sending a new version.]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v11 2/4] drivers/acpi: Introduce Platform Firmware Runtime Update device driver
2021-12-03 19:37 ` Rafael J. Wysocki
@ 2021-12-04 8:07 ` Chen Yu
0 siblings, 0 replies; 17+ messages in thread
From: Chen Yu @ 2021-12-04 8:07 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: ACPI Devel Maling List, Greg Kroah-Hartman, Ard Biesheuvel,
Len Brown, Ashok Raj, Andy Shevchenko, Mike Rapoport,
Linux Kernel Mailing List
On Fri, Dec 03, 2021 at 08:37:44PM +0100, Rafael J. Wysocki wrote:
> On Sun, Nov 21, 2021 at 4:17 PM Chen Yu <yu.c.chen@intel.com> wrote:
> >
> > Introduce the pfru_update driver which can be used for Platform Firmware
> > Runtime code injection and driver update [1].
>
> The PFRU TLA already covers the "update" part. so the naming is a bit unclean.
>
> IMO any of pfrut_update, or pfr_update, or even pfru alone would be better.
>
> My favorite is pfr_update, and the telemetry part can be called
> pfr_telementry for symmetry.
>
Ok, will change it to this name.
> > The user is expected to
> > provide the update firmware in the form of capsule file, and pass it to
>
> This is an EFI capsule and not necessarily in a separate file, so why
> don't you just say "EFI capsule"?
>
ok, will do.
> > the driver via ioctl.
>
> It is actually passed by writing the capsule to a device special file, isn't it?
>
Yes, it is, I'll revise it.
> The ioctl() interface is necessary for setup/cleanup.
>
> > Then the driver would hand this capsule file to the
> > Platform Firmware Runtime Update via the ACPI device _DSM method. At last
>
> "The capsule is transferred by the driver to the platform firmware
> with the help of an ACPI _DSM method under the special ACPI Platform
> Firmware Runtime Update device (INTC1080)"
>
Ok.
> > the low level Management Mode would do the firmware update.
>
> "and the actual firmware update is carried out by the low-level
> Management Mode code in the platform firmware."
>
Ok.
> > The corresponding userspace tool and man page will be introduced at
> > tools/power/acpi/tools/pfru.
>
> The above is irrelevant for this particular patch IMO.
>
> What is relevant, and missing, is a bit of justification, like why it
> is a good idea to add this driver to the kernel. There are some
> pieces of it in the Kconfig help text and elsewhere, but it should be
> there in the patch changelog too.
>
Ok, will add it in the changelog.
> > Link: https://uefi.org/sites/default/files/resources/Intel_MM_OS_Interface_Spec_Rev100.pdf # [1]
> > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > ---
>
> [cut the history]
>
> > ---
> > .../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 | 601 ++++++++++++++++++
> > include/uapi/linux/pfru.h | 174 +++++
> > 7 files changed, 793 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
> >
> > diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > index cfe6cccf0f44..adf830410e0d 100644
> > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > @@ -367,6 +367,7 @@ Code Seq# Include File Comments
> > <mailto:aherrman@de.ibm.com>
> > 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
>
> If this is called pfrut.h, it would be more consistent with the description.
>
Ok, will change it.
> > 0xF3 00-3F drivers/usb/misc/sisusbvga/sisusb.h sisfb (in development)
> > <mailto:thomas@winischhofer.net>
> > 0xF6 all LTTng Linux Trace Toolkit Next Generation
> > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> > index cdbdf68bd98f..1cf26e404b63 100644
> > --- a/drivers/acpi/Kconfig
> > +++ b/drivers/acpi/Kconfig
> > @@ -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
> > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> > index 3018714e87d9..9c2c5ddff6ec 100644
> > --- a/drivers/acpi/Makefile
> > +++ b/drivers/acpi/Makefile
> > @@ -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
> > diff --git a/drivers/acpi/pfru/Kconfig b/drivers/acpi/pfru/Kconfig
> > new file mode 100644
> > index 000000000000..fd0a41b88fac
> > --- /dev/null
> > +++ b/drivers/acpi/pfru/Kconfig
> > @@ -0,0 +1,13 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +config ACPI_PFRU
>
> The telemetry will depend on the same config option, so I would call
> it ACPI_PFRUT.
>
Ok.
> > + tristate "ACPI Platform Firmware Runtime Update (PFRU)"
>
> And I would simply say "ACPI Platform Firmware Runtime Update and
> Telemetry" here.
>
Ok.
> > + 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.
>
> The above needs to be edited. What about using something like this instead:
>
> "This mechanism allows certain pieces of the platform firmware to be
> updated on the fly while the system is running (runtime) without the
> need to restart it, which is key in the cases when the system needs to
> be available 100% of the time and it cannot afford the downtime
> related to restarting it, or when the work carried out by the system
> is particularly important, so it cannot be interrupted, and it is not
> practical to wait until it is complete.
>
> The existing firmware code can be modified (driver update) or extended
> by adding new code to the firmware (code injection)."
>
Ok, will change it.
> The telemetry part will be added separately later, so it need not be
> mentioned here just yet.
>
Ok.
> > +
> > + To compile this driver as module, choose M here:
> > + the modules will be called pfru_update and pfru_telemetry.
> > diff --git a/drivers/acpi/pfru/Makefile b/drivers/acpi/pfru/Makefile
> > new file mode 100644
> > index 000000000000..098cbe80cf3d
> > --- /dev/null
> > +++ b/drivers/acpi/pfru/Makefile
> > @@ -0,0 +1,2 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +obj-$(CONFIG_ACPI_PFRU) += pfru_update.o
> > diff --git a/drivers/acpi/pfru/pfru_update.c b/drivers/acpi/pfru/pfru_update.c
> > new file mode 100644
> > index 000000000000..cf670d052cc9
> > --- /dev/null
> > +++ b/drivers/acpi/pfru/pfru_update.c
>
> So why don't you call this file pfr_update.c?
>
Ok, will change it.
> > @@ -0,0 +1,601 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * ACPI Platform Firmware Runtime Update Device Driver
> > + *
> > + * Copyright (C) 2021 Intel Corporation
> > + * Author: Chen Yu <yu.c.chen@intel.com>
>
> Please describe what the driver is for here at least briefly.
>
Ok, will do.
> > + */
> > +#include <linux/acpi.h>
> > +#include <linux/device.h>
> > +#include <linux/efi.h>
> > +#include <linux/err.h>
> > +#include <linux/errno.h>
> > +#include <linux/file.h>
> > +#include <linux/fs.h>
> > +#include <linux/idr.h>
> > +#include <linux/miscdevice.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/string.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/uio.h>
> > +#include <linux/uuid.h>
> > +
> > +#include <uapi/linux/pfru.h>
> > +
> > +#define PFRU_FUNC_STANDARD_QUERY 0
> > +#define PFRU_FUNC_QUERY_UPDATE_CAP 1
> > +#define PFRU_FUNC_QUERY_BUF 2
> > +#define PFRU_FUNC_START 3
> > +
> > +#define PFRU_CODE_INJECT_TYPE 1
> > +#define PFRU_DRIVER_UPDATE_TYPE 2
> > +
> > +#define PFRU_REVID_1 1
> > +#define PFRU_REVID_2 2
> > +#define PFRU_DEFAULT_REV_ID PFRU_REVID_1
> > +
> > +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,
> > + CAP_NR_IDX
> > +};
> > +
> > +enum buf_index {
> > + BUF_STATUS_IDX = 0,
> > + BUF_EXT_STATUS_IDX = 1,
> > + BUF_ADDR_LOW_IDX = 2,
> > + BUF_ADDR_HI_IDX = 3,
> > + BUF_SIZE_IDX = 4,
> > + BUF_NR_IDX
> > +};
> > +
> > +enum update_index {
> > + UPDATE_STATUS_IDX = 0,
> > + UPDATE_EXT_STATUS_IDX = 1,
> > + UPDATE_AUTH_TIME_LOW_IDX = 2,
> > + UPDATE_AUTH_TIME_HI_IDX = 3,
> > + UPDATE_EXEC_TIME_LOW_IDX = 4,
> > + UPDATE_EXEC_TIME_HI_IDX = 5,
> > + UPDATE_NR_IDX
> > +};
> > +
> > +enum pfru_start_action {
>
> The pfru prefix here (and below) is OK.
>
> > + START_STAGE = 0,
> > + START_ACTIVATE = 1,
> > + START_STAGE_ACTIVATE = 2,
> > +};
> > +
> > +struct pfru_device {
> > + u32 rev_id, index;
> > + struct device *parent_dev;
> > + struct miscdevice miscdev;
> > +};
> > +
> > +static DEFINE_IDA(pfru_ida);
> > +
> > +static const guid_t pfru_guid =
> > + GUID_INIT(0xECF9533B, 0x4A3C, 0x4E89, 0x93, 0x9E, 0xC7, 0x71,
> > + 0x12, 0x60, 0x1C, 0x6D);
> > +
> > +static const guid_t pfru_code_inj_guid =
> > + GUID_INIT(0xB2F84B79, 0x7B6E, 0x4E45, 0x88, 0x5F, 0x3F, 0xB9,
> > + 0xBB, 0x18, 0x54, 0x02);
> > +
> > +static const guid_t pfru_drv_update_guid =
> > + GUID_INIT(0x4569DD8C, 0x75F1, 0x429A, 0xA3, 0xD6, 0x24, 0xDE,
> > + 0x80, 0x97, 0xA0, 0xDF);
>
> The above are _DSM UUIDs, right?
>
Yes.
> If so, please add a comment pointing to their definitions and
> explaining briefly what they are for.
>
Ok, will do.
> > +
> > +static inline int pfru_valid_revid(u32 id)
> > +{
> > + return id == PFRU_REVID_1 || id == PFRU_REVID_2;
> > +}
> > +
> > +static inline struct pfru_device *to_pfru_dev(struct file *file)
> > +{
> > + return container_of(file->private_data, struct pfru_device, miscdev);
> > +}
> > +
> > +static int query_capability(struct pfru_update_cap_info *cap_hdr,
> > + struct pfru_device *pfru_dev)
> > +{
> > + acpi_handle handle = ACPI_HANDLE(pfru_dev->parent_dev);
> > + union acpi_object *out_obj;
> > + int ret = -EINVAL;
> > +
> > + out_obj = acpi_evaluate_dsm_typed(handle, &pfru_guid,
> > + pfru_dev->rev_id,
> > + PFRU_FUNC_QUERY_UPDATE_CAP,
> > + NULL, ACPI_TYPE_PACKAGE);
> > + if (!out_obj)
> > + return ret;
> > +
> > + if (out_obj->package.count < CAP_NR_IDX)
> > + goto free_acpi_buffer;
> > +
> > + if (out_obj->package.elements[CAP_STATUS_IDX].type != ACPI_TYPE_INTEGER)
> > + goto free_acpi_buffer;
> > +
> > + cap_hdr->status = out_obj->package.elements[CAP_STATUS_IDX].integer.value;
> > +
> > + if (out_obj->package.elements[CAP_UPDATE_IDX].type != ACPI_TYPE_INTEGER)
> > + goto free_acpi_buffer;
> > +
> > + cap_hdr->update_cap = out_obj->package.elements[CAP_UPDATE_IDX].integer.value;
> > +
> > + if (out_obj->package.elements[CAP_CODE_TYPE_IDX].type != ACPI_TYPE_BUFFER)
> > + goto free_acpi_buffer;
> > +
> > + memcpy(&cap_hdr->code_type,
> > + out_obj->package.elements[CAP_CODE_TYPE_IDX].buffer.pointer,
> > + out_obj->package.elements[CAP_CODE_TYPE_IDX].buffer.length);
> > +
> > + if (out_obj->package.elements[CAP_FW_VER_IDX].type != ACPI_TYPE_INTEGER)
> > + goto free_acpi_buffer;
> > +
> > + cap_hdr->fw_version =
> > + out_obj->package.elements[CAP_FW_VER_IDX].integer.value;
> > +
> > + if (out_obj->package.elements[CAP_CODE_RT_VER_IDX].type != ACPI_TYPE_INTEGER)
> > + goto free_acpi_buffer;
> > +
> > + cap_hdr->code_rt_version =
> > + out_obj->package.elements[CAP_CODE_RT_VER_IDX].integer.value;
> > +
> > + if (out_obj->package.elements[CAP_DRV_TYPE_IDX].type != ACPI_TYPE_BUFFER)
> > + goto free_acpi_buffer;
> > +
> > + memcpy(&cap_hdr->drv_type,
> > + out_obj->package.elements[CAP_DRV_TYPE_IDX].buffer.pointer,
> > + out_obj->package.elements[CAP_DRV_TYPE_IDX].buffer.length);
> > +
> > + if (out_obj->package.elements[CAP_DRV_RT_VER_IDX].type != ACPI_TYPE_INTEGER)
> > + goto free_acpi_buffer;
> > +
> > + cap_hdr->drv_rt_version =
> > + out_obj->package.elements[CAP_DRV_RT_VER_IDX].integer.value;
> > +
> > + if (out_obj->package.elements[CAP_DRV_SVN_IDX].type != ACPI_TYPE_INTEGER)
> > + goto free_acpi_buffer;
> > +
> > + cap_hdr->drv_svn =
> > + out_obj->package.elements[CAP_DRV_SVN_IDX].integer.value;
> > +
> > + if (out_obj->package.elements[CAP_PLAT_ID_IDX].type != ACPI_TYPE_BUFFER)
> > + goto free_acpi_buffer;
> > +
> > + memcpy(&cap_hdr->platform_id,
> > + out_obj->package.elements[CAP_PLAT_ID_IDX].buffer.pointer,
> > + out_obj->package.elements[CAP_PLAT_ID_IDX].buffer.length);
> > +
> > + if (out_obj->package.elements[CAP_OEM_ID_IDX].type != ACPI_TYPE_BUFFER)
> > + goto free_acpi_buffer;
> > +
> > + memcpy(&cap_hdr->oem_id,
> > + out_obj->package.elements[CAP_OEM_ID_IDX].buffer.pointer,
> > + out_obj->package.elements[CAP_OEM_ID_IDX].buffer.length);
> > +
> > + if (out_obj->package.elements[CAP_OEM_INFO_IDX].type != ACPI_TYPE_BUFFER)
> > + goto free_acpi_buffer;
> > +
> > + cap_hdr->oem_info_len =
> > + out_obj->package.elements[CAP_OEM_INFO_IDX].buffer.length;
> > +
> > + ret = 0;
>
> The number of goto statements and possibly redundant memory updates
> can be reduced by doing all of the checks upfront (possibly in one big
> if () statement even). And there would be fewer LOC then.
>
Ok, I'll compact this.
> > +
> > +free_acpi_buffer:
> > + kfree(out_obj);
> > +
> > + return ret;
> > +}
> > +
> > +static int query_buffer(struct pfru_com_buf_info *info,
> > + struct pfru_device *pfru_dev)
> > +{
> > + acpi_handle handle = ACPI_HANDLE(pfru_dev->parent_dev);
> > + union acpi_object *out_obj;
> > + int ret = -EINVAL;
> > +
> > + out_obj = acpi_evaluate_dsm_typed(handle, &pfru_guid,
> > + pfru_dev->rev_id, PFRU_FUNC_QUERY_BUF,
> > + NULL, ACPI_TYPE_PACKAGE);
> > + if (!out_obj)
> > + return ret;
> > +
> > + if (out_obj->package.count < BUF_NR_IDX)
> > + goto free_acpi_buffer;
> > +
> > + if (out_obj->package.elements[BUF_STATUS_IDX].type != ACPI_TYPE_INTEGER)
> > + goto free_acpi_buffer;
> > +
> > + info->status = out_obj->package.elements[BUF_STATUS_IDX].integer.value;
> > +
> > + if (out_obj->package.elements[BUF_EXT_STATUS_IDX].type != ACPI_TYPE_INTEGER)
> > + goto free_acpi_buffer;
> > +
> > + info->ext_status =
> > + out_obj->package.elements[BUF_EXT_STATUS_IDX].integer.value;
> > +
> > + if (out_obj->package.elements[BUF_ADDR_LOW_IDX].type != ACPI_TYPE_INTEGER)
> > + goto free_acpi_buffer;
> > +
> > + info->addr_lo =
> > + out_obj->package.elements[BUF_ADDR_LOW_IDX].integer.value;
> > +
> > + if (out_obj->package.elements[BUF_ADDR_HI_IDX].type != ACPI_TYPE_INTEGER)
> > + goto free_acpi_buffer;
> > +
> > + info->addr_hi =
> > + out_obj->package.elements[BUF_ADDR_HI_IDX].integer.value;
> > +
> > + if (out_obj->package.elements[BUF_SIZE_IDX].type != ACPI_TYPE_INTEGER)
> > + goto free_acpi_buffer;
> > +
> > + info->buf_size = out_obj->package.elements[BUF_SIZE_IDX].integer.value;
>
> The same comment applies to this function.
>
> > +
> > + ret = 0;
> > +
> > +free_acpi_buffer:
> > + kfree(out_obj);
> > +
> > + return ret;
> > +}
> > +
> > +static int get_image_type(const struct efi_manage_capsule_image_header *img_hdr,
> > + struct pfru_device *pfru_dev)
> > +{
> > + const efi_guid_t *image_type_id = &img_hdr->image_type_id;
> > +
> > + /* check whether this is a code injection or driver update */
> > + if (guid_equal(image_type_id, &pfru_code_inj_guid))
> > + return PFRU_CODE_INJECT_TYPE;
> > +
> > + if (guid_equal(image_type_id, &pfru_drv_update_guid))
> > + return PFRU_DRIVER_UPDATE_TYPE;
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static int adjust_efi_size(const struct efi_manage_capsule_image_header *img_hdr,
> > + int size)
> > +{
> > + /*
> > + * The (u64 hw_ins) was introduced in UEFI spec version 2,
> > + * and (u64 capsule_support) was introduced in version 3.
> > + * The size needs to be adjusted accordingly. That is to
> > + * say, version 1 should subtract the size of hw_ins+capsule_support,
> > + * and version 2 should sbstract the size of capsule_support.
> > + */
> > + size += sizeof(struct efi_manage_capsule_image_header);
> > + switch (img_hdr->ver) {
> > + case 1:
> > + return size - 2 * sizeof(u64);
> > + case 2:
> > + return size - sizeof(u64);
> > + default:
> > + /* only support version 1 and 2 */
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static bool valid_version(const void *data, struct pfru_update_cap_info *cap,
> > + struct pfru_device *pfru_dev)
> > +{
> > + struct pfru_payload_hdr *payload_hdr;
> > + const efi_capsule_header_t *cap_hdr = data;
> > + const struct efi_manage_capsule_header *m_hdr;
> > + const struct efi_manage_capsule_image_header *m_img_hdr;
> > + const struct efi_image_auth *auth;
> > + int type, size;
> > +
> > + /*
> > + * Sanity check if the capsule image has a newer version
> > + * than current one. In this way, 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)
>
> So this really isn't "valid", but "applicable", so I would call the
> function applicable_image().
>
Ok.
> And I would use a different comment text, like the following:
>
> "If the code in the capsule is older than the current firmware code,
> the update will be rejected by the firmware, so check the version of
> it upfront without engaging the Management Mode update mechanism which
> may be costly."
>
Ok, will do.
> > + */
> > + size = cap_hdr->headersize;
> > + m_hdr = data + size;
> > + /*
> > + * Current data structure size plus variable array indicated
> > + * by number of (emb_drv_cnt + payload_cnt)
> > + */
> > + size += offsetof(struct efi_manage_capsule_header, offset_list) +
> > + (m_hdr->emb_drv_cnt + m_hdr->payload_cnt) * sizeof(u64);
> > + m_img_hdr = data + size;
> > +
> > + type = get_image_type(m_img_hdr, pfru_dev);
> > + if (type < 0)
> > + return false;
> > +
> > + size = adjust_efi_size(m_img_hdr, size);
> > + if (size < 0)
> > + return false;
> > +
> > + auth = data + size;
> > + size += sizeof(u64) + auth->auth_info.hdr.len;
> > + payload_hdr = (struct pfru_payload_hdr *)(data + size);
> > +
> > + /* finally compare the version */
> > + if (type == PFRU_CODE_INJECT_TYPE)
> > + return payload_hdr->rt_ver >= cap->code_rt_version;
> > + else
>
> Redundant else.
>
Will remove it.
> > + return payload_hdr->rt_ver >= cap->drv_rt_version;
> > +}
> > +
> > +static void dump_update_result(struct pfru_updated_result *result,
> > + struct pfru_device *pfru_dev)
>
> What about calling this print_ipdate_debug_info()?
>
Ok, will do.
> > +{
> > + dev_dbg(pfru_dev->parent_dev, "Update result:\n");
> > + dev_dbg(pfru_dev->parent_dev, "Status:%d\n", result->status);
> > + dev_dbg(pfru_dev->parent_dev, "Extended Status:%d\n", result->ext_status);
> > + dev_dbg(pfru_dev->parent_dev, "Authentication Time Low:%lld\n",
> > + result->low_auth_time);
> > + dev_dbg(pfru_dev->parent_dev, "Authentication Time High:%lld\n",
> > + result->high_auth_time);
> > + dev_dbg(pfru_dev->parent_dev, "Execution Time Low:%lld\n",
> > + result->low_exec_time);
> > + dev_dbg(pfru_dev->parent_dev, "Execution Time High:%lld\n",
> > + result->high_exec_time);
> > +}
> > +
> > +static int start_acpi_update(int action, struct pfru_device *pfru_dev)
>
> Why not just start_update()?
>
Ok, will change it.
> > +{
> > + union acpi_object *out_obj, in_obj, in_buf;
> > + struct pfru_updated_result update_result;
> > + acpi_handle handle;
> > + int ret = -EINVAL;
> > +
> > + memset(&in_obj, 0, sizeof(in_obj));
> > + memset(&in_buf, 0, sizeof(in_buf));
> > + in_obj.type = ACPI_TYPE_PACKAGE;
> > + in_obj.package.count = 1;
> > + in_obj.package.elements = &in_buf;
> > + in_buf.type = ACPI_TYPE_INTEGER;
> > + in_buf.integer.value = action;
> > +
> > + handle = ACPI_HANDLE(pfru_dev->parent_dev);
> > + out_obj = acpi_evaluate_dsm_typed(handle, &pfru_guid,
> > + pfru_dev->rev_id, PFRU_FUNC_START,
> > + &in_obj, ACPI_TYPE_PACKAGE);
> > + if (!out_obj)
> > + return ret;
> > +
> > + if (out_obj->package.count < UPDATE_NR_IDX)
> > + goto free_acpi_buffer;
> > +
> > + if (out_obj->package.elements[UPDATE_STATUS_IDX].type != ACPI_TYPE_INTEGER)
> > + goto free_acpi_buffer;
> > +
> > + update_result.status =
> > + out_obj->package.elements[UPDATE_STATUS_IDX].integer.value;
> > +
> > + if (out_obj->package.elements[UPDATE_EXT_STATUS_IDX].type != ACPI_TYPE_INTEGER)
> > + goto free_acpi_buffer;
> > +
> > + update_result.ext_status =
> > + out_obj->package.elements[UPDATE_EXT_STATUS_IDX].integer.value;
> > +
> > + if (out_obj->package.elements[UPDATE_AUTH_TIME_LOW_IDX].type != ACPI_TYPE_INTEGER)
> > + goto free_acpi_buffer;
> > +
> > + update_result.low_auth_time =
> > + out_obj->package.elements[UPDATE_AUTH_TIME_LOW_IDX].integer.value;
> > +
> > + if (out_obj->package.elements[UPDATE_AUTH_TIME_HI_IDX].type != ACPI_TYPE_INTEGER)
> > + goto free_acpi_buffer;
> > +
> > + update_result.high_auth_time =
> > + out_obj->package.elements[UPDATE_AUTH_TIME_HI_IDX].integer.value;
> > +
> > + if (out_obj->package.elements[UPDATE_EXEC_TIME_LOW_IDX].type != ACPI_TYPE_INTEGER)
> > + goto free_acpi_buffer;
> > +
> > + update_result.low_exec_time =
> > + out_obj->package.elements[UPDATE_EXEC_TIME_LOW_IDX].integer.value;
> > +
> > + if (out_obj->package.elements[UPDATE_EXEC_TIME_HI_IDX].type != ACPI_TYPE_INTEGER)
> > + goto free_acpi_buffer;
> > +
> > + update_result.high_exec_time =
> > + out_obj->package.elements[UPDATE_EXEC_TIME_HI_IDX].integer.value;
>
> Again, checks can be combined to reduce the number of goto statements etc.
>
> > +
> > + dump_update_result(&update_result, pfru_dev);
> > + ret = 0;
> > +
> > +free_acpi_buffer:
> > + kfree(out_obj);
> > +
> > + return ret;
> > +}
> > +
> > +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) {
> > + 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)))
> > + return -EFAULT;
> > +
> > + return 0;
>
> I would add a blank line before each "case xxx:" line to visually
> separate the cases from each other.
>
Ok, will do.
> > + case PFRU_IOC_SET_REV:
> > + if (copy_from_user(&rev, p, sizeof(rev)))
> > + 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;
> > + }
> > +}
> > +
> > +static ssize_t pfru_write(struct file *file, const char __user *buf,
> > + size_t len, loff_t *ppos)
> > +{
> > + struct pfru_device *pfru_dev = to_pfru_dev(file);
> > + struct pfru_update_cap_info cap;
> > + struct pfru_com_buf_info buf_info;
> > + phys_addr_t phy_addr;
> > + struct iov_iter iter;
> > + struct iovec iov;
> > + char *buf_ptr;
> > + int ret;
> > +
> > + ret = query_buffer(&buf_info, pfru_dev);
> > + if (ret)
> > + return ret;
> > +
> > + if (len > buf_info.buf_size)
> > + return -EINVAL;
> > +
> > + iov.iov_base = (void __user *)buf;
> > + iov.iov_len = len;
> > + iov_iter_init(&iter, WRITE, &iov, 1, len);
> > +
> > + /* map the communication buffer */
> > + phy_addr = (phys_addr_t)((buf_info.addr_hi << 32) | buf_info.addr_lo);
> > + buf_ptr = memremap(phy_addr, buf_info.buf_size, MEMREMAP_WB);
> > + if (IS_ERR(buf_ptr))
> > + return PTR_ERR(buf_ptr);
> > +
> > + if (!copy_from_iter_full(buf_ptr, len, &iter)) {
> > + ret = -EINVAL;
> > + goto unmap;
> > + }
> > +
> > + /* check if the capsule header has a valid version number */
> > + ret = query_capability(&cap, pfru_dev);
> > + if (ret)
> > + goto unmap;
> > +
> > + if (cap.status != DSM_SUCCEED)
> > + ret = -EBUSY;
>
> Can this check be made by query_capability() and -EBUSY returned by it
> if the check triggers?
>
Yes, I'll change it in next version.
> > + else if (!valid_version(buf_ptr, &cap, pfru_dev))
> > + ret = -EINVAL;
> > +
> > +unmap:
> > + memunmap(buf_ptr);
> > +
> > + return ret ?: len;
> > +}
> > +
> > +static const struct file_operations acpi_pfru_fops = {
> > + .owner = THIS_MODULE,
> > + .write = pfru_write,
> > + .unlocked_ioctl = pfru_ioctl,
> > + .llseek = noop_llseek,
> > +};
> > +
> > +static int acpi_pfru_remove(struct platform_device *pdev)
> > +{
> > + struct pfru_device *pfru_dev = platform_get_drvdata(pdev);
> > +
> > + misc_deregister(&pfru_dev->miscdev);
> > +
> > + return 0;
> > +}
> > +
> > +static void pfru_put_idx(void *data)
> > +{
> > + struct pfru_device *pfru_dev = data;
> > +
> > + ida_free(&pfru_ida, pfru_dev->index);
> > +}
> > +
> > +static int acpi_pfru_probe(struct platform_device *pdev)
> > +{
> > + acpi_handle handle = ACPI_HANDLE(&pdev->dev);
> > + struct pfru_device *pfru_dev;
> > + int ret;
> > +
> > + if (!acpi_has_method(handle, "_DSM")) {
> > + dev_dbg(&pdev->dev, "Missing _DSM\n");
> > + return -ENODEV;
> > + }
> > +
> > + pfru_dev = devm_kzalloc(&pdev->dev, sizeof(*pfru_dev), GFP_KERNEL);
> > + if (!pfru_dev)
> > + return -ENOMEM;
> > +
> > + ret = ida_alloc(&pfru_ida, GFP_KERNEL);
> > + if (ret < 0)
> > + return ret;
> > +
> > + pfru_dev->index = ret;
> > + ret = devm_add_action_or_reset(&pdev->dev, pfru_put_idx, pfru_dev);
> > + if (ret)
> > + return ret;
> > +
> > + pfru_dev->rev_id = PFRU_DEFAULT_REV_ID;
> > + pfru_dev->parent_dev = &pdev->dev;
> > +
> > + pfru_dev->miscdev.minor = MISC_DYNAMIC_MINOR;
> > + pfru_dev->miscdev.name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> > + "pfru%d", pfru_dev->index);
> > + if (!pfru_dev->miscdev.name)
> > + return -ENOMEM;
> > +
> > + pfru_dev->miscdev.nodename = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> > + "acpi_pfru%d", pfru_dev->index);
>
> So in accordance with the naming considerations above, I would call
> this devis "acpi_pfr_update".
>
Ok, will do.
> > + if (!pfru_dev->miscdev.nodename)
> > + return -ENOMEM;
> > +
> > + pfru_dev->miscdev.fops = &acpi_pfru_fops;
> > + pfru_dev->miscdev.parent = &pdev->dev;
> > +
> > + ret = misc_register(&pfru_dev->miscdev);
> > + if (ret)
> > + return ret;
> > +
> > + platform_set_drvdata(pdev, pfru_dev);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct acpi_device_id acpi_pfru_ids[] = {
> > + {"INTC1080"},
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(acpi, acpi_pfru_ids);
> > +
> > +static struct platform_driver acpi_pfru_driver = {
> > + .driver = {
> > + .name = "pfru_update",
>
> "pfr_update" ?
>
Ok.
> > + .acpi_match_table = acpi_pfru_ids,
> > + },
> > + .probe = acpi_pfru_probe,
> > + .remove = acpi_pfru_remove,
> > +};
> > +module_platform_driver(acpi_pfru_driver);
> > +
> > +MODULE_DESCRIPTION("Platform Firmware Runtime Update device driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/uapi/linux/pfru.h b/include/uapi/linux/pfru.h
> > new file mode 100644
> > index 000000000000..fed50cb01309
> > --- /dev/null
> > +++ b/include/uapi/linux/pfru.h
> > @@ -0,0 +1,174 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +/*
> > + * Platform Firmware Runtime Update header
> > + *
> > + * Copyright(c) 2021 Intel Corporation. All rights reserved.
> > + */
> > +#ifndef __PFRU_H__
> > +#define __PFRU_H__
> > +
> > +#include <linux/ioctl.h>
> > +#include <linux/types.h>
> > +
> > +#define PFRU_MAGIC_FOR_IOCTL 0xEE
>
> If you follow my comments regarding the naming, this should be
> PFRUT_MAGIC_FOR_IOCTL, or even PFRUT_IOCTL_MAGIC for brevity.
>
>
Ok, will change it to PFRUT_IOCTL_MAGIC.
> > +
> > +/**
> > + * PFRU_IOC_SET_REV - _IOW(PFRU_MAGIC_FOR_IOCTL, 0x01, unsigned int)
> > + *
> > + * Return:
> > + * * 0 - success
> > + * * -EFAULT - fail to read the revision id
> > + * * -EINVAL - user provides an invalid revision id
> > + *
> > + * Set the Revision ID for PFRU Runtime Update.
> > + */
> > +#define PFRU_IOC_SET_REV _IOW(PFRU_MAGIC_FOR_IOCTL, 0x01, unsigned int)
> > +
> > +/**
> > + * PFRU_IOC_STAGE - _IOW(PFRU_MAGIC_FOR_IOCTL, 0x02, unsigned int)
> > + *
> > + * Return:
> > + * * 0 - success
> > + * * -EINVAL - stage phase returns invalid result
> > + *
> > + * Stage a capsule image from communication buffer and perform authentication.
> > + */
> > +#define PFRU_IOC_STAGE _IOW(PFRU_MAGIC_FOR_IOCTL, 0x02, unsigned int)
> > +
> > +/**
> > + * PFRU_IOC_ACTIVATE - _IOW(PFRU_MAGIC_FOR_IOCTL, 0x03, unsigned int)
> > + *
> > + * Return:
> > + * * 0 - success
> > + * * -EINVAL - activate phase returns invalid result
> > + *
> > + * Activate a previous staged capsule image.
>
> "previously"
>
Ok.
> > + */
> > +#define PFRU_IOC_ACTIVATE _IOW(PFRU_MAGIC_FOR_IOCTL, 0x03, unsigned int)
> > +
> > +/**
> > + * PFRU_IOC_STAGE_ACTIVATE - _IOW(PFRU_MAGIC_FOR_IOCTL, 0x04, unsigned int)
> > + *
> > + * Return:
> > + * * 0 - success
> > + * * -EINVAL - stage/activate phase returns invalid result.
> > + *
> > + * Perform both stage and activation action.
> > + */
> > +#define PFRU_IOC_STAGE_ACTIVATE _IOW(PFRU_MAGIC_FOR_IOCTL, 0x04, unsigned int)
> > +
> > +/**
> > + * PFRU_IOC_QUERY_CAP - _IOR(PFRU_MAGIC_FOR_IOCTL, 0x05,
> > + * struct pfru_update_cap_info)
> > + *
> > + * Return:
> > + * * 0 - success
> > + * * -EINVAL - query phase returns invalid result
> > + * * -EFAULT - the result fails to be copied to userspace
> > + *
> > + * Retrieve information about the PFRU Runtime Update capability.
>
> "information on"
>
Ok.
> > + * The information is a struct pfru_update_cap_info.
> > + */
> > +#define PFRU_IOC_QUERY_CAP _IOR(PFRU_MAGIC_FOR_IOCTL, 0x05, struct pfru_update_cap_info)
> > +
> > +/**
> > + * struct pfru_payload_hdr - Capsule file payload header.
> > + *
> > + * @sig: Signature of this capsule file.
> > + * @hdr_version: Revision of this header structure.
> > + * @hdr_size: Size of this header, including the OemHeader bytes.
> > + * @hw_ver: The supported firmware version.
> > + * @rt_ver: Version of the code injection image.
> > + * @platform_id: A platform specific GUID to specify the platform what
> > + * this capsule image support.
> > + */
> > +struct pfru_payload_hdr {
> > + __u32 sig;
> > + __u32 hdr_version;
> > + __u32 hdr_size;
> > + __u32 hw_ver;
> > + __u32 rt_ver;
> > + __u8 platform_id[16];
> > +};
> > +
> > +enum pfru_dsm_status {
> > + DSM_SUCCEED = 0,
> > + DSM_FUNC_NOT_SUPPORT = 1,
> > + DSM_INVAL_INPUT = 2,
> > + DSM_HARDWARE_ERR = 3,
> > + DSM_RETRY_SUGGESTED = 4,
> > + DSM_UNKNOWN = 5,
> > + DSM_FUNC_SPEC_ERR = 6,
> > +};
> > +
> > +/**
> > + * struct pfru_update_cap_info - Runtime update capability information.
> > + *
> > + * @status: Indicator of whether this query succeed.
> > + * @update_cap: Bitmap to indicate whether the feature is supported.
> > + * @code_type: A buffer containing an image type GUID.
> > + * @fw_version: Platform firmware version.
> > + * @code_rt_version: Code injection runtime version for anti-rollback.
> > + * @drv_type: A buffer containing an image type GUID.
> > + * @drv_rt_version: The version of the driver update runtime code.
> > + * @drv_svn: The secure version number(SVN) of the driver update runtime code.
> > + * @platform_id: A buffer containing a platform ID GUID.
> > + * @oem_id: A buffer containing an OEM ID GUID.
> > + * @oem_info_len: Length of the buffer containing the vendor specific information.
> > + */
> > +struct pfru_update_cap_info {
> > + __u32 status;
> > + __u32 update_cap;
> > +
> > + __u8 code_type[16];
> > + __u32 fw_version;
> > + __u32 code_rt_version;
> > +
> > + __u8 drv_type[16];
> > + __u32 drv_rt_version;
> > + __u32 drv_svn;
> > +
> > + __u8 platform_id[16];
> > + __u8 oem_id[16];
> > +
> > + __u32 oem_info_len;
> > +};
> > +
> > +/**
> > + * struct pfru_com_buf_info - Communication buffer information.
> > + *
> > + * @status: Indicator of whether this query succeed.
> > + * @ext_status: Implementation specific query result.
> > + * @addr_lo: Low 32bit physical address of the communication buffer to hold
> > + * a runtime update package.
> > + * @addr_hi: High 32bit physical address of the communication buffer to hold
> > + * a runtime update package.
> > + * @buf_size: Maximum size in bytes of the communication buffer.
> > + */
> > +struct pfru_com_buf_info {
> > + __u32 status;
> > + __u32 ext_status;
> > + __u64 addr_lo;
> > + __u64 addr_hi;
> > + __u32 buf_size;
> > +};
> > +
> > +/**
> > + * struct pfru_updated_result - Platform firmware runtime update result information.
> > + * @status: Indicator of whether this update succeed.
> > + * @ext_status: Implementation specific update result.
> > + * @low_auth_time: Low 32bit value of image authentication time in nanosecond.
> > + * @high_auth_time: High 32bit value of image authentication time in nanosecond.
> > + * @low_exec_time: Low 32bit value of image execution time in nanosecond.
> > + * @high_exec_time: High 32bit value of image execution time in nanosecond.
> > + */
> > +struct pfru_updated_result {
> > + __u32 status;
> > + __u32 ext_status;
> > + __u64 low_auth_time;
> > + __u64 high_auth_time;
> > + __u64 low_exec_time;
> > + __u64 high_exec_time;
> > +};
> > +
> > +#endif /* __PFRU_H__ */
> > --
>
> Overall, I think that the design is fine now, it just needs to be made nicer.
>
> [Please wait for my comments on the next patch before sending a new version.]
Ok, will wait for it. Thanks!
Best,
Chenyu
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v11 3/4] drivers/acpi: Introduce Platform Firmware Runtime Update Telemetry
2021-11-21 15:16 ` [PATCH v11 3/4] drivers/acpi: Introduce Platform Firmware Runtime Update Telemetry Chen Yu
@ 2021-12-07 19:44 ` Rafael J. Wysocki
2021-12-08 7:03 ` Chen Yu
0 siblings, 1 reply; 17+ messages in thread
From: Rafael J. Wysocki @ 2021-12-07 19:44 UTC (permalink / raw)
To: Chen Yu
Cc: ACPI Devel Maling List, Greg Kroah-Hartman, Rafael J. Wysocki,
Ard Biesheuvel, Len Brown, Ashok Raj, Andy Shevchenko,
Mike Rapoport, Linux Kernel Mailing List
On Sun, Nov 21, 2021 at 4:17 PM Chen Yu <yu.c.chen@intel.com> wrote:
>
First of all, the subject of the patch should be "Introduce Platform
Firmware Runtime Telemetry", because there is no "update" part in it.
> 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 above paragraph is redundant IMO. It provides a little useful
information IMV.
> 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.
In fact, this driver allows user space to fetch telemetry data from
the firmware with the help of the Platform Firmware Runtime Telemetry
interface.
> PFRU and PFRU Telemetry both invoke _DSM to trigger the low level actions.
"PFRU and PFRT " or "PFRU and PFR Telemetry ", please.
> 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.
I would write the above in the following way:
"Both PFRU and PFRT are based on ACPI _DSM interfaces located under
special device objects in the ACPI Namespace, but these interfaces are
different from each other, so it is better to provide a separate
driver from each of them. However, they share some common definitions
and naming conventions."
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---
[cut the history]
> ---
> drivers/acpi/pfru/Makefile | 2 +-
> drivers/acpi/pfru/pfru_telemetry.c | 457 +++++++++++++++++++++++++++++
> include/uapi/linux/pfru.h | 88 ++++++
> 3 files changed, 546 insertions(+), 1 deletion(-)
> create mode 100644 drivers/acpi/pfru/pfru_telemetry.c
>
> diff --git a/drivers/acpi/pfru/Makefile b/drivers/acpi/pfru/Makefile
> index 098cbe80cf3d..9ba11efe10b3 100644
> --- a/drivers/acpi/pfru/Makefile
> +++ b/drivers/acpi/pfru/Makefile
Actually, I'm not sure if a separate directory for this is needed.
The files (pfrut.h, pfr_update.c and pfr_telemetry.c) can be located
directly in drivers/acpi/ as far as I'm concerned.
> @@ -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
Please also update Kconfig with the information regarding PFRT (which
is not going to be added by the previous patch I suppose).
> diff --git a/drivers/acpi/pfru/pfru_telemetry.c b/drivers/acpi/pfru/pfru_telemetry.c
> new file mode 100644
> index 000000000000..5140a4591c9e
> --- /dev/null
> +++ b/drivers/acpi/pfru/pfru_telemetry.c
> @@ -0,0 +1,457 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ACPI Platform Firmware Runtime Update Telemetry Driver
"ACPI Platform Runtime Telemetry driver"
> + *
> + * Copyright (C) 2021 Intel Corporation
> + * Author: Chen Yu <yu.c.chen@intel.com>
Please describe the driver here at least briefly (what it is for etc.).
> + */
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/file.h>
> +#include <linux/fs.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/mm.h>
> +#include <linux/platform_device.h>
> +#include <linux/string.h>
> +#include <linux/uaccess.h>
> +#include <linux/uio.h>
> +#include <linux/uuid.h>
> +
> +#include <uapi/linux/pfru.h>
> +
> +#define PFRU_LOG_EXEC_IDX 0
> +#define PFRU_LOG_HISTORY_IDX 1
> +
> +#define PFRU_LOG_ERR 0
> +#define PFRU_LOG_WARN 1
> +#define PFRU_LOG_INFO 2
> +#define PFRU_LOG_VERB 4
> +
> +#define PFRU_FUNC_SET_LEV 1
> +#define PFRU_FUNC_GET_LEV 2
> +#define PFRU_FUNC_GET_DATA 3
> +
> +#define PFRU_REVID_1 1
> +#define PFRU_REVID_2 2
> +#define PFRU_DEFAULT_REV_ID PFRU_REVID_1
I would use the PFRT_ prefix in the above symbols.
> +
> +enum log_index {
> + LOG_STATUS_IDX = 0,
> + LOG_EXT_STATUS_IDX = 1,
> + LOG_MAX_SZ_IDX = 2,
> + LOG_CHUNK1_LO_IDX = 3,
> + LOG_CHUNK1_HI_IDX = 4,
> + LOG_CHUNK1_SZ_IDX = 5,
> + LOG_CHUNK2_LO_IDX = 6,
> + LOG_CHUNK2_HI_IDX = 7,
> + LOG_CHUNK2_SZ_IDX = 8,
> + LOG_ROLLOVER_CNT_IDX = 9,
> + LOG_RESET_CNT_IDX = 10,
> + LOG_NR_IDX
> +};
> +
> +struct pfru_log_device {
'pfrt_log_device"
> + int index;
> + struct pfru_log_info info;
> + struct device *parent_dev;
> + struct miscdevice miscdev;
> +};
> +
> +static const guid_t pfru_log_guid =
> + GUID_INIT(0x75191659, 0x8178, 0x4D9D, 0xB8, 0x8F, 0xAC, 0x5E,
> + 0x5E, 0x93, 0xE8, 0xBF);
The UUID needs to be documented.
> +
> +static DEFINE_IDA(pfru_log_ida);
pfrt_log_ida
Generally, I would use the pfrt_ prefix in the names below instead of pfru_.
> +
> +static inline struct pfru_log_device *to_pfru_log_dev(struct file *file)
> +{
> + return container_of(file->private_data, struct pfru_log_device, miscdev);
> +}
> +
> +static int get_pfru_log_data_info(struct pfru_log_data_info *data_info,
> + struct pfru_log_device *pfru_log_dev)
> +{
> + acpi_handle handle = ACPI_HANDLE(pfru_log_dev->parent_dev);
> + union acpi_object *out_obj, in_obj, in_buf;
> + int ret = -EINVAL;
> +
> + memset(&in_obj, 0, sizeof(in_obj));
> + memset(&in_buf, 0, sizeof(in_buf));
> + in_obj.type = ACPI_TYPE_PACKAGE;
> + in_obj.package.count = 1;
> + in_obj.package.elements = &in_buf;
> + in_buf.type = ACPI_TYPE_INTEGER;
> + in_buf.integer.value = pfru_log_dev->info.log_type;
> +
> + out_obj = acpi_evaluate_dsm_typed(handle, &pfru_log_guid,
> + pfru_log_dev->info.log_revid, PFRU_FUNC_GET_DATA,
> + &in_obj, ACPI_TYPE_PACKAGE);
> + if (!out_obj)
> + return ret;
> +
> + if (out_obj->package.count < LOG_NR_IDX)
> + goto free_acpi_buffer;
> +
> + if (out_obj->package.elements[LOG_STATUS_IDX].type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + data_info->status = out_obj->package.elements[LOG_STATUS_IDX].integer.value;
> +
> + if (out_obj->package.elements[LOG_EXT_STATUS_IDX].type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + data_info->ext_status =
> + out_obj->package.elements[LOG_EXT_STATUS_IDX].integer.value;
> +
> + if (out_obj->package.elements[LOG_MAX_SZ_IDX].type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + data_info->max_data_size =
> + out_obj->package.elements[LOG_MAX_SZ_IDX].integer.value;
> +
> + if (out_obj->package.elements[LOG_CHUNK1_LO_IDX].type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + data_info->chunk1_addr_lo =
> + out_obj->package.elements[LOG_CHUNK1_LO_IDX].integer.value;
> +
> + if (out_obj->package.elements[LOG_CHUNK1_HI_IDX].type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + data_info->chunk1_addr_hi =
> + out_obj->package.elements[LOG_CHUNK1_HI_IDX].integer.value;
> +
> + if (out_obj->package.elements[LOG_CHUNK1_SZ_IDX].type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + data_info->chunk1_size =
> + out_obj->package.elements[LOG_CHUNK1_SZ_IDX].integer.value;
> +
> + if (out_obj->package.elements[LOG_CHUNK2_LO_IDX].type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + data_info->chunk2_addr_lo =
> + out_obj->package.elements[LOG_CHUNK2_LO_IDX].integer.value;
> +
> + if (out_obj->package.elements[LOG_CHUNK2_HI_IDX].type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + data_info->chunk2_addr_hi =
> + out_obj->package.elements[LOG_CHUNK2_HI_IDX].integer.value;
> +
> + if (out_obj->package.elements[LOG_CHUNK2_SZ_IDX].type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + data_info->chunk2_size =
> + out_obj->package.elements[LOG_CHUNK2_SZ_IDX].integer.value;
> +
> + if (out_obj->package.elements[LOG_ROLLOVER_CNT_IDX].type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + data_info->rollover_cnt =
> + out_obj->package.elements[LOG_ROLLOVER_CNT_IDX].integer.value;
> +
> + if (out_obj->package.elements[LOG_RESET_CNT_IDX].type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + data_info->reset_cnt =
> + out_obj->package.elements[LOG_RESET_CNT_IDX].integer.value;
Like in some functions in the other patch, it would be good to reduce
the number of goto statements and possibly redundant updates by
grouping the checks.
> +
> + ret = 0;
> +
> +free_acpi_buffer:
> + kfree(out_obj);
> +
> + return ret;
> +}
> +
> +static int set_pfru_log_level(int level, struct pfru_log_device *pfru_log_dev)
> +{
> + acpi_handle handle = ACPI_HANDLE(pfru_log_dev->parent_dev);
> + union acpi_object *out_obj, *obj, in_obj, in_buf;
> + enum pfru_dsm_status status;
> + int ret = -EINVAL;
> +
> + memset(&in_obj, 0, sizeof(in_obj));
> + memset(&in_buf, 0, sizeof(in_buf));
> + in_obj.type = ACPI_TYPE_PACKAGE;
> + in_obj.package.count = 1;
> + in_obj.package.elements = &in_buf;
> + in_buf.type = ACPI_TYPE_INTEGER;
> + in_buf.integer.value = level;
> +
> + out_obj = acpi_evaluate_dsm_typed(handle, &pfru_log_guid,
> + pfru_log_dev->info.log_revid, PFRU_FUNC_SET_LEV,
> + &in_obj, ACPI_TYPE_PACKAGE);
> + if (!out_obj)
> + return -EINVAL;
> +
> + obj = &out_obj->package.elements[0];
> + status = obj->integer.value;
> + if (status)
> + goto free_acpi_buffer;
> +
> + obj = &out_obj->package.elements[1];
> + status = obj->integer.value;
> + if (status)
> + goto free_acpi_buffer;
> +
> + ret = 0;
> +
> +free_acpi_buffer:
> + kfree(out_obj);
> +
> + return ret;
> +}
> +
> +static int get_pfru_log_level(struct pfru_log_device *pfru_log_dev)
> +{
> + acpi_handle handle = ACPI_HANDLE(pfru_log_dev->parent_dev);
> + union acpi_object *out_obj, *obj;
> + enum pfru_dsm_status status;
> + int ret = -EINVAL;
> +
> + out_obj = acpi_evaluate_dsm_typed(handle, &pfru_log_guid,
> + pfru_log_dev->info.log_revid, PFRU_FUNC_GET_LEV,
> + NULL, ACPI_TYPE_PACKAGE);
> + if (!out_obj)
> + return -EINVAL;
> +
> + obj = &out_obj->package.elements[0];
> + if (obj->type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + status = obj->integer.value;
> + if (status)
> + goto free_acpi_buffer;
> +
> + obj = &out_obj->package.elements[1];
> + if (obj->type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + status = obj->integer.value;
> + if (status)
> + goto free_acpi_buffer;
> +
> + obj = &out_obj->package.elements[2];
> + if (obj->type != ACPI_TYPE_INTEGER)
> + goto free_acpi_buffer;
> +
> + ret = obj->integer.value;
> +
> +free_acpi_buffer:
> + kfree(out_obj);
> +
> + return ret;
> +}
> +
> +static int valid_log_level(u32 level)
> +{
> + return level == PFRU_LOG_ERR || level == PFRU_LOG_WARN ||
> + level == PFRU_LOG_INFO || level == PFRU_LOG_VERB;
> +}
> +
> +static int valid_log_type(u32 type)
> +{
> + return type == PFRU_LOG_EXEC_IDX || type == PFRU_LOG_HISTORY_IDX;
> +}
> +
> +static inline int valid_log_revid(u32 id)
> +{
> + return id == PFRU_REVID_1 || id == PFRU_REVID_2;
> +}
> +
> +static long pfru_log_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> + struct pfru_log_device *pfru_log_dev = to_pfru_log_dev(file);
> + struct pfru_log_data_info data_info;
> + struct pfru_log_info info;
> + void __user *p;
> + int ret = 0;
> +
> + p = (void __user *)arg;
> +
> + switch (cmd) {
> + case PFRU_LOG_IOC_SET_INFO:
> + if (copy_from_user(&info, p, sizeof(info)))
> + return -EFAULT;
> +
> + if (valid_log_revid(info.log_revid))
> + pfru_log_dev->info.log_revid = info.log_revid;
> +
> + if (valid_log_level(info.log_level)) {
> + ret = set_pfru_log_level(info.log_level, pfru_log_dev);
> + if (ret < 0)
> + return ret;
> +
> + pfru_log_dev->info.log_level = info.log_level;
> + }
> +
> + if (valid_log_type(info.log_type))
> + pfru_log_dev->info.log_type = info.log_type;
> +
> + return 0;
Please use blank lines to separate the cases visually from each other.
> + case PFRU_LOG_IOC_GET_INFO:
> + info.log_level = get_pfru_log_level(pfru_log_dev);
> + if (ret < 0)
> + return ret;
> +
> + info.log_type = pfru_log_dev->info.log_type;
> + info.log_revid = pfru_log_dev->info.log_revid;
> + if (copy_to_user(p, &info, sizeof(info)))
> + return -EFAULT;
> +
> + return 0;
> + case PFRU_LOG_IOC_GET_DATA_INFO:
> + ret = get_pfru_log_data_info(&data_info, pfru_log_dev);
> + if (ret)
> + return ret;
> +
> + if (copy_to_user(p, &data_info, sizeof(struct pfru_log_data_info)))
> + return -EFAULT;
> +
> + return 0;
> + default:
> + return -ENOTTY;
> + }
> +}
> +
> +static int
> +pfru_log_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> + struct pfru_log_device *pfru_log_dev;
> + struct pfru_log_data_info info;
> + unsigned long psize, vsize;
> + phys_addr_t base_addr;
> + int ret;
> +
> + if (vma->vm_flags & VM_WRITE)
> + return -EROFS;
> +
> + /* changing from read to write with mprotect is not allowed */
> + vma->vm_flags &= ~VM_MAYWRITE;
> +
> + pfru_log_dev = to_pfru_log_dev(file);
> +
> + ret = get_pfru_log_data_info(&info, pfru_log_dev);
> + if (ret)
> + return ret;
> +
> + base_addr = (phys_addr_t)((info.chunk2_addr_hi << 32) | info.chunk2_addr_lo);
> + /* pfru update has not been launched yet */
> + if (!base_addr)
> + return -ENODEV;
> +
> + psize = info.max_data_size;
> + /* base address and total buffer size must be page aligned */
> + if (!PAGE_ALIGNED(base_addr) || !PAGE_ALIGNED(psize))
> + return -ENODEV;
> +
> + vsize = vma->vm_end - vma->vm_start;
> + if (vsize > psize)
> + return -EINVAL;
> +
> + vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> + if (io_remap_pfn_range(vma, vma->vm_start, PFN_DOWN(base_addr),
> + vsize, vma->vm_page_prot))
> + return -EAGAIN;
> +
> + return 0;
> +}
> +
> +static const struct file_operations acpi_pfru_log_fops = {
> + .owner = THIS_MODULE,
> + .mmap = pfru_log_mmap,
> + .unlocked_ioctl = pfru_log_ioctl,
> + .llseek = noop_llseek,
> +};
> +
> +static int acpi_pfru_log_remove(struct platform_device *pdev)
> +{
> + struct pfru_log_device *pfru_log_dev = platform_get_drvdata(pdev);
> +
> + misc_deregister(&pfru_log_dev->miscdev);
> +
> + return 0;
> +}
> +
> +static void pfru_log_put_idx(void *data)
> +{
> + struct pfru_log_device *pfru_log_dev = data;
> +
> + ida_free(&pfru_log_ida, pfru_log_dev->index);
> +}
> +
> +static int acpi_pfru_log_probe(struct platform_device *pdev)
> +{
> + acpi_handle handle = ACPI_HANDLE(&pdev->dev);
> + struct pfru_log_device *pfru_log_dev;
> + int ret;
> +
> + if (!acpi_has_method(handle, "_DSM")) {
> + dev_dbg(&pdev->dev, "Missing _DSM\n");
> + return -ENODEV;
> + }
> +
> + pfru_log_dev = devm_kzalloc(&pdev->dev, sizeof(*pfru_log_dev), GFP_KERNEL);
> + if (!pfru_log_dev)
> + return -ENOMEM;
> +
> + ret = ida_alloc(&pfru_log_ida, GFP_KERNEL);
> + if (ret < 0)
> + return ret;
> +
> + pfru_log_dev->index = ret;
> + ret = devm_add_action_or_reset(&pdev->dev, pfru_log_put_idx, pfru_log_dev);
> + if (ret)
> + return ret;
> +
> + pfru_log_dev->info.log_revid = PFRU_DEFAULT_REV_ID;
> + pfru_log_dev->parent_dev = &pdev->dev;
> +
> + pfru_log_dev->miscdev.minor = MISC_DYNAMIC_MINOR;
> + pfru_log_dev->miscdev.name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> + "pfru_telemetry%d",
I would call it "pfr_telemetry" (without the "u" in the prefix).
> + pfru_log_dev->index);
> + if (!pfru_log_dev->miscdev.name)
> + return -ENOMEM;
> +
> + pfru_log_dev->miscdev.nodename = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> + "acpi_pfru_telemetry%d",
And this one (analogously) "acpi_pfr_telemetry".
> + pfru_log_dev->index);
> + if (!pfru_log_dev->miscdev.nodename)
> + return -ENOMEM;
> +
> + pfru_log_dev->miscdev.fops = &acpi_pfru_log_fops;
> + pfru_log_dev->miscdev.parent = &pdev->dev;
> +
> + ret = misc_register(&pfru_log_dev->miscdev);
> + if (ret)
> + return ret;
> +
> + platform_set_drvdata(pdev, pfru_log_dev);
> +
> + return 0;
> +}
> +
> +static const struct acpi_device_id acpi_pfru_log_ids[] = {
> + {"INTC1081"},
> + {}
> +};
> +MODULE_DEVICE_TABLE(acpi, acpi_pfru_log_ids);
> +
> +static struct platform_driver acpi_pfru_log_driver = {
> + .driver = {
> + .name = "pfru_telemetry",
> + .acpi_match_table = acpi_pfru_log_ids,
> + },
> + .probe = acpi_pfru_log_probe,
> + .remove = acpi_pfru_log_remove,
> +};
> +module_platform_driver(acpi_pfru_log_driver);
> +
> +MODULE_DESCRIPTION("Platform Firmware Runtime Update Telemetry driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/uapi/linux/pfru.h b/include/uapi/linux/pfru.h
> index fed50cb01309..2f17714e05b7 100644
> --- a/include/uapi/linux/pfru.h
> +++ b/include/uapi/linux/pfru.h
> @@ -171,4 +171,92 @@ struct pfru_updated_result {
> __u64 high_exec_time;
> };
>
> +/**
> + * struct pfru_log_data_info - Log Data from telemetry service.
> + * @status: Indicator of whether this update succeed.
> + * @ext_status: Implementation specific update result.
> + * @chunk1_addr_lo: Low 32bit physical address of the telemetry data chunk1
> + * starting address.
> + * @chunk1_addr_hi: High 32bit physical address of the telemetry data chunk1
> + * starting address.
> + * @chunk2_addr_lo: Low 32bit physical address of the telemetry data chunk2
> + * starting address.
> + * @chunk2_addr_hi: High 32bit physical address of the telemetry data chunk2
> + * starting address.
> + * @max_data_size: Maximum supported size of data of all data chunks combined.
> + * @chunk1_size: Data size in bytes of the telemetry data chunk1 buffer.
> + * @chunk2_size: Data size in bytes of the telemetry data chunk2 buffer.
> + * @rollover_cnt: Number of times telemetry data buffer is overwritten
> + * since telemetry buffer reset.
> + * @reset_cnt: Number of times telemetry services resets that results in
> + * rollover count and data chunk buffers are reset.
> + */
> +struct pfru_log_data_info {
> + __u32 status;
> + __u32 ext_status;
> + __u64 chunk1_addr_lo;
> + __u64 chunk1_addr_hi;
> + __u64 chunk2_addr_lo;
> + __u64 chunk2_addr_hi;
> + __u32 max_data_size;
> + __u32 chunk1_size;
> + __u32 chunk2_size;
> + __u32 rollover_cnt;
> + __u32 reset_cnt;
> +};
> +
> +/**
> + * struct pfru_log_info - Telemetry log information.
> + * @log_level: The telemetry log level.
> + * @log_type: The telemetry log type(history and execution).
> + * @log_revid: The telemetry log revision id.
> + */
> +struct pfru_log_info {
> + __u32 log_level;
> + __u32 log_type;
> + __u32 log_revid;
> +};
> +
> +/**
> + * PFRU_LOG_IOC_SET_INFO - _IOW(PFRU_MAGIC_FOR_IOCTL, 0x06,
> + * struct pfru_log_info)
> + *
> + * Return:
> + * * 0 - success
> + * * -EFAULT - fail to get the setting parameter
> + * * -EINVAL - fail to set the log level
> + *
> + * Set the PFRU log level and log type. The input information is
> + * a struct pfru_log_info.
> + */
> +#define PFRU_LOG_IOC_SET_INFO _IOW(PFRU_MAGIC_FOR_IOCTL, 0x06, struct pfru_log_info)
> +
> +/**
> + * PFRU_LOG_IOC_GET_INFO - _IOR(PFRU_MAGIC_FOR_IOCTL, 0x07,
> + * struct pfru_log_info)
> + *
> + * Return:
> + * * 0 - success
> + * * -EINVAL - fail to get the log level
> + * * -EFAULT - fail to copy the result back to userspace
> + *
> + * Retrieve log level and log type of the PFRU telemetry. The information is
> + * a struct pfru_log_info.
> + */
> +#define PFRU_LOG_IOC_GET_INFO _IOR(PFRU_MAGIC_FOR_IOCTL, 0x07, struct pfru_log_info)
> +
> +/**
> + * PFRU_LOG_IOC_GET_DATA_INFO - _IOR(PFRU_MAGIC_FOR_IOCTL, 0x08,
> + * struct pfru_log_data_info)
> + *
> + * Return:
> + * * 0 - success
> + * * -EINVAL - fail to get the log buffer information
> + * * -EFAULT - fail to copy the log buffer information to userspace
> + *
> + * Retrieve data information about the PFRU telemetry. The information
> + * is a struct pfru_log_data_info.
> + */
> +#define PFRU_LOG_IOC_GET_DATA_INFO _IOR(PFRU_MAGIC_FOR_IOCTL, 0x08, struct pfru_log_data_info)
> +
> #endif /* __PFRU_H__ */
And of course it needs to be rebased to take the changes in the
previous patch into account.
Overall, it's almost there IMO.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v11 4/4] tools: Introduce power/acpi/tools/pfru
2021-11-21 15:17 ` [PATCH v11 4/4] tools: Introduce power/acpi/tools/pfru Chen Yu
2021-11-22 11:26 ` Andy Shevchenko
@ 2021-12-07 19:52 ` Rafael J. Wysocki
2021-12-08 7:20 ` Chen Yu
1 sibling, 1 reply; 17+ messages in thread
From: Rafael J. Wysocki @ 2021-12-07 19:52 UTC (permalink / raw)
To: Chen Yu
Cc: ACPI Devel Maling List, Greg Kroah-Hartman, Rafael J. Wysocki,
Ard Biesheuvel, Len Brown, Ashok Raj, Andy Shevchenko,
Mike Rapoport, Linux Kernel Mailing List, Robert Moore
On Sun, Nov 21, 2021 at 4:18 PM Chen Yu <yu.c.chen@intel.com> wrote:
>
> 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.
>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---
> v11:No change since v10.
> v10:No change since v9.
> v9: Add this tool into tools/power/acpi build infrastructure.
> (Andy Shevchenko)
> 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/.gitignore | 1 +
> tools/power/acpi/Makefile | 16 +-
> tools/power/acpi/Makefile.rules | 2 +-
> tools/power/acpi/man/pfru.8 | 137 +++++++++
I would call it pfrut, because it is for both the update and telemetry
retrieval.
> tools/power/acpi/tools/pfru/Makefile | 23 ++
> tools/power/acpi/tools/pfru/pfru.c | 417 +++++++++++++++++++++++++++
> 6 files changed, 587 insertions(+), 9 deletions(-)
> create mode 100644 tools/power/acpi/man/pfru.8
> create mode 100644 tools/power/acpi/tools/pfru/Makefile
> create mode 100644 tools/power/acpi/tools/pfru/pfru.c
>
> diff --git a/tools/power/acpi/.gitignore b/tools/power/acpi/.gitignore
> index 0b319fc8bb17..a6f196912999 100644
> --- a/tools/power/acpi/.gitignore
> +++ b/tools/power/acpi/.gitignore
> @@ -2,4 +2,5 @@
> /acpidbg
> /acpidump
> /ec
> +/pfru
> /include/
> diff --git a/tools/power/acpi/Makefile b/tools/power/acpi/Makefile
> index a249c50ebf55..2fcf09d3add3 100644
> --- a/tools/power/acpi/Makefile
> +++ b/tools/power/acpi/Makefile
> @@ -9,18 +9,18 @@ include ../../scripts/Makefile.include
>
> .NOTPARALLEL:
>
> -all: acpidbg acpidump ec
> -clean: acpidbg_clean acpidump_clean ec_clean
> -install: acpidbg_install acpidump_install ec_install
> -uninstall: acpidbg_uninstall acpidump_uninstall ec_uninstall
> +all: acpidbg acpidump ec pfru
> +clean: acpidbg_clean acpidump_clean ec_clean pfru_clean
> +install: acpidbg_install acpidump_install ec_install pfru_install
> +uninstall: acpidbg_uninstall acpidump_uninstall ec_uninstall pfru_uninstall
>
> -acpidbg acpidump ec: FORCE
> +acpidbg acpidump ec pfru: FORCE
> $(call descend,tools/$@,all)
> -acpidbg_clean acpidump_clean ec_clean:
> +acpidbg_clean acpidump_clean ec_clean pfru_clean:
> $(call descend,tools/$(@:_clean=),clean)
> -acpidbg_install acpidump_install ec_install:
> +acpidbg_install acpidump_install ec_install pfru_install:
> $(call descend,tools/$(@:_install=),install)
> -acpidbg_uninstall acpidump_uninstall ec_uninstall:
> +acpidbg_uninstall acpidump_uninstall ec_uninstall pfru_uninstall:
> $(call descend,tools/$(@:_uninstall=),uninstall)
>
> .PHONY: FORCE
> diff --git a/tools/power/acpi/Makefile.rules b/tools/power/acpi/Makefile.rules
> index 2a6c170b57cd..68aa7e92d554 100644
> --- a/tools/power/acpi/Makefile.rules
> +++ b/tools/power/acpi/Makefile.rules
> @@ -9,7 +9,7 @@ objdir := $(OUTPUT)tools/$(TOOL)/
> toolobjs := $(addprefix $(objdir),$(TOOL_OBJS))
> $(OUTPUT)$(TOOL): $(toolobjs) FORCE
> $(ECHO) " LD " $(subst $(OUTPUT),,$@)
> - $(QUIET) $(LD) $(CFLAGS) $(LDFLAGS) $(toolobjs) -L$(OUTPUT) -o $@
> + $(QUIET) $(LD) $(CFLAGS) $(toolobjs) $(LDFLAGS) -L$(OUTPUT) -o $@
> $(ECHO) " STRIP " $(subst $(OUTPUT),,$@)
> $(QUIET) $(STRIPCMD) $@
>
> diff --git a/tools/power/acpi/man/pfru.8 b/tools/power/acpi/man/pfru.8
> new file mode 100644
> index 000000000000..d9cda7beaa3c
> --- /dev/null
> +++ b/tools/power/acpi/man/pfru.8
> @@ -0,0 +1,137 @@
> +.TH "PFRU" "8" "October 2021" "pfru 1.0" ""
> +.hy
> +.SH Name
> +.PP
> +pfru \- Platform Firmware Runtime Update tool
> +.SH SYNOPSIS
> +.PP
> +\f[B]pfru\f[R] [\f[I]Options\f[R]]
> +.SH DESCRIPTION
> +.PP
> +The PFRU(Platform Firmware Runtime Update) kernel interface is designed
> +to
> +.PD 0
> +.P
> +.PD
> +interact with the platform firmware interface defined in the
> +.PD 0
> +.P
> +.PD
> +Management Mode Firmware Runtime
> +Update (https://uefi.org/sites/default/files/resources/Intel_MM_OS_Interface_Spec_Rev100.pdf)
> +.PD 0
> +.P
> +.PD
> +\f[B]pfru\f[R] is the tool to interact with the kernel interface.
> +.PD 0
> +.P
> +.PD
> +.SH OPTIONS
> +.TP
> +.B \f[B]\-h\f[R], \f[B]\-\-help\f[R]
> +Display helper information.
> +.TP
> +.B \f[B]\-l\f[R], \f[B]\-\-load\f[R]
> +Load the capsule file into the system.
> +To be more specific, the capsule file will be copied to the
> +communication buffer.
> +.TP
> +.B \f[B]\-s\f[R], \f[B]\-\-stage\f[R]
> +Stage the capsule image from communication buffer into Management Mode
> +and perform authentication.
> +.TP
> +.B \f[B]\-a\f[R], \f[B]\-\-activate\f[R]
> +Activate a previous staged capsule image.
> +.TP
> +.B \f[B]\-u\f[R], \f[B]\-\-update\f[R]
> +Perform both stage and activation actions.
> +.TP
> +.B \f[B]\-q\f[R], \f[B]\-\-query\f[R]
> +Query the update capability.
> +.TP
> +.B \f[B]\-d\f[R], \f[B]\-\-setrev\f[R]
> +Set the revision ID of code injection/driver update.
> +.TP
> +.B \f[B]\-D\f[R], \f[B]\-\-setrevlog\f[R]
> +Set the revision ID of telemetry.
> +.TP
> +.B \f[B]\-G\f[R], \f[B]\-\-getloginfo\f[R]
> +Get telemetry log information and print it out.
> +.TP
> +.B \f[B]\-T\f[R], \f[B]\-\-type\f[R]
> +Set the telemetry log data type.
> +.TP
> +.B \f[B]\-L\f[R], \f[B]\-\-level\f[R]
> +Set the telemetry log level.
> +.TP
> +.B \f[B]\-R\f[R], \f[B]\-\-read\f[R]
> +Read all the telemetry data and print it out.
> +.SH EXAMPLES
> +.PP
> +\f[B]pfru \-G\f[R]
> +.PP
> +log_level:4
> +.PD 0
> +.P
> +.PD
> +log_type:0
> +.PD 0
> +.P
> +.PD
> +log_revid:2
> +.PD 0
> +.P
> +.PD
> +max_data_size:65536
> +.PD 0
> +.P
> +.PD
> +chunk1_size:0
> +.PD 0
> +.P
> +.PD
> +chunk2_size:1401
> +.PD 0
> +.P
> +.PD
> +rollover_cnt:0
> +.PD 0
> +.P
> +.PD
> +reset_cnt:4
> +.PP
> +\f[B]pfru \-q\f[R]
> +.PP
> +code injection image type:794bf8b2\-6e7b\-454e\-885f\-3fb9bb185402
> +.PD 0
> +.P
> +.PD
> +fw_version:0
> +.PD 0
> +.P
> +.PD
> +code_rt_version:1
> +.PD 0
> +.P
> +.PD
> +driver update image type:0e5f0b14\-f849\-7945\-ad81\-bc7b6d2bb245
> +.PD 0
> +.P
> +.PD
> +drv_rt_version:0
> +.PD 0
> +.P
> +.PD
> +drv_svn:0
> +.PD 0
> +.P
> +.PD
> +platform id:39214663\-b1a8\-4eaa\-9024\-f2bb53ea4723
> +.PD 0
> +.P
> +.PD
> +oem id:a36db54f\-ea2a\-e14e\-b7c4\-b5780e51ba3d
> +.PP
> +\f[B]pfru \-l yours.cap \-u \-T 1 \-L 4\f[R]
> +.SH AUTHORS
> +Chen Yu.
> diff --git a/tools/power/acpi/tools/pfru/Makefile b/tools/power/acpi/tools/pfru/Makefile
> new file mode 100644
> index 000000000000..09002a81e10c
> --- /dev/null
> +++ b/tools/power/acpi/tools/pfru/Makefile
> @@ -0,0 +1,23 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +
> +include ../../Makefile.config
> +
> +TOOL = pfru
> +EXTRA_INSTALL = install-man
> +EXTRA_UNINSTALL = uninstall-man
> +
> +CFLAGS += -Wall -O2
> +CFLAGS += -DPFRU_HEADER='"../../../../../include/uapi/linux/pfru.h"'
> +LDFLAGS += -luuid
> +
> +TOOL_OBJS = \
> + pfru.o
> +
> +include ../../Makefile.rules
> +
> +install-man: $(srctree)/man/pfru.8
> + $(ECHO) " INST " pfru.8
> + $(QUIET) $(INSTALL_DATA) -D $< $(DESTDIR)$(mandir)/man8/pfru.8
> +uninstall-man:
> + $(ECHO) " UNINST " pfru.8
> + $(QUIET) rm -f $(DESTDIR)$(mandir)/man8/pfru.8
> diff --git a/tools/power/acpi/tools/pfru/pfru.c b/tools/power/acpi/tools/pfru/pfru.c
> new file mode 100644
> index 000000000000..eeec5043f825
> --- /dev/null
> +++ b/tools/power/acpi/tools/pfru/pfru.c
> @@ -0,0 +1,417 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Platform Firmware Runtime Update tool to do Management
> + * Mode code injection/driver update and telemetry retrieval.
I would be good to say a bit more here, like what interfaces are used
by this tool etc.
> + */
> +#define _GNU_SOURCE
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +#include <unistd.h>
> +#include <getopt.h>
> +#include <sys/ioctl.h>
> +#include <sys/mman.h>
> +#include <uuid/uuid.h>
> +#include PFRU_HEADER
> +
> +char *capsule_name;
> +int action, query_cap, log_type, log_level, log_read, log_getinfo,
> + revid, log_revid;
> +int set_log_level, set_log_type,
> + set_revid, set_log_revid;
> +
> +char *progname;
> +
> +#define LOG_ERR 0
> +#define LOG_WARN 1
> +#define LOG_INFO 2
> +#define LOG_VERB 4
> +#define LOG_EXEC_IDX 0
> +#define LOG_HISTORY_IDX 1
> +#define REVID_1 1
> +#define REVID_2 2
> +
> +static int valid_log_level(int level)
> +{
> + return level == LOG_ERR || level == LOG_WARN ||
> + level == LOG_INFO || level == LOG_VERB;
> +}
> +
> +static int valid_log_type(int type)
> +{
> + return type == LOG_EXEC_IDX || type == LOG_HISTORY_IDX;
> +}
> +
> +static inline int valid_log_revid(int id)
> +{
> + return id == REVID_1 || id == REVID_2;
> +}
> +
> +static void help(void)
> +{
> + fprintf(stderr,
> + "usage: %s [OPTIONS]\n"
> + " code injection:\n"
> + " -l, --load\n"
> + " -s, --stage\n"
> + " -a, --activate\n"
> + " -u, --update [stage and activate]\n"
> + " -q, --query\n"
> + " -d, --revid update\n"
> + " telemetry:\n"
> + " -G, --getloginfo\n"
> + " -T, --type(0:execution, 1:history)\n"
> + " -L, --level(0, 1, 2, 4)\n"
> + " -R, --read\n"
> + " -D, --revid log\n",
> + progname);
> +}
> +
> +char *option_string = "l:sauqd:GT:L:RD:h";
> +static struct option long_options[] = {
> + {"load", required_argument, 0, 'l'},
> + {"stage", no_argument, 0, 's'},
> + {"activate", no_argument, 0, 'a'},
> + {"update", no_argument, 0, 'u'},
> + {"query", no_argument, 0, 'q'},
> + {"getloginfo", no_argument, 0, 'G'},
> + {"type", required_argument, 0, 'T'},
> + {"level", required_argument, 0, 'L'},
> + {"read", no_argument, 0, 'R'},
> + {"setrev", required_argument, 0, 'd'},
> + {"setrevlog", required_argument, 0, 'D'},
> + {"help", no_argument, 0, 'h'},
> + {}
> +};
> +
> +static void parse_options(int argc, char **argv)
> +{
> + int option_index = 0;
> + char *pathname;
> + int opt;
> +
> + pathname = strdup(argv[0]);
> + progname = basename(pathname);
> +
> + while ((opt = getopt_long_only(argc, argv, option_string,
> + long_options, &option_index)) != -1) {
> + switch (opt) {
> + case 'l':
> + capsule_name = optarg;
> + break;
> + case 's':
> + action = 1;
> + break;
> + case 'a':
> + action = 2;
> + break;
> + case 'u':
> + action = 3;
> + break;
> + case 'q':
> + query_cap = 1;
> + break;
> + case 'G':
> + log_getinfo = 1;
> + break;
> + case 'T':
> + log_type = atoi(optarg);
> + set_log_type = 1;
> + break;
> + case 'L':
> + log_level = atoi(optarg);
> + set_log_level = 1;
> + break;
> + case 'R':
> + log_read = 1;
> + break;
> + case 'd':
> + revid = atoi(optarg);
> + set_revid = 1;
> + break;
> + case 'D':
> + log_revid = atoi(optarg);
> + set_log_revid = 1;
> + break;
> + case 'h':
> + help();
> + exit(0);
> + default:
> + break;
> + }
> + }
> +}
> +
> +void print_cap(struct pfru_update_cap_info *cap)
> +{
> + char *uuid;
> +
> + uuid = malloc(37);
> + if (!uuid) {
> + perror("Can not allocate uuid buffer\n");
> + exit(1);
> + }
> +
> + uuid_unparse(cap->code_type, uuid);
> + printf("code injection image type:%s\n", uuid);
> + printf("fw_version:%d\n", cap->fw_version);
> + printf("code_rt_version:%d\n", cap->code_rt_version);
> +
> + uuid_unparse(cap->drv_type, uuid);
> + printf("driver update image type:%s\n", uuid);
> + printf("drv_rt_version:%d\n", cap->drv_rt_version);
> + printf("drv_svn:%d\n", cap->drv_svn);
> +
> + uuid_unparse(cap->platform_id, uuid);
> + printf("platform id:%s\n", uuid);
> + uuid_unparse(cap->oem_id, uuid);
> + printf("oem id:%s\n", uuid);
> + printf("oem information length:%d\n", cap->oem_info_len);
> +
> + free(uuid);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + int fd_update, fd_update_log, fd_capsule;
> + struct pfru_log_data_info data_info;
> + struct pfru_log_info info;
> + struct pfru_update_cap_info cap;
> + void *addr_map_capsule;
> + struct stat st;
> + char *log_buf;
> + int ret = 0;
> +
> + if (getuid() != 0) {
> + printf("Please run the tool as root - Exiting.\n");
> + return 1;
> + }
> +
> + parse_options(argc, argv);
> +
> + fd_update = open("/dev/acpi_pfru0", O_RDWR);
> + if (fd_update < 0) {
> + printf("PFRU device not supported - Quit...\n");
> + return 1;
> + }
> +
> + fd_update_log = open("/dev/acpi_pfru_telemetry0", O_RDWR);
> + if (fd_update_log < 0) {
> + printf("PFRU telemetry device not supported - Quit...\n");
> + return 1;
> + }
> +
> + if (query_cap) {
> + ret = ioctl(fd_update, PFRU_IOC_QUERY_CAP, &cap);
> + if (ret)
> + perror("Query Update Capability info failed.");
> + else
> + print_cap(&cap);
> +
> + close(fd_update);
> + close(fd_update_log);
> +
> + return ret;
> + }
> +
> + if (log_getinfo) {
> + ret = ioctl(fd_update_log, PFRU_LOG_IOC_GET_DATA_INFO, &data_info);
> + if (ret) {
> + perror("Get telemetry data info failed.");
> + close(fd_update);
> + close(fd_update_log);
> +
> + return 1;
> + }
> +
> + ret = ioctl(fd_update_log, PFRU_LOG_IOC_GET_INFO, &info);
> + if (ret) {
> + perror("Get telemetry info failed.");
> + close(fd_update);
> + close(fd_update_log);
> +
> + return 1;
> + }
> +
> + printf("log_level:%d\n", info.log_level);
> + printf("log_type:%d\n", info.log_type);
> + printf("log_revid:%d\n", info.log_revid);
> + printf("max_data_size:%d\n", data_info.max_data_size);
> + printf("chunk1_size:%d\n", data_info.chunk1_size);
> + printf("chunk2_size:%d\n", data_info.chunk2_size);
> + printf("rollover_cnt:%d\n", data_info.rollover_cnt);
> + printf("reset_cnt:%d\n", data_info.reset_cnt);
> +
> + return 0;
> + }
> +
> + info.log_level = -1;
> + info.log_type = -1;
> + info.log_revid = -1;
> +
> + if (set_log_level) {
> + if (!valid_log_level(log_level)) {
> + printf("Invalid log level %d\n",
> + log_level);
> + } else {
> + info.log_level = log_level;
> + }
> + }
> +
> + if (set_log_type) {
> + if (!valid_log_type(log_type)) {
> + printf("Invalid log type %d\n",
> + log_type);
> + } else {
> + info.log_type = log_type;
> + }
> + }
> +
> + if (set_log_revid) {
> + if (!valid_log_revid(log_revid)) {
> + printf("Invalid log revid %d, unchanged.\n",
> + log_revid);
> + } else {
> + info.log_revid = log_revid;
> + }
> + }
> +
> + ret = ioctl(fd_update_log, PFRU_LOG_IOC_SET_INFO, &info);
> + if (ret) {
> + perror("Log information set failed.(log_level, log_type, log_revid)");
> + close(fd_update);
> + close(fd_update_log);
> +
> + return 1;
> + }
> +
> + if (set_revid) {
> + ret = ioctl(fd_update, PFRU_IOC_SET_REV, &revid);
> + if (ret) {
> + perror("pfru update revid set failed");
> + close(fd_update);
> + close(fd_update_log);
> +
> + return 1;
> + }
> +
> + printf("pfru update revid set to %d\n", revid);
> + }
> +
> + if (capsule_name) {
> + fd_capsule = open(capsule_name, O_RDONLY);
> + if (fd_capsule < 0) {
> + perror("Can not open capsule file...");
> + close(fd_update);
> + close(fd_update_log);
> +
> + return 1;
> + }
> +
> + if (fstat(fd_capsule, &st) < 0) {
> + perror("Can not fstat capsule file...");
> + close(fd_capsule);
> + close(fd_update);
> + close(fd_update_log);
> +
> + return 1;
> + }
> +
> + addr_map_capsule = mmap(NULL, st.st_size, PROT_READ, MAP_SHARED,
> + fd_capsule, 0);
> + if (addr_map_capsule == MAP_FAILED) {
> + perror("Failed to mmap capsule file.");
> + close(fd_capsule);
> + close(fd_update);
> + close(fd_update_log);
> +
> + return 1;
> + }
> +
> + ret = write(fd_update, (char *)addr_map_capsule, st.st_size);
> + printf("Load %d bytes of capsule file into the system\n",
> + ret);
> +
> + if (ret == -1) {
> + perror("Failed to load capsule file");
> + close(fd_capsule);
> + close(fd_update);
> + close(fd_update_log);
> +
> + return 1;
> + }
> +
> + munmap(addr_map_capsule, st.st_size);
> + close(fd_capsule);
> + printf("Load done.\n");
> + }
> +
> + if (action) {
> + if (action == 1) {
> + ret = ioctl(fd_update, PFRU_IOC_STAGE, NULL);
> + } else if (action == 2) {
> + ret = ioctl(fd_update, PFRU_IOC_ACTIVATE, NULL);
> + } else if (action == 3) {
> + ret = ioctl(fd_update, PFRU_IOC_STAGE_ACTIVATE, NULL);
> + } else {
> + close(fd_update);
> + close(fd_update_log);
> +
> + return 1;
> + }
> + printf("Update finished, return %d\n", ret);
> + }
> +
> + close(fd_update);
> +
> + if (log_read) {
> + void *p_mmap;
> + int max_data_sz;
> +
> + ret = ioctl(fd_update_log, PFRU_LOG_IOC_GET_DATA_INFO, &data_info);
> + if (ret) {
> + perror("Get telemetry data info failed.");
> + close(fd_update_log);
> +
> + return 1;
> + }
> +
> + max_data_sz = data_info.max_data_size;
> + if (!max_data_sz) {
> + printf("No telemetry data available.\n");
> + close(fd_update_log);
> +
> + return 1;
> + }
> +
> + log_buf = malloc(max_data_sz + 1);
> + if (!log_buf) {
> + perror("log_buf allocate failed.");
> + close(fd_update_log);
> +
> + return 1;
> + }
> +
> + p_mmap = mmap(NULL, max_data_sz, PROT_READ, MAP_SHARED, fd_update_log, 0);
> + if (p_mmap == MAP_FAILED) {
> + perror("mmap error.");
> + close(fd_update_log);
> +
> + return 1;
> + }
> +
> + memcpy(log_buf, p_mmap, max_data_sz);
> + log_buf[max_data_sz] = '\0';
> + printf("%s\n", log_buf);
> + free(log_buf);
> +
> + munmap(p_mmap, max_data_sz);
> + }
> +
> + close(fd_update_log);
> +
> + return 0;
> +}
> --
Please adjust the patch for the build fixes sent separately.
Otherwise it is fine with me.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v11 3/4] drivers/acpi: Introduce Platform Firmware Runtime Update Telemetry
2021-12-07 19:44 ` Rafael J. Wysocki
@ 2021-12-08 7:03 ` Chen Yu
0 siblings, 0 replies; 17+ messages in thread
From: Chen Yu @ 2021-12-08 7:03 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: ACPI Devel Maling List, Greg Kroah-Hartman, Ard Biesheuvel,
Len Brown, Ashok Raj, Andy Shevchenko, Mike Rapoport,
Linux Kernel Mailing List
On Tue, Dec 07, 2021 at 08:44:52PM +0100, Rafael J. Wysocki wrote:
> On Sun, Nov 21, 2021 at 4:17 PM Chen Yu <yu.c.chen@intel.com> wrote:
> >
>
> First of all, the subject of the patch should be "Introduce Platform
> Firmware Runtime Telemetry", because there is no "update" part in it.
>
Ok, will change it.
> > 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 above paragraph is redundant IMO. It provides a little useful
> information IMV.
>
Ok, will remove it
> > 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.
>
> In fact, this driver allows user space to fetch telemetry data from
> the firmware with the help of the Platform Firmware Runtime Telemetry
> interface.
>
Ok.
> > PFRU and PFRU Telemetry both invoke _DSM to trigger the low level actions.
>
> "PFRU and PFRT " or "PFRU and PFR Telemetry ", please.
>
Ok will use the former one.
> > 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.
>
> I would write the above in the following way:
>
> "Both PFRU and PFRT are based on ACPI _DSM interfaces located under
> special device objects in the ACPI Namespace, but these interfaces are
> different from each other, so it is better to provide a separate
> driver from each of them. However, they share some common definitions
> and naming conventions."
>
Ok, will change it.
> > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > ---
>
> [cut the history]
>
> > ---
> > drivers/acpi/pfru/Makefile | 2 +-
> > drivers/acpi/pfru/pfru_telemetry.c | 457 +++++++++++++++++++++++++++++
> > include/uapi/linux/pfru.h | 88 ++++++
> > 3 files changed, 546 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/acpi/pfru/pfru_telemetry.c
> >
> > diff --git a/drivers/acpi/pfru/Makefile b/drivers/acpi/pfru/Makefile
> > index 098cbe80cf3d..9ba11efe10b3 100644
> > --- a/drivers/acpi/pfru/Makefile
> > +++ b/drivers/acpi/pfru/Makefile
>
> Actually, I'm not sure if a separate directory for this is needed.
>
> The files (pfrut.h, pfr_update.c and pfr_telemetry.c) can be located
> directly in drivers/acpi/ as far as I'm concerned.
>
pfr_update.c and pfr_telemetry.c could be put in drivers/acpi, but pfrut.h
is used by user space tool for ioctl interface and shares some data structures
between the driver and user space tool, so the pfrut.h might need to be put
to include/uapi/linux/pfrut.h
> > @@ -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
>
> Please also update Kconfig with the information regarding PFRT (which
> is not going to be added by the previous patch I suppose).
>
Ok, will do.
> > diff --git a/drivers/acpi/pfru/pfru_telemetry.c b/drivers/acpi/pfru/pfru_telemetry.c
> > new file mode 100644
> > index 000000000000..5140a4591c9e
> > --- /dev/null
> > +++ b/drivers/acpi/pfru/pfru_telemetry.c
> > @@ -0,0 +1,457 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * ACPI Platform Firmware Runtime Update Telemetry Driver
>
> "ACPI Platform Runtime Telemetry driver"
>
Ok.
> > + *
> > + * Copyright (C) 2021 Intel Corporation
> > + * Author: Chen Yu <yu.c.chen@intel.com>
>
> Please describe the driver here at least briefly (what it is for etc.).
>
Ok, will do.
> > + */
> > +#include <linux/acpi.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/errno.h>
> > +#include <linux/file.h>
> > +#include <linux/fs.h>
> > +#include <linux/miscdevice.h>
> > +#include <linux/module.h>
> > +#include <linux/mm.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/string.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/uio.h>
> > +#include <linux/uuid.h>
> > +
> > +#include <uapi/linux/pfru.h>
> > +
> > +#define PFRU_LOG_EXEC_IDX 0
> > +#define PFRU_LOG_HISTORY_IDX 1
> > +
> > +#define PFRU_LOG_ERR 0
> > +#define PFRU_LOG_WARN 1
> > +#define PFRU_LOG_INFO 2
> > +#define PFRU_LOG_VERB 4
> > +
> > +#define PFRU_FUNC_SET_LEV 1
> > +#define PFRU_FUNC_GET_LEV 2
> > +#define PFRU_FUNC_GET_DATA 3
> > +
> > +#define PFRU_REVID_1 1
> > +#define PFRU_REVID_2 2
> > +#define PFRU_DEFAULT_REV_ID PFRU_REVID_1
>
> I would use the PFRT_ prefix in the above symbols.
>
Ok, will change them accordingly.
> > +
> > +enum log_index {
> > + LOG_STATUS_IDX = 0,
> > + LOG_EXT_STATUS_IDX = 1,
> > + LOG_MAX_SZ_IDX = 2,
> > + LOG_CHUNK1_LO_IDX = 3,
> > + LOG_CHUNK1_HI_IDX = 4,
> > + LOG_CHUNK1_SZ_IDX = 5,
> > + LOG_CHUNK2_LO_IDX = 6,
> > + LOG_CHUNK2_HI_IDX = 7,
> > + LOG_CHUNK2_SZ_IDX = 8,
> > + LOG_ROLLOVER_CNT_IDX = 9,
> > + LOG_RESET_CNT_IDX = 10,
> > + LOG_NR_IDX
> > +};
> > +
> > +struct pfru_log_device {
>
> 'pfrt_log_device"
>
Ok.
> > + int index;
> > + struct pfru_log_info info;
> > + struct device *parent_dev;
> > + struct miscdevice miscdev;
> > +};
> > +
> > +static const guid_t pfru_log_guid =
> > + GUID_INIT(0x75191659, 0x8178, 0x4D9D, 0xB8, 0x8F, 0xAC, 0x5E,
> > + 0x5E, 0x93, 0xE8, 0xBF);
>
> The UUID needs to be documented.
>
Ok.
> > +
> > +static DEFINE_IDA(pfru_log_ida);
>
> pfrt_log_ida
>
> Generally, I would use the pfrt_ prefix in the names below instead of pfru_.
>
Ok, will change them accordingly.
> > +
> > +static inline struct pfru_log_device *to_pfru_log_dev(struct file *file)
> > +{
> > + return container_of(file->private_data, struct pfru_log_device, miscdev);
> > +}
> > +
> > +static int get_pfru_log_data_info(struct pfru_log_data_info *data_info,
> > + struct pfru_log_device *pfru_log_dev)
> > +{
> > + acpi_handle handle = ACPI_HANDLE(pfru_log_dev->parent_dev);
> > + union acpi_object *out_obj, in_obj, in_buf;
> > + int ret = -EINVAL;
> > +
> > + memset(&in_obj, 0, sizeof(in_obj));
> > + memset(&in_buf, 0, sizeof(in_buf));
> > + in_obj.type = ACPI_TYPE_PACKAGE;
> > + in_obj.package.count = 1;
> > + in_obj.package.elements = &in_buf;
> > + in_buf.type = ACPI_TYPE_INTEGER;
> > + in_buf.integer.value = pfru_log_dev->info.log_type;
> > +
> > + out_obj = acpi_evaluate_dsm_typed(handle, &pfru_log_guid,
> > + pfru_log_dev->info.log_revid, PFRU_FUNC_GET_DATA,
> > + &in_obj, ACPI_TYPE_PACKAGE);
> > + if (!out_obj)
> > + return ret;
> > +
> > + if (out_obj->package.count < LOG_NR_IDX)
> > + goto free_acpi_buffer;
> > +
> > + if (out_obj->package.elements[LOG_STATUS_IDX].type != ACPI_TYPE_INTEGER)
> > + goto free_acpi_buffer;
> > +
> > + data_info->status = out_obj->package.elements[LOG_STATUS_IDX].integer.value;
> > +
> > + if (out_obj->package.elements[LOG_EXT_STATUS_IDX].type != ACPI_TYPE_INTEGER)
> > + goto free_acpi_buffer;
> > +
> > + data_info->ext_status =
> > + out_obj->package.elements[LOG_EXT_STATUS_IDX].integer.value;
> > +
> > + if (out_obj->package.elements[LOG_MAX_SZ_IDX].type != ACPI_TYPE_INTEGER)
> > + goto free_acpi_buffer;
> > +
> > + data_info->max_data_size =
> > + out_obj->package.elements[LOG_MAX_SZ_IDX].integer.value;
> > +
> > + if (out_obj->package.elements[LOG_CHUNK1_LO_IDX].type != ACPI_TYPE_INTEGER)
> > + goto free_acpi_buffer;
> > +
> > + data_info->chunk1_addr_lo =
> > + out_obj->package.elements[LOG_CHUNK1_LO_IDX].integer.value;
> > +
> > + if (out_obj->package.elements[LOG_CHUNK1_HI_IDX].type != ACPI_TYPE_INTEGER)
> > + goto free_acpi_buffer;
> > +
> > + data_info->chunk1_addr_hi =
> > + out_obj->package.elements[LOG_CHUNK1_HI_IDX].integer.value;
> > +
> > + if (out_obj->package.elements[LOG_CHUNK1_SZ_IDX].type != ACPI_TYPE_INTEGER)
> > + goto free_acpi_buffer;
> > +
> > + data_info->chunk1_size =
> > + out_obj->package.elements[LOG_CHUNK1_SZ_IDX].integer.value;
> > +
> > + if (out_obj->package.elements[LOG_CHUNK2_LO_IDX].type != ACPI_TYPE_INTEGER)
> > + goto free_acpi_buffer;
> > +
> > + data_info->chunk2_addr_lo =
> > + out_obj->package.elements[LOG_CHUNK2_LO_IDX].integer.value;
> > +
> > + if (out_obj->package.elements[LOG_CHUNK2_HI_IDX].type != ACPI_TYPE_INTEGER)
> > + goto free_acpi_buffer;
> > +
> > + data_info->chunk2_addr_hi =
> > + out_obj->package.elements[LOG_CHUNK2_HI_IDX].integer.value;
> > +
> > + if (out_obj->package.elements[LOG_CHUNK2_SZ_IDX].type != ACPI_TYPE_INTEGER)
> > + goto free_acpi_buffer;
> > +
> > + data_info->chunk2_size =
> > + out_obj->package.elements[LOG_CHUNK2_SZ_IDX].integer.value;
> > +
> > + if (out_obj->package.elements[LOG_ROLLOVER_CNT_IDX].type != ACPI_TYPE_INTEGER)
> > + goto free_acpi_buffer;
> > +
> > + data_info->rollover_cnt =
> > + out_obj->package.elements[LOG_ROLLOVER_CNT_IDX].integer.value;
> > +
> > + if (out_obj->package.elements[LOG_RESET_CNT_IDX].type != ACPI_TYPE_INTEGER)
> > + goto free_acpi_buffer;
> > +
> > + data_info->reset_cnt =
> > + out_obj->package.elements[LOG_RESET_CNT_IDX].integer.value;
>
> Like in some functions in the other patch, it would be good to reduce
> the number of goto statements and possibly redundant updates by
> grouping the checks.
>
Ok, will do.
> > +
> > + ret = 0;
> > +
> > +free_acpi_buffer:
> > + kfree(out_obj);
> > +
> > + return ret;
> > +}
> > +
[cut]
> > +static long pfru_log_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > +{
> > + struct pfru_log_device *pfru_log_dev = to_pfru_log_dev(file);
> > + struct pfru_log_data_info data_info;
> > + struct pfru_log_info info;
> > + void __user *p;
> > + int ret = 0;
> > +
> > + p = (void __user *)arg;
> > +
> > + switch (cmd) {
> > + case PFRU_LOG_IOC_SET_INFO:
> > + if (copy_from_user(&info, p, sizeof(info)))
> > + return -EFAULT;
> > +
> > + if (valid_log_revid(info.log_revid))
> > + pfru_log_dev->info.log_revid = info.log_revid;
> > +
> > + if (valid_log_level(info.log_level)) {
> > + ret = set_pfru_log_level(info.log_level, pfru_log_dev);
> > + if (ret < 0)
> > + return ret;
> > +
> > + pfru_log_dev->info.log_level = info.log_level;
> > + }
> > +
> > + if (valid_log_type(info.log_type))
> > + pfru_log_dev->info.log_type = info.log_type;
> > +
> > + return 0;
>
> Please use blank lines to separate the cases visually from each other.
>
Ok.
> > + case PFRU_LOG_IOC_GET_INFO:
> > + info.log_level = get_pfru_log_level(pfru_log_dev);
> > + if (ret < 0)
> > + return ret;
> > +
[cut]
> > + pfru_log_dev->info.log_revid = PFRU_DEFAULT_REV_ID;
> > + pfru_log_dev->parent_dev = &pdev->dev;
> > +
> > + pfru_log_dev->miscdev.minor = MISC_DYNAMIC_MINOR;
> > + pfru_log_dev->miscdev.name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> > + "pfru_telemetry%d",
>
> I would call it "pfr_telemetry" (without the "u" in the prefix).
>
Ok.
> > + pfru_log_dev->index);
> > + if (!pfru_log_dev->miscdev.name)
> > + return -ENOMEM;
> > +
> > + pfru_log_dev->miscdev.nodename = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> > + "acpi_pfru_telemetry%d",
>
> And this one (analogously) "acpi_pfr_telemetry".
>
Ok, will do.
> > + pfru_log_dev->index);
> > + if (!pfru_log_dev->miscdev.nodename)
> > + return -ENOMEM;
> > +
[cut]
>
> And of course it needs to be rebased to take the changes in the
> previous patch into account.
>
Ok, will do.
> Overall, it's almost there IMO.
thanks,
Chenyu
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v11 4/4] tools: Introduce power/acpi/tools/pfru
2021-12-07 19:52 ` Rafael J. Wysocki
@ 2021-12-08 7:20 ` Chen Yu
0 siblings, 0 replies; 17+ messages in thread
From: Chen Yu @ 2021-12-08 7:20 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: ACPI Devel Maling List, Greg Kroah-Hartman, Ard Biesheuvel,
Len Brown, Ashok Raj, Andy Shevchenko, Mike Rapoport,
Linux Kernel Mailing List, Robert Moore
On Tue, Dec 07, 2021 at 08:52:03PM +0100, Rafael J. Wysocki wrote:
> On Sun, Nov 21, 2021 at 4:18 PM Chen Yu <yu.c.chen@intel.com> wrote:
> >
> > 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.
> >
> > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > ---
> > v11:No change since v10.
> > v10:No change since v9.
> > v9: Add this tool into tools/power/acpi build infrastructure.
> > (Andy Shevchenko)
> > 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/.gitignore | 1 +
> > tools/power/acpi/Makefile | 16 +-
> > tools/power/acpi/Makefile.rules | 2 +-
> > tools/power/acpi/man/pfru.8 | 137 +++++++++
>
> I would call it pfrut, because it is for both the update and telemetry
> retrieval.
>
Ok, will change it.
[cut]
> > diff --git a/tools/power/acpi/tools/pfru/pfru.c b/tools/power/acpi/tools/pfru/pfru.c
> > new file mode 100644
> > index 000000000000..eeec5043f825
> > --- /dev/null
> > +++ b/tools/power/acpi/tools/pfru/pfru.c
> > @@ -0,0 +1,417 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Platform Firmware Runtime Update tool to do Management
> > + * Mode code injection/driver update and telemetry retrieval.
>
> I would be good to say a bit more here, like what interfaces are used
> by this tool etc.
>
Ok, will do.
> > +}
> > --
[cut]
>
> Please adjust the patch for the build fixes sent separately.
>
> Otherwise it is fine with me.
Ok, that patch has been sent separately.
thanks,
Chenyu
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-12-08 7:20 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-21 15:15 [PATCH v11 0/4] Introduce Platform Firmware Runtime Update and Telemetry drivers Chen Yu
2021-11-21 15:16 ` [PATCH v11 1/4] efi: Introduce EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER and corresponding structures Chen Yu
2021-12-03 18:20 ` Rafael J. Wysocki
2021-12-03 18:22 ` Ard Biesheuvel
2021-11-21 15:16 ` [PATCH v11 2/4] drivers/acpi: Introduce Platform Firmware Runtime Update device driver Chen Yu
2021-12-03 19:37 ` Rafael J. Wysocki
2021-12-04 8:07 ` Chen Yu
2021-11-21 15:16 ` [PATCH v11 3/4] drivers/acpi: Introduce Platform Firmware Runtime Update Telemetry Chen Yu
2021-12-07 19:44 ` Rafael J. Wysocki
2021-12-08 7:03 ` Chen Yu
2021-11-21 15:17 ` [PATCH v11 4/4] tools: Introduce power/acpi/tools/pfru Chen Yu
2021-11-22 11:26 ` Andy Shevchenko
2021-11-22 15:48 ` Chen Yu
2021-11-22 17:18 ` Andy Shevchenko
2021-11-23 1:15 ` Chen Yu
2021-12-07 19:52 ` Rafael J. Wysocki
2021-12-08 7:20 ` Chen Yu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.