Re: 回复: [edk2-devel] [edk2-platforms] [patch 00/35] Consume RegisterFilterLibNull instance

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

I know that NetworkPkg already provided a case of DSC header file.
But NetworkPkg provides drivers and the dependent libs. Multiple instances of a single lib class are not common in NetworkPkg. So, for NetworkPkg, the consumer can simplify include the DSC header files.


But for MdePkg, there are so many lib instances for a single lib class. I am not sure how we create the DSC header file (which lib instance should be chosen).

Maybe having a DSC header file can avoid changing lots of code in each platform DSC *today*. But imagine that we look at the platform DSC file after couple years/months, we might be more frustrated about which lib instances are used in the platform.

Still, I am open to see what the final MdePkg DSC header will be like.

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Thursday, March 18, 2021 9:25 PM
> To: gaoliming <gaoliming@byosoft.com.cn>; Bi, Dandan <dandan.bi@intel.com>; devel@edk2.groups.io; ardb@kernel.org;
> 'Andrew Fish' <afish@apple.com>
> Cc: 'Leif Lindholm' <leif@nuviainc.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: 回复: [edk2-devel] [edk2-platforms] [patch 00/35] Consume RegisterFilterLibNull instance
> 
> On 03/18/21 05:43, gaoliming wrote:
> > Laszlo and Dandan:
> >
> >> -----邮件原件-----
> >> 发件人: Laszlo Ersek <lersek@redhat.com>
> >> 发送时间: 2021年3月18日 1:08
> >> 收件人: Bi, Dandan <dandan.bi@intel.com>; devel@edk2.groups.io;
> >> gaoliming@byosoft.com.cn; ardb@kernel.org; 'Andrew Fish'
> >> <afish@apple.com>
> >> 抄送: 'Leif Lindholm' <leif@nuviainc.com>; Kinney, Michael D
> >> <michael.d.kinney@intel.com>
> >> 主题: Re: [edk2-devel] [edk2-platforms] [patch 00/35] Consume
> >> RegisterFilterLibNull instance
> >>
> >> On 03/17/21 16:05, Bi, Dandan wrote:
> >>>> -----Original Message-----
> >>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> >>>> gaoliming
> >>>> Sent: Wednesday, March 17, 2021 11:05 AM
> >>>> To: devel@edk2.groups.io; ardb@kernel.org; Bi, Dandan
> >>>> <dandan.bi@intel.com>; 'Laszlo Ersek' <lersek@redhat.com>; 'Andrew
> >> Fish'
> >>>> <afish@apple.com>
> >>>> Cc: 'Leif Lindholm' <leif@nuviainc.com>; Kinney, Michael D
> >>>> <michael.d.kinney@intel.com>
> >>>> Subject: 回复: [edk2-devel] [edk2-platforms] [patch 00/35] Consume
> >>>> RegisterFilterLibNull instance
> >>>>
> >>>> Ard and Dandan:
> >>>>
> >>>>> -----邮件原件-----
> >>>>> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Ard
> >>>> Biesheuvel
> >>>>> 发送时间: 2021年3月16日 23:01
> >>>>> 收件人: devel@edk2.groups.io; dandan.bi@intel.com; Laszlo Ersek
> >>>>> <lersek@redhat.com>; Andrew Fish <afish@apple.com>
> >>>>> 抄送: Leif Lindholm <leif@nuviainc.com>; Michael D Kinney
> >>>>> <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>
> >>>>> 主题: Re: [edk2-devel] [edk2-platforms] [patch 00/35] Consume
> >>>>> RegisterFilterLibNull instance
> >>>>>
> >>>>> On Tue, 16 Mar 2021 at 15:56, Dandan Bi <dandan.bi@intel.com> wrote:
> >>>>>>
> >>>>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3246
> >>>>>> RFC: https://edk2.groups.io/g/devel/message/72530
> >>>>>>
> >>>>>>
> >>>>>> Add RegisterFilterLibNull in dsc files in edk2-platforms repo, which
> >>>>>> will be consumed by IoLib and BaseLib.
> >>>>>>
> >>>>>> This is the following update in edk2-platforms repo for the change
> >>>>>> in edk2, which will add RegisterFilterLib dependency for IoLib and
> >>>>>> BaseLib to
> >>>>> filter/trace
> >>>>>> port IO/MMIO/MSR access.
> >>>>>> https://edk2.groups.io/g/devel/message/72754
> >>>>>>
> >>>>>> Cc: Leif Lindholm <leif@nuviainc.com>
> >>>>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> >>>>>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> >>>>>>
> >>>>>
> >>>>> It is a bit disappointing that we have to update every platform in
> >>>>> existence again to apply a change to a core module.
> >>>>>
> >>>>
> >>>> I suggest to add MdePkg.dsc.inc file to include the default library instance,
> >>>> and update all Platform DSC to include it. Then, for the future change, no
> >>>> change is required for platform DSC.
> >>>>
> >>>> Because this patch set updates every platform DSC, I suggest to introduce
> >>>> MdePkg.dsc.inc file in this patch set.
> >>>
> >>> Hi Liming,
> >>>
> >>> I agree that add MdePkg.dsc.inc file to include the default library instance
> >> and make it consumed by platform dsc will benefit future similar incompatible
> >> changes.
> >>> But I wonder to know whether we could do it in a separated task/topic,  as
> >>> 1.  It should be a code infrastructure design change/improvement in edk2.
> >>> 2.  Personally I don't hope the new solution will have much impact on my
> >> current schedule, but it seems have.
> >>> And we may need to:
> >>> 1). Clarify the default library instances which should be added in
> >> MdePkg.dsc.inc
> >>>     The library instances in MdePkg.dsc.inc should be generic enough to
> >> be widely included in platform dsc files.
> >>> 2). Update dsc files in edk2 and edk2-platforms repo to include
> >> MdePkg.dsc.inc and cleanup the default Lib instance in dsc files.
> >>
> >> (1) The file name should be "MdeLibs.dsc.inc", and it should be
> >> structured similarly "NetworkLibs.dsc.inc" -- no [LibraryClasses] header
> >> should be part of the file.
> >>
> > I agree.
> >
> >> (2) The introduction of "MdeLibs.dsc.inc" is a big task, in my opinion.
> >>
> > The full MdeLibs.dsc.inc is a big task. But, it should be a separate task.
> >
> > For this patch set, the first version MdeLibs.dsc.inc can be added. It only includes RegisterFilterLibNull library instance.
> > And, this patch set will update every platform DSC to include MdeLibs.dsc.inc.
> > The future incompatible change can update MdeLibs.dsc.inc only, and avoid the change in each platform DSC.
> > I would like to resolve the potential incompatible change by MdeLibs.dsc.inc on the first step.
> 
> That's a good idea!
> 
> Dandan is touching up a bunch of DSC files already, so we might even use
> this opportunity to introduce "MdeLibs.dsc.inc". And, at this time,
> "MdeLibs.dsc.inc" would be really small, and not hard to review for
> individual platforms -- in particular it wouldn't attempt to *replace*
> existent lib class resolutions. So indeed this looks like a nice
> approach to me.
> 
> Thanks!
> Laszlo
> 
> >
> > Thanks
> > Liming
> >> As I stated earlier, I wouldn't like to review a patch for OvmfPkg that
> >> replaces (say) 50-100 lines of library class resolutions with a simple
> >> !include directive. Such a patch is unreviewable, as I'd have no way of
> >> carefully comparing the before-after state, let alone a way of *pointing
> >> out* (in comments) where exactly a problem was.
> >>
> >> So I think such an include file would require a patch set, and the lib
> >> class resolutions should be migrated in small *topical* steps. Such as:
> >>
> >> - introduce the DSC include file as empty
> >> - add the !include directive to platform DSCs
> >> - add a small set of libraries (with some topical coherence) to the
> >> include file,
> >> - remove the same set of resolutions from platform DSCs,
> >> - repeat the last two steps until all "topics" have been covered.
> >>
> >> Thanks
> >> Laszlo
> >>
> >>
> >>>
> >>>
> >>> Thanks,
> >>> Dandan
> >>>>
> >>>> Thanks
> >>>> Liming
> >>>>> Is there really not a better way to provide a 'default' resolution for
> >>>>> a library class? Maybe a change to the .DEC format, so that the file
> >>>>> which defines the library class can provide a resolution that is used
> >>>>> if none is provided by the .DSC file?
> >>>>>
> >>>>>
> >>>>>
> >>>>>> Dandan Bi (35):
> >>>>>>   Drivers/ASIX: Consume RegisterFilterLibNull instance
> >>>>>>   Drivers/DisplayLink: Consume RegisterFilterLibNull instance
> >>>>>>   Drivers/OptionRomPkg: Consume RegisterFilterLibNull instance
> >>>>>>   Features/Debugging: Consume RegisterFilterLibNull instance
> >>>>>>   Features/Network: Consume RegisterFilterLibNull instance
> >>>>>>   Features/OutOfBandManagement: Consume RegisterFilterLibNull
> >>>>> instance
> >>>>>>   Features/PowerManagement: Consume RegisterFilterLibNull
> >> instance
> >>>>>>   Features/SystemInformation: Consume RegisterFilterLibNull instance
> >>>>>>   Features/UserInterface: Consume RegisterFilterLibNull instance
> >>>>>>   Platform/AMD: Consume RegisterFilterLibNull instance
> >>>>>>   Platform/ARM: Consume RegisterFilterLibNull instance
> >>>>>>   Platform/BeagleBoard: Consume RegisterFilterLibNull instance
> >>>>>>   Platform/BoardModulePkg: Consume RegisterFilterLibNull instance
> >>>>>>   Platform/MinPlatformPkg: Consume RegisterFilterLibNull instance
> >>>>>>   Platform/QuarkPlatformPkg: Consume RegisterFilterLibNull instance
> >>>>>>   Platform/Vlv2TbltDevicePkg: Consume RegisterFilterLibNull instance
> >>>>>>   Platform/LeMaker: Consume RegisterFilterLibNull instance
> >>>>>>   Platform/Qemu: Consume RegisterFilterLibNull instance
> >>>>>>   Platform/RaspberryPi: Consume RegisterFilterLibNull instance
> >>>>>>   Platform/RISC-V: Consume RegisterFilterLibNull instance
> >>>>>>   Platform/SiFive: Consume RegisterFilterLibNull instance
> >>>>>>   Platform/Socionext: Consume RegisterFilterLibNull instance
> >>>>>>   Platform/SoftIron: Consume RegisterFilterLibNull instance
> >>>>>>   Silicon/Hisilicon: Consume RegisterFilterLibNull instance
> >>>>>>   Silicon/CoffeelakeSiliconPkg: Consume RegisterFilterLibNull instance
> >>>>>>   Silicon/IntelSiliconPkg: Consume RegisterFilterLibNull instance
> >>>>>>   Silicon/KabylakeSiliconPkg: Consume RegisterFilterLibNull instance
> >>>>>>   Silicon/QuarkSocPkg: Consume RegisterFilterLibNull instance
> >>>>>>   Silicon/TigerlakeSiliconPkg: Consume RegisterFilterLibNull instance
> >>>>>>   Silicon/Marvell: Consume RegisterFilterLibNull instance
> >>>>>>   Silicon/NXP: Consume RegisterFilterLibNull instance
> >>>>>>   Silicon/Openmoko: Consume RegisterFilterLibNull instance
> >>>>>>   Silicon/RISC_V: Consume RegisterFilterLibNull instance
> >>>>>>   Silicon/Synopsys/DesignWare: Consume RegisterFilterLibNull
> >> instance
> >>>>>>   Silicon/TexasInstruments: Consume RegisterFilterLibNull instance
> >>>>>>
> >>>>>>  Drivers/ASIX/Asix.dsc
> >>>>> | 1 +
> >>>>>>  Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkPkg.dsc          | 1
> >> +
> >>>>>>  Drivers/OptionRomPkg/OptionRomPkg.dsc
> >>>>> | 3 ++-
> >>>>>>  .../Debugging/AcpiDebugFeaturePkg/Include/AcpiDebugFeature.dsc |
> >> 3
> >>>>> ++-
> >>>>>>  .../Debugging/BeepDebugFeaturePkg/Include/BeepDebugFeature.dsc
> >> |
> >>>> 3
> >>>>> ++-
> >>>>>>  .../PostCodeDebugFeaturePkg/Include/PostCodeDebugFeature.dsc
> >> | 3
> >>>>> ++-
> >>>>>>  .../Debugging/Usb3DebugFeaturePkg/Include/Usb3DebugFeature.dsc
> >> |
> >>>> 3
> >>>>> ++-
> >>>>>>  .../Intel/Network/NetworkFeaturePkg/Include/NetworkFeature.dsc |
> >> 3
> >>>>> ++-
> >>>>>>  .../OutOfBandManagement/IpmiFeaturePkg/Include/IpmiFeature.dsc
> >> |
> >>>> 3
> >>>>> ++-
> >>>>>>  .../OutOfBandManagement/SpcrFeaturePkg/Include/SpcrFeature.dsc
> >> |
> >>>> 3
> >>>>> ++-
> >>>>>>  .../Intel/PowerManagement/S3FeaturePkg/Include/S3Feature.dsc
> >> | 3
> >>>>> ++-
> >>>>>>  .../SmbiosFeaturePkg/Include/SmbiosFeature.dsc
> >> | 3
> >>>>> ++-
> >>>>>>  .../Intel/UserInterface/LogoFeaturePkg/Include/LogoFeature.dsc | 3
> >> ++-
> >>>>>>  .../UserAuthFeaturePkg/Include/UserAuthFeature.dsc
> >> | 3
> >>>>> ++-
> >>>>>>  .../Include/VirtualKeyboardFeature.dsc
> >> | 3
> >>>>> ++-
> >>>>>>  Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
> >>>>> | 1 +
> >>>>>>  Platform/ARM/SgiPkg/PlatformStandaloneMm.dsc
> >>>>> | 1 +
> >>>>>>  Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc
> >>>>> | 1 +
> >>>>>>  Platform/BeagleBoard/BeagleBoardPkg/BeagleBoardPkg.dsc
> >> |
> >>>>> 3 ++-
> >>>>>>  Platform/Intel/BoardModulePkg/BoardModulePkg.dsc
> >> |
> >>>>> 3 ++-
> >>>>>>  Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc
> >> | 3
> >>>>> ++-
> >>>>>>  Platform/Intel/QuarkPlatformPkg/Quark.dsc
> >> |
> >>>>> 1 +
> >>>>>>  Platform/Intel/QuarkPlatformPkg/QuarkMin.dsc
> >> |
> >>>>> 1 +
> >>>>>>  Platform/Intel/Vlv2TbltDevicePkg/PlatformPkgIA32.dsc           |
> >> 3
> >>>>> ++-
> >>>>>>  Platform/Intel/Vlv2TbltDevicePkg/PlatformPkgX64.dsc            |
> >> 3
> >>>>> ++-
> >>>>>>  Platform/LeMaker/CelloBoard/CelloBoard.dsc
> >> |
> >>>>> 1 +
> >>>>>>  Platform/Qemu/SbsaQemu/SbsaQemu.dsc
> >>>>> | 1 +
> >>>>>>  Platform/RISC-V/PlatformPkg/RiscVPlatformPkg.dsc
> >> | 1
> >>>>> +
> >>>>>>  Platform/RaspberryPi/RPi3/RPi3.dsc
> >> |
> >>>>> 3 ++-
> >>>>>>  Platform/RaspberryPi/RPi4/RPi4.dsc
> >> |
> >>>>> 3 ++-
> >>>>>>  Platform/SiFive/U5SeriesPkg/FreedomU500VC707Board/U500.dsc
> >> |
> >>>>> 1 +
> >>>>>>  .../U5SeriesPkg/FreedomU540HiFiveUnleashedBoard/U540.dsc
> >> |
> >>>>> 1 +
> >>>>>>  Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
> >> | 1
> >>>>> +
> >>>>>>  Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
> >> | 1
> >>>>> +
> >>>>>>  Platform/SoftIron/Overdrive1000Board/Overdrive1000Board.dsc
> >> | 1
> >>>>> +
> >>>>>>  Silicon/Hisilicon/Hisilicon.dsc.inc                            | 1
> >> +
> >>>>>>  Silicon/Intel/CoffeelakeSiliconPkg/CoffeelakeSiliconPkg.dsc    | 1 +
> >>>>>>  Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc              | 3
> >> ++-
> >>>>>>  Silicon/Intel/KabylakeSiliconPkg/KabylakeSiliconPkg.dsc        | 3
> >> ++-
> >>>>>>  Silicon/Intel/QuarkSocPkg/QuarkSocPkg.dsc
> >> |
> >>>>> 3 ++-
> >>>>>>  Silicon/Intel/TigerlakeSiliconPkg/TigerlakeSiliconPkg.dsc      | 1 +
> >>>>>>  Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> >> |
> >>>>> 1 +
> >>>>>>  Silicon/NXP/NxpQoriqLs.dsc.inc
> >> |
> >>>>> 1 +
> >>>>>>  Silicon/Openmoko/Openmoko.dsc
> >>>>> | 1 +
> >>>>>>  Silicon/RISC-V/ProcessorPkg/RiscVProcessorPkg.dsc              |
> >> 1 +
> >>>>>>  Silicon/Synopsys/DesignWare/DesignWare.dsc
> >> |
> >>>>> 1 +
> >>>>>>  Silicon/TexasInstruments/Omap35xxPkg/Omap35xxPkg.dsc
> >>>>> | 1 +
> >>>>>>  47 files changed, 70 insertions(+), 23 deletions(-)
> >>>>>>
> >>>>>> --
> >>>>>> 2.18.0.windows.1
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>
> >
> >
> >
> 
> 
> 
> 
>