Re: [PATCH 3/4] SecurityPkg: add TpmIoLibMmio instance

["Zhang, Chao B" <chao.b.zhang@intel.com>]

Hi Eugene:
     TpmIoLib is designed to abstract the various ways of access(MMIO/SPI/I2C GPIO programing……) to TPM register space. From my perspective, It is necessary to have library interface well designed to fit all the cases we can see.

From: Cohen, Eugene [mailto:eugene@hp.com]
Sent: Wednesday, November 14, 2018 10:59 PM
To: Zhang, Chao B <chao.b.zhang@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>
Cc: Bin, Sung-Uk (빈성욱) <sunguk-bin@hp.com>
Subject: RE: [PATCH 3/4] SecurityPkg: add TpmIoLibMmio instance

Mike, Chao, Jiewen

Ø  [Chao] Infineon chip mentioned by Mike is an example but its register space doesn’t comply to PTP spec
Ø  [Mike] My experience is with DTPM and some I2C TPMs at 1.2 level.

We have experience with the TPM 1.2 Infineon I2C device and used a completely custom solution.  But I think that may be a 1.2 versus 2.0 difference.  I get the impression that TCG cleaned up their act a bit for the 2.0 spec – in fact we can see text to this effect in the PTP 1.03 spec<https://trustedcomputinggroup.org/wp-content/uploads/TCG_PC_Client_Platform_TPM_Profile_PTP_2.0_r1.03_v22.pdf> Seciton 2.3:

The CRB Interface is intended to be physical-bus agnostic, so that it could
be implemented on an LPC or SPI interface, as specified in this specification or on
another physical interface not specified.

Reading a bit deeper in the PTP spec it looks like there are two register layouts but not driven by the physical bus (LPC, SPI, I2C) but rather the access method (FIFO or CRB access mode) – see section 5.3.2 called "Register Space Addresses" to see the FIFO and CRB register layouts juxtaposed.

Looking at I2C in the PTP spec I can now see the situation is totally different – I2C uses a variation of FIFO mode and has a significantly different layout of registers, comparing Table 10 to Table 48 in the PTP spec.  So now I see where you're coming from (and why we didn't initially understand the concern).


Given that HP's use case is SPI and SPI is aligned to LPC, we believe going forward with the TpmIoLib abstraction is still quite useful.  Whenever somebody needs to support I2C TPM2 devices then they will need to author another DeviceLib instance since the register layout is different.  (Will there ever be a Quark with an I2C TPM2?)

TPM experts, I'd appreciate if you could confirm that my analysis of LPC/I2C/SPI and CRB/FIFO is correct.


Ø  [Mike] I would recommend that a full implementation of TpmIoLib for a few non MMIO TPM devices be completed and pass validation before we consider adding new lib class to edk2.  Perhaps using an edk2-staging branch would be a better place to start and you can document in the Readme.md the criteria that must be met before the new lib class meets the requirements for edk2/master.

This plan is fine by me – our intent in the original Request for Comment emails was to understand if a TpmIoLib style abstraction would be acceptable to save us the work of going down a path that can't be upstreamed and having to fix it later.  I think the answer you are giving is "yes, as long as it really works".  We are developing and validating this support right now so as long as the TPM stack doesn't change out from under us on edk2/master while we are developing it should be straightforward to upstream the portions we can share and the results, so long as it is understood that we cannot upstream the full stack due to the proprietary HW elements – as such I'm not sure how useful the staging branch would be although I don't mind doing it.  (In fact I think branch-based pull requests are more reviewable than email patchsets but I think I may be in the minority on this mailing list).

Thanks,

Eugene


From: Zhang, Chao B <chao.b.zhang@intel.com<mailto:chao.b.zhang@intel.com>>
Sent: Tuesday, November 13, 2018 6:53 PM
To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
Cc: Bin, Sung-Uk (빈성욱) <sunguk-bin@hp.com<mailto:sunguk-bin@hp.com>>
Subject: RE: [PATCH 3/4] SecurityPkg: add TpmIoLibMmio instance

Hi All:
   PTP 1.3 spec already include I2C support. It sees I2C TPM communication into 3 layers
      Application  Layer  -> Already implemented TCG PEI/TCG DXE
      TCG-I2C                   ->  Not implemented by UEFI TCG (Infineon chip mentioned by Mike is an example but its register space doesn’t comply to PTP spec)
        I2C                          ->  What TpmIoLib also need to address
   It will be good to have more use cases to see if TpmIoLib is sufficiently designed to meet generic TPM devices covered by TCG spec.


From: Kinney, Michael D
Sent: Wednesday, November 14, 2018 8:44 AM
To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Zhang, Chao B <chao.b.zhang@intel.com<mailto:chao.b.zhang@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Cc: Bin, Sung-Uk (???) <sunguk-bin@hp.com<mailto:sunguk-bin@hp.com>>
Subject: RE: [PATCH 3/4] SecurityPkg: add TpmIoLibMmio instance

Hi Eugene,

My experience is with DTPM and some I2C TPMs at 1.2 level.

One of the I2C TPMs was significantly different, so the TpmIoLib concept does not apply.

QuarkPlatformPkg\Library\Tpm12DeviceLibAtmelI2c

The other could potentially use something like TpmIoLib, but may require some different delays and timeout values than DTPM.

QuarkPlatformPkg\Library\Tpm12DeviceLibInfineonI2c

So maybe we offer 2 methods to port a TPM.  Once is TpmIoLib if the register layout and required delays are the same as DTPM with potentially a different base, and the other is to just implement the DeviceLib.

I would recommend that a full implementation of TpmIoLib for a few non MMIO TPM devices be completed and pass validation before we consider adding new lib class to edk2.  Perhaps using an edk2-staging branch would be a better place to start and you can document in the Readme.md the criteria that must be met before the new lib class meets the requirements for edk2/master.

Mike

From: Cohen, Eugene [mailto:eugene@hp.com]
Sent: Tuesday, November 13, 2018 3:24 PM
To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Zhang, Chao B <chao.b.zhang@intel.com<mailto:chao.b.zhang@intel.com>>
Cc: Bin, Sung-Uk (빈성욱) <sunguk-bin@hp.com<mailto:sunguk-bin@hp.com>>
Subject: RE: [PATCH 3/4] SecurityPkg: add TpmIoLibMmio instance

Mike,

There is a prevalence of base address nomenclature in the DTPM library like:

                return TisPcRequestUseTpm ((TIS_PC_REGISTERS_PTR) (UINTN) PcdGet64 (PcdTpmBaseAddress));

like from Tpm2Tis.c.

Ø  shouldn’t the Address be the relative address from a base?

I think it is, to the extent that the PcdTpmBaseAddress already exists.

Ø  Or are you using the full DTPM MMIO address on purpose and you expect non DTPM MMIO implementations to translate from the DTPM MMIO address to the equivalent register access in on an alternate bus type?

My thought is that the PcdTpmBaseAddress could be set to zero (or whatever non-MMIO offset makes sense).

Ø  Wouldn't it be better to have an abstraction for different TPM registers and the DTPM implementation would convert to a full MMIO address in the lib implementation?

I've been led to understand that the TPM registers are the same in both cases.  I haven't verified this myself - but if you have data to the contrary please let us know so we stop going down this path.

My main reason for resisting creating a new library at the Tpm2DeviceLib layer because the DTPM library contains a lot of logic around how to talk to the TPM that extends well beyond the access mechanism that we would not want to duplicate to another library instance.  I'm looking inside Tpm2Tis.c and Tpm2Ptp.c to get this perspective.

Thanks,

Eugene

From: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Sent: Tuesday, November 13, 2018 3:58 PM
To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Zhang, Chao B <chao.b.zhang@intel.com<mailto:chao.b.zhang@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Cc: Bin, Sung-Uk (빈성욱) <sunguk-bin@hp.com<mailto:sunguk-bin@hp.com>>
Subject: RE: [PATCH 3/4] SecurityPkg: add TpmIoLibMmio instance

Eugene,

It appears you are expecting the Address parameter
to be the full MMIO address for DTPM. If you are
wanting this lib class to abstract the access for
different bus types, shouldn’t the Address be the
relative address from a base?

Or are you using the full DTPM MMIO address on
purpose and you expect non DTPM MMIO implementations
to translate from the DTPM MMIO address to the
equivalent register access in on an alternate bus
type?

Wouldn't it be better to have an abstraction for
different TPM registers and the DTPM implementation
would convert to a full MMIO address in the lib
implementation?

Thanks,

Mike


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-
> bounces@lists.01.org<mailto:bounces@lists.01.org>] On Behalf Of Cohen, Eugene
> Sent: Tuesday, November 13, 2018 2:13 PM
> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen
> <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Zhang, Chao B
> <chao.b.zhang@intel.com<mailto:chao.b.zhang@intel.com>>
> Cc: Bin, Sung-Uk (빈성욱) <sunguk-bin@hp.com<mailto:sunguk-bin@hp.com>>
> Subject: [edk2] [PATCH 3/4] SecurityPkg: add
> TpmIoLibMmio instance
>
> SecurityPkg: add TpmIoLibMmio instance
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Cc: Chao Zhang <chao.b.zhang@intel.com<mailto:chao.b.zhang@intel.com>>
> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
> Signed-off-by: Eugene Cohen <eugene@hp.com<mailto:eugene@hp.com>>
> ---
> SecurityPkg/Library/TpmIoLibMmio/TpmIoLibMmio.c |
> 221 ++++++++++++++++++++
> SecurityPkg/Library/TpmIoLibMmio/TpmIoLibMmio.inf |
> 41 ++++
> 2 files changed, 262 insertions(+)
>
> diff --git
> a/SecurityPkg/Library/TpmIoLibMmio/TpmIoLibMmio.c
> b/SecurityPkg/Library/TpmIoLibMmio/TpmIoLibMmio.c
> new file mode 100644
> index 0000000..56f3187
> --- /dev/null
> +++ b/SecurityPkg/Library/TpmIoLibMmio/TpmIoLibMmio.c
> @@ -0,0 +1,221 @@
> +/** @file
> + This library is to abstract TPM2 register accesses
> so that a common
> + interface can be used for multiple underlying busses
> such as TPM,
> + SPI, or I2C access.
> +
> +Copyright (c) 2018 HP Development Company, L.P.
> +This program and the accompanying materials
> +are licensed and made available under the terms and
> conditions of the BSD License
> +which accompanies this distribution. The full text of
> the license may be found at
> +http://opensource.org/licenses/bsd-license.php
> +
> +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN
> "AS IS" BASIS,
> +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include <Base.h>
> +
> +#include <Library/TpmIoLib.h>
> +#include <Library/IoLib.h>
> +
> +
> +
> +/**
> + Reads an 8-bit TPM register.
> +
> + Reads the 8-bit TPM register specified by Address.
> The 8-bit read value is
> + returned. This function must guarantee that all TPM
> read and write
> + operations are serialized.
> +
> + If 8-bit TPM register operations are not supported,
> then ASSERT().
> +
> + @param Address The TPM register to read.
> +
> + @return The value read.
> +
> +**/
> +UINT8
> +EFIAPI
> +TpmRead8 (
> + IN UINTN Address
> + )
> +{
> + return MmioRead8 (Address);
> +}
> +
> +/**
> + Writes an 8-bit TPM register.
> +
> + Writes the 8-bit TPM register specified by Address
> with the value specified
> + by Value and returns Value. This function must
> guarantee that all TPM read
> + and write operations are serialized.
> +
> + If 8-bit TPM register operations are not supported,
> then ASSERT().
> +
> + @param Address The TPM register to write.
> + @param Value The value to write to the TPM
> register.
> +
> + @return Value.
> +
> +**/
> +UINT8
> +EFIAPI
> +TpmWrite8 (
> + IN UINTN Address,
> + IN UINT8 Value
> + )
> +{
> + return MmioWrite8 (Address, Value);
> +}
> +
> +
> +/**
> + Reads a 16-bit TPM register.
> +
> + Reads the 16-bit TPM register specified by Address.
> The 16-bit read value is
> + returned. This function must guarantee that all TPM
> read and write
> + operations are serialized.
> +
> + If 16-bit TPM register operations are not supported,
> then ASSERT().
> + If Address is not aligned on a 16-bit boundary, then
> ASSERT().
> +
> + @param Address The TPM register to read.
> +
> + @return The value read.
> +
> +**/
> +UINT16
> +EFIAPI
> +TpmRead16 (
> + IN UINTN Address
> + )
> +{
> + return MmioRead16 (Address);
> +}
> +
> +
> +/**
> + Writes a 16-bit TPM register.
> +
> + Writes the 16-bit TPM register specified by Address
> with the value specified
> + by Value and returns Value. This function must
> guarantee that all TPM read
> + and write operations are serialized.
> +
> + If 16-bit TPM register operations are not supported,
> then ASSERT().
> + If Address is not aligned on a 16-bit boundary, then
> ASSERT().
> +
> + @param Address The TPM register to write.
> + @param Value The value to write to the TPM
> register.
> +
> + @return Value.
> +
> +**/
> +UINT16
> +EFIAPI
> +TpmWrite16 (
> + IN UINTN Address,
> + IN UINT16 Value
> + )
> +{
> + return MmioWrite16 (Address, Value);
> +}
> +
> +/**
> + Reads a 32-bit TPM register.
> +
> + Reads the 32-bit TPM register specified by Address.
> The 32-bit read value is
> + returned. This function must guarantee that all TPM
> read and write
> + operations are serialized.
> +
> + If 32-bit TPM register operations are not supported,
> then ASSERT().
> + If Address is not aligned on a 32-bit boundary, then
> ASSERT().
> +
> + @param Address The TPM register to read.
> +
> + @return The value read.
> +
> +**/
> +UINT32
> +EFIAPI
> +TpmRead32 (
> + IN UINTN Address
> + )
> +{
> + return MmioRead32 (Address);
> +}
> +
> +/**
> + Writes a 32-bit TPM register.
> +
> + Writes the 32-bit TPM register specified by Address
> with the value specified
> + by Value and returns Value. This function must
> guarantee that all TPM read
> + and write operations are serialized.
> +
> + If 32-bit TPM register operations are not supported,
> then ASSERT().
> + If Address is not aligned on a 32-bit boundary, then
> ASSERT().
> +
> + @param Address The TPM register to write.
> + @param Value The value to write to the TPM
> register.
> +
> + @return Value.
> +
> +**/
> +UINT32
> +EFIAPI
> +TpmWrite32 (
> + IN UINTN Address,
> + IN UINT32 Value
> + )
> +{
> + return MmioWrite32 (Address, Value);
> +}
> +
> +
> +/**
> + Reads a 64-bit TPM register.
> +
> + Reads the 64-bit TPM register specified by Address.
> The 64-bit read value is
> + returned. This function must guarantee that all TPM
> read and write
> + operations are serialized.
> +
> + If 64-bit TPM register operations are not supported,
> then ASSERT().
> + If Address is not aligned on a 64-bit boundary, then
> ASSERT().
> +
> + @param Address The TPM register to read.
> +
> + @return The value read.
> +
> +**/
> +UINT64
> +EFIAPI
> +TpmRead64 (
> + IN UINTN Address
> + )
> +{
> + return MmioRead64 (Address);
> +}
> +
> +/**
> + Writes a 64-bit TPM register.
> +
> + Writes the 64-bit TPM register specified by Address
> with the value specified
> + by Value and returns Value. This function must
> guarantee that all TPM read
> + and write operations are serialized.
> +
> + If 64-bit TPM register operations are not supported,
> then ASSERT().
> + If Address is not aligned on a 64-bit boundary, then
> ASSERT().
> +
> + @param Address The TPM register to write.
> + @param Value The value to write to the TPM
> register.
> +
> +**/
> +UINT64
> +EFIAPI
> +TpmWrite64 (
> + IN UINTN Address,
> + IN UINT64 Value
> + )
> +{
> + return MmioWrite64 (Address, Value);
> +}
> diff --git
> a/SecurityPkg/Library/TpmIoLibMmio/TpmIoLibMmio.inf
> b/SecurityPkg/Library/TpmIoLibMmio/TpmIoLibMmio.inf
> new file mode 100644
> index 0000000..a40f90e
> --- /dev/null
> +++ b/SecurityPkg/Library/TpmIoLibMmio/TpmIoLibMmio.inf
> @@ -0,0 +1,41 @@
> +## @file
> +# Provides BaseCrypto SHA1 hash service
> +#
> +# This library is to abstract TPM2 register accesses
> so that a common
> +# interface can be used for multiple underlying
> busses such as TPM,
> +# SPI, or I2C access.
> +#
> +# Copyright (c) 2018 HP Development Company, L.P.
> +# This program and the accompanying materials
> +# are licensed and made available under the terms and
> conditions of the BSD License
> +# which accompanies this distribution. The full text
> of the license may be found at
> +# http://opensource.org/licenses/bsd-license.php
> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON
> AN "AS IS" BASIS,
> +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> EITHER EXPRESS OR IMPLIED.
> +#
> +##
> +
> +[Defines]
> + INF_VERSION = 0x00010005
> + BASE_NAME = TpmIoLibMmio
> + FILE_GUID = B51DE0C6-8A48-4AF9-
> BF88-5A46CC853FC8
> + MODULE_TYPE = BASE
> + VERSION_STRING = 1.0
> + LIBRARY_CLASS = NULL
> +
> +#
> +# The following information is for reference only and
> not required by the build tools.
> +#
> +# VALID_ARCHITECTURES = IA32 X64 ARM
> AARCH64
> +#
> +
> +[Sources]
> + TpmIoLibMmio.c
> +
> +[Packages]
> + MdePkg/MdePkg.dec
> + SecurityPkg/SecurityPkg.dec
> +
> +[LibraryClasses]
> + BaseLib
> + IoLib
> --
> 2.7.4
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel