Hi Laszlo, Have sent out the formal patch for review.
Thanks, Star [EOM] > -----Original Message----- > From: Zeng, Star > Sent: Friday, August 7, 2015 9:36 AM > To: Zeng, Star; Laszlo Ersek; Paolo Bonzini > Cc: Justen, Jordan L; edk2-de...@ml01.01.org; Andrew Fish; Yao, Jiewen; > Chen Fan; Fan, Jeff > Subject: RE: [edk2] [PATCH] MdeModulePkg DxeIpl: Add stack NX support > > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of > > Zeng, Star > > Sent: Friday, August 7, 2015 9:28 AM > > To: Laszlo Ersek; Paolo Bonzini > > Cc: Justen, Jordan L; edk2-de...@ml01.01.org; Andrew Fish; Yao, > > Jiewen; Chen Fan; Fan, Jeff > > Subject: Re: [edk2] [PATCH] MdeModulePkg DxeIpl: Add stack NX support > > > > > -----Original Message----- > > > From: Laszlo Ersek [mailto:ler...@redhat.com] > > > Sent: Thursday, August 6, 2015 9:43 PM > > > To: Zeng, Star; Paolo Bonzini > > > Cc: Andrew Fish; Justen, Jordan L; edk2-de...@ml01.01.org; Yao, > > > Jiewen; Chen Fan; Fan, Jeff > > > Subject: Re: [edk2] [PATCH] MdeModulePkg DxeIpl: Add stack NX > > > support > > > > > > Hi Star, > > > > > > On 08/06/15 11:44, Zeng, Star wrote: > > > > Hi Laszlo, > > > > > > > > Could you help take a try with the attached patch on your VM > > > > before I send > > > it for formal review? > > > > > > I got your patch via your first (public) message as well -- not the > > > one reflected by the list software, but on the direct route. So, I > > > can test it and even comment on it a little bit below (in > > > Thunderbird plaintext attachments are displayed inline, and if you > > > select the full contents of the "body" window before hitting Reply > > > All, then the entire selection will be quoted, including the inline > > > displayed text > > attachments). > > > > > > ** So, test results: > > > > > > - With X64 DXE, your patch solves the issue for me. > > > > > > - With Ia32 DXE, your patch also soves the issue. > > > > > > - With this patch applied, if I use Ia32 DXE *plus* my SMM series, then: > > > > > > - Using TCG in QEMU (= software emulation), the patch solves the > > > issue. > > > > > > - Using KVM with QEMU (= hardware virtualization), I continue seeing > > > reboots, but they hit in a different place. Somewhere inside or > > > after the SMBASE relocation. I believe this is a KVM bug. I will > > > send a separate email about this. > > > > > > ** Comments on the patch: > > > > > > > CpuDxe.diff > > > > > > > > > > > > 9238355e710a898a148efa072c67693fac631e42 > > > > UefiCpuPkg/CpuDxe/ApStartup.c | 140 > > > > +++++++++++++++++++++++++++++++++++++++++- > > > > 1 file changed, 138 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/UefiCpuPkg/CpuDxe/ApStartup.c > > > > b/UefiCpuPkg/CpuDxe/ApStartup.c index 7613b47..59cf8e6 100644 > > > > --- a/UefiCpuPkg/CpuDxe/ApStartup.c > > > > +++ b/UefiCpuPkg/CpuDxe/ApStartup.c > > > > @@ -19,6 +19,47 @@ > > > > #pragma pack(1) > > > > > > > > typedef struct { > > > > + UINT8 MoveIa32EferMsrToEcx[5]; > > > > + UINT8 ReadIa32EferMsr[2]; > > > > + UINT8 SetExecuteDisableBitEnableBit[4]; > > > > + UINT8 WriteIa32EferMsr[2]; > > > > + > > > > +#if defined (MDE_CPU_IA32) > > > > + UINT8 MovEaxCr3; > > > > + UINT32 Cr3Value; > > > > + UINT8 MovCr3Eax[3]; > > > > + > > > > + UINT8 MoveCr4ToEax[3]; > > > > + UINT8 SetCr4Bit5[4]; > > > > + UINT8 MoveEaxToCr4[3]; > > > > + > > > > + UINT8 MoveCr0ToEax[3]; > > > > + UINT8 SetCr0PagingBit[4]; > > > > + UINT8 MoveEaxToCr0[3]; > > > > +#endif > > > > +} ENABLE_EXECUTE_DISABLE_CODE; > > > > + > > > > +ENABLE_EXECUTE_DISABLE_CODE > > mEnableExecuteDisbleCodeTemplate > > > = { > > > > > > (1) This should be STATIC. (Although, I realize mStartupCodeTemplate > > > is not STATIC either, so feel free to ignore this.) > > > > > > > + { 0xB9, 0x80, 0x00, 0x00, 0xC0 }, // mov ecx, 0xc0000080 > > > > + { 0x0F, 0x32 }, // rdmsr > > > > + { 0x0F, 0xBA, 0xE8, 0x0B }, // bts eax, 11 > > > > + { 0x0F, 0x30 }, // wrmsr > > > > + > > > > +#if defined (MDE_CPU_IA32) > > > > + 0xB8, 0x00000000, // mov eax, cr3 value > > > > + { 0x0F, 0x22, 0xd8 }, // mov cr3, eax > > > > + > > > > + { 0x0F, 0x20, 0xE0 }, // mov eax, cr4 > > > > + { 0x0F, 0xBA, 0xE8, 0x05 }, // bts eax, 5 > > > > + { 0x0F, 0x22, 0xE0 }, // mov cr4, eax > > > > + > > > > + { 0x0F, 0x20, 0xC0 }, // mov eax, cr0 > > > > + { 0x0F, 0xBA, 0xE8, 0x1F }, // bts eax, 31 > > > > + { 0x0F, 0x22, 0xC0 }, // mov cr0, eax > > > > +#endif > > > > +}; > > > > + > > > > +typedef struct { > > > > UINT8 JmpToCli[2]; > > > > > > > > UINT16 GdtLimit; > > > > @@ -56,6 +97,12 @@ typedef struct { > > > > // > > > > // Transition to X64 > > > > // > > > > + > > > > + // > > > > + // Code placeholder to enable Execute Disable Bit // > > > > + ENABLE_EXECUTE_DISABLE_CODE EnableExecuteDisble; > > > > + > > > > UINT8 MovEaxCr3; > > > > UINT32 Cr3Value; > > > > UINT8 MovCr3Eax[3]; > > > > @@ -75,7 +122,12 @@ typedef struct { > > > > //UINT8 DeadLoop[2]; > > > > > > > > UINT8 FarJmp32LongMode; UINT32 LongJmpOffset; UINT16 > > > > LongJmpSelector; -#endif // defined (MDE_CPU_X64) > > > > +#else > > > > + // > > > > + // Code placeholder to enable IA32 PAE Execute Disable > > > > + // > > > > + ENABLE_EXECUTE_DISABLE_CODE EnableExecuteDisble; #endif > > > > > > (2) I think that you can eliminate the #else branch (more precisely, > > > you don't need to introduce it at all). Simply add the > > > ENABLE_EXECUTE_DISABLE_CODE field above the > > > > > > #if defined (MDE_CPU_X64) > > > > > > line, so that it becomes common to X64 and Ia32. I agree that the > > > comment / purpose of the field differs between X64 and Ia32, but > > > that could be spelled out in one, common comment block. > > > > Good comments, agree with it. > > > > > > > > (3) Also, please search the whole patch for occurrences of "Disble", > > > that's a typo. > > > > My fault, will correct it. > > > > > > > > > > > > > #if defined (MDE_CPU_X64) > > > > UINT8 MovEaxOrRaxCpuDxeEntry[2]; UINTN CpuDxeEntryValue; @@ > - > > > 207,6 > > > > +259,17 @@ STARTUP_CODE mStartupCodeTemplate = { > > > > { 0x8e, 0xd0 }, // mov ss, ax > > > > > > > > #if defined (MDE_CPU_X64) > > > > + // > > > > + // Code placeholder to enable Execute Disable Bit // Default > > > > + is all NOP¡ªNo Operation > > > > > > (4) Your patch should be pure ASCII. There seems to be a UTF-8 > > > sequence between "NOP" and "No Operation", 0xA1 0xAA. > > > > I copied it from IA32 manual, will correct it. > > > > > > > > > + // > > > > + { > > > > + { 0x90, 0x90, 0x90, 0x90, 0x90 }, // mov ecx, 0xc0000080 > > > > + { 0x90, 0x90 }, // rdmsr > > > > + { 0x90, 0x90, 0x90, 0x90 }, // bts eax, 11 > > > > + { 0x90, 0x90 }, // wrmsr > > > > + }, > > > > + > > > > > > (5) I don't think it is useful to preserve the assembly instructions > > > here, in comments; the binary contents is just NOPs. The comments to > > > the right don't match. > > > > > > > 0xB8, 0x00000000, // mov eax, cr3 value > > > > { 0x0F, 0x22, 0xd8 }, // mov cr3, eax > > > > > > > > @@ -226,7 +289,29 @@ STARTUP_CODE mStartupCodeTemplate = { > > > > 0xEA, // FarJmp32LongMode > > > > OFFSET_OF(STARTUP_CODE, MovEaxOrRaxCpuDxeEntry), > > > > LINEAR_CODE64_SEL, > > > > -#endif // defined (MDE_CPU_X64) > > > > +#else > > > > + // > > > > + // Code placeholder to enable IA32 PAE Execute Disable > > > > + // Default is all NOP¡ªNo Operation > > > > + // > > > > + { > > > > + { 0x90, 0x90, 0x90, 0x90, 0x90 }, // mov ecx, 0xc0000080 > > > > + { 0x90, 0x90 }, // rdmsr > > > > + { 0x90, 0x90, 0x90, 0x90 }, // bts eax, 11 > > > > + { 0x90, 0x90 }, // wrmsr > > > > + > > > > + 0x90, 0x90909090, // mov eax, cr3 value > > > > + { 0x90, 0x90, 0x90 }, // mov cr3, eax > > > > + > > > > + { 0x90, 0x90, 0x90 }, // mov eax, cr4 > > > > + { 0x90, 0x90, 0x90, 0x90 }, // bts eax, 5 > > > > + { 0x90, 0x90, 0x90 }, // mov cr4, eax > > > > + > > > > + { 0x90, 0x90, 0x90 }, // mov eax, cr0 > > > > + { 0x90, 0x90, 0x90, 0x90 }, // bts eax, 31 > > > > + { 0x90, 0x90, 0x90 }, // mov cr0, eax > > > > + }, > > > > +#endif > > > > > > (6) Ditto. > > > > Ok, will remove them for (5) and (6). > > > > > > > > > > > > > //0xeb, 0xfe, // jmp $ > > > > #if defined (MDE_CPU_X64) > > > > @@ -241,6 +326,48 @@ STARTUP_CODE mStartupCodeTemplate = > > > { volatile > > > > STARTUP_CODE *StartupCode = NULL; > > > > > > > > /** > > > > + The function will check if BSP Execute Disable is enabled. > > > > + DxeIpl may have enabled Execute Disable for BSP, APs need to > > > > + get the status and sync up the settings. > > > > + > > > > + @retval TRUE BSP Execute Disable is enabled. > > > > + @retval FALSE BSP Execute Disable is not enabled. > > > > + > > > > +**/ > > > > +BOOLEAN > > > > +IsBspExecuteDisableEnabled ( > > > > + VOID > > > > + ) > > > > +{ > > > > + UINT32 RegEax; > > > > + UINT32 RegEdx; > > > > + UINT64 MsrRegisters; > > > > + BOOLEAN Enabled; > > > > + > > > > + Enabled = FALSE; > > > > + AsmCpuid (0x80000000, &RegEax, NULL, NULL, NULL); if (RegEax > > > > + >= > > > > + 0x80000001) { > > > > + AsmCpuid (0x80000001, NULL, NULL, NULL, &RegEdx); > > > > + // > > > > + // Cpuid 0x80000001 > > > > + // Bit 20: Execute Disable Bit available. > > > > + // > > > > + if ((RegEdx & BIT20) != 0) { > > > > + MsrRegisters = AsmReadMsr64 (0xC0000080); > > > > + // > > > > + // Msr 0xC0000080 > > > > + // Bit 11: Execute Disable Bit enable. > > > > + // > > > > + if ((MsrRegisters & BIT11) != 0) { > > > > + Enabled = TRUE; > > > > + } > > > > + } > > > > + } > > > > + > > > > + return Enabled; > > > > +} > > > > + > > > > +/** > > > > Prepares Startup Code for APs. > > > > This function prepares Startup Code for APs. > > > > > > > > @@ -280,9 +407,18 @@ PrepareAPStartupCode ( > > > > > > > > StartupCode->FlatJmpOffset += (UINT32) StartAddress; > > > > > > > > + if (IsBspExecuteDisableEnabled ()) { > > > > + CopyMem ( > > > > + (VOID*) &StartupCode->EnableExecuteDisble, > > > > > > (7) No need for the (VOID*) cast. > > > > With (VOID *) cast, I met build failure below. > > Correct one typo above, "With" need to be "Without". > > > > > f:\svngit\edk2gitsvn\UefiCpuPkg\CpuDxe\ApStartup.c(412) : error C2220: > > warning treated as error - no 'object' file generated > > f:\svngit\edk2gitsvn\UefiCpuPkg\CpuDxe\ApStartup.c(412) : warning > C4090: > > 'function' : different 'volatile' qualifiers NMAKE : fatal error U1077: > > '"C:\Program Files (x86)\Microsoft Visual Studio > > 12.0\Vc\bin\x86_amd64\cl.exe"' : return code '0x2' > > Stop. > > > > I also saw the cast at CopyMem ((VOID*) StartupCode, > > &mStartupCodeTemplate, sizeof (*StartupCode));. > > > > > > > > > + &mEnableExecuteDisbleCodeTemplate, > > > > + sizeof (ENABLE_EXECUTE_DISABLE_CODE) > > > > + ); > > > > + } > > > > #if defined (MDE_CPU_X64) > > > > StartupCode->Cr3Value = (UINT32) AsmReadCr3 (); > > > > StartupCode->LongJmpOffset += (UINT32) StartAddress; > > > > +#else > > > > + StartupCode->EnableExecuteDisble.Cr3Value = (UINT32) > AsmReadCr3 > > > > +(); > > > > > > (8) I think the (UINT32) cast can be dropped in this branch, but > > > it's not important. > > > > Prefer to keep it as Cr3Value is defined as UINT32 although UINTN == > > UINT32 at IA32 build. > > > > > > > > > #endif > > > > > > > > return EFI_SUCCESS; > > > > > > > > > > So, I think the patch is good, and that we should look into the KVM > > > problem separately. I'll follow up soon. > > > > > > Thanks > > > Laszlo > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
