subject:"\[edk2\] \[PATCH\] MinPlatformPkg\: Support TCO base locked by FSP"

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

2019-01-15 Thread 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 ; edk2-devel@lists.01.org
> Cc: Desimone, Nathaniel L ; Zeng, Star
> 
> 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 ; Zeng, Star
> > ; Kubacki, Michael A
> > 
> > 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 ; Zeng,
> > > Star ; Chiu, Chasel 
> > > 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 
> > > Cc: Star Zeng 
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Chasel Chiu 
> > > ---
> > >
> > > 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.
> > > +Copyright (c) 2017 - 2019, Intel Corporation. All rights
> > > +reserved.
> > >  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
> > >

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

2019-01-15 Thread 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 ; Zeng, Star
> ; Kubacki, Michael A 
> 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 ; Zeng, Star
> > ; Chiu, Chasel 
> > 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 
> > Cc: Star Zeng 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Chasel Chiu 
> > ---
> >
> > 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.
> > +Copyright (c) 2017 - 2019, Intel Corporation. All rights
> > +reserved.
> >  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

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

2019-01-14 Thread 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 ; Zeng, Star
> ; Chiu, Chasel 
> 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 
> Cc: Star Zeng 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Chasel Chiu 
> ---
> 
> 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.
> +Copyright (c) 2017 - 2019, Intel Corporation. All rights reserved.
>  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

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

2019-01-14 Thread 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 
Cc: Star Zeng 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Chasel Chiu 
---
 
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.
+Copyright (c) 2017 - 2019, Intel Corporation. All rights reserved.
 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

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

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

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

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

4 matches

Site Navigation

Mail list logo

Footer information