Re: [PATCH v5 0/6] Add ARM support for VS2017

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 26 January 2018 at 15:05, Gao, Liming <liming.gao@intel.com> wrote:
> After the change in ArmPkg is reviewed, I will help push this patch serial.
>

That change is

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Thanks.

>> -----Original Message-----
>> From: Pete Batard [mailto:pete@akeo.ie]
>> Sent: Thursday, January 25, 2018 7:32 PM
>> To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
>> Cc: ard.biesheuvel@linaro.org; eugene@hp.com
>> Subject: Re: [PATCH v5 0/6] Add ARM support for VS2017
>>
>> Thanks Liming.
>>
>>  From the way you framed your comment, I'm not completely sure if
>> there's action still needed on my side...
>>
>> Do I actually need to resubmit a patch set with "Reviewed By"?
>> Or, now that each patch should have flagged as reviewed, are we simply
>> supposed to wait for formal integration?
>>
>> Regards,
>>
>> /Pete
>>
>> On 2018.01.25 10:27, Gao, Liming wrote:
>> > Pete:
>> >    The changes in BaseTools and MdePkg are good to me. You can add my R-B.
>> >
>> >> -----Original Message-----
>> >> From: Pete Batard [mailto:pete@akeo.ie]
>> >> Sent: Friday, January 12, 2018 9:33 PM
>> >> To: edk2-devel@lists.01.org
>> >> Cc: Gao, Liming <liming.gao@intel.com>; ard.biesheuvel@linaro.org;
>> >> eugene@hp.com
>> >> Subject: [PATCH v5 0/6] Add ARM support for VS2017
>> >>
>> >> (Same as v4, except for the AREA names where we replaced the RVCT macros,
>> >> in
>> >> patch 4/6, so that it matches what would have been produced with the
>> >> macros)
>> >>
>> >>
>> >> The following series adds ARM compilation support for the VS2017 toolchain.
>> >> * PATCH 1 targets the disabling of VS Level 4 warnings. The disabled warnings
>> >>   for ARM are now aligned with IA32 and X64.
>> >> * PATCH 2 adds a NULL handler for the base stack check, since this is a GCC
>> >>   functionality.
>> >> * PATCH 3 updates MdePkg/Library/BaseLib so that the RVCT assembly
>> >> sources
>> >>   are also used for MSFT.
>> >> * PATCH 4 adds the required compiler intrinsics replacements for division,
>> >>   shift, by reusing the RVCT code, as well as memset/memcpy.
>> >> * PATCH 5 adds variable argument handlers for print output. Note that this
>> >>   is done without relying on any external headers, with the VA_ARG macro
>> >>   having been reverse engineered from MSFT ARM assembly output.
>> >> * PATCH 6 enables the selection of ARM in the conf templates.
>> >>
>> >> With these patches, VS2017 toolchain users should be able to compile
>> >> regular UEFI ARM applications using EDK2. Note that, unlike ARM64 support,
>> >> ARM support does not require a specific update of Visual Studio 2017, as
>> >> the ARM toolchain has been available from the very first release.
>> >>
>> >> Additional notes:
>> >>
>> >> We tested compiling and running the full UEFI Shell with this series, as
>> >> well as a small set of applications and drivers, and found no issues.
>> >> With an additional patch [1], it is also possible to use this proposal to
>> >> compile a complete QEMU ARM firmware. As the patch shows, the changes
>> >> that
>> >> need to be applied to the EDK2 sources to achieve this are actually very
>> >> minimal.
>> >>
>> >> However, the generated firmware does not currently boot, possibly because
>> >> of the following warnings being generated by the MS compiler:
>> >> - ArmCpuDxe.dll : warning LNK4072: section count 118 exceeds max (96);
>> >> image may not run
>> >> - UiApp.dll : warning LNK4072: section count 113 exceeds max (96); image may
>> >> not run
>> >>
>> >> As far as I could see, the section count max is hardcoded so a workaround
>> >> would be needed to address those.
>> >>
>> >> Also, because the VS2017 ARM compiler forces a section alignment of 4096
>> >> bytes (which in turn forces use to use /FILEALIGN:4096 as a linker option
>> >> for the firmware generation), the generated firmware exceeds 2MB and we
>> >> had to double its size to 4MB.
>> >>
>> >> At this stage, since the goal of this series is to allow users to compile
>> >> regular ARM UEFI applications using the VS2017 toolchain, I have no plans
>> >> to spend more time on the QEMU firmware issues, especially as I suspect
>> >> that reducing the firmware size back to 2 MB may not be achievable without
>> >> Microsoft altering their compiler. I am however hopeful that ARM
>> >> specialists can take this matter over eventually...
>> >>
>> >> Regards,
>> >>
>> >> /Pete
>> >>
>> >> [1]
>> >> https://github.com/pbatard/edk2/commit/c4ce41094a46f4f3dc7ccc64a906048
>> >> 13f037b13
>> >>
>> >> Pete Batard (6):
>> >>   MdePkg: Disable some Level 4 warnings for VS2017/ARM
>> >>   MdePkg/Library/BaseStackCheckLib: Add Null handler for VS2017/ARM
>> >>   MdePkg/Library/BaseLib: Enable VS2017/ARM builds
>> >>   ArmPkg/Library/CompilerIntrinsicsLib: Enable VS2017/ARM builds
>> >>   MdePkg/Include: Add VA list support for VS2017/ARM
>> >>   BaseTools/Conf: Add VS2017/ARM support
>> >>
>> >> ArmPkg/Library/CompilerIntrinsicsLib/Arm/div.asm               | 43 +++++++--
>> >> ArmPkg/Library/CompilerIntrinsicsLib/Arm/ldivmod.asm           | 40 ++++++--
>> >> ArmPkg/Library/CompilerIntrinsicsLib/Arm/llsr.asm              | 22 +++--
>> >> ArmPkg/Library/CompilerIntrinsicsLib/Arm/uldiv.asm             | 29 +++++-
>> >> ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf | 16 +++-
>> >> ArmPkg/Library/CompilerIntrinsicsLib/memcpy_ms.c               | 34 +++++++
>> >> ArmPkg/Library/CompilerIntrinsicsLib/memset_ms.c               | 33 +++++++
>> >> BaseTools/Conf/build_rule.template                             | 31 ++++++-
>> >> BaseTools/Conf/tools_def.template                              | 31 +++++++
>> >> MdePkg/Include/Arm/ProcessorBind.h                             | 96
>> >> +++++++++++++++-----
>> >> MdePkg/Include/Base.h                                          | 13 +++
>> >> MdePkg/Library/BaseLib/Arm/CpuBreakpoint.asm                   |  5 +-
>> >> MdePkg/Library/BaseLib/BaseLib.inf                             | 16 +++-
>> >> MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf         |  5 +-
>> >> MdePkg/Library/BaseStackCheckLib/BaseStackCheckNull.c          | 18 ++++
>> >> 15 files changed, 372 insertions(+), 60 deletions(-)
>> >> create mode 100644 ArmPkg/Library/CompilerIntrinsicsLib/memcpy_ms.c
>> >> create mode 100644 ArmPkg/Library/CompilerIntrinsicsLib/memset_ms.c
>> >> create mode 100644
>> >> MdePkg/Library/BaseStackCheckLib/BaseStackCheckNull.c
>> >>
>> >> --
>> >> 2.9.3.windows.2
>> >
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel