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

["Wang, Jian J" <jian.j.wang@intel.com>]

Very good analysis and investigation.

Regards,
Jian


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, January 30, 2018 3:49 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; 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>; Radim
> Kr?má? <rkrcmar@redhat.com>
> Subject: Re: [edk2] [PATCH 4/6] UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if
> it's supported
> 
> 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