-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
base: master
Are you sure you want to change the base?
Conversation
daa8d95
to
f34280f
Compare
return TL_OPS_INVALID; | ||
} | ||
|
||
if (Tlh->TotalSize <= 0) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
ab43eef
to
1f5f434
Compare
@LeviYeoReum do you have any comment as you know more about the ArmTransferListLib ? |
@@ -0,0 +1,68 @@ | |||
/** @file |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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 */ |
There was a problem hiding this comment.
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 ( |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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] |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 } } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gTlHobGuid => gTransferListHobGuid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(also in commit message)
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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();
}
There was a problem hiding this comment.
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.
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]>
fd4a544
to
06a53f2
Compare
IN UINT16 TagId | ||
) | ||
{ | ||
TRANSFER_ENTRY_HEADER *Te = NULL; |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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.
Description
This patch series primarily deals with support for the Firmware Handoff specification.
This includes:
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