Re: [PATCH] MinPlatformPkg: Support TCO base locked by FSP

["Chiu, Chasel" <chasel.chiu@intel.com>]

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