Message-Id: <D01CLI2TC5SZ.1A48PDHM5F3UA@kernel.org>
Date: Sat, 23 Mar 2024 20:40:48 +0200
From: "Jarkko Sakkinen" <jarkko@...nel.org>
To: "Daniel P. Smith" <dpsmith@...rtussolutions.com>,
 <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

On Sat Mar 23, 2024 at 2:39 AM EET, Daniel P. Smith wrote:
> 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.

I don't necessarily disagree but it is out of scope for this. I'm not
sure tho why "mobile" CRB would ever need that sort of separate
dicussion.

Some CRB implementations have localities some don't, and also fTPM
implementations on x86 vary, no need to state that separately for
mobile.

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

So what is the error in the current want, i.e. what do you want to
change? I think this type of stuff can be extended but I don't want
to pollute this with too much detail at this point.

Only what matters for daily kernel development as reminder is what
this type of doc's should contain.

BR, Jarkko

Powered by blists - more mailing lists