From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-x230.google.com (mail-wr0-x230.google.com [IPv6:2a00:1450:400c:c0c::230]) (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 2ED288222D for ; Wed, 22 Feb 2017 05:11:54 -0800 (PST) Received: by mail-wr0-x230.google.com with SMTP id 97so1631268wrb.0 for ; Wed, 22 Feb 2017 05:11:54 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=yL3ru3AjKqT7yRlL2qwXJpMheUAjOrZ6ta/W2oChVHA=; b=V+73yBYvuphq86Qhm82E8IR+InDwhEtC1gQky5ZFHyNXpZEZ/9TtSRtB4pp/4uTEd3 HmyfHOE95rLhwCJ4qugc2O5QaS8BR+OF5xsy8Fnctjttk14MNwnXHEJDp9dERhNz29kR S5jq2b6s6lkrl28ZdtQGbQmDsXKl4ykVqemEU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=yL3ru3AjKqT7yRlL2qwXJpMheUAjOrZ6ta/W2oChVHA=; b=cbp0ClS695ZoBMD4wSAM/t7PlEJh6KlX7gsu1ZoC5QGtT09nUK0roWAl9H+vWt0NlG OYO+R99KdS0qbcFn7xD/q1YcB8FQ2DyKY1G20SAYCuOSvgGjcv/q0/ZRUFOwsT8mLcwR q0lDUVzcoBthkHtBiqzkcb21RkcnC+uZunnUPSCzRo1w4Co6h8H1F8FkEYJ+a/35tKnU +/W7by5avAlayts4rjUl3gRHwuVo2jVm3l3JS85HalWji2gFb6NhYO2lHZUbh4FVoYFP 8tVz8k1OcgMNNMvBPlpneJ8e1KtZiahcDdSjNvEtYN+kUPyzo2CyKmRbDowzPZUY0+9B uv3w== X-Gm-Message-State: AMke39lKJM6grmjyJuwyYmfR0nvN/AzTbOw664TgIEVSH0Triw3lZ+pmdvfGII0EYjgPxvuR X-Received: by 10.223.139.220 with SMTP id w28mr25257158wra.172.1487769112641; Wed, 22 Feb 2017 05:11:52 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id t30sm1696549wra.52.2017.02.22.05.11.51 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 22 Feb 2017 05:11:52 -0800 (PST) Date: Wed, 22 Feb 2017 13:11:50 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: "edk2-devel@lists.01.org" , Heyi Guo Message-ID: <20170222131150.GU16034@bivouac.eciton.net> References: <1487756301-15646-1-git-send-email-ard.biesheuvel@linaro.org> <1487756301-15646-2-git-send-email-ard.biesheuvel@linaro.org> <20170222120628.GR16034@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH 1/4] ArmPlatformPkg/ArmPlatformStackLib: use callee preserved registers X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Feb 2017 13:11:54 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Wed, Feb 22, 2017 at 12:52:10PM +0000, Ard Biesheuvel wrote: > On 22 February 2017 at 12:06, Leif Lindholm wrote: > > On Wed, Feb 22, 2017 at 09:38:18AM +0000, Ard Biesheuvel wrote: > >> The entry code in ArmPlatformStackSet () is a 1:1 transliteration of the > >> ARM version, which uses the callee preserved registers r3 - r7 (*) to > >> preserve the function arguments and the link register across a call to > >> ArmPlatformIsPrimaryCore (). > >> > >> However, x4 - x7 are not callee preserved on AARCH64, and so we should use > >> registers >= x18 instead. > > > > Err ... >= x19 really, no? > > I mean, I guess it technically does not matter here, but the PCS > > explicitly recommends against manual use of x18. > > > > It recommends against it on portable code, because it is up to the > OS/execution environment to define the semantics of x18. Since the > UEFI spec does not mention x18 at all (except for the definition of > EFI_SYSTEM_CONTEXT_AARCH64), it indeed makes sense to avoid it. > > >> While we're at it, drop an unnecessary preserve > >> of the link register, and simplify/deobfuscate the calculation of the > >> secondary stack position. > >> > >> (*) On ARM, r3 is not callee preserved either, but this should be addressed > >> in a separate patch. > > > > Agreed, but doesn't need to go into the final commit message? > > > > Nope, but stating that r3 - r7 are callee save on ARM without > mentioning that this is a mistake feels wrong as well. Fair enough. You can drop that one and use the reviewed-by. > >> > >> Contributed-under: TianoCore Contribution Agreement 1.0 > >> Signed-off-by: Ard Biesheuvel > >> --- > >> ArmPlatformPkg/Library/ArmPlatformStackLib/AArch64/ArmPlatformStackLib.S | 43 +++++++++----------- > >> 1 file changed, 19 insertions(+), 24 deletions(-) > >> > >> diff --git a/ArmPlatformPkg/Library/ArmPlatformStackLib/AArch64/ArmPlatformStackLib.S b/ArmPlatformPkg/Library/ArmPlatformStackLib/AArch64/ArmPlatformStackLib.S > >> index 65d7d6c6d686..e219d53cb71d 100644 > >> --- a/ArmPlatformPkg/Library/ArmPlatformStackLib/AArch64/ArmPlatformStackLib.S > >> +++ b/ArmPlatformPkg/Library/ArmPlatformStackLib/AArch64/ArmPlatformStackLib.S > >> @@ -22,13 +22,13 @@ > >> // ); > >> ASM_FUNC(ArmPlatformStackSet) > >> // Save parameters > >> - mov x6, x3 > >> - mov x5, x2 > >> - mov x4, x1 > >> - mov x3, x0 > >> + mov x26, x3 > >> + mov x25, x2 > >> + mov x24, x1 > >> + mov x23, x0 > > > > Hah, I was going to bikeshed about why you weren't starting from x19, > > then realised you're just prepending the 2 and avoiding typos. > > I approve. > > > > If you fold in my comments on commit message: > > Reviewed-by: Leif Lindholm > > > >> > >> // Save the Link register > >> - mov x7, x30 > >> + mov x27, x30 > >> > >> // Identify Stack > >> mov x0, x1 > >> @@ -36,13 +36,13 @@ ASM_FUNC(ArmPlatformStackSet) > >> cmp x0, #1 > >> > >> // Restore parameters > >> - mov x0, x3 > >> - mov x1, x4 > >> - mov x2, x5 > >> - mov x3, x6 > >> + mov x0, x23 > >> + mov x1, x24 > >> + mov x2, x25 > >> + mov x3, x26 > >> > >> // Restore the Link register > >> - mov x30, x7 > >> + mov x30, x27 > >> > >> b.ne 0f > >> > >> @@ -57,10 +57,7 @@ ASM_FUNC(ArmPlatformStackSet) > >> // IN UINTN SecondaryStackSize > >> // ); > >> ASM_FUNC(ArmPlatformStackSetPrimary) > >> - // Save the Link register > >> - mov x4, x30 > >> - > >> - // Add stack of primary stack to StackBase > >> + // Add size of primary stack to StackBase > >> add x0, x0, x2 > >> > >> // Compute SecondaryCoresCount * SecondaryCoreStackSize > >> @@ -70,7 +67,7 @@ ASM_FUNC(ArmPlatformStackSetPrimary) > >> // Set Primary Stack ((StackBase + PrimaryStackSize) + (SecondaryCoresCount * SecondaryCoreStackSize)) > >> add sp, x0, x3 > >> > >> - br x4 > >> + ret > >> > >> //VOID > >> //ArmPlatformStackSetSecondary ( > >> @@ -81,30 +78,28 @@ ASM_FUNC(ArmPlatformStackSetPrimary) > >> // ); > >> ASM_FUNC(ArmPlatformStackSetSecondary) > >> // Save the Link register > >> - mov x4, x30 > >> + mov x24, x30 > >> mov sp, x0 > >> > >> // Get Core Position > >> mov x0, x1 > >> bl ASM_PFX(ArmPlatformGetCorePosition) > >> - mov x5, x0 > >> + mov x25, x0 > >> > >> // Get Primary Core Position > >> bl ASM_PFX(ArmPlatformGetPrimaryCoreMpId) > >> bl ASM_PFX(ArmPlatformGetCorePosition) > >> > >> // Get Secondary Core Position. We should get consecutive secondary stack number from 1...(CoreCount-1) > >> - cmp x5, x0 > >> - b.ls 1f > >> + cmp x25, x0 > >> + > >> // Decrement the position if after the primary core > >> - sub x5, x5, #1 > >> -1: > >> - add x5, x5, #1 > >> + cinc x25, x25, ls > >> > >> // Compute top of the secondary stack > >> - mul x3, x3, x5 > >> + mul x3, x3, x25 > >> > >> // Set stack > >> add sp, sp, x3 > >> > >> - br x4 > >> + ret x24 > >> -- > >> 2.7.4