From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x233.google.com (mail-wm0-x233.google.com [IPv6:2a00:1450:400c:c09::233]) (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 3E4348222D for ; Wed, 22 Feb 2017 04:06:32 -0800 (PST) Received: by mail-wm0-x233.google.com with SMTP id v186so138446373wmd.0 for ; Wed, 22 Feb 2017 04:06:32 -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=0+wt7LsJS4mj2s+3az8FMzosBMNhxtBvOAyiwmP2vbw=; b=ji1MOlW3n84GlDMDg8igNC8OOau0zQ5cGib1jzP8NJz/PXwwDPBINUKJfByjhf3SFk wWxCQqF0teCO/ikM475bgT9l8uWgK7hJsJC6i1X/xbkHSP4hpHShDj6vnvCS03vxzeMq yT5k7FRc+fCSX6/GUr8tWiwv2F/siwLqvg5PA= 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=0+wt7LsJS4mj2s+3az8FMzosBMNhxtBvOAyiwmP2vbw=; b=ueyC9sBuA/g0k/00Pi8UXZzAm0ZyO/1N4SDaSo2KGhj6qE1kEDWMFBXwsz1vIjuZmY Q16NHpmr43BNib3TAEFUDv2XAL5zx5WfuVlHaCx8n2+8HHxnWR9d1m+AE+t4eZu8tnJy kj8vG92XHQpis8bwqF4G8hqtSCj6/4SG6f+BVGcDUzFYKqE/QF9Uxeglvd2EPePJLX9E cqv+II8RS8y0FAreV++3xf+iJIfv16izGhu5dbJm7WPcCR4Ds3cF+l0XFIEwQtPFbr2E 9OvkOcwVkQqSl0XW7SR3EtV1HIeFOAFpqEJ0lxErrpYmre3MMucW2Uc24Xv+4Foj8dfq DwfQ== X-Gm-Message-State: AMke39ka/FCVn5YI4Iu2T+H0+uH9xfxbehvdRJ7DkhRhcRtGAwxSqtjOcqOCVUV3+dkDddSM X-Received: by 10.28.136.13 with SMTP id k13mr2223047wmd.94.1487765190763; Wed, 22 Feb 2017 04:06:30 -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 96sm1515803wrb.14.2017.02.22.04.06.29 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 22 Feb 2017 04:06:30 -0800 (PST) Date: Wed, 22 Feb 2017 12:06:28 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: edk2-devel@lists.01.org, heyi.guo@linaro.org Message-ID: <20170222120628.GR16034@bivouac.eciton.net> References: <1487756301-15646-1-git-send-email-ard.biesheuvel@linaro.org> <1487756301-15646-2-git-send-email-ard.biesheuvel@linaro.org> MIME-Version: 1.0 In-Reply-To: <1487756301-15646-2-git-send-email-ard.biesheuvel@linaro.org> 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 12:06:32 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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. > 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? > > 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