From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by mx.groups.io with SMTP id smtpd.web08.9037.1627835909340025048 for ; Sun, 01 Aug 2021 09:38:29 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=XMXxl5v5; spf=pass (domain: kernel.org, ip: 198.145.29.99, mailfrom: ardb@kernel.org) Received: by mail.kernel.org (Postfix) with ESMTPSA id BCB13610A0 for ; Sun, 1 Aug 2021 16:38:28 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1627835908; bh=QUK7hVHD+8wzrPLELwVXVukcEueOxR24PCYs0j7DTkA=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=XMXxl5v5+KkxhoH1qHfYjKc28QlRx12rHYhS8fwsC8veuhMYpWlcwgSvFqm0t3Wl/ LM0SzaTyIcrHncqUqaDi9Mqjhhp/Rqj2QYWRTyrc4Cpa2fM6Mu9yOT1LSVZFMqHw5b VSQLYOpnUWm7nG12lTD846NGANFiFCF+hgokOh3oYGm34oaO9kMryJXnUAYCaQM6yQ b8p5F7Cl9I16jODC/sQKjK8E2ZlXFz/mk/bflsr0TvR0IQkCYmPGW86dleBMpquT01 T1KF5fCTRqmjaaNCK80NnNo9VHJF5sr2c44Im6lbsy9RvYd9f4f6kKvSnz8ncUqPC7 /uNM+Y5xTg2YQ== Received: by mail-ot1-f42.google.com with SMTP id c7-20020a9d27870000b02904d360fbc71bso15180980otb.10 for ; Sun, 01 Aug 2021 09:38:28 -0700 (PDT) X-Gm-Message-State: AOAM533SXaYDTnQF5uWPsAEBokBk/A1S1Z80KG2e3iS84snCbknhpadE H5mxVcSiJRI0d7js9EHzMvzqmfp/KtAlB7FL/TE= X-Google-Smtp-Source: ABdhPJyOK3u2y6v4iTbG7byswVcUSAsSE81ey6c/aJ8DBkpFCfscApskZF/0QeSYds4AL8T3EV/trClf4UqqMkug53A= X-Received: by 2002:a05:6830:34a6:: with SMTP id c38mr8677586otu.108.1627835908039; Sun, 01 Aug 2021 09:38:28 -0700 (PDT) MIME-Version: 1.0 References: <20210730173510.24235-1-sayanta.pattanayak@arm.com> In-Reply-To: From: "Ard Biesheuvel" Date: Sun, 1 Aug 2021 18:38:17 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2][PATCH v1 1/1] StandaloneMmPkg: add support to populate StMM boot data from device tree To: Sayanta Pattanayak Cc: edk2-devel-groups-io , Sami Mujawar , Achin Gupta , Bret Barkelew , Jiewen Yao , Andrew Fish Content-Type: text/plain; charset="UTF-8" (correct Achin's email address, cc other replyers) On Sun, 1 Aug 2021 at 18:36, Ard Biesheuvel wrote: > > On Fri, 30 Jul 2021 at 19:35, Sayanta Pattanayak > wrote: > > > > Introduce support to populate StMM boot data via DTS parsing. > > Why? Don't we have FF-A manifests for this? I would expect the secure > partition manager to marshal this data into the appropriate format > when necessary. > > > 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 = <..>; > > 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 > > I don't think we should apply this change. DT is not part of the > original SPM or current FF-A spec, right? So please fix this in the > S-EL1 component instead. > > > > --- > > Link to github branch with this patch - > > https://github.com/SayantaP-arm/edk2/tree/stmm-dts > > > > 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 > > #include > > > > +#include > > #include > > #include > > #include > > @@ -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 > >