From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x22d.google.com (mail-it0-x22d.google.com [IPv6:2607:f8b0:4001:c0b::22d]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 675FD21CEB0F9 for ; Tue, 19 Sep 2017 13:11:42 -0700 (PDT) Received: by mail-it0-x22d.google.com with SMTP id d123so677837ith.3 for ; Tue, 19 Sep 2017 13:14:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=qlCl3IzVUF3Zit/WgeexkpPQTJ8T+TyJTrZ0JCIjBFs=; b=BwOKI9iG00U6/zE96dqdu0jnvKXe1EbmXCuetXAPc0+YPyYvymjIyarvFfqx2vTmH3 qk+TupxCcV090nazg3AJYlDcafW55NYEk2QKl4suA0e10aR+Aanj8pK9zT7YaD+bfudx GfmmBy6Di3i1oKmNYbzkAjpieLe9YFVuRhelE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=qlCl3IzVUF3Zit/WgeexkpPQTJ8T+TyJTrZ0JCIjBFs=; b=Zriazfv9oOb6GmTNaXCTPQhsJpJHVwLMW7lP+Ec8X5he3W+8ZvsAQZhmTvkU7lc78e 1s1X6vjouHw21g5dMuyDGENNst/sFHygrGe59/eU8HBXm09b3caXHXoAHVeoiPrnvUyv zl+rJASpJOn3GnAQYST5BYNouEp1PMX+VbTeC2a/ciMLz9r3VH2ZCbHGWFjGHpZIT6CO I/snO+i62mAC5A6/95/4L0h51SpJWqdC6BQ3YFYgiDVZkkqLOmnjD40k1AHJckRzK5k2 W0lXnXJPHXOFq647LmmdshzuPLuAWB0N3HdZCxs+7jcpypAHyfUSpeuWwWLb8qqso5td htPw== X-Gm-Message-State: AHPjjUiXrE/CAIWFpg/eLmINe65nyMpTkQ9MUqK9WlgdP5EBI+Ug4Q3M PoMb8KNGatExJqNwm8lYv2Eats9qjxMR9lycyMociw== X-Google-Smtp-Source: AOwi7QCkNGLoRYQhOjb8nCXQIFuOf9vhDYHqGTPa6auEURSklyClLiwr5juUkuoJNhl+AZ/hI94THMj0bl/xmmnTGqM= X-Received: by 10.36.36.67 with SMTP id f64mr3469128ita.10.1505852086406; Tue, 19 Sep 2017 13:14:46 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.152.18 with HTTP; Tue, 19 Sep 2017 13:14:45 -0700 (PDT) In-Reply-To: <20170919195529.29176-1-supreeth.venkatesh@arm.com> References: <20170919195529.29176-1-supreeth.venkatesh@arm.com> From: Ard Biesheuvel Date: Tue, 19 Sep 2017 13:14:45 -0700 Message-ID: To: Supreeth Venkatesh Cc: "edk2-devel@lists.01.org" , Leif Lindholm 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: Tue, 19 Sep 2017 20:11:42 -0000 Content-Type: text/plain; charset="UTF-8" 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. > 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.
> +# 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