* [PATCH 0/4] Add ARM64 support for VS2017
@ 2018-02-14 13:08 Pete Batard
0 siblings, 0 replies; 17+ messages in thread
From: Pete Batard @ 2018-02-14 13:08 UTC (permalink / raw)
To: edk2-devel; +Cc: liming.gao, ard.biesheuvel
This series completes VS2017 support by enabling AARCH64 compilation.
* PATCH 1 targets the disabling of VS Level 4 warnings. The disabled
warnings for ARM64 are the same as the ones for IA32, X64 and ARM.
* PATCH 2 adds assembly source in MdePkg/Library/BaseLib for various low
level required functions. These new assembly files were converted from
their GCC version, with minor changes applied to make them palatable
to the MSFT assembler.
* PATCH 3 adds variable argument handlers for print output. This is
achieved without relying on any external toolchain headers. However
a call to the __va_start() compiler intrinsic function is now being
used for the VA_START macros, which we apply for ARM as well.
* PATCH 4 enables the selection of ARM64 in the conf templates.
One item of note is that the build options for ARM64 are the same as
for ARM, except for /BASE:0 which was removed to avoid error:
'invalid base address 0x0; ARM64 image cannot have base address below 4GB'
With these patches, VS2017 toolchain users should be able to compile
regular UEFI ARM64 applications using EDK2.
Note however that ARM64 support requires the use of Visual Studio 2017
Update 4 or later (a.k.a. v15.4), as native ARM64 compilation was not
included in any version of Visual Studio prior to that.
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 saw no issues.
Since we also modified the VA_START() macro for ARM, we also re-ran
similar tests for ARM, to confirm that there was no regression there.
Finally, we did not test the generation of a complete QEMU ARM64 firmware
as it requires porting a handful of assembly sources, that don't exist
yet, and our focus is with the generation of working AARCH64 drivers or
applications. Hopefully, this can be tackled as VS2017/ARM64 sees more
usage...
Regards,
/Pete
Pete Batard (4):
MdePkg: Disable some Level 4 warnings for VS2017/ARM64
MdePkg/Library/BaseLib: Enable VS2017/ARM64 builds
MdePkg/Include: Add VA list support for VS2017/ARM64
BaseTools/Conf: Add VS2017/ARM64 support
BaseTools/Conf/build_rule.template | 2 +-
BaseTools/Conf/tools_def.template | 32 ++++++-
MdePkg/Include/AArch64/ProcessorBind.h | 53 +++++++++-
MdePkg/Include/Base.h | 7 +-
MdePkg/Library/BaseLib/AArch64/CpuBreakpoint.asm | 39 ++++++++
MdePkg/Library/BaseLib/AArch64/DisableInterrupts.asm | 37 +++++++
MdePkg/Library/BaseLib/AArch64/EnableInterrupts.asm | 37 +++++++
MdePkg/Library/BaseLib/AArch64/GetInterruptsState.asm | 49 ++++++++++
MdePkg/Library/BaseLib/AArch64/MemoryFence.asm | 38 ++++++++
MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm | 101 ++++++++++++++++++++
MdePkg/Library/BaseLib/AArch64/SwitchStack.asm | 69 +++++++++++++
MdePkg/Library/BaseLib/BaseLib.inf | 8 ++
12 files changed, 463 insertions(+), 9 deletions(-)
create mode 100644 MdePkg/Library/BaseLib/AArch64/CpuBreakpoint.asm
create mode 100644 MdePkg/Library/BaseLib/AArch64/DisableInterrupts.asm
create mode 100644 MdePkg/Library/BaseLib/AArch64/EnableInterrupts.asm
create mode 100644 MdePkg/Library/BaseLib/AArch64/GetInterruptsState.asm
create mode 100644 MdePkg/Library/BaseLib/AArch64/MemoryFence.asm
create mode 100644 MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
create mode 100644 MdePkg/Library/BaseLib/AArch64/SwitchStack.asm
--
2.9.3.windows.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 0/4] Add ARM64 support for VS2017
@ 2018-02-23 9:49 Pete Batard
2018-02-23 9:50 ` [PATCH 1/4] MdePkg: Disable some Level 4 warnings for VS2017/ARM64 Pete Batard
` (5 more replies)
0 siblings, 6 replies; 17+ messages in thread
From: Pete Batard @ 2018-02-23 9:49 UTC (permalink / raw)
To: edk2-devel; +Cc: liming.gao, ard.biesheuvel
This is v2, which just removes a redundant #if defined(_MSC_EXTENSIONS) in 1/4.
This series completes VS2017 support by enabling AARCH64 compilation.
* PATCH 1 targets the disabling of VS Level 4 warnings. The disabled
warnings for ARM64 are the same as the ones for IA32, X64 and ARM.
* PATCH 2 adds assembly source in MdePkg/Library/BaseLib for various low
level required functions. These new assembly files were converted from
their GCC version, with minor changes applied to make them palatable
to the MSFT assembler.
* PATCH 3 adds variable argument handlers for print output. This is
achieved without relying on any external toolchain headers. However
a call to the __va_start() compiler intrinsic function is now being
used for the VA_START macros, which we apply for ARM as well.
* PATCH 4 enables the selection of ARM64 in the conf templates.
One item of note is that the build options for ARM64 are the same as
for ARM, except for /BASE:0 which was removed to avoid error:
'invalid base address 0x0; ARM64 image cannot have base address below 4GB'
With these patches, VS2017 toolchain users should be able to compile
regular UEFI ARM64 applications using EDK2.
Note however that ARM64 support requires the use of Visual Studio 2017
Update 4 or later (a.k.a. v15.4), as native ARM64 compilation was not
included in any version of Visual Studio prior to that.
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 saw no issues.
Since we also modified the VA_START() macro for ARM, we also re-ran
similar tests for ARM, to confirm that there was no regression there.
Finally, we did not test the generation of a complete QEMU ARM64 firmware
as it requires porting a handful of assembly sources, that don't exist
yet, and our focus is with the generation of working AARCH64 drivers or
applications. Hopefully, this can be tackled as VS2017/ARM64 sees more
usage...
Regards,
/Pete
Pete Batard (4):
MdePkg: Disable some Level 4 warnings for VS2017/ARM64
MdePkg/Library/BaseLib: Enable VS2017/ARM64 builds
MdePkg/Include: Add VA list support for VS2017/ARM64
BaseTools/Conf: Add VS2017/ARM64 support
BaseTools/Conf/build_rule.template | 2 +-
BaseTools/Conf/tools_def.template | 32 ++++++-
MdePkg/Include/AArch64/ProcessorBind.h | 53 +++++++++-
MdePkg/Include/Base.h | 7 +-
MdePkg/Library/BaseLib/AArch64/CpuBreakpoint.asm | 39 ++++++++
MdePkg/Library/BaseLib/AArch64/DisableInterrupts.asm | 37 +++++++
MdePkg/Library/BaseLib/AArch64/EnableInterrupts.asm | 37 +++++++
MdePkg/Library/BaseLib/AArch64/GetInterruptsState.asm | 49 ++++++++++
MdePkg/Library/BaseLib/AArch64/MemoryFence.asm | 38 ++++++++
MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm | 101 ++++++++++++++++++++
MdePkg/Library/BaseLib/AArch64/SwitchStack.asm | 69 +++++++++++++
MdePkg/Library/BaseLib/BaseLib.inf | 8 ++
12 files changed, 463 insertions(+), 9 deletions(-)
create mode 100644 MdePkg/Library/BaseLib/AArch64/CpuBreakpoint.asm
create mode 100644 MdePkg/Library/BaseLib/AArch64/DisableInterrupts.asm
create mode 100644 MdePkg/Library/BaseLib/AArch64/EnableInterrupts.asm
create mode 100644 MdePkg/Library/BaseLib/AArch64/GetInterruptsState.asm
create mode 100644 MdePkg/Library/BaseLib/AArch64/MemoryFence.asm
create mode 100644 MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
create mode 100644 MdePkg/Library/BaseLib/AArch64/SwitchStack.asm
--
2.9.3.windows.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/4] MdePkg: Disable some Level 4 warnings for VS2017/ARM64
2018-02-23 9:49 [PATCH 0/4] Add ARM64 support for VS2017 Pete Batard
@ 2018-02-23 9:50 ` Pete Batard
2018-02-23 9:50 ` [PATCH 2/4] MdePkg/Library/BaseLib: Enable VS2017/ARM64 builds Pete Batard
` (4 subsequent siblings)
5 siblings, 0 replies; 17+ messages in thread
From: Pete Batard @ 2018-02-23 9:50 UTC (permalink / raw)
To: edk2-devel; +Cc: liming.gao, ard.biesheuvel
We disable the exact same warnings as IA32 and X64.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pete Batard <pete@akeo.ie>
---
MdePkg/Include/AArch64/ProcessorBind.h | 53 +++++++++++++++++++-
1 file changed, 51 insertions(+), 2 deletions(-)
diff --git a/MdePkg/Include/AArch64/ProcessorBind.h b/MdePkg/Include/AArch64/ProcessorBind.h
index bc473562f9e5..968c18f915ae 100644
--- a/MdePkg/Include/AArch64/ProcessorBind.h
+++ b/MdePkg/Include/AArch64/ProcessorBind.h
@@ -1,7 +1,7 @@
/** @file
Processor or Compiler specific defines and types for AArch64.
- Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
Portions copyright (c) 2011 - 2013, ARM Ltd. All rights reserved.<BR>
@@ -30,7 +30,53 @@
#pragma pack()
#endif
-#if _MSC_EXTENSIONS
+#if defined(_MSC_EXTENSIONS)
+
+//
+// Disable some level 4 compilation warnings (same as IA32 and X64)
+//
+
+//
+// Disabling bitfield type checking warnings.
+//
+#pragma warning ( disable : 4214 )
+
+//
+// Disabling the unreferenced formal parameter warnings.
+//
+#pragma warning ( disable : 4100 )
+
+//
+// Disable slightly different base types warning as CHAR8 * can not be set
+// to a constant string.
+//
+#pragma warning ( disable : 4057 )
+
+//
+// ASSERT(FALSE) or while (TRUE) are legal constructs so suppress this warning
+//
+#pragma warning ( disable : 4127 )
+
+//
+// This warning is caused by functions defined but not used. For precompiled header only.
+//
+#pragma warning ( disable : 4505 )
+
+//
+// This warning is caused by empty (after preprocessing) source file. For precompiled header only.
+//
+#pragma warning ( disable : 4206 )
+
+//
+// Disable 'potentially uninitialized local variable X used' warnings
+//
+#pragma warning ( disable : 4701 )
+
+//
+// Disable 'potentially uninitialized local pointer variable X used' warnings
+//
+#pragma warning ( disable : 4703 )
+
//
// use Microsoft* C compiler dependent integer width types
//
@@ -45,7 +91,9 @@
typedef unsigned char UINT8;
typedef char CHAR8;
typedef signed char INT8;
+
#else
+
//
// Assume standard AARCH64 alignment.
//
@@ -60,6 +108,7 @@
typedef unsigned char UINT8;
typedef char CHAR8;
typedef signed char INT8;
+
#endif
///
--
2.9.3.windows.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/4] MdePkg/Library/BaseLib: Enable VS2017/ARM64 builds
2018-02-23 9:49 [PATCH 0/4] Add ARM64 support for VS2017 Pete Batard
2018-02-23 9:50 ` [PATCH 1/4] MdePkg: Disable some Level 4 warnings for VS2017/ARM64 Pete Batard
@ 2018-02-23 9:50 ` Pete Batard
2018-02-23 9:50 ` [PATCH 3/4] MdePkg/Include: Add VA list support for VS2017/ARM64 Pete Batard
` (3 subsequent siblings)
5 siblings, 0 replies; 17+ messages in thread
From: Pete Batard @ 2018-02-23 9:50 UTC (permalink / raw)
To: edk2-devel; +Cc: liming.gao, ard.biesheuvel
Required GCC assembly files are converted for the MSFT assembler
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pete Batard <pete@akeo.ie>
---
MdePkg/Library/BaseLib/AArch64/CpuBreakpoint.asm | 39 ++++++++
MdePkg/Library/BaseLib/AArch64/DisableInterrupts.asm | 37 +++++++
MdePkg/Library/BaseLib/AArch64/EnableInterrupts.asm | 37 +++++++
MdePkg/Library/BaseLib/AArch64/GetInterruptsState.asm | 49 ++++++++++
MdePkg/Library/BaseLib/AArch64/MemoryFence.asm | 38 ++++++++
MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm | 101 ++++++++++++++++++++
MdePkg/Library/BaseLib/AArch64/SwitchStack.asm | 69 +++++++++++++
MdePkg/Library/BaseLib/BaseLib.inf | 8 ++
8 files changed, 378 insertions(+)
diff --git a/MdePkg/Library/BaseLib/AArch64/CpuBreakpoint.asm b/MdePkg/Library/BaseLib/AArch64/CpuBreakpoint.asm
new file mode 100644
index 000000000000..17e993f5b77e
--- /dev/null
+++ b/MdePkg/Library/BaseLib/AArch64/CpuBreakpoint.asm
@@ -0,0 +1,39 @@
+;------------------------------------------------------------------------------
+;
+; CpuBreakpoint() for AArch64
+;
+; Copyright (c) 2006 - 2009, Intel Corporation. All rights reserved.<BR>
+; Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
+; Portions copyright (c) 2011 - 2013, ARM Ltd. All rights reserved.<BR>
+; This program and the accompanying materials
+; are licensed and made available under the terms and conditions of the BSD License
+; which accompanies this distribution. The full text of the license may be found at
+; http://opensource.org/licenses/bsd-license.php.
+;
+; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+;
+;------------------------------------------------------------------------------
+
+
+ EXPORT CpuBreakpoint
+ AREA BaseLib_LowLevel, CODE, READONLY
+
+;/**
+; Generates a breakpoint on the CPU.
+;
+; Generates a breakpoint on the CPU. The breakpoint must be implemented such
+; that code can resume normal execution after the breakpoint.
+;
+;**/
+;VOID
+;EFIAPI
+;CpuBreakpoint (
+; VOID
+; );
+;
+CpuBreakpoint
+ svc 0xdbdb // Superviser exception. Takes 16bit arg -> Armv7 had 'swi' here.
+ ret
+
+ END
diff --git a/MdePkg/Library/BaseLib/AArch64/DisableInterrupts.asm b/MdePkg/Library/BaseLib/AArch64/DisableInterrupts.asm
new file mode 100644
index 000000000000..498493454c7d
--- /dev/null
+++ b/MdePkg/Library/BaseLib/AArch64/DisableInterrupts.asm
@@ -0,0 +1,37 @@
+;------------------------------------------------------------------------------
+;
+; DisableInterrupts() for AArch64
+;
+; Copyright (c) 2006 - 2009, Intel Corporation. All rights reserved.<BR>
+; Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
+; Portions copyright (c) 2011 - 2013, ARM Ltd. All rights reserved.<BR>
+; This program and the accompanying materials
+; are licensed and made available under the terms and conditions of the BSD License
+; which accompanies this distribution. The full text of the license may be found at
+; http://opensource.org/licenses/bsd-license.php.
+;
+; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+;
+;------------------------------------------------------------------------------
+
+ EXPORT DisableInterrupts
+ AREA BaseLib_LowLevel, CODE, READONLY
+
+DAIF_WR_IRQ_BIT EQU (1 << 1)
+
+;/**
+; Disables CPU interrupts.
+;
+;**/
+;VOID
+;EFIAPI
+;DisableInterrupts (
+; VOID
+; );
+;
+DisableInterrupts
+ msr daifset, #DAIF_WR_IRQ_BIT
+ ret
+
+ END
diff --git a/MdePkg/Library/BaseLib/AArch64/EnableInterrupts.asm b/MdePkg/Library/BaseLib/AArch64/EnableInterrupts.asm
new file mode 100644
index 000000000000..ec3d6e45ff8a
--- /dev/null
+++ b/MdePkg/Library/BaseLib/AArch64/EnableInterrupts.asm
@@ -0,0 +1,37 @@
+;------------------------------------------------------------------------------
+;
+; EnableInterrupts() for AArch64
+;
+; Copyright (c) 2006 - 2009, Intel Corporation. All rights reserved.<BR>
+; Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
+; Portions copyright (c) 2011 - 2013, ARM Ltd. All rights reserved.<BR>
+; This program and the accompanying materials
+; are licensed and made available under the terms and conditions of the BSD License
+; which accompanies this distribution. The full text of the license may be found at
+; http://opensource.org/licenses/bsd-license.php.
+;
+; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+;
+;------------------------------------------------------------------------------
+
+ EXPORT EnableInterrupts
+ AREA BaseLib_LowLevel, CODE, READONLY
+
+DAIF_WR_IRQ_BIT EQU (1 << 1)
+
+;/**
+; Enables CPU interrupts.
+;
+;**/
+;VOID
+;EFIAPI
+;EnableInterrupts (
+; VOID
+; );
+;
+EnableInterrupts
+ msr daifclr, #DAIF_WR_IRQ_BIT
+ ret
+
+ END
diff --git a/MdePkg/Library/BaseLib/AArch64/GetInterruptsState.asm b/MdePkg/Library/BaseLib/AArch64/GetInterruptsState.asm
new file mode 100644
index 000000000000..d64b0d513ce3
--- /dev/null
+++ b/MdePkg/Library/BaseLib/AArch64/GetInterruptsState.asm
@@ -0,0 +1,49 @@
+;------------------------------------------------------------------------------
+;
+; GetInterruptState() function for AArch64
+;
+; Copyright (c) 2006 - 2009, Intel Corporation. All rights reserved.<BR>
+; Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
+; Portions copyright (c) 2011 - 2013, ARM Ltd. All rights reserved.<BR>
+; This program and the accompanying materials
+; are licensed and made available under the terms and conditions of the BSD License
+; which accompanies this distribution. The full text of the license may be found at
+; http://opensource.org/licenses/bsd-license.php.
+;
+; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+;
+;------------------------------------------------------------------------------
+
+ EXPORT GetInterruptState
+ AREA BaseLib_LowLevel, CODE, READONLY
+
+DAIF_RD_IRQ_BIT EQU (1 << 7)
+
+;/**
+; Retrieves the current CPU interrupt state.
+;
+; Returns TRUE is interrupts are currently enabled. Otherwise
+; returns FALSE.
+;
+; @retval TRUE CPU interrupts are enabled.
+; @retval FALSE CPU interrupts are disabled.
+;
+;**/
+;
+;BOOLEAN
+;EFIAPI
+;GetInterruptState (
+; VOID
+; );
+;
+GetInterruptState
+ mrs x0, daif
+ mov w0, wzr
+ tst x0, #DAIF_RD_IRQ_BIT // Check IRQ mask; set Z=1 if clear/unmasked
+ bne exit // if Z=1 (eq) return 1, else 0
+ mov w0, #1
+exit
+ ret
+
+ END
diff --git a/MdePkg/Library/BaseLib/AArch64/MemoryFence.asm b/MdePkg/Library/BaseLib/AArch64/MemoryFence.asm
new file mode 100644
index 000000000000..84dede698ee0
--- /dev/null
+++ b/MdePkg/Library/BaseLib/AArch64/MemoryFence.asm
@@ -0,0 +1,38 @@
+;------------------------------------------------------------------------------
+;
+; MemoryFence() for AArch64
+;
+; Copyright (c) 2013, ARM Ltd. All rights reserved.
+;
+; This program and the accompanying materials
+; are licensed and made available under the terms and conditions of the BSD License
+; which accompanies this distribution. The full text of the license may be found at
+; http://opensource.org/licenses/bsd-license.php.
+;
+; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+;
+;------------------------------------------------------------------------------
+
+ EXPORT MemoryFence
+ AREA BaseLib_LowLevel, CODE, READONLY
+
+;/**
+; Used to serialize load and store operations.
+;
+; All loads and stores that proceed calls to this function are guaranteed to be
+; globally visible when this function returns.
+;
+;**/
+;VOID
+;EFIAPI
+;MemoryFence (
+; VOID
+; );
+;
+MemoryFence
+ // System wide Data Memory Barrier.
+ dmb sy
+ ret
+
+ END
diff --git a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
new file mode 100644
index 000000000000..e0a9715ff2d1
--- /dev/null
+++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
@@ -0,0 +1,101 @@
+;------------------------------------------------------------------------------
+;
+; Copyright (c) 2009-2013, ARM Ltd. All rights reserved.
+; This program and the accompanying materials
+; are licensed and made available under the terms and conditions of the BSD License
+; which accompanies this distribution. The full text of the license may be found at
+; http://opensource.org/licenses/bsd-license.php.
+;
+; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+;
+;------------------------------------------------------------------------------
+
+ EXPORT SetJump
+ EXPORT InternalLongJump
+ AREA BaseLib_LowLevel, CODE, READONLY
+
+#define GPR_LAYOUT \
+ REG_PAIR (x19, x20, #0); \
+ REG_PAIR (x21, x22, #16); \
+ REG_PAIR (x23, x24, #32); \
+ REG_PAIR (x25, x26, #48); \
+ REG_PAIR (x27, x28, #64); \
+ REG_PAIR (x29, x30, #80);/*FP, LR*/ \
+ REG_ONE (x16, #96) /*IP0*/
+
+#define FPR_LAYOUT \
+ REG_PAIR ( d8, d9, #112); \
+ REG_PAIR (d10, d11, #128); \
+ REG_PAIR (d12, d13, #144); \
+ REG_PAIR (d14, d15, #160);
+
+;/**
+; Saves the current CPU context that can be restored with a call to LongJump() and returns 0.#
+;
+; Saves the current CPU context in the buffer specified by JumpBuffer and returns 0. The initial
+; call to SetJump() must always return 0. Subsequent calls to LongJump() cause a non-zero
+; value to be returned by SetJump().
+;
+; If JumpBuffer is NULL, then ASSERT().
+; For IPF CPUs, if JumpBuffer is not aligned on a 16-byte boundary, then ASSERT().
+;
+; @param JumpBuffer A pointer to CPU context buffer.
+;
+;**/
+;
+;UINTN
+;EFIAPI
+;SetJump (
+; IN BASE_LIBRARY_JUMP_BUFFER *JumpBuffer // X0
+; );
+;
+SetJump
+ mov x16, sp // use IP0 so save SP
+#define REG_PAIR(REG1, REG2, OFFS) stp REG1, REG2, [x0, OFFS]
+#define REG_ONE(REG1, OFFS) str REG1, [x0, OFFS]
+ GPR_LAYOUT
+ FPR_LAYOUT
+#undef REG_PAIR
+#undef REG_ONE
+ mov w0, #0
+ ret
+
+;/**
+; Restores the CPU context that was saved with SetJump().#
+;
+; Restores the CPU context from the buffer specified by JumpBuffer.
+; This function never returns to the caller.
+; Instead is resumes execution based on the state of JumpBuffer.
+;
+; @param JumpBuffer A pointer to CPU context buffer.
+; @param Value The value to return when the SetJump() context is restored.
+;
+;**/
+;VOID
+;EFIAPI
+;InternalLongJump (
+; IN BASE_LIBRARY_JUMP_BUFFER *JumpBuffer, // X0
+; IN UINTN Value // X1
+; );
+;
+InternalLongJump
+#define REG_PAIR(REG1, REG2, OFFS) ldp REG1, REG2, [x0, OFFS]
+#define REG_ONE(REG1, OFFS) ldr REG1, [x0, OFFS]
+ GPR_LAYOUT
+ FPR_LAYOUT
+#undef REG_PAIR
+#undef REG_ONE
+ mov sp, x16
+ cmp w1, #0
+ mov w0, #1
+ beq exit
+ mov w0, w1
+exit
+ // use br not ret, as ret is guaranteed to mispredict
+ br x30
+
+ASM_FUNCTION_REMOVE_IF_UNREFERENCED
+
+ END
+
diff --git a/MdePkg/Library/BaseLib/AArch64/SwitchStack.asm b/MdePkg/Library/BaseLib/AArch64/SwitchStack.asm
new file mode 100644
index 000000000000..c1b2de07e205
--- /dev/null
+++ b/MdePkg/Library/BaseLib/AArch64/SwitchStack.asm
@@ -0,0 +1,69 @@
+//------------------------------------------------------------------------------
+//
+// Copyright (c) 2006 - 2009, Intel Corporation. All rights reserved.<BR>
+// Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
+// Portions copyright (c) 2011 - 2013, ARM Limited. All rights reserved.<BR>
+// This program and the accompanying materials
+// are licensed and made available under the terms and conditions of the BSD License
+// which accompanies this distribution. The full text of the license may be found at
+// http://opensource.org/licenses/bsd-license.php.
+//
+// THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+// WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+//
+//------------------------------------------------------------------------------
+
+ EXPORT InternalSwitchStackAsm
+ EXPORT CpuPause
+ AREA BaseLib_LowLevel, CODE, READONLY
+
+/**
+//
+// This allows the caller to switch the stack and goes to the new entry point
+//
+// @param EntryPoint The pointer to the location to enter
+// @param Context Parameter to pass in
+// @param Context2 Parameter2 to pass in
+// @param NewStack New Location of the stack
+//
+// @return Nothing. Goes to the Entry Point passing in the new parameters
+//
+VOID
+EFIAPI
+InternalSwitchStackAsm (
+ SWITCH_STACK_ENTRY_POINT EntryPoint,
+ VOID *Context,
+ VOID *Context2,
+ VOID *NewStack
+ );
+**/
+InternalSwitchStackAsm
+ mov x29, #0
+ mov x30, x0
+ mov sp, x3
+ mov x0, x1
+ mov x1, x2
+ ret
+
+/**
+//
+// Requests CPU to pause for a short period of time.
+//
+// Requests CPU to pause for a short period of time. Typically used in MP
+// systems to prevent memory starvation while waiting for a spin lock.
+//
+VOID
+EFIAPI
+CpuPause (
+ VOID
+ )
+**/
+CpuPause
+ nop
+ nop
+ nop
+ nop
+ nop
+ ret
+
+ END
diff --git a/MdePkg/Library/BaseLib/BaseLib.inf b/MdePkg/Library/BaseLib/BaseLib.inf
index 3c07e6bad977..80d00ebed75b 100644
--- a/MdePkg/Library/BaseLib/BaseLib.inf
+++ b/MdePkg/Library/BaseLib/BaseLib.inf
@@ -867,6 +867,14 @@ [Sources.AARCH64]
AArch64/SetJumpLongJump.S | GCC
AArch64/CpuBreakpoint.S | GCC
+ AArch64/MemoryFence.asm | MSFT
+ AArch64/SwitchStack.asm | MSFT
+ AArch64/EnableInterrupts.asm | MSFT
+ AArch64/DisableInterrupts.asm | MSFT
+ AArch64/GetInterruptsState.asm | MSFT
+ AArch64/SetJumpLongJump.asm | MSFT
+ AArch64/CpuBreakpoint.asm | MSFT
+
[Packages]
MdePkg/MdePkg.dec
--
2.9.3.windows.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 3/4] MdePkg/Include: Add VA list support for VS2017/ARM64
2018-02-23 9:49 [PATCH 0/4] Add ARM64 support for VS2017 Pete Batard
2018-02-23 9:50 ` [PATCH 1/4] MdePkg: Disable some Level 4 warnings for VS2017/ARM64 Pete Batard
2018-02-23 9:50 ` [PATCH 2/4] MdePkg/Library/BaseLib: Enable VS2017/ARM64 builds Pete Batard
@ 2018-02-23 9:50 ` Pete Batard
2018-02-23 9:50 ` [PATCH 4/4] BaseTools/Conf: Add VS2017/ARM64 support Pete Batard
` (2 subsequent siblings)
5 siblings, 0 replies; 17+ messages in thread
From: Pete Batard @ 2018-02-23 9:50 UTC (permalink / raw)
To: edk2-devel; +Cc: liming.gao, ard.biesheuvel
We need to explicitly call the built-in __va_start() for ARM64, otherwise
the variable parameters are not properly enqueued for the next function
calls.
Also do the same for ARM, as as it doesn't harm us.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pete Batard <pete@akeo.ie>
---
MdePkg/Include/Base.h | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h
index a94182f08886..4f7bd4449c36 100644
--- a/MdePkg/Include/Base.h
+++ b/MdePkg/Include/Base.h
@@ -668,16 +668,15 @@ struct _LIST_ENTRY {
#define VA_COPY(Dest, Start) __va_copy (Dest, Start)
-#elif defined(_M_ARM)
+#elif defined(_M_ARM) || defined(_M_ARM64)
//
// MSFT ARM variable argument list support.
-// Same as the generic macros below, except for VA_ARG that needs extra adjustment.
//
typedef char* VA_LIST;
-#define VA_START(Marker, Parameter) (Marker = (VA_LIST) ((UINTN) & (Parameter) + _INT_SIZE_OF(Parameter)))
-#define VA_ARG(Marker, TYPE) (*(TYPE *) ((Marker += _INT_SIZE_OF(TYPE) + ((-(INTN)Marker) & (sizeof(TYPE) - 1))) - _INT_SIZE_OF (TYPE)))
+#define VA_START(Marker, Parameter) __va_start (&Marker, &Parameter, _INT_SIZE_OF (Parameter), __alignof(Parameter), &Parameter)
+#define VA_ARG(Marker, TYPE) (*(TYPE *) ((Marker += _INT_SIZE_OF (TYPE) + ((-(INTN)Marker) & (sizeof(TYPE) - 1))) - _INT_SIZE_OF (TYPE)))
#define VA_END(Marker) (Marker = (VA_LIST) 0)
#define VA_COPY(Dest, Start) ((void)((Dest) = (Start)))
--
2.9.3.windows.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 4/4] BaseTools/Conf: Add VS2017/ARM64 support
2018-02-23 9:49 [PATCH 0/4] Add ARM64 support for VS2017 Pete Batard
` (2 preceding siblings ...)
2018-02-23 9:50 ` [PATCH 3/4] MdePkg/Include: Add VA list support for VS2017/ARM64 Pete Batard
@ 2018-02-23 9:50 ` Pete Batard
2018-02-23 11:55 ` [PATCH 0/4] Add ARM64 support for VS2017 Ard Biesheuvel
2018-03-15 6:15 ` Gao, Liming
5 siblings, 0 replies; 17+ messages in thread
From: Pete Batard @ 2018-02-23 9:50 UTC (permalink / raw)
To: edk2-devel; +Cc: liming.gao, ard.biesheuvel
Build options for ARM64 are the same as for ARM, except for /BASE:0
which is removed from DLINK flags to avoid LNK1355 error:
invalid base address 0x0; ARM64 image cannot have base address below 4GB
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pete Batard <pete@akeo.ie>
---
BaseTools/Conf/build_rule.template | 2 +-
BaseTools/Conf/tools_def.template | 32 ++++++++++++++++++--
2 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/BaseTools/Conf/build_rule.template b/BaseTools/Conf/build_rule.template
index 77ed282e0311..308505b3dca5 100755
--- a/BaseTools/Conf/build_rule.template
+++ b/BaseTools/Conf/build_rule.template
@@ -206,7 +206,7 @@
# For RVCTCYGWIN ASM_FLAGS must be first to work around pathing issues
"$(ASM)" $(ASM_FLAGS) -o ${dst} $(INC) ${d_path}(+)${s_base}.iii
-[Assembly-Code-File.COMMON.ARM]
+[Assembly-Code-File.COMMON.ARM,Assembly-Code-File.COMMON.AARCH64]
# Remove --convert-hex for ARM as it breaks MSFT assemblers
<InputFile.MSFT, InputFile.INTEL, InputFile.RVCT>
?.asm, ?.Asm, ?.ASM
diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
index 427ad60b0e26..703019129f79 100755
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -80,6 +80,7 @@ DEFINE VS2017_BIN_HOST = DEF(VS2017_BIN)\HostDEF(VS2017_HOST)\DEF(VS2017_HOST
DEFINE VS2017_BIN_IA32 = DEF(VS2017_BIN)\HostDEF(VS2017_HOST)\x86
DEFINE VS2017_BIN_X64 = DEF(VS2017_BIN)\HostDEF(VS2017_HOST)\x64
DEFINE VS2017_BIN_ARM = DEF(VS2017_BIN)\HostDEF(VS2017_HOST)\arm
+DEFINE VS2017_BIN_AARCH64 = DEF(VS2017_BIN)\HostDEF(VS2017_HOST)\arm64
DEFINE WINSDK_BIN = ENV(WINSDK_PREFIX)
DEFINE WINSDKx86_BIN = ENV(WINSDKx86_PREFIX)
@@ -329,7 +330,7 @@ DEFINE DTC_BIN = ENV(DTC_PREFIX)dtc
# Intel(r) ACPI Compiler (iasl.exe) from
# https://acpica.org/downloads
# VS2017 -win32- Requires:
-# Microsoft Visual Studio 2017 version 15.2 or later
+# Microsoft Visual Studio 2017 version 15.2 (15.4 for ARM64) or later
# Optional:
# Required to build EBC drivers:
# Intel(r) Compiler for Efi Byte Code (Intel(r) EBC Compiler)
@@ -337,7 +338,7 @@ DEFINE DTC_BIN = ENV(DTC_PREFIX)dtc
# Intel(r) ACPI Compiler (iasl.exe) from
# https://acpica.org/downloads
# Note:
-# Building of XIP firmware images for ARM is not currently supported (only applications).
+# Building of XIP firmware images for ARM/ARM64 is not currently supported (only applications).
# /FILEALIGN:4096 and other changes are needed for ARM firmware builds.
# DDK3790 -win32- Requires:
# Microsoft Windows Server 2003 Driver Development Kit (Microsoft WINDDK) version 3790.1830
@@ -4200,6 +4201,33 @@ NOOPT_VS2017_ARM_ASM_FLAGS = /nologo
RELEASE_VS2017_ARM_DLINK_FLAGS = /NOLOGO /NODEFAULTLIB /IGNORE:4001 /IGNORE:4254 /OPT:REF /OPT:ICF=10 /MAP /SECTION:.xdata,D /SECTION:.pdata,D /MACHINE:ARM /LTCG /DLL /ENTRY:$(IMAGE_ENTRY_POINT) /SUBSYSTEM:EFI_BOOT_SERVICE_DRIVER /SAFESEH:NO /BASE:0 /DRIVER /MERGE:.rdata=.data
NOOPT_VS2017_ARM_DLINK_FLAGS = /NOLOGO /NODEFAULTLIB /IGNORE:4001 /OPT:REF /OPT:ICF=10 /MAP /SECTION:.xdata,D /SECTION:.pdata,D /MACHINE:ARM /LTCG /DLL /ENTRY:$(IMAGE_ENTRY_POINT) /SUBSYSTEM:EFI_BOOT_SERVICE_DRIVER /SAFESEH:NO /BASE:0 /DRIVER /DEBUG
+#####################
+# AARCH64 definitions
+#####################
+*_VS2017_AARCH64_CC_PATH = DEF(VS2017_BIN_AARCH64)\cl.exe
+*_VS2017_AARCH64_VFRPP_PATH = DEF(VS2017_BIN_AARCH64)\cl.exe
+*_VS2017_AARCH64_SLINK_PATH = DEF(VS2017_BIN_AARCH64)\lib.exe
+*_VS2017_AARCH64_DLINK_PATH = DEF(VS2017_BIN_AARCH64)\link.exe
+*_VS2017_AARCH64_APP_PATH = DEF(VS2017_BIN_AARCH64)\cl.exe
+*_VS2017_AARCH64_PP_PATH = DEF(VS2017_BIN_AARCH64)\cl.exe
+*_VS2017_AARCH64_ASM_PATH = DEF(VS2017_BIN_AARCH64)\armasm64.exe
+*_VS2017_AARCH64_ASLCC_PATH = DEF(VS2017_BIN_AARCH64)\cl.exe
+*_VS2017_AARCH64_ASLPP_PATH = DEF(VS2017_BIN_AARCH64)\cl.exe
+*_VS2017_AARCH64_ASLDLINK_PATH = DEF(VS2017_BIN_AARCH64)\link.exe
+
+ *_VS2017_AARCH64_MAKE_FLAGS = /nologo
+ DEBUG_VS2017_AARCH64_CC_FLAGS = /nologo /c /WX /GS- /W4 /Gs32768 /D UNICODE /O1b2 /GL /FIAutoGen.h /EHs-c- /GR- /GF /Gy /Zi /Gm /Gw /Oi-
+RELEASE_VS2017_AARCH64_CC_FLAGS = /nologo /c /WX /GS- /W4 /Gs32768 /D UNICODE /O1b2 /GL /FIAutoGen.h /EHs-c- /GR- /GF /Gw /Oi-
+NOOPT_VS2017_AARCH64_CC_FLAGS = /nologo /c /WX /GS- /W4 /Gs32768 /D UNICODE /FIAutoGen.h /EHs-c- /GR- /GF /Gy /Zi /Gm /Od /Oi-
+
+ DEBUG_VS2017_AARCH64_ASM_FLAGS = /nologo /g
+RELEASE_VS2017_AARCH64_ASM_FLAGS = /nologo
+NOOPT_VS2017_AARCH64_ASM_FLAGS = /nologo
+
+ DEBUG_VS2017_AARCH64_DLINK_FLAGS = /NOLOGO /NODEFAULTLIB /IGNORE:4001 /OPT:REF /OPT:ICF=10 /MAP /SECTION:.xdata,D /SECTION:.pdata,D /MACHINE:ARM64 /LTCG /DLL /ENTRY:$(IMAGE_ENTRY_POINT) /SUBSYSTEM:EFI_BOOT_SERVICE_DRIVER /SAFESEH:NO /DRIVER /DEBUG
+RELEASE_VS2017_AARCH64_DLINK_FLAGS = /NOLOGO /NODEFAULTLIB /IGNORE:4001 /IGNORE:4254 /OPT:REF /OPT:ICF=10 /MAP /SECTION:.xdata,D /SECTION:.pdata,D /MACHINE:ARM64 /LTCG /DLL /ENTRY:$(IMAGE_ENTRY_POINT) /SUBSYSTEM:EFI_BOOT_SERVICE_DRIVER /SAFESEH:NO /DRIVER /MERGE:.rdata=.data
+NOOPT_VS2017_AARCH64_DLINK_FLAGS = /NOLOGO /NODEFAULTLIB /IGNORE:4001 /OPT:REF /OPT:ICF=10 /MAP /SECTION:.xdata,D /SECTION:.pdata,D /MACHINE:ARM64 /LTCG /DLL /ENTRY:$(IMAGE_ENTRY_POINT) /SUBSYSTEM:EFI_BOOT_SERVICE_DRIVER /SAFESEH:NO /DRIVER /DEBUG
+
##################
# EBC definitions
##################
--
2.9.3.windows.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 0/4] Add ARM64 support for VS2017
2018-02-23 9:49 [PATCH 0/4] Add ARM64 support for VS2017 Pete Batard
` (3 preceding siblings ...)
2018-02-23 9:50 ` [PATCH 4/4] BaseTools/Conf: Add VS2017/ARM64 support Pete Batard
@ 2018-02-23 11:55 ` Ard Biesheuvel
2018-02-23 13:14 ` Pete Batard
2018-03-15 6:15 ` Gao, Liming
5 siblings, 1 reply; 17+ messages in thread
From: Ard Biesheuvel @ 2018-02-23 11:55 UTC (permalink / raw)
To: Pete Batard; +Cc: edk2-devel@lists.01.org, Gao, Liming
On 23 February 2018 at 09:49, Pete Batard <pete@akeo.ie> wrote:
> This is v2, which just removes a redundant #if defined(_MSC_EXTENSIONS) in 1/4.
>
> This series completes VS2017 support by enabling AARCH64 compilation.
> * PATCH 1 targets the disabling of VS Level 4 warnings. The disabled
> warnings for ARM64 are the same as the ones for IA32, X64 and ARM.
> * PATCH 2 adds assembly source in MdePkg/Library/BaseLib for various low
> level required functions. These new assembly files were converted from
> their GCC version, with minor changes applied to make them palatable
> to the MSFT assembler.
> * PATCH 3 adds variable argument handlers for print output. This is
> achieved without relying on any external toolchain headers. However
> a call to the __va_start() compiler intrinsic function is now being
> used for the VA_START macros, which we apply for ARM as well.
> * PATCH 4 enables the selection of ARM64 in the conf templates.
> One item of note is that the build options for ARM64 are the same as
> for ARM, except for /BASE:0 which was removed to avoid error:
> 'invalid base address 0x0; ARM64 image cannot have base address below 4GB'
>
This series looks fine to me, with the exception of the error
mentioned here, which seems strange to me. It does appear to be a
toolchain issue rather than anything else, so if you can build working
binaries with these patches, it's all fine by me.
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> With these patches, VS2017 toolchain users should be able to compile
> regular UEFI ARM64 applications using EDK2.
>
> Note however that ARM64 support requires the use of Visual Studio 2017
> Update 4 or later (a.k.a. v15.4), as native ARM64 compilation was not
> included in any version of Visual Studio prior to that.
>
> 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 saw no issues.
> Since we also modified the VA_START() macro for ARM, we also re-ran
> similar tests for ARM, to confirm that there was no regression there.
>
> Finally, we did not test the generation of a complete QEMU ARM64 firmware
> as it requires porting a handful of assembly sources, that don't exist
> yet, and our focus is with the generation of working AARCH64 drivers or
> applications. Hopefully, this can be tackled as VS2017/ARM64 sees more
> usage...
>
> Regards,
>
> /Pete
>
> Pete Batard (4):
> MdePkg: Disable some Level 4 warnings for VS2017/ARM64
> MdePkg/Library/BaseLib: Enable VS2017/ARM64 builds
> MdePkg/Include: Add VA list support for VS2017/ARM64
> BaseTools/Conf: Add VS2017/ARM64 support
>
> BaseTools/Conf/build_rule.template | 2 +-
> BaseTools/Conf/tools_def.template | 32 ++++++-
> MdePkg/Include/AArch64/ProcessorBind.h | 53 +++++++++-
> MdePkg/Include/Base.h | 7 +-
> MdePkg/Library/BaseLib/AArch64/CpuBreakpoint.asm | 39 ++++++++
> MdePkg/Library/BaseLib/AArch64/DisableInterrupts.asm | 37 +++++++
> MdePkg/Library/BaseLib/AArch64/EnableInterrupts.asm | 37 +++++++
> MdePkg/Library/BaseLib/AArch64/GetInterruptsState.asm | 49 ++++++++++
> MdePkg/Library/BaseLib/AArch64/MemoryFence.asm | 38 ++++++++
> MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm | 101 ++++++++++++++++++++
> MdePkg/Library/BaseLib/AArch64/SwitchStack.asm | 69 +++++++++++++
> MdePkg/Library/BaseLib/BaseLib.inf | 8 ++
> 12 files changed, 463 insertions(+), 9 deletions(-)
> create mode 100644 MdePkg/Library/BaseLib/AArch64/CpuBreakpoint.asm
> create mode 100644 MdePkg/Library/BaseLib/AArch64/DisableInterrupts.asm
> create mode 100644 MdePkg/Library/BaseLib/AArch64/EnableInterrupts.asm
> create mode 100644 MdePkg/Library/BaseLib/AArch64/GetInterruptsState.asm
> create mode 100644 MdePkg/Library/BaseLib/AArch64/MemoryFence.asm
> create mode 100644 MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
> create mode 100644 MdePkg/Library/BaseLib/AArch64/SwitchStack.asm
>
> --
> 2.9.3.windows.2
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/4] Add ARM64 support for VS2017
2018-02-23 11:55 ` [PATCH 0/4] Add ARM64 support for VS2017 Ard Biesheuvel
@ 2018-02-23 13:14 ` Pete Batard
0 siblings, 0 replies; 17+ messages in thread
From: Pete Batard @ 2018-02-23 13:14 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel@lists.01.org, Gao, Liming
On 2018.02.23 11:55, Ard Biesheuvel wrote:
>> * PATCH 4 enables the selection of ARM64 in the conf templates.
>> One item of note is that the build options for ARM64 are the same as
>> for ARM, except for /BASE:0 which was removed to avoid error:
>> 'invalid base address 0x0; ARM64 image cannot have base address below 4GB'
>>
>
> This series looks fine to me, with the exception of the error
> mentioned here, which seems strange to me. It does appear to be a
> toolchain issue rather than anything else, so if you can build working
> binaries with these patches, it's all fine by me.
Thanks Ard.
The thing about /BASE:0 producing a LNK1355 error above is that it only
seems to occur with applications (you will see it if you try to build
the Shell or MdeModulePkg\Application\HelloWorld for instance) and not
drivers.
I too suspect that this may have to do with the public-facing
VS2017/ARM64 toolchain being brand new, since it was only introduced
with the last major VS update, and maybe still needing some ironing out
when it comes to the generation of non Windows applications.
So far I have not seen any ill effects from the removal of /BASE:0.
Once this series has been integrated (so that it's easier for the VS dev
team to test), I'll try to report this issue to
https://developercommunity.visualstudio.com, to find out what they have
to say about it.
Regards,
/Pete
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/4] Add ARM64 support for VS2017
2018-02-23 9:49 [PATCH 0/4] Add ARM64 support for VS2017 Pete Batard
` (4 preceding siblings ...)
2018-02-23 11:55 ` [PATCH 0/4] Add ARM64 support for VS2017 Ard Biesheuvel
@ 2018-03-15 6:15 ` Gao, Liming
2018-03-15 9:28 ` Pete Batard
5 siblings, 1 reply; 17+ messages in thread
From: Gao, Liming @ 2018-03-15 6:15 UTC (permalink / raw)
To: Pete Batard, edk2-devel@lists.01.org; +Cc: ard.biesheuvel@linaro.org
Pete:
For new added ASM file in BaseLib, could you use the same comment style for them? ASM use ; for the comment. Most of new files uses ; as the comment, but switchstack is not. Besides, compared to Arm arch assembly file, I don't find CpuPause.asm. Is it required?
Thanks
Liming
>-----Original Message-----
>From: Pete Batard [mailto:pete@akeo.ie]
>Sent: Friday, February 23, 2018 5:50 PM
>To: edk2-devel@lists.01.org
>Cc: Gao, Liming <liming.gao@intel.com>; ard.biesheuvel@linaro.org
>Subject: [PATCH 0/4] Add ARM64 support for VS2017
>
>This is v2, which just removes a redundant #if defined(_MSC_EXTENSIONS) in
>1/4.
>
>This series completes VS2017 support by enabling AARCH64 compilation.
>* PATCH 1 targets the disabling of VS Level 4 warnings. The disabled
> warnings for ARM64 are the same as the ones for IA32, X64 and ARM.
>* PATCH 2 adds assembly source in MdePkg/Library/BaseLib for various low
> level required functions. These new assembly files were converted from
> their GCC version, with minor changes applied to make them palatable
> to the MSFT assembler.
>* PATCH 3 adds variable argument handlers for print output. This is
> achieved without relying on any external toolchain headers. However
> a call to the __va_start() compiler intrinsic function is now being
> used for the VA_START macros, which we apply for ARM as well.
>* PATCH 4 enables the selection of ARM64 in the conf templates.
> One item of note is that the build options for ARM64 are the same as
> for ARM, except for /BASE:0 which was removed to avoid error:
> 'invalid base address 0x0; ARM64 image cannot have base address below
>4GB'
>
>With these patches, VS2017 toolchain users should be able to compile
>regular UEFI ARM64 applications using EDK2.
>
>Note however that ARM64 support requires the use of Visual Studio 2017
>Update 4 or later (a.k.a. v15.4), as native ARM64 compilation was not
>included in any version of Visual Studio prior to that.
>
>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 saw no issues.
>Since we also modified the VA_START() macro for ARM, we also re-ran
>similar tests for ARM, to confirm that there was no regression there.
>
>Finally, we did not test the generation of a complete QEMU ARM64 firmware
>as it requires porting a handful of assembly sources, that don't exist
>yet, and our focus is with the generation of working AARCH64 drivers or
>applications. Hopefully, this can be tackled as VS2017/ARM64 sees more
>usage...
>
>Regards,
>
>/Pete
>
>Pete Batard (4):
> MdePkg: Disable some Level 4 warnings for VS2017/ARM64
> MdePkg/Library/BaseLib: Enable VS2017/ARM64 builds
> MdePkg/Include: Add VA list support for VS2017/ARM64
> BaseTools/Conf: Add VS2017/ARM64 support
>
> BaseTools/Conf/build_rule.template | 2 +-
> BaseTools/Conf/tools_def.template | 32 ++++++-
> MdePkg/Include/AArch64/ProcessorBind.h | 53 +++++++++-
> MdePkg/Include/Base.h | 7 +-
> MdePkg/Library/BaseLib/AArch64/CpuBreakpoint.asm | 39 ++++++++
> MdePkg/Library/BaseLib/AArch64/DisableInterrupts.asm | 37 +++++++
> MdePkg/Library/BaseLib/AArch64/EnableInterrupts.asm | 37 +++++++
> MdePkg/Library/BaseLib/AArch64/GetInterruptsState.asm | 49 ++++++++++
> MdePkg/Library/BaseLib/AArch64/MemoryFence.asm | 38 ++++++++
> MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm | 101
>++++++++++++++++++++
> MdePkg/Library/BaseLib/AArch64/SwitchStack.asm | 69 +++++++++++++
> MdePkg/Library/BaseLib/BaseLib.inf | 8 ++
> 12 files changed, 463 insertions(+), 9 deletions(-)
> create mode 100644 MdePkg/Library/BaseLib/AArch64/CpuBreakpoint.asm
> create mode 100644
>MdePkg/Library/BaseLib/AArch64/DisableInterrupts.asm
> create mode 100644 MdePkg/Library/BaseLib/AArch64/EnableInterrupts.asm
> create mode 100644
>MdePkg/Library/BaseLib/AArch64/GetInterruptsState.asm
> create mode 100644 MdePkg/Library/BaseLib/AArch64/MemoryFence.asm
> create mode 100644
>MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
> create mode 100644 MdePkg/Library/BaseLib/AArch64/SwitchStack.asm
>
>--
>2.9.3.windows.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/4] Add ARM64 support for VS2017
2018-03-15 6:15 ` Gao, Liming
@ 2018-03-15 9:28 ` Pete Batard
2018-03-16 8:24 ` Gao, Liming
0 siblings, 1 reply; 17+ messages in thread
From: Pete Batard @ 2018-03-15 9:28 UTC (permalink / raw)
To: Gao, Liming, edk2-devel@lists.01.org; +Cc: ard.biesheuvel@linaro.org
Hi Liming,
Thanks for reviewing the patches.
On 2018.03.15 06:15, Gao, Liming wrote:
> Pete:
> For new added ASM file in BaseLib, could you use the same comment style
> for them? ASM use ; for the comment. Most of new files uses ; as the
> comment, but switchstack is not.
This is because SwitchStack.asm is simply SwitchStack.S, with the GCC
assembler specifics removed, and MSVC assembler specifics added.
I did not change the comment style from the original files, so the real
issue here is that our GCC assembly files for AARCH64 do not use the
same comment style.
I'm fine with trying to harmonize the comment styles, but seeing as this
needs to be done for both the .S and .asm, I'd rather send a patch to do
that *after* these VS2017 changes have been applied, as I don't consider
this correction to in scope of this patch series (because logically, the
introduction of VS2017 should not alter any of the .S files, unless we
reuse them, which we don't).
If you agree to apply this series, I'll make sure to send a non
VS2017-specific additional patch, that does what you request for both
the .S and .asm.
> Besides, compared to Arm arch assembly
> file, I don't find CpuPause.asm. Is it required?
That file doesn't exist for GCC (as you will see there is no
MdePkg/Library/BaseLib/AArch64/CpuPause.S), so we don't have one for
VS2017 either.
Regards,
/Pete
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/4] Add ARM64 support for VS2017
2018-03-15 9:28 ` Pete Batard
@ 2018-03-16 8:24 ` Gao, Liming
2018-03-16 11:03 ` Pete Batard
0 siblings, 1 reply; 17+ messages in thread
From: Gao, Liming @ 2018-03-16 8:24 UTC (permalink / raw)
To: Pete Batard, edk2-devel@lists.01.org; +Cc: ard.biesheuvel@linaro.org
Pete:
.S for GCC assembly, .asm for MSFT assembly. They can have the different comment style.
Here, my comment is to make sure .asm files have the same comment style. I don't request to change .S file.
>-----Original Message-----
>From: Pete Batard [mailto:pete@akeo.ie]
>Sent: Thursday, March 15, 2018 5:28 PM
>To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
>Cc: ard.biesheuvel@linaro.org
>Subject: Re: [PATCH 0/4] Add ARM64 support for VS2017
>
>Hi Liming,
>
>Thanks for reviewing the patches.
>
>On 2018.03.15 06:15, Gao, Liming wrote:
>> Pete:
>> For new added ASM file in BaseLib, could you use the same comment style
>> for them? ASM use ; for the comment. Most of new files uses ; as the
>> comment, but switchstack is not.
>
>This is because SwitchStack.asm is simply SwitchStack.S, with the GCC
>assembler specifics removed, and MSVC assembler specifics added.
>
>I did not change the comment style from the original files, so the real
>issue here is that our GCC assembly files for AARCH64 do not use the
>same comment style.
>
>I'm fine with trying to harmonize the comment styles, but seeing as this
>needs to be done for both the .S and .asm, I'd rather send a patch to do
>that *after* these VS2017 changes have been applied, as I don't consider
>this correction to in scope of this patch series (because logically, the
>introduction of VS2017 should not alter any of the .S files, unless we
>reuse them, which we don't).
>
>If you agree to apply this series, I'll make sure to send a non
>VS2017-specific additional patch, that does what you request for both
>the .S and .asm.
>
>> Besides, compared to Arm arch assembly
>> file, I don't find CpuPause.asm. Is it required?
>
>That file doesn't exist for GCC (as you will see there is no
>MdePkg/Library/BaseLib/AArch64/CpuPause.S), so we don't have one for
>VS2017 either.
>
>Regards,
>
>/Pete
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/4] Add ARM64 support for VS2017
2018-03-16 8:24 ` Gao, Liming
@ 2018-03-16 11:03 ` Pete Batard
2018-03-16 15:56 ` Gao, Liming
0 siblings, 1 reply; 17+ messages in thread
From: Pete Batard @ 2018-03-16 11:03 UTC (permalink / raw)
To: edk2-devel@lists.01.org; +Cc: Gao, Liming, Ard Biesheuvel
On 2018.03.16 08:24, Gao, Liming wrote:
> Pete:
> .S for GCC assembly, .asm for MSFT assembly. They can have the different comment style.
Yes, but as I explained, the actual original issue is that our current
.S files do *not* have the same comment styles in the first place.
If you look at MdePkg/Library/BaseLib/AArch64/SwitchStack.S, you'll see
that is uses '//' for comments, whereas other .S files, such as
MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S, use '#'.
So that is our actual issue here: Regardless of VS2017, the GCC assembly
files for AARCH64 we currently have do not use the same comment style.
Thus, the only reason why the .asm don't have the same comment style in
our proposal is because the .S, which we derived the .asm from, don't.
This means that either we should fix the .S too, or we shouldn't fix
anything at all.
> Here, my comment is to make sure .asm files have the same comment style. I don't request to change .S file.
And what I am saying is that it makes little sense to harmonize the
comment style for the .asm files, if we're not going to do the same for
the .S files as well. It just doesn't seem fair in my book to have the
VS2017 assembly files held to a higher standard than the GCC ones. So
either we need to fix both, or we fix none at all.
But as I indicated in my last e-mail, I am planning to send an
additional patch that does comment harmonization, for both .S and .asm,
*after* this VS2017 series has been applied to mainline. So the change
you request will happen. Just not as part of this patch series.
And the reason I have insist on splitting these changes is because, if
we have to alter the .S files to be consistent, then this comment
harmonization request should logically be handled separately from the
VS2017 effort.
Please let me know if you still think having a future separate patch,
that will do .S and .asm comment harmonization, does not make sense.
Regards,
/Pete
>
>> -----Original Message-----
>> From: Pete Batard [mailto:pete@akeo.ie]
>> Sent: Thursday, March 15, 2018 5:28 PM
>> To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
>> Cc: ard.biesheuvel@linaro.org
>> Subject: Re: [PATCH 0/4] Add ARM64 support for VS2017
>>
>> Hi Liming,
>>
>> Thanks for reviewing the patches.
>>
>> On 2018.03.15 06:15, Gao, Liming wrote:
>>> Pete:
>>> For new added ASM file in BaseLib, could you use the same comment style
>>> for them? ASM use ; for the comment. Most of new files uses ; as the
>>> comment, but switchstack is not.
>>
>> This is because SwitchStack.asm is simply SwitchStack.S, with the GCC
>> assembler specifics removed, and MSVC assembler specifics added.
>>
>> I did not change the comment style from the original files, so the real
>> issue here is that our GCC assembly files for AARCH64 do not use the
>> same comment style.
>>
>> I'm fine with trying to harmonize the comment styles, but seeing as this
>> needs to be done for both the .S and .asm, I'd rather send a patch to do
>> that *after* these VS2017 changes have been applied, as I don't consider
>> this correction to in scope of this patch series (because logically, the
>> introduction of VS2017 should not alter any of the .S files, unless we
>> reuse them, which we don't).
>>
>> If you agree to apply this series, I'll make sure to send a non
>> VS2017-specific additional patch, that does what you request for both
>> the .S and .asm.
>>
>>> Besides, compared to Arm arch assembly
>>> file, I don't find CpuPause.asm. Is it required?
>>
>> That file doesn't exist for GCC (as you will see there is no
>> MdePkg/Library/BaseLib/AArch64/CpuPause.S), so we don't have one for
>> VS2017 either.
>>
>> Regards,
>>
>> /Pete
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/4] Add ARM64 support for VS2017
2018-03-16 11:03 ` Pete Batard
@ 2018-03-16 15:56 ` Gao, Liming
2018-03-16 16:11 ` Pete Batard
0 siblings, 1 reply; 17+ messages in thread
From: Gao, Liming @ 2018-03-16 15:56 UTC (permalink / raw)
To: Pete Batard, edk2-devel@lists.01.org; +Cc: Ard Biesheuvel
Pete:
I understand the existing .S file has the inconsistent comment style. I also know new added ASM files are converted from .S files. But, my comment is for this patch that adds new ASM files. I expect new added ASM files have the same style. If you check ARM arch ASM files, you will find they all have the same style.
Thanks
Liming
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Pete Batard
> Sent: Friday, March 16, 2018 7:04 PM
> To: edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2] [PATCH 0/4] Add ARM64 support for VS2017
>
> On 2018.03.16 08:24, Gao, Liming wrote:
> > Pete:
> > .S for GCC assembly, .asm for MSFT assembly. They can have the different comment style.
>
> Yes, but as I explained, the actual original issue is that our current
> .S files do *not* have the same comment styles in the first place.
>
> If you look at MdePkg/Library/BaseLib/AArch64/SwitchStack.S, you'll see
> that is uses '//' for comments, whereas other .S files, such as
> MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S, use '#'.
>
> So that is our actual issue here: Regardless of VS2017, the GCC assembly
> files for AARCH64 we currently have do not use the same comment style.
>
> Thus, the only reason why the .asm don't have the same comment style in
> our proposal is because the .S, which we derived the .asm from, don't.
> This means that either we should fix the .S too, or we shouldn't fix
> anything at all.
>
> > Here, my comment is to make sure .asm files have the same comment style. I don't request to change .S file.
>
> And what I am saying is that it makes little sense to harmonize the
> comment style for the .asm files, if we're not going to do the same for
> the .S files as well. It just doesn't seem fair in my book to have the
> VS2017 assembly files held to a higher standard than the GCC ones. So
> either we need to fix both, or we fix none at all.
>
> But as I indicated in my last e-mail, I am planning to send an
> additional patch that does comment harmonization, for both .S and .asm,
> *after* this VS2017 series has been applied to mainline. So the change
> you request will happen. Just not as part of this patch series.
>
> And the reason I have insist on splitting these changes is because, if
> we have to alter the .S files to be consistent, then this comment
> harmonization request should logically be handled separately from the
> VS2017 effort.
>
> Please let me know if you still think having a future separate patch,
> that will do .S and .asm comment harmonization, does not make sense.
>
> Regards,
>
> /Pete
>
> >
> >> -----Original Message-----
> >> From: Pete Batard [mailto:pete@akeo.ie]
> >> Sent: Thursday, March 15, 2018 5:28 PM
> >> To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
> >> Cc: ard.biesheuvel@linaro.org
> >> Subject: Re: [PATCH 0/4] Add ARM64 support for VS2017
> >>
> >> Hi Liming,
> >>
> >> Thanks for reviewing the patches.
> >>
> >> On 2018.03.15 06:15, Gao, Liming wrote:
> >>> Pete:
> >>> For new added ASM file in BaseLib, could you use the same comment style
> >>> for them? ASM use ; for the comment. Most of new files uses ; as the
> >>> comment, but switchstack is not.
> >>
> >> This is because SwitchStack.asm is simply SwitchStack.S, with the GCC
> >> assembler specifics removed, and MSVC assembler specifics added.
> >>
> >> I did not change the comment style from the original files, so the real
> >> issue here is that our GCC assembly files for AARCH64 do not use the
> >> same comment style.
> >>
> >> I'm fine with trying to harmonize the comment styles, but seeing as this
> >> needs to be done for both the .S and .asm, I'd rather send a patch to do
> >> that *after* these VS2017 changes have been applied, as I don't consider
> >> this correction to in scope of this patch series (because logically, the
> >> introduction of VS2017 should not alter any of the .S files, unless we
> >> reuse them, which we don't).
> >>
> >> If you agree to apply this series, I'll make sure to send a non
> >> VS2017-specific additional patch, that does what you request for both
> >> the .S and .asm.
> >>
> >>> Besides, compared to Arm arch assembly
> >>> file, I don't find CpuPause.asm. Is it required?
> >>
> >> That file doesn't exist for GCC (as you will see there is no
> >> MdePkg/Library/BaseLib/AArch64/CpuPause.S), so we don't have one for
> >> VS2017 either.
> >>
> >> Regards,
> >>
> >> /Pete
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/4] Add ARM64 support for VS2017
2018-03-16 15:56 ` Gao, Liming
@ 2018-03-16 16:11 ` Pete Batard
2018-03-16 16:31 ` Gao, Liming
0 siblings, 1 reply; 17+ messages in thread
From: Pete Batard @ 2018-03-16 16:11 UTC (permalink / raw)
To: edk2-devel@lists.01.org, Gao, Liming; +Cc: Ard Biesheuvel
I understand where you're coming from, but that means I have to recreate
this patch set, and then create a new patch for the .S (because it makes
zero sense to require the same comment style on the .asm and not request
a follow through for the .S).
My time being limited, I'd rather only have to produce one new patch,
that will harmonize the comments for both .S and .asm at the same time.
The end result will be exactly the same, so I'm going to have to insist
that we split the comment harmonization (which is a very minor issue and
should hardly be seen as a showstopper for the patch series in the first
place) into a subsequent patch.
Thank you,
/Pete
On 2018.03.16 15:56, Gao, Liming wrote:
> Pete:
> I understand the existing .S file has the inconsistent comment style. I also know new added ASM files are converted from .S files. But, my comment is for this patch that adds new ASM files. I expect new added ASM files have the same style. If you check ARM arch ASM files, you will find they all have the same style.
>
> Thanks
> Liming
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Pete Batard
>> Sent: Friday, March 16, 2018 7:04 PM
>> To: edk2-devel@lists.01.org
>> Cc: Gao, Liming <liming.gao@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Subject: Re: [edk2] [PATCH 0/4] Add ARM64 support for VS2017
>>
>> On 2018.03.16 08:24, Gao, Liming wrote:
>>> Pete:
>>> .S for GCC assembly, .asm for MSFT assembly. They can have the different comment style.
>>
>> Yes, but as I explained, the actual original issue is that our current
>> .S files do *not* have the same comment styles in the first place.
>>
>> If you look at MdePkg/Library/BaseLib/AArch64/SwitchStack.S, you'll see
>> that is uses '//' for comments, whereas other .S files, such as
>> MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S, use '#'.
>>
>> So that is our actual issue here: Regardless of VS2017, the GCC assembly
>> files for AARCH64 we currently have do not use the same comment style.
>>
>> Thus, the only reason why the .asm don't have the same comment style in
>> our proposal is because the .S, which we derived the .asm from, don't.
>> This means that either we should fix the .S too, or we shouldn't fix
>> anything at all.
>>
>>> Here, my comment is to make sure .asm files have the same comment style. I don't request to change .S file.
>>
>> And what I am saying is that it makes little sense to harmonize the
>> comment style for the .asm files, if we're not going to do the same for
>> the .S files as well. It just doesn't seem fair in my book to have the
>> VS2017 assembly files held to a higher standard than the GCC ones. So
>> either we need to fix both, or we fix none at all.
>>
>> But as I indicated in my last e-mail, I am planning to send an
>> additional patch that does comment harmonization, for both .S and .asm,
>> *after* this VS2017 series has been applied to mainline. So the change
>> you request will happen. Just not as part of this patch series.
>>
>> And the reason I have insist on splitting these changes is because, if
>> we have to alter the .S files to be consistent, then this comment
>> harmonization request should logically be handled separately from the
>> VS2017 effort.
>>
>> Please let me know if you still think having a future separate patch,
>> that will do .S and .asm comment harmonization, does not make sense.
>>
>> Regards,
>>
>> /Pete
>>
>>>
>>>> -----Original Message-----
>>>> From: Pete Batard [mailto:pete@akeo.ie]
>>>> Sent: Thursday, March 15, 2018 5:28 PM
>>>> To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
>>>> Cc: ard.biesheuvel@linaro.org
>>>> Subject: Re: [PATCH 0/4] Add ARM64 support for VS2017
>>>>
>>>> Hi Liming,
>>>>
>>>> Thanks for reviewing the patches.
>>>>
>>>> On 2018.03.15 06:15, Gao, Liming wrote:
>>>>> Pete:
>>>>> For new added ASM file in BaseLib, could you use the same comment style
>>>>> for them? ASM use ; for the comment. Most of new files uses ; as the
>>>>> comment, but switchstack is not.
>>>>
>>>> This is because SwitchStack.asm is simply SwitchStack.S, with the GCC
>>>> assembler specifics removed, and MSVC assembler specifics added.
>>>>
>>>> I did not change the comment style from the original files, so the real
>>>> issue here is that our GCC assembly files for AARCH64 do not use the
>>>> same comment style.
>>>>
>>>> I'm fine with trying to harmonize the comment styles, but seeing as this
>>>> needs to be done for both the .S and .asm, I'd rather send a patch to do
>>>> that *after* these VS2017 changes have been applied, as I don't consider
>>>> this correction to in scope of this patch series (because logically, the
>>>> introduction of VS2017 should not alter any of the .S files, unless we
>>>> reuse them, which we don't).
>>>>
>>>> If you agree to apply this series, I'll make sure to send a non
>>>> VS2017-specific additional patch, that does what you request for both
>>>> the .S and .asm.
>>>>
>>>>> Besides, compared to Arm arch assembly
>>>>> file, I don't find CpuPause.asm. Is it required?
>>>>
>>>> That file doesn't exist for GCC (as you will see there is no
>>>> MdePkg/Library/BaseLib/AArch64/CpuPause.S), so we don't have one for
>>>> VS2017 either.
>>>>
>>>> Regards,
>>>>
>>>> /Pete
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/4] Add ARM64 support for VS2017
2018-03-16 16:11 ` Pete Batard
@ 2018-03-16 16:31 ` Gao, Liming
2018-03-16 16:35 ` Pete Batard
0 siblings, 1 reply; 17+ messages in thread
From: Gao, Liming @ 2018-03-16 16:31 UTC (permalink / raw)
To: Pete Batard, edk2-devel@lists.01.org; +Cc: Ard Biesheuvel
Yes. This is a minor issue. So, I think the effort is small. If it is a big task to you, you can separate it into another patch.
And, I don't expect this minor issue break your patches. I give my Reviewed-by: Liming Gao <liming.gao@intel.com>
Thanks
Liming
> -----Original Message-----
> From: Pete Batard [mailto:pete@akeo.ie]
> Sent: Saturday, March 17, 2018 12:12 AM
> To: edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2] [PATCH 0/4] Add ARM64 support for VS2017
>
> I understand where you're coming from, but that means I have to recreate
> this patch set, and then create a new patch for the .S (because it makes
> zero sense to require the same comment style on the .asm and not request
> a follow through for the .S).
>
> My time being limited, I'd rather only have to produce one new patch,
> that will harmonize the comments for both .S and .asm at the same time.
>
> The end result will be exactly the same, so I'm going to have to insist
> that we split the comment harmonization (which is a very minor issue and
> should hardly be seen as a showstopper for the patch series in the first
> place) into a subsequent patch.
>
> Thank you,
>
> /Pete
>
> On 2018.03.16 15:56, Gao, Liming wrote:
> > Pete:
> > I understand the existing .S file has the inconsistent comment style. I also know new added ASM files are converted from .S files.
> But, my comment is for this patch that adds new ASM files. I expect new added ASM files have the same style. If you check ARM arch
> ASM files, you will find they all have the same style.
> >
> > Thanks
> > Liming
> >> -----Original Message-----
> >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Pete Batard
> >> Sent: Friday, March 16, 2018 7:04 PM
> >> To: edk2-devel@lists.01.org
> >> Cc: Gao, Liming <liming.gao@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> Subject: Re: [edk2] [PATCH 0/4] Add ARM64 support for VS2017
> >>
> >> On 2018.03.16 08:24, Gao, Liming wrote:
> >>> Pete:
> >>> .S for GCC assembly, .asm for MSFT assembly. They can have the different comment style.
> >>
> >> Yes, but as I explained, the actual original issue is that our current
> >> .S files do *not* have the same comment styles in the first place.
> >>
> >> If you look at MdePkg/Library/BaseLib/AArch64/SwitchStack.S, you'll see
> >> that is uses '//' for comments, whereas other .S files, such as
> >> MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S, use '#'.
> >>
> >> So that is our actual issue here: Regardless of VS2017, the GCC assembly
> >> files for AARCH64 we currently have do not use the same comment style.
> >>
> >> Thus, the only reason why the .asm don't have the same comment style in
> >> our proposal is because the .S, which we derived the .asm from, don't.
> >> This means that either we should fix the .S too, or we shouldn't fix
> >> anything at all.
> >>
> >>> Here, my comment is to make sure .asm files have the same comment style. I don't request to change .S file.
> >>
> >> And what I am saying is that it makes little sense to harmonize the
> >> comment style for the .asm files, if we're not going to do the same for
> >> the .S files as well. It just doesn't seem fair in my book to have the
> >> VS2017 assembly files held to a higher standard than the GCC ones. So
> >> either we need to fix both, or we fix none at all.
> >>
> >> But as I indicated in my last e-mail, I am planning to send an
> >> additional patch that does comment harmonization, for both .S and .asm,
> >> *after* this VS2017 series has been applied to mainline. So the change
> >> you request will happen. Just not as part of this patch series.
> >>
> >> And the reason I have insist on splitting these changes is because, if
> >> we have to alter the .S files to be consistent, then this comment
> >> harmonization request should logically be handled separately from the
> >> VS2017 effort.
> >>
> >> Please let me know if you still think having a future separate patch,
> >> that will do .S and .asm comment harmonization, does not make sense.
> >>
> >> Regards,
> >>
> >> /Pete
> >>
> >>>
> >>>> -----Original Message-----
> >>>> From: Pete Batard [mailto:pete@akeo.ie]
> >>>> Sent: Thursday, March 15, 2018 5:28 PM
> >>>> To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
> >>>> Cc: ard.biesheuvel@linaro.org
> >>>> Subject: Re: [PATCH 0/4] Add ARM64 support for VS2017
> >>>>
> >>>> Hi Liming,
> >>>>
> >>>> Thanks for reviewing the patches.
> >>>>
> >>>> On 2018.03.15 06:15, Gao, Liming wrote:
> >>>>> Pete:
> >>>>> For new added ASM file in BaseLib, could you use the same comment style
> >>>>> for them? ASM use ; for the comment. Most of new files uses ; as the
> >>>>> comment, but switchstack is not.
> >>>>
> >>>> This is because SwitchStack.asm is simply SwitchStack.S, with the GCC
> >>>> assembler specifics removed, and MSVC assembler specifics added.
> >>>>
> >>>> I did not change the comment style from the original files, so the real
> >>>> issue here is that our GCC assembly files for AARCH64 do not use the
> >>>> same comment style.
> >>>>
> >>>> I'm fine with trying to harmonize the comment styles, but seeing as this
> >>>> needs to be done for both the .S and .asm, I'd rather send a patch to do
> >>>> that *after* these VS2017 changes have been applied, as I don't consider
> >>>> this correction to in scope of this patch series (because logically, the
> >>>> introduction of VS2017 should not alter any of the .S files, unless we
> >>>> reuse them, which we don't).
> >>>>
> >>>> If you agree to apply this series, I'll make sure to send a non
> >>>> VS2017-specific additional patch, that does what you request for both
> >>>> the .S and .asm.
> >>>>
> >>>>> Besides, compared to Arm arch assembly
> >>>>> file, I don't find CpuPause.asm. Is it required?
> >>>>
> >>>> That file doesn't exist for GCC (as you will see there is no
> >>>> MdePkg/Library/BaseLib/AArch64/CpuPause.S), so we don't have one for
> >>>> VS2017 either.
> >>>>
> >>>> Regards,
> >>>>
> >>>> /Pete
> >>
> >> _______________________________________________
> >> edk2-devel mailing list
> >> edk2-devel@lists.01.org
> >> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/4] Add ARM64 support for VS2017
2018-03-16 16:31 ` Gao, Liming
@ 2018-03-16 16:35 ` Pete Batard
2018-03-19 9:07 ` Gao, Liming
0 siblings, 1 reply; 17+ messages in thread
From: Pete Batard @ 2018-03-16 16:35 UTC (permalink / raw)
To: Gao, Liming, edk2-devel@lists.01.org; +Cc: Ard Biesheuvel
Thanks Liming, much appreciated!
I'll send the comment harmonization patch as soon as I see the
VS2017/ARM64 changes in edk2 mainline.
Regards,
/Pete
On 2018.03.16 16:31, Gao, Liming wrote:
> Yes. This is a minor issue. So, I think the effort is small. If it is a big task to you, you can separate it into another patch.
>
> And, I don't expect this minor issue break your patches. I give my Reviewed-by: Liming Gao <liming.gao@intel.com>
>
> Thanks
> Liming
>> -----Original Message-----
>> From: Pete Batard [mailto:pete@akeo.ie]
>> Sent: Saturday, March 17, 2018 12:12 AM
>> To: edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Subject: Re: [edk2] [PATCH 0/4] Add ARM64 support for VS2017
>>
>> I understand where you're coming from, but that means I have to recreate
>> this patch set, and then create a new patch for the .S (because it makes
>> zero sense to require the same comment style on the .asm and not request
>> a follow through for the .S).
>>
>> My time being limited, I'd rather only have to produce one new patch,
>> that will harmonize the comments for both .S and .asm at the same time.
>>
>> The end result will be exactly the same, so I'm going to have to insist
>> that we split the comment harmonization (which is a very minor issue and
>> should hardly be seen as a showstopper for the patch series in the first
>> place) into a subsequent patch.
>>
>> Thank you,
>>
>> /Pete
>>
>> On 2018.03.16 15:56, Gao, Liming wrote:
>>> Pete:
>>> I understand the existing .S file has the inconsistent comment style. I also know new added ASM files are converted from .S files.
>> But, my comment is for this patch that adds new ASM files. I expect new added ASM files have the same style. If you check ARM arch
>> ASM files, you will find they all have the same style.
>>>
>>> Thanks
>>> Liming
>>>> -----Original Message-----
>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Pete Batard
>>>> Sent: Friday, March 16, 2018 7:04 PM
>>>> To: edk2-devel@lists.01.org
>>>> Cc: Gao, Liming <liming.gao@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> Subject: Re: [edk2] [PATCH 0/4] Add ARM64 support for VS2017
>>>>
>>>> On 2018.03.16 08:24, Gao, Liming wrote:
>>>>> Pete:
>>>>> .S for GCC assembly, .asm for MSFT assembly. They can have the different comment style.
>>>>
>>>> Yes, but as I explained, the actual original issue is that our current
>>>> .S files do *not* have the same comment styles in the first place.
>>>>
>>>> If you look at MdePkg/Library/BaseLib/AArch64/SwitchStack.S, you'll see
>>>> that is uses '//' for comments, whereas other .S files, such as
>>>> MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S, use '#'.
>>>>
>>>> So that is our actual issue here: Regardless of VS2017, the GCC assembly
>>>> files for AARCH64 we currently have do not use the same comment style.
>>>>
>>>> Thus, the only reason why the .asm don't have the same comment style in
>>>> our proposal is because the .S, which we derived the .asm from, don't.
>>>> This means that either we should fix the .S too, or we shouldn't fix
>>>> anything at all.
>>>>
>>>>> Here, my comment is to make sure .asm files have the same comment style. I don't request to change .S file.
>>>>
>>>> And what I am saying is that it makes little sense to harmonize the
>>>> comment style for the .asm files, if we're not going to do the same for
>>>> the .S files as well. It just doesn't seem fair in my book to have the
>>>> VS2017 assembly files held to a higher standard than the GCC ones. So
>>>> either we need to fix both, or we fix none at all.
>>>>
>>>> But as I indicated in my last e-mail, I am planning to send an
>>>> additional patch that does comment harmonization, for both .S and .asm,
>>>> *after* this VS2017 series has been applied to mainline. So the change
>>>> you request will happen. Just not as part of this patch series.
>>>>
>>>> And the reason I have insist on splitting these changes is because, if
>>>> we have to alter the .S files to be consistent, then this comment
>>>> harmonization request should logically be handled separately from the
>>>> VS2017 effort.
>>>>
>>>> Please let me know if you still think having a future separate patch,
>>>> that will do .S and .asm comment harmonization, does not make sense.
>>>>
>>>> Regards,
>>>>
>>>> /Pete
>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Pete Batard [mailto:pete@akeo.ie]
>>>>>> Sent: Thursday, March 15, 2018 5:28 PM
>>>>>> To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
>>>>>> Cc: ard.biesheuvel@linaro.org
>>>>>> Subject: Re: [PATCH 0/4] Add ARM64 support for VS2017
>>>>>>
>>>>>> Hi Liming,
>>>>>>
>>>>>> Thanks for reviewing the patches.
>>>>>>
>>>>>> On 2018.03.15 06:15, Gao, Liming wrote:
>>>>>>> Pete:
>>>>>>> For new added ASM file in BaseLib, could you use the same comment style
>>>>>>> for them? ASM use ; for the comment. Most of new files uses ; as the
>>>>>>> comment, but switchstack is not.
>>>>>>
>>>>>> This is because SwitchStack.asm is simply SwitchStack.S, with the GCC
>>>>>> assembler specifics removed, and MSVC assembler specifics added.
>>>>>>
>>>>>> I did not change the comment style from the original files, so the real
>>>>>> issue here is that our GCC assembly files for AARCH64 do not use the
>>>>>> same comment style.
>>>>>>
>>>>>> I'm fine with trying to harmonize the comment styles, but seeing as this
>>>>>> needs to be done for both the .S and .asm, I'd rather send a patch to do
>>>>>> that *after* these VS2017 changes have been applied, as I don't consider
>>>>>> this correction to in scope of this patch series (because logically, the
>>>>>> introduction of VS2017 should not alter any of the .S files, unless we
>>>>>> reuse them, which we don't).
>>>>>>
>>>>>> If you agree to apply this series, I'll make sure to send a non
>>>>>> VS2017-specific additional patch, that does what you request for both
>>>>>> the .S and .asm.
>>>>>>
>>>>>>> Besides, compared to Arm arch assembly
>>>>>>> file, I don't find CpuPause.asm. Is it required?
>>>>>>
>>>>>> That file doesn't exist for GCC (as you will see there is no
>>>>>> MdePkg/Library/BaseLib/AArch64/CpuPause.S), so we don't have one for
>>>>>> VS2017 either.
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> /Pete
>>>>
>>>> _______________________________________________
>>>> edk2-devel mailing list
>>>> edk2-devel@lists.01.org
>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/4] Add ARM64 support for VS2017
2018-03-16 16:35 ` Pete Batard
@ 2018-03-19 9:07 ` Gao, Liming
0 siblings, 0 replies; 17+ messages in thread
From: Gao, Liming @ 2018-03-19 9:07 UTC (permalink / raw)
To: Pete Batard, edk2-devel@lists.01.org; +Cc: Ard Biesheuvel
Pete:
I push your patches into edk2 trunk. Please check them.
>-----Original Message-----
>From: Pete Batard [mailto:pete@akeo.ie]
>Sent: Saturday, March 17, 2018 12:36 AM
>To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
>Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>Subject: Re: [edk2] [PATCH 0/4] Add ARM64 support for VS2017
>
>Thanks Liming, much appreciated!
>
>I'll send the comment harmonization patch as soon as I see the
>VS2017/ARM64 changes in edk2 mainline.
>
>Regards,
>
>/Pete
>
>On 2018.03.16 16:31, Gao, Liming wrote:
>> Yes. This is a minor issue. So, I think the effort is small. If it is a big task to you,
>you can separate it into another patch.
>>
>> And, I don't expect this minor issue break your patches. I give my Reviewed-
>by: Liming Gao <liming.gao@intel.com>
>>
>> Thanks
>> Liming
>>> -----Original Message-----
>>> From: Pete Batard [mailto:pete@akeo.ie]
>>> Sent: Saturday, March 17, 2018 12:12 AM
>>> To: edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Subject: Re: [edk2] [PATCH 0/4] Add ARM64 support for VS2017
>>>
>>> I understand where you're coming from, but that means I have to recreate
>>> this patch set, and then create a new patch for the .S (because it makes
>>> zero sense to require the same comment style on the .asm and not
>request
>>> a follow through for the .S).
>>>
>>> My time being limited, I'd rather only have to produce one new patch,
>>> that will harmonize the comments for both .S and .asm at the same time.
>>>
>>> The end result will be exactly the same, so I'm going to have to insist
>>> that we split the comment harmonization (which is a very minor issue and
>>> should hardly be seen as a showstopper for the patch series in the first
>>> place) into a subsequent patch.
>>>
>>> Thank you,
>>>
>>> /Pete
>>>
>>> On 2018.03.16 15:56, Gao, Liming wrote:
>>>> Pete:
>>>> I understand the existing .S file has the inconsistent comment style. I
>also know new added ASM files are converted from .S files.
>>> But, my comment is for this patch that adds new ASM files. I expect new
>added ASM files have the same style. If you check ARM arch
>>> ASM files, you will find they all have the same style.
>>>>
>>>> Thanks
>>>> Liming
>>>>> -----Original Message-----
>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
>Of Pete Batard
>>>>> Sent: Friday, March 16, 2018 7:04 PM
>>>>> To: edk2-devel@lists.01.org
>>>>> Cc: Gao, Liming <liming.gao@intel.com>; Ard Biesheuvel
><ard.biesheuvel@linaro.org>
>>>>> Subject: Re: [edk2] [PATCH 0/4] Add ARM64 support for VS2017
>>>>>
>>>>> On 2018.03.16 08:24, Gao, Liming wrote:
>>>>>> Pete:
>>>>>> .S for GCC assembly, .asm for MSFT assembly. They can have the
>different comment style.
>>>>>
>>>>> Yes, but as I explained, the actual original issue is that our current
>>>>> .S files do *not* have the same comment styles in the first place.
>>>>>
>>>>> If you look at MdePkg/Library/BaseLib/AArch64/SwitchStack.S, you'll
>see
>>>>> that is uses '//' for comments, whereas other .S files, such as
>>>>> MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S, use '#'.
>>>>>
>>>>> So that is our actual issue here: Regardless of VS2017, the GCC assembly
>>>>> files for AARCH64 we currently have do not use the same comment style.
>>>>>
>>>>> Thus, the only reason why the .asm don't have the same comment style
>in
>>>>> our proposal is because the .S, which we derived the .asm from, don't.
>>>>> This means that either we should fix the .S too, or we shouldn't fix
>>>>> anything at all.
>>>>>
>>>>>> Here, my comment is to make sure .asm files have the same
>comment style. I don't request to change .S file.
>>>>>
>>>>> And what I am saying is that it makes little sense to harmonize the
>>>>> comment style for the .asm files, if we're not going to do the same for
>>>>> the .S files as well. It just doesn't seem fair in my book to have the
>>>>> VS2017 assembly files held to a higher standard than the GCC ones. So
>>>>> either we need to fix both, or we fix none at all.
>>>>>
>>>>> But as I indicated in my last e-mail, I am planning to send an
>>>>> additional patch that does comment harmonization, for both .S and .asm,
>>>>> *after* this VS2017 series has been applied to mainline. So the change
>>>>> you request will happen. Just not as part of this patch series.
>>>>>
>>>>> And the reason I have insist on splitting these changes is because, if
>>>>> we have to alter the .S files to be consistent, then this comment
>>>>> harmonization request should logically be handled separately from the
>>>>> VS2017 effort.
>>>>>
>>>>> Please let me know if you still think having a future separate patch,
>>>>> that will do .S and .asm comment harmonization, does not make sense.
>>>>>
>>>>> Regards,
>>>>>
>>>>> /Pete
>>>>>
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Pete Batard [mailto:pete@akeo.ie]
>>>>>>> Sent: Thursday, March 15, 2018 5:28 PM
>>>>>>> To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
>>>>>>> Cc: ard.biesheuvel@linaro.org
>>>>>>> Subject: Re: [PATCH 0/4] Add ARM64 support for VS2017
>>>>>>>
>>>>>>> Hi Liming,
>>>>>>>
>>>>>>> Thanks for reviewing the patches.
>>>>>>>
>>>>>>> On 2018.03.15 06:15, Gao, Liming wrote:
>>>>>>>> Pete:
>>>>>>>> For new added ASM file in BaseLib, could you use the same
>comment style
>>>>>>>> for them? ASM use ; for the comment. Most of new files uses ; as
>the
>>>>>>>> comment, but switchstack is not.
>>>>>>>
>>>>>>> This is because SwitchStack.asm is simply SwitchStack.S, with the GCC
>>>>>>> assembler specifics removed, and MSVC assembler specifics added.
>>>>>>>
>>>>>>> I did not change the comment style from the original files, so the real
>>>>>>> issue here is that our GCC assembly files for AARCH64 do not use the
>>>>>>> same comment style.
>>>>>>>
>>>>>>> I'm fine with trying to harmonize the comment styles, but seeing as
>this
>>>>>>> needs to be done for both the .S and .asm, I'd rather send a patch to
>do
>>>>>>> that *after* these VS2017 changes have been applied, as I don't
>consider
>>>>>>> this correction to in scope of this patch series (because logically, the
>>>>>>> introduction of VS2017 should not alter any of the .S files, unless we
>>>>>>> reuse them, which we don't).
>>>>>>>
>>>>>>> If you agree to apply this series, I'll make sure to send a non
>>>>>>> VS2017-specific additional patch, that does what you request for both
>>>>>>> the .S and .asm.
>>>>>>>
>>>>>>>> Besides, compared to Arm arch assembly
>>>>>>>> file, I don't find CpuPause.asm. Is it required?
>>>>>>>
>>>>>>> That file doesn't exist for GCC (as you will see there is no
>>>>>>> MdePkg/Library/BaseLib/AArch64/CpuPause.S), so we don't have
>one for
>>>>>>> VS2017 either.
>>>>>>>
>>>>>>> Regards,
>>>>>>>
>>>>>>> /Pete
>>>>>
>>>>> _______________________________________________
>>>>> edk2-devel mailing list
>>>>> edk2-devel@lists.01.org
>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2018-03-19 9:01 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-02-23 9:49 [PATCH 0/4] Add ARM64 support for VS2017 Pete Batard
2018-02-23 9:50 ` [PATCH 1/4] MdePkg: Disable some Level 4 warnings for VS2017/ARM64 Pete Batard
2018-02-23 9:50 ` [PATCH 2/4] MdePkg/Library/BaseLib: Enable VS2017/ARM64 builds Pete Batard
2018-02-23 9:50 ` [PATCH 3/4] MdePkg/Include: Add VA list support for VS2017/ARM64 Pete Batard
2018-02-23 9:50 ` [PATCH 4/4] BaseTools/Conf: Add VS2017/ARM64 support Pete Batard
2018-02-23 11:55 ` [PATCH 0/4] Add ARM64 support for VS2017 Ard Biesheuvel
2018-02-23 13:14 ` Pete Batard
2018-03-15 6:15 ` Gao, Liming
2018-03-15 9:28 ` Pete Batard
2018-03-16 8:24 ` Gao, Liming
2018-03-16 11:03 ` Pete Batard
2018-03-16 15:56 ` Gao, Liming
2018-03-16 16:11 ` Pete Batard
2018-03-16 16:31 ` Gao, Liming
2018-03-16 16:35 ` Pete Batard
2018-03-19 9:07 ` Gao, Liming
-- strict thread matches above, loose matches on Subject: below --
2018-02-14 13:08 Pete Batard
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox