ArmTransferListLib added utility functions and support to capture the TL base address #10759

PrachotanReddy · 2025-02-12T21:11:04Z

Description

This patch series primarily deals with support for the Firmware Handoff specification.
This includes:

Added utilities to ArmTransferListLib to check validity of a TL and dump its contents.
Define and add PcdTransferListBaseAddress to store the base address of the TL coming in on register x3 if the TL signature is detected.
Define a TlHobGuid to hold the base address.
Build the TlHobGuid if a valid TransferList is present.
Added functionality to ShellPkg to display the installed DT address via the dmem utility.

How This Was Tested

Tested in Base FVP RevC platform by checking Boot, dumping the TL contents and verifying the DT address using dmem.

Integration Instructions

N/A

ArmPkg/Library/ArmTransferListLib/ArmTransferListLib.c

pierregondois · 2025-02-17T15:28:50Z

ArmPkg/Library/ArmTransferListLib/ArmTransferListLib.c

+    return TL_OPS_INVALID;
+  }
+
+  if (Tlh->TotalSize <= 0) {


TotalSize is unsigned, so I don't think it is useful to check against negative numbers.
Maybe it should be better to check that:

if (Tlh->TotalSize < Tlh->HeaderSize) { // error }

NIT: https://github.com/FirmwareHandoff/firmware_handoff/blob/main/source/transfer_list.rst states that used_size and total_size must be multiple of 8

Changed to check if TotalSize is non-zero.

ArmPlatformPkg/ArmFvpPlatformDxe/ArmFvpPlatformDxe.c

ArmPlatformPkg/PlatformPei/PlatformPeiLib.c

ArmPlatformPkg/ArmFvpPlatformDxe/ArmFvpPlatformDxe.c

ArmPlatformPkg/PlatformPei/PlatformPeiLib.c

pierregondois · 2025-02-21T14:33:00Z

@LeviYeoReum do you have any comment as you know more about the ArmTransferListLib ?

leiflindholm · 2025-03-04T17:12:55Z

ArmPlatformPkg/ArmFvpPlatformDxe/ArmFvpPlatformDxe.c

@@ -0,0 +1,68 @@
+/** @file


This code belongs in edk2-platforms, not edk2.

leiflindholm · 2025-03-04T17:15:38Z

ArmPlatformPkg/ArmFvpPlatformDxe/ArmFvpPlatformDxe.c

+#include <Library/ArmTransferListLib.h>
+#include <Guid/TlHob.h>
+#include <Guid/FdtHob.h>
+#include <libfdt.h>


And even if in edk2-platforms, https://github.com/tianocore/edk2/tree/master/MdePkg/Library/BaseFdtLib wrapper should be used. The broken EmbeddedPkg version is way overdue deletion.

leiflindholm · 2025-03-04T17:20:33Z

ArmPlatformPkg/PlatformPei/PlatformPeiLib.c

@@ -1,6 +1,6 @@
 /** @file

-  Copyright (c) 2011-2014, ARM Limited. All rights reserved.
+  Copyright (c) 2011-2024, ARM Limited. All rights reserved.


This module is used by a whole lot of platforms. And this change breaks all of them.
Meanwhile, the change is not currently relevant for almost any of them.

While that may change in the future, this needs to be introduced in a way that lets platforms opt into it.

Thanks for the comments. Is there a specific reason you think this change breaks other platforms? This was written in a way to be backward compatible even on platforms that didn't support transfer list.

I think the problem is that the PlatformPeiLib change should have been a separate patch, with a clear commit message stating how PcdTransferListBaseAddress is initialised and what is the impact on platforms that do not support transfer list.

It adds a dependency on libfdt, which not all platforms use and hence have an entry for in their .dsc.

It dereferences memory at whatever address the default PcdTransferListBaseAddress happens to be if not explicitly set. Even if that happens not to crash a given platform, that is still invalid behaviour.

leiflindholm · 2025-03-04T17:25:02Z

ShellPkg/Library/UefiShellDebug1CommandsLib/Dmem.c

@@ -491,6 +491,11 @@ ShellCommandRunDmem (
              continue;


commit message: I don't think the "Fixes:" tag is appropriate here.

leiflindholm · 2025-03-04T17:27:42Z

ArmPkg/Include/IndustryStandard/ArmTransferList.h

+ * Operation codes indicating the validity of the Transfer List.
+ */
+typedef enum {
+  TL_OPS_INVALID,  /* invalid for any operation */


A lot of the naming in this set clashes with both edk2 coding style and with existing code adjacent.

TL_ -> TRANSFER_LIST_

leiflindholm · 2025-03-04T17:28:22Z

ArmPkg/Include/Library/ArmTransferListLib.h

+**/
+VOID
+EFIAPI
+TlDump (


TransferListDump (and so on for other function names).

leiflindholm · 2025-03-04T17:29:25Z

ArmPkg/Include/Library/ArmTransferListLib.h

+/**
+  Dump the transfer list to the debug output.
+
+  @param [in]   Tlh       TransferListHeader


TransferListHeader, and the description "Transfer list header" (and so on for other arguments)

leiflindholm · 2025-03-04T17:30:12Z

ArmPkg/Include/Library/ArmTransferListLib.h

+/**
+  Find a Transfer Entry Node in the Transfer List matched with the given tag-id.
+
+  @param [in]   Tlh       Pointer to the Transfer List Header


This one gets the description right, "Pointer to the Transfer List Header". Use that description throughout.

ArmPkg/Library/ArmTransferListLib/ArmTransferListLib.c

samimujawar · 2025-03-13T17:24:23Z

ArmPlatformPkg/ArmPlatformPkg.dec

@@ -119,3 +119,6 @@
  gArmPlatformTokenSpaceGuid.PcdPL031RtcPpmAccuracy|300000000|UINT32|0x00000022

  gArmPlatformTokenSpaceGuid.PcdWatchdogCount|0x0|UINT32|0x00000033
+
+[PcdsPatchableInModule]


Is it possible to add a comment to clarify that a platform DSC must not populate this value, and that this value will be updated by the initial startup code by discovering if Transfer List is supported, please?

samimujawar · 2025-03-13T17:31:41Z

ArmPlatformPkg/PeilessSec/AArch64/ModuleEntryPoint.S

@@ -8,6 +8,9 @@
 #include <AsmMacroLib.h>

 ASM_FUNC(_ModuleEntryPoint)
+  // Set the TransferList Base Address from register x3


Is it possible to check that we have a valid Transfer List as per the handoff spec, see https://firmwarehandoff.github.io/firmware_handoff/main/register_conventions.html#aarch64-receiver ?
i.e. X1[31:0]: set to the TL signature (4a0f_b10b),
X1[39:32]: version of the register convention used. Set to 1 for the AArch64 convention specified in this document.
X1[63:40]: reserved, must be zero
Register X2 must be set to 0

That way we will be confident that we have a valid Transfer list, before we initialise PcdTransferListBaseAddress.

Also it will be good to clarify in the commit message, what is the impact for platforms that do not support transfer list, and how do they cope with this addition?

samimujawar · 2025-03-13T17:35:51Z

ArmPkg/ArmPkg.dec

@@ -104,6 +104,7 @@
  gArmMpCoreInfoGuid = { 0xa4ee0728, 0xe5d7, 0x4ac5,  {0xb2, 0x1e, 0x65, 0x8e, 0xd8, 0x57, 0xe8, 0x34} }

  gArmMmuReplaceLiveTranslationEntryFuncGuid = { 0xa8b50ff3, 0x08ec, 0x4dd3, {0xbf, 0x04, 0x28, 0xbf, 0x71, 0x75, 0xc7, 0x4a} }
+  gTlHobGuid = { 0xebe7bae8, 0xfe18, 0x43c5, { 0xbf, 0x3f, 0xf2, 0xb1, 0xaf, 0xb2, 0xdf, 0xb8 } }


gTlHobGuid => gTransferListHobGuid

(also in commit message)

And really, the filename too.

samimujawar · 2025-03-13T17:38:56Z

ArmPlatformPkg/ArmFvpPlatformDxe/ArmFvpPlatformDxe.c

+  IN EFI_SYSTEM_TABLE  *SystemTable
+  )
+{
+  VOID  *Hob;


Instead of this Dxe, would it be better to implement a library like https://github.com/tianocore/edk2-platforms/blob/master/Platform/ARM/VExpressPkg/Library/ArmVExpressDtPlatformDtbLoaderLib/ArmVExpressDtPlatformDtbLoaderLib.c that implements the DtPlatformLoadDtb() which is consumed by https://github.com/tianocore/edk2/blob/master/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c

Also, that library would need to be in edk2-platforms repo.

samimujawar · 2025-03-13T17:45:23Z

ArmPlatformPkg/PlatformPei/PlatformPeiLib.c

@@ -1,6 +1,6 @@
 /** @file

-  Copyright (c) 2011-2014, ARM Limited. All rights reserved.
+  Copyright (c) 2011-2024, ARM Limited. All rights reserved.


I think the problem is that the PlatformPeiLib change should have been a separate patch, with a clear commit message stating how PcdTransferListBaseAddress is initialised and what is the impact on platforms that do not support transfer list.

samimujawar · 2025-03-13T17:59:37Z

ArmPlatformPkg/PlatformPei/PlatformPeiLib.c

+  UINT64                 *FdtHobData;
+  VOID                   *NewBase;
+
+  TransferListBase = (VOID *)(UINTN)PcdGet64 (PcdTransferListBaseAddress);


I think this code should be moved in a separate functions and comments should be added, e.g.
// The early startup code verifies if a handoff list has been
// passed by the Sender (on arm platforms this would be TF-A).
// On detecting a valid Transfer List the early startup code
// initialises PcdTransferListBaseAddress with the address
// where the transfer list is stored in memory.
// Check if a transfer list is present and install the
// gTransferListHobGuid and gFdtHobGuid.
if (PcdGet64 (PcdTransferListBaseAddress) !=0) {
// 1. install the gTransferListHobGuid
InstallTransferListHob();
// 2. search the transfer list to see if a DTB is present and build the gFdtHobGuid
InitialiseFdtHobFromTransferList();
}

@leiflindholm please let us know if the suggested approach would work, or you think it would be better to introduce a new PlatformPeiLib that handles the Transfer List.

Added functionality TransferList Library TlVerifyChecksum - Verify TL CheckSum for correctness TlCheckHeader - Check if TL header is valid, return suitable opcodes validating the TL Header TlFindEntry - Find a specific entry on the TL using the TagId TlDump - Dump the contents of the Tl Header and the entry headers Signed-off-by: Prachotan Reddy Bathi <[email protected]>

Introducing TransferList to FVP Initialize PcdTransferListBaseAddress Capture tl address from register x3 Refer to section 3 of the FW Handoff Specification https://firmwarehandoff.github.io/firmware_handoff The TL header is present at the base address captured by this Pcd. Signed-off-by: Prachotan Reddy Bathi <[email protected]>

TlGuid HOB will hold the TransferList base address https://firmwarehandoff.github.io/firmware_handoff Signed-off-by: Prachotan Reddy Bathi <[email protected]>

TlGuid HOB holds TransferList(TL) base address If there's no valid TransferList found, Guid HOB is not built, boot progresses as usual. Signed-off-by: Prachotan Bathi <[email protected]>

DTB address can be modified through the config table. Use this address in dmem output. EmbeddedPkg dependency added to ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.inf and ShellPkg/ShellPkg.ci.yaml Fixes: 42b0443 ("ShellPkg: UefiShellDebug1CommandsLib: Uefi Config Tables in Dmem.c") Signed-off-by: Prachotan Reddy Bathi <[email protected]>

leiflindholm · 2025-03-18T12:02:17Z

ArmPkg/Library/ArmTransferListLib/ArmTransferListLib.c

+  IN UINT16                TagId
+  )
+{
+  TRANSFER_ENTRY_HEADER  *Te = NULL;


In the existing code I reworked, I renamed this local variable "Entry" in all functions. Can you please do the same in order to maintain alignment across the file?

leiflindholm · 2025-03-18T12:03:02Z

ArmPkg/Include/IndustryStandard/ArmTransferList.h

@@ -2,7 +2,7 @@
  Header file defining a Transfer List and Transfer Entry as specified by the
  A-profile Firmware Handoff Protocol specification.

-  Copyright (c) 2024, Arm Limited. All rights reserved.<BR>
+  Copyright (c) 2025, Arm Limited. All rights reserved.<BR>


Thanks a lot for that rework. However, could you update the commit message to match?

leiflindholm · 2025-03-18T12:13:24Z

ArmPlatformPkg/PeilessSec/AArch64/ModuleEntryPoint.S

+  adr   x4, PcdGet64(PcdTransferListBaseAddress)
+  str   x3, [x4]
+
+_SetSVCMode:


This is an ominous-looking change at first. But also kind of nonsensical (AArch64 doesn't have an SVC mode).

@ardbiesheuvel this is clearly a hangaround from the original of this file being based on the AArch32 version. But is that even a valid thing to do there anymore, or is this a hangaround of "change out of Secure Monitor Mode"?

I feel like we should have a separate commit deleting this label, at least on AArch64, and figure out a new label name here.

PrachotanReddy force-pushed the handoffupstream branch 3 times, most recently from daa8d95 to f34280f Compare February 13, 2025 21:09

PrachotanReddy marked this pull request as ready for review February 13, 2025 21:42

tianocore-assign-reviewers bot requested review from ajfish, ardbiesheuvel, leiflindholm, makubacki, mdkinney, pierregondois and samimujawar February 13, 2025 21:42

pierregondois reviewed Feb 17, 2025

View reviewed changes

PrachotanReddy force-pushed the handoffupstream branch 3 times, most recently from ab43eef to 1f5f434 Compare February 18, 2025 20:22

leiflindholm reviewed Mar 4, 2025

View reviewed changes

ArmPkg/Library/ArmTransferListLib/ArmTransferListLib.c Show resolved Hide resolved

samimujawar requested changes Mar 13, 2025

View reviewed changes

PrachotanReddy and others added 4 commits March 17, 2025 17:42

ArmPkg: Add TlHob Guid

3350ae6

TlGuid HOB will hold the TransferList base address https://firmwarehandoff.github.io/firmware_handoff Signed-off-by: Prachotan Reddy Bathi <[email protected]>

ArmPlatformPkg: Introduce TL Guid Hob

aebe71c

TlGuid HOB holds TransferList(TL) base address If there's no valid TransferList found, Guid HOB is not built, boot progresses as usual. Signed-off-by: Prachotan Bathi <[email protected]>

PrachotanReddy force-pushed the handoffupstream branch from fd4a544 to 06a53f2 Compare March 17, 2025 22:46

PrachotanReddy changed the title ~~ArmTransferListLib added utility functions and support for FVP~~ ArmTransferListLib added utility functions and support to capture the TL base address Mar 17, 2025

PrachotanReddy marked this pull request as draft March 17, 2025 22:57

leiflindholm reviewed Mar 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ArmTransferListLib added utility functions and support to capture the TL base address #10759

ArmTransferListLib added utility functions and support to capture the TL base address #10759

PrachotanReddy commented Feb 12, 2025 •

edited

Loading

pierregondois Feb 17, 2025

PrachotanReddy Feb 19, 2025

pierregondois commented Feb 21, 2025

leiflindholm Mar 4, 2025

leiflindholm Mar 4, 2025

leiflindholm Mar 4, 2025

PrachotanReddy Mar 5, 2025

samimujawar Mar 13, 2025

leiflindholm Mar 13, 2025

leiflindholm Mar 4, 2025

leiflindholm Mar 4, 2025

leiflindholm Mar 4, 2025

leiflindholm Mar 4, 2025

leiflindholm Mar 4, 2025

samimujawar Mar 13, 2025

samimujawar Mar 13, 2025

samimujawar Mar 13, 2025

leiflindholm Mar 18, 2025

leiflindholm Mar 18, 2025

samimujawar Mar 13, 2025

samimujawar Mar 13, 2025

samimujawar Mar 13, 2025

samimujawar Mar 13, 2025

samimujawar Mar 13, 2025

leiflindholm Mar 18, 2025

leiflindholm Mar 18, 2025

leiflindholm Mar 18, 2025

ArmTransferListLib added utility functions and support to capture the TL base address #10759

Are you sure you want to change the base?

ArmTransferListLib added utility functions and support to capture the TL base address #10759

Conversation

PrachotanReddy commented Feb 12, 2025 • edited Loading

Description

How This Was Tested

Integration Instructions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierregondois commented Feb 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PrachotanReddy commented Feb 12, 2025 •

edited

Loading