Re: [PATCH 00/14] rid PiSmmCpuDxeSmm of DB-encoded instructions

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Laszlo,

Thanks for all the work on this series and the very
detailed commit messages.

Liming's email on removing the .S and .asm files is an
RFC.  We need to see this RFC approved before we can
commit changes to remove .S and .asm files.  This should
be a separate activity.

One odd thing I see in this series is that the instruction
patch label in the .nasm file is just a label and does not
have any storage associated with it.  But in the C code
the type UINT8 is used with the label which implies some
storage.  Can we make the globals in C code be a pointer
(maybe VOID *) instead of UINT8?

Thanks,

Mike

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, February 2, 2018 6:40 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong,
> Eric <eric.dong@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Leif Lindholm
> <leif.lindholm@linaro.org>; Gao, Liming
> <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Ni, Ruiyu
> <ruiyu.ni@intel.com>
> Subject: [PATCH 00/14] rid PiSmmCpuDxeSmm of DB-encoded
> instructions
> 
> Repo:   https://github.com/lersek/edk2.git
> Branch: patch_insn_x86
> 
> Patch 01 is a comment cleanup patch for "BaseLib.h".
> 
> Patch 02 introduces PatchInstructionX86() to BaseLib,
> based on the
> recent discussion.
> 
> Patch 03 removes *.S and *.asm files from PiSmmCpuDxeSmm,
> so that the
> rest of the series only needs to concern itself with
> *.nasm files. (The
> subject of removing *.S and *.asm files for x86 was
> broached by Liming
> on the list earlier; it's handy for this series.)
> 
> Patches 04 through 14 replace the DB encodings of
> instructions in
> PiSmmCpuDxeSmm NASM source code. Most of the time the new
> PatchInstructionX86() function is utilized, but in some
> cases, not even
> PatchInstructionX86() is needed.
> 
> Tested the following OSes with this series (all cases
> used -D
> SMM_REQUIRE, 2-4 VCPUs, both normal boot and S3, on KVM):
> 
> - IA32
>   - Fedora 26
> 
> - IA32X64
>   - Fedora 26
>   - Windows 7
>   - Windows 8.1
>   - Windows 10
>   - Windows Server 2008 R2
>   - Windows Server 2012 R2
>   - Windows Server 2016 (normal boot only -- S3 is
> untestable at this
>     time due to QXL GPU driver signing issues)
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> 
> Thanks,
> Laszlo
> 
> Laszlo Ersek (14):
>   MdePkg/BaseLib.h: state preprocessing conditions in
> comments after
>     #endifs
>   MdePkg/BaseLib: add PatchInstructionX86()
>   UefiCpuPkg/PiSmmCpuDxeSmm: remove *.S and *.asm
> assembly files
>   UefiCpuPkg/PiSmmCpuDxeSmm: patch "gSmbase" with
> PatchInstructionX86()
>   UefiCpuPkg/PiSmmCpuDxeSmm: patch "gSmiStack" with
>     PatchInstructionX86()
>   UefiCpuPkg/PiSmmCpuDxeSmm: patch "gSmiCr3" with
> PatchInstructionX86()
>   UefiCpuPkg/PiSmmCpuDxeSmm: patch "XdSupported" with
>     PatchInstructionX86()
>   UefiCpuPkg/PiSmmCpuDxeSmm: remove unneeded DBs from X64
> SmmStartup()
>   UefiCpuPkg/PiSmmCpuDxeSmm: patch "gSmmCr3" with
> PatchInstructionX86()
>   UefiCpuPkg/PiSmmCpuDxeSmm: patch "gSmmCr4" with
> PatchInstructionX86()
>   UefiCpuPkg/PiSmmCpuDxeSmm: patch "gSmmCr0" with
> PatchInstructionX86()
>   UefiCpuPkg/PiSmmCpuDxeSmm: eliminate "gSmmJmpAddr" and
> related DBs
>   UefiCpuPkg/PiSmmCpuDxeSmm: patch "gSmmInitStack" with
>     PatchInstructionX86()
>   UefiCpuPkg/PiSmmCpuDxeSmm: remove DBs from
>     SmmRelocationSemaphoreComplete32()
> 
>  MdePkg/Include/Library/BaseLib.h                |  62 +-
>  MdePkg/Library/BaseLib/BaseLib.inf              |   2 +
>  MdePkg/Library/BaseLib/X86PatchInstruction.c    |  89
> +++
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c               |   4 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/MpFuncs.S        | 165 --
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/MpFuncs.asm      | 168 --
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S       | 215 --
> ----
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm     | 223 --
> ----
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm    |  25 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.S   | 696 --
> -----------------
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm | 713 --
> ------------------
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.S        |  84 --
> -
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.asm      |  94 --
> -
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm     |  30 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c      |  27 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h      |  21 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf    |  20 -
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c          |   7 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h  |   1 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c      |  16 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/MpFuncs.S         | 204 --
> ----
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/MpFuncs.asm       | 206 --
> ----
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/Semaphore.c       |  16 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.S        | 243 --
> -----
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.asm      | 242 --
> -----
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm     |  25 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiException.S    | 365 --
> --------
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiException.asm  | 383 --
> ---------
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.S         | 141 --
> --
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.asm       | 132 --
> --
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm      |  76 +-
> -
>  31 files changed, 271 insertions(+), 4424 deletions(-)
>  create mode 100644
> MdePkg/Library/BaseLib/X86PatchInstruction.c
>  delete mode 100644
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/MpFuncs.S
>  delete mode 100644
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/MpFuncs.asm
>  delete mode 100644
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S
>  delete mode 100644
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm
>  delete mode 100644
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.S
>  delete mode 100644
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm
>  delete mode 100644
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.S
>  delete mode 100644
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.asm
>  delete mode 100644
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/MpFuncs.S
>  delete mode 100644
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/MpFuncs.asm
>  delete mode 100644
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.S
>  delete mode 100644
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.asm
>  delete mode 100644
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiException.S
>  delete mode 100644
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiException.asm
>  delete mode 100644
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.S
>  delete mode 100644
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.asm
> 
> --
> 2.14.1.3.gb7cf6e02401b