OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

[edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Posted by Rebecca Cran 2 years, 10 months ago

TPM support hasn't been tested and any lines in the .dsc and .fdf files
that appear to show support are bogus. Remove them.

This fixes https://bugzilla.tianocore.org/show_bug.cgi?id=3354 .

Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
---
 OvmfPkg/Bhyve/BhyveX64.dsc | 64 --------------------
 OvmfPkg/Bhyve/BhyveX64.fdf | 15 -----
 2 files changed, 79 deletions(-)

diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
index d8792812ab..cbf896e89b 100644
--- a/OvmfPkg/Bhyve/BhyveX64.dsc
+++ b/OvmfPkg/Bhyve/BhyveX64.dsc
@@ -31,8 +31,6 @@
   DEFINE SECURE_BOOT_ENABLE      = FALSE
   DEFINE SMM_REQUIRE             = FALSE
   DEFINE SOURCE_DEBUG_ENABLE     = FALSE
-  DEFINE TPM_ENABLE              = FALSE
-  DEFINE TPM_CONFIG_ENABLE       = FALSE
 
   #
   # Network definition
@@ -221,16 +219,8 @@
   OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
   XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf
 
-
-!if $(TPM_ENABLE) == TRUE
-  Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
-  Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
-  Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
-  TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
-!else
   Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
   TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
-!endif
 
 [LibraryClasses.common]
   BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
@@ -292,11 +282,6 @@
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
   PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
 
-!if $(TPM_ENABLE) == TRUE
-  BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
-  Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
-!endif
-
   MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
 
 [LibraryClasses.common.DXE_CORE]
@@ -366,9 +351,6 @@
 !endif
   PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf
-!if $(TPM_ENABLE) == TRUE
-  Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
-!endif
 
 [LibraryClasses.common.UEFI_APPLICATION]
   PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
@@ -563,22 +545,12 @@
 
   gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
 
-!if $(TPM_ENABLE) == TRUE
-  gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
-!endif
-
   # MdeModulePkg resolution sets up the system display resolution
   gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution|0
   gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution|0
   gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|0
   gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|0
 
-[PcdsDynamicHii]
-!if $(TPM_ENABLE) == TRUE && $(TPM_CONFIG_ENABLE) == TRUE
-  gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
-  gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
-!endif
-
 ################################################################################
 #
 # Components Section - list of all EDK II Modules needed by this Platform.
@@ -618,19 +590,6 @@
     <LibraryClasses>
   }
 
-!if $(TPM_ENABLE) == TRUE
-  OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
-  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
-    <LibraryClasses>
-      HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
-      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
-      NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
-      NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
-      NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
-      NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
-  }
-!endif
-
   #
   # DXE Phase modules
   #
@@ -653,9 +612,6 @@
     <LibraryClasses>
 !if $(SECURE_BOOT_ENABLE) == TRUE
       NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
-!endif
-!if $(TPM_ENABLE) == TRUE
-      NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
 !endif
   }
 
@@ -841,23 +797,3 @@
       NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
   }
 
-
-  #
-  # TPM support
-  #
-!if $(TPM_ENABLE) == TRUE
-  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
-    <LibraryClasses>
-      Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
-      NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
-      HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
-      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
-      NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
-      NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
-      NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
-      NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
-  }
-!if $(TPM_CONFIG_ENABLE) == TRUE
-  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
-!endif
-!endif
diff --git a/OvmfPkg/Bhyve/BhyveX64.fdf b/OvmfPkg/Bhyve/BhyveX64.fdf
index 3eff36dac1..fbd63a395a 100644
--- a/OvmfPkg/Bhyve/BhyveX64.fdf
+++ b/OvmfPkg/Bhyve/BhyveX64.fdf
@@ -158,11 +158,6 @@ INF  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
 INF  OvmfPkg/Bhyve/SmmAccess/SmmAccessPei.inf
 !endif
 
-!if $(TPM_ENABLE) == TRUE
-INF  OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
-INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
-!endif
-
 ################################################################################
 
 [FV.DXEFV]
@@ -333,16 +328,6 @@ INF  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
 INF  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
 !endif
 
-#
-# TPM support
-#
-!if $(TPM_ENABLE) == TRUE
-INF  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
-!if $(TPM_CONFIG_ENABLE) == TRUE
-INF  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
-!endif
-!endif
-
 ################################################################################
 
 [FV.FVMAIN_COMPACT]
-- 
2.32.0




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

Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Posted by Peter Grehan 2 years, 10 months ago

On 6/13/21 6:43 AM, Rebecca Cran wrote:
> TPM support hasn't been tested and any lines in the .dsc and .fdf files
> that appear to show support are bogus. Remove them.
> 
> This fixes https://bugzilla.tianocore.org/show_bug.cgi?id=3354 .
> 
> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> ---
>   OvmfPkg/Bhyve/BhyveX64.dsc | 64 --------------------
>   OvmfPkg/Bhyve/BhyveX64.fdf | 15 -----
>   2 files changed, 79 deletions(-)
> 
> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
> index d8792812ab..cbf896e89b 100644
> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
> @@ -31,8 +31,6 @@
>     DEFINE SECURE_BOOT_ENABLE      = FALSE
>     DEFINE SMM_REQUIRE             = FALSE
>     DEFINE SOURCE_DEBUG_ENABLE     = FALSE
> -  DEFINE TPM_ENABLE              = FALSE
> -  DEFINE TPM_CONFIG_ENABLE       = FALSE
>   
>     #
>     # Network definition
> @@ -221,16 +219,8 @@
>     OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
>     XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf
>   
> -
> -!if $(TPM_ENABLE) == TRUE
> -  Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
> -  Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
> -  Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
> -  TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> -!else
>     Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
>     TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
> -!endif
>   
>   [LibraryClasses.common]
>     BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> @@ -292,11 +282,6 @@
>     CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
>     PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
>   
> -!if $(TPM_ENABLE) == TRUE
> -  BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> -  Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
> -!endif
> -
>     MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
>   
>   [LibraryClasses.common.DXE_CORE]
> @@ -366,9 +351,6 @@
>   !endif
>     PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
>     MpInitLib|UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf
> -!if $(TPM_ENABLE) == TRUE
> -  Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
> -!endif
>   
>   [LibraryClasses.common.UEFI_APPLICATION]
>     PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> @@ -563,22 +545,12 @@
>   
>     gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
>   
> -!if $(TPM_ENABLE) == TRUE
> -  gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
> -!endif
> -
>     # MdeModulePkg resolution sets up the system display resolution
>     gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution|0
>     gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution|0
>     gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|0
>     gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|0
>   
> -[PcdsDynamicHii]
> -!if $(TPM_ENABLE) == TRUE && $(TPM_CONFIG_ENABLE) == TRUE
> -  gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
> -  gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
> -!endif
> -
>   ################################################################################
>   #
>   # Components Section - list of all EDK II Modules needed by this Platform.
> @@ -618,19 +590,6 @@
>       <LibraryClasses>
>     }
>   
> -!if $(TPM_ENABLE) == TRUE
> -  OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> -  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> -    <LibraryClasses>
> -      HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> -  }
> -!endif
> -
>     #
>     # DXE Phase modules
>     #
> @@ -653,9 +612,6 @@
>       <LibraryClasses>
>   !if $(SECURE_BOOT_ENABLE) == TRUE
>         NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> -!endif
> -!if $(TPM_ENABLE) == TRUE
> -      NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
>   !endif
>     }
>   
> @@ -841,23 +797,3 @@
>         NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
>     }
>   
> -
> -  #
> -  # TPM support
> -  #
> -!if $(TPM_ENABLE) == TRUE
> -  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
> -    <LibraryClasses>
> -      Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
> -      NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
> -      HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> -  }
> -!if $(TPM_CONFIG_ENABLE) == TRUE
> -  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> -!endif
> -!endif
> diff --git a/OvmfPkg/Bhyve/BhyveX64.fdf b/OvmfPkg/Bhyve/BhyveX64.fdf
> index 3eff36dac1..fbd63a395a 100644
> --- a/OvmfPkg/Bhyve/BhyveX64.fdf
> +++ b/OvmfPkg/Bhyve/BhyveX64.fdf
> @@ -158,11 +158,6 @@ INF  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
>   INF  OvmfPkg/Bhyve/SmmAccess/SmmAccessPei.inf
>   !endif
>   
> -!if $(TPM_ENABLE) == TRUE
> -INF  OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> -INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> -!endif
> -
>   ################################################################################
>   
>   [FV.DXEFV]
> @@ -333,16 +328,6 @@ INF  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
>   INF  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
>   !endif
>   
> -#
> -# TPM support
> -#
> -!if $(TPM_ENABLE) == TRUE
> -INF  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
> -!if $(TPM_CONFIG_ENABLE) == TRUE
> -INF  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> -!endif
> -!endif
> -
>   ################################################################################
>   
>   [FV.FVMAIN_COMPACT]
> 

Reviewed-by: Peter Grehan <grehan@freebsd.org>



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

Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Posted by Ard Biesheuvel 2 years, 10 months ago

(+ Sean, Mike)

On Sat, 12 Jun 2021 at 22:43, Rebecca Cran <rebecca@bsdio.com> wrote:
>
> TPM support hasn't been tested and any lines in the .dsc and .fdf files
> that appear to show support are bogus. Remove them.
>
> This fixes https://bugzilla.tianocore.org/show_bug.cgi?id=3354 .
>
> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>

Strangely enough, this patch gets rejected by PatchCheck for lack of a
Signed-off-by line

https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=24198&view=results

The patch itself looks good to me

Acked-by: Ard Biesheuvel <ardb@kernel.org>

so if anyone else manages to fix the CI issue, feel free to push the
patch with my R-b (and Peter's, given in reply to this message)

(I will go offline for 3 weeks after Friday)

> ---
>  OvmfPkg/Bhyve/BhyveX64.dsc | 64 --------------------
>  OvmfPkg/Bhyve/BhyveX64.fdf | 15 -----
>  2 files changed, 79 deletions(-)
>
> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
> index d8792812ab..cbf896e89b 100644
> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
> @@ -31,8 +31,6 @@
>    DEFINE SECURE_BOOT_ENABLE      = FALSE
>    DEFINE SMM_REQUIRE             = FALSE
>    DEFINE SOURCE_DEBUG_ENABLE     = FALSE
> -  DEFINE TPM_ENABLE              = FALSE
> -  DEFINE TPM_CONFIG_ENABLE       = FALSE
>
>    #
>    # Network definition
> @@ -221,16 +219,8 @@
>    OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
>    XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf
>
> -
> -!if $(TPM_ENABLE) == TRUE
> -  Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
> -  Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
> -  Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
> -  TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> -!else
>    Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
>    TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
> -!endif
>
>  [LibraryClasses.common]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> @@ -292,11 +282,6 @@
>    CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
>    PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
>
> -!if $(TPM_ENABLE) == TRUE
> -  BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> -  Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
> -!endif
> -
>    MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
>
>  [LibraryClasses.common.DXE_CORE]
> @@ -366,9 +351,6 @@
>  !endif
>    PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
>    MpInitLib|UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf
> -!if $(TPM_ENABLE) == TRUE
> -  Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
> -!endif
>
>  [LibraryClasses.common.UEFI_APPLICATION]
>    PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> @@ -563,22 +545,12 @@
>
>    gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
>
> -!if $(TPM_ENABLE) == TRUE
> -  gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
> -!endif
> -
>    # MdeModulePkg resolution sets up the system display resolution
>    gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution|0
>    gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution|0
>    gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|0
>    gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|0
>
> -[PcdsDynamicHii]
> -!if $(TPM_ENABLE) == TRUE && $(TPM_CONFIG_ENABLE) == TRUE
> -  gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
> -  gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
> -!endif
> -
>  ################################################################################
>  #
>  # Components Section - list of all EDK II Modules needed by this Platform.
> @@ -618,19 +590,6 @@
>      <LibraryClasses>
>    }
>
> -!if $(TPM_ENABLE) == TRUE
> -  OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> -  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> -    <LibraryClasses>
> -      HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> -  }
> -!endif
> -
>    #
>    # DXE Phase modules
>    #
> @@ -653,9 +612,6 @@
>      <LibraryClasses>
>  !if $(SECURE_BOOT_ENABLE) == TRUE
>        NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> -!endif
> -!if $(TPM_ENABLE) == TRUE
> -      NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
>  !endif
>    }
>
> @@ -841,23 +797,3 @@
>        NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
>    }
>
> -
> -  #
> -  # TPM support
> -  #
> -!if $(TPM_ENABLE) == TRUE
> -  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
> -    <LibraryClasses>
> -      Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
> -      NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
> -      HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> -  }
> -!if $(TPM_CONFIG_ENABLE) == TRUE
> -  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> -!endif
> -!endif
> diff --git a/OvmfPkg/Bhyve/BhyveX64.fdf b/OvmfPkg/Bhyve/BhyveX64.fdf
> index 3eff36dac1..fbd63a395a 100644
> --- a/OvmfPkg/Bhyve/BhyveX64.fdf
> +++ b/OvmfPkg/Bhyve/BhyveX64.fdf
> @@ -158,11 +158,6 @@ INF  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
>  INF  OvmfPkg/Bhyve/SmmAccess/SmmAccessPei.inf
>  !endif
>
> -!if $(TPM_ENABLE) == TRUE
> -INF  OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> -INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> -!endif
> -
>  ################################################################################
>
>  [FV.DXEFV]
> @@ -333,16 +328,6 @@ INF  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
>  INF  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
>  !endif
>
> -#
> -# TPM support
> -#
> -!if $(TPM_ENABLE) == TRUE
> -INF  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
> -!if $(TPM_CONFIG_ENABLE) == TRUE
> -INF  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> -!endif
> -!endif
> -
>  ################################################################################
>
>  [FV.FVMAIN_COMPACT]
> --
> 2.32.0
>
>


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

Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Posted by Sean 2 years, 10 months ago

Ard,

The PR you are trying to "push" has a mergify merge in it which is 
causing patchcheck to fail.

https://github.com/tianocore/edk2/pull/1727/commits



Mike,

I think github has better features now around things like auto complete 
PR when "checks pass" and allow rebase merging and finally protections 
to only allow linear history.  Might be time to talk about changes to 
mergify/github.  I know you are busy so maybe opening up to more of the 
edk2 community or actively looking for someone willing to provide best 
practices for github usage (rust-lang and nodejs both do a lot with 
github).


Thanks
Sean





On 6/16/2021 8:58 AM, Ard Biesheuvel wrote:
> (+ Sean, Mike)
> 
> On Sat, 12 Jun 2021 at 22:43, Rebecca Cran <rebecca@bsdio.com> wrote:
>>
>> TPM support hasn't been tested and any lines in the .dsc and .fdf files
>> that appear to show support are bogus. Remove them.
>>
>> This fixes https://bugzilla.tianocore.org/show_bug.cgi?id=3354 .
>>
>> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> 
> Strangely enough, this patch gets rejected by PatchCheck for lack of a
> Signed-off-by line
> 
> https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=24198&view=results
> 
> The patch itself looks good to me
> 
> Acked-by: Ard Biesheuvel <ardb@kernel.org>
> 
> so if anyone else manages to fix the CI issue, feel free to push the
> patch with my R-b (and Peter's, given in reply to this message)
> 
> (I will go offline for 3 weeks after Friday)
> 
>> ---
>>   OvmfPkg/Bhyve/BhyveX64.dsc | 64 --------------------
>>   OvmfPkg/Bhyve/BhyveX64.fdf | 15 -----
>>   2 files changed, 79 deletions(-)
>>
>> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
>> index d8792812ab..cbf896e89b 100644
>> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
>> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
>> @@ -31,8 +31,6 @@
>>     DEFINE SECURE_BOOT_ENABLE      = FALSE
>>     DEFINE SMM_REQUIRE             = FALSE
>>     DEFINE SOURCE_DEBUG_ENABLE     = FALSE
>> -  DEFINE TPM_ENABLE              = FALSE
>> -  DEFINE TPM_CONFIG_ENABLE       = FALSE
>>
>>     #
>>     # Network definition
>> @@ -221,16 +219,8 @@
>>     OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
>>     XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf
>>
>> -
>> -!if $(TPM_ENABLE) == TRUE
>> -  Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
>> -  Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
>> -  Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
>> -  TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
>> -!else
>>     Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
>>     TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
>> -!endif
>>
>>   [LibraryClasses.common]
>>     BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
>> @@ -292,11 +282,6 @@
>>     CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
>>     PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
>>
>> -!if $(TPM_ENABLE) == TRUE
>> -  BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
>> -  Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
>> -!endif
>> -
>>     MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
>>
>>   [LibraryClasses.common.DXE_CORE]
>> @@ -366,9 +351,6 @@
>>   !endif
>>     PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
>>     MpInitLib|UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf
>> -!if $(TPM_ENABLE) == TRUE
>> -  Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
>> -!endif
>>
>>   [LibraryClasses.common.UEFI_APPLICATION]
>>     PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
>> @@ -563,22 +545,12 @@
>>
>>     gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
>>
>> -!if $(TPM_ENABLE) == TRUE
>> -  gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
>> -!endif
>> -
>>     # MdeModulePkg resolution sets up the system display resolution
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution|0
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution|0
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|0
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|0
>>
>> -[PcdsDynamicHii]
>> -!if $(TPM_ENABLE) == TRUE && $(TPM_CONFIG_ENABLE) == TRUE
>> -  gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
>> -  gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
>> -!endif
>> -
>>   ################################################################################
>>   #
>>   # Components Section - list of all EDK II Modules needed by this Platform.
>> @@ -618,19 +590,6 @@
>>       <LibraryClasses>
>>     }
>>
>> -!if $(TPM_ENABLE) == TRUE
>> -  OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> -  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
>> -    <LibraryClasses>
>> -      HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
>> -      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
>> -      NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
>> -      NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
>> -      NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
>> -      NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
>> -  }
>> -!endif
>> -
>>     #
>>     # DXE Phase modules
>>     #
>> @@ -653,9 +612,6 @@
>>       <LibraryClasses>
>>   !if $(SECURE_BOOT_ENABLE) == TRUE
>>         NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
>> -!endif
>> -!if $(TPM_ENABLE) == TRUE
>> -      NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
>>   !endif
>>     }
>>
>> @@ -841,23 +797,3 @@
>>         NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
>>     }
>>
>> -
>> -  #
>> -  # TPM support
>> -  #
>> -!if $(TPM_ENABLE) == TRUE
>> -  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
>> -    <LibraryClasses>
>> -      Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
>> -      NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
>> -      HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
>> -      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
>> -      NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
>> -      NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
>> -      NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
>> -      NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
>> -  }
>> -!if $(TPM_CONFIG_ENABLE) == TRUE
>> -  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
>> -!endif
>> -!endif
>> diff --git a/OvmfPkg/Bhyve/BhyveX64.fdf b/OvmfPkg/Bhyve/BhyveX64.fdf
>> index 3eff36dac1..fbd63a395a 100644
>> --- a/OvmfPkg/Bhyve/BhyveX64.fdf
>> +++ b/OvmfPkg/Bhyve/BhyveX64.fdf
>> @@ -158,11 +158,6 @@ INF  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
>>   INF  OvmfPkg/Bhyve/SmmAccess/SmmAccessPei.inf
>>   !endif
>>
>> -!if $(TPM_ENABLE) == TRUE
>> -INF  OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> -INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>> -!endif
>> -
>>   ################################################################################
>>
>>   [FV.DXEFV]
>> @@ -333,16 +328,6 @@ INF  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
>>   INF  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
>>   !endif
>>
>> -#
>> -# TPM support
>> -#
>> -!if $(TPM_ENABLE) == TRUE
>> -INF  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
>> -!if $(TPM_CONFIG_ENABLE) == TRUE
>> -INF  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
>> -!endif
>> -!endif
>> -
>>   ################################################################################
>>
>>   [FV.FVMAIN_COMPACT]
>> --
>> 2.32.0
>>
>>
> 
> 
> 
> 
> 


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

Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Posted by Ard Biesheuvel 2 years, 10 months ago

On Wed, 16 Jun 2021 at 21:00, Sean Brogan <spbrogan@outlook.com> wrote:
>
> Ard,
>
> The PR you are trying to "push" has a mergify merge in it which is
> causing patchcheck to fail.
>
> https://github.com/tianocore/edk2/pull/1727/commits
>

Ah, I missed that. So the base of my branch was out of date, and the
merge was done automatically, right? I think we still want linear
history in EDK2, so this should probably be rejected before even
reaching the point where the tools perform the merge by themselves.

>
>
> Mike,
>
> I think github has better features now around things like auto complete
> PR when "checks pass" and allow rebase merging and finally protections
> to only allow linear history.  Might be time to talk about changes to
> mergify/github.  I know you are busy so maybe opening up to more of the
> edk2 community or actively looking for someone willing to provide best
> practices for github usage (rust-lang and nodejs both do a lot with
> github).
>
>
> Thanks
> Sean
>
>
>
>
>
> On 6/16/2021 8:58 AM, Ard Biesheuvel wrote:
> > (+ Sean, Mike)
> >
> > On Sat, 12 Jun 2021 at 22:43, Rebecca Cran <rebecca@bsdio.com> wrote:
> >>
> >> TPM support hasn't been tested and any lines in the .dsc and .fdf files
> >> that appear to show support are bogus. Remove them.
> >>
> >> This fixes https://bugzilla.tianocore.org/show_bug.cgi?id=3354 .
> >>
> >> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> >
> > Strangely enough, this patch gets rejected by PatchCheck for lack of a
> > Signed-off-by line
> >
> > https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=24198&view=results
> >
> > The patch itself looks good to me
> >
> > Acked-by: Ard Biesheuvel <ardb@kernel.org>
> >
> > so if anyone else manages to fix the CI issue, feel free to push the
> > patch with my R-b (and Peter's, given in reply to this message)
> >
> > (I will go offline for 3 weeks after Friday)
> >
> >> ---
> >>   OvmfPkg/Bhyve/BhyveX64.dsc | 64 --------------------
> >>   OvmfPkg/Bhyve/BhyveX64.fdf | 15 -----
> >>   2 files changed, 79 deletions(-)
> >>
> >> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
> >> index d8792812ab..cbf896e89b 100644
> >> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
> >> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
> >> @@ -31,8 +31,6 @@
> >>     DEFINE SECURE_BOOT_ENABLE      = FALSE
> >>     DEFINE SMM_REQUIRE             = FALSE
> >>     DEFINE SOURCE_DEBUG_ENABLE     = FALSE
> >> -  DEFINE TPM_ENABLE              = FALSE
> >> -  DEFINE TPM_CONFIG_ENABLE       = FALSE
> >>
> >>     #
> >>     # Network definition
> >> @@ -221,16 +219,8 @@
> >>     OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
> >>     XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf
> >>
> >> -
> >> -!if $(TPM_ENABLE) == TRUE
> >> -  Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
> >> -  Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
> >> -  Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
> >> -  TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> >> -!else
> >>     Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
> >>     TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
> >> -!endif
> >>
> >>   [LibraryClasses.common]
> >>     BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> >> @@ -292,11 +282,6 @@
> >>     CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
> >>     PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
> >>
> >> -!if $(TPM_ENABLE) == TRUE
> >> -  BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> >> -  Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
> >> -!endif
> >> -
> >>     MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
> >>
> >>   [LibraryClasses.common.DXE_CORE]
> >> @@ -366,9 +351,6 @@
> >>   !endif
> >>     PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
> >>     MpInitLib|UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf
> >> -!if $(TPM_ENABLE) == TRUE
> >> -  Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
> >> -!endif
> >>
> >>   [LibraryClasses.common.UEFI_APPLICATION]
> >>     PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> >> @@ -563,22 +545,12 @@
> >>
> >>     gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
> >>
> >> -!if $(TPM_ENABLE) == TRUE
> >> -  gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
> >> -!endif
> >> -
> >>     # MdeModulePkg resolution sets up the system display resolution
> >>     gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution|0
> >>     gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution|0
> >>     gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|0
> >>     gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|0
> >>
> >> -[PcdsDynamicHii]
> >> -!if $(TPM_ENABLE) == TRUE && $(TPM_CONFIG_ENABLE) == TRUE
> >> -  gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
> >> -  gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
> >> -!endif
> >> -
> >>   ################################################################################
> >>   #
> >>   # Components Section - list of all EDK II Modules needed by this Platform.
> >> @@ -618,19 +590,6 @@
> >>       <LibraryClasses>
> >>     }
> >>
> >> -!if $(TPM_ENABLE) == TRUE
> >> -  OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> >> -  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> >> -    <LibraryClasses>
> >> -      HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
> >> -      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> >> -      NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> >> -      NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> >> -      NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> >> -      NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> >> -  }
> >> -!endif
> >> -
> >>     #
> >>     # DXE Phase modules
> >>     #
> >> @@ -653,9 +612,6 @@
> >>       <LibraryClasses>
> >>   !if $(SECURE_BOOT_ENABLE) == TRUE
> >>         NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> >> -!endif
> >> -!if $(TPM_ENABLE) == TRUE
> >> -      NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
> >>   !endif
> >>     }
> >>
> >> @@ -841,23 +797,3 @@
> >>         NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
> >>     }
> >>
> >> -
> >> -  #
> >> -  # TPM support
> >> -  #
> >> -!if $(TPM_ENABLE) == TRUE
> >> -  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
> >> -    <LibraryClasses>
> >> -      Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
> >> -      NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
> >> -      HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
> >> -      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> >> -      NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> >> -      NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> >> -      NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> >> -      NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> >> -  }
> >> -!if $(TPM_CONFIG_ENABLE) == TRUE
> >> -  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> >> -!endif
> >> -!endif
> >> diff --git a/OvmfPkg/Bhyve/BhyveX64.fdf b/OvmfPkg/Bhyve/BhyveX64.fdf
> >> index 3eff36dac1..fbd63a395a 100644
> >> --- a/OvmfPkg/Bhyve/BhyveX64.fdf
> >> +++ b/OvmfPkg/Bhyve/BhyveX64.fdf
> >> @@ -158,11 +158,6 @@ INF  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> >>   INF  OvmfPkg/Bhyve/SmmAccess/SmmAccessPei.inf
> >>   !endif
> >>
> >> -!if $(TPM_ENABLE) == TRUE
> >> -INF  OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> >> -INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> >> -!endif
> >> -
> >>   ################################################################################
> >>
> >>   [FV.DXEFV]
> >> @@ -333,16 +328,6 @@ INF  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
> >>   INF  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> >>   !endif
> >>
> >> -#
> >> -# TPM support
> >> -#
> >> -!if $(TPM_ENABLE) == TRUE
> >> -INF  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
> >> -!if $(TPM_CONFIG_ENABLE) == TRUE
> >> -INF  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> >> -!endif
> >> -!endif
> >> -
> >>   ################################################################################
> >>
> >>   [FV.FVMAIN_COMPACT]
> >> --
> >> 2.32.0
> >>
> >>
> >
> >
> > 
> >
> >


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

Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Posted by Michael D Kinney 2 years, 10 months ago

Hi Ard,

I was not merged into edk2/master.

PatchCheck failed due to the merge commit in the submitted PR.

You will just need to resubmit the PR without the merge commit and rebased against latest edk2/master, and it will then be able to go through.

Mike

> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Wednesday, June 16, 2021 2:55 PM
> To: Sean Brogan <spbrogan@outlook.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Peter Grehan <grehan@freebsd.org>; Laszlo Ersek <lersek@redhat.com>;
> Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>; Sean Brogan
> <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>; edk2-devel-groups-io <devel@edk2.groups.io>
> Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
> 
> On Wed, 16 Jun 2021 at 21:00, Sean Brogan <spbrogan@outlook.com> wrote:
> >
> > Ard,
> >
> > The PR you are trying to "push" has a mergify merge in it which is
> > causing patchcheck to fail.
> >
> > https://github.com/tianocore/edk2/pull/1727/commits
> >
> 
> Ah, I missed that. So the base of my branch was out of date, and the
> merge was done automatically, right? I think we still want linear
> history in EDK2, so this should probably be rejected before even
> reaching the point where the tools perform the merge by themselves.
> 
> >
> >
> > Mike,
> >
> > I think github has better features now around things like auto complete
> > PR when "checks pass" and allow rebase merging and finally protections
> > to only allow linear history.  Might be time to talk about changes to
> > mergify/github.  I know you are busy so maybe opening up to more of the
> > edk2 community or actively looking for someone willing to provide best
> > practices for github usage (rust-lang and nodejs both do a lot with
> > github).
> >
> >
> > Thanks
> > Sean
> >
> >
> >
> >
> >
> > On 6/16/2021 8:58 AM, Ard Biesheuvel wrote:
> > > (+ Sean, Mike)
> > >
> > > On Sat, 12 Jun 2021 at 22:43, Rebecca Cran <rebecca@bsdio.com> wrote:
> > >>
> > >> TPM support hasn't been tested and any lines in the .dsc and .fdf files
> > >> that appear to show support are bogus. Remove them.
> > >>
> > >> This fixes https://bugzilla.tianocore.org/show_bug.cgi?id=3354 .
> > >>
> > >> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> > >
> > > Strangely enough, this patch gets rejected by PatchCheck for lack of a
> > > Signed-off-by line
> > >
> > > https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=24198&view=results
> > >
> > > The patch itself looks good to me
> > >
> > > Acked-by: Ard Biesheuvel <ardb@kernel.org>
> > >
> > > so if anyone else manages to fix the CI issue, feel free to push the
> > > patch with my R-b (and Peter's, given in reply to this message)
> > >
> > > (I will go offline for 3 weeks after Friday)
> > >
> > >> ---
> > >>   OvmfPkg/Bhyve/BhyveX64.dsc | 64 --------------------
> > >>   OvmfPkg/Bhyve/BhyveX64.fdf | 15 -----
> > >>   2 files changed, 79 deletions(-)
> > >>
> > >> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
> > >> index d8792812ab..cbf896e89b 100644
> > >> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
> > >> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
> > >> @@ -31,8 +31,6 @@
> > >>     DEFINE SECURE_BOOT_ENABLE      = FALSE
> > >>     DEFINE SMM_REQUIRE             = FALSE
> > >>     DEFINE SOURCE_DEBUG_ENABLE     = FALSE
> > >> -  DEFINE TPM_ENABLE              = FALSE
> > >> -  DEFINE TPM_CONFIG_ENABLE       = FALSE
> > >>
> > >>     #
> > >>     # Network definition
> > >> @@ -221,16 +219,8 @@
> > >>     OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
> > >>     XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf
> > >>
> > >> -
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> -  Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
> > >> -  Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
> > >> -  Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
> > >> -  TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> > >> -!else
> > >>     Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
> > >>     TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
> > >> -!endif
> > >>
> > >>   [LibraryClasses.common]
> > >>     BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> > >> @@ -292,11 +282,6 @@
> > >>     CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
> > >>     PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
> > >>
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> -  BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> > >> -  Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
> > >> -!endif
> > >> -
> > >>     MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
> > >>
> > >>   [LibraryClasses.common.DXE_CORE]
> > >> @@ -366,9 +351,6 @@
> > >>   !endif
> > >>     PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
> > >>     MpInitLib|UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> -  Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
> > >> -!endif
> > >>
> > >>   [LibraryClasses.common.UEFI_APPLICATION]
> > >>     PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> > >> @@ -563,22 +545,12 @@
> > >>
> > >>     gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
> > >>
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> -  gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
> > >> -!endif
> > >> -
> > >>     # MdeModulePkg resolution sets up the system display resolution
> > >>     gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution|0
> > >>     gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution|0
> > >>     gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|0
> > >>     gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|0
> > >>
> > >> -[PcdsDynamicHii]
> > >> -!if $(TPM_ENABLE) == TRUE && $(TPM_CONFIG_ENABLE) == TRUE
> > >> -
> gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
> > >> -  gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
> > >> -!endif
> > >> -
> > >>   ################################################################################
> > >>   #
> > >>   # Components Section - list of all EDK II Modules needed by this Platform.
> > >> @@ -618,19 +590,6 @@
> > >>       <LibraryClasses>
> > >>     }
> > >>
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> -  OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> > >> -  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> > >> -    <LibraryClasses>
> > >> -      HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
> > >> -      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> > >> -      NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> > >> -      NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> > >> -      NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> > >> -      NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> > >> -  }
> > >> -!endif
> > >> -
> > >>     #
> > >>     # DXE Phase modules
> > >>     #
> > >> @@ -653,9 +612,6 @@
> > >>       <LibraryClasses>
> > >>   !if $(SECURE_BOOT_ENABLE) == TRUE
> > >>         NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> > >> -!endif
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> -      NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
> > >>   !endif
> > >>     }
> > >>
> > >> @@ -841,23 +797,3 @@
> > >>         NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
> > >>     }
> > >>
> > >> -
> > >> -  #
> > >> -  # TPM support
> > >> -  #
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> -  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
> > >> -    <LibraryClasses>
> > >> -      Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
> > >> -      NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
> > >> -      HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
> > >> -      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> > >> -      NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> > >> -      NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> > >> -      NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> > >> -      NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> > >> -  }
> > >> -!if $(TPM_CONFIG_ENABLE) == TRUE
> > >> -  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> > >> -!endif
> > >> -!endif
> > >> diff --git a/OvmfPkg/Bhyve/BhyveX64.fdf b/OvmfPkg/Bhyve/BhyveX64.fdf
> > >> index 3eff36dac1..fbd63a395a 100644
> > >> --- a/OvmfPkg/Bhyve/BhyveX64.fdf
> > >> +++ b/OvmfPkg/Bhyve/BhyveX64.fdf
> > >> @@ -158,11 +158,6 @@ INF  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> > >>   INF  OvmfPkg/Bhyve/SmmAccess/SmmAccessPei.inf
> > >>   !endif
> > >>
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> -INF  OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> > >> -INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> > >> -!endif
> > >> -
> > >>   ################################################################################
> > >>
> > >>   [FV.DXEFV]
> > >> @@ -333,16 +328,6 @@ INF  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
> > >>   INF  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> > >>   !endif
> > >>
> > >> -#
> > >> -# TPM support
> > >> -#
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> -INF  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
> > >> -!if $(TPM_CONFIG_ENABLE) == TRUE
> > >> -INF  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> > >> -!endif
> > >> -!endif
> > >> -
> > >>   ################################################################################
> > >>
> > >>   [FV.FVMAIN_COMPACT]
> > >> --
> > >> 2.32.0
> > >>
> > >>
> > >
> > >
> > > 
> > >
> > >


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

Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Posted by Michael D Kinney 2 years, 10 months ago

Hi Sean,

Mergify had added a queue feature to handle the rebases automatically and make sure
CI passes in the order that the PRs are being applied to the base branch.

I have setup the edk2-codereview repo with a smaller set of CI tests to skip the
compiles to provide a way to experiment with these features quickly.  I have 
also simplified the Azure Pipelines templates to remove the FINISHED and FAILED
jobs and the 10 second delay.  I have also removed all references to status
check names from the Mergify config file, so the status checks are only
configured using the GitHub protected branches feature.

Here is the Mergify config that is working:

    https://github.com/tianocore/edk2-codereview/blob/master/.mergify/config.yml

Here is the Azure Pipelines file that has been simplified removing FINISHED and FAILED jobs
(also removes DEBUG, RELEASE, NOOPT builds for fast testing):

    https://github.com/tianocore/edk2-codereview/blob/master/.azurepipelines/templates/pr-gate-build-job.yml

I then did an experiment sending a PR that is out of date with the base branch.
Mergify auto rebased and ran CI tests and added commits from the PR to the base
branch.

I did a 2nd experiment sending 2 PRs at the same time.  The first PR finishes its
CI tests and was merged.  The 2nd PR was auto rebased and CI tests run and was merged.
No developer interaction required on either one after submitting the PR.  This 
resolves a common issue seen by Maintainers that process a number of unrelated
patch sets at the same time.  They can all be submitted on top of each other without
having to wait for each one to complete and rebase the next one before submitting.

The only open issue remaining is that Mergify auto rebase adds a merge commit
to the set of commits in the PR.  The merge commit is not added to the base branch
when it is done.  Since the merge commit is present in the PR, the patch check fails.
We need to update patch check to ignore merge commits by the Mergify agent.  I have
temporarily disabled the patch check CI status check in edk2-codereview to
work around this issue.  Once I have a fix for patch check, I will re-enable the
patch check status check.

I also think this approach will fix the issue that has been seen where Mergify merged
before all the platform tests were completed.  We just need to make sure the
platform checks are in the list of enabled status checks in the GitHub protected
branch configuration.  I will verify this issue is resolved using the edk2-codereview
repo.

Best regards,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sean
> Sent: Wednesday, June 16, 2021 12:00 PM
> To: ardb@kernel.org; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Peter Grehan <grehan@freebsd.org>; Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> Justen, Jordan L <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>;
> devel@edk2.groups.io
> Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
> 
> Ard,
> 
> The PR you are trying to "push" has a mergify merge in it which is
> causing patchcheck to fail.
> 
> https://github.com/tianocore/edk2/pull/1727/commits
> 
> 
> 
> Mike,
> 
> I think github has better features now around things like auto complete
> PR when "checks pass" and allow rebase merging and finally protections
> to only allow linear history.  Might be time to talk about changes to
> mergify/github.  I know you are busy so maybe opening up to more of the
> edk2 community or actively looking for someone willing to provide best
> practices for github usage (rust-lang and nodejs both do a lot with
> github).
> 
> 
> Thanks
> Sean
> 
> 
> 
> 
> 
> On 6/16/2021 8:58 AM, Ard Biesheuvel wrote:
> > (+ Sean, Mike)
> >
> > On Sat, 12 Jun 2021 at 22:43, Rebecca Cran <rebecca@bsdio.com> wrote:
> >>
> >> TPM support hasn't been tested and any lines in the .dsc and .fdf files
> >> that appear to show support are bogus. Remove them.
> >>
> >> This fixes https://bugzilla.tianocore.org/show_bug.cgi?id=3354 .
> >>
> >> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> >
> > Strangely enough, this patch gets rejected by PatchCheck for lack of a
> > Signed-off-by line
> >
> > https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=24198&view=results
> >
> > The patch itself looks good to me
> >
> > Acked-by: Ard Biesheuvel <ardb@kernel.org>
> >
> > so if anyone else manages to fix the CI issue, feel free to push the
> > patch with my R-b (and Peter's, given in reply to this message)
> >
> > (I will go offline for 3 weeks after Friday)
> >
> >> ---
> >>   OvmfPkg/Bhyve/BhyveX64.dsc | 64 --------------------
> >>   OvmfPkg/Bhyve/BhyveX64.fdf | 15 -----
> >>   2 files changed, 79 deletions(-)
> >>
> >> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
> >> index d8792812ab..cbf896e89b 100644
> >> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
> >> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
> >> @@ -31,8 +31,6 @@
> >>     DEFINE SECURE_BOOT_ENABLE      = FALSE
> >>     DEFINE SMM_REQUIRE             = FALSE
> >>     DEFINE SOURCE_DEBUG_ENABLE     = FALSE
> >> -  DEFINE TPM_ENABLE              = FALSE
> >> -  DEFINE TPM_CONFIG_ENABLE       = FALSE
> >>
> >>     #
> >>     # Network definition
> >> @@ -221,16 +219,8 @@
> >>     OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
> >>     XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf
> >>
> >> -
> >> -!if $(TPM_ENABLE) == TRUE
> >> -  Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
> >> -  Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
> >> -  Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
> >> -  TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> >> -!else
> >>     Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
> >>     TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
> >> -!endif
> >>
> >>   [LibraryClasses.common]
> >>     BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> >> @@ -292,11 +282,6 @@
> >>     CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
> >>     PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
> >>
> >> -!if $(TPM_ENABLE) == TRUE
> >> -  BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> >> -  Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
> >> -!endif
> >> -
> >>     MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
> >>
> >>   [LibraryClasses.common.DXE_CORE]
> >> @@ -366,9 +351,6 @@
> >>   !endif
> >>     PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
> >>     MpInitLib|UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf
> >> -!if $(TPM_ENABLE) == TRUE
> >> -  Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
> >> -!endif
> >>
> >>   [LibraryClasses.common.UEFI_APPLICATION]
> >>     PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> >> @@ -563,22 +545,12 @@
> >>
> >>     gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
> >>
> >> -!if $(TPM_ENABLE) == TRUE
> >> -  gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00}
> >> -!endif
> >> -
> >>     # MdeModulePkg resolution sets up the system display resolution
> >>     gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution|0
> >>     gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution|0
> >>     gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|0
> >>     gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|0
> >>
> >> -[PcdsDynamicHii]
> >> -!if $(TPM_ENABLE) == TRUE && $(TPM_CONFIG_ENABLE) == TRUE
> >> -
> gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
> >> -  gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
> >> -!endif
> >> -
> >>   ################################################################################
> >>   #
> >>   # Components Section - list of all EDK II Modules needed by this Platform.
> >> @@ -618,19 +590,6 @@
> >>       <LibraryClasses>
> >>     }
> >>
> >> -!if $(TPM_ENABLE) == TRUE
> >> -  OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> >> -  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> >> -    <LibraryClasses>
> >> -      HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
> >> -      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> >> -      NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> >> -      NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> >> -      NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> >> -      NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> >> -  }
> >> -!endif
> >> -
> >>     #
> >>     # DXE Phase modules
> >>     #
> >> @@ -653,9 +612,6 @@
> >>       <LibraryClasses>
> >>   !if $(SECURE_BOOT_ENABLE) == TRUE
> >>         NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> >> -!endif
> >> -!if $(TPM_ENABLE) == TRUE
> >> -      NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
> >>   !endif
> >>     }
> >>
> >> @@ -841,23 +797,3 @@
> >>         NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
> >>     }
> >>
> >> -
> >> -  #
> >> -  # TPM support
> >> -  #
> >> -!if $(TPM_ENABLE) == TRUE
> >> -  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
> >> -    <LibraryClasses>
> >> -      Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
> >> -      NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
> >> -      HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
> >> -      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> >> -      NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> >> -      NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> >> -      NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> >> -      NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> >> -  }
> >> -!if $(TPM_CONFIG_ENABLE) == TRUE
> >> -  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> >> -!endif
> >> -!endif
> >> diff --git a/OvmfPkg/Bhyve/BhyveX64.fdf b/OvmfPkg/Bhyve/BhyveX64.fdf
> >> index 3eff36dac1..fbd63a395a 100644
> >> --- a/OvmfPkg/Bhyve/BhyveX64.fdf
> >> +++ b/OvmfPkg/Bhyve/BhyveX64.fdf
> >> @@ -158,11 +158,6 @@ INF  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> >>   INF  OvmfPkg/Bhyve/SmmAccess/SmmAccessPei.inf
> >>   !endif
> >>
> >> -!if $(TPM_ENABLE) == TRUE
> >> -INF  OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> >> -INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> >> -!endif
> >> -
> >>   ################################################################################
> >>
> >>   [FV.DXEFV]
> >> @@ -333,16 +328,6 @@ INF  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
> >>   INF  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> >>   !endif
> >>
> >> -#
> >> -# TPM support
> >> -#
> >> -!if $(TPM_ENABLE) == TRUE
> >> -INF  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
> >> -!if $(TPM_CONFIG_ENABLE) == TRUE
> >> -INF  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> >> -!endif
> >> -!endif
> >> -
> >>   ################################################################################
> >>
> >>   [FV.FVMAIN_COMPACT]
> >> --
> >> 2.32.0
> >>
> >>
> >
> >
> >
> >
> >
> 
> 
> 
> 



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

Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Posted by Michael D Kinney 2 years, 10 months ago

Link to Mergify queue feature documentation

    https://docs.mergify.io/actions/queue/#merge-queue-problem

Mike

> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Thursday, June 17, 2021 2:53 PM
> To: devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Peter Grehan <grehan@freebsd.org>; Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> Justen, Jordan L <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
> Subject: RE: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
> 
> Hi Sean,
> 
> Mergify had added a queue feature to handle the rebases automatically and make sure
> CI passes in the order that the PRs are being applied to the base branch.
> 
> I have setup the edk2-codereview repo with a smaller set of CI tests to skip the
> compiles to provide a way to experiment with these features quickly.  I have
> also simplified the Azure Pipelines templates to remove the FINISHED and FAILED
> jobs and the 10 second delay.  I have also removed all references to status
> check names from the Mergify config file, so the status checks are only
> configured using the GitHub protected branches feature.
> 
> Here is the Mergify config that is working:
> 
>     https://github.com/tianocore/edk2-codereview/blob/master/.mergify/config.yml
> 
> Here is the Azure Pipelines file that has been simplified removing FINISHED and FAILED jobs
> (also removes DEBUG, RELEASE, NOOPT builds for fast testing):
> 
>     https://github.com/tianocore/edk2-codereview/blob/master/.azurepipelines/templates/pr-gate-build-job.yml
> 
> I then did an experiment sending a PR that is out of date with the base branch.
> Mergify auto rebased and ran CI tests and added commits from the PR to the base
> branch.
> 
> I did a 2nd experiment sending 2 PRs at the same time.  The first PR finishes its
> CI tests and was merged.  The 2nd PR was auto rebased and CI tests run and was merged.
> No developer interaction required on either one after submitting the PR.  This
> resolves a common issue seen by Maintainers that process a number of unrelated
> patch sets at the same time.  They can all be submitted on top of each other without
> having to wait for each one to complete and rebase the next one before submitting.
> 
> The only open issue remaining is that Mergify auto rebase adds a merge commit
> to the set of commits in the PR.  The merge commit is not added to the base branch
> when it is done.  Since the merge commit is present in the PR, the patch check fails.
> We need to update patch check to ignore merge commits by the Mergify agent.  I have
> temporarily disabled the patch check CI status check in edk2-codereview to
> work around this issue.  Once I have a fix for patch check, I will re-enable the
> patch check status check.
> 
> I also think this approach will fix the issue that has been seen where Mergify merged
> before all the platform tests were completed.  We just need to make sure the
> platform checks are in the list of enabled status checks in the GitHub protected
> branch configuration.  I will verify this issue is resolved using the edk2-codereview
> repo.
> 
> Best regards,
> 
> Mike
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sean
> > Sent: Wednesday, June 16, 2021 12:00 PM
> > To: ardb@kernel.org; Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: Peter Grehan <grehan@freebsd.org>; Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> > Justen, Jordan L <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>;
> > devel@edk2.groups.io
> > Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
> >
> > Ard,
> >
> > The PR you are trying to "push" has a mergify merge in it which is
> > causing patchcheck to fail.
> >
> > https://github.com/tianocore/edk2/pull/1727/commits
> >
> >
> >
> > Mike,
> >
> > I think github has better features now around things like auto complete
> > PR when "checks pass" and allow rebase merging and finally protections
> > to only allow linear history.  Might be time to talk about changes to
> > mergify/github.  I know you are busy so maybe opening up to more of the
> > edk2 community or actively looking for someone willing to provide best
> > practices for github usage (rust-lang and nodejs both do a lot with
> > github).
> >
> >
> > Thanks
> > Sean
> >
> >
> >
> >
> >
> > On 6/16/2021 8:58 AM, Ard Biesheuvel wrote:
> > > (+ Sean, Mike)
> > >
> > > On Sat, 12 Jun 2021 at 22:43, Rebecca Cran <rebecca@bsdio.com> wrote:
> > >>
> > >> TPM support hasn't been tested and any lines in the .dsc and .fdf files
> > >> that appear to show support are bogus. Remove them.
> > >>
> > >> This fixes https://bugzilla.tianocore.org/show_bug.cgi?id=3354 .
> > >>
> > >> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> > >
> > > Strangely enough, this patch gets rejected by PatchCheck for lack of a
> > > Signed-off-by line
> > >
> > > https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=24198&view=results
> > >
> > > The patch itself looks good to me
> > >
> > > Acked-by: Ard Biesheuvel <ardb@kernel.org>
> > >
> > > so if anyone else manages to fix the CI issue, feel free to push the
> > > patch with my R-b (and Peter's, given in reply to this message)
> > >
> > > (I will go offline for 3 weeks after Friday)
> > >
> > >> ---
> > >>   OvmfPkg/Bhyve/BhyveX64.dsc | 64 --------------------
> > >>   OvmfPkg/Bhyve/BhyveX64.fdf | 15 -----
> > >>   2 files changed, 79 deletions(-)
> > >>
> > >> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
> > >> index d8792812ab..cbf896e89b 100644
> > >> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
> > >> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
> > >> @@ -31,8 +31,6 @@
> > >>     DEFINE SECURE_BOOT_ENABLE      = FALSE
> > >>     DEFINE SMM_REQUIRE             = FALSE
> > >>     DEFINE SOURCE_DEBUG_ENABLE     = FALSE
> > >> -  DEFINE TPM_ENABLE              = FALSE
> > >> -  DEFINE TPM_CONFIG_ENABLE       = FALSE
> > >>
> > >>     #
> > >>     # Network definition
> > >> @@ -221,16 +219,8 @@
> > >>     OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
> > >>     XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf
> > >>
> > >> -
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> -  Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
> > >> -  Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
> > >> -  Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
> > >> -  TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> > >> -!else
> > >>     Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
> > >>     TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
> > >> -!endif
> > >>
> > >>   [LibraryClasses.common]
> > >>     BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> > >> @@ -292,11 +282,6 @@
> > >>     CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
> > >>     PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
> > >>
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> -  BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> > >> -  Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
> > >> -!endif
> > >> -
> > >>     MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
> > >>
> > >>   [LibraryClasses.common.DXE_CORE]
> > >> @@ -366,9 +351,6 @@
> > >>   !endif
> > >>     PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
> > >>     MpInitLib|UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> -  Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
> > >> -!endif
> > >>
> > >>   [LibraryClasses.common.UEFI_APPLICATION]
> > >>     PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> > >> @@ -563,22 +545,12 @@
> > >>
> > >>     gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
> > >>
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> -  gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00,
> > 0x00, 0x00, 0x00, 0x00, 0x00}
> > >> -!endif
> > >> -
> > >>     # MdeModulePkg resolution sets up the system display resolution
> > >>     gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution|0
> > >>     gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution|0
> > >>     gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|0
> > >>     gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|0
> > >>
> > >> -[PcdsDynamicHii]
> > >> -!if $(TPM_ENABLE) == TRUE && $(TPM_CONFIG_ENABLE) == TRUE
> > >> -
> > gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
> > >> -  gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
> > >> -!endif
> > >> -
> > >>   ################################################################################
> > >>   #
> > >>   # Components Section - list of all EDK II Modules needed by this Platform.
> > >> @@ -618,19 +590,6 @@
> > >>       <LibraryClasses>
> > >>     }
> > >>
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> -  OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> > >> -  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> > >> -    <LibraryClasses>
> > >> -      HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
> > >> -      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> > >> -      NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> > >> -      NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> > >> -      NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> > >> -      NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> > >> -  }
> > >> -!endif
> > >> -
> > >>     #
> > >>     # DXE Phase modules
> > >>     #
> > >> @@ -653,9 +612,6 @@
> > >>       <LibraryClasses>
> > >>   !if $(SECURE_BOOT_ENABLE) == TRUE
> > >>         NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> > >> -!endif
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> -      NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
> > >>   !endif
> > >>     }
> > >>
> > >> @@ -841,23 +797,3 @@
> > >>         NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
> > >>     }
> > >>
> > >> -
> > >> -  #
> > >> -  # TPM support
> > >> -  #
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> -  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
> > >> -    <LibraryClasses>
> > >> -      Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
> > >> -      NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
> > >> -      HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
> > >> -      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> > >> -      NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> > >> -      NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> > >> -      NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> > >> -      NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> > >> -  }
> > >> -!if $(TPM_CONFIG_ENABLE) == TRUE
> > >> -  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> > >> -!endif
> > >> -!endif
> > >> diff --git a/OvmfPkg/Bhyve/BhyveX64.fdf b/OvmfPkg/Bhyve/BhyveX64.fdf
> > >> index 3eff36dac1..fbd63a395a 100644
> > >> --- a/OvmfPkg/Bhyve/BhyveX64.fdf
> > >> +++ b/OvmfPkg/Bhyve/BhyveX64.fdf
> > >> @@ -158,11 +158,6 @@ INF  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> > >>   INF  OvmfPkg/Bhyve/SmmAccess/SmmAccessPei.inf
> > >>   !endif
> > >>
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> -INF  OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> > >> -INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> > >> -!endif
> > >> -
> > >>   ################################################################################
> > >>
> > >>   [FV.DXEFV]
> > >> @@ -333,16 +328,6 @@ INF  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
> > >>   INF  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> > >>   !endif
> > >>
> > >> -#
> > >> -# TPM support
> > >> -#
> > >> -!if $(TPM_ENABLE) == TRUE
> > >> -INF  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
> > >> -!if $(TPM_CONFIG_ENABLE) == TRUE
> > >> -INF  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> > >> -!endif
> > >> -!endif
> > >> -
> > >>   ################################################################################
> > >>
> > >>   [FV.FVMAIN_COMPACT]
> > >> --
> > >> 2.32.0
> > >>
> > >>
> > >
> > >
> > >
> > >
> > >
> >
> >
> > 
> >



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

Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Posted by Michael D Kinney 2 years, 10 months ago

Hi Sean,

Here is the proposed fix to PatchCheck.py to not flag a Mergify merge commit as an error:

    https://github.com/tianocore/edk2-codereview/commit/1b264739387ae940b42d70b5cd25b7c68ed21318

I have re-enabled PatchCheck in GitHub branch protection rules, and I did 2 PRs at same time and 
they were auto rebased by Mergify and passed PatchCheck test.

Best regards,

Mike


> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Thursday, June 17, 2021 2:55 PM
> To: devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Peter Grehan <grehan@freebsd.org>; Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> Justen, Jordan L <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
> Subject: RE: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
> 
> Link to Mergify queue feature documentation
> 
>     https://docs.mergify.io/actions/queue/#merge-queue-problem
> 
> Mike
> 
> > -----Original Message-----
> > From: Kinney, Michael D <michael.d.kinney@intel.com>
> > Sent: Thursday, June 17, 2021 2:53 PM
> > To: devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org; Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: Peter Grehan <grehan@freebsd.org>; Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> > Justen, Jordan L <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
> > Subject: RE: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
> >
> > Hi Sean,
> >
> > Mergify had added a queue feature to handle the rebases automatically and make sure
> > CI passes in the order that the PRs are being applied to the base branch.
> >
> > I have setup the edk2-codereview repo with a smaller set of CI tests to skip the
> > compiles to provide a way to experiment with these features quickly.  I have
> > also simplified the Azure Pipelines templates to remove the FINISHED and FAILED
> > jobs and the 10 second delay.  I have also removed all references to status
> > check names from the Mergify config file, so the status checks are only
> > configured using the GitHub protected branches feature.
> >
> > Here is the Mergify config that is working:
> >
> >     https://github.com/tianocore/edk2-codereview/blob/master/.mergify/config.yml
> >
> > Here is the Azure Pipelines file that has been simplified removing FINISHED and FAILED jobs
> > (also removes DEBUG, RELEASE, NOOPT builds for fast testing):
> >
> >     https://github.com/tianocore/edk2-codereview/blob/master/.azurepipelines/templates/pr-gate-build-job.yml
> >
> > I then did an experiment sending a PR that is out of date with the base branch.
> > Mergify auto rebased and ran CI tests and added commits from the PR to the base
> > branch.
> >
> > I did a 2nd experiment sending 2 PRs at the same time.  The first PR finishes its
> > CI tests and was merged.  The 2nd PR was auto rebased and CI tests run and was merged.
> > No developer interaction required on either one after submitting the PR.  This
> > resolves a common issue seen by Maintainers that process a number of unrelated
> > patch sets at the same time.  They can all be submitted on top of each other without
> > having to wait for each one to complete and rebase the next one before submitting.
> >
> > The only open issue remaining is that Mergify auto rebase adds a merge commit
> > to the set of commits in the PR.  The merge commit is not added to the base branch
> > when it is done.  Since the merge commit is present in the PR, the patch check fails.
> > We need to update patch check to ignore merge commits by the Mergify agent.  I have
> > temporarily disabled the patch check CI status check in edk2-codereview to
> > work around this issue.  Once I have a fix for patch check, I will re-enable the
> > patch check status check.
> >
> > I also think this approach will fix the issue that has been seen where Mergify merged
> > before all the platform tests were completed.  We just need to make sure the
> > platform checks are in the list of enabled status checks in the GitHub protected
> > branch configuration.  I will verify this issue is resolved using the edk2-codereview
> > repo.
> >
> > Best regards,
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sean
> > > Sent: Wednesday, June 16, 2021 12:00 PM
> > > To: ardb@kernel.org; Kinney, Michael D <michael.d.kinney@intel.com>
> > > Cc: Peter Grehan <grehan@freebsd.org>; Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> > > Justen, Jordan L <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran
> <rebecca@bsdio.com>;
> > > devel@edk2.groups.io
> > > Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
> > >
> > > Ard,
> > >
> > > The PR you are trying to "push" has a mergify merge in it which is
> > > causing patchcheck to fail.
> > >
> > > https://github.com/tianocore/edk2/pull/1727/commits
> > >
> > >
> > >
> > > Mike,
> > >
> > > I think github has better features now around things like auto complete
> > > PR when "checks pass" and allow rebase merging and finally protections
> > > to only allow linear history.  Might be time to talk about changes to
> > > mergify/github.  I know you are busy so maybe opening up to more of the
> > > edk2 community or actively looking for someone willing to provide best
> > > practices for github usage (rust-lang and nodejs both do a lot with
> > > github).
> > >
> > >
> > > Thanks
> > > Sean
> > >
> > >
> > >
> > >
> > >
> > > On 6/16/2021 8:58 AM, Ard Biesheuvel wrote:
> > > > (+ Sean, Mike)
> > > >
> > > > On Sat, 12 Jun 2021 at 22:43, Rebecca Cran <rebecca@bsdio.com> wrote:
> > > >>
> > > >> TPM support hasn't been tested and any lines in the .dsc and .fdf files
> > > >> that appear to show support are bogus. Remove them.
> > > >>
> > > >> This fixes https://bugzilla.tianocore.org/show_bug.cgi?id=3354 .
> > > >>
> > > >> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> > > >
> > > > Strangely enough, this patch gets rejected by PatchCheck for lack of a
> > > > Signed-off-by line
> > > >
> > > > https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=24198&view=results
> > > >
> > > > The patch itself looks good to me
> > > >
> > > > Acked-by: Ard Biesheuvel <ardb@kernel.org>
> > > >
> > > > so if anyone else manages to fix the CI issue, feel free to push the
> > > > patch with my R-b (and Peter's, given in reply to this message)
> > > >
> > > > (I will go offline for 3 weeks after Friday)
> > > >
> > > >> ---
> > > >>   OvmfPkg/Bhyve/BhyveX64.dsc | 64 --------------------
> > > >>   OvmfPkg/Bhyve/BhyveX64.fdf | 15 -----
> > > >>   2 files changed, 79 deletions(-)
> > > >>
> > > >> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
> > > >> index d8792812ab..cbf896e89b 100644
> > > >> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
> > > >> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
> > > >> @@ -31,8 +31,6 @@
> > > >>     DEFINE SECURE_BOOT_ENABLE      = FALSE
> > > >>     DEFINE SMM_REQUIRE             = FALSE
> > > >>     DEFINE SOURCE_DEBUG_ENABLE     = FALSE
> > > >> -  DEFINE TPM_ENABLE              = FALSE
> > > >> -  DEFINE TPM_CONFIG_ENABLE       = FALSE
> > > >>
> > > >>     #
> > > >>     # Network definition
> > > >> @@ -221,16 +219,8 @@
> > > >>
> OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
> > > >>     XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf
> > > >>
> > > >> -
> > > >> -!if $(TPM_ENABLE) == TRUE
> > > >> -  Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
> > > >> -  Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
> > > >> -  Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
> > > >> -  TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> > > >> -!else
> > > >>     Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
> > > >>     TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
> > > >> -!endif
> > > >>
> > > >>   [LibraryClasses.common]
> > > >>     BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> > > >> @@ -292,11 +282,6 @@
> > > >>     CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
> > > >>     PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
> > > >>
> > > >> -!if $(TPM_ENABLE) == TRUE
> > > >> -  BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> > > >> -  Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
> > > >> -!endif
> > > >> -
> > > >>     MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
> > > >>
> > > >>   [LibraryClasses.common.DXE_CORE]
> > > >> @@ -366,9 +351,6 @@
> > > >>   !endif
> > > >>     PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
> > > >>     MpInitLib|UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf
> > > >> -!if $(TPM_ENABLE) == TRUE
> > > >> -  Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
> > > >> -!endif
> > > >>
> > > >>   [LibraryClasses.common.UEFI_APPLICATION]
> > > >>     PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> > > >> @@ -563,22 +545,12 @@
> > > >>
> > > >>     gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
> > > >>
> > > >> -!if $(TPM_ENABLE) == TRUE
> > > >> -  gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > 0x00,
> > > 0x00, 0x00, 0x00, 0x00, 0x00}
> > > >> -!endif
> > > >> -
> > > >>     # MdeModulePkg resolution sets up the system display resolution
> > > >>     gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution|0
> > > >>     gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution|0
> > > >>     gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|0
> > > >>     gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|0
> > > >>
> > > >> -[PcdsDynamicHii]
> > > >> -!if $(TPM_ENABLE) == TRUE && $(TPM_CONFIG_ENABLE) == TRUE
> > > >> -
> > >
> gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
> > > >> -  gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
> > > >> -!endif
> > > >> -
> > > >>   ################################################################################
> > > >>   #
> > > >>   # Components Section - list of all EDK II Modules needed by this Platform.
> > > >> @@ -618,19 +590,6 @@
> > > >>       <LibraryClasses>
> > > >>     }
> > > >>
> > > >> -!if $(TPM_ENABLE) == TRUE
> > > >> -  OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> > > >> -  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> > > >> -    <LibraryClasses>
> > > >> -      HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
> > > >> -      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> > > >> -      NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> > > >> -      NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> > > >> -      NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> > > >> -      NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> > > >> -  }
> > > >> -!endif
> > > >> -
> > > >>     #
> > > >>     # DXE Phase modules
> > > >>     #
> > > >> @@ -653,9 +612,6 @@
> > > >>       <LibraryClasses>
> > > >>   !if $(SECURE_BOOT_ENABLE) == TRUE
> > > >>         NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> > > >> -!endif
> > > >> -!if $(TPM_ENABLE) == TRUE
> > > >> -      NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
> > > >>   !endif
> > > >>     }
> > > >>
> > > >> @@ -841,23 +797,3 @@
> > > >>         NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
> > > >>     }
> > > >>
> > > >> -
> > > >> -  #
> > > >> -  # TPM support
> > > >> -  #
> > > >> -!if $(TPM_ENABLE) == TRUE
> > > >> -  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
> > > >> -    <LibraryClasses>
> > > >> -      Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
> > > >> -      NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
> > > >> -      HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
> > > >> -      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> > > >> -      NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> > > >> -      NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> > > >> -      NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> > > >> -      NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> > > >> -  }
> > > >> -!if $(TPM_CONFIG_ENABLE) == TRUE
> > > >> -  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> > > >> -!endif
> > > >> -!endif
> > > >> diff --git a/OvmfPkg/Bhyve/BhyveX64.fdf b/OvmfPkg/Bhyve/BhyveX64.fdf
> > > >> index 3eff36dac1..fbd63a395a 100644
> > > >> --- a/OvmfPkg/Bhyve/BhyveX64.fdf
> > > >> +++ b/OvmfPkg/Bhyve/BhyveX64.fdf
> > > >> @@ -158,11 +158,6 @@ INF  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> > > >>   INF  OvmfPkg/Bhyve/SmmAccess/SmmAccessPei.inf
> > > >>   !endif
> > > >>
> > > >> -!if $(TPM_ENABLE) == TRUE
> > > >> -INF  OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> > > >> -INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> > > >> -!endif
> > > >> -
> > > >>   ################################################################################
> > > >>
> > > >>   [FV.DXEFV]
> > > >> @@ -333,16 +328,6 @@ INF  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
> > > >>   INF  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> > > >>   !endif
> > > >>
> > > >> -#
> > > >> -# TPM support
> > > >> -#
> > > >> -!if $(TPM_ENABLE) == TRUE
> > > >> -INF  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
> > > >> -!if $(TPM_CONFIG_ENABLE) == TRUE
> > > >> -INF  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> > > >> -!endif
> > > >> -!endif
> > > >> -
> > > >>   ################################################################################
> > > >>
> > > >>   [FV.FVMAIN_COMPACT]
> > > >> --
> > > >> 2.32.0
> > > >>
> > > >>
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > > 
> > >



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

Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Posted by Laszlo Ersek 2 years, 10 months ago

On 06/17/21 23:53, Kinney, Michael D wrote:
> Hi Sean,
> 
> Mergify had added a queue feature to handle the rebases automatically and make sure
> CI passes in the order that the PRs are being applied to the base branch.

I'm opposed to *unconditional* auto-rebase.

On one hand, it is indeed unreasonable to require a human to manually
rebase a "ShellPkg/Application/AcpiViewApp" series just because a series
for "SecurityPkg/FvReportPei" was merged a bit earlier. In other words,
merge requests for unrelated modules should not block each other.

On the other hand, auto-rebase is a bad idea if both series modify at
least one module in common (especially if both series modify at least
one *file* in common). In case there is a contextual conflict, even if
the conflict can be auto-resolved, and even if that resolution
*compiles*, it has to be reviewed by a human first.

I regularly use the git-range-diff command for this.

At Red Hat we've seen obscure bugs due to silent mis-merges (not in edk2
-- in different packages); such issues are difficult to debug.

Bisectability helps for sure, but only if the community treats
bisectability with high priority in the first place. (That is, if every
contributor builds their patch set at every stage, before submitting it
for review.)

Can we restrict the auto-rebase feature to such merge requests whose
cumulative diffstats do not intersect?

Thanks,
Laszlo

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

Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Posted by Michael D Kinney 2 years, 10 months ago

Hi Laszlo,

I am trying the following configuration that is very conservative: 

    actions:
      queue:
        method: rebase
        rebase_fallback: none
        name: default

The auto rebase only attempts a strict rebase.  If that attempt at a strict rebase fails
then it will show that there is a conflict that the developer must take care of.

I believe any combination of 2 PRs that have overlapping diff stat should fail
a strict rebase.  The following link describes the method and rebase_fallback
settings in the queue command.

	https://docs.mergify.io/actions/queue/#id2

I would be more concerned if we used a method of merge or a rebase_fallback of 
merge.

Are there examples you can think of where the diff stat overlap and the strict
rebase will succeed?

Another option to consider is to define an additional 'auto-rebase' label that is
off by default to enable the auto rebase feature.  By default the PR must be synced
with head when submitted.  Only if a maintainer sets the 'auto-rebase' label will
an auto-rebase be attempted.  

I also want to make it easy for non-maintainers to submit PRs and get CI test results.
So auto rebase may be useful for that use case.  Perhaps the 'auto-rebase' label 
can be considered when the 'push' label is also set.

Thanks,

Mike

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, June 22, 2021 8:17 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
> Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
> Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
> 
> On 06/17/21 23:53, Kinney, Michael D wrote:
> > Hi Sean,
> >
> > Mergify had added a queue feature to handle the rebases automatically and make sure
> > CI passes in the order that the PRs are being applied to the base branch.
> 
> I'm opposed to *unconditional* auto-rebase.
> 
> On one hand, it is indeed unreasonable to require a human to manually
> rebase a "ShellPkg/Application/AcpiViewApp" series just because a series
> for "SecurityPkg/FvReportPei" was merged a bit earlier. In other words,
> merge requests for unrelated modules should not block each other.
> 
> On the other hand, auto-rebase is a bad idea if both series modify at
> least one module in common (especially if both series modify at least
> one *file* in common). In case there is a contextual conflict, even if
> the conflict can be auto-resolved, and even if that resolution
> *compiles*, it has to be reviewed by a human first.
> 
> I regularly use the git-range-diff command for this.
> 
> At Red Hat we've seen obscure bugs due to silent mis-merges (not in edk2
> -- in different packages); such issues are difficult to debug.
> 
> Bisectability helps for sure, but only if the community treats
> bisectability with high priority in the first place. (That is, if every
> contributor builds their patch set at every stage, before submitting it
> for review.)
> 
> Can we restrict the auto-rebase feature to such merge requests whose
> cumulative diffstats do not intersect?
> 
> Thanks,
> Laszlo


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

Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Posted by Laszlo Ersek 2 years, 10 months ago

On 06/22/21 17:38, Kinney, Michael D wrote:
> Hi Laszlo,
>
> I am trying the following configuration that is very conservative:
>
>     actions:
>       queue:
>         method: rebase
>         rebase_fallback: none
>         name: default
>
> The auto rebase only attempts a strict rebase.  If that attempt at a
> strict rebase fails then it will show that there is a conflict that
> the developer must take care of.
>
> I believe any combination of 2 PRs that have overlapping diff stat
> should fail a strict rebase.  The following link describes the method
> and rebase_fallback settings in the queue command.
>
> 	https://docs.mergify.io/actions/queue/#id2
>
> I would be more concerned if we used a method of merge or a
> rebase_fallback of merge.
>
> Are there examples you can think of where the diff stat overlap and
> the strict rebase will succeed?

I've read the strict rebase definition and the above link in the mergify
documentation, but I'm none the wiser.

Consider the following test case (with master @ 7471751a4d81):

  git checkout -b b1 master
  git am b1.patch           # attached
  git checkout -b b2 master
  git am b2.patch           # attached
  git branch b2-rebase b2
  git rebase b1 b2-rebase

Locally, this produces the following message for me:

> First, rewinding head to replay your work on top of it...
> Applying: world
> Using index info to reconstruct a base tree...
> M       ReadMe.rst
> Falling back to patching base and 3-way merge...
> Auto-merging ReadMe.rst

The rebase succeeds and produces the expected result, but that result is
*exactly* what a human should review.

I don't know if mergify catches the above. While the rebase succeeds
locally, it should not succeed in mergify.

Using the "git rebase -i" (interactive) command, which uses a different
rebase backend (based on git-cherry-pick, not on git-am), and specifying
a single "pick" command, the rebase still succeeds; this time without
producing any diagnostic messages even. So from an auto-rebase
perspective, it's even less desirable.

Thanks
Laszlo

>
> Another option to consider is to define an additional 'auto-rebase' label that is
> off by default to enable the auto rebase feature.  By default the PR must be synced
> with head when submitted.  Only if a maintainer sets the 'auto-rebase' label will
> an auto-rebase be attempted.
>
> I also want to make it easy for non-maintainers to submit PRs and get CI test results.
> So auto rebase may be useful for that use case.  Perhaps the 'auto-rebase' label
> can be considered when the 'push' label is also set.
>
> Thanks,
>
> Mike
>
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Tuesday, June 22, 2021 8:17 AM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
>> Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
>> <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
>> Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
>>
>> On 06/17/21 23:53, Kinney, Michael D wrote:
>>> Hi Sean,
>>>
>>> Mergify had added a queue feature to handle the rebases automatically and make sure
>>> CI passes in the order that the PRs are being applied to the base branch.
>>
>> I'm opposed to *unconditional* auto-rebase.
>>
>> On one hand, it is indeed unreasonable to require a human to manually
>> rebase a "ShellPkg/Application/AcpiViewApp" series just because a series
>> for "SecurityPkg/FvReportPei" was merged a bit earlier. In other words,
>> merge requests for unrelated modules should not block each other.
>>
>> On the other hand, auto-rebase is a bad idea if both series modify at
>> least one module in common (especially if both series modify at least
>> one *file* in common). In case there is a contextual conflict, even if
>> the conflict can be auto-resolved, and even if that resolution
>> *compiles*, it has to be reviewed by a human first.
>>
>> I regularly use the git-range-diff command for this.
>>
>> At Red Hat we've seen obscure bugs due to silent mis-merges (not in edk2
>> -- in different packages); such issues are difficult to debug.
>>
>> Bisectability helps for sure, but only if the community treats
>> bisectability with high priority in the first place. (That is, if every
>> contributor builds their patch set at every stage, before submitting it
>> for review.)
>>
>> Can we restrict the auto-rebase feature to such merge requests whose
>> cumulative diffstats do not intersect?
>>
>> Thanks,
>> Laszlo



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

Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Posted by Michael D Kinney 2 years, 10 months ago

Hi Laszlo,

Thank you for the test case.

I created 2 PRs against edk2-codereview using your patches.  
I made minor update to commit messages to pass patch check.

    https://github.com/tianocore/edk2-codereview/pull/18
    https://github.com/tianocore/edk2-codereview/pull/19

Found another issue with PatchCheck for the Mergify merge commit and fixed that.

Mergify did process #18 and merged it in after passing all CI.  Mergify rebased
#19 successfully and merged it after passing all CI.  I do not think this was
your expected result.

I looked more closely at the patches you provided.  They were not overlapping 
in the lines of Readme.rst.  This is why no merge conflict was detected.

I then created 2 new PRs that added text to the same line # in Readme.rst.

    https://github.com/tianocore/edk2-codereview/pull/21
    https://github.com/tianocore/edk2-codereview/pull/22

PR #21 passed all CI tests and was merged.  Mergify then attempted to
rebase #22 and got a merge conflict and is still in the open state waiting
for the developer to manually handle the merge conflict.

Mike

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, June 23, 2021 8:17 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
> Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
> Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
> 
> On 06/22/21 17:38, Kinney, Michael D wrote:
> > Hi Laszlo,
> >
> > I am trying the following configuration that is very conservative:
> >
> >     actions:
> >       queue:
> >         method: rebase
> >         rebase_fallback: none
> >         name: default
> >
> > The auto rebase only attempts a strict rebase.  If that attempt at a
> > strict rebase fails then it will show that there is a conflict that
> > the developer must take care of.
> >
> > I believe any combination of 2 PRs that have overlapping diff stat
> > should fail a strict rebase.  The following link describes the method
> > and rebase_fallback settings in the queue command.
> >
> > 	https://docs.mergify.io/actions/queue/#id2
> >
> > I would be more concerned if we used a method of merge or a
> > rebase_fallback of merge.
> >
> > Are there examples you can think of where the diff stat overlap and
> > the strict rebase will succeed?
> 
> I've read the strict rebase definition and the above link in the mergify
> documentation, but I'm none the wiser.
> 
> Consider the following test case (with master @ 7471751a4d81):
> 
>   git checkout -b b1 master
>   git am b1.patch           # attached
>   git checkout -b b2 master
>   git am b2.patch           # attached
>   git branch b2-rebase b2
>   git rebase b1 b2-rebase
> 
> Locally, this produces the following message for me:
> 
> > First, rewinding head to replay your work on top of it...
> > Applying: world
> > Using index info to reconstruct a base tree...
> > M       ReadMe.rst
> > Falling back to patching base and 3-way merge...
> > Auto-merging ReadMe.rst
> 
> The rebase succeeds and produces the expected result, but that result is
> *exactly* what a human should review.
> 
> I don't know if mergify catches the above. While the rebase succeeds
> locally, it should not succeed in mergify.
> 
> Using the "git rebase -i" (interactive) command, which uses a different
> rebase backend (based on git-cherry-pick, not on git-am), and specifying
> a single "pick" command, the rebase still succeeds; this time without
> producing any diagnostic messages even. So from an auto-rebase
> perspective, it's even less desirable.
> 
> Thanks
> Laszlo
> 
> >
> > Another option to consider is to define an additional 'auto-rebase' label that is
> > off by default to enable the auto rebase feature.  By default the PR must be synced
> > with head when submitted.  Only if a maintainer sets the 'auto-rebase' label will
> > an auto-rebase be attempted.
> >
> > I also want to make it easy for non-maintainers to submit PRs and get CI test results.
> > So auto rebase may be useful for that use case.  Perhaps the 'auto-rebase' label
> > can be considered when the 'push' label is also set.
> >
> > Thanks,
> >
> > Mike
> >
> >> -----Original Message-----
> >> From: Laszlo Ersek <lersek@redhat.com>
> >> Sent: Tuesday, June 22, 2021 8:17 AM
> >> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
> >> Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
> >> <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
> >> Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
> >>
> >> On 06/17/21 23:53, Kinney, Michael D wrote:
> >>> Hi Sean,
> >>>
> >>> Mergify had added a queue feature to handle the rebases automatically and make sure
> >>> CI passes in the order that the PRs are being applied to the base branch.
> >>
> >> I'm opposed to *unconditional* auto-rebase.
> >>
> >> On one hand, it is indeed unreasonable to require a human to manually
> >> rebase a "ShellPkg/Application/AcpiViewApp" series just because a series
> >> for "SecurityPkg/FvReportPei" was merged a bit earlier. In other words,
> >> merge requests for unrelated modules should not block each other.
> >>
> >> On the other hand, auto-rebase is a bad idea if both series modify at
> >> least one module in common (especially if both series modify at least
> >> one *file* in common). In case there is a contextual conflict, even if
> >> the conflict can be auto-resolved, and even if that resolution
> >> *compiles*, it has to be reviewed by a human first.
> >>
> >> I regularly use the git-range-diff command for this.
> >>
> >> At Red Hat we've seen obscure bugs due to silent mis-merges (not in edk2
> >> -- in different packages); such issues are difficult to debug.
> >>
> >> Bisectability helps for sure, but only if the community treats
> >> bisectability with high priority in the first place. (That is, if every
> >> contributor builds their patch set at every stage, before submitting it
> >> for review.)
> >>
> >> Can we restrict the auto-rebase feature to such merge requests whose
> >> cumulative diffstats do not intersect?
> >>
> >> Thanks,
> >> Laszlo



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

Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Posted by Laszlo Ersek 2 years, 10 months ago

On 06/23/21 20:44, Kinney, Michael D wrote:
>
> Hi Laszlo,
>
> Thank you for the test case.
>
> I created 2 PRs against edk2-codereview using your patches.
> I made minor update to commit messages to pass patch check.
>
>     https://github.com/tianocore/edk2-codereview/pull/18
>     https://github.com/tianocore/edk2-codereview/pull/19
>
> Found another issue with PatchCheck for the Mergify merge commit and
> fixed that.
>
> Mergify did process #18 and merged it in after passing all CI. Mergify
> rebased #19 successfully and merged it after passing all CI. I do not
> think this was your expected result.

Indeed, my "desired" result at least would have been for mergify to
reject the rebase.

> I looked more closely at the patches you provided.  They were not
> overlapping in the lines of Readme.rst.  This is why no merge conflict
> was detected.

More precisely, a contextual conflict *was* determined between the
patches, but that conflict was auto-resolved.

This is risky when done in an automated fashion. It is an extremely
convenient feature of git, when used interactively; that is, when the
auto-merge (automatic conflict resolution) is semantically verified by a
human. Git takes away the chore of conflict resolution, presents a
"likely good" end result, and a human only needs to *look* at the end
result, not *implement* it.

But that "human look" is exactly what's missing from mergify.

Basically what I'd like for mergify is to turn off automatic conflict
resolution.

More or less, speaking in terms of the stand-alone "patch" utility
<https://man7.org/linux/man-pages/man1/patch.1.html>, my preference is
to set the "fuzz factor" to zero.

One way a human reviews such context differences is with git-range-diff.
Continuing my previous example commands:

$ git range-diff --color master..b2 b1..b2-rebase

1:  02dc81e58bd6 ! 1:  2cf39d4b1790 world
    @@ -6,8 +6,8 @@
     --- a/ReadMe.rst
     +++ b/ReadMe.rst
     @@
    -
      A modern, feature-rich, cross-platform firmware development
    +   HELLO
      environment for the UEFI and PI specifications from www.uefi.org.
     +  WORLD

This output shows that the "world" addition is the same (it is identical
between pre-rebase and post-rebase in the commit), but the context has
changed. During the rebase, the leading empty line of the context
disappeared, and a HELLO line in the middle of the leading context
appeared.

This result may or may not be semantically correct; it needs a human
decision. What if the original purpose of the "world" patch author was
to say WORLD but only without HELLO? When they looked at the code, there
was no HELLO yet.

git-range-diff is very powerful, but reading its output takes some
getting used to. (Colorization with the "--color" option is basically
required for understanding; I can't reproduce it in this email, alas.)

I don't want to obsess about this forever, I just want us all to be
aware that this risk exists.

Thanks,
Laszlo

>
> I then created 2 new PRs that added text to the same line # in Readme.rst.
>
>     https://github.com/tianocore/edk2-codereview/pull/21
>     https://github.com/tianocore/edk2-codereview/pull/22
>
> PR #21 passed all CI tests and was merged.  Mergify then attempted to
> rebase #22 and got a merge conflict and is still in the open state waiting
> for the developer to manually handle the merge conflict.

This case is not worrisome; when there is a clear conflict that cannot be auto-resolved, I'm not concerned.

My concern is the sneaky contextual conflict that *appears* auto-resolvable, but is semantically broken. Those things exist.

Thanks
Laszlo

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

Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Posted by Michael D Kinney 2 years, 10 months ago

Hi Laszlo,

I understand your point. 

I am trying to balance the ease of use for developers, reducing overhead for maintainers, and
prevent bad commits.

I think you are saying that you want to make sure a maintainer carefully reviews changes
across multiple PRs that are in the same area of code.  The CI checks will of course make
sure the code builds and passes the basic boot tests, but those tests do not have full
coverage so an interaction issue between two PRs that pass build and boot but have 
unintended behavior side effects are what require detailed manual review.

I am going to remove the auto-rebase by default and add a optional label that can
be set by a maintainer to enable auto-rebase.  If a maintainer is confident that 
a set of PRs being submitted at the same time with the 'push' label are independent,
then the maintainer can also set 'auto-rebase'.  If they are not confident, then 
they can send PRs one at a time with only 'push' label and manually rebase each
additional PR and review the manual rebase to make sure there are no unintended
side effects.

Any objections to this direction?

Thanks,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Wednesday, June 23, 2021 12:45 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
> Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
> Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
> 
> On 06/23/21 20:44, Kinney, Michael D wrote:
> >
> > Hi Laszlo,
> >
> > Thank you for the test case.
> >
> > I created 2 PRs against edk2-codereview using your patches.
> > I made minor update to commit messages to pass patch check.
> >
> >     https://github.com/tianocore/edk2-codereview/pull/18
> >     https://github.com/tianocore/edk2-codereview/pull/19
> >
> > Found another issue with PatchCheck for the Mergify merge commit and
> > fixed that.
> >
> > Mergify did process #18 and merged it in after passing all CI. Mergify
> > rebased #19 successfully and merged it after passing all CI. I do not
> > think this was your expected result.
> 
> Indeed, my "desired" result at least would have been for mergify to
> reject the rebase.
> 
> > I looked more closely at the patches you provided.  They were not
> > overlapping in the lines of Readme.rst.  This is why no merge conflict
> > was detected.
> 
> More precisely, a contextual conflict *was* determined between the
> patches, but that conflict was auto-resolved.
> 
> This is risky when done in an automated fashion. It is an extremely
> convenient feature of git, when used interactively; that is, when the
> auto-merge (automatic conflict resolution) is semantically verified by a
> human. Git takes away the chore of conflict resolution, presents a
> "likely good" end result, and a human only needs to *look* at the end
> result, not *implement* it.
> 
> But that "human look" is exactly what's missing from mergify.
> 
> Basically what I'd like for mergify is to turn off automatic conflict
> resolution.
> 
> More or less, speaking in terms of the stand-alone "patch" utility
> <https://man7.org/linux/man-pages/man1/patch.1.html>, my preference is
> to set the "fuzz factor" to zero.
> 
> 
> One way a human reviews such context differences is with git-range-diff.
> Continuing my previous example commands:
> 
> $ git range-diff --color master..b2 b1..b2-rebase
> 
> 1:  02dc81e58bd6 ! 1:  2cf39d4b1790 world
>     @@ -6,8 +6,8 @@
>      --- a/ReadMe.rst
>      +++ b/ReadMe.rst
>      @@
>     -
>       A modern, feature-rich, cross-platform firmware development
>     +   HELLO
>       environment for the UEFI and PI specifications from www.uefi.org.
>      +  WORLD
> 
> This output shows that the "world" addition is the same (it is identical
> between pre-rebase and post-rebase in the commit), but the context has
> changed. During the rebase, the leading empty line of the context
> disappeared, and a HELLO line in the middle of the leading context
> appeared.
> 
> This result may or may not be semantically correct; it needs a human
> decision. What if the original purpose of the "world" patch author was
> to say WORLD but only without HELLO? When they looked at the code, there
> was no HELLO yet.
> 
> git-range-diff is very powerful, but reading its output takes some
> getting used to. (Colorization with the "--color" option is basically
> required for understanding; I can't reproduce it in this email, alas.)
> 
> I don't want to obsess about this forever, I just want us all to be
> aware that this risk exists.
> 
> Thanks,
> Laszlo
> 
> >
> > I then created 2 new PRs that added text to the same line # in Readme.rst.
> >
> >     https://github.com/tianocore/edk2-codereview/pull/21
> >     https://github.com/tianocore/edk2-codereview/pull/22
> >
> > PR #21 passed all CI tests and was merged.  Mergify then attempted to
> > rebase #22 and got a merge conflict and is still in the open state waiting
> > for the developer to manually handle the merge conflict.
> 
> This case is not worrisome; when there is a clear conflict that cannot be auto-resolved, I'm not concerned.
> 
> My concern is the sneaky contextual conflict that *appears* auto-resolvable, but is semantically broken. Those things
> exist.
> 
> Thanks
> Laszlo
> 
> 
> 
> 
> 



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

回复: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Posted by gaoliming 2 years, 10 months ago

Mike:
  If 'auto-rebase' label is not set, the behavior is still same to current one. Right? 
  If yes, I agree this proposal. The maintainer can optionally set 'auto-rebase'.

Thanks
Liming
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Michael D
> Kinney
> 发送时间: 2021年6月24日 6:08
> 收件人: devel@edk2.groups.io; lersek@redhat.com; spbrogan@outlook.com;
> ardb@kernel.org; Kinney, Michael D <michael.d.kinney@intel.com>
> 抄送: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran
> <rebecca@bsdio.com>
> 主题: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE
> remnants
> 
> Hi Laszlo,
> 
> I understand your point.
> 
> I am trying to balance the ease of use for developers, reducing overhead for
> maintainers, and
> prevent bad commits.
> 
> I think you are saying that you want to make sure a maintainer carefully
> reviews changes
> across multiple PRs that are in the same area of code.  The CI checks will of
> course make
> sure the code builds and passes the basic boot tests, but those tests do not
> have full
> coverage so an interaction issue between two PRs that pass build and boot
> but have
> unintended behavior side effects are what require detailed manual review.
> 
> I am going to remove the auto-rebase by default and add a optional label that
> can
> be set by a maintainer to enable auto-rebase.  If a maintainer is confident
> that
> a set of PRs being submitted at the same time with the 'push' label are
> independent,
> then the maintainer can also set 'auto-rebase'.  If they are not confident,
> then
> they can send PRs one at a time with only 'push' label and manually rebase
> each
> additional PR and review the manual rebase to make sure there are no
> unintended
> side effects.
> 
> Any objections to this direction?
> 
> Thanks,
> 
> Mike
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> Ersek
> > Sent: Wednesday, June 23, 2021 12:45 PM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
> > Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Justen, Jordan L
> > <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>;
> Rebecca Cran <rebecca@bsdio.com>
> > Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE
> remnants
> >
> > On 06/23/21 20:44, Kinney, Michael D wrote:
> > >
> > > Hi Laszlo,
> > >
> > > Thank you for the test case.
> > >
> > > I created 2 PRs against edk2-codereview using your patches.
> > > I made minor update to commit messages to pass patch check.
> > >
> > >     https://github.com/tianocore/edk2-codereview/pull/18
> > >     https://github.com/tianocore/edk2-codereview/pull/19
> > >
> > > Found another issue with PatchCheck for the Mergify merge commit and
> > > fixed that.
> > >
> > > Mergify did process #18 and merged it in after passing all CI. Mergify
> > > rebased #19 successfully and merged it after passing all CI. I do not
> > > think this was your expected result.
> >
> > Indeed, my "desired" result at least would have been for mergify to
> > reject the rebase.
> >
> > > I looked more closely at the patches you provided.  They were not
> > > overlapping in the lines of Readme.rst.  This is why no merge conflict
> > > was detected.
> >
> > More precisely, a contextual conflict *was* determined between the
> > patches, but that conflict was auto-resolved.
> >
> > This is risky when done in an automated fashion. It is an extremely
> > convenient feature of git, when used interactively; that is, when the
> > auto-merge (automatic conflict resolution) is semantically verified by a
> > human. Git takes away the chore of conflict resolution, presents a
> > "likely good" end result, and a human only needs to *look* at the end
> > result, not *implement* it.
> >
> > But that "human look" is exactly what's missing from mergify.
> >
> > Basically what I'd like for mergify is to turn off automatic conflict
> > resolution.
> >
> > More or less, speaking in terms of the stand-alone "patch" utility
> > <https://man7.org/linux/man-pages/man1/patch.1.html>, my preference is
> > to set the "fuzz factor" to zero.
> >
> >
> > One way a human reviews such context differences is with git-range-diff.
> > Continuing my previous example commands:
> >
> > $ git range-diff --color master..b2 b1..b2-rebase
> >
> > 1:  02dc81e58bd6 ! 1:  2cf39d4b1790 world
> >     @@ -6,8 +6,8 @@
> >      --- a/ReadMe.rst
> >      +++ b/ReadMe.rst
> >      @@
> >     -
> >       A modern, feature-rich, cross-platform firmware development
> >     +   HELLO
> >       environment for the UEFI and PI specifications from www.uefi.org.
> >      +  WORLD
> >
> > This output shows that the "world" addition is the same (it is identical
> > between pre-rebase and post-rebase in the commit), but the context has
> > changed. During the rebase, the leading empty line of the context
> > disappeared, and a HELLO line in the middle of the leading context
> > appeared.
> >
> > This result may or may not be semantically correct; it needs a human
> > decision. What if the original purpose of the "world" patch author was
> > to say WORLD but only without HELLO? When they looked at the code,
> there
> > was no HELLO yet.
> >
> > git-range-diff is very powerful, but reading its output takes some
> > getting used to. (Colorization with the "--color" option is basically
> > required for understanding; I can't reproduce it in this email, alas.)
> >
> > I don't want to obsess about this forever, I just want us all to be
> > aware that this risk exists.
> >
> > Thanks,
> > Laszlo
> >
> > >
> > > I then created 2 new PRs that added text to the same line # in
> Readme.rst.
> > >
> > >     https://github.com/tianocore/edk2-codereview/pull/21
> > >     https://github.com/tianocore/edk2-codereview/pull/22
> > >
> > > PR #21 passed all CI tests and was merged.  Mergify then attempted to
> > > rebase #22 and got a merge conflict and is still in the open state waiting
> > > for the developer to manually handle the merge conflict.
> >
> > This case is not worrisome; when there is a clear conflict that cannot be
> auto-resolved, I'm not concerned.
> >
> > My concern is the sneaky contextual conflict that *appears* auto-resolvable,
> but is semantically broken. Those things
> > exist.
> >
> > Thanks
> > Laszlo
> >
> >
> >
> >
> >
> 
> 
> 
> 
> 





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

Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Posted by Michael D Kinney 2 years, 10 months ago

Liming,

Yes.  I am working on a configuration with will keep the current 'push' behavior
and add the 'auto-rebase' option.

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of gaoliming
> Sent: Wednesday, June 23, 2021 6:10 PM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>; lersek@redhat.com; spbrogan@outlook.com;
> ardb@kernel.org
> Cc: 'Peter Grehan' <grehan@freebsd.org>; 'Ard Biesheuvel' <ardb+tianocore@kernel.org>; Justen, Jordan L
> <jordan.l.justen@intel.com>; 'Sean Brogan' <sean.brogan@microsoft.com>; 'Rebecca Cran' <rebecca@bsdio.com>
> Subject: 回复: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
> 
> Mike:
>   If 'auto-rebase' label is not set, the behavior is still same to current one. Right?
>   If yes, I agree this proposal. The maintainer can optionally set 'auto-rebase'.
> 
> Thanks
> Liming
> > -----邮件原件-----
> > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Michael D
> > Kinney
> > 发送时间: 2021年6月24日 6:08
> > 收件人: devel@edk2.groups.io; lersek@redhat.com; spbrogan@outlook.com;
> > ardb@kernel.org; Kinney, Michael D <michael.d.kinney@intel.com>
> > 抄送: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel
> > <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
> > Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran
> > <rebecca@bsdio.com>
> > 主题: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE
> > remnants
> >
> > Hi Laszlo,
> >
> > I understand your point.
> >
> > I am trying to balance the ease of use for developers, reducing overhead for
> > maintainers, and
> > prevent bad commits.
> >
> > I think you are saying that you want to make sure a maintainer carefully
> > reviews changes
> > across multiple PRs that are in the same area of code.  The CI checks will of
> > course make
> > sure the code builds and passes the basic boot tests, but those tests do not
> > have full
> > coverage so an interaction issue between two PRs that pass build and boot
> > but have
> > unintended behavior side effects are what require detailed manual review.
> >
> > I am going to remove the auto-rebase by default and add a optional label that
> > can
> > be set by a maintainer to enable auto-rebase.  If a maintainer is confident
> > that
> > a set of PRs being submitted at the same time with the 'push' label are
> > independent,
> > then the maintainer can also set 'auto-rebase'.  If they are not confident,
> > then
> > they can send PRs one at a time with only 'push' label and manually rebase
> > each
> > additional PR and review the manual rebase to make sure there are no
> > unintended
> > side effects.
> >
> > Any objections to this direction?
> >
> > Thanks,
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> > Ersek
> > > Sent: Wednesday, June 23, 2021 12:45 PM
> > > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> > devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
> > > Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel
> > <ardb+tianocore@kernel.org>; Justen, Jordan L
> > > <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>;
> > Rebecca Cran <rebecca@bsdio.com>
> > > Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE
> > remnants
> > >
> > > On 06/23/21 20:44, Kinney, Michael D wrote:
> > > >
> > > > Hi Laszlo,
> > > >
> > > > Thank you for the test case.
> > > >
> > > > I created 2 PRs against edk2-codereview using your patches.
> > > > I made minor update to commit messages to pass patch check.
> > > >
> > > >     https://github.com/tianocore/edk2-codereview/pull/18
> > > >     https://github.com/tianocore/edk2-codereview/pull/19
> > > >
> > > > Found another issue with PatchCheck for the Mergify merge commit and
> > > > fixed that.
> > > >
> > > > Mergify did process #18 and merged it in after passing all CI. Mergify
> > > > rebased #19 successfully and merged it after passing all CI. I do not
> > > > think this was your expected result.
> > >
> > > Indeed, my "desired" result at least would have been for mergify to
> > > reject the rebase.
> > >
> > > > I looked more closely at the patches you provided.  They were not
> > > > overlapping in the lines of Readme.rst.  This is why no merge conflict
> > > > was detected.
> > >
> > > More precisely, a contextual conflict *was* determined between the
> > > patches, but that conflict was auto-resolved.
> > >
> > > This is risky when done in an automated fashion. It is an extremely
> > > convenient feature of git, when used interactively; that is, when the
> > > auto-merge (automatic conflict resolution) is semantically verified by a
> > > human. Git takes away the chore of conflict resolution, presents a
> > > "likely good" end result, and a human only needs to *look* at the end
> > > result, not *implement* it.
> > >
> > > But that "human look" is exactly what's missing from mergify.
> > >
> > > Basically what I'd like for mergify is to turn off automatic conflict
> > > resolution.
> > >
> > > More or less, speaking in terms of the stand-alone "patch" utility
> > > <https://man7.org/linux/man-pages/man1/patch.1.html>, my preference is
> > > to set the "fuzz factor" to zero.
> > >
> > >
> > > One way a human reviews such context differences is with git-range-diff.
> > > Continuing my previous example commands:
> > >
> > > $ git range-diff --color master..b2 b1..b2-rebase
> > >
> > > 1:  02dc81e58bd6 ! 1:  2cf39d4b1790 world
> > >     @@ -6,8 +6,8 @@
> > >      --- a/ReadMe.rst
> > >      +++ b/ReadMe.rst
> > >      @@
> > >     -
> > >       A modern, feature-rich, cross-platform firmware development
> > >     +   HELLO
> > >       environment for the UEFI and PI specifications from www.uefi.org.
> > >      +  WORLD
> > >
> > > This output shows that the "world" addition is the same (it is identical
> > > between pre-rebase and post-rebase in the commit), but the context has
> > > changed. During the rebase, the leading empty line of the context
> > > disappeared, and a HELLO line in the middle of the leading context
> > > appeared.
> > >
> > > This result may or may not be semantically correct; it needs a human
> > > decision. What if the original purpose of the "world" patch author was
> > > to say WORLD but only without HELLO? When they looked at the code,
> > there
> > > was no HELLO yet.
> > >
> > > git-range-diff is very powerful, but reading its output takes some
> > > getting used to. (Colorization with the "--color" option is basically
> > > required for understanding; I can't reproduce it in this email, alas.)
> > >
> > > I don't want to obsess about this forever, I just want us all to be
> > > aware that this risk exists.
> > >
> > > Thanks,
> > > Laszlo
> > >
> > > >
> > > > I then created 2 new PRs that added text to the same line # in
> > Readme.rst.
> > > >
> > > >     https://github.com/tianocore/edk2-codereview/pull/21
> > > >     https://github.com/tianocore/edk2-codereview/pull/22
> > > >
> > > > PR #21 passed all CI tests and was merged.  Mergify then attempted to
> > > > rebase #22 and got a merge conflict and is still in the open state waiting
> > > > for the developer to manually handle the merge conflict.
> > >
> > > This case is not worrisome; when there is a clear conflict that cannot be
> > auto-resolved, I'm not concerned.
> > >
> > > My concern is the sneaky contextual conflict that *appears* auto-resolvable,
> > but is semantically broken. Those things
> > > exist.
> > >
> > > Thanks
> > > Laszlo
> > >
> > >
> > >
> > >
> > >
> >
> >
> >
> >
> >
> 
> 
> 
> 
> 
> 
> 



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

Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Posted by Michael D Kinney 2 years, 10 months ago

Hi,

I have checked in a new Mergify configuration file into edk2-codereview.  Have not had a chance to 
test it yet, but I think it has all the updates that have been discussed:

1) Allows non EDK II Maintainers to open a PR and for an EDK II Maintainers to set labels.
2) Removed status check names from Mergify config and use the status checks listed in branch protections
3) Simplify the number of rules in Mergify config
4) Add support for 'main' branch.
5) Optional support for auto-rebase using 'auto-rebase' label.

https://github.com/tianocore/edk2-codereview/blob/master/.mergify/config.yml

Please try doing some additional tests to see if it behaves as expected.

I will get back to this on Monday or Tuesday next week and will start preparing a 
few code reviews to update the edk2 repo with these changes.

Thanks,

Mike

> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Wednesday, June 23, 2021 6:21 PM
> To: devel@edk2.groups.io; gaoliming@byosoft.com.cn; lersek@redhat.com; spbrogan@outlook.com; ardb@kernel.org; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Cc: 'Peter Grehan' <grehan@freebsd.org>; 'Ard Biesheuvel' <ardb+tianocore@kernel.org>; Justen, Jordan L
> <jordan.l.justen@intel.com>; 'Sean Brogan' <sean.brogan@microsoft.com>; 'Rebecca Cran' <rebecca@bsdio.com>
> Subject: RE: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
> 
> Liming,
> 
> Yes.  I am working on a configuration with will keep the current 'push' behavior
> and add the 'auto-rebase' option.
> 
> Mike
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of gaoliming
> > Sent: Wednesday, June 23, 2021 6:10 PM
> > To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>; lersek@redhat.com; spbrogan@outlook.com;
> > ardb@kernel.org
> > Cc: 'Peter Grehan' <grehan@freebsd.org>; 'Ard Biesheuvel' <ardb+tianocore@kernel.org>; Justen, Jordan L
> > <jordan.l.justen@intel.com>; 'Sean Brogan' <sean.brogan@microsoft.com>; 'Rebecca Cran' <rebecca@bsdio.com>
> > Subject: 回复: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
> >
> > Mike:
> >   If 'auto-rebase' label is not set, the behavior is still same to current one. Right?
> >   If yes, I agree this proposal. The maintainer can optionally set 'auto-rebase'.
> >
> > Thanks
> > Liming
> > > -----邮件原件-----
> > > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Michael D
> > > Kinney
> > > 发送时间: 2021年6月24日 6:08
> > > 收件人: devel@edk2.groups.io; lersek@redhat.com; spbrogan@outlook.com;
> > > ardb@kernel.org; Kinney, Michael D <michael.d.kinney@intel.com>
> > > 抄送: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel
> > > <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
> > > Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran
> > > <rebecca@bsdio.com>
> > > 主题: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE
> > > remnants
> > >
> > > Hi Laszlo,
> > >
> > > I understand your point.
> > >
> > > I am trying to balance the ease of use for developers, reducing overhead for
> > > maintainers, and
> > > prevent bad commits.
> > >
> > > I think you are saying that you want to make sure a maintainer carefully
> > > reviews changes
> > > across multiple PRs that are in the same area of code.  The CI checks will of
> > > course make
> > > sure the code builds and passes the basic boot tests, but those tests do not
> > > have full
> > > coverage so an interaction issue between two PRs that pass build and boot
> > > but have
> > > unintended behavior side effects are what require detailed manual review.
> > >
> > > I am going to remove the auto-rebase by default and add a optional label that
> > > can
> > > be set by a maintainer to enable auto-rebase.  If a maintainer is confident
> > > that
> > > a set of PRs being submitted at the same time with the 'push' label are
> > > independent,
> > > then the maintainer can also set 'auto-rebase'.  If they are not confident,
> > > then
> > > they can send PRs one at a time with only 'push' label and manually rebase
> > > each
> > > additional PR and review the manual rebase to make sure there are no
> > > unintended
> > > side effects.
> > >
> > > Any objections to this direction?
> > >
> > > Thanks,
> > >
> > > Mike
> > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> > > Ersek
> > > > Sent: Wednesday, June 23, 2021 12:45 PM
> > > > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> > > devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
> > > > Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel
> > > <ardb+tianocore@kernel.org>; Justen, Jordan L
> > > > <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>;
> > > Rebecca Cran <rebecca@bsdio.com>
> > > > Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE
> > > remnants
> > > >
> > > > On 06/23/21 20:44, Kinney, Michael D wrote:
> > > > >
> > > > > Hi Laszlo,
> > > > >
> > > > > Thank you for the test case.
> > > > >
> > > > > I created 2 PRs against edk2-codereview using your patches.
> > > > > I made minor update to commit messages to pass patch check.
> > > > >
> > > > >     https://github.com/tianocore/edk2-codereview/pull/18
> > > > >     https://github.com/tianocore/edk2-codereview/pull/19
> > > > >
> > > > > Found another issue with PatchCheck for the Mergify merge commit and
> > > > > fixed that.
> > > > >
> > > > > Mergify did process #18 and merged it in after passing all CI. Mergify
> > > > > rebased #19 successfully and merged it after passing all CI. I do not
> > > > > think this was your expected result.
> > > >
> > > > Indeed, my "desired" result at least would have been for mergify to
> > > > reject the rebase.
> > > >
> > > > > I looked more closely at the patches you provided.  They were not
> > > > > overlapping in the lines of Readme.rst.  This is why no merge conflict
> > > > > was detected.
> > > >
> > > > More precisely, a contextual conflict *was* determined between the
> > > > patches, but that conflict was auto-resolved.
> > > >
> > > > This is risky when done in an automated fashion. It is an extremely
> > > > convenient feature of git, when used interactively; that is, when the
> > > > auto-merge (automatic conflict resolution) is semantically verified by a
> > > > human. Git takes away the chore of conflict resolution, presents a
> > > > "likely good" end result, and a human only needs to *look* at the end
> > > > result, not *implement* it.
> > > >
> > > > But that "human look" is exactly what's missing from mergify.
> > > >
> > > > Basically what I'd like for mergify is to turn off automatic conflict
> > > > resolution.
> > > >
> > > > More or less, speaking in terms of the stand-alone "patch" utility
> > > > <https://man7.org/linux/man-pages/man1/patch.1.html>, my preference is
> > > > to set the "fuzz factor" to zero.
> > > >
> > > >
> > > > One way a human reviews such context differences is with git-range-diff.
> > > > Continuing my previous example commands:
> > > >
> > > > $ git range-diff --color master..b2 b1..b2-rebase
> > > >
> > > > 1:  02dc81e58bd6 ! 1:  2cf39d4b1790 world
> > > >     @@ -6,8 +6,8 @@
> > > >      --- a/ReadMe.rst
> > > >      +++ b/ReadMe.rst
> > > >      @@
> > > >     -
> > > >       A modern, feature-rich, cross-platform firmware development
> > > >     +   HELLO
> > > >       environment for the UEFI and PI specifications from www.uefi.org.
> > > >      +  WORLD
> > > >
> > > > This output shows that the "world" addition is the same (it is identical
> > > > between pre-rebase and post-rebase in the commit), but the context has
> > > > changed. During the rebase, the leading empty line of the context
> > > > disappeared, and a HELLO line in the middle of the leading context
> > > > appeared.
> > > >
> > > > This result may or may not be semantically correct; it needs a human
> > > > decision. What if the original purpose of the "world" patch author was
> > > > to say WORLD but only without HELLO? When they looked at the code,
> > > there
> > > > was no HELLO yet.
> > > >
> > > > git-range-diff is very powerful, but reading its output takes some
> > > > getting used to. (Colorization with the "--color" option is basically
> > > > required for understanding; I can't reproduce it in this email, alas.)
> > > >
> > > > I don't want to obsess about this forever, I just want us all to be
> > > > aware that this risk exists.
> > > >
> > > > Thanks,
> > > > Laszlo
> > > >
> > > > >
> > > > > I then created 2 new PRs that added text to the same line # in
> > > Readme.rst.
> > > > >
> > > > >     https://github.com/tianocore/edk2-codereview/pull/21
> > > > >     https://github.com/tianocore/edk2-codereview/pull/22
> > > > >
> > > > > PR #21 passed all CI tests and was merged.  Mergify then attempted to
> > > > > rebase #22 and got a merge conflict and is still in the open state waiting
> > > > > for the developer to manually handle the merge conflict.
> > > >
> > > > This case is not worrisome; when there is a clear conflict that cannot be
> > > auto-resolved, I'm not concerned.
> > > >
> > > > My concern is the sneaky contextual conflict that *appears* auto-resolvable,
> > > but is semantically broken. Those things
> > > > exist.
> > > >
> > > > Thanks
> > > > Laszlo
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
> > >
> >
> >
> >
> >
> >
> > 
> >



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

Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Posted by Laszlo Ersek 2 years, 10 months ago

On 06/24/21 00:07, Kinney, Michael D wrote:
> Hi Laszlo,
> 
> I understand your point. 
> 
> I am trying to balance the ease of use for developers, reducing overhead for maintainers, and
> prevent bad commits.
> 
> I think you are saying that you want to make sure a maintainer carefully reviews changes
> across multiple PRs that are in the same area of code.  The CI checks will of course make
> sure the code builds and passes the basic boot tests, but those tests do not have full
> coverage so an interaction issue between two PRs that pass build and boot but have 
> unintended behavior side effects are what require detailed manual review.
> 
> I am going to remove the auto-rebase by default and add a optional label that can
> be set by a maintainer to enable auto-rebase.  If a maintainer is confident that 
> a set of PRs being submitted at the same time with the 'push' label are independent,
> then the maintainer can also set 'auto-rebase'.  If they are not confident, then 
> they can send PRs one at a time with only 'push' label and manually rebase each
> additional PR and review the manual rebase to make sure there are no unintended
> side effects.

Sounds great, thank you!
Laszlo

> 
> Any objections to this direction?
> 
> Thanks,
> 
> Mike
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
>> Sent: Wednesday, June 23, 2021 12:45 PM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
>> Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
>> <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
>> Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
>>
>> On 06/23/21 20:44, Kinney, Michael D wrote:
>>>
>>> Hi Laszlo,
>>>
>>> Thank you for the test case.
>>>
>>> I created 2 PRs against edk2-codereview using your patches.
>>> I made minor update to commit messages to pass patch check.
>>>
>>>     https://github.com/tianocore/edk2-codereview/pull/18
>>>     https://github.com/tianocore/edk2-codereview/pull/19
>>>
>>> Found another issue with PatchCheck for the Mergify merge commit and
>>> fixed that.
>>>
>>> Mergify did process #18 and merged it in after passing all CI. Mergify
>>> rebased #19 successfully and merged it after passing all CI. I do not
>>> think this was your expected result.
>>
>> Indeed, my "desired" result at least would have been for mergify to
>> reject the rebase.
>>
>>> I looked more closely at the patches you provided.  They were not
>>> overlapping in the lines of Readme.rst.  This is why no merge conflict
>>> was detected.
>>
>> More precisely, a contextual conflict *was* determined between the
>> patches, but that conflict was auto-resolved.
>>
>> This is risky when done in an automated fashion. It is an extremely
>> convenient feature of git, when used interactively; that is, when the
>> auto-merge (automatic conflict resolution) is semantically verified by a
>> human. Git takes away the chore of conflict resolution, presents a
>> "likely good" end result, and a human only needs to *look* at the end
>> result, not *implement* it.
>>
>> But that "human look" is exactly what's missing from mergify.
>>
>> Basically what I'd like for mergify is to turn off automatic conflict
>> resolution.
>>
>> More or less, speaking in terms of the stand-alone "patch" utility
>> <https://man7.org/linux/man-pages/man1/patch.1.html>, my preference is
>> to set the "fuzz factor" to zero.
>>
>>
>> One way a human reviews such context differences is with git-range-diff.
>> Continuing my previous example commands:
>>
>> $ git range-diff --color master..b2 b1..b2-rebase
>>
>> 1:  02dc81e58bd6 ! 1:  2cf39d4b1790 world
>>     @@ -6,8 +6,8 @@
>>      --- a/ReadMe.rst
>>      +++ b/ReadMe.rst
>>      @@
>>     -
>>       A modern, feature-rich, cross-platform firmware development
>>     +   HELLO
>>       environment for the UEFI and PI specifications from www.uefi.org.
>>      +  WORLD
>>
>> This output shows that the "world" addition is the same (it is identical
>> between pre-rebase and post-rebase in the commit), but the context has
>> changed. During the rebase, the leading empty line of the context
>> disappeared, and a HELLO line in the middle of the leading context
>> appeared.
>>
>> This result may or may not be semantically correct; it needs a human
>> decision. What if the original purpose of the "world" patch author was
>> to say WORLD but only without HELLO? When they looked at the code, there
>> was no HELLO yet.
>>
>> git-range-diff is very powerful, but reading its output takes some
>> getting used to. (Colorization with the "--color" option is basically
>> required for understanding; I can't reproduce it in this email, alas.)
>>
>> I don't want to obsess about this forever, I just want us all to be
>> aware that this risk exists.
>>
>> Thanks,
>> Laszlo
>>
>>>
>>> I then created 2 new PRs that added text to the same line # in Readme.rst.
>>>
>>>     https://github.com/tianocore/edk2-codereview/pull/21
>>>     https://github.com/tianocore/edk2-codereview/pull/22
>>>
>>> PR #21 passed all CI tests and was merged.  Mergify then attempted to
>>> rebase #22 and got a merge conflict and is still in the open state waiting
>>> for the developer to manually handle the merge conflict.
>>
>> This case is not worrisome; when there is a clear conflict that cannot be auto-resolved, I'm not concerned.
>>
>> My concern is the sneaky contextual conflict that *appears* auto-resolvable, but is semantically broken. Those things
>> exist.
>>
>> Thanks
>> Laszlo
>>
>>
>>
>> 
>>
> 



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

Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Posted by Michael D Kinney 2 years, 9 months ago

Hi Laszlo,

I did many experiments and could not get the exact behavior I proposed.

Here is the best I can do with the behavior of GitHub and Mergify:

1) I further simplified Mergify configuration so personal builds ('push'
   label not set) are no longer auto closed.  Any developer doing a
   personal build that wants to abandon the change should manually close
   the PR.  We may need to periodically review PRs that have been open
   for an extended period of time and close them and developers can reopen
   if that was a mistake.

2) The 'push' label always does the safest possible rebase and merge.
   If many PRs are queued at a time, then each one is rebased in turn,
   all CI checks are run and if all CI checks pass, then PR is 
   added with linear history to the base branch.

3) If a maintainer wants to manually send a sequence of PRs through 
   one at a time and review the state before sending the next one, then
   I recommending sending each PR as a personal build (always rebase against
   latest base branch before submitting PR).  Review the commits and status
   checks.  If the PR looks good and passes all status checks and the state
   from GitHub is that the PR is 'up-to-date' with the base branch, then
   the maintainer can set the 'push' label and the PR is merged immediately
   without re-running the status checks since there have been no changes.  

   If other PRs were merged into the base branch while the PR status checks
   were run, then the personal build will show that the branch is out-of-date
   with the base branch.  The maintainer can do a local rebase and review
   the changes and do a forced push of the updated PR.  This will trigger the
   personal build again.  If that passes and the branch is 'up-to-date', then
   the 'push' label can be set to immediately merge.  Repeat as needed.

   NOTE: If there is a lot of PR activity from other maintainers at the same
   time as this manual process is being used, then the manual process may
   have to rebase and force push several times until there is a quiet window.


The simplified Mergify configuration file is shown below.

queue_rules:
  - name: default
    conditions:
      - base~=(^main|^master|^stable/)
      - label=push

pull_request_rules:
  - name: Automatically merge a PR when all required checks pass and 'push' label is present
    conditions:
      - base~=(^main|^master|^stable/)
      - label=push
    actions:
      queue:
        method: rebase
        rebase_fallback: none
        name: default

  - name: Post a comment on a PR that can not be merged due to a merge conflict
    conditions:
      - base~=(^main|^master|^stable/)
      - conflict
    actions:
      comment:
        message: PR can not be merged due to conflict.  Please rebase and resubmit


Let me know if this process of using personal builds and setting 'push' label
if PR is 'up-to-date' is acceptable.  I have tested this process with a few
different scenarios in the edk2-codereview repo, and they all worked as expected.

Best regards,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Monday, June 28, 2021 5:23 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
> Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
> Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
> 
> On 06/24/21 00:07, Kinney, Michael D wrote:
> > Hi Laszlo,
> >
> > I understand your point.
> >
> > I am trying to balance the ease of use for developers, reducing overhead for maintainers, and
> > prevent bad commits.
> >
> > I think you are saying that you want to make sure a maintainer carefully reviews changes
> > across multiple PRs that are in the same area of code.  The CI checks will of course make
> > sure the code builds and passes the basic boot tests, but those tests do not have full
> > coverage so an interaction issue between two PRs that pass build and boot but have
> > unintended behavior side effects are what require detailed manual review.
> >
> > I am going to remove the auto-rebase by default and add a optional label that can
> > be set by a maintainer to enable auto-rebase.  If a maintainer is confident that
> > a set of PRs being submitted at the same time with the 'push' label are independent,
> > then the maintainer can also set 'auto-rebase'.  If they are not confident, then
> > they can send PRs one at a time with only 'push' label and manually rebase each
> > additional PR and review the manual rebase to make sure there are no unintended
> > side effects.
> 
> Sounds great, thank you!
> Laszlo
> 
> >
> > Any objections to this direction?
> >
> > Thanks,
> >
> > Mike
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> >> Sent: Wednesday, June 23, 2021 12:45 PM
> >> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
> >> Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
> >> <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
> >> Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
> >>
> >> On 06/23/21 20:44, Kinney, Michael D wrote:
> >>>
> >>> Hi Laszlo,
> >>>
> >>> Thank you for the test case.
> >>>
> >>> I created 2 PRs against edk2-codereview using your patches.
> >>> I made minor update to commit messages to pass patch check.
> >>>
> >>>     https://github.com/tianocore/edk2-codereview/pull/18
> >>>     https://github.com/tianocore/edk2-codereview/pull/19
> >>>
> >>> Found another issue with PatchCheck for the Mergify merge commit and
> >>> fixed that.
> >>>
> >>> Mergify did process #18 and merged it in after passing all CI. Mergify
> >>> rebased #19 successfully and merged it after passing all CI. I do not
> >>> think this was your expected result.
> >>
> >> Indeed, my "desired" result at least would have been for mergify to
> >> reject the rebase.
> >>
> >>> I looked more closely at the patches you provided.  They were not
> >>> overlapping in the lines of Readme.rst.  This is why no merge conflict
> >>> was detected.
> >>
> >> More precisely, a contextual conflict *was* determined between the
> >> patches, but that conflict was auto-resolved.
> >>
> >> This is risky when done in an automated fashion. It is an extremely
> >> convenient feature of git, when used interactively; that is, when the
> >> auto-merge (automatic conflict resolution) is semantically verified by a
> >> human. Git takes away the chore of conflict resolution, presents a
> >> "likely good" end result, and a human only needs to *look* at the end
> >> result, not *implement* it.
> >>
> >> But that "human look" is exactly what's missing from mergify.
> >>
> >> Basically what I'd like for mergify is to turn off automatic conflict
> >> resolution.
> >>
> >> More or less, speaking in terms of the stand-alone "patch" utility
> >> <https://man7.org/linux/man-pages/man1/patch.1.html>, my preference is
> >> to set the "fuzz factor" to zero.
> >>
> >>
> >> One way a human reviews such context differences is with git-range-diff.
> >> Continuing my previous example commands:
> >>
> >> $ git range-diff --color master..b2 b1..b2-rebase
> >>
> >> 1:  02dc81e58bd6 ! 1:  2cf39d4b1790 world
> >>     @@ -6,8 +6,8 @@
> >>      --- a/ReadMe.rst
> >>      +++ b/ReadMe.rst
> >>      @@
> >>     -
> >>       A modern, feature-rich, cross-platform firmware development
> >>     +   HELLO
> >>       environment for the UEFI and PI specifications from www.uefi.org.
> >>      +  WORLD
> >>
> >> This output shows that the "world" addition is the same (it is identical
> >> between pre-rebase and post-rebase in the commit), but the context has
> >> changed. During the rebase, the leading empty line of the context
> >> disappeared, and a HELLO line in the middle of the leading context
> >> appeared.
> >>
> >> This result may or may not be semantically correct; it needs a human
> >> decision. What if the original purpose of the "world" patch author was
> >> to say WORLD but only without HELLO? When they looked at the code, there
> >> was no HELLO yet.
> >>
> >> git-range-diff is very powerful, but reading its output takes some
> >> getting used to. (Colorization with the "--color" option is basically
> >> required for understanding; I can't reproduce it in this email, alas.)
> >>
> >> I don't want to obsess about this forever, I just want us all to be
> >> aware that this risk exists.
> >>
> >> Thanks,
> >> Laszlo
> >>
> >>>
> >>> I then created 2 new PRs that added text to the same line # in Readme.rst.
> >>>
> >>>     https://github.com/tianocore/edk2-codereview/pull/21
> >>>     https://github.com/tianocore/edk2-codereview/pull/22
> >>>
> >>> PR #21 passed all CI tests and was merged.  Mergify then attempted to
> >>> rebase #22 and got a merge conflict and is still in the open state waiting
> >>> for the developer to manually handle the merge conflict.
> >>
> >> This case is not worrisome; when there is a clear conflict that cannot be auto-resolved, I'm not concerned.
> >>
> >> My concern is the sneaky contextual conflict that *appears* auto-resolvable, but is semantically broken. Those things
> >> exist.
> >>
> >> Thanks
> >> Laszlo
> >>
> >>
> >>
> >>
> >>
> >
> 
> 
> 
> 
> 



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

Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Posted by Laszlo Ersek 2 years, 9 months ago

On 07/07/21 08:00, Kinney, Michael D wrote:
> Hi Laszlo,
> 
> I did many experiments and could not get the exact behavior I proposed.
> 
> Here is the best I can do with the behavior of GitHub and Mergify:
> 
> 1) I further simplified Mergify configuration so personal builds ('push'
>    label not set) are no longer auto closed.  Any developer doing a
>    personal build that wants to abandon the change should manually close
>    the PR.

This sounds OK to me.

>    We may need to periodically review PRs that have been open
>    for an extended period of time and close them and developers can reopen
>    if that was a mistake.

Yes, I've been periodically checking PRs that were stuck open anyway.

> 
> 2) The 'push' label always does the safest possible rebase and merge.
>    If many PRs are queued at a time, then each one is rebased in turn,
>    all CI checks are run and if all CI checks pass, then PR is 
>    added with linear history to the base branch.

OK.

> 
> 3) If a maintainer wants to manually send a sequence of PRs through 
>    one at a time and review the state before sending the next one, then
>    I recommending sending each PR as a personal build (always rebase against
>    latest base branch before submitting PR).  Review the commits and status
>    checks.  If the PR looks good and passes all status checks and the state
>    from GitHub is that the PR is 'up-to-date' with the base branch, then
>    the maintainer can set the 'push' label and the PR is merged immediately
>    without re-running the status checks since there have been no changes.  

So this is the key development. OK.

> 
>    If other PRs were merged into the base branch while the PR status checks
>    were run, then the personal build will show that the branch is out-of-date
>    with the base branch.  The maintainer can do a local rebase and review
>    the changes and do a forced push of the updated PR.  This will trigger the
>    personal build again.  If that passes and the branch is 'up-to-date', then
>    the 'push' label can be set to immediately merge.  Repeat as needed.
> 
>    NOTE: If there is a lot of PR activity from other maintainers at the same
>    time as this manual process is being used, then the manual process may
>    have to rebase and force push several times until there is a quiet window.

Makes sense.

> 
> 
> The simplified Mergify configuration file is shown below.
> 
> queue_rules:
>   - name: default
>     conditions:
>       - base~=(^main|^master|^stable/)
>       - label=push
> 
> pull_request_rules:
>   - name: Automatically merge a PR when all required checks pass and 'push' label is present
>     conditions:
>       - base~=(^main|^master|^stable/)
>       - label=push
>     actions:
>       queue:
>         method: rebase
>         rebase_fallback: none
>         name: default
> 
>   - name: Post a comment on a PR that can not be merged due to a merge conflict
>     conditions:
>       - base~=(^main|^master|^stable/)
>       - conflict
>     actions:
>       comment:
>         message: PR can not be merged due to conflict.  Please rebase and resubmit
> 
> 
> Let me know if this process of using personal builds and setting 'push' label
> if PR is 'up-to-date' is acceptable.  I have tested this process with a few
> different scenarios in the edk2-codereview repo, and they all worked as expected.

Sounds all fine to me; thank you for working it out!
Laszlo


> 
> Best regards,
> 
> Mike
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
>> Sent: Monday, June 28, 2021 5:23 AM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
>> Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
>> <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
>> Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
>>
>> On 06/24/21 00:07, Kinney, Michael D wrote:
>>> Hi Laszlo,
>>>
>>> I understand your point.
>>>
>>> I am trying to balance the ease of use for developers, reducing overhead for maintainers, and
>>> prevent bad commits.
>>>
>>> I think you are saying that you want to make sure a maintainer carefully reviews changes
>>> across multiple PRs that are in the same area of code.  The CI checks will of course make
>>> sure the code builds and passes the basic boot tests, but those tests do not have full
>>> coverage so an interaction issue between two PRs that pass build and boot but have
>>> unintended behavior side effects are what require detailed manual review.
>>>
>>> I am going to remove the auto-rebase by default and add a optional label that can
>>> be set by a maintainer to enable auto-rebase.  If a maintainer is confident that
>>> a set of PRs being submitted at the same time with the 'push' label are independent,
>>> then the maintainer can also set 'auto-rebase'.  If they are not confident, then
>>> they can send PRs one at a time with only 'push' label and manually rebase each
>>> additional PR and review the manual rebase to make sure there are no unintended
>>> side effects.
>>
>> Sounds great, thank you!
>> Laszlo
>>
>>>
>>> Any objections to this direction?
>>>
>>> Thanks,
>>>
>>> Mike
>>>
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
>>>> Sent: Wednesday, June 23, 2021 12:45 PM
>>>> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
>>>> Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
>>>> <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
>>>> Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
>>>>
>>>> On 06/23/21 20:44, Kinney, Michael D wrote:
>>>>>
>>>>> Hi Laszlo,
>>>>>
>>>>> Thank you for the test case.
>>>>>
>>>>> I created 2 PRs against edk2-codereview using your patches.
>>>>> I made minor update to commit messages to pass patch check.
>>>>>
>>>>>     https://github.com/tianocore/edk2-codereview/pull/18
>>>>>     https://github.com/tianocore/edk2-codereview/pull/19
>>>>>
>>>>> Found another issue with PatchCheck for the Mergify merge commit and
>>>>> fixed that.
>>>>>
>>>>> Mergify did process #18 and merged it in after passing all CI. Mergify
>>>>> rebased #19 successfully and merged it after passing all CI. I do not
>>>>> think this was your expected result.
>>>>
>>>> Indeed, my "desired" result at least would have been for mergify to
>>>> reject the rebase.
>>>>
>>>>> I looked more closely at the patches you provided.  They were not
>>>>> overlapping in the lines of Readme.rst.  This is why no merge conflict
>>>>> was detected.
>>>>
>>>> More precisely, a contextual conflict *was* determined between the
>>>> patches, but that conflict was auto-resolved.
>>>>
>>>> This is risky when done in an automated fashion. It is an extremely
>>>> convenient feature of git, when used interactively; that is, when the
>>>> auto-merge (automatic conflict resolution) is semantically verified by a
>>>> human. Git takes away the chore of conflict resolution, presents a
>>>> "likely good" end result, and a human only needs to *look* at the end
>>>> result, not *implement* it.
>>>>
>>>> But that "human look" is exactly what's missing from mergify.
>>>>
>>>> Basically what I'd like for mergify is to turn off automatic conflict
>>>> resolution.
>>>>
>>>> More or less, speaking in terms of the stand-alone "patch" utility
>>>> <https://man7.org/linux/man-pages/man1/patch.1.html>, my preference is
>>>> to set the "fuzz factor" to zero.
>>>>
>>>>
>>>> One way a human reviews such context differences is with git-range-diff.
>>>> Continuing my previous example commands:
>>>>
>>>> $ git range-diff --color master..b2 b1..b2-rebase
>>>>
>>>> 1:  02dc81e58bd6 ! 1:  2cf39d4b1790 world
>>>>     @@ -6,8 +6,8 @@
>>>>      --- a/ReadMe.rst
>>>>      +++ b/ReadMe.rst
>>>>      @@
>>>>     -
>>>>       A modern, feature-rich, cross-platform firmware development
>>>>     +   HELLO
>>>>       environment for the UEFI and PI specifications from www.uefi.org.
>>>>      +  WORLD
>>>>
>>>> This output shows that the "world" addition is the same (it is identical
>>>> between pre-rebase and post-rebase in the commit), but the context has
>>>> changed. During the rebase, the leading empty line of the context
>>>> disappeared, and a HELLO line in the middle of the leading context
>>>> appeared.
>>>>
>>>> This result may or may not be semantically correct; it needs a human
>>>> decision. What if the original purpose of the "world" patch author was
>>>> to say WORLD but only without HELLO? When they looked at the code, there
>>>> was no HELLO yet.
>>>>
>>>> git-range-diff is very powerful, but reading its output takes some
>>>> getting used to. (Colorization with the "--color" option is basically
>>>> required for understanding; I can't reproduce it in this email, alas.)
>>>>
>>>> I don't want to obsess about this forever, I just want us all to be
>>>> aware that this risk exists.
>>>>
>>>> Thanks,
>>>> Laszlo
>>>>
>>>>>
>>>>> I then created 2 new PRs that added text to the same line # in Readme.rst.
>>>>>
>>>>>     https://github.com/tianocore/edk2-codereview/pull/21
>>>>>     https://github.com/tianocore/edk2-codereview/pull/22
>>>>>
>>>>> PR #21 passed all CI tests and was merged.  Mergify then attempted to
>>>>> rebase #22 and got a merge conflict and is still in the open state waiting
>>>>> for the developer to manually handle the merge conflict.
>>>>
>>>> This case is not worrisome; when there is a clear conflict that cannot be auto-resolved, I'm not concerned.
>>>>
>>>> My concern is the sneaky contextual conflict that *appears* auto-resolvable, but is semantically broken. Those things
>>>> exist.
>>>>
>>>> Thanks
>>>> Laszlo
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>>
>> 
>>
> 



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

Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Posted by Laszlo Ersek 2 years, 10 months ago

On 06/12/21 22:43, Rebecca Cran wrote:
> TPM support hasn't been tested and any lines in the .dsc and .fdf files
> that appear to show support are bogus. Remove them.
> 
> This fixes https://bugzilla.tianocore.org/show_bug.cgi?id=3354 .
> 
> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> ---
>  OvmfPkg/Bhyve/BhyveX64.dsc | 64 --------------------
>  OvmfPkg/Bhyve/BhyveX64.fdf | 15 -----
>  2 files changed, 79 deletions(-)

I'll try to review this patch soon; I'm dealing with the usual post-PTO
email apocalypse.

Thanks,
Laszlo



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

Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Posted by Laszlo Ersek 2 years, 10 months ago

On 06/12/21 22:43, Rebecca Cran wrote:
> TPM support hasn't been tested and any lines in the .dsc and .fdf files
> that appear to show support are bogus. Remove them.
> 
> This fixes https://bugzilla.tianocore.org/show_bug.cgi?id=3354 .
> 
> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> ---
>  OvmfPkg/Bhyve/BhyveX64.dsc | 64 --------------------
>  OvmfPkg/Bhyve/BhyveX64.fdf | 15 -----
>  2 files changed, 79 deletions(-)
> 
> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
> index d8792812ab..cbf896e89b 100644
> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
> @@ -31,8 +31,6 @@
>    DEFINE SECURE_BOOT_ENABLE      = FALSE
>    DEFINE SMM_REQUIRE             = FALSE
>    DEFINE SOURCE_DEBUG_ENABLE     = FALSE
> -  DEFINE TPM_ENABLE              = FALSE
> -  DEFINE TPM_CONFIG_ENABLE       = FALSE
>  
>    #
>    # Network definition
> @@ -221,16 +219,8 @@
>    OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
>    XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf
>  
> -
> -!if $(TPM_ENABLE) == TRUE
> -  Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
> -  Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
> -  Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
> -  TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> -!else
>    Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
>    TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
> -!endif
>  
>  [LibraryClasses.common]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> @@ -292,11 +282,6 @@
>    CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
>    PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
>  
> -!if $(TPM_ENABLE) == TRUE
> -  BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> -  Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
> -!endif
> -
>    MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
>  
>  [LibraryClasses.common.DXE_CORE]
> @@ -366,9 +351,6 @@
>  !endif
>    PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
>    MpInitLib|UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf
> -!if $(TPM_ENABLE) == TRUE
> -  Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
> -!endif
>  
>  [LibraryClasses.common.UEFI_APPLICATION]
>    PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> @@ -563,22 +545,12 @@
>  
>    gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
>  
> -!if $(TPM_ENABLE) == TRUE
> -  gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
> -!endif
> -
>    # MdeModulePkg resolution sets up the system display resolution
>    gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution|0
>    gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution|0
>    gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|0
>    gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|0
>  
> -[PcdsDynamicHii]
> -!if $(TPM_ENABLE) == TRUE && $(TPM_CONFIG_ENABLE) == TRUE
> -  gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
> -  gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
> -!endif
> -
>  ################################################################################
>  #
>  # Components Section - list of all EDK II Modules needed by this Platform.
> @@ -618,19 +590,6 @@
>      <LibraryClasses>
>    }
>  
> -!if $(TPM_ENABLE) == TRUE
> -  OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> -  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> -    <LibraryClasses>
> -      HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> -  }
> -!endif
> -
>    #
>    # DXE Phase modules
>    #
> @@ -653,9 +612,6 @@
>      <LibraryClasses>
>  !if $(SECURE_BOOT_ENABLE) == TRUE
>        NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> -!endif
> -!if $(TPM_ENABLE) == TRUE
> -      NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
>  !endif
>    }
>  
> @@ -841,23 +797,3 @@
>        NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
>    }
>  
> -
> -  #
> -  # TPM support
> -  #
> -!if $(TPM_ENABLE) == TRUE
> -  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
> -    <LibraryClasses>
> -      Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
> -      NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
> -      HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> -      NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> -  }
> -!if $(TPM_CONFIG_ENABLE) == TRUE
> -  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> -!endif
> -!endif
> diff --git a/OvmfPkg/Bhyve/BhyveX64.fdf b/OvmfPkg/Bhyve/BhyveX64.fdf
> index 3eff36dac1..fbd63a395a 100644
> --- a/OvmfPkg/Bhyve/BhyveX64.fdf
> +++ b/OvmfPkg/Bhyve/BhyveX64.fdf
> @@ -158,11 +158,6 @@ INF  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
>  INF  OvmfPkg/Bhyve/SmmAccess/SmmAccessPei.inf
>  !endif
>  
> -!if $(TPM_ENABLE) == TRUE
> -INF  OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> -INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> -!endif
> -
>  ################################################################################
>  
>  [FV.DXEFV]
> @@ -333,16 +328,6 @@ INF  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
>  INF  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
>  !endif
>  
> -#
> -# TPM support
> -#
> -!if $(TPM_ENABLE) == TRUE
> -INF  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
> -!if $(TPM_CONFIG_ENABLE) == TRUE
> -INF  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> -!endif
> -!endif
> -
>  ################################################################################
>  
>  [FV.FVMAIN_COMPACT]
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



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

Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Posted by Laszlo Ersek 2 years, 10 months ago

On 06/12/21 22:43, Rebecca Cran wrote:
> TPM support hasn't been tested and any lines in the .dsc and .fdf files
> that appear to show support are bogus. Remove them.
> 
> This fixes https://bugzilla.tianocore.org/show_bug.cgi?id=3354 .
> 
> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> ---
>  OvmfPkg/Bhyve/BhyveX64.dsc | 64 --------------------
>  OvmfPkg/Bhyve/BhyveX64.fdf | 15 -----
>  2 files changed, 79 deletions(-)

Merged as commit 12e34cd2f790, via
<https://github.com/tianocore/edk2/pull/1759>.

Thanks
Laszlo



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