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

["Yao, Jiewen" <jiewen.yao@intel.com>]

I think it is a good idea, Mike.


In another threat, I mentioned that we need a real example for SPI instance, to prove this method is feasible.



There are 2 purposes.

1) To prove this patch works for SPI.

We need a real example to use this TpmIoLib.

It is OK to keep it close source if it is IP.

I just want to make sure this approach is feasible.



2) To demonstrate how to do that.

If you can use EFI_SPI_PROTOCOL, that would be perfect.


Thank you
Yao Jiewen


From: Kinney, Michael D
Sent: Wednesday, November 14, 2018 8:44 AM
To: Cohen, Eugene <eugene@hp.com>; edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Bin, Sung-Uk (???) <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