On 06/20/16 10:07, Gao, Liming wrote: > Laszlo: > New changes: > > 1. Yes. nasm is listed first in BUILDRULEORDER so nasm source file will > be used. > > 3. The files in MdePkg are auto converted. Most of the files in other > packages manually are updated. They are updated based on the auto converted > version.
As I suspected, the manual conversion of the mode switches / mixed mode code turned out to be a bit risky. I ran the following tests with OVMF (all VMs used 4 or 8 VCPUs): Fedora guest: * Ia32 build with SMM_REQUIRE: S3 resume is broken. I'll follow up with a patch for this. (It can be squashed into your corresponding patch, but some credit in the commit message would be nice :) -- it wasn't easy to find and fix this issue.) * Ia32 build without SMM_REQUIRE: S3 works fine. * Ia32X64 build with SMM_REQUIRE: S3 resume is broken. It has the same error as the Ia32 build, but in two places. I'll follow up with another patch for this. * Ia32X64 build without SMM_REQUIRE: S3 works fine. * X64 build with SMM_REQUIRE: in this case, S3Resume2Pei doesn't support S3, so I disabled it on the QEMU command line, and tested only cold-boot. It works fine. * X64 build without SMM_REQUIRE: S3 works fine. For the rest of the patches in the series: it seems they are fine (my testing doesn't seem to have turned up any more errors), but I don't think I'll figure out what patches exactly (from the 351 :)) I should respond to with my Tested-by. Instead, I collected a list of object files that got built for my most common tests. I'm attaching that list. For those patches that modify files *only* on this list, if you guys are willing to track them down (maybe with a script?), I'm okay to add my T-b, I guess. After applying my two fixup patches mentioned above, I did some more tests with the Ia32X64 + SMM_REQUIRE build: - Tested key enrollment and SB enablement with the "EnrollDefaultKeys.efi" app. (This used to trigger an SMM stack overflow earlier, until we fixed it in 509f8425b75d^..0d0c245dfb14.) It works fine. - Tested an S3 cycle with a Windows 8.1 guest. It's a PASS. Thanks Laszlo > Verification > > 1. I compile .asm and .nasm source, and use VS dumpbin tool to dump the > obj file to compare the disassembly code. > > Yes. I give the example here for window users. Most linux user doesn't need > to configure NASM_PREFIX, because nasm is in system PATH. > > Thanks > Liming > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Saturday, June 18, 2016 5:51 AM > To: Gao, Liming <liming....@intel.com>; edk2-de...@ml01.01.org > Cc: Justen, Jordan L <jordan.l.jus...@intel.com> > Subject: Re: [edk2] [Patch 000/351] Convert EDK II core packages to NASM for > IA32/X64 > > On 06/16/16 03:46, Liming Gao wrote: >> These patches are available in https://github.com/lgao4/edk2.git nasm-v1. >> The nasm-v1 branch sha1 is 42bec457c575b6cb7c9fc1730cbea263ffce7b1c >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Liming Gao >> Signed-off-by: Jordan Justen >> >> These patches base on previous patches in >> http://thread.gmane.org/gmane.comp.bios.tianocore.devel/10977. >> New changes in these patches includes: >> 1. Keep ASM, GAS and NASM all. User can configure BUILDRULEORDER to specify >> which one will be used. > > The current catch-all rule seems to be > > Conf/tools_def.template:*_*_*_*_BUILDRULEORDER = nasm asm Asm ASM S s > > I guess that should continue working for me, right? > >> 2. Add NASM in new modules from UefiCpuPkg, IntelFsp2Pkg and >> IntelFsp2WrapperPkg. >> 3. Use 16bit and 32bit assembly code to replace hard code db. > > This last step looks pretty error prone -- was this automated, or > converted manually? > >> The done test includes: >> 1. Verify the output obj file from ASM and NASM to make sure the output obj >> have the same disassembly code. > > OTOH, binary comparison should make it pretty safe. > >> 2. Verify OvmfIa32X64 boot to Shell on VS2015x86. >> 3. Verify OVMF X64 when booting to arch linux, and with S3 suspend/resume. >> >> These patches convert these package to use NASM for IA32 & X64 >> * MdePkg >> * MdeModulePkg >> * IntelFrameworkModulePkg >> * UefiCpuPkg >> * SourceLevelDebugPkg >> * PcAtChipsetPkg >> * IntelFsp2Pkg >> * IntelFsp2WrapperPkg >> >> Package maintainers: Could you help review and verify your package? If you >> find >> any issues, please let us know. > > I'm adding this to my TODO list. Hopefully I can play a little with this > next week. > > Just to make sure, the above BUILDRULEORDER should ensure that the new > NASM files will be utilized in my build, right? > >> Notes: these patches will cause NASM compiler to be required for all IA32 >> and X64 >> toolchains with the default tool configuration. >> NASM compiler can be downloaded from http://www.nasm.us/ >> After download it, please configure NASM_PREFIX env to point to its >> directory. >> For example, I place nasm.exe in C:\nasm directory, then set >> NASM_PREFIX=C:\nasm\ > > (I think this advice is Windows-specific; for me the "nasm" binary is > simply on PATH -- /usr/bin/nasm.) > > 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
