[PATCH v3 0/5] Introduce Platform Firmware Runtime Update and Telemetry drivers

[PATCH v3 0/5] Introduce Platform Firmware Runtime Update and Telemetry drivers

[Chen Yu]

High Service Level Agreements (SLAs) requires that the system runs without
service interruptions. Generally, system firmware provides runtime services
such as RAS(Reliability, Availability and Serviceability) features, UEFI runtime
services and ACPI services. Currently if there is any firmware code changes in
these code area, the system firmware update and reboot is required. Example of
bug fix could be wrong register size or location of the register. This means
customer services are not available during the firmware upgrade, which could
approach several minutes, resulting in not able to meet SLAs.

Intel provides a mechanism named Management Mode Runtime Update to help the users
update the firmware without having to reboot[1].

This series provides the following facilities.

  1. Perform a runtime firmware driver update and activate.
  2. Ability to inject firmware code at runtime, for dynamic instrumentation.
  3. Facility to retrieve logs from runtime firmware update and activate telemetry.
     (The telemetry is based on runtime firmware update: it records the logs during
      runtime update(code injection and driver update).

The Management Mode Runtime Update OS Interface Specification[1] provides two ACPI
device objects to interface with system firmware to perform these updates. This patch
series introduces the drivers for those ACPI devices.

[1] https://uefi.org/sites/default/files/resources/Intel_MM_OS_Interface_Spec_Rev100.pdf

=============
- Change from v2 to v3:
  - Use valid types for structures that cross the user/kernel boundry
    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 (5):
  Documentation: Introduce Platform Firmware Runtime Update
    documentation
  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
  selftests/pfru: add test for Platform Firmware Runtime Update and
    Telemetry

 .../userspace-api/ioctl/ioctl-number.rst      |   1 +
 Documentation/x86/index.rst                   |   1 +
 Documentation/x86/pfru.rst                    | 100 +++
 drivers/acpi/Kconfig                          |   1 +
 drivers/acpi/Makefile                         |   1 +
 drivers/acpi/pfru/Kconfig                     |  29 +
 drivers/acpi/pfru/Makefile                    |   3 +
 drivers/acpi/pfru/pfru_telemetry.c            | 412 +++++++++++++
 drivers/acpi/pfru/pfru_update.c               | 567 ++++++++++++++++++
 include/linux/efi.h                           |  50 ++
 include/uapi/linux/pfru.h                     | 149 +++++
 tools/testing/selftests/Makefile              |   1 +
 tools/testing/selftests/pfru/Makefile         |   7 +
 tools/testing/selftests/pfru/config           |   2 +
 tools/testing/selftests/pfru/pfru_test.c      | 328 ++++++++++
 15 files changed, 1652 insertions(+)
 create mode 100644 Documentation/x86/pfru.rst
 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/testing/selftests/pfru/Makefile
 create mode 100644 tools/testing/selftests/pfru/config
 create mode 100644 tools/testing/selftests/pfru/pfru_test.c

-- 
2.25.1

[PATCH v3 1/5] Documentation: Introduce Platform Firmware Runtime Update documentation

[Chen Yu]

Add the Platform Firmware Runtime Update/Telemetry documentation.

Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
v3: No change since v2.
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).
---
 Documentation/x86/index.rst |   1 +
 Documentation/x86/pfru.rst  | 100 ++++++++++++++++++++++++++++++++++++
 2 files changed, 101 insertions(+)
 create mode 100644 Documentation/x86/pfru.rst

diff --git a/Documentation/x86/index.rst b/Documentation/x86/index.rst
index 383048396336..3791fabf964c 100644
--- a/Documentation/x86/index.rst
+++ b/Documentation/x86/index.rst
@@ -37,3 +37,4 @@ x86-specific Documentation
    sgx
    features
    elf_auxvec
+   pfru
diff --git a/Documentation/x86/pfru.rst b/Documentation/x86/pfru.rst
new file mode 100644
index 000000000000..29fe0e518e6d
--- /dev/null
+++ b/Documentation/x86/pfru.rst
@@ -0,0 +1,100 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+========================================================
+The Linux Platform Firmware Runtime Update and Telemetry
+========================================================
+
+According to the specification of `Management Mode Firmware Runtime Update
+<https://uefi.org/sites/default/files/resources/Intel_MM_OS_Interface_Spec_
+Rev100.pdf>`_, certain computing systems require high Service Level Agreements
+(SLAs) where system reboot fewer firmware updates are required to deploy
+firmware changes to address bug fixes, security updates and to debug and root
+cause issues. This technology is called Intel Seamless Update. The management
+mode (MM), UEFI runtime services and ACPI services handle most of the system
+runtime functions. Changing the MM code execution during runtime is called MM
+Runtime Update. Since the "MM" acronyms might be misunderstood as "Memory
+Management", this driver uses the name of "Platform Firmware Runtime Update"
+(PFRU).
+
+PFRU provides the following facilities: Performs a runtime firmware driver
+update and activate. Ability to inject firmware code at runtime, for dynamic
+instrumentation. PFRU Telemetry is a service which allows Runtime Update
+handler to produce telemetry data to upper layer OS consumer at runtime. The
+OS provides interfaces to let the users query the telemetry data via read
+operations. The specification specifies the interface and recommended policy
+to extract the data, the format and use are left to individual OEM's and BIOS
+implementations on what that data represents.
+
+PFRU interfaces
+===============
+
+The user space tool manipulates on /dev/pfru/update for code injection and
+driver update. PFRU stands for Platform Firmware Runtime Update, and the
+/dev/pfru directory might be reserved for future usage::
+
+ 1. mmap the capsule file.
+    fd_capsule = open("capsule.cap", O_RDONLY);
+    fstat(fd_capsule, &stat);
+    addr = mmap(0, stat.st_size, PROT_READ, fd_capsule);
+
+ 2. Get the capability information(version control, etc) from BIOS via
+    read() and do sanity check in user space.
+    fd_update = open("/dev/pfru/update", O_RDWR);
+    read(fd_update, &cap, sizeof(cap));
+    sanity_check(&cap);
+
+ 3. Write the capsule file to runtime update communication buffer.
+    kernel might return error if capsule file size is longer than
+    communication buffer.
+    write(fd_update, addr, stat.st_size);
+
+ 4. Stage the code injection.
+    ioctl(fd_update, PFRU_IOC_STATGE);
+
+ 5. Activate the code injection.
+    ioctl(fd_update, PFRU_IOC_ACTIVATE);
+
+ 6. Stage and activate the code injection.
+    ioctl(fd_update, PFRU_IOC_STAGE_ACTIVATE);
+
+    PFRU_IOC_STATGE: Stage a capsule image from communication buffer
+    and perform authentication.
+    [PFRU_IOC_ACTIVATE] Activate a previous staged capsule image.
+    [PFRU_IOC_STAGE_ACTIVATE] Perform both stage and activation actions.
+
+
+PFRU Telemetry interfaces
+=========================
+
+The user space tool manipulates on /dev/pfru/telemetry for PFRU telemetry log::
+
+ 1. Open telemetry device
+    fd_log = open("/dev/pfru/telemetry", O_RDWR);
+
+ 2. Get log level, log type, revision_id via one ioctl invoke
+    ioctl(fd_log, PFRU_IOC_GET_LOG_INFO, &info);
+
+ 3. Set log level, log type, revision_id
+    ioctl(fd_log, PFRU_IOC_SET_LOG_INFO, &info);
+
+ 4. ioctl(fd_log, PFRU_IOC_GET_DATA_INFO, &data_info);
+    Query the information of PFRU telemetry log buffer. The user is
+    responsible for parsing the result per the specification.
+
+ 5. Read the telemetry data:
+    read(fd_log, buf, data_info.size);
+
+Please refer to tools/testing/selftests/pfru/pfru_test.c for detail.
+
+According to the specification of `Management Mode Firmware Runtime Update
+<https://uefi.org/sites/default/files/resources/Intel_MM_OS_Interface_Spec_
+Rev100.pdf>`_, the telemetry buffer is a wrap around buffer. If the telemetry
+buffer gets full, most recent log data will overwrite old log data. Besides,
+it is required in the spec that the read of telemetry should support both full
+data retrieval and delta telemetry data retrieval. Since this requirement is
+more likely a policy we leave this implementation in user space. That is to
+say, it is recommended for the user to double-read the telemetry parameters
+such as chunk1_size, chunk2_size, rollover_cnt in data_info structure to make
+sure that there is no more data appended while the user is reading the buffer.
+Besides, only after the runtime update has been run at least once, the telemetry
+log would have valid data, otherwise errno code of EBUSY would be returned.
-- 
2.25.1

[PATCH v3 2/5] efi: Introduce EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER and corresponding structures

[Chen Yu]

Platform Firmware Runtime Update image starts with UEFI headers, and the
headers are defined in UEFI specification, but some of them have not been
defined in the kernel yet.

For example, the header layout of a capsule file looks like this:

EFI_CAPSULE_HEADER
EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER
EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER
EFI_FIRMWARE_IMAGE_AUTHENTICATION

These structures would be used by the Platform Firmware Runtime Update
driver to parse the format of capsule file to verify if the corresponding
version number is valid. The EFI_CAPSULE_HEADER has been defined in the
kernel, however the rest are not, thus introduce corresponding UEFI
structures accordingly.

The reason why efi_manage_capsule_header_t and
efi_manage_capsule_image_header_t are packedi might be that:
According to the uefi spec,
[Figure 23-6 Firmware Management and Firmware Image Management headers]
EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER is located at the lowest offset
within the body of the capsule. And this structure is designed to be
unaligned to save space, because in this way the adjacent drivers and
binary payload elements could start on byte boundary with no padding.
And the EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER is at the head of
each payload, so packing this structure also makes room for more data.

Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
 include/linux/efi.h | 50 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/include/linux/efi.h b/include/linux/efi.h
index 6b5d36babfcc..19ff834e1388 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -148,6 +148,56 @@ typedef struct {
 	u32 imagesize;
 } efi_capsule_header_t;
 
+#pragma pack(1)
+
+/* EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER */
+typedef struct {
+	u32	ver;
+	u16	emb_drv_cnt;
+	u16	payload_cnt;
+	/*
+	 * Variable array indicated by number of
+	 * (emb_drv_cnt + payload_cnt)
+	 */
+	u64	offset_list[];
+} efi_manage_capsule_header_t;
+
+/* EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER */
+typedef struct {
+	u32	ver;
+	guid_t	image_type_id;
+	u8	image_index;
+	u8	reserved_bytes[3];
+	u32	image_size;
+	u32	vendor_code_size;
+	/* ver = 2. */
+	u64	hw_ins;
+	/* ver = v3. */
+	u64	capsule_support;
+} efi_manage_capsule_image_header_t;
+
+#pragma pack()
+
+/* WIN_CERTIFICATE */
+typedef struct {
+	u32	len;
+	u16	rev;
+	u16	cert_type;
+} win_cert_t;
+
+/* WIN_CERTIFICATE_UEFI_GUID */
+typedef struct {
+	win_cert_t	hdr;
+	guid_t		cert_type;
+	u8		cert_data[];
+} win_cert_uefi_guid_t;
+
+/* EFI_FIRMWARE_IMAGE_AUTHENTICATIO */
+typedef struct {
+	u64				mon_count;
+	win_cert_uefi_guid_t		auth_info;
+} efi_image_auth_t;
+
 /*
  * EFI capsule flags
  */
-- 
2.25.1

[PATCH v3 3/5] drivers/acpi: Introduce Platform Firmware Runtime Update device driver

[Chen Yu]

Introduce the pfru_update driver which can be used for Platform Firmware
Runtime code injection and driver update. 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.

Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
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                     |  15 +
 drivers/acpi/pfru/Makefile                    |   2 +
 drivers/acpi/pfru/pfru_update.c               | 567 ++++++++++++++++++
 include/uapi/linux/pfru.h                     | 101 ++++
 7 files changed, 688 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 2e8134059c87..6e5a82fff408 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -365,6 +365,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-1F  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 1da360c51d66..1d8d2e2cefac 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..3f31b7d95f3b
--- /dev/null
+++ b/drivers/acpi/pfru/Kconfig
@@ -0,0 +1,15 @@
+# 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.
+
+	  For more information, see:
+	  <file:Documentation/x86/pfru_update.rst>
+
+	  To compile this driver as a module, choose M here:
+	  the module will be called pfru_update.
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..cd2039a195c9
--- /dev/null
+++ b/drivers/acpi/pfru/pfru_update.c
@@ -0,0 +1,567 @@
+// 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/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>
+
+enum cap_index {
+	CAP_STATUS_IDX,
+	CAP_UPDATE_IDX,
+	CAP_CODE_TYPE_IDX,
+	CAP_FW_VER_IDX,
+	CAP_CODE_RT_VER_IDX,
+	CAP_DRV_TYPE_IDX,
+	CAP_DRV_RT_VER_IDX,
+	CAP_DRV_SVN_IDX,
+	CAP_PLAT_ID_IDX,
+	CAP_OEM_ID_IDX,
+	CAP_OEM_INFO_IDX,
+};
+
+enum buf_index {
+	BUF_STATUS_IDX,
+	BUF_EXT_STATUS_IDX,
+	BUF_ADDR_LOW_IDX,
+	BUF_ADDR_HI_IDX,
+	BUF_SIZE_IDX,
+};
+
+enum update_index {
+	UPDATE_STATUS_IDX,
+	UPDATE_EXT_STATUS_IDX,
+	UPDATE_AUTH_TIME_LOW_IDX,
+	UPDATE_AUTH_TIME_HI_IDX,
+	UPDATE_EXEC_TIME_LOW_IDX,
+	UPDATE_EXEC_TIME_HI_IDX,
+};
+
+struct pfru_device {
+	guid_t uuid, code_uuid, drv_uuid;
+	int rev_id;
+	struct device *dev;
+};
+
+/*
+ * There would be only one instance of pfru_device.
+ */
+static struct pfru_device *pfru_dev;
+
+static bool valid_cap_type(int idx, union acpi_object *obj)
+{
+	acpi_object_type type = obj->type;
+
+	if (idx == CAP_STATUS_IDX || idx == CAP_UPDATE_IDX ||
+	    idx == CAP_FW_VER_IDX || idx == CAP_CODE_RT_VER_IDX ||
+	    idx == CAP_DRV_RT_VER_IDX || idx == CAP_DRV_SVN_IDX)
+		return type == ACPI_TYPE_INTEGER;
+	else if (idx == CAP_CODE_TYPE_IDX || idx == CAP_DRV_TYPE_IDX ||
+		 idx == CAP_PLAT_ID_IDX || idx == CAP_OEM_ID_IDX ||
+		 idx == CAP_OEM_INFO_IDX)
+		return type == ACPI_TYPE_BUFFER;
+	else
+		return false;
+}
+
+static int query_capability(struct pfru_update_cap_info *cap)
+{
+	union acpi_object *out_obj;
+	acpi_handle handle;
+	int i, ret = -EINVAL;
+
+	handle = ACPI_HANDLE(pfru_dev->dev);
+	out_obj = acpi_evaluate_dsm_typed(handle, &pfru_dev->uuid,
+					  pfru_dev->rev_id, FUNC_QUERY_UPDATE_CAP,
+					  NULL, ACPI_TYPE_PACKAGE);
+	if (!out_obj)
+		return -EINVAL;
+
+	for (i = 0; i < out_obj->package.count; i++) {
+		union acpi_object *obj = &out_obj->package.elements[i];
+
+		if (!valid_cap_type(i, obj))
+			goto free_acpi_buffer;
+
+		switch (i) {
+		case CAP_STATUS_IDX:
+			cap->status = obj->integer.value;
+			break;
+		case CAP_UPDATE_IDX:
+			cap->update_cap = obj->integer.value;
+			break;
+		case CAP_CODE_TYPE_IDX:
+			memcpy(&cap->code_type, obj->buffer.pointer,
+			       obj->buffer.length);
+			break;
+		case CAP_FW_VER_IDX:
+			cap->fw_version = obj->integer.value;
+			break;
+		case CAP_CODE_RT_VER_IDX:
+			cap->code_rt_version = obj->integer.value;
+			break;
+		case CAP_DRV_TYPE_IDX:
+			memcpy(&cap->drv_type, obj->buffer.pointer,
+			       obj->buffer.length);
+			break;
+		case CAP_DRV_RT_VER_IDX:
+			cap->drv_rt_version = obj->integer.value;
+			break;
+		case CAP_DRV_SVN_IDX:
+			cap->drv_svn = obj->integer.value;
+			break;
+		case CAP_PLAT_ID_IDX:
+			memcpy(&cap->platform_id, obj->buffer.pointer,
+			       obj->buffer.length);
+			break;
+		case CAP_OEM_ID_IDX:
+			memcpy(&cap->oem_id, obj->buffer.pointer,
+			       obj->buffer.length);
+			break;
+		case CAP_OEM_INFO_IDX:
+			/*vendor specific data*/
+			break;
+		default:
+			pr_err("Incorrect format of Update Capability.\n");
+			goto free_acpi_buffer;
+		}
+	}
+	ret = 0;
+
+free_acpi_buffer:
+	ACPI_FREE(out_obj);
+
+	return ret;
+}
+
+static int query_buffer(struct pfru_com_buf_info *info)
+{
+	union acpi_object *out_obj;
+	acpi_handle handle;
+	int i, ret = -EINVAL;
+
+	handle = ACPI_HANDLE(pfru_dev->dev);
+	out_obj = acpi_evaluate_dsm_typed(handle, &pfru_dev->uuid,
+					  pfru_dev->rev_id, FUNC_QUERY_BUF,
+					  NULL, ACPI_TYPE_PACKAGE);
+	if (!out_obj)
+		return -EINVAL;
+
+	for (i = 0; i < out_obj->package.count; i++) {
+		union acpi_object *obj = &out_obj->package.elements[i];
+
+		if (obj->type != ACPI_TYPE_INTEGER)
+			goto free_acpi_buffer;
+
+		switch (i) {
+		case BUF_STATUS_IDX:
+			info->status = obj->integer.value;
+			break;
+		case BUF_EXT_STATUS_IDX:
+			info->ext_status = obj->integer.value;
+			break;
+		case BUF_ADDR_LOW_IDX:
+			info->addr_lo = obj->integer.value;
+			break;
+		case BUF_ADDR_HI_IDX:
+			info->addr_hi = obj->integer.value;
+			break;
+		case BUF_SIZE_IDX:
+			info->buf_size = obj->integer.value;
+			break;
+		default:
+			pr_err("Incorrect format of Communication Buffer.\n");
+			goto free_acpi_buffer;
+		}
+	}
+	ret = 0;
+
+free_acpi_buffer:
+	ACPI_FREE(out_obj);
+
+	return ret;
+}
+
+static int get_image_type(efi_manage_capsule_image_header_t *img_hdr,
+			  int *type)
+{
+	guid_t *image_type_id;
+
+	/* check whether this is a code injection or driver update */
+	image_type_id = &img_hdr->image_type_id;
+	if (guid_equal(image_type_id, &pfru_dev->code_uuid))
+		*type = CODE_INJECT_TYPE;
+	else if (guid_equal(image_type_id, &pfru_dev->drv_uuid))
+		*type = DRIVER_UPDATE_TYPE;
+	else
+		return -EINVAL;
+
+	return 0;
+}
+
+/*
+ * 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.
+ */
+static int adjust_efi_size(efi_manage_capsule_image_header_t *img_hdr,
+			   int *size)
+{
+	int tmp_size = *size;
+
+	tmp_size += sizeof(efi_manage_capsule_image_header_t);
+	switch (img_hdr->ver) {
+	case 1:
+		tmp_size -= 2 * sizeof(u64);
+		break;
+	case 2:
+		tmp_size -= sizeof(u64);
+		break;
+	default:
+		/* only support version 1 and 2 */
+		return -EINVAL;
+	}
+	*size = tmp_size;
+
+	return 0;
+}
+
+/*
+ * Sanity check if the capsule image has a newer version than current one.
+ * Return: true if it is valid, false otherwise.
+ */
+static bool valid_version(const void *data, struct pfru_update_cap_info *cap)
+{
+	struct pfru_payload_hdr *payload_hdr;
+	efi_capsule_header_t *cap_hdr;
+	efi_manage_capsule_header_t *m_hdr;
+	efi_manage_capsule_image_header_t *m_img_hdr;
+	efi_image_auth_t *auth;
+	int type, size, ret;
+
+	cap_hdr = (efi_capsule_header_t *)data;
+	size = cap_hdr->headersize;
+	m_hdr = (efi_manage_capsule_header_t *)(data + size);
+	/*
+	 * Current data structure size plus variable array indicated
+	 * by number of (emb_drv_cnt + payload_cnt)
+	 */
+	size += sizeof(efi_manage_capsule_header_t) +
+		      (m_hdr->emb_drv_cnt + m_hdr->payload_cnt) * sizeof(u64);
+	m_img_hdr = (efi_manage_capsule_image_header_t *)(data + size);
+
+	ret = get_image_type(m_img_hdr, &type);
+	if (ret)
+		return false;
+
+	ret = adjust_efi_size(m_img_hdr, &size);
+	if (ret)
+		return false;
+
+	auth = (efi_image_auth_t *)(data + size);
+	size += sizeof(u64) + auth->auth_info.hdr.len;
+	payload_hdr = (struct pfru_payload_hdr *)(data + size);
+
+	/* Finally, compare the version. */
+	if (type == 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)
+{
+	pr_debug("Update result:\n");
+	pr_debug("Status:%d\n", result->status);
+	pr_debug("Extended Status:%d\n", result->ext_status);
+	pr_debug("Authentication Time Low:%lld\n", result->low_auth_time);
+	pr_debug("Authentication Time High:%lld\n", result->high_auth_time);
+	pr_debug("Execution Time Low:%lld\n", result->low_exec_time);
+	pr_debug("Execution Time High:%lld\n", result->high_exec_time);
+}
+
+static int start_acpi_update(int action)
+{
+	union acpi_object *out_obj, in_obj, in_buf;
+	struct pfru_updated_result update_result;
+	acpi_handle handle;
+	int i, 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->dev);
+	out_obj = acpi_evaluate_dsm_typed(handle, &pfru_dev->uuid,
+					  pfru_dev->rev_id, FUNC_START,
+					  &in_obj, ACPI_TYPE_PACKAGE);
+	if (!out_obj)
+		return -EINVAL;
+
+	for (i = 0; i < out_obj->package.count; i++) {
+		union acpi_object *obj = &out_obj->package.elements[i];
+
+		if (obj->type != ACPI_TYPE_INTEGER)
+			goto free_acpi_buffer;
+		switch (i) {
+		case UPDATE_STATUS_IDX:
+			update_result.status = obj->integer.value;
+			break;
+		case UPDATE_EXT_STATUS_IDX:
+			update_result.ext_status = obj->integer.value;
+			break;
+		case UPDATE_AUTH_TIME_LOW_IDX:
+			update_result.low_auth_time = obj->integer.value;
+			break;
+		case UPDATE_AUTH_TIME_HI_IDX:
+			update_result.high_auth_time = obj->integer.value;
+			break;
+		case UPDATE_EXEC_TIME_LOW_IDX:
+			update_result.low_exec_time = obj->integer.value;
+			break;
+		case UPDATE_EXEC_TIME_HI_IDX:
+			update_result.high_exec_time = obj->integer.value;
+			break;
+		default:
+			pr_err("Incorrect format of Runtime Update result.\n");
+			goto free_acpi_buffer;
+		}
+	}
+	dump_update_result(&update_result);
+	ret = 0;
+
+free_acpi_buffer:
+	ACPI_FREE(out_obj);
+
+	return ret;
+}
+
+static long pfru_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	void __user *p;
+	int ret = 0, rev;
+
+	p = (void __user *)arg;
+
+	switch (cmd) {
+	case PFRU_IOC_SET_REV:
+		if (copy_from_user(&rev, p, sizeof(unsigned int)))
+			return -EFAULT;
+		if (!pfru_valid_revid(rev))
+			return -EFAULT;
+		pfru_dev->rev_id = rev;
+		break;
+	case PFRU_IOC_STAGE:
+		ret = start_acpi_update(START_STAGE);
+		break;
+	case PFRU_IOC_ACTIVATE:
+		ret = start_acpi_update(START_ACTIVATE);
+		break;
+	case PFRU_IOC_STAGE_ACTIVATE:
+		ret = start_acpi_update(START_STAGE_ACTIVATE);
+		break;
+	default:
+		ret = -ENOIOCTLCMD;
+		break;
+	}
+
+	return ret;
+}
+
+#ifdef CONFIG_COMPAT
+static long compat_pfru_ioctl(struct file *filep, unsigned int cmd,
+			      unsigned long arg)
+{
+	return pfru_ioctl(filep, cmd, arg);
+}
+#endif
+
+static int pfru_open(struct inode *inode, struct file *file)
+{
+	return capable(CAP_SYS_RAWIO) ? stream_open(inode, file) : -EPERM;
+}
+
+static ssize_t pfru_write(struct file *file, const char __user *buf,
+			  size_t len, loff_t *ppos)
+{
+	struct pfru_update_cap_info cap;
+	struct pfru_com_buf_info info;
+	phys_addr_t phy_addr;
+	struct iov_iter iter;
+	struct iovec iov;
+	char *buf_ptr;
+	int ret;
+
+	ret = query_buffer(&info);
+	if (ret)
+		return ret;
+
+	if (len > 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)(info.addr_lo | (info.addr_hi << 32));
+	buf_ptr = memremap(phy_addr, info.buf_size, MEMREMAP_WB);
+	if (IS_ERR(buf_ptr))
+		return PTR_ERR(buf_ptr);
+	if (!copy_from_iter_full(buf_ptr, len, &iter)) {
+		pr_err("error! could not read capsule file\n");
+		ret = -EINVAL;
+		goto unmap;
+	}
+
+	/* Check if the capsule header has a valid version number. */
+	ret = query_capability(&cap);
+	if (ret)
+		goto unmap;
+
+	if (cap.status != DSM_SUCCEED) {
+		ret = -EBUSY;
+		goto unmap;
+	}
+	if (!valid_version(buf_ptr, &cap)) {
+		ret = -EINVAL;
+		goto unmap;
+	}
+	ret = 0;
+unmap:
+	memunmap(buf_ptr);
+
+	return ret ?: len;
+}
+
+static ssize_t pfru_read(struct file *filp, char __user *ubuf,
+			 size_t size, loff_t *off)
+{
+	struct pfru_update_cap_info cap;
+	int ret;
+
+	ret = query_capability(&cap);
+	if (ret)
+		return ret;
+
+	size = min_t(size_t, size, sizeof(cap));
+
+	if (copy_to_user(ubuf, &cap, size))
+		return -EFAULT;
+
+	return size;
+}
+
+static const struct file_operations acpi_pfru_fops = {
+	.owner		= THIS_MODULE,
+	.write		= pfru_write,
+	.read		= pfru_read,
+	.open		= pfru_open,
+	.unlocked_ioctl = pfru_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl	= compat_pfru_ioctl,
+#endif
+	.llseek		= noop_llseek,
+};
+
+static struct miscdevice pfru_misc_dev = {
+	.minor = MISC_DYNAMIC_MINOR,
+	.name = "pfru_update",
+	.nodename = "pfru/update",
+	.fops = &acpi_pfru_fops,
+};
+
+static int acpi_pfru_remove(struct platform_device *pdev)
+{
+	misc_deregister(&pfru_misc_dev);
+	kfree(pfru_dev);
+	pfru_dev = NULL;
+
+	return 0;
+}
+
+static int acpi_pfru_probe(struct platform_device *pdev)
+{
+	acpi_handle handle;
+	int ret;
+
+	if (pfru_dev) {
+		pr_err("Duplicated PFRU INTC1080 detected, skip...\n");
+		return 0;
+	}
+
+	pfru_dev = kzalloc(sizeof(*pfru_dev), GFP_KERNEL);
+	if (!pfru_dev)
+		return -ENOMEM;
+
+	ret = guid_parse(PFRU_UUID, &pfru_dev->uuid);
+	if (ret)
+		goto out;
+	ret = guid_parse(PFRU_CODE_INJ_UUID, &pfru_dev->code_uuid);
+	if (ret)
+		goto out;
+	ret = guid_parse(PFRU_DRV_UPDATE_UUID, &pfru_dev->drv_uuid);
+	if (ret)
+		goto out;
+
+	/* default rev id is 1 */
+	pfru_dev->rev_id = 1;
+	pfru_dev->dev = &pdev->dev;
+	handle = ACPI_HANDLE(pfru_dev->dev);
+	if (!acpi_has_method(handle, "_DSM")) {
+		pr_err("Missing _DSM\n");
+		ret = -ENODEV;
+		goto out;
+	}
+
+	ret = misc_register(&pfru_misc_dev);
+	if (ret)
+		goto out;
+
+	return 0;
+out:
+	kfree(pfru_dev);
+	pfru_dev = NULL;
+
+	return ret;
+}
+
+static const struct acpi_device_id acpi_pfru_ids[] = {
+	{"INTC1080", 0},
+	{}
+};
+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..ca0b7433f79f
--- /dev/null
+++ b/include/uapi/linux/pfru.h
@@ -0,0 +1,101 @@
+/* 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/uuid.h>
+
+#define PFRU_UUID		"ECF9533B-4A3C-4E89-939E-C77112601C6D"
+#define PFRU_CODE_INJ_UUID		"B2F84B79-7B6E-4E45-885F-3FB9BB185402"
+#define PFRU_DRV_UPDATE_UUID		"4569DD8C-75F1-429A-A3D6-24DE8097A0DF"
+
+#define FUNC_STANDARD_QUERY	0
+#define FUNC_QUERY_UPDATE_CAP	1
+#define FUNC_QUERY_BUF		2
+#define FUNC_START		3
+
+#define CODE_INJECT_TYPE	1
+#define DRIVER_UPDATE_TYPE	2
+
+#define REVID_1		1
+#define REVID_2		2
+
+#define PFRU_MAGIC 0xEE
+
+#define PFRU_IOC_SET_REV _IOW(PFRU_MAGIC, 0x01, unsigned int)
+#define PFRU_IOC_STAGE _IOW(PFRU_MAGIC, 0x02, unsigned int)
+#define PFRU_IOC_ACTIVATE _IOW(PFRU_MAGIC, 0x03, unsigned int)
+#define PFRU_IOC_STAGE_ACTIVATE _IOW(PFRU_MAGIC, 0x04, unsigned int)
+
+static inline int pfru_valid_revid(int id)
+{
+	return (id == REVID_1) || (id == REVID_2);
+}
+
+/* Capsule file payload header */
+struct pfru_payload_hdr {
+	__u32	sig;
+	__u32	hdr_version;
+	__u32	hdr_size;
+	__u32	hw_ver;
+	__u32	rt_ver;
+	uuid_t	platform_id;
+};
+
+enum pfru_start_action {
+	START_STAGE,
+	START_ACTIVATE,
+	START_STAGE_ACTIVATE,
+};
+
+enum pfru_dsm_status {
+	DSM_SUCCEED,
+	DSM_FUNC_NOT_SUPPORT,
+	DSM_INVAL_INPUT,
+	DSM_HARDWARE_ERR,
+	DSM_RETRY_SUGGESTED,
+	DSM_UNKNOWN,
+	DSM_FUNC_SPEC_ERR,
+};
+
+struct pfru_update_cap_info {
+	enum pfru_dsm_status status;
+	__u32 update_cap;
+
+	uuid_t code_type;
+	__u32 fw_version;
+	__u32 code_rt_version;
+
+	uuid_t drv_type;
+	__u32 drv_rt_version;
+	__u32 drv_svn;
+
+	uuid_t platform_id;
+	uuid_t oem_id;
+
+	char oem_info[];
+};
+
+struct pfru_com_buf_info {
+	enum pfru_dsm_status status;
+	enum pfru_dsm_status ext_status;
+	__u64 addr_lo;
+	__u64 addr_hi;
+	__u32 buf_size;
+};
+
+struct pfru_updated_result {
+	enum pfru_dsm_status status;
+	enum pfru_dsm_status ext_status;
+	__u64 low_auth_time;
+	__u64 high_auth_time;
+	__u64 low_exec_time;
+	__u64 high_exec_time;
+};
+
+#endif /* __PFRU_H__ */
-- 
2.25.1

[PATCH v3 4/5] drivers/acpi: Introduce Platform Firmware Runtime Update Telemetry

[Chen Yu]

Platform Firmware Runtime Update(PFRU) Telemetry Service is part of RoT
(Root of Trust), which allows PFRU handler and other PFRU drivers to
produce telemetry data to upper layer OS consumer at runtime.

The linux provides interfaces for the user to query the parameters of
telemetry data, and the user could read out the telemetry data
accordingly.

Typical log looks like:

[SmmRuntimeUpdateHandler.ProcessSmmRuntimeUpdate]
ProcessSmmRuntimeUpdate = START, Action = 2
[SmmRuntimeUpdateHandler.ProcessSmmRuntimeUpdate]
FwVersion = 0, CodeInjectionVersion = 1
[ShadowSmmRuntimeUpdateImage]
Image = 0x74D9B000, ImageSize = 0x1172
[ProcessSmmRuntimeUpdate]
ShadowSmmRuntimeUpdateImage.Status = Success
[ValidateSmmRuntimeUpdateImage]
CapsuleHeader.CapsuleGuid = 6DCBD5ED-E82D-4C44-BDA1-7194199AD92A
[ValidateSmmRuntimeUpdateImage]
FmpCapHeader.Version = 1
[ValidateSmmRuntimeUpdateImage]
FmpCapImageHeader.UpdateImageTypeId = B2F84B79-7B6E-4E45-885F-3FB9BB185402
[ValidateSmmRuntimeUpdateImage]
SmmRuntimeUpdateVerifyImageWithDenylist.Status = Success
[ValidateSmmRuntimeUpdateImage]
SmmRuntimeUpdateVerifyImageWithAllowlist.Status = Success
[SmmCodeInjectionVerifyPayloadHeader]
PayloadHeader.Signature = 0x31494353
[SmmCodeInjectionVerifyPayloadHeader]
PayloadHeader.PlatformId = 63462139-A8B1-AA4E-9024-F2BB53EA4723
[SmmCodeInjectionVerifyPayloadHeader]
PayloadHeader.SupportedSmmFirmwareVersion = 0,
PayloadHeader.SmmCodeInjectionRuntimeVersion = 1
[ProcessSmmRuntimeUpdate]
ValidateSmmRuntimeUpdateImage.Status = Success
CPU CSR[0B102D28] Before = 7FBF830E
CPU CSR[0B102D28] After = 7FBF8310
[ProcessSmmRuntimeUpdate] ProcessSmmCodeInjection.Status = Success
[SmmRuntimeUpdateHandler.ProcessSmmRuntimeUpdate]
ProcessSmmRuntimeUpdate = End, Status = Success

Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
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/Kconfig          |  14 +
 drivers/acpi/pfru/Makefile         |   1 +
 drivers/acpi/pfru/pfru_telemetry.c | 412 +++++++++++++++++++++++++++++
 include/uapi/linux/pfru.h          |  43 +++
 4 files changed, 470 insertions(+)
 create mode 100644 drivers/acpi/pfru/pfru_telemetry.c

diff --git a/drivers/acpi/pfru/Kconfig b/drivers/acpi/pfru/Kconfig
index 3f31b7d95f3b..e2934058884e 100644
--- a/drivers/acpi/pfru/Kconfig
+++ b/drivers/acpi/pfru/Kconfig
@@ -13,3 +13,17 @@ config ACPI_PFRU
 
 	  To compile this driver as a module, choose M here:
 	  the module will be called pfru_update.
+
+config ACPI_PFRU_TELEMETRY
+	tristate "ACPI Platform Firmware Runtime Update Telemetry Service"
+	depends on ACPI_PFRU
+	help
+	  PFRU(Platform Firmware Runtime Update) Telemetry Service is part of
+	  RoT(Root of Trust), which allows Platform Firmware Runtime Update handler
+	  and other PFRU drivers to produce telemetry data to upper layer OS consumer
+	  at runtime.
+
+	  For more information, see:
+	  <file:Documentation/x86/pfru_update.rst>
+
+	  If unsure, please say N.
diff --git a/drivers/acpi/pfru/Makefile b/drivers/acpi/pfru/Makefile
index 098cbe80cf3d..30060ba320ea 100644
--- a/drivers/acpi/pfru/Makefile
+++ b/drivers/acpi/pfru/Makefile
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_ACPI_PFRU) += pfru_update.o
+obj-$(CONFIG_ACPI_PFRU_TELEMETRY) += 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..96dc9e69edc0
--- /dev/null
+++ b/drivers/acpi/pfru/pfru_telemetry.c
@@ -0,0 +1,412 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ACPI Platform Firmware Runtime Update
+ * Telemetry Service Device Driver
+ *
+ * Copyright (C) 2021 Intel Corporation
+ * Author: Chen Yu <yu.c.chen@intel.com>
+ */
+#include <linux/acpi.h>
+#include <linux/errno.h>
+#include <linux/fs.h>
+#include <linux/file.h>
+#include <linux/minmax.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/mm.h>
+#include <linux/platform_device.h>
+#include <linux/uuid.h>
+#include <linux/uaccess.h>
+#include <uapi/linux/pfru.h>
+
+enum update_index {
+	LOG_STATUS_IDX,
+	LOG_EXT_STATUS_IDX,
+	LOG_MAX_SZ_IDX,
+	LOG_CHUNK1_LO_IDX,
+	LOG_CHUNK1_HI_IDX,
+	LOG_CHUNK1_SZ_IDX,
+	LOG_CHUNK2_LO_IDX,
+	LOG_CHUNK2_HI_IDX,
+	LOG_CHUNK2_SZ_IDX,
+	LOG_ROLLOVER_CNT_IDX,
+	LOG_RESET_CNT_IDX,
+};
+
+struct pfru_telem_device {
+	struct device *dev;
+	guid_t uuid;
+	struct pfru_telem_info info;
+};
+
+/*
+ * There would be only one instance of pfru_telem_device.
+ */
+static struct pfru_telem_device *telem_dev;
+
+static int get_pfru_data_info(struct pfru_telem_data_info *data_info,
+			      int log_type)
+{
+	union acpi_object *out_obj, in_obj, in_buf;
+	acpi_handle handle;
+	int i, ret = -EINVAL;
+
+	handle = ACPI_HANDLE(telem_dev->dev);
+
+	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 = log_type;
+
+	out_obj = acpi_evaluate_dsm_typed(handle, &telem_dev->uuid,
+					  telem_dev->info.log_revid, FUNC_GET_DATA,
+					  &in_obj, ACPI_TYPE_PACKAGE);
+	if (!out_obj) {
+		pr_err("Failed to invoke _DSM\n");
+		return -EINVAL;
+	}
+
+	for (i = 0; i < out_obj->package.count; i++) {
+		union acpi_object *obj = &out_obj->package.elements[i];
+
+		if (obj->type != ACPI_TYPE_INTEGER)
+			goto free_acpi_buffer;
+
+		switch (i) {
+		case LOG_STATUS_IDX:
+			data_info->status = obj->integer.value;
+			break;
+		case LOG_EXT_STATUS_IDX:
+			data_info->ext_status = obj->integer.value;
+			break;
+		case LOG_MAX_SZ_IDX:
+			data_info->max_data_size = obj->integer.value;
+			break;
+		case LOG_CHUNK1_LO_IDX:
+			data_info->chunk1_addr_lo = obj->integer.value;
+			break;
+		case LOG_CHUNK1_HI_IDX:
+			data_info->chunk1_addr_hi = obj->integer.value;
+			break;
+		case LOG_CHUNK1_SZ_IDX:
+			data_info->chunk1_size = obj->integer.value;
+			break;
+		case LOG_CHUNK2_LO_IDX:
+			data_info->chunk2_addr_lo = obj->integer.value;
+			break;
+		case LOG_CHUNK2_HI_IDX:
+			data_info->chunk2_addr_hi = obj->integer.value;
+			break;
+		case LOG_CHUNK2_SZ_IDX:
+			data_info->chunk2_size = obj->integer.value;
+			break;
+		case LOG_ROLLOVER_CNT_IDX:
+			data_info->rollover_cnt = obj->integer.value;
+			break;
+		case LOG_RESET_CNT_IDX:
+			data_info->reset_cnt = obj->integer.value;
+			break;
+		default:
+			pr_err("Incorrect format of Telemetry info.\n");
+			goto free_acpi_buffer;
+		}
+	}
+	ret = 0;
+
+free_acpi_buffer:
+	ACPI_FREE(out_obj);
+
+	return ret;
+}
+
+static int set_pfru_log_level(int level)
+{
+	union acpi_object *out_obj, *obj, in_obj, in_buf;
+	enum pfru_dsm_status status;
+	acpi_handle handle;
+	int ret = -EINVAL;
+
+	handle = ACPI_HANDLE(telem_dev->dev);
+
+	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, &telem_dev->uuid,
+					  telem_dev->info.log_revid, 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) {
+		pr_err("get MM telemetry level error status %d\n",
+		       status);
+		goto free_acpi_buffer;
+	}
+
+	obj = &out_obj->package.elements[1];
+	status = obj->integer.value;
+	if (status) {
+		pr_err("get MM telemetry level error extend status %d\n",
+		       status);
+		goto free_acpi_buffer;
+	}
+	ret = 0;
+
+free_acpi_buffer:
+	ACPI_FREE(out_obj);
+
+	return ret;
+}
+
+static int get_pfru_log_level(int *level)
+{
+	union acpi_object *out_obj, *obj;
+	enum pfru_dsm_status status;
+	acpi_handle handle;
+	int ret = -EINVAL;
+
+	handle = ACPI_HANDLE(telem_dev->dev);
+	out_obj = acpi_evaluate_dsm_typed(handle, &telem_dev->uuid,
+					  telem_dev->info.log_revid, 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) {
+		pr_err("get MM telemetry level error status %d\n",
+		       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) {
+		pr_err("get MM telemetry level error status %d\n",
+		       status);
+		goto free_acpi_buffer;
+	}
+
+	obj = &out_obj->package.elements[2];
+	if (obj->type != ACPI_TYPE_INTEGER)
+		goto free_acpi_buffer;
+	*level = obj->integer.value;
+
+	ret = 0;
+
+free_acpi_buffer:
+	ACPI_FREE(out_obj);
+
+	return ret;
+}
+
+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 long pfru_telemetry_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+
+{
+	struct pfru_telem_data_info data_info;
+	struct pfru_telem_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 (pfru_valid_revid(info.log_revid))
+			telem_dev->info.log_revid = info.log_revid;
+
+		if (valid_log_level(info.log_level)) {
+			ret = set_pfru_log_level(info.log_level);
+			if (ret)
+				return ret;
+			telem_dev->info.log_level = info.log_level;
+		}
+		if (valid_log_type(info.log_type))
+			telem_dev->info.log_type = info.log_type;
+		break;
+	case PFRU_LOG_IOC_GET_INFO:
+		ret = get_pfru_log_level(&info.log_level);
+		if (ret)
+			return ret;
+		info.log_type = telem_dev->info.log_type;
+		info.log_revid = telem_dev->info.log_revid;
+		if (copy_to_user(p, &info, sizeof(info)))
+			ret = -EFAULT;
+		break;
+	case PFRU_LOG_IOC_GET_DATA_INFO:
+		ret = get_pfru_data_info(&data_info, telem_dev->info.log_type);
+		if (ret)
+			return ret;
+		if (copy_to_user(p, &data_info, sizeof(struct pfru_telem_data_info)))
+			ret = -EFAULT;
+		break;
+	default:
+		ret = -ENOIOCTLCMD;
+		break;
+	}
+	return ret;
+}
+
+static ssize_t pfru_telemetry_read(struct file *filp, char __user *ubuf,
+				   size_t size, loff_t *off)
+{
+	struct pfru_telem_data_info info;
+	phys_addr_t base_addr;
+	int buf_size, ret;
+	char *buf_ptr;
+
+	if (*off < 0)
+		return -EINVAL;
+
+	ret = get_pfru_data_info(&info, telem_dev->info.log_type);
+	if (ret) {
+		pr_err("Could not get telemetry data info %d\n", ret);
+		return ret;
+	}
+
+	base_addr = (phys_addr_t)(info.chunk2_addr_lo |
+			(info.chunk2_addr_hi << 32));
+	if (!base_addr) {
+		pr_err("Telemetry data not ready\n");
+		return -EBUSY;
+	}
+
+	buf_size = info.max_data_size;
+	if (*off >= buf_size)
+		return 0;
+
+	buf_ptr = memremap(base_addr, buf_size, MEMREMAP_WB);
+	if (IS_ERR(buf_ptr))
+		return PTR_ERR(buf_ptr);
+
+	size = min_t(size_t, size, buf_size - *off);
+
+	ret = -EFAULT;
+	if (copy_to_user(ubuf, buf_ptr + *off, size))
+		goto out;
+	ret = 0;
+out:
+	memunmap(buf_ptr);
+
+	return ret ? ret : size;
+}
+
+#ifdef CONFIG_COMPAT
+static long compat_pfru_telemetry_ioctl(struct file *filep, unsigned int cmd,
+					unsigned long arg)
+{
+	return pfru_telemetry_ioctl(filep, cmd, arg);
+}
+#endif
+
+static const struct file_operations acpi_pfru_telemetry_fops = {
+	.owner		= THIS_MODULE,
+	.read		= pfru_telemetry_read,
+	.unlocked_ioctl = pfru_telemetry_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl	= compat_pfru_telemetry_ioctl,
+#endif
+	.llseek		= noop_llseek,
+};
+
+static struct miscdevice pfru_telemetry_misc_dev = {
+	.minor = MISC_DYNAMIC_MINOR,
+	.name = "pfru_telemetry",
+	.nodename = "pfru/telemetry",
+	.fops = &acpi_pfru_telemetry_fops,
+};
+
+static int acpi_pfru_telemetry_remove(struct platform_device *pdev)
+{
+	misc_deregister(&pfru_telemetry_misc_dev);
+	kfree(telem_dev);
+	telem_dev = NULL;
+
+	return 0;
+}
+
+static int acpi_pfru_telemetry_probe(struct platform_device *pdev)
+{
+	acpi_handle handle;
+	int ret;
+
+	if (telem_dev) {
+		pr_err("Duplicated PFRU Telemetry INTC1081 detected, skip...\n");
+		return 0;
+	}
+
+	telem_dev = kzalloc(sizeof(*telem_dev), GFP_KERNEL);
+	if (!telem_dev)
+		return -ENOMEM;
+
+	ret = guid_parse(PFRU_TELEMETRY_UUID, &telem_dev->uuid);
+	if (ret)
+		goto out;
+
+	telem_dev->info.log_revid = 1;
+	telem_dev->dev = &pdev->dev;
+	handle = ACPI_HANDLE(telem_dev->dev);
+	if (!acpi_has_method(handle, "_DSM")) {
+		pr_err("Missing _DSM\n");
+		ret = -ENODEV;
+		goto out;
+	}
+
+	ret = misc_register(&pfru_telemetry_misc_dev);
+	if (ret)
+		goto out;
+
+	return 0;
+out:
+	kfree(telem_dev);
+	telem_dev = NULL;
+
+	return ret;
+}
+
+static const struct acpi_device_id acpi_pfru_telemetry_ids[] = {
+	{"INTC1081", 0},
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, acpi_pfru_telemetry_ids);
+
+static struct platform_driver acpi_pfru_telemetry_driver = {
+	.driver = {
+		.name = "pfru_telemetry",
+		.acpi_match_table = acpi_pfru_telemetry_ids,
+	},
+	.probe = acpi_pfru_telemetry_probe,
+	.remove = acpi_pfru_telemetry_remove,
+};
+module_platform_driver(acpi_pfru_telemetry_driver);
+
+MODULE_DESCRIPTION("Platform Firmware Runtime Update Telemetry Service device driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/uapi/linux/pfru.h b/include/uapi/linux/pfru.h
index ca0b7433f79f..b04852602133 100644
--- a/include/uapi/linux/pfru.h
+++ b/include/uapi/linux/pfru.h
@@ -98,4 +98,47 @@ struct pfru_updated_result {
 	__u64 high_exec_time;
 };
 
+#define PFRU_TELEMETRY_UUID	"75191659-8178-4D9D-B88F-AC5E5E93E8BF"
+
+/* Telemetry structures. */
+struct pfru_telem_data_info {
+	enum pfru_dsm_status status;
+	enum pfru_dsm_status 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_telem_info {
+	__u32 log_level;
+	__u32 log_type;
+	__u32 log_revid;
+};
+
+/* Two logs: history and execution log */
+#define LOG_EXEC_IDX	0
+#define LOG_HISTORY_IDX	1
+#define NR_LOG_TYPE	2
+
+#define LOG_ERR		0
+#define LOG_WARN	1
+#define LOG_INFO	2
+#define LOG_VERB	4
+
+#define FUNC_SET_LEV		1
+#define FUNC_GET_LEV		2
+#define FUNC_GET_DATA		3
+
+#define LOG_NAME_SIZE		10
+
+#define PFRU_LOG_IOC_SET_INFO _IOW(PFRU_MAGIC, 0x05, struct pfru_telem_info)
+#define PFRU_LOG_IOC_GET_INFO _IOR(PFRU_MAGIC, 0x06, struct pfru_telem_info)
+#define PFRU_LOG_IOC_GET_DATA_INFO _IOR(PFRU_MAGIC, 0x07, struct pfru_telem_data_info)
+
 #endif /* __PFRU_H__ */
-- 
2.25.1

[PATCH v3 5/5] selftests/pfru: add test for Platform Firmware Runtime Update and Telemetry

[Chen Yu]

Introduce a simple test for Platform Firmware Runtime Update and Telemetry
drivers. It is based on ioctl to either update firmware driver or code
injection, and read corresponding PFRU Telemetry log into user space.

For example:

./pfru_test -h
usage: pfru_test [OPTIONS]
 code injection:
  -l, --load
  -s, --stage
  -a, --activate
  -u, --update [stage and activate]
  -q, --query
  -d, --revid update
 telemetry:
  -G, --getloginfo
  -T, --type(0:execution, 1:history)
  -L, --level(0, 1, 2, 4)
  -R, --read
  -D, --revid log

./pfru_test -G
 log_level:4
 log_type:0
 log_revid:2
 max_data_size:65536
 chunk1_size:0
 chunk2_size:1401
 rollover_cnt:0
 reset_cnt:4

./pfru_test -q
 code injection image type:794bf8b2-6e7b-454e-885f-3fb9bb185402
 fw_version:0
 code_rt_version:1
 driver update image type:0e5f0b14-f849-7945-ad81-bc7b6d2bb245
 drv_rt_version:0
 drv_svn:0
 platform id:39214663-b1a8-4eaa-9024-f2bb53ea4723
 oem id:a36db54f-ea2a-e14e-b7c4-b5780e51ba3d

launch the update:
./pfru_test -l yours.cap -u -T 1 -L 4

Tested-by: Dou Shengnan <shengnanx.dou@intel.com>
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
v3: No change since v2.
v2: 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)
---
 include/uapi/linux/pfru.h                |   5 +
 tools/testing/selftests/Makefile         |   1 +
 tools/testing/selftests/pfru/Makefile    |   7 +
 tools/testing/selftests/pfru/config      |   2 +
 tools/testing/selftests/pfru/pfru_test.c | 328 +++++++++++++++++++++++
 5 files changed, 343 insertions(+)
 create mode 100644 tools/testing/selftests/pfru/Makefile
 create mode 100644 tools/testing/selftests/pfru/config
 create mode 100644 tools/testing/selftests/pfru/pfru_test.c

diff --git a/include/uapi/linux/pfru.h b/include/uapi/linux/pfru.h
index b04852602133..c8a98efd5eb6 100644
--- a/include/uapi/linux/pfru.h
+++ b/include/uapi/linux/pfru.h
@@ -8,7 +8,12 @@
 #define __PFRU_H__
 
 #include <linux/ioctl.h>
+#ifdef __KERNEL__
 #include <linux/uuid.h>
+#else
+#include <uuid/uuid.h>
+#include <linux/types.h>
+#endif
 
 #define PFRU_UUID		"ECF9533B-4A3C-4E89-939E-C77112601C6D"
 #define PFRU_CODE_INJ_UUID		"B2F84B79-7B6E-4E45-885F-3FB9BB185402"
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index c852eb40c4f7..9f1d7b5ea4a7 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -58,6 +58,7 @@ TARGETS += seccomp
 TARGETS += sgx
 TARGETS += sigaltstack
 TARGETS += size
+TARGETS += pfru
 TARGETS += sparc64
 TARGETS += splice
 TARGETS += static_keys
diff --git a/tools/testing/selftests/pfru/Makefile b/tools/testing/selftests/pfru/Makefile
new file mode 100644
index 000000000000..c61916ccf637
--- /dev/null
+++ b/tools/testing/selftests/pfru/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0+
+
+CFLAGS += -Wall -O2
+LDLIBS := -luuid
+
+TEST_GEN_PROGS := pfru_test
+include ../lib.mk
diff --git a/tools/testing/selftests/pfru/config b/tools/testing/selftests/pfru/config
new file mode 100644
index 000000000000..37f53609acbd
--- /dev/null
+++ b/tools/testing/selftests/pfru/config
@@ -0,0 +1,2 @@
+CONFIG_ACPI_PFRU=m
+CONFIG_ACPI_PFRU_TELEMETRY=m
diff --git a/tools/testing/selftests/pfru/pfru_test.c b/tools/testing/selftests/pfru/pfru_test.c
new file mode 100644
index 000000000000..b3799d73ab1c
--- /dev/null
+++ b/tools/testing/selftests/pfru/pfru_test.c
@@ -0,0 +1,328 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Tests Runtime Update/Telemetry (see Documentation/x86/pfru_update.rst)
+ */
+#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 "../../../../include/uapi/linux/pfru.h"
+
+#define MAX_LOG_SIZE 65536
+
+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;
+
+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 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)
+{
+	char *pathname;
+	int c;
+
+	pathname = strdup(argv[0]);
+	progname = basename(pathname);
+
+	while (1) {
+		int option_index = 0;
+
+		c = getopt_long(argc, argv, option_string,
+				long_options, &option_index);
+		if (c == -1)
+			break;
+		switch (c) {
+		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();
+			break;
+		default:
+			break;
+		}
+	}
+}
+
+void print_cap(struct pfru_update_cap_info *cap)
+{
+	char *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);
+
+	free(uuid);
+}
+
+int main(int argc, char *argv[])
+{
+	int fd_update, fd_log, fd_capsule;
+	struct pfru_telem_data_info data_info;
+	struct pfru_telem_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 test as root - Exiting.\n");
+		return 1;
+	}
+
+	parse_options(argc, argv);
+
+	fd_update = open("/dev/pfru/update", O_RDWR);
+	if (fd_update < 0) {
+		printf("PFRU device not supported - Quit...\n");
+		return 1;
+	}
+
+	if (query_cap) {
+		ret = read(fd_update, &cap, sizeof(cap));
+		if (ret == -1) {
+			perror("Read error.");
+			return 1;
+		}
+		print_cap(&cap);
+	}
+
+	fd_log = open("/dev/pfru/telemetry", O_RDWR);
+	if (fd_log < 0) {
+		printf("PFRU telemetry not supported. Skip...\n");
+		goto skip_log_set;
+	}
+
+	if (log_getinfo) {
+		ret = ioctl(fd_log, PFRU_LOG_IOC_GET_DATA_INFO, &data_info);
+		if (ret) {
+			perror("Get log data info failed.");
+			return 1;
+		}
+		ret = ioctl(fd_log, PFRU_LOG_IOC_GET_INFO, &info);
+		if (ret) {
+			perror("Get log info failed.");
+			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 (!pfru_valid_revid(log_revid)) {
+			printf("Invalid log revid %d\n",
+			       log_revid);
+		} else {
+			info.log_revid = log_revid;
+		}
+	}
+
+	ret = ioctl(fd_log, PFRU_LOG_IOC_SET_INFO, &info);
+	if (ret) {
+		perror("Log information set failed.(log_level, log_type, log_revid)");
+		return 1;
+	}
+
+skip_log_set:
+	if (set_revid) {
+		ret = ioctl(fd_update, PFRU_IOC_SET_REV, &revid);
+		if (ret) {
+			perror("pfru update revid set failed");
+			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...");
+			return 1;
+		}
+		if (fstat(fd_capsule, &st) < 0) {
+			perror("Can not fstat capsule file...");
+			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.");
+			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");
+			return 1;
+		}
+		munmap(addr_map_capsule, st.st_size);
+		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
+			return 1;
+		printf("Update finished, return %d\n", ret);
+	}
+
+	if (fd_log > 0 && log_read) {
+		log_buf = malloc(MAX_LOG_SIZE + 1);
+		if (!log_buf) {
+			perror("log_buf allocate failed.");
+			return 1;
+		}
+		ret = read(fd_log, log_buf, MAX_LOG_SIZE);
+		if (ret == -1) {
+			perror("Read error.");
+			return 1;
+		}
+		log_buf[ret] = '\0';
+		printf("%s\n", log_buf);
+		free(log_buf);
+	}
+
+	return 0;
+}
-- 
2.25.1

Re: [PATCH v3 3/5] drivers/acpi: Introduce Platform Firmware Runtime Update device driver

[Greg Kroah-Hartman]

On Fri, Sep 17, 2021 at 12:02:18AM +0800, Chen Yu wrote:
> Introduce the pfru_update driver which can be used for Platform Firmware
> Runtime code injection and driver update. 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.
> 
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>

Where is the userspace code that uses this ioctl and has tested it out
to verify it works properly?  A link to that in the changelog would be
great to have.

> +static void dump_update_result(struct pfru_updated_result *result)
> +{
> +	pr_debug("Update result:\n");
> +	pr_debug("Status:%d\n", result->status);
> +	pr_debug("Extended Status:%d\n", result->ext_status);
> +	pr_debug("Authentication Time Low:%lld\n", result->low_auth_time);
> +	pr_debug("Authentication Time High:%lld\n", result->high_auth_time);
> +	pr_debug("Execution Time Low:%lld\n", result->low_exec_time);
> +	pr_debug("Execution Time High:%lld\n", result->high_exec_time);

Why not dev_dbg()?  Same for all pr_* calls in this "driver".


> +static long pfru_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> +	void __user *p;
> +	int ret = 0, rev;
> +
> +	p = (void __user *)arg;
> +
> +	switch (cmd) {
> +	case PFRU_IOC_SET_REV:
> +		if (copy_from_user(&rev, p, sizeof(unsigned int)))
> +			return -EFAULT;
> +		if (!pfru_valid_revid(rev))
> +			return -EFAULT;
> +		pfru_dev->rev_id = rev;
> +		break;
> +	case PFRU_IOC_STAGE:
> +		ret = start_acpi_update(START_STAGE);
> +		break;
> +	case PFRU_IOC_ACTIVATE:
> +		ret = start_acpi_update(START_ACTIVATE);
> +		break;
> +	case PFRU_IOC_STAGE_ACTIVATE:
> +		ret = start_acpi_update(START_STAGE_ACTIVATE);
> +		break;
> +	default:
> +		ret = -ENOIOCTLCMD;

Wrong value :(



> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +#ifdef CONFIG_COMPAT
> +static long compat_pfru_ioctl(struct file *filep, unsigned int cmd,
> +			      unsigned long arg)
> +{
> +	return pfru_ioctl(filep, cmd, arg);
> +}
> +#endif

Why is this compat ioctl needed at all?

> +static struct miscdevice pfru_misc_dev = {
> +	.minor = MISC_DYNAMIC_MINOR,
> +	.name = "pfru_update",
> +	.nodename = "pfru/update",

Why is this in a subdirectory?  What requires this?  Why not just
"pfru"?

thanks,

greg k-h

Re: [PATCH v3 3/5] drivers/acpi: Introduce Platform Firmware Runtime Update device driver

[Chen Yu]

Hi Greg,
On Tue, Sep 21, 2021 at 05:59:05PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Sep 17, 2021 at 12:02:18AM +0800, Chen Yu wrote:
> > Introduce the pfru_update driver which can be used for Platform Firmware
> > Runtime code injection and driver update. 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.
> > 
> > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> 
> Where is the userspace code that uses this ioctl and has tested it out
> to verify it works properly?  A link to that in the changelog would be
> great to have.
> 
The patch [5/5] is a self testing tool to test the whole feature. I'll send a
new version and Cc you too.
> > +static void dump_update_result(struct pfru_updated_result *result)
> > +{
> > +	pr_debug("Update result:\n");
> > +	pr_debug("Status:%d\n", result->status);
> > +	pr_debug("Extended Status:%d\n", result->ext_status);
> > +	pr_debug("Authentication Time Low:%lld\n", result->low_auth_time);
> > +	pr_debug("Authentication Time High:%lld\n", result->high_auth_time);
> > +	pr_debug("Execution Time Low:%lld\n", result->low_exec_time);
> > +	pr_debug("Execution Time High:%lld\n", result->high_exec_time);
> 
> Why not dev_dbg()?  Same for all pr_* calls in this "driver".
> 
>
Ok, I'll switch to dev_dbg() in next version. 
> > +static long pfru_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > +{
> > +	void __user *p;
> > +	int ret = 0, rev;
> > +
> > +	p = (void __user *)arg;
> > +
> > +	switch (cmd) {
> > +	case PFRU_IOC_SET_REV:
> > +		if (copy_from_user(&rev, p, sizeof(unsigned int)))
> > +			return -EFAULT;
> > +		if (!pfru_valid_revid(rev))
> > +			return -EFAULT;
> > +		pfru_dev->rev_id = rev;
> > +		break;
> > +	case PFRU_IOC_STAGE:
> > +		ret = start_acpi_update(START_STAGE);
> > +		break;
> > +	case PFRU_IOC_ACTIVATE:
> > +		ret = start_acpi_update(START_ACTIVATE);
> > +		break;
> > +	case PFRU_IOC_STAGE_ACTIVATE:
> > +		ret = start_acpi_update(START_STAGE_ACTIVATE);
> > +		break;
> > +	default:
> > +		ret = -ENOIOCTLCMD;
> 
> Wrong value :(
Previously I thought that ENOIOCTLCMD stands for 'invalid ioctl command'.
After checking the lkml discussion, it seems that ENOIOCTLCMD should not
be returned to user space. ENOTTY might be more suitible if I understand
correctly.
http://lkml.iu.edu/hypermail/linux/kernel/0105.1/0734.html
> > +		break;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +#ifdef CONFIG_COMPAT
> > +static long compat_pfru_ioctl(struct file *filep, unsigned int cmd,
> > +			      unsigned long arg)
> > +{
> > +	return pfru_ioctl(filep, cmd, arg);
> > +}
> > +#endif
> 
> Why is this compat ioctl needed at all?
> 
We can not control if the user space tool would be compiled as 32bit.
But I realize that a compat_ptr() was missing. Will fix it in next version.
> > +static struct miscdevice pfru_misc_dev = {
> > +	.minor = MISC_DYNAMIC_MINOR,
> > +	.name = "pfru_update",
> > +	.nodename = "pfru/update",
> 
> Why is this in a subdirectory?  What requires this?  Why not just
> "pfru"?
> 
The pfru directory might be reused for pfru_telemetry device, whose driver
is in 4/5 patch, I'll Cc you with the whole patch set in next version.

Thanks,
Chenyu

Re: [PATCH v3 3/5] drivers/acpi: Introduce Platform Firmware Runtime Update device driver

[Greg Kroah-Hartman]

On Wed, Sep 22, 2021 at 05:04:42PM +0800, Chen Yu wrote:
> Hi Greg,
> On Tue, Sep 21, 2021 at 05:59:05PM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Sep 17, 2021 at 12:02:18AM +0800, Chen Yu wrote:
> > > Introduce the pfru_update driver which can be used for Platform Firmware
> > > Runtime code injection and driver update. 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.
> > > 
> > > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > 
> > Where is the userspace code that uses this ioctl and has tested it out
> > to verify it works properly?  A link to that in the changelog would be
> > great to have.
> > 
> The patch [5/5] is a self testing tool to test the whole feature. I'll send a
> new version and Cc you too.

That tests it, but does not answer the question of who will actually use
this.  What userspace tool needs this new api?

> > > +static void dump_update_result(struct pfru_updated_result *result)
> > > +{
> > > +	pr_debug("Update result:\n");
> > > +	pr_debug("Status:%d\n", result->status);
> > > +	pr_debug("Extended Status:%d\n", result->ext_status);
> > > +	pr_debug("Authentication Time Low:%lld\n", result->low_auth_time);
> > > +	pr_debug("Authentication Time High:%lld\n", result->high_auth_time);
> > > +	pr_debug("Execution Time Low:%lld\n", result->low_exec_time);
> > > +	pr_debug("Execution Time High:%lld\n", result->high_exec_time);
> > 
> > Why not dev_dbg()?  Same for all pr_* calls in this "driver".
> > 
> >
> Ok, I'll switch to dev_dbg() in next version. 
> > > +static long pfru_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > > +{
> > > +	void __user *p;
> > > +	int ret = 0, rev;
> > > +
> > > +	p = (void __user *)arg;
> > > +
> > > +	switch (cmd) {
> > > +	case PFRU_IOC_SET_REV:
> > > +		if (copy_from_user(&rev, p, sizeof(unsigned int)))
> > > +			return -EFAULT;
> > > +		if (!pfru_valid_revid(rev))
> > > +			return -EFAULT;
> > > +		pfru_dev->rev_id = rev;
> > > +		break;
> > > +	case PFRU_IOC_STAGE:
> > > +		ret = start_acpi_update(START_STAGE);
> > > +		break;
> > > +	case PFRU_IOC_ACTIVATE:
> > > +		ret = start_acpi_update(START_ACTIVATE);
> > > +		break;
> > > +	case PFRU_IOC_STAGE_ACTIVATE:
> > > +		ret = start_acpi_update(START_STAGE_ACTIVATE);
> > > +		break;
> > > +	default:
> > > +		ret = -ENOIOCTLCMD;
> > 
> > Wrong value :(
> Previously I thought that ENOIOCTLCMD stands for 'invalid ioctl command'.
> After checking the lkml discussion, it seems that ENOIOCTLCMD should not
> be returned to user space. ENOTTY might be more suitible if I understand
> correctly.
> http://lkml.iu.edu/hypermail/linux/kernel/0105.1/0734.html

Yes, ENOTTY is correct.


> > > +		break;
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +#ifdef CONFIG_COMPAT
> > > +static long compat_pfru_ioctl(struct file *filep, unsigned int cmd,
> > > +			      unsigned long arg)
> > > +{
> > > +	return pfru_ioctl(filep, cmd, arg);
> > > +}
> > > +#endif
> > 
> > Why is this compat ioctl needed at all?
> > 
> We can not control if the user space tool would be compiled as 32bit.

Then create your ioctl so that a compat ioctl is not needed at all.
There is no need to ever use this for new ioctl commands these days.

> But I realize that a compat_ptr() was missing. Will fix it in next version.
> > > +static struct miscdevice pfru_misc_dev = {
> > > +	.minor = MISC_DYNAMIC_MINOR,
> > > +	.name = "pfru_update",
> > > +	.nodename = "pfru/update",
> > 
> > Why is this in a subdirectory?  What requires this?  Why not just
> > "pfru"?
> > 
> The pfru directory might be reused for pfru_telemetry device, whose driver
> is in 4/5 patch, I'll Cc you with the whole patch set in next version.

"might be" is not a valid reason.  Why does this simple driver deserve a
whole /dev/ subdirectory?

thanks,

greg k-h

Re: [PATCH v3 3/5] drivers/acpi: Introduce Platform Firmware Runtime Update device driver

[Chen Yu]

On Wed, Sep 22, 2021 at 11:10:02AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Sep 22, 2021 at 05:04:42PM +0800, Chen Yu wrote:
> > Hi Greg,
> > On Tue, Sep 21, 2021 at 05:59:05PM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, Sep 17, 2021 at 12:02:18AM +0800, Chen Yu wrote:
> > > > Introduce the pfru_update driver which can be used for Platform Firmware
> > > > Runtime code injection and driver update. 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.
> > > > 
> > > > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > > 
> > > Where is the userspace code that uses this ioctl and has tested it out
> > > to verify it works properly?  A link to that in the changelog would be
> > > great to have.
> > > 
> > The patch [5/5] is a self testing tool to test the whole feature. I'll send a
> > new version and Cc you too.
> 
> That tests it, but does not answer the question of who will actually use
> this.  What userspace tool needs this new api?
>
One end user is the cloud user. Currently there is no dedicated userspace
tool developed to use this feature AFAIK. It was expected that the end users
could refer to the self test tool to customize their tools. I'm not sure if
this is the proper way to propose the feature, may I have your suggestion on
this, should I create a separate git repository for this tool, or put it in
tools/selftestings as it is now?
> > > > +static void dump_update_result(struct pfru_updated_result *result)
> > > > +{
> > > > +	pr_debug("Update result:\n");
> > > > +	pr_debug("Status:%d\n", result->status);
> > > > +	pr_debug("Extended Status:%d\n", result->ext_status);
> > > > +	pr_debug("Authentication Time Low:%lld\n", result->low_auth_time);
> > > > +	pr_debug("Authentication Time High:%lld\n", result->high_auth_time);
> > > > +	pr_debug("Execution Time Low:%lld\n", result->low_exec_time);
> > > > +	pr_debug("Execution Time High:%lld\n", result->high_exec_time);
> > > 
> > > Why not dev_dbg()?  Same for all pr_* calls in this "driver".
> > > 
> > >
> > Ok, I'll switch to dev_dbg() in next version. 
> > > > +static long pfru_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > > > +{
> > > > +	void __user *p;
> > > > +	int ret = 0, rev;
> > > > +
> > > > +	p = (void __user *)arg;
> > > > +
> > > > +	switch (cmd) {
> > > > +	case PFRU_IOC_SET_REV:
> > > > +		if (copy_from_user(&rev, p, sizeof(unsigned int)))
> > > > +			return -EFAULT;
> > > > +		if (!pfru_valid_revid(rev))
> > > > +			return -EFAULT;
> > > > +		pfru_dev->rev_id = rev;
> > > > +		break;
> > > > +	case PFRU_IOC_STAGE:
> > > > +		ret = start_acpi_update(START_STAGE);
> > > > +		break;
> > > > +	case PFRU_IOC_ACTIVATE:
> > > > +		ret = start_acpi_update(START_ACTIVATE);
> > > > +		break;
> > > > +	case PFRU_IOC_STAGE_ACTIVATE:
> > > > +		ret = start_acpi_update(START_STAGE_ACTIVATE);
> > > > +		break;
> > > > +	default:
> > > > +		ret = -ENOIOCTLCMD;
> > > 
> > > Wrong value :(
> > Previously I thought that ENOIOCTLCMD stands for 'invalid ioctl command'.
> > After checking the lkml discussion, it seems that ENOIOCTLCMD should not
> > be returned to user space. ENOTTY might be more suitible if I understand
> > correctly.
> > http://lkml.iu.edu/hypermail/linux/kernel/0105.1/0734.html
> 
> Yes, ENOTTY is correct.
> 
>
> > > > +		break;
> > > > +	}
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +#ifdef CONFIG_COMPAT
> > > > +static long compat_pfru_ioctl(struct file *filep, unsigned int cmd,
> > > > +			      unsigned long arg)
> > > > +{
> > > > +	return pfru_ioctl(filep, cmd, arg);
> > > > +}
> > > > +#endif
> > > 
> > > Why is this compat ioctl needed at all?
> > > 
> > We can not control if the user space tool would be compiled as 32bit.
> 
> Then create your ioctl so that a compat ioctl is not needed at all.
> There is no need to ever use this for new ioctl commands these days.
> 
After checking the code, it seems that this driver only needs to deal with
unlocked_ioctl() without touching compat_ioctl(). Because even if the user
space is 32bit, the compat_ioctl() would invoke unlocked_ioctl() first
with a parameter of compat_ptr(arg). It should be safe to remove the compat_ioctl
hook in this driver.
> > But I realize that a compat_ptr() was missing. Will fix it in next version.
> > > > +static struct miscdevice pfru_misc_dev = {
> > > > +	.minor = MISC_DYNAMIC_MINOR,
> > > > +	.name = "pfru_update",
> > > > +	.nodename = "pfru/update",
> > > 
> > > Why is this in a subdirectory?  What requires this?  Why not just
> > > "pfru"?
> > > 
> > The pfru directory might be reused for pfru_telemetry device, whose driver
> > is in 4/5 patch, I'll Cc you with the whole patch set in next version.
> 
> "might be" is not a valid reason.  Why does this simple driver deserve a
> whole /dev/ subdirectory?
> 
There are pfru_update and pfru_telemetry in the patch, and there is plan to
add a pfru_prm device in the future, which stands for "Platform Runtime Mechanism".
I'll move them to /dev/ in next version.

Thanks,
Chenyu

Re: [PATCH v3 3/5] drivers/acpi: Introduce Platform Firmware Runtime Update device driver

[Greg Kroah-Hartman]

On Thu, Sep 23, 2021 at 12:33:21AM +0800, Chen Yu wrote:
> On Wed, Sep 22, 2021 at 11:10:02AM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Sep 22, 2021 at 05:04:42PM +0800, Chen Yu wrote:
> > > Hi Greg,
> > > On Tue, Sep 21, 2021 at 05:59:05PM +0200, Greg Kroah-Hartman wrote:
> > > > On Fri, Sep 17, 2021 at 12:02:18AM +0800, Chen Yu wrote:
> > > > > Introduce the pfru_update driver which can be used for Platform Firmware
> > > > > Runtime code injection and driver update. 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.
> > > > > 
> > > > > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > > > 
> > > > Where is the userspace code that uses this ioctl and has tested it out
> > > > to verify it works properly?  A link to that in the changelog would be
> > > > great to have.
> > > > 
> > > The patch [5/5] is a self testing tool to test the whole feature. I'll send a
> > > new version and Cc you too.
> > 
> > That tests it, but does not answer the question of who will actually use
> > this.  What userspace tool needs this new api?
> >
> One end user is the cloud user.

What exactly do you mean by "cloud user"?

> Currently there is no dedicated userspace tool developed to use this
> feature AFAIK.

Wonderful, then it is not needed to be added to the kernel :)

> It was expected that the end users
> could refer to the self test tool to customize their tools. I'm not sure if
> this is the proper way to propose the feature, may I have your suggestion on
> this, should I create a separate git repository for this tool, or put it in
> tools/selftestings as it is now?

No, do not add this to the kernel unless you have a real need and user
for this.


> > > > > +static struct miscdevice pfru_misc_dev = {
> > > > > +	.minor = MISC_DYNAMIC_MINOR,
> > > > > +	.name = "pfru_update",
> > > > > +	.nodename = "pfru/update",
> > > > 
> > > > Why is this in a subdirectory?  What requires this?  Why not just
> > > > "pfru"?
> > > > 
> > > The pfru directory might be reused for pfru_telemetry device, whose driver
> > > is in 4/5 patch, I'll Cc you with the whole patch set in next version.
> > 
> > "might be" is not a valid reason.  Why does this simple driver deserve a
> > whole /dev/ subdirectory?
> > 
> There are pfru_update and pfru_telemetry in the patch, and there is plan to
> add a pfru_prm device in the future, which stands for "Platform Runtime Mechanism".
> I'll move them to /dev/ in next version.

That is a very generic name for a very platform specific and arch
specific interface.  As this is an ACPI interface, why not use that name
prefix?

thanks,

greg k-h

Re: [PATCH v3 1/5] Documentation: Introduce Platform Firmware Runtime Update documentation

[Rafael J. Wysocki]

On Thu, Sep 16, 2021 at 5:53 PM Chen Yu <yu.c.chen@intel.com> wrote:
>
> Add the Platform Firmware Runtime Update/Telemetry documentation.
>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---
> v3: No change since v2.
> 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).
> ---
>  Documentation/x86/index.rst |   1 +
>  Documentation/x86/pfru.rst  | 100 ++++++++++++++++++++++++++++++++++++

The location of this file is somewhat questionable to me.

The interface described by it talks to the platform firmware via ACPI
methods, there's nothing x86-specific in it.

Besides, it is hard to say who's the target audience of this document.

>  2 files changed, 101 insertions(+)
>  create mode 100644 Documentation/x86/pfru.rst
>
> diff --git a/Documentation/x86/index.rst b/Documentation/x86/index.rst
> index 383048396336..3791fabf964c 100644
> --- a/Documentation/x86/index.rst
> +++ b/Documentation/x86/index.rst
> @@ -37,3 +37,4 @@ x86-specific Documentation
>     sgx
>     features
>     elf_auxvec
> +   pfru
> diff --git a/Documentation/x86/pfru.rst b/Documentation/x86/pfru.rst
> new file mode 100644
> index 000000000000..29fe0e518e6d
> --- /dev/null
> +++ b/Documentation/x86/pfru.rst
> @@ -0,0 +1,100 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +========================================================
> +The Linux Platform Firmware Runtime Update and Telemetry
> +========================================================

The "The Linux" piece doesn't add any value here IMO.

> +
> +According to the specification of `Management Mode Firmware Runtime Update
> +<https://uefi.org/sites/default/files/resources/Intel_MM_OS_Interface_Spec_
> +Rev100.pdf>`_, certain computing systems require high Service Level Agreements
> +(SLAs) where system reboot fewer firmware updates are required to deploy
> +firmware changes to address bug fixes, security updates and to debug and root
> +cause issues. This technology is called Intel Seamless Update. The management
> +mode (MM), UEFI runtime services and ACPI services handle most of the system
> +runtime functions. Changing the MM code execution during runtime is called MM
> +Runtime Update. Since the "MM" acronyms might be misunderstood as "Memory
> +Management", this driver uses the name of "Platform Firmware Runtime Update"
> +(PFRU).

In the above paragraph it is not necessary to mention the "high SLAs"
at all.  IMO it would be sufficient to say something like "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."

> +
> +PFRU provides the following facilities: Performs a runtime firmware driver
> +update and activate. Ability to inject firmware code at runtime, for dynamic
> +instrumentation. PFRU Telemetry is a service which allows Runtime Update
> +handler to produce telemetry data to upper layer OS consumer at runtime. The
> +OS provides interfaces to let the users query the telemetry data via read
> +operations. The specification specifies the interface and recommended policy
> +to extract the data, the format and use are left to individual OEM's and BIOS
> +implementations on what that data represents.

I'd just say "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."  And the part about how the PFRU telemetry is
supposed to work belongs to the description of that part.

> +
> +PFRU interfaces
> +===============

Because this doesn't really specify the kernel ABI, the part below
belongs to the man page of the tool that you're going to provide.

In order to document the ABI, please add a document to
Documentation/ABI/testing/ along the lines of
Documentation/ABI/testing/rtc-cdev

> +
> +The user space tool manipulates on /dev/pfru/update for code injection and
> +driver update. PFRU stands for Platform Firmware Runtime Update, and the
> +/dev/pfru directory might be reserved for future usage::
> +
> + 1. mmap the capsule file.
> +    fd_capsule = open("capsule.cap", O_RDONLY);
> +    fstat(fd_capsule, &stat);
> +    addr = mmap(0, stat.st_size, PROT_READ, fd_capsule);
> +
> + 2. Get the capability information(version control, etc) from BIOS via
> +    read() and do sanity check in user space.
> +    fd_update = open("/dev/pfru/update", O_RDWR);
> +    read(fd_update, &cap, sizeof(cap));
> +    sanity_check(&cap);
> +
> + 3. Write the capsule file to runtime update communication buffer.
> +    kernel might return error if capsule file size is longer than
> +    communication buffer.
> +    write(fd_update, addr, stat.st_size);
> +
> + 4. Stage the code injection.
> +    ioctl(fd_update, PFRU_IOC_STATGE);
> +
> + 5. Activate the code injection.
> +    ioctl(fd_update, PFRU_IOC_ACTIVATE);
> +
> + 6. Stage and activate the code injection.
> +    ioctl(fd_update, PFRU_IOC_STAGE_ACTIVATE);
> +
> +    PFRU_IOC_STATGE: Stage a capsule image from communication buffer
> +    and perform authentication.
> +    [PFRU_IOC_ACTIVATE] Activate a previous staged capsule image.
> +    [PFRU_IOC_STAGE_ACTIVATE] Perform both stage and activation actions.
> +
> +
> +PFRU Telemetry interfaces
> +=========================
> +
> +The user space tool manipulates on /dev/pfru/telemetry for PFRU telemetry log::
> +
> + 1. Open telemetry device
> +    fd_log = open("/dev/pfru/telemetry", O_RDWR);
> +
> + 2. Get log level, log type, revision_id via one ioctl invoke
> +    ioctl(fd_log, PFRU_IOC_GET_LOG_INFO, &info);
> +
> + 3. Set log level, log type, revision_id
> +    ioctl(fd_log, PFRU_IOC_SET_LOG_INFO, &info);
> +
> + 4. ioctl(fd_log, PFRU_IOC_GET_DATA_INFO, &data_info);
> +    Query the information of PFRU telemetry log buffer. The user is
> +    responsible for parsing the result per the specification.
> +
> + 5. Read the telemetry data:
> +    read(fd_log, buf, data_info.size);
> +
> +Please refer to tools/testing/selftests/pfru/pfru_test.c for detail.
> +
> +According to the specification of `Management Mode Firmware Runtime Update

I'd say "According to the ... specification," (ie. without the "of").

> +<https://uefi.org/sites/default/files/resources/Intel_MM_OS_Interface_Spec_
> +Rev100.pdf>`_, the telemetry buffer is a wrap around buffer. If the telemetry
> +buffer gets full, most recent log data will overwrite old log data. Besides,
> +it is required in the spec that the read of telemetry should support both full
> +data retrieval and delta telemetry data retrieval. Since this requirement is
> +more likely a policy we leave this implementation in user space. That is to
> +say, it is recommended for the user to double-read the telemetry parameters
> +such as chunk1_size, chunk2_size, rollover_cnt in data_info structure to make

It is not clear what you mean by "double read".  It looks like you
wanted to say that it was recommended to call the
PFRU_IOC_GET_DATA_INFO ioctl() before a "continuation" read in order
to check if more data was available.

> +sure that there is no more data appended while the user is reading the buffer.
> +Besides, only after the runtime update has been run at least once, the telemetry
> +log would have valid data, otherwise errno code of EBUSY would be returned.
> --

Re: [PATCH v3 2/5] efi: Introduce EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER and corresponding structures

[Rafael J. Wysocki]

On Thu, Sep 16, 2021 at 5:54 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. The EFI_CAPSULE_HEADER has been defined in the
> kernel, however the rest are not, thus introduce corresponding UEFI
> structures accordingly.
>
> The reason why efi_manage_capsule_header_t and
> efi_manage_capsule_image_header_t are packedi might be that:
> According to the uefi spec,
> [Figure 23-6 Firmware Management and Firmware Image Management headers]
> EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER is located at the lowest offset
> within the body of the capsule. And this structure is designed to be
> unaligned to save space, because in this way the adjacent drivers and
> binary payload elements could start on byte boundary with no padding.
> And the EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER is at the head of
> each payload, so packing this structure also makes room for more data.

IMO it would be sufficient to say that both
EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER and
EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER need not be aligned and
so the corresponding data types should be packed.

>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---
>  include/linux/efi.h | 50 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
>
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 6b5d36babfcc..19ff834e1388 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -148,6 +148,56 @@ typedef struct {
>         u32 imagesize;
>  } efi_capsule_header_t;
>
> +#pragma pack(1)
> +
> +/* EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER */
> +typedef struct {
> +       u32     ver;
> +       u16     emb_drv_cnt;
> +       u16     payload_cnt;
> +       /*
> +        * Variable array indicated by number of
> +        * (emb_drv_cnt + payload_cnt)
> +        */
> +       u64     offset_list[];
> +} efi_manage_capsule_header_t;
> +
> +/* EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER */
> +typedef struct {
> +       u32     ver;
> +       guid_t  image_type_id;
> +       u8      image_index;
> +       u8      reserved_bytes[3];
> +       u32     image_size;
> +       u32     vendor_code_size;
> +       /* ver = 2. */
> +       u64     hw_ins;
> +       /* ver = v3. */
> +       u64     capsule_support;
> +} efi_manage_capsule_image_header_t;
> +
> +#pragma pack()
> +
> +/* WIN_CERTIFICATE */
> +typedef struct {
> +       u32     len;
> +       u16     rev;
> +       u16     cert_type;
> +} win_cert_t;
> +
> +/* WIN_CERTIFICATE_UEFI_GUID */
> +typedef struct {
> +       win_cert_t      hdr;
> +       guid_t          cert_type;
> +       u8              cert_data[];
> +} win_cert_uefi_guid_t;
> +
> +/* EFI_FIRMWARE_IMAGE_AUTHENTICATIO */
> +typedef struct {
> +       u64                             mon_count;
> +       win_cert_uefi_guid_t            auth_info;
> +} efi_image_auth_t;
> +
>  /*
>   * EFI capsule flags
>   */
> --
> 2.25.1
>

Re: [PATCH v3 3/5] drivers/acpi: Introduce Platform Firmware Runtime Update device driver

[Rafael J. Wysocki]

On Wed, Sep 22, 2021 at 7:28 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Sep 23, 2021 at 12:33:21AM +0800, Chen Yu wrote:
> > On Wed, Sep 22, 2021 at 11:10:02AM +0200, Greg Kroah-Hartman wrote:
> > > On Wed, Sep 22, 2021 at 05:04:42PM +0800, Chen Yu wrote:
> > > > Hi Greg,
> > > > On Tue, Sep 21, 2021 at 05:59:05PM +0200, Greg Kroah-Hartman wrote:
> > > > > On Fri, Sep 17, 2021 at 12:02:18AM +0800, Chen Yu wrote:
> > > > > > Introduce the pfru_update driver which can be used for Platform Firmware
> > > > > > Runtime code injection and driver update. 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.
> > > > > >
> > > > > > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > > > >
> > > > > Where is the userspace code that uses this ioctl and has tested it out
> > > > > to verify it works properly?  A link to that in the changelog would be
> > > > > great to have.
> > > > >
> > > > The patch [5/5] is a self testing tool to test the whole feature. I'll send a
> > > > new version and Cc you too.
> > >
> > > That tests it, but does not answer the question of who will actually use
> > > this.  What userspace tool needs this new api?
> > >
> > One end user is the cloud user.
>
> What exactly do you mean by "cloud user"?
>
> > Currently there is no dedicated userspace tool developed to use this
> > feature AFAIK.
>
> Wonderful, then it is not needed to be added to the kernel :)
>
> > It was expected that the end users
> > could refer to the self test tool to customize their tools. I'm not sure if
> > this is the proper way to propose the feature, may I have your suggestion on
> > this, should I create a separate git repository for this tool, or put it in
> > tools/selftestings as it is now?
>
> No, do not add this to the kernel unless you have a real need and user
> for this.
>
>
> > > > > > +static struct miscdevice pfru_misc_dev = {
> > > > > > +     .minor = MISC_DYNAMIC_MINOR,
> > > > > > +     .name = "pfru_update",
> > > > > > +     .nodename = "pfru/update",
> > > > >
> > > > > Why is this in a subdirectory?  What requires this?  Why not just
> > > > > "pfru"?
> > > > >
> > > > The pfru directory might be reused for pfru_telemetry device, whose driver
> > > > is in 4/5 patch, I'll Cc you with the whole patch set in next version.
> > >
> > > "might be" is not a valid reason.  Why does this simple driver deserve a
> > > whole /dev/ subdirectory?
> > >
> > There are pfru_update and pfru_telemetry in the patch, and there is plan to
> > add a pfru_prm device in the future, which stands for "Platform Runtime Mechanism".
> > I'll move them to /dev/ in next version.
>
> That is a very generic name for a very platform specific and arch
> specific interface.  As this is an ACPI interface, why not use that name
> prefix?

It is not supposed to be either arch-specific or platform-specific.
The spec is hosted by the UEFI Forum and it is fairly generic IIUC.
In principle, it could be used to update any kind of platform firmware
possible to update without system restart.

That said, the I/F to the platform firmware is based on ACPI methods,
so "acpi_" would be a reasonable prefix choice.

Re: [PATCH v3 3/5] drivers/acpi: Introduce Platform Firmware Runtime Update device driver

[Rafael J. Wysocki]

On Thu, Sep 16, 2021 at 5:56 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. 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.
>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---
> 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                     |  15 +
>  drivers/acpi/pfru/Makefile                    |   2 +
>  drivers/acpi/pfru/pfru_update.c               | 567 ++++++++++++++++++
>  include/uapi/linux/pfru.h                     | 101 ++++
>  7 files changed, 688 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 2e8134059c87..6e5a82fff408 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -365,6 +365,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-1F  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 1da360c51d66..1d8d2e2cefac 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..3f31b7d95f3b
> --- /dev/null
> +++ b/drivers/acpi/pfru/Kconfig
> @@ -0,0 +1,15 @@
> +# 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.
> +
> +         For more information, see:
> +         <file:Documentation/x86/pfru_update.rst>
> +
> +         To compile this driver as a module, choose M here:
> +         the module will be called pfru_update.
> 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..cd2039a195c9
> --- /dev/null
> +++ b/drivers/acpi/pfru/pfru_update.c
> @@ -0,0 +1,567 @@
> +// 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/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>
> +
> +enum cap_index {
> +       CAP_STATUS_IDX,
> +       CAP_UPDATE_IDX,
> +       CAP_CODE_TYPE_IDX,
> +       CAP_FW_VER_IDX,
> +       CAP_CODE_RT_VER_IDX,
> +       CAP_DRV_TYPE_IDX,
> +       CAP_DRV_RT_VER_IDX,
> +       CAP_DRV_SVN_IDX,
> +       CAP_PLAT_ID_IDX,
> +       CAP_OEM_ID_IDX,
> +       CAP_OEM_INFO_IDX,
> +};
> +
> +enum buf_index {
> +       BUF_STATUS_IDX,
> +       BUF_EXT_STATUS_IDX,
> +       BUF_ADDR_LOW_IDX,
> +       BUF_ADDR_HI_IDX,
> +       BUF_SIZE_IDX,
> +};
> +
> +enum update_index {
> +       UPDATE_STATUS_IDX,
> +       UPDATE_EXT_STATUS_IDX,
> +       UPDATE_AUTH_TIME_LOW_IDX,
> +       UPDATE_AUTH_TIME_HI_IDX,
> +       UPDATE_EXEC_TIME_LOW_IDX,
> +       UPDATE_EXEC_TIME_HI_IDX,
> +};
> +
> +struct pfru_device {
> +       guid_t uuid, code_uuid, drv_uuid;
> +       int rev_id;
> +       struct device *dev;
> +};
> +
> +/*
> + * There would be only one instance of pfru_device.
> + */
> +static struct pfru_device *pfru_dev;
> +
> +static bool valid_cap_type(int idx, union acpi_object *obj)
> +{
> +       acpi_object_type type = obj->type;
> +
> +       if (idx == CAP_STATUS_IDX || idx == CAP_UPDATE_IDX ||
> +           idx == CAP_FW_VER_IDX || idx == CAP_CODE_RT_VER_IDX ||
> +           idx == CAP_DRV_RT_VER_IDX || idx == CAP_DRV_SVN_IDX)
> +               return type == ACPI_TYPE_INTEGER;
> +       else if (idx == CAP_CODE_TYPE_IDX || idx == CAP_DRV_TYPE_IDX ||
> +                idx == CAP_PLAT_ID_IDX || idx == CAP_OEM_ID_IDX ||
> +                idx == CAP_OEM_INFO_IDX)
> +               return type == ACPI_TYPE_BUFFER;
> +       else
> +               return false;
> +}

IMO, this is way overdesigned.  It is not necessary to do all of these
checks for every element of the package in query_capability().

> +
> +static int query_capability(struct pfru_update_cap_info *cap)
> +{
> +       union acpi_object *out_obj;
> +       acpi_handle handle;
> +       int i, ret = -EINVAL;
> +
> +       handle = ACPI_HANDLE(pfru_dev->dev);
> +       out_obj = acpi_evaluate_dsm_typed(handle, &pfru_dev->uuid,
> +                                         pfru_dev->rev_id, FUNC_QUERY_UPDATE_CAP,
> +                                         NULL, ACPI_TYPE_PACKAGE);
> +       if (!out_obj)
> +               return -EINVAL;
> +
> +       for (i = 0; i < out_obj->package.count; i++) {

It is not very useful to do a loop if there is a switch () on the
control variable in every step.

> +               union acpi_object *obj = &out_obj->package.elements[i];
> +
> +               if (!valid_cap_type(i, obj))
> +                       goto free_acpi_buffer;
> +
> +               switch (i) {
> +               case CAP_STATUS_IDX:
> +                       cap->status = obj->integer.value;
> +                       break;

The above can be written as

if (out_obj->package.elements[CAP_STATUS_IDX].type != ACPI_TYPE_INTEGER)
        goto free_acpi_buffer;

cap->status = out_obj->package.elements[CAP_STATUS_IDX].integer.value;

and analogously for all of the other index values.

But check the number of elements upfront.

> +               case CAP_UPDATE_IDX:
> +                       cap->update_cap = obj->integer.value;
> +                       break;
> +               case CAP_CODE_TYPE_IDX:
> +                       memcpy(&cap->code_type, obj->buffer.pointer,
> +                              obj->buffer.length);
> +                       break;
> +               case CAP_FW_VER_IDX:
> +                       cap->fw_version = obj->integer.value;
> +                       break;
> +               case CAP_CODE_RT_VER_IDX:
> +                       cap->code_rt_version = obj->integer.value;
> +                       break;
> +               case CAP_DRV_TYPE_IDX:
> +                       memcpy(&cap->drv_type, obj->buffer.pointer,
> +                              obj->buffer.length);
> +                       break;
> +               case CAP_DRV_RT_VER_IDX:
> +                       cap->drv_rt_version = obj->integer.value;
> +                       break;
> +               case CAP_DRV_SVN_IDX:
> +                       cap->drv_svn = obj->integer.value;
> +                       break;
> +               case CAP_PLAT_ID_IDX:
> +                       memcpy(&cap->platform_id, obj->buffer.pointer,
> +                              obj->buffer.length);
> +                       break;
> +               case CAP_OEM_ID_IDX:
> +                       memcpy(&cap->oem_id, obj->buffer.pointer,
> +                              obj->buffer.length);
> +                       break;
> +               case CAP_OEM_INFO_IDX:
> +                       /*vendor specific data*/
> +                       break;
> +               default:
> +                       pr_err("Incorrect format of Update Capability.\n");

Why pr_err() and not dev_dbg()?

Besides, it looks like you're going to fail if the package has more
elements than expected, but is this really a big deal?

Moreover, what if the number of package elements is too small?

> +                       goto free_acpi_buffer;
> +               }
> +       }
> +       ret = 0;
> +
> +free_acpi_buffer:
> +       ACPI_FREE(out_obj);
> +
> +       return ret;
> +}
> +
> +static int query_buffer(struct pfru_com_buf_info *info)
> +{
> +       union acpi_object *out_obj;
> +       acpi_handle handle;
> +       int i, ret = -EINVAL;
> +
> +       handle = ACPI_HANDLE(pfru_dev->dev);
> +       out_obj = acpi_evaluate_dsm_typed(handle, &pfru_dev->uuid,
> +                                         pfru_dev->rev_id, FUNC_QUERY_BUF,
> +                                         NULL, ACPI_TYPE_PACKAGE);
> +       if (!out_obj)
> +               return -EINVAL;
> +
> +       for (i = 0; i < out_obj->package.count; i++) {

Again, what is the benefit from doing this loop?

> +               union acpi_object *obj = &out_obj->package.elements[i];
> +
> +               if (obj->type != ACPI_TYPE_INTEGER)
> +                       goto free_acpi_buffer;
> +
> +               switch (i) {
> +               case BUF_STATUS_IDX:
> +                       info->status = obj->integer.value;
> +                       break;
> +               case BUF_EXT_STATUS_IDX:
> +                       info->ext_status = obj->integer.value;
> +                       break;
> +               case BUF_ADDR_LOW_IDX:
> +                       info->addr_lo = obj->integer.value;
> +                       break;
> +               case BUF_ADDR_HI_IDX:
> +                       info->addr_hi = obj->integer.value;
> +                       break;
> +               case BUF_SIZE_IDX:
> +                       info->buf_size = obj->integer.value;
> +                       break;
> +               default:
> +                       pr_err("Incorrect format of Communication Buffer.\n");
> +                       goto free_acpi_buffer;
> +               }
> +       }
> +       ret = 0;
> +
> +free_acpi_buffer:
> +       ACPI_FREE(out_obj);
> +
> +       return ret;
> +}
> +
> +static int get_image_type(efi_manage_capsule_image_header_t *img_hdr,
> +                         int *type)
> +{
> +       guid_t *image_type_id;
> +
> +       /* check whether this is a code injection or driver update */
> +       image_type_id = &img_hdr->image_type_id;

Anything wrong with doing this assignment as initialization?

> +       if (guid_equal(image_type_id, &pfru_dev->code_uuid))
> +               *type = CODE_INJECT_TYPE;
> +       else if (guid_equal(image_type_id, &pfru_dev->drv_uuid))
> +               *type = DRIVER_UPDATE_TYPE;

And what would be wrong with returning the type or a negative error code?

> +       else
> +               return -EINVAL;
> +
> +       return 0;
> +}
> +
> +/*
> + * 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.

Either turn this into a kerneldoc comment or put it inside the function.

> + */
> +static int adjust_efi_size(efi_manage_capsule_image_header_t *img_hdr,
> +                          int *size)
> +{
> +       int tmp_size = *size;
> +
> +       tmp_size += sizeof(efi_manage_capsule_image_header_t);
> +       switch (img_hdr->ver) {
> +       case 1:
> +               tmp_size -= 2 * sizeof(u64);
> +               break;
> +       case 2:
> +               tmp_size -= sizeof(u64);
> +               break;
> +       default:
> +               /* only support version 1 and 2 */
> +               return -EINVAL;
> +       }
> +       *size = tmp_size;

Why not simply return the size or a negative error code?

> +
> +       return 0;
> +}
> +
> +/*
> + * Sanity check if the capsule image has a newer version than current one.
> + * Return: true if it is valid, false otherwise.

Would it hurt if this was a proper kerneldoc comment?

> + */
> +static bool valid_version(const void *data, struct pfru_update_cap_info *cap)
> +{
> +       struct pfru_payload_hdr *payload_hdr;
> +       efi_capsule_header_t *cap_hdr;
> +       efi_manage_capsule_header_t *m_hdr;
> +       efi_manage_capsule_image_header_t *m_img_hdr;
> +       efi_image_auth_t *auth;
> +       int type, size, ret;
> +
> +       cap_hdr = (efi_capsule_header_t *)data;
> +       size = cap_hdr->headersize;
> +       m_hdr = (efi_manage_capsule_header_t *)(data + size);
> +       /*
> +        * Current data structure size plus variable array indicated
> +        * by number of (emb_drv_cnt + payload_cnt)
> +        */
> +       size += sizeof(efi_manage_capsule_header_t) +
> +                     (m_hdr->emb_drv_cnt + m_hdr->payload_cnt) * sizeof(u64);
> +       m_img_hdr = (efi_manage_capsule_image_header_t *)(data + size);
> +
> +       ret = get_image_type(m_img_hdr, &type);
> +       if (ret)
> +               return false;
> +
> +       ret = adjust_efi_size(m_img_hdr, &size);
> +       if (ret)
> +               return false;
> +
> +       auth = (efi_image_auth_t *)(data + size);
> +       size += sizeof(u64) + auth->auth_info.hdr.len;
> +       payload_hdr = (struct pfru_payload_hdr *)(data + size);
> +
> +       /* Finally, compare the version. */
> +       if (type == 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)
> +{
> +       pr_debug("Update result:\n");
> +       pr_debug("Status:%d\n", result->status);
> +       pr_debug("Extended Status:%d\n", result->ext_status);
> +       pr_debug("Authentication Time Low:%lld\n", result->low_auth_time);
> +       pr_debug("Authentication Time High:%lld\n", result->high_auth_time);
> +       pr_debug("Execution Time Low:%lld\n", result->low_exec_time);
> +       pr_debug("Execution Time High:%lld\n", result->high_exec_time);

All of these could be dev_dbg() I suppose?

> +}
> +
> +static int start_acpi_update(int action)
> +{
> +       union acpi_object *out_obj, in_obj, in_buf;
> +       struct pfru_updated_result update_result;
> +       acpi_handle handle;
> +       int i, 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->dev);
> +       out_obj = acpi_evaluate_dsm_typed(handle, &pfru_dev->uuid,
> +                                         pfru_dev->rev_id, FUNC_START,
> +                                         &in_obj, ACPI_TYPE_PACKAGE);
> +       if (!out_obj)
> +               return -EINVAL;
> +
> +       for (i = 0; i < out_obj->package.count; i++) {

Same comment regarding the benefit of doing a loop: why is it needed?

> +               union acpi_object *obj = &out_obj->package.elements[i];
> +
> +               if (obj->type != ACPI_TYPE_INTEGER)
> +                       goto free_acpi_buffer;
> +               switch (i) {
> +               case UPDATE_STATUS_IDX:
> +                       update_result.status = obj->integer.value;
> +                       break;
> +               case UPDATE_EXT_STATUS_IDX:
> +                       update_result.ext_status = obj->integer.value;
> +                       break;
> +               case UPDATE_AUTH_TIME_LOW_IDX:
> +                       update_result.low_auth_time = obj->integer.value;
> +                       break;
> +               case UPDATE_AUTH_TIME_HI_IDX:
> +                       update_result.high_auth_time = obj->integer.value;
> +                       break;
> +               case UPDATE_EXEC_TIME_LOW_IDX:
> +                       update_result.low_exec_time = obj->integer.value;
> +                       break;
> +               case UPDATE_EXEC_TIME_HI_IDX:
> +                       update_result.high_exec_time = obj->integer.value;
> +                       break;
> +               default:
> +                       pr_err("Incorrect format of Runtime Update result.\n");
> +                       goto free_acpi_buffer;
> +               }
> +       }
> +       dump_update_result(&update_result);
> +       ret = 0;
> +
> +free_acpi_buffer:
> +       ACPI_FREE(out_obj);
> +
> +       return ret;
> +}
> +
> +static long pfru_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> +       void __user *p;
> +       int ret = 0, rev;
> +
> +       p = (void __user *)arg;
> +
> +       switch (cmd) {
> +       case PFRU_IOC_SET_REV:
> +               if (copy_from_user(&rev, p, sizeof(unsigned int)))
> +                       return -EFAULT;
> +               if (!pfru_valid_revid(rev))
> +                       return -EFAULT;

Why is this the right error code to return here?

> +               pfru_dev->rev_id = rev;
> +               break;
> +       case PFRU_IOC_STAGE:
> +               ret = start_acpi_update(START_STAGE);
> +               break;
> +       case PFRU_IOC_ACTIVATE:
> +               ret = start_acpi_update(START_ACTIVATE);
> +               break;
> +       case PFRU_IOC_STAGE_ACTIVATE:
> +               ret = start_acpi_update(START_STAGE_ACTIVATE);
> +               break;
> +       default:
> +               ret = -ENOIOCTLCMD;
> +               break;
> +       }
> +
> +       return ret;
> +}
> +
> +#ifdef CONFIG_COMPAT
> +static long compat_pfru_ioctl(struct file *filep, unsigned int cmd,
> +                             unsigned long arg)
> +{
> +       return pfru_ioctl(filep, cmd, arg);
> +}
> +#endif
> +
> +static int pfru_open(struct inode *inode, struct file *file)
> +{
> +       return capable(CAP_SYS_RAWIO) ? stream_open(inode, file) : -EPERM;
> +}
> +
> +static ssize_t pfru_write(struct file *file, const char __user *buf,
> +                         size_t len, loff_t *ppos)
> +{
> +       struct pfru_update_cap_info cap;
> +       struct pfru_com_buf_info info;
> +       phys_addr_t phy_addr;
> +       struct iov_iter iter;
> +       struct iovec iov;
> +       char *buf_ptr;
> +       int ret;
> +
> +       ret = query_buffer(&info);
> +       if (ret)
> +               return ret;
> +
> +       if (len > 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)(info.addr_lo | (info.addr_hi << 32));
> +       buf_ptr = memremap(phy_addr, info.buf_size, MEMREMAP_WB);
> +       if (IS_ERR(buf_ptr))
> +               return PTR_ERR(buf_ptr);

Empty line here, please.

> +       if (!copy_from_iter_full(buf_ptr, len, &iter)) {
> +               pr_err("error! could not read capsule file\n");

dev_dbg()?

> +               ret = -EINVAL;
> +               goto unmap;
> +       }
> +
> +       /* Check if the capsule header has a valid version number. */
> +       ret = query_capability(&cap);
> +       if (ret)
> +               goto unmap;

ret is guaranteed to be 0 here, so you can do

if (cap.status != DSM_SUCCEED)
        ret = -EBUSY;
else if (!valid_version(buf_ptr, &cap))
        ret = -EINVAL;

and the gotos and the "ret = 0" statement below won't be necessary.

> +
> +       if (cap.status != DSM_SUCCEED) {
> +               ret = -EBUSY;
> +               goto unmap;
> +       }
> +       if (!valid_version(buf_ptr, &cap)) {
> +               ret = -EINVAL;
> +               goto unmap;
> +       }
> +       ret = 0;
> +unmap:
> +       memunmap(buf_ptr);
> +
> +       return ret ?: len;
> +}
> +
> +static ssize_t pfru_read(struct file *filp, char __user *ubuf,
> +                        size_t size, loff_t *off)
> +{
> +       struct pfru_update_cap_info cap;
> +       int ret;
> +
> +       ret = query_capability(&cap);
> +       if (ret)
> +               return ret;
> +
> +       size = min_t(size_t, size, sizeof(cap));
> +
> +       if (copy_to_user(ubuf, &cap, size))
> +               return -EFAULT;

Well, if the read() is only needed for this, maybe consider
implementing it as an ioctl() command and using read() for the
telemetry retrieval?  Then, you won't need the other special device
file, the write() will be the code injection/update, the read() will
be telemetry retrieval and all of the rest can be ioctl()s under one
special device file.

> +
> +       return size;
> +}
> +
> +static const struct file_operations acpi_pfru_fops = {
> +       .owner          = THIS_MODULE,
> +       .write          = pfru_write,
> +       .read           = pfru_read,
> +       .open           = pfru_open,
> +       .unlocked_ioctl = pfru_ioctl,
> +#ifdef CONFIG_COMPAT
> +       .compat_ioctl   = compat_pfru_ioctl,
> +#endif
> +       .llseek         = noop_llseek,
> +};
> +
> +static struct miscdevice pfru_misc_dev = {
> +       .minor = MISC_DYNAMIC_MINOR,
> +       .name = "pfru_update",
> +       .nodename = "pfru/update",
> +       .fops = &acpi_pfru_fops,
> +};
> +
> +static int acpi_pfru_remove(struct platform_device *pdev)
> +{
> +       misc_deregister(&pfru_misc_dev);
> +       kfree(pfru_dev);
> +       pfru_dev = NULL;
> +
> +       return 0;
> +}
> +
> +static int acpi_pfru_probe(struct platform_device *pdev)
> +{
> +       acpi_handle handle;
> +       int ret;
> +
> +       if (pfru_dev) {
> +               pr_err("Duplicated PFRU INTC1080 detected, skip...\n");
> +               return 0;
> +       }
> +
> +       pfru_dev = kzalloc(sizeof(*pfru_dev), GFP_KERNEL);
> +       if (!pfru_dev)
> +               return -ENOMEM;
> +
> +       ret = guid_parse(PFRU_UUID, &pfru_dev->uuid);
> +       if (ret)
> +               goto out;
> +       ret = guid_parse(PFRU_CODE_INJ_UUID, &pfru_dev->code_uuid);
> +       if (ret)
> +               goto out;
> +       ret = guid_parse(PFRU_DRV_UPDATE_UUID, &pfru_dev->drv_uuid);
> +       if (ret)
> +               goto out;
> +
> +       /* default rev id is 1 */
> +       pfru_dev->rev_id = 1;
> +       pfru_dev->dev = &pdev->dev;
> +       handle = ACPI_HANDLE(pfru_dev->dev);
> +       if (!acpi_has_method(handle, "_DSM")) {
> +               pr_err("Missing _DSM\n");
> +               ret = -ENODEV;
> +               goto out;
> +       }
> +
> +       ret = misc_register(&pfru_misc_dev);
> +       if (ret)
> +               goto out;
> +
> +       return 0;
> +out:
> +       kfree(pfru_dev);
> +       pfru_dev = NULL;
> +
> +       return ret;
> +}
> +
> +static const struct acpi_device_id acpi_pfru_ids[] = {
> +       {"INTC1080", 0},
> +       {}
> +};
> +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..ca0b7433f79f
> --- /dev/null
> +++ b/include/uapi/linux/pfru.h
> @@ -0,0 +1,101 @@
> +/* 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/uuid.h>
> +
> +#define PFRU_UUID              "ECF9533B-4A3C-4E89-939E-C77112601C6D"
> +#define PFRU_CODE_INJ_UUID             "B2F84B79-7B6E-4E45-885F-3FB9BB185402"
> +#define PFRU_DRV_UPDATE_UUID           "4569DD8C-75F1-429A-A3D6-24DE8097A0DF"
> +
> +#define FUNC_STANDARD_QUERY    0
> +#define FUNC_QUERY_UPDATE_CAP  1
> +#define FUNC_QUERY_BUF         2
> +#define FUNC_START             3
> +
> +#define CODE_INJECT_TYPE       1
> +#define DRIVER_UPDATE_TYPE     2
> +
> +#define REVID_1                1
> +#define REVID_2                2
> +
> +#define PFRU_MAGIC 0xEE
> +
> +#define PFRU_IOC_SET_REV _IOW(PFRU_MAGIC, 0x01, unsigned int)
> +#define PFRU_IOC_STAGE _IOW(PFRU_MAGIC, 0x02, unsigned int)
> +#define PFRU_IOC_ACTIVATE _IOW(PFRU_MAGIC, 0x03, unsigned int)
> +#define PFRU_IOC_STAGE_ACTIVATE _IOW(PFRU_MAGIC, 0x04, unsigned int)
> +
> +static inline int pfru_valid_revid(int id)
> +{
> +       return (id == REVID_1) || (id == REVID_2);
> +}
> +
> +/* Capsule file payload header */
> +struct pfru_payload_hdr {
> +       __u32   sig;
> +       __u32   hdr_version;
> +       __u32   hdr_size;
> +       __u32   hw_ver;
> +       __u32   rt_ver;
> +       uuid_t  platform_id;
> +};
> +
> +enum pfru_start_action {
> +       START_STAGE,
> +       START_ACTIVATE,
> +       START_STAGE_ACTIVATE,
> +};
> +
> +enum pfru_dsm_status {
> +       DSM_SUCCEED,
> +       DSM_FUNC_NOT_SUPPORT,
> +       DSM_INVAL_INPUT,
> +       DSM_HARDWARE_ERR,
> +       DSM_RETRY_SUGGESTED,
> +       DSM_UNKNOWN,
> +       DSM_FUNC_SPEC_ERR,
> +};
> +
> +struct pfru_update_cap_info {
> +       enum pfru_dsm_status status;
> +       __u32 update_cap;
> +
> +       uuid_t code_type;
> +       __u32 fw_version;
> +       __u32 code_rt_version;
> +
> +       uuid_t drv_type;
> +       __u32 drv_rt_version;
> +       __u32 drv_svn;
> +
> +       uuid_t platform_id;
> +       uuid_t oem_id;
> +
> +       char oem_info[];
> +};
> +
> +struct pfru_com_buf_info {
> +       enum pfru_dsm_status status;
> +       enum pfru_dsm_status ext_status;
> +       __u64 addr_lo;
> +       __u64 addr_hi;
> +       __u32 buf_size;
> +};
> +
> +struct pfru_updated_result {
> +       enum pfru_dsm_status status;
> +       enum pfru_dsm_status ext_status;
> +       __u64 low_auth_time;
> +       __u64 high_auth_time;
> +       __u64 low_exec_time;
> +       __u64 high_exec_time;
> +};
> +
> +#endif /* __PFRU_H__ */
> --

Re: [PATCH v3 4/5] drivers/acpi: Introduce Platform Firmware Runtime Update Telemetry

[Rafael J. Wysocki]

On Thu, Sep 16, 2021 at 5:58 PM Chen Yu <yu.c.chen@intel.com> wrote:
>
> 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 provides interfaces for the user to query the parameters of
> telemetry data, and the user could read out the telemetry data
> accordingly.
>
> Typical log looks like:
>
> [SmmRuntimeUpdateHandler.ProcessSmmRuntimeUpdate]
> ProcessSmmRuntimeUpdate = START, Action = 2
> [SmmRuntimeUpdateHandler.ProcessSmmRuntimeUpdate]
> FwVersion = 0, CodeInjectionVersion = 1
> [ShadowSmmRuntimeUpdateImage]
> Image = 0x74D9B000, ImageSize = 0x1172
> [ProcessSmmRuntimeUpdate]
> ShadowSmmRuntimeUpdateImage.Status = Success
> [ValidateSmmRuntimeUpdateImage]
> CapsuleHeader.CapsuleGuid = 6DCBD5ED-E82D-4C44-BDA1-7194199AD92A
> [ValidateSmmRuntimeUpdateImage]
> FmpCapHeader.Version = 1
> [ValidateSmmRuntimeUpdateImage]
> FmpCapImageHeader.UpdateImageTypeId = B2F84B79-7B6E-4E45-885F-3FB9BB185402
> [ValidateSmmRuntimeUpdateImage]
> SmmRuntimeUpdateVerifyImageWithDenylist.Status = Success
> [ValidateSmmRuntimeUpdateImage]
> SmmRuntimeUpdateVerifyImageWithAllowlist.Status = Success
> [SmmCodeInjectionVerifyPayloadHeader]
> PayloadHeader.Signature = 0x31494353
> [SmmCodeInjectionVerifyPayloadHeader]
> PayloadHeader.PlatformId = 63462139-A8B1-AA4E-9024-F2BB53EA4723
> [SmmCodeInjectionVerifyPayloadHeader]
> PayloadHeader.SupportedSmmFirmwareVersion = 0,
> PayloadHeader.SmmCodeInjectionRuntimeVersion = 1
> [ProcessSmmRuntimeUpdate]
> ValidateSmmRuntimeUpdateImage.Status = Success
> CPU CSR[0B102D28] Before = 7FBF830E
> CPU CSR[0B102D28] After = 7FBF8310
> [ProcessSmmRuntimeUpdate] ProcessSmmCodeInjection.Status = Success
> [SmmRuntimeUpdateHandler.ProcessSmmRuntimeUpdate]
> ProcessSmmRuntimeUpdate = End, Status = Success
>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---
> 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/Kconfig          |  14 +
>  drivers/acpi/pfru/Makefile         |   1 +
>  drivers/acpi/pfru/pfru_telemetry.c | 412 +++++++++++++++++++++++++++++
>  include/uapi/linux/pfru.h          |  43 +++
>  4 files changed, 470 insertions(+)
>  create mode 100644 drivers/acpi/pfru/pfru_telemetry.c
>
> diff --git a/drivers/acpi/pfru/Kconfig b/drivers/acpi/pfru/Kconfig
> index 3f31b7d95f3b..e2934058884e 100644
> --- a/drivers/acpi/pfru/Kconfig
> +++ b/drivers/acpi/pfru/Kconfig
> @@ -13,3 +13,17 @@ config ACPI_PFRU
>
>           To compile this driver as a module, choose M here:
>           the module will be called pfru_update.
> +
> +config ACPI_PFRU_TELEMETRY

Why does this need a separate Kconfig option?

> +       tristate "ACPI Platform Firmware Runtime Update Telemetry Service"
> +       depends on ACPI_PFRU
> +       help
> +         PFRU(Platform Firmware Runtime Update) Telemetry Service is part of
> +         RoT(Root of Trust), which allows Platform Firmware Runtime Update handler
> +         and other PFRU drivers to produce telemetry data to upper layer OS consumer
> +         at runtime.
> +
> +         For more information, see:
> +         <file:Documentation/x86/pfru_update.rst>
> +
> +         If unsure, please say N.
> diff --git a/drivers/acpi/pfru/Makefile b/drivers/acpi/pfru/Makefile
> index 098cbe80cf3d..30060ba320ea 100644
> --- a/drivers/acpi/pfru/Makefile
> +++ b/drivers/acpi/pfru/Makefile
> @@ -1,2 +1,3 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  obj-$(CONFIG_ACPI_PFRU) += pfru_update.o
> +obj-$(CONFIG_ACPI_PFRU_TELEMETRY) += pfru_telemetry.o

I'm not sure about this ...

> diff --git a/drivers/acpi/pfru/pfru_telemetry.c b/drivers/acpi/pfru/pfru_telemetry.c
> new file mode 100644
> index 000000000000..96dc9e69edc0
> --- /dev/null
> +++ b/drivers/acpi/pfru/pfru_telemetry.c
> @@ -0,0 +1,412 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ACPI Platform Firmware Runtime Update
> + * Telemetry Service Device Driver
> + *
> + * Copyright (C) 2021 Intel Corporation
> + * Author: Chen Yu <yu.c.chen@intel.com>
> + */
> +#include <linux/acpi.h>
> +#include <linux/errno.h>
> +#include <linux/fs.h>
> +#include <linux/file.h>
> +#include <linux/minmax.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/mm.h>
> +#include <linux/platform_device.h>
> +#include <linux/uuid.h>
> +#include <linux/uaccess.h>
> +#include <uapi/linux/pfru.h>
> +
> +enum update_index {
> +       LOG_STATUS_IDX,
> +       LOG_EXT_STATUS_IDX,
> +       LOG_MAX_SZ_IDX,
> +       LOG_CHUNK1_LO_IDX,
> +       LOG_CHUNK1_HI_IDX,
> +       LOG_CHUNK1_SZ_IDX,
> +       LOG_CHUNK2_LO_IDX,
> +       LOG_CHUNK2_HI_IDX,
> +       LOG_CHUNK2_SZ_IDX,
> +       LOG_ROLLOVER_CNT_IDX,
> +       LOG_RESET_CNT_IDX,
> +};
> +
> +struct pfru_telem_device {
> +       struct device *dev;
> +       guid_t uuid;
> +       struct pfru_telem_info info;
> +};
> +
> +/*
> + * There would be only one instance of pfru_telem_device.
> + */
> +static struct pfru_telem_device *telem_dev;
> +
> +static int get_pfru_data_info(struct pfru_telem_data_info *data_info,
> +                             int log_type)
> +{
> +       union acpi_object *out_obj, in_obj, in_buf;
> +       acpi_handle handle;
> +       int i, ret = -EINVAL;
> +
> +       handle = ACPI_HANDLE(telem_dev->dev);
> +
> +       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 = log_type;
> +
> +       out_obj = acpi_evaluate_dsm_typed(handle, &telem_dev->uuid,
> +                                         telem_dev->info.log_revid, FUNC_GET_DATA,
> +                                         &in_obj, ACPI_TYPE_PACKAGE);
> +       if (!out_obj) {
> +               pr_err("Failed to invoke _DSM\n");
> +               return -EINVAL;
> +       }
> +
> +       for (i = 0; i < out_obj->package.count; i++) {

Is the loop really necessary?

> +               union acpi_object *obj = &out_obj->package.elements[i];
> +
> +               if (obj->type != ACPI_TYPE_INTEGER)
> +                       goto free_acpi_buffer;
> +
> +               switch (i) {
> +               case LOG_STATUS_IDX:
> +                       data_info->status = obj->integer.value;
> +                       break;
> +               case LOG_EXT_STATUS_IDX:
> +                       data_info->ext_status = obj->integer.value;
> +                       break;
> +               case LOG_MAX_SZ_IDX:
> +                       data_info->max_data_size = obj->integer.value;
> +                       break;
> +               case LOG_CHUNK1_LO_IDX:
> +                       data_info->chunk1_addr_lo = obj->integer.value;
> +                       break;
> +               case LOG_CHUNK1_HI_IDX:
> +                       data_info->chunk1_addr_hi = obj->integer.value;
> +                       break;
> +               case LOG_CHUNK1_SZ_IDX:
> +                       data_info->chunk1_size = obj->integer.value;
> +                       break;
> +               case LOG_CHUNK2_LO_IDX:
> +                       data_info->chunk2_addr_lo = obj->integer.value;
> +                       break;
> +               case LOG_CHUNK2_HI_IDX:
> +                       data_info->chunk2_addr_hi = obj->integer.value;
> +                       break;
> +               case LOG_CHUNK2_SZ_IDX:
> +                       data_info->chunk2_size = obj->integer.value;
> +                       break;
> +               case LOG_ROLLOVER_CNT_IDX:
> +                       data_info->rollover_cnt = obj->integer.value;
> +                       break;
> +               case LOG_RESET_CNT_IDX:
> +                       data_info->reset_cnt = obj->integer.value;
> +                       break;
> +               default:
> +                       pr_err("Incorrect format of Telemetry info.\n");

dev_dbg()?  And likewise below.

> +                       goto free_acpi_buffer;
> +               }
> +       }
> +       ret = 0;
> +
> +free_acpi_buffer:
> +       ACPI_FREE(out_obj);
> +
> +       return ret;
> +}
> +
> +static int set_pfru_log_level(int level)
> +{
> +       union acpi_object *out_obj, *obj, in_obj, in_buf;
> +       enum pfru_dsm_status status;
> +       acpi_handle handle;
> +       int ret = -EINVAL;
> +
> +       handle = ACPI_HANDLE(telem_dev->dev);
> +
> +       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, &telem_dev->uuid,
> +                                         telem_dev->info.log_revid, 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) {
> +               pr_err("get MM telemetry level error status %d\n",
> +                      status);
> +               goto free_acpi_buffer;
> +       }
> +
> +       obj = &out_obj->package.elements[1];
> +       status = obj->integer.value;
> +       if (status) {
> +               pr_err("get MM telemetry level error extend status %d\n",
> +                      status);
> +               goto free_acpi_buffer;
> +       }
> +       ret = 0;
> +
> +free_acpi_buffer:
> +       ACPI_FREE(out_obj);
> +
> +       return ret;
> +}
> +
> +static int get_pfru_log_level(int *level)
> +{
> +       union acpi_object *out_obj, *obj;
> +       enum pfru_dsm_status status;
> +       acpi_handle handle;
> +       int ret = -EINVAL;
> +
> +       handle = ACPI_HANDLE(telem_dev->dev);
> +       out_obj = acpi_evaluate_dsm_typed(handle, &telem_dev->uuid,
> +                                         telem_dev->info.log_revid, 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) {
> +               pr_err("get MM telemetry level error status %d\n",
> +                      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) {
> +               pr_err("get MM telemetry level error status %d\n",
> +                      status);
> +               goto free_acpi_buffer;
> +       }
> +
> +       obj = &out_obj->package.elements[2];
> +       if (obj->type != ACPI_TYPE_INTEGER)
> +               goto free_acpi_buffer;
> +       *level = obj->integer.value;
> +
> +       ret = 0;
> +
> +free_acpi_buffer:
> +       ACPI_FREE(out_obj);
> +
> +       return ret;
> +}
> +
> +static int valid_log_level(int level)
> +{
> +       return (level == LOG_ERR) || (level == LOG_WARN) ||
> +               (level == LOG_INFO) || (level == LOG_VERB);

The parens are redundant AFAICS.

> +}
> +
> +static int valid_log_type(int type)
> +{
> +       return (type == LOG_EXEC_IDX) || (type == LOG_HISTORY_IDX);

And here too.

> +}
> +
> +static long pfru_telemetry_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +
> +{
> +       struct pfru_telem_data_info data_info;
> +       struct pfru_telem_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;

Empty line, please.

> +               if (pfru_valid_revid(info.log_revid))
> +                       telem_dev->info.log_revid = info.log_revid;
> +
> +               if (valid_log_level(info.log_level)) {
> +                       ret = set_pfru_log_level(info.log_level);
> +                       if (ret)
> +                               return ret;

And here.

> +                       telem_dev->info.log_level = info.log_level;
> +               }
> +               if (valid_log_type(info.log_type))
> +                       telem_dev->info.log_type = info.log_type;

And here.  And analogously below.

Generally speaking, I tend to prefer putting empty code lines after an
if () statement that is not followed by a block.

> +               break;
> +       case PFRU_LOG_IOC_GET_INFO:
> +               ret = get_pfru_log_level(&info.log_level);
> +               if (ret)
> +                       return ret;
> +               info.log_type = telem_dev->info.log_type;
> +               info.log_revid = telem_dev->info.log_revid;
> +               if (copy_to_user(p, &info, sizeof(info)))
> +                       ret = -EFAULT;
> +               break;
> +       case PFRU_LOG_IOC_GET_DATA_INFO:
> +               ret = get_pfru_data_info(&data_info, telem_dev->info.log_type);
> +               if (ret)
> +                       return ret;
> +               if (copy_to_user(p, &data_info, sizeof(struct pfru_telem_data_info)))
> +                       ret = -EFAULT;
> +               break;
> +       default:
> +               ret = -ENOIOCTLCMD;
> +               break;
> +       }
> +       return ret;
> +}
> +
> +static ssize_t pfru_telemetry_read(struct file *filp, char __user *ubuf,
> +                                  size_t size, loff_t *off)
> +{
> +       struct pfru_telem_data_info info;
> +       phys_addr_t base_addr;
> +       int buf_size, ret;
> +       char *buf_ptr;
> +
> +       if (*off < 0)
> +               return -EINVAL;
> +
> +       ret = get_pfru_data_info(&info, telem_dev->info.log_type);
> +       if (ret) {
> +               pr_err("Could not get telemetry data info %d\n", ret);
> +               return ret;
> +       }
> +
> +       base_addr = (phys_addr_t)(info.chunk2_addr_lo |
> +                       (info.chunk2_addr_hi << 32));

This line doesn't need to be broken as far as I'm concerned.

> +       if (!base_addr) {
> +               pr_err("Telemetry data not ready\n");
> +               return -EBUSY;
> +       }
> +
> +       buf_size = info.max_data_size;
> +       if (*off >= buf_size)
> +               return 0;
> +
> +       buf_ptr = memremap(base_addr, buf_size, MEMREMAP_WB);
> +       if (IS_ERR(buf_ptr))
> +               return PTR_ERR(buf_ptr);
> +
> +       size = min_t(size_t, size, buf_size - *off);
> +
> +       ret = -EFAULT;
> +       if (copy_to_user(ubuf, buf_ptr + *off, size))
> +               goto out;
> +       ret = 0;

So this would be far more readable in the following form IMO:

size = min_t(size_t, size, buf_size - *off);
if (copy_to_user(ubuf, buf_ptr + *off, size))
        ret = -EFAULT;
else
        ret = 0;

And it looks like you won't need the "out" label any more then.

> +out:
> +       memunmap(buf_ptr);
> +
> +       return ret ? ret : size;
> +}
> +
> +#ifdef CONFIG_COMPAT
> +static long compat_pfru_telemetry_ioctl(struct file *filep, unsigned int cmd,
> +                                       unsigned long arg)
> +{
> +       return pfru_telemetry_ioctl(filep, cmd, arg);
> +}
> +#endif
> +
> +static const struct file_operations acpi_pfru_telemetry_fops = {
> +       .owner          = THIS_MODULE,
> +       .read           = pfru_telemetry_read,
> +       .unlocked_ioctl = pfru_telemetry_ioctl,
> +#ifdef CONFIG_COMPAT
> +       .compat_ioctl   = compat_pfru_telemetry_ioctl,
> +#endif
> +       .llseek         = noop_llseek,
> +};

And I would consider combining this with the update interface so that
the write() updates the firmware code and the read() gets telemetry
data from it.

> +
> +static struct miscdevice pfru_telemetry_misc_dev = {
> +       .minor = MISC_DYNAMIC_MINOR,
> +       .name = "pfru_telemetry",
> +       .nodename = "pfru/telemetry",
> +       .fops = &acpi_pfru_telemetry_fops,
> +};
> +
> +static int acpi_pfru_telemetry_remove(struct platform_device *pdev)
> +{
> +       misc_deregister(&pfru_telemetry_misc_dev);
> +       kfree(telem_dev);
> +       telem_dev = NULL;
> +
> +       return 0;
> +}
> +
> +static int acpi_pfru_telemetry_probe(struct platform_device *pdev)
> +{
> +       acpi_handle handle;
> +       int ret;
> +
> +       if (telem_dev) {
> +               pr_err("Duplicated PFRU Telemetry INTC1081 detected, skip...\n");
> +               return 0;
> +       }
> +
> +       telem_dev = kzalloc(sizeof(*telem_dev), GFP_KERNEL);
> +       if (!telem_dev)
> +               return -ENOMEM;
> +
> +       ret = guid_parse(PFRU_TELEMETRY_UUID, &telem_dev->uuid);
> +       if (ret)
> +               goto out;
> +
> +       telem_dev->info.log_revid = 1;
> +       telem_dev->dev = &pdev->dev;
> +       handle = ACPI_HANDLE(telem_dev->dev);
> +       if (!acpi_has_method(handle, "_DSM")) {
> +               pr_err("Missing _DSM\n");
> +               ret = -ENODEV;
> +               goto out;
> +       }
> +
> +       ret = misc_register(&pfru_telemetry_misc_dev);
> +       if (ret)
> +               goto out;
> +
> +       return 0;
> +out:
> +       kfree(telem_dev);
> +       telem_dev = NULL;
> +
> +       return ret;
> +}
> +
> +static const struct acpi_device_id acpi_pfru_telemetry_ids[] = {
> +       {"INTC1081", 0},
> +       {}
> +};
> +MODULE_DEVICE_TABLE(acpi, acpi_pfru_telemetry_ids);
> +
> +static struct platform_driver acpi_pfru_telemetry_driver = {
> +       .driver = {
> +               .name = "pfru_telemetry",
> +               .acpi_match_table = acpi_pfru_telemetry_ids,
> +       },
> +       .probe = acpi_pfru_telemetry_probe,
> +       .remove = acpi_pfru_telemetry_remove,
> +};
> +module_platform_driver(acpi_pfru_telemetry_driver);
> +
> +MODULE_DESCRIPTION("Platform Firmware Runtime Update Telemetry Service device driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/uapi/linux/pfru.h b/include/uapi/linux/pfru.h
> index ca0b7433f79f..b04852602133 100644
> --- a/include/uapi/linux/pfru.h
> +++ b/include/uapi/linux/pfru.h
> @@ -98,4 +98,47 @@ struct pfru_updated_result {
>         __u64 high_exec_time;
>  };
>
> +#define PFRU_TELEMETRY_UUID    "75191659-8178-4D9D-B88F-AC5E5E93E8BF"
> +
> +/* Telemetry structures. */
> +struct pfru_telem_data_info {
> +       enum pfru_dsm_status status;
> +       enum pfru_dsm_status 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_telem_info {
> +       __u32 log_level;
> +       __u32 log_type;
> +       __u32 log_revid;
> +};
> +
> +/* Two logs: history and execution log */
> +#define LOG_EXEC_IDX   0
> +#define LOG_HISTORY_IDX        1
> +#define NR_LOG_TYPE    2
> +
> +#define LOG_ERR                0
> +#define LOG_WARN       1
> +#define LOG_INFO       2
> +#define LOG_VERB       4
> +
> +#define FUNC_SET_LEV           1
> +#define FUNC_GET_LEV           2
> +#define FUNC_GET_DATA          3
> +
> +#define LOG_NAME_SIZE          10
> +
> +#define PFRU_LOG_IOC_SET_INFO _IOW(PFRU_MAGIC, 0x05, struct pfru_telem_info)
> +#define PFRU_LOG_IOC_GET_INFO _IOR(PFRU_MAGIC, 0x06, struct pfru_telem_info)
> +#define PFRU_LOG_IOC_GET_DATA_INFO _IOR(PFRU_MAGIC, 0x07, struct pfru_telem_data_info)
> +
>  #endif /* __PFRU_H__ */
> --

Re: [PATCH v3 3/5] drivers/acpi: Introduce Platform Firmware Runtime Update device driver

[Chen Yu]

On Mon, Sep 27, 2021 at 07:40:39PM +0200, Rafael J. Wysocki wrote:
> On Wed, Sep 22, 2021 at 7:28 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Sep 23, 2021 at 12:33:21AM +0800, Chen Yu wrote:
> > > On Wed, Sep 22, 2021 at 11:10:02AM +0200, Greg Kroah-Hartman wrote:
> > > > On Wed, Sep 22, 2021 at 05:04:42PM +0800, Chen Yu wrote:
> > > > > Hi Greg,
> > > > > On Tue, Sep 21, 2021 at 05:59:05PM +0200, Greg Kroah-Hartman wrote:
> > > > > > On Fri, Sep 17, 2021 at 12:02:18AM +0800, Chen Yu wrote:
> > > > > > > Introduce the pfru_update driver which can be used for Platform Firmware
> > > > > > > Runtime code injection and driver update. 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.
> > > > > > >
> > > > > > > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > > > > >
> > > > > > Where is the userspace code that uses this ioctl and has tested it out
> > > > > > to verify it works properly?  A link to that in the changelog would be
> > > > > > great to have.
> > > > > >
> > > > > The patch [5/5] is a self testing tool to test the whole feature. I'll send a
> > > > > new version and Cc you too.
> > > >
> > > > That tests it, but does not answer the question of who will actually use
> > > > this.  What userspace tool needs this new api?
> > > >
> > > Currently there is no dedicated userspace tool developed to use this
> > > feature AFAIK.
> >
> > Wonderful, then it is not needed to be added to the kernel :)
> >
[snip]
> > > It was expected that the end users
> > > could refer to the self test tool to customize their tools. I'm not sure if
> > > this is the proper way to propose the feature, may I have your suggestion on
> > > this, should I create a separate git repository for this tool, or put it in
> > > tools/selftestings as it is now?
> >
> > No, do not add this to the kernel unless you have a real need and user
> > for this.
After revisiting this patch set, I'll revise it to better describe the background
and usage model, and also propose the user space tool to fully demonstrate how to
use this feature.

thanks,
Chenyu

Re: [PATCH v3 3/5] drivers/acpi: Introduce Platform Firmware Runtime Update device driver

[Chen Yu]

On Mon, Sep 27, 2021 at 07:40:39PM +0200, Rafael J. Wysocki wrote:
> On Wed, Sep 22, 2021 at 7:28 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Sep 23, 2021 at 12:33:21AM +0800, Chen Yu wrote:
> > > On Wed, Sep 22, 2021 at 11:10:02AM +0200, Greg Kroah-Hartman wrote:
> > > > On Wed, Sep 22, 2021 at 05:04:42PM +0800, Chen Yu wrote:
> > > > > Hi Greg,
> > > > > On Tue, Sep 21, 2021 at 05:59:05PM +0200, Greg Kroah-Hartman wrote:
> > > > > > On Fri, Sep 17, 2021 at 12:02:18AM +0800, Chen Yu wrote:
> > > > > > > Introduce the pfru_update driver which can be used for Platform Firmware
> > > > > > > Runtime code injection and driver update. 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.
> > > > > > >
> > > > > > > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
[snip]
> > > > > > > +static struct miscdevice pfru_misc_dev = {
> > > > > > > +     .minor = MISC_DYNAMIC_MINOR,
> > > > > > > +     .name = "pfru_update",
> > > > > > > +     .nodename = "pfru/update",
> > > > > >
> > > > > > Why is this in a subdirectory?  What requires this?  Why not just
> > > > > > "pfru"?
> > > > > >
> > > > > The pfru directory might be reused for pfru_telemetry device, whose driver
> > > > > is in 4/5 patch, I'll Cc you with the whole patch set in next version.
> > > >
> > > > "might be" is not a valid reason.  Why does this simple driver deserve a
> > > > whole /dev/ subdirectory?
> > > >
> > > There are pfru_update and pfru_telemetry in the patch, and there is plan to
> > > add a pfru_prm device in the future, which stands for "Platform Runtime Mechanism".
> > > I'll move them to /dev/ in next version.
> >
> > That is a very generic name for a very platform specific and arch
> > specific interface.  As this is an ACPI interface, why not use that name
> > prefix?
> 
> It is not supposed to be either arch-specific or platform-specific.
> The spec is hosted by the UEFI Forum and it is fairly generic IIUC.
> In principle, it could be used to update any kind of platform firmware
> possible to update without system restart.
> 
> That said, the I/F to the platform firmware is based on ACPI methods,
> so "acpi_" would be a reasonable prefix choice.
Ok, will change it in next version.

Thanks,
Chenyu

Re: [PATCH v3 3/5] drivers/acpi: Introduce Platform Firmware Runtime Update device driver

[Chen Yu]

On Tue, Sep 28, 2021 at 02:22:28PM +0200, Rafael J. Wysocki wrote:
> On Thu, Sep 16, 2021 at 5:56 PM Chen Yu <yu.c.chen@intel.com> wrote:
[snip]
> > +static bool valid_cap_type(int idx, union acpi_object *obj)
> > +{
> > +       acpi_object_type type = obj->type;
> > +
> > +       if (idx == CAP_STATUS_IDX || idx == CAP_UPDATE_IDX ||
> > +           idx == CAP_FW_VER_IDX || idx == CAP_CODE_RT_VER_IDX ||
> > +           idx == CAP_DRV_RT_VER_IDX || idx == CAP_DRV_SVN_IDX)
> > +               return type == ACPI_TYPE_INTEGER;
> > +       else if (idx == CAP_CODE_TYPE_IDX || idx == CAP_DRV_TYPE_IDX ||
> > +                idx == CAP_PLAT_ID_IDX || idx == CAP_OEM_ID_IDX ||
> > +                idx == CAP_OEM_INFO_IDX)
> > +               return type == ACPI_TYPE_BUFFER;
> > +       else
> > +               return false;
> > +}
> 
> IMO, this is way overdesigned.  It is not necessary to do all of these
> checks for every element of the package in query_capability().
>
[snip] 
> It is not very useful to do a loop if there is a switch () on the
> control variable in every step.
> 
> > +               union acpi_object *obj = &out_obj->package.elements[i];
> > +
> > +               if (!valid_cap_type(i, obj))
> > +                       goto free_acpi_buffer;
> > +
> > +               switch (i) {
> > +               case CAP_STATUS_IDX:
> > +                       cap->status = obj->integer.value;
> > +                       break;
> 
> The above can be written as
> 
> if (out_obj->package.elements[CAP_STATUS_IDX].type != ACPI_TYPE_INTEGER)
>         goto free_acpi_buffer;
> 
> cap->status = out_obj->package.elements[CAP_STATUS_IDX].integer.value;
> 
> and analogously for all of the other index values.
> 
> But check the number of elements upfront.
> 
Ok, got it. The next version will combine above together, and eliminate
the loop, switch logic.
> > +               case CAP_UPDATE_IDX:
> > +                       cap->update_cap = obj->integer.value;
> > +                       break;
> > +               case CAP_CODE_TYPE_IDX:
> > +                       memcpy(&cap->code_type, obj->buffer.pointer,
> > +                              obj->buffer.length);
> > +                       break;
> > +               case CAP_FW_VER_IDX:
> > +                       cap->fw_version = obj->integer.value;
> > +                       break;
> > +               case CAP_CODE_RT_VER_IDX:
> > +                       cap->code_rt_version = obj->integer.value;
> > +                       break;
> > +               case CAP_DRV_TYPE_IDX:
> > +                       memcpy(&cap->drv_type, obj->buffer.pointer,
> > +                              obj->buffer.length);
> > +                       break;
> > +               case CAP_DRV_RT_VER_IDX:
> > +                       cap->drv_rt_version = obj->integer.value;
> > +                       break;
> > +               case CAP_DRV_SVN_IDX:
> > +                       cap->drv_svn = obj->integer.value;
> > +                       break;
> > +               case CAP_PLAT_ID_IDX:
> > +                       memcpy(&cap->platform_id, obj->buffer.pointer,
> > +                              obj->buffer.length);
> > +                       break;
> > +               case CAP_OEM_ID_IDX:
> > +                       memcpy(&cap->oem_id, obj->buffer.pointer,
> > +                              obj->buffer.length);
> > +                       break;
> > +               case CAP_OEM_INFO_IDX:
> > +                       /*vendor specific data*/
> > +                       break;
> > +               default:
> > +                       pr_err("Incorrect format of Update Capability.\n");
> 
> Why pr_err() and not dev_dbg()?
>
Will change to dev_dbg() in next version. 
> Besides, it looks like you're going to fail if the package has more
> elements than expected, but is this really a big deal?
> 
> Moreover, what if the number of package elements is too small?
> 
Ok, will change it to check if the number of package elements is larger/equals to
expected in the spec.
> > +                       goto free_acpi_buffer;
> > +               }
> > +       }
> > +       ret = 0;
> > +
> > +free_acpi_buffer:
> > +       ACPI_FREE(out_obj);
> > +
> > +       return ret;
> > +}
> > +
> > +static int query_buffer(struct pfru_com_buf_info *info)
> > +{
> > +       union acpi_object *out_obj;
> > +       acpi_handle handle;
> > +       int i, ret = -EINVAL;
> > +
> > +       handle = ACPI_HANDLE(pfru_dev->dev);
> > +       out_obj = acpi_evaluate_dsm_typed(handle, &pfru_dev->uuid,
> > +                                         pfru_dev->rev_id, FUNC_QUERY_BUF,
> > +                                         NULL, ACPI_TYPE_PACKAGE);
> > +       if (!out_obj)
> > +               return -EINVAL;
> > +
> > +       for (i = 0; i < out_obj->package.count; i++) {
> 
> Again, what is the benefit from doing this loop?
>
Will eliminate the loop in next version. 
[snip]
> > +
> > +static int get_image_type(efi_manage_capsule_image_header_t *img_hdr,
> > +                         int *type)
> > +{
> > +       guid_t *image_type_id;
> > +
> > +       /* check whether this is a code injection or driver update */
> > +       image_type_id = &img_hdr->image_type_id;
> 
> Anything wrong with doing this assignment as initialization?
> 
Ok, will change it in next version.
> > +       if (guid_equal(image_type_id, &pfru_dev->code_uuid))
> > +               *type = CODE_INJECT_TYPE;
> > +       else if (guid_equal(image_type_id, &pfru_dev->drv_uuid))
> > +               *type = DRIVER_UPDATE_TYPE;
> 
> And what would be wrong with returning the type or a negative error code?
> 
Will change it in next version.
> > +       else
> > +               return -EINVAL;
> > +
> > +       return 0;
> > +}
> > +
> > +/*
> > + * 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.
> 
> Either turn this into a kerneldoc comment or put it inside the function.
> 
Will put it inside the function.
> > + */
> > +static int adjust_efi_size(efi_manage_capsule_image_header_t *img_hdr,
> > +                          int *size)
> > +{
> > +       int tmp_size = *size;
> > +
> > +       tmp_size += sizeof(efi_manage_capsule_image_header_t);
> > +       switch (img_hdr->ver) {
> > +       case 1:
> > +               tmp_size -= 2 * sizeof(u64);
> > +               break;
> > +       case 2:
> > +               tmp_size -= sizeof(u64);
> > +               break;
> > +       default:
> > +               /* only support version 1 and 2 */
> > +               return -EINVAL;
> > +       }
> > +       *size = tmp_size;
> 
> Why not simply return the size or a negative error code?
> 
Will change it in next version.
> > +
> > +       return 0;
> > +}
> > +
> > +/*
> > + * Sanity check if the capsule image has a newer version than current one.
> > + * Return: true if it is valid, false otherwise.
> 
> Would it hurt if this was a proper kerneldoc comment?
> 
Will put it inside the function.
[snip]
> > +
> > +static void dump_update_result(struct pfru_updated_result *result)
> > +{
> > +       pr_debug("Update result:\n");
> > +       pr_debug("Status:%d\n", result->status);
> > +       pr_debug("Extended Status:%d\n", result->ext_status);
> > +       pr_debug("Authentication Time Low:%lld\n", result->low_auth_time);
> > +       pr_debug("Authentication Time High:%lld\n", result->high_auth_time);
> > +       pr_debug("Execution Time Low:%lld\n", result->low_exec_time);
> > +       pr_debug("Execution Time High:%lld\n", result->high_exec_time);
> 
> All of these could be dev_dbg() I suppose?
>
Ok, will change it in next version. 
> > +}
> > +
> > +
[snip]
> > +       for (i = 0; i < out_obj->package.count; i++) {
> 
> Same comment regarding the benefit of doing a loop: why is it needed?
> 
Will remove the loop in next version.
[snip]
> > +
> > +       switch (cmd) {
> > +       case PFRU_IOC_SET_REV:
> > +               if (copy_from_user(&rev, p, sizeof(unsigned int)))
> > +                       return -EFAULT;
> > +               if (!pfru_valid_revid(rev))
> > +                       return -EFAULT;
> 
> Why is this the right error code to return here?
> 
Will change EFAULT to EINVAL in next version.
[snip]
> > +       /* map the communication buffer */
> > +       phy_addr = (phys_addr_t)(info.addr_lo | (info.addr_hi << 32));
> > +       buf_ptr = memremap(phy_addr, info.buf_size, MEMREMAP_WB);
> > +       if (IS_ERR(buf_ptr))
> > +               return PTR_ERR(buf_ptr);
> 
> Empty line here, please.
> 
Ok, will do.
> > +       if (!copy_from_iter_full(buf_ptr, len, &iter)) {
> > +               pr_err("error! could not read capsule file\n");
> 
> dev_dbg()?
> 
Ok, will change it in next version.
> > +               ret = -EINVAL;
> > +               goto unmap;
> > +       }
> > +
> > +       /* Check if the capsule header has a valid version number. */
> > +       ret = query_capability(&cap);
> > +       if (ret)
> > +               goto unmap;
> 
> ret is guaranteed to be 0 here, so you can do
> 
> if (cap.status != DSM_SUCCEED)
>         ret = -EBUSY;
> else if (!valid_version(buf_ptr, &cap))
>         ret = -EINVAL;
> 
> and the gotos and the "ret = 0" statement below won't be necessary.
> 
Ok, will do in next version.
[snip]
> > +static ssize_t pfru_read(struct file *filp, char __user *ubuf,
> > +                        size_t size, loff_t *off)
> > +{
> > +       struct pfru_update_cap_info cap;
> > +       int ret;
> > +
> > +       ret = query_capability(&cap);
> > +       if (ret)
> > +               return ret;
> > +
> > +       size = min_t(size_t, size, sizeof(cap));
> > +
> > +       if (copy_to_user(ubuf, &cap, size))
> > +               return -EFAULT;
> 
> Well, if the read() is only needed for this, maybe consider
> implementing it as an ioctl() command and using read() for the
> telemetry retrieval?  Then, you won't need the other special device
> file, the write() will be the code injection/update, the read() will
> be telemetry retrieval and all of the rest can be ioctl()s under one
> special device file.
> 
Got it, will try to combine the two modules into one.

thanks,
Chenyu