Re: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface


Gao, Zhichao
 

-----Original Message-----
From: Yao, Jiewen <jiewen.yao@...>
Sent: Friday, January 3, 2020 1:59 PM
To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@...>; Gao, Zhichao
<zhichao.gao@...>
Cc: Wang, Jian J <jian.j.wang@...>; Zhang, Chao B
<chao.b.zhang@...>; Justen, Jordan L <jordan.l.justen@...>;
Laszlo Ersek <lersek@...>; Ard Biesheuvel <ard.biesheuvel@...>;
Marc-André Lureau <marcandre.lureau@...>; Stefan Berger
<stefanb@...>
Subject: RE: [edk2-devel] [PATCH 00/13] Extend and fix the TCG/TCG2 Physical
Presence Interface

Some other comment:

6. I disagree to move NvData.h to securityPkg.
This is designed to be an internal variable.
Why it need to move to security pkg to share with other driver?

I agree that duplicating code is bad solution.
And exposing internal data structure is as bad as duplicating code.

By design, the platform can have its own TCG platform lib and its own NvData.h.
Would you please help me understand why this is needed?
Some definitions in the NvData.h are useful for Tcg2VerndorLib, such as:
#define TPM_DEVICE_NULL 0
#define TPM_DEVICE_1_2 1
#define TPM_DEVICE_2_0_DTPM 2
#define TPM_DEVICE_MIN TPM_DEVICE_1_2
#define TPM_DEVICE_MAX TPM_DEVICE_2_0_DTPM
#define TPM_DEVICE_DEFAULT TPM_DEVICE_1_2
I'd better to move the common definition to a normal tcg device related header file instead of moving this file.


7. Removing assert has no relationship with this extend PPI interface.
If you want, please file another Bugzilla and handle it separately.

8. Fixing bug to follow spec is good.
But I do not see the relationship with the PPI interface extension.
Please file another Bugzilla and handle that separately.
For #7, #8 and other non-extend changes, I would add new Bugzilla to separate the patches.

Thanks,
Zhichao


Thank you
Yao Jiewen



-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao,
Jiewen
Sent: Friday, January 3, 2020 1:30 PM
To: Gao, Zhichao <zhichao.gao@...>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@...>; Zhang, Chao B
<chao.b.zhang@...>; Justen, Jordan L
<jordan.l.justen@...>; Laszlo Ersek <lersek@...>; Ard
Biesheuvel <ard.biesheuvel@...>; Marc-André Lureau
<marcandre.lureau@...>; Stefan Berger <stefanb@...>
Subject: Re: [edk2-devel] [PATCH 00/13] Extend and fix the TCG/TCG2
Physical Presence Interface

What is the platform use case?
Please give at least some examples.


-----Original Message-----
From: Gao, Zhichao <zhichao.gao@...>
Sent: Friday, January 3, 2020 1:08 PM
To: Yao, Jiewen <jiewen.yao@...>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@...>; Zhang, Chao B
<chao.b.zhang@...>; Justen, Jordan L
<jordan.l.justen@...>; Laszlo Ersek <lersek@...>; Ard
Biesheuvel
<ard.biesheuvel@...>;
Marc-André Lureau <marcandre.lureau@...>; Stefan Berger
<stefanb@...>
Subject: RE: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical
Presence Interface

See below.

-----Original Message-----
From: Yao, Jiewen
Sent: Friday, January 3, 2020 11:09 AM
To: Gao, Zhichao <zhichao.gao@...>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@...>; Zhang, Chao B
<chao.b.zhang@...>; Justen, Jordan L
<jordan.l.justen@...>; Laszlo Ersek <lersek@...>; Ard
Biesheuvel <ard.biesheuvel@...>; Marc-André Lureau
<marcandre.lureau@...>; Stefan Berger
<stefanb@...>
Subject: RE: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical
Presence Interface

Hi
I am not clear on the purpose of this extension.

The Bugzilla just describes the solution.
But what is the problem you are trying to resolve?

I completely don’t understand.

Please do consider add the background information there.
Or it is hard for me to comment.


Thank you
Yao Jiewen

-----Original Message-----
From: Gao, Zhichao <zhichao.gao@...>
Sent: Friday, January 3, 2020 11:04 AM
To: devel@edk2.groups.io
Cc: Yao, Jiewen <jiewen.yao@...>; Wang, Jian J
<jian.j.wang@...>; Zhang, Chao B <chao.b.zhang@...>;
Justen, Jordan L <jordan.l.justen@...>; Laszlo Ersek
<lersek@...>; Ard Biesheuvel <ard.biesheuvel@...>;
Marc-André Lureau <marcandre.lureau@...>; Stefan Berger
<stefanb@...>
Subject: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical
Presence Interface

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2443

1. Add two interfaces Tcg2PpVendorLibExecutePendingRequestEx and
Tcg2PpVendorLibSubmitRequestToPreOSFunctionEx to Tcg2PpVendorLib.
It
has one more parameter PPData (type EFI_TCG2_PHYSICAL_PRESENCE)
to transfer more data.
2. Use the Ex version instead of original one in
Tcg2PhysicalPresenceLib 3. Add a pcd
PcdPhysicalPresenceUserConfirmTimeout to control the user
confirm input key timeout.
4. Add FunctionIndex to structure type
EFI_TCG2_PHYSICAL_PRESENCE to transfer
mTcgNvs->PhysicalPresence.Parameter data.
5. Add parameter FunctionIndex to
Tcg2PhysicalPresenceLibSubmitRequestToPreOSFunctionEx
to initialize the PPdata.
Background:
Some platforms implement their own Tcg2PpVendorLib and want to
operate with more parameters.

#1-#2 and #4-#5 changes aim to add extend the interface with PPdata.
PPdata pointer can transfer most of the required parameter to do the
operation.
All the extend changes is for the PPdata and transfer to the
platform implemented interfaces. This would affect the platforms
which implement
their
own Tcg2PpVendorLib.
But for open source platform, I didn't see any implementation of it.

#3 aims to add a pcd to let the customers to decide whether to wait
for the input forever or with a timeout count.

6. Move Tcg2ConfigNvData.h from SecurityPkg/Tcg/Tcg2Config to
SecurityPkg/Include.
It is useful for platform code to implement their own
Tcg2PhysicalPresenceLib.

#6 is a movement to decrease the duplicated code at platform side if
the platform code implement its own TCG library or driver.

7. Replace the ASSERT with error code return in
TpmPhysicalPresenceLib
#7 aims to remove the ASSERT because it is not critical. ASSERT when
fail to
call
TpmPhysicalPresence and GetTpmCapability is not a good behavior.

8. Fix one operation
(PHYSICAL_PRESENCE_DEACTIVATE_DISABLE_OWNER_FALSE) flow of
TcgPhysicalPresenceLib (refer to Physical Presence Interface Spec Page
37).

#8 is a bug fix to follow the spec.

Thanks,
Zhichao


Cc: Jiewen Yao <jiewen.yao@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Chao Zhang <chao.b.zhang@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Ard Biesheuvel <ard.biesheuvel@...>
Cc: Marc-André Lureau <marcandre.lureau@...>
Cc: Stefan Berger <stefanb@...>
Signed-off-by: Zhichao Gao <zhichao.gao@...> Zhichao Gao (13):
SecurityPkg/Tcg2PpVerndorLib: Add two Ex function to handle PPdata
SecurityPkg/Tcg2PpVendorLib: Add implementation of new Ex function
SecurityPkg/Tcg2PhysicalPresenceLib: Use the new Ex function
SecurityPkg/SmmTcg2PhysicalPresenceLib: Use the new Ex function
SecurityPkg/dec: Add a pcd for user response wait time
OvmfPkg/Tcg2PhysicalPresenceLib: Use pcd for user response wait time
SecurityPkg/Tcg2PhysicalPresenceLib: Use Pcd for user resp wait time
SecurityPkg/TcgPyhsicalPresenceLib: Use Pcd for user resp wait time
SecurityPkg/Tcg2PhysicalPresenceData.h: Add FunctionIndex for PPdata
SecurityPkg/Tcg2PhysicalPresenceLib: Extend the submit preOS func
SecurityPkg: Move the Tcg2ConfigNvData.h to Include folder
SecurityPkg/TcgPhysicalPresenceLib: Replace the ASSERT with error
code
SecurityPkg/TcgPhysicalPresenceLib: Fix the operation of 11

.../DxeTcg2PhysicalPresenceLib.c | 63 +++++++---
.../DxeTcg2PhysicalPresenceLib.inf | 6 +-
.../Include/Guid/Tcg2PhysicalPresenceData.h | 3 +-
.../Include/Library/Tcg2PhysicalPresenceLib.h | 4 +-
SecurityPkg/Include/Library/Tcg2PpVendorLib.h | 54 ++++++++-
.../Tcg2Config => Include}/Tcg2ConfigNvData.h | 2 +-
.../DxeTcg2PhysicalPresenceLib.c | 68 ++++++++---
.../DxeTcg2PhysicalPresenceLib.inf | 4 +-
.../DxeTcgPhysicalPresenceLib.c | 110 ++++++++++++------
.../DxeTcgPhysicalPresenceLib.inf | 6 +-
.../SmmTcg2PhysicalPresenceLib.c | 15 ++-
.../Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.c | 61 +++++++++-
SecurityPkg/SecurityPkg.dec | 7 +-
SecurityPkg/SecurityPkg.uni | 7 +-
SecurityPkg/Tcg/Tcg2Config/Tcg2Config.vfr | 4 +-
SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf | 3 +-
SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigImpl.h | 4 +-
SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf | 3 +-
SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c | 4 +-
SecurityPkg/Tcg/Tcg2Config/TpmDetection.c | 4 +-
SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c | 10 +-
21 files changed, 347 insertions(+), 95 deletions(-) rename
SecurityPkg/{Tcg/Tcg2Config => Include}/Tcg2ConfigNvData.h (94%)

--
2.21.0.windows.1

Join devel@edk2.groups.io to automatically receive all group messages.