Re: [edk2] [PATCH 2/7] ovmf: link with Tcg2ConfigPei module

2018-03-02 Thread Laszlo Ersek 
On 03/01/18 15:59, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Feb 23, 2018 at 6:31 PM, Laszlo Ersek  wrote:

>> (6) Now, I realize Tcg2Pei *apparently* depends on
>> gEfiPeiReadOnlyVariable2PpiGuid (i.e., read-only variable access in the
>> PEI phase) as well. That's a bug in the INF file (the [depex] section).
>> If you grep the Tcg2Pei module source for the GUID, the [depex] section
>> is the only hit. Can you please submit a separate patch that removes it
>> from the depex?
> 
> I don't get how you came to that conclusion, both
> SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c and
> SecurityPkg/Tcg/Tcg2Config/TpmDetection.c match. Apparently, the
> variable is used in s3 mode, in DetectTpmDevice().

In my point (6) above, I was  talking about Tcg2Pei, not Tcg2ConfigPei.

If you grep "SecurityPkg/Tcg/Tcg2Pei" for
"gEfiPeiReadOnlyVariable2PpiGuid", the only hit is the depex section in
the INF file.

I think that's a bug in "SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf", and the
PPI GUID should removed from there, as a separate patch.

>>> ---
>>>  OvmfPkg/OvmfPkgX64.dsc | 20 
>>>  OvmfPkg/OvmfPkgX64.fdf |  3 +++
>>>  2 files changed, 23 insertions(+)
>>
>> Is there any particular reason to exclude the Ia32 and Ia32X64 builds?
>>
>> If not, then please modify all three sets of dsc/fdf files identically.
> 
> I'd rather keep this as a TODO item for now, since we are not close to
> a final version, and it's annoying to have to fix each files etc..

OK.

Thanks!
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 2/7] ovmf: link with Tcg2ConfigPei module

2018-03-01 Thread Marc-André Lureau 
Hi

On Fri, Feb 23, 2018 at 6:31 PM, Laszlo Ersek  wrote:
> On 02/23/18 14:23, marcandre.lur...@redhat.com wrote:
>> From: Marc-André Lureau 
>>
>> This module initializes TPM device type based on variable and
>> detection.
>
> (1) I suggest we say the following here:
>
> "The Tcg2ConfigPei module informs the firmware globally about the TPM
> device type, by setting the PcdTpmInstanceGuid PCD to the appropriate
> GUID value. The original module under SecurityPkg can perform device
> detection, or read a cached value from a non-volatile UEFI variable.
> OvmfPkg's clone of the module always performs the hardware detection."
>

ok

> Becase...
>
>> The module requires VariablePei, which is built with
>> MEM_VARSTORE_EMU_ENABLE=FALSE.
>
> (2) ... as I hinted in my response to your blurb, and also as suggested
> by "Tcg2ConfigPei.inf", we should clone Tcg2ConfigPei for OVMF, and
> *trim it* quite a bit.
>
> - The new location should be "OvmfPkg/Tcg/Tcg2Config/".
>
> - We need not copy the ".uni" file (also drop MODULE_UNI_FILE from the
> INF file)
>
> - Re-generate FILE_GUID in the INF file with "uuidgen"
>
> - Remove all PEI-phase variable access; always perform the hw detection.
>
> - I would even suggest removing support for TPM1.2. Just check whether
> TPM2 is available or not.
>

ok

>
> (3) Ultimately, this is what the module should do:
>
> - Check the QEMU hardware for TPM2 availability only
>
> - If found, set the dynamic PCD "PcdTpmInstanceGuid" to
>   &gEfiTpmDeviceInstanceTpm20DtpmGuid. This is what informs the rest of
>   the firmware about the TPM type.
>
> - Install the gEfiTpmDeviceSelectedGuid PPI. This action permits the
>   PEI_CORE to dispatch the Tcg2Pei module, which consumes the above PCD.
>   In effect, the gEfiTpmDeviceSelectedGuid PPI serializes the setting
>   and the consumption of the "TPM type" PCD.
>
> - If no TPM2 was found, install gPeiTpmInitializationDonePpiGuid.
>   (Normally this is performed by Tcg2Pei, but Tcg2Pei doesn't do it if
>   no TPM2 is available. So in that case our Tcg2ConfigPei must do it.)

ok

>
> (4) Regarding the TPM detection itself. It looks like DetectTpmDevice()
> [SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c] calls a number of TPM1.2
> functions. If the earliest one fails, it assumes "no TPM" at all, but if
> only a later call fails, it deduces, from the 1.2 failure, that TPM2 exists.
>
> I think we can do better than this, in our Tcg2ConfigPei clone:
>
> - We should call Tpm2RequestUseTpm() directly, from
> "SecurityPkg/Include/Library/Tpm2DeviceLib.h".
>
> - And, Tpm2Startup(), from
> "SecurityPkg/Include/Library/Tpm2CommandLib.h", will be called by Tcg2Pei.

ok

>
> (5) Finally, there's no need to set "PcdTpmInitializationPolicy" to
> anything. I don't see it consumed by any module that we should include
> in OVMF. (More on this below.)
>

ok

>
> (6) Now, I realize Tcg2Pei *apparently* depends on
> gEfiPeiReadOnlyVariable2PpiGuid (i.e., read-only variable access in the
> PEI phase) as well. That's a bug in the INF file (the [depex] section).
> If you grep the Tcg2Pei module source for the GUID, the [depex] section
> is the only hit. Can you please submit a separate patch that removes it
> from the depex?

I don't get how you came to that conclusion, both
SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c and
SecurityPkg/Tcg/Tcg2Config/TpmDetection.c match. Apparently, the
variable is used in s3 mode, in DetectTpmDevice().

I'll drop it from OvmfPkg version for now.

>
>
>> CC: Laszlo Ersek 
>> CC: Stefan Berger 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Marc-André Lureau 
>> ---
>>  OvmfPkg/OvmfPkgX64.dsc | 20 
>>  OvmfPkg/OvmfPkgX64.fdf |  3 +++
>>  2 files changed, 23 insertions(+)
>
> Is there any particular reason to exclude the Ia32 and Ia32X64 builds?
>
> If not, then please modify all three sets of dsc/fdf files identically.

I'd rather keep this as a TODO item for now, since we are not close to
a final version, and it's annoying to have to fix each files etc..

>
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index 32c57b04e1..b5cbe8430f 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -40,6 +40,7 @@
>
> (7) Please implement the following git settings in your edk2 clone:
>
> https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-05
>
> (in particular "xfuncname")
>
> and
>
> https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-09
>
> This will help reviewers see what section of the DSC / FDF / INI / DEC
> files are modified by a patch hunk.
>

done

>>DEFINE SMM_REQUIRE = FALSE
>>DEFINE TLS_ENABLE  = FALSE
>>DEFINE MEM_VARSTORE_EMU_ENABLE = TRUE
>> +  DEFINE TPM2_ENABLE = FALSE
>>
>>#
>># Flash size selection. Setting FD_SIZE_IN_KB on the command lin

Re: [edk2] [PATCH 2/7] ovmf: link with Tcg2ConfigPei module

2018-02-23 Thread Laszlo Ersek 
On 02/23/18 14:23, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau 
> 
> This module initializes TPM device type based on variable and
> detection.

(1) I suggest we say the following here:

"The Tcg2ConfigPei module informs the firmware globally about the TPM
device type, by setting the PcdTpmInstanceGuid PCD to the appropriate
GUID value. The original module under SecurityPkg can perform device
detection, or read a cached value from a non-volatile UEFI variable.
OvmfPkg's clone of the module always performs the hardware detection."

Becase...

> The module requires VariablePei, which is built with
> MEM_VARSTORE_EMU_ENABLE=FALSE.

(2) ... as I hinted in my response to your blurb, and also as suggested
by "Tcg2ConfigPei.inf", we should clone Tcg2ConfigPei for OVMF, and
*trim it* quite a bit.

- The new location should be "OvmfPkg/Tcg/Tcg2Config/".

- We need not copy the ".uni" file (also drop MODULE_UNI_FILE from the
INF file)

- Re-generate FILE_GUID in the INF file with "uuidgen"

- Remove all PEI-phase variable access; always perform the hw detection.

- I would even suggest removing support for TPM1.2. Just check whether
TPM2 is available or not.


(3) Ultimately, this is what the module should do:

- Check the QEMU hardware for TPM2 availability only

- If found, set the dynamic PCD "PcdTpmInstanceGuid" to
  &gEfiTpmDeviceInstanceTpm20DtpmGuid. This is what informs the rest of
  the firmware about the TPM type.

- Install the gEfiTpmDeviceSelectedGuid PPI. This action permits the
  PEI_CORE to dispatch the Tcg2Pei module, which consumes the above PCD.
  In effect, the gEfiTpmDeviceSelectedGuid PPI serializes the setting
  and the consumption of the "TPM type" PCD.

- If no TPM2 was found, install gPeiTpmInitializationDonePpiGuid.
  (Normally this is performed by Tcg2Pei, but Tcg2Pei doesn't do it if
  no TPM2 is available. So in that case our Tcg2ConfigPei must do it.)


(4) Regarding the TPM detection itself. It looks like DetectTpmDevice()
[SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c] calls a number of TPM1.2
functions. If the earliest one fails, it assumes "no TPM" at all, but if
only a later call fails, it deduces, from the 1.2 failure, that TPM2 exists.

I think we can do better than this, in our Tcg2ConfigPei clone:

- We should call Tpm2RequestUseTpm() directly, from
"SecurityPkg/Include/Library/Tpm2DeviceLib.h".

- And, Tpm2Startup(), from
"SecurityPkg/Include/Library/Tpm2CommandLib.h", will be called by Tcg2Pei.


(5) Finally, there's no need to set "PcdTpmInitializationPolicy" to
anything. I don't see it consumed by any module that we should include
in OVMF. (More on this below.)


(6) Now, I realize Tcg2Pei *apparently* depends on
gEfiPeiReadOnlyVariable2PpiGuid (i.e., read-only variable access in the
PEI phase) as well. That's a bug in the INF file (the [depex] section).
If you grep the Tcg2Pei module source for the GUID, the [depex] section
is the only hit. Can you please submit a separate patch that removes it
from the depex?


> CC: Laszlo Ersek 
> CC: Stefan Berger 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Marc-André Lureau 
> ---
>  OvmfPkg/OvmfPkgX64.dsc | 20 
>  OvmfPkg/OvmfPkgX64.fdf |  3 +++
>  2 files changed, 23 insertions(+)

Is there any particular reason to exclude the Ia32 and Ia32X64 builds?

If not, then please modify all three sets of dsc/fdf files identically.

> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 32c57b04e1..b5cbe8430f 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -40,6 +40,7 @@

(7) Please implement the following git settings in your edk2 clone:

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-05

(in particular "xfuncname")

and

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-09

This will help reviewers see what section of the DSC / FDF / INI / DEC
files are modified by a patch hunk.

>DEFINE SMM_REQUIRE = FALSE
>DEFINE TLS_ENABLE  = FALSE
>DEFINE MEM_VARSTORE_EMU_ENABLE = TRUE
> +  DEFINE TPM2_ENABLE = FALSE
>  
>#
># Flash size selection. Setting FD_SIZE_IN_KB on the command line directly 
> to
> @@ -209,6 +210,11 @@
>
> OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
>XenHypercallLib|OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf
>  
> +!if $(TPM2_ENABLE) == TRUE
> +  Tpm12CommandLib|SecurityPkg/Library/Tpm12CommandLib/Tpm12CommandLib.inf
> +  Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
> +!endif
> +

(8) For the patch, as posted, resolving Tpm2CommandLib looks unneeded,
because "SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf" doesn't seem to
depend on that lib class.

However, for the OvmfPkg clone of Tcg2ConfigPei that I'm

[edk2] [PATCH 2/7] ovmf: link with Tcg2ConfigPei module

2018-02-23 Thread marcandre . lureau 
From: Marc-André Lureau 

This module initializes TPM device type based on variable and
detection.

The module requires VariablePei, which is built with
MEM_VARSTORE_EMU_ENABLE=FALSE.

CC: Laszlo Ersek 
CC: Stefan Berger 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Marc-André Lureau 
---
 OvmfPkg/OvmfPkgX64.dsc | 20 
 OvmfPkg/OvmfPkgX64.fdf |  3 +++
 2 files changed, 23 insertions(+)

diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 32c57b04e1..b5cbe8430f 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -40,6 +40,7 @@
   DEFINE SMM_REQUIRE = FALSE
   DEFINE TLS_ENABLE  = FALSE
   DEFINE MEM_VARSTORE_EMU_ENABLE = TRUE
+  DEFINE TPM2_ENABLE = FALSE
 
   #
   # Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
@@ -209,6 +210,11 @@
   
OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
   XenHypercallLib|OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf
 
+!if $(TPM2_ENABLE) == TRUE
+  Tpm12CommandLib|SecurityPkg/Library/Tpm12CommandLib/Tpm12CommandLib.inf
+  Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
+!endif
+
 [LibraryClasses.common]
   BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
 
@@ -272,6 +278,10 @@
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf
   PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
   QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiLib.inf
+!if $(TPM2_ENABLE)
+  Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibDTpm/Tpm12DeviceLibDTpm.inf
+  Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
+!endif
 
 [LibraryClasses.common.DXE_CORE]
   HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf
@@ -558,6 +568,12 @@
 
   gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
 
+!if $(TPM2_ENABLE) == TRUE
+  gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0xb6, 0xe5, 0x01, 0x8b, 
0x19, 0x4f, 0xe8, 0x46, 0xab, 0x93, 0x1c, 0x53, 0x67, 0x1b, 0x90, 0xcc}
+  gEfiSecurityPkgTokenSpaceGuid.PcdTpm2InitializationPolicy|1
+  gEfiSecurityPkgTokenSpaceGuid.PcdTpmInitializationPolicy|1
+!endif
+
 

 #
 # Components Section - list of all EDK II Modules needed by this Platform.
@@ -629,6 +645,10 @@
 
   MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
 
+!if $(TPM2_ENABLE) == TRUE
+  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
+!endif
+
 !if $(SECURE_BOOT_ENABLE) == TRUE
   MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
 
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index bb46a409d9..dc35d0a1f7 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -168,6 +168,9 @@ INF  UefiCpuPkg/CpuMpPei/CpuMpPei.inf
 INF  MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf
 INF  MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
 !endif
+!if $(TPM2_ENABLE) == TRUE
+INF  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
+!endif
 
 

 
-- 
2.16.1.73.g5832b7e9f2

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel