Re: [PATCH 4/6] UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if it's supported

[Laszlo Ersek <lersek@redhat.com>]

On 01/29/18 20:48, Laszlo Ersek wrote:
> On 01/29/18 10:02, Wang, Jian J wrote:
>> Hi Laszlo,
>>
>> I don't know the history of these code but I guess they're converted
>> from .asm file. That may be why there's "DB 66h" prefix. I think
>> you're right these tricks should be replaced with more formal ways.
>> Please submit a bz tracker for it.
> 
> I submitted <https://bugzilla.tianocore.org/show_bug.cgi?id=866>.
> 
>> As to the issue, I don't have clue right now. The code seems no
>> problem. Since msr write didn't happen, code flow is correct. And
>> those code has executed on real 32-bit platform without problem. I
>> need more time to investigate it.
> 
> I think I've found the issue; more exactly I narrowed it down a bit. I
> remember that the same problem drove me mad a few years ago. :)
> 
> The issue is that in the middle of such processor mode switches, no jump
> instructions work *at all* on KVM. I don't know why, this is just my
> experience. The KVM behavior could even be justified by the Intel SDM.
> I'm unsure.
> 
> Let's look at the patch with more context:
> 
>>  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
>>  ASM_PFX(gSmmCr3): DD 0
>>      mov     cr3, eax
>>      DB      0x67, 0x66
>>      lgdt    [cs:ebp + (ASM_PFX(gcSmiInitGdtr) - ASM_PFX(SmmStartup))]
>>      DB      0x66, 0xb8
>>  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:
> 
> This code has exactly one jump, and in practice it is never taken
> (because NX support is ubiquitous on physical platforms).
> 
> In my testing I added some other conditional jumps -- I reimplemented
> IsExecuteDisableBitAvailable() from
> "MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c", which has more
> conditions. All the conditional jumps that were *not* taken didn't
> cause any issues. (This is why the same logic from the patch works for
> me in the X64 version, because there the "jz" is never taken, since NX
> is always available there.) However, the first jump that *was* taken in
> my testing immediately hung or crashed the IA32 guest.
> 
> Finally I replaced the entire NX management code with an unconditional
> jump forward:
> 
>     jmp jump_here
> jump_here:
> 
> and even this hung / crashed the guest.
> 
> For some reason this section of the code is unfit for jumping, under KVM
> anyway.

I found a work-around for this.

While short jumps (= relative to EIP) do not work under KVM, in initial
SMM, near jumps to absolute 32-bit addresses (specified indirectly via
registers) do work [*]. Except, the address calculation has to take into
account the trick that PiSmmCpuDxeSmm applies.

Namely, for initial SMBASE relocation,

- while the *short* gcSmmInitTemplate routine is copied to SMBASE+32KB
  (that is, to (0x3_0000 + 0x8000)), and executed from there,

- the SmmStartup routine is actually executed from the *body* of
  PiSmmCpuDxeSmm -- that is, from SMRAM, to which place the SMM Core
  loaded and *relocated* PiSmmCpuDxeSmm, via normal SMM driver dispatch.

This is why gcSmmInitTemplate jumps to SmmStartup as follows, in
"UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm":

> BITS 16
> ASM_PFX(gcSmmInitTemplate):
>     mov ebp, ASM_PFX(SmmStartup)
>     sub ebp, 0x30000
>     jmp ebp
>
> ASM_PFX(gcSmmInitSize): DW $ - ASM_PFX(gcSmmInitTemplate)

The "mov ebp, ASM_PFX(SmmStartup)" instruction is relocated at
PiSmmCpuDxeSmm dispatch time, to the absolute address of SmmStartup in
the body of PiSmmCpuDxeSmm. This absolute address is relative to zero.
However, when the "jmp ebp" is executed in initial SMM mode, the CS
register is not zero (i.e. EIP:=EBP will not be interpreted relative to
zero). Instead, CS is initially set to 0x3000 in SMM, implying a code
segment base that equals SMBASE (0x3_0000). Hence the "sub ebp, 0x30000"
-- it compensates the original relocation of the "mov" for the suddenly
increased CS base.

The same applies to any near jump (with absolute indirect target) in the
SmmStartup routine. All these jump instructions are relocated within the
body of PiSmmCpuDxeSmm (at driver dispatch time) against a *zero* code
segment base, but when they are actually executed in initial SMM, they
are executed against a code segment base of 0x3_0000.

I will send a patch (or a patch set, not sure yet).

[*] segment limits are raised to 4GB, and near jumps can use such
addresses with the "o32" (32-bit operand-size override) prefix. In fact,
NASM inserts the prefix automatically upon seeing eg. "jmp ebx" in BITS 16.

Thanks!
Laszlo