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

["Ni, Ray" <ray.ni@intel.com>]

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