From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x230.google.com (mail-it0-x230.google.com [IPv6:2607:f8b0:4001:c0b::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 18A3D8222D for ; Wed, 22 Feb 2017 04:52:12 -0800 (PST) Received: by mail-it0-x230.google.com with SMTP id h10so130133814ith.1 for ; Wed, 22 Feb 2017 04:52:12 -0800 (PST) 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=FvzTj44+7mqfJvSmRyLlhxjoLT9wjAPCqGsjO8TZMIY=; b=ZkA1iO+3JFR2C+qMMTmbFC2Lx79l13VpefqwXn33rYbMsZ1vYHq4A+Tq+kEN5V2YTN Qp8O+lbO1w9Df6+UjxZlVQ4oyGUH2NxqCJDIvtQTu+XbiD3TCpvJmOzWwCDvCdKuG7oF 7H+56RLUPZVn+BfPZd2zbkyhGgYhiRoWj7DVs= 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=FvzTj44+7mqfJvSmRyLlhxjoLT9wjAPCqGsjO8TZMIY=; b=b7Tuo/Ogj+xsnXVs2XElinQdW0ysxzOec7EIP6uzhKgS8nmOBFe9PFxXxejUMcjb8m 0zXFclRr9zUPZV/Hfhy19pQZVlnBkEo6mY6quWoMLGJqClyi9tQTQHvvPIJV7VEEOgXg bfGH2tFTuOrGm5DvO5py5UD+Z1Sc/C/VSNVvwAuc8Jfyc6yVuGfWA6+uWkR4g/tLjMZf nzemjAgiqjXq1WeUAPNxpfBTrb9yTLGjyAraymfhvKb8AL4868Tofa3ee0KPUsJQvVUC 5zd2blfCS/9xwJMSRJxgUn7xCtlXZCTyWeSajT9CcmfkISfuY7Vj/OKo5CAaHrWEriY/ UR3A== X-Gm-Message-State: AMke39kl+CXYfX9Ka+kPEUflY5rkfshJkC6SDIjrW84Rk2lTXCshLG1u00jjuMU1u5QaYKgOP7qhuLn82v8OPzVH X-Received: by 10.36.207.212 with SMTP id y203mr1758126itf.63.1487767931288; Wed, 22 Feb 2017 04:52:11 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.198.134 with HTTP; Wed, 22 Feb 2017 04:52:10 -0800 (PST) In-Reply-To: <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> <20170222120628.GR16034@bivouac.eciton.net> From: Ard Biesheuvel Date: Wed, 22 Feb 2017 12:52:10 +0000 Message-ID: To: Leif Lindholm Cc: "edk2-devel@lists.01.org" , Heyi Guo 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:52:12 -0000 Content-Type: text/plain; charset=UTF-8 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. >> >> 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