[PATCH v4 0/5] Add support for the TPM FF-A start method

[PATCH v4 0/5] Add support for the TPM FF-A start method

[Stuart Yoder]

Firmware Framework for Arm A-profile (FF-A) is a messaging framework
for Arm-based systems, and in the context of the TPM CRB driver is used
to signal 'start' to a CRB-based TPM service which is hosted in an
FF-A secure partition running in TrustZone.

These patches add support for the CRB FF-A start method defined
in the TCG ACPI specification v1.4 and the FF-A ABI defined
in the Arm TPM Service CRB over FF-A (DEN0138) specification:
https://developer.arm.com/documentation/den0138/latest/

The first patch adds an FF-A driver to handle the FF-A messaging when
communicating with a CRB-based TPM secure partition built on FF-A.
The driver is probed when the TPM secure partition is discovered by
the Linux FF-A infrastructure.

The second patch consolidates the check for idle support in the CRB
driver to one place.

The third patch defines the new ACPI start method enumeration for
CRB over FF-A.

The fourth patch adds support for the FF-A ACPI start method to
the TPM crb driver.

The fifth patch adds documentation explaining how the CRB driver
and FF-A relate.

Version 4
-fix warning from kernel test robot in patch 1
-fix warnings from checkpatch.pl --strict
-clean up unecessary parenthesis usage
-update variable declaration to be reverse tree order
-document exported functions in tpm_crb_ffa driver
-remove unnecessary author and maintainer info in tpm_crb_ffa driver
-fix declaration of variables to be in reverse tree order

Version 3
-changed prefixes used throughout patch series to tpm_crb_ffa*

Version 2
-updates to cover letter to define FF-A
-added new patch with documentation
-created pull request in ACPIA and added link to the patch
 updating actbl3.h
-added tpm_ prefix to the FF-A CRB driver

Stuart Yoder (5):
  tpm_crb: implement driver compliant to CRB over FF-A
  tpm_crb: clean-up and refactor check for idle support
  ACPICA: add start method for Arm FF-A
  tpm_crb: add support for the Arm FF-A start method
  Documentation: tpm: add documentation for the CRB FF-A interface

 Documentation/security/tpm/tpm_ffa_crb.rst |  65 ++++
 drivers/char/tpm/Kconfig                   |   9 +
 drivers/char/tpm/Makefile                  |   1 +
 drivers/char/tpm/tpm_crb.c                 | 105 +++++--
 drivers/char/tpm/tpm_crb_ffa.c             | 348 +++++++++++++++++++++
 drivers/char/tpm/tpm_crb_ffa.h             |  25 ++
 include/acpi/actbl3.h                      |   1 +
 7 files changed, 535 insertions(+), 19 deletions(-)
 create mode 100644 Documentation/security/tpm/tpm_ffa_crb.rst
 create mode 100644 drivers/char/tpm/tpm_crb_ffa.c
 create mode 100644 drivers/char/tpm/tpm_crb_ffa.h

-- 
2.34.1

[PATCH v4 1/5] tpm_crb: implement driver compliant to CRB over FF-A

[Stuart Yoder]

The Arm specification TPM Service CRB over FF-A specification
defines the FF-A messages to interact with a CRB-based TPM
implemented as an FF-A secure partition.

Spec URL:
https://developer.arm.com/documentation/den0138/latest/

This driver is probed when a TPM Secure Partition is
discovered by the FF-A subsystem. It exposes APIs
used by the TPM CRB driver to send notifications to
the TPM.

Signed-off-by: Stuart Yoder <stuart.yoder@arm.com>
---
 drivers/char/tpm/Kconfig       |   9 +
 drivers/char/tpm/Makefile      |   1 +
 drivers/char/tpm/tpm_crb_ffa.c | 348 +++++++++++++++++++++++++++++++++
 drivers/char/tpm/tpm_crb_ffa.h |  25 +++
 4 files changed, 383 insertions(+)
 create mode 100644 drivers/char/tpm/tpm_crb_ffa.c
 create mode 100644 drivers/char/tpm/tpm_crb_ffa.h

diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index 0fc9a510e059..4c85b8c00b12 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -210,6 +210,15 @@ config TCG_CRB
 	  from within Linux.  To compile this driver as a module, choose
 	  M here; the module will be called tpm_crb.
 
+config TCG_ARM_CRB_FFA
+	tristate "TPM CRB over Arm FF-A Transport"
+	depends on ARM_FFA_TRANSPORT
+	default y if (TCG_CRB && ARM_FFA_TRANSPORT)
+	help
+	  If the Arm FF-A transport is used to access the TPM say Yes.
+	  To compile this driver as a module, choose M here; the module
+	  will be called tpm_crb_ffa.
+
 config TCG_VTPM_PROXY
 	tristate "VTPM Proxy Interface"
 	depends on TCG_TPM
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 9bb142c75243..2b004df8c04b 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -42,5 +42,6 @@ obj-$(CONFIG_TCG_IBMVTPM) += tpm_ibmvtpm.o
 obj-$(CONFIG_TCG_TIS_ST33ZP24) += st33zp24/
 obj-$(CONFIG_TCG_XEN) += xen-tpmfront.o
 obj-$(CONFIG_TCG_CRB) += tpm_crb.o
+obj-$(CONFIG_TCG_ARM_CRB_FFA) += tpm_crb_ffa.o
 obj-$(CONFIG_TCG_VTPM_PROXY) += tpm_vtpm_proxy.o
 obj-$(CONFIG_TCG_FTPM_TEE) += tpm_ftpm_tee.o
diff --git a/drivers/char/tpm/tpm_crb_ffa.c b/drivers/char/tpm/tpm_crb_ffa.c
new file mode 100644
index 000000000000..bd3c6c41264b
--- /dev/null
+++ b/drivers/char/tpm/tpm_crb_ffa.c
@@ -0,0 +1,348 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2024 Arm Ltd.
+ *
+ * This device driver implements the TPM CRB start method
+ * as defined in the TPM Service Command Response Buffer
+ * Interface Over FF-A (DEN0138).
+ */
+
+#define pr_fmt(fmt) "CRB_FFA: " fmt
+
+#include <linux/arm_ffa.h>
+#include "tpm_crb_ffa.h"
+
+/* TPM service function status codes */
+#define CRB_FFA_OK			0x05000001
+#define CRB_FFA_OK_RESULTS_RETURNED	0x05000002
+#define CRB_FFA_NOFUNC			0x8e000001
+#define CRB_FFA_NOTSUP			0x8e000002
+#define CRB_FFA_INVARG			0x8e000005
+#define CRB_FFA_INV_CRB_CTRL_DATA	0x8e000006
+#define CRB_FFA_ALREADY			0x8e000009
+#define CRB_FFA_DENIED			0x8e00000a
+#define CRB_FFA_NOMEM			0x8e00000b
+
+#define CRB_FFA_VERSION_MAJOR	1
+#define CRB_FFA_VERSION_MINOR	0
+
+/* version encoding */
+#define CRB_FFA_MAJOR_VERSION_MASK  GENMASK(30, 16)
+#define CRB_FFA_MINOR_VERSION_MASK  GENMASK(15, 0)
+#define CRB_FFA_MAJOR_VERSION(x)    ((u16)(FIELD_GET(CRB_FFA_MAJOR_VERSION_MASK, (x))))
+#define CRB_FFA_MINOR_VERSION(x)    ((u16)(FIELD_GET(CRB_FFA_MINOR_VERSION_MASK, (x))))
+
+/*
+ * Normal world sends requests with FFA_MSG_SEND_DIRECT_REQ and
+ * responses are returned with FFA_MSG_SEND_DIRECT_RESP for normal
+ * messages.
+ *
+ * All requests with FFA_MSG_SEND_DIRECT_REQ and FFA_MSG_SEND_DIRECT_RESP
+ * are using the AArch32 SMC calling convention with register usage as
+ * defined in FF-A specification:
+ * w0:    Function ID (0x8400006F or 0x84000070)
+ * w1:    Source/Destination IDs
+ * w2:    Reserved (MBZ)
+ * w3-w7: Implementation defined, free to be used below
+ */
+
+/*
+ * Returns the version of the interface that is available
+ * Call register usage:
+ * w3:    Not used (MBZ)
+ * w4:    TPM service function ID, CRB_FFA_GET_INTERFACE_VERSION
+ * w5-w7: Reserved (MBZ)
+ *
+ * Return register usage:
+ * w3:    Not used (MBZ)
+ * w4:    TPM service function status
+ * w5:    TPM service interface version
+ *        Bits[31:16]: major version
+ *        Bits[15:0]: minor version
+ * w6-w7: Reserved (MBZ)
+ *
+ * Possible function status codes in register w4:
+ *     CRB_FFA_OK_RESULTS_RETURNED: The version of the interface has been
+ *                                  returned.
+ */
+#define CRB_FFA_GET_INTERFACE_VERSION 0x0f000001
+
+/*
+ * Return information on a given feature of the TPM service
+ * Call register usage:
+ * w3:    Not used (MBZ)
+ * w4:    TPM service function ID, CRB_FFA_START
+ * w5:    Start function qualifier
+ *            Bits[31:8] (MBZ)
+ *            Bits[7:0]
+ *              0: Notifies TPM that a command is ready to be processed
+ *              1: Notifies TPM that a locality request is ready to be processed
+ * w6:    TPM locality, one of 0..4
+ *            -If the start function qualifier is 0, identifies the locality
+ *             from where the command originated.
+ *            -If the start function qualifier is 1, identifies the locality
+ *             of the locality request
+ * w6-w7: Reserved (MBZ)
+ *
+ * Return register usage:
+ * w3:    Not used (MBZ)
+ * w4:    TPM service function status
+ * w5-w7: Reserved (MBZ)
+ *
+ * Possible function status codes in register w4:
+ *     CRB_FFA_OK: the TPM service has been notified successfully
+ *     CRB_FFA_INVARG: one or more arguments are not valid
+ *     CRB_FFA_INV_CRB_CTRL_DATA: CRB control data or locality control
+ *         data at the given TPM locality is not valid
+ *     CRB_FFA_DENIED: the TPM has previously disabled locality requests and
+ *         command processing at the given locality
+ */
+#define CRB_FFA_START 0x0f000201
+
+struct tpm_crb_ffa {
+	struct ffa_device *ffa_dev;
+	u16 major_version;
+	u16 minor_version;
+	struct mutex msg_data_lock; /* lock to protect sending of FF-A messages */
+	struct ffa_send_direct_data direct_msg_data;
+};
+
+static struct tpm_crb_ffa *tpm_crb_ffa;
+
+static int tpm_crb_ffa_to_linux_errno(int errno)
+{
+	int rc;
+
+	switch (errno) {
+	case CRB_FFA_OK:
+		rc = 0;
+		break;
+	case CRB_FFA_OK_RESULTS_RETURNED:
+		rc = 0;
+		break;
+	case CRB_FFA_NOFUNC:
+		rc = -ENOENT;
+		break;
+	case CRB_FFA_NOTSUP:
+		rc = -EPERM;
+		break;
+	case CRB_FFA_INVARG:
+		rc = -EINVAL;
+		break;
+	case CRB_FFA_INV_CRB_CTRL_DATA:
+		rc = -ENOEXEC;
+		break;
+	case CRB_FFA_ALREADY:
+		rc = -EEXIST;
+		break;
+	case CRB_FFA_DENIED:
+		rc = -EACCES;
+		break;
+	case CRB_FFA_NOMEM:
+		rc = -ENOMEM;
+		break;
+	default:
+		rc = -EINVAL;
+	}
+
+	return rc;
+}
+
+/**
+ * tpm_crb_ffa_init - called by the CRB driver to do any needed initialization
+ *
+ * This function is called by the tpm_crb driver during the tpm_crb
+ * driver's initialization. If the tpm_crb_ffa has not been probed
+ * yet, returns -ENOENT in order to force a retry.  If th ffa_crb
+ * driver had been probed  but failed with an error, returns -ENODEV
+ * in order to prevent further retries.
+ *
+ * Return: 0 on success, negative error code on failure.
+ */
+int tpm_crb_ffa_init(void)
+{
+	if (!tpm_crb_ffa)
+		return -ENOENT;
+
+	if (IS_ERR_VALUE(tpm_crb_ffa))
+		return -ENODEV;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(tpm_crb_ffa_init);
+
+static int __tpm_crb_ffa_send_recieve(unsigned long func_id,
+				      unsigned long a0,
+				      unsigned long a1,
+				      unsigned long a2)
+{
+	const struct ffa_msg_ops *msg_ops;
+	int ret;
+
+	if (!tpm_crb_ffa)
+		return -ENOENT;
+
+	msg_ops = tpm_crb_ffa->ffa_dev->ops->msg_ops;
+
+	memset(&tpm_crb_ffa->direct_msg_data, 0x00,
+	       sizeof(struct ffa_send_direct_data));
+
+	tpm_crb_ffa->direct_msg_data.data1 = func_id;
+	tpm_crb_ffa->direct_msg_data.data2 = a0;
+	tpm_crb_ffa->direct_msg_data.data3 = a1;
+	tpm_crb_ffa->direct_msg_data.data4 = a2;
+
+	ret = msg_ops->sync_send_receive(tpm_crb_ffa->ffa_dev,
+			&tpm_crb_ffa->direct_msg_data);
+	if (!ret)
+		ret = tpm_crb_ffa_to_linux_errno(tpm_crb_ffa->direct_msg_data.data1);
+
+	return ret;
+}
+
+/**
+ * tpm_crb_ffa_get_interface_version() - gets the ABI version of the TPM service
+ * @major: Pointer to caller-allocated buffer to hold the major version
+ *         number the ABI
+ * @minor: Pointer to caller-allocated buffer to hold the minor version
+ *         number the ABI
+ *
+ * Returns the major and minor version of the ABI of the FF-A based TPM.
+ * Allows the caller to evaluate its compatibility with the version of
+ * the ABI.
+ *
+ * Return: 0 on success, negative error code on failure.
+ */
+int tpm_crb_ffa_get_interface_version(u16 *major, u16 *minor)
+{
+	int rc;
+
+	if (!tpm_crb_ffa)
+		return -ENOENT;
+
+	if (IS_ERR_VALUE(tpm_crb_ffa))
+		return -ENODEV;
+
+	if (!major || !minor)
+		return -EINVAL;
+
+	guard(mutex)(&tpm_crb_ffa->msg_data_lock);
+
+	rc = __tpm_crb_ffa_send_recieve(CRB_FFA_GET_INTERFACE_VERSION, 0x00, 0x00, 0x00);
+	if (!rc) {
+		*major = CRB_FFA_MAJOR_VERSION(tpm_crb_ffa->direct_msg_data.data2);
+		*minor = CRB_FFA_MINOR_VERSION(tpm_crb_ffa->direct_msg_data.data2);
+	}
+
+	return rc;
+}
+EXPORT_SYMBOL_GPL(tpm_crb_ffa_get_interface_version);
+
+/**
+ * tpm_crb_ffa_start() - signals the TPM that a field has changed in the CRB
+ * @request_type: Identifies whether the change to the CRB is in the command
+ *                fields or locality fields.
+ * @locality: Specifies the locality number.
+ *
+ * Used by the CRB driver
+ * that might be useful to those using or modifying it.  Begins with
+ * empty comment line, and may include additional embedded empty
+ * comment lines.
+ *
+ * Return: 0 on success, negative error code on failure.
+ */
+int tpm_crb_ffa_start(int request_type, int locality)
+{
+	if (!tpm_crb_ffa)
+		return -ENOENT;
+
+	if (IS_ERR_VALUE(tpm_crb_ffa))
+		return -ENODEV;
+
+	guard(mutex)(&tpm_crb_ffa->msg_data_lock);
+
+	return __tpm_crb_ffa_send_recieve(CRB_FFA_START, request_type, locality, 0x00);
+}
+EXPORT_SYMBOL_GPL(tpm_crb_ffa_start);
+
+static int tpm_crb_ffa_probe(struct ffa_device *ffa_dev)
+{
+	struct tpm_crb_ffa *p;
+	int rc;
+
+	/* only one instance of a TPM partition is supported */
+	if (tpm_crb_ffa && !IS_ERR_VALUE(tpm_crb_ffa))
+		return -EEXIST;
+
+	tpm_crb_ffa = ERR_PTR(-ENODEV); // set tpm_crb_ffa so we can detect probe failure
+
+	if (!ffa_partition_supports_direct_recv(ffa_dev)) {
+		pr_err("TPM partition doesn't support direct message receive.\n");
+		return -EINVAL;
+	}
+
+	p = kzalloc(sizeof(*tpm_crb_ffa), GFP_KERNEL);
+	if (!p)
+		return -ENOMEM;
+	tpm_crb_ffa = p;
+
+	mutex_init(&tpm_crb_ffa->msg_data_lock);
+	tpm_crb_ffa->ffa_dev = ffa_dev;
+	ffa_dev_set_drvdata(ffa_dev, tpm_crb_ffa);
+
+	/* if TPM is aarch32 use 32-bit SMCs */
+	if (!ffa_partition_check_property(ffa_dev, FFA_PARTITION_AARCH64_EXEC))
+		ffa_dev->ops->msg_ops->mode_32bit_set(ffa_dev);
+
+	/* verify compatibility of TPM service version number */
+	rc = tpm_crb_ffa_get_interface_version(&tpm_crb_ffa->major_version,
+					       &tpm_crb_ffa->minor_version);
+	if (rc) {
+		pr_err("failed to get crb interface version. rc:%d", rc);
+		goto out;
+	}
+
+	pr_info("ABI version %u.%u", tpm_crb_ffa->major_version,
+		tpm_crb_ffa->minor_version);
+
+	if (tpm_crb_ffa->major_version != CRB_FFA_VERSION_MAJOR ||
+	   (tpm_crb_ffa->minor_version > 0 &&
+	    tpm_crb_ffa->minor_version < CRB_FFA_VERSION_MINOR)) {
+		pr_err("Incompatible ABI version");
+		goto out;
+	}
+
+	return 0;
+
+out:
+	kfree(tpm_crb_ffa);
+	tpm_crb_ffa = ERR_PTR(-ENODEV);
+	return -EINVAL;
+}
+
+static void tpm_crb_ffa_remove(struct ffa_device *ffa_dev)
+{
+	kfree(tpm_crb_ffa);
+	tpm_crb_ffa = NULL;
+}
+
+static const struct ffa_device_id tpm_crb_ffa_device_id[] = {
+	/* 17b862a4-1806-4faf-86b3-089a58353861 */
+	{ UUID_INIT(0x17b862a4, 0x1806, 0x4faf,
+		    0x86, 0xb3, 0x08, 0x9a, 0x58, 0x35, 0x38, 0x61) },
+	{}
+};
+
+static struct ffa_driver tpm_crb_ffa_driver = {
+	.name = "ffa-crb",
+	.probe = tpm_crb_ffa_probe,
+	.remove = tpm_crb_ffa_remove,
+	.id_table = tpm_crb_ffa_device_id,
+};
+
+module_ffa_driver(tpm_crb_ffa_driver);
+
+MODULE_AUTHOR("Arm");
+MODULE_DESCRIPTION("FFA CRB driver");
+MODULE_VERSION("1.0");
+MODULE_LICENSE("GPL");
diff --git a/drivers/char/tpm/tpm_crb_ffa.h b/drivers/char/tpm/tpm_crb_ffa.h
new file mode 100644
index 000000000000..23a9b93287a1
--- /dev/null
+++ b/drivers/char/tpm/tpm_crb_ffa.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2024 Arm Ltd.
+ *
+ * This device driver implements the TPM CRB start method
+ * as defined in the TPM Service Command Response Buffer
+ * Interface Over FF-A (DEN0138).
+ */
+#ifndef _TPM_CRB_FFA_H
+#define _TPM_CRB_FFA_H
+
+#if IS_ENABLED(CONFIG_TCG_ARM_CRB_FFA)
+int tpm_crb_ffa_init(void);
+int tpm_crb_ffa_get_interface_version(u16 *major, u16 *minor);
+int tpm_crb_ffa_start(int request_type, int locality);
+#else
+static inline int tpm_crb_ffa_init(void) { return 0; }
+static inline int tpm_crb_ffa_get_interface_version(u16 *major, u16 *minor) { return 0; }
+static inline int tpm_crb_ffa_start(int request_type, int locality) { return 0; }
+#endif
+
+#define CRB_FFA_START_TYPE_COMMAND 0
+#define CRB_FFA_START_TYPE_LOCALITY_REQUEST 1
+
+#endif
-- 
2.34.1

[PATCH v4 2/5] tpm_crb: clean-up and refactor check for idle support

[Stuart Yoder]

-Clean up unnecessary parenthesis usage
-Refactor the two checks for whether the TPM supports idle into a single
 inline function.

Signed-off-by: Stuart Yoder <stuart.yoder@arm.com>
---
 drivers/char/tpm/tpm_crb.c | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index ea085b14ab7c..31db879f1324 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -115,6 +115,16 @@ struct tpm2_crb_pluton {
 	u64 reply_addr;
 };
 
+/*
+ * Returns true if the start method supports idle.
+ */
+static inline bool tpm_crb_has_idle(u32 start_method)
+{
+	return start_method == ACPI_TPM2_START_METHOD ||
+	       start_method == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD ||
+	       start_method == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC;
+}
+
 static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
 				unsigned long timeout)
 {
@@ -173,9 +183,7 @@ static int __crb_go_idle(struct device *dev, struct crb_priv *priv)
 {
 	int rc;
 
-	if ((priv->sm == ACPI_TPM2_START_METHOD) ||
-	    (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) ||
-	    (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC))
+	if (!tpm_crb_has_idle(priv->sm))
 		return 0;
 
 	iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
@@ -222,9 +230,7 @@ static int __crb_cmd_ready(struct device *dev, struct crb_priv *priv)
 {
 	int rc;
 
-	if ((priv->sm == ACPI_TPM2_START_METHOD) ||
-	    (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) ||
-	    (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC))
+	if (!tpm_crb_has_idle(priv->sm))
 		return 0;
 
 	iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->regs_t->ctrl_req);
@@ -423,13 +429,13 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	 * report only ACPI start but in practice seems to require both
 	 * CRB start, hence invoking CRB start method if hid == MSFT0101.
 	 */
-	if ((priv->sm == ACPI_TPM2_COMMAND_BUFFER) ||
-	    (priv->sm == ACPI_TPM2_MEMORY_MAPPED) ||
-	    (!strcmp(priv->hid, "MSFT0101")))
+	if (priv->sm == ACPI_TPM2_COMMAND_BUFFER ||
+	    priv->sm == ACPI_TPM2_MEMORY_MAPPED ||
+	    !strcmp(priv->hid, "MSFT0101"))
 		iowrite32(CRB_START_INVOKE, &priv->regs_t->ctrl_start);
 
-	if ((priv->sm == ACPI_TPM2_START_METHOD) ||
-	    (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD))
+	if (priv->sm == ACPI_TPM2_START_METHOD ||
+	    priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
 		rc = crb_do_acpi_start(chip);
 
 	if (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC) {
@@ -449,8 +455,8 @@ static void crb_cancel(struct tpm_chip *chip)
 
 	iowrite32(CRB_CANCEL_INVOKE, &priv->regs_t->ctrl_cancel);
 
-	if (((priv->sm == ACPI_TPM2_START_METHOD) ||
-	    (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)) &&
+	if ((priv->sm == ACPI_TPM2_START_METHOD ||
+	     priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) &&
 	     crb_do_acpi_start(chip))
 		dev_err(&chip->dev, "ACPI Start failed\n");
 }
@@ -609,8 +615,8 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	 * the control area, as one nice sane region except for some older
 	 * stuff that puts the control area outside the ACPI IO region.
 	 */
-	if ((priv->sm == ACPI_TPM2_COMMAND_BUFFER) ||
-	    (priv->sm == ACPI_TPM2_MEMORY_MAPPED)) {
+	if (priv->sm == ACPI_TPM2_COMMAND_BUFFER ||
+	    priv->sm == ACPI_TPM2_MEMORY_MAPPED) {
 		if (iores &&
 		    buf->control_address == iores->start +
 		    sizeof(*priv->regs_h))
-- 
2.34.1

[PATCH v4 3/5] ACPICA: add start method for Arm FF-A

[Stuart Yoder]

Add TPM start method for Arm FF-A defined in the TCG ACPI
specification v1.4.

Link: https://github.com/acpica/acpica/pull/1000
Signed-off-by: Stuart Yoder <stuart.yoder@arm.com>
---
 include/acpi/actbl3.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/acpi/actbl3.h b/include/acpi/actbl3.h
index 5cd755143b7d..a97b1dbab975 100644
--- a/include/acpi/actbl3.h
+++ b/include/acpi/actbl3.h
@@ -466,6 +466,7 @@ struct acpi_tpm2_phy {
 #define ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC       11	/* V1.2 Rev 8 */
 #define ACPI_TPM2_RESERVED                          12
 #define ACPI_TPM2_COMMAND_BUFFER_WITH_PLUTON        13
+#define ACPI_TPM2_CRB_WITH_ARM_FFA                  15
 
 /* Optional trailer appears after any start_method subtables */
 
-- 
2.34.1

[PATCH v4 4/5] tpm_crb: add support for the Arm FF-A start method

[Stuart Yoder]

The TCG ACPI spec v1.4 defines a start method for the
TPMs implemented with the Arm CRB over FF-A ABI.

Add support for the FF-A start method, and use interfaces
provided by the ffa_crb driver to interact with the
FF-A based TPM.

Signed-off-by: Stuart Yoder <stuart.yoder@arm.com>
---
 drivers/char/tpm/tpm_crb.c | 71 +++++++++++++++++++++++++++++++++++---
 1 file changed, 66 insertions(+), 5 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 31db879f1324..2a57650ba9b4 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -19,6 +19,7 @@
 #ifdef CONFIG_ARM64
 #include <linux/arm-smccc.h>
 #endif
+#include "tpm_crb_ffa.h"
 #include "tpm.h"
 
 #define ACPI_SIG_TPM2 "TPM2"
@@ -100,6 +101,8 @@ struct crb_priv {
 	u32 smc_func_id;
 	u32 __iomem *pluton_start_addr;
 	u32 __iomem *pluton_reply_addr;
+	u8 ffa_flags;
+	u8 ffa_attributes;
 };
 
 struct tpm2_crb_smc {
@@ -110,6 +113,14 @@ struct tpm2_crb_smc {
 	u32 smc_func_id;
 };
 
+/* CRB over FFA start method parameters in TCG2 ACPI table */
+struct tpm2_crb_ffa {
+	u8 flags;
+	u8 attributes;
+	u16 partition_id;
+	u8 reserved[8];
+};
+
 struct tpm2_crb_pluton {
 	u64 start_addr;
 	u64 reply_addr;
@@ -122,7 +133,8 @@ static inline bool tpm_crb_has_idle(u32 start_method)
 {
 	return start_method == ACPI_TPM2_START_METHOD ||
 	       start_method == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD ||
-	       start_method == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC;
+	       start_method == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC ||
+	       start_method == ACPI_TPM2_CRB_WITH_ARM_FFA;
 }
 
 static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
@@ -261,13 +273,20 @@ static int crb_cmd_ready(struct tpm_chip *chip)
 static int __crb_request_locality(struct device *dev,
 				  struct crb_priv *priv, int loc)
 {
-	u32 value = CRB_LOC_STATE_LOC_ASSIGNED |
-		    CRB_LOC_STATE_TPM_REG_VALID_STS;
+	u32 value = CRB_LOC_STATE_LOC_ASSIGNED | CRB_LOC_STATE_TPM_REG_VALID_STS;
+	int rc;
 
 	if (!priv->regs_h)
 		return 0;
 
 	iowrite32(CRB_LOC_CTRL_REQUEST_ACCESS, &priv->regs_h->loc_ctrl);
+
+	if (priv->sm == ACPI_TPM2_CRB_WITH_ARM_FFA) {
+		rc = tpm_crb_ffa_start(CRB_FFA_START_TYPE_LOCALITY_REQUEST, loc);
+		if (rc)
+			return rc;
+	}
+
 	if (!crb_wait_for_reg_32(&priv->regs_h->loc_state, value, value,
 				 TPM2_TIMEOUT_C)) {
 		dev_warn(dev, "TPM_LOC_STATE_x.requestAccess timed out\n");
@@ -287,14 +306,21 @@ static int crb_request_locality(struct tpm_chip *chip, int loc)
 static int __crb_relinquish_locality(struct device *dev,
 				     struct crb_priv *priv, int loc)
 {
-	u32 mask = CRB_LOC_STATE_LOC_ASSIGNED |
-		   CRB_LOC_STATE_TPM_REG_VALID_STS;
+	u32 mask = CRB_LOC_STATE_LOC_ASSIGNED | CRB_LOC_STATE_TPM_REG_VALID_STS;
 	u32 value = CRB_LOC_STATE_TPM_REG_VALID_STS;
+	int rc;
 
 	if (!priv->regs_h)
 		return 0;
 
 	iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs_h->loc_ctrl);
+
+	if (priv->sm == ACPI_TPM2_CRB_WITH_ARM_FFA) {
+		rc = tpm_crb_ffa_start(CRB_FFA_START_TYPE_LOCALITY_REQUEST, loc);
+		if (rc)
+			return rc;
+	}
+
 	if (!crb_wait_for_reg_32(&priv->regs_h->loc_state, mask, value,
 				 TPM2_TIMEOUT_C)) {
 		dev_warn(dev, "TPM_LOC_STATE_x.Relinquish timed out\n");
@@ -443,6 +469,11 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, size_t len)
 		rc = tpm_crb_smc_start(&chip->dev, priv->smc_func_id);
 	}
 
+	if (priv->sm == ACPI_TPM2_CRB_WITH_ARM_FFA) {
+		iowrite32(CRB_START_INVOKE, &priv->regs_t->ctrl_start);
+		rc = tpm_crb_ffa_start(CRB_FFA_START_TYPE_COMMAND, chip->locality);
+	}
+
 	if (rc)
 		return rc;
 
@@ -452,6 +483,7 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, size_t len)
 static void crb_cancel(struct tpm_chip *chip)
 {
 	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
+	int rc;
 
 	iowrite32(CRB_CANCEL_INVOKE, &priv->regs_t->ctrl_cancel);
 
@@ -459,6 +491,12 @@ static void crb_cancel(struct tpm_chip *chip)
 	     priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) &&
 	     crb_do_acpi_start(chip))
 		dev_err(&chip->dev, "ACPI Start failed\n");
+
+	if (priv->sm == ACPI_TPM2_CRB_WITH_ARM_FFA) {
+		rc = tpm_crb_ffa_start(CRB_FFA_START_TYPE_COMMAND, chip->locality);
+		if (rc)
+			dev_err(&chip->dev, "FF-A Start failed\n");
+	}
 }
 
 static bool crb_req_canceled(struct tpm_chip *chip, u8 status)
@@ -616,6 +654,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	 * stuff that puts the control area outside the ACPI IO region.
 	 */
 	if (priv->sm == ACPI_TPM2_COMMAND_BUFFER ||
+	    priv->sm == ACPI_TPM2_CRB_WITH_ARM_FFA ||
 	    priv->sm == ACPI_TPM2_MEMORY_MAPPED) {
 		if (iores &&
 		    buf->control_address == iores->start +
@@ -737,6 +776,7 @@ static int crb_acpi_add(struct acpi_device *device)
 	struct tpm_chip *chip;
 	struct device *dev = &device->dev;
 	struct tpm2_crb_smc *crb_smc;
+	struct tpm2_crb_ffa *crb_ffa;
 	struct tpm2_crb_pluton *crb_pluton;
 	acpi_status status;
 	u32 sm;
@@ -775,6 +815,27 @@ static int crb_acpi_add(struct acpi_device *device)
 		priv->smc_func_id = crb_smc->smc_func_id;
 	}
 
+	if (sm == ACPI_TPM2_CRB_WITH_ARM_FFA) {
+		if (buf->header.length < (sizeof(*buf) + sizeof(*crb_ffa))) {
+			dev_err(dev,
+				FW_BUG "TPM2 ACPI table has wrong size %u for start method type %d\n",
+				buf->header.length,
+				ACPI_TPM2_CRB_WITH_ARM_FFA);
+			rc = -EINVAL;
+			goto out;
+		}
+		crb_ffa = ACPI_ADD_PTR(struct tpm2_crb_ffa, buf, sizeof(*buf));
+		priv->ffa_flags = crb_ffa->flags;
+		priv->ffa_attributes = crb_ffa->attributes;
+		rc = tpm_crb_ffa_init();
+		if (rc) {
+			if (rc == -ENOENT) {  // FF-A driver is not available yet
+				rc = -EPROBE_DEFER;
+			}
+			goto out;
+		}
+	}
+
 	if (sm == ACPI_TPM2_COMMAND_BUFFER_WITH_PLUTON) {
 		if (buf->header.length < (sizeof(*buf) + sizeof(*crb_pluton))) {
 			dev_err(dev,
-- 
2.34.1

[PATCH v4 5/5] Documentation: tpm: add documentation for the CRB FF-A interface

[Stuart Yoder]

Add documentation providing details of how the CRB driver interacts
with FF-A.

Signed-off-by: Stuart Yoder <stuart.yoder@arm.com>
---
 Documentation/security/tpm/tpm_ffa_crb.rst | 65 ++++++++++++++++++++++
 1 file changed, 65 insertions(+)
 create mode 100644 Documentation/security/tpm/tpm_ffa_crb.rst

diff --git a/Documentation/security/tpm/tpm_ffa_crb.rst b/Documentation/security/tpm/tpm_ffa_crb.rst
new file mode 100644
index 000000000000..0184193da3c7
--- /dev/null
+++ b/Documentation/security/tpm/tpm_ffa_crb.rst
@@ -0,0 +1,65 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+========================
+TPM CRB over FF-A Driver
+========================
+
+The TPM Command Response Buffer (CRB) interface is a standard TPM interface
+defined in the TCG PC Client Platform TPM Profile (PTP) Specification [1]_.
+The CRB provides a structured set of control registers a client uses when
+interacting with a TPM as well as a data buffer for storing TPM commands and
+responses. A CRB interface can be implemented in:
+
+- hardware registers in a discrete TPM chip
+
+- in memory for a TPM running in isolated environment where shared memory
+  allows a client to interact with the TPM
+
+The Firmware Framework for Arm A-profile (FF-A) [2]_ is a specification
+that defines interfaces and protocols for the following purposes:
+
+- Compartmentalize firmware into software partitions that run in the Arm
+  Secure world environment (also know as TrustZone)
+
+- Provide a standard interface for software components in the Non-secure
+  state, for example OS and Hypervisors, to communicate with this firmware.
+
+A TPM can be implemented as an FF-A secure service.  This could be a firmware
+TPM or could potentially be a TPM service that acts as a proxy to a discrete
+TPM chip. An FF-A based TPM abstracts hardware details (e.g. bus controller
+and chip selects) away from the OS and can protect locality 4 from access
+by an OS.  The TCG-defined CRB interface is used by clients to interact
+with the TPM service.
+
+The Arm TPM Service Command Response Buffer Interface Over FF-A [3]_
+specification defines FF-A messages that can be used by a client to signal
+when updates have been made to the CRB.
+
+How the Linux CRB driver interacts with FF-A is summarized below:
+
+- The tpm_crb_ffa driver registers with the FF-A subsystem in the kernel
+  with an architected TPM service UUID defined in the CRB over FF-A spec.
+
+- If a TPM service is discovered by FF-A, the probe() function in the
+  tpm_crb_ffa driver runs, and the driver initializes.
+
+- The probing and initialization of the Linux CRB driver is triggered
+  by the discovery of a TPM advertised via ACPI.  The CRB driver can
+  detect the type of TPM through the ACPI 'start' method.  The start
+  method for Arm FF-A was defined in TCG ACPI v1.4 [4]_.
+
+- When the CRB driver performs its normal functions such as signaling 'start'
+  and locality request/relinquish it invokes the tpm_crb_ffa_start() funnction
+  in the tpm_crb_ffa driver which handles the FF-A messaging to the TPM.
+
+References
+==========
+
+.. [1] **TCG PC Client Platform TPM Profile (PTP) Specification**
+   https://trustedcomputinggroup.org/resource/pc-client-platform-tpm-profile-ptp-specification/
+.. [2] **Arm Firmware Framework for Arm A-profile (FF-A)**
+   https://developer.arm.com/documentation/den0077/latest/
+.. [3] **Arm TPM Service Command Response Buffer Interface Over FF-A**
+   https://developer.arm.com/documentation/den0138/latest/
+.. [4] **TCG ACPI Specification**
+   https://trustedcomputinggroup.org/resource/tcg-acpi-specification/
-- 
2.34.1

Re: [PATCH v4 1/5] tpm_crb: implement driver compliant to CRB over FF-A

[Jarkko Sakkinen]

 On Mon, 2025-02-17 at 16:49 -0600, Stuart Yoder wrote:
> The Arm specification TPM Service CRB over FF-A specification
> defines the FF-A messages to interact with a CRB-based TPM
> implemented as an FF-A secure partition.
> 
> Spec URL:
> https://developer.arm.com/documentation/den0138/latest/
> 
> This driver is probed when a TPM Secure Partition is
> discovered by the FF-A subsystem. It exposes APIs
> used by the TPM CRB driver to send notifications to
> the TPM.
> 
> Signed-off-by: Stuart Yoder <stuart.yoder@arm.com>
> ---
>  drivers/char/tpm/Kconfig       |   9 +
>  drivers/char/tpm/Makefile      |   1 +
>  drivers/char/tpm/tpm_crb_ffa.c | 348
> +++++++++++++++++++++++++++++++++
>  drivers/char/tpm/tpm_crb_ffa.h |  25 +++
>  4 files changed, 383 insertions(+)
>  create mode 100644 drivers/char/tpm/tpm_crb_ffa.c
>  create mode 100644 drivers/char/tpm/tpm_crb_ffa.h
> 
> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index 0fc9a510e059..4c85b8c00b12 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -210,6 +210,15 @@ config TCG_CRB
>  	  from within Linux.  To compile this driver as a module,
> choose
>  	  M here; the module will be called tpm_crb.
>  
> +config TCG_ARM_CRB_FFA
> +	tristate "TPM CRB over Arm FF-A Transport"
> +	depends on ARM_FFA_TRANSPORT
> +	default y if (TCG_CRB && ARM_FFA_TRANSPORT)
> +	help
> +	  If the Arm FF-A transport is used to access the TPM say
> Yes.
> +	  To compile this driver as a module, choose M here; the
> module
> +	  will be called tpm_crb_ffa.
> +
>  config TCG_VTPM_PROXY
>  	tristate "VTPM Proxy Interface"
>  	depends on TCG_TPM
> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> index 9bb142c75243..2b004df8c04b 100644
> --- a/drivers/char/tpm/Makefile
> +++ b/drivers/char/tpm/Makefile
> @@ -42,5 +42,6 @@ obj-$(CONFIG_TCG_IBMVTPM) += tpm_ibmvtpm.o
>  obj-$(CONFIG_TCG_TIS_ST33ZP24) += st33zp24/
>  obj-$(CONFIG_TCG_XEN) += xen-tpmfront.o
>  obj-$(CONFIG_TCG_CRB) += tpm_crb.o
> +obj-$(CONFIG_TCG_ARM_CRB_FFA) += tpm_crb_ffa.o
>  obj-$(CONFIG_TCG_VTPM_PROXY) += tpm_vtpm_proxy.o
>  obj-$(CONFIG_TCG_FTPM_TEE) += tpm_ftpm_tee.o
> diff --git a/drivers/char/tpm/tpm_crb_ffa.c
> b/drivers/char/tpm/tpm_crb_ffa.c
> new file mode 100644
> index 000000000000..bd3c6c41264b
> --- /dev/null
> +++ b/drivers/char/tpm/tpm_crb_ffa.c
> @@ -0,0 +1,348 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2024 Arm Ltd.

Agreed that, this type of copyright makes sense. It would be harder to
decipher the data from the commit log.

Also one corner case where I plan to use copyrighta in-file personally:
I'm mainlining an OOT driver with a bunch of contributors over time,
and in that I'll list all the historical contributors with appropriate
copyright platters :-)

Generally speaking, I think can be added as long as it has an actual
function that brings measurable value.

> + *
> + * This device driver implements the TPM CRB start method
> + * as defined in the TPM Service Command Response Buffer
> + * Interface Over FF-A (DEN0138).
> + */
> +
> +#define pr_fmt(fmt) "CRB_FFA: " fmt
> +
> +#include <linux/arm_ffa.h>
> +#include "tpm_crb_ffa.h"
> +
> +/* TPM service function status codes */
> +#define CRB_FFA_OK			0x05000001
> +#define CRB_FFA_OK_RESULTS_RETURNED	0x05000002
> +#define CRB_FFA_NOFUNC			0x8e000001
> +#define CRB_FFA_NOTSUP			0x8e000002
> +#define CRB_FFA_INVARG			0x8e000005
> +#define CRB_FFA_INV_CRB_CTRL_DATA	0x8e000006
> +#define CRB_FFA_ALREADY			0x8e000009
> +#define CRB_FFA_DENIED			0x8e00000a
> +#define CRB_FFA_NOMEM			0x8e00000b
> +
> +#define CRB_FFA_VERSION_MAJOR	1
> +#define CRB_FFA_VERSION_MINOR	0
> +
> +/* version encoding */
> +#define CRB_FFA_MAJOR_VERSION_MASK  GENMASK(30, 16)
> +#define CRB_FFA_MINOR_VERSION_MASK  GENMASK(15, 0)
> +#define CRB_FFA_MAJOR_VERSION(x)   
> ((u16)(FIELD_GET(CRB_FFA_MAJOR_VERSION_MASK, (x))))
> +#define CRB_FFA_MINOR_VERSION(x)   
> ((u16)(FIELD_GET(CRB_FFA_MINOR_VERSION_MASK, (x))))
> +
> +/*
> + * Normal world sends requests with FFA_MSG_SEND_DIRECT_REQ and
> + * responses are returned with FFA_MSG_SEND_DIRECT_RESP for normal
> + * messages.
> + *
> + * All requests with FFA_MSG_SEND_DIRECT_REQ and
> FFA_MSG_SEND_DIRECT_RESP
> + * are using the AArch32 SMC calling convention with register usage
> as
> + * defined in FF-A specification:
> + * w0:    Function ID (0x8400006F or 0x84000070)
> + * w1:    Source/Destination IDs
> + * w2:    Reserved (MBZ)
> + * w3-w7: Implementation defined, free to be used below
> + */
> +
> +/*
> + * Returns the version of the interface that is available
> + * Call register usage:
> + * w3:    Not used (MBZ)
> + * w4:    TPM service function ID, CRB_FFA_GET_INTERFACE_VERSION
> + * w5-w7: Reserved (MBZ)
> + *
> + * Return register usage:
> + * w3:    Not used (MBZ)
> + * w4:    TPM service function status
> + * w5:    TPM service interface version
> + *        Bits[31:16]: major version
> + *        Bits[15:0]: minor version
> + * w6-w7: Reserved (MBZ)
> + *
> + * Possible function status codes in register w4:
> + *     CRB_FFA_OK_RESULTS_RETURNED: The version of the interface has
> been
> + *                                  returned.
> + */
> +#define CRB_FFA_GET_INTERFACE_VERSION 0x0f000001
> +
> +/*
> + * Return information on a given feature of the TPM service
> + * Call register usage:
> + * w3:    Not used (MBZ)
> + * w4:    TPM service function ID, CRB_FFA_START
> + * w5:    Start function qualifier
> + *            Bits[31:8] (MBZ)
> + *            Bits[7:0]
> + *              0: Notifies TPM that a command is ready to be
> processed
> + *              1: Notifies TPM that a locality request is ready to
> be processed
> + * w6:    TPM locality, one of 0..4
> + *            -If the start function qualifier is 0, identifies the
> locality
> + *             from where the command originated.
> + *            -If the start function qualifier is 1, identifies the
> locality
> + *             of the locality request
> + * w6-w7: Reserved (MBZ)
> + *
> + * Return register usage:
> + * w3:    Not used (MBZ)
> + * w4:    TPM service function status
> + * w5-w7: Reserved (MBZ)
> + *
> + * Possible function status codes in register w4:
> + *     CRB_FFA_OK: the TPM service has been notified successfully
> + *     CRB_FFA_INVARG: one or more arguments are not valid
> + *     CRB_FFA_INV_CRB_CTRL_DATA: CRB control data or locality
> control
> + *         data at the given TPM locality is not valid
> + *     CRB_FFA_DENIED: the TPM has previously disabled locality
> requests and
> + *         command processing at the given locality
> + */
> +#define CRB_FFA_START 0x0f000201
> +
> +struct tpm_crb_ffa {
> +	struct ffa_device *ffa_dev;
> +	u16 major_version;
> +	u16 minor_version;
> +	struct mutex msg_data_lock; /* lock to protect sending of
> FF-A messages */

Nit:

	/* Lock to protect sending of FFA-messages: */
	struct mutex msg_data_lock; 

It is a bit more readable and double comma makes pretty obvious
which field it is pointing at.

> +	struct ffa_send_direct_data direct_msg_data;
> +};
> +
> +static struct tpm_crb_ffa *tpm_crb_ffa;
> +
> +static int tpm_crb_ffa_to_linux_errno(int errno)
> +{
> +	int rc;
> +
> +	switch (errno) {
> +	case CRB_FFA_OK:
> +		rc = 0;
> +		break;
> +	case CRB_FFA_OK_RESULTS_RETURNED:
> +		rc = 0;
> +		break;
> +	case CRB_FFA_NOFUNC:
> +		rc = -ENOENT;
> +		break;
> +	case CRB_FFA_NOTSUP:
> +		rc = -EPERM;
> +		break;
> +	case CRB_FFA_INVARG:
> +		rc = -EINVAL;
> +		break;
> +	case CRB_FFA_INV_CRB_CTRL_DATA:
> +		rc = -ENOEXEC;
> +		break;
> +	case CRB_FFA_ALREADY:
> +		rc = -EEXIST;
> +		break;
> +	case CRB_FFA_DENIED:
> +		rc = -EACCES;
> +		break;
> +	case CRB_FFA_NOMEM:
> +		rc = -ENOMEM;
> +		break;
> +	default:
> +		rc = -EINVAL;
> +	}
> +
> +	return rc;
> +}
> +
> +/**
> + * tpm_crb_ffa_init - called by the CRB driver to do any needed
> initialization
> + *
> + * This function is called by the tpm_crb driver during the tpm_crb
> + * driver's initialization. If the tpm_crb_ffa has not been probed
> + * yet, returns -ENOENT in order to force a retry.  If th ffa_crb
> + * driver had been probed  but failed with an error, returns -ENODEV
> + * in order to prevent further retries.
> + *
> + * Return: 0 on success, negative error code on failure.
> + */
> +int tpm_crb_ffa_init(void)
> +{
> +	if (!tpm_crb_ffa)
> +		return -ENOENT;
> +
> +	if (IS_ERR_VALUE(tpm_crb_ffa))
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(tpm_crb_ffa_init);
> +
> +static int __tpm_crb_ffa_send_recieve(unsigned long func_id,
> +				      unsigned long a0,
> +				      unsigned long a1,
> +				      unsigned long a2)
> +{
> +	const struct ffa_msg_ops *msg_ops;
> +	int ret;
> +
> +	if (!tpm_crb_ffa)
> +		return -ENOENT;
> +
> +	msg_ops = tpm_crb_ffa->ffa_dev->ops->msg_ops;
> +
> +	memset(&tpm_crb_ffa->direct_msg_data, 0x00,
> +	       sizeof(struct ffa_send_direct_data));
> +
> +	tpm_crb_ffa->direct_msg_data.data1 = func_id;
> +	tpm_crb_ffa->direct_msg_data.data2 = a0;
> +	tpm_crb_ffa->direct_msg_data.data3 = a1;
> +	tpm_crb_ffa->direct_msg_data.data4 = a2;
> +
> +	ret = msg_ops->sync_send_receive(tpm_crb_ffa->ffa_dev,
> +			&tpm_crb_ffa->direct_msg_data);
> +	if (!ret)
> +		ret = tpm_crb_ffa_to_linux_errno(tpm_crb_ffa-
> >direct_msg_data.data1);
> +
> +	return ret;
> +}
> +
> +/**
> + * tpm_crb_ffa_get_interface_version() - gets the ABI version of the
> TPM service
> + * @major: Pointer to caller-allocated buffer to hold the major
> version
> + *         number the ABI
> + * @minor: Pointer to caller-allocated buffer to hold the minor
> version
> + *         number the ABI
> + *
> + * Returns the major and minor version of the ABI of the FF-A based
> TPM.
> + * Allows the caller to evaluate its compatibility with the version
> of
> + * the ABI.
> + *
> + * Return: 0 on success, negative error code on failure.
> + */
> +int tpm_crb_ffa_get_interface_version(u16 *major, u16 *minor)
> +{
> +	int rc;
> +
> +	if (!tpm_crb_ffa)
> +		return -ENOENT;
> +
> +	if (IS_ERR_VALUE(tpm_crb_ffa))
> +		return -ENODEV;
> +
> +	if (!major || !minor)
> +		return -EINVAL;
> +
> +	guard(mutex)(&tpm_crb_ffa->msg_data_lock);
> +
> +	rc =
> __tpm_crb_ffa_send_recieve(CRB_FFA_GET_INTERFACE_VERSION, 0x00, 0x00,
> 0x00);
> +	if (!rc) {
> +		*major = CRB_FFA_MAJOR_VERSION(tpm_crb_ffa-
> >direct_msg_data.data2);
> +		*minor = CRB_FFA_MINOR_VERSION(tpm_crb_ffa-
> >direct_msg_data.data2);
> +	}
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(tpm_crb_ffa_get_interface_version);
> +
> +/**
> + * tpm_crb_ffa_start() - signals the TPM that a field has changed in
> the CRB
> + * @request_type: Identifies whether the change to the CRB is in the
> command
> + *                fields or locality fields.
> + * @locality: Specifies the locality number.
> + *
> + * Used by the CRB driver
> + * that might be useful to those using or modifying it.  Begins with
> + * empty comment line, and may include additional embedded empty
> + * comment lines.
> + *
> + * Return: 0 on success, negative error code on failure.
> + */
> +int tpm_crb_ffa_start(int request_type, int locality)
> +{
> +	if (!tpm_crb_ffa)
> +		return -ENOENT;
> +
> +	if (IS_ERR_VALUE(tpm_crb_ffa))
> +		return -ENODEV;
> +
> +	guard(mutex)(&tpm_crb_ffa->msg_data_lock);
> +
> +	return __tpm_crb_ffa_send_recieve(CRB_FFA_START,
> request_type, locality, 0x00);
> +}
> +EXPORT_SYMBOL_GPL(tpm_crb_ffa_start);
> +
> +static int tpm_crb_ffa_probe(struct ffa_device *ffa_dev)
> +{
> +	struct tpm_crb_ffa *p;
> +	int rc;
> +
> +	/* only one instance of a TPM partition is supported */
> +	if (tpm_crb_ffa && !IS_ERR_VALUE(tpm_crb_ffa))
> +		return -EEXIST;
> +
> +	tpm_crb_ffa = ERR_PTR(-ENODEV); // set tpm_crb_ffa so we can
> detect probe failure
> +
> +	if (!ffa_partition_supports_direct_recv(ffa_dev)) {
> +		pr_err("TPM partition doesn't support direct message
> receive.\n");
> +		return -EINVAL;
> +	}
> +
> +	p = kzalloc(sizeof(*tpm_crb_ffa), GFP_KERNEL);
> +	if (!p)
> +		return -ENOMEM;
> +	tpm_crb_ffa = p;
> +
> +	mutex_init(&tpm_crb_ffa->msg_data_lock);
> +	tpm_crb_ffa->ffa_dev = ffa_dev;
> +	ffa_dev_set_drvdata(ffa_dev, tpm_crb_ffa);
> +
> +	/* if TPM is aarch32 use 32-bit SMCs */
> +	if (!ffa_partition_check_property(ffa_dev,
> FFA_PARTITION_AARCH64_EXEC))
> +		ffa_dev->ops->msg_ops->mode_32bit_set(ffa_dev);
> +
> +	/* verify compatibility of TPM service version number */
> +	rc = tpm_crb_ffa_get_interface_version(&tpm_crb_ffa-
> >major_version,
> +					       &tpm_crb_ffa-
> >minor_version);
> +	if (rc) {
> +		pr_err("failed to get crb interface version. rc:%d",
> rc);
> +		goto out;
> +	}
> +
> +	pr_info("ABI version %u.%u", tpm_crb_ffa->major_version,
> +		tpm_crb_ffa->minor_version);
> +
> +	if (tpm_crb_ffa->major_version != CRB_FFA_VERSION_MAJOR ||
> +	   (tpm_crb_ffa->minor_version > 0 &&
> +	    tpm_crb_ffa->minor_version < CRB_FFA_VERSION_MINOR)) {
> +		pr_err("Incompatible ABI version");
> +		goto out;
> +	}
> +
> +	return 0;
> +
> +out:
> +	kfree(tpm_crb_ffa);
> +	tpm_crb_ffa = ERR_PTR(-ENODEV);
> +	return -EINVAL;
> +}
> +
> +static void tpm_crb_ffa_remove(struct ffa_device *ffa_dev)
> +{
> +	kfree(tpm_crb_ffa);
> +	tpm_crb_ffa = NULL;
> +}
> +
> +static const struct ffa_device_id tpm_crb_ffa_device_id[] = {
> +	/* 17b862a4-1806-4faf-86b3-089a58353861 */
> +	{ UUID_INIT(0x17b862a4, 0x1806, 0x4faf,
> +		    0x86, 0xb3, 0x08, 0x9a, 0x58, 0x35, 0x38, 0x61)
> },
> +	{}
> +};
> +
> +static struct ffa_driver tpm_crb_ffa_driver = {
> +	.name = "ffa-crb",
> +	.probe = tpm_crb_ffa_probe,
> +	.remove = tpm_crb_ffa_remove,
> +	.id_table = tpm_crb_ffa_device_id,
> +};
> +
> +module_ffa_driver(tpm_crb_ffa_driver);
> +
> +MODULE_AUTHOR("Arm");
> +MODULE_DESCRIPTION("FFA CRB driver");
> +MODULE_VERSION("1.0");

Take this with a grain of salt, i.e. I'm not going to NAK for this but
I would not personally add MODULE_VERSION() for new drivers as it does
not have any function.

> +MODULE_LICENSE("GPL");
> diff --git a/drivers/char/tpm/tpm_crb_ffa.h
> b/drivers/char/tpm/tpm_crb_ffa.h
> new file mode 100644
> index 000000000000..23a9b93287a1
> --- /dev/null
> +++ b/drivers/char/tpm/tpm_crb_ffa.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2024 Arm Ltd.
> + *
> + * This device driver implements the TPM CRB start method
> + * as defined in the TPM Service Command Response Buffer
> + * Interface Over FF-A (DEN0138).
> + */
> +#ifndef _TPM_CRB_FFA_H
> +#define _TPM_CRB_FFA_H
> +
> +#if IS_ENABLED(CONFIG_TCG_ARM_CRB_FFA)
> +int tpm_crb_ffa_init(void);
> +int tpm_crb_ffa_get_interface_version(u16 *major, u16 *minor);
> +int tpm_crb_ffa_start(int request_type, int locality);
> +#else
> +static inline int tpm_crb_ffa_init(void) { return 0; }
> +static inline int tpm_crb_ffa_get_interface_version(u16 *major, u16
> *minor) { return 0; }
> +static inline int tpm_crb_ffa_start(int request_type, int locality)
> { return 0; }
> +#endif
> +
> +#define CRB_FFA_START_TYPE_COMMAND 0
> +#define CRB_FFA_START_TYPE_LOCALITY_REQUEST 1
> +
> +#endif

BR, Jarkko

Re: [PATCH v4 2/5] tpm_crb: clean-up and refactor check for idle support

[Jarkko Sakkinen]

On Mon, 2025-02-17 at 16:49 -0600, Stuart Yoder wrote:
> -Clean up unnecessary parenthesis usage
> -Refactor the two checks for whether the TPM supports idle into a
> single
>  inline function.

IMHO, this would be better:

"Refactor TPM idle check to tpm_crb_has_idle(), and reduce paraentheses
usage in start method checks."

> 
> Signed-off-by: Stuart Yoder <stuart.yoder@arm.com>
> ---
>  drivers/char/tpm/tpm_crb.c | 36 +++++++++++++++++++++---------------
>  1 file changed, 21 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index ea085b14ab7c..31db879f1324 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -115,6 +115,16 @@ struct tpm2_crb_pluton {
>  	u64 reply_addr;
>  };
>  
> +/*
> + * Returns true if the start method supports idle.
> + */
> +static inline bool tpm_crb_has_idle(u32 start_method)
> +{
> +	return start_method == ACPI_TPM2_START_METHOD ||
> +	       start_method ==
> ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD ||
> +	       start_method ==
> ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC;
> +}
> +
>  static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32
> value,
>  				unsigned long timeout)
>  {
> @@ -173,9 +183,7 @@ static int __crb_go_idle(struct device *dev,
> struct crb_priv *priv)
>  {
>  	int rc;
>  
> -	if ((priv->sm == ACPI_TPM2_START_METHOD) ||
> -	    (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
> ||
> -	    (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC))
> +	if (!tpm_crb_has_idle(priv->sm))
>  		return 0;
>  
>  	iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
> @@ -222,9 +230,7 @@ static int __crb_cmd_ready(struct device *dev,
> struct crb_priv *priv)
>  {
>  	int rc;
>  
> -	if ((priv->sm == ACPI_TPM2_START_METHOD) ||
> -	    (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
> ||
> -	    (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC))
> +	if (!tpm_crb_has_idle(priv->sm))
>  		return 0;
>  
>  	iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->regs_t->ctrl_req);
> @@ -423,13 +429,13 @@ static int crb_send(struct tpm_chip *chip, u8
> *buf, size_t len)
>  	 * report only ACPI start but in practice seems to require
> both
>  	 * CRB start, hence invoking CRB start method if hid ==
> MSFT0101.
>  	 */
> -	if ((priv->sm == ACPI_TPM2_COMMAND_BUFFER) ||
> -	    (priv->sm == ACPI_TPM2_MEMORY_MAPPED) ||
> -	    (!strcmp(priv->hid, "MSFT0101")))
> +	if (priv->sm == ACPI_TPM2_COMMAND_BUFFER ||
> +	    priv->sm == ACPI_TPM2_MEMORY_MAPPED ||
> +	    !strcmp(priv->hid, "MSFT0101"))
>  		iowrite32(CRB_START_INVOKE, &priv->regs_t-
> >ctrl_start);
>  
> -	if ((priv->sm == ACPI_TPM2_START_METHOD) ||
> -	    (priv->sm ==
> ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD))
> +	if (priv->sm == ACPI_TPM2_START_METHOD ||
> +	    priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
>  		rc = crb_do_acpi_start(chip);
>  
>  	if (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC) {
> @@ -449,8 +455,8 @@ static void crb_cancel(struct tpm_chip *chip)
>  
>  	iowrite32(CRB_CANCEL_INVOKE, &priv->regs_t->ctrl_cancel);
>  
> -	if (((priv->sm == ACPI_TPM2_START_METHOD) ||
> -	    (priv->sm ==
> ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)) &&
> +	if ((priv->sm == ACPI_TPM2_START_METHOD ||
> +	     priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
> &&
>  	     crb_do_acpi_start(chip))
>  		dev_err(&chip->dev, "ACPI Start failed\n");
>  }
> @@ -609,8 +615,8 @@ static int crb_map_io(struct acpi_device *device,
> struct crb_priv *priv,
>  	 * the control area, as one nice sane region except for some
> older
>  	 * stuff that puts the control area outside the ACPI IO
> region.
>  	 */
> -	if ((priv->sm == ACPI_TPM2_COMMAND_BUFFER) ||
> -	    (priv->sm == ACPI_TPM2_MEMORY_MAPPED)) {
> +	if (priv->sm == ACPI_TPM2_COMMAND_BUFFER ||
> +	    priv->sm == ACPI_TPM2_MEMORY_MAPPED) {
>  		if (iores &&
>  		    buf->control_address == iores->start +
>  		    sizeof(*priv->regs_h))


BR, Jarkko

Re: [PATCH v4 3/5] ACPICA: add start method for Arm FF-A

[Jarkko Sakkinen]

On Mon, 2025-02-17 at 16:49 -0600, Stuart Yoder wrote:
> Add TPM start method for Arm FF-A defined in the TCG ACPI
> specification v1.4.
> 
> Link: https://github.com/acpica/acpica/pull/1000
> Signed-off-by: Stuart Yoder <stuart.yoder@arm.com>
> ---
>  include/acpi/actbl3.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/acpi/actbl3.h b/include/acpi/actbl3.h
> index 5cd755143b7d..a97b1dbab975 100644
> --- a/include/acpi/actbl3.h
> +++ b/include/acpi/actbl3.h
> @@ -466,6 +466,7 @@ struct acpi_tpm2_phy {
>  #define ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC       11	/* V1.2 Rev
> 8 */
>  #define ACPI_TPM2_RESERVED                          12
>  #define ACPI_TPM2_COMMAND_BUFFER_WITH_PLUTON        13
> +#define ACPI_TPM2_CRB_WITH_ARM_FFA                  15
>  
>  /* Optional trailer appears after any start_method subtables */
>  

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko

Re: [PATCH v4 4/5] tpm_crb: add support for the Arm FF-A start method

[Jarkko Sakkinen]

On Mon, 2025-02-17 at 16:49 -0600, Stuart Yoder wrote:
> The TCG ACPI spec v1.4 defines a start method for the
> TPMs implemented with the Arm CRB over FF-A ABI.
> 
> Add support for the FF-A start method, and use interfaces
> provided by the ffa_crb driver to interact with the
> FF-A based TPM.
> 
> Signed-off-by: Stuart Yoder <stuart.yoder@arm.com>
> ---
>  drivers/char/tpm/tpm_crb.c | 71 +++++++++++++++++++++++++++++++++++-
> --
>  1 file changed, 66 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 31db879f1324..2a57650ba9b4 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -19,6 +19,7 @@
>  #ifdef CONFIG_ARM64
>  #include <linux/arm-smccc.h>
>  #endif
> +#include "tpm_crb_ffa.h"
>  #include "tpm.h"
>  
>  #define ACPI_SIG_TPM2 "TPM2"
> @@ -100,6 +101,8 @@ struct crb_priv {
>  	u32 smc_func_id;
>  	u32 __iomem *pluton_start_addr;
>  	u32 __iomem *pluton_reply_addr;
> +	u8 ffa_flags;
> +	u8 ffa_attributes;
>  };
>  
>  struct tpm2_crb_smc {
> @@ -110,6 +113,14 @@ struct tpm2_crb_smc {
>  	u32 smc_func_id;
>  };
>  
> +/* CRB over FFA start method parameters in TCG2 ACPI table */
> +struct tpm2_crb_ffa {
> +	u8 flags;
> +	u8 attributes;
> +	u16 partition_id;
> +	u8 reserved[8];
> +};
> +
>  struct tpm2_crb_pluton {
>  	u64 start_addr;
>  	u64 reply_addr;
> @@ -122,7 +133,8 @@ static inline bool tpm_crb_has_idle(u32
> start_method)
>  {
>  	return start_method == ACPI_TPM2_START_METHOD ||
>  	       start_method ==
> ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD ||
> -	       start_method ==
> ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC;
> +	       start_method == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC
> ||
> +	       start_method == ACPI_TPM2_CRB_WITH_ARM_FFA;
>  }
>  
>  static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32
> value,
> @@ -261,13 +273,20 @@ static int crb_cmd_ready(struct tpm_chip *chip)
>  static int __crb_request_locality(struct device *dev,
>  				  struct crb_priv *priv, int loc)
>  {
> -	u32 value = CRB_LOC_STATE_LOC_ASSIGNED |
> -		    CRB_LOC_STATE_TPM_REG_VALID_STS;
> +	u32 value = CRB_LOC_STATE_LOC_ASSIGNED |
> CRB_LOC_STATE_TPM_REG_VALID_STS;
> +	int rc;
>  
>  	if (!priv->regs_h)
>  		return 0;
>  
>  	iowrite32(CRB_LOC_CTRL_REQUEST_ACCESS, &priv->regs_h-
> >loc_ctrl);
> +
> +	if (priv->sm == ACPI_TPM2_CRB_WITH_ARM_FFA) {
> +		rc =
> tpm_crb_ffa_start(CRB_FFA_START_TYPE_LOCALITY_REQUEST, loc);
> +		if (rc)
> +			return rc;
> +	}
> +
>  	if (!crb_wait_for_reg_32(&priv->regs_h->loc_state, value,
> value,
>  				 TPM2_TIMEOUT_C)) {
>  		dev_warn(dev, "TPM_LOC_STATE_x.requestAccess timed
> out\n");
> @@ -287,14 +306,21 @@ static int crb_request_locality(struct tpm_chip
> *chip, int loc)
>  static int __crb_relinquish_locality(struct device *dev,
>  				     struct crb_priv *priv, int loc)
>  {
> -	u32 mask = CRB_LOC_STATE_LOC_ASSIGNED |
> -		   CRB_LOC_STATE_TPM_REG_VALID_STS;
> +	u32 mask = CRB_LOC_STATE_LOC_ASSIGNED |
> CRB_LOC_STATE_TPM_REG_VALID_STS;
>  	u32 value = CRB_LOC_STATE_TPM_REG_VALID_STS;
> +	int rc;
>  
>  	if (!priv->regs_h)
>  		return 0;
>  
>  	iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs_h->loc_ctrl);
> +
> +	if (priv->sm == ACPI_TPM2_CRB_WITH_ARM_FFA) {
> +		rc =
> tpm_crb_ffa_start(CRB_FFA_START_TYPE_LOCALITY_REQUEST, loc);
> +		if (rc)
> +			return rc;
> +	}
> +
>  	if (!crb_wait_for_reg_32(&priv->regs_h->loc_state, mask,
> value,
>  				 TPM2_TIMEOUT_C)) {
>  		dev_warn(dev, "TPM_LOC_STATE_x.Relinquish timed
> out\n");
> @@ -443,6 +469,11 @@ static int crb_send(struct tpm_chip *chip, u8
> *buf, size_t len)
>  		rc = tpm_crb_smc_start(&chip->dev, priv-
> >smc_func_id);
>  	}
>  
> +	if (priv->sm == ACPI_TPM2_CRB_WITH_ARM_FFA) {
> +		iowrite32(CRB_START_INVOKE, &priv->regs_t-
> >ctrl_start);
> +		rc = tpm_crb_ffa_start(CRB_FFA_START_TYPE_COMMAND,
> chip->locality);
> +	}
> +
>  	if (rc)
>  		return rc;
>  
> @@ -452,6 +483,7 @@ static int crb_send(struct tpm_chip *chip, u8
> *buf, size_t len)
>  static void crb_cancel(struct tpm_chip *chip)
>  {
>  	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> +	int rc;
>  
>  	iowrite32(CRB_CANCEL_INVOKE, &priv->regs_t->ctrl_cancel);
>  
> @@ -459,6 +491,12 @@ static void crb_cancel(struct tpm_chip *chip)
>  	     priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
> &&
>  	     crb_do_acpi_start(chip))
>  		dev_err(&chip->dev, "ACPI Start failed\n");
> +
> +	if (priv->sm == ACPI_TPM2_CRB_WITH_ARM_FFA) {
> +		rc = tpm_crb_ffa_start(CRB_FFA_START_TYPE_COMMAND,
> chip->locality);
> +		if (rc)
> +			dev_err(&chip->dev, "FF-A Start failed\n");
> +	}
>  }
>  
>  static bool crb_req_canceled(struct tpm_chip *chip, u8 status)
> @@ -616,6 +654,7 @@ static int crb_map_io(struct acpi_device *device,
> struct crb_priv *priv,
>  	 * stuff that puts the control area outside the ACPI IO
> region.
>  	 */
>  	if (priv->sm == ACPI_TPM2_COMMAND_BUFFER ||
> +	    priv->sm == ACPI_TPM2_CRB_WITH_ARM_FFA ||
>  	    priv->sm == ACPI_TPM2_MEMORY_MAPPED) {
>  		if (iores &&
>  		    buf->control_address == iores->start +
> @@ -737,6 +776,7 @@ static int crb_acpi_add(struct acpi_device
> *device)
>  	struct tpm_chip *chip;
>  	struct device *dev = &device->dev;
>  	struct tpm2_crb_smc *crb_smc;
> +	struct tpm2_crb_ffa *crb_ffa;
>  	struct tpm2_crb_pluton *crb_pluton;
>  	acpi_status status;
>  	u32 sm;
> @@ -775,6 +815,27 @@ static int crb_acpi_add(struct acpi_device
> *device)
>  		priv->smc_func_id = crb_smc->smc_func_id;
>  	}
>  
> +	if (sm == ACPI_TPM2_CRB_WITH_ARM_FFA) {
> +		if (buf->header.length < (sizeof(*buf) +
> sizeof(*crb_ffa))) {
> +			dev_err(dev,
> +				FW_BUG "TPM2 ACPI table has wrong
> size %u for start method type %d\n",
> +				buf->header.length,
> +				ACPI_TPM2_CRB_WITH_ARM_FFA);
> +			rc = -EINVAL;
> +			goto out;
> +		}
> +		crb_ffa = ACPI_ADD_PTR(struct tpm2_crb_ffa, buf,
> sizeof(*buf));
> +		priv->ffa_flags = crb_ffa->flags;
> +		priv->ffa_attributes = crb_ffa->attributes;
> +		rc = tpm_crb_ffa_init();
> +		if (rc) {
> +			if (rc == -ENOENT) {  // FF-A driver is not
> available yet
> +				rc = -EPROBE_DEFER;
> +			}
> +			goto out;
> +		}
> +	}
> +
>  	if (sm == ACPI_TPM2_COMMAND_BUFFER_WITH_PLUTON) {
>  		if (buf->header.length < (sizeof(*buf) +
> sizeof(*crb_pluton))) {
>  			dev_err(dev,

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko

Re: [PATCH v4 0/5] Add support for the TPM FF-A start method

[Jarkko Sakkinen]

On Mon, Feb 17, 2025 at 04:49:41PM -0600, Stuart Yoder wrote:
> Firmware Framework for Arm A-profile (FF-A) is a messaging framework
> for Arm-based systems, and in the context of the TPM CRB driver is used
> to signal 'start' to a CRB-based TPM service which is hosted in an
> FF-A secure partition running in TrustZone.
> 
> These patches add support for the CRB FF-A start method defined
> in the TCG ACPI specification v1.4 and the FF-A ABI defined
> in the Arm TPM Service CRB over FF-A (DEN0138) specification:
> https://developer.arm.com/documentation/den0138/latest/
> 
> The first patch adds an FF-A driver to handle the FF-A messaging when
> communicating with a CRB-based TPM secure partition built on FF-A.
> The driver is probed when the TPM secure partition is discovered by
> the Linux FF-A infrastructure.
> 
> The second patch consolidates the check for idle support in the CRB
> driver to one place.
> 
> The third patch defines the new ACPI start method enumeration for
> CRB over FF-A.
> 
> The fourth patch adds support for the FF-A ACPI start method to
> the TPM crb driver.
> 
> The fifth patch adds documentation explaining how the CRB driver
> and FF-A relate.
> 
> Version 4
> -fix warning from kernel test robot in patch 1
> -fix warnings from checkpatch.pl --strict
> -clean up unecessary parenthesis usage
> -update variable declaration to be reverse tree order
> -document exported functions in tpm_crb_ffa driver
> -remove unnecessary author and maintainer info in tpm_crb_ffa driver
> -fix declaration of variables to be in reverse tree order
> 
> Version 3
> -changed prefixes used throughout patch series to tpm_crb_ffa*
> 
> Version 2
> -updates to cover letter to define FF-A
> -added new patch with documentation
> -created pull request in ACPIA and added link to the patch
>  updating actbl3.h
> -added tpm_ prefix to the FF-A CRB driver
> 
> Stuart Yoder (5):
>   tpm_crb: implement driver compliant to CRB over FF-A
>   tpm_crb: clean-up and refactor check for idle support
>   ACPICA: add start method for Arm FF-A
>   tpm_crb: add support for the Arm FF-A start method
>   Documentation: tpm: add documentation for the CRB FF-A interface
> 
>  Documentation/security/tpm/tpm_ffa_crb.rst |  65 ++++
>  drivers/char/tpm/Kconfig                   |   9 +
>  drivers/char/tpm/Makefile                  |   1 +
>  drivers/char/tpm/tpm_crb.c                 | 105 +++++--
>  drivers/char/tpm/tpm_crb_ffa.c             | 348 +++++++++++++++++++++
>  drivers/char/tpm/tpm_crb_ffa.h             |  25 ++
>  include/acpi/actbl3.h                      |   1 +
>  7 files changed, 535 insertions(+), 19 deletions(-)
>  create mode 100644 Documentation/security/tpm/tpm_ffa_crb.rst
>  create mode 100644 drivers/char/tpm/tpm_crb_ffa.c
>  create mode 100644 drivers/char/tpm/tpm_crb_ffa.h
> 
> -- 
> 2.34.1
> 

checkpatch.pl --strict reported me some issues.

The first one is:

WARNING: please write a help paragraph that fully describes the config symbol
#41: FILE: drivers/char/tpm/Kconfig:213:
+config TCG_ARM_CRB_FFA
+       tristate "TPM CRB over Arm FF-A Transport"
+       depends on ARM_FFA_TRANSPORT
+       default y if (TCG_CRB && ARM_FFA_TRANSPORT)
+       help
+         If the Arm FF-A transport is used to access the TPM say Yes.
+         To compile this driver as a module, choose M here; the module
+         will be called tpm_crb_ffa.
+

To be totally honest with I've never fully grabbed what checkpatch means
by that message, i.e. what is the threshold for "fully described" :-)

So if someone can give advice on this, awesome, and let's do +1 round
of the patch, but with my limited knowledge I have no legit reason to
block this.

Nit: checkpatch could improve a bit that error message because "fully
describe" is not unambiguous terminology.

The second issue:

CHECK: Alignment should match open parenthesis
#378: FILE: drivers/char/tpm/tpm_crb_ffa.c:309:
+       if (tpm_crb_ffa->major_version != CRB_FFA_VERSION_MAJOR ||
+          (tpm_crb_ffa->minor_version > 0 &&

if (tpm_crb_ffa->major_version != CRB_FFA_VERSION_MAJOR ||
    (tpm_crb_ffa->minor_version > 0 &&

I think it should be like this.

The final issue reported:

WARNING: line length of 102 exceeds 100 columns
#764: FILE: drivers/char/tpm/tpm_crb.c:821:
+                               FW_BUG "TPM2 ACPI table has wrong size %u for start method type %d\n",

You could put that into two separate lines but maybe it would
even make sense to create helper for the branch given the deep
levels of nesting.

I.e. maybe it would make sense to encapsulate this into a helper:

		if (buf->header.length < (sizeof(*buf) + sizeof(*crb_ffa))) {
			dev_err(dev,
				FW_BUG "TPM2 ACPI table has wrong size %u for start method type %d\n",
				buf->header.length,
				ACPI_TPM2_CRB_WITH_ARM_FFA);
			rc = -EINVAL;
			goto out;
		}
		crb_ffa = ACPI_ADD_PTR(struct tpm2_crb_ffa, buf, sizeof(*buf));
		priv->ffa_flags = crb_ffa->flags;
		priv->ffa_attributes = crb_ffa->attributes;
		rc = tpm_crb_ffa_init();
		if (rc) {
			if (rc == -ENOENT) {  // FF-A driver is not available yet
				rc = -EPROBE_DEFER;
			}
			goto out;
		}


I looked at tpm_crb_ffa_init() too, and it's somewhat trivial, not very
useful and has only single call site in kernel:

/**
 * tpm_crb_ffa_init - called by the CRB driver to do any needed initialization
 *
 * This function is called by the tpm_crb driver during the tpm_crb
 * driver's initialization. If the tpm_crb_ffa has not been probed
 * yet, returns -ENOENT in order to force a retry.  If th ffa_crb
 * driver had been probed  but failed with an error, returns -ENODEV
 * in order to prevent further retries.
 *
 * Return: 0 on success, negative error code on failure.
 */
int tpm_crb_ffa_init(void)
{
	if (!tpm_crb_ffa)
		return -ENOENT;

	if (IS_ERR_VALUE(tpm_crb_ffa))
		return -ENODEV;

	return 0;
}
EXPORT_SYMBOL_GPL(tpm_crb_ffa_init);


Some questions arise here:

1. Wouldn't it be better idea to implement the code block residing
   in tpm_crb here?
2. Why it needs to be exported? Why it is not in the same compilation
   unit with tpm_crb? Cross-compilation unit calls extrapolates
   complexity and should have strong reasoning behind...

So yeah, this is area where we definitely need some rework. Sorry
about this boomerang. Sometimes checkpatch opens up new thoughts
and ideas :-) It's better to take step back always and do right
thing right. It's much more time consuming for all of us to fixup
things and backport fixes etc.

BR,  Jarkko

Re: [PATCH v4 0/5] Add support for the TPM FF-A start method

[Jason Gunthorpe]

On Sun, Mar 02, 2025 at 09:33:59PM +0200, Jarkko Sakkinen wrote:
> WARNING: line length of 102 exceeds 100 columns
> #764: FILE: drivers/char/tpm/tpm_crb.c:821:
> +                               FW_BUG "TPM2 ACPI table has wrong size %u for start method type %d\n",

Just ignore that, it is an error in checkpatch. Strings are required to
be long. I suspect FW_BUG confused it.

Jason

Re: [PATCH v4 0/5] Add support for the TPM FF-A start method

[Jarkko Sakkinen]

On Mon, 2025-03-03 at 12:55 -0400, Jason Gunthorpe wrote:
> On Sun, Mar 02, 2025 at 09:33:59PM +0200, Jarkko Sakkinen wrote:
> > WARNING: line length of 102 exceeds 100 columns
> > #764: FILE: drivers/char/tpm/tpm_crb.c:821:
> > +                              FW_BUG "TPM2 ACPI table has wrong
> > size %u for start method type %d\n",
> 
> Just ignore that, it is an error in checkpatch. Strings are required
> to
> be long. I suspect FW_BUG confused it.

Yep, as its own issue I think you are right.

I also noticed couple of additional style issues not picked
by checkpatch:

	if (rc == -ENOENT) {  // FF-A driver is not available yet
		rc = -EPROBE_DEFER;
	}

I.e. extra curly braces and "//" comment.

Should be:

	/* If driver is not available yet, request probe retry: */
	if (rc == -ENOENT)
		rc = -EPROBE_DEFER;

> 
> Jason
> 

BR, Jarkko

Re: [PATCH v4 0/5] Add support for the TPM FF-A start method

[Stuart Yoder]

On 3/4/25 10:02 AM, Jarkko Sakkinen wrote:
> On Mon, 2025-03-03 at 12:55 -0400, Jason Gunthorpe wrote:
>> On Sun, Mar 02, 2025 at 09:33:59PM +0200, Jarkko Sakkinen wrote:
>>> WARNING: line length of 102 exceeds 100 columns
>>> #764: FILE: drivers/char/tpm/tpm_crb.c:821:
>>> +                              FW_BUG "TPM2 ACPI table has wrong
>>> size %u for start method type %d\n",
>>
>> Just ignore that, it is an error in checkpatch. Strings are required
>> to
>> be long. I suspect FW_BUG confused it.
> 
> Yep, as its own issue I think you are right.
> 
> I also noticed couple of additional style issues not picked
> by checkpatch:
> 
> 	if (rc == -ENOENT) {  // FF-A driver is not available yet
> 		rc = -EPROBE_DEFER;
> 	}
> 
> I.e. extra curly braces and "//" comment.
> 
> Should be:
> 
> 	/* If driver is not available yet, request probe retry: */
> 	if (rc == -ENOENT)
> 		rc = -EPROBE_DEFER;
> 

I will respin and fix this.

Thanks,
Stuart