public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Gao, Liming" <liming.gao@intel.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	Leif Lindholm <leif.lindholm@linaro.org>
Cc: "Kinney, Michael D" <michael.d.kinney@intel.com>,
	"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Subject: Re: [PATCH] MdePkg/BaseLib: implement SpeculationBarrier() for ARM and AArch64
Date: Tue, 12 Feb 2019 00:26:36 +0000	[thread overview]
Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14E3DB3BF@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <CAKv+Gu9mMSnWtL8SrS1KJ4jWms=ufMNugFmhoJUCvnHi4uYSLg@mail.gmail.com>

Ard:
  I agree your comments not to add PCD until there is the real problem. 

Thanks
Liming
>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard
>Biesheuvel
>Sent: Tuesday, February 12, 2019 1:32 AM
>To: Leif Lindholm <leif.lindholm@linaro.org>
>Cc: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-
>devel@lists.01.org; Gao, Liming <liming.gao@intel.com>
>Subject: Re: [edk2] [PATCH] MdePkg/BaseLib: implement SpeculationBarrier()
>for ARM and AArch64
>
>On Mon, 11 Feb 2019 at 15:41, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>>
>> On Wed, Feb 06, 2019 at 12:08:22AM +0000, Ard Biesheuvel wrote:
>> > Replace the dummy C implementation of SpeculationBarrier() with
>> > implementations consisting of the recommended DSB SY + ISB sequence,
>> > as recommended by ARM in the whitepaper "Cache Speculation Side-
>channels"
>> > version 2.4, dated October 2018.
>> >
>> > Contributed-under: TianoCore Contribution Agreement 1.1
>> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>> Patch looks fine.
>> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>>
>> Question: do we expect performance impact to be sufficient to
>> motivate a Pcd to be able to disable the barrier on unaffected
>> processors?
>>
>
>Currently, these are only used on some codepaths in the MM component
>of the variable store, which do not look like hot paths to me.
>
>In general, I think it should be fine to defer doing something like
>this until someone highlights it as an actual problem (and has the
>numbers to prove it)
>
>
>> > ---
>> >  MdePkg/Library/BaseLib/AArch64/SpeculationBarrier.S   | 39
>++++++++++++++++++++
>> >  MdePkg/Library/BaseLib/AArch64/SpeculationBarrier.asm | 38
>+++++++++++++++++++
>> >  MdePkg/Library/BaseLib/Arm/SpeculationBarrier.S       | 39
>++++++++++++++++++++
>> >  MdePkg/Library/BaseLib/Arm/SpeculationBarrier.asm     | 39
>++++++++++++++++++++
>> >  MdePkg/Library/BaseLib/Arm/SpeculationBarrier.c       | 30 ---------------
>> >  MdePkg/Library/BaseLib/BaseLib.inf                    |  7 +++-
>> >  6 files changed, 160 insertions(+), 32 deletions(-)
>> >
>> > diff --git a/MdePkg/Library/BaseLib/AArch64/SpeculationBarrier.S
>b/MdePkg/Library/BaseLib/AArch64/SpeculationBarrier.S
>> > new file mode 100644
>> > index 000000000000..500bdadca5d2
>> > --- /dev/null
>> > +++ b/MdePkg/Library/BaseLib/AArch64/SpeculationBarrier.S
>> > @@ -0,0 +1,39 @@
>> > +##------------------------------------------------------------------------------
>> > +#
>> > +# SpeculationBarrier() for AArch64
>> > +#
>> > +# Copyright (c) 2019, Linaro 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.
>> > +#
>> > +##------------------------------------------------------------------------------
>> > +
>> > +.text
>> > +.p2align 2
>> > +
>> > +GCC_ASM_EXPORT(SpeculationBarrier)
>> > +
>> > +
>> > +#/**
>> > +#  Uses as a barrier to stop speculative execution.
>> > +#
>> > +#  Ensures that no later instruction will execute speculatively, until all
>prior
>> > +#  instructions have completed.
>> > +#
>> > +#**/
>> > +#VOID
>> > +#EFIAPI
>> > +#SpeculationBarrier (
>> > +#  VOID
>> > +#  );
>> > +#
>> > +ASM_PFX(SpeculationBarrier):
>> > +    dsb  sy
>> > +    isb
>> > +    ret
>> > diff --git a/MdePkg/Library/BaseLib/AArch64/SpeculationBarrier.asm
>b/MdePkg/Library/BaseLib/AArch64/SpeculationBarrier.asm
>> > new file mode 100644
>> > index 000000000000..0c4b915b7798
>> > --- /dev/null
>> > +++ b/MdePkg/Library/BaseLib/AArch64/SpeculationBarrier.asm
>> > @@ -0,0 +1,38 @@
>> > +;------------------------------------------------------------------------------
>> > +;
>> > +; SpeculationBarrier() for AArch64
>> > +;
>> > +; Copyright (c) 2019, Linaro 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.
>> > +;
>> > +;------------------------------------------------------------------------------
>> > +
>> > +  EXPORT SpeculationBarrier
>> > +  AREA BaseLib_LowLevel, CODE, READONLY
>> > +
>> > +;/**
>> > +;  Uses as a barrier to stop speculative execution.
>> > +;
>> > +;  Ensures that no later instruction will execute speculatively, until all prior
>> > +;  instructions have completed.
>> > +;
>> > +;**/
>> > +;VOID
>> > +;EFIAPI
>> > +;SpeculationBarrier (
>> > +;  VOID
>> > +;  );
>> > +;
>> > +SpeculationBarrier
>> > +    dsb   sy
>> > +    isb
>> > +    ret
>> > +
>> > +  END
>> > diff --git a/MdePkg/Library/BaseLib/Arm/SpeculationBarrier.S
>b/MdePkg/Library/BaseLib/Arm/SpeculationBarrier.S
>> > new file mode 100644
>> > index 000000000000..7857558aba17
>> > --- /dev/null
>> > +++ b/MdePkg/Library/BaseLib/Arm/SpeculationBarrier.S
>> > @@ -0,0 +1,39 @@
>> > +##------------------------------------------------------------------------------
>> > +#
>> > +# SpeculationBarrier() for AArch64
>> > +#
>> > +# Copyright (c) 2019, Linaro 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.
>> > +#
>> > +##------------------------------------------------------------------------------
>> > +
>> > +.text
>> > +.p2align 2
>> > +
>> > +GCC_ASM_EXPORT(SpeculationBarrier)
>> > +
>> > +
>> > +#/**
>> > +#  Uses as a barrier to stop speculative execution.
>> > +#
>> > +#  Ensures that no later instruction will execute speculatively, until all
>prior
>> > +#  instructions have completed.
>> > +#
>> > +#**/
>> > +#VOID
>> > +#EFIAPI
>> > +#SpeculationBarrier (
>> > +#  VOID
>> > +#  );
>> > +#
>> > +ASM_PFX(SpeculationBarrier):
>> > +    dsb
>> > +    isb
>> > +    bx   lr
>> > diff --git a/MdePkg/Library/BaseLib/Arm/SpeculationBarrier.asm
>b/MdePkg/Library/BaseLib/Arm/SpeculationBarrier.asm
>> > new file mode 100644
>> > index 000000000000..425cd3de9e22
>> > --- /dev/null
>> > +++ b/MdePkg/Library/BaseLib/Arm/SpeculationBarrier.asm
>> > @@ -0,0 +1,39 @@
>> > +;------------------------------------------------------------------------------
>> > +;
>> > +; SpeculationBarrier() for AArch64
>> > +;
>> > +; Copyright (c) 2019, Linaro 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.
>> > +;
>> > +;------------------------------------------------------------------------------
>> > +
>> > +    EXPORT SpeculationBarrier
>> > +
>> > +    AREA MemoryBarriers, CODE, READONLY
>> > +
>> > +;/**
>> > +;  Uses as a barrier to stop speculative execution.
>> > +;
>> > +;  Ensures that no later instruction will execute speculatively, until all prior
>> > +;  instructions have completed.
>> > +;
>> > +;**/
>> > +;VOID
>> > +;EFIAPI
>> > +;SpeculationBarrier (
>> > +;  VOID
>> > +;  );
>> > +;
>> > +SpeculationBarrier
>> > +    dsb
>> > +    isb
>> > +    bx    lr
>> > +
>> > +  END
>> > diff --git a/MdePkg/Library/BaseLib/Arm/SpeculationBarrier.c
>b/MdePkg/Library/BaseLib/Arm/SpeculationBarrier.c
>> > deleted file mode 100644
>> > index 8a6165a10227..000000000000
>> > --- a/MdePkg/Library/BaseLib/Arm/SpeculationBarrier.c
>> > +++ /dev/null
>> > @@ -1,30 +0,0 @@
>> > -/** @file
>> > -  SpeculationBarrier() function for ARM.
>> > -
>> > -  Copyright (C) 2018, Intel Corporation. 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.
>> > -
>> > -**/
>> > -
>> > -
>> > -/**
>> > -  Uses as a barrier to stop speculative execution.
>> > -
>> > -  Ensures that no later instruction will execute speculatively, until all prior
>> > -  instructions have completed.
>> > -
>> > -**/
>> > -VOID
>> > -EFIAPI
>> > -SpeculationBarrier (
>> > -  VOID
>> > -  )
>> > -{
>> > -}
>> > diff --git a/MdePkg/Library/BaseLib/BaseLib.inf
>b/MdePkg/Library/BaseLib/BaseLib.inf
>> > index d195c5417b2e..f25a067a2335 100644
>> > --- a/MdePkg/Library/BaseLib/BaseLib.inf
>> > +++ b/MdePkg/Library/BaseLib/BaseLib.inf
>> > @@ -552,7 +552,6 @@
>> >  [Sources.ARM]
>> >    Arm/InternalSwitchStack.c
>> >    Arm/Unaligned.c
>> > -  Arm/SpeculationBarrier.c
>> >    Math64.c                   | RVCT
>> >    Math64.c                   | MSFT
>> >
>> > @@ -564,6 +563,7 @@
>> >    Arm/CpuPause.asm           | RVCT
>> >    Arm/CpuBreakpoint.asm      | RVCT
>> >    Arm/MemoryFence.asm        | RVCT
>> > +  Arm/SpeculationBarrier.S   | RVCT
>> >
>> >    Arm/SwitchStack.asm        | MSFT
>> >    Arm/SetJumpLongJump.asm    | MSFT
>> > @@ -573,6 +573,7 @@
>> >    Arm/CpuPause.asm           | MSFT
>> >    Arm/CpuBreakpoint.asm      | MSFT
>> >    Arm/MemoryFence.asm        | MSFT
>> > +  Arm/SpeculationBarrier.asm | MSFT
>> >
>> >    Arm/Math64.S                  | GCC
>> >    Arm/SwitchStack.S             | GCC
>> > @@ -582,11 +583,11 @@
>> >    Arm/SetJumpLongJump.S         | GCC
>> >    Arm/CpuBreakpoint.S           | GCC
>> >    Arm/MemoryFence.S             | GCC
>> > +  Arm/SpeculationBarrier.S      | GCC
>> >
>> >  [Sources.AARCH64]
>> >    Arm/InternalSwitchStack.c
>> >    Arm/Unaligned.c
>> > -  Arm/SpeculationBarrier.c
>> >    Math64.c
>> >
>> >    AArch64/MemoryFence.S             | GCC
>> > @@ -596,6 +597,7 @@
>> >    AArch64/GetInterruptsState.S      | GCC
>> >    AArch64/SetJumpLongJump.S         | GCC
>> >    AArch64/CpuBreakpoint.S           | GCC
>> > +  AArch64/SpeculationBarrier.S      | GCC
>> >
>> >    AArch64/MemoryFence.asm           | MSFT
>> >    AArch64/SwitchStack.asm           | MSFT
>> > @@ -604,6 +606,7 @@
>> >    AArch64/GetInterruptsState.asm    | MSFT
>> >    AArch64/SetJumpLongJump.asm       | MSFT
>> >    AArch64/CpuBreakpoint.asm         | MSFT
>> > +  AArch64/SpeculationBarrier.asm    | MSFT
>> >
>> >  [Packages]
>> >    MdePkg/MdePkg.dec
>> > --
>> > 2.17.1
>> >
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel


  reply	other threads:[~2019-02-12  0:26 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-06  0:08 [PATCH] MdePkg/BaseLib: implement SpeculationBarrier() for ARM and AArch64 Ard Biesheuvel
2019-02-11 14:27 ` Gao, Liming
2019-02-11 14:41 ` Leif Lindholm
2019-02-11 17:32   ` Ard Biesheuvel
2019-02-12  0:26     ` Gao, Liming [this message]
2019-02-12 21:58       ` Ard Biesheuvel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4A89E2EF3DFEDB4C8BFDE51014F606A14E3DB3BF@SHSMSX104.ccr.corp.intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox