On 03/08/21 22:55, Kinney, Michael D wrote: > > >> -----Original Message----- >> From: Laszlo Ersek <ler...@redhat.com> >> Sent: Monday, March 8, 2021 7:38 AM >> To: devel@edk2.groups.io; Bi, Dandan <dandan...@intel.com> >> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Liming Gao >> <gaolim...@byosoft.com.cn>; Liu, Zhiguang >> <zhiguang....@intel.com> >> Subject: Re: [edk2-devel] [RFC][patch] Add a new library class >> RegisterFilterLib in edk2 to filter/trace port IO/MMIO/MSR >> access >> >> On 03/08/21 06:15, Dandan Bi wrote: >>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3246 >>> >>> 1.Purpose: >>> Skip port IO/MMIO/MSR access in some emulatoion env. >>> Trace port IO/MMIO/MSR access. >>> >>> 2.Plan to do in Edk2: >>> Filter and trace in low level APIs in BaseIoLibIntrinsic and BaseLib. >>> Add a new library class (RegisterFilterLib) for the filter and trace >>> functionality. >>> >>> 3.Plan to filter and trace scope in Edk2 : >>> a. Port IO R/W: IA32 X64 (Only filter/trace for IA32 X64) >>> b. MMIO R/W: IA32 X64 EBC ARM AARCH64 RISCV64 (Filter/trace for the >>> Arches supported in BaseIoLibIntrinsic.inf) >>> c. MSR R/W: IA32 X64 (Only filter/trace for IA32 X64, if other ARCH has >>> similar use case can add new APIs per needs) >>> >>> 4.RegisterFilterLib Library Class: >>> a. Add RegisterFilterLib library class for the filter and trace operation. >>> b. Add RegisterFilterLib.h in MdePkg/Include/Library. >>> c. 12 APIs will be added to filter and trace port IO, MMIO and MSR access. >>> d. Add a NULL instance RegisterFilterLibNull in MdePkg/Library.(Verified >>> that null instance will not impact binary >> size.) >>> e. Platform can implement its own RegisterFilterLib instance. >>> >>> 12 APIs can be divided into 2 categories: >>> 6 [Before] APIs use to check whether need to execute port IO/MMIO/MSR >>> access or do some tracing before access. >>> 6 [After] APIs use to trace after port IO/MMIO/MSR access. >>> The detailed API definitions are included in this patch. >>> >>> For port IO access: >>> FilterBeforeIoRead >>> FilterAfterIoRead >>> FilterBeforeIoWrite >>> FilterAfterIoWrite >>> >>> For MMIO access: >>> FilterBeforeMmIoRead >>> FilterAfterMmIoRead >>> FilterBeforeMmIoWrite >>> FilterAfterMmIoWrite >>> >>> For MSR access: >>> FilterBeforeMsrRead >>> FilterAfterMsrRead >>> FilterBeforeMsrWrite >>> FilterAfterMsrWrite >>> >>> 5.Change and Impact >>> a. Add the RegisterFilterLib libary class and RegisterFilterLibNull >>> instance firstly. >>> b. Update the dsc in edk2 and edk2-platform repo to consume the >>> RegisterFilterLibNull instance. >>> c. Update the BaseLib and IoLib to consume RegisterFilterLib. >>> >>> This is an incompatible change. >>> No code change in BaseLib and IoLib consumers, only need to change dsc to >>> consume new FilterLib instance. >>> Update BaseIoLibIntrinsic.inf and BaseIoLibIntrinsicSev.inf to consume >>> RegisterFilterLib for all supported Arch >>> Update BaseLib.inf to consume RegisterFilterLib only for IA32 and X64 >> >> Seems like a sound plan, but there are more IoLib instances than the above: >> >> MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf >> MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicArmVirt.inf >> MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf > > I agree that all 3 of these need to be included in the plan. > >> MdePkg/Library/DxeIoLibCpuIo2/DxeIoLibCpuIo2.inf >> MdePkg/Library/PeiIoLibCpuIo/PeiIoLibCpuIo.inf >> MdePkg/Library/SmmIoLibSmmCpuIo2/SmmIoLibSmmCpuIo2.inf > > The IoLib instances all perform their I/O operation by calling > a dynamic PPI/Protocol services. I would recommend that we do not update > these instances, and instead only apply the RegisterFilterLib to > IoLib instances that perform he direct access to the hardware. > Any IoLib instances that access the hardware through a PPI/Protocol > should not be updated. > > We have a few implementations of the CPI I/O PPI/Protocol that > use the BaseIoLibIntrinsics, so those would actually be covered > by the first set of lib instances. If a platform decides to > implement a new version of the CPU I/O PPI/Protocol that does not > use one of the BaseIoLibInstrinsic instances, then they would > have the option of using the RegisterFilterLib in that new > implementation of the CPI I/O PPI/Protocol.
Thanks for the explanation; I agree. Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#72584): https://edk2.groups.io/g/devel/message/72584 Mute This Topic: https://groups.io/mt/81167761/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
