[edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
Michael D Kinney
michael.d.kinney at intel.com
Fri Jun 18 04:11:44 UTC 2021
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 at intel.com>
> Sent: Thursday, June 17, 2021 2:55 PM
> To: devel at edk2.groups.io; spbrogan at outlook.com; ardb at kernel.org; Kinney, Michael D <michael.d.kinney at intel.com>
> Cc: Peter Grehan <grehan at freebsd.org>; Laszlo Ersek <lersek at redhat.com>; Ard Biesheuvel <ardb+tianocore at kernel.org>;
> Justen, Jordan L <jordan.l.justen at intel.com>; Sean Brogan <sean.brogan at microsoft.com>; Rebecca Cran <rebecca at bsdio.com>
> Subject: RE: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
>
> Link to Mergify queue feature documentation
>
> https://docs.mergify.io/actions/queue/#merge-queue-problem
>
> Mike
>
> > -----Original Message-----
> > From: Kinney, Michael D <michael.d.kinney at intel.com>
> > Sent: Thursday, June 17, 2021 2:53 PM
> > To: devel at edk2.groups.io; spbrogan at outlook.com; ardb at kernel.org; Kinney, Michael D <michael.d.kinney at intel.com>
> > Cc: Peter Grehan <grehan at freebsd.org>; Laszlo Ersek <lersek at redhat.com>; Ard Biesheuvel <ardb+tianocore at kernel.org>;
> > Justen, Jordan L <jordan.l.justen at intel.com>; Sean Brogan <sean.brogan at microsoft.com>; Rebecca Cran <rebecca at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Sean
> > > Sent: Wednesday, June 16, 2021 12:00 PM
> > > To: ardb at kernel.org; Kinney, Michael D <michael.d.kinney at intel.com>
> > > Cc: Peter Grehan <grehan at freebsd.org>; Laszlo Ersek <lersek at redhat.com>; Ard Biesheuvel <ardb+tianocore at kernel.org>;
> > > Justen, Jordan L <jordan.l.justen at intel.com>; Sean Brogan <sean.brogan at microsoft.com>; Rebecca Cran
> <rebecca at bsdio.com>;
> > > devel at 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 at 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 at 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 at kernel.org>
> > > >
> > > > so if anyone else manages to fix the CI issue, feel free to push the
> > > > patch with my R-b (and Peter's, given in reply to this message)
> > > >
> > > > (I will go offline for 3 weeks after Friday)
> > > >
> > > >> ---
> > > >> OvmfPkg/Bhyve/BhyveX64.dsc | 64 --------------------
> > > >> OvmfPkg/Bhyve/BhyveX64.fdf | 15 -----
> > > >> 2 files changed, 79 deletions(-)
> > > >>
> > > >> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
> > > >> index d8792812ab..cbf896e89b 100644
> > > >> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
> > > >> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
> > > >> @@ -31,8 +31,6 @@
> > > >> DEFINE SECURE_BOOT_ENABLE = FALSE
> > > >> DEFINE SMM_REQUIRE = FALSE
> > > >> DEFINE SOURCE_DEBUG_ENABLE = FALSE
> > > >> - DEFINE TPM_ENABLE = FALSE
> > > >> - DEFINE TPM_CONFIG_ENABLE = FALSE
> > > >>
> > > >> #
> > > >> # Network definition
> > > >> @@ -221,16 +219,8 @@
> > > >>
> OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
> > > >> XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf
> > > >>
> > > >> -
> > > >> -!if $(TPM_ENABLE) == TRUE
> > > >> - Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
> > > >> - Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
> > > >> - Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
> > > >> - TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> > > >> -!else
> > > >> Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
> > > >> TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
> > > >> -!endif
> > > >>
> > > >> [LibraryClasses.common]
> > > >> BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> > > >> @@ -292,11 +282,6 @@
> > > >> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
> > > >> PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
> > > >>
> > > >> -!if $(TPM_ENABLE) == TRUE
> > > >> - BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> > > >> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
> > > >> -!endif
> > > >> -
> > > >> MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
> > > >>
> > > >> [LibraryClasses.common.DXE_CORE]
> > > >> @@ -366,9 +351,6 @@
> > > >> !endif
> > > >> PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
> > > >> MpInitLib|UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf
> > > >> -!if $(TPM_ENABLE) == TRUE
> > > >> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
> > > >> -!endif
> > > >>
> > > >> [LibraryClasses.common.UEFI_APPLICATION]
> > > >> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> > > >> @@ -563,22 +545,12 @@
> > > >>
> > > >> gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
> > > >>
> > > >> -!if $(TPM_ENABLE) == TRUE
> > > >> - gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > 0x00,
> > > 0x00, 0x00, 0x00, 0x00, 0x00}
> > > >> -!endif
> > > >> -
> > > >> # MdeModulePkg resolution sets up the system display resolution
> > > >> gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution|0
> > > >> gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution|0
> > > >> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|0
> > > >> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|0
> > > >>
> > > >> -[PcdsDynamicHii]
> > > >> -!if $(TPM_ENABLE) == TRUE && $(TPM_CONFIG_ENABLE) == TRUE
> > > >> -
> > >
> gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
> > > >> - gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
> > > >> -!endif
> > > >> -
> > > >> ################################################################################
> > > >> #
> > > >> # Components Section - list of all EDK II Modules needed by this Platform.
> > > >> @@ -618,19 +590,6 @@
> > > >> <LibraryClasses>
> > > >> }
> > > >>
> > > >> -!if $(TPM_ENABLE) == TRUE
> > > >> - OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> > > >> - SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> > > >> - <LibraryClasses>
> > > >> - HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
> > > >> - NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> > > >> - NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> > > >> - NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> > > >> - NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> > > >> - NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> > > >> - }
> > > >> -!endif
> > > >> -
> > > >> #
> > > >> # DXE Phase modules
> > > >> #
> > > >> @@ -653,9 +612,6 @@
> > > >> <LibraryClasses>
> > > >> !if $(SECURE_BOOT_ENABLE) == TRUE
> > > >> NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> > > >> -!endif
> > > >> -!if $(TPM_ENABLE) == TRUE
> > > >> - NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
> > > >> !endif
> > > >> }
> > > >>
> > > >> @@ -841,23 +797,3 @@
> > > >> NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
> > > >> }
> > > >>
> > > >> -
> > > >> - #
> > > >> - # TPM support
> > > >> - #
> > > >> -!if $(TPM_ENABLE) == TRUE
> > > >> - SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
> > > >> - <LibraryClasses>
> > > >> - Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
> > > >> - NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
> > > >> - HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
> > > >> - NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> > > >> - NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> > > >> - NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> > > >> - NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> > > >> - NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> > > >> - }
> > > >> -!if $(TPM_CONFIG_ENABLE) == TRUE
> > > >> - SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> > > >> -!endif
> > > >> -!endif
> > > >> diff --git a/OvmfPkg/Bhyve/BhyveX64.fdf b/OvmfPkg/Bhyve/BhyveX64.fdf
> > > >> index 3eff36dac1..fbd63a395a 100644
> > > >> --- a/OvmfPkg/Bhyve/BhyveX64.fdf
> > > >> +++ b/OvmfPkg/Bhyve/BhyveX64.fdf
> > > >> @@ -158,11 +158,6 @@ INF UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> > > >> INF OvmfPkg/Bhyve/SmmAccess/SmmAccessPei.inf
> > > >> !endif
> > > >>
> > > >> -!if $(TPM_ENABLE) == TRUE
> > > >> -INF OvmfPkg/Bhyve/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> > > >> -INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> > > >> -!endif
> > > >> -
> > > >> ################################################################################
> > > >>
> > > >> [FV.DXEFV]
> > > >> @@ -333,16 +328,6 @@ INF MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
> > > >> INF MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> > > >> !endif
> > > >>
> > > >> -#
> > > >> -# TPM support
> > > >> -#
> > > >> -!if $(TPM_ENABLE) == TRUE
> > > >> -INF SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
> > > >> -!if $(TPM_CONFIG_ENABLE) == TRUE
> > > >> -INF SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
> > > >> -!endif
> > > >> -!endif
> > > >> -
> > > >> ################################################################################
> > > >>
> > > >> [FV.FVMAIN_COMPACT]
> > > >> --
> > > >> 2.32.0
> > > >>
> > > >>
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76735): https://edk2.groups.io/g/devel/message/76735
Mute This Topic: https://groups.io/mt/83497624/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list