On 11/11/14 21:11, Scott Duplichan wrote: > Laszlo Ersek [mailto:ler...@redhat.com] wrote: > > ]Apparently I wasn't quick enough to answer (which is not a surprise > ]these days), but I do have some points to raise... > ] > ]On 11/11/14 00:41, Jordan Justen wrote: > ]> On 2014-11-10 15:05:07, Scott Duplichan wrote: > ]>> OvmfPkg: fix VS2010 build failures > ]>> > ]>> Contributed-under: TianoCore Contribution Agreement 1.0 > ]>> Signed-off-by: Scott Duplichan <sc...@notabs.org> > ]>> --- > ]>> > ]>> Here is one way to get OvmfPkg building with Visual Studio again. > ]>> Several of the changes are needed to make OvmfPkg work with the > ]>> proposed NOOPT build, in combination with IA32 target. The > ]>> combination of 32-bit, Microsoft compiler, and optimization > ]>> disabled causes generation of helper function calls for multiply, > ]>> divide, shift, etc when operating on 64-bit integers. Rather than > ]>> make significant code changes to add explicit function calls, this > ]>> patch type casts some UINT64 integers to UINTN. That way, the 64-bit > ]>> build is unaffected. An argument that this safe is based on the > ]>> fact that the 64-bit values are addresses in most cases, and > ]>> 32-bit code cannot ordinarily access memory above 4GB. In one > ]>> case the 64-bit is the size of flash memory. If a 64MB flash > ]>> memory is used, it would have to be located above 4GB where 32-bit > ]>> code cannot access it. > ]>> > ]>> Other changes are due to Microsoft's warning about integer truncation. > ]>> EDK2 enables this warning for Microsoft tool chains, but not for > ]>> gcc tool chains. At least one integer truncation warning is not > ]>> handled well by the Microsoft compiler, even with version 2010. > ]>> Variable SegmentC holds the value 0xC0000, yet the compiler warns > ]>> it is left shifted by 12 bits. On the other hand, I don't understand > ]>> why the value is left shifted by 12. In any case, the final address > ]>> fits in 32 bits. > ]>> > ]>> For both Microsoft and gcc compilers, the #include "inttypes.h" fails > ]>> for me. The change to XenPvBlkDxe.inf is how I got it to work. > ]> > ]> Can you separate out the 'inttypes.h' Xen fix? > ] > ]The patch has the following diffstat: > ] > ] QemuFlashFvbServicesRuntimeDxe/FwBlockService.c | 6 +-- > ] QemuFlashFvbServicesRuntimeDxe/QemuFlash.c | 2 - > ] QemuVideoDxe/VbeShim.c | 12 +++--- > ] VirtioScsiDxe/VirtioScsi.c | 2 - > ] XenBusDxe/EventChannel.c | 6 +-- > ] XenBusDxe/GrantTable.c | 8 ++-- > ] XenBusDxe/XenBus.c | 6 +-- > ] XenBusDxe/XenHypercall.c | 8 ++-- > ] XenBusDxe/XenStore.c | 28 ++++++++-------- > ] XenPvBlkDxe/BlockFront.c | 18 +++++----- > ] XenPvBlkDxe/BlockIo.c | 2 - > ] XenPvBlkDxe/XenPvBlkDxe.inf | 1 > ] 12 files changed, 50 insertions(+), 49 deletions(-) > ] > ]Here's some changes I'd like to see -- these are not "hard requirements" > ]by any means, just how I'd prefer to work with this patch. > ] > ](1) Please post two independent patches (not even in one series): one > ]for Xen, the other for non-Xen. I'd like to review the non-Xen one, and > ]I think we can ask Anthony Perard to review the Xen one. > > OK, they are attached and appended. I would be happy to resubmit without > changes for the NOOPT build. I remember the NOOPT build easy to get > working. But that was for x64 code. NOOPT + IA32 + Microsoft is not > so easy. > > ](2) Please convince SVN somehow to include the function names in the > ]hunks (usually the "-p" option for "diff"). Otherwise it's hard to jump > ]to the containing function with any tags-capable editor. And, such > ]patches cannot be validated without seeing the context. > > OK, see attached/appended. > > ](3) In this case I have the opposite opinion about casts vs. the verbose > ]64-bit functions, like LShiftU64(), DivU64x32(), etc. Please consider > ]using that family wherever possible, instead of the casts. For example, > ]VirtioBlkDxe already uses ModU64x32() and DivU64x32(). > ] > ](4) The initial value of SegmentC is a (64-bit) linear address. It is > ]left-shifted by three nibbles for the UINT32 fields in question because > ]those are prepared for 16-bit real mode, and their representation is > ]segment:offset, packed in a UINT32. In other words, the 0x000C_xxxx > ]linear addresses are transformed into C000:xxxx, represented as > ]0xC000_xxxx. This is required by the VBE (VGA BIOS Extension) docs. > > OK thanks, I see it now. A vbeFarPtr contains segment:offset for use > in real mode and is never accessed as a flat address.
I took your "ovmf-ia32-msft-non-xen.patch" and reworked it to my taste. In addition I split it up a little bit too. I either kept your S-o-b per patch, or at least added a Suggested-by. I'm just about to submit that series (which includes two other patches as well). Can you please test if the patches resolve your build problem? If so, please respond with a Tested-by there. With regard to "ovmf-ia32-msft-xen.patch", I'll leave that to others -- preferably Anthony. Thanks! Laszlo ------------------------------------------------------------------------------ Comprehensive Server Monitoring with Site24x7. Monitor 10 servers for $9/Month. Get alerted through email, SMS, voice calls or mobile push notifications. Take corrective actions from your mobile device. http://pubads.g.doubleclick.net/gampad/clk?id=154624111&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel