[edk2-devel] [RFC][patch] Add a new library class RegisterFilterLib in edk2 to filter/trace port IO/MMIO/MSR access

Dandan Bi posted 1 patch 3 years, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/edk2 tags/patchew/20210308051532.13872-1-dandan.bi@intel.com
There is a newer version of this series
MdePkg/Include/Library/RegisterFilterLib.h | 224 +++++++++++++++++++++
1 file changed, 224 insertions(+)
create mode 100644 MdePkg/Include/Library/RegisterFilterLib.h
[edk2-devel] [RFC][patch] Add a new library class RegisterFilterLib in edk2 to filter/trace port IO/MMIO/MSR access
Posted by Dandan Bi 3 years, 1 month ago
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

This topic has been reviewed in Tiano Design meeting of 2021/0305
RegisterFilterLib header file and desgin foil can be found in:
https://edk2.groups.io/g/devel/files/Designs/2021/0305


Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Signed-off-by: Dandan Bi <dandan.bi@intel.com>
---
 MdePkg/Include/Library/RegisterFilterLib.h | 224 +++++++++++++++++++++
 1 file changed, 224 insertions(+)
 create mode 100644 MdePkg/Include/Library/RegisterFilterLib.h

diff --git a/MdePkg/Include/Library/RegisterFilterLib.h b/MdePkg/Include/Library/RegisterFilterLib.h
new file mode 100644
index 0000000000..be111304ba
--- /dev/null
+++ b/MdePkg/Include/Library/RegisterFilterLib.h
@@ -0,0 +1,224 @@
+/** @file
+  Public include file for the Port IO/MMIO/MSR filter Library
+
+Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef __REGISTER_FILTER_LIB_H__
+#define __REGISTER_FILTER_LIB_H__
+
+typedef enum {
+  FilterWidth8,
+  FilterWidth16,
+  FilterWidth32,
+  FilterWidth64
+} FILTER_IO_WIDTH;
+
+/**
+  Filter IO read operation before read IO port.
+  It is used to filter IO read operation.
+
+  It will return the flag to decide whether require read real IO port.
+  It can be used for emulation environment.
+
+  @param[in]       Width    Signifies the width of the I/O operation.
+  @param[in]       Address  The base address of the I/O operation.
+  @param[in]       Buffer   The destination buffer to store the results.
+
+**/
+BOOLEAN
+EFIAPI
+FilterBeforeIoRead (
+  IN FILTER_IO_WIDTH   Width,
+  IN UINTN             Address,
+  IN OUT VOID          *Buffer
+  );
+
+/**
+  Trace IO read operation after read IO port.
+  It is used to trace IO operation.
+
+  @param[in]       Width    Signifies the width of the I/O operation.
+  @param[in]       Address  The base address of the I/O operation.
+  @param[in]       Buffer   The destination buffer to store the results.
+
+**/
+VOID
+EFIAPI
+FilterAfterIoRead (
+  IN FILTER_IO_WIDTH  Width,
+  IN UINTN            Address,
+  IN VOID             *Buffer
+  );
+/**
+  Filter IO Write operation before wirte IO port.
+  It is used to filter IO operation.
+
+  It will return the flag to decide whether require read write IO port.
+  It can be used for emulation environment.
+
+  @param[in]       Width    Signifies the width of the I/O operation.
+  @param[in]       Address  The base address of the I/O operation.
+  @param[in]       Buffer   The source buffer from which to BeforeWrite data.
+
+**/
+BOOLEAN
+EFIAPI
+FilterBeforeIoWrite (
+  IN FILTER_IO_WIDTH  Width,
+  IN UINTN            Address,
+  IN VOID             *Buffer
+  );
+
+  /**
+  Trace IO Write operation after wirte IO port.
+  It is used to trace IO operation.
+
+  @param[in]       Width    Signifies the width of the I/O operation.
+  @param[in]       Address  The base address of the I/O operation.
+  @param[in]       Buffer   The source buffer from which to BeforeWrite data.
+
+**/
+VOID
+EFIAPI
+FilterAfterIoWrite (
+  IN FILTER_IO_WIDTH  Width,
+  IN UINTN            Address,
+  IN VOID             *Buffer
+  );
+
+/**
+  Filter memory IO before Read operation.
+
+  It will return the flag to decide whether require read real MMIO.
+  It can be used for emulation environment.
+
+  @param[in]       Width    Signifies the width of the memory I/O operation.
+  @param[in]       Address  The base address of the memory I/O operation.
+  @param[in]       Buffer   The destination buffer to store the results.
+
+**/
+BOOLEAN
+EFIAPI
+FilterBeforeMmIoRead (
+  IN FILTER_IO_WIDTH  Width,
+  IN UINTN            Address,
+  IN OUT VOID         *Buffer
+  );
+
+/**
+  Tracer memory IO after read operation
+
+  @param[in]       Width    Signifies the width of the memory I/O operation.
+  @param[in]       Address  The base address of the memory I/O operation.
+  @param[in]       Buffer   The destination buffer to store the results.
+
+**/
+VOID
+EFIAPI
+FilterAfterMmIoRead (
+  IN FILTER_IO_WIDTH  Width,
+  IN UINTN            Address,
+  IN VOID             *Buffer
+  );
+
+/**
+  Filter memory IO before write operation
+
+  It will return the flag to decide whether require wirte real MMIO.
+  It can be used for emulation environment.
+
+  @param[in]       Width    Signifies the width of the memory I/O operation.
+  @param[in]       Address  The base address of the memory I/O operation.
+  @param[in]       Buffer   The source buffer from which to BeforeWrite data.
+
+**/
+BOOLEAN
+EFIAPI
+FilterBeforeMmIoWrite (
+  IN FILTER_IO_WIDTH  Width,
+  IN UINTN            Address,
+  IN VOID             *Buffer
+  );
+
+/**
+  Tracer memory IO after write operation
+
+  @param[in]       Width    Signifies the width of the memory I/O operation.
+  @param[in]       Address  The base address of the memory I/O operation.
+  @param[in]       Buffer   The source buffer from which to BeforeWrite data.
+
+**/
+VOID
+EFIAPI
+FilterAfterMmIoWrite (
+  IN FILTER_IO_WIDTH  Width,
+  IN UINTN            Address,
+  IN VOID             *Buffer
+  );
+
+/**
+  Filter MSR before read operation.
+
+  It will return the flag to decide whether require read real MSR.
+  It can be used for emulation environment.
+
+  @param  Index                     The 8-bit Machine Specific Register index to BeforeWrite.
+  @param  Value                     The 64-bit value to BeforeRead from the Machine Specific Register.
+
+**/
+BOOLEAN
+EFIAPI
+FilterBeforeMsrRead (
+  IN UINT32           Index,
+  IN OUT UINT64       *Value
+  );
+
+/**
+  Trace MSR after read operation
+
+  @param  Index                     The 8-bit Machine Specific Register index to BeforeWrite.
+  @param  Value                     The 64-bit value to BeforeRead from the Machine Specific Register.
+
+**/
+VOID
+EFIAPI
+FilterAfterMsrRead (
+  IN UINT32            Index,
+  IN UINT64            *Value
+  );
+
+/**
+  Filter MSR before write operation
+
+  It will return the flag to decide whether require write real MSR.
+  It can be used for emulation environment.
+
+  @param  Index                     The 8-bit Machine Specific Register index to BeforeWrite.
+  @param  Value                     The 64-bit value to BeforeWrite to the Machine Specific Register.
+
+**/
+BOOLEAN
+EFIAPI
+FilterBeforeMsrWrite (
+  IN UINT32           Index,
+  IN UINT64           *Value
+  );
+
+/**
+  Trace MSR after write operation
+
+  @param  Index                     The 8-bit Machine Specific Register index to BeforeWrite.
+  @param  Value                     The 64-bit value to BeforeWrite to the Machine Specific Register.
+
+**/
+VOID
+EFIAPI
+FilterAfterMsrWrite (
+  IN UINT32            Index,
+  IN UINT64            *Value
+  );
+
+#endif // __REGISTER_FILTER_LIB_H__
-- 
2.18.0.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72530): https://edk2.groups.io/g/devel/message/72530
Mute This Topic: https://groups.io/mt/81167761/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [RFC][patch] Add a new library class RegisterFilterLib in edk2 to filter/trace port IO/MMIO/MSR access
Posted by Laszlo Ersek 3 years, 1 month ago
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
MdePkg/Library/DxeIoLibCpuIo2/DxeIoLibCpuIo2.inf
MdePkg/Library/PeiIoLibCpuIo/PeiIoLibCpuIo.inf
MdePkg/Library/SmmIoLibSmmCpuIo2/SmmIoLibSmmCpuIo2.inf

Thanks
Laszlo

> 
> This topic has been reviewed in Tiano Design meeting of 2021/0305
> RegisterFilterLib header file and desgin foil can be found in:
> https://edk2.groups.io/g/devel/files/Designs/2021/0305
> 
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Signed-off-by: Dandan Bi <dandan.bi@intel.com>
> ---
>  MdePkg/Include/Library/RegisterFilterLib.h | 224 +++++++++++++++++++++
>  1 file changed, 224 insertions(+)
>  create mode 100644 MdePkg/Include/Library/RegisterFilterLib.h
> 
> diff --git a/MdePkg/Include/Library/RegisterFilterLib.h b/MdePkg/Include/Library/RegisterFilterLib.h
> new file mode 100644
> index 0000000000..be111304ba
> --- /dev/null
> +++ b/MdePkg/Include/Library/RegisterFilterLib.h
> @@ -0,0 +1,224 @@
> +/** @file
> +  Public include file for the Port IO/MMIO/MSR filter Library
> +
> +Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef __REGISTER_FILTER_LIB_H__
> +#define __REGISTER_FILTER_LIB_H__
> +
> +typedef enum {
> +  FilterWidth8,
> +  FilterWidth16,
> +  FilterWidth32,
> +  FilterWidth64
> +} FILTER_IO_WIDTH;
> +
> +/**
> +  Filter IO read operation before read IO port.
> +  It is used to filter IO read operation.
> +
> +  It will return the flag to decide whether require read real IO port.
> +  It can be used for emulation environment.
> +
> +  @param[in]       Width    Signifies the width of the I/O operation.
> +  @param[in]       Address  The base address of the I/O operation.
> +  @param[in]       Buffer   The destination buffer to store the results.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +FilterBeforeIoRead (
> +  IN FILTER_IO_WIDTH   Width,
> +  IN UINTN             Address,
> +  IN OUT VOID          *Buffer
> +  );
> +
> +/**
> +  Trace IO read operation after read IO port.
> +  It is used to trace IO operation.
> +
> +  @param[in]       Width    Signifies the width of the I/O operation.
> +  @param[in]       Address  The base address of the I/O operation.
> +  @param[in]       Buffer   The destination buffer to store the results.
> +
> +**/
> +VOID
> +EFIAPI
> +FilterAfterIoRead (
> +  IN FILTER_IO_WIDTH  Width,
> +  IN UINTN            Address,
> +  IN VOID             *Buffer
> +  );
> +/**
> +  Filter IO Write operation before wirte IO port.
> +  It is used to filter IO operation.
> +
> +  It will return the flag to decide whether require read write IO port.
> +  It can be used for emulation environment.
> +
> +  @param[in]       Width    Signifies the width of the I/O operation.
> +  @param[in]       Address  The base address of the I/O operation.
> +  @param[in]       Buffer   The source buffer from which to BeforeWrite data.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +FilterBeforeIoWrite (
> +  IN FILTER_IO_WIDTH  Width,
> +  IN UINTN            Address,
> +  IN VOID             *Buffer
> +  );
> +
> +  /**
> +  Trace IO Write operation after wirte IO port.
> +  It is used to trace IO operation.
> +
> +  @param[in]       Width    Signifies the width of the I/O operation.
> +  @param[in]       Address  The base address of the I/O operation.
> +  @param[in]       Buffer   The source buffer from which to BeforeWrite data.
> +
> +**/
> +VOID
> +EFIAPI
> +FilterAfterIoWrite (
> +  IN FILTER_IO_WIDTH  Width,
> +  IN UINTN            Address,
> +  IN VOID             *Buffer
> +  );
> +
> +/**
> +  Filter memory IO before Read operation.
> +
> +  It will return the flag to decide whether require read real MMIO.
> +  It can be used for emulation environment.
> +
> +  @param[in]       Width    Signifies the width of the memory I/O operation.
> +  @param[in]       Address  The base address of the memory I/O operation.
> +  @param[in]       Buffer   The destination buffer to store the results.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +FilterBeforeMmIoRead (
> +  IN FILTER_IO_WIDTH  Width,
> +  IN UINTN            Address,
> +  IN OUT VOID         *Buffer
> +  );
> +
> +/**
> +  Tracer memory IO after read operation
> +
> +  @param[in]       Width    Signifies the width of the memory I/O operation.
> +  @param[in]       Address  The base address of the memory I/O operation.
> +  @param[in]       Buffer   The destination buffer to store the results.
> +
> +**/
> +VOID
> +EFIAPI
> +FilterAfterMmIoRead (
> +  IN FILTER_IO_WIDTH  Width,
> +  IN UINTN            Address,
> +  IN VOID             *Buffer
> +  );
> +
> +/**
> +  Filter memory IO before write operation
> +
> +  It will return the flag to decide whether require wirte real MMIO.
> +  It can be used for emulation environment.
> +
> +  @param[in]       Width    Signifies the width of the memory I/O operation.
> +  @param[in]       Address  The base address of the memory I/O operation.
> +  @param[in]       Buffer   The source buffer from which to BeforeWrite data.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +FilterBeforeMmIoWrite (
> +  IN FILTER_IO_WIDTH  Width,
> +  IN UINTN            Address,
> +  IN VOID             *Buffer
> +  );
> +
> +/**
> +  Tracer memory IO after write operation
> +
> +  @param[in]       Width    Signifies the width of the memory I/O operation.
> +  @param[in]       Address  The base address of the memory I/O operation.
> +  @param[in]       Buffer   The source buffer from which to BeforeWrite data.
> +
> +**/
> +VOID
> +EFIAPI
> +FilterAfterMmIoWrite (
> +  IN FILTER_IO_WIDTH  Width,
> +  IN UINTN            Address,
> +  IN VOID             *Buffer
> +  );
> +
> +/**
> +  Filter MSR before read operation.
> +
> +  It will return the flag to decide whether require read real MSR.
> +  It can be used for emulation environment.
> +
> +  @param  Index                     The 8-bit Machine Specific Register index to BeforeWrite.
> +  @param  Value                     The 64-bit value to BeforeRead from the Machine Specific Register.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +FilterBeforeMsrRead (
> +  IN UINT32           Index,
> +  IN OUT UINT64       *Value
> +  );
> +
> +/**
> +  Trace MSR after read operation
> +
> +  @param  Index                     The 8-bit Machine Specific Register index to BeforeWrite.
> +  @param  Value                     The 64-bit value to BeforeRead from the Machine Specific Register.
> +
> +**/
> +VOID
> +EFIAPI
> +FilterAfterMsrRead (
> +  IN UINT32            Index,
> +  IN UINT64            *Value
> +  );
> +
> +/**
> +  Filter MSR before write operation
> +
> +  It will return the flag to decide whether require write real MSR.
> +  It can be used for emulation environment.
> +
> +  @param  Index                     The 8-bit Machine Specific Register index to BeforeWrite.
> +  @param  Value                     The 64-bit value to BeforeWrite to the Machine Specific Register.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +FilterBeforeMsrWrite (
> +  IN UINT32           Index,
> +  IN UINT64           *Value
> +  );
> +
> +/**
> +  Trace MSR after write operation
> +
> +  @param  Index                     The 8-bit Machine Specific Register index to BeforeWrite.
> +  @param  Value                     The 64-bit value to BeforeWrite to the Machine Specific Register.
> +
> +**/
> +VOID
> +EFIAPI
> +FilterAfterMsrWrite (
> +  IN UINT32            Index,
> +  IN UINT64            *Value
> +  );
> +
> +#endif // __REGISTER_FILTER_LIB_H__
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72537): https://edk2.groups.io/g/devel/message/72537
Mute This Topic: https://groups.io/mt/81167761/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [RFC][patch] Add a new library class RegisterFilterLib in edk2 to filter/trace port IO/MMIO/MSR access
Posted by Michael D Kinney 3 years, 1 month ago

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Monday, March 8, 2021 7:38 AM
> To: devel@edk2.groups.io; Bi, Dandan <dandan.bi@intel.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang
> <zhiguang.liu@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
> Laszlo
> 
> >
> > This topic has been reviewed in Tiano Design meeting of 2021/0305
> > RegisterFilterLib header file and desgin foil can be found in:
> > https://edk2.groups.io/g/devel/files/Designs/2021/0305
> >
> >
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> > Signed-off-by: Dandan Bi <dandan.bi@intel.com>
> > ---
> >  MdePkg/Include/Library/RegisterFilterLib.h | 224 +++++++++++++++++++++
> >  1 file changed, 224 insertions(+)
> >  create mode 100644 MdePkg/Include/Library/RegisterFilterLib.h
> >
> > diff --git a/MdePkg/Include/Library/RegisterFilterLib.h b/MdePkg/Include/Library/RegisterFilterLib.h
> > new file mode 100644
> > index 0000000000..be111304ba
> > --- /dev/null
> > +++ b/MdePkg/Include/Library/RegisterFilterLib.h
> > @@ -0,0 +1,224 @@
> > +/** @file
> > +  Public include file for the Port IO/MMIO/MSR filter Library
> > +
> > +Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
> > +SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#ifndef __REGISTER_FILTER_LIB_H__
> > +#define __REGISTER_FILTER_LIB_H__
> > +
> > +typedef enum {
> > +  FilterWidth8,
> > +  FilterWidth16,
> > +  FilterWidth32,
> > +  FilterWidth64
> > +} FILTER_IO_WIDTH;
> > +
> > +/**
> > +  Filter IO read operation before read IO port.
> > +  It is used to filter IO read operation.
> > +
> > +  It will return the flag to decide whether require read real IO port.
> > +  It can be used for emulation environment.
> > +
> > +  @param[in]       Width    Signifies the width of the I/O operation.
> > +  @param[in]       Address  The base address of the I/O operation.
> > +  @param[in]       Buffer   The destination buffer to store the results.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +FilterBeforeIoRead (
> > +  IN FILTER_IO_WIDTH   Width,
> > +  IN UINTN             Address,
> > +  IN OUT VOID          *Buffer
> > +  );
> > +
> > +/**
> > +  Trace IO read operation after read IO port.
> > +  It is used to trace IO operation.
> > +
> > +  @param[in]       Width    Signifies the width of the I/O operation.
> > +  @param[in]       Address  The base address of the I/O operation.
> > +  @param[in]       Buffer   The destination buffer to store the results.
> > +
> > +**/
> > +VOID
> > +EFIAPI
> > +FilterAfterIoRead (
> > +  IN FILTER_IO_WIDTH  Width,
> > +  IN UINTN            Address,
> > +  IN VOID             *Buffer
> > +  );
> > +/**
> > +  Filter IO Write operation before wirte IO port.
> > +  It is used to filter IO operation.
> > +
> > +  It will return the flag to decide whether require read write IO port.
> > +  It can be used for emulation environment.
> > +
> > +  @param[in]       Width    Signifies the width of the I/O operation.
> > +  @param[in]       Address  The base address of the I/O operation.
> > +  @param[in]       Buffer   The source buffer from which to BeforeWrite data.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +FilterBeforeIoWrite (
> > +  IN FILTER_IO_WIDTH  Width,
> > +  IN UINTN            Address,
> > +  IN VOID             *Buffer
> > +  );
> > +
> > +  /**
> > +  Trace IO Write operation after wirte IO port.
> > +  It is used to trace IO operation.
> > +
> > +  @param[in]       Width    Signifies the width of the I/O operation.
> > +  @param[in]       Address  The base address of the I/O operation.
> > +  @param[in]       Buffer   The source buffer from which to BeforeWrite data.
> > +
> > +**/
> > +VOID
> > +EFIAPI
> > +FilterAfterIoWrite (
> > +  IN FILTER_IO_WIDTH  Width,
> > +  IN UINTN            Address,
> > +  IN VOID             *Buffer
> > +  );
> > +
> > +/**
> > +  Filter memory IO before Read operation.
> > +
> > +  It will return the flag to decide whether require read real MMIO.
> > +  It can be used for emulation environment.
> > +
> > +  @param[in]       Width    Signifies the width of the memory I/O operation.
> > +  @param[in]       Address  The base address of the memory I/O operation.
> > +  @param[in]       Buffer   The destination buffer to store the results.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +FilterBeforeMmIoRead (
> > +  IN FILTER_IO_WIDTH  Width,
> > +  IN UINTN            Address,
> > +  IN OUT VOID         *Buffer
> > +  );
> > +
> > +/**
> > +  Tracer memory IO after read operation
> > +
> > +  @param[in]       Width    Signifies the width of the memory I/O operation.
> > +  @param[in]       Address  The base address of the memory I/O operation.
> > +  @param[in]       Buffer   The destination buffer to store the results.
> > +
> > +**/
> > +VOID
> > +EFIAPI
> > +FilterAfterMmIoRead (
> > +  IN FILTER_IO_WIDTH  Width,
> > +  IN UINTN            Address,
> > +  IN VOID             *Buffer
> > +  );
> > +
> > +/**
> > +  Filter memory IO before write operation
> > +
> > +  It will return the flag to decide whether require wirte real MMIO.
> > +  It can be used for emulation environment.
> > +
> > +  @param[in]       Width    Signifies the width of the memory I/O operation.
> > +  @param[in]       Address  The base address of the memory I/O operation.
> > +  @param[in]       Buffer   The source buffer from which to BeforeWrite data.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +FilterBeforeMmIoWrite (
> > +  IN FILTER_IO_WIDTH  Width,
> > +  IN UINTN            Address,
> > +  IN VOID             *Buffer
> > +  );
> > +
> > +/**
> > +  Tracer memory IO after write operation
> > +
> > +  @param[in]       Width    Signifies the width of the memory I/O operation.
> > +  @param[in]       Address  The base address of the memory I/O operation.
> > +  @param[in]       Buffer   The source buffer from which to BeforeWrite data.
> > +
> > +**/
> > +VOID
> > +EFIAPI
> > +FilterAfterMmIoWrite (
> > +  IN FILTER_IO_WIDTH  Width,
> > +  IN UINTN            Address,
> > +  IN VOID             *Buffer
> > +  );
> > +
> > +/**
> > +  Filter MSR before read operation.
> > +
> > +  It will return the flag to decide whether require read real MSR.
> > +  It can be used for emulation environment.
> > +
> > +  @param  Index                     The 8-bit Machine Specific Register index to BeforeWrite.
> > +  @param  Value                     The 64-bit value to BeforeRead from the Machine Specific Register.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +FilterBeforeMsrRead (
> > +  IN UINT32           Index,
> > +  IN OUT UINT64       *Value
> > +  );
> > +
> > +/**
> > +  Trace MSR after read operation
> > +
> > +  @param  Index                     The 8-bit Machine Specific Register index to BeforeWrite.
> > +  @param  Value                     The 64-bit value to BeforeRead from the Machine Specific Register.
> > +
> > +**/
> > +VOID
> > +EFIAPI
> > +FilterAfterMsrRead (
> > +  IN UINT32            Index,
> > +  IN UINT64            *Value
> > +  );
> > +
> > +/**
> > +  Filter MSR before write operation
> > +
> > +  It will return the flag to decide whether require write real MSR.
> > +  It can be used for emulation environment.
> > +
> > +  @param  Index                     The 8-bit Machine Specific Register index to BeforeWrite.
> > +  @param  Value                     The 64-bit value to BeforeWrite to the Machine Specific Register.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +FilterBeforeMsrWrite (
> > +  IN UINT32           Index,
> > +  IN UINT64           *Value
> > +  );
> > +
> > +/**
> > +  Trace MSR after write operation
> > +
> > +  @param  Index                     The 8-bit Machine Specific Register index to BeforeWrite.
> > +  @param  Value                     The 64-bit value to BeforeWrite to the Machine Specific Register.
> > +
> > +**/
> > +VOID
> > +EFIAPI
> > +FilterAfterMsrWrite (
> > +  IN UINT32            Index,
> > +  IN UINT64            *Value
> > +  );
> > +
> > +#endif // __REGISTER_FILTER_LIB_H__
> >



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72545): https://edk2.groups.io/g/devel/message/72545
Mute This Topic: https://groups.io/mt/81167761/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [RFC][patch] Add a new library class RegisterFilterLib in edk2 to filter/trace port IO/MMIO/MSR access
Posted by Laszlo Ersek 3 years, 1 month ago
On 03/08/21 22:55, Kinney, Michael D wrote:
> 
> 
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Monday, March 8, 2021 7:38 AM
>> To: devel@edk2.groups.io; Bi, Dandan <dandan.bi@intel.com>
>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang
>> <zhiguang.liu@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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-