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

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

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.
>>>>
>>>>
>>>
>>>
>>> 
>>>
>>