From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web09.922.1614822545280277880 for ; Wed, 03 Mar 2021 17:49:05 -0800 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@linux.microsoft.com header.s=default header.b=cxcc4VZx; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [10.124.238.202] (unknown [131.107.174.202]) by linux.microsoft.com (Postfix) with ESMTPSA id 5552920B83EA; Wed, 3 Mar 2021 17:49:04 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 5552920B83EA DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1614822544; bh=3TdjffeAQXHUoRtjDr9AK+ZxzuatMUpB11qOjMvIDq8=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=cxcc4VZx0eJwfHWM5qR2QNSN/6raNN8HnsGSPHH8SXFXIvcJLcemdNKFZFhst/y7g cMdMb205IOpBST1Bnd/sAYyG4XAn2qeb+z+kz+6YyXSdtWmDNBNx7mNW48sLWK3Jcp pPw4j4KCCw/j6BPnPZ9xu3mIrAW2SPdH3vaMyLgs= Subject: Re: [edk2-devel] [edk2-platforms][RFC] SpiFlashCommonLib Refactor To: devel@edk2.groups.io, ray.ni@intel.com, Bret Barkelew , "Dong, Guo" Cc: "Chaganty, Rangasai V" , "Chiu, Chasel" , "Desimone, Nathaniel L" , "Luo, Heng" , "Agyeman, Prince" , "gaoliming@byosoft.com.cn" , "Dong, Eric" References: <2a2a1cc0-3de9-fbd5-1bfb-eb5dbc9d1bc6@linux.microsoft.com> <9a764723-06b6-555a-dfb8-6dee940e7276@linux.microsoft.com> From: "Michael Kubacki" Message-ID: Date: Wed, 3 Mar 2021 17:49:05 -0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable Going back a bit earlier in the thread, I believe the interface is=20 already abstracted at the right level - the Firmware Volume Block=20 protocol defined in MdePkg. Here we have a driver that produces the protocol with a helper library.=20 I believe both of these are implementation details behind the protocol=20 and I view IntelSiliconPkg as a reasonable place to hold the current=20 implementation within the scope it currently serves. To be clear, I'm also just consolidating what already exists in the=20 instances I mentioned at the beginning of the thread so you can use that= =20 as a reference for what to expect. The main goal of this particular=20 activity is to simply eliminate redundant code that already exists in=20 edk2-platforms. Thanks, Michael On 3/3/2021 4:54 PM, Ni, Ray wrote: > Bret, >=20 > UefiPayloadPkg contains UEFI modules that=92re needed for booting UEFI O= S. >=20 > So, it=92s about that UefiPayloadPkg cannot contain code outside of edk2= repo. >=20 > Thanks, >=20 > Ray >=20 > *From:* Bret Barkelew > *Sent:* Thursday, March 4, 2021 7:59 AM > *To:* devel@edk2.groups.io; Ni, Ray ; Dong, Guo=20 > ; Michael Kubacki > *Cc:* Chaganty, Rangasai V ; Chiu, Chasel= = =20 > ; Desimone, Nathaniel L=20 > ; Luo, Heng ;=20 > Agyeman, Prince ; gaoliming@byosoft.com.cn;=20 > Dong, Eric > *Subject:* RE: [edk2-devel] [edk2-platforms][RFC] SpiFlashCommonLib Refa= ctor >=20 > Furthermore, is it the code that cannot cross the repo boundary, or the= =20 > interface? >=20 > Is there any use in defining the interface at a lower level such as=20 > MdeModulePkg? Then a platform can consume the implementation from=20 > IntelSiliconPkg if it so chooses? >=20 > - Bret >=20 > *From: *Ni, Ray via groups.io > *Sent: *Wednesday, March 3, 2021 3:38 PM > *To: *Dong, Guo ; Michael Kubacki=20 > ; devel@edk2.groups.io=20 > > *Cc: *Chaganty, Rangasai V ; Chiu,= = =20 > Chasel ; Desimone, Nathaniel L=20 > ; Luo, Heng=20 > ; Agyeman, Prince=20 > ; gaoliming@byosoft.com.cn=20 > ; Dong, Eric > *Subject: *[EXTERNAL] Re: [edk2-devel] [edk2-platforms][RFC]=20 > SpiFlashCommonLib Refactor >=20 > Guo, > I understand your concern that UefiPayloadPkg should NOT use any=20 > components outside of edk2 repo (edk2-platform repo in this case). > If the two SPI drivers you are talking about are the same one, we could= =20 > go back to put the driver to PcAtchipsetPkg. > Let's see what the driver contains and then decide where to put. >=20 > Thanks, > Ray >=20 > > -----Original Message----- > > From: Dong, Guo > > > Sent: Thursday, March 4, 2021 7:22 AM > > To: Michael Kubacki >; devel@edk2.groups.io=20 > ; Ni, Ray > > > Cc: Chaganty, Rangasai V >; Chiu, Chasel=20 > >; Desimone,=20 > Nathaniel L > > >; Luo, Heng >; Agyeman, Prince >; > > gaoliming@byosoft.com.cn ; Dong,=20 > Eric > > > Subject: RE: [edk2-devel] [edk2-platforms][RFC] SpiFlashCommonLib=20 > Refactor > > > > > > Hi Michael, > > > > I didn't initiate any discussion on this yet. And I am not sure if=20 > this idea could be accepted. > > From my view point, IntelSiliconPkg is a proper place for SPI flash= =20 > library. > > But UefiPayloadPkg could not use that package since it is in another= =20 > repo. > > Since these dependencies, you could go a head to put it into=20 > IntelSiliconPkg. > > > > Once I clean up my branch (expected complete next week), I could send= = =20 > my patch for your > > reference so that we could at least share code if possible to reduce= =20 > the code maintenance. > > > > Thanks, > > Guo > > > > > -----Original Message----- > > > From: Michael Kubacki > > > > Sent: Wednesday, March 3, 2021 3:54 PM > > > To: devel@edk2.groups.io ; Dong, Guo= =20 > >; Ni, Ray > > > > > > > Cc: Chaganty, Rangasai V >; Chiu, Chasel > > > >; Desimone,= =20 > Nathaniel L > > > >; Luo, Heng >; > > > Agyeman, Prince >; gaoliming@byosoft.com.cn=20 > ; > > > Dong, Eric > > > > Subject: Re: [edk2-devel] [edk2-platforms][RFC] SpiFlashCommonLib > > > Refactor > > > > > > Hi Guo, > > > > > > That's good to hear. > > > > > > Does this new "common SPI flash library" and "SMM FVB driver" have > > > equivalent functionality to the instances being discussed here? > > > > > > For the platforms I have in mind, IntelSiliconPkg is an allowed > > > dependency whereas UefiPayloadPkg is not. > > > > > > I have begun the work (at low priority) discussed earlier in the=20 > thread, > > > please let me know if I should continue. > > > > > > Thanks, > > > Michael > > > > > > On 3/3/2021 1:58 PM, Guo Dong wrote: > > > > > > > > Hi Michael, > > > > > > > > We had created a common SPI flash library and a common SMM FVB dr= iver > > > for all the platforms I have (including Apollo lake, Coffee lake,= =20 > Kaby Lake, > > > Comet Lake, Tiger Lake, Elkhart Lake, etc.).=A0 we plan to upstream= = =20 > this for UEFI > > > Payload. > > > > If this one could be upstream to UefiPayloadPkg, then each=20 > platform could > > > leverage it. > > > > > > > > BTW, together with this, we plan to upstream SMM support, secure = boot > > > and measured boot for UEFI Payload. > > > > So we could use a single UEFI Payload with these advanced=20 > features=A0 on > > > different platforms. > > > > > > > > Thanks, > > > > Guo > > > > > > > > > > > >> -----Original Message----- > > > >> From: Ni, Ray > > > > >> Sent: Monday, March 1, 2021 5:52 PM > > > >> To: Michael Kubacki >; > > > >> devel@edk2.groups.io > > > >> Cc: Chaganty, Rangasai V >; Chiu, Chasel > > > >> >;=20 > Desimone, Nathaniel L > > > >> >; Luo, Heng >; > > > >> Agyeman, Prince >; > > > gaoliming@byosoft.com.cn ; > > > >> Dong, Eric >;= =20 > Dong, Guo > > > > >> Subject: RE: [edk2-devel] [edk2-platforms][RFC] SpiFlashCommonLi= b > > > >> Refactor > > > >> > > > >> Michael, > > > >> I am good with that. I am also curious how you merge all the=20 > different SPI > > > >> flash implementation into one. > > > >> > > > >> Thanks, > > > >> Ray > > > >> > > > >>> -----Original Message----- > > > >>> From: Michael Kubacki > > > > >>> Sent: Tuesday, March 2, 2021 3:16 AM > > > >>> To: devel@edk2.groups.io ; Ni, Ray= = =20 > > > > > >>> Cc: Chaganty, Rangasai V >; Chiu, Chasel > > > >> >;=20 > Desimone, Nathaniel L > > > >>> >; Luo, Heng >; > > > >> Agyeman, Prince >; > > > >>> gaoliming@byosoft.com.cn ;=20 > Dong, Eric > > > > >>> Subject: Re: [edk2-devel] [edk2-platforms][RFC] SpiFlashCommonL= ib > > > >> Refactor > > > >>> > > > >>> Hi Ray, > > > >>> > > > >>> That sounds reasonable to me. > > > >>> > > > >>> I was attempting to preserve the design that isolates the > > > >>> silicon-specific logic to a library via an interface to a silic= on > > > >>> package. However, the real abstraction here is the firmware vol= ume > > > block > > > >>> protocol which could simply be produced by a silicon driver=20 > with the > > > >>> separation of such logic to a library being an implementation= =20 > detail of > > > >>> the driver. > > > >>> > > > >>> In summary, here is the updated proposal: > > > >>> > > > >>> 1. Consolidate the library interface into a single header in > > > >>> IntelSiliconPkg. > > > >>> > > > >>> 2. Consolidate the library implementation into a single instanc= e in > > > >>> IntelSiliconPkg. > > > >>> > > > >>> 3. Move SpiFvbServiceSmm out of MinPlatformPkg into=20 > IntelSiliconPkg. > > > >>> > > > >>> 4. Add SpiFvbServiceStandaloneMm to IntelSiliconPkg sharing > > > >>> implementation with SpiFvbServiceSmm where appropriate. > > > >>> > > > >>> Intel board packages would then use the SpiFlashCommonLib from > > > >>> IntelSiliconPkg (a generation specific instance could be create= d if > > > >>> needed) and use the SpiFvbServiceXyz driver from IntelSiliconPk= g. > > > >>> > > > >>> Please let me know if this is acceptable and I'd be happy to=20 > send the > > > >>> patches. > > > >>> > > > >>> Thanks, > > > >>> Michael > > > >>> > > > >>> On 3/1/2021 1:07 AM, Ni, Ray wrote: > > > >>>> Michael, > > > >>>> I agree with your thoughts to consolidate the lib header and > > > >> implementation to IntelSiliconPkg. > > > >>>> I didn't read the different implementations. But the=20 > implementation > > > >> consolidation means you see all the existing > > > >>> implementations are the same. Right? > > > >>>> > > > >>>> But why don't you put the driver in IntelSiliconPkg as well?= =20 > Creating an > > > >> advanced feature for this fundamental service seems > > > >>> over-kill. > > > >>>> > > > >>>> Thanks, > > > >>>> Ray > > > >>>> > > > >>>>> -----Original Message----- > > > >>>>> From: Chaganty, Rangasai V > > > > >>>>> Sent: Monday, March 1, 2021 4:46 PM > > > >>>>> To: Ni, Ray > > > > >>>>> Subject: RE: [edk2-platforms][RFC] SpiFlashCommonLib Refactor > > > >>>>> > > > >>>>> Did you get a chance to look into this ? > > > >>>>> > > > >>>>> -----Original Message----- > > > >>>>> From: Michael Kubacki > > > > >>>>> Sent: Tuesday, February 16, 2021 4:58 PM > > > >>>>> To: devel@edk2.groups.io ; Ni,= =20 > Ray >; Chaganty, > > > >> Rangasai V >; Chiu, > > > >>> Chasel > > > >>>>> >;=20 > Desimone, Nathaniel L > > > >> >; Luo, Heng >; > > > >>>>> Agyeman, Prince >; > > > >> gaoliming@byosoft.com.cn ;=20 > Dong, Eric > > > > >>>>> Subject: [edk2-platforms][RFC] SpiFlashCommonLib Refactor > > > >>>>> > > > >>>>> Hello, > > > >>>>> > > > >>>>> I'm planning to submit support for Standalone MM in > > > >> SpiFlashCommonLib soon. Currently, there's quite a bit of=20 > duplication > > > >>> with > > > >>>>> SpiFlashCommonLib. > > > >>>>> > > > >>>>> I would like to have this Standalone MM support be available= =20 > in as > > > >> consistent of a location as possible so I'd like to see if > > > >>> there is > > > >>>>> anything I can do to help clean this up in the early part of= =20 > the patch > > > >> series. > > > >>>>> > > > >>>>> > > > >>>>> The library interface is currently defined in the following= =20 > header files: > > > >>>>> > > > >>>>> 1. > > > >> Platform\Intel\MinPlatformPkg\Include\Library\SpiFlashCommonLib.= h > > > >>>>> > > > >>>>> 2.=20 > Silicon\Intel\SimicsIch10Pkg\Include\Library\SpiFlashCommonLib.h > > > >>>>> > > > >>>>> 3. > > > >> > > >=20 > Silicon\Intel\KabylakeSiliconPkg\Pch\Include\Library\SpiFlashCommonLib.h > > > >>>>> > > > >>>>> 4. > > > >>>>> > > > >> > > >=20 > Silicon\Intel\CoffeelakeSiliconPkg\Pch\Include\Library\SpiFlashCommonLib= . > > > >> h > > > >>>>> > > > >>>>> > > > >>>>> Instances of SmmSpiFlashCommonLib implementation exist in a m= ix > > > of > > > >> platform and silicon packages: > > > >>>>> > > > >>>>> 1. > > > >>>>> > > > >> > > > Silicon\Intel\SimicsIch10Pkg\Library\SmmSpiFlashCommonLib\SmmSpiFla= shC > > > >> ommonLib.inf > > > >>>>> > > > >>>>> 2. > > > >>>>> > > > >> > > > Platform\Intel\TigerlakeOpenBoardPkg\Library\SmmSpiFlashCommonLib\S > > > >> mmSpiFlashCommonLib.inf > > > >>>>> > > > >>>>> 3. > > > >>>>> > > > >> > > > Silicon\Intel\KabylakeSiliconPkg\Pch\Library\SmmSpiFlashCommonLib\S= mm > > > >> SpiFlashCommonLib.inf > > > >>>>> > > > >>>>> 4. > > > >>>>> > > > >> > > > Silicon\Intel\CoffeelakeSiliconPkg\Pch\Library\SmmSpiFlashCommonLib= \Sm > > > >> mSpiFlashCommonLib.inf > > > >>>>> > > > >>>>> 5. > > > >>>>> > > > >> > > >=20 > Platform\Intel\MinPlatformPkg\Flash\Library\SpiFlashCommonLibNull\SpiFla > > > >> shCommonLibNull.inf > > > >>>>> > > > >>>>> > > > >>>>> The library class is currently consumed in the following INFs= : > > > >>>>> > > > >>>>> 1. > > > >> > > > Platform\Intel\MinPlatformPkg\Flash\SpiFvbService\SpiFvbServiceSmm.= inf > > > >>>>> > > > >>>>> 2. > > > >>>>> > > > >> > > >=20 > Platform\Intel\MinPlatformPkg\Flash\SpiFvbService\SpiFvbServiceStandalon > > > >> eMm.inf > > > >>>>> > > > >>>>> > > > >>>>> My understanding is: > > > >>>>> > > > >>>>> 1. The header file is defined in each silicon package because= = =20 > silicon > > > >> cannot depend upon platform (i.e. the MinPlatformPkg > > > >>>>> header). > > > >>>>> > > > >>>>> 2. The header is present in each silicon package because the > > > >> implementation is silicon-specific and these packages cannot > > > >>>>> depend on one another. > > > >>>>> > > > >>>>> 3. The header is defined in MinPlatformPkg because MinPlatfor= mPkg > > > >> should be silicon vendor agnostic (cannot depend on the > > > >>>>> silicon packages). > > > >>>>> > > > >>>>> 4. The header is needed in MinPlatformPkg because the > > > SpiFvbService > > > >> there depends on SPI flash operations implemented in > > > >>>>> SpiFlashCommonLib. > > > >>>>> > > > >>>>> > > > >>>>> Here's an initial proposal: > > > >>>>> > > > >>>>> 1. Consolidate the library interface into a single header. In > > > >>>>> IntelSiliconPkg? > > > >>>>> > > > >>>>> 2. Consolidate library implementation into a single instance.= In > > > >>>>> IntelSiliconPkg? > > > >>>>> > > > >>>>> 3. Move SpiFvbServiceXyz out of MinPlatformPkg. > > > >>>>>=A0=A0=A0=A0=A0=A0 3.a. Make a "SPI flash" feature? > > > >>>>>=A0=A0=A0=A0=A0=A0 3.b. Allow the Intel implementation of this= feature to=20 > depend on > > > >>>>> SpiFlashCommonLib defined in IntelSiliconPkg. > > > >>>>> > > > >>>>> Intel board packages could then use the SpiFlashCommonLib fro= m > > > >>>>> IntelSiliconPkg (a generation specific instance could be=20 > created if > > > >>>>> needed) and use the SpiFvbServiceXyz driver from the "SpiFlas= h" > > > >> feature. > > > >>>>> > > > >>>>> Look forward to your thoughts and feedback. > > > >>>>> > > > >>>>> Thanks, > > > >>>>> Michael > > > >>>> > > > >>>> > > > >>>> > > > >>>> > > > >>>> > > > > > > > > > > > > > > > > > > > > >=20 >=20 >=20