* [PATCH 0/3] UefiCpuPkg/PiSmmCpuDxeSmm: fix IA32 SmmStartup() regression on KVM
@ 2018-01-30 15:33 Laszlo Ersek
2018-01-30 15:33 ` [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup() Laszlo Ersek
` (5 more replies)
0 siblings, 6 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-01-30 15:33 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Eric Dong, Jian J Wang, Jiewen Yao, Paolo Bonzini, Ruiyu Ni
Repo: https://github.com/lersek/edk2.git
Branch: smm_startup_ia32_nocond
This small series fixes the IA32 SMM regression on KVM that I reported
recently. The first two patches are cleanups (no functional changes),
the third patch is the fix.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Thanks
Laszlo
Laszlo Ersek (3):
UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
UefiCpuPkg/PiSmmCpuDxeSmm: remove unneeded DBs from IA32 SmmStartup()
UefiCpuPkg/PiSmmCpuDxeSmm: eliminate conditional jump in IA32
SmmStartup()
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 28 +++++++++-----------
1 file changed, 13 insertions(+), 15 deletions(-)
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
2018-01-30 15:33 [PATCH 0/3] UefiCpuPkg/PiSmmCpuDxeSmm: fix IA32 SmmStartup() regression on KVM Laszlo Ersek
@ 2018-01-30 15:33 ` Laszlo Ersek
2018-01-30 17:22 ` Kinney, Michael D
2018-01-30 15:33 ` [PATCH 2/3] UefiCpuPkg/PiSmmCpuDxeSmm: remove unneeded DBs from " Laszlo Ersek
` (4 subsequent siblings)
5 siblings, 1 reply; 27+ messages in thread
From: Laszlo Ersek @ 2018-01-30 15:33 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Eric Dong, Jian J Wang, Jiewen Yao, Paolo Bonzini, Ruiyu Ni
The gSmmCr3, gSmmCr4, gSmmCr0 and gSmmJmpAddr global variables are used
for patching assembly instructions, thus we can never remove the DB
encodings for those instructions. At least we should add the intended
meanings in comments.
This patch only changes comments.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
index e96dd8d2392a..08534dba64b7 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
@@ -44,34 +44,34 @@ global ASM_PFX(SmmStartup)
ASM_PFX(SmmStartup):
DB 0x66
mov eax, 0x80000001 ; read capability
cpuid
DB 0x66
mov ebx, edx ; rdmsr will change edx. keep it in ebx.
- DB 0x66, 0xb8
+ DB 0x66, 0xb8 ; mov eax, imm32
ASM_PFX(gSmmCr3): DD 0
mov cr3, eax
DB 0x67, 0x66
lgdt [cs:ebp + (ASM_PFX(gcSmiInitGdtr) - ASM_PFX(SmmStartup))]
- DB 0x66, 0xb8
+ DB 0x66, 0xb8 ; mov eax, imm32
ASM_PFX(gSmmCr4): DD 0
mov cr4, eax
DB 0x66
mov ecx, 0xc0000080 ; IA32_EFER MSR
rdmsr
DB 0x66
test ebx, BIT20 ; check NXE capability
jz .1
or ah, BIT3 ; set NXE bit
wrmsr
.1:
- DB 0x66, 0xb8
+ DB 0x66, 0xb8 ; mov eax, imm32
ASM_PFX(gSmmCr0): DD 0
DB 0xbf, PROTECT_MODE_DS, 0 ; mov di, PROTECT_MODE_DS
mov cr0, eax
- DB 0x66, 0xea ; jmp far [ptr48]
+ DB 0x66, 0xea ; jmp far [ptr48]
ASM_PFX(gSmmJmpAddr):
DD @32bit
DW PROTECT_MODE_CS
@32bit:
mov ds, edi
mov es, edi
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 2/3] UefiCpuPkg/PiSmmCpuDxeSmm: remove unneeded DBs from IA32 SmmStartup()
2018-01-30 15:33 [PATCH 0/3] UefiCpuPkg/PiSmmCpuDxeSmm: fix IA32 SmmStartup() regression on KVM Laszlo Ersek
2018-01-30 15:33 ` [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup() Laszlo Ersek
@ 2018-01-30 15:33 ` Laszlo Ersek
2018-01-31 5:45 ` Ni, Ruiyu
2018-01-30 15:33 ` [PATCH 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: eliminate conditional jump in " Laszlo Ersek
` (3 subsequent siblings)
5 siblings, 1 reply; 27+ messages in thread
From: Laszlo Ersek @ 2018-01-30 15:33 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Eric Dong, Jian J Wang, Jiewen Yao, Paolo Bonzini, Ruiyu Ni
The SmmStartup() executes in SMM, which is very similar to real mode. Add
"BITS 16" before it and "BITS 32" after it (just before the @32bit label).
Remove the manual 0x66 operand-size override prefixes, for selecting
32-bit operands -- the sizes of our operands trigger NASM to insert the
prefixes automatically in almost every spot. The one place where we have
to add it back manually is the LGDT instruction. (The 0x67 address-size
override prefix is also auto-generated.)
This patch causes NASM to generate byte-identical object code (determined
by disassembling both the pre-patch and post-patch versions, and comparing
the listings), except:
> @@ -158,7 +158,7 @@
> 00000142 6689D3 mov ebx,edx
> 00000145 66B800000000 mov eax,0x0
> 0000014B 0F22D8 mov cr3,eax
> -0000014E 67662E0F0155F6 o32 lgdt [cs:ebp-0xa]
> +0000014E 2E66670F0155F6 o32 lgdt [cs:ebp-0xa]
> 00000155 66B800000000 mov eax,0x0
> 0000015B 0F22E0 mov cr4,eax
> 0000015E 66B9800000C0 mov ecx,0xc0000080
The only difference is the prefix list order, it changes from:
- 0x67, 0x66, 0x2E
to
- 0x2E, 0x66, 0x67
(0x2E is "CS segment override").
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=866
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
index 08534dba64b7..9231aa5b3ded 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
@@ -38,43 +38,42 @@ global ASM_PFX(gcSmmInitTemplate)
ASM_PFX(gcSmiInitGdtr):
DW 0
DQ 0
global ASM_PFX(SmmStartup)
+
+BITS 16
ASM_PFX(SmmStartup):
- DB 0x66
mov eax, 0x80000001 ; read capability
cpuid
- DB 0x66
mov ebx, edx ; rdmsr will change edx. keep it in ebx.
DB 0x66, 0xb8 ; mov eax, imm32
ASM_PFX(gSmmCr3): DD 0
mov cr3, eax
- DB 0x67, 0x66
- lgdt [cs:ebp + (ASM_PFX(gcSmiInitGdtr) - ASM_PFX(SmmStartup))]
+o32 lgdt [cs:ebp + (ASM_PFX(gcSmiInitGdtr) - ASM_PFX(SmmStartup))]
DB 0x66, 0xb8 ; mov eax, imm32
ASM_PFX(gSmmCr4): DD 0
mov cr4, eax
- DB 0x66
mov ecx, 0xc0000080 ; IA32_EFER MSR
rdmsr
- DB 0x66
test ebx, BIT20 ; check NXE capability
jz .1
or ah, BIT3 ; set NXE bit
wrmsr
.1:
DB 0x66, 0xb8 ; mov eax, imm32
ASM_PFX(gSmmCr0): DD 0
- DB 0xbf, PROTECT_MODE_DS, 0 ; mov di, PROTECT_MODE_DS
+ mov di, PROTECT_MODE_DS
mov cr0, eax
DB 0x66, 0xea ; jmp far [ptr48]
ASM_PFX(gSmmJmpAddr):
DD @32bit
DW PROTECT_MODE_CS
+
+BITS 32
@32bit:
mov ds, edi
mov es, edi
mov fs, edi
mov gs, edi
mov ss, edi
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: eliminate conditional jump in IA32 SmmStartup()
2018-01-30 15:33 [PATCH 0/3] UefiCpuPkg/PiSmmCpuDxeSmm: fix IA32 SmmStartup() regression on KVM Laszlo Ersek
2018-01-30 15:33 ` [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup() Laszlo Ersek
2018-01-30 15:33 ` [PATCH 2/3] UefiCpuPkg/PiSmmCpuDxeSmm: remove unneeded DBs from " Laszlo Ersek
@ 2018-01-30 15:33 ` Laszlo Ersek
2018-01-31 5:12 ` Ni, Ruiyu
2018-01-30 16:37 ` [PATCH 0/3] UefiCpuPkg/PiSmmCpuDxeSmm: fix IA32 SmmStartup() regression on KVM Paolo Bonzini
` (2 subsequent siblings)
5 siblings, 1 reply; 27+ messages in thread
From: Laszlo Ersek @ 2018-01-30 15:33 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Eric Dong, Jian J Wang, Jiewen Yao, Paolo Bonzini, Ruiyu Ni
SMM emulation under KVM crashes the guest when the "jz" branch, added in
commit d4d87596c11d ("UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if it's
supported", 2018-01-18), is taken.
Rework the propagation of CPUID.80000001H:EDX.NX [bit 20] to IA32_EFER.NXE
[bit 11] so that no code is executed conditionally.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Ref: http://mid.mail-archive.com/d6fff558-6c4f-9ca6-74a7-e7cd9d007276@redhat.com
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
index 9231aa5b3ded..102e0bdbabc8 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
@@ -44,26 +44,25 @@ global ASM_PFX(SmmStartup)
BITS 16
ASM_PFX(SmmStartup):
mov eax, 0x80000001 ; read capability
cpuid
mov ebx, edx ; rdmsr will change edx. keep it in ebx.
+ and ebx, BIT20 ; extract XD capability bit
+ shr ebx, 9 ; shift bit to IA32_EFER NXE position
DB 0x66, 0xb8 ; mov eax, imm32
ASM_PFX(gSmmCr3): DD 0
mov cr3, eax
o32 lgdt [cs:ebp + (ASM_PFX(gcSmiInitGdtr) - ASM_PFX(SmmStartup))]
DB 0x66, 0xb8 ; mov eax, imm32
ASM_PFX(gSmmCr4): DD 0
mov cr4, eax
mov ecx, 0xc0000080 ; IA32_EFER MSR
rdmsr
- test ebx, BIT20 ; check NXE capability
- jz .1
- or ah, BIT3 ; set NXE bit
+ or eax, ebx ; set NXE bit if XD is available
wrmsr
-.1:
DB 0x66, 0xb8 ; mov eax, imm32
ASM_PFX(gSmmCr0): DD 0
mov di, PROTECT_MODE_DS
mov cr0, eax
DB 0x66, 0xea ; jmp far [ptr48]
ASM_PFX(gSmmJmpAddr):
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 0/3] UefiCpuPkg/PiSmmCpuDxeSmm: fix IA32 SmmStartup() regression on KVM
2018-01-30 15:33 [PATCH 0/3] UefiCpuPkg/PiSmmCpuDxeSmm: fix IA32 SmmStartup() regression on KVM Laszlo Ersek
` (2 preceding siblings ...)
2018-01-30 15:33 ` [PATCH 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: eliminate conditional jump in " Laszlo Ersek
@ 2018-01-30 16:37 ` Paolo Bonzini
2018-01-31 12:17 ` Laszlo Ersek
2018-02-01 1:20 ` Wang, Jian J
5 siblings, 0 replies; 27+ messages in thread
From: Paolo Bonzini @ 2018-01-30 16:37 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01; +Cc: Eric Dong, Jian J Wang, Jiewen Yao, Ruiyu Ni
On 30/01/2018 10:33, Laszlo Ersek wrote:
> Repo: https://github.com/lersek/edk2.git
> Branch: smm_startup_ia32_nocond
>
> This small series fixes the IA32 SMM regression on KVM that I reported
> recently. The first two patches are cleanups (no functional changes),
> the third patch is the fix.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Paolo
> Thanks
> Laszlo
>
> Laszlo Ersek (3):
> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
> UefiCpuPkg/PiSmmCpuDxeSmm: remove unneeded DBs from IA32 SmmStartup()
> UefiCpuPkg/PiSmmCpuDxeSmm: eliminate conditional jump in IA32
> SmmStartup()
>
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 28 +++++++++-----------
> 1 file changed, 13 insertions(+), 15 deletions(-)
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
2018-01-30 15:33 ` [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup() Laszlo Ersek
@ 2018-01-30 17:22 ` Kinney, Michael D
2018-01-30 18:17 ` Laszlo Ersek
0 siblings, 1 reply; 27+ messages in thread
From: Kinney, Michael D @ 2018-01-30 17:22 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01, Kinney, Michael D
Cc: Ni, Ruiyu, Yao, Jiewen, Dong, Eric, Paolo Bonzini
Laszlo,
The DBs can be removed if the label is moved after
the instruction and the patch is done to the label
minus the size of the patch value.
Mike
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org]
> On Behalf Of Laszlo Ersek
> Sent: Tuesday, January 30, 2018 7:34 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Paolo Bonzini <pbonzini@redhat.com>
> Subject: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm:
> update comments in IA32 SmmStartup()
>
> The gSmmCr3, gSmmCr4, gSmmCr0 and gSmmJmpAddr global
> variables are used
> for patching assembly instructions, thus we can never
> remove the DB
> encodings for those instructions. At least we should add
> the intended
> meanings in comments.
>
> This patch only changes comments.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> index e96dd8d2392a..08534dba64b7 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> @@ -44,34 +44,34 @@ global ASM_PFX(SmmStartup)
> ASM_PFX(SmmStartup):
> DB 0x66
> mov eax, 0x80000001 ; read
> capability
> cpuid
> DB 0x66
> mov ebx, edx ; rdmsr will
> change edx. keep it in ebx.
> - DB 0x66, 0xb8
> + DB 0x66, 0xb8 ; mov eax, imm32
> ASM_PFX(gSmmCr3): DD 0
> mov cr3, eax
> DB 0x67, 0x66
> lgdt [cs:ebp + (ASM_PFX(gcSmiInitGdtr) -
> ASM_PFX(SmmStartup))]
> - DB 0x66, 0xb8
> + DB 0x66, 0xb8 ; mov eax, imm32
> ASM_PFX(gSmmCr4): DD 0
> mov cr4, eax
> DB 0x66
> mov ecx, 0xc0000080 ; IA32_EFER MSR
> rdmsr
> DB 0x66
> test ebx, BIT20 ; check NXE
> capability
> jz .1
> or ah, BIT3 ; set NXE bit
> wrmsr
> .1:
> - DB 0x66, 0xb8
> + DB 0x66, 0xb8 ; mov eax, imm32
> ASM_PFX(gSmmCr0): DD 0
> DB 0xbf, PROTECT_MODE_DS, 0 ; mov di,
> PROTECT_MODE_DS
> mov cr0, eax
> - DB 0x66, 0xea ; jmp far
> [ptr48]
> + DB 0x66, 0xea ; jmp far
> [ptr48]
> ASM_PFX(gSmmJmpAddr):
> DD @32bit
> DW PROTECT_MODE_CS
> @32bit:
> mov ds, edi
> mov es, edi
> --
> 2.14.1.3.gb7cf6e02401b
>
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
2018-01-30 17:22 ` Kinney, Michael D
@ 2018-01-30 18:17 ` Laszlo Ersek
2018-01-30 20:31 ` Kinney, Michael D
0 siblings, 1 reply; 27+ messages in thread
From: Laszlo Ersek @ 2018-01-30 18:17 UTC (permalink / raw)
To: Kinney, Michael D, edk2-devel-01
Cc: Ni, Ruiyu, Yao, Jiewen, Dong, Eric, Paolo Bonzini
On 01/30/18 18:22, Kinney, Michael D wrote:
> Laszlo,
>
> The DBs can be removed if the label is moved after
> the instruction and the patch is done to the label
> minus the size of the patch value.
Indeed I haven't thought of this.
If I understand correctly, it means
extern UINT8 gSmmCr0;
*(UINT32*)(&gSmmCr0 - sizeof (UINT32)) = (UINT32)AsmReadCr0 ();
TBH, the DB feels less ugly to me than this :)
Still, if you think it would be an acceptable price to pay for removing
the remaining DBs, I can respin.
Thanks
Laszlo
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org]
>> On Behalf Of Laszlo Ersek
>> Sent: Tuesday, January 30, 2018 7:34 AM
>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen
>> <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>;
>> Paolo Bonzini <pbonzini@redhat.com>
>> Subject: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm:
>> update comments in IA32 SmmStartup()
>>
>> The gSmmCr3, gSmmCr4, gSmmCr0 and gSmmJmpAddr global
>> variables are used
>> for patching assembly instructions, thus we can never
>> remove the DB
>> encodings for those instructions. At least we should add
>> the intended
>> meanings in comments.
>>
>> This patch only changes comments.
>>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>> index e96dd8d2392a..08534dba64b7 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>> @@ -44,34 +44,34 @@ global ASM_PFX(SmmStartup)
>> ASM_PFX(SmmStartup):
>> DB 0x66
>> mov eax, 0x80000001 ; read
>> capability
>> cpuid
>> DB 0x66
>> mov ebx, edx ; rdmsr will
>> change edx. keep it in ebx.
>> - DB 0x66, 0xb8
>> + DB 0x66, 0xb8 ; mov eax, imm32
>> ASM_PFX(gSmmCr3): DD 0
>> mov cr3, eax
>> DB 0x67, 0x66
>> lgdt [cs:ebp + (ASM_PFX(gcSmiInitGdtr) -
>> ASM_PFX(SmmStartup))]
>> - DB 0x66, 0xb8
>> + DB 0x66, 0xb8 ; mov eax, imm32
>> ASM_PFX(gSmmCr4): DD 0
>> mov cr4, eax
>> DB 0x66
>> mov ecx, 0xc0000080 ; IA32_EFER MSR
>> rdmsr
>> DB 0x66
>> test ebx, BIT20 ; check NXE
>> capability
>> jz .1
>> or ah, BIT3 ; set NXE bit
>> wrmsr
>> .1:
>> - DB 0x66, 0xb8
>> + DB 0x66, 0xb8 ; mov eax, imm32
>> ASM_PFX(gSmmCr0): DD 0
>> DB 0xbf, PROTECT_MODE_DS, 0 ; mov di,
>> PROTECT_MODE_DS
>> mov cr0, eax
>> - DB 0x66, 0xea ; jmp far
>> [ptr48]
>> + DB 0x66, 0xea ; jmp far
>> [ptr48]
>> ASM_PFX(gSmmJmpAddr):
>> DD @32bit
>> DW PROTECT_MODE_CS
>> @32bit:
>> mov ds, edi
>> mov es, edi
>> --
>> 2.14.1.3.gb7cf6e02401b
>>
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
2018-01-30 18:17 ` Laszlo Ersek
@ 2018-01-30 20:31 ` Kinney, Michael D
2018-01-30 21:26 ` Kinney, Michael D
2018-01-30 21:45 ` Laszlo Ersek
0 siblings, 2 replies; 27+ messages in thread
From: Kinney, Michael D @ 2018-01-30 20:31 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01, Kinney, Michael D
Cc: Ni, Ruiyu, Paolo Bonzini, Yao, Jiewen, Dong, Eric
Laszlo,
We have already used this technique in other NASM files
to remove DBs.
Let us know if you have suggestions on how to make the
C code that performs the patches easier to read and
maintain.
Mike
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org]
> On Behalf Of Laszlo Ersek
> Sent: Tuesday, January 30, 2018 10:17 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-
> devel-01 <edk2-devel@lists.01.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini
> <pbonzini@redhat.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2] [PATCH 1/3]
> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
> SmmStartup()
>
> On 01/30/18 18:22, Kinney, Michael D wrote:
> > Laszlo,
> >
> > The DBs can be removed if the label is moved after
> > the instruction and the patch is done to the label
> > minus the size of the patch value.
>
> Indeed I haven't thought of this.
>
> If I understand correctly, it means
>
> extern UINT8 gSmmCr0;
>
> *(UINT32*)(&gSmmCr0 - sizeof (UINT32)) =
> (UINT32)AsmReadCr0 ();
>
> TBH, the DB feels less ugly to me than this :)
>
> Still, if you think it would be an acceptable price to
> pay for removing
> the remaining DBs, I can respin.
>
> Thanks
> Laszlo
>
> >> -----Original Message-----
> >> From: edk2-devel [mailto:edk2-devel-
> bounces@lists.01.org]
> >> On Behalf Of Laszlo Ersek
> >> Sent: Tuesday, January 30, 2018 7:34 AM
> >> To: edk2-devel-01 <edk2-devel@lists.01.org>
> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen
> >> <jiewen.yao@intel.com>; Dong, Eric
> <eric.dong@intel.com>;
> >> Paolo Bonzini <pbonzini@redhat.com>
> >> Subject: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm:
> >> update comments in IA32 SmmStartup()
> >>
> >> The gSmmCr3, gSmmCr4, gSmmCr0 and gSmmJmpAddr global
> >> variables are used
> >> for patching assembly instructions, thus we can never
> >> remove the DB
> >> encodings for those instructions. At least we should
> add
> >> the intended
> >> meanings in comments.
> >>
> >> This patch only changes comments.
> >>
> >> Cc: Eric Dong <eric.dong@intel.com>
> >> Cc: Jian J Wang <jian.j.wang@intel.com>
> >> Cc: Jiewen Yao <jiewen.yao@intel.com>
> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> >> Contributed-under: TianoCore Contribution Agreement
> 1.1
> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> >> ---
> >> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 8 ++++-
> ---
> >> 1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git
> a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> >> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> >> index e96dd8d2392a..08534dba64b7 100644
> >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> >> @@ -44,34 +44,34 @@ global ASM_PFX(SmmStartup)
> >> ASM_PFX(SmmStartup):
> >> DB 0x66
> >> mov eax, 0x80000001 ; read
> >> capability
> >> cpuid
> >> DB 0x66
> >> mov ebx, edx ; rdmsr will
> >> change edx. keep it in ebx.
> >> - DB 0x66, 0xb8
> >> + DB 0x66, 0xb8 ; mov eax,
> imm32
> >> ASM_PFX(gSmmCr3): DD 0
> >> mov cr3, eax
> >> DB 0x67, 0x66
> >> lgdt [cs:ebp + (ASM_PFX(gcSmiInitGdtr) -
> >> ASM_PFX(SmmStartup))]
> >> - DB 0x66, 0xb8
> >> + DB 0x66, 0xb8 ; mov eax,
> imm32
> >> ASM_PFX(gSmmCr4): DD 0
> >> mov cr4, eax
> >> DB 0x66
> >> mov ecx, 0xc0000080 ; IA32_EFER
> MSR
> >> rdmsr
> >> DB 0x66
> >> test ebx, BIT20 ; check NXE
> >> capability
> >> jz .1
> >> or ah, BIT3 ; set NXE bit
> >> wrmsr
> >> .1:
> >> - DB 0x66, 0xb8
> >> + DB 0x66, 0xb8 ; mov eax,
> imm32
> >> ASM_PFX(gSmmCr0): DD 0
> >> DB 0xbf, PROTECT_MODE_DS, 0 ; mov di,
> >> PROTECT_MODE_DS
> >> mov cr0, eax
> >> - DB 0x66, 0xea ; jmp far
> >> [ptr48]
> >> + DB 0x66, 0xea ; jmp far
> >> [ptr48]
> >> ASM_PFX(gSmmJmpAddr):
> >> DD @32bit
> >> DW PROTECT_MODE_CS
> >> @32bit:
> >> mov ds, edi
> >> mov es, edi
> >> --
> >> 2.14.1.3.gb7cf6e02401b
> >>
> >>
> >> _______________________________________________
> >> edk2-devel mailing list
> >> edk2-devel@lists.01.org
> >> https://lists.01.org/mailman/listinfo/edk2-devel
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
2018-01-30 20:31 ` Kinney, Michael D
@ 2018-01-30 21:26 ` Kinney, Michael D
2018-01-30 21:55 ` Laszlo Ersek
2018-01-30 21:45 ` Laszlo Ersek
1 sibling, 1 reply; 27+ messages in thread
From: Kinney, Michael D @ 2018-01-30 21:26 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01, Kinney, Michael D
Cc: Ni, Ruiyu, Paolo Bonzini, Yao, Jiewen, Dong, Eric
Laszlo,
Maybe we can add a macro to help:
#define PATCH_X86_ASM(Label,Type,Value) *((Type *)(&Label) - 1) = (Type)(Value)
PATCH_X86_ASM (gSmmCr0, UINT32, AsmReadCr0());
Mike
> -----Original Message-----
> From: Kinney, Michael D
> Sent: Tuesday, January 30, 2018 12:31 PM
> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel-01
> <edk2-devel@lists.01.org>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini
> <pbonzini@redhat.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: RE: [edk2] [PATCH 1/3]
> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
> SmmStartup()
>
> Laszlo,
>
> We have already used this technique in other NASM files
> to remove DBs.
>
> Let us know if you have suggestions on how to make the
> C code that performs the patches easier to read and
> maintain.
>
> Mike
>
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-
> bounces@lists.01.org]
> > On Behalf Of Laszlo Ersek
> > Sent: Tuesday, January 30, 2018 10:17 AM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> edk2-
> > devel-01 <edk2-devel@lists.01.org>
> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini
> > <pbonzini@redhat.com>; Yao, Jiewen
> > <jiewen.yao@intel.com>; Dong, Eric
> <eric.dong@intel.com>
> > Subject: Re: [edk2] [PATCH 1/3]
> > UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
> > SmmStartup()
> >
> > On 01/30/18 18:22, Kinney, Michael D wrote:
> > > Laszlo,
> > >
> > > The DBs can be removed if the label is moved after
> > > the instruction and the patch is done to the label
> > > minus the size of the patch value.
> >
> > Indeed I haven't thought of this.
> >
> > If I understand correctly, it means
> >
> > extern UINT8 gSmmCr0;
> >
> > *(UINT32*)(&gSmmCr0 - sizeof (UINT32)) =
> > (UINT32)AsmReadCr0 ();
> >
> > TBH, the DB feels less ugly to me than this :)
> >
> > Still, if you think it would be an acceptable price to
> > pay for removing
> > the remaining DBs, I can respin.
> >
> > Thanks
> > Laszlo
> >
> > >> -----Original Message-----
> > >> From: edk2-devel [mailto:edk2-devel-
> > bounces@lists.01.org]
> > >> On Behalf Of Laszlo Ersek
> > >> Sent: Tuesday, January 30, 2018 7:34 AM
> > >> To: edk2-devel-01 <edk2-devel@lists.01.org>
> > >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen
> > >> <jiewen.yao@intel.com>; Dong, Eric
> > <eric.dong@intel.com>;
> > >> Paolo Bonzini <pbonzini@redhat.com>
> > >> Subject: [edk2] [PATCH 1/3]
> UefiCpuPkg/PiSmmCpuDxeSmm:
> > >> update comments in IA32 SmmStartup()
> > >>
> > >> The gSmmCr3, gSmmCr4, gSmmCr0 and gSmmJmpAddr global
> > >> variables are used
> > >> for patching assembly instructions, thus we can
> never
> > >> remove the DB
> > >> encodings for those instructions. At least we should
> > add
> > >> the intended
> > >> meanings in comments.
> > >>
> > >> This patch only changes comments.
> > >>
> > >> Cc: Eric Dong <eric.dong@intel.com>
> > >> Cc: Jian J Wang <jian.j.wang@intel.com>
> > >> Cc: Jiewen Yao <jiewen.yao@intel.com>
> > >> Cc: Paolo Bonzini <pbonzini@redhat.com>
> > >> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > >> Contributed-under: TianoCore Contribution Agreement
> > 1.1
> > >> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> > >> ---
> > >> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 8
> ++++-
> > ---
> > >> 1 file changed, 4 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git
> > a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> > >> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> > >> index e96dd8d2392a..08534dba64b7 100644
> > >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> > >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> > >> @@ -44,34 +44,34 @@ global ASM_PFX(SmmStartup)
> > >> ASM_PFX(SmmStartup):
> > >> DB 0x66
> > >> mov eax, 0x80000001 ; read
> > >> capability
> > >> cpuid
> > >> DB 0x66
> > >> mov ebx, edx ; rdmsr
> will
> > >> change edx. keep it in ebx.
> > >> - DB 0x66, 0xb8
> > >> + DB 0x66, 0xb8 ; mov eax,
> > imm32
> > >> ASM_PFX(gSmmCr3): DD 0
> > >> mov cr3, eax
> > >> DB 0x67, 0x66
> > >> lgdt [cs:ebp + (ASM_PFX(gcSmiInitGdtr) -
> > >> ASM_PFX(SmmStartup))]
> > >> - DB 0x66, 0xb8
> > >> + DB 0x66, 0xb8 ; mov eax,
> > imm32
> > >> ASM_PFX(gSmmCr4): DD 0
> > >> mov cr4, eax
> > >> DB 0x66
> > >> mov ecx, 0xc0000080 ; IA32_EFER
> > MSR
> > >> rdmsr
> > >> DB 0x66
> > >> test ebx, BIT20 ; check NXE
> > >> capability
> > >> jz .1
> > >> or ah, BIT3 ; set NXE
> bit
> > >> wrmsr
> > >> .1:
> > >> - DB 0x66, 0xb8
> > >> + DB 0x66, 0xb8 ; mov eax,
> > imm32
> > >> ASM_PFX(gSmmCr0): DD 0
> > >> DB 0xbf, PROTECT_MODE_DS, 0 ; mov di,
> > >> PROTECT_MODE_DS
> > >> mov cr0, eax
> > >> - DB 0x66, 0xea ; jmp far
> > >> [ptr48]
> > >> + DB 0x66, 0xea ; jmp far
> > >> [ptr48]
> > >> ASM_PFX(gSmmJmpAddr):
> > >> DD @32bit
> > >> DW PROTECT_MODE_CS
> > >> @32bit:
> > >> mov ds, edi
> > >> mov es, edi
> > >> --
> > >> 2.14.1.3.gb7cf6e02401b
> > >>
> > >>
> > >> _______________________________________________
> > >> edk2-devel mailing list
> > >> edk2-devel@lists.01.org
> > >> https://lists.01.org/mailman/listinfo/edk2-devel
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
2018-01-30 20:31 ` Kinney, Michael D
2018-01-30 21:26 ` Kinney, Michael D
@ 2018-01-30 21:45 ` Laszlo Ersek
2018-01-30 22:25 ` Kinney, Michael D
1 sibling, 1 reply; 27+ messages in thread
From: Laszlo Ersek @ 2018-01-30 21:45 UTC (permalink / raw)
To: Kinney, Michael D, edk2-devel-01
Cc: Ni, Ruiyu, Paolo Bonzini, Yao, Jiewen, Dong, Eric
On 01/30/18 21:31, Kinney, Michael D wrote:
> Laszlo,
>
> We have already used this technique in other NASM files
> to remove DBs.
OK.
> Let us know if you have suggestions on how to make the
> C code that performs the patches easier to read and
> maintain.
How about this:
VOID
PatchAssembly (
VOID *BufferEnd,
UINT64 PatchValue,
UINTN ValueSize
)
{
CopyMem (
(VOID *)((UINTN)BufferEnd - ValueSize),
&PatchValue,
ValueSize
);
}
extern UINT8 gAsmSmmCr0;
extern UINT8 gAsmSmmCr3;
extern UINT8 gAsmSmmCr4;
...
{
PatchAssembly (&gAsmSmmCr0, AsmReadCr0 (), 4);
PatchAssembly (&gAsmSmmCr3, AsmReadCr3 (), 4);
PatchAssembly (&gAsmSmmCr4, AsmReadCr4 (), 4);
...
}
(I think it's fine to open-code the last argument as "4", rather than
"sizeof (UINT32)", because for patching, we must have intimate knowledge
of the instruction anyway.)
To me, this is easier to read, because:
- there are no complex casts in the "business logic"
- the size is spelled out once per patching
- the function name and the variable names make it clear we are patching
separately compiled assembly code that was linked into the same
module.
What do you think?
Thanks!
Laszlo
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org]
>> On Behalf Of Laszlo Ersek
>> Sent: Tuesday, January 30, 2018 10:17 AM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-
>> devel-01 <edk2-devel@lists.01.org>
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini
>> <pbonzini@redhat.com>; Yao, Jiewen
>> <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
>> Subject: Re: [edk2] [PATCH 1/3]
>> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
>> SmmStartup()
>>
>> On 01/30/18 18:22, Kinney, Michael D wrote:
>>> Laszlo,
>>>
>>> The DBs can be removed if the label is moved after
>>> the instruction and the patch is done to the label
>>> minus the size of the patch value.
>>
>> Indeed I haven't thought of this.
>>
>> If I understand correctly, it means
>>
>> extern UINT8 gSmmCr0;
>>
>> *(UINT32*)(&gSmmCr0 - sizeof (UINT32)) =
>> (UINT32)AsmReadCr0 ();
>>
>> TBH, the DB feels less ugly to me than this :)
>>
>> Still, if you think it would be an acceptable price to
>> pay for removing
>> the remaining DBs, I can respin.
>>
>> Thanks
>> Laszlo
>>
>>>> -----Original Message-----
>>>> From: edk2-devel [mailto:edk2-devel-
>> bounces@lists.01.org]
>>>> On Behalf Of Laszlo Ersek
>>>> Sent: Tuesday, January 30, 2018 7:34 AM
>>>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen
>>>> <jiewen.yao@intel.com>; Dong, Eric
>> <eric.dong@intel.com>;
>>>> Paolo Bonzini <pbonzini@redhat.com>
>>>> Subject: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm:
>>>> update comments in IA32 SmmStartup()
>>>>
>>>> The gSmmCr3, gSmmCr4, gSmmCr0 and gSmmJmpAddr global
>>>> variables are used
>>>> for patching assembly instructions, thus we can never
>>>> remove the DB
>>>> encodings for those instructions. At least we should
>> add
>>>> the intended
>>>> meanings in comments.
>>>>
>>>> This patch only changes comments.
>>>>
>>>> Cc: Eric Dong <eric.dong@intel.com>
>>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>>> Contributed-under: TianoCore Contribution Agreement
>> 1.1
>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>> ---
>>>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 8 ++++-
>> ---
>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git
>> a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>> index e96dd8d2392a..08534dba64b7 100644
>>>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>> @@ -44,34 +44,34 @@ global ASM_PFX(SmmStartup)
>>>> ASM_PFX(SmmStartup):
>>>> DB 0x66
>>>> mov eax, 0x80000001 ; read
>>>> capability
>>>> cpuid
>>>> DB 0x66
>>>> mov ebx, edx ; rdmsr will
>>>> change edx. keep it in ebx.
>>>> - DB 0x66, 0xb8
>>>> + DB 0x66, 0xb8 ; mov eax,
>> imm32
>>>> ASM_PFX(gSmmCr3): DD 0
>>>> mov cr3, eax
>>>> DB 0x67, 0x66
>>>> lgdt [cs:ebp + (ASM_PFX(gcSmiInitGdtr) -
>>>> ASM_PFX(SmmStartup))]
>>>> - DB 0x66, 0xb8
>>>> + DB 0x66, 0xb8 ; mov eax,
>> imm32
>>>> ASM_PFX(gSmmCr4): DD 0
>>>> mov cr4, eax
>>>> DB 0x66
>>>> mov ecx, 0xc0000080 ; IA32_EFER
>> MSR
>>>> rdmsr
>>>> DB 0x66
>>>> test ebx, BIT20 ; check NXE
>>>> capability
>>>> jz .1
>>>> or ah, BIT3 ; set NXE bit
>>>> wrmsr
>>>> .1:
>>>> - DB 0x66, 0xb8
>>>> + DB 0x66, 0xb8 ; mov eax,
>> imm32
>>>> ASM_PFX(gSmmCr0): DD 0
>>>> DB 0xbf, PROTECT_MODE_DS, 0 ; mov di,
>>>> PROTECT_MODE_DS
>>>> mov cr0, eax
>>>> - DB 0x66, 0xea ; jmp far
>>>> [ptr48]
>>>> + DB 0x66, 0xea ; jmp far
>>>> [ptr48]
>>>> ASM_PFX(gSmmJmpAddr):
>>>> DD @32bit
>>>> DW PROTECT_MODE_CS
>>>> @32bit:
>>>> mov ds, edi
>>>> mov es, edi
>>>> --
>>>> 2.14.1.3.gb7cf6e02401b
>>>>
>>>>
>>>> _______________________________________________
>>>> edk2-devel mailing list
>>>> edk2-devel@lists.01.org
>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
2018-01-30 21:26 ` Kinney, Michael D
@ 2018-01-30 21:55 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-01-30 21:55 UTC (permalink / raw)
To: Kinney, Michael D, edk2-devel-01
Cc: Ni, Ruiyu, Paolo Bonzini, Yao, Jiewen, Dong, Eric
On 01/30/18 22:26, Kinney, Michael D wrote:
> Laszlo,
>
> Maybe we can add a macro to help:
>
> #define PATCH_X86_ASM(Label,Type,Value) *((Type *)(&Label) - 1) = (Type)(Value)
>
> PATCH_X86_ASM (gSmmCr0, UINT32, AsmReadCr0());
Before sending my previous email, I thought of something like this. Here
I slightly dislike:
- the unaligned access to a wider-than-byte object
- the larger potential for triggering undefined behavior in the C
language implementation (due to type punning) -- doing the arithmetic
in UINTN and using CopyMem for the data movement should mitigate that
- the requirement that the object to patch has to be followed by the
same number of bytes (otherwise linking might fail, I think). For
example if we'd like to patch 4 bytes at the very end of the assembly
code, e.g. in a jmp instruction, we'd have to match that with a DD,
just so the extern UINT32 global variable could be declared and
linked. I think requiring just one byte trailing padding and making
all such markers UINT8 is easier to handle
- the requirement that the declared type of "gSmmCr0" match the type
passed to the macro as 2nd argument
That said, I'm fine using either of PATCH_X86_ASM() and PatchAssembly().
Thanks!
Laszlo
>
> Mike
>
>> -----Original Message-----
>> From: Kinney, Michael D
>> Sent: Tuesday, January 30, 2018 12:31 PM
>> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel-01
>> <edk2-devel@lists.01.org>; Kinney, Michael D
>> <michael.d.kinney@intel.com>
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini
>> <pbonzini@redhat.com>; Yao, Jiewen
>> <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
>> Subject: RE: [edk2] [PATCH 1/3]
>> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
>> SmmStartup()
>>
>> Laszlo,
>>
>> We have already used this technique in other NASM files
>> to remove DBs.
>>
>> Let us know if you have suggestions on how to make the
>> C code that performs the patches easier to read and
>> maintain.
>>
>> Mike
>>
>>> -----Original Message-----
>>> From: edk2-devel [mailto:edk2-devel-
>> bounces@lists.01.org]
>>> On Behalf Of Laszlo Ersek
>>> Sent: Tuesday, January 30, 2018 10:17 AM
>>> To: Kinney, Michael D <michael.d.kinney@intel.com>;
>> edk2-
>>> devel-01 <edk2-devel@lists.01.org>
>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini
>>> <pbonzini@redhat.com>; Yao, Jiewen
>>> <jiewen.yao@intel.com>; Dong, Eric
>> <eric.dong@intel.com>
>>> Subject: Re: [edk2] [PATCH 1/3]
>>> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
>>> SmmStartup()
>>>
>>> On 01/30/18 18:22, Kinney, Michael D wrote:
>>>> Laszlo,
>>>>
>>>> The DBs can be removed if the label is moved after
>>>> the instruction and the patch is done to the label
>>>> minus the size of the patch value.
>>>
>>> Indeed I haven't thought of this.
>>>
>>> If I understand correctly, it means
>>>
>>> extern UINT8 gSmmCr0;
>>>
>>> *(UINT32*)(&gSmmCr0 - sizeof (UINT32)) =
>>> (UINT32)AsmReadCr0 ();
>>>
>>> TBH, the DB feels less ugly to me than this :)
>>>
>>> Still, if you think it would be an acceptable price to
>>> pay for removing
>>> the remaining DBs, I can respin.
>>>
>>> Thanks
>>> Laszlo
>>>
>>>>> -----Original Message-----
>>>>> From: edk2-devel [mailto:edk2-devel-
>>> bounces@lists.01.org]
>>>>> On Behalf Of Laszlo Ersek
>>>>> Sent: Tuesday, January 30, 2018 7:34 AM
>>>>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen
>>>>> <jiewen.yao@intel.com>; Dong, Eric
>>> <eric.dong@intel.com>;
>>>>> Paolo Bonzini <pbonzini@redhat.com>
>>>>> Subject: [edk2] [PATCH 1/3]
>> UefiCpuPkg/PiSmmCpuDxeSmm:
>>>>> update comments in IA32 SmmStartup()
>>>>>
>>>>> The gSmmCr3, gSmmCr4, gSmmCr0 and gSmmJmpAddr global
>>>>> variables are used
>>>>> for patching assembly instructions, thus we can
>> never
>>>>> remove the DB
>>>>> encodings for those instructions. At least we should
>>> add
>>>>> the intended
>>>>> meanings in comments.
>>>>>
>>>>> This patch only changes comments.
>>>>>
>>>>> Cc: Eric Dong <eric.dong@intel.com>
>>>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>>>> Contributed-under: TianoCore Contribution Agreement
>>> 1.1
>>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>>> ---
>>>>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 8
>> ++++-
>>> ---
>>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git
>>> a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>>> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>>> index e96dd8d2392a..08534dba64b7 100644
>>>>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>>> @@ -44,34 +44,34 @@ global ASM_PFX(SmmStartup)
>>>>> ASM_PFX(SmmStartup):
>>>>> DB 0x66
>>>>> mov eax, 0x80000001 ; read
>>>>> capability
>>>>> cpuid
>>>>> DB 0x66
>>>>> mov ebx, edx ; rdmsr
>> will
>>>>> change edx. keep it in ebx.
>>>>> - DB 0x66, 0xb8
>>>>> + DB 0x66, 0xb8 ; mov eax,
>>> imm32
>>>>> ASM_PFX(gSmmCr3): DD 0
>>>>> mov cr3, eax
>>>>> DB 0x67, 0x66
>>>>> lgdt [cs:ebp + (ASM_PFX(gcSmiInitGdtr) -
>>>>> ASM_PFX(SmmStartup))]
>>>>> - DB 0x66, 0xb8
>>>>> + DB 0x66, 0xb8 ; mov eax,
>>> imm32
>>>>> ASM_PFX(gSmmCr4): DD 0
>>>>> mov cr4, eax
>>>>> DB 0x66
>>>>> mov ecx, 0xc0000080 ; IA32_EFER
>>> MSR
>>>>> rdmsr
>>>>> DB 0x66
>>>>> test ebx, BIT20 ; check NXE
>>>>> capability
>>>>> jz .1
>>>>> or ah, BIT3 ; set NXE
>> bit
>>>>> wrmsr
>>>>> .1:
>>>>> - DB 0x66, 0xb8
>>>>> + DB 0x66, 0xb8 ; mov eax,
>>> imm32
>>>>> ASM_PFX(gSmmCr0): DD 0
>>>>> DB 0xbf, PROTECT_MODE_DS, 0 ; mov di,
>>>>> PROTECT_MODE_DS
>>>>> mov cr0, eax
>>>>> - DB 0x66, 0xea ; jmp far
>>>>> [ptr48]
>>>>> + DB 0x66, 0xea ; jmp far
>>>>> [ptr48]
>>>>> ASM_PFX(gSmmJmpAddr):
>>>>> DD @32bit
>>>>> DW PROTECT_MODE_CS
>>>>> @32bit:
>>>>> mov ds, edi
>>>>> mov es, edi
>>>>> --
>>>>> 2.14.1.3.gb7cf6e02401b
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> edk2-devel mailing list
>>>>> edk2-devel@lists.01.org
>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
2018-01-30 21:45 ` Laszlo Ersek
@ 2018-01-30 22:25 ` Kinney, Michael D
2018-01-31 5:44 ` Ni, Ruiyu
2018-01-31 10:40 ` Laszlo Ersek
0 siblings, 2 replies; 27+ messages in thread
From: Kinney, Michael D @ 2018-01-30 22:25 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01, Kinney, Michael D
Cc: Ni, Ruiyu, Paolo Bonzini, Yao, Jiewen, Dong, Eric
Laszlo,
I agree that the function is better than a macro.
I thought of the alignment issues as well. CopyMem()
is a good solution. We could also consider
WriteUnalignedxx() functions in BaseLib.
I was originally thinking this functionality would go
into BaseLib. But with the use of CopyMem(), we can't
do that. Maybe we should use WriteUnalignedxx() and
add some ASSERT() checks.
VOID
PatchAssembly (
VOID *BufferEnd,
UINT64 PatchValue,
UINTN ValueSize
)
{
ASSERT ((UINTN)BufferEnd > ValueSize);
switch (ValueSize) {
case 1:
ASSERT (PatchValue <= MAX_UINT8);
*((UINT8 *)BufferEnd - 1) = (UINT8)PatchValue;
case 2:
ASSERT (PatchValue <= MAX_UINT16);
WriteUnaligned16 ((UINT16 *)(BufferEnd) - 1, (UINT16)PatchValue));
break;
case 4:
ASSERT (PatchValue <= MAX_UINT32);
WriteUnaligned32 ((UINT32 *)(BufferEnd) - 1, (UINT32)PatchValue));
break;
case 8:
WriteUnaligned64 ((UINT64 *)(BufferEnd) - 1, PatchValue));
break;
default:
ASSERT (FALSE);
}
}
Mike
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, January 30, 2018 1:45 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-
> devel-01 <edk2-devel@lists.01.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini
> <pbonzini@redhat.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2] [PATCH 1/3]
> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
> SmmStartup()
>
> On 01/30/18 21:31, Kinney, Michael D wrote:
> > Laszlo,
> >
> > We have already used this technique in other NASM files
> > to remove DBs.
>
> OK.
>
> > Let us know if you have suggestions on how to make the
> > C code that performs the patches easier to read and
> > maintain.
>
> How about this:
>
> VOID
> PatchAssembly (
> VOID *BufferEnd,
> UINT64 PatchValue,
> UINTN ValueSize
> )
> {
> CopyMem (
> (VOID *)((UINTN)BufferEnd - ValueSize),
> &PatchValue,
> ValueSize
> );
> }
>
> extern UINT8 gAsmSmmCr0;
> extern UINT8 gAsmSmmCr3;
> extern UINT8 gAsmSmmCr4;
>
> ...
> {
> PatchAssembly (&gAsmSmmCr0, AsmReadCr0 (), 4);
> PatchAssembly (&gAsmSmmCr3, AsmReadCr3 (), 4);
> PatchAssembly (&gAsmSmmCr4, AsmReadCr4 (), 4);
> ...
> }
>
> (I think it's fine to open-code the last argument as "4",
> rather than
> "sizeof (UINT32)", because for patching, we must have
> intimate knowledge
> of the instruction anyway.)
>
> To me, this is easier to read, because:
>
> - there are no complex casts in the "business logic"
> - the size is spelled out once per patching
> - the function name and the variable names make it clear
> we are patching
> separately compiled assembly code that was linked into
> the same
> module.
>
> What do you think?
>
> Thanks!
> Laszlo
>
> >> -----Original Message-----
> >> From: edk2-devel [mailto:edk2-devel-
> bounces@lists.01.org]
> >> On Behalf Of Laszlo Ersek
> >> Sent: Tuesday, January 30, 2018 10:17 AM
> >> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> edk2-
> >> devel-01 <edk2-devel@lists.01.org>
> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini
> >> <pbonzini@redhat.com>; Yao, Jiewen
> >> <jiewen.yao@intel.com>; Dong, Eric
> <eric.dong@intel.com>
> >> Subject: Re: [edk2] [PATCH 1/3]
> >> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
> >> SmmStartup()
> >>
> >> On 01/30/18 18:22, Kinney, Michael D wrote:
> >>> Laszlo,
> >>>
> >>> The DBs can be removed if the label is moved after
> >>> the instruction and the patch is done to the label
> >>> minus the size of the patch value.
> >>
> >> Indeed I haven't thought of this.
> >>
> >> If I understand correctly, it means
> >>
> >> extern UINT8 gSmmCr0;
> >>
> >> *(UINT32*)(&gSmmCr0 - sizeof (UINT32)) =
> >> (UINT32)AsmReadCr0 ();
> >>
> >> TBH, the DB feels less ugly to me than this :)
> >>
> >> Still, if you think it would be an acceptable price to
> >> pay for removing
> >> the remaining DBs, I can respin.
> >>
> >> Thanks
> >> Laszlo
> >>
> >>>> -----Original Message-----
> >>>> From: edk2-devel [mailto:edk2-devel-
> >> bounces@lists.01.org]
> >>>> On Behalf Of Laszlo Ersek
> >>>> Sent: Tuesday, January 30, 2018 7:34 AM
> >>>> To: edk2-devel-01 <edk2-devel@lists.01.org>
> >>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen
> >>>> <jiewen.yao@intel.com>; Dong, Eric
> >> <eric.dong@intel.com>;
> >>>> Paolo Bonzini <pbonzini@redhat.com>
> >>>> Subject: [edk2] [PATCH 1/3]
> UefiCpuPkg/PiSmmCpuDxeSmm:
> >>>> update comments in IA32 SmmStartup()
> >>>>
> >>>> The gSmmCr3, gSmmCr4, gSmmCr0 and gSmmJmpAddr global
> >>>> variables are used
> >>>> for patching assembly instructions, thus we can
> never
> >>>> remove the DB
> >>>> encodings for those instructions. At least we should
> >> add
> >>>> the intended
> >>>> meanings in comments.
> >>>>
> >>>> This patch only changes comments.
> >>>>
> >>>> Cc: Eric Dong <eric.dong@intel.com>
> >>>> Cc: Jian J Wang <jian.j.wang@intel.com>
> >>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
> >>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> >>>> Contributed-under: TianoCore Contribution Agreement
> >> 1.1
> >>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> >>>> ---
> >>>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 8
> ++++-
> >> ---
> >>>> 1 file changed, 4 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git
> >> a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> >>>> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> >>>> index e96dd8d2392a..08534dba64b7 100644
> >>>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> >>>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> >>>> @@ -44,34 +44,34 @@ global ASM_PFX(SmmStartup)
> >>>> ASM_PFX(SmmStartup):
> >>>> DB 0x66
> >>>> mov eax, 0x80000001 ; read
> >>>> capability
> >>>> cpuid
> >>>> DB 0x66
> >>>> mov ebx, edx ; rdmsr
> will
> >>>> change edx. keep it in ebx.
> >>>> - DB 0x66, 0xb8
> >>>> + DB 0x66, 0xb8 ; mov eax,
> >> imm32
> >>>> ASM_PFX(gSmmCr3): DD 0
> >>>> mov cr3, eax
> >>>> DB 0x67, 0x66
> >>>> lgdt [cs:ebp + (ASM_PFX(gcSmiInitGdtr) -
> >>>> ASM_PFX(SmmStartup))]
> >>>> - DB 0x66, 0xb8
> >>>> + DB 0x66, 0xb8 ; mov eax,
> >> imm32
> >>>> ASM_PFX(gSmmCr4): DD 0
> >>>> mov cr4, eax
> >>>> DB 0x66
> >>>> mov ecx, 0xc0000080 ; IA32_EFER
> >> MSR
> >>>> rdmsr
> >>>> DB 0x66
> >>>> test ebx, BIT20 ; check NXE
> >>>> capability
> >>>> jz .1
> >>>> or ah, BIT3 ; set NXE
> bit
> >>>> wrmsr
> >>>> .1:
> >>>> - DB 0x66, 0xb8
> >>>> + DB 0x66, 0xb8 ; mov eax,
> >> imm32
> >>>> ASM_PFX(gSmmCr0): DD 0
> >>>> DB 0xbf, PROTECT_MODE_DS, 0 ; mov di,
> >>>> PROTECT_MODE_DS
> >>>> mov cr0, eax
> >>>> - DB 0x66, 0xea ; jmp far
> >>>> [ptr48]
> >>>> + DB 0x66, 0xea ; jmp far
> >>>> [ptr48]
> >>>> ASM_PFX(gSmmJmpAddr):
> >>>> DD @32bit
> >>>> DW PROTECT_MODE_CS
> >>>> @32bit:
> >>>> mov ds, edi
> >>>> mov es, edi
> >>>> --
> >>>> 2.14.1.3.gb7cf6e02401b
> >>>>
> >>>>
> >>>> _______________________________________________
> >>>> edk2-devel mailing list
> >>>> edk2-devel@lists.01.org
> >>>> https://lists.01.org/mailman/listinfo/edk2-devel
> >>
> >> _______________________________________________
> >> edk2-devel mailing list
> >> edk2-devel@lists.01.org
> >> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: eliminate conditional jump in IA32 SmmStartup()
2018-01-30 15:33 ` [PATCH 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: eliminate conditional jump in " Laszlo Ersek
@ 2018-01-31 5:12 ` Ni, Ruiyu
0 siblings, 0 replies; 27+ messages in thread
From: Ni, Ruiyu @ 2018-01-31 5:12 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
Cc: Eric Dong, Jian J Wang, Jiewen Yao, Paolo Bonzini
On 1/30/2018 11:33 PM, Laszlo Ersek wrote:
> SMM emulation under KVM crashes the guest when the "jz" branch, added in
> commit d4d87596c11d ("UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if it's
> supported", 2018-01-18), is taken.
>
> Rework the propagation of CPUID.80000001H:EDX.NX [bit 20] to IA32_EFER.NXE
> [bit 11] so that no code is executed conditionally.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Ref: http://mid.mail-archive.com/d6fff558-6c4f-9ca6-74a7-e7cd9d007276@redhat.com
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> index 9231aa5b3ded..102e0bdbabc8 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> @@ -44,26 +44,25 @@ global ASM_PFX(SmmStartup)
>
> BITS 16
> ASM_PFX(SmmStartup):
> mov eax, 0x80000001 ; read capability
> cpuid
> mov ebx, edx ; rdmsr will change edx. keep it in ebx.
> + and ebx, BIT20 ; extract XD capability bit
Per CPUID_EXTENDED_CPU_SIG_EDX definition in
UefiCpuPkg/Include/Register/CpuId.h, the BIT name is NX.
> + shr ebx, 9 ; shift bit to IA32_EFER NXE position
How about changing above comments to:
; shift bit to IA32_EFER.NXE[BIT11] position?
> DB 0x66, 0xb8 ; mov eax, imm32
> ASM_PFX(gSmmCr3): DD 0
> mov cr3, eax
> o32 lgdt [cs:ebp + (ASM_PFX(gcSmiInitGdtr) - ASM_PFX(SmmStartup))]
> DB 0x66, 0xb8 ; mov eax, imm32
> ASM_PFX(gSmmCr4): DD 0
> mov cr4, eax
> mov ecx, 0xc0000080 ; IA32_EFER MSR
> rdmsr
> - test ebx, BIT20 ; check NXE capability
> - jz .1
> - or ah, BIT3 ; set NXE bit
> + or eax, ebx ; set NXE bit if XD is available
; Bit name is NX.
> wrmsr
> -.1:
> DB 0x66, 0xb8 ; mov eax, imm32
> ASM_PFX(gSmmCr0): DD 0
> mov di, PROTECT_MODE_DS
> mov cr0, eax
> DB 0x66, 0xea ; jmp far [ptr48]
> ASM_PFX(gSmmJmpAddr):
>
Very clever solution to remove jz.
With the minor comments update,
Reviewed-by: Ruiyu Ni <ruiyu.ni@Intel.com>
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
2018-01-30 22:25 ` Kinney, Michael D
@ 2018-01-31 5:44 ` Ni, Ruiyu
2018-01-31 5:54 ` Ni, Ruiyu
2018-01-31 10:42 ` Laszlo Ersek
2018-01-31 10:40 ` Laszlo Ersek
1 sibling, 2 replies; 27+ messages in thread
From: Ni, Ruiyu @ 2018-01-31 5:44 UTC (permalink / raw)
To: Kinney, Michael D, Laszlo Ersek, edk2-devel-01
Cc: Paolo Bonzini, Yao, Jiewen, Dong, Eric
Mike, Laszlo,
Does the patch only apply to the operand?
If so, PatchAssembly looks too general.
How about the name like PatchAssemblyOperand?
On 1/31/2018 6:25 AM, Kinney, Michael D wrote:
> Laszlo,
>
> I agree that the function is better than a macro.
>
> I thought of the alignment issues as well. CopyMem()
> is a good solution. We could also consider
> WriteUnalignedxx() functions in BaseLib.
>
> I was originally thinking this functionality would go
> into BaseLib. But with the use of CopyMem(), we can't
> do that. Maybe we should use WriteUnalignedxx() and
> add some ASSERT() checks.
>
> VOID
> PatchAssembly (
> VOID *BufferEnd,
> UINT64 PatchValue,
> UINTN ValueSize
> )
> {
> ASSERT ((UINTN)BufferEnd > ValueSize);
> switch (ValueSize) {
> case 1:
> ASSERT (PatchValue <= MAX_UINT8);
> *((UINT8 *)BufferEnd - 1) = (UINT8)PatchValue;
> case 2:
> ASSERT (PatchValue <= MAX_UINT16);
> WriteUnaligned16 ((UINT16 *)(BufferEnd) - 1, (UINT16)PatchValue));
> break;
> case 4:
> ASSERT (PatchValue <= MAX_UINT32);
> WriteUnaligned32 ((UINT32 *)(BufferEnd) - 1, (UINT32)PatchValue));
> break;
> case 8:
> WriteUnaligned64 ((UINT64 *)(BufferEnd) - 1, PatchValue));
> break;
> default:
> ASSERT (FALSE);
> }
> }
>
> Mike
>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Tuesday, January 30, 2018 1:45 PM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-
>> devel-01 <edk2-devel@lists.01.org>
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini
>> <pbonzini@redhat.com>; Yao, Jiewen
>> <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
>> Subject: Re: [edk2] [PATCH 1/3]
>> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
>> SmmStartup()
>>
>> On 01/30/18 21:31, Kinney, Michael D wrote:
>>> Laszlo,
>>>
>>> We have already used this technique in other NASM files
>>> to remove DBs.
>>
>> OK.
>>
>>> Let us know if you have suggestions on how to make the
>>> C code that performs the patches easier to read and
>>> maintain.
>>
>> How about this:
>>
>> VOID
>> PatchAssembly (
>> VOID *BufferEnd,
>> UINT64 PatchValue,
>> UINTN ValueSize
>> )
>> {
>> CopyMem (
>> (VOID *)((UINTN)BufferEnd - ValueSize),
>> &PatchValue,
>> ValueSize
>> );
>> }
>>
>> extern UINT8 gAsmSmmCr0;
>> extern UINT8 gAsmSmmCr3;
>> extern UINT8 gAsmSmmCr4;
>>
>> ...
>> {
>> PatchAssembly (&gAsmSmmCr0, AsmReadCr0 (), 4);
>> PatchAssembly (&gAsmSmmCr3, AsmReadCr3 (), 4);
>> PatchAssembly (&gAsmSmmCr4, AsmReadCr4 (), 4);
>> ...
>> }
>>
>> (I think it's fine to open-code the last argument as "4",
>> rather than
>> "sizeof (UINT32)", because for patching, we must have
>> intimate knowledge
>> of the instruction anyway.)
>>
>> To me, this is easier to read, because:
>>
>> - there are no complex casts in the "business logic"
>> - the size is spelled out once per patching
>> - the function name and the variable names make it clear
>> we are patching
>> separately compiled assembly code that was linked into
>> the same
>> module.
>>
>> What do you think?
>>
>> Thanks!
>> Laszlo
>>
>>>> -----Original Message-----
>>>> From: edk2-devel [mailto:edk2-devel-
>> bounces@lists.01.org]
>>>> On Behalf Of Laszlo Ersek
>>>> Sent: Tuesday, January 30, 2018 10:17 AM
>>>> To: Kinney, Michael D <michael.d.kinney@intel.com>;
>> edk2-
>>>> devel-01 <edk2-devel@lists.01.org>
>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini
>>>> <pbonzini@redhat.com>; Yao, Jiewen
>>>> <jiewen.yao@intel.com>; Dong, Eric
>> <eric.dong@intel.com>
>>>> Subject: Re: [edk2] [PATCH 1/3]
>>>> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
>>>> SmmStartup()
>>>>
>>>> On 01/30/18 18:22, Kinney, Michael D wrote:
>>>>> Laszlo,
>>>>>
>>>>> The DBs can be removed if the label is moved after
>>>>> the instruction and the patch is done to the label
>>>>> minus the size of the patch value.
>>>>
>>>> Indeed I haven't thought of this.
>>>>
>>>> If I understand correctly, it means
>>>>
>>>> extern UINT8 gSmmCr0;
>>>>
>>>> *(UINT32*)(&gSmmCr0 - sizeof (UINT32)) =
>>>> (UINT32)AsmReadCr0 ();
>>>>
>>>> TBH, the DB feels less ugly to me than this :)
>>>>
>>>> Still, if you think it would be an acceptable price to
>>>> pay for removing
>>>> the remaining DBs, I can respin.
>>>>
>>>> Thanks
>>>> Laszlo
>>>>
>>>>>> -----Original Message-----
>>>>>> From: edk2-devel [mailto:edk2-devel-
>>>> bounces@lists.01.org]
>>>>>> On Behalf Of Laszlo Ersek
>>>>>> Sent: Tuesday, January 30, 2018 7:34 AM
>>>>>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>>>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen
>>>>>> <jiewen.yao@intel.com>; Dong, Eric
>>>> <eric.dong@intel.com>;
>>>>>> Paolo Bonzini <pbonzini@redhat.com>
>>>>>> Subject: [edk2] [PATCH 1/3]
>> UefiCpuPkg/PiSmmCpuDxeSmm:
>>>>>> update comments in IA32 SmmStartup()
>>>>>>
>>>>>> The gSmmCr3, gSmmCr4, gSmmCr0 and gSmmJmpAddr global
>>>>>> variables are used
>>>>>> for patching assembly instructions, thus we can
>> never
>>>>>> remove the DB
>>>>>> encodings for those instructions. At least we should
>>>> add
>>>>>> the intended
>>>>>> meanings in comments.
>>>>>>
>>>>>> This patch only changes comments.
>>>>>>
>>>>>> Cc: Eric Dong <eric.dong@intel.com>
>>>>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>>>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>>>>> Contributed-under: TianoCore Contribution Agreement
>>>> 1.1
>>>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>>>> ---
>>>>>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 8
>> ++++-
>>>> ---
>>>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git
>>>> a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>>>> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>>>> index e96dd8d2392a..08534dba64b7 100644
>>>>>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>>>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>>>> @@ -44,34 +44,34 @@ global ASM_PFX(SmmStartup)
>>>>>> ASM_PFX(SmmStartup):
>>>>>> DB 0x66
>>>>>> mov eax, 0x80000001 ; read
>>>>>> capability
>>>>>> cpuid
>>>>>> DB 0x66
>>>>>> mov ebx, edx ; rdmsr
>> will
>>>>>> change edx. keep it in ebx.
>>>>>> - DB 0x66, 0xb8
>>>>>> + DB 0x66, 0xb8 ; mov eax,
>>>> imm32
>>>>>> ASM_PFX(gSmmCr3): DD 0
>>>>>> mov cr3, eax
>>>>>> DB 0x67, 0x66
>>>>>> lgdt [cs:ebp + (ASM_PFX(gcSmiInitGdtr) -
>>>>>> ASM_PFX(SmmStartup))]
>>>>>> - DB 0x66, 0xb8
>>>>>> + DB 0x66, 0xb8 ; mov eax,
>>>> imm32
>>>>>> ASM_PFX(gSmmCr4): DD 0
>>>>>> mov cr4, eax
>>>>>> DB 0x66
>>>>>> mov ecx, 0xc0000080 ; IA32_EFER
>>>> MSR
>>>>>> rdmsr
>>>>>> DB 0x66
>>>>>> test ebx, BIT20 ; check NXE
>>>>>> capability
>>>>>> jz .1
>>>>>> or ah, BIT3 ; set NXE
>> bit
>>>>>> wrmsr
>>>>>> .1:
>>>>>> - DB 0x66, 0xb8
>>>>>> + DB 0x66, 0xb8 ; mov eax,
>>>> imm32
>>>>>> ASM_PFX(gSmmCr0): DD 0
>>>>>> DB 0xbf, PROTECT_MODE_DS, 0 ; mov di,
>>>>>> PROTECT_MODE_DS
>>>>>> mov cr0, eax
>>>>>> - DB 0x66, 0xea ; jmp far
>>>>>> [ptr48]
>>>>>> + DB 0x66, 0xea ; jmp far
>>>>>> [ptr48]
>>>>>> ASM_PFX(gSmmJmpAddr):
>>>>>> DD @32bit
>>>>>> DW PROTECT_MODE_CS
>>>>>> @32bit:
>>>>>> mov ds, edi
>>>>>> mov es, edi
>>>>>> --
>>>>>> 2.14.1.3.gb7cf6e02401b
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> edk2-devel mailing list
>>>>>> edk2-devel@lists.01.org
>>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>>
>>>> _______________________________________________
>>>> edk2-devel mailing list
>>>> edk2-devel@lists.01.org
>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/3] UefiCpuPkg/PiSmmCpuDxeSmm: remove unneeded DBs from IA32 SmmStartup()
2018-01-30 15:33 ` [PATCH 2/3] UefiCpuPkg/PiSmmCpuDxeSmm: remove unneeded DBs from " Laszlo Ersek
@ 2018-01-31 5:45 ` Ni, Ruiyu
0 siblings, 0 replies; 27+ messages in thread
From: Ni, Ruiyu @ 2018-01-31 5:45 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
Cc: Eric Dong, Jian J Wang, Jiewen Yao, Paolo Bonzini
On 1/30/2018 11:33 PM, Laszlo Ersek wrote:
> The SmmStartup() executes in SMM, which is very similar to real mode. Add
> "BITS 16" before it and "BITS 32" after it (just before the @32bit label).
>
> Remove the manual 0x66 operand-size override prefixes, for selecting
> 32-bit operands -- the sizes of our operands trigger NASM to insert the
> prefixes automatically in almost every spot. The one place where we have
> to add it back manually is the LGDT instruction. (The 0x67 address-size
> override prefix is also auto-generated.)
>
> This patch causes NASM to generate byte-identical object code (determined
> by disassembling both the pre-patch and post-patch versions, and comparing
> the listings), except:
>
>> @@ -158,7 +158,7 @@
>> 00000142 6689D3 mov ebx,edx
>> 00000145 66B800000000 mov eax,0x0
>> 0000014B 0F22D8 mov cr3,eax
>> -0000014E 67662E0F0155F6 o32 lgdt [cs:ebp-0xa]
>> +0000014E 2E66670F0155F6 o32 lgdt [cs:ebp-0xa]
>> 00000155 66B800000000 mov eax,0x0
>> 0000015B 0F22E0 mov cr4,eax
>> 0000015E 66B9800000C0 mov ecx,0xc0000080
>
> The only difference is the prefix list order, it changes from:
>
> - 0x67, 0x66, 0x2E
>
> to
>
> - 0x2E, 0x66, 0x67
>
> (0x2E is "CS segment override").
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=866
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> index 08534dba64b7..9231aa5b3ded 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> @@ -38,43 +38,42 @@ global ASM_PFX(gcSmmInitTemplate)
>
> ASM_PFX(gcSmiInitGdtr):
> DW 0
> DQ 0
>
> global ASM_PFX(SmmStartup)
> +
> +BITS 16
> ASM_PFX(SmmStartup):
> - DB 0x66
> mov eax, 0x80000001 ; read capability
> cpuid
> - DB 0x66
> mov ebx, edx ; rdmsr will change edx. keep it in ebx.
> DB 0x66, 0xb8 ; mov eax, imm32
> ASM_PFX(gSmmCr3): DD 0
> mov cr3, eax
> - DB 0x67, 0x66
> - lgdt [cs:ebp + (ASM_PFX(gcSmiInitGdtr) - ASM_PFX(SmmStartup))]
> +o32 lgdt [cs:ebp + (ASM_PFX(gcSmiInitGdtr) - ASM_PFX(SmmStartup))]
> DB 0x66, 0xb8 ; mov eax, imm32
> ASM_PFX(gSmmCr4): DD 0
> mov cr4, eax
> - DB 0x66
> mov ecx, 0xc0000080 ; IA32_EFER MSR
> rdmsr
> - DB 0x66
> test ebx, BIT20 ; check NXE capability
> jz .1
> or ah, BIT3 ; set NXE bit
> wrmsr
> .1:
> DB 0x66, 0xb8 ; mov eax, imm32
> ASM_PFX(gSmmCr0): DD 0
> - DB 0xbf, PROTECT_MODE_DS, 0 ; mov di, PROTECT_MODE_DS
> + mov di, PROTECT_MODE_DS
> mov cr0, eax
> DB 0x66, 0xea ; jmp far [ptr48]
> ASM_PFX(gSmmJmpAddr):
> DD @32bit
> DW PROTECT_MODE_CS
> +
> +BITS 32
> @32bit:
> mov ds, edi
> mov es, edi
> mov fs, edi
> mov gs, edi
> mov ss, edi
>
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
2018-01-31 5:44 ` Ni, Ruiyu
@ 2018-01-31 5:54 ` Ni, Ruiyu
2018-01-31 10:56 ` Laszlo Ersek
2018-01-31 10:42 ` Laszlo Ersek
1 sibling, 1 reply; 27+ messages in thread
From: Ni, Ruiyu @ 2018-01-31 5:54 UTC (permalink / raw)
To: Kinney, Michael D, Laszlo Ersek, edk2-devel-01
Cc: Paolo Bonzini, Yao, Jiewen, Dong, Eric
On 1/31/2018 1:44 PM, Ni, Ruiyu wrote:
> Mike, Laszlo,
> Does the patch only apply to the operand?
> If so, PatchAssembly looks too general.
> How about the name like PatchAssemblyOperand?
>
>
> On 1/31/2018 6:25 AM, Kinney, Michael D wrote:
>> Laszlo,
>>
>> I agree that the function is better than a macro.
>>
>> I thought of the alignment issues as well. CopyMem()
>> is a good solution. We could also consider
>> WriteUnalignedxx() functions in BaseLib.
>>
>> I was originally thinking this functionality would go
>> into BaseLib. But with the use of CopyMem(), we can't
>> do that. Maybe we should use WriteUnalignedxx() and
>> add some ASSERT() checks.
>>
>> VOID
>> PatchAssembly (
>> VOID *BufferEnd,
>> UINT64 PatchValue,
>> UINTN ValueSize
>> )
>> {
>> ASSERT ((UINTN)BufferEnd > ValueSize);
>> switch (ValueSize) {
>> case 1:
>> ASSERT (PatchValue <= MAX_UINT8);
>> *((UINT8 *)BufferEnd - 1) = (UINT8)PatchValue;
>> case 2:
>> ASSERT (PatchValue <= MAX_UINT16);
>> WriteUnaligned16 ((UINT16 *)(BufferEnd) - 1, (UINT16)PatchValue));
>> break;
>> case 4:
>> ASSERT (PatchValue <= MAX_UINT32);
>> WriteUnaligned32 ((UINT32 *)(BufferEnd) - 1, (UINT32)PatchValue));
>> break;
>> case 8:
>> WriteUnaligned64 ((UINT64 *)(BufferEnd) - 1, PatchValue));
>> break;
>> default:
>> ASSERT (FALSE);
>> }
>> }
>>
>> Mike
>>
>>> -----Original Message-----
>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>> Sent: Tuesday, January 30, 2018 1:45 PM
>>> To: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-
>>> devel-01 <edk2-devel@lists.01.org>
>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini
>>> <pbonzini@redhat.com>; Yao, Jiewen
>>> <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
>>> Subject: Re: [edk2] [PATCH 1/3]
>>> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
>>> SmmStartup()
>>>
>>> On 01/30/18 21:31, Kinney, Michael D wrote:
>>>> Laszlo,
>>>>
>>>> We have already used this technique in other NASM files
>>>> to remove DBs.
>>>
>>> OK.
>>>
>>>> Let us know if you have suggestions on how to make the
>>>> C code that performs the patches easier to read and
>>>> maintain.
>>>
>>> How about this:
>>>
>>> VOID
>>> PatchAssembly (
>>> VOID *BufferEnd,
>>> UINT64 PatchValue,
>>> UINTN ValueSize
>>> )
>>> {
>>> CopyMem (
>>> (VOID *)((UINTN)BufferEnd - ValueSize),
>>> &PatchValue,
>>> ValueSize
>>> );
>>> }
>>>
>>> extern UINT8 gAsmSmmCr0;
>>> extern UINT8 gAsmSmmCr3;
>>> extern UINT8 gAsmSmmCr4;
>>>
>>> ...
>>> {
>>> PatchAssembly (&gAsmSmmCr0, AsmReadCr0 (), 4);
>>> PatchAssembly (&gAsmSmmCr3, AsmReadCr3 (), 4);
>>> PatchAssembly (&gAsmSmmCr4, AsmReadCr4 (), 4);
>>> ...
>>> }
>>>
>>> (I think it's fine to open-code the last argument as "4",
>>> rather than
>>> "sizeof (UINT32)", because for patching, we must have
>>> intimate knowledge
>>> of the instruction anyway.)
>>>
>>> To me, this is easier to read, because:
>>>
>>> - there are no complex casts in the "business logic"
>>> - the size is spelled out once per patching
>>> - the function name and the variable names make it clear
>>> we are patching
>>> separately compiled assembly code that was linked into
>>> the same
>>> module.
>>>
>>> What do you think?
>>>
>>> Thanks!
>>> Laszlo
>>>
>>>>> -----Original Message-----
>>>>> From: edk2-devel [mailto:edk2-devel-
>>> bounces@lists.01.org]
>>>>> On Behalf Of Laszlo Ersek
>>>>> Sent: Tuesday, January 30, 2018 10:17 AM
>>>>> To: Kinney, Michael D <michael.d.kinney@intel.com>;
>>> edk2-
>>>>> devel-01 <edk2-devel@lists.01.org>
>>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini
>>>>> <pbonzini@redhat.com>; Yao, Jiewen
>>>>> <jiewen.yao@intel.com>; Dong, Eric
>>> <eric.dong@intel.com>
>>>>> Subject: Re: [edk2] [PATCH 1/3]
>>>>> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
>>>>> SmmStartup()
>>>>>
>>>>> On 01/30/18 18:22, Kinney, Michael D wrote:
>>>>>> Laszlo,
>>>>>>
>>>>>> The DBs can be removed if the label is moved after
>>>>>> the instruction and the patch is done to the label
>>>>>> minus the size of the patch value.
>>>>>
>>>>> Indeed I haven't thought of this.
>>>>>
>>>>> If I understand correctly, it means
>>>>>
>>>>> extern UINT8 gSmmCr0;
>>>>>
>>>>> *(UINT32*)(&gSmmCr0 - sizeof (UINT32)) =
>>>>> (UINT32)AsmReadCr0 ();
>>>>>
>>>>> TBH, the DB feels less ugly to me than this :)
>>>>>
>>>>> Still, if you think it would be an acceptable price to
>>>>> pay for removing
>>>>> the remaining DBs, I can respin.
>>>>>
>>>>> Thanks
>>>>> Laszlo
>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: edk2-devel [mailto:edk2-devel-
>>>>> bounces@lists.01.org]
>>>>>>> On Behalf Of Laszlo Ersek
>>>>>>> Sent: Tuesday, January 30, 2018 7:34 AM
>>>>>>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>>>>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen
>>>>>>> <jiewen.yao@intel.com>; Dong, Eric
>>>>> <eric.dong@intel.com>;
>>>>>>> Paolo Bonzini <pbonzini@redhat.com>
>>>>>>> Subject: [edk2] [PATCH 1/3]
>>> UefiCpuPkg/PiSmmCpuDxeSmm:
>>>>>>> update comments in IA32 SmmStartup()
>>>>>>>
>>>>>>> The gSmmCr3, gSmmCr4, gSmmCr0 and gSmmJmpAddr global
>>>>>>> variables are used
>>>>>>> for patching assembly instructions, thus we can
>>> never
>>>>>>> remove the DB
>>>>>>> encodings for those instructions. At least we should
>>>>> add
>>>>>>> the intended
>>>>>>> meanings in comments.
>>>>>>>
>>>>>>> This patch only changes comments.
>>>>>>>
>>>>>>> Cc: Eric Dong <eric.dong@intel.com>
>>>>>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>>>>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>>>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>>>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>>>>>> Contributed-under: TianoCore Contribution Agreement
>>>>> 1.1
>>>>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>>>>> ---
>>>>>>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 8
>>> ++++-
>>>>> ---
>>>>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git
>>>>> a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>>>>> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>>>>> index e96dd8d2392a..08534dba64b7 100644
>>>>>>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>>>>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>>>>> @@ -44,34 +44,34 @@ global ASM_PFX(SmmStartup)
>>>>>>> ASM_PFX(SmmStartup):
>>>>>>> DB 0x66
>>>>>>> mov eax, 0x80000001 ; read
>>>>>>> capability
>>>>>>> cpuid
>>>>>>> DB 0x66
>>>>>>> mov ebx, edx ; rdmsr
>>> will
>>>>>>> change edx. keep it in ebx.
>>>>>>> - DB 0x66, 0xb8
>>>>>>> + DB 0x66, 0xb8 ; mov eax,
>>>>> imm32
>>>>>>> ASM_PFX(gSmmCr3): DD 0
>>>>>>> mov cr3, eax
>>>>>>> DB 0x67, 0x66
>>>>>>> lgdt [cs:ebp + (ASM_PFX(gcSmiInitGdtr) -
>>>>>>> ASM_PFX(SmmStartup))]
>>>>>>> - DB 0x66, 0xb8
>>>>>>> + DB 0x66, 0xb8 ; mov eax,
>>>>> imm32
>>>>>>> ASM_PFX(gSmmCr4): DD 0
>>>>>>> mov cr4, eax
>>>>>>> DB 0x66
>>>>>>> mov ecx, 0xc0000080 ; IA32_EFER
>>>>> MSR
>>>>>>> rdmsr
>>>>>>> DB 0x66
>>>>>>> test ebx, BIT20 ; check NXE
>>>>>>> capability
>>>>>>> jz .1
>>>>>>> or ah, BIT3 ; set NXE
>>> bit
>>>>>>> wrmsr
>>>>>>> .1:
>>>>>>> - DB 0x66, 0xb8
>>>>>>> + DB 0x66, 0xb8 ; mov eax,
>>>>> imm32
>>>>>>> ASM_PFX(gSmmCr0): DD 0
>>>>>>> DB 0xbf, PROTECT_MODE_DS, 0 ; mov di,
>>>>>>> PROTECT_MODE_DS
>>>>>>> mov cr0, eax
>>>>>>> - DB 0x66, 0xea ; jmp far
>>>>>>> [ptr48]
>>>>>>> + DB 0x66, 0xea ; jmp far
>>>>>>> [ptr48]
>>>>>>> ASM_PFX(gSmmJmpAddr):
>>>>>>> DD @32bit
>>>>>>> DW PROTECT_MODE_CS
>>>>>>> @32bit:
>>>>>>> mov ds, edi
>>>>>>> mov es, edi
>>>>>>> --
>>>>>>> 2.14.1.3.gb7cf6e02401b
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> edk2-devel mailing list
>>>>>>> edk2-devel@lists.01.org
>>>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>>>
>>>>> _______________________________________________
>>>>> edk2-devel mailing list
>>>>> edk2-devel@lists.01.org
>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>
>
>
Laszlo, Mike,
Considering this patch doesn't make the code worse,
actually improved a tiny bit, can we firstly check in the three patches?
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
2018-01-30 22:25 ` Kinney, Michael D
2018-01-31 5:44 ` Ni, Ruiyu
@ 2018-01-31 10:40 ` Laszlo Ersek
2018-01-31 22:11 ` Kinney, Michael D
2018-02-02 10:06 ` Ard Biesheuvel
1 sibling, 2 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-01-31 10:40 UTC (permalink / raw)
To: Kinney, Michael D, edk2-devel-01
Cc: Ni, Ruiyu, Paolo Bonzini, Yao, Jiewen, Dong, Eric, Ard Biesheuvel,
Leif Lindholm (Linaro address)
On 01/30/18 23:25, Kinney, Michael D wrote:
> Laszlo,
>
> I agree that the function is better than a macro.
>
> I thought of the alignment issues as well. CopyMem()
> is a good solution. We could also consider
> WriteUnalignedxx() functions in BaseLib.
IMO, the WriteUnalignedxx functions are a bit pointless in the exact
form they are declared (this was discussed earlier esp. with regard to
aarch64). The functions take pointers to objects that already have the
target type, such as
UINT32
EFIAPI
WriteUnaligned32 (
OUT UINT32 *Buffer,
IN UINT32 Value
)
Here the type of Buffer should be (VOID *), not (UINT32 *). Otherwise,
the undefined behavior (due to mis-alignment) surfaces as soon as the
function is called with an unaligned pointer (i.e. before the target
area is actually written).
> I was originally thinking this functionality would go
> into BaseLib. But with the use of CopyMem(), we can't
> do that.
Can we put it in BaseMemoryLib instead (which is where CopyMem() is
from)? That library class is still low-level enough. And, while I count
9 library instances, PatchAssembly() is not a large function, we could
tolerate adding it to all 9 instances, identically.
Let me also ask the opposite question: should we perhaps make the
PatchAssembly() API *less* abstract? (Also suggested by your naming of
the macro, PATCH_X86_ASM.) If the instruction encoding on e.g. AARCH64
doesn't lend itself to such patching (= expressed through the address
right after the instruction), then even BaseMemoryLib may be too generic
for the API.
> Maybe we should use WriteUnalignedxx() and
> add some ASSERT() checks.
>
> VOID
> PatchAssembly (
> VOID *BufferEnd,
> UINT64 PatchValue,
> UINTN ValueSize
> )
> {
> ASSERT ((UINTN)BufferEnd > ValueSize);
> switch (ValueSize) {
> case 1:
> ASSERT (PatchValue <= MAX_UINT8);
> *((UINT8 *)BufferEnd - 1) = (UINT8)PatchValue;
> case 2:
> ASSERT (PatchValue <= MAX_UINT16);
> WriteUnaligned16 ((UINT16 *)(BufferEnd) - 1, (UINT16)PatchValue));
> break;
> case 4:
> ASSERT (PatchValue <= MAX_UINT32);
> WriteUnaligned32 ((UINT32 *)(BufferEnd) - 1, (UINT32)PatchValue));
> break;
> case 8:
> WriteUnaligned64 ((UINT64 *)(BufferEnd) - 1, PatchValue));
> break;
> default:
> ASSERT (FALSE);
> }
> }
In my opinion:
- If Ard and Leif say that PatchAssembly() API makes sense for AARCH64,
then I think we can go with the above generic implementation (for
BaseLib).
- If Ard and Leif say the API is only useful on x86, then I suggest that
we implement the API separately for all arches (still in BaseLib):
- On x86, we should simply open-code the unaligned accesses (like you
originall suggested). The pointer arithmetic will look a bit wild,
but it's safely hidden behind a BaseLib API, so client code will
look nice.
- On all other arches, we should implement the function with
ASSERT(FALSE).
Thanks!
Laszlo
>
> Mike
>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Tuesday, January 30, 2018 1:45 PM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-
>> devel-01 <edk2-devel@lists.01.org>
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini
>> <pbonzini@redhat.com>; Yao, Jiewen
>> <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
>> Subject: Re: [edk2] [PATCH 1/3]
>> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
>> SmmStartup()
>>
>> On 01/30/18 21:31, Kinney, Michael D wrote:
>>> Laszlo,
>>>
>>> We have already used this technique in other NASM files
>>> to remove DBs.
>>
>> OK.
>>
>>> Let us know if you have suggestions on how to make the
>>> C code that performs the patches easier to read and
>>> maintain.
>>
>> How about this:
>>
>> VOID
>> PatchAssembly (
>> VOID *BufferEnd,
>> UINT64 PatchValue,
>> UINTN ValueSize
>> )
>> {
>> CopyMem (
>> (VOID *)((UINTN)BufferEnd - ValueSize),
>> &PatchValue,
>> ValueSize
>> );
>> }
>>
>> extern UINT8 gAsmSmmCr0;
>> extern UINT8 gAsmSmmCr3;
>> extern UINT8 gAsmSmmCr4;
>>
>> ...
>> {
>> PatchAssembly (&gAsmSmmCr0, AsmReadCr0 (), 4);
>> PatchAssembly (&gAsmSmmCr3, AsmReadCr3 (), 4);
>> PatchAssembly (&gAsmSmmCr4, AsmReadCr4 (), 4);
>> ...
>> }
>>
>> (I think it's fine to open-code the last argument as "4",
>> rather than
>> "sizeof (UINT32)", because for patching, we must have
>> intimate knowledge
>> of the instruction anyway.)
>>
>> To me, this is easier to read, because:
>>
>> - there are no complex casts in the "business logic"
>> - the size is spelled out once per patching
>> - the function name and the variable names make it clear
>> we are patching
>> separately compiled assembly code that was linked into
>> the same
>> module.
>>
>> What do you think?
>>
>> Thanks!
>> Laszlo
>>
>>>> -----Original Message-----
>>>> From: edk2-devel [mailto:edk2-devel-
>> bounces@lists.01.org]
>>>> On Behalf Of Laszlo Ersek
>>>> Sent: Tuesday, January 30, 2018 10:17 AM
>>>> To: Kinney, Michael D <michael.d.kinney@intel.com>;
>> edk2-
>>>> devel-01 <edk2-devel@lists.01.org>
>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini
>>>> <pbonzini@redhat.com>; Yao, Jiewen
>>>> <jiewen.yao@intel.com>; Dong, Eric
>> <eric.dong@intel.com>
>>>> Subject: Re: [edk2] [PATCH 1/3]
>>>> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
>>>> SmmStartup()
>>>>
>>>> On 01/30/18 18:22, Kinney, Michael D wrote:
>>>>> Laszlo,
>>>>>
>>>>> The DBs can be removed if the label is moved after
>>>>> the instruction and the patch is done to the label
>>>>> minus the size of the patch value.
>>>>
>>>> Indeed I haven't thought of this.
>>>>
>>>> If I understand correctly, it means
>>>>
>>>> extern UINT8 gSmmCr0;
>>>>
>>>> *(UINT32*)(&gSmmCr0 - sizeof (UINT32)) =
>>>> (UINT32)AsmReadCr0 ();
>>>>
>>>> TBH, the DB feels less ugly to me than this :)
>>>>
>>>> Still, if you think it would be an acceptable price to
>>>> pay for removing
>>>> the remaining DBs, I can respin.
>>>>
>>>> Thanks
>>>> Laszlo
>>>>
>>>>>> -----Original Message-----
>>>>>> From: edk2-devel [mailto:edk2-devel-
>>>> bounces@lists.01.org]
>>>>>> On Behalf Of Laszlo Ersek
>>>>>> Sent: Tuesday, January 30, 2018 7:34 AM
>>>>>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>>>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen
>>>>>> <jiewen.yao@intel.com>; Dong, Eric
>>>> <eric.dong@intel.com>;
>>>>>> Paolo Bonzini <pbonzini@redhat.com>
>>>>>> Subject: [edk2] [PATCH 1/3]
>> UefiCpuPkg/PiSmmCpuDxeSmm:
>>>>>> update comments in IA32 SmmStartup()
>>>>>>
>>>>>> The gSmmCr3, gSmmCr4, gSmmCr0 and gSmmJmpAddr global
>>>>>> variables are used
>>>>>> for patching assembly instructions, thus we can
>> never
>>>>>> remove the DB
>>>>>> encodings for those instructions. At least we should
>>>> add
>>>>>> the intended
>>>>>> meanings in comments.
>>>>>>
>>>>>> This patch only changes comments.
>>>>>>
>>>>>> Cc: Eric Dong <eric.dong@intel.com>
>>>>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>>>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>>>>> Contributed-under: TianoCore Contribution Agreement
>>>> 1.1
>>>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>>>> ---
>>>>>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 8
>> ++++-
>>>> ---
>>>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git
>>>> a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>>>> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>>>> index e96dd8d2392a..08534dba64b7 100644
>>>>>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>>>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>>>> @@ -44,34 +44,34 @@ global ASM_PFX(SmmStartup)
>>>>>> ASM_PFX(SmmStartup):
>>>>>> DB 0x66
>>>>>> mov eax, 0x80000001 ; read
>>>>>> capability
>>>>>> cpuid
>>>>>> DB 0x66
>>>>>> mov ebx, edx ; rdmsr
>> will
>>>>>> change edx. keep it in ebx.
>>>>>> - DB 0x66, 0xb8
>>>>>> + DB 0x66, 0xb8 ; mov eax,
>>>> imm32
>>>>>> ASM_PFX(gSmmCr3): DD 0
>>>>>> mov cr3, eax
>>>>>> DB 0x67, 0x66
>>>>>> lgdt [cs:ebp + (ASM_PFX(gcSmiInitGdtr) -
>>>>>> ASM_PFX(SmmStartup))]
>>>>>> - DB 0x66, 0xb8
>>>>>> + DB 0x66, 0xb8 ; mov eax,
>>>> imm32
>>>>>> ASM_PFX(gSmmCr4): DD 0
>>>>>> mov cr4, eax
>>>>>> DB 0x66
>>>>>> mov ecx, 0xc0000080 ; IA32_EFER
>>>> MSR
>>>>>> rdmsr
>>>>>> DB 0x66
>>>>>> test ebx, BIT20 ; check NXE
>>>>>> capability
>>>>>> jz .1
>>>>>> or ah, BIT3 ; set NXE
>> bit
>>>>>> wrmsr
>>>>>> .1:
>>>>>> - DB 0x66, 0xb8
>>>>>> + DB 0x66, 0xb8 ; mov eax,
>>>> imm32
>>>>>> ASM_PFX(gSmmCr0): DD 0
>>>>>> DB 0xbf, PROTECT_MODE_DS, 0 ; mov di,
>>>>>> PROTECT_MODE_DS
>>>>>> mov cr0, eax
>>>>>> - DB 0x66, 0xea ; jmp far
>>>>>> [ptr48]
>>>>>> + DB 0x66, 0xea ; jmp far
>>>>>> [ptr48]
>>>>>> ASM_PFX(gSmmJmpAddr):
>>>>>> DD @32bit
>>>>>> DW PROTECT_MODE_CS
>>>>>> @32bit:
>>>>>> mov ds, edi
>>>>>> mov es, edi
>>>>>> --
>>>>>> 2.14.1.3.gb7cf6e02401b
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> edk2-devel mailing list
>>>>>> edk2-devel@lists.01.org
>>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>>
>>>> _______________________________________________
>>>> edk2-devel mailing list
>>>> edk2-devel@lists.01.org
>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
2018-01-31 5:44 ` Ni, Ruiyu
2018-01-31 5:54 ` Ni, Ruiyu
@ 2018-01-31 10:42 ` Laszlo Ersek
1 sibling, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-01-31 10:42 UTC (permalink / raw)
To: Ni, Ruiyu, Kinney, Michael D, edk2-devel-01
Cc: Paolo Bonzini, Yao, Jiewen, Dong, Eric
On 01/31/18 06:44, Ni, Ruiyu wrote:
> Mike, Laszlo,
> Does the patch only apply to the operand?
> If so, PatchAssembly looks too general.
> How about the name like PatchAssemblyOperand?
Originally I meant to call the function "PatchInstruction", but then I
thought it could be used for patching any other artifacts too, in object
code that comes from NASM.
I'm also fine calling it PatchAssemblyOperand, or even
PatchInstructionTrailingOperand :)
Thanks
Laszlo
> On 1/31/2018 6:25 AM, Kinney, Michael D wrote:
>> Laszlo,
>>
>> I agree that the function is better than a macro.
>>
>> I thought of the alignment issues as well. CopyMem()
>> is a good solution. We could also consider
>> WriteUnalignedxx() functions in BaseLib.
>>
>> I was originally thinking this functionality would go
>> into BaseLib. But with the use of CopyMem(), we can't
>> do that. Maybe we should use WriteUnalignedxx() and
>> add some ASSERT() checks.
>>
>> VOID
>> PatchAssembly (
>> VOID *BufferEnd,
>> UINT64 PatchValue,
>> UINTN ValueSize
>> )
>> {
>> ASSERT ((UINTN)BufferEnd > ValueSize);
>> switch (ValueSize) {
>> case 1:
>> ASSERT (PatchValue <= MAX_UINT8);
>> *((UINT8 *)BufferEnd - 1) = (UINT8)PatchValue;
>> case 2:
>> ASSERT (PatchValue <= MAX_UINT16);
>> WriteUnaligned16 ((UINT16 *)(BufferEnd) - 1, (UINT16)PatchValue));
>> break;
>> case 4:
>> ASSERT (PatchValue <= MAX_UINT32);
>> WriteUnaligned32 ((UINT32 *)(BufferEnd) - 1, (UINT32)PatchValue));
>> break;
>> case 8:
>> WriteUnaligned64 ((UINT64 *)(BufferEnd) - 1, PatchValue));
>> break;
>> default:
>> ASSERT (FALSE);
>> }
>> }
>>
>> Mike
>>
>>> -----Original Message-----
>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>> Sent: Tuesday, January 30, 2018 1:45 PM
>>> To: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-
>>> devel-01 <edk2-devel@lists.01.org>
>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini
>>> <pbonzini@redhat.com>; Yao, Jiewen
>>> <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
>>> Subject: Re: [edk2] [PATCH 1/3]
>>> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
>>> SmmStartup()
>>>
>>> On 01/30/18 21:31, Kinney, Michael D wrote:
>>>> Laszlo,
>>>>
>>>> We have already used this technique in other NASM files
>>>> to remove DBs.
>>>
>>> OK.
>>>
>>>> Let us know if you have suggestions on how to make the
>>>> C code that performs the patches easier to read and
>>>> maintain.
>>>
>>> How about this:
>>>
>>> VOID
>>> PatchAssembly (
>>> VOID *BufferEnd,
>>> UINT64 PatchValue,
>>> UINTN ValueSize
>>> )
>>> {
>>> CopyMem (
>>> (VOID *)((UINTN)BufferEnd - ValueSize),
>>> &PatchValue,
>>> ValueSize
>>> );
>>> }
>>>
>>> extern UINT8 gAsmSmmCr0;
>>> extern UINT8 gAsmSmmCr3;
>>> extern UINT8 gAsmSmmCr4;
>>>
>>> ...
>>> {
>>> PatchAssembly (&gAsmSmmCr0, AsmReadCr0 (), 4);
>>> PatchAssembly (&gAsmSmmCr3, AsmReadCr3 (), 4);
>>> PatchAssembly (&gAsmSmmCr4, AsmReadCr4 (), 4);
>>> ...
>>> }
>>>
>>> (I think it's fine to open-code the last argument as "4",
>>> rather than
>>> "sizeof (UINT32)", because for patching, we must have
>>> intimate knowledge
>>> of the instruction anyway.)
>>>
>>> To me, this is easier to read, because:
>>>
>>> - there are no complex casts in the "business logic"
>>> - the size is spelled out once per patching
>>> - the function name and the variable names make it clear
>>> we are patching
>>> separately compiled assembly code that was linked into
>>> the same
>>> module.
>>>
>>> What do you think?
>>>
>>> Thanks!
>>> Laszlo
>>>
>>>>> -----Original Message-----
>>>>> From: edk2-devel [mailto:edk2-devel-
>>> bounces@lists.01.org]
>>>>> On Behalf Of Laszlo Ersek
>>>>> Sent: Tuesday, January 30, 2018 10:17 AM
>>>>> To: Kinney, Michael D <michael.d.kinney@intel.com>;
>>> edk2-
>>>>> devel-01 <edk2-devel@lists.01.org>
>>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini
>>>>> <pbonzini@redhat.com>; Yao, Jiewen
>>>>> <jiewen.yao@intel.com>; Dong, Eric
>>> <eric.dong@intel.com>
>>>>> Subject: Re: [edk2] [PATCH 1/3]
>>>>> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
>>>>> SmmStartup()
>>>>>
>>>>> On 01/30/18 18:22, Kinney, Michael D wrote:
>>>>>> Laszlo,
>>>>>>
>>>>>> The DBs can be removed if the label is moved after
>>>>>> the instruction and the patch is done to the label
>>>>>> minus the size of the patch value.
>>>>>
>>>>> Indeed I haven't thought of this.
>>>>>
>>>>> If I understand correctly, it means
>>>>>
>>>>> extern UINT8 gSmmCr0;
>>>>>
>>>>> *(UINT32*)(&gSmmCr0 - sizeof (UINT32)) =
>>>>> (UINT32)AsmReadCr0 ();
>>>>>
>>>>> TBH, the DB feels less ugly to me than this :)
>>>>>
>>>>> Still, if you think it would be an acceptable price to
>>>>> pay for removing
>>>>> the remaining DBs, I can respin.
>>>>>
>>>>> Thanks
>>>>> Laszlo
>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: edk2-devel [mailto:edk2-devel-
>>>>> bounces@lists.01.org]
>>>>>>> On Behalf Of Laszlo Ersek
>>>>>>> Sent: Tuesday, January 30, 2018 7:34 AM
>>>>>>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>>>>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen
>>>>>>> <jiewen.yao@intel.com>; Dong, Eric
>>>>> <eric.dong@intel.com>;
>>>>>>> Paolo Bonzini <pbonzini@redhat.com>
>>>>>>> Subject: [edk2] [PATCH 1/3]
>>> UefiCpuPkg/PiSmmCpuDxeSmm:
>>>>>>> update comments in IA32 SmmStartup()
>>>>>>>
>>>>>>> The gSmmCr3, gSmmCr4, gSmmCr0 and gSmmJmpAddr global
>>>>>>> variables are used
>>>>>>> for patching assembly instructions, thus we can
>>> never
>>>>>>> remove the DB
>>>>>>> encodings for those instructions. At least we should
>>>>> add
>>>>>>> the intended
>>>>>>> meanings in comments.
>>>>>>>
>>>>>>> This patch only changes comments.
>>>>>>>
>>>>>>> Cc: Eric Dong <eric.dong@intel.com>
>>>>>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>>>>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>>>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>>>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>>>>>> Contributed-under: TianoCore Contribution Agreement
>>>>> 1.1
>>>>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>>>>> ---
>>>>>>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 8
>>> ++++-
>>>>> ---
>>>>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git
>>>>> a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>>>>> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>>>>> index e96dd8d2392a..08534dba64b7 100644
>>>>>>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>>>>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>>>>> @@ -44,34 +44,34 @@ global ASM_PFX(SmmStartup)
>>>>>>> ASM_PFX(SmmStartup):
>>>>>>> DB 0x66
>>>>>>> mov eax, 0x80000001 ; read
>>>>>>> capability
>>>>>>> cpuid
>>>>>>> DB 0x66
>>>>>>> mov ebx, edx ; rdmsr
>>> will
>>>>>>> change edx. keep it in ebx.
>>>>>>> - DB 0x66, 0xb8
>>>>>>> + DB 0x66, 0xb8 ; mov eax,
>>>>> imm32
>>>>>>> ASM_PFX(gSmmCr3): DD 0
>>>>>>> mov cr3, eax
>>>>>>> DB 0x67, 0x66
>>>>>>> lgdt [cs:ebp + (ASM_PFX(gcSmiInitGdtr) -
>>>>>>> ASM_PFX(SmmStartup))]
>>>>>>> - DB 0x66, 0xb8
>>>>>>> + DB 0x66, 0xb8 ; mov eax,
>>>>> imm32
>>>>>>> ASM_PFX(gSmmCr4): DD 0
>>>>>>> mov cr4, eax
>>>>>>> DB 0x66
>>>>>>> mov ecx, 0xc0000080 ; IA32_EFER
>>>>> MSR
>>>>>>> rdmsr
>>>>>>> DB 0x66
>>>>>>> test ebx, BIT20 ; check NXE
>>>>>>> capability
>>>>>>> jz .1
>>>>>>> or ah, BIT3 ; set NXE
>>> bit
>>>>>>> wrmsr
>>>>>>> .1:
>>>>>>> - DB 0x66, 0xb8
>>>>>>> + DB 0x66, 0xb8 ; mov eax,
>>>>> imm32
>>>>>>> ASM_PFX(gSmmCr0): DD 0
>>>>>>> DB 0xbf, PROTECT_MODE_DS, 0 ; mov di,
>>>>>>> PROTECT_MODE_DS
>>>>>>> mov cr0, eax
>>>>>>> - DB 0x66, 0xea ; jmp far
>>>>>>> [ptr48]
>>>>>>> + DB 0x66, 0xea ; jmp far
>>>>>>> [ptr48]
>>>>>>> ASM_PFX(gSmmJmpAddr):
>>>>>>> DD @32bit
>>>>>>> DW PROTECT_MODE_CS
>>>>>>> @32bit:
>>>>>>> mov ds, edi
>>>>>>> mov es, edi
>>>>>>> --
>>>>>>> 2.14.1.3.gb7cf6e02401b
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> edk2-devel mailing list
>>>>>>> edk2-devel@lists.01.org
>>>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>>>
>>>>> _______________________________________________
>>>>> edk2-devel mailing list
>>>>> edk2-devel@lists.01.org
>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
2018-01-31 5:54 ` Ni, Ruiyu
@ 2018-01-31 10:56 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-01-31 10:56 UTC (permalink / raw)
To: Ni, Ruiyu, Kinney, Michael D, edk2-devel-01
Cc: Paolo Bonzini, Yao, Jiewen, Dong, Eric
On 01/31/18 06:54, Ni, Ruiyu wrote:
> Laszlo, Mike,
> Considering this patch doesn't make the code worse,
> actually improved a tiny bit, can we firstly check in the three patches?
I agree; the PatchAssembly() discussion is taking quite a bit of
thought, meanwhile IA32 SMM is broken on KVM -- and even on QEMU! (Paolo
helped me test that, and yes, QEMU/TCG is affected the exact same way.)
I will go ahead and push the patches with the reviews from Paolo and
Ray, with the following small modifications:
* In the commit message of the first patch, I'll change
we can never remove
to
we can't yet remove
in the commit message.
* In the third patch, I will change the commit message from
SMM emulation under KVM
to
SMM emulation under both KVM and QEMU (TCG)
* In the third patch, I will update the code comments as requested by
Ray.
> Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 0/3] UefiCpuPkg/PiSmmCpuDxeSmm: fix IA32 SmmStartup() regression on KVM
2018-01-30 15:33 [PATCH 0/3] UefiCpuPkg/PiSmmCpuDxeSmm: fix IA32 SmmStartup() regression on KVM Laszlo Ersek
` (3 preceding siblings ...)
2018-01-30 16:37 ` [PATCH 0/3] UefiCpuPkg/PiSmmCpuDxeSmm: fix IA32 SmmStartup() regression on KVM Paolo Bonzini
@ 2018-01-31 12:17 ` Laszlo Ersek
2018-02-01 1:20 ` Wang, Jian J
5 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-01-31 12:17 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ruiyu Ni, Jiewen Yao, Eric Dong, Paolo Bonzini
On 01/30/18 16:33, Laszlo Ersek wrote:
> Repo: https://github.com/lersek/edk2.git
> Branch: smm_startup_ia32_nocond
>
> This small series fixes the IA32 SMM regression on KVM that I reported
> recently. The first two patches are cleanups (no functional changes),
> the third patch is the fix.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>
> Thanks
> Laszlo
>
> Laszlo Ersek (3):
> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
> UefiCpuPkg/PiSmmCpuDxeSmm: remove unneeded DBs from IA32 SmmStartup()
> UefiCpuPkg/PiSmmCpuDxeSmm: eliminate conditional jump in IA32
> SmmStartup()
>
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 28 +++++++++-----------
> 1 file changed, 13 insertions(+), 15 deletions(-)
>
Code differences between the posted version and the pushed version,
according to Ray's comments for patch #3, displayed with "git diff
--word-diff" (look for [-...-] and {+...+} markers):
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> index 102e0bdbabc8..d64fcd48d03e 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> @@ -47,8 +47,8 @@ ASM_PFX(SmmStartup):
> mov eax, 0x80000001 ; read capability
> cpuid
> mov ebx, edx ; rdmsr will change edx. keep it in ebx.
> and ebx, BIT20 ; extract [-XD-]{+NX+} capability bit
> shr ebx, 9 ; shift bit to [-IA32_EFER NXE-]{+IA32_EFER.NXE[BIT11]+} position
> DB 0x66, 0xb8 ; mov eax, imm32
> ASM_PFX(gSmmCr3): DD 0
> mov cr3, eax
> @@ -58,7 +58,7 @@ ASM_PFX(gSmmCr4): DD 0
> mov cr4, eax
> mov ecx, 0xc0000080 ; IA32_EFER MSR
> rdmsr
> or eax, ebx ; set NXE bit if [-XD-]{+NX+} is available
> wrmsr
> DB 0x66, 0xb8 ; mov eax, imm32
> ASM_PFX(gSmmCr0): DD 0
Series pushed as d5988a8ac971..8d4d55b15b58.
Let's continue the PatchAssembly() discussion; once we reach an
agreement on that, I'll post patches for that too.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
2018-01-31 10:40 ` Laszlo Ersek
@ 2018-01-31 22:11 ` Kinney, Michael D
2018-02-02 6:05 ` Laszlo Ersek
2018-02-02 10:06 ` Ard Biesheuvel
1 sibling, 1 reply; 27+ messages in thread
From: Kinney, Michael D @ 2018-01-31 22:11 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01, Kinney, Michael D
Cc: Ni, Ruiyu, Paolo Bonzini, Yao, Jiewen, Dong, Eric, Ard Biesheuvel,
Leif Lindholm (Linaro address)
Laszlo,
I agree the Unaligned functions have issues.
We should see if we could change the param type.
It should be a backwards compatible change to
go from a type specific pointer to VOID *. But
need to check with all supported compilers.
We can have arch specific functions and macros.
There are many in BaseLib.h. This way, if a macro
or function is used by an unsupported arch, the
build will fail. I also like some of the name
change suggestions. Maybe PatchInstructionX86()
and change the parameter name to InstructionEnd.
BaseLib.h
==========
#if defined (MDE_CPU_IA32) || defined (MDE_CPU_X64)
VOID
EFIAPI
PatchInstructionX86 (
VOID *InstructionEnd,
UINT64 PatchValue,
UINTN ValueSize
);
#endif
BaseLib Instance
==========
VOID
EFIAPI
PatchInstructionX86 (
VOID *InstructionEnd,
UINT64 PatchValue,
UINTN ValueSize
)
{
ASSERT ((UINTN)InstructionEnd > ValueSize);
switch (ValueSize) {
case 1:
ASSERT (PatchValue <= MAX_UINT8);
*((UINT8 *)InstructionEnd - 1) = (UINT8)PatchValue;
case 2:
ASSERT (PatchValue <= MAX_UINT16);
WriteUnaligned16 ((UINT16 *)(InstructionEnd) - 1, (UINT16)PatchValue));
break;
case 4:
ASSERT (PatchValue <= MAX_UINT32);
WriteUnaligned32 ((UINT32 *)(InstructionEnd) - 1, (UINT32)PatchValue));
break;
case 8:
WriteUnaligned64 ((UINT64 *)(InstructionEnd) - 1, PatchValue));
break;
default:
ASSERT (FALSE);
}
}
Mike
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, January 31, 2018 2:40 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-
> devel-01 <edk2-devel@lists.01.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini
> <pbonzini@redhat.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Ard Biesheuvel <ard.biesheuvel@linaro.org>; Leif Lindholm
> (Linaro address) <leif.lindholm@linaro.org>
> Subject: Re: [edk2] [PATCH 1/3]
> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
> SmmStartup()
>
> On 01/30/18 23:25, Kinney, Michael D wrote:
> > Laszlo,
> >
> > I agree that the function is better than a macro.
> >
> > I thought of the alignment issues as well. CopyMem()
> > is a good solution. We could also consider
> > WriteUnalignedxx() functions in BaseLib.
>
> IMO, the WriteUnalignedxx functions are a bit pointless
> in the exact
> form they are declared (this was discussed earlier esp.
> with regard to
> aarch64). The functions take pointers to objects that
> already have the
> target type, such as
>
> UINT32
> EFIAPI
> WriteUnaligned32 (
> OUT UINT32 *Buffer,
> IN UINT32 Value
> )
>
> Here the type of Buffer should be (VOID *), not (UINT32
> *). Otherwise,
> the undefined behavior (due to mis-alignment) surfaces as
> soon as the
> function is called with an unaligned pointer (i.e. before
> the target
> area is actually written).
>
> > I was originally thinking this functionality would go
> > into BaseLib. But with the use of CopyMem(), we can't
> > do that.
>
> Can we put it in BaseMemoryLib instead (which is where
> CopyMem() is
> from)? That library class is still low-level enough. And,
> while I count
> 9 library instances, PatchAssembly() is not a large
> function, we could
> tolerate adding it to all 9 instances, identically.
>
> Let me also ask the opposite question: should we perhaps
> make the
> PatchAssembly() API *less* abstract? (Also suggested by
> your naming of
> the macro, PATCH_X86_ASM.) If the instruction encoding on
> e.g. AARCH64
> doesn't lend itself to such patching (= expressed through
> the address
> right after the instruction), then even BaseMemoryLib may
> be too generic
> for the API.
>
> > Maybe we should use WriteUnalignedxx() and
> > add some ASSERT() checks.
> >
> > VOID
> > PatchAssembly (
> > VOID *BufferEnd,
> > UINT64 PatchValue,
> > UINTN ValueSize
> > )
> > {
> > ASSERT ((UINTN)BufferEnd > ValueSize);
> > switch (ValueSize) {
> > case 1:
> > ASSERT (PatchValue <= MAX_UINT8);
> > *((UINT8 *)BufferEnd - 1) = (UINT8)PatchValue;
> > case 2:
> > ASSERT (PatchValue <= MAX_UINT16);
> > WriteUnaligned16 ((UINT16 *)(BufferEnd) - 1,
> (UINT16)PatchValue));
> > break;
> > case 4:
> > ASSERT (PatchValue <= MAX_UINT32);
> > WriteUnaligned32 ((UINT32 *)(BufferEnd) - 1,
> (UINT32)PatchValue));
> > break;
> > case 8:
> > WriteUnaligned64 ((UINT64 *)(BufferEnd) - 1,
> PatchValue));
> > break;
> > default:
> > ASSERT (FALSE);
> > }
> > }
>
> In my opinion:
>
> - If Ard and Leif say that PatchAssembly() API makes
> sense for AARCH64,
> then I think we can go with the above generic
> implementation (for
> BaseLib).
>
> - If Ard and Leif say the API is only useful on x86, then
> I suggest that
> we implement the API separately for all arches (still
> in BaseLib):
>
> - On x86, we should simply open-code the unaligned
> accesses (like you
> originall suggested). The pointer arithmetic will
> look a bit wild,
> but it's safely hidden behind a BaseLib API, so
> client code will
> look nice.
>
> - On all other arches, we should implement the function
> with
> ASSERT(FALSE).
>
> Thanks!
> Laszlo
>
> >
> > Mike
> >
> >> -----Original Message-----
> >> From: Laszlo Ersek [mailto:lersek@redhat.com]
> >> Sent: Tuesday, January 30, 2018 1:45 PM
> >> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> edk2-
> >> devel-01 <edk2-devel@lists.01.org>
> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini
> >> <pbonzini@redhat.com>; Yao, Jiewen
> >> <jiewen.yao@intel.com>; Dong, Eric
> <eric.dong@intel.com>
> >> Subject: Re: [edk2] [PATCH 1/3]
> >> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
> >> SmmStartup()
> >>
> >> On 01/30/18 21:31, Kinney, Michael D wrote:
> >>> Laszlo,
> >>>
> >>> We have already used this technique in other NASM
> files
> >>> to remove DBs.
> >>
> >> OK.
> >>
> >>> Let us know if you have suggestions on how to make
> the
> >>> C code that performs the patches easier to read and
> >>> maintain.
> >>
> >> How about this:
> >>
> >> VOID
> >> PatchAssembly (
> >> VOID *BufferEnd,
> >> UINT64 PatchValue,
> >> UINTN ValueSize
> >> )
> >> {
> >> CopyMem (
> >> (VOID *)((UINTN)BufferEnd - ValueSize),
> >> &PatchValue,
> >> ValueSize
> >> );
> >> }
> >>
> >> extern UINT8 gAsmSmmCr0;
> >> extern UINT8 gAsmSmmCr3;
> >> extern UINT8 gAsmSmmCr4;
> >>
> >> ...
> >> {
> >> PatchAssembly (&gAsmSmmCr0, AsmReadCr0 (), 4);
> >> PatchAssembly (&gAsmSmmCr3, AsmReadCr3 (), 4);
> >> PatchAssembly (&gAsmSmmCr4, AsmReadCr4 (), 4);
> >> ...
> >> }
> >>
> >> (I think it's fine to open-code the last argument as
> "4",
> >> rather than
> >> "sizeof (UINT32)", because for patching, we must have
> >> intimate knowledge
> >> of the instruction anyway.)
> >>
> >> To me, this is easier to read, because:
> >>
> >> - there are no complex casts in the "business logic"
> >> - the size is spelled out once per patching
> >> - the function name and the variable names make it
> clear
> >> we are patching
> >> separately compiled assembly code that was linked
> into
> >> the same
> >> module.
> >>
> >> What do you think?
> >>
> >> Thanks!
> >> Laszlo
> >>
> >>>> -----Original Message-----
> >>>> From: edk2-devel [mailto:edk2-devel-
> >> bounces@lists.01.org]
> >>>> On Behalf Of Laszlo Ersek
> >>>> Sent: Tuesday, January 30, 2018 10:17 AM
> >>>> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> >> edk2-
> >>>> devel-01 <edk2-devel@lists.01.org>
> >>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini
> >>>> <pbonzini@redhat.com>; Yao, Jiewen
> >>>> <jiewen.yao@intel.com>; Dong, Eric
> >> <eric.dong@intel.com>
> >>>> Subject: Re: [edk2] [PATCH 1/3]
> >>>> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
> >>>> SmmStartup()
> >>>>
> >>>> On 01/30/18 18:22, Kinney, Michael D wrote:
> >>>>> Laszlo,
> >>>>>
> >>>>> The DBs can be removed if the label is moved after
> >>>>> the instruction and the patch is done to the label
> >>>>> minus the size of the patch value.
> >>>>
> >>>> Indeed I haven't thought of this.
> >>>>
> >>>> If I understand correctly, it means
> >>>>
> >>>> extern UINT8 gSmmCr0;
> >>>>
> >>>> *(UINT32*)(&gSmmCr0 - sizeof (UINT32)) =
> >>>> (UINT32)AsmReadCr0 ();
> >>>>
> >>>> TBH, the DB feels less ugly to me than this :)
> >>>>
> >>>> Still, if you think it would be an acceptable price
> to
> >>>> pay for removing
> >>>> the remaining DBs, I can respin.
> >>>>
> >>>> Thanks
> >>>> Laszlo
> >>>>
> >>>>>> -----Original Message-----
> >>>>>> From: edk2-devel [mailto:edk2-devel-
> >>>> bounces@lists.01.org]
> >>>>>> On Behalf Of Laszlo Ersek
> >>>>>> Sent: Tuesday, January 30, 2018 7:34 AM
> >>>>>> To: edk2-devel-01 <edk2-devel@lists.01.org>
> >>>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen
> >>>>>> <jiewen.yao@intel.com>; Dong, Eric
> >>>> <eric.dong@intel.com>;
> >>>>>> Paolo Bonzini <pbonzini@redhat.com>
> >>>>>> Subject: [edk2] [PATCH 1/3]
> >> UefiCpuPkg/PiSmmCpuDxeSmm:
> >>>>>> update comments in IA32 SmmStartup()
> >>>>>>
> >>>>>> The gSmmCr3, gSmmCr4, gSmmCr0 and gSmmJmpAddr
> global
> >>>>>> variables are used
> >>>>>> for patching assembly instructions, thus we can
> >> never
> >>>>>> remove the DB
> >>>>>> encodings for those instructions. At least we
> should
> >>>> add
> >>>>>> the intended
> >>>>>> meanings in comments.
> >>>>>>
> >>>>>> This patch only changes comments.
> >>>>>>
> >>>>>> Cc: Eric Dong <eric.dong@intel.com>
> >>>>>> Cc: Jian J Wang <jian.j.wang@intel.com>
> >>>>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
> >>>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >>>>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> >>>>>> Contributed-under: TianoCore Contribution
> Agreement
> >>>> 1.1
> >>>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> >>>>>> ---
> >>>>>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 8
> >> ++++-
> >>>> ---
> >>>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
> >>>>>>
> >>>>>> diff --git
> >>>> a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> >>>>>> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> >>>>>> index e96dd8d2392a..08534dba64b7 100644
> >>>>>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> >>>>>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> >>>>>> @@ -44,34 +44,34 @@ global ASM_PFX(SmmStartup)
> >>>>>> ASM_PFX(SmmStartup):
> >>>>>> DB 0x66
> >>>>>> mov eax, 0x80000001 ; read
> >>>>>> capability
> >>>>>> cpuid
> >>>>>> DB 0x66
> >>>>>> mov ebx, edx ; rdmsr
> >> will
> >>>>>> change edx. keep it in ebx.
> >>>>>> - DB 0x66, 0xb8
> >>>>>> + DB 0x66, 0xb8 ; mov
> eax,
> >>>> imm32
> >>>>>> ASM_PFX(gSmmCr3): DD 0
> >>>>>> mov cr3, eax
> >>>>>> DB 0x67, 0x66
> >>>>>> lgdt [cs:ebp + (ASM_PFX(gcSmiInitGdtr) -
> >>>>>> ASM_PFX(SmmStartup))]
> >>>>>> - DB 0x66, 0xb8
> >>>>>> + DB 0x66, 0xb8 ; mov
> eax,
> >>>> imm32
> >>>>>> ASM_PFX(gSmmCr4): DD 0
> >>>>>> mov cr4, eax
> >>>>>> DB 0x66
> >>>>>> mov ecx, 0xc0000080 ;
> IA32_EFER
> >>>> MSR
> >>>>>> rdmsr
> >>>>>> DB 0x66
> >>>>>> test ebx, BIT20 ; check
> NXE
> >>>>>> capability
> >>>>>> jz .1
> >>>>>> or ah, BIT3 ; set NXE
> >> bit
> >>>>>> wrmsr
> >>>>>> .1:
> >>>>>> - DB 0x66, 0xb8
> >>>>>> + DB 0x66, 0xb8 ; mov
> eax,
> >>>> imm32
> >>>>>> ASM_PFX(gSmmCr0): DD 0
> >>>>>> DB 0xbf, PROTECT_MODE_DS, 0 ; mov di,
> >>>>>> PROTECT_MODE_DS
> >>>>>> mov cr0, eax
> >>>>>> - DB 0x66, 0xea ; jmp
> far
> >>>>>> [ptr48]
> >>>>>> + DB 0x66, 0xea ; jmp far
> >>>>>> [ptr48]
> >>>>>> ASM_PFX(gSmmJmpAddr):
> >>>>>> DD @32bit
> >>>>>> DW PROTECT_MODE_CS
> >>>>>> @32bit:
> >>>>>> mov ds, edi
> >>>>>> mov es, edi
> >>>>>> --
> >>>>>> 2.14.1.3.gb7cf6e02401b
> >>>>>>
> >>>>>>
> >>>>>> _______________________________________________
> >>>>>> edk2-devel mailing list
> >>>>>> edk2-devel@lists.01.org
> >>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
> >>>>
> >>>> _______________________________________________
> >>>> edk2-devel mailing list
> >>>> edk2-devel@lists.01.org
> >>>> https://lists.01.org/mailman/listinfo/edk2-devel
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> >
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 0/3] UefiCpuPkg/PiSmmCpuDxeSmm: fix IA32 SmmStartup() regression on KVM
2018-01-30 15:33 [PATCH 0/3] UefiCpuPkg/PiSmmCpuDxeSmm: fix IA32 SmmStartup() regression on KVM Laszlo Ersek
` (4 preceding siblings ...)
2018-01-31 12:17 ` Laszlo Ersek
@ 2018-02-01 1:20 ` Wang, Jian J
5 siblings, 0 replies; 27+ messages in thread
From: Wang, Jian J @ 2018-02-01 1:20 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
Cc: Dong, Eric, Yao, Jiewen, Paolo Bonzini, Ni, Ruiyu
Hi Laszlo,
Thank you very much for fixing this issue.
Regards,
Jian
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, January 30, 2018 11:34 PM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Dong, Eric <eric.dong@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> Yao, Jiewen <jiewen.yao@intel.com>; Paolo Bonzini <pbonzini@redhat.com>;
> Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: [PATCH 0/3] UefiCpuPkg/PiSmmCpuDxeSmm: fix IA32 SmmStartup()
> regression on KVM
>
> Repo: https://github.com/lersek/edk2.git
> Branch: smm_startup_ia32_nocond
>
> This small series fixes the IA32 SMM regression on KVM that I reported
> recently. The first two patches are cleanups (no functional changes),
> the third patch is the fix.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>
> Thanks
> Laszlo
>
> Laszlo Ersek (3):
> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
> UefiCpuPkg/PiSmmCpuDxeSmm: remove unneeded DBs from IA32
> SmmStartup()
> UefiCpuPkg/PiSmmCpuDxeSmm: eliminate conditional jump in IA32
> SmmStartup()
>
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 28 +++++++++-----------
> 1 file changed, 13 insertions(+), 15 deletions(-)
>
> --
> 2.14.1.3.gb7cf6e02401b
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
2018-01-31 22:11 ` Kinney, Michael D
@ 2018-02-02 6:05 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-02-02 6:05 UTC (permalink / raw)
To: Kinney, Michael D, edk2-devel-01
Cc: Ni, Ruiyu, Dong, Eric, Ard Biesheuvel,
Leif Lindholm (Linaro address), Yao, Jiewen, Paolo Bonzini
On 01/31/18 23:11, Kinney, Michael D wrote:
> Laszlo,
>
> I agree the Unaligned functions have issues.
> We should see if we could change the param type.
> It should be a backwards compatible change to
> go from a type specific pointer to VOID *. But
> need to check with all supported compilers.
>
> We can have arch specific functions and macros.
> There are many in BaseLib.h. This way, if a macro
> or function is used by an unsupported arch, the
> build will fail. I also like some of the name
> change suggestions. Maybe PatchInstructionX86()
> and change the parameter name to InstructionEnd.
>
> BaseLib.h
> ==========
> #if defined (MDE_CPU_IA32) || defined (MDE_CPU_X64)
>
> VOID
> EFIAPI
> PatchInstructionX86 (
> VOID *InstructionEnd,
> UINT64 PatchValue,
> UINTN ValueSize
> );
>
> #endif
>
> BaseLib Instance
> ==========
> VOID
> EFIAPI
> PatchInstructionX86 (
> VOID *InstructionEnd,
> UINT64 PatchValue,
> UINTN ValueSize
> )
> {
> ASSERT ((UINTN)InstructionEnd > ValueSize);
> switch (ValueSize) {
> case 1:
> ASSERT (PatchValue <= MAX_UINT8);
> *((UINT8 *)InstructionEnd - 1) = (UINT8)PatchValue;
> case 2:
> ASSERT (PatchValue <= MAX_UINT16);
> WriteUnaligned16 ((UINT16 *)(InstructionEnd) - 1, (UINT16)PatchValue));
> break;
> case 4:
> ASSERT (PatchValue <= MAX_UINT32);
> WriteUnaligned32 ((UINT32 *)(InstructionEnd) - 1, (UINT32)PatchValue));
> break;
> case 8:
> WriteUnaligned64 ((UINT64 *)(InstructionEnd) - 1, PatchValue));
> break;
> default:
> ASSERT (FALSE);
> }
> }
I managed to remove all instruction DBs from PiSmmCpuDxeSmm. I plan to
post the patches this week or the next.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
2018-01-31 10:40 ` Laszlo Ersek
2018-01-31 22:11 ` Kinney, Michael D
@ 2018-02-02 10:06 ` Ard Biesheuvel
2018-02-02 13:26 ` Laszlo Ersek
2018-02-02 13:28 ` Leif Lindholm
1 sibling, 2 replies; 27+ messages in thread
From: Ard Biesheuvel @ 2018-02-02 10:06 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Kinney, Michael D, edk2-devel-01, Ni, Ruiyu, Paolo Bonzini,
Yao, Jiewen, Dong, Eric, Leif Lindholm (Linaro address)
On 31 January 2018 at 10:40, Laszlo Ersek <lersek@redhat.com> wrote:
> On 01/30/18 23:25, Kinney, Michael D wrote:
>> Laszlo,
>>
>> I agree that the function is better than a macro.
>>
>> I thought of the alignment issues as well. CopyMem()
>> is a good solution. We could also consider
>> WriteUnalignedxx() functions in BaseLib.
>
> IMO, the WriteUnalignedxx functions are a bit pointless in the exact
> form they are declared (this was discussed earlier esp. with regard to
> aarch64). The functions take pointers to objects that already have the
> target type, such as
>
> UINT32
> EFIAPI
> WriteUnaligned32 (
> OUT UINT32 *Buffer,
> IN UINT32 Value
> )
>
> Here the type of Buffer should be (VOID *), not (UINT32 *). Otherwise,
> the undefined behavior (due to mis-alignment) surfaces as soon as the
> function is called with an unaligned pointer (i.e. before the target
> area is actually written).
>
>> I was originally thinking this functionality would go
>> into BaseLib. But with the use of CopyMem(), we can't
>> do that.
>
> Can we put it in BaseMemoryLib instead (which is where CopyMem() is
> from)? That library class is still low-level enough. And, while I count
> 9 library instances, PatchAssembly() is not a large function, we could
> tolerate adding it to all 9 instances, identically.
>
> Let me also ask the opposite question: should we perhaps make the
> PatchAssembly() API *less* abstract? (Also suggested by your naming of
> the macro, PATCH_X86_ASM.) If the instruction encoding on e.g. AARCH64
> doesn't lend itself to such patching (= expressed through the address
> right after the instruction), then even BaseMemoryLib may be too generic
> for the API.
>
>> Maybe we should use WriteUnalignedxx() and
>> add some ASSERT() checks.
>>
>> VOID
>> PatchAssembly (
>> VOID *BufferEnd,
>> UINT64 PatchValue,
>> UINTN ValueSize
>> )
>> {
>> ASSERT ((UINTN)BufferEnd > ValueSize);
>> switch (ValueSize) {
>> case 1:
>> ASSERT (PatchValue <= MAX_UINT8);
>> *((UINT8 *)BufferEnd - 1) = (UINT8)PatchValue;
>> case 2:
>> ASSERT (PatchValue <= MAX_UINT16);
>> WriteUnaligned16 ((UINT16 *)(BufferEnd) - 1, (UINT16)PatchValue));
>> break;
>> case 4:
>> ASSERT (PatchValue <= MAX_UINT32);
>> WriteUnaligned32 ((UINT32 *)(BufferEnd) - 1, (UINT32)PatchValue));
>> break;
>> case 8:
>> WriteUnaligned64 ((UINT64 *)(BufferEnd) - 1, PatchValue));
>> break;
>> default:
>> ASSERT (FALSE);
>> }
>> }
>
> In my opinion:
>
> - If Ard and Leif say that PatchAssembly() API makes sense for AARCH64,
> then I think we can go with the above generic implementation (for
> BaseLib).
>
Code patching on ARM/AARCH64 has some hoops to jump through, i.e.,
clean the D-cache to the point of unification, invalidate the I-cache,
probably some barriers in case the patching function happened to end
up in the same cache line as the patchee (which may not be a concern
for this specific use case, but it does need to be taken into account
if this is turned into a patch-any-assembly-anywhere function)
So if the PatchAssembly() prototype does end up in a generic library
class, we'd have to provide ARM and AARCH64 specific implementations
anyway, and given that I don't see any use for this on ARM/AARCH64 in
the first place, I think this should belong in an IA32/X64 specific
package.
> - If Ard and Leif say the API is only useful on x86, then I suggest that
> we implement the API separately for all arches (still in BaseLib):
>
> - On x86, we should simply open-code the unaligned accesses (like you
> originall suggested). The pointer arithmetic will look a bit wild,
> but it's safely hidden behind a BaseLib API, so client code will
> look nice.
>
> - On all other arches, we should implement the function with
> ASSERT(FALSE).
>
> Thanks!
> Laszlo
>
>>
>> Mike
>>
>>> -----Original Message-----
>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>> Sent: Tuesday, January 30, 2018 1:45 PM
>>> To: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-
>>> devel-01 <edk2-devel@lists.01.org>
>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini
>>> <pbonzini@redhat.com>; Yao, Jiewen
>>> <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
>>> Subject: Re: [edk2] [PATCH 1/3]
>>> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
>>> SmmStartup()
>>>
>>> On 01/30/18 21:31, Kinney, Michael D wrote:
>>>> Laszlo,
>>>>
>>>> We have already used this technique in other NASM files
>>>> to remove DBs.
>>>
>>> OK.
>>>
>>>> Let us know if you have suggestions on how to make the
>>>> C code that performs the patches easier to read and
>>>> maintain.
>>>
>>> How about this:
>>>
>>> VOID
>>> PatchAssembly (
>>> VOID *BufferEnd,
>>> UINT64 PatchValue,
>>> UINTN ValueSize
>>> )
>>> {
>>> CopyMem (
>>> (VOID *)((UINTN)BufferEnd - ValueSize),
>>> &PatchValue,
>>> ValueSize
>>> );
>>> }
>>>
>>> extern UINT8 gAsmSmmCr0;
>>> extern UINT8 gAsmSmmCr3;
>>> extern UINT8 gAsmSmmCr4;
>>>
>>> ...
>>> {
>>> PatchAssembly (&gAsmSmmCr0, AsmReadCr0 (), 4);
>>> PatchAssembly (&gAsmSmmCr3, AsmReadCr3 (), 4);
>>> PatchAssembly (&gAsmSmmCr4, AsmReadCr4 (), 4);
>>> ...
>>> }
>>>
>>> (I think it's fine to open-code the last argument as "4",
>>> rather than
>>> "sizeof (UINT32)", because for patching, we must have
>>> intimate knowledge
>>> of the instruction anyway.)
>>>
>>> To me, this is easier to read, because:
>>>
>>> - there are no complex casts in the "business logic"
>>> - the size is spelled out once per patching
>>> - the function name and the variable names make it clear
>>> we are patching
>>> separately compiled assembly code that was linked into
>>> the same
>>> module.
>>>
>>> What do you think?
>>>
>>> Thanks!
>>> Laszlo
>>>
>>>>> -----Original Message-----
>>>>> From: edk2-devel [mailto:edk2-devel-
>>> bounces@lists.01.org]
>>>>> On Behalf Of Laszlo Ersek
>>>>> Sent: Tuesday, January 30, 2018 10:17 AM
>>>>> To: Kinney, Michael D <michael.d.kinney@intel.com>;
>>> edk2-
>>>>> devel-01 <edk2-devel@lists.01.org>
>>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini
>>>>> <pbonzini@redhat.com>; Yao, Jiewen
>>>>> <jiewen.yao@intel.com>; Dong, Eric
>>> <eric.dong@intel.com>
>>>>> Subject: Re: [edk2] [PATCH 1/3]
>>>>> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
>>>>> SmmStartup()
>>>>>
>>>>> On 01/30/18 18:22, Kinney, Michael D wrote:
>>>>>> Laszlo,
>>>>>>
>>>>>> The DBs can be removed if the label is moved after
>>>>>> the instruction and the patch is done to the label
>>>>>> minus the size of the patch value.
>>>>>
>>>>> Indeed I haven't thought of this.
>>>>>
>>>>> If I understand correctly, it means
>>>>>
>>>>> extern UINT8 gSmmCr0;
>>>>>
>>>>> *(UINT32*)(&gSmmCr0 - sizeof (UINT32)) =
>>>>> (UINT32)AsmReadCr0 ();
>>>>>
>>>>> TBH, the DB feels less ugly to me than this :)
>>>>>
>>>>> Still, if you think it would be an acceptable price to
>>>>> pay for removing
>>>>> the remaining DBs, I can respin.
>>>>>
>>>>> Thanks
>>>>> Laszlo
>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: edk2-devel [mailto:edk2-devel-
>>>>> bounces@lists.01.org]
>>>>>>> On Behalf Of Laszlo Ersek
>>>>>>> Sent: Tuesday, January 30, 2018 7:34 AM
>>>>>>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>>>>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen
>>>>>>> <jiewen.yao@intel.com>; Dong, Eric
>>>>> <eric.dong@intel.com>;
>>>>>>> Paolo Bonzini <pbonzini@redhat.com>
>>>>>>> Subject: [edk2] [PATCH 1/3]
>>> UefiCpuPkg/PiSmmCpuDxeSmm:
>>>>>>> update comments in IA32 SmmStartup()
>>>>>>>
>>>>>>> The gSmmCr3, gSmmCr4, gSmmCr0 and gSmmJmpAddr global
>>>>>>> variables are used
>>>>>>> for patching assembly instructions, thus we can
>>> never
>>>>>>> remove the DB
>>>>>>> encodings for those instructions. At least we should
>>>>> add
>>>>>>> the intended
>>>>>>> meanings in comments.
>>>>>>>
>>>>>>> This patch only changes comments.
>>>>>>>
>>>>>>> Cc: Eric Dong <eric.dong@intel.com>
>>>>>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>>>>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>>>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>>>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>>>>>> Contributed-under: TianoCore Contribution Agreement
>>>>> 1.1
>>>>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>>>>> ---
>>>>>>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 8
>>> ++++-
>>>>> ---
>>>>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git
>>>>> a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>>>>> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>>>>> index e96dd8d2392a..08534dba64b7 100644
>>>>>>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>>>>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>>>>> @@ -44,34 +44,34 @@ global ASM_PFX(SmmStartup)
>>>>>>> ASM_PFX(SmmStartup):
>>>>>>> DB 0x66
>>>>>>> mov eax, 0x80000001 ; read
>>>>>>> capability
>>>>>>> cpuid
>>>>>>> DB 0x66
>>>>>>> mov ebx, edx ; rdmsr
>>> will
>>>>>>> change edx. keep it in ebx.
>>>>>>> - DB 0x66, 0xb8
>>>>>>> + DB 0x66, 0xb8 ; mov eax,
>>>>> imm32
>>>>>>> ASM_PFX(gSmmCr3): DD 0
>>>>>>> mov cr3, eax
>>>>>>> DB 0x67, 0x66
>>>>>>> lgdt [cs:ebp + (ASM_PFX(gcSmiInitGdtr) -
>>>>>>> ASM_PFX(SmmStartup))]
>>>>>>> - DB 0x66, 0xb8
>>>>>>> + DB 0x66, 0xb8 ; mov eax,
>>>>> imm32
>>>>>>> ASM_PFX(gSmmCr4): DD 0
>>>>>>> mov cr4, eax
>>>>>>> DB 0x66
>>>>>>> mov ecx, 0xc0000080 ; IA32_EFER
>>>>> MSR
>>>>>>> rdmsr
>>>>>>> DB 0x66
>>>>>>> test ebx, BIT20 ; check NXE
>>>>>>> capability
>>>>>>> jz .1
>>>>>>> or ah, BIT3 ; set NXE
>>> bit
>>>>>>> wrmsr
>>>>>>> .1:
>>>>>>> - DB 0x66, 0xb8
>>>>>>> + DB 0x66, 0xb8 ; mov eax,
>>>>> imm32
>>>>>>> ASM_PFX(gSmmCr0): DD 0
>>>>>>> DB 0xbf, PROTECT_MODE_DS, 0 ; mov di,
>>>>>>> PROTECT_MODE_DS
>>>>>>> mov cr0, eax
>>>>>>> - DB 0x66, 0xea ; jmp far
>>>>>>> [ptr48]
>>>>>>> + DB 0x66, 0xea ; jmp far
>>>>>>> [ptr48]
>>>>>>> ASM_PFX(gSmmJmpAddr):
>>>>>>> DD @32bit
>>>>>>> DW PROTECT_MODE_CS
>>>>>>> @32bit:
>>>>>>> mov ds, edi
>>>>>>> mov es, edi
>>>>>>> --
>>>>>>> 2.14.1.3.gb7cf6e02401b
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> edk2-devel mailing list
>>>>>>> edk2-devel@lists.01.org
>>>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>>>
>>>>> _______________________________________________
>>>>> edk2-devel mailing list
>>>>> edk2-devel@lists.01.org
>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
2018-02-02 10:06 ` Ard Biesheuvel
@ 2018-02-02 13:26 ` Laszlo Ersek
2018-02-02 13:28 ` Leif Lindholm
1 sibling, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-02-02 13:26 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Kinney, Michael D, edk2-devel-01, Ni, Ruiyu, Paolo Bonzini,
Yao, Jiewen, Dong, Eric, Leif Lindholm (Linaro address)
On 02/02/18 11:06, Ard Biesheuvel wrote:
> On 31 January 2018 at 10:40, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 01/30/18 23:25, Kinney, Michael D wrote:
>>> Laszlo,
>>>
>>> I agree that the function is better than a macro.
>>>
>>> I thought of the alignment issues as well. CopyMem()
>>> is a good solution. We could also consider
>>> WriteUnalignedxx() functions in BaseLib.
>>
>> IMO, the WriteUnalignedxx functions are a bit pointless in the exact
>> form they are declared (this was discussed earlier esp. with regard to
>> aarch64). The functions take pointers to objects that already have the
>> target type, such as
>>
>> UINT32
>> EFIAPI
>> WriteUnaligned32 (
>> OUT UINT32 *Buffer,
>> IN UINT32 Value
>> )
>>
>> Here the type of Buffer should be (VOID *), not (UINT32 *). Otherwise,
>> the undefined behavior (due to mis-alignment) surfaces as soon as the
>> function is called with an unaligned pointer (i.e. before the target
>> area is actually written).
>>
>>> I was originally thinking this functionality would go
>>> into BaseLib. But with the use of CopyMem(), we can't
>>> do that.
>>
>> Can we put it in BaseMemoryLib instead (which is where CopyMem() is
>> from)? That library class is still low-level enough. And, while I count
>> 9 library instances, PatchAssembly() is not a large function, we could
>> tolerate adding it to all 9 instances, identically.
>>
>> Let me also ask the opposite question: should we perhaps make the
>> PatchAssembly() API *less* abstract? (Also suggested by your naming of
>> the macro, PATCH_X86_ASM.) If the instruction encoding on e.g. AARCH64
>> doesn't lend itself to such patching (= expressed through the address
>> right after the instruction), then even BaseMemoryLib may be too generic
>> for the API.
>>
>>> Maybe we should use WriteUnalignedxx() and
>>> add some ASSERT() checks.
>>>
>>> VOID
>>> PatchAssembly (
>>> VOID *BufferEnd,
>>> UINT64 PatchValue,
>>> UINTN ValueSize
>>> )
>>> {
>>> ASSERT ((UINTN)BufferEnd > ValueSize);
>>> switch (ValueSize) {
>>> case 1:
>>> ASSERT (PatchValue <= MAX_UINT8);
>>> *((UINT8 *)BufferEnd - 1) = (UINT8)PatchValue;
>>> case 2:
>>> ASSERT (PatchValue <= MAX_UINT16);
>>> WriteUnaligned16 ((UINT16 *)(BufferEnd) - 1, (UINT16)PatchValue));
>>> break;
>>> case 4:
>>> ASSERT (PatchValue <= MAX_UINT32);
>>> WriteUnaligned32 ((UINT32 *)(BufferEnd) - 1, (UINT32)PatchValue));
>>> break;
>>> case 8:
>>> WriteUnaligned64 ((UINT64 *)(BufferEnd) - 1, PatchValue));
>>> break;
>>> default:
>>> ASSERT (FALSE);
>>> }
>>> }
>>
>> In my opinion:
>>
>> - If Ard and Leif say that PatchAssembly() API makes sense for AARCH64,
>> then I think we can go with the above generic implementation (for
>> BaseLib).
>>
>
> Code patching on ARM/AARCH64 has some hoops to jump through, i.e.,
> clean the D-cache to the point of unification, invalidate the I-cache,
> probably some barriers in case the patching function happened to end
> up in the same cache line as the patchee (which may not be a concern
> for this specific use case, but it does need to be taken into account
> if this is turned into a patch-any-assembly-anywhere function)
>
> So if the PatchAssembly() prototype does end up in a generic library
> class, we'd have to provide ARM and AARCH64 specific implementations
> anyway, and given that I don't see any use for this on ARM/AARCH64 in
> the first place, I think this should belong in an IA32/X64 specific
> package.
Thank you for the response!
For now I'm going to post the series with the function introduced as
PatchInstructionX86() to BaseLib, visibly only to IA32 and X64 edk2
platforms. If a better place than BaseLib looks necessary, I can move it
according to review comments.
Thanks!
Laszlo
>
>> - If Ard and Leif say the API is only useful on x86, then I suggest that
>> we implement the API separately for all arches (still in BaseLib):
>>
>> - On x86, we should simply open-code the unaligned accesses (like you
>> originall suggested). The pointer arithmetic will look a bit wild,
>> but it's safely hidden behind a BaseLib API, so client code will
>> look nice.
>>
>> - On all other arches, we should implement the function with
>> ASSERT(FALSE).
>>
>> Thanks!
>> Laszlo
>>
>>>
>>> Mike
>>>
>>>> -----Original Message-----
>>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>>> Sent: Tuesday, January 30, 2018 1:45 PM
>>>> To: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-
>>>> devel-01 <edk2-devel@lists.01.org>
>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini
>>>> <pbonzini@redhat.com>; Yao, Jiewen
>>>> <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
>>>> Subject: Re: [edk2] [PATCH 1/3]
>>>> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
>>>> SmmStartup()
>>>>
>>>> On 01/30/18 21:31, Kinney, Michael D wrote:
>>>>> Laszlo,
>>>>>
>>>>> We have already used this technique in other NASM files
>>>>> to remove DBs.
>>>>
>>>> OK.
>>>>
>>>>> Let us know if you have suggestions on how to make the
>>>>> C code that performs the patches easier to read and
>>>>> maintain.
>>>>
>>>> How about this:
>>>>
>>>> VOID
>>>> PatchAssembly (
>>>> VOID *BufferEnd,
>>>> UINT64 PatchValue,
>>>> UINTN ValueSize
>>>> )
>>>> {
>>>> CopyMem (
>>>> (VOID *)((UINTN)BufferEnd - ValueSize),
>>>> &PatchValue,
>>>> ValueSize
>>>> );
>>>> }
>>>>
>>>> extern UINT8 gAsmSmmCr0;
>>>> extern UINT8 gAsmSmmCr3;
>>>> extern UINT8 gAsmSmmCr4;
>>>>
>>>> ...
>>>> {
>>>> PatchAssembly (&gAsmSmmCr0, AsmReadCr0 (), 4);
>>>> PatchAssembly (&gAsmSmmCr3, AsmReadCr3 (), 4);
>>>> PatchAssembly (&gAsmSmmCr4, AsmReadCr4 (), 4);
>>>> ...
>>>> }
>>>>
>>>> (I think it's fine to open-code the last argument as "4",
>>>> rather than
>>>> "sizeof (UINT32)", because for patching, we must have
>>>> intimate knowledge
>>>> of the instruction anyway.)
>>>>
>>>> To me, this is easier to read, because:
>>>>
>>>> - there are no complex casts in the "business logic"
>>>> - the size is spelled out once per patching
>>>> - the function name and the variable names make it clear
>>>> we are patching
>>>> separately compiled assembly code that was linked into
>>>> the same
>>>> module.
>>>>
>>>> What do you think?
>>>>
>>>> Thanks!
>>>> Laszlo
>>>>
>>>>>> -----Original Message-----
>>>>>> From: edk2-devel [mailto:edk2-devel-
>>>> bounces@lists.01.org]
>>>>>> On Behalf Of Laszlo Ersek
>>>>>> Sent: Tuesday, January 30, 2018 10:17 AM
>>>>>> To: Kinney, Michael D <michael.d.kinney@intel.com>;
>>>> edk2-
>>>>>> devel-01 <edk2-devel@lists.01.org>
>>>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini
>>>>>> <pbonzini@redhat.com>; Yao, Jiewen
>>>>>> <jiewen.yao@intel.com>; Dong, Eric
>>>> <eric.dong@intel.com>
>>>>>> Subject: Re: [edk2] [PATCH 1/3]
>>>>>> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
>>>>>> SmmStartup()
>>>>>>
>>>>>> On 01/30/18 18:22, Kinney, Michael D wrote:
>>>>>>> Laszlo,
>>>>>>>
>>>>>>> The DBs can be removed if the label is moved after
>>>>>>> the instruction and the patch is done to the label
>>>>>>> minus the size of the patch value.
>>>>>>
>>>>>> Indeed I haven't thought of this.
>>>>>>
>>>>>> If I understand correctly, it means
>>>>>>
>>>>>> extern UINT8 gSmmCr0;
>>>>>>
>>>>>> *(UINT32*)(&gSmmCr0 - sizeof (UINT32)) =
>>>>>> (UINT32)AsmReadCr0 ();
>>>>>>
>>>>>> TBH, the DB feels less ugly to me than this :)
>>>>>>
>>>>>> Still, if you think it would be an acceptable price to
>>>>>> pay for removing
>>>>>> the remaining DBs, I can respin.
>>>>>>
>>>>>> Thanks
>>>>>> Laszlo
>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: edk2-devel [mailto:edk2-devel-
>>>>>> bounces@lists.01.org]
>>>>>>>> On Behalf Of Laszlo Ersek
>>>>>>>> Sent: Tuesday, January 30, 2018 7:34 AM
>>>>>>>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>>>>>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen
>>>>>>>> <jiewen.yao@intel.com>; Dong, Eric
>>>>>> <eric.dong@intel.com>;
>>>>>>>> Paolo Bonzini <pbonzini@redhat.com>
>>>>>>>> Subject: [edk2] [PATCH 1/3]
>>>> UefiCpuPkg/PiSmmCpuDxeSmm:
>>>>>>>> update comments in IA32 SmmStartup()
>>>>>>>>
>>>>>>>> The gSmmCr3, gSmmCr4, gSmmCr0 and gSmmJmpAddr global
>>>>>>>> variables are used
>>>>>>>> for patching assembly instructions, thus we can
>>>> never
>>>>>>>> remove the DB
>>>>>>>> encodings for those instructions. At least we should
>>>>>> add
>>>>>>>> the intended
>>>>>>>> meanings in comments.
>>>>>>>>
>>>>>>>> This patch only changes comments.
>>>>>>>>
>>>>>>>> Cc: Eric Dong <eric.dong@intel.com>
>>>>>>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>>>>>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>>>>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>>>>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>>>>>>> Contributed-under: TianoCore Contribution Agreement
>>>>>> 1.1
>>>>>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>>>>>> ---
>>>>>>>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 8
>>>> ++++-
>>>>>> ---
>>>>>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git
>>>>>> a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>>>>>> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>>>>>> index e96dd8d2392a..08534dba64b7 100644
>>>>>>>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>>>>>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
>>>>>>>> @@ -44,34 +44,34 @@ global ASM_PFX(SmmStartup)
>>>>>>>> ASM_PFX(SmmStartup):
>>>>>>>> DB 0x66
>>>>>>>> mov eax, 0x80000001 ; read
>>>>>>>> capability
>>>>>>>> cpuid
>>>>>>>> DB 0x66
>>>>>>>> mov ebx, edx ; rdmsr
>>>> will
>>>>>>>> change edx. keep it in ebx.
>>>>>>>> - DB 0x66, 0xb8
>>>>>>>> + DB 0x66, 0xb8 ; mov eax,
>>>>>> imm32
>>>>>>>> ASM_PFX(gSmmCr3): DD 0
>>>>>>>> mov cr3, eax
>>>>>>>> DB 0x67, 0x66
>>>>>>>> lgdt [cs:ebp + (ASM_PFX(gcSmiInitGdtr) -
>>>>>>>> ASM_PFX(SmmStartup))]
>>>>>>>> - DB 0x66, 0xb8
>>>>>>>> + DB 0x66, 0xb8 ; mov eax,
>>>>>> imm32
>>>>>>>> ASM_PFX(gSmmCr4): DD 0
>>>>>>>> mov cr4, eax
>>>>>>>> DB 0x66
>>>>>>>> mov ecx, 0xc0000080 ; IA32_EFER
>>>>>> MSR
>>>>>>>> rdmsr
>>>>>>>> DB 0x66
>>>>>>>> test ebx, BIT20 ; check NXE
>>>>>>>> capability
>>>>>>>> jz .1
>>>>>>>> or ah, BIT3 ; set NXE
>>>> bit
>>>>>>>> wrmsr
>>>>>>>> .1:
>>>>>>>> - DB 0x66, 0xb8
>>>>>>>> + DB 0x66, 0xb8 ; mov eax,
>>>>>> imm32
>>>>>>>> ASM_PFX(gSmmCr0): DD 0
>>>>>>>> DB 0xbf, PROTECT_MODE_DS, 0 ; mov di,
>>>>>>>> PROTECT_MODE_DS
>>>>>>>> mov cr0, eax
>>>>>>>> - DB 0x66, 0xea ; jmp far
>>>>>>>> [ptr48]
>>>>>>>> + DB 0x66, 0xea ; jmp far
>>>>>>>> [ptr48]
>>>>>>>> ASM_PFX(gSmmJmpAddr):
>>>>>>>> DD @32bit
>>>>>>>> DW PROTECT_MODE_CS
>>>>>>>> @32bit:
>>>>>>>> mov ds, edi
>>>>>>>> mov es, edi
>>>>>>>> --
>>>>>>>> 2.14.1.3.gb7cf6e02401b
>>>>>>>>
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> edk2-devel mailing list
>>>>>>>> edk2-devel@lists.01.org
>>>>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>>>>
>>>>>> _______________________________________________
>>>>>> edk2-devel mailing list
>>>>>> edk2-devel@lists.01.org
>>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>
>>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
2018-02-02 10:06 ` Ard Biesheuvel
2018-02-02 13:26 ` Laszlo Ersek
@ 2018-02-02 13:28 ` Leif Lindholm
2018-02-02 13:36 ` Laszlo Ersek
1 sibling, 1 reply; 27+ messages in thread
From: Leif Lindholm @ 2018-02-02 13:28 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Laszlo Ersek, Kinney, Michael D, edk2-devel-01, Ni, Ruiyu,
Paolo Bonzini, Yao, Jiewen, Dong, Eric
On Fri, Feb 02, 2018 at 10:06:07AM +0000, Ard Biesheuvel wrote:
> On 31 January 2018 at 10:40, Laszlo Ersek <lersek@redhat.com> wrote:
> > On 01/30/18 23:25, Kinney, Michael D wrote:
> >> Laszlo,
> >>
> >> I agree that the function is better than a macro.
> >>
> >> I thought of the alignment issues as well. CopyMem()
> >> is a good solution. We could also consider
> >> WriteUnalignedxx() functions in BaseLib.
> >
> > IMO, the WriteUnalignedxx functions are a bit pointless in the exact
> > form they are declared (this was discussed earlier esp. with regard to
> > aarch64). The functions take pointers to objects that already have the
> > target type, such as
> >
> > UINT32
> > EFIAPI
> > WriteUnaligned32 (
> > OUT UINT32 *Buffer,
> > IN UINT32 Value
> > )
> >
> > Here the type of Buffer should be (VOID *), not (UINT32 *). Otherwise,
> > the undefined behavior (due to mis-alignment) surfaces as soon as the
> > function is called with an unaligned pointer (i.e. before the target
> > area is actually written).
> >
> >> I was originally thinking this functionality would go
> >> into BaseLib. But with the use of CopyMem(), we can't
> >> do that.
> >
> > Can we put it in BaseMemoryLib instead (which is where CopyMem() is
> > from)? That library class is still low-level enough. And, while I count
> > 9 library instances, PatchAssembly() is not a large function, we could
> > tolerate adding it to all 9 instances, identically.
> >
> > Let me also ask the opposite question: should we perhaps make the
> > PatchAssembly() API *less* abstract? (Also suggested by your naming of
> > the macro, PATCH_X86_ASM.) If the instruction encoding on e.g. AARCH64
> > doesn't lend itself to such patching (= expressed through the address
> > right after the instruction), then even BaseMemoryLib may be too generic
> > for the API.
> >
> >> Maybe we should use WriteUnalignedxx() and
> >> add some ASSERT() checks.
> >>
> >> VOID
> >> PatchAssembly (
> >> VOID *BufferEnd,
> >> UINT64 PatchValue,
> >> UINTN ValueSize
> >> )
> >> {
> >> ASSERT ((UINTN)BufferEnd > ValueSize);
> >> switch (ValueSize) {
> >> case 1:
> >> ASSERT (PatchValue <= MAX_UINT8);
> >> *((UINT8 *)BufferEnd - 1) = (UINT8)PatchValue;
> >> case 2:
> >> ASSERT (PatchValue <= MAX_UINT16);
> >> WriteUnaligned16 ((UINT16 *)(BufferEnd) - 1, (UINT16)PatchValue));
> >> break;
> >> case 4:
> >> ASSERT (PatchValue <= MAX_UINT32);
> >> WriteUnaligned32 ((UINT32 *)(BufferEnd) - 1, (UINT32)PatchValue));
> >> break;
> >> case 8:
> >> WriteUnaligned64 ((UINT64 *)(BufferEnd) - 1, PatchValue));
> >> break;
> >> default:
> >> ASSERT (FALSE);
> >> }
> >> }
> >
> > In my opinion:
> >
> > - If Ard and Leif say that PatchAssembly() API makes sense for AARCH64,
> > then I think we can go with the above generic implementation (for
> > BaseLib).
> >
>
> Code patching on ARM/AARCH64 has some hoops to jump through, i.e.,
> clean the D-cache to the point of unification, invalidate the I-cache,
> probably some barriers in case the patching function happened to end
> up in the same cache line as the patchee
Not just the same cache line. Prefetching can happen whenever, for
whatever reason.
> (which may not be a concern
> for this specific use case, but it does need to be taken into account
> if this is turned into a patch-any-assembly-anywhere function)
>
> So if the PatchAssembly() prototype does end up in a generic library
> class, we'd have to provide ARM and AARCH64 specific implementations
> anyway, and given that I don't see any use for this on ARM/AARCH64 in
> the first place, I think this should belong in an IA32/X64 specific
> package.
I also don't see a specific use for this on ARM* at the moment. But if
this is going to become more widespread, it would be useful to
introduce a higher-level layer with more portable semantics (I don't
know RISC-V, but could imagine they require similar).
However, at that point, we would probably want something
buffer-oriented rather than instruction-oriented, since we'd like to
keep the overhead down if writing more than one register's worth.
/
Leif
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
2018-02-02 13:28 ` Leif Lindholm
@ 2018-02-02 13:36 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-02-02 13:36 UTC (permalink / raw)
To: Leif Lindholm, Ard Biesheuvel
Cc: Kinney, Michael D, edk2-devel-01, Ni, Ruiyu, Paolo Bonzini,
Yao, Jiewen, Dong, Eric
On 02/02/18 14:28, Leif Lindholm wrote:
> On Fri, Feb 02, 2018 at 10:06:07AM +0000, Ard Biesheuvel wrote:
>> On 31 January 2018 at 10:40, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 01/30/18 23:25, Kinney, Michael D wrote:
>>>> Laszlo,
>>>>
>>>> I agree that the function is better than a macro.
>>>>
>>>> I thought of the alignment issues as well. CopyMem()
>>>> is a good solution. We could also consider
>>>> WriteUnalignedxx() functions in BaseLib.
>>>
>>> IMO, the WriteUnalignedxx functions are a bit pointless in the exact
>>> form they are declared (this was discussed earlier esp. with regard to
>>> aarch64). The functions take pointers to objects that already have the
>>> target type, such as
>>>
>>> UINT32
>>> EFIAPI
>>> WriteUnaligned32 (
>>> OUT UINT32 *Buffer,
>>> IN UINT32 Value
>>> )
>>>
>>> Here the type of Buffer should be (VOID *), not (UINT32 *). Otherwise,
>>> the undefined behavior (due to mis-alignment) surfaces as soon as the
>>> function is called with an unaligned pointer (i.e. before the target
>>> area is actually written).
>>>
>>>> I was originally thinking this functionality would go
>>>> into BaseLib. But with the use of CopyMem(), we can't
>>>> do that.
>>>
>>> Can we put it in BaseMemoryLib instead (which is where CopyMem() is
>>> from)? That library class is still low-level enough. And, while I count
>>> 9 library instances, PatchAssembly() is not a large function, we could
>>> tolerate adding it to all 9 instances, identically.
>>>
>>> Let me also ask the opposite question: should we perhaps make the
>>> PatchAssembly() API *less* abstract? (Also suggested by your naming of
>>> the macro, PATCH_X86_ASM.) If the instruction encoding on e.g. AARCH64
>>> doesn't lend itself to such patching (= expressed through the address
>>> right after the instruction), then even BaseMemoryLib may be too generic
>>> for the API.
>>>
>>>> Maybe we should use WriteUnalignedxx() and
>>>> add some ASSERT() checks.
>>>>
>>>> VOID
>>>> PatchAssembly (
>>>> VOID *BufferEnd,
>>>> UINT64 PatchValue,
>>>> UINTN ValueSize
>>>> )
>>>> {
>>>> ASSERT ((UINTN)BufferEnd > ValueSize);
>>>> switch (ValueSize) {
>>>> case 1:
>>>> ASSERT (PatchValue <= MAX_UINT8);
>>>> *((UINT8 *)BufferEnd - 1) = (UINT8)PatchValue;
>>>> case 2:
>>>> ASSERT (PatchValue <= MAX_UINT16);
>>>> WriteUnaligned16 ((UINT16 *)(BufferEnd) - 1, (UINT16)PatchValue));
>>>> break;
>>>> case 4:
>>>> ASSERT (PatchValue <= MAX_UINT32);
>>>> WriteUnaligned32 ((UINT32 *)(BufferEnd) - 1, (UINT32)PatchValue));
>>>> break;
>>>> case 8:
>>>> WriteUnaligned64 ((UINT64 *)(BufferEnd) - 1, PatchValue));
>>>> break;
>>>> default:
>>>> ASSERT (FALSE);
>>>> }
>>>> }
>>>
>>> In my opinion:
>>>
>>> - If Ard and Leif say that PatchAssembly() API makes sense for AARCH64,
>>> then I think we can go with the above generic implementation (for
>>> BaseLib).
>>>
>>
>> Code patching on ARM/AARCH64 has some hoops to jump through, i.e.,
>> clean the D-cache to the point of unification, invalidate the I-cache,
>> probably some barriers in case the patching function happened to end
>> up in the same cache line as the patchee
>
> Not just the same cache line. Prefetching can happen whenever, for
> whatever reason.
>
>> (which may not be a concern
>> for this specific use case, but it does need to be taken into account
>> if this is turned into a patch-any-assembly-anywhere function)
>>
>> So if the PatchAssembly() prototype does end up in a generic library
>> class, we'd have to provide ARM and AARCH64 specific implementations
>> anyway, and given that I don't see any use for this on ARM/AARCH64 in
>> the first place, I think this should belong in an IA32/X64 specific
>> package.
>
> I also don't see a specific use for this on ARM* at the moment. But if
> this is going to become more widespread, it would be useful to
> introduce a higher-level layer with more portable semantics (I don't
> know RISC-V, but could imagine they require similar).
> However, at that point, we would probably want something
> buffer-oriented rather than instruction-oriented, since we'd like to
> keep the overhead down if writing more than one register's worth.
I'll CC you and Ard on the BaseLib patches; hopefully
PatchInstructionX86() will be possible to reimplement in terms of the
more generic, buffer-oriented API, once we introduce that.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2018-02-02 13:31 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-01-30 15:33 [PATCH 0/3] UefiCpuPkg/PiSmmCpuDxeSmm: fix IA32 SmmStartup() regression on KVM Laszlo Ersek
2018-01-30 15:33 ` [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup() Laszlo Ersek
2018-01-30 17:22 ` Kinney, Michael D
2018-01-30 18:17 ` Laszlo Ersek
2018-01-30 20:31 ` Kinney, Michael D
2018-01-30 21:26 ` Kinney, Michael D
2018-01-30 21:55 ` Laszlo Ersek
2018-01-30 21:45 ` Laszlo Ersek
2018-01-30 22:25 ` Kinney, Michael D
2018-01-31 5:44 ` Ni, Ruiyu
2018-01-31 5:54 ` Ni, Ruiyu
2018-01-31 10:56 ` Laszlo Ersek
2018-01-31 10:42 ` Laszlo Ersek
2018-01-31 10:40 ` Laszlo Ersek
2018-01-31 22:11 ` Kinney, Michael D
2018-02-02 6:05 ` Laszlo Ersek
2018-02-02 10:06 ` Ard Biesheuvel
2018-02-02 13:26 ` Laszlo Ersek
2018-02-02 13:28 ` Leif Lindholm
2018-02-02 13:36 ` Laszlo Ersek
2018-01-30 15:33 ` [PATCH 2/3] UefiCpuPkg/PiSmmCpuDxeSmm: remove unneeded DBs from " Laszlo Ersek
2018-01-31 5:45 ` Ni, Ruiyu
2018-01-30 15:33 ` [PATCH 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: eliminate conditional jump in " Laszlo Ersek
2018-01-31 5:12 ` Ni, Ruiyu
2018-01-30 16:37 ` [PATCH 0/3] UefiCpuPkg/PiSmmCpuDxeSmm: fix IA32 SmmStartup() regression on KVM Paolo Bonzini
2018-01-31 12:17 ` Laszlo Ersek
2018-02-01 1:20 ` Wang, Jian J
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox