Very good analysis and investigation. Regards, Jian
> -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Tuesday, January 30, 2018 3:49 AM > To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org > Cc: Ni, Ruiyu <ruiyu...@intel.com>; Paolo Bonzini <pbonz...@redhat.com>; Yao, > Jiewen <jiewen....@intel.com>; Dong, Eric <eric.d...@intel.com>; Radim > Kr?má? <rkrc...@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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
