Re: [PATCH] MdePkg/BaseLib: implement SpeculationBarrier() for ARM and AArch64

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Tue, 12 Feb 2019 at 01:26, Gao, Liming <liming.gao@intel.com> wrote:
>
> Ard:
>   I agree your comments not to add PCD until there is the real problem.
>

Pushed as 1a35dd723bbf..c0959b4426b2

Thanks all


> 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