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

Reply via email to