Re: 回复: [edk2-devel] [edk2-platforms] [patch 00/35] Consume RegisterFilterLibNull instance
Laszlo Ersek
lersek at redhat.com
Thu Mar 18 13:25:18 UTC 2021
On 03/18/21 05:43, gaoliming wrote:
> Laszlo and Dandan:
>
>> -----邮件原件-----
>> 发件人: Laszlo Ersek <lersek at redhat.com>
>> 发送时间: 2021年3月18日 1:08
>> 收件人: Bi, Dandan <dandan.bi at intel.com>; devel at edk2.groups.io;
>> gaoliming at byosoft.com.cn; ardb at kernel.org; 'Andrew Fish'
>> <afish at apple.com>
>> 抄送: 'Leif Lindholm' <leif at nuviainc.com>; Kinney, Michael D
>> <michael.d.kinney at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of
>>>> gaoliming
>>>> Sent: Wednesday, March 17, 2021 11:05 AM
>>>> To: devel at edk2.groups.io; ardb at kernel.org; Bi, Dandan
>>>> <dandan.bi at intel.com>; 'Laszlo Ersek' <lersek at redhat.com>; 'Andrew
>> Fish'
>>>> <afish at apple.com>
>>>> Cc: 'Leif Lindholm' <leif at nuviainc.com>; Kinney, Michael D
>>>> <michael.d.kinney at intel.com>
>>>> Subject: 回复: [edk2-devel] [edk2-platforms] [patch 00/35] Consume
>>>> RegisterFilterLibNull instance
>>>>
>>>> Ard and Dandan:
>>>>
>>>>> -----邮件原件-----
>>>>> 发件人: devel at edk2.groups.io <devel at edk2.groups.io> 代表 Ard
>>>> Biesheuvel
>>>>> 发送时间: 2021年3月16日 23:01
>>>>> 收件人: devel at edk2.groups.io; dandan.bi at intel.com; Laszlo Ersek
>>>>> <lersek at redhat.com>; Andrew Fish <afish at apple.com>
>>>>> 抄送: Leif Lindholm <leif at nuviainc.com>; Michael D Kinney
>>>>> <michael.d.kinney at intel.com>; Liming Gao <gaoliming at 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 at 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 at nuviainc.com>
>>>>>> Cc: Michael D Kinney <michael.d.kinney at intel.com>
>>>>>> Cc: Liming Gao <gaoliming at 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
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73008): https://edk2.groups.io/g/devel/message/73008
Mute This Topic: https://groups.io/mt/81422760/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list