Re: [EXTERNAL] [edk2-devel] [edk2][PATCH v1 1/1] StandaloneMmPkg: add support to populate StMM boot data from device tree

["Andrew Fish" <afish@apple.com>]

[-- Attachment #1: Type: text/plain, Size: 17638 bytes --]



> On Jul 30, 2021, at 12:34 PM, Bret Barkelew via groups.io <http://groups.io/> <bret.barkelew=microsoft.com@groups.io <mailto:bret.barkelew=microsoft.com@groups.io>> wrote:
> 
> I don’t think this is a good dependency. StandaloneMmCore shouldn’t require anything from EmbeddedPkg, and if it does there’s probably some more generalization necessary.
>
> Further, we can’t require the MmCore (which should be considered generic at the level of PEI Core or DXE Core) include some external library like Libfdt without significant justification.
>

I think we should understand if this is being driven by some kind of standard? Maybe the Flat Device Tree lib should be part of the MdePkg? Or maybe MdeModulePkg?

Thanks,

Andrew Fish

> - Bret 
>
> From: Sayanta Pattanayak via groups.io <mailto:sayanta.pattanayak=arm.com@groups.io>
> Sent: Friday, July 30, 2021 10:36 AM
> To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> Cc: Ard Biesheuvel <mailto:ardb+tianocore@kernel.org>; Sami Mujawar <mailto:sami.mujawar@arm.com>; Achin Gupta <mailto:achin.gupta@arm.comw>
> Subject: [EXTERNAL] [edk2-devel] [edk2][PATCH v1 1/1] StandaloneMmPkg: add support to populate StMM boot data from device tree
>
> Introduce support to populate StMM boot data via DTS parsing. The DTB is
> passed as a boot argument by a binary of higer exception level.
> Previously it was achieved by placing the boot data structure in a
> shared buffer and the address of this shared buffer was passed by the
> binary of higher exception level. Now either of the option can be used
> for populating StMM boot info.
> 
> StMM boot information structure binding in device tree can be of following
> prototype. Property values are not mentioned here.
> 
> bootarg {
>   compatible = "bootargs";
>   h_type  = <..>;
>   h_version = <..>;
>   h_size    = <..>;
>   h_attr    = <..>;
>   sp_mem_base         = <..>;
>   sp_mem_limit        = <..>;
>   sp_image_base       = <..>;
>   sp_stack_base       = <..>;
>   sp_heap_base        = <..>;
>   sp_ns_comm_buf_base = <..>;fc
>   sp_shared_buf_base  = <..>;
>   sp_image_size       = <..>;
>   sp_pcpu_stack_size  = <..>;
>   sp_heap_size        = <..>;
>   sp_ns_comm_buf_size = <..>;
>   sp_shared_buf_size  = <..>;
>   num_sp_mem_regions  = <..>;
>   num_cpus            = <..>;
> };
> 
> Addition of DTS supoort involves a dependency on FdtLib from EmbeddedPkg.
> 
> Signed-off-by: Sayanta Pattanayak <sayanta.pattanayak@arm.com <mailto:sayanta.pattanayak@arm.com>>
> ---
>  Link to github branch with this patch -
>  https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FSayantaP-arm%2Fedk2%2Ftree%2Fstmm-dts&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7Cb365266c83cb433d1e0108d953806c83%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637632633786132182%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=gJw%2BH8Dj9lih1DNrIzbNPToJn9nH%2BHqpvpSr2JJrS4I%3D&amp;reserved=0 <https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FSayantaP-arm%2Fedk2%2Ftree%2Fstmm-dts&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7Cb365266c83cb433d1e0108d953806c83%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637632633786132182%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=gJw%2BH8Dj9lih1DNrIzbNPToJn9nH%2BHqpvpSr2JJrS4I%3D&amp;reserved=0>
> 
>  StandaloneMmPkg/StandaloneMmPkg.dsc                                                     |   1 +
>  StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf       |   3 +
>  StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c | 153 ++++++++++++++++++--
>  3 files changed, 143 insertions(+), 14 deletions(-)
> 
> diff --git a/StandaloneMmPkg/StandaloneMmPkg.dsc b/StandaloneMmPkg/StandaloneMmPkg.dsc
> index 0c45df95e2dd..e3a3a6ee3ba1 100644
> --- a/StandaloneMmPkg/StandaloneMmPkg.dsc
> +++ b/StandaloneMmPkg/StandaloneMmPkg.dsc
> @@ -49,6 +49,7 @@
>    HobLib|StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf
>    IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
>    MemLib|StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.inf
> +  FdtLib|EmbeddedPkg/Library/FdtLib/FdtLib.inf
>    MemoryAllocationLib|StandaloneMmPkg/Library/StandaloneMmCoreMemoryAllocationLib/StandaloneMmCoreMemoryAllocationLib.inf
>    MmServicesTableLib|MdePkg/Library/StandaloneMmServicesTableLib/StandaloneMmServicesTableLib.inf
>    PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
> diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
> index 4fa426f58ef4..0a2e519dd664 100644
> --- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
> +++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
> @@ -30,6 +30,7 @@
>    X64/StandaloneMmCoreEntryPoint.c
>
>  [Packages]
> +  EmbeddedPkg/EmbeddedPkg.dec
>    MdePkg/MdePkg.dec
>    MdeModulePkg/MdeModulePkg.dec
>    StandaloneMmPkg/StandaloneMmPkg.dec
> @@ -40,10 +41,12 @@
>  [LibraryClasses]
>    BaseLib
>    DebugLib
> +  FdtLib
>
>  [LibraryClasses.AARCH64]
>    StandaloneMmMmuLib
>    ArmSvcLib
> +  FdtLib
>
>  [Guids]
>    gMpInformationHobGuid
> diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
> index 6c50f470aa35..cc09d75dac36 100644
> --- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
> +++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
> @@ -16,6 +16,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #include <Guid/MmramMemoryReserve.h>
>  #include <Guid/MpInformation.h>
>
> +#include <libfdt.h>
>  #include <Library/ArmMmuLib.h>
>  #include <Library/ArmSvcLib.h>
>  #include <Library/DebugLib.h>
> @@ -45,33 +46,31 @@ STATIC CONST UINT32 mSpmMinorVerFfa = SPM_MINOR_VERSION_FFA;
>  PI_MM_ARM_TF_CPU_DRIVER_ENTRYPOINT      CpuDriverEntryPoint = NULL;
>
>  /**
> -  Retrieve a pointer to and print the boot information passed by privileged
> -  secure firmware.
> +  Prints boot information.
>
> -  @param  [in] SharedBufAddress   The pointer memory shared with privileged
> -                                  firmware.
> +  This function prints the boot information, which is passed by privileged
> +  secure firmware through shared buffer or other mechanism.
>
> +  @param  [in] PayloadBootInfo   Pointer to StandaloneMM Boot Info structure.
>  **/
> -EFI_SECURE_PARTITION_BOOT_INFO *
> -GetAndPrintBootinformation (
> -  IN VOID                      *SharedBufAddress
> +VOID
> +PrintBootinformation (
> +  IN EFI_SECURE_PARTITION_BOOT_INFO *PayloadBootInfo
>  )
>  {
> -  EFI_SECURE_PARTITION_BOOT_INFO *PayloadBootInfo;
>    EFI_SECURE_PARTITION_CPU_INFO  *PayloadCpuInfo;
>    UINTN                          Index;
>
> -  PayloadBootInfo = (EFI_SECURE_PARTITION_BOOT_INFO *) SharedBufAddress;
>
>    if (PayloadBootInfo == NULL) {
>      DEBUG ((DEBUG_ERROR, "PayloadBootInfo NULL\n"));
> -    return NULL;
> +    return;
>    }
>
>    if (PayloadBootInfo->Header.Version != BOOT_PAYLOAD_VERSION) {
>      DEBUG ((DEBUG_ERROR, "Boot Information Version Mismatch. Current=0x%x, Expected=0x%x.\n",
>              PayloadBootInfo->Header.Version, BOOT_PAYLOAD_VERSION));
> -    return NULL;
> +    return;
>    }
>
>    DEBUG ((DEBUG_INFO, "NumSpMemRegions - 0x%x\n", PayloadBootInfo->NumSpMemRegions));
> @@ -96,7 +95,7 @@ GetAndPrintBootinformation (
>
>    if (PayloadCpuInfo == NULL) {
>      DEBUG ((DEBUG_ERROR, "PayloadCpuInfo NULL\n"));
> -    return NULL;
> +    return;
>    }
>
>    for (Index = 0; Index < PayloadBootInfo->NumCpus; Index++) {
> @@ -105,7 +104,7 @@ GetAndPrintBootinformation (
>      DEBUG ((DEBUG_INFO, "Flags           - 0x%x\n", PayloadCpuInfo[Index].Flags));
>    }
>
> -  return PayloadBootInfo;
> +  return;
>  }
>
>  /**
> @@ -194,6 +193,119 @@ DelegatedEventLoop (
>    }
>  }
>
> +/**
> +  Populates StandAloneMM boot information structure.
> +
> +  This function receives dtb Address, where StMM Boot information specific
> +  properties will be looked out to form the booting structure of type
> +  EFI_SECURE_PARTITION_BOOT_INFO. At first, the properties for StandAloneMM
> +  ConfigSize and  Memory limit will be checked out. Boot information will
> +  be stored at address (Memory Limit - ConfigSize). Thereafter all boot
> +  information specific properties will be parsed and corresponding values
> +  will be obtained.
> +
> +  @param  [out] BootInfo   Pointer, where Boot Info structure will be populated.
> +  @param  [in] DtbAddress  Address of the Device tree from where Boot
> +                           information will be fetched.
> +**/
> +VOID
> +PopulateBootinformation (
> +  OUT EFI_SECURE_PARTITION_BOOT_INFO **BootInfo,
> +  IN VOID   *DtbAddress
> +)
> +{
> +  INT32           Offset;
> +  CONST UINT32    *Property;
> +  CONST UINT64    *Property64;
> +  UINT32          ConfigSize;
> +  UINT64          SpMemLimit;
> +  EFI_SECURE_PARTITION_BOOT_INFO *PayloadBootInfo;
> +
> +  Offset = fdt_node_offset_by_compatible (DtbAddress, -1, "config-size");
> +  if (Offset < 0) {
> +    DEBUG ((DEBUG_WARN, "Total Config Size is not  defined\n"));
> +  } else {
> +    Property = fdt_getprop (DtbAddress, Offset, "size", NULL);
> +    if (Property) {
> +      ConfigSize = fdt32_to_cpu (*Property);
> +      DEBUG ((DEBUG_INFO, "stmm dtb config-size  = 0x%x \n", ConfigSize));
> +    }
> +  }
> +
> +  Offset = fdt_node_offset_by_compatible (DtbAddress, -1, "bootargs");
> +  if (Offset >= 0) {
> +    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_mem_limit", NULL);
> +    SpMemLimit = fdt64_to_cpu (*Property64);
> +  }
> +
> +  if (SpMemLimit && ConfigSize)
> +    PayloadBootInfo =
> +      (EFI_SECURE_PARTITION_BOOT_INFO *)(SpMemLimit - ConfigSize);
> +
> +  if (PayloadBootInfo) {
> +    PayloadBootInfo->SpMemLimit = SpMemLimit;
> +
> +    Property =  fdt_getprop (DtbAddress, Offset, "h_type", NULL);
> +    PayloadBootInfo->Header.Type = (UINT8) fdt32_to_cpu(*Property);
> +
> +    Property =  fdt_getprop (DtbAddress, Offset, "h_version", NULL);
> +    PayloadBootInfo->Header.Version = (UINT8) fdt32_to_cpu(*Property);
> +
> +    Property =  fdt_getprop (DtbAddress, Offset, "h_size", NULL);
> +    PayloadBootInfo->Header.Size = (UINT8) fdt32_to_cpu(*Property);
> +
> +    Property =  fdt_getprop (DtbAddress, Offset, "h_attr", NULL);
> +    PayloadBootInfo->Header.Attr = fdt32_to_cpu(*Property);
> +
> +    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_mem_base", NULL);
> +    PayloadBootInfo->SpMemBase = fdt64_to_cpu(*Property64);
> +
> +    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_image_base", NULL);
> +    PayloadBootInfo->SpImageBase = fdt64_to_cpu(*Property64);
> +
> +    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_stack_base", NULL);
> +    PayloadBootInfo->SpStackBase = fdt64_to_cpu(*Property64);
> +
> +    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_heap_base", NULL);
> +    PayloadBootInfo->SpHeapBase = fdt64_to_cpu(*Property64);
> +
> +    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_ns_comm_buf_base", NULL);
> +    PayloadBootInfo->SpNsCommBufBase = fdt64_to_cpu(*Property64);
> +
> +    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_shared_buf_base", NULL);
> +    PayloadBootInfo->SpSharedBufBase = fdt64_to_cpu(*Property64);
> +
> +    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_image_size", NULL);
> +    PayloadBootInfo->SpImageSize = fdt64_to_cpu(*Property64);
> +
> +    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_pcpu_stack_size", NULL);
> +    PayloadBootInfo->SpPcpuStackSize = fdt64_to_cpu(*Property64);
> +
> +    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_heap_size", NULL);
> +    PayloadBootInfo->SpHeapSize = fdt64_to_cpu(*Property64);
> +
> +    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_ns_comm_buf_size", NULL);
> +    PayloadBootInfo->SpNsCommBufSize = fdt64_to_cpu(*Property64);
> +
> +    Property64 =  fdt_getprop (DtbAddress, Offset, "sp_shared_buf_size", NULL);
> +    PayloadBootInfo->SpPcpuSharedBufSize = fdt64_to_cpu(*Property64);
> +
> +    Property =  fdt_getprop (DtbAddress, Offset, "num_sp_mem_regions", NULL);
> +    PayloadBootInfo->NumSpMemRegions = fdt32_to_cpu(*Property);
> +
> +    Property =  fdt_getprop (DtbAddress, Offset, "num_cpus", NULL);
> +    PayloadBootInfo->NumCpus = fdt32_to_cpu(*Property);
> +
> +    PayloadBootInfo->CpuInfo =
> +      (EFI_SECURE_PARTITION_CPU_INFO *)((UINT64)PayloadBootInfo +
> +                                        sizeof(EFI_SECURE_PARTITION_BOOT_INFO));
> +  }
> +
> +  *BootInfo = PayloadBootInfo;
> +
> +  return;
> +}
> +
>  /**
>    Query the SPM version, check compatibility and return success if compatible.
>
> @@ -313,6 +425,7 @@ _ModuleEntryPoint (
>    VOID                                    *TeData;
>    UINTN                                   TeDataSize;
>    EFI_PHYSICAL_ADDRESS                    ImageBase;
> +  VOID                                    *DtbAddress;
>
>    // Get Secure Partition Manager Version Information
>    Status = GetSpmVersion ();
> @@ -320,12 +433,24 @@ _ModuleEntryPoint (
>      goto finish;
>    }
>
> -  PayloadBootInfo = GetAndPrintBootinformation (SharedBufAddress);
> +  // In cookie1 the DTB address is passed. With reference to DTB, Boot
> +  // info structure can be populated.
> +  // If cookie1 doesn't have any value, then Boot info is copied from
> +  // Sharedbuffer.
> +  if (cookie1) {
> +    DtbAddress = (void *)cookie1;
> +    PopulateBootinformation (&PayloadBootInfo, DtbAddress);
> +  } else {
> +    PayloadBootInfo = (EFI_SECURE_PARTITION_BOOT_INFO *)SharedBufAddress;
> +  }
> +
>    if (PayloadBootInfo == NULL) {
>      Status = EFI_UNSUPPORTED;
>      goto finish;
>    }
>
> +  PrintBootinformation (PayloadBootInfo);
> +
>    // Locate PE/COFF File information for the Standalone MM core module
>    Status = LocateStandaloneMmCorePeCoffData (
>               (EFI_FIRMWARE_VOLUME_HEADER *) PayloadBootInfo->SpImageBase,
> -- 
> 2.17.1
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io <http://groups.io/> Links: You receive all messages sent to this group.
> View/Reply Online (#78449): https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F78449&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7Cb365266c83cb433d1e0108d953806c83%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637632633786132182%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=0DbkyBJe0prgx2vSCVVsrI0GoVbCipcfmbZp83LREjo%3D&amp;reserved=0 <https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F78449&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7Cb365266c83cb433d1e0108d953806c83%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637632633786132182%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=0DbkyBJe0prgx2vSCVVsrI0GoVbCipcfmbZp83LREjo%3D&amp;reserved=0>
> Mute This Topic: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.io%2Fmt%2F84555304%2F1852292&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7Cb365266c83cb433d1e0108d953806c83%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637632633786132182%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=1u3cr0MmJmjrkxtGgfOe1mb8%2F7vUXaoyUr5%2FmC1Uq%2F8%3D&amp;reserved=0 <https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.io%2Fmt%2F84555304%2F1852292&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7Cb365266c83cb433d1e0108d953806c83%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637632633786132182%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=1u3cr0MmJmjrkxtGgfOe1mb8%2F7vUXaoyUr5%2FmC1Uq%2F8%3D&amp;reserved=0>
> Group Owner: devel+owner@edk2.groups.io <mailto:devel+owner@edk2.groups.io>
> Unsubscribe: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Funsub&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7Cb365266c83cb433d1e0108d953806c83%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637632633786142135%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=Yr%2BSNqWHllfxHkFqBhCyP2TE4IGRLFLm%2FCCWexjB0AM%3D&amp;reserved=0 <https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Funsub&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7Cb365266c83cb433d1e0108d953806c83%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637632633786142135%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=Yr%2BSNqWHllfxHkFqBhCyP2TE4IGRLFLm%2FCCWexjB0AM%3D&amp;reserved=0> [bret.barkelew@microsoft.com <mailto:bret.barkelew@microsoft.com>]
> -=-=-=-=-=-=
> 
> 
>
> 


[-- Attachment #2: Type: text/html, Size: 30637 bytes --]