On Tue, 2017-09-19 at 13:14 -0700, Ard Biesheuvel wrote: > Hi Supreeth, > > On 19 September 2017 at 12:55, Supreeth Venkatesh > <[email protected]> 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 <[email protected]> > > Signed-off-by: Supreeth Venkatesh <[email protected]> > > --- > > 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 > > [email protected] > > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
