* [PATCH] MinPlatformPkg: Support TCO base locked by FSP @ 2019-01-15 4:07 Chasel, Chiu 2019-01-15 4:15 ` Chiu, Chasel 0 siblings, 1 reply; 4+ messages in thread From: Chasel, Chiu @ 2019-01-15 4:07 UTC (permalink / raw) To: edk2-devel; +Cc: Nate DeSimone, Star Zeng, Chasel Chiu 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.desimone@intel.com> Cc: Star Zeng <star.zeng@intel.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Chasel Chiu <chasel.chiu@intel.com> --- Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiDxeSmmPchCycleDecodingLib/PchCycleDecodingLib.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiDxeSmmPchCycleDecodingLib/PchCycleDecodingLib.c b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiDxeSmmPchCycleDecodingLib/PchCycleDecodingLib.c index 68b0b5dd4b..e135ef1f3e 100644 --- a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiDxeSmmPchCycleDecodingLib/PchCycleDecodingLib.c +++ b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiDxeSmmPchCycleDecodingLib/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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] MinPlatformPkg: Support TCO base locked by FSP 2019-01-15 4:07 [PATCH] MinPlatformPkg: Support TCO base locked by FSP Chasel, Chiu @ 2019-01-15 4:15 ` Chiu, Chasel 2019-01-15 8:11 ` Kubacki, Michael A 0 siblings, 1 reply; 4+ messages in thread From: Chiu, Chasel @ 2019-01-15 4:15 UTC (permalink / raw) To: edk2-devel@lists.01.org Cc: Desimone, Nathaniel L, Zeng, Star, Kubacki, Michael A + 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.desimone@intel.com>; Zeng, Star > <star.zeng@intel.com>; Chiu, Chasel <chasel.chiu@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.desimone@intel.com> > Cc: Star Zeng <star.zeng@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Chasel Chiu <chasel.chiu@intel.com> > --- > > Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiDxeSmmPchCycleDecodingLib > /PchCycleDecodingLib.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git > a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiDxeSmmPchCycleDecodingLi > b/PchCycleDecodingLib.c > b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiDxeSmmPchCycleDecodingL > ib/PchCycleDecodingLib.c > index 68b0b5dd4b..e135ef1f3e 100644 > --- > a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiDxeSmmPchCycleDecodingLi > b/PchCycleDecodingLib.c > +++ b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiDxeSmmPchCycleDeco > +++ 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] MinPlatformPkg: Support TCO base locked by FSP 2019-01-15 4:15 ` Chiu, Chasel @ 2019-01-15 8:11 ` Kubacki, Michael A 2019-01-15 8:16 ` Chiu, Chasel 0 siblings, 1 reply; 4+ messages in thread From: Kubacki, Michael A @ 2019-01-15 8:11 UTC (permalink / raw) To: Chiu, Chasel, edk2-devel@lists.01.org; +Cc: Desimone, Nathaniel L, Zeng, Star 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.desimone@intel.com>; Zeng, Star > <star.zeng@intel.com>; Kubacki, Michael A <michael.a.kubacki@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.desimone@intel.com>; Zeng, Star > > <star.zeng@intel.com>; Chiu, Chasel <chasel.chiu@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.desimone@intel.com> > > Cc: Star Zeng <star.zeng@intel.com> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Chasel Chiu <chasel.chiu@intel.com> > > --- > > > > Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiDxeSmmPchCycleDecoding > > Lib /PchCycleDecodingLib.c | 17 +++++++++-------- > > 1 file changed, 9 insertions(+), 8 deletions(-) > > > > diff --git > > a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiDxeSmmPchCycleDecodi > > ngLi > > b/PchCycleDecodingLib.c > > b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiDxeSmmPchCycleDecodi > > ngL > > ib/PchCycleDecodingLib.c > > index 68b0b5dd4b..e135ef1f3e 100644 > > --- > > a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiDxeSmmPchCycleDecodi > > ngLi > > b/PchCycleDecodingLib.c > > +++ b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiDxeSmmPchCycleDe > > +++ 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] MinPlatformPkg: Support TCO base locked by FSP 2019-01-15 8:11 ` Kubacki, Michael A @ 2019-01-15 8:16 ` Chiu, Chasel 0 siblings, 0 replies; 4+ messages in thread From: Chiu, Chasel @ 2019-01-15 8:16 UTC (permalink / raw) To: Kubacki, Michael A, edk2-devel@lists.01.org Cc: Desimone, Nathaniel L, Zeng, Star 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.chiu@intel.com>; edk2-devel@lists.01.org > Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Zeng, Star > <star.zeng@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.desimone@intel.com>; Zeng, Star > > <star.zeng@intel.com>; Kubacki, Michael A > > <michael.a.kubacki@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.desimone@intel.com>; Zeng, > > > Star <star.zeng@intel.com>; Chiu, Chasel <chasel.chiu@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.desimone@intel.com> > > > Cc: Star Zeng <star.zeng@intel.com> > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > Signed-off-by: Chasel Chiu <chasel.chiu@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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-01-15 8:18 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-01-15 4:07 [PATCH] MinPlatformPkg: Support TCO base locked by FSP Chasel, Chiu 2019-01-15 4:15 ` Chiu, Chasel 2019-01-15 8:11 ` Kubacki, Michael A 2019-01-15 8:16 ` Chiu, Chasel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox