Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

PrachotanReddy
Copy link
Contributor

@PrachotanReddy PrachotanReddy commented Feb 12, 2025

Description

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

  1. Added utilities to ArmTransferListLib to check validity of a TL and dump its contents.
  2. Define and add PcdTransferListBaseAddress to store the base address of the TL coming in on register x3 if the TL signature is detected.
  3. Define a TlHobGuid to hold the base address.
  4. Build the TlHobGuid if a valid TransferList is present.
  5. 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

@PrachotanReddy PrachotanReddy force-pushed the handoffupstream branch 3 times, most recently from daa8d95 to f34280f Compare February 13, 2025 21:09
@PrachotanReddy PrachotanReddy marked this pull request as ready for review February 13, 2025 21:42
return TL_OPS_INVALID;
}

if (Tlh->TotalSize <= 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to check if TotalSize is non-zero.

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

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

@@ -0,0 +1,68 @@
/** @file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code belongs in edk2-platforms, not edk2.

#include <Library/ArmTransferListLib.h>
#include <Guid/TlHob.h>
#include <Guid/FdtHob.h>
#include <libfdt.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Copyright (c) 2011-2014, ARM Limited. All rights reserved.
Copyright (c) 2011-2024, ARM Limited. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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.

@@ -491,6 +491,11 @@ ShellCommandRunDmem (
continue;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

TL_ -> TRANSFER_LIST_

**/
VOID
EFIAPI
TlDump (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TransferListDump (and so on for other function names).

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

gArmPlatformTokenSpaceGuid.PcdWatchdogCount|0x0|UINT32|0x00000033

[PcdsPatchableInModule]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

ASM_FUNC(_ModuleEntryPoint)
// Set the TransferList Base Address from register x3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@@ -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 } }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gTlHobGuid => gTransferListHobGuid

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(also in commit message)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And really, the filename too.

IN EFI_SYSTEM_TABLE *SystemTable
)
{
VOID *Hob;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copyright (c) 2011-2014, ARM Limited. All rights reserved.
Copyright (c) 2011-2024, ARM Limited. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

UINT64 *FdtHobData;
VOID *NewBase;

TransferListBase = (VOID *)(UINTN)PcdGet64 (PcdTransferListBaseAddress);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

PrachotanReddy and others added 4 commits March 17, 2025 17:42
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]>
@PrachotanReddy 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 PrachotanReddy marked this pull request as draft March 17, 2025 22:57
IN UINT16 TagId
)
{
TRANSFER_ENTRY_HEADER *Te = NULL;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@@ -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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

adr x4, PcdGet64(PcdTransferListBaseAddress)
str x3, [x4]

_SetSVCMode:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants