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

[PATCH v5 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 5
-tpm_ffa_crb patch: removed module version
-tpm_ffa_crb patch: fixed module description
-tpm_ffa_crb patch: updated comment on mutex declaration
-reworded commit message for patch 2 as per Jarkko's 
 suggestion
-added Acked tag by Sudeep to patch 1 for FF-A changes 
-added Reviewed-by tag to patches 3 and 4

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 v5 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.

Acked-by: Sudeep Holla <sudeep.holla@arm.com>
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..3169a87a56b6
--- /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;
+	/* lock to protect sending of FF-A messages: */
+	struct mutex msg_data_lock;
+	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("TPM CRB FFA driver");
+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

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

[Jarkko Sakkinen]

On Wed, Feb 19, 2025 at 02:10:10PM -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.
> 
> Acked-by: Sudeep Holla <sudeep.holla@arm.com>
> Signed-off-by: Stuart Yoder <stuart.yoder@arm.com>

Cutting hairs now but as I cannot test this and this is 1/5:
can this patch be run without 2/5-4/5?

The policy is that every patch should leave kernel tree to
a state where it compiles and runs.

BR, Jarkko

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

[Jarkko Sakkinen]

On Thu, Feb 20, 2025 at 11:28:58AM +0200, Jarkko Sakkinen wrote:
> On Wed, Feb 19, 2025 at 02:10:10PM -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.
> > 
> > Acked-by: Sudeep Holla <sudeep.holla@arm.com>
> > Signed-off-by: Stuart Yoder <stuart.yoder@arm.com>
> 
> Cutting hairs now but as I cannot test this and this is 1/5:
> can this patch be run without 2/5-4/5?
> 
> The policy is that every patch should leave kernel tree to
> a state where it compiles and runs.

The root reason for this is that wrong ordered patch sets
commited to Git add difficulty to bisection in the long-term.

BR, Jarkko

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

[Stuart Yoder]

On 2/20/25 3:28 AM, Jarkko Sakkinen wrote:
> On Wed, Feb 19, 2025 at 02:10:10PM -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.
>>
>> Acked-by: Sudeep Holla <sudeep.holla@arm.com>
>> Signed-off-by: Stuart Yoder <stuart.yoder@arm.com>
> 
> Cutting hairs now but as I cannot test this and this is 1/5:
> can this patch be run without 2/5-4/5?

Yes. This 1/5 patch has no compile time or runtime dependency
on 2/5-4/5.

Thanks,
Stuart

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

[Stuart Yoder]

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))
-- 
2.34.1

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

[Jarkko Sakkinen]

On Wed, Feb 19, 2025 at 02:10:11PM -0600, Stuart Yoder wrote:
> 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))
> -- 
> 2.34.1
> 

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

BR, Jarkko

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

[Stuart Yoder]

On 2/20/25 3:29 AM, Jarkko Sakkinen wrote:
> On Wed, Feb 19, 2025 at 02:10:11PM -0600, Stuart Yoder wrote:
>> 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)) 
>> -- 
>> 2.34.1
>>
> 
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

Thanks for the review.  Do you want me to respin and send out
a v6 with your Reviewed-by tags on patches 2/5 and 5/5?

Thanks,
Stuart

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

[Paul Menzel]

Dear Stuart,


Thank you for the patch. Should you respin, you could spell *clean up* 
in the summary/title with a space.

The diff looks good.


Kind regards,

Paul

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

[Jarkko Sakkinen]

On Thu, 2025-02-20 at 20:09 +0100, Paul Menzel wrote:
> Dear Stuart,
> 
> 
> Thank you for the patch. Should you respin, you could spell *clean
> up* 
> in the summary/title with a space.
> 
> The diff looks good.
> 
> 
> Kind regards,
> 
> Paul

So for the sake of overall good it is better maybe to keep
this immutable as this will make it more plausible for
potential testers.

BR, Jarkko

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

[Jarkko Sakkinen]

On Thu, 2025-02-20 at 13:04 -0600, Stuart Yoder wrote:
> 
> 
> On 2/20/25 3:29 AM, Jarkko Sakkinen wrote:
> > On Wed, Feb 19, 2025 at 02:10:11PM -0600, Stuart Yoder wrote:
> > > 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)) 
> > > -- 
> > > 2.34.1
> > > 
> > 
> > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> 
> Thanks for the review.  Do you want me to respin and send out
> a v6 with your Reviewed-by tags on patches 2/5 and 5/5?

It's fine I can append them :-)

You might have to hold up until to the first work week of
March because I'm actually on holiday up until that but
yeah no need for extra noise.

Up until that tested-by's to this patch set version are
obviously welcome but it's now definitely good enough
as far as I'm concerned.

> 
> Thanks,
> Stuart

BR, Jarkko

[PATCH v5 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
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
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 v5 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.

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
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 v5 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 v5 5/5] Documentation: tpm: add documentation for the CRB FF-A interface

[Jarkko Sakkinen]

On Wed, Feb 19, 2025 at 02:10:14PM -0600, Stuart Yoder wrote:
> 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
> 

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

BR, Jarkko