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
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 02/03/18 01:45, Kinney, Michael D wrote: > 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. Sure, I can drop that patch, but then the PiSmmCpuDxeSmm changes in the other patches will divert the NASM files from the .S and .asm files. Is that (temporary) non-uniformity better than removing the .S and .asm files? > 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. No, this is not correct; the storage that is associated with each of these "patch labels" is the one byte (UINT8) directly following the label -- whatever that byte might be. It is generally part of a totally unrelated instruction. In case we had to patch an immediate operand that happened to comprise the very last byte(s) of a NASM source file, *then* we'd have to add one dummy DB at the end, just so there was something that the label directly refered to. This is why UINT8 is a good type here, because it requires us to add the least amount of padding. > 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? I don't think so. For building the addresses, we rely on the linker, and the linker needs definitions (allocations) of objects. Your above observation is correct (i.e. that storage is required), my addition to that is that storage is *already* allocated (one UINT8 per patch label / symbol). Thanks! Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo, Let's see if we can close on the timeline for the .S/.asm RFC this week. I am concerned about making them UINT8 from C code because future maintainer may think that the patch value type is UINT8. Labels in assembly that are defined to be a function that is callable from C code does not have a storage type. Why can't we make these labels the same way? Mike > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Monday, February 5, 2018 2:28 AM > To: Kinney, Michael D <michael.d.kinney@intel.com>; 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>; Ni, Ruiyu <ruiyu.ni@intel.com> > Subject: Re: [PATCH 00/14] rid PiSmmCpuDxeSmm of DB- > encoded instructions > > On 02/03/18 01:45, Kinney, Michael D wrote: > > 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. > > Sure, I can drop that patch, but then the PiSmmCpuDxeSmm > changes in the > other patches will divert the NASM files from the .S and > .asm files. Is > that (temporary) non-uniformity better than removing the > .S and .asm files? > > > 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. > > No, this is not correct; the storage that is associated > with each of > these "patch labels" is the one byte (UINT8) directly > following the > label -- whatever that byte might be. It is generally > part of a totally > unrelated instruction. > > In case we had to patch an immediate operand that > happened to comprise > the very last byte(s) of a NASM source file, *then* we'd > have to add one > dummy DB at the end, just so there was something that the > label directly > refered to. > > This is why UINT8 is a good type here, because it > requires us to add the > least amount of padding. > > > 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? > > I don't think so. For building the addresses, we rely on > the linker, and > the linker needs definitions (allocations) of objects. > Your above > observation is correct (i.e. that storage is required), > my addition to > that is that storage is *already* allocated (one UINT8 > per patch label / > symbol). > > Thanks! > Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 02/05/18 19:22, Kinney, Michael D wrote:
> Laszlo,
>
> Let's see if we can close on the timeline for
> the .S/.asm RFC this week.
>
> I am concerned about making them UINT8 from C code
> because future maintainer may think that the patch
> value type is UINT8.
>
> Labels in assembly that are defined to be a function
> that is callable from C code does not have a storage
> type. Why can't we make these labels the same way?
To my understanding, the labels in the NASM source code for functions
and variables look the same; the actual declaration only comes from the
C code.
(Assuming we declare a NASM label as a function in the C source, nothing
in the toolchain enforces an actual match between caller and callee; it
is possible to call the function (from C) through a declaration that
doesn't match the actual assembly implementation. IOW it's up to us to
avoid such bugs.)
If I understand correctly, you are suggesting that we take a label from
the NASM source that stands right after an instruction to patch, and we
declare it as a function in the C source. (With what prototype though?
The label does not actually introduce a function definition in the
assembly code; it would make no sense to call it.) Then, for the
patching, I presume your suggestion is to convert the address of the
function to UINTN, perform the subtraction, etc. Something like:
typedef VOID (X86_ASSEMBLY_LABEL) (VOID);
(This is not a pointer-to-function type, but a function type.)
A declaration using the typedef would be
extern X86_ASSEMBLY_LABEL gPatchCr3;
(This declares an extern function, not a pointer to a function.)
The patching function would take a pointer to a function:
VOID
EFIAPI
PatchInstructionX86 (
OUT X86_ASSEMBLY_LABEL *InstructionEnd,
IN UINT64 PatchValue,
IN UINTN ValueSize
);
and the implementation would have to do e.g.
WriteUnaligned32 (
(UINT32 *)(UINTN)InstructionEnd - 1,
(UINT32)PatchValue
);
It would be called like
PatchInstructionX86 (&gPatchCr3, Value, 4);
But, what does this buy us in comparison to just:
typedef UINT8 X86_ASSEMBLY_LABEL;
?
If you worry that a future maintainer misunderstands the UINT8, then we
can as well hide the UINT8 behind a typedef; X86_ASSEMBLY_LABEL doesn't
have to be a function type for the hiding. (Conversely, when using a
function type as underlying type, I worry that a future maintainer might
be tempted to call them :) )
Thanks,
Laszlo
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Monday, February 5, 2018 2:28 AM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; 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>; Ni, Ruiyu <ruiyu.ni@intel.com>
>> Subject: Re: [PATCH 00/14] rid PiSmmCpuDxeSmm of DB-
>> encoded instructions
>>
>> On 02/03/18 01:45, Kinney, Michael D wrote:
>>> 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.
>>
>> Sure, I can drop that patch, but then the PiSmmCpuDxeSmm
>> changes in the
>> other patches will divert the NASM files from the .S and
>> .asm files. Is
>> that (temporary) non-uniformity better than removing the
>> .S and .asm files?
>>
>>> 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.
>>
>> No, this is not correct; the storage that is associated
>> with each of
>> these "patch labels" is the one byte (UINT8) directly
>> following the
>> label -- whatever that byte might be. It is generally
>> part of a totally
>> unrelated instruction.
>>
>> In case we had to patch an immediate operand that
>> happened to comprise
>> the very last byte(s) of a NASM source file, *then* we'd
>> have to add one
>> dummy DB at the end, just so there was something that the
>> label directly
>> refered to.
>>
>> This is why UINT8 is a good type here, because it
>> requires us to add the
>> least amount of padding.
>>
>>> 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?
>>
>> I don't think so. For building the addresses, we rely on
>> the linker, and
>> the linker needs definitions (allocations) of objects.
>> Your above
>> observation is correct (i.e. that storage is required),
>> my addition to
>> that is that storage is *already* allocated (one UINT8
>> per patch label /
>> symbol).
>>
>> Thanks!
>> Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo, I do like this typedef idea. typedef VOID (X86_ASSEMBLY_LABEL) (VOID); Maybe change the name so it is clearer that this should never be used in a call. A comment block about the typedef can also clarify the expected usage. typedef VOID (X86_ASSEMBLY_PATCH_LABEL) (VOID); Thanks, Mike > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Monday, February 5, 2018 11:23 AM > To: Kinney, Michael D <michael.d.kinney@intel.com>; > 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>; Ni, Ruiyu <ruiyu.ni@intel.com> > Subject: Re: [PATCH 00/14] rid PiSmmCpuDxeSmm of DB- > encoded instructions > > On 02/05/18 19:22, Kinney, Michael D wrote: > > Laszlo, > > > > Let's see if we can close on the timeline for > > the .S/.asm RFC this week. > > > > I am concerned about making them UINT8 from C code > > because future maintainer may think that the patch > > value type is UINT8. > > > > Labels in assembly that are defined to be a function > > that is callable from C code does not have a storage > > type. Why can't we make these labels the same way? > > To my understanding, the labels in the NASM source code > for functions > and variables look the same; the actual declaration > only comes from the > C code. > > (Assuming we declare a NASM label as a function in the > C source, nothing > in the toolchain enforces an actual match between > caller and callee; it > is possible to call the function (from C) through a > declaration that > doesn't match the actual assembly implementation. IOW > it's up to us to > avoid such bugs.) > > If I understand correctly, you are suggesting that we > take a label from > the NASM source that stands right after an instruction > to patch, and we > declare it as a function in the C source. (With what > prototype though? > The label does not actually introduce a function > definition in the > assembly code; it would make no sense to call it.) > Then, for the > patching, I presume your suggestion is to convert the > address of the > function to UINTN, perform the subtraction, etc. > Something like: > > typedef VOID (X86_ASSEMBLY_LABEL) (VOID); > > (This is not a pointer-to-function type, but a function > type.) > > A declaration using the typedef would be > > extern X86_ASSEMBLY_LABEL gPatchCr3; > > (This declares an extern function, not a pointer to a > function.) > > The patching function would take a pointer to a > function: > > VOID > EFIAPI > PatchInstructionX86 ( > OUT X86_ASSEMBLY_LABEL *InstructionEnd, > IN UINT64 PatchValue, > IN UINTN ValueSize > ); > > and the implementation would have to do e.g. > > WriteUnaligned32 ( > (UINT32 *)(UINTN)InstructionEnd - 1, > (UINT32)PatchValue > ); > > It would be called like > > PatchInstructionX86 (&gPatchCr3, Value, 4); > > > But, what does this buy us in comparison to just: > > typedef UINT8 X86_ASSEMBLY_LABEL; > > ? > > If you worry that a future maintainer misunderstands > the UINT8, then we > can as well hide the UINT8 behind a typedef; > X86_ASSEMBLY_LABEL doesn't > have to be a function type for the hiding. (Conversely, > when using a > function type as underlying type, I worry that a future > maintainer might > be tempted to call them :) ) > > Thanks, > Laszlo > > >> -----Original Message----- > >> From: Laszlo Ersek [mailto:lersek@redhat.com] > >> Sent: Monday, February 5, 2018 2:28 AM > >> To: Kinney, Michael D <michael.d.kinney@intel.com>; > 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>; Ni, Ruiyu > <ruiyu.ni@intel.com> > >> Subject: Re: [PATCH 00/14] rid PiSmmCpuDxeSmm of DB- > >> encoded instructions > >> > >> On 02/03/18 01:45, Kinney, Michael D wrote: > >>> 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. > >> > >> Sure, I can drop that patch, but then the > PiSmmCpuDxeSmm > >> changes in the > >> other patches will divert the NASM files from the .S > and > >> .asm files. Is > >> that (temporary) non-uniformity better than removing > the > >> .S and .asm files? > >> > >>> 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. > >> > >> No, this is not correct; the storage that is > associated > >> with each of > >> these "patch labels" is the one byte (UINT8) > directly > >> following the > >> label -- whatever that byte might be. It is > generally > >> part of a totally > >> unrelated instruction. > >> > >> In case we had to patch an immediate operand that > >> happened to comprise > >> the very last byte(s) of a NASM source file, *then* > we'd > >> have to add one > >> dummy DB at the end, just so there was something > that the > >> label directly > >> refered to. > >> > >> This is why UINT8 is a good type here, because it > >> requires us to add the > >> least amount of padding. > >> > >>> 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? > >> > >> I don't think so. For building the addresses, we > rely on > >> the linker, and > >> the linker needs definitions (allocations) of > objects. > >> Your above > >> observation is correct (i.e. that storage is > required), > >> my addition to > >> that is that storage is *already* allocated (one > UINT8 > >> per patch label / > >> symbol). > >> > >> Thanks! > >> Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.