[PATCH 0/3] UefiCpuPkg/PiSmmCpuDxeSmm: fix IA32 SmmStartup() regression on KVM

[PATCH 0/3] UefiCpuPkg/PiSmmCpuDxeSmm: fix IA32 SmmStartup() regression on KVM

[Laszlo Ersek]

Repo:   https://github.com/lersek/edk2.git
Branch: smm_startup_ia32_nocond

This small series fixes the IA32 SMM regression on KVM that I reported
recently. The first two patches are cleanups (no functional changes),
the third patch is the fix.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>

Thanks
Laszlo

Laszlo Ersek (3):
  UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
  UefiCpuPkg/PiSmmCpuDxeSmm: remove unneeded DBs from IA32 SmmStartup()
  UefiCpuPkg/PiSmmCpuDxeSmm: eliminate conditional jump in IA32
    SmmStartup()

 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 28 +++++++++-----------
 1 file changed, 13 insertions(+), 15 deletions(-)

-- 
2.14.1.3.gb7cf6e02401b

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

[Laszlo Ersek]

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

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

[Kinney, Michael D]

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

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

[Laszlo Ersek]

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

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

[Kinney, Michael D]

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

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

[Kinney, Michael D]

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

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

[Laszlo Ersek]

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

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

[Laszlo Ersek]

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

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

[Kinney, Michael D]

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

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

[Ni, Ruiyu]

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

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

[Ni, Ruiyu]

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

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

[Laszlo Ersek]

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

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

[Laszlo Ersek]

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
>>
> 
> 

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

[Laszlo Ersek]

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
> 

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

[Kinney, Michael D]

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
> >

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

[Laszlo Ersek]

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

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

[Ard Biesheuvel]

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
>>
>

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

[Laszlo Ersek]

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
>>>
>>

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

[Leif Lindholm]

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

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

[Laszlo Ersek]

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

[PATCH 2/3] UefiCpuPkg/PiSmmCpuDxeSmm: remove unneeded DBs from IA32 SmmStartup()

[Laszlo Ersek]

The SmmStartup() executes in SMM, which is very similar to real mode. Add
"BITS 16" before it and "BITS 32" after it (just before the @32bit label).

Remove the manual 0x66 operand-size override prefixes, for selecting
32-bit operands -- the sizes of our operands trigger NASM to insert the
prefixes automatically in almost every spot. The one place where we have
to add it back manually is the LGDT instruction. (The 0x67 address-size
override prefix is also auto-generated.)

This patch causes NASM to generate byte-identical object code (determined
by disassembling both the pre-patch and post-patch versions, and comparing
the listings), except:

> @@ -158,7 +158,7 @@
>  00000142  6689D3            mov ebx,edx
>  00000145  66B800000000      mov eax,0x0
>  0000014B  0F22D8            mov cr3,eax
> -0000014E  67662E0F0155F6    o32 lgdt [cs:ebp-0xa]
> +0000014E  2E66670F0155F6    o32 lgdt [cs:ebp-0xa]
>  00000155  66B800000000      mov eax,0x0
>  0000015B  0F22E0            mov cr4,eax
>  0000015E  66B9800000C0      mov ecx,0xc0000080

The only difference is the prefix list order, it changes from:

- 0x67, 0x66, 0x2E

to

- 0x2E, 0x66, 0x67

(0x2E is "CS segment override").

Cc: Eric Dong <eric.dong@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=866
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
index 08534dba64b7..9231aa5b3ded 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
@@ -38,43 +38,42 @@ global ASM_PFX(gcSmmInitTemplate)
 
 ASM_PFX(gcSmiInitGdtr):
             DW      0
             DQ      0
 
 global ASM_PFX(SmmStartup)
+
+BITS 16
 ASM_PFX(SmmStartup):
-    DB      0x66
     mov     eax, 0x80000001             ; read capability
     cpuid
-    DB      0x66
     mov     ebx, edx                    ; rdmsr will change edx. keep it in ebx.
     DB      0x66, 0xb8                  ; mov eax, imm32
 ASM_PFX(gSmmCr3): DD 0
     mov     cr3, eax
-    DB      0x67, 0x66
-    lgdt    [cs:ebp + (ASM_PFX(gcSmiInitGdtr) - ASM_PFX(SmmStartup))]
+o32 lgdt    [cs:ebp + (ASM_PFX(gcSmiInitGdtr) - ASM_PFX(SmmStartup))]
     DB      0x66, 0xb8                  ; mov eax, imm32
 ASM_PFX(gSmmCr4): DD 0
     mov     cr4, eax
-    DB      0x66
     mov     ecx, 0xc0000080             ; IA32_EFER MSR
     rdmsr
-    DB      0x66
     test    ebx, BIT20                  ; check NXE capability
     jz      .1
     or      ah, BIT3                    ; set NXE bit
     wrmsr
 .1:
     DB      0x66, 0xb8                  ; mov eax, imm32
 ASM_PFX(gSmmCr0): DD 0
-    DB      0xbf, PROTECT_MODE_DS, 0    ; mov di, PROTECT_MODE_DS
+    mov     di, PROTECT_MODE_DS
     mov     cr0, eax
     DB      0x66, 0xea                  ; jmp far [ptr48]
 ASM_PFX(gSmmJmpAddr):
     DD      @32bit
     DW      PROTECT_MODE_CS
+
+BITS 32
 @32bit:
     mov     ds, edi
     mov     es, edi
     mov     fs, edi
     mov     gs, edi
     mov     ss, edi
-- 
2.14.1.3.gb7cf6e02401b

Re: [PATCH 2/3] UefiCpuPkg/PiSmmCpuDxeSmm: remove unneeded DBs from IA32 SmmStartup()

[Ni, Ruiyu]

On 1/30/2018 11:33 PM, Laszlo Ersek wrote:
> The SmmStartup() executes in SMM, which is very similar to real mode. Add
> "BITS 16" before it and "BITS 32" after it (just before the @32bit label).
> 
> Remove the manual 0x66 operand-size override prefixes, for selecting
> 32-bit operands -- the sizes of our operands trigger NASM to insert the
> prefixes automatically in almost every spot. The one place where we have
> to add it back manually is the LGDT instruction. (The 0x67 address-size
> override prefix is also auto-generated.)
> 
> This patch causes NASM to generate byte-identical object code (determined
> by disassembling both the pre-patch and post-patch versions, and comparing
> the listings), except:
> 
>> @@ -158,7 +158,7 @@
>>   00000142  6689D3            mov ebx,edx
>>   00000145  66B800000000      mov eax,0x0
>>   0000014B  0F22D8            mov cr3,eax
>> -0000014E  67662E0F0155F6    o32 lgdt [cs:ebp-0xa]
>> +0000014E  2E66670F0155F6    o32 lgdt [cs:ebp-0xa]
>>   00000155  66B800000000      mov eax,0x0
>>   0000015B  0F22E0            mov cr4,eax
>>   0000015E  66B9800000C0      mov ecx,0xc0000080
> 
> The only difference is the prefix list order, it changes from:
> 
> - 0x67, 0x66, 0x2E
> 
> to
> 
> - 0x2E, 0x66, 0x67
> 
> (0x2E is "CS segment override").
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=866
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>   UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 13 ++++++-------
>   1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> index 08534dba64b7..9231aa5b3ded 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> @@ -38,43 +38,42 @@ global ASM_PFX(gcSmmInitTemplate)
>   
>   ASM_PFX(gcSmiInitGdtr):
>               DW      0
>               DQ      0
>   
>   global ASM_PFX(SmmStartup)
> +
> +BITS 16
>   ASM_PFX(SmmStartup):
> -    DB      0x66
>       mov     eax, 0x80000001             ; read capability
>       cpuid
> -    DB      0x66
>       mov     ebx, edx                    ; rdmsr will change edx. keep it in ebx.
>       DB      0x66, 0xb8                  ; mov eax, imm32
>   ASM_PFX(gSmmCr3): DD 0
>       mov     cr3, eax
> -    DB      0x67, 0x66
> -    lgdt    [cs:ebp + (ASM_PFX(gcSmiInitGdtr) - ASM_PFX(SmmStartup))]
> +o32 lgdt    [cs:ebp + (ASM_PFX(gcSmiInitGdtr) - ASM_PFX(SmmStartup))]
>       DB      0x66, 0xb8                  ; mov eax, imm32
>   ASM_PFX(gSmmCr4): DD 0
>       mov     cr4, eax
> -    DB      0x66
>       mov     ecx, 0xc0000080             ; IA32_EFER MSR
>       rdmsr
> -    DB      0x66
>       test    ebx, BIT20                  ; check NXE capability
>       jz      .1
>       or      ah, BIT3                    ; set NXE bit
>       wrmsr
>   .1:
>       DB      0x66, 0xb8                  ; mov eax, imm32
>   ASM_PFX(gSmmCr0): DD 0
> -    DB      0xbf, PROTECT_MODE_DS, 0    ; mov di, PROTECT_MODE_DS
> +    mov     di, PROTECT_MODE_DS
>       mov     cr0, eax
>       DB      0x66, 0xea                  ; jmp far [ptr48]
>   ASM_PFX(gSmmJmpAddr):
>       DD      @32bit
>       DW      PROTECT_MODE_CS
> +
> +BITS 32
>   @32bit:
>       mov     ds, edi
>       mov     es, edi
>       mov     fs, edi
>       mov     gs, edi
>       mov     ss, edi
> 
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>

-- 
Thanks,
Ray

[PATCH 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: eliminate conditional jump in IA32 SmmStartup()

[Laszlo Ersek]

SMM emulation under KVM crashes the guest when the "jz" branch, added in
commit d4d87596c11d ("UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if it's
supported", 2018-01-18), is taken.

Rework the propagation of CPUID.80000001H:EDX.NX [bit 20] to IA32_EFER.NXE
[bit 11] so that no code is executed conditionally.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Ref: http://mid.mail-archive.com/d6fff558-6c4f-9ca6-74a7-e7cd9d007276@redhat.com
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
index 9231aa5b3ded..102e0bdbabc8 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
@@ -44,26 +44,25 @@ global ASM_PFX(SmmStartup)
 
 BITS 16
 ASM_PFX(SmmStartup):
     mov     eax, 0x80000001             ; read capability
     cpuid
     mov     ebx, edx                    ; rdmsr will change edx. keep it in ebx.
+    and     ebx, BIT20                  ; extract XD capability bit
+    shr     ebx, 9                      ; shift bit to IA32_EFER NXE position
     DB      0x66, 0xb8                  ; mov eax, imm32
 ASM_PFX(gSmmCr3): DD 0
     mov     cr3, eax
 o32 lgdt    [cs:ebp + (ASM_PFX(gcSmiInitGdtr) - ASM_PFX(SmmStartup))]
     DB      0x66, 0xb8                  ; mov eax, imm32
 ASM_PFX(gSmmCr4): DD 0
     mov     cr4, eax
     mov     ecx, 0xc0000080             ; IA32_EFER MSR
     rdmsr
-    test    ebx, BIT20                  ; check NXE capability
-    jz      .1
-    or      ah, BIT3                    ; set NXE bit
+    or      eax, ebx                    ; set NXE bit if XD is available
     wrmsr
-.1:
     DB      0x66, 0xb8                  ; mov eax, imm32
 ASM_PFX(gSmmCr0): DD 0
     mov     di, PROTECT_MODE_DS
     mov     cr0, eax
     DB      0x66, 0xea                  ; jmp far [ptr48]
 ASM_PFX(gSmmJmpAddr):
-- 
2.14.1.3.gb7cf6e02401b

Re: [PATCH 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: eliminate conditional jump in IA32 SmmStartup()

[Ni, Ruiyu]

On 1/30/2018 11:33 PM, Laszlo Ersek wrote:
> SMM emulation under KVM crashes the guest when the "jz" branch, added in
> commit d4d87596c11d ("UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if it's
> supported", 2018-01-18), is taken.
> 
> Rework the propagation of CPUID.80000001H:EDX.NX [bit 20] to IA32_EFER.NXE
> [bit 11] so that no code is executed conditionally.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Ref: http://mid.mail-archive.com/d6fff558-6c4f-9ca6-74a7-e7cd9d007276@redhat.com
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>   UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> index 9231aa5b3ded..102e0bdbabc8 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> @@ -44,26 +44,25 @@ global ASM_PFX(SmmStartup)
>   
>   BITS 16
>   ASM_PFX(SmmStartup):
>       mov     eax, 0x80000001             ; read capability
>       cpuid
>       mov     ebx, edx                    ; rdmsr will change edx. keep it in ebx.
> +    and     ebx, BIT20                  ; extract XD capability bit
Per CPUID_EXTENDED_CPU_SIG_EDX definition in
UefiCpuPkg/Include/Register/CpuId.h, the BIT name is NX.

> +    shr     ebx, 9                      ; shift bit to IA32_EFER NXE position
How about changing above comments to:
; shift bit to IA32_EFER.NXE[BIT11] position?

>       DB      0x66, 0xb8                  ; mov eax, imm32
>   ASM_PFX(gSmmCr3): DD 0
>       mov     cr3, eax
>   o32 lgdt    [cs:ebp + (ASM_PFX(gcSmiInitGdtr) - ASM_PFX(SmmStartup))]
>       DB      0x66, 0xb8                  ; mov eax, imm32
>   ASM_PFX(gSmmCr4): DD 0
>       mov     cr4, eax
>       mov     ecx, 0xc0000080             ; IA32_EFER MSR
>       rdmsr
> -    test    ebx, BIT20                  ; check NXE capability
> -    jz      .1
> -    or      ah, BIT3                    ; set NXE bit
> +    or      eax, ebx                    ; set NXE bit if XD is available
; Bit name is NX.
>       wrmsr
> -.1:
>       DB      0x66, 0xb8                  ; mov eax, imm32
>   ASM_PFX(gSmmCr0): DD 0
>       mov     di, PROTECT_MODE_DS
>       mov     cr0, eax
>       DB      0x66, 0xea                  ; jmp far [ptr48]
>   ASM_PFX(gSmmJmpAddr):
> 
Very clever solution to remove jz.
With the minor comments update,
Reviewed-by: Ruiyu Ni <ruiyu.ni@Intel.com>

-- 
Thanks,
Ray

Re: [PATCH 0/3] UefiCpuPkg/PiSmmCpuDxeSmm: fix IA32 SmmStartup() regression on KVM

[Paolo Bonzini]

On 30/01/2018 10:33, Laszlo Ersek wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: smm_startup_ia32_nocond
> 
> This small series fixes the IA32 SMM regression on KVM that I reported
> recently. The first two patches are cleanups (no functional changes),
> the third patch is the fix.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

> Thanks
> Laszlo
> 
> Laszlo Ersek (3):
>   UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
>   UefiCpuPkg/PiSmmCpuDxeSmm: remove unneeded DBs from IA32 SmmStartup()
>   UefiCpuPkg/PiSmmCpuDxeSmm: eliminate conditional jump in IA32
>     SmmStartup()
> 
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 28 +++++++++-----------
>  1 file changed, 13 insertions(+), 15 deletions(-)
> 

Re: [PATCH 0/3] UefiCpuPkg/PiSmmCpuDxeSmm: fix IA32 SmmStartup() regression on KVM

[Laszlo Ersek]

On 01/30/18 16:33, Laszlo Ersek wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: smm_startup_ia32_nocond
>
> This small series fixes the IA32 SMM regression on KVM that I reported
> recently. The first two patches are cleanups (no functional changes),
> the third patch is the fix.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>
> Thanks
> Laszlo
>
> Laszlo Ersek (3):
>   UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
>   UefiCpuPkg/PiSmmCpuDxeSmm: remove unneeded DBs from IA32 SmmStartup()
>   UefiCpuPkg/PiSmmCpuDxeSmm: eliminate conditional jump in IA32
>     SmmStartup()
>
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 28 +++++++++-----------
>  1 file changed, 13 insertions(+), 15 deletions(-)
>

Code differences between the posted version and the pushed version,
according to Ray's comments for patch #3, displayed with "git diff
--word-diff" (look for [-...-] and {+...+} markers):

> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> index 102e0bdbabc8..d64fcd48d03e 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> @@ -47,8 +47,8 @@ ASM_PFX(SmmStartup):
>     mov     eax, 0x80000001             ; read capability
>     cpuid
>     mov     ebx, edx                    ; rdmsr will change edx. keep it in ebx.
>     and     ebx, BIT20                  ; extract [-XD-]{+NX+} capability bit
>     shr     ebx, 9                      ; shift bit to [-IA32_EFER NXE-]{+IA32_EFER.NXE[BIT11]+} position
>     DB      0x66, 0xb8                  ; mov eax, imm32
> ASM_PFX(gSmmCr3): DD 0
>     mov     cr3, eax
> @@ -58,7 +58,7 @@ ASM_PFX(gSmmCr4): DD 0
>     mov     cr4, eax
>     mov     ecx, 0xc0000080             ; IA32_EFER MSR
>     rdmsr
>     or      eax, ebx                    ; set NXE bit if [-XD-]{+NX+} is available
>     wrmsr
>     DB      0x66, 0xb8                  ; mov eax, imm32
> ASM_PFX(gSmmCr0): DD 0

Series pushed as d5988a8ac971..8d4d55b15b58.

Let's continue the PatchAssembly() discussion; once we reach an
agreement on that, I'll post patches for that too.

Thanks!
Laszlo

Re: [PATCH 0/3] UefiCpuPkg/PiSmmCpuDxeSmm: fix IA32 SmmStartup() regression on KVM

[Wang, Jian J]

Hi Laszlo,

Thank you very much for fixing this issue.

Regards,
Jian


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, January 30, 2018 11:34 PM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Dong, Eric <eric.dong@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> Yao, Jiewen <jiewen.yao@intel.com>; Paolo Bonzini <pbonzini@redhat.com>;
> Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: [PATCH 0/3] UefiCpuPkg/PiSmmCpuDxeSmm: fix IA32 SmmStartup()
> regression on KVM
> 
> Repo:   https://github.com/lersek/edk2.git
> Branch: smm_startup_ia32_nocond
> 
> This small series fixes the IA32 SMM regression on KVM that I reported
> recently. The first two patches are cleanups (no functional changes),
> the third patch is the fix.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (3):
>   UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
>   UefiCpuPkg/PiSmmCpuDxeSmm: remove unneeded DBs from IA32
> SmmStartup()
>   UefiCpuPkg/PiSmmCpuDxeSmm: eliminate conditional jump in IA32
>     SmmStartup()
> 
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 28 +++++++++-----------
>  1 file changed, 13 insertions(+), 15 deletions(-)
> 
> --
> 2.14.1.3.gb7cf6e02401b