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:ler...@redhat.com]
Sent: Tuesday, January 30, 2018 1:45 PM
To: Kinney, Michael D <michael.d.kin...@intel.com>; edk2-
devel-01 <edk2-devel@lists.01.org>
Cc: Ni, Ruiyu <ruiyu...@intel.com>; Paolo Bonzini
<pbonz...@redhat.com>; Yao, Jiewen
<jiewen....@intel.com>; Dong, Eric <eric.d...@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-
boun...@lists.01.org]
On Behalf Of Laszlo Ersek
Sent: Tuesday, January 30, 2018 10:17 AM
To: Kinney, Michael D <michael.d.kin...@intel.com>;
edk2-
devel-01 <edk2-devel@lists.01.org>
Cc: Ni, Ruiyu <ruiyu...@intel.com>; Paolo Bonzini
<pbonz...@redhat.com>; Yao, Jiewen
<jiewen....@intel.com>; Dong, Eric
<eric.d...@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-
boun...@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...@intel.com>; Yao, Jiewen
<jiewen....@intel.com>; Dong, Eric
<eric.d...@intel.com>;
Paolo Bonzini <pbonz...@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.d...@intel.com>
Cc: Jian J Wang <jian.j.w...@intel.com>
Cc: Jiewen Yao <jiewen....@intel.com>
Cc: Paolo Bonzini <pbonz...@redhat.com>
Cc: Ruiyu Ni <ruiyu...@intel.com>
Contributed-under: TianoCore Contribution Agreement
1.1
Signed-off-by: Laszlo Ersek <ler...@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...@intel.com>
--
Thanks,
Ray
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
