Re: [edk2-platforms][PATCH V1 1/2] KabylakeSiliconPkg: Remove ResetSystemLib.h override

["Kubacki, Michael A" <michael.a.kubacki@intel.com>]

This dependency existed prior to this change (and still does exist). It was obfuscated in such a way that contributed to this problem.

See the previous library header path:
  \Silicon\Intel\KabylakeSiliconPkg\SampleCode\MdeModulePkg\Include\Library\ResetSystemLib.h

The fact KabylakeSiliconPkg implements a MdeModulePkg library API introduces the dependency on MdeModulePkg. Hiding a redundant definition of the API locally does not eliminate the dependency in any meaningful way.

I think the practice of "freezing" an API with a local copy only works if the codebase is locked onto a specific stable tag in which the upstream API is not expected to change. Zhichao rightfully added the new function definition to the KabylakeSiliconPkg library class implementation because a board package consumer would expect a ResetSystemLib library class instance to be compliant with the API defined in MdeModulePkg and link the ResetSystem () function. The only problem was a set of circumstances that led to the duplicate symbol definition for ResetSystem () with PchResetLib.

So I view the task of eliminating the package dependency as a larger separate effort outside the scope of this change. But I do not agree with maintaining redundant local copies of edk2 APIs in packages in edk2-platforms.

Thanks,
Michael

> -----Original Message-----
> From: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>
> Sent: Tuesday, November 26, 2019 10:43 PM
> To: Kubacki, Michael A <michael.a.kubacki@intel.com>;
> devel@edk2.groups.io
> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>
> Subject: RE: [edk2-platforms][PATCH V1 1/2] KabylakeSiliconPkg: Remove
> ResetSystemLib.h override
> 
> This change is introducing SiliconPkg's dependency on MdeModulePkg.
> SiliconPkg dependencies should be limited to a selected few packages and
> this seems to be an unnecessary addition to the dependency list.
> The reset interfaces are providing generic reset services and perhaps better
> suited in packages like MdePkg.
> 
> -----Original Message-----
> From: Kubacki, Michael A
> Sent: Tuesday, November 26, 2019 6:57 PM
> To: devel@edk2.groups.io
> Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Chiu, Chasel
> <chasel.chiu@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>
> Subject: [edk2-platforms][PATCH V1 1/2] KabylakeSiliconPkg: Remove
> ResetSystemLib.h override
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2375
> 
> Removes a stale ResetSystemLib.h override in KabylakeSiliconPkg that does
> not contain the prototype for ResetSystem () and ResetPlatformSpecific ().
> 
> The ResetSystemLib.h file from MdeModulePkg will be used. Any INF files
> that did not include the MdeModulePkg.dec under [Packages] were updated
> to do so.
> 
> Cc: Sai Chaganty <rangasai.v.chaganty@intel.com>
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> Signed-off-by: Michael Kubacki <michael.a.kubacki@intel.com>
> ---
> 
> Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeResetSystemLib/DxeResetS
> ystemLib.inf               |  3 +-
> 
> Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeRuntimeResetSystemLib/Dx
> eRuntimeResetSystemLib.inf |  3 +-
> 
> Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiPchResetLib/PeiPchResetLib.
> inf                     |  3 +-
> 
> Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiResetSystemLib/PeiResetSys
> temLib.inf               |  3 +-
> 
> Silicon/Intel/KabylakeSiliconPkg/SampleCode/MdeModulePkg/Include/Libra
> ry/ResetSystemLib.h          | 62 --------------------
>  5 files changed, 8 insertions(+), 66 deletions(-)
> 
> diff --git
> a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeResetSystemLib/DxeRese
> tSystemLib.inf
> b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeResetSystemLib/DxeRese
> tSystemLib.inf
> index aa8877140a..46313bf35f 100644
> ---
> a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeResetSystemLib/DxeRese
> tSystemLib.inf
> +++ b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeResetSystemLib/Dxe
> +++ ResetSystemLib.inf
> @@ -1,7 +1,7 @@
>  ## @file
>  # Component description file for Intel Ich7 Reset System Library.
>  #
> -# Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2017 - 2019, Intel Corporation. All rights
> +reserved.<BR>
>  #
>  # SPDX-License-Identifier: BSD-2-Clause-Patent  # @@ -35,6 +35,7 @@
> PchCycleDecodingLib
> 
>  [Packages]
>  MdePkg/MdePkg.dec
> +MdeModulePkg/MdeModulePkg.dec
>  KabylakeSiliconPkg/SiPkg.dec
> 
> 
> diff --git
> a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeRuntimeResetSystemLib/
> DxeRuntimeResetSystemLib.inf
> b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeRuntimeResetSystemLib/
> DxeRuntimeResetSystemLib.inf
> index 6b27661603..c7fad31c71 100644
> ---
> a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeRuntimeResetSystemLib/
> DxeRuntimeResetSystemLib.inf
> +++
> b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeRuntimeResetSystem
> +++ Lib/DxeRuntimeResetSystemLib.inf
> @@ -1,7 +1,7 @@
>  ## @file
>  # Component description file for Intel Ich7 Reset System Library.
>  #
> -# Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2017 - 2019, Intel Corporation. All rights
> +reserved.<BR>
>  #
>  # SPDX-License-Identifier: BSD-2-Clause-Patent  # @@ -36,6 +36,7 @@
> PchCycleDecodingLib
> 
>  [Packages]
>  MdePkg/MdePkg.dec
> +MdeModulePkg/MdeModulePkg.dec
>  KabylakeSiliconPkg/SiPkg.dec
> 
> 
> diff --git
> a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiPchResetLib/PeiPchResetL
> ib.inf
> b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiPchResetLib/PeiPchResetL
> ib.inf
> index b04f4006ef..29f69078a4 100644
> ---
> a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiPchResetLib/PeiPchResetL
> ib.inf
> +++ b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiPchResetLib/PeiPch
> +++ ResetLib.inf
> @@ -1,7 +1,7 @@
>  ## @file
>  # Component description file for PCH Reset Lib Pei Phase  # -# Copyright (c)
> 2017, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2017 - 2019, Intel Corporation. All rights
> +reserved.<BR>
>  #
>  # SPDX-License-Identifier: BSD-2-Clause-Patent  # @@ -28,6 +28,7 @@
> ResetSystemLib
> 
>  [Packages]
>  MdePkg/MdePkg.dec
> +MdeModulePkg/MdeModulePkg.dec
>  KabylakeSiliconPkg/SiPkg.dec
> 
>  [Sources]
> diff --git
> a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiResetSystemLib/PeiResetS
> ystemLib.inf
> b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiResetSystemLib/PeiReset
> SystemLib.inf
> index 18a92a6f18..3c6ff78863 100644
> ---
> a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiResetSystemLib/PeiResetS
> ystemLib.inf
> +++ b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiResetSystemLib/Pei
> +++ ResetSystemLib.inf
> @@ -1,7 +1,7 @@
>  ## @file
>  # Component description file for Intel Ich7 Reset System Library.
>  #
> -# Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2017 - 2019, Intel Corporation. All rights
> +reserved.<BR>
>  #
>  # SPDX-License-Identifier: BSD-2-Clause-Patent  # @@ -32,6 +32,7 @@
> PchCycleDecodingLib
> 
>  [Packages]
>  MdePkg/MdePkg.dec
> +MdeModulePkg/MdeModulePkg.dec
>  KabylakeSiliconPkg/SiPkg.dec
> 
> 
> diff --git
> a/Silicon/Intel/KabylakeSiliconPkg/SampleCode/MdeModulePkg/Include/Lib
> rary/ResetSystemLib.h
> b/Silicon/Intel/KabylakeSiliconPkg/SampleCode/MdeModulePkg/Include/Lib
> rary/ResetSystemLib.h
> deleted file mode 100644
> index 75d3e15ed7..0000000000
> ---
> a/Silicon/Intel/KabylakeSiliconPkg/SampleCode/MdeModulePkg/Include/Lib
> rary/ResetSystemLib.h
> +++ /dev/null
> @@ -1,62 +0,0 @@
> -/** @file
> -  System reset Library Services.  This library class defines a set of
> -  methods that reset the whole system.
> -
> -Copyright (c) 2005 - 2010, Intel Corporation. All rights reserved.<BR>
> -SPDX-License-Identifier: BSD-2-Clause-Patent
> -
> -**/
> -
> -#ifndef __RESET_SYSTEM_LIB_H__
> -#define __RESET_SYSTEM_LIB_H__
> -
> -/**
> -  This function causes a system-wide reset (cold reset), in which
> -  all circuitry within the system returns to its initial state. This type of reset
> -  is asynchronous to system operation and operates without regard to
> -  cycle boundaries.
> -
> -  If this function returns, it means that the system does not support cold
> reset.
> -**/
> -VOID
> -EFIAPI
> -ResetCold (
> -  VOID
> -  );
> -
> -/**
> -  This function causes a system-wide initialization (warm reset), in which all
> processors
> -  are set to their initial state. Pending cycles are not corrupted.
> -
> -  If this function returns, it means that the system does not support warm
> reset.
> -**/
> -VOID
> -EFIAPI
> -ResetWarm (
> -  VOID
> -  );
> -
> -/**
> -  This function causes the system to enter a power state equivalent
> -  to the ACPI G2/S5 or G3 states.
> -
> -  If this function returns, it means that the system does not support
> shutdown reset.
> -**/
> -VOID
> -EFIAPI
> -ResetShutdown (
> -  VOID
> -  );
> -
> -/**
> -  This function causes the system to enter S3 and then wake up immediately.
> -
> -  If this function returns, it means that the system does not support S3
> feature.
> -**/
> -VOID
> -EFIAPI
> -EnterS3WithImmediateWake (
> -  VOID
> -  );
> -
> -#endif
> --
> 2.16.2.windows.1
>