[PATCH] MinPlatformPkg: Support TCO base locked by FSP

[PATCH] MinPlatformPkg: Support TCO base locked by FSP

[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

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

[Chiu, Chasel]

+ 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

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

[Kubacki, Michael A]

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

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

[Chiu, Chasel]

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