[PATCH v2 0/2] StandaloneMM: Update permissions for Standalone MM drivers memory area

[PATCH v2 0/2] StandaloneMM: Update permissions for Standalone MM drivers memory area

[Sughosh Ganu]

Changes since v1:
A new patch has been added to reflect the library class added for
changing the MMU attributes in StandaloneMM image, based on review
comments from Ard Biesheuvel.


These patches needs to be applied on top of the following patch series
 - "ArmPkg related changes for StandaloneMM package".


Sughosh Ganu (2):
  StandaloneMM: Include the newly added library class for MMU functions
  StandaloneMM: Update permissions for Standalone MM drivers memory area

 .../StandaloneMmCoreEntryPoint.inf                 |   2 +-
 .../StandaloneMmPeCoffExtraActionLib.inf           |  18 +-
 .../StandaloneMmPeCoffExtraActionLib.c             | 222 +++++++++++++++++++++
 3 files changed, 234 insertions(+), 8 deletions(-)
 copy ArmPkg/Library/RvdPeCoffExtraActionLib/RvdPeCoffExtraActionLib.inf => StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf (72%)
 create mode 100644 StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.c

-- 
2.7.4

[PATCH v2 1/2] StandaloneMM: Include the newly added library class for MMU functions

[Sughosh Ganu]

The MMU functions needed for StandaloneMM image are now exported
through a separate library class. Make the corresponding change in the
core's entry point inf file so that it references the correct library
class for modifying the MMU attributes.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Sughosh Ganu <sughosh.ganu@arm.com>
---
 .../Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf   | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
index 66184c4a00f3..3222cd359f3e 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
@@ -45,7 +45,7 @@ [LibraryClasses]
   DebugLib
 
 [LibraryClasses.AARCH64]
-  ArmMmuLib
+  StandaloneMmMmuLib
   ArmSvcLib
 
 [Guids]
-- 
2.7.4

[PATCH v2 2/2] StandaloneMM: Update permissions for Standalone MM drivers memory area

[Sughosh Ganu]

The StandaloneMM image executes in S-EL0 on reference Arm platforms
and is deployed by the trusted firmware as BL32 image. Memory for the
Standalone MM drivers is marked as RW+XN initially, allowing the
drivers to be loaded into the memory. Once loaded, the memory
attributes need to be changed to RO+XN for rodata sections and RO+X
for code sections.

Achieve this through the extra action 'UpdatePeCoffPermissions' to
request the privileged firmware in EL3 to update the permissions.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Sughosh Ganu <sughosh.ganu@arm.com>
---
 .../StandaloneMmPeCoffExtraActionLib.inf           |  18 +-
 .../StandaloneMmPeCoffExtraActionLib.c             | 222 +++++++++++++++++++++
 2 files changed, 233 insertions(+), 7 deletions(-)
 copy ArmPkg/Library/RvdPeCoffExtraActionLib/RvdPeCoffExtraActionLib.inf => StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf (72%)
 create mode 100644 StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.c

diff --git a/ArmPkg/Library/RvdPeCoffExtraActionLib/RvdPeCoffExtraActionLib.inf b/StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf
similarity index 72%
copy from ArmPkg/Library/RvdPeCoffExtraActionLib/RvdPeCoffExtraActionLib.inf
copy to StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf
index 3be0237a3689..87734f69b75d 100644
--- a/ArmPkg/Library/RvdPeCoffExtraActionLib/RvdPeCoffExtraActionLib.inf
+++ b/StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf
@@ -16,9 +16,9 @@
 #**/
 
 [Defines]
-  INF_VERSION                    = 0x00010005
-  BASE_NAME                      = RvdUnixPeCoffExtraActionLib
-  FILE_GUID                      = 5EDEB7E7-EA55-4E92-8216-335AC98A3B11
+  INF_VERSION                    = 0x0001000A
+  BASE_NAME                      = StandaloneMmPeCoffExtraActionLib
+  FILE_GUID                      = 8B40543B-9588-48F8-840C-5A60E6DB1B03
   MODULE_TYPE                    = BASE
   VERSION_STRING                 = 1.0
   LIBRARY_CLASS                  = PeCoffExtraActionLib
@@ -30,12 +30,16 @@ [Defines]
 #
 
 [Sources.common]
-  RvdPeCoffExtraActionLib.c
+  StandaloneMmPeCoffExtraActionLib.c
 
 [Packages]
-  MdePkg/MdePkg.dec
   ArmPkg/ArmPkg.dec
+  MdePkg/MdePkg.dec
+  StandaloneMmPkg/StandaloneMmPkg.dec
+
+[FeaturePcd]
+  gStandaloneMmPkgTokenSpaceGuid.PcdStandaloneMmEnable
 
 [LibraryClasses]
-  DebugLib
-  SemihostLib
+  StandaloneMmMmuLib
+  PcdLib
diff --git a/StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.c b/StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.c
new file mode 100644
index 000000000000..1c9fec201916
--- /dev/null
+++ b/StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.c
@@ -0,0 +1,222 @@
+/**@file
+
+Copyright (c) 2006 - 2009, Intel Corporation. All rights reserved.<BR>
+Portions copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
+Portions copyright (c) 2011 - 2018, ARM Ltd. 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.
+
+**/
+
+#include <PiDxe.h>
+
+#include <Library/ArmMmuLib.h>
+#include <Library/BaseLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/PcdLib.h>
+#include <Library/PeCoffLib.h>
+#include <Library/PeCoffExtraActionLib.h>
+
+typedef RETURN_STATUS (*REGION_PERMISSION_UPDATE_FUNC) (
+  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
+  IN  UINT64                    Length
+  );
+
+STATIC
+RETURN_STATUS
+UpdatePeCoffPermissions (
+  IN  CONST PE_COFF_LOADER_IMAGE_CONTEXT      *ImageContext,
+  IN  REGION_PERMISSION_UPDATE_FUNC           NoExecUpdater,
+  IN  REGION_PERMISSION_UPDATE_FUNC           ReadOnlyUpdater
+  )
+{
+  RETURN_STATUS                         Status;
+  EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION   Hdr;
+  EFI_IMAGE_OPTIONAL_HEADER_UNION       HdrData;
+  UINTN                                 Size;
+  UINTN                                 ReadSize;
+  UINT32                                SectionHeaderOffset;
+  UINTN                                 NumberOfSections;
+  UINTN                                 Index;
+  EFI_IMAGE_SECTION_HEADER              SectionHeader;
+  PE_COFF_LOADER_IMAGE_CONTEXT          TmpContext;
+  EFI_PHYSICAL_ADDRESS                  Base;
+
+  //
+  // We need to copy ImageContext since PeCoffLoaderGetImageInfo ()
+  // will mangle the ImageAddress field
+  //
+  CopyMem (&TmpContext, ImageContext, sizeof (TmpContext));
+
+  if (TmpContext.PeCoffHeaderOffset == 0) {
+    Status = PeCoffLoaderGetImageInfo (&TmpContext);
+    if (RETURN_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR,
+        "%a: PeCoffLoaderGetImageInfo () failed (Status = %r)\n",
+        __FUNCTION__, Status));
+      return Status;
+    }
+  }
+
+  if (TmpContext.IsTeImage &&
+      TmpContext.ImageAddress == ImageContext->ImageAddress) {
+    DEBUG ((DEBUG_INFO, "%a: ignoring XIP TE image at 0x%lx\n", __FUNCTION__,
+      ImageContext->ImageAddress));
+    return RETURN_SUCCESS;
+  }
+
+  if (TmpContext.SectionAlignment < EFI_PAGE_SIZE) {
+    //
+    // The sections need to be at least 4 KB aligned, since that is the
+    // granularity at which we can tighten permissions. So just clear the
+    // noexec permissions on the entire region.
+    //
+    if (!TmpContext.IsTeImage) {
+      DEBUG ((DEBUG_WARN,
+        "%a: non-TE Image at 0x%lx has SectionAlignment < 4 KB (%lu)\n",
+        __FUNCTION__, ImageContext->ImageAddress, TmpContext.SectionAlignment));
+    }
+    Base = ImageContext->ImageAddress & ~(EFI_PAGE_SIZE - 1);
+    Size = ImageContext->ImageAddress - Base + ImageContext->ImageSize;
+    return NoExecUpdater (Base, ALIGN_VALUE (Size, EFI_PAGE_SIZE));
+  }
+
+  //
+  // Read the PE/COFF Header. For PE32 (32-bit) this will read in too much
+  // data, but that should not hurt anything. Hdr.Pe32->OptionalHeader.Magic
+  // determines if this is a PE32 or PE32+ image. The magic is in the same
+  // location in both images.
+  //
+  Hdr.Union = &HdrData;
+  Size = sizeof (EFI_IMAGE_OPTIONAL_HEADER_UNION);
+  ReadSize = Size;
+  Status = TmpContext.ImageRead (TmpContext.Handle,
+                         TmpContext.PeCoffHeaderOffset, &Size, Hdr.Pe32);
+  if (RETURN_ERROR (Status) || (Size != ReadSize)) {
+    DEBUG ((DEBUG_ERROR,
+      "%a: TmpContext.ImageRead () failed (Status = %r)\n",
+      __FUNCTION__, Status));
+    return Status;
+  }
+
+  ASSERT (Hdr.Pe32->Signature == EFI_IMAGE_NT_SIGNATURE);
+
+  SectionHeaderOffset = TmpContext.PeCoffHeaderOffset + sizeof (UINT32) +
+                        sizeof (EFI_IMAGE_FILE_HEADER);
+  NumberOfSections    = (UINTN)(Hdr.Pe32->FileHeader.NumberOfSections);
+
+  switch (Hdr.Pe32->OptionalHeader.Magic) {
+    case EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC:
+      SectionHeaderOffset += Hdr.Pe32->FileHeader.SizeOfOptionalHeader;
+      break;
+    case EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC:
+      SectionHeaderOffset += Hdr.Pe32Plus->FileHeader.SizeOfOptionalHeader;
+      break;
+    default:
+      ASSERT (FALSE);
+  }
+
+  //
+  // Iterate over the sections
+  //
+  for (Index = 0; Index < NumberOfSections; Index++) {
+    //
+    // Read section header from file
+    //
+    Size = sizeof (EFI_IMAGE_SECTION_HEADER);
+    ReadSize = Size;
+    Status = TmpContext.ImageRead (TmpContext.Handle, SectionHeaderOffset,
+                                   &Size, &SectionHeader);
+    if (RETURN_ERROR (Status) || (Size != ReadSize)) {
+      DEBUG ((DEBUG_ERROR,
+        "%a: TmpContext.ImageRead () failed (Status = %r)\n",
+        __FUNCTION__, Status));
+      return Status;
+    }
+
+    Base = TmpContext.ImageAddress + SectionHeader.VirtualAddress;
+
+    if ((SectionHeader.Characteristics & EFI_IMAGE_SCN_MEM_EXECUTE) == 0) {
+
+      if ((SectionHeader.Characteristics & EFI_IMAGE_SCN_MEM_WRITE) == 0 &&
+          TmpContext.ImageType != EFI_IMAGE_SUBSYSTEM_EFI_RUNTIME_DRIVER) {
+
+        DEBUG ((DEBUG_INFO,
+          "%a: Mapping section %d of image at 0x%lx with RO-XN permissions and size 0x%x\n",
+          __FUNCTION__, Index, Base, SectionHeader.Misc.VirtualSize));
+        ReadOnlyUpdater (Base, SectionHeader.Misc.VirtualSize);
+      } else {
+        DEBUG ((DEBUG_WARN,
+          "%a: Mapping section %d of image at 0x%lx with RW-XN permissions and size 0x%x\n",
+          __FUNCTION__, Index, Base, SectionHeader.Misc.VirtualSize));
+      }
+    } else {
+        DEBUG ((DEBUG_INFO,
+          "%a: Mapping section %d of image at 0x%lx with RO-XN permissions and size 0x%x\n",
+           __FUNCTION__, Index, Base, SectionHeader.Misc.VirtualSize));
+        ReadOnlyUpdater (Base, SectionHeader.Misc.VirtualSize);
+
+        DEBUG ((DEBUG_INFO,
+          "%a: Mapping section %d of image at 0x%lx with RO-X permissions and size 0x%x\n",
+          __FUNCTION__, Index, Base, SectionHeader.Misc.VirtualSize));
+        NoExecUpdater (Base, SectionHeader.Misc.VirtualSize);
+    }
+
+    SectionHeaderOffset += sizeof (EFI_IMAGE_SECTION_HEADER);
+  }
+  return RETURN_SUCCESS;
+}
+
+/**
+  Performs additional actions after a PE/COFF image has been loaded and relocated.
+
+  If ImageContext is NULL, then ASSERT().
+
+  @param  ImageContext  Pointer to the image context structure that describes the
+                        PE/COFF image that has already been loaded and relocated.
+
+**/
+VOID
+EFIAPI
+PeCoffLoaderRelocateImageExtraAction (
+  IN OUT PE_COFF_LOADER_IMAGE_CONTEXT  *ImageContext
+  )
+{
+  UpdatePeCoffPermissions (
+    ImageContext,
+    ArmClearMemoryRegionNoExec,
+    ArmSetMemoryRegionReadOnly
+    );
+}
+
+
+
+/**
+  Performs additional actions just before a PE/COFF image is unloaded.  Any resources
+  that were allocated by PeCoffLoaderRelocateImageExtraAction() must be freed.
+
+  If ImageContext is NULL, then ASSERT().
+
+  @param  ImageContext  Pointer to the image context structure that describes the
+                        PE/COFF image that is being unloaded.
+
+**/
+VOID
+EFIAPI
+PeCoffLoaderUnloadImageExtraAction (
+  IN OUT PE_COFF_LOADER_IMAGE_CONTEXT  *ImageContext
+  )
+{
+  UpdatePeCoffPermissions (
+    ImageContext,
+    ArmSetMemoryRegionNoExec,
+    ArmClearMemoryRegionReadOnly
+    );
+}
-- 
2.7.4

Re: [PATCH v2 0/2] StandaloneMM: Update permissions for Standalone MM drivers memory area

[Achin Gupta]

Hi Sughosh,

+Jiewen

I took the patches for a spin and it looks like the FVP port is broken. Some
reasons are:

1. The build breaks due to a reference to ArmMmuLib in StandaloneMmPkg.dsc
2. There is a broken dependency on PL011UartClockLib in StandaloneMmPkg.dsc
3. GCC flags to enforce strict alignment and no fp are required in
   StandaloneMmPkg.dsc to avoid a runtime fault
4. There is a data structre in StandaloneMmCoreEntryPoint.c that needs to be
   memzeroed due to the alignment checks

Even after these fixes, I am unable to boot the MM SP. The SP boots with the
previous revision of your patches and the above fixes. Something has broken
between the two. I am suspecting the MMU library for S-EL0.

Lets sort this out first. Apart from this, could you move this library into
an AArch64 directory as is the case for other Arm specific libraries
e.g. StandaloneMmCoreEntryPoint/AArch64

cheers,
Achin

On Tue, Nov 27, 2018 at 11:52:53AM +0530, Sughosh Ganu wrote:
> Changes since v1:
> A new patch has been added to reflect the library class added for
> changing the MMU attributes in StandaloneMM image, based on review
> comments from Ard Biesheuvel.
> 
> 
> These patches needs to be applied on top of the following patch series
>  - "ArmPkg related changes for StandaloneMM package".
> 
> 
> Sughosh Ganu (2):
>   StandaloneMM: Include the newly added library class for MMU functions
>   StandaloneMM: Update permissions for Standalone MM drivers memory area
> 
>  .../StandaloneMmCoreEntryPoint.inf                 |   2 +-
>  .../StandaloneMmPeCoffExtraActionLib.inf           |  18 +-
>  .../StandaloneMmPeCoffExtraActionLib.c             | 222 +++++++++++++++++++++
>  3 files changed, 234 insertions(+), 8 deletions(-)
>  copy ArmPkg/Library/RvdPeCoffExtraActionLib/RvdPeCoffExtraActionLib.inf => StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf (72%)
>  create mode 100644 StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.c
> 
> -- 
> 2.7.4
> 

Re: [PATCH v2 0/2] StandaloneMM: Update permissions for Standalone MM drivers memory area

[Sughosh Ganu]

hi Achin,

On Sat Dec 01, 2018 at 05:08:50AM +0530, Achin Gupta wrote:
> Hi Sughosh,
> 
> +Jiewen
> 
> I took the patches for a spin and it looks like the FVP port is broken. Some
> reasons are:
> 
> 1. The build breaks due to a reference to ArmMmuLib in StandaloneMmPkg.dsc
> 2. There is a broken dependency on PL011UartClockLib in StandaloneMmPkg.dsc
> 3. GCC flags to enforce strict alignment and no fp are required in
>    StandaloneMmPkg.dsc to avoid a runtime fault
> 4. There is a data structre in StandaloneMmCoreEntryPoint.c that needs to be
>    memzeroed due to the alignment checks
> 
> Even after these fixes, I am unable to boot the MM SP. The SP boots with the
> previous revision of your patches and the above fixes. Something has broken
> between the two. I am suspecting the MMU library for S-EL0.

I had tested the patches which i had sent out for ArmPkg changes with
the error handling and error reporting feature on sgi575 before
posting the patches. In addition to the changes that you mention, i
was required to make a couple of more changes in the StandadloneMm
description file.

1) StandaloneMmMmuLib|ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf

This was changed from ArmMmuLib which was used earlier.

2) PeCoffExtraActionLib|StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf

I had changed this to reflect the change made in the patch
StandaloneMM: Update permissions for Standalone MM drivers memory area

Can you please confirm that you tested with these two additional
changes made. Meanwhile, I will incorporate your review comments which
you make below. Thanks.

-sughosh

> 
> Lets sort this out first. Apart from this, could you move this library into
> an AArch64 directory as is the case for other Arm specific libraries
> e.g. StandaloneMmCoreEntryPoint/AArch64
> 
> cheers,
> Achin
> 
> On Tue, Nov 27, 2018 at 11:52:53AM +0530, Sughosh Ganu wrote:
> > Changes since v1:
> > A new patch has been added to reflect the library class added for
> > changing the MMU attributes in StandaloneMM image, based on review
> > comments from Ard Biesheuvel.
> > 
> > 
> > These patches needs to be applied on top of the following patch series
> >  - "ArmPkg related changes for StandaloneMM package".
> > 
> > 
> > Sughosh Ganu (2):
> >   StandaloneMM: Include the newly added library class for MMU functions
> >   StandaloneMM: Update permissions for Standalone MM drivers memory area
> > 
> >  .../StandaloneMmCoreEntryPoint.inf                 |   2 +-
> >  .../StandaloneMmPeCoffExtraActionLib.inf           |  18 +-
> >  .../StandaloneMmPeCoffExtraActionLib.c             | 222 +++++++++++++++++++++
> >  3 files changed, 234 insertions(+), 8 deletions(-)
> >  copy ArmPkg/Library/RvdPeCoffExtraActionLib/RvdPeCoffExtraActionLib.inf => StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf (72%)
> >  create mode 100644 StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.c
> > 
> > -- 
> > 2.7.4
> > 

-- 
-sughosh

Re: [PATCH v2 0/2] StandaloneMM: Update permissions for Standalone MM drivers memory area

[Achin Gupta]

Hi Sughosh,

On Sat, Dec 01, 2018 at 09:56:52AM +0530, Sughosh Ganu wrote:
> hi Achin,
> 
> On Sat Dec 01, 2018 at 05:08:50AM +0530, Achin Gupta wrote:
> > Hi Sughosh,
> > 
> > +Jiewen
> > 
> > I took the patches for a spin and it looks like the FVP port is broken. Some
> > reasons are:
> > 
> > 1. The build breaks due to a reference to ArmMmuLib in StandaloneMmPkg.dsc
> > 2. There is a broken dependency on PL011UartClockLib in StandaloneMmPkg.dsc
> > 3. GCC flags to enforce strict alignment and no fp are required in
> >    StandaloneMmPkg.dsc to avoid a runtime fault
> > 4. There is a data structre in StandaloneMmCoreEntryPoint.c that needs to be
> >    memzeroed due to the alignment checks
> > 
> > Even after these fixes, I am unable to boot the MM SP. The SP boots with the
> > previous revision of your patches and the above fixes. Something has broken
> > between the two. I am suspecting the MMU library for S-EL0.
> 
> I had tested the patches which i had sent out for ArmPkg changes with
> the error handling and error reporting feature on sgi575 before
> posting the patches. In addition to the changes that you mention, i
> was required to make a couple of more changes in the StandadloneMm
> description file.
> 
> 1) StandaloneMmMmuLib|ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
> 
> This was changed from ArmMmuLib which was used earlier.

yes! this was the change i was referring to in 1.

> 
> 2) PeCoffExtraActionLib|StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf
> 
> I had changed this to reflect the change made in the patch
> StandaloneMM: Update permissions for Standalone MM drivers memory area

Doh! of course it does not work without this change. Thanks! Could you roll it
in your patchstack?

> 
> Can you please confirm that you tested with these two additional
> changes made. Meanwhile, I will incorporate your review comments which
> you make below. Thanks.

I was able to initialise the SP on the FVP and the MM communication driver
initialised correctly. I did not test the MM SP further. I have pushed my
changes on top of your patches here [1]. Could you please check they work for
you as well and include the relevant changes in the next rev of your patchstack?

cheers,
Achin

[1] https://github.com/achingupta/edk2/commits/ag/stmm_perm_v2

> 
> -sughosh
> 
> > 
> > Lets sort this out first. Apart from this, could you move this library into
> > an AArch64 directory as is the case for other Arm specific libraries
> > e.g. StandaloneMmCoreEntryPoint/AArch64
> > 
> > cheers,
> > Achin
> > 
> > On Tue, Nov 27, 2018 at 11:52:53AM +0530, Sughosh Ganu wrote:
> > > Changes since v1:
> > > A new patch has been added to reflect the library class added for
> > > changing the MMU attributes in StandaloneMM image, based on review
> > > comments from Ard Biesheuvel.
> > > 
> > > 
> > > These patches needs to be applied on top of the following patch series
> > >  - "ArmPkg related changes for StandaloneMM package".
> > > 
> > > 
> > > Sughosh Ganu (2):
> > >   StandaloneMM: Include the newly added library class for MMU functions
> > >   StandaloneMM: Update permissions for Standalone MM drivers memory area
> > > 
> > >  .../StandaloneMmCoreEntryPoint.inf                 |   2 +-
> > >  .../StandaloneMmPeCoffExtraActionLib.inf           |  18 +-
> > >  .../StandaloneMmPeCoffExtraActionLib.c             | 222 +++++++++++++++++++++
> > >  3 files changed, 234 insertions(+), 8 deletions(-)
> > >  copy ArmPkg/Library/RvdPeCoffExtraActionLib/RvdPeCoffExtraActionLib.inf => StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf (72%)
> > >  create mode 100644 StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.c
> > > 
> > > -- 
> > > 2.7.4
> > > 
> 
> -- 
> -sughosh

Re: [PATCH v2 0/2] StandaloneMM: Update permissions for Standalone MM drivers memory area

[Sughosh Ganu]

hi Achin,

On Sun Dec 02, 2018 at 12:15:47AM +0530, Achin Gupta wrote:
> Hi Sughosh,
> 
> On Sat, Dec 01, 2018 at 09:56:52AM +0530, Sughosh Ganu wrote:
> > hi Achin,
> > 
> > On Sat Dec 01, 2018 at 05:08:50AM +0530, Achin Gupta wrote:
> > > Hi Sughosh,
> > > 
> > > +Jiewen
> > > 
> > > I took the patches for a spin and it looks like the FVP port is broken. Some
> > > reasons are:
> > > 
> > > 1. The build breaks due to a reference to ArmMmuLib in StandaloneMmPkg.dsc
> > > 2. There is a broken dependency on PL011UartClockLib in StandaloneMmPkg.dsc
> > > 3. GCC flags to enforce strict alignment and no fp are required in
> > >    StandaloneMmPkg.dsc to avoid a runtime fault
> > > 4. There is a data structre in StandaloneMmCoreEntryPoint.c that needs to be
> > >    memzeroed due to the alignment checks
> > > 
> > > Even after these fixes, I am unable to boot the MM SP. The SP boots with the
> > > previous revision of your patches and the above fixes. Something has broken
> > > between the two. I am suspecting the MMU library for S-EL0.
> > 
> > I had tested the patches which i had sent out for ArmPkg changes with
> > the error handling and error reporting feature on sgi575 before
> > posting the patches. In addition to the changes that you mention, i
> > was required to make a couple of more changes in the StandadloneMm
> > description file.
> > 
> > 1) StandaloneMmMmuLib|ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
> > 
> > This was changed from ArmMmuLib which was used earlier.
> 
> yes! this was the change i was referring to in 1.
> 
> > 
> > 2) PeCoffExtraActionLib|StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf
> > 
> > I had changed this to reflect the change made in the patch
> > StandaloneMM: Update permissions for Standalone MM drivers memory area
> 
> Doh! of course it does not work without this change. Thanks! Could you roll it
> in your patchstack?
> 
> > 
> > Can you please confirm that you tested with these two additional
> > changes made. Meanwhile, I will incorporate your review comments which
> > you make below. Thanks.
> 
> I was able to initialise the SP on the FVP and the MM communication driver
> initialised correctly. I did not test the MM SP further. I have pushed my
> changes on top of your patches here [1]. Could you please check they work for
> you as well and include the relevant changes in the next rev of your patchstack?

Sure. I will post these fixes as well. I will post it as a separate
series, since the contents are pretty different from the other two
patches. Thanks.

-sughosh


> 
> cheers,
> Achin
> 
> [1] https://github.com/achingupta/edk2/commits/ag/stmm_perm_v2
> 
> > 
> > -sughosh
> > 
> > > 
> > > Lets sort this out first. Apart from this, could you move this library into
> > > an AArch64 directory as is the case for other Arm specific libraries
> > > e.g. StandaloneMmCoreEntryPoint/AArch64
> > > 
> > > cheers,
> > > Achin
> > > 
> > > On Tue, Nov 27, 2018 at 11:52:53AM +0530, Sughosh Ganu wrote:
> > > > Changes since v1:
> > > > A new patch has been added to reflect the library class added for
> > > > changing the MMU attributes in StandaloneMM image, based on review
> > > > comments from Ard Biesheuvel.
> > > > 
> > > > 
> > > > These patches needs to be applied on top of the following patch series
> > > >  - "ArmPkg related changes for StandaloneMM package".
> > > > 
> > > > 
> > > > Sughosh Ganu (2):
> > > >   StandaloneMM: Include the newly added library class for MMU functions
> > > >   StandaloneMM: Update permissions for Standalone MM drivers memory area
> > > > 
> > > >  .../StandaloneMmCoreEntryPoint.inf                 |   2 +-
> > > >  .../StandaloneMmPeCoffExtraActionLib.inf           |  18 +-
> > > >  .../StandaloneMmPeCoffExtraActionLib.c             | 222 +++++++++++++++++++++
> > > >  3 files changed, 234 insertions(+), 8 deletions(-)
> > > >  copy ArmPkg/Library/RvdPeCoffExtraActionLib/RvdPeCoffExtraActionLib.inf => StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf (72%)
> > > >  create mode 100644 StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.c
> > > > 
> > > > -- 
> > > > 2.7.4
> > > > 
> > 
> > -- 
> > -sughosh

-- 
-sughosh