[PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants


Rebecca Cran
 

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=3D3354 .

Signed-off-by: Rebecca Cran <rebecca@...>
---
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 =3D FALSE=0D
DEFINE SMM_REQUIRE =3D FALSE=0D
DEFINE SOURCE_DEBUG_ENABLE =3D FALSE=0D
- DEFINE TPM_ENABLE =3D FALSE=0D
- DEFINE TPM_CONFIG_ENABLE =3D FALSE=0D
=0D
#=0D
# Network definition=0D
@@ -221,16 +219,8 @@
OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib=
/BaseOrderedCollectionRedBlackTreeLib.inf=0D
XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf=0D
=0D
-=0D
-!if $(TPM_ENABLE) =3D=3D TRUE=0D
- Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf=0D
- Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeT=
cg2PhysicalPresenceLib.inf=0D
- Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibN=
ull.inf=0D
- TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasure=
mentLib.inf=0D
-!else=0D
Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeT=
cg2PhysicalPresenceLib.inf=0D
TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurem=
entLibNull.inf=0D
-!endif=0D
=0D
[LibraryClasses.common]=0D
BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf=0D
@@ -292,11 +282,6 @@
CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuE=
xceptionHandlerLib.inf=0D
PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf=0D
=0D
-!if $(TPM_ENABLE) =3D=3D TRUE=0D
- BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf=0D
- Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.in=
f=0D
-!endif=0D
-=0D
MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLi=
b.inf=0D
=0D
[LibraryClasses.common.DXE_CORE]=0D
@@ -366,9 +351,6 @@
!endif=0D
PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf=0D
MpInitLib|UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf=0D
-!if $(TPM_ENABLE) =3D=3D TRUE=0D
- Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.in=
f=0D
-!endif=0D
=0D
[LibraryClasses.common.UEFI_APPLICATION]=0D
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf=0D
@@ -563,22 +545,12 @@
=0D
gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00=0D
=0D
-!if $(TPM_ENABLE) =3D=3D TRUE=0D
- gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00=
, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}=0D
-!endif=0D
-=0D
# MdeModulePkg resolution sets up the system display resolution=0D
gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution|0=0D
gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution|0=0D
gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|0=0D
gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|0=0D
=0D
-[PcdsDynamicHii]=0D
-!if $(TPM_ENABLE) =3D=3D TRUE && $(TPM_CONFIG_ENABLE) =3D=3D TRUE=0D
- gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_=
VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS=0D
- gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2C=
onfigFormSetGuid|0x8|3|NV,BS=0D
-!endif=0D
-=0D
##########################################################################=
######=0D
#=0D
# Components Section - list of all EDK II Modules needed by this Platform.=
=0D
@@ -618,19 +590,6 @@
<LibraryClasses>=0D
}=0D
=0D
-!if $(TPM_ENABLE) =3D=3D TRUE=0D
- OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf=0D
- SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {=0D
- <LibraryClasses>=0D
- HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCrypt=
oRouterPei.inf=0D
- NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf=
=0D
- NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256=
.inf=0D
- NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384=
.inf=0D
- NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512=
.inf=0D
- NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf=0D
- }=0D
-!endif=0D
-=0D
#=0D
# DXE Phase modules=0D
#=0D
@@ -653,9 +612,6 @@
<LibraryClasses>=0D
!if $(SECURE_BOOT_ENABLE) =3D=3D TRUE=0D
NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificatio=
nLib.inf=0D
-!endif=0D
-!if $(TPM_ENABLE) =3D=3D TRUE=0D
- NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib=
.inf=0D
!endif=0D
}=0D
=0D
@@ -841,23 +797,3 @@
NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf=0D
}=0D
=0D
-=0D
- #=0D
- # TPM support=0D
- #=0D
-!if $(TPM_ENABLE) =3D=3D TRUE=0D
- SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {=0D
- <LibraryClasses>=0D
- Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibR=
outerDxe.inf=0D
- NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf=0D
- HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCrypt=
oRouterDxe.inf=0D
- NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf=
=0D
- NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256=
.inf=0D
- NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384=
.inf=0D
- NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512=
.inf=0D
- NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf=0D
- }=0D
-!if $(TPM_CONFIG_ENABLE) =3D=3D TRUE=0D
- SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf=0D
-!endif=0D
-!endif=0D
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/S3Resume2P=
ei.inf
INF OvmfPkg/Bhyve/SmmAccess/SmmAccessPei.inf=0D
!endif=0D
=0D
-!if $(TPM_ENABLE) =3D=3D TRUE=0D
-INF OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf=0D
-INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf=0D
-!endif=0D
-=0D
##########################################################################=
######=0D
=0D
[FV.DXEFV]=0D
@@ -333,16 +328,6 @@ INF MdeModulePkg/Universal/FaultTolerantWriteDxe/Faul=
tTolerantWriteDxe.inf
INF MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf=0D
!endif=0D
=0D
-#=0D
-# TPM support=0D
-#=0D
-!if $(TPM_ENABLE) =3D=3D TRUE=0D
-INF SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf=0D
-!if $(TPM_CONFIG_ENABLE) =3D=3D TRUE=0D
-INF SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf=0D
-!endif=0D
-!endif=0D
-=0D
##########################################################################=
######=0D
=0D
[FV.FVMAIN_COMPACT]=0D
--=20
2.32.0


Peter Grehan
 

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@...>
---
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@...>


Ard Biesheuvel
 

(+ Sean, Mike)

On Sat, 12 Jun 2021 at 22:43, Rebecca Cran <rebecca@...> 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@...>
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@...>

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


Sean
 

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@...> 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@...>
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@...>
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


Ard Biesheuvel
 

On Wed, 16 Jun 2021 at 21:00, Sean Brogan <spbrogan@...> 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@...> 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@...>
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@...>

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





Michael D Kinney
 

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@...>
Sent: Wednesday, June 16, 2021 2:55 PM
To: Sean Brogan <spbrogan@...>
Cc: Kinney, Michael D <michael.d.kinney@...>; Peter Grehan <grehan@...>; Laszlo Ersek <lersek@...>;
Ard Biesheuvel <ardb+tianocore@...>; Justen, Jordan L <jordan.l.justen@...>; Sean Brogan
<sean.brogan@...>; Rebecca Cran <rebecca@...>; 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@...> 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@...> 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@...>
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@...>

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





Michael D Kinney
 

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@...; Kinney, Michael D <michael.d.kinney@...>
Cc: Peter Grehan <grehan@...>; Laszlo Ersek <lersek@...>; Ard Biesheuvel <ardb+tianocore@...>;
Justen, Jordan L <jordan.l.justen@...>; Sean Brogan <sean.brogan@...>; Rebecca Cran <rebecca@...>;
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@...> 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@...>
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@...>

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







Michael D Kinney
 

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@...>
Sent: Thursday, June 17, 2021 2:53 PM
To: devel@edk2.groups.io; spbrogan@...; ardb@...; Kinney, Michael D <michael.d.kinney@...>
Cc: Peter Grehan <grehan@...>; Laszlo Ersek <lersek@...>; Ard Biesheuvel <ardb+tianocore@...>;
Justen, Jordan L <jordan.l.justen@...>; Sean Brogan <sean.brogan@...>; Rebecca Cran <rebecca@...>
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@...; Kinney, Michael D <michael.d.kinney@...>
Cc: Peter Grehan <grehan@...>; Laszlo Ersek <lersek@...>; Ard Biesheuvel <ardb+tianocore@...>;
Justen, Jordan L <jordan.l.justen@...>; Sean Brogan <sean.brogan@...>; Rebecca Cran <rebecca@...>;
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@...> 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@...>
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@...>

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







Michael D Kinney
 

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@...>
Sent: Thursday, June 17, 2021 2:55 PM
To: devel@edk2.groups.io; spbrogan@...; ardb@...; Kinney, Michael D <michael.d.kinney@...>
Cc: Peter Grehan <grehan@...>; Laszlo Ersek <lersek@...>; Ard Biesheuvel <ardb+tianocore@...>;
Justen, Jordan L <jordan.l.justen@...>; Sean Brogan <sean.brogan@...>; Rebecca Cran <rebecca@...>
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@...>
Sent: Thursday, June 17, 2021 2:53 PM
To: devel@edk2.groups.io; spbrogan@...; ardb@...; Kinney, Michael D <michael.d.kinney@...>
Cc: Peter Grehan <grehan@...>; Laszlo Ersek <lersek@...>; Ard Biesheuvel <ardb+tianocore@...>;
Justen, Jordan L <jordan.l.justen@...>; Sean Brogan <sean.brogan@...>; Rebecca Cran <rebecca@...>
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@...; Kinney, Michael D <michael.d.kinney@...>
Cc: Peter Grehan <grehan@...>; Laszlo Ersek <lersek@...>; Ard Biesheuvel <ardb+tianocore@...>;
Justen, Jordan L <jordan.l.justen@...>; Sean Brogan <sean.brogan@...>; Rebecca Cran
<rebecca@...>;
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@...> 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@...>
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@...>

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







Laszlo Ersek
 

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


Michael D Kinney
 

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@...>
Sent: Tuesday, June 22, 2021 8:17 AM
To: Kinney, Michael D <michael.d.kinney@...>; devel@edk2.groups.io; spbrogan@...; ardb@...
Cc: Peter Grehan <grehan@...>; Ard Biesheuvel <ardb+tianocore@...>; Justen, Jordan L
<jordan.l.justen@...>; Sean Brogan <sean.brogan@...>; Rebecca Cran <rebecca@...>
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


Laszlo Ersek
 

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@...>
---
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


Laszlo Ersek
 

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@...>
Sent: Tuesday, June 22, 2021 8:17 AM
To: Kinney, Michael D <michael.d.kinney@...>; devel@edk2.groups.io; spbrogan@...; ardb@...
Cc: Peter Grehan <grehan@...>; Ard Biesheuvel <ardb+tianocore@...>; Justen, Jordan L
<jordan.l.justen@...>; Sean Brogan <sean.brogan@...>; Rebecca Cran <rebecca@...>
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


Michael D Kinney
 

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@...>
Sent: Wednesday, June 23, 2021 8:17 AM
To: Kinney, Michael D <michael.d.kinney@...>; devel@edk2.groups.io; spbrogan@...; ardb@...
Cc: Peter Grehan <grehan@...>; Ard Biesheuvel <ardb+tianocore@...>; Justen, Jordan L
<jordan.l.justen@...>; Sean Brogan <sean.brogan@...>; Rebecca Cran <rebecca@...>
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@...>
Sent: Tuesday, June 22, 2021 8:17 AM
To: Kinney, Michael D <michael.d.kinney@...>; devel@edk2.groups.io; spbrogan@...; ardb@...
Cc: Peter Grehan <grehan@...>; Ard Biesheuvel <ardb+tianocore@...>; Justen, Jordan L
<jordan.l.justen@...>; Sean Brogan <sean.brogan@...>; Rebecca Cran <rebecca@...>
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


Laszlo Ersek
 

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


Michael D Kinney
 

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@...>; devel@edk2.groups.io; spbrogan@...; ardb@...
Cc: Peter Grehan <grehan@...>; Ard Biesheuvel <ardb+tianocore@...>; Justen, Jordan L
<jordan.l.justen@...>; Sean Brogan <sean.brogan@...>; Rebecca Cran <rebecca@...>
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





Laszlo Ersek
 

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@...>
---
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@...>


Laszlo Ersek
 

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@...>
---
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


Laszlo Ersek
 

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@...>; devel@edk2.groups.io; spbrogan@...; ardb@...
Cc: Peter Grehan <grehan@...>; Ard Biesheuvel <ardb+tianocore@...>; Justen, Jordan L
<jordan.l.justen@...>; Sean Brogan <sean.brogan@...>; Rebecca Cran <rebecca@...>
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





Michael D Kinney
 

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@...>; devel@edk2.groups.io; spbrogan@...; ardb@...
Cc: Peter Grehan <grehan@...>; Ard Biesheuvel <ardb+tianocore@...>; Justen, Jordan L
<jordan.l.justen@...>; Sean Brogan <sean.brogan@...>; Rebecca Cran <rebecca@...>
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@...>; devel@edk2.groups.io; spbrogan@...; ardb@...
Cc: Peter Grehan <grehan@...>; Ard Biesheuvel <ardb+tianocore@...>; Justen, Jordan L
<jordan.l.justen@...>; Sean Brogan <sean.brogan@...>; Rebecca Cran <rebecca@...>
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