From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Chandni Cherukuri <chandni.cherukuri@arm.com>
Cc: "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
Leif Lindholm <leif.lindholm@linaro.org>
Subject: Re: [PATCH 1/2] Platform/ARM/Sgi: Install a Platform ID HOB
Date: Fri, 15 Jun 2018 08:19:35 +0200 [thread overview]
Message-ID: <CAKv+Gu-Rhu4ukckyySHHs920h1x4t-DDNmYwcCgbsjYtKtiqFA@mail.gmail.com> (raw)
In-Reply-To: <CAKv+Gu9HXHfD+-oNWm6Xm3u5qxaHhSZen05Ya_sa7Z7P_oUPrA@mail.gmail.com>
On 15 June 2018 at 08:16, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Hello Chandni,
>
> On 15 June 2018 at 07:55, Chandni Cherukuri <chandni.cherukuri@arm.com> 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 <chandni.cherukuri@arm.com>
>> ---
>> .../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 <Library/ArmPlatformLib.h>
>> #include <Library/BaseLib.h>
>> #include <Ppi/ArmMpCoreInfo.h>
>> +#include <SgiPlatform.h>
>> +
>> +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 <Library/DebugLib.h>
>> +#include <Library/HobLib.h>
>> +#include <Library/PeimEntryPoint.h>
>> +#include <Library/PeiServicesLib.h>
>> +#include <libfdt.h>
>> +#include <SgiPlatform.h>
>> +
>> +/**
>> +
>> + 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
>>
next prev parent reply other threads:[~2018-06-15 6:19 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-15 5:55 [PATCH 0/2] ARM Dynamic Configuration Chandni Cherukuri
2018-06-15 5:55 ` [PATCH 1/2] Platform/ARM/Sgi: Install a Platform ID HOB Chandni Cherukuri
2018-06-15 6:16 ` Ard Biesheuvel
2018-06-15 6:19 ` Ard Biesheuvel [this message]
2018-06-15 9:03 ` Leif Lindholm
2018-06-15 10:18 ` Leif Lindholm
2018-06-15 12:10 ` chandni cherukuri
2018-06-15 5:55 ` [PATCH 2/2] Platform/ARM/Sgi: Pick ACPI tables to install based on platform ID Chandni Cherukuri
2018-06-15 6:18 ` Ard Biesheuvel
2018-06-15 10:22 ` Leif Lindholm
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKv+Gu-Rhu4ukckyySHHs920h1x4t-DDNmYwcCgbsjYtKtiqFA@mail.gmail.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox