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

[Laszlo Ersek <lersek@redhat.com>]

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.

Thanks
Laszlo