Message-ID: <5a494de5-b004-440c-bcdf-7bdfa3a8c508@apertussolutions.com>
Date: Fri, 22 Mar 2024 20:39:28 -0400
From: "Daniel P. Smith" <dpsmith@...rtussolutions.com>
To: Jarkko Sakkinen <jarkko@...nel.org>, linux-integrity@...r.kernel.org
Cc: Jonathan Corbet <corbet@....net>,
 Lino Sanfilippo <l.sanfilippo@...bus.com>, Jason Gunthorpe <jgg@...pe.ca>,
 Peter Huewe <peterhuewe@....de>,
 James Bottomley <James.Bottomley@...senPartnership.com>,
 Alexander Steffen <Alexander.Steffen@...ineon.com>,
 keyrings@...r.kernel.org, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, Randy Dunlap <rdunlap@...radead.org>,
 Richard Cochran <richardcochran@...il.com>,
 "open list:PTP HARDWARE CLOCK SUPPORT:Keyword:(?:b|_)ptp(?:b|_)"
 <netdev@...r.kernel.org>
Subject: Re: [PATCH v4] Documentation: tpm_tis

Hi Jarkko,

On 3/22/24 08:35, Jarkko Sakkinen wrote:
> Based recent discussions on LKML, provide preliminary bits of tpm_tis_core
> dependent drivers. Includes only bare essentials but can be extended later
> on case by case. This way some people may even want to read it later on.
> 
> Cc: Jonathan Corbet <corbet@....net>
> CC: Daniel P. Smith <dpsmith@...rtussolutions.com>
> Cc: Lino Sanfilippo <l.sanfilippo@...bus.com>
> Cc: Jason Gunthorpe <jgg@...pe.ca>
> Cc: Peter Huewe <peterhuewe@....de>
> Cc: James Bottomley <James.Bottomley@...senPartnership.com>
> Cc: Alexander Steffen <Alexander.Steffen@...ineon.com>
> Cc: keyrings@...r.kernel.org
> Cc: linux-doc@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org
> Cc: linux-integrity@...r.kernel.org
> Cc: Randy Dunlap <rdunlap@...radead.org>
> Signed-off-by: Jarkko Sakkinen <jarkko@...nel.org>
> ---
> v4:
> - Extended the text to address some of Stefan's concerns with v2.
> - Had to unfortunately remove Randy's reviewed-by because of this, given
>    the amount of text added.
> v3:
> - Fixed incorrect buffer size:
>    https://lore.kernel.org/linux-integrity/d957dbd3-4975-48d7-abc5-1a01c0959ea3@linux.ibm.com/
> v2:
> - Fixed errors reported by Randy:
>    https://lore.kernel.org/all/aed28265-d677-491a-a045-24b351854b24@infradead.org/
> - Improved the text a bit to have a better presentation.
> ---
>   Documentation/security/tpm/index.rst   |  1 +
>   Documentation/security/tpm/tpm_tis.rst | 46 ++++++++++++++++++++++++++
>   2 files changed, 47 insertions(+)
>   create mode 100644 Documentation/security/tpm/tpm_tis.rst
> 
> diff --git a/Documentation/security/tpm/index.rst b/Documentation/security/tpm/index.rst
> index fc40e9f23c85..f27a17f60a96 100644
> --- a/Documentation/security/tpm/index.rst
> +++ b/Documentation/security/tpm/index.rst
> @@ -5,6 +5,7 @@ Trusted Platform Module documentation
>   .. toctree::
>   
>      tpm_event_log
> +   tpm_tis
>      tpm_vtpm_proxy
>      xen-tpmfront
>      tpm_ftpm_tee
> diff --git a/Documentation/security/tpm/tpm_tis.rst b/Documentation/security/tpm/tpm_tis.rst
> new file mode 100644
> index 000000000000..b448ea3db71d
> --- /dev/null
> +++ b/Documentation/security/tpm/tpm_tis.rst
> @@ -0,0 +1,46 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=========================
> +TPM FIFO interface driver
> +=========================
> +
> +TCG PTP Specification defines two interface types: FIFO and CRB. The former is

I believe in the spec, the authors were specific to classify these as 
software interfaces. Not sure if you would want to carry that 
distinction into this document.

> +based on sequenced read and write operations,  and the latter is based on a
> +buffer containing the full command or response.
> +
> +FIFO (First-In-First-Out) interface is used by the tpm_tis_core dependent
> +drivers. Originally Linux had only a driver called tpm_tis, which covered
> +memory mapped (aka MMIO) interface but it was later on extended to cover other
> +physical interfaces supported by the TCG standard.

Would it be worth clarifying here that one of those interfaces is 
defined in the Mobile TPM specification, which also refers to its 
interface as the CRB interface. In the past, this has caused great 
confusion when working with individuals from the embedded community, 
e.g., Arm. The Mobile TPM CRB interface, which can also be found being 
used by some generations of AMD fTPM, is a doorbell style interface 
using general-purpose memory. I would also point out that the Mobile TPM 
CRB interface does not provide for the concept of localities.

In relation to the MMIO backed interfaces, I have heard comment that the 
software interfaces were not meant to require the physical interface be 
MMIO. In fact, in section 9.2, "Hardware Implementation of a TPM in a PC 
Client Platform", there is a comment about Locality 4 registers being 
accessible via an implementation specific mechanism other than MMIO. 
Additionally, there were some discussions about clarifying the PTP on 
how the software interfaces might be expected to work for a 
general-purpose memory backed implementation.

> +For legacy compliance the original MMIO driver is called tpm_tis and the
> +framework for FIFO drivers is named as tpm_tis_core. The postfix "tis" in
> +tpm_tis comes from the TPM Interface Specification, which is the hardware
> +interface specification for TPM 1.x chips.
> +
> +Communication is based on a 20 KiB buffer shared by the TPM chip through a
> +hardware bus or memory map, depending on the physical wiring. The buffer is
> +further split into five equal-size 4 KiB buffers, which provide equivalent
> +sets of registers for communication between the CPU and TPM. These
> +communication endpoints are called localities in the TCG terminology.
> +
> +When the kernel wants to send commands to the TPM chip, it first reserves
> +locality 0 by setting the requestUse bit in the TPM_ACCESS register. The bit is
> +cleared by the chip when the access is granted. Once it completes its
> +communication, the kernel writes the TPM_ACCESS.activeLocality bit. This
> +informs the chip that the locality has been relinquished.
> +
> +Pending localities are served in order by the chip in descending order, one at
> +a time:

I think I get what you are trying to say, but I find the wording here 
could be a bit misleading. Instead of saying they are served in order, I 
would suggest saying something to the extent that: there are five 
localities, more than one can be requested at a time, but only one will 
ever be active. Selection priority when multiple requests are pending is 
detailed in the Informative comment on locality priority in Section 
6.2.1, "TPM Locality Levels".

> +- Locality 0 has the lowest priority.
> +- Locality 5 has the highest priority.

Do you mean Locality 4?

> +Further information on the purpose and meaning of the localities can be found
> +in section 3.2 of the TCG PC Client Platform TPM Profile Specification.
> +
> +References
> +==========
> +
> +TCG PC Client Platform TPM Profile (PTP) Specification
> +https://trustedcomputinggroup.org/resource/pc-client-platform-tpm-profile-ptp-specification/

Powered by blists - more mailing lists