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:[email protected]]
Sent: Wednesday, November 14, 2018 10:59 PM
To: Zhang, Chao B <[email protected]>; Kinney, Michael D
<[email protected]>; [email protected]; Yao, Jiewen
<[email protected]>
Cc: Bin, Sung-Uk (빈성욱) <[email protected]>
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 <[email protected]<mailto:[email protected]>>
Sent: Tuesday, November 13, 2018 6:53 PM
To: Kinney, Michael D
<[email protected]<mailto:[email protected]>>; Cohen, Eugene
<[email protected]<mailto:[email protected]>>;
[email protected]<mailto:[email protected]>; Yao, Jiewen
<[email protected]<mailto:[email protected]>>
Cc: Bin, Sung-Uk (빈성욱) <[email protected]<mailto:[email protected]>>
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 <[email protected]<mailto:[email protected]>>;
[email protected]<mailto:[email protected]>; Yao, Jiewen
<[email protected]<mailto:[email protected]>>; Zhang, Chao B
<[email protected]<mailto:[email protected]>>; Kinney, Michael D
<[email protected]<mailto:[email protected]>>
Cc: Bin, Sung-Uk (???) <[email protected]<mailto:[email protected]>>
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:[email protected]]
Sent: Tuesday, November 13, 2018 3:24 PM
To: Kinney, Michael D
<[email protected]<mailto:[email protected]>>;
[email protected]<mailto:[email protected]>; Yao, Jiewen
<[email protected]<mailto:[email protected]>>; Zhang, Chao B
<[email protected]<mailto:[email protected]>>
Cc: Bin, Sung-Uk (빈성욱) <[email protected]<mailto:[email protected]>>
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
<[email protected]<mailto:[email protected]>>
Sent: Tuesday, November 13, 2018 3:58 PM
To: Cohen, Eugene <[email protected]<mailto:[email protected]>>;
[email protected]<mailto:[email protected]>; Yao, Jiewen
<[email protected]<mailto:[email protected]>>; Zhang, Chao B
<[email protected]<mailto:[email protected]>>; Kinney, Michael D
<[email protected]<mailto:[email protected]>>
Cc: Bin, Sung-Uk (빈성욱) <[email protected]<mailto:[email protected]>>
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-
> [email protected]<mailto:[email protected]>] On Behalf Of Cohen, Eugene
> Sent: Tuesday, November 13, 2018 2:13 PM
> To: [email protected]<mailto:[email protected]>; Yao, Jiewen
> <[email protected]<mailto:[email protected]>>; Zhang, Chao B
> <[email protected]<mailto:[email protected]>>
> Cc: Bin, Sung-Uk (빈성욱) <[email protected]<mailto:[email protected]>>
> Subject: [edk2] [PATCH 3/4] SecurityPkg: add
> TpmIoLibMmio instance
>
> SecurityPkg: add TpmIoLibMmio instance
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Cc: Chao Zhang <[email protected]<mailto:[email protected]>>
> Cc: Jiewen Yao <[email protected]<mailto:[email protected]>>
> Signed-off-by: Eugene Cohen <[email protected]<mailto:[email protected]>>
> ---
> 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
> [email protected]<mailto:[email protected]>
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
