Re: [PATCH 1/1] ArmPkg/ArmSvcLib: Add ArmSvcLib implementation.

[Supreeth Venkatesh <supreeth.venkatesh@arm.com>]

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