On Mon, 2013-12-09 at 09:38 +0000, Gao, Liming wrote: > David: > I have some comments. > 1. This patch just cleans up hard coded offset. So, it should still be > compiled to the same binary instruction to original one. Have you > compared the binary instruction? If the binary is different, how to > make sure its functionality is same?
I covered this in my original commit message. There are a few trivial differences where the assembler uses a 16-bit offset instead of an 8-bit offset for a register-based address, and I even gave an example: - ab: 8d 46 0c lea 0xc(%bp),%ax + ab: 8d 86 0d 00 lea 0xd(%bp),%ax As I said, these are harmless. I have tested the code on OVMF under KVM, and also on a real 32-bit platform (which required my later patch). > 2. I see Andrew mentions llvm doesn't support 16-bit mode. So, this > change will cause llvm/clang/Xcode build failure. Right? Yes. I prodded Andrew in private email about that when he didn't respond, in fact. And we have since had the discussion about the S3 resume trampoline which has similar issues. In that discussion, Andrew said: "Given the thrash on these [other] 2 files, maybe it worth doing the GNU native mnemonics version to get things working and then porting that back to llvm after it is stable?" Given that we're already planning to do that elsewhere in EDKII, I think it's a good idea here too. In fact, perhaps I should have answered your question by saying "No. It won't break the LLVM build because the LLVM build will be broken already." :) Let's fix things using sane mnemonics that actually say what they mean — and then *later* once it's entirely stable we can look at making it unmaintainable by turning it into .byte arrays or whatever we need to do to make it build with broken toolchains... if they really haven't fixed the toolchain by then. In the interim, I will personally fund a licence for GNU gas for any affected individuals who need it to compile the code. > 3. I see your another change to simplify the code. What's the benefit > of this change? That one isn't just a simplification. It's a fix for a violation of the SDM which causes failures on real hardware. See Vol 3A §9.9.2 "Switching back to Real-Address Mode". The existing code violates these requirements in *many* ways. The most important violation for me right now is that the first instruction after clearing CR0.PE *must* be a far JMP. We currently do some other stuff, followed by a far RET. Which doesn't work on the board I'm playing with this week. I still need to try to set up a Windows build environment (or at least get a copy of MASM.EXE I can run under Wine) and test the corresponding changes in the Ia32/Thunk16.asm version, so I can submit that patch for real. > 4. For MdePkg\Library\BaseLib\X64\Thunk16.S, have you found the > similar issue? That's a different can of worms, which *also* violates the SDM in some of the same ways, and also wants cleaning up. But that's a separate sequence of patches and not actually on my critical path to making things work on real hardware this week. However, since you mention it, we should probably sync up with the code in EdkCompatibilityPkg/Foundation/Library/EdkIIGlueLib/Library/BaseLib/ which *is* almost identical. I note it's already missed out on the most recent bug-fix (r14037). -- David Woodhouse Open Source Technology Centre david.woodho...@intel.com Intel Corporation
smime.p7s
Description: S/MIME cryptographic signature
------------------------------------------------------------------------------ Sponsored by Intel(R) XDK Develop, test and display web and hybrid apps with a single code base. Download it for free now! http://pubads.g.doubleclick.net/gampad/clk?id=111408631&iu=/4140/ostg.clktrk
_______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
