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

["Michael D Kinney" <michael.d.kinney@intel.com>]

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