Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull

["Michael Kubacki" <mikuback@linux.microsoft.com>]

I think it would be nice to have both. That was just an example of where 
SmmAccessLib could be linked that would cause two instances of the 
library API to be invoked by common code that is linked to two different 
modules.

Some platforms might not use BoardInitLib that could benefit from the 
PEIM in IntelSiliconPkg.

On 9/9/2021 10:58 AM, Ni, Ray wrote:
> I think refactoring it to a PEIM is better.
> But I am not sure if having below Bugzilla completed can meet your needs without refactoring the lib to PEIM.
> 
>> I don't want to get too distracted with the example given, but I
>> completely agree that a different library instance should be used for
>> pre-memory and post-memory. I think the library interface is too broad
>> in scope and that contributes to causing this issue so I filed this BZ
>> to request the BoardInitLib API be refactored:
>> https://bugzilla.tianocore.org/show_bug.cgi?id=3578
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
>> Sent: Thursday, September 9, 2021 10:54 PM
>> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
>> Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull
>>
>> So you would rather leave it as a library class instead of refactoring
>> it to a PEIM?
>>
>> Again, the problem is it is a library class. So I am asking whether you
>> want to treat it as a library class or you are going to refactor it to a
>> PEIM.
>>
>> On 9/9/2021 10:49 AM, Ni, Ray wrote:
>>> No, I don't.
>>> I still don't think having a NULL SmmAccessLib is a good idea.
>>>
>>>> -----Original Message-----
>>>> From: Michael Kubacki <mikuback@linux.microsoft.com>
>>>> Sent: Thursday, September 9, 2021 10:12 PM
>>>> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
>>>> Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg: Add BaseSmmAccessLibNull
>>>>
>>>> Ray,
>>>>
>>>> Do you have plans to do something here? Whether take this patch or
>>>> refactor SmmAccessLib to a PEIM?
>>>>
>>>> Thanks,
>>>> Michael
>>>>
>>>> On 8/20/2021 3:34 PM, Michael Kubacki wrote:
>>>>> Since you asked for an example that was just one that I provided. I
>>>>> don't think it detracts from the fact that a NULL instance makes sense
>>>>> if the SmmAccessLib library class exists. The fact that a NULL instance
>>>>> could not be allowed to exist is also confusing.
>>>>>
>>>>> I don't want to get too distracted with the example given, but I
>>>>> completely agree that a different library instance should be used for
>>>>> pre-memory and post-memory. I think the library interface is too broad
>>>>> in scope and that contributes to causing this issue so I filed this BZ
>>>>> to request the BoardInitLib API be refactored:
>>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=3578
>>>>>
>>>>>    From the other email thread about SmmAccessLib, I think we are on the
>>>>> same page that the library would be better as a PEIM. Is that something
>>>>> that could be done soon? Or could we have this until that is done?
>>>>>
>>>>> Thanks,
>>>>> Michael
>>>>>
>>>>> On 8/20/2021 1:33 AM, Ni, Ray wrote:
>>>>>> Null SmmAccessLib is confusing to me. Have you evaluated the option:
>>>>>> Create two instances of BoardInitLib for pre-mem and post-mem. Pre-mem
>>>>>> one doesn’t link to SmmAccessLib
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
>>>>>>> Michael Kubacki
>>>>>>> Sent: Friday, August 20, 2021 12:53 AM
>>>>>>> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
>>>>>>> Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1]
>>>>>>> IntelSiliconPkg: Add BaseSmmAccessLibNull
>>>>>>>
>>>>>>> I don't understand your argument.
>>>>>>>
>>>>>>> The library class (SmmAccessLib) that already exists is the abstraction
>>>>>>> layer. This is not introducing a new layer of abstraction. It is using
>>>>>>> the current layer of abstraction.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Michael
>>>>>>>
>>>>>>> On 8/19/2021 5:49 AM, Ni, Ray wrote:
>>>>>>>> Michael,
>>>>>>>>
>>>>>>>> I don’t think scenario #1 is a good reason for NULL instance of
>>>>>>>> SmmAccessLib. The root cause is BoardInitLib lib class supports pre-mem
>>>>>>>> and post-mem board init.
>>>>>>>>
>>>>>>>> Below solution can avoid NULL SmmAccessLib:
>>>>>>>>
>>>>>>>> Create two instances of BoardInitLib for pre-mem and post-mem. Pre-mem
>>>>>>>> one doesn’t link to SmmAccessLib.
>>>>>>>>
>>>>>>>> For scenario #2, if a particular platform doesn’t support S3, why does
>>>>>>>> this platform include the PEIM?
>>>>>>>>
>>>>>>>> Please understand that I want to avoid introducing more abstraction
>>>>>>>> layers.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Ray
>>>>>>>>
>>>>>>>> *From:* Michael Kubacki <mikuback@linux.microsoft.com>
>>>>>>>> *Sent:* Friday, August 13, 2021 10:16 AM
>>>>>>>> *To:* Ni; Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
>>>>>>>> *Subject:* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1]
>>>>>>>> IntelSiliconPkg: Add BaseSmmAccessLibNull
>>>>>>>>
>>>>>>>> Sure.
>>>>>>>>
>>>>>>>> Scenario #1:
>>>>>>>>
>>>>>>>> MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.inf and
>>>>>>>> MinPlatormPkg/PlatformInit/PlatformInitPei/PlatformInitPostMem.inf both
>>>>>>>> link against an instance of BoardInitLib.
>>>>>>>>
>>>>>>>> Many boards link against a single BoardInitLib instance. See example -
>>>>>>>> https://github.com/tianocore/edk2-
>>>>>>>
>>>>
>> platforms/blob/cd4e6b716c7d1bcde94035e7dce14b53a553e103/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/OpenB
>>>>>>>
>>>>>>> oardPkg.dsc#L203
>>>>>>>> <https://github.com/tianocore/edk2-
>>>>>>>
>>>>
>> platforms/blob/cd4e6b716c7d1bcde94035e7dce14b53a553e103/Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/OpenB
>>>>>>>
>>>>>>> oardPkg.dsc#L203>
>>>>>>>>
>>>>>>>> That BoardInitLib instance may link against SmmAccessLib.
>>>>>>>> PlatformInitPreMem may wish to library class override the SmmAccessLib
>>>>>>>> to the NULL instance while keeping it to non-NULL instance in
>>>>>>>> PlatformInitPostMem.
>>>>>>>>
>>>>>>>> Scenario #2:
>>>>>>>>
>>>>>>>> A PEIM is built that checks whether the boot mode is S3. If so, it
>>>>>>>> calls
>>>>>>>> PeiInstallSmmAccessPpi(). A particular platform does not support S3,
>>>>>>>> therefore, it links BaseSmmAccessLibNull as its library instance for
>>>>>>>> SmmAccessLib.
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>
>>>
>>>
>>>
>>>
>>
>>
>> 
>>
>