Re: [edk2-devel] [edk2-platforms][PATCH V1 09/20] StandaloneMmPkg: parse SP manifest and populate new boot information

["Achin Gupta" <achin.gupta@arm.com>]

Hi Chris,

Prior to FF-A, an impdef structure populated by TF-A was passed to a
SPM_MM based StMM SP. The data structures used for this are
EFI_SECURE_PARTITION_BOOT_INFO and EFI_SECURE_PARTITION_CPU_INFO.

With FF-A, we have reduced the amount of information that is passed
e.g. the per-cpu information is not passed. The remaining information
has been generalized so that it is not specific to StMM SPs.

This information is only used at boot time. Hence, passing it once via
the FF-A boot information protocol made sense.

The FF-A spec does not specify the format in which boot information is
passed. An SP manifest is specified in a DT by default. So, this was
the natural choice. The same information could be passed using a HOB
list but that would require additional support in TF-A.

I hope this helps.

cheers,
Achin


On Thu, 2023-07-13 at 09:48 -0700, Chris Fernald wrote:
> Achin/Nishant, could you explain the motivation behind falling back
> to
> device tree for some of the secure partition information? It seems
> like
> we have this large abstraction framework using FF-A and it seems a
> bit
> odd to have the secure partition have to directly read device tree
> for
> some of this information when everything else is query-able from the
> framework itself.
>
> Thanks,
>
> Chris Fernald
>
> On 7/13/2023 8:24 AM, Girish Mahadevan via groups.io wrote:
> > I had one comment , in-line.
> >
> > Thanks
> > Girish
> >
> > On 7/11/2023 8:36 AM, Nishant Sharma via groups.io wrote:
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > From: Achin Gupta <achin.gupta@arm.com>
> > >
> > > This patch discovers the SP manifest in DT format passed by the
> > > SPMC. It
> > > then parses it to obtain the boot information required to
> > > initialise the
> > > SP.
> > >
> > > Signed-off-by: Achin Gupta <achin.gupta@arm.com>
> > > Signed-off-by: Sayanta Pattanayak <sayanta.pattanayak@arm.com>
> > > Signed-off-by: Nishant Sharma <nishant.sharma@arm.com>
> > > ---
> > > StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h
> > > |
> > > 2 +-
> > > StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/Standalone
> > > MmCoreEntryPoint.c
> > > > 389 +++++++++++++++++++-
> > >   2 files changed, 381 insertions(+), 10 deletions(-)
> > >
> > > diff --git
> > > a/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.
> > > h
> > > b/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.
> > > h
> > > index c965192c702e..90d67a2f25b5 100644
> > > ---
> > > a/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.
> > > h
> > > +++
> > > b/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.
> > > h
> > > @@ -2,7 +2,7 @@
> > >     Entry point to the Standalone MM Foundation when initialized
> > > during the SEC
> > >     phase on ARM platforms
> > >
> > > -Copyright (c) 2017 - 2021, Arm Ltd. All rights reserved.<BR>
> > > +Copyright (c) 2017 - 2023, Arm Ltd. All rights reserved.<BR>
> > >   SPDX-License-Identifier: BSD-2-Clause-Patent
> > >
> > >   **/
> > > diff --git
> > > a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/Standalo
> > > neMmCoreEntryPoint.c
> > > b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/Standalo
> > > neMmCoreEntryPoint.c
> > >
> > > index 9f6af55c86c4..505786aff07c 100644
> > > ---
> > > a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/Standalo
> > > neMmCoreEntryPoint.c
> > > +++
> > > b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/Standalo
> > > neMmCoreEntryPoint.c
> > > @@ -38,6 +38,10 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > >
> > >   #define BOOT_PAYLOAD_VERSION  1
> > >
> > > +#define FFA_PAGE_4K 0
> > > +#define FFA_PAGE_16K 1
> > > +#define FFA_PAGE_64K 2
> > > +
> > >   PI_MM_ARM_TF_CPU_DRIVER_ENTRYPOINT  CpuDriverEntryPoint = NULL;
> > >
> > >   /**
> > > @@ -106,6 +110,7 @@ GetAndPrintBootinformation (
> > >     }
> > >
> > >     return PayloadBootInfo;
> > > +}
> > >
> > >   /**
> > >     An StMM SP implements partial support for FF-A v1.0. The FF-A
> > > ABIs are used to
> > > @@ -266,6 +271,308 @@ DelegatedEventLoop (
> > >     }
> > >   }
> > >
> > > +STATIC
> > > +BOOLEAN
> > > +CheckDescription (
> > > +    IN VOID   * DtbAddress,
> > > +    IN INT32    Offset,
> > > +    OUT CHAR8 * Description,
> > > +    OUT UINT32  Size
> > > +    )
> > > +{
> > > +  CONST CHAR8 * Property;
> > > +  INT32 LenP;
> > > +
> > > +  Property = fdt_getprop (DtbAddress, Offset, "description",
> > > &LenP);
> > > +  if (Property == NULL) {
> > > +    return FALSE;
> > > +  }
> > > +
> > > + return CompareMem (Description, Property, MIN(Size,
> > > (UINT32)LenP))
> > > == 0;
> > > +
> > > +}
> > > +
> > > +STATIC
> > > +EFI_STATUS
> > > +ReadProperty32 (
> > > +    IN  VOID   * DtbAddress,
> > > +    IN  INT32    Offset,
> > > +    IN  CHAR8  * Property,
> > > +    OUT UINT32 * Value
> > > +    )
> > > +{
> > > +  CONST UINT32 * Property32;
> > > +
> > > +  Property32 =  fdt_getprop (DtbAddress, Offset, Property,
> > > NULL);
> > > +  if (Property32 == NULL) {
> > > +    DEBUG ((
> > > +          DEBUG_ERROR,
> > > +          "%s: Missing in FF-A boot information manifest\n",
> > > +          Property
> > > +          ));
> > > +    return EFI_INVALID_PARAMETER;
> > > +  }
> > > +
> > > +  *Value = fdt32_to_cpu (*Property32);
> > > +
> > > +  return EFI_SUCCESS;
> > > +}
> > > +
> > > +STATIC
> > > +EFI_STATUS
> > > +ReadProperty64 (
> > > +    IN  VOID   * DtbAddress,
> > > +    IN  INT32    Offset,
> > > +    IN  CHAR8  * Property,
> > > +    OUT UINT64 * Value
> > > +    )
> > > +{
> > > +  CONST UINT64 * Property64;
> > > +
> > > +  Property64 =  fdt_getprop (DtbAddress, Offset, Property,
> > > NULL);
> > > +  if (Property64 == NULL) {
> > > +    DEBUG ((
> > > +          DEBUG_ERROR,
> > > +          "%s: Missing in FF-A boot information manifest\n",
> > > +          Property
> > > +          ));
> > > +    return EFI_INVALID_PARAMETER;
> > > +  }
> > > +
> > > +  *Value = fdt64_to_cpu (*Property64);
> > > +
> > > +  return EFI_SUCCESS;
> > > +}
> > > +
> > > +STATIC
> > > +BOOLEAN
> > > +ReadRegionInfo (
> > > +    IN VOID  *DtbAddress,
> > > +    IN INT32  Node,
> > > +    IN CHAR8 *Region,
> > > +    IN UINTN  RegionStrSize,
> > > +    IN UINT32 PageSize,
> > > +    OUT UINT64 *Address,
> > > +    OUT UINT64 *Size
> > > +    )
> > > +{
> > > +  BOOLEAN FoundBuffer;
> > > +  INTN Status = 0;
> > > +
> > > +  FoundBuffer = CheckDescription (
> > > +      DtbAddress,
> > > +      Node,
> > > +      Region,
> > > +      RegionStrSize
> > > +      );
> > > +  if (!FoundBuffer) {
> > > +    return FALSE;
> > > +  }
> > > +
> > > +  DEBUG ((DEBUG_INFO, "Found Node: %a\n", Region));
> > > +  Status = ReadProperty64 (
> > > +      DtbAddress,
> > > +      Node,
> > > +      "base-address",
> > > +      Address
> > > +      );
> > > +  if (Status != EFI_SUCCESS) {
> > > +    DEBUG ((DEBUG_ERROR, "base-address missing in DTB"));
> > > +    return FALSE;
> > > +  }
> > > +  DEBUG ((
> > > +        DEBUG_INFO,
> > > +        "base = 0x%llx\n",
> > > +        *Address
> > > +        ));
> > > +
> > > +  Status = ReadProperty32 (
> > > +      DtbAddress,
> > > +      Node,
> > > +      "pages-count",
> > > +      (UINT32*)Size
> > > +      );
> > > +  if (Status != EFI_SUCCESS) {
> > > +    DEBUG ((DEBUG_ERROR, "pages-count missing in DTB"));
> > > +    return FALSE;
> > > +  }
> > > +
> > > +  DEBUG ((DEBUG_ERROR, "pages-count: 0x%lx\n", *Size));
> > > +
> > > +  *Size = *Size * PageSize;
> > > +  DEBUG ((
> > > +        DEBUG_INFO,
> > > +        "Size = 0x%llx\n",
> > > +        *Size
> > > +        ));
> > > +
> > > +  return TRUE;
> > > +}
> > > +
> > > +/**
> > > +
> > > +  Populates FF-A boot information structure.
> > > +
> > > +  This function receives the address of a DTB from which boot
> > > information defind
> > > +  by FF-A and required to initialize the standalone environment
> > > is
> > > extracted.
> > > +
> > > +  @param [in, out] StmmBootInfo  Pointer to a pre-allocated boot
> > > info structure to be
> > > +                                 populated.
> > > +  @param [in]      DtbAddress    Address of the Device tree from
> > > where boot
> > > +                                 information will be fetched.
> > > +**/
> > > +STATIC
> > > +EFI_STATUS
> > > +PopulateBootinformation (
> > > +  IN  OUT  EFI_STMM_BOOT_INFO *StmmBootInfo,
> > > +  IN       VOID              *DtbAddress
> > > +)
> > > +{
> > > +  INTN Status;
> > > +  INT32 Offset;
> > > +  INT32 Node;
> > > +  BOOLEAN FoundNsCommBuffer = FALSE;
> > > +  BOOLEAN FoundSharedBuffer = FALSE;
> > > +  BOOLEAN FoundHeap = FALSE;
> > > +  UINT32 PageSize;
> > > +
> > > +  Offset = fdt_node_offset_by_compatible (DtbAddress, -1,
> > > "arm,ffa-manifest-1.0");
> > > +  DEBUG ((DEBUG_INFO, "Offset  = %d \n", Offset));
> > > +  if (Offset < 0) {
> > > +    DEBUG ((DEBUG_ERROR, "Missing FF-A boot information in
> > > manifest\n"));
> > > +    return EFI_NOT_FOUND;
> > > +  }
> > > +
> > > +  Status = ReadProperty64 (
> > > +      DtbAddress,
> > > +      Offset,
> > > +      "load-address",
> > > +      &StmmBootInfo->SpMemBase
> > > +      );
> > > +  if (Status != EFI_SUCCESS) {
> > > +    return Status;
> > > +  }
> > > +  DEBUG ((DEBUG_INFO, "sp mem base  = 0x%llx\n",
> > > StmmBootInfo->SpMemBase));
> > > +
> > > +  Status = ReadProperty64 (
> > > +      DtbAddress,
> > > +      Offset,
> > > +      "image-size",
> > > +      &StmmBootInfo->SpMemSize
> > > +      );
> > > +  if (Status != EFI_SUCCESS) {
> > > +    return Status;
> > > +  }
> > > +  DEBUG ((DEBUG_INFO, "sp mem size  = 0x%llx\n",
> > > StmmBootInfo->SpMemSize));
> > > +
> > > +  Status = ReadProperty32 (DtbAddress, Offset, "xlat-granule",
> > > &PageSize);
> > > +  if (Status != EFI_SUCCESS) {
> > > +    return Status;
> > > +  }
> > > +
> > > +  /*  EFI_PAGE_SIZE is 4KB */
> > > +  switch (PageSize) {
> > > +    case FFA_PAGE_4K:
> > > +      PageSize = EFI_PAGE_SIZE;
> > > +      break;
> > > +
> > > +    case FFA_PAGE_16K:
> > > +      PageSize = 4 * EFI_PAGE_SIZE;
> > > +      break;
> > > +
> > > +    case FFA_PAGE_64K:
> > > +      PageSize = 16 * EFI_PAGE_SIZE;
> > > +      break;
> > > +
> > > +    default:
> > > +      DEBUG ((DEBUG_ERROR, "Invalid page type = %lu\n",
> > > PageSize));
> > > +      return EFI_INVALID_PARAMETER;
> > > +      break;
> > > +  };
> > > +
> > > +  DEBUG ((DEBUG_INFO, "Page Size = 0x%lx\n", PageSize));
> > > +
> > > +  Offset = fdt_subnode_offset_namelen (
> > > +      DtbAddress,
> > > +      Offset,
> > > +      "memory-regions",
> > > +      sizeof("memory-regions") - 1
> > > +      );
> > > +  if (Offset < 1) {
> > > +    DEBUG ((
> > > +          DEBUG_ERROR,
> > > +          "%s: Missing in FF-A boot information manifest\n",
> > > +          "memory-regions"
> > > +          ));
> > > +    return EFI_INVALID_PARAMETER;
> > > +  }
> > > +
> > > +  for (
> > > +      Node = fdt_first_subnode (DtbAddress, Offset);
> > > +      Node > 0;
> > > +      Node = fdt_next_subnode (DtbAddress, Node)) {
> > > +    if (!FoundNsCommBuffer) {
> > > +      FoundNsCommBuffer = ReadRegionInfo (
> > > +          DtbAddress,
> > > +          Node,
> > > +          "ns-comm",
> > > +          sizeof ("ns-comm") - 1,
> > > +          PageSize,
> > > +          &StmmBootInfo->SpNsCommBufBase,
> > > +          &StmmBootInfo->SpNsCommBufSize
> > > +          );
> > > +    }
> > > +
> > > +    if (!FoundHeap) {
> > > +      FoundHeap = ReadRegionInfo (
> > > +          DtbAddress,
> > > +          Node,
> > > +          "heap",
> > > +          sizeof ("heap") - 1,
> > > +          PageSize,
> > > +          &StmmBootInfo->SpHeapBase,
> > > +          &StmmBootInfo->SpHeapSize
> > > +          );
> > > +    }
> > > +
> > > +    if (!FoundSharedBuffer) {
> > > +      FoundSharedBuffer = ReadRegionInfo (
> > > +          DtbAddress,
> > > +          Node,
> > > +          "shared-buff",
> > > +          sizeof ("shared-buff") - 1,
> > > +          PageSize,
> > > +          &StmmBootInfo->SpSharedBufBase,
> > > +          &StmmBootInfo->SpSharedBufSize
> > > +          );
> > > +    }
> > > +  }
> > > +
> > > +  if (!FoundNsCommBuffer) {
> > > +    DEBUG ((DEBUG_ERROR, "Failed to find ns-comm buffer
> > > info\n"));
> > > +    return EFI_INVALID_PARAMETER;
> > > +  }
> > > +
> > > +  if (!FoundHeap) {
> > > +    DEBUG ((DEBUG_ERROR, "Failed to find heap buffer info\n"));
> > > +    return EFI_INVALID_PARAMETER;
> > > +  }
> > > +
> > > +  if (!FoundSharedBuffer) {
> > > +    DEBUG ((DEBUG_ERROR, "Failed to find shared buffer
> > > info\n"));
> > > +    return EFI_INVALID_PARAMETER;
> > > +  }
> > > +
> > > +  // Populate CPU information under the assumption made in the
> > > FF-A
> > > spec that
> > > +  // this is a uniprocessor SP that is capable of migration. So,
> > > it
> > > is fine if
> > > +  // it sees 0 as both its physical and linear cpu id
> > > +  StmmBootInfo->CpuInfo.Mpidr = 0;
> > > +  StmmBootInfo->CpuInfo.LinearId = 0;
> > > +  StmmBootInfo->CpuInfo.Flags = 0;
> > > +
> > > +  return EFI_SUCCESS;
> > > +}
> >
> > [GM]
> > Outside of the mandatory properties, I expect that there could be
> > some
> > custom fields in the dtb manifest. Can you add a means for a vendor
> > to
> > hook in parsing properties that only apply to them. An OEM Library
> > ?
> > > +
> > >   /**
> > >     Query the SPM version, check compatibility and return success
> > > if
> > > compatible.
> > >
> > > @@ -343,6 +650,49 @@ InitArmSvcArgs (
> > >     }
> > >   }
> > >
> > > +
> > > +STATIC
> > > +EFI_STATUS
> > > +GetSpManifest (
> > > +  IN  OUT     UINT64 **SpManifestAddr,
> > > +  IN          VOID    *BootInfoAddr
> > > +  )
> > > +{
> > > +  EFI_FFA_BOOT_INFO_HEADER *FfaBootInfo;
> > > +  EFI_FFA_BOOT_INFO_DESC   *FfaBootInfoDesc;
> > > +
> > > +  // Paranoid check to avoid an inadvertent NULL pointer
> > > dereference.
> > > +  if (BootInfoAddr == NULL) {
> > > +    DEBUG ((DEBUG_ERROR, "FF-A Boot information is NULL\n"));
> > > +    return EFI_INVALID_PARAMETER;
> > > +  }
> > > +
> > > +  // Check boot information magic number.
> > > +  FfaBootInfo = (EFI_FFA_BOOT_INFO_HEADER *) BootInfoAddr;
> > > +  if (FfaBootInfo->Magic != FFA_INIT_DESC_SIGNATURE) {
> > > +    DEBUG ((
> > > +          DEBUG_ERROR, "FfaBootInfo Magic no. is invalid
> > > 0x%ux\n",
> > > +          FfaBootInfo->Magic
> > > +          ));
> > > +    return EFI_INVALID_PARAMETER;
> > > +  }
> > > +
> > > +
> > > +  FfaBootInfoDesc =
> > > +    (EFI_FFA_BOOT_INFO_DESC *)((UINT8 *)BootInfoAddr +
> > > +        FfaBootInfo->OffsetBootInfoDesc);
> > > +
> > > +  if (FfaBootInfoDesc->Type ==
> > > +      (FFA_BOOT_INFO_TYPE(FFA_BOOT_INFO_TYPE_STD) |
> > > +      FFA_BOOT_INFO_TYPE_ID(FFA_BOOT_INFO_TYPE_ID_FDT))) {
> > > +    *SpManifestAddr = (UINT64 *) FfaBootInfoDesc->Content;
> > > +    return EFI_SUCCESS;
> > > +  }
> > > +
> > > +  DEBUG ((DEBUG_ERROR, "SP manifest not found \n"));
> > > +  return EFI_NOT_FOUND;
> > > +}
> > > +
> > >   /**
> > >     The entry point of Standalone MM Foundation.
> > >
> > > @@ -363,6 +713,7 @@ ModuleEntryPoint (
> > >   {
> > >     PE_COFF_LOADER_IMAGE_CONTEXT    ImageContext;
> > >     EFI_SECURE_PARTITION_BOOT_INFO  *PayloadBootInfo;
> > > +  EFI_STMM_BOOT_INFO              StmmBootInfo = {0};
> > >     ARM_SVC_ARGS                    InitMmFoundationSvcArgs;
> > >     EFI_STATUS                      Status;
> > >     INT32                           Ret;
> > > @@ -372,6 +723,8 @@ ModuleEntryPoint (
> > >     VOID                            *TeData;
> > >     UINTN                           TeDataSize;
> > >     EFI_PHYSICAL_ADDRESS            ImageBase;
> > > +  UINT64                          *DtbAddress;
> > > +  EFI_FIRMWARE_VOLUME_HEADER      *BfvAddress;
> > >     BOOLEAN                         UseOnlyFfaAbis = FALSE;
> > >
> > >     if (FixedPcdGet32 (PcdFfaEnable) != 0) {
> > > @@ -384,18 +737,36 @@ ModuleEntryPoint (
> > >       goto finish;
> > >     }
> > >
> > > -  PayloadBootInfo = GetAndPrintBootinformation
> > > (SharedBufAddress);
> > > -  if (PayloadBootInfo == NULL) {
> > > -    Status = EFI_UNSUPPORTED;
> > > -    goto finish;
> > > +  // If only FF-A is used, the DTB address is passed in the Boot
> > > information
> > > +  // structure. Else, the Boot info is copied from Sharedbuffer.
> > > +  if (UseOnlyFfaAbis) {
> > > +    Status = GetSpManifest (&DtbAddress, SharedBufAddress);
> > > +    if (Status != EFI_SUCCESS) {
> > > +      goto finish;
> > > +    }
> > > +
> > > +    // Extract boot information from the DTB
> > > +    Status = PopulateBootinformation (&StmmBootInfo, (VOID *)
> > > DtbAddress);
> > > +    if (Status != EFI_SUCCESS) {
> > > +      goto finish;
> > > +    }
> > > +
> > > +    // Stash the base address of the boot firmware volume
> > > +    BfvAddress = (EFI_FIRMWARE_VOLUME_HEADER *)
> > > StmmBootInfo.SpMemBase;
> > > +  } else {
> > > +    PayloadBootInfo = GetAndPrintBootinformation
> > > (SharedBufAddress);
> > > +    if (PayloadBootInfo == NULL) {
> > > +      Status = EFI_UNSUPPORTED;
> > > +      goto finish;
> > > +    }
> > > +
> > > +    // Stash the base address of the boot firmware volume
> > > +    BfvAddress = (EFI_FIRMWARE_VOLUME_HEADER *)
> > > PayloadBootInfo->SpImageBase;
> > >     }
> > >
> > > +
> > >     // Locate PE/COFF File information for the Standalone MM core
> > > module
> > > -  Status = LocateStandaloneMmCorePeCoffData (
> > > -             (EFI_FIRMWARE_VOLUME_HEADER
> > > *)(UINTN)PayloadBootInfo->SpImageBase,
> > > -             &TeData,
> > > -             &TeDataSize
> > > -             );
> > > +  Status = LocateStandaloneMmCorePeCoffData (BfvAddress,
> > > &TeData,
> > > &TeDataSize);
> > >
> > >     if (EFI_ERROR (Status)) {
> > >       goto finish;
> > > --
> > > 2.34.1
> > >
> > >
> > >
> > > -=-=-=-=-=-=
> > > Groups.io Links: You receive all messages sent to this group.
> > > View/Reply Online (#106801):
> > > https://edk2.groups.io/g/devel/message/106801
> > > Mute This Topic: https://groups.io/mt/100079881/6098446
> > > Group Owner: devel+owner@edk2.groups.io
> > > Unsubscribe: https://edk2.groups.io/g/devel/unsub
> > > [gmahadevan@nvidia.com]
> > > -=-=-=-=-=-=
> > >
> > >
> >
> >
> > 
> >
> >

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.