[edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()

Laszlo Ersek posted 3 patches 6 years, 10 months ago
[edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
Posted by Laszlo Ersek 6 years, 10 months ago
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
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
Posted by Kinney, Michael D 6 years, 10 months ago
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
Posted by Laszlo Ersek 6 years, 10 months ago
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
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
Posted by Kinney, Michael D 6 years, 10 months ago
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
Posted by Kinney, Michael D 6 years, 10 months ago
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
Posted by Laszlo Ersek 6 years, 10 months ago
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
Posted by Laszlo Ersek 6 years, 10 months ago
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
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
Posted by Kinney, Michael D 6 years, 10 months ago
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
Posted by Ni, Ruiyu 6 years, 10 months ago
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
Posted by Ni, Ruiyu 6 years, 10 months ago
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
Posted by Laszlo Ersek 6 years, 10 months ago
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
Posted by Laszlo Ersek 6 years, 10 months ago
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
>>
> 
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
Posted by Laszlo Ersek 6 years, 10 months ago
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
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
Posted by Ard Biesheuvel 6 years, 10 months ago
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
>>
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
Posted by Laszlo Ersek 6 years, 10 months ago
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
>>>
>>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
Posted by Leif Lindholm 6 years, 10 months ago
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
Posted by Laszlo Ersek 6 years, 10 months ago
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
Posted by Kinney, Michael D 6 years, 10 months ago
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
> >

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
Posted by Laszlo Ersek 6 years, 10 months ago
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel