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.
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. Regards, Jian > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Monday, January 29, 2018 6:47 AM > To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org > Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yao, Jiewen <jiewen....@intel.com>; Dong, > Eric <eric.d...@intel.com>; Paolo Bonzini <pbonz...@redhat.com>; Radim > Kr?má? <rkrc...@redhat.com> > Subject: Re: [edk2] [PATCH 4/6] UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if > it's supported > > Hello Jian, > > On 01/15/18 09:54, Jian J Wang wrote: > > If PcdDxeNxMemoryProtectionPolicy is set to enable protection for memory > > of EfiBootServicesCode, EfiConventionalMemory, the BIOS will hang at a page > > fault exception triggered by PiSmmCpuDxeSmm. > > > > The root cause is that PiSmmCpuDxeSmm will access default SMM RAM > starting > > at 0x30000 which is marked as non-executable, but NX feature was not > > enabled during SMM initialization. Accessing memory which has invalid > > attributes set will cause page fault exception. This patch fixes it by > > checking NX capability in cpuid and enable NXE in EFER MSR if it's > > available. > > > > Cc: Jiewen Yao <jiewen....@intel.com> > > Cc: Ruiyu Ni <ruiyu...@intel.com> > > Cc: Eric Dong <eric.d...@intel.com> > > Cc: Laszlo Ersek <ler...@redhat.com> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Jian J Wang <jian.j.w...@intel.com> > > --- > > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 14 ++++++++++++++ > > UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm | 12 +++++++++++- > > 2 files changed, 25 insertions(+), 1 deletion(-) > > This patch also breaks OVMF on KVM. However, the circumstances and the > symptom differ from those of the other regression that I reported under > patch 1/6 in this series [1] [2]. > > Namely, this affects the IA32 build, with SMM: > > $ build -a IA32 -p OvmfPkg/OvmfPkgIa32.dsc -D SECURE_BOOT_ENABLE \ > -D SMM_REQUIRE -t GCC48 -b NOOPT -D HTTP_BOOT_ENABLE > > The value of PcdDxeNxMemoryProtectionPolicy is zero in this build too. > > The virtual CPU model that this OVMF build runs on does *not* support > NX. (Please refer to "OvmfPkg/README": > > > === SMM support === > > > > [...] > > > > * QEMU binary and options specific to 32-bit guests: > > > > $ qemu-system-i386 -cpu coreduo,-nx \ > > ) > > The boot hangs at the following location: > > > Loading SMM driver at 0x0007FFA2000 EntryPoint=0x0007FFA844D > PiSmmCpuDxeSmm.efi > > SMRR Base: 0x7F000000, SMRR Size: 0x1000000 > > PcdCpuSmmCodeAccessCheckEnable = 1 > > mAddressEncMask = 0x0 > > SMRAM TileSize = 0x00002000 (0x00001000, 0x00001000) > > SMRAM SaveState Buffer (0x7FF94000, 0x0000E000) > > CPU[000] APIC ID=0000 SMBASE=7FF8C000 SaveState=7FF9BC00 > Size=00000400 > > CPU[001] APIC ID=0001 SMBASE=7FF8E000 SaveState=7FF9DC00 > Size=00000400 > > CPU[002] APIC ID=0002 SMBASE=7FF90000 SaveState=7FF9FC00 > Size=00000400 > > CPU[003] APIC ID=0003 SMBASE=7FF92000 SaveState=7FFA1C00 > Size=00000400 > > <--HANG--> > > KVM trace (excerpt): > > > 1 CPU-4989 [002] 16163.874223: kvm_enter_smm: vcpu 1: entering SMM, > smbase 0x30000 > > 2 CPU-4989 [002] 16163.874244: kvm_fpu: load > > 3 CPU-4989 [002] 16163.874245: kvm_entry: vcpu 1 > > 4 CPU-4989 [002] 16163.874247: kvm_exit: reason EPT_VIOLATION > > rip > 0x8000 info 184 0 > > 5 CPU-4989 [002] 16163.874247: kvm_page_fault: address 38000 > error_code 184 > > 6 CPU-4989 [002] 16163.874251: kvm_entry: vcpu 1 > > 7 CPU-4989 [002] 16163.874253: kvm_exit: reason EPT_VIOLATION > > rip > 0x7ff864ba info 184 0 > > 8 CPU-4989 [002] 16163.874253: kvm_page_fault: address 7ffb64ba > error_code 184 > > 9 CPU-4989 [002] 16163.874256: kvm_entry: vcpu 1 > > 10 CPU-4989 [002] 16163.874257: kvm_exit: reason CPUID rip > 0x7ff864c0 info 0 0 > > 11 CPU-4989 [002] 16163.874258: kvm_cpuid: func 80000001 rax 6e8 > rbx 0 rcx 0 rdx 0 > > 12 CPU-4989 [002] 16163.874258: kvm_entry: vcpu 1 > > 13 CPU-4989 [002] 16163.874259: kvm_exit: reason CR_ACCESS rip > 0x7ff864cb info 3 0 > > 14 CPU-4989 [002] 16163.874260: kvm_cr: cr_write 3 = 0x0 > > 15 CPU-4989 [002] 16163.874261: kvm_entry: vcpu 1 > > 16 CPU-4989 [002] 16163.874262: kvm_exit: reason CR_ACCESS rip > 0x7ff864db info 4 0 > > 17 CPU-4989 [002] 16163.874263: kvm_cr: cr_write 4 = 0x640 > > 18 CPU-4989 [002] 16163.874273: kvm_entry: vcpu 1 > > 19 CPU-4989 [002] 16163.874274: kvm_exit: reason MSR_READ rip > 0x7ff864e4 info 0 0 > > 20 CPU-4989 [002] 16163.874275: kvm_msr: msr_read c0000080 = > > 0x0 > > 21 CPU-4989 [002] 16163.874276: kvm_entry: vcpu 1 > > 22 CPU-4989 [002] 16163.874277: kvm_exit: reason EPT_VIOLATION > rip 0x64f4 info 184 0 > > 23 CPU-4989 [002] 16163.874277: kvm_page_fault: address 364f4 > error_code 184 > > 24 CPU-4989 [002] 16163.874282: kvm_entry: vcpu 1 > > 25 CPU-4989 [002] 16163.874283: kvm_exit: reason EPT_VIOLATION > rip 0x64f4 info 183 0 > > 26 CPU-4989 [002] 16163.874283: kvm_page_fault: address f000 > error_code 183 > > 27 CPU-4989 [002] 16163.874288: kvm_entry: vcpu 1 > > 28 CPU-4989 [002] 16163.874292: kvm_exit: reason EPT_VIOLATION > rip 0x7000 info 184 0 > > 29 CPU-4989 [002] 16163.874293: kvm_page_fault: address 37000 > error_code 184 > > 30 CPU-4989 [002] 16163.874295: kvm_entry: vcpu 1 > > 31 CPU-4989 [002] 16163.874300: kvm_exit: reason CPUID rip > 0x7ff864c0 info 0 0 > > 32 CPU-4989 [002] 16163.874301: kvm_cpuid: func 80000001 rax 6e8 > rbx 0 rcx 0 rdx 0 > > 33 CPU-4989 [002] 16163.874301: kvm_entry: vcpu 1 > > 34 CPU-4989 [002] 16163.874302: kvm_exit: reason CR_ACCESS rip > 0x7ff864cb info 3 0 > > 35 CPU-4989 [002] 16163.874303: kvm_cr: cr_write 3 = 0x0 > > 36 CPU-4989 [002] 16163.874304: kvm_entry: vcpu 1 > > After this point, lines 19-36 repeat infinitely. > > The above trace corresponds to the following, from > "UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm": > > > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 43) global > ASM_PFX(SmmStartup) > > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 44) > ASM_PFX(SmmStartup): > > d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 45) DB 0x66 > > d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 46) mov eax, > 0x80000001 ; read capability > > d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 47) cpuid > > d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 48) DB 0x66 > > d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 49) mov ebx, > edx ; rdmsr will change edx. keep it in ebx. > > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 50) DB 0x66, > 0xb8 > > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 51) > ASM_PFX(gSmmCr3): DD 0 > > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 52) mov cr3, > > eax > > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 53) DB 0x67, > 0x66 > > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 54) lgdt > > [cs:ebp + > (ASM_PFX(gcSmiInitGdtr) - ASM_PFX(SmmStartup))] > > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 55) DB 0x66, > 0xb8 > > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 56) > ASM_PFX(gSmmCr4): DD 0 > > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 57) mov cr4, > > eax > > d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 58) DB 0x66 > > d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 59) mov ecx, > 0xc0000080 ; IA32_EFER MSR > > d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 60) rdmsr > > d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 61) DB 0x66 > > d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 62) test ebx, > BIT20 ; check NXE capability > > d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 63) jz .1 > > d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 64) or ah, > BIT3 ; set NXE bit > > d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 65) wrmsr > > d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 66) .1: > > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 67) DB 0x66, > 0xb8 > > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 68) > ASM_PFX(gSmmCr0): DD 0 > > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 69) DB 0xbf, > PROTECT_MODE_DS, 0 ; mov di, PROTECT_MODE_DS > > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 70) mov cr0, > > eax > > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 71) DB 0x66, > 0xea ; jmp far [ptr48] > > The WRMSR is never reached (which is fine) but the CR0 write is also not > reached, ever. Instead, we seem to be stuck in SMM forever, looping back > to SmmStartup. > > Here's the bisection log: > > > git bisect start > > # good: [018432f0ce1b42541977f61f9c7607257a4bf43a] > MdeModulePkg/Ip4Dxe: Add an independent timer for reconfig checking > > git bisect good 018432f0ce1b42541977f61f9c7607257a4bf43a > > # bad: [06c1f423e17fe5ddef824d688d21c83730238ba6] BeagleBoardPkg: > reroute Firmware Vendor Pcd to MdeModulePkg > > git bisect bad 06c1f423e17fe5ddef824d688d21c83730238ba6 > > # bad: [8ab0bd2397c9d3922e0c7dbb1aa6f7e08799079f] MdePkg/DMAR: Add > the definition for DMA_CTRL_PLATFORM_OPT_IN_FLAG bit > > git bisect bad 8ab0bd2397c9d3922e0c7dbb1aa6f7e08799079f > > # good: [24a105a7d8b4b8312743cf265f869dc049b7ff92] BaseTools: Disable > warning varargs in XCODE5 align to CLANG38 > > git bisect good 24a105a7d8b4b8312743cf265f869dc049b7ff92 > > # good: [b2725f57c7a1e6feeb176f1563a4f1a8c2eb6c6f] IntelSiliconPkg > IntelVTdPmrPei: Get high top by host address width > > git bisect good b2725f57c7a1e6feeb176f1563a4f1a8c2eb6c6f > > # good: [4f10654e04601fe67a750c9b5a4242efd4141569] UefiCpuPkg/CpuDxe: > fix SetMemoryAttributes issue in 32-bit mode > > git bisect good 4f10654e04601fe67a750c9b5a4242efd4141569 > > # good: [fbe2c4b9be98a5c2b9c1f6976f51e2456467e752] UefiCpuPkg/CpuDxe: > clear NX attr for page directory > > git bisect good fbe2c4b9be98a5c2b9c1f6976f51e2456467e752 > > # bad: [94c0129d244f91fa0a7b122414872da49a35f853] > MdeModulePkg/PiSmmCore: remove NX attr for SMM RAM > > git bisect bad 94c0129d244f91fa0a7b122414872da49a35f853 > > # bad: [d4d87596c11d6e3f8220b6d9677797c802af3a33] > UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if it's supported > > git bisect bad d4d87596c11d6e3f8220b6d9677797c802af3a33 > > # first bad commit: [d4d87596c11d6e3f8220b6d9677797c802af3a33] > UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if it's supported > > > Finally, I have an independent question: why are we still adding *new* > 0x66 size prefixes with "DB"? NASM supports 16-bit, 32-bit and 64-bit > assembly code in the same source file, and such prefixes can be encoded > symbolically [3]: > > > Explicit address-size and operand-size prefixes A16, A32, A64, O16 and > > O32, O64 are provided -- one example of their use is given in chapter > > 10. > > Thanks > Laszlo > > [1] https://lists.01.org/pipermail/edk2-devel/2018-January/020582.html > [2] https://lists.01.org/pipermail/edk2-devel/2018-January/020586.html > [3] http://www.nasm.us/doc/nasmdoc3.html _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
