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

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

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
> >