Import PeiDxeTpmPlatformHierarchyLib.c from edk2-platforms. Fix some bugs
from the original code and simplify parts of it.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 .../Include/Library/TpmPlatformHierarchyLib.h |  27 +++
 .../PeiDxeTpmPlatformHierarchyLib.c           | 200 ++++++++++++++++++
 .../PeiDxeTpmPlatformHierarchyLib.inf         |  40 ++++
 3 files changed, 267 insertions(+)
 create mode 100644 OvmfPkg/Include/Library/TpmPlatformHierarchyLib.h
 create mode 100644 OvmfPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.c
 create mode 100644 OvmfPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf

diff --git a/OvmfPkg/Include/Library/TpmPlatformHierarchyLib.h b/OvmfPkg/Include/Library/TpmPlatformHierarchyLib.h
new file mode 100644
index 0000000000..a872fa09dc
--- /dev/null
+++ b/OvmfPkg/Include/Library/TpmPlatformHierarchyLib.h
@@ -0,0 +1,27 @@
+/** @file
+    TPM Platform Hierarchy configuration library.
+
+    This library provides functions for customizing the TPM's Platform Hierarchy
+    Authorization Value (platformAuth) and Platform Hierarchy Authorization
+    Policy (platformPolicy) can be defined through this function.
+
+Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+Copyright (c) Microsoft Corporation.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef _TPM_PLATFORM_HIERARCHY_LIB_H_
+#define _TPM_PLATFORM_HIERARCHY_LIB_H_
+
+/**
+   This service will perform the TPM Platform Hierarchy configuration at the SmmReadyToLock event.
+
+**/
+VOID
+EFIAPI
+ConfigureTpmPlatformHierarchy (
+  VOID
+  );
+
+#endif
diff --git a/OvmfPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.c b/OvmfPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.c
new file mode 100644
index 0000000000..a0dc848abd
--- /dev/null
+++ b/OvmfPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.c
@@ -0,0 +1,200 @@
+/** @file
+    TPM Platform Hierarchy configuration library.
+
+    This library provides functions for customizing the TPM's Platform Hierarchy
+    Authorization Value (platformAuth) and Platform Hierarchy Authorization
+    Policy (platformPolicy) can be defined through this function.
+
+    Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+    Copyright (c) Microsoft Corporation.<BR>
+    SPDX-License-Identifier: BSD-2-Clause-Patent
+
+    @par Specification Reference:
+    https://trustedcomputinggroup.org/resource/tcg-tpm-v2-0-provisioning-guidance/
+**/
+
+#include <Uefi.h>
+
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/RngLib.h>
+#include <Library/Tpm2CommandLib.h>
+#include <Library/Tpm2DeviceLib.h>
+
+//
+// The authorization value may be no larger than the digest produced by the hash
+//   algorithm used for context integrity.
+//
+
+UINT16       mAuthSize;
+
+/**
+  Generate high-quality entropy source through RDRAND.
+
+  @param[in]   Length        Size of the buffer, in bytes, to fill with.
+  @param[out]  Entropy       Pointer to the buffer to store the entropy data.
+
+  @retval EFI_SUCCESS        Entropy generation succeeded.
+  @retval EFI_NOT_READY      Failed to request random data.
+
+**/
+EFI_STATUS
+EFIAPI
+RdRandGenerateEntropy (
+  IN UINTN         Length,
+  OUT UINT8        *Entropy
+  )
+{
+  EFI_STATUS  Status;
+  UINTN       BlockCount;
+  UINT64      Seed[2];
+  UINT8       *Ptr;
+
+  Status = EFI_NOT_READY;
+  BlockCount = Length / sizeof(Seed);
+  Ptr = (UINT8 *)Entropy;
+
+  //
+  // Generate high-quality seed for DRBG Entropy
+  //
+  while (BlockCount > 0) {
+    Status = GetRandomNumber128 (Seed);
+    if (EFI_ERROR (Status)) {
+      return Status;
+    }
+    CopyMem (Ptr, Seed, sizeof(Seed));
+
+    BlockCount--;
+    Ptr = Ptr + sizeof(Seed);
+  }
+
+  //
+  // Populate the remained data as request.
+  //
+  Status = GetRandomNumber128 (Seed);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+  CopyMem (Ptr, Seed, (Length % sizeof(Seed)));
+
+  return Status;
+}
+
+/**
+  This function returns the maximum size of TPM2B_AUTH; this structure is used for an authorization value
+  and limits an authValue to being no larger than the largest digest produced by a TPM.
+
+  @param[out] AuthSize                 Tpm2 Auth size
+
+  @retval EFI_SUCCESS                  Auth size returned.
+  @retval EFI_DEVICE_ERROR             Can not return platform auth due to device error.
+
+**/
+EFI_STATUS
+EFIAPI
+GetAuthSize (
+  OUT UINT16            *AuthSize
+  )
+{
+  EFI_STATUS            Status;
+  TPML_PCR_SELECTION    Pcrs;
+  UINTN                 Index;
+  UINT16                DigestSize;
+
+  Status = EFI_SUCCESS;
+
+  while (mAuthSize == 0) {
+
+    mAuthSize = SHA1_DIGEST_SIZE;
+    ZeroMem (&Pcrs, sizeof (TPML_PCR_SELECTION));
+    Status = Tpm2GetCapabilityPcrs (&Pcrs);
+
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "Tpm2GetCapabilityPcrs fail!\n"));
+      break;
+    }
+
+    DEBUG ((DEBUG_ERROR, "Tpm2GetCapabilityPcrs - %08x\n", Pcrs.count));
+
+    for (Index = 0; Index < Pcrs.count; Index++) {
+      DEBUG ((DEBUG_ERROR, "alg - %x\n", Pcrs.pcrSelections[Index].hash));
+
+      switch (Pcrs.pcrSelections[Index].hash) {
+      case TPM_ALG_SHA1:
+        DigestSize = SHA1_DIGEST_SIZE;
+        break;
+      case TPM_ALG_SHA256:
+        DigestSize = SHA256_DIGEST_SIZE;
+        break;
+      case TPM_ALG_SHA384:
+        DigestSize = SHA384_DIGEST_SIZE;
+        break;
+      case TPM_ALG_SHA512:
+        DigestSize = SHA512_DIGEST_SIZE;
+        break;
+      case TPM_ALG_SM3_256:
+        DigestSize = SM3_256_DIGEST_SIZE;
+        break;
+      default:
+        DigestSize = SHA1_DIGEST_SIZE;
+        break;
+      }
+
+      if (DigestSize > mAuthSize) {
+        mAuthSize = DigestSize;
+      }
+    }
+    break;
+  }
+
+  *AuthSize = mAuthSize;
+  return Status;
+}
+
+/**
+  Set PlatformAuth to random value.
+**/
+VOID
+RandomizePlatformAuth (
+  VOID
+  )
+{
+  EFI_STATUS                        Status;
+  UINT16                            AuthSize;
+  TPM2B_AUTH                        NewPlatformAuth;
+
+  //
+  // Send Tpm2HierarchyChange Auth with random value to avoid PlatformAuth being null
+  //
+
+  GetAuthSize (&AuthSize);
+
+  NewPlatformAuth.size = AuthSize;
+
+  //
+  // Create the random bytes in the destination buffer
+  //
+
+  RdRandGenerateEntropy (NewPlatformAuth.size, NewPlatformAuth.buffer);
+
+  //
+  // Send Tpm2HierarchyChangeAuth command with the new Auth value
+  //
+  Status = Tpm2HierarchyChangeAuth (TPM_RH_PLATFORM, NULL, &NewPlatformAuth);
+  DEBUG ((DEBUG_INFO, "Tpm2HierarchyChangeAuth Result: - %r\n", Status));
+  ZeroMem (NewPlatformAuth.buffer, AuthSize);
+}
+
+/**
+   This service defines the configuration of the Platform Hierarchy Authorization Value (platformAuth)
+   and Platform Hierarchy Authorization Policy (platformPolicy)
+
+**/
+VOID
+EFIAPI
+ConfigureTpmPlatformHierarchy (
+  )
+{
+  RandomizePlatformAuth ();
+}
diff --git a/OvmfPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf b/OvmfPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf
new file mode 100644
index 0000000000..a413e02302
--- /dev/null
+++ b/OvmfPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf
@@ -0,0 +1,40 @@
+### @file
+#
+#   TPM Platform Hierarchy configuration library.
+#
+#   This library provides functions for customizing the TPM's Platform Hierarchy
+#   Authorization Value (platformAuth) and Platform Hierarchy Authorization
+#   Policy (platformPolicy) can be defined through this function.
+#
+# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) Microsoft Corporation.<BR>
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+###
+
+[Defines]
+  INF_VERSION                    = 0x00010005
+  BASE_NAME                      = PeiDxeTpmPlatformHierarchyLib
+  FILE_GUID                      = 7794F92C-4E8E-4E57-9E4A-49A0764C7D73
+  MODULE_TYPE                    = PEIM
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = TpmPlatformHierarchyLib|PEIM DXE_DRIVER
+
+[LibraryClasses]
+  BaseLib
+  BaseMemoryLib
+  DebugLib
+  MemoryAllocationLib
+  RngLib
+  Tpm2CommandLib
+  Tpm2DeviceLib
+
+[Packages]
+  MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  SecurityPkg/SecurityPkg.dec
+  CryptoPkg/CryptoPkg.dec
+
+[Sources]
+  PeiDxeTpmPlatformHierarchyLib.c
-- 
2.31.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#79194): https://edk2.groups.io/g/devel/message/79194
Mute This Topic: https://groups.io/mt/84845083/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

This seems like a bad place for a general purpose lib that many other 
platforms may take a dependency on.

In v1 this was SecurityPkg.  OvmfPkg is a platform package and therefore 
not a good place to define broad interfaces.

What caused this to move here?

Thanks
Sean






On 8/12/2021 9:59 AM, Stefan Berger wrote:
> Import PeiDxeTpmPlatformHierarchyLib.c from edk2-platforms. Fix some bugs
> from the original code and simplify parts of it.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>   .../Include/Library/TpmPlatformHierarchyLib.h |  27 +++
>   .../PeiDxeTpmPlatformHierarchyLib.c           | 200 ++++++++++++++++++
>   .../PeiDxeTpmPlatformHierarchyLib.inf         |  40 ++++
>   3 files changed, 267 insertions(+)
>   create mode 100644 OvmfPkg/Include/Library/TpmPlatformHierarchyLib.h
>   create mode 100644 OvmfPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.c
>   create mode 100644 OvmfPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf
> 
> diff --git a/OvmfPkg/Include/Library/TpmPlatformHierarchyLib.h b/OvmfPkg/Include/Library/TpmPlatformHierarchyLib.h
> new file mode 100644
> index 0000000000..a872fa09dc
> --- /dev/null
> +++ b/OvmfPkg/Include/Library/TpmPlatformHierarchyLib.h
> @@ -0,0 +1,27 @@
> +/** @file
> +    TPM Platform Hierarchy configuration library.
> +
> +    This library provides functions for customizing the TPM's Platform Hierarchy
> +    Authorization Value (platformAuth) and Platform Hierarchy Authorization
> +    Policy (platformPolicy) can be defined through this function.
> +
> +Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) Microsoft Corporation.<BR>
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef _TPM_PLATFORM_HIERARCHY_LIB_H_
> +#define _TPM_PLATFORM_HIERARCHY_LIB_H_
> +
> +/**
> +   This service will perform the TPM Platform Hierarchy configuration at the SmmReadyToLock event.
> +
> +**/
> +VOID
> +EFIAPI
> +ConfigureTpmPlatformHierarchy (
> +  VOID
> +  );
> +
> +#endif
> diff --git a/OvmfPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.c b/OvmfPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.c
> new file mode 100644
> index 0000000000..a0dc848abd
> --- /dev/null
> +++ b/OvmfPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.c
> @@ -0,0 +1,200 @@
> +/** @file
> +    TPM Platform Hierarchy configuration library.
> +
> +    This library provides functions for customizing the TPM's Platform Hierarchy
> +    Authorization Value (platformAuth) and Platform Hierarchy Authorization
> +    Policy (platformPolicy) can be defined through this function.
> +
> +    Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> +    Copyright (c) Microsoft Corporation.<BR>
> +    SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +    @par Specification Reference:
> +    https://trustedcomputinggroup.org/resource/tcg-tpm-v2-0-provisioning-guidance/
> +**/
> +
> +#include <Uefi.h>
> +
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/RngLib.h>
> +#include <Library/Tpm2CommandLib.h>
> +#include <Library/Tpm2DeviceLib.h>
> +
> +//
> +// The authorization value may be no larger than the digest produced by the hash
> +//   algorithm used for context integrity.
> +//
> +
> +UINT16       mAuthSize;
> +
> +/**
> +  Generate high-quality entropy source through RDRAND.
> +
> +  @param[in]   Length        Size of the buffer, in bytes, to fill with.
> +  @param[out]  Entropy       Pointer to the buffer to store the entropy data.
> +
> +  @retval EFI_SUCCESS        Entropy generation succeeded.
> +  @retval EFI_NOT_READY      Failed to request random data.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +RdRandGenerateEntropy (
> +  IN UINTN         Length,
> +  OUT UINT8        *Entropy
> +  )
> +{
> +  EFI_STATUS  Status;
> +  UINTN       BlockCount;
> +  UINT64      Seed[2];
> +  UINT8       *Ptr;
> +
> +  Status = EFI_NOT_READY;
> +  BlockCount = Length / sizeof(Seed);
> +  Ptr = (UINT8 *)Entropy;
> +
> +  //
> +  // Generate high-quality seed for DRBG Entropy
> +  //
> +  while (BlockCount > 0) {
> +    Status = GetRandomNumber128 (Seed);
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +    CopyMem (Ptr, Seed, sizeof(Seed));
> +
> +    BlockCount--;
> +    Ptr = Ptr + sizeof(Seed);
> +  }
> +
> +  //
> +  // Populate the remained data as request.
> +  //
> +  Status = GetRandomNumber128 (Seed);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +  CopyMem (Ptr, Seed, (Length % sizeof(Seed)));
> +
> +  return Status;
> +}
> +
> +/**
> +  This function returns the maximum size of TPM2B_AUTH; this structure is used for an authorization value
> +  and limits an authValue to being no larger than the largest digest produced by a TPM.
> +
> +  @param[out] AuthSize                 Tpm2 Auth size
> +
> +  @retval EFI_SUCCESS                  Auth size returned.
> +  @retval EFI_DEVICE_ERROR             Can not return platform auth due to device error.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +GetAuthSize (
> +  OUT UINT16            *AuthSize
> +  )
> +{
> +  EFI_STATUS            Status;
> +  TPML_PCR_SELECTION    Pcrs;
> +  UINTN                 Index;
> +  UINT16                DigestSize;
> +
> +  Status = EFI_SUCCESS;
> +
> +  while (mAuthSize == 0) {
> +
> +    mAuthSize = SHA1_DIGEST_SIZE;
> +    ZeroMem (&Pcrs, sizeof (TPML_PCR_SELECTION));
> +    Status = Tpm2GetCapabilityPcrs (&Pcrs);
> +
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "Tpm2GetCapabilityPcrs fail!\n"));
> +      break;
> +    }
> +
> +    DEBUG ((DEBUG_ERROR, "Tpm2GetCapabilityPcrs - %08x\n", Pcrs.count));
> +
> +    for (Index = 0; Index < Pcrs.count; Index++) {
> +      DEBUG ((DEBUG_ERROR, "alg - %x\n", Pcrs.pcrSelections[Index].hash));
> +
> +      switch (Pcrs.pcrSelections[Index].hash) {
> +      case TPM_ALG_SHA1:
> +        DigestSize = SHA1_DIGEST_SIZE;
> +        break;
> +      case TPM_ALG_SHA256:
> +        DigestSize = SHA256_DIGEST_SIZE;
> +        break;
> +      case TPM_ALG_SHA384:
> +        DigestSize = SHA384_DIGEST_SIZE;
> +        break;
> +      case TPM_ALG_SHA512:
> +        DigestSize = SHA512_DIGEST_SIZE;
> +        break;
> +      case TPM_ALG_SM3_256:
> +        DigestSize = SM3_256_DIGEST_SIZE;
> +        break;
> +      default:
> +        DigestSize = SHA1_DIGEST_SIZE;
> +        break;
> +      }
> +
> +      if (DigestSize > mAuthSize) {
> +        mAuthSize = DigestSize;
> +      }
> +    }
> +    break;
> +  }
> +
> +  *AuthSize = mAuthSize;
> +  return Status;
> +}
> +
> +/**
> +  Set PlatformAuth to random value.
> +**/
> +VOID
> +RandomizePlatformAuth (
> +  VOID
> +  )
> +{
> +  EFI_STATUS                        Status;
> +  UINT16                            AuthSize;
> +  TPM2B_AUTH                        NewPlatformAuth;
> +
> +  //
> +  // Send Tpm2HierarchyChange Auth with random value to avoid PlatformAuth being null
> +  //
> +
> +  GetAuthSize (&AuthSize);
> +
> +  NewPlatformAuth.size = AuthSize;
> +
> +  //
> +  // Create the random bytes in the destination buffer
> +  //
> +
> +  RdRandGenerateEntropy (NewPlatformAuth.size, NewPlatformAuth.buffer);
> +
> +  //
> +  // Send Tpm2HierarchyChangeAuth command with the new Auth value
> +  //
> +  Status = Tpm2HierarchyChangeAuth (TPM_RH_PLATFORM, NULL, &NewPlatformAuth);
> +  DEBUG ((DEBUG_INFO, "Tpm2HierarchyChangeAuth Result: - %r\n", Status));
> +  ZeroMem (NewPlatformAuth.buffer, AuthSize);
> +}
> +
> +/**
> +   This service defines the configuration of the Platform Hierarchy Authorization Value (platformAuth)
> +   and Platform Hierarchy Authorization Policy (platformPolicy)
> +
> +**/
> +VOID
> +EFIAPI
> +ConfigureTpmPlatformHierarchy (
> +  )
> +{
> +  RandomizePlatformAuth ();
> +}
> diff --git a/OvmfPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf b/OvmfPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf
> new file mode 100644
> index 0000000000..a413e02302
> --- /dev/null
> +++ b/OvmfPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf
> @@ -0,0 +1,40 @@
> +### @file
> +#
> +#   TPM Platform Hierarchy configuration library.
> +#
> +#   This library provides functions for customizing the TPM's Platform Hierarchy
> +#   Authorization Value (platformAuth) and Platform Hierarchy Authorization
> +#   Policy (platformPolicy) can be defined through this function.
> +#
> +# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) Microsoft Corporation.<BR>
> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +###
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = PeiDxeTpmPlatformHierarchyLib
> +  FILE_GUID                      = 7794F92C-4E8E-4E57-9E4A-49A0764C7D73
> +  MODULE_TYPE                    = PEIM
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = TpmPlatformHierarchyLib|PEIM DXE_DRIVER
> +
> +[LibraryClasses]
> +  BaseLib
> +  BaseMemoryLib
> +  DebugLib
> +  MemoryAllocationLib
> +  RngLib
> +  Tpm2CommandLib
> +  Tpm2DeviceLib
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  SecurityPkg/SecurityPkg.dec
> +  CryptoPkg/CryptoPkg.dec
> +
> +[Sources]
> +  PeiDxeTpmPlatformHierarchyLib.c
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#79208): https://edk2.groups.io/g/devel/message/79208
Mute This Topic: https://groups.io/mt/84845083/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 8/12/21 4:59 PM, Sean Brogan wrote:
> This seems like a bad place for a general purpose lib that many other 
> platforms may take a dependency on.
>
> In v1 this was SecurityPkg.  OvmfPkg is a platform package and 
> therefore not a good place to define broad interfaces.
>
> What caused this to move here?


Option 2 from this message: 
https://listman.redhat.com/archives/edk2-devel-archive/2021-August/msg00398.html

   Stefan


>
> Thanks
> Sean
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#79209): https://edk2.groups.io/g/devel/message/79209
Mute This Topic: https://groups.io/mt/84845083/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Thanks for the link as i missed that message.

To me this just points out more problems with how OVMF is being managed 
in the edk2 project and the uselessness of edk2 platforms as anything 
more than just a dumping ground repo to hold sample code.  But that is a 
problem larger than this patchset.

I guess if you are going doing option 2 can we rename the library 
interface you are defining in OvmfPkg so it doesn't conflict with the 
existing one in edk2-platforms/minplatform.  That would mean change:

* name in OvmfPkg.dec file
* header file in OvmfPkg/Include/Library
* all references in DSC file for mapping an instance
* all references in your INFs for dependency

Thanks
Sean






On 8/12/2021 3:19 PM, Stefan Berger wrote:
> 
> On 8/12/21 4:59 PM, Sean Brogan wrote:
>> This seems like a bad place for a general purpose lib that many other 
>> platforms may take a dependency on.
>>
>> In v1 this was SecurityPkg.  OvmfPkg is a platform package and 
>> therefore not a good place to define broad interfaces.
>>
>> What caused this to move here?
> 
> 
> Option 2 from this message: 
> https://listman.redhat.com/archives/edk2-devel-archive/2021-August/msg00398.html 
> 
> 
>    Stefan
> 
> 
>>
>> Thanks
>> Sean
>>
>>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#79291): https://edk2.groups.io/g/devel/message/79291
Mute This Topic: https://groups.io/mt/84845083/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 8/13/21 2:47 PM, Sean Brogan wrote:
> Thanks for the link as i missed that message.
>
> To me this just points out more problems with how OVMF is being 
> managed in the edk2 project and the uselessness of edk2 platforms as 
> anything more than just a dumping ground repo to hold sample code.  
> But that is a problem larger than this patchset.
>
> I guess if you are going doing option 2 can we rename the library 
> interface you are defining in OvmfPkg so it doesn't conflict with the 
> existing one in edk2-platforms/minplatform.  That would mean change:


I have now created v5 here with the latest code appearing in SecurityPkg 
again: 
https://github.com/stefanberger/edk2/commits/stefanberger/ovmf_disable_platform_hierarchy.v5

I can probably post that pretty quickly but I'll be out for a while. If 
it's urgent, someone else can pick it it up from there. I tested it on 
QEMU for x86 and aarch64 and test-compiled on various platforms that I 
touched (some didn't compile for me before the changes).

What I wasn't sure about is whether edk2-platforms is a 'holding area' 
for code to be imported ideally 1:1 into edk2. So I ended up making 
those changes already in v1 to cut out a dependency. If what I have in 
v5 (or also v4) is sufficient for general consumption, then let's put it 
into SecurityPkg.


    Stefan


>
> * name in OvmfPkg.dec file
> * header file in OvmfPkg/Include/Library
> * all references in DSC file for mapping an instance
> * all references in your INFs for dependency
>
> Thanks
> Sean
>
>
>
>
>
>
> On 8/12/2021 3:19 PM, Stefan Berger wrote:
>>
>> On 8/12/21 4:59 PM, Sean Brogan wrote:
>>> This seems like a bad place for a general purpose lib that many 
>>> other platforms may take a dependency on.
>>>
>>> In v1 this was SecurityPkg.  OvmfPkg is a platform package and 
>>> therefore not a good place to define broad interfaces.
>>>
>>> What caused this to move here?
>>
>>
>> Option 2 from this message: 
>> https://listman.redhat.com/archives/edk2-devel-archive/2021-August/msg00398.html 
>>
>>
>>    Stefan
>>
>>
>>>
>>> Thanks
>>> Sean
>>>
>>>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#79292): https://edk2.groups.io/g/devel/message/79292
Mute This Topic: https://groups.io/mt/84845083/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Good day Stefan,

Do you think you could split the first patch into a 1:1 initial import 
and have a modifications commit separately?

One of my big issues with EDK II is duplicated code. I look at it, I 
don't understand why it is duplicated. I look at the differences, I 
don't understand why they are there. I dig deeper into the matter (e.g. 
git blame), and I realise there is no reason but someone was taking a 
copy+paste route, and future contributors did not know or care about the 
respective other piece of code. This series standalone is basically that 
out-of-the-box, with no future changes needed. I'll suggest once more to 
enforce a code duplication ban, both within edk2, and between edk2 and 
edk2-platforms.

It would of course be great if you submitted a follow-up series to drop 
the old library from edk2-platforms. If you plan not to, and all 
maintainers agree to merge this without such a series being submitted, 
please CC me so I know when this is merged and can propose such a patch 
set myself.

Best regards,
Marvin

On 13/08/2021 21:02, Stefan Berger wrote:
>
> On 8/13/21 2:47 PM, Sean Brogan wrote:
>> Thanks for the link as i missed that message.
>>
>> To me this just points out more problems with how OVMF is being 
>> managed in the edk2 project and the uselessness of edk2 platforms as 
>> anything more than just a dumping ground repo to hold sample code.  
>> But that is a problem larger than this patchset.
>>
>> I guess if you are going doing option 2 can we rename the library 
>> interface you are defining in OvmfPkg so it doesn't conflict with the 
>> existing one in edk2-platforms/minplatform. That would mean change:
>
>
> I have now created v5 here with the latest code appearing in 
> SecurityPkg again: 
> https://github.com/stefanberger/edk2/commits/stefanberger/ovmf_disable_platform_hierarchy.v5
>
> I can probably post that pretty quickly but I'll be out for a while. 
> If it's urgent, someone else can pick it it up from there. I tested it 
> on QEMU for x86 and aarch64 and test-compiled on various platforms 
> that I touched (some didn't compile for me before the changes).
>
> What I wasn't sure about is whether edk2-platforms is a 'holding area' 
> for code to be imported ideally 1:1 into edk2. So I ended up making 
> those changes already in v1 to cut out a dependency. If what I have in 
> v5 (or also v4) is sufficient for general consumption, then let's put 
> it into SecurityPkg.
>
>
>    Stefan
>
>
>>
>> * name in OvmfPkg.dec file
>> * header file in OvmfPkg/Include/Library
>> * all references in DSC file for mapping an instance
>> * all references in your INFs for dependency
>>
>> Thanks
>> Sean
>>
>>
>>
>>
>>
>>
>> On 8/12/2021 3:19 PM, Stefan Berger wrote:
>>>
>>> On 8/12/21 4:59 PM, Sean Brogan wrote:
>>>> This seems like a bad place for a general purpose lib that many 
>>>> other platforms may take a dependency on.
>>>>
>>>> In v1 this was SecurityPkg.  OvmfPkg is a platform package and 
>>>> therefore not a good place to define broad interfaces.
>>>>
>>>> What caused this to move here?
>>>
>>>
>>> Option 2 from this message: 
>>> https://listman.redhat.com/archives/edk2-devel-archive/2021-August/msg00398.html 
>>>
>>>
>>>    Stefan
>>>
>>>
>>>>
>>>> Thanks
>>>> Sean
>>>>
>>>>
>
>
> 
>
>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#79309): https://edk2.groups.io/g/devel/message/79309
Mute This Topic: https://groups.io/mt/84845083/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-