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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

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