Introduce Platform Firmware Runtime Update and Telemetry drivers

Introduce the pfru_update driver which can be used for Platform Firmware Runtime code injection and driver update [1]. The user is expected to provide the update firmware in the form of capsule file, and pass it to the driver via ioctl. Then the driver would hand this capsule file to the Platform Firmware Runtime Update via the ACPI device _DSM method. At last the low level Management Mode would do the firmware update.

The corresponding userspace tool and man page will be introduced at tools/power/acpi/pfru.

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

Signed-off-by Chen Yu <[email protected]>

---
v7: Use ida_alloc() to allocate a ID, and release the ID when
device is removed. (Greg Kroah-Hartman)
Check the _DSM method at early stage, before allocate or parse anything in acpi_pfru_probe(). (Greg Kroah-Hartman)
Set the parent of the misc device. (Greg Kroah-Hartman) Use module_platform_driver() instead of platform_driver_register() in module_init(). Separate pfru driver and pfru_telemetry driver to two files. (Greg Kroah-Hartman)
v6: Use Link: tag to add the specification download address.
(Andy Shevchenko)
Remove linux/uuid.h and use raw buffers to contain uuid. (Andy Shevchenko)
Drop comma for each terminator entry in the enum structure. (Andy Shevchenko)
Remove redundant 'else' in get_image_type().
(Andy Shevchenko)
Directly return results from the switch cases in adjust_efi_size() and pfru_ioctl().(Andy Shevchenko)
Keep comment style consistent by removing the period for one line comment.
(Andy Shevchenko)
Remove devm_kfree() if .probe() failed.
(Andy Shevchenko)
v5: Remove Documentation/ABI/pfru, and move the content to kernel doc
in include/uapi/linux/pfru.h (Greg Kroah-Hartman)
Shrink the range of ioctl numbers declared in Documentation/userspace-api/ioctl/ioctl-number.rst
from 16 to 8. (Greg Kroah-Hartman)
Change global variable struct pfru_device *pfru_dev to
per ACPI device. (Greg Kroah-Hartman)
Unregister the misc device in acpi_pfru_remove().
(Greg Kroah-Hartman)
Convert the kzalloc() to devm_kzalloc() in the driver so as to avoid freeing the memory. (Greg Kroah-Hartman)
Fix the compile error by declaring the pfru_log_ioctl() as static. (kernel test robot LKP)
Change to global variable misc_device to per ACPI device. (Greg Kroah-Hartman)
v4: Replace all pr_err() with dev_dbg() (Greg Kroah-Hartman,
Rafael J. Wysocki)
Returns ENOTTY rather than ENOIOCTLCMD if invalid ioctl command is provided. (Greg Kroah-Hartman)
Remove compat ioctl. (Greg Kroah-Hartman)
Rename /dev/pfru/pfru_update to /dev/acpi_pfru (Greg Kroah-Hartman) Simplify the check for element of the package in query_capability() (Rafael J. Wysocki)
Remove the loop in query_capability(), query_buffer() and query the package elemenet directly. (Rafael J. Wysocki)
Check the the number of elements in case the number of package elements is too small. (Rafael J. Wysocki)
Doing the assignment as initialization in get_image_type(). Meanwhile, returns the type or a negative error code in get_image_type(). (Rafael J. Wysocki)
Put the comments inside the function. (Rafael J. Wysocki) Returns the size or a negative error code in adjust_efi_size() (Rafael J. Wysocki)
Fix the return value from EFAULT to EINVAL if pfru_valid_revid() does not pass. (Rafael J. Wysocki)
Change the write() to be the code injection/update, the read() to be telemetry retrieval and all of the rest to be ioctl()s under one special device file.(Rafael J. Wysocki)
Putting empty code lines after an if () statement that is not followed by a block. (Rafael J. Wysocki)
Remove "goto" tags to make the code more readable. (Rafael J. Wysocki)
v3: Use __u32 instead of int and __64 instead of unsigned long
in include/uapi/linux/pfru.h (Greg Kroah-Hartman)
Rename the structure in uapi to start with a prefix pfru so as to avoid confusing in the global namespace. (Greg Kroah-Hartman)
v2: Add sanity check for duplicated instance of ACPI device.
Update the driver to work with allocated pfru_device objects. (Mike Rapoport)
For each switch case pair, get rid of the magic case numbers and add a default clause with the error handling.
(Mike Rapoport)
Move the obj->type checks outside the switch to reduce redundancy. (Mike Rapoport)
Parse the code_inj_id and drv_update_id at driver initialization time to reduce the re-parsing at runtime.(Mike Rapoport) Explain in detail how the size needs to be adjusted when doing version check.(Mike Rapoport)
Rename parse_update_result() to dump_update_result()(Mike Rapoport) Remove redundant return.(Mike Rapoport)
Do not expose struct capsulate_buf_info to uapi, since it is not needed in userspace.(Mike Rapoport)

---
 .../userspace-api/ioctl/ioctl-number.rst      |   1 +
 drivers/acpi/Kconfig                          |   1 +
 drivers/acpi/Makefile                         |   1 +
 drivers/acpi/pfru/Kconfig                     |  13 +
 drivers/acpi/pfru/Makefile                    |   2 +
 drivers/acpi/pfru/pfru_update.c               | 572 ++++++++++++++++++
 include/uapi/linux/pfru.h                     | 186 ++++++
 7 files changed, 776 insertions(+)
 create mode 100644 drivers/acpi/pfru/Kconfig
 create mode 100644 drivers/acpi/pfru/Makefile
 create mode 100644 drivers/acpi/pfru/pfru_update.c
 create mode 100644 include/uapi/linux/pfru.h

Documentation/userspace-api/ioctl/ioctl-number.rst +1 -0

index 2e8134059c87..6f7c86b6deb7 100644
@@ -365,6 +365,7 @@ Code  Seq#    Include File                                           Comments
                                                                      <mailto:[email protected]>
 0xE5  00-3F  linux/fuse.h
 0xEC  00-01  drivers/platform/chrome/cros_ec_dev.h                   ChromeOS EC driver
+0xEE  00-08  uapi/linux/pfru.h                                       Platform Firmware Runtime Update and Telemetry
 0xF3  00-3F  drivers/usb/misc/sisusbvga/sisusb.h                     sisfb (in development)
                                                                      <mailto:[email protected]>
 0xF6  all                                                            LTTng Linux Trace Toolkit Next Generation

drivers/acpi/Kconfig +1 -0

index 1da360c51d66..1d8d2e2cefac 100644
@@ -482,6 +482,7 @@ source "drivers/acpi/nfit/Kconfig"
 source "drivers/acpi/numa/Kconfig"
 source "drivers/acpi/apei/Kconfig"
 source "drivers/acpi/dptf/Kconfig"
+source "drivers/acpi/pfru/Kconfig"
 
 config ACPI_WATCHDOG
 	bool

drivers/acpi/Makefile +1 -0

index 3018714e87d9..9c2c5ddff6ec 100644
@@ -102,6 +102,7 @@ obj-$(CONFIG_ACPI_CPPC_LIB)	+= cppc_acpi.o
 obj-$(CONFIG_ACPI_SPCR_TABLE)	+= spcr.o
 obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o
 obj-$(CONFIG_ACPI_PPTT) 	+= pptt.o
+obj-$(CONFIG_ACPI_PFRU)		+= pfru/
 
 # processor has its own "processor." module_param namespace
 processor-y			:= processor_driver.o

drivers/acpi/pfru/Kconfig new file +13 -0

new file mode 100644
index 000000000000..5b31675b173a
@@ -0,0 +1,13 @@
+# SPDX-License-Identifier: GPL-2.0
+config ACPI_PFRU
+	tristate "ACPI Platform Firmware Runtime Update (PFRU)"
+	depends on 64BIT
+	help
+	  In order to reduce the system reboot times and update the platform firmware
+	  in time, Platform Firmware Runtime Update is leveraged to patch the system
+	  without reboot. This driver supports Platform Firmware Runtime Update,
+	  which is composed of two parts: code injection and driver update. It also
+	  allows telemetry data to be retrieved from the platform firmware.
+
+	  To compile this driver as a module, choose M here:
+	  the module will be called pfru_update.

drivers/acpi/pfru/Makefile new file +2 -0

new file mode 100644
index 000000000000..098cbe80cf3d
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_ACPI_PFRU) += pfru_update.o

drivers/acpi/pfru/pfru_update.c new file +51 -0

new file mode 100644
index 000000000000..a9953eb6c2d5
@@ -0,0 +1,572 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ACPI Platform Firmware Runtime Update Device Driver
+ *
+ * Copyright (C) 2021 Intel Corporation
+ * Author: Chen Yu <[email protected]>
+ */
+#include <linux/acpi.h>
+#include <linux/device.h>
+#include <linux/efi.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/file.h>
+#include <linux/fs.h>
+#include <linux/idr.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/string.h>
+#include <linux/uaccess.h>
+#include <linux/uio.h>
+#include <linux/uuid.h>
+#include <uapi/linux/pfru.h>
+
+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,
+	CAP_NR_IDX
+};
+
+enum buf_index {
+	BUF_STATUS_IDX,
+	BUF_EXT_STATUS_IDX,
+	BUF_ADDR_LOW_IDX,
+	BUF_ADDR_HI_IDX,
+	BUF_SIZE_IDX,
+	BUF_NR_IDX
+};
+
+enum update_index {
+	UPDATE_STATUS_IDX,
+	UPDATE_EXT_STATUS_IDX,

drivers/acpi/pfru/pfru_update.c new file +308 -0

+	struct miscdevice miscdev;
+};
+
+static DEFINE_IDA(pfru_ida);
+
+static inline struct pfru_device *to_pfru_dev(struct file *file)
+{
+	return container_of(file->private_data, struct pfru_device, miscdev);
+}
+
+static int query_capability(struct pfru_update_cap_info *cap,
+			    struct pfru_device *pfru_dev)
+{
+	union acpi_object *out_obj;
+	acpi_handle handle;
+	int 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 ret;
+
+	if (out_obj->package.count < CAP_NR_IDX)
+		goto free_acpi_buffer;
+
+	if (out_obj->package.elements[CAP_STATUS_IDX].type != ACPI_TYPE_INTEGER)
+		goto free_acpi_buffer;
+
+	cap->status = out_obj->package.elements[CAP_STATUS_IDX].integer.value;
+
+	if (out_obj->package.elements[CAP_UPDATE_IDX].type != ACPI_TYPE_INTEGER)
+		goto free_acpi_buffer;
+
+	cap->update_cap = out_obj->package.elements[CAP_UPDATE_IDX].integer.value;
+
+	if (out_obj->package.elements[CAP_CODE_TYPE_IDX].type != ACPI_TYPE_BUFFER)
+		goto free_acpi_buffer;
+
+	memcpy(&cap->code_type,
+	       out_obj->package.elements[CAP_CODE_TYPE_IDX].buffer.pointer,
+	       out_obj->package.elements[CAP_CODE_TYPE_IDX].buffer.length);
+
+	if (out_obj->package.elements[CAP_FW_VER_IDX].type != ACPI_TYPE_INTEGER)
+		goto free_acpi_buffer;
+
+	cap->fw_version =
+		out_obj->package.elements[CAP_FW_VER_IDX].integer.value;
+
+	if (out_obj->package.elements[CAP_CODE_RT_VER_IDX].type != ACPI_TYPE_INTEGER)
+		goto free_acpi_buffer;
+
+	cap->code_rt_version =
+		out_obj->package.elements[CAP_CODE_RT_VER_IDX].integer.value;
+
+	if (out_obj->package.elements[CAP_DRV_TYPE_IDX].type != ACPI_TYPE_BUFFER)
+		goto free_acpi_buffer;
+
+	memcpy(&cap->drv_type,
+	       out_obj->package.elements[CAP_DRV_TYPE_IDX].buffer.pointer,
+	       out_obj->package.elements[CAP_DRV_TYPE_IDX].buffer.length);
+
+	if (out_obj->package.elements[CAP_DRV_RT_VER_IDX].type != ACPI_TYPE_INTEGER)
+		goto free_acpi_buffer;
+
+	cap->drv_rt_version =
+		out_obj->package.elements[CAP_DRV_RT_VER_IDX].integer.value;
+
+	if (out_obj->package.elements[CAP_DRV_SVN_IDX].type != ACPI_TYPE_INTEGER)
+		goto free_acpi_buffer;
+
+	cap->drv_svn =
+		out_obj->package.elements[CAP_DRV_SVN_IDX].integer.value;
+
+	if (out_obj->package.elements[CAP_PLAT_ID_IDX].type != ACPI_TYPE_BUFFER)
+		goto free_acpi_buffer;
+
+	memcpy(&cap->platform_id,
+	       out_obj->package.elements[CAP_PLAT_ID_IDX].buffer.pointer,
+	       out_obj->package.elements[CAP_PLAT_ID_IDX].buffer.length);
+
+	if (out_obj->package.elements[CAP_OEM_ID_IDX].type != ACPI_TYPE_BUFFER)
+		goto free_acpi_buffer;
+
+	memcpy(&cap->oem_id,
+	       out_obj->package.elements[CAP_OEM_ID_IDX].buffer.pointer,
+	       out_obj->package.elements[CAP_OEM_ID_IDX].buffer.length);
+	ret = 0;
+free_acpi_buffer:
+	ACPI_FREE(out_obj);
+
+	return ret;
+}
+
+static int query_buffer(struct pfru_com_buf_info *info,
+			struct pfru_device *pfru_dev)
+{
+	union acpi_object *out_obj;
+	acpi_handle handle;
+	int 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 ret;
+
+	if (out_obj->package.count < BUF_NR_IDX)
+		goto free_acpi_buffer;
+
+	if (out_obj->package.elements[BUF_STATUS_IDX].type != ACPI_TYPE_INTEGER)
+		goto free_acpi_buffer;
+
+	info->status = out_obj->package.elements[BUF_STATUS_IDX].integer.value;
+
+	if (out_obj->package.elements[BUF_EXT_STATUS_IDX].type != ACPI_TYPE_INTEGER)
+		goto free_acpi_buffer;
+
+	info->ext_status =
+		out_obj->package.elements[BUF_EXT_STATUS_IDX].integer.value;
+
+	if (out_obj->package.elements[BUF_ADDR_LOW_IDX].type != ACPI_TYPE_INTEGER)
+		goto free_acpi_buffer;
+
+	info->addr_lo =
+		out_obj->package.elements[BUF_ADDR_LOW_IDX].integer.value;
+
+	if (out_obj->package.elements[BUF_ADDR_HI_IDX].type != ACPI_TYPE_INTEGER)
+		goto free_acpi_buffer;
+
+	info->addr_hi =
+		out_obj->package.elements[BUF_ADDR_HI_IDX].integer.value;
+
+	if (out_obj->package.elements[BUF_SIZE_IDX].type != ACPI_TYPE_INTEGER)
+		goto free_acpi_buffer;
+
+	info->buf_size = out_obj->package.elements[BUF_SIZE_IDX].integer.value;
+
+	ret = 0;
+free_acpi_buffer:
+	ACPI_FREE(out_obj);
+
+	return ret;
+}
+
+static int get_image_type(struct efi_manage_capsule_image_header *img_hdr,
+			  struct pfru_device *pfru_dev)
+{
+	guid_t *image_type_id = &img_hdr->image_type_id;
+
+	/* check whether this is a code injection or driver update */
+	if (guid_equal(image_type_id, &pfru_dev->code_uuid))
+		return CODE_INJECT_TYPE;
+
+	if (guid_equal(image_type_id, &pfru_dev->drv_uuid))
+		return DRIVER_UPDATE_TYPE;
+
+	return -EINVAL;
+}
+
+static int adjust_efi_size(struct efi_manage_capsule_image_header *img_hdr,
+			   int size)
+{
+	/*
+	 * The (u64 hw_ins) was introduced in UEFI spec version 2,
+	 * and (u64 capsule_support) was introduced in version 3.
+	 * The size needs to be adjusted accordingly. That is to
+	 * say, version 1 should subtract the size of hw_ins+capsule_support,
+	 * and version 2 should sbstract the size of capsule_support.
+	 */
+	size += sizeof(struct efi_manage_capsule_image_header);
+	switch (img_hdr->ver) {
+	case 1:
+		return size - 2 * sizeof(u64);
+	case 2:
+		return size - sizeof(u64);
+	default:
+		/* only support version 1 and 2 */
+		return -EINVAL;
+	}
+}
+
+static bool valid_version(const void *data, struct pfru_update_cap_info *cap,
+			  struct pfru_device *pfru_dev)
+{
+	struct pfru_payload_hdr *payload_hdr;
+	efi_capsule_header_t *cap_hdr;
+	struct efi_manage_capsule_header *m_hdr;
+	struct efi_manage_capsule_image_header *m_img_hdr;
+	struct efi_image_auth *auth;
+	int type, size;
+
+	/*
+	 * Sanity check if the capsule image has a newer version
+	 * than current one.
+	 */
+	cap_hdr = (efi_capsule_header_t *)data;
+	size = cap_hdr->headersize;
+	m_hdr = (struct efi_manage_capsule_header *)(data + size);
+	/*
+	 * Current data structure size plus variable array indicated
+	 * by number of (emb_drv_cnt + payload_cnt)
+	 */
+	size += sizeof(struct efi_manage_capsule_header) +
+		      (m_hdr->emb_drv_cnt + m_hdr->payload_cnt) * sizeof(u64);
+	m_img_hdr = (struct efi_manage_capsule_image_header *)(data + size);
+
+	type = get_image_type(m_img_hdr, pfru_dev);
+	if (type < 0)
+		return false;
+
+	size = adjust_efi_size(m_img_hdr, size);
+	if (size < 0)
+		return false;
+
+	auth = (struct efi_image_auth *)(data + size);
+	size += sizeof(u64) + auth->auth_info.hdr.len;
+	payload_hdr = (struct pfru_payload_hdr *)(data + size);
+
+	/* finally compare the version */
+	if (type == CODE_INJECT_TYPE)
+		return payload_hdr->rt_ver >= cap->code_rt_version;
+	else
+		return payload_hdr->rt_ver >= cap->drv_rt_version;
+}
+
+static void dump_update_result(struct pfru_updated_result *result,
+			       struct pfru_device *pfru_dev)
+{
+	dev_dbg(pfru_dev->dev, "Update result:\n");
+	dev_dbg(pfru_dev->dev, "Status:%d\n", result->status);
+	dev_dbg(pfru_dev->dev, "Extended Status:%d\n", result->ext_status);
+	dev_dbg(pfru_dev->dev, "Authentication Time Low:%lld\n",
+		result->low_auth_time);
+	dev_dbg(pfru_dev->dev, "Authentication Time High:%lld\n",
+		result->high_auth_time);
+	dev_dbg(pfru_dev->dev, "Execution Time Low:%lld\n",
+		result->low_exec_time);
+	dev_dbg(pfru_dev->dev, "Execution Time High:%lld\n",
+		result->high_exec_time);
+}
+
+static int start_acpi_update(int action, struct pfru_device *pfru_dev)
+{
+	union acpi_object *out_obj, in_obj, in_buf;
+	struct pfru_updated_result update_result;
+	acpi_handle handle;
+	int ret = -EINVAL;
+
+	memset(&in_obj, 0, sizeof(in_obj));
+	memset(&in_buf, 0, sizeof(in_buf));
+	in_obj.type = ACPI_TYPE_PACKAGE;
+	in_obj.package.count = 1;
+	in_obj.package.elements = &in_buf;
+	in_buf.type = ACPI_TYPE_INTEGER;
+	in_buf.integer.value = action;
+
+	handle = ACPI_HANDLE(pfru_dev->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 ret;
+
+	if (out_obj->package.count < UPDATE_NR_IDX)
+		goto free_acpi_buffer;
+
+	if (out_obj->package.elements[UPDATE_STATUS_IDX].type != ACPI_TYPE_INTEGER)
+		goto free_acpi_buffer;
+
+	update_result.status =
+		out_obj->package.elements[UPDATE_STATUS_IDX].integer.value;
+
+	if (out_obj->package.elements[UPDATE_EXT_STATUS_IDX].type != ACPI_TYPE_INTEGER)
+		goto free_acpi_buffer;
+
+	update_result.ext_status =
+		out_obj->package.elements[UPDATE_EXT_STATUS_IDX].integer.value;
+
+	if (out_obj->package.elements[UPDATE_AUTH_TIME_LOW_IDX].type != ACPI_TYPE_INTEGER)
+		goto free_acpi_buffer;
+
+	update_result.low_auth_time =
+		out_obj->package.elements[UPDATE_AUTH_TIME_LOW_IDX].integer.value;
+
+	if (out_obj->package.elements[UPDATE_AUTH_TIME_HI_IDX].type != ACPI_TYPE_INTEGER)
+		goto free_acpi_buffer;
+
+	update_result.high_auth_time =
+		out_obj->package.elements[UPDATE_AUTH_TIME_HI_IDX].integer.value;
+
+	if (out_obj->package.elements[UPDATE_EXEC_TIME_LOW_IDX].type != ACPI_TYPE_INTEGER)
+		goto free_acpi_buffer;
+
+	update_result.low_exec_time =
+		out_obj->package.elements[UPDATE_EXEC_TIME_LOW_IDX].integer.value;
+
+	if (out_obj->package.elements[UPDATE_EXEC_TIME_HI_IDX].type != ACPI_TYPE_INTEGER)
+		goto free_acpi_buffer;
+
+	update_result.high_exec_time =
+		out_obj->package.elements[UPDATE_EXEC_TIME_HI_IDX].integer.value;
+
+	dump_update_result(&update_result, pfru_dev);
+	ret = 0;

drivers/acpi/pfru/pfru_update.c new file +4 -0

+	struct pfru_device *pfru_dev;
+	void __user *p;
+	int ret, rev;
+

drivers/acpi/pfru/pfru_update.c new file +154 -0

+
+		return 0;
+	case PFRU_IOC_STAGE:
+		return start_acpi_update(START_STAGE, pfru_dev);
+	case PFRU_IOC_ACTIVATE:
+		return start_acpi_update(START_ACTIVATE, pfru_dev);
+	case PFRU_IOC_STAGE_ACTIVATE:
+		return start_acpi_update(START_STAGE_ACTIVATE, pfru_dev);
+	default:
+		return -ENOTTY;
+	}
+}
+
+static ssize_t pfru_write(struct file *file, const char __user *buf,
+			  size_t len, loff_t *ppos)
+{
+	struct pfru_update_cap_info cap;
+	struct pfru_com_buf_info info;
+	struct pfru_device *pfru_dev;
+	phys_addr_t phy_addr;
+	struct iov_iter iter;
+	struct iovec iov;
+	char *buf_ptr;
+	int ret;
+
+	pfru_dev = to_pfru_dev(file);
+
+	ret = query_buffer(&info, pfru_dev);
+	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)) {
+		ret = -EINVAL;
+		goto unmap;
+	}
+
+	/* check if the capsule header has a valid version number */
+	ret = query_capability(&cap, pfru_dev);
+	if (ret)
+		goto unmap;
+
+	if (cap.status != DSM_SUCCEED)
+		ret = -EBUSY;
+	else if (!valid_version(buf_ptr, &cap, pfru_dev))
+		ret = -EINVAL;
+unmap:
+	memunmap(buf_ptr);
+
+	return ret ?: len;
+}
+
+static const struct file_operations acpi_pfru_fops = {
+	.owner		= THIS_MODULE,
+	.write		= pfru_write,
+	.unlocked_ioctl = pfru_ioctl,
+	.llseek		= noop_llseek,
+};
+
+static int acpi_pfru_remove(struct platform_device *pdev)
+{
+	struct pfru_device *pfru_dev = platform_get_drvdata(pdev);
+
+	misc_deregister(&pfru_dev->miscdev);
+	ida_free(&pfru_ida, pfru_dev->index);
+
+	return 0;
+}
+
+static int acpi_pfru_probe(struct platform_device *pdev)
+{
+	struct pfru_device *pfru_dev;
+	acpi_handle handle;
+	int ret;
+
+	handle = ACPI_HANDLE(&pdev->dev);
+	if (!acpi_has_method(handle, "_DSM")) {
+		dev_dbg(&pdev->dev, "Missing _DSM\n");
+		return -ENODEV;
+	}
+
+	pfru_dev = devm_kzalloc(&pdev->dev, sizeof(*pfru_dev), GFP_KERNEL);
+	if (!pfru_dev)
+		return -ENOMEM;
+
+	ret = guid_parse(PFRU_UUID, &pfru_dev->uuid);
+	if (ret)
+		return ret;
+
+	ret = guid_parse(PFRU_CODE_INJ_UUID, &pfru_dev->code_uuid);
+	if (ret)
+		return ret;
+
+	ret = guid_parse(PFRU_DRV_UPDATE_UUID, &pfru_dev->drv_uuid);
+	if (ret)
+		return ret;
+
+	ret = ida_alloc(&pfru_ida, GFP_KERNEL);
+	if (ret < 0)
+		return ret;
+
+	pfru_dev->index = ret;
+	/* default rev id is 1 */
+	pfru_dev->rev_id = 1;
+	pfru_dev->dev = &pdev->dev;
+
+	pfru_dev->miscdev.minor = MISC_DYNAMIC_MINOR;
+	pfru_dev->miscdev.name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
+						"pfru%d", pfru_dev->index);
+	if (!pfru_dev->miscdev.name) {
+		ret = -ENOMEM;
+		goto failed;
+	}
+
+	pfru_dev->miscdev.nodename = devm_kasprintf(&pdev->dev, GFP_KERNEL,
+						    "acpi_pfru%d", pfru_dev->index);
+	if (!pfru_dev->miscdev.nodename) {
+		ret = -ENOMEM;
+		goto failed;
+	}
+
+	pfru_dev->miscdev.fops = &acpi_pfru_fops;
+	pfru_dev->miscdev.parent = &pdev->dev;
+
+	ret = misc_register(&pfru_dev->miscdev);
+	if (ret)
+		goto failed;
+
+	platform_set_drvdata(pdev, pfru_dev);
+
+	return 0;
+failed:
+	ida_free(&pfru_ida, pfru_dev->index);
+
+	return ret;
+}
+
+static const struct acpi_device_id acpi_pfru_ids[] = {
+	{"INTC1080", 0},
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, acpi_pfru_ids);

drivers/acpi/pfru/pfru_update.c new file +3 -0

+
+MODULE_DESCRIPTION("Platform Firmware Runtime Update device driver");
+MODULE_LICENSE("GPL v2");

include/uapi/linux/pfru.h new file +30 -0

new file mode 100644
index 000000000000..d6f57a75afd2
@@ -0,0 +1,186 @@
+/* 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/types.h>
+#include <linux/ioctl.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 UUID_SIZE 16

include/uapi/linux/pfru.h new file +24 -0

+ *
+ * Stage a capsule image from communication buffer and perform authentication.
+ */
+#define PFRU_IOC_STAGE _IOW(PFRU_MAGIC, 0x02, unsigned int)
+/**
+ * PFRU_IOC_ACTIVATE - _IOW(PFRU_MAGIC, 0x03, unsigned int)
+ *
+ * Return: 0 on success, -errno on failure
+ *
+ * Activate a previous staged capsule image.
+ */
+#define PFRU_IOC_ACTIVATE _IOW(PFRU_MAGIC, 0x03, unsigned int)
+/**
+ * PFRU_IOC_STAGE_ACTIVATE - _IOW(PFRU_MAGIC, 0x04, unsigned int)
+ *
+ * Return: 0 on success, -errno on failure
+ *
+ * Perform both stage and activation actions.
+ */
+#define PFRU_IOC_STAGE_ACTIVATE _IOW(PFRU_MAGIC, 0x04, unsigned int)
+/**
+ * PFRU_IOC_QUERY_CAP - _IOR(PFRU_MAGIC, 0x05,
+ *			     struct pfru_update_cap_info)
+ *

include/uapi/linux/pfru.h new file +26 -0

+}
+
+/**
+ * struct pfru_payload_hdr - Capsule file payload header.
+ *
+ * @sig: Signature of this capsule file.
+ * @hdr_version: Revision of this header structure.
+ * @hdr_size: Size of this header, including the OemHeader bytes.
+ * @hw_ver: The supported firmware version.
+ * @rt_ver: Version of the code injection image.
+ * @platform_id: A platform specific GUID to specify the platform what
+ *               this capsule image support.
+ */
+struct pfru_payload_hdr {
+	__u32 sig;
+	__u32 hdr_version;
+	__u32 hdr_size;
+	__u32 hw_ver;
+	__u32 rt_ver;
+	__u8 platform_id[UUID_SIZE];
+};
+
+enum pfru_start_action {
+	START_STAGE,
+	START_ACTIVATE,
+	START_STAGE_ACTIVATE,

include/uapi/linux/pfru.h new file +12 -0

+};
+
+/**
+ * struct pfru_update_cap_info - Runtime update capability information.
+ *
+ * @status: Indicator of whether this query succeed.
+ * @update_cap: Bitmap to indicate whether the feature is supported.
+ * @code_type: A buffer containing an image type GUID.
+ * @fw_version: Platform firmware version.
+ * @code_rt_version: Code injection runtime version for anti-rollback.
+ * @drv_type: A buffer containing an image type GUID.
+ * @drv_rt_version: The version of the driver update runtime code.

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

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

EFI_CAPSULE_HEADER
EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER
EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER
EFI_FIRMWARE_IMAGE_AUTHENTICATION

These structures would be used by the Platform Firmware Runtime Update driver to parse the format of capsule file to verify if the corresponding version number is valid. The EFI_CAPSULE_HEADER has been defined in the kernel, however the rest are not, thus introduce corresponding UEFI structures accordingly. Besides, EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER and EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER are required to be packed in the uefi specification. And Ard has pointed out that, the __packed attribute does indicate to the compiler that the entire thing can appear misaligned in memory. So if one follows the other in the capsule header, the __packed attribute may be appropriate to ensure that the second one is not accessed using misaligned loads and stores.

include/linux/efi.h +9 -0

+	u8	image_index;
+	u8	reserved_bytes[3];
+	u32	image_size;
+	u32	vendor_code_size;
+	/* ver = 2. */
+	u64	hw_ins;
+	/* ver = v3. */
+	u64	capsule_support;
+} __packed;

(quoted diff) +2 -0

+	u8		cert_data[];
+};

Introduce the pfru_update driver which can be used for Platform Firmware Runtime code injection and driver update [1]. The user is expected to provide the update firmware in the form of capsule file, and pass it to the driver via ioctl. Then the driver would hand this capsule file to the Platform Firmware Runtime Update via the ACPI device _DSM method. At last the low level Management Mode would do the firmware update.

The corresponding userspace tool and man page will be introduced at tools/power/acpi/pfru.

drivers/acpi/pfru/pfru_update.c new file +9 -0

+	/* check whether this is a code injection or driver update */
+	if (guid_equal(image_type_id, &pfru_dev->code_uuid))
+		return CODE_INJECT_TYPE;
+
+	if (guid_equal(image_type_id, &pfru_dev->drv_uuid))
+		return DRIVER_UPDATE_TYPE;
+
+	return -EINVAL;
+}

drivers/acpi/pfru/pfru_update.c new file +5 -0

+static bool valid_version(const void *data, struct pfru_update_cap_info *cap,
+			  struct pfru_device *pfru_dev)
+{
+	struct pfru_payload_hdr *payload_hdr;
+	efi_capsule_header_t *cap_hdr;

drivers/acpi/pfru/pfru_update.c new file +28 -0

+	size = cap_hdr->headersize;
+	m_hdr = (struct efi_manage_capsule_header *)(data + size);
+	/*
+	 * Current data structure size plus variable array indicated
+	 * by number of (emb_drv_cnt + payload_cnt)
+	 */
+	size += sizeof(struct efi_manage_capsule_header) +
+		      (m_hdr->emb_drv_cnt + m_hdr->payload_cnt) * sizeof(u64);
+	m_img_hdr = (struct efi_manage_capsule_image_header *)(data + size);
+
+	type = get_image_type(m_img_hdr, pfru_dev);
+	if (type < 0)
+		return false;
+
+	size = adjust_efi_size(m_img_hdr, size);
+	if (size < 0)
+		return false;
+
+	auth = (struct efi_image_auth *)(data + size);
+	size += sizeof(u64) + auth->auth_info.hdr.len;
+	payload_hdr = (struct pfru_payload_hdr *)(data + size);
+
+	/* finally compare the version */
+	if (type == CODE_INJECT_TYPE)
+		return payload_hdr->rt_ver >= cap->code_rt_version;
+	else
+		return payload_hdr->rt_ver >= cap->drv_rt_version;
+}

+}

drivers/acpi/pfru/pfru_update.c new file +5 -0

+	ret = ida_alloc(&pfru_ida, GFP_KERNEL);
+	if (ret < 0)
+		return ret;
+
+	pfru_dev->index = ret;

drivers/acpi/pfru/pfru_update.c new file +1 -0

+	pfru_dev->rev_id = 1;

+failed:

drivers/acpi/pfru/pfru_update.c new file +4 -0

+	ida_free(&pfru_ida, pfru_dev->index);
+
+	return ret;
+}

+#define UUID_SIZE 16

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.

The corresponding userspace tool and man page will be introduced at tools/power/acpi/pfru.

drivers/acpi/pfru/pfru_telemetry.c new file +4 -0

+#include <linux/acpi.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/errno.h>

+#include <uapi/linux/pfru.h>

+static DEFINE_IDA(pfru_log_ida);

+}

Quoted 13 lines · Chen Yu

Introduce the pfru_update driver which can be used for Platform Firmware Runtime code injection and driver update [1]. The user is expected to provide the update firmware in the form of capsule file, and pass it to the driver via ioctl. Then the driver would hand this capsule file to the Platform Firmware Runtime Update via the ACPI device _DSM method. At last the low level Management Mode would do the firmware update.

The corresponding userspace tool and man page will be introduced at tools/power/acpi/pfru.

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

Signed-off-by Chen Yu <[email protected]>

---

drivers/acpi/pfru/pfru_update.c line 61

Quoted 3 lines

drivers/acpi/pfru/pfru_update.c new file +3 -0

+struct pfru_device {
+	guid_t uuid, code_uuid, drv_uuid;
+	int rev_id, index;

shouldn't rev_id be a u32 as you are copying this to/from userspace?

drivers/acpi/pfru/pfru_update.c line 62

Quoted 1 line

drivers/acpi/pfru/pfru_update.c new file +1 -0

+	struct device *dev;

What device is this, the parent? Child? Sibling? need a hint...

Quoted 11 lines

drivers/acpi/pfru/pfru_update.c new file +11 -0

+	struct miscdevice miscdev;
+};
+
+static DEFINE_IDA(pfru_ida);
+
+static inline struct pfru_device *to_pfru_dev(struct file *file)
+{
+	return container_of(file->private_data, struct pfru_device, miscdev);
+}
+
+

drivers/acpi/pfru/pfru_update.c line 380

Quoted 3 lines

drivers/acpi/pfru/pfru_update.c new file +3 -0

+static long pfru_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	struct pfru_update_cap_info cap;

You have a variable-length structure on the stack. What ever could go wrong with that?

{sigh}

How was this tested???

Quoted 4 lines

drivers/acpi/pfru/pfru_update.c new file +4 -0

+	struct pfru_device *pfru_dev;
+	void __user *p;
+	int ret, rev;
+

drivers/acpi/pfru/pfru_update.c line 394

Quoted 10 lines

drivers/acpi/pfru/pfru_update.c new file +10 -0

+	pfru_dev = to_pfru_dev(file);
+	p = (void __user *)arg;
+
+	switch (cmd) {
+	case PFRU_IOC_QUERY_CAP:
+		ret = query_capability(&cap, pfru_dev);
+		if (ret)
+			return ret;
+
+		if (copy_to_user(p, &cap, sizeof(cap)))

But "sizeof" is not the real size of this structure, it's a variable length structure, right?

Again, how was this tested?

drivers/acpi/pfru/pfru_update.c line 399

Quoted 5 lines

drivers/acpi/pfru/pfru_update.c new file +5 -0

+			return -EFAULT;
+
+		return 0;
+	case PFRU_IOC_SET_REV:
+		if (copy_from_user(&rev, p, sizeof(unsigned int)))

But this isn't an unsigned int, it's an "int" here.

So again, make this explicit, probably u32, right?

drivers/acpi/pfru/pfru_update.c line 405

Quoted 6 lines

drivers/acpi/pfru/pfru_update.c new file +6 -0

+			return -EFAULT;
+
+		if (!pfru_valid_revid(rev))
+			return -EINVAL;
+
+		pfru_dev->rev_id = rev;

Why can't this just be a sysfs file?

Heck, why can't all of these be sysfs files, why do you need a char device at all? Who is going to use this thing?

Quoted 22 lines

drivers/acpi/pfru/pfru_update.c new file +22 -0

+
+		return 0;
+	case PFRU_IOC_STAGE:
+		return start_acpi_update(START_STAGE, pfru_dev);
+	case PFRU_IOC_ACTIVATE:
+		return start_acpi_update(START_ACTIVATE, pfru_dev);
+	case PFRU_IOC_STAGE_ACTIVATE:
+		return start_acpi_update(START_STAGE_ACTIVATE, pfru_dev);
+	default:
+		return -ENOTTY;
+	}
+}
+
+static int acpi_pfru_remove(struct platform_device *pdev)
+{
+	struct pfru_device *pfru_dev = platform_get_drvdata(pdev);
+
+	misc_deregister(&pfru_dev->miscdev);
+	ida_free(&pfru_ida, pfru_dev->index);
+
+	return 0;
+}

drivers/acpi/pfru/pfru_update.c line 569

Quoted 4 lines

drivers/acpi/pfru/pfru_update.c new file +4 -0

+	.probe = acpi_pfru_probe,
+	.remove = acpi_pfru_remove,
+};
+module_platform_driver(acpi_pfru_driver);

Are you cleaning up your ida structure when the module is unloaded?

Quoted 19 lines

drivers/acpi/pfru/pfru_update.c new file +3 -0

+
+MODULE_DESCRIPTION("Platform Firmware Runtime Update device driver");
+MODULE_LICENSE("GPL v2");

include/uapi/linux/pfru.h new file +10 -0

new file mode 100644
index 000000000000..d6f57a75afd2
@@ -0,0 +1,186 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Platform Firmware Runtime Update header
+ *
+ * Copyright(c) 2021 Intel Corporation. All rights reserved.
+ */
+
+/**
+ * PFRU_IOC_SET_REV - _IOW(PFRU_MAGIC, 0x01, unsigned int)
+ *

include/uapi/linux/pfru.h line 40

Quoted 6 lines

include/uapi/linux/pfru.h new file +6 -0

+ * Return: 0 on success, -errno on failure
+ *
+ * Set the Revision ID for PFRU Runtime Update. It could be either 1 or 2.
+ */
+#define PFRU_IOC_SET_REV _IOW(PFRU_MAGIC, 0x01, unsigned int)
+/**

Please use blank lines between these, it's hard to read.

include/uapi/linux/pfru.h line 43

Quoted 3 lines

include/uapi/linux/pfru.h new file +3 -0

+ * PFRU_IOC_STAGE - _IOW(PFRU_MAGIC, 0x02, unsigned int)
+ *
+ * Return: 0 on success, -errno on failure

What -errno?

Same for all of these, be explicit please.

Quoted 29 lines

include/uapi/linux/pfru.h new file +29 -0

+ *
+ * Stage a capsule image from communication buffer and perform authentication.
+ */
+#define PFRU_IOC_STAGE _IOW(PFRU_MAGIC, 0x02, unsigned int)
+/**
+ * PFRU_IOC_ACTIVATE - _IOW(PFRU_MAGIC, 0x03, unsigned int)
+ *
+ * Return: 0 on success, -errno on failure
+ *
+ * Activate a previous staged capsule image.
+ */
+#define PFRU_IOC_ACTIVATE _IOW(PFRU_MAGIC, 0x03, unsigned int)
+/**
+ * PFRU_IOC_STAGE_ACTIVATE - _IOW(PFRU_MAGIC, 0x04, unsigned int)
+ *
+ * Return: 0 on success, -errno on failure
+ *
+ * Perform both stage and activation actions.
+ */
+#define PFRU_IOC_STAGE_ACTIVATE _IOW(PFRU_MAGIC, 0x04, unsigned int)
+/**
+ * PFRU_IOC_QUERY_CAP - _IOR(PFRU_MAGIC, 0x05,
+ *			     struct pfru_update_cap_info)
+ *
+ * Return: 0 on success, -errno on failure.
+ *
+ * Retrieve information about the PFRU Runtime Update capability.
+ * The information is a struct pfru_update_cap_info.
+ */

include/uapi/linux/pfru.h line 77

Quoted 5 lines

include/uapi/linux/pfru.h new file +5 -0

+#define PFRU_IOC_QUERY_CAP _IOR(PFRU_MAGIC, 0x05, struct pfru_update_cap_info)
+
+static inline int pfru_valid_revid(int id)
+{
+	return id == REVID_1 || id == REVID_2;

Why is this in a uapi file? You just poluted the global namespace of all userspace programs with this function :(

Quoted 31 lines

include/uapi/linux/pfru.h new file +31 -0

+}
+
+/**
+ * struct pfru_payload_hdr - Capsule file payload header.
+ *
+ * @sig: Signature of this capsule file.
+ * @hdr_version: Revision of this header structure.
+ * @hdr_size: Size of this header, including the OemHeader bytes.
+ * @hw_ver: The supported firmware version.
+ * @rt_ver: Version of the code injection image.
+ * @platform_id: A platform specific GUID to specify the platform what
+ *               this capsule image support.
+ */
+struct pfru_payload_hdr {
+	__u32 sig;
+	__u32 hdr_version;
+	__u32 hdr_size;
+	__u32 hw_ver;
+	__u32 rt_ver;
+	__u8 platform_id[UUID_SIZE];
+};
+
+enum pfru_start_action {
+	START_STAGE,
+	START_ACTIVATE,
+	START_STAGE_ACTIVATE,
+};
+
+enum pfru_dsm_status {
+	DSM_SUCCEED,
+	DSM_FUNC_NOT_SUPPORT,

include/uapi/linux/pfru.h line 113

Quoted 5 lines

include/uapi/linux/pfru.h new file +5 -0

+	DSM_INVAL_INPUT,
+	DSM_HARDWARE_ERR,
+	DSM_RETRY_SUGGESTED,
+	DSM_UNKNOWN,
+	DSM_FUNC_SPEC_ERR,

Do you need to be explicit about the numbers here? I never remember if this is guaranteed by all C compilers or not...

Quoted 16 lines

include/uapi/linux/pfru.h new file +16 -0

+};
+
+/**
+ * struct pfru_update_cap_info - Runtime update capability information.
+ *
+ * @status: Indicator of whether this query succeed.
+ * @update_cap: Bitmap to indicate whether the feature is supported.
+ * @code_type: A buffer containing an image type GUID.
+ * @fw_version: Platform firmware version.
+ * @code_rt_version: Code injection runtime version for anti-rollback.
+ * @drv_type: A buffer containing an image type GUID.
+ * @drv_rt_version: The version of the driver update runtime code.
+ * @drv_svn: The secure version number(SVN) of the driver update runtime code.
+ * @platform_id: A buffer containing a platform ID GUID.
+ * @oem_id: A buffer containing an OEM ID GUID.
+ * @oem_info: A buffer containing the vendor specific information.

include/uapi/linux/pfru.h line 135

Quoted 6 lines

include/uapi/linux/pfru.h new file +6 -0

+ */
+struct pfru_update_cap_info {
+	__u32 status;
+	__u32 update_cap;
+
+	__u8 code_type[UUID_SIZE];

Don't we have a uuid structure that crosses the user/kernel boundry?

Quoted 6 lines

include/uapi/linux/pfru.h new file +6 -0

+	__u32 fw_version;
+	__u32 code_rt_version;
+
+	__u8 drv_type[UUID_SIZE];
+	__u32 drv_rt_version;
+	__u32 drv_svn;

include/uapi/linux/pfru.h line 146

Quoted 5 lines

include/uapi/linux/pfru.h new file +5 -0

+
+	__u8 platform_id[UUID_SIZE];
+	__u8 oem_id[UUID_SIZE];
+
+	char oem_info[];

As mentioned above, this is variable length, right? Where are you copying this extra data and how big do you know it is going to be?

Quoted 9 lines · Chen Yu

Introduce the pfru_update driver which can be used for Platform Firmware Runtime code injection and driver update [1]. The user is expected to provide the update firmware in the form of capsule file, and pass it to the driver via ioctl. Then the driver would hand this capsule file to the Platform Firmware Runtime Update via the ACPI device _DSM method. At last the low level Management Mode would do the firmware update.

The corresponding userspace tool and man page will be introduced at tools/power/acpi/pfru.

...

drivers/acpi/pfru/pfru_update.c line 214

Quoted 4 lines

drivers/acpi/pfru/pfru_update.c new file +4 -0

+static int get_image_type(struct efi_manage_capsule_image_header *img_hdr,
+			  struct pfru_device *pfru_dev)
+{
+	guid_t *image_type_id = &img_hdr->image_type_id;

efi_guid_t ?

Quoted 9 lines

drivers/acpi/pfru/pfru_update.c new file +9 -0

+	/* check whether this is a code injection or driver update */
+	if (guid_equal(image_type_id, &pfru_dev->code_uuid))
+		return CODE_INJECT_TYPE;
+
+	if (guid_equal(image_type_id, &pfru_dev->drv_uuid))
+		return DRIVER_UPDATE_TYPE;
+
+	return -EINVAL;
+}

...

Quoted 9 lines

drivers/acpi/pfru/pfru_update.c new file +9 -0

+static bool valid_version(const void *data, struct pfru_update_cap_info *cap,
+			  struct pfru_device *pfru_dev)
+{
+	struct pfru_payload_hdr *payload_hdr;
+	efi_capsule_header_t *cap_hdr;
+	struct efi_manage_capsule_header *m_hdr;
+	struct efi_manage_capsule_image_header *m_img_hdr;
+	struct efi_image_auth *auth;
+	int type, size;

drivers/acpi/pfru/pfru_update.c line 262

Quoted 6 lines

drivers/acpi/pfru/pfru_update.c new file +6 -0

+
+	/*
+	 * Sanity check if the capsule image has a newer version
+	 * than current one.
+	 */
+	cap_hdr = (efi_capsule_header_t *)data;

Why casting?

Quoted 28 lines

drivers/acpi/pfru/pfru_update.c new file +28 -0

+	size = cap_hdr->headersize;
+	m_hdr = (struct efi_manage_capsule_header *)(data + size);
+	/*
+	 * Current data structure size plus variable array indicated
+	 * by number of (emb_drv_cnt + payload_cnt)
+	 */
+	size += sizeof(struct efi_manage_capsule_header) +
+		      (m_hdr->emb_drv_cnt + m_hdr->payload_cnt) * sizeof(u64);
+	m_img_hdr = (struct efi_manage_capsule_image_header *)(data + size);
+
+	type = get_image_type(m_img_hdr, pfru_dev);
+	if (type < 0)
+		return false;
+
+	size = adjust_efi_size(m_img_hdr, size);
+	if (size < 0)
+		return false;
+
+	auth = (struct efi_image_auth *)(data + size);
+	size += sizeof(u64) + auth->auth_info.hdr.len;
+	payload_hdr = (struct pfru_payload_hdr *)(data + size);
+
+	/* finally compare the version */
+	if (type == CODE_INJECT_TYPE)
+		return payload_hdr->rt_ver >= cap->code_rt_version;
+	else
+		return payload_hdr->rt_ver >= cap->drv_rt_version;
+}

...

Quoted 6 lines

drivers/acpi/pfru/pfru_update.c new file +6 -0

+static long pfru_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	struct pfru_update_cap_info cap;
+	struct pfru_device *pfru_dev;
+	void __user *p;
+	int ret, rev;

drivers/acpi/pfru/pfru_update.c line 386

Quoted 2 lines

drivers/acpi/pfru/pfru_update.c new file +2 -0

+	pfru_dev = to_pfru_dev(file);
+	p = (void __user *)arg;

Can be combined with definitions above. Ditto for the rest cases in the code.

Quoted 1 line

+}

...

drivers/acpi/pfru/pfru_update.c line 445

Quoted 1 line

drivers/acpi/pfru/pfru_update.c new file +1 -0

+	phy_addr = (phys_addr_t)(info.addr_lo | (info.addr_hi << 32));

Does it compile without warnings for 32-bit target?

...

Quoted 7 lines

drivers/acpi/pfru/pfru_update.c new file +7 -0

+	ret = guid_parse(PFRU_UUID, &pfru_dev->uuid);
+	if (ret)
+		return ret;
+
+	ret = guid_parse(PFRU_CODE_INJ_UUID, &pfru_dev->code_uuid);
+	if (ret)
+		return ret;

drivers/acpi/pfru/pfru_update.c line 513

Quoted 4 lines

drivers/acpi/pfru/pfru_update.c new file +4 -0

+
+	ret = guid_parse(PFRU_DRV_UPDATE_UUID, &pfru_dev->drv_uuid);
+	if (ret)
+		return ret;

Why do you need to keep zillions of copies of the data which seems is not going to be changed? Three global variables should be enough, no?

Quoted 5 lines

drivers/acpi/pfru/pfru_update.c new file +5 -0

+	ret = ida_alloc(&pfru_ida, GFP_KERNEL);
+	if (ret < 0)
+		return ret;
+
+	pfru_dev->index = ret;

...

drivers/acpi/pfru/pfru_update.c line 520

Quoted 1 line

drivers/acpi/pfru/pfru_update.c new file +1 -0

+	/* default rev id is 1 */

Shouldn't you rather define this magic and drop this doubtful comment?

Quoted 1 line

drivers/acpi/pfru/pfru_update.c new file +1 -0

+	pfru_dev->rev_id = 1;

...

Quoted 1 line

+failed:

Make you labeling consistent. The usual pattern is to explain what will be happened when goto to the certain label, for example, here is 'err_free_ida'. Also, add an empty line everywhere before labels.

Quoted 4 lines

drivers/acpi/pfru/pfru_update.c new file +4 -0

+	ida_free(&pfru_ida, pfru_dev->index);
+
+	return ret;
+}

...

Quoted 1 line

+#define UUID_SIZE 16

It must not be here at all.
Or it should be properly namespaced.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Quoted 5 lines · Chen Yu

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

The linux provides interfaces for the user to query the parameters of

Linux kernel

Quoted 5 lines

telemetry data, and the user could read out the telemetry data accordingly.

The corresponding userspace tool and man page will be introduced at tools/power/acpi/pfru.

...

Quoted 4 lines

drivers/acpi/pfru/pfru_telemetry.c new file +4 -0

+#include <linux/acpi.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/errno.h>

drivers/acpi/pfru/pfru_telemetry.c line 21

Quoted 10 lines

drivers/acpi/pfru/pfru_telemetry.c new file +10 -0

+#include <linux/file.h>
+#include <linux/fs.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/mm.h>
+#include <linux/platform_device.h>
+#include <linux/string.h>
+#include <linux/uaccess.h>
+#include <linux/uio.h>
+#include <linux/uuid.h>

+ blank line?

Quoted 1 line

+#include <uapi/linux/pfru.h>

...

Quoted 1 line

+static DEFINE_IDA(pfru_log_ida);

Do you need any mutex against operations on IDA? (I don't remember if it incorporates any synchronization primitives).

...

Looking into the code I have feelings of déjà-vu. Has it really had nothing in common with the previous patch?

...

drivers/acpi/pfru/pfru_telemetry.c line 238

Quoted 4 lines

drivers/acpi/pfru/pfru_telemetry.c new file +4 -0

+static int valid_log_level(int level)
+{
+	return level == LOG_ERR || level == LOG_WARN ||
+		level == LOG_INFO || level == LOG_VERB;

Indentation.

Quoted 1 line

+}

...

This ordering in ->probe() is not okay:

	devm_*()
	non-devm_*()
	devm_*()
	non-devm_*()

One mustn't interleave these. The allowed are:

Case 1:

	non-devm_*()

Case 2:

	devm_*()

Case 3:

	devm_*()
	non-devm_*()

Otherwise in ->remove() you have wrong release ordering which may hide subtle bugs.

Above comment is applicable to the other patch as well as some comments from there are applicable here.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Quoted 22 lines · Chen Yu

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

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

EFI_CAPSULE_HEADER
EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER
EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER
EFI_FIRMWARE_IMAGE_AUTHENTICATION

These structures would be used by the Platform Firmware Runtime Update driver to parse the format of capsule file to verify if the corresponding version number is valid. The EFI_CAPSULE_HEADER has been defined in the kernel, however the rest are not, thus introduce corresponding UEFI structures accordingly. Besides, EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER and EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER are required to be packed in the uefi specification. And Ard has pointed out that, the __packed attribute does indicate to the compiler that the entire thing can appear misaligned in memory. So if one follows the other in the capsule header, the __packed attribute may be appropriate to ensure that the second one is not accessed using misaligned loads and stores.

...

include/linux/efi.h typedef struct { · line 166

Quoted 5 lines

include/linux/efi.h +4 -0

+/* EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER */
+struct efi_manage_capsule_image_header {
+	u32	ver;
 
+	guid_t	image_type_id;

Shouldn't it be efi_guid_t ?

Quoted 1 line · Andy Shevchenko

On Wed, Oct 27, 2021 at 03:07:51PM +0800, Chen Yu wrote:

Quoted 4 lines

drivers/acpi/pfru/pfru_update.c line 214

Quoted 1 line

drivers/acpi/pfru/pfru_update.c new file +1 -0

+	guid_t *image_type_id = &img_hdr->image_type_id;

efi_guid_t ?

efi_guid_t is a 32-bit aligned guid_t, which is for the case when efi_guid_t* arguments are 32-bit aligned. And it is for 32-bit ARM. Since this code injection is only for 64-bit, the guid is not required to be strictly 32-bit aligned I suppose?

Quoted 10 lines

drivers/acpi/pfru/pfru_update.c line 511

Quoted 5 lines

drivers/acpi/pfru/pfru_update.c new file +3 -0

+	ret = guid_parse(PFRU_UUID, &pfru_dev->uuid);
 
+	ret = guid_parse(PFRU_CODE_INJ_UUID, &pfru_dev->code_uuid);
 
+	ret = guid_parse(PFRU_DRV_UPDATE_UUID, &pfru_dev->drv_uuid);

Why do you need to keep zillions of copies of the data which seems is not going to be changed? Three global variables should be enough, no?

The guid information is embedded in each pfru_dev and only calculated once during probe. I thought people try to avoid using global variables if possible?

Quoted 1 line · Andy Shevchenko

On Wed, Oct 27, 2021 at 03:08:05PM +0800, Chen Yu wrote:

Quoted 3 lines

Looking into the code I have feelings of déjà-vu. Has it really had nothing in common with the previous patch?

They both invokes _DSM to trigger the low level actions. However the input parameters and return ACPI package as well as the functions are different and hard to extract the common code between them.

Quoted 2 lines · Chen Yu

Quoted 1 line · Andy Shevchenko

On Wed, Oct 27, 2021 at 03:07:51PM +0800, Chen Yu wrote:

...

Quoted 8 lines

Quoted 4 lines

drivers/acpi/pfru/pfru_update.c line 214

Quoted 1 line

drivers/acpi/pfru/pfru_update.c new file +1 -0

+	guid_t *image_type_id = &img_hdr->image_type_id;

efi_guid_t ?

efi_guid_t is a 32-bit aligned guid_t, which is for the case when efi_guid_t* arguments are 32-bit aligned. And it is for 32-bit ARM. Since this code injection is only for 64-bit, the guid is not required to be strictly 32-bit aligned I suppose?

But the type of that member may not be guid_t. So, I expect not to see guid_t at all here or as a internal member with the export/import GUID API in place.

See, for example
d1c6e08e7503 ("libnvdimm/labels: Add uuid helpers")

...

Quoted 10 lines

Quoted 10 lines

drivers/acpi/pfru/pfru_update.c line 511

Quoted 5 lines

drivers/acpi/pfru/pfru_update.c new file +3 -0

+	ret = guid_parse(PFRU_UUID, &pfru_dev->uuid);
 
+	ret = guid_parse(PFRU_CODE_INJ_UUID, &pfru_dev->code_uuid);
 
+	ret = guid_parse(PFRU_DRV_UPDATE_UUID, &pfru_dev->drv_uuid);

Why do you need to keep zillions of copies of the data which seems is not going to be changed? Three global variables should be enough, no?

Quoted 3 lines

The guid information is embedded in each pfru_dev and only calculated once during probe. I thought people try to avoid using global variables if possible?

Use your common sense. You might have a lot of data duplication, and this data is definitely not something that needs any kind of serialization / personification / etc.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Quoted 15 lines · Andy Shevchenko

Quoted 2 lines · Chen Yu

Quoted 1 line · Andy Shevchenko

On Wed, Oct 27, 2021 at 03:07:51PM +0800, Chen Yu wrote:

...

Quoted 8 lines

Quoted 4 lines

drivers/acpi/pfru/pfru_update.c line 214

Quoted 1 line

drivers/acpi/pfru/pfru_update.c new file +1 -0

+	guid_t *image_type_id = &img_hdr->image_type_id;

efi_guid_t ?

efi_guid_t is a 32-bit aligned guid_t, which is for the case when efi_guid_t* arguments are 32-bit aligned. And it is for 32-bit ARM. Since this code injection is only for 64-bit, the guid is not required to be strictly 32-bit aligned I suppose?

Quoted 6 lines

But the type of that member may not be guid_t. So, I expect not to see guid_t at all here or as a internal member with the export/import GUID API in place.

See, for example
d1c6e08e7503 ("libnvdimm/labels: Add uuid helpers")

8b03aa0e0e5a ("libnvdimm/labels: Add type-guid helpers") I suppose. Do you mean, since the label is not delcared as uuid type, then need to treat the data region as a uuid. But it is actually not a formal 'uuid' type, so uuid_t is not applicable here. And this also applys to the case in this patch?

Quoted 16 lines

...

Quoted 13 lines

Quoted 10 lines

drivers/acpi/pfru/pfru_update.c line 511

Quoted 5 lines

drivers/acpi/pfru/pfru_update.c new file +3 -0

+	ret = guid_parse(PFRU_UUID, &pfru_dev->uuid);
 
+	ret = guid_parse(PFRU_CODE_INJ_UUID, &pfru_dev->code_uuid);
 
+	ret = guid_parse(PFRU_DRV_UPDATE_UUID, &pfru_dev->drv_uuid);

Why do you need to keep zillions of copies of the data which seems is not going to be changed? Three global variables should be enough, no?

The guid information is embedded in each pfru_dev and only calculated once during probe. I thought people try to avoid using global variables if possible?

Quoted 4 lines

Use your common sense. You might have a lot of data duplication, and this data is definitely not something that needs any kind of serialization / personification / etc.

Ok, I'll switch to global variables in next version.

Quoted 1 line · Chen Yu

On Mon, Nov 01, 2021 at 01:21:36PM +0200, Andy Shevchenko wrote:

Quoted 1 line

Ok, I'll switch to global variables in next version.

Wait, no, why?

Keep them per-device unless you can somehow be
guaranteed there will never be more than one of these ACPI devices in a system.

It's simpler this way, no need to worry about global state at all.

Quoted 2 lines · Greg Kroah-Hartman

Quoted 1 line · Chen Yu

On Mon, Nov 01, 2021 at 01:21:36PM +0200, Andy Shevchenko wrote:

...

Quoted 3 lines

Quoted 1 line

Ok, I'll switch to global variables in next version.

Wait, no, why?

But why should we have a duplication of basically static data?

Quoted 3 lines

Keep them per-device unless you can somehow be
guaranteed there will never be more than one of these ACPI devices in a system.

I guess you missed my point. These definitions are _always_ the same. It does not matter how many devices in the system.

Chen, is my perception correct?
If no, then do what Greg says.

Quoted 2 lines

It's simpler this way, no need to worry about global state at all.

Actually I have no idea why we even have strings in and not raw buffers. Moreover, I haven't got why even we have them in uAPI.

Signature · 3 lines

-- 
With Best Regards,
Andy Shevchenko

Quoted 1 line · Andy Shevchenko

On Mon, Nov 01, 2021 at 02:16:41PM +0100, Greg Kroah-Hartman wrote:

Quoted 2 lines

Actually I have no idea why we even have strings in and not raw buffers. Moreover, I haven't got why even we have them in uAPI.

I see. These uuid could be put into the .c and there is no need for the user to be aware of these values.