From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by mx.groups.io with SMTP id smtpd.web11.1296.1575275814799796415 for ; Mon, 02 Dec 2019 00:36:54 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.88, mailfrom: rangasai.v.chaganty@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 02 Dec 2019 00:36:54 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,268,1571727600"; d="scan'208";a="212958007" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by orsmga006.jf.intel.com with ESMTP; 02 Dec 2019 00:36:53 -0800 Received: from fmsmsx156.amr.corp.intel.com (10.18.116.74) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 2 Dec 2019 00:36:53 -0800 Received: from fmsmsx104.amr.corp.intel.com ([169.254.3.245]) by fmsmsx156.amr.corp.intel.com ([169.254.13.184]) with mapi id 14.03.0439.000; Mon, 2 Dec 2019 00:36:52 -0800 From: "Chaganty, Rangasai V" To: "Kubacki, Michael A" , "devel@edk2.groups.io" CC: "Chiu, Chasel" , "Desimone, Nathaniel L" , "Gao, Zhichao" Subject: Re: [edk2-platforms][PATCH V1 1/2] KabylakeSiliconPkg: Remove ResetSystemLib.h override Thread-Topic: [edk2-platforms][PATCH V1 1/2] KabylakeSiliconPkg: Remove ResetSystemLib.h override Thread-Index: AQHVpM5X0s/aoxngbkySRqgbHjTTV6eej95QgAFRNoCAAGHBAIAGQv1Q Date: Mon, 2 Dec 2019 08:36:51 +0000 Message-ID: References: <20191127025643.32056-1-michael.a.kubacki@intel.com> <20191127025643.32056-2-michael.a.kubacki@intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZmQ0NzRhYzYtY2RjYi00ODZkLWJlOWYtNGFmZGVhNWJiM2U3IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiVnVpM293MUNZV29IS0Zld1dcL0I0cnJWS0czSHdcLzliTjkzMmxpSDNpYVBYbzlaSmVEY1JqWm1aYjdCVkIrXC9BVCJ9 x-ctpclassification: CTP_NT x-originating-ip: [10.1.200.106] MIME-Version: 1.0 Return-Path: rangasai.v.chaganty@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Michael,=20 I agree on eliminating the redundant copies Edk2 APIs from Edk2Platform pac= kages and I also think it can be done without introducing newer dependencie= s as indicated in my previous response. That said, the topic of package dependencies, especially for silicon initia= lization code needs a broader discussion and is outside the scope of this r= eview. Let's take care of this change for now and address the VS2017 build issue a= nd let's be prepared for further changes as we make progress on the package= dependency discussions. Reviewed-by: Sai Chaganty Thanks, Sai -----Original Message----- From: Kubacki, Michael A =20 Sent: Wednesday, November 27, 2019 4:31 PM To: Chaganty, Rangasai V ; devel@edk2.groups= .io Cc: Chiu, Chasel ; Desimone, Nathaniel L ; Gao, Zhichao Subject: RE: [edk2-platforms][PATCH V1 1/2] KabylakeSiliconPkg: Remove Rese= tSystemLib.h override Hi Sai, I'd like to fix the VS2017 build failure asap. What would you like done to = resolve this? I would prefer to eliminate the local ResetSystemLib.h file in KabylakeSili= conPkg but I'd be happy to revise the patch series based on your suggestion= . Thanks, Michael > -----Original Message----- > From: Kubacki, Michael A > Sent: Wednesday, November 27, 2019 10:42 AM > To: Chaganty, Rangasai V ;=20 > devel@edk2.groups.io > Cc: Chiu, Chasel ; Desimone, Nathaniel L=20 > ; Gao, Zhichao > Subject: RE: [edk2-platforms][PATCH V1 1/2] KabylakeSiliconPkg: Remove=20 > ResetSystemLib.h override >=20 > This dependency existed prior to this change (and still does exist).=20 > It was obfuscated in such a way that contributed to this problem. >=20 > See the previous library header path: >=20 > \Silicon\Intel\KabylakeSiliconPkg\SampleCode\MdeModulePkg\Include\Libr > ary\ResetSystemLib.h >=20 > The fact KabylakeSiliconPkg implements a MdeModulePkg library API=20 > introduces the dependency on MdeModulePkg. Hiding a redundant=20 > definition of the API locally does not eliminate the dependency in any=20 > meaningful way. >=20 > I think the practice of "freezing" an API with a local copy only works=20 > if the codebase is locked onto a specific stable tag in which the=20 > upstream API is not expected to change. Zhichao rightfully added the=20 > new function definition to the KabylakeSiliconPkg library class=20 > implementation because a board package consumer would expect a=20 > ResetSystemLib library class instance to be compliant with the API=20 > defined in MdeModulePkg and link the ResetSystem > () function. The only problem was a set of circumstances that led to=20 > the duplicate symbol definition for ResetSystem () with PchResetLib. >=20 > So I view the task of eliminating the package dependency as a larger=20 > separate effort outside the scope of this change. But I do not agree=20 > with maintaining redundant local copies of edk2 APIs in packages in edk2-= platforms. >=20 > Thanks, > Michael >=20 > > -----Original Message----- > > From: Chaganty, Rangasai V > > Sent: Tuesday, November 26, 2019 10:43 PM > > To: Kubacki, Michael A ;=20 > > devel@edk2.groups.io > > Cc: Chiu, Chasel ; Desimone, Nathaniel L=20 > > ; Gao, Zhichao > > > Subject: RE: [edk2-platforms][PATCH V1 1/2] KabylakeSiliconPkg:=20 > > Remove ResetSystemLib.h override > > > > This change is introducing SiliconPkg's dependency on MdeModulePkg. > > SiliconPkg dependencies should be limited to a selected few packages=20 > > and this seems to be an unnecessary addition to the dependency list. > > The reset interfaces are providing generic reset services and=20 > > 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 ; Chiu,=20 > > Chasel ; Desimone, Nathaniel L=20 > > ; Gao, Zhichao > > > Subject: [edk2-platforms][PATCH V1 1/2] KabylakeSiliconPkg: Remove=20 > > ResetSystemLib.h override > > > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D2375 > > > > Removes a stale ResetSystemLib.h override in KabylakeSiliconPkg that=20 > > does not contain the prototype for ResetSystem () and > ResetPlatformSpecific (). > > > > The ResetSystemLib.h file from MdeModulePkg will be used. Any INF=20 > > files that did not include the MdeModulePkg.dec under [Packages]=20 > > were updated to do so. > > > > Cc: Sai Chaganty > > Cc: Chasel Chiu > > Cc: Nate DeSimone > > Cc: Zhichao Gao > > Signed-off-by: Michael Kubacki > > --- > > > > > Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeResetSystemLib/DxeRese > tS > > ystemLib.inf | 3 +- > > > > Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeRuntimeResetSystemLi > > b/ > > Dx > > eRuntimeResetSystemLib.inf | 3 +- > > > > > Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiPchResetLib/PeiPchResetLi= b. > > inf | 3 +- > > > > > Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiResetSystemLib/PeiRese > tSys > > 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/Dxe > > Re > > se > > tSystemLib.inf > > b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeResetSystemLib/Dxe > > Re > > se > > tSystemLib.inf > > index aa8877140a..46313bf35f 100644 > > --- > > a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeResetSystemLib/Dxe > > Re > > se > > tSystemLib.inf > > +++ b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeResetSystemLib > > +++ /D > > +++ xe > > +++ ResetSystemLib.inf > > @@ -1,7 +1,7 @@ > > ## @file > > # Component description file for Intel Ich7 Reset System Library. > > # > > -# Copyright (c) 2017, Intel Corporation. All rights reserved.
> > +# Copyright (c) 2017 - 2019, Intel Corporation. All rights=20 > > +reserved.
> > # > > # SPDX-License-Identifier: BSD-2-Clause-Patent # @@ -35,6 +35,7 @@=20 > > PchCycleDecodingLib > > > > [Packages] > > MdePkg/MdePkg.dec > > +MdeModulePkg/MdeModulePkg.dec > > KabylakeSiliconPkg/SiPkg.dec > > > > > > diff --git > > a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeRuntimeResetSystem > > Li > > b/ > > DxeRuntimeResetSystemLib.inf > > b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeRuntimeResetSystem > > Li > > b/ > > DxeRuntimeResetSystemLib.inf > > index 6b27661603..c7fad31c71 100644 > > --- > > a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/DxeRuntimeResetSystem > > Li > > b/ > > 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.
> > +# Copyright (c) 2017 - 2019, Intel Corporation. All rights=20 > > +reserved.
> > # > > # SPDX-License-Identifier: BSD-2-Clause-Patent # @@ -36,6 +36,7 @@=20 > > PchCycleDecodingLib > > > > [Packages] > > MdePkg/MdePkg.dec > > +MdeModulePkg/MdeModulePkg.dec > > KabylakeSiliconPkg/SiPkg.dec > > > > > > diff --git > > a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiPchResetLib/PeiPch > > Re > > setL > > ib.inf > > b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiPchResetLib/PeiPch > > Re > > setL > > ib.inf > > index b04f4006ef..29f69078a4 100644 > > --- > > a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiPchResetLib/PeiPch > > Re > > setL > > ib.inf > > +++ b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiPchResetLib/Pe > > +++ iP > > +++ ch > > +++ ResetLib.inf > > @@ -1,7 +1,7 @@ > > ## @file > > # Component description file for PCH Reset Lib Pei Phase # -#=20 > > Copyright (c) 2017, Intel Corporation. All rights reserved.
> > +# Copyright (c) 2017 - 2019, Intel Corporation. All rights=20 > > +reserved.
> > # > > # SPDX-License-Identifier: BSD-2-Clause-Patent # @@ -28,6 +28,7 @@=20 > > ResetSystemLib > > > > [Packages] > > MdePkg/MdePkg.dec > > +MdeModulePkg/MdeModulePkg.dec > > KabylakeSiliconPkg/SiPkg.dec > > > > [Sources] > > diff --git > > a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiResetSystemLib/Pei > > Re > > setS > > ystemLib.inf > > b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiResetSystemLib/Pei > > Re > > set > > SystemLib.inf > > index 18a92a6f18..3c6ff78863 100644 > > --- > > a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiResetSystemLib/Pei > > Re > > setS > > ystemLib.inf > > +++ b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiResetSystemLib > > +++ /P > > +++ ei > > +++ ResetSystemLib.inf > > @@ -1,7 +1,7 @@ > > ## @file > > # Component description file for Intel Ich7 Reset System Library. > > # > > -# Copyright (c) 2017, Intel Corporation. All rights reserved.
> > +# Copyright (c) 2017 - 2019, Intel Corporation. All rights=20 > > +reserved.
> > # > > # SPDX-License-Identifier: BSD-2-Clause-Patent # @@ -32,6 +32,7 @@=20 > > 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=20 > > of > > - methods that reset the whole system. > > - > > -Copyright (c) 2005 - 2010, Intel Corporation. All rights=20 > > reserved.
> > -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.=20 > > This type of reset > > - is asynchronous to system operation and operates without regard=20 > > to > > - cycle boundaries. > > - > > - If this function returns, it means that the system does not=20 > > support cold reset. > > -**/ > > -VOID > > -EFIAPI > > -ResetCold ( > > - VOID > > - ); > > - > > -/** > > - This function causes a system-wide initialization (warm reset),=20 > > 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=20 > > 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=20 > > 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=20 > > support > > S3 feature. > > -**/ > > -VOID > > -EFIAPI > > -EnterS3WithImmediateWake ( > > - VOID > > - ); > > - > > -#endif > > -- > > 2.16.2.windows.1 > >