From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (usa-sjc-mx-foss1.foss.arm.com [217.140.101.70]) by ml01.01.org (Postfix) with ESMTP id 5628F21ECCB1C for ; Wed, 20 Sep 2017 08:33:01 -0700 (PDT) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id AA8E215A2; Wed, 20 Sep 2017 08:36:06 -0700 (PDT) Received: from [10.0.2.15] (u203142.usa.arm.com [10.118.30.109]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id 4ADE53F58C; Wed, 20 Sep 2017 08:36:05 -0700 (PDT) Message-ID: <1505921765.2959.9.camel@arm.com> From: Supreeth Venkatesh To: Ard Biesheuvel Cc: "edk2-devel@lists.01.org" , Leif Lindholm Date: Wed, 20 Sep 2017 10:36:05 -0500 In-Reply-To: References: <20170919195529.29176-1-supreeth.venkatesh@arm.com> X-Mailer: Evolution 3.18.5.2-0ubuntu3.2 Mime-Version: 1.0 Subject: Re: [PATCH 1/1] ArmPkg/ArmSvcLib: Add ArmSvcLib implementation. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 Sep 2017 15:33:01 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit On Tue, 2017-09-19 at 13:14 -0700, Ard Biesheuvel wrote: > Hi Supreeth, > > On 19 September 2017 at 12:55, Supreeth Venkatesh > 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 > > Signed-off-by: Supreeth Venkatesh > > --- > >  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.
> > +#  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