* [PATCH 1/1] ArmPkg/ArmSvcLib: Add ArmSvcLib implementation.
@ 2017-09-19 19:55 Supreeth Venkatesh
2017-09-19 20:14 ` Ard Biesheuvel
0 siblings, 1 reply; 3+ messages in thread
From: Supreeth Venkatesh @ 2017-09-19 19:55 UTC (permalink / raw)
To: edk2-devel; +Cc: leif.lindholm, ard.biesheuvel, Supreeth Venkatesh, Achin Gupta
This patch adds a library that enables invocation of SVCs from Exception
Level EL0. It will be used by the Standalone MM environment to request
services from a software running in a privileged EL e.g. ARM Trusted
Firmware. The library is a derived directly from Arm SMC Library.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Achin Gupta <achin.gupta@arm.com>
Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
---
ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S | 41 ++++++++++++++++++++++++
ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.S | 52 +++++++++++++++++++++++++++++++
ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.asm | 51 ++++++++++++++++++++++++++++++
ArmPkg/Library/ArmSvcLib/ArmSvcLib.inf | 31 ++++++++++++++++++
4 files changed, 175 insertions(+)
create mode 100644 ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S
create mode 100644 ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.S
create mode 100644 ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.asm
create mode 100644 ArmPkg/Library/ArmSvcLib/ArmSvcLib.inf
diff --git a/ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S b/ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S
new file mode 100644
index 0000000000..70122bbb0e
--- /dev/null
+++ b/ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S
@@ -0,0 +1,41 @@
+//
+// Copyright (c) 2012 - 2017, ARM Limited. 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.
+//
+//
+
+.text
+.align 3
+
+GCC_ASM_EXPORT(ArmCallSvc)
+
+ASM_PFX(ArmCallSvc):
+ // Push x0 on the stack - The stack must always be quad-word aligned
+ str x0, [sp, #-16]!
+
+ // Load the SVC arguments values into the appropriate registers
+ ldp x6, x7, [x0, #48]
+ ldp x4, x5, [x0, #32]
+ ldp x2, x3, [x0, #16]
+ ldp x0, x1, [x0, #0]
+
+ svc #0
+
+ // Pop the ARM_SVC_ARGS structure address from the stack into x9
+ ldr x9, [sp], #16
+
+ // Store the SVC returned values into the ARM_SVC_ARGS structure.
+ // A SVC call can return up to 4 values - we do not need to store back x4-x7.
+ stp x2, x3, [x9, #16]
+ stp x0, x1, [x9, #0]
+
+ mov x0, x9
+
+ ret
diff --git a/ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.S b/ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.S
new file mode 100644
index 0000000000..a7ac2966c3
--- /dev/null
+++ b/ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.S
@@ -0,0 +1,52 @@
+//
+// Copyright (c) 2016 - 2017, ARM Limited. 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.
+//
+//
+
+.text
+.align 3
+.arch_extension sec
+
+GCC_ASM_EXPORT(ArmCallSvc)
+
+ASM_PFX(ArmCallSvc):
+ push {r4-r8}
+ // r0 will be popped just after the SVC call
+ push {r0}
+
+ // Load the SVC arguments values into the appropriate registers
+ ldr r7, [r0, #28]
+ ldr r6, [r0, #24]
+ ldr r5, [r0, #20]
+ ldr r4, [r0, #16]
+ ldr r3, [r0, #12]
+ ldr r2, [r0, #8]
+ ldr r1, [r0, #4]
+ ldr r0, [r0, #0]
+
+ svc #0
+
+ // Pop the ARM_SVC_ARGS structure address from the stack into r8
+ pop {r8}
+
+ // Load the SVC returned values into the appropriate registers
+ // A SVC call can return up to 4 values - we do not need to store back r4-r7.
+ str r3, [r8, #12]
+ str r2, [r8, #8]
+ str r1, [r8, #4]
+ str r0, [r8, #0]
+
+ mov r0, r8
+
+ // Restore the registers r4-r8
+ pop {r4-r8}
+
+ bx lr
diff --git a/ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.asm b/ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.asm
new file mode 100644
index 0000000000..25edbf1939
--- /dev/null
+++ b/ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.asm
@@ -0,0 +1,51 @@
+//
+// Copyright (c) 2016 - 2017, ARM Limited. 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.
+//
+//
+
+
+ INCLUDE AsmMacroExport.inc
+
+ RVCT_ASM_EXPORT ArmCallSvc
+ push {r4-r8}
+ // r0 will be popped just after the SVC call
+ push {r0}
+
+ // Load the SVC arguments values into the appropriate registers
+ ldr r7, [r0, #28]
+ ldr r6, [r0, #24]
+ ldr r5, [r0, #20]
+ ldr r4, [r0, #16]
+ ldr r3, [r0, #12]
+ ldr r2, [r0, #8]
+ ldr r1, [r0, #4]
+ ldr r0, [r0, #0]
+
+ svc #0
+
+ // Pop the ARM_SVC_ARGS structure address from the stack into r8
+ pop {r8}
+
+ // Load the SVC returned values into the appropriate registers
+ // A SVC call can return up to 4 values - we do not need to store back r4-r7.
+ str r3, [r8, #12]
+ str r2, [r8, #8]
+ str r1, [r8, #4]
+ str r0, [r8, #0]
+
+ mov r0, r8
+
+ // Restore the registers r4-r8
+ pop {r4-r8}
+
+ bx lr
+
+ END
diff --git a/ArmPkg/Library/ArmSvcLib/ArmSvcLib.inf b/ArmPkg/Library/ArmSvcLib/ArmSvcLib.inf
new file mode 100644
index 0000000000..0c41eaec9a
--- /dev/null
+++ b/ArmPkg/Library/ArmSvcLib/ArmSvcLib.inf
@@ -0,0 +1,31 @@
+#/** @file
+#
+# Copyright (c) 2016 - 2017, 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.
+#
+#**/
+
+[Defines]
+ INF_VERSION = 0x00010005
+ BASE_NAME = ArmSvcLib
+ FILE_GUID = eb3f17d5-a3cc-4eac-8912-84162d0f79da
+ MODULE_TYPE = BASE
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = ArmSvcLib
+
+[Sources.ARM]
+ Arm/ArmSvc.asm | RVCT
+ Arm/ArmSvc.S | GCC
+
+[Sources.AARCH64]
+ AArch64/ArmSvc.S
+
+[Packages]
+ ArmPkg/ArmPkg.dec
+ MdePkg/MdePkg.dec
--
2.14.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 1/1] ArmPkg/ArmSvcLib: Add ArmSvcLib implementation.
2017-09-19 19:55 [PATCH 1/1] ArmPkg/ArmSvcLib: Add ArmSvcLib implementation Supreeth Venkatesh
@ 2017-09-19 20:14 ` Ard Biesheuvel
2017-09-20 15:36 ` Supreeth Venkatesh
0 siblings, 1 reply; 3+ messages in thread
From: Ard Biesheuvel @ 2017-09-19 20:14 UTC (permalink / raw)
To: Supreeth Venkatesh; +Cc: edk2-devel@lists.01.org, Leif Lindholm
Hi Supreeth,
On 19 September 2017 at 12:55, Supreeth Venkatesh
<supreeth.venkatesh@arm.com> wrote:
> This patch adds a library that enables invocation of SVCs from Exception
> Level EL0. It will be used by the Standalone MM environment to request
> services from a software running in a privileged EL e.g. ARM Trusted
> Firmware. The library is a derived directly from Arm SMC Library.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Achin Gupta <achin.gupta@arm.com>
> Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
> ---
> ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S | 41 ++++++++++++++++++++++++
> ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.S | 52 +++++++++++++++++++++++++++++++
> ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.asm | 51 ++++++++++++++++++++++++++++++
> ArmPkg/Library/ArmSvcLib/ArmSvcLib.inf | 31 ++++++++++++++++++
Please add this library to the [Components] section of ArmPkg.dsc so
it gets built when we build that package.
Also, it appears the header file is missing, although I suppose it
could be part of a separate patch. However, this is 1/1 and it's
missing from this patch.
> 4 files changed, 175 insertions(+)
> create mode 100644 ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S
> create mode 100644 ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.S
> create mode 100644 ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.asm
> create mode 100644 ArmPkg/Library/ArmSvcLib/ArmSvcLib.inf
>
> diff --git a/ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S b/ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S
> new file mode 100644
> index 0000000000..70122bbb0e
> --- /dev/null
> +++ b/ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S
> @@ -0,0 +1,41 @@
> +//
> +// Copyright (c) 2012 - 2017, ARM Limited. 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.
> +//
> +//
> +
> +.text
> +.align 3
> +
> +GCC_ASM_EXPORT(ArmCallSvc)
> +
> +ASM_PFX(ArmCallSvc):
> + // Push x0 on the stack - The stack must always be quad-word aligned
> + str x0, [sp, #-16]!
> +
Please create a proper stackframe here, by pushing the frame pointer
and return address to the stack as well. You should always do that
when you use the stack, even if the return address and frame pointer
are not corrupted by this function.
> + // Load the SVC arguments values into the appropriate registers
> + ldp x6, x7, [x0, #48]
> + ldp x4, x5, [x0, #32]
> + ldp x2, x3, [x0, #16]
> + ldp x0, x1, [x0, #0]
> +
> + svc #0
> +
> + // Pop the ARM_SVC_ARGS structure address from the stack into x9
> + ldr x9, [sp], #16
> +
> + // Store the SVC returned values into the ARM_SVC_ARGS structure.
> + // A SVC call can return up to 4 values - we do not need to store back x4-x7.
> + stp x2, x3, [x9, #16]
> + stp x0, x1, [x9, #0]
> +
Nit: no reason to do the stores in opposite order here.
> + mov x0, x9
> +
> + ret
> diff --git a/ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.S b/ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.S
> new file mode 100644
> index 0000000000..a7ac2966c3
> --- /dev/null
> +++ b/ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.S
> @@ -0,0 +1,52 @@
> +//
> +// Copyright (c) 2016 - 2017, ARM Limited. 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.
> +//
> +//
> +
> +.text
> +.align 3
> +.arch_extension sec
> +
> +GCC_ASM_EXPORT(ArmCallSvc)
> +
> +ASM_PFX(ArmCallSvc):
> + push {r4-r8}
> + // r0 will be popped just after the SVC call
> + push {r0}
> +
I'd prefer it if this didn't leave the stack misaligned between the
two instructions. Is there anything wrong with
push {r0, r4-r8}
?
> + // Load the SVC arguments values into the appropriate registers
> + ldr r7, [r0, #28]
> + ldr r6, [r0, #24]
> + ldr r5, [r0, #20]
> + ldr r4, [r0, #16]
> + ldr r3, [r0, #12]
> + ldr r2, [r0, #8]
> + ldr r1, [r0, #4]
> + ldr r0, [r0, #0]
> +
Does
ldm r0, {r0-r7}
not work here?
> + svc #0
> +
> + // Pop the ARM_SVC_ARGS structure address from the stack into r8
> + pop {r8}
> +
> + // Load the SVC returned values into the appropriate registers
> + // A SVC call can return up to 4 values - we do not need to store back r4-r7.
> + str r3, [r8, #12]
> + str r2, [r8, #8]
> + str r1, [r8, #4]
> + str r0, [r8, #0]
> +
> + mov r0, r8
> +
> + // Restore the registers r4-r8
> + pop {r4-r8}
> +
Same here. Please find a way to pop the stack in a single operation.
> + bx lr
> diff --git a/ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.asm b/ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.asm
> new file mode 100644
> index 0000000000..25edbf1939
> --- /dev/null
> +++ b/ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.asm
> @@ -0,0 +1,51 @@
> +//
> +// Copyright (c) 2016 - 2017, ARM Limited. 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.
> +//
> +//
> +
> +
> + INCLUDE AsmMacroExport.inc
> +
> + RVCT_ASM_EXPORT ArmCallSvc
> + push {r4-r8}
> + // r0 will be popped just after the SVC call
> + push {r0}
> +
> + // Load the SVC arguments values into the appropriate registers
> + ldr r7, [r0, #28]
> + ldr r6, [r0, #24]
> + ldr r5, [r0, #20]
> + ldr r4, [r0, #16]
> + ldr r3, [r0, #12]
> + ldr r2, [r0, #8]
> + ldr r1, [r0, #4]
> + ldr r0, [r0, #0]
> +
> + svc #0
> +
> + // Pop the ARM_SVC_ARGS structure address from the stack into r8
> + pop {r8}
> +
> + // Load the SVC returned values into the appropriate registers
> + // A SVC call can return up to 4 values - we do not need to store back r4-r7.
> + str r3, [r8, #12]
> + str r2, [r8, #8]
> + str r1, [r8, #4]
> + str r0, [r8, #0]
> +
> + mov r0, r8
> +
> + // Restore the registers r4-r8
> + pop {r4-r8}
> +
> + bx lr
> +
> + END
Same comments as above
> diff --git a/ArmPkg/Library/ArmSvcLib/ArmSvcLib.inf b/ArmPkg/Library/ArmSvcLib/ArmSvcLib.inf
> new file mode 100644
> index 0000000000..0c41eaec9a
> --- /dev/null
> +++ b/ArmPkg/Library/ArmSvcLib/ArmSvcLib.inf
> @@ -0,0 +1,31 @@
> +#/** @file
> +#
> +# Copyright (c) 2016 - 2017, 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.
> +#
> +#**/
> +
> +[Defines]
> + INF_VERSION = 0x00010005
This should be 1.26 IIRC? (0x0001001A)
> + BASE_NAME = ArmSvcLib
> + FILE_GUID = eb3f17d5-a3cc-4eac-8912-84162d0f79da
$ git grep -i eb3f17d5
ArmPkg/Library/ArmSmcLib/ArmSmcLib.inf: FILE_GUID
= eb3f17d5-a3cc-4eac-8912-84162d0f79da
> + MODULE_TYPE = BASE
> + VERSION_STRING = 1.0
> + LIBRARY_CLASS = ArmSvcLib
> +
> +[Sources.ARM]
> + Arm/ArmSvc.asm | RVCT
> + Arm/ArmSvc.S | GCC
> +
> +[Sources.AARCH64]
> + AArch64/ArmSvc.S
> +
> +[Packages]
> + ArmPkg/ArmPkg.dec
> + MdePkg/MdePkg.dec
> --
> 2.14.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 1/1] ArmPkg/ArmSvcLib: Add ArmSvcLib implementation.
2017-09-19 20:14 ` Ard Biesheuvel
@ 2017-09-20 15:36 ` Supreeth Venkatesh
0 siblings, 0 replies; 3+ messages in thread
From: Supreeth Venkatesh @ 2017-09-20 15:36 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel@lists.01.org, Leif Lindholm
On Tue, 2017-09-19 at 13:14 -0700, Ard Biesheuvel wrote:
> Hi Supreeth,
>
> On 19 September 2017 at 12:55, Supreeth Venkatesh
> <supreeth.venkatesh@arm.com> wrote:
> >
> > This patch adds a library that enables invocation of SVCs from
> > Exception
> > Level EL0. It will be used by the Standalone MM environment to
> > request
> > services from a software running in a privileged EL e.g. ARM
> > Trusted
> > Firmware. The library is a derived directly from Arm SMC Library.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Achin Gupta <achin.gupta@arm.com>
> > Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
> > ---
> > ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S | 41
> > ++++++++++++++++++++++++
> > ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.S | 52
> > +++++++++++++++++++++++++++++++
> > ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.asm | 51
> > ++++++++++++++++++++++++++++++
> > ArmPkg/Library/ArmSvcLib/ArmSvcLib.inf | 31 ++++++++++++++++++
> Please add this library to the [Components] section of ArmPkg.dsc so
> it gets built when we build that package.
>
> Also, it appears the header file is missing, although I suppose it
> could be part of a separate patch. However, this is 1/1 and it's
> missing from this patch.
Ard,
Thank you for reviewing it.
I intended to send ArmPkg.dsc and header changes as a separate patch.
However, if you want it as part of this series, I can do that as well.
>
>
> >
> > 4 files changed, 175 insertions(+)
> > create mode 100644 ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S
> > create mode 100644 ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.S
> > create mode 100644 ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.asm
> > create mode 100644 ArmPkg/Library/ArmSvcLib/ArmSvcLib.inf
> >
> > diff --git a/ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S
> > b/ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S
> > new file mode 100644
> > index 0000000000..70122bbb0e
> > --- /dev/null
> > +++ b/ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S
> > @@ -0,0 +1,41 @@
> > +//
> > +// Copyright (c) 2012 - 2017, ARM Limited. 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.
> > +//
> > +//
> > +
> > +.text
> > +.align 3
> > +
> > +GCC_ASM_EXPORT(ArmCallSvc)
> > +
> > +ASM_PFX(ArmCallSvc):
> > + // Push x0 on the stack - The stack must always be quad-word
> > aligned
> > + str x0, [sp, #-16]!
> > +
> Please create a proper stackframe here, by pushing the frame pointer
> and return address to the stack as well. You should always do that
> when you use the stack, even if the return address and frame pointer
> are not corrupted by this function.
Can do. However, this is a direct replica of ArmSmcLib. Let me know if
you want me to send a patch to reflect the above changes in ArmSmcLib
as well.
>
> >
> > + // Load the SVC arguments values into the appropriate registers
> > + ldp x6, x7, [x0, #48]
> > + ldp x4, x5, [x0, #32]
> > + ldp x2, x3, [x0, #16]
> > + ldp x0, x1, [x0, #0]
> > +
> > + svc #0
> > +
> > + // Pop the ARM_SVC_ARGS structure address from the stack into x9
> > + ldr x9, [sp], #16
> > +
> > + // Store the SVC returned values into the ARM_SVC_ARGS
> > structure.
> > + // A SVC call can return up to 4 values - we do not need to
> > store back x4-x7.
> > + stp x2, x3, [x9, #16]
> > + stp x0, x1, [x9, #0]
> > +
> Nit: no reason to do the stores in opposite order here.
Agree. Can change it to reflect ascending register order.
>
> >
> > + mov x0, x9
> > +
> > + ret
> > diff --git a/ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.S
> > b/ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.S
> > new file mode 100644
> > index 0000000000..a7ac2966c3
> > --- /dev/null
> > +++ b/ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.S
> > @@ -0,0 +1,52 @@
> > +//
> > +// Copyright (c) 2016 - 2017, ARM Limited. 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.
> > +//
> > +//
> > +
> > +.text
> > +.align 3
> > +.arch_extension sec
> > +
> > +GCC_ASM_EXPORT(ArmCallSvc)
> > +
> > +ASM_PFX(ArmCallSvc):
> > + push {r4-r8}
> > + // r0 will be popped just after the SVC call
> > + push {r0}
> > +
> I'd prefer it if this didn't leave the stack misaligned between the
> two instructions. Is there anything wrong with
>
> push {r0, r4-r8}
>
> ?
Can do. However, this is a direct replica of ArmSmcLib. Let me know if
you want me to send a patch to reflect the above changes in ArmSmcLib
as well. (May be few weeks later, if needed)
>
> >
> > + // Load the SVC arguments values into the appropriate
> > registers
> > + ldr r7, [r0, #28]
> > + ldr r6, [r0, #24]
> > + ldr r5, [r0, #20]
> > + ldr r4, [r0, #16]
> > + ldr r3, [r0, #12]
> > + ldr r2, [r0, #8]
> > + ldr r1, [r0, #4]
> > + ldr r0, [r0, #0]
> > +
> Does
>
> ldm r0, {r0-r7}
>
> not work here?
Should work. Will test it and update the patch. However, this is a
direct replica of ArmSmcLib. Let me know if you want me to send a patch
to reflect the above changes in ArmSmcLib as well. (May be few weeks
later, if needed)
>
> >
> > + svc #0
> > +
> > + // Pop the ARM_SVC_ARGS structure address from the stack into
> > r8
> > + pop {r8}
> > +
> > + // Load the SVC returned values into the appropriate registers
> > + // A SVC call can return up to 4 values - we do not need to
> > store back r4-r7.
> > + str r3, [r8, #12]
> > + str r2, [r8, #8]
> > + str r1, [r8, #4]
> > + str r0, [r8, #0]
> > +
> > + mov r0, r8
> > +
> > + // Restore the registers r4-r8
> > + pop {r4-r8}
> > +
> Same here. Please find a way to pop the stack in a single operation.
I guess "stm" can be used. Will test it and send an updated patch.
Same Comment regarding ArmSmcLib.
>
> >
> > + bx lr
> > diff --git a/ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.asm
> > b/ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.asm
> > new file mode 100644
> > index 0000000000..25edbf1939
> > --- /dev/null
> > +++ b/ArmPkg/Library/ArmSvcLib/Arm/ArmSvc.asm
> > @@ -0,0 +1,51 @@
> > +//
> > +// Copyright (c) 2016 - 2017, ARM Limited. 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.
> > +//
> > +//
> > +
> > +
> > + INCLUDE AsmMacroExport.inc
> > +
> > + RVCT_ASM_EXPORT ArmCallSvc
> > + push {r4-r8}
> > + // r0 will be popped just after the SVC call
> > + push {r0}
> > +
> > + // Load the SVC arguments values into the appropriate
> > registers
> > + ldr r7, [r0, #28]
> > + ldr r6, [r0, #24]
> > + ldr r5, [r0, #20]
> > + ldr r4, [r0, #16]
> > + ldr r3, [r0, #12]
> > + ldr r2, [r0, #8]
> > + ldr r1, [r0, #4]
> > + ldr r0, [r0, #0]
> > +
> > + svc #0
> > +
> > + // Pop the ARM_SVC_ARGS structure address from the stack into
> > r8
> > + pop {r8}
> > +
> > + // Load the SVC returned values into the appropriate registers
> > + // A SVC call can return up to 4 values - we do not need to
> > store back r4-r7.
> > + str r3, [r8, #12]
> > + str r2, [r8, #8]
> > + str r1, [r8, #4]
> > + str r0, [r8, #0]
> > +
> > + mov r0, r8
> > +
> > + // Restore the registers r4-r8
> > + pop {r4-r8}
> > +
> > + bx lr
> > +
> > + END
> Same comments as above
Ditto.
>
> >
> > diff --git a/ArmPkg/Library/ArmSvcLib/ArmSvcLib.inf
> > b/ArmPkg/Library/ArmSvcLib/ArmSvcLib.inf
> > new file mode 100644
> > index 0000000000..0c41eaec9a
> > --- /dev/null
> > +++ b/ArmPkg/Library/ArmSvcLib/ArmSvcLib.inf
> > @@ -0,0 +1,31 @@
> > +#/** @file
> > +#
> > +# Copyright (c) 2016 - 2017, 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.
> > +#
> > +#**/
> > +
> > +[Defines]
> > + INF_VERSION = 0x00010005
> This should be 1.26 IIRC? (0x0001001A)
>
> >
> > + BASE_NAME = ArmSvcLib
> > + FILE_GUID = eb3f17d5-a3cc-4eac-8912-
> > 84162d0f79da
> $ git grep -i eb3f17d5
> ArmPkg/Library/ArmSmcLib/ArmSmcLib.inf: FILE_GUID
> = eb3f17d5-a3cc-4eac-8912-84162d0f79da
>
Thanks for pointing it out. Thats what happens, if I blindly replicate
ArmSmcLib :(.
> >
> > + MODULE_TYPE = BASE
> > + VERSION_STRING = 1.0
> > + LIBRARY_CLASS = ArmSvcLib
> > +
> > +[Sources.ARM]
> > + Arm/ArmSvc.asm | RVCT
> > + Arm/ArmSvc.S | GCC
> > +
> > +[Sources.AARCH64]
> > + AArch64/ArmSvc.S
> > +
> > +[Packages]
> > + ArmPkg/ArmPkg.dec
> > + MdePkg/MdePkg.dec
> > --
> > 2.14.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-09-20 15:33 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-19 19:55 [PATCH 1/1] ArmPkg/ArmSvcLib: Add ArmSvcLib implementation Supreeth Venkatesh
2017-09-19 20:14 ` Ard Biesheuvel
2017-09-20 15:36 ` Supreeth Venkatesh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox