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

["Laszlo Ersek" <lersek@redhat.com>]

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.

(2) The introduction of "MdeLibs.dsc.inc" is a big task, in my opinion.

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