Re: [PATCH v4 5/5] ArmPkg/ArmMmuLib: Add MMU Library suitable for use in S-EL0.

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

On Tue, 27 Nov 2018 at 09:36, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
>
> hi Ard,
>
> On Tue Nov 27, 2018 at 09:14:52AM +0100, Ard Biesheuvel wrote:
> > On Tue, 27 Nov 2018 at 07:20, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
> > >
> > > From: Achin Gupta <achin.gupta@arm.com>
> > >
> > > The Standalone MM environment runs in S-EL0 in AArch64 on ARM Standard
> > > Platforms. Privileged firmware e.g. ARM Trusted Firmware sets up its
> > > architectural context including the initial translation tables for the
> > > S-EL1/EL0 translation regime. The MM environment will still request ARM
> > > TF to change the memory attributes of memory regions during
> > > initialization.
> > >
> > > The Standalone MM image is a FV that encapsulates the MM foundation
> > > and drivers. These are PE-COFF images with data and text segments.
> > > To initialise the MM environment, Arm Trusted Firmware has to create
> > > translation tables with sane default attributes for the memory
> > > occupied by the FV. This library sends SVCs to ARM Trusted Firmware
> > > to request memory permissions change for data and text segments.
> > >
> > > This patch adds a simple MMU library suitable for execution in S-EL0 and
> > > requesting memory permissions change operations from Arm Trusted Firmware.
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@arm.com>
> > > ---
> > >  ArmPkg/ArmPkg.dec                                                                                                                                       |   1 +
> > >  ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf => ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf |  23 +--
> > >  ArmPkg/Include/Library/{ArmMmuLib.h => StandaloneMmMmuLib.h}                                                                                            |  38 +---
> > >  ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c                                                                                       | 185 ++++++++++++++++++++
> > >  4 files changed, 203 insertions(+), 44 deletions(-)
>
>
> <snip>
>
> > > +#endif /* __STANDALONEMM_MMU_LIB__ */
> > > diff --git a/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c
> > > new file mode 100644
> > > index 000000000000..d7d87b7d5d69
> > > --- /dev/null
> > > +++ b/ArmPkg/Library/StandaloneMmMmuLib/Aarch64/ArmMmuStandaloneMmLib.c
> > > @@ -0,0 +1,185 @@
> > > +/** @file
> > > +*  File managing the MMU for ARMv8 architecture in S-EL0
> > > +*
> > > +*  Copyright (c) 2017 - 2018, ARM Limited. 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.
> > > +*
> > > +**/
> > > +
> > > +#include <Uefi.h>
> > > +#include <Chipset/AArch64.h>
> >
> > Why do you need this include? If you can drop it, can you also make
> > the library generic (i.e., supporting ARM as well as AArch64) ?
> >
> > (apologies for not spotting this before)
>
> I can remove the header file if it is superfluous. But regarding your
> comment on making this code generic for Arm as well, i guess we
> can work on refactoring the code when/if we actually require to
> support this on Arm. I am not sure if we are going to have a use-case
> for StandaloneMM on Arm. Currently, we are only supporting it on
> Aarch64 based platforms. Is that fine. Please let me know. Thanks.
>

I'd strongly prefer this code to be generic if you are not using any
AArch64 specific facilities.

AFAICT, we'd simply need to move the file out of the AArch64 directory
and rename [Sources.AARCH64] to [Sources] in the .inf file if the
header dependency is indeed superfluous.