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

["Ni, Ruiyu" <ruiyu.ni@Intel.com>]

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