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


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@intel.com>
Sent: Thursday, June 17, 2021 2:53 PM
To: devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Peter Grehan <grehan@freebsd.org>; Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
Justen, Jordan L <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
Subject: RE: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Hi Sean,

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

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

Here is the Mergify config that is working:

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

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

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

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

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

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

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

Best regards,

Mike

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sean
Sent: Wednesday, June 16, 2021 12:00 PM
To: ardb@kernel.org; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Peter Grehan <grehan@freebsd.org>; Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
Justen, Jordan L <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>;
devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Ard,

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

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



Mike,

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


Thanks
Sean





On 6/16/2021 8:58 AM, Ard Biesheuvel wrote:
(+ Sean, Mike)

On Sat, 12 Jun 2021 at 22:43, Rebecca Cran <rebecca@bsdio.com> wrote:

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

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

Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
Strangely enough, this patch gets rejected by PatchCheck for lack of a
Signed-off-by line

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

The patch itself looks good to me

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

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

(I will go offline for 3 weeks after Friday)

---
OvmfPkg/Bhyve/BhyveX64.dsc | 64 --------------------
OvmfPkg/Bhyve/BhyveX64.fdf | 15 -----
2 files changed, 79 deletions(-)

diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
index d8792812ab..cbf896e89b 100644
--- a/OvmfPkg/Bhyve/BhyveX64.dsc
+++ b/OvmfPkg/Bhyve/BhyveX64.dsc
@@ -31,8 +31,6 @@
DEFINE SECURE_BOOT_ENABLE = FALSE
DEFINE SMM_REQUIRE = FALSE
DEFINE SOURCE_DEBUG_ENABLE = FALSE
- DEFINE TPM_ENABLE = FALSE
- DEFINE TPM_CONFIG_ENABLE = FALSE

#
# Network definition
@@ -221,16 +219,8 @@
OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf

-
-!if $(TPM_ENABLE) == TRUE
- Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
- Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
- Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
- TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
-!else
Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
-!endif

[LibraryClasses.common]
BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
@@ -292,11 +282,6 @@
CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf

-!if $(TPM_ENABLE) == TRUE
- BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
- Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
-!endif
-
MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf

[LibraryClasses.common.DXE_CORE]
@@ -366,9 +351,6 @@
!endif
PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
MpInitLib|UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf
-!if $(TPM_ENABLE) == TRUE
- Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
-!endif

[LibraryClasses.common.UEFI_APPLICATION]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
@@ -563,22 +545,12 @@

gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00

-!if $(TPM_ENABLE) == TRUE
- gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00,
0x00, 0x00, 0x00, 0x00, 0x00}
-!endif
-
# MdeModulePkg resolution sets up the system display resolution
gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution|0
gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution|0
gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|0
gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|0

-[PcdsDynamicHii]
-!if $(TPM_ENABLE) == TRUE && $(TPM_CONFIG_ENABLE) == TRUE
-
gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
- gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
-!endif
-
################################################################################
#
# Components Section - list of all EDK II Modules needed by this Platform.
@@ -618,19 +590,6 @@
<LibraryClasses>
}

-!if $(TPM_ENABLE) == TRUE
- OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
- SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
- <LibraryClasses>
- HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
- NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
- NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
- NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
- NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
- NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
- }
-!endif
-
#
# DXE Phase modules
#
@@ -653,9 +612,6 @@
<LibraryClasses>
!if $(SECURE_BOOT_ENABLE) == TRUE
NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
-!endif
-!if $(TPM_ENABLE) == TRUE
- NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
!endif
}

@@ -841,23 +797,3 @@
NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
}

-
- #
- # TPM support
- #
-!if $(TPM_ENABLE) == TRUE
- SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
- <LibraryClasses>
- Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
- NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
- HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
- NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
- NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
- NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
- NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
- NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
- }
-!if $(TPM_CONFIG_ENABLE) == TRUE
- SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
-!endif
-!endif
diff --git a/OvmfPkg/Bhyve/BhyveX64.fdf b/OvmfPkg/Bhyve/BhyveX64.fdf
index 3eff36dac1..fbd63a395a 100644
--- a/OvmfPkg/Bhyve/BhyveX64.fdf
+++ b/OvmfPkg/Bhyve/BhyveX64.fdf
@@ -158,11 +158,6 @@ INF UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
INF OvmfPkg/Bhyve/SmmAccess/SmmAccessPei.inf
!endif

-!if $(TPM_ENABLE) == TRUE
-INF OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
-INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
-!endif
-
################################################################################

[FV.DXEFV]
@@ -333,16 +328,6 @@ INF MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
INF MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
!endif

-#
-# TPM support
-#
-!if $(TPM_ENABLE) == TRUE
-INF SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
-!if $(TPM_CONFIG_ENABLE) == TRUE
-INF SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
-!endif
-!endif
-
################################################################################

[FV.FVMAIN_COMPACT]
--
2.32.0






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