Good point! I will send another patch for this.
Thanks! Chasel > -----Original Message----- > From: Kubacki, Michael A > Sent: Tuesday, January 15, 2019 4:12 PM > To: Chiu, Chasel <chasel.c...@intel.com>; edk2-devel@lists.01.org > Cc: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com>; Zeng, Star > <star.z...@intel.com> > Subject: RE: [PATCH] MinPlatformPkg: Support TCO base locked by FSP > > According to the function description, PchTcoBaseSet ( ) should ensure the > following steps are done before returning success: > 1. set Smbus PCI offset 54h [8] to enable TCO base address. > 2. program Smbus PCI offset 50h [15:5] to TCO base address. > 3. set Smbus PCI offset 54h [8] to enable TCO base address. > 4. program "TCO Base Address" PCR[DMI] + 2778h[15:5, 1] to [Smbus PCI offset > 50h[15:5], 1]. > > Currently the patch updates PchTcoBaseSet ( ) to return EFI_SUCCESS if it > finds > the TCOCTL lock is already set, however, it doesn't test the other conditions > are > met. This is different from returning EFI_SUCCESS if the lock is not already > set. > The lock could have erroneously been set by HW/SW and this would return that > the PCH TCO base address was successfully set when it may not be. > > What about adding a PchTcoIsLocked () that checks if the lock is set and the > caller > not call PchTcoBaseSet () if PchTcoIsLocked () returns true? Then > PchTcoBaseSet > () can continue to return an error if it cannot update the base address. > > Regards, > Michael > > > -----Original Message----- > > From: Chiu, Chasel > > Sent: Monday, January 14, 2019 8:15 PM > > To: edk2-devel@lists.01.org > > Cc: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com>; Zeng, Star > > <star.z...@intel.com>; Kubacki, Michael A > > <michael.a.kuba...@intel.com> > > Subject: RE: [PATCH] MinPlatformPkg: Support TCO base locked by FSP > > > > > > + Michael to review this too. > > > > Thanks! > > Chasel > > > > > > > -----Original Message----- > > > From: Chiu, Chasel > > > Sent: Tuesday, January 15, 2019 12:07 PM > > > To: edk2-devel@lists.01.org > > > Cc: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com>; Zeng, > > > Star <star.z...@intel.com>; Chiu, Chasel <chasel.c...@intel.com> > > > Subject: [PATCH] MinPlatformPkg: Support TCO base locked by FSP > > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1457 > > > > > > Per security recommendation TCO Base should be initialized and > > > locked by FSP and MinPlatform should support both TCO Base locked > > > and not locked > > scenarios. > > > > > > Cc: Nate DeSimone <nathaniel.l.desim...@intel.com> > > > Cc: Star Zeng <star.z...@intel.com> > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > Signed-off-by: Chasel Chiu <chasel.c...@intel.com> > > > --- > > > > > > Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiDxeSmmPchCycleDecodi > > > ng Lib /PchCycleDecodingLib.c | 17 +++++++++-------- > > > 1 file changed, 9 insertions(+), 8 deletions(-) > > > > > > diff --git > > > a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiDxeSmmPchCycleDeco > > > di > > > ngLi > > > b/PchCycleDecodingLib.c > > > b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiDxeSmmPchCycleDeco > > > di > > > ngL > > > ib/PchCycleDecodingLib.c > > > index 68b0b5dd4b..e135ef1f3e 100644 > > > --- > > > a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiDxeSmmPchCycleDeco > > > di > > > ngLi > > > b/PchCycleDecodingLib.c > > > +++ b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiDxeSmmPchCycle > > > +++ De > > > +++ co > > > +++ dingLib/PchCycleDecodingLib.c > > > @@ -1,7 +1,7 @@ > > > /** @file > > > PCH cycle deocding configuration and query library. > > > > > > -Copyright (c) 2017, Intel Corporation. All rights reserved.<BR> > > > +Copyright (c) 2017 - 2019, Intel Corporation. All rights > > > +reserved.<BR> > > > This program and the accompanying materials are licensed and made > > > available under the terms and conditions of the BSD License that > > > accompanies this distribution. > > > The full text of the license may be found at @@ -352,17 +352,18 @@ > > > PchTcoBaseSet ( > > > } > > > // > > > // Verify TCO base is not locked. > > > + // If it is locked already, skip following steps. > > > // > > > if ((MmioRead8 (SmbusBase + R_PCH_SMBUS_TCOCTL) & > > > B_PCH_SMBUS_TCOCTL_TCO_BASE_LOCK) != 0) { > > > - ASSERT (FALSE); > > > - return EFI_DEVICE_ERROR; > > > + return EFI_SUCCESS; > > > } > > > // > > > // Disable TCO in SMBUS Device first before changing base address. > > > + // Byte access to not touch the TCO_BASE_LOCK bit > > > // > > > - MmioAnd16 ( > > > - SmbusBase + R_PCH_SMBUS_TCOCTL, > > > - (UINT16) ~B_PCH_SMBUS_TCOCTL_TCO_BASE_EN > > > + MmioAnd8 ( > > > + SmbusBase + R_PCH_SMBUS_TCOCTL + 1, > > > + (UINT8) ~(B_PCH_SMBUS_TCOCTL_TCO_BASE_EN >> 8) > > > ); > > > // > > > // Program TCO in SMBUS Device > > > @@ -373,11 +374,11 @@ PchTcoBaseSet ( > > > Address > > > ); > > > // > > > - // Enable TCO in SMBUS Device > > > + // Enable TCO in SMBUS Device and lock TCO BASE > > > // > > > MmioOr16 ( > > > SmbusBase + R_PCH_SMBUS_TCOCTL, > > > - B_PCH_SMBUS_TCOCTL_TCO_BASE_EN > > > + B_PCH_SMBUS_TCOCTL_TCO_BASE_EN | > > > B_PCH_SMBUS_TCOCTL_TCO_BASE_LOCK > > > ); > > > // > > > // Program "TCO Base Address" PCR[DMI] + 2778h[15:5, 1] to [SMBUS > > > PCI offset 50h[15:5], 1]. > > > -- > > > 2.13.3.windows.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
