From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::244; helo=mail-it0-x244.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x244.google.com (mail-it0-x244.google.com [IPv6:2607:f8b0:4001:c0b::244]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 034A4210FB9ED for ; Thu, 14 Jun 2018 23:19:36 -0700 (PDT) Received: by mail-it0-x244.google.com with SMTP id a195-v6so1339972itd.3 for ; Thu, 14 Jun 2018 23:19:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=Tm+u5PmJb4Hgh464G1RA3rZvalgyXRE/M4pl13JhzFc=; b=K91eqVov5AsIjG9fCV+g39d6/qJFyp8sWq3ORh0MGDPFEn5I1Nz7AeoapS9eCjDGCz teJiqCAj23fpA2ZhN+eMitVj81qXtlHJQ7qYF0diaZA71Dv5jYxdjjIAI4iqmJn9uDHN iWoY8WRHhVOMxBGmbevrNdgDIRYz2wC/zR47o= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=Tm+u5PmJb4Hgh464G1RA3rZvalgyXRE/M4pl13JhzFc=; b=EczXvUcwjivQo74pfFSPNl6+Apc2lQ5spdachg2PBP4B+hY332UrdrS/qI+VKhOiLO cLDcXegfO2H0ZG0noBTClE/z62vT+ISZMV8daHw7vvYrO+H7omAyyIoSCt6Jn2FN+0k7 BpgxVM4427xx+PPYDu/S8kz3QqIuSNcc7k0WerX35P2GsPB5Zy1IjAhv+kAqiZIklzYc M03bIOFTnkF6siANGY2OgFTFTz6cBcQ9xGvQyGo9OOiYJ7iYouiQToxxgixlMOWtvgCS CB8Br0MM2cXur0VDolqGsWJJCz7Kva3QZfykeo/k42fqQCtj8W0mfOxih386DZlwt4Eb +nmg== X-Gm-Message-State: APt69E2rIWDIwKVGuHk2nQFZXcqPLElWSYrQwMYdk1jsWRfmViDvmLwK wQMlxYxYzXDkODSJvk3a9epxIs51E/p1Qiqtk5o6BvmG X-Google-Smtp-Source: ADUXVKKXyxrlAqR6VEgWQByDDEaITSvg4/igQ8yYJ196GvpcIG9z69X2fSTAsKI+czE/iFa6B/IJI0BCvzzD0aLNd7A= X-Received: by 2002:a24:e105:: with SMTP id n5-v6mr135074ith.68.1529043576188; Thu, 14 Jun 2018 23:19:36 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:bbc7:0:0:0:0:0 with HTTP; Thu, 14 Jun 2018 23:19:35 -0700 (PDT) In-Reply-To: References: <1529042132-29377-1-git-send-email-chandni.cherukuri@arm.com> <1529042132-29377-2-git-send-email-chandni.cherukuri@arm.com> From: Ard Biesheuvel Date: Fri, 15 Jun 2018 08:19:35 +0200 Message-ID: To: Chandni Cherukuri Cc: "edk2-devel@lists.01.org" , Leif Lindholm Subject: Re: [PATCH 1/2] Platform/ARM/Sgi: Install a Platform ID HOB X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 15 Jun 2018 06:19:37 -0000 Content-Type: text/plain; charset="UTF-8" On 15 June 2018 at 08:16, Ard Biesheuvel wrote: > Hello Chandni, > > On 15 June 2018 at 07:55, Chandni Cherukuri wrote: >> On SGI platforms, the trusted firmware executes prior to >> the SEC phase. It supplies to the SEC phase a pointer to >> a fdt, that contains platform specific information such as >> part number and config number of the SGI platform. The >> platform code that executes during the SEC phase creates a >> PPI that allows access to other PEIMs to obtain a copy of the >> fdt pointer. >> >> Further, during the PEI phase, a Platform ID PEIM installs a >> Platform ID HOB. The Platform ID HOB can be used by DXE >> drivers to identify the platform it is executing on. >> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Chandni Cherukuri >> --- >> .../SgiPkg/Library/PlatformLib/AArch64/Helper.S | 15 ++- >> .../ARM/SgiPkg/Library/PlatformLib/PlatformLib.c | 10 ++ >> .../ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf | 3 + >> .../Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c | 108 +++++++++++++++++++++ >> .../SgiPlatformPeiLib/SgiPlatformPeiLib.inf | 40 ++++++++ >> Platform/ARM/SgiPkg/SgiPlatform.dec | 5 + >> 6 files changed, 177 insertions(+), 4 deletions(-) >> create mode 100644 Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c >> create mode 100644 Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf >> > > Please use '--stat=250 --stat-graph-width=20' when using git > format-patch so that we get to see the entire path names in the > diffstat > >> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S b/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S >> index dab6c77..80b93ea 100644 >> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S >> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S >> @@ -22,15 +22,22 @@ GCC_ASM_EXPORT(ArmPlatformPeiBootAction) >> GCC_ASM_EXPORT(ArmPlatformGetCorePosition) >> GCC_ASM_EXPORT(ArmPlatformGetPrimaryCoreMpId) >> GCC_ASM_EXPORT(ArmPlatformIsPrimaryCore) >> +GCC_ASM_EXPORT(HwConfigDtBlob) >> + >> +HwConfigDtBlob: .long 0 >> > > Please move this definition to PlatformLib.c > >> // >> // First platform specific function to be called in the PEI phase >> // >> -// This function is actually the first function called by the PrePi >> -// or PrePeiCore modules. It allows to retrieve arguments passed to >> -// the UEFI firmware through the CPU registers. >> -// >> +// The trusted firmware passed the hw config DT blob in x1 register. >> +// Keep a copy of it in a global variable. >> +//VOID >> +//ArmPlatformPeiBootAction ( >> +// VOID >> +// ); >> ASM_PFX(ArmPlatformPeiBootAction): >> + ldr x10, =HwConfigDtBlob > > Please use adr to take the address of HwConfigDtBlob > >> + str x1, [x10] >> ret >> >> //UINTN >> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c >> index ea3201a..54860e0 100644 >> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c >> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c >> @@ -15,6 +15,10 @@ >> #include >> #include >> #include >> +#include >> + >> +extern UINT64 HwConfigDtBlob; >> +STATIC SGI_HW_CONFIG_INFO_PPI mHwConfigDtInfoPpi; >> > > Where is this PPI declared? A PPI is like a protocol, it has its own > declaration in a header file under Include/Ppi in the package > >> STATIC ARM_CORE_INFO mCoreInfoTable[] = { >> { >> @@ -36,6 +40,7 @@ ArmPlatformInitialize ( >> IN UINTN MpId >> ) >> { >> + mHwConfigDtInfoPpi.HwConfigDtAddr = &HwConfigDtBlob; >> return RETURN_SUCCESS; >> } >> >> @@ -59,6 +64,11 @@ EFI_PEI_PPI_DESCRIPTOR gPlatformPpiTable[] = { >> EFI_PEI_PPI_DESCRIPTOR_PPI, >> &gArmMpCoreInfoPpiGuid, >> &mMpCoreInfoPpi >> + }, >> + { >> + EFI_PEI_PPI_DESCRIPTOR_PPI, >> + &gHwConfigDtInfoPpiGuid, >> + &mHwConfigDtInfoPpi >> } >> }; >> >> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf >> index 42e14d5..5d4bf72 100644 >> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf >> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf >> @@ -61,7 +61,10 @@ >> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress >> >> [Guids] >> + gArmSgiPlatformIdDescriptorGuid >> gEfiHobListGuid ## CONSUMES ## SystemTable >> + gFdtTableGuid >> >> [Ppis] >> gArmMpCoreInfoPpiGuid >> + gHwConfigDtInfoPpiGuid >> diff --git a/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c >> new file mode 100644 >> index 0000000..f6446e6 >> --- /dev/null >> +++ b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c >> @@ -0,0 +1,108 @@ >> +/** @file >> +* >> +* Copyright (c) 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 >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +/** >> + >> + This function returns the Platform ID of the platform >> + >> + This functions locates the HwConfig PPI and gets the base address of HW CONFIG >> + DT from which the platform ID is obtained using FDT helper functions >> + >> + @return returns the platform ID on success else returns 0 on error >> + >> +**/ >> + >> +STATIC >> +UINT32 >> +GetSgiPlatformId ( >> + VOID >> +) >> +{ >> + CONST UINT32 *Property; >> + INT32 Offset; >> + CONST VOID *HwCfgDtBlob; >> + SGI_HW_CONFIG_INFO_PPI *HwConfigInfoPpi; >> + EFI_STATUS Status; >> + >> + Status = PeiServicesLocatePpi (&gHwConfigDtInfoPpiGuid, 0, NULL, (VOID**)&HwConfigInfoPpi); > > Please check all your patches for overly long lines. 80 columns is that maximum > >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_ERROR, "PeiServicesLocatePpi failed with error %r\n", Status)); >> + return 0; >> + } >> + >> + HwCfgDtBlob = (VOID *)(*(HwConfigInfoPpi->HwConfigDtAddr)); >> + if (fdt_check_header (HwCfgDtBlob) != 0 ) { >> + DEBUG ((DEBUG_ERROR, "Invalid DTB file %p passed as HW_CONFIG\n", HwCfgDtBlob)); >> + return 0; >> + } >> + >> + Offset = fdt_subnode_offset (HwCfgDtBlob, 0, "system-id"); >> + if (Offset == -FDT_ERR_NOTFOUND) { >> + DEBUG ((DEBUG_ERROR, "Invalid DTB : system-id node not found\n")); >> + return 0; >> + } >> + >> + Property = fdt_getprop (HwCfgDtBlob, Offset, "platform-id", NULL); >> + if (Property == NULL) { >> + DEBUG ((DEBUG_ERROR, "Platform Id is NULL\n")); >> + return 0; >> + } >> + >> + return fdt32_to_cpu (*Property); >> +} >> + >> +/** >> + >> + This function creates a Platform ID HOB and assigns PlatformId as the >> + HobData >> + >> + @return asserts on error and returns EFI_INVALID_PARAMETER. On success >> + returns EFI_SUCCESS >> + >> +**/ >> +EFI_STATUS >> +EFIAPI >> +SgiPlatformIdPeim ( >> + IN EFI_PEI_FILE_HANDLE FileHandle, >> + IN CONST EFI_PEI_SERVICES **PeiServices >> +) >> +{ >> + SGI_PLATFORM_DESCRIPTOR *HobData; >> + >> + //Create platform descriptor HOB >> + HobData = (SGI_PLATFORM_DESCRIPTOR *)BuildGuidHob (&gArmSgiPlatformIdDescriptorGuid, >> + sizeof (SGI_PLATFORM_DESCRIPTOR)); >> + >> + //Get the platform id from the platform specific hw_config device tree >> + if (HobData != NULL) { > > Please invert the sense of this conditional, and return > EFI_OUT_OF_RESOURCES if HobData is NULL > >> + HobData->PlatformId = GetSgiPlatformId (); >> + if (HobData->PlatformId == 0) { >> + ASSERT (FALSE); >> + return EFI_INVALID_PARAMETER; >> + } >> + } else { > > After you have inverted the if() condition, you can drop the else as well > >> + DEBUG ((DEBUG_ERROR, "Platform HoB is NULL\n")); >> + ASSERT (FALSE); >> + return EFI_INVALID_PARAMETER; >> + } >> + >> + return EFI_SUCCESS; >> +} >> diff --git a/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf >> new file mode 100644 >> index 0000000..502d6ac >> --- /dev/null >> +++ b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf >> @@ -0,0 +1,40 @@ >> +# >> +# Copyright (c) 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. >> +# >> + >> +[Defines] >> + INF_VERSION = 0x0001001A >> + BASE_NAME = SgiPlatformIdPeiLib I forgot: this is a module not a library. So it should be called SgiPlatformIdPei or SgiPlatformIdPeim not ...Lib >> + FILE_GUID = a9936f3e-6a1b-11e8-8ce0-fffe69b86863 >> + MODULE_TYPE = PEIM >> + VERSION_STRING = 1.0 >> + ENTRY_POINT = SgiPlatformIdPeim >> + >> +[Packages] >> + EmbeddedPkg/EmbeddedPkg.dec >> + MdePkg/MdePkg.dec >> + Platform/ARM/SgiPkg/SgiPlatform.dec >> + >> +[LibraryClasses] >> + FdtLib >> + PeimEntryPoint >> + >> +[Sources] >> + SgiPlatformPeiLib.c >> + >> +[Guids] >> + gArmSgiPlatformIdDescriptorGuid >> + >> +[Ppis] >> + gHwConfigDtInfoPpiGuid >> + >> +[Depex] >> + TRUE >> diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dec b/Platform/ARM/SgiPkg/SgiPlatform.dec >> index d995937..ea9f6c5 100644 >> --- a/Platform/ARM/SgiPkg/SgiPlatform.dec >> +++ b/Platform/ARM/SgiPkg/SgiPlatform.dec >> @@ -29,9 +29,14 @@ >> Include # Root include for the package >> >> [Guids.common] >> + # ARM Sgi Platform ID descriptor >> + gArmSgiPlatformIdDescriptorGuid = { 0xf56f152a, 0xad2a, 0x11e6, { 0xb1, 0xa7, 0x00, 0x50, 0x56, 0x3c, 0x44, 0xcc } } >> gArmSgiTokenSpaceGuid = { 0x577d6941, 0xaea1, 0x40b4, { 0x90, 0x93, 0x2a, 0x86, 0x61, 0x72, 0x5a, 0x57 } } >> gSgi575AcpiTablesiFileGuid = { 0xc712719a, 0x0aaf, 0x438c, { 0x9c, 0xdd, 0x35, 0xab, 0x4d, 0x60, 0x20, 0x7d } } >> >> [PcdsFeatureFlag.common] >> # Set this PCD to TRUE to enable virtio support. >> gArmSgiTokenSpaceGuid.PcdVirtioSupported|TRUE|BOOLEAN|0x00000001 >> + >> +[Ppis] >> + gHwConfigDtInfoPpiGuid = { 0x6f606eb3, 0x9123, 0x4e15, { 0xa8, 0x9b, 0x0f, 0xac, 0x66, 0xef, 0xd0, 0x17 } } >> -- >> 2.7.4 >>