* [edk2-platforms][PATCH 0/2] Rip out "internal DTB" support in FdtDxe
@ 2020-04-30 21:16 Andrei Warkentin
2020-04-30 21:16 ` [edk2-platforms][PATCH 1/2] RPi: rip out FdtDxe logic to use internal DTB Andrei Warkentin
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Andrei Warkentin @ 2020-04-30 21:16 UTC (permalink / raw)
To: devel; +Cc: ard.biesheuvel, leif, pete, philmd
Dear all,
This patch set removes the internal (embedded) DTB support from Pi 3
and Pi 4 platforms, since these platforms should *always* be using
the devicetree passed by the VideoCore firmware.
Andrei Warkentin (2):
RPi: rip out FdtDxe logic to use internal DTB
RPi: remove any mention of an "internal DTB"
Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c | 206 ++++--------------
.../RaspberryPi/Drivers/FdtDxe/FdtDxe.inf | 4 -
Platform/RaspberryPi/RPi3/RPi3.fdf | 11 -
Platform/RaspberryPi/RPi3/Readme.md | 3 +-
Platform/RaspberryPi/RPi4/RPi4.fdf | 8 -
Platform/RaspberryPi/RPi4/Readme.md | 3 +-
Platform/RaspberryPi/RaspberryPi.dec | 7 -
7 files changed, 40 insertions(+), 202 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 12+ messages in thread* [edk2-platforms][PATCH 1/2] RPi: rip out FdtDxe logic to use internal DTB 2020-04-30 21:16 [edk2-platforms][PATCH 0/2] Rip out "internal DTB" support in FdtDxe Andrei Warkentin @ 2020-04-30 21:16 ` Andrei Warkentin 2020-05-01 10:45 ` Pete Batard 2020-05-01 13:19 ` Ard Biesheuvel 2020-04-30 21:16 ` [edk2-platforms][PATCH 2/2] RPi: remove any mention of an "internal DTB" Andrei Warkentin 2020-05-01 17:08 ` [edk2-platforms][PATCH 0/2] Rip out "internal DTB" support in FdtDxe Ard Biesheuvel 2 siblings, 2 replies; 12+ messages in thread From: Andrei Warkentin @ 2020-04-30 21:16 UTC (permalink / raw) To: devel; +Cc: ard.biesheuvel, leif, pete, philmd Initially, FdtDxe used an internal (embedded in UEFI) FDT, because it was neither understood how to consume the one loaded by the VideoCore firmware, nor understood just how important it is to use the DTB provided by config.txt. Embedding the DT was a bad idea, because: - Permanently stale - No overlays Also, on devices like the Pi 4 you _have_ to have a DT around for the start4 VPU firmware to pick up, otherwise the board is left in an inconsistent state. So we're being proscriptive now about DT use with config.txt, which means this internal DT logic is deadc code. Further FdtDxe cleanups are possible and will be handled separately, specifically: - probably no need to use a separate allocation for patched DT (optimize memory used) - suspicious use of EfiBootServicesData (I filed https://github.com/ARM-software/ebbr/issues/45 to sort out the real requirements) Testing: Booted Ubuntu 18.04 on Pi 2B (1.2). Signed-off-by: Andrei Warkentin <andrey.warkentin@gmail.com> --- Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c | 206 ++++---------------- Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf | 4 - Platform/RaspberryPi/RPi3/RPi3.fdf | 11 -- Platform/RaspberryPi/RPi4/RPi4.fdf | 8 - Platform/RaspberryPi/RaspberryPi.dec | 7 - 5 files changed, 38 insertions(+), 198 deletions(-) diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c index e7143f57..187b9566 100644 --- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c +++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c @@ -335,90 +335,6 @@ CleanSimpleFramebuffer ( return EFI_SUCCESS; } -#define MAX_CMDLINE_SIZE 512 - -STATIC -EFI_STATUS -UpdateBootArgs ( - VOID - ) -{ - INTN Node; - INTN Retval; - EFI_STATUS Status; - CHAR8 *CommandLine; - - // - // Locate the /chosen node - // - Node = fdt_path_offset (mFdtImage, "/chosen"); - if (Node < 0) { - DEBUG ((DEBUG_ERROR, "%a: failed to locate /chosen node\n", __FUNCTION__)); - return EFI_NOT_FOUND; - } - - // - // If /chosen/bootargs already exists, we want to add a space character - // before adding the firmware supplied arguments. However, the RpiFirmware - // protocol expects a 32-bit aligned buffer. So let's allocate 4 bytes of - // slack, and skip the first 3 when passing this buffer into libfdt. - // - CommandLine = AllocatePool (MAX_CMDLINE_SIZE) + 4; - if (!CommandLine) { - DEBUG ((DEBUG_ERROR, "%a: failed to allocate memory\n", __FUNCTION__)); - return EFI_OUT_OF_RESOURCES; - } - - // - // Get the command line from the firmware - // - Status = mFwProtocol->GetCommandLine (MAX_CMDLINE_SIZE, CommandLine + 4); - if (EFI_ERROR (Status)) { - DEBUG ((DEBUG_ERROR, "%a: failed to retrieve command line\n", __FUNCTION__)); - return Status; - } - - if (AsciiStrLen (CommandLine + 4) == 0) { - DEBUG ((DEBUG_INFO, "%a: empty command line received\n", __FUNCTION__)); - return EFI_SUCCESS; - } - - CommandLine[3] = ' '; - - Retval = fdt_appendprop_string (mFdtImage, Node, "bootargs", &CommandLine[3]); - if (Retval != 0) { - DEBUG ((DEBUG_ERROR, "%a: failed to set /chosen/bootargs property (%d)\n", - __FUNCTION__, Retval)); - } - - DEBUG_CODE_BEGIN (); - CONST CHAR8 *Prop; - INT32 Length; - INT32 Index; - - Node = fdt_path_offset (mFdtImage, "/chosen"); - ASSERT (Node >= 0); - - Prop = fdt_getprop (mFdtImage, Node, "bootargs", &Length); - ASSERT (Prop != NULL); - - DEBUG ((DEBUG_INFO, "Command line set from firmware (length %d):\n'", Length)); - - for (Index = 0; Index < Length; Index++, Prop++) { - if (*Prop == '\0') { - continue; - } - DEBUG ((DEBUG_INFO, "%c", *Prop)); - } - - DEBUG ((DEBUG_INFO, "'\n")); - DEBUG_CODE_END (); - - FreePool (CommandLine - 4); - return EFI_SUCCESS; -} - - /** @param ImageHandle of the loaded driver @param SystemTable Pointer to the System Table @@ -435,13 +351,10 @@ FdtDxeInitialize ( IN EFI_SYSTEM_TABLE *SystemTable ) { + INT32 Retval; EFI_STATUS Status; - EFI_GUID *FdtGuid; - VOID *FdtImage; UINTN FdtSize; - INT32 Retval; - UINT32 BoardRevision; - BOOLEAN Internal; + VOID *FdtImage = NULL; if (PcdGet32 (PcdOptDeviceTree) == 0) { DEBUG ((DEBUG_INFO, "Device Tree disabled per user configuration\n")); @@ -450,77 +363,28 @@ FdtDxeInitialize ( Status = gBS->LocateProtocol (&gRaspberryPiFirmwareProtocolGuid, NULL, (VOID**)&mFwProtocol); - ASSERT_EFI_ERROR (Status); + if (mFwProtocol == NULL) { + /* + * Impossible because of the depex... + */ + ASSERT_EFI_ERROR (Status); + return EFI_NOT_FOUND; + } - Internal = FALSE; FdtImage = (VOID*)(UINTN)PcdGet32 (PcdFdtBaseAddress); Retval = fdt_check_header (FdtImage); - if (Retval == 0) { - /* - * Have FDT passed via config.txt. - */ - FdtSize = fdt_totalsize (FdtImage); - DEBUG ((DEBUG_INFO, "Device Tree passed via config.txt (0x%lx bytes)\n", FdtSize)); - Status = EFI_SUCCESS; - } else { - /* - * Use one of the embedded FDT's. - */ - Internal = TRUE; - DEBUG ((DEBUG_INFO, "No/Bad Device Tree found at address 0x%p (%a), " - "looking up internal one...\n", FdtImage, fdt_strerror (Retval))); + if (Retval != 0) { /* - * Query the board revision to differentiate between models. + * Any one of: + * - Invalid config.txt device_tree_address (not PcdFdtBaseAddress) + * - Missing FDT for your Pi variant (if not overriding via device_tree=) */ - Status = mFwProtocol->GetModelRevision (&BoardRevision); - if (EFI_ERROR (Status)) { - DEBUG ((DEBUG_ERROR, "Failed to get board type: %r\n", Status)); - DEBUG ((DEBUG_INFO, "Using default internal Device Tree\n")); - FdtGuid = &gRaspberryPiDefaultFdtGuid; - } else { - // www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md - switch ((BoardRevision >> 4) & 0xFF) { - case 0x08: - DEBUG ((DEBUG_INFO, "Using Raspberry Pi 3 Model B internal Device Tree\n")); - FdtGuid = &gRaspberryPi3ModelBFdtGuid; - break; - case 0x0D: - DEBUG ((DEBUG_INFO, "Using Raspberry Pi 3 Model B+ internal Device Tree\n")); - FdtGuid = &gRaspberryPi3ModelBPlusFdtGuid; - break; - case 0x11: - DEBUG ((DEBUG_INFO, "Using Raspberry Pi 4 Model B internal Device Tree\n")); - FdtGuid = &gRaspberryPi4ModelBFdtGuid; - break; - default: - DEBUG ((DEBUG_INFO, "Using default internal Device Tree\n")); - FdtGuid = &gRaspberryPiDefaultFdtGuid; - break; - } - } - do { - Status = GetSectionFromAnyFv (FdtGuid, EFI_SECTION_RAW, 0, &FdtImage, &FdtSize); - if (Status == EFI_SUCCESS) { - if (fdt_check_header (FdtImage) != 0) { - Status = EFI_INCOMPATIBLE_VERSION; - } - } - // No retry needed if we are successful or are dealing with the default Fdt. - if ( (Status == EFI_SUCCESS) || - (CompareGuid (FdtGuid, &gRaspberryPiDefaultFdtGuid)) ) - break; - // Otherwise, try one more time with the default Fdt. An example of this - // is if we detected a non-default Fdt, that isn't included in the FDF. - DEBUG ((DEBUG_INFO, "Internal Device Tree was not found for this platform, " - "falling back to default...\n")); - FdtGuid = &gRaspberryPiDefaultFdtGuid; - } while (1); + DEBUG ((DEBUG_ERROR, "No devicetree passed via config.txt\n")); + return EFI_NOT_FOUND; } - if (EFI_ERROR (Status)) { - DEBUG ((DEBUG_ERROR, "Failed to locate Device Tree: %r\n", Status)); - return Status; - } + FdtSize = fdt_totalsize (FdtImage); + DEBUG ((DEBUG_INFO, "Devicetree passed via config.txt (0x%lx bytes)\n", FdtSize)); /* * Probably overkill. @@ -529,12 +393,19 @@ FdtDxeInitialize ( Status = gBS->AllocatePages (AllocateAnyPages, EfiBootServicesData, EFI_SIZE_TO_PAGES (FdtSize), (EFI_PHYSICAL_ADDRESS*)&mFdtImage); if (EFI_ERROR (Status)) { - DEBUG ((DEBUG_ERROR, "Failed to allocate Device Tree: %r\n", Status)); - return Status; + DEBUG ((DEBUG_ERROR, "Failed to allocate devicetree: %r\n", Status)); + goto out; } Retval = fdt_open_into (FdtImage, mFdtImage, FdtSize); - ASSERT (Retval == 0); + if (Retval != 0) { + DEBUG ((DEBUG_ERROR, "fdt_open_into failed: %d\n", Retval)); + goto out; + } + + /* + * These are all best-effort. + */ Status = SanitizePSCI (); if (EFI_ERROR (Status)) { @@ -566,19 +437,18 @@ FdtDxeInitialize ( Print (L"Failed to update USB compatible properties: %r\n", Status); } - if (Internal) { - /* - * A GPU-provided DTB already has the full command line. - */ - Status = UpdateBootArgs (); - if (EFI_ERROR (Status)) { - Print (L"Failed to update boot arguments: %r\n", Status); - } - } - - DEBUG ((DEBUG_INFO, "Installed Device Tree at address 0x%p\n", mFdtImage)); + DEBUG ((DEBUG_INFO, "Installed devicetree at address %p\n", mFdtImage)); Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mFdtImage); - ASSERT_EFI_ERROR (Status); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "Couldn't register devicetree: %r\n", Status)); + goto out; + } +out: + if (EFI_ERROR(Status)) { + if (mFdtImage != NULL) { + gBS->FreePages ((EFI_PHYSICAL_ADDRESS) mFdtImage, EFI_SIZE_TO_PAGES (FdtSize)); + } + } return Status; } diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf index fc37353f..49ba5ba1 100644 --- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf +++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf @@ -35,10 +35,6 @@ [Guids] gFdtTableGuid - gRaspberryPi3ModelBFdtGuid - gRaspberryPi3ModelBPlusFdtGuid - gRaspberryPi4ModelBFdtGuid - gRaspberryPiDefaultFdtGuid [Protocols] gRaspberryPiFirmwareProtocolGuid ## CONSUMES diff --git a/Platform/RaspberryPi/RPi3/RPi3.fdf b/Platform/RaspberryPi/RPi3/RPi3.fdf index daedc443..e854cd21 100644 --- a/Platform/RaspberryPi/RPi3/RPi3.fdf +++ b/Platform/RaspberryPi/RPi3/RPi3.fdf @@ -303,17 +303,6 @@ READ_LOCK_STATUS = TRUE # INF Platform/RaspberryPi/Drivers/LogoDxe/LogoDxe.inf - # - # Device Tree support (used by FdtDxe) - # GUIDs should match gRaspberryPi#####FdtGuid's from the .dec - # - FILE FREEFORM = DF5DA223-1D27-47C3-8D1B-9A41B55A18BC { - SECTION RAW = Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2710-rpi-3-b.dtb - } - FILE FREEFORM = 3D523012-73FE-40E5-892E-1A4DF60F3C0C { - SECTION RAW = Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2710-rpi-3-b-plus.dtb - } - [FV.FVMAIN_COMPACT] FvAlignment = 16 ERASE_POLARITY = 1 diff --git a/Platform/RaspberryPi/RPi4/RPi4.fdf b/Platform/RaspberryPi/RPi4/RPi4.fdf index c3e9cfc4..b1f7aa23 100644 --- a/Platform/RaspberryPi/RPi4/RPi4.fdf +++ b/Platform/RaspberryPi/RPi4/RPi4.fdf @@ -307,14 +307,6 @@ READ_LOCK_STATUS = TRUE # INF Platform/RaspberryPi/Drivers/LogoDxe/LogoDxe.inf - # - # Device Tree support (used by FdtDxe) - # GUIDs should match gRaspberryPi#####FdtGuid's from the .dec - # - FILE FREEFORM = 80AB6833-CAE4-4CEE-B59D-EB2039B05551 { - SECTION RAW = Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2711-rpi-4-b.dtb - } - [FV.FVMAIN_COMPACT] FvAlignment = 16 ERASE_POLARITY = 1 diff --git a/Platform/RaspberryPi/RaspberryPi.dec b/Platform/RaspberryPi/RaspberryPi.dec index b66322be..66ef6186 100644 --- a/Platform/RaspberryPi/RaspberryPi.dec +++ b/Platform/RaspberryPi/RaspberryPi.dec @@ -26,13 +26,6 @@ gRaspberryPiTokenSpaceGuid = {0xCD7CC258, 0x31DB, 0x11E6, {0x9F, 0xD3, 0x63, 0xB0, 0xB8, 0xEE, 0xD6, 0xB5}} gRaspberryPiEventResetGuid = {0xCD7CC258, 0x31DB, 0x11E6, {0x9F, 0xD3, 0x63, 0xB4, 0xB4, 0xE4, 0xD4, 0xB4}} gConfigDxeFormSetGuid = {0xCD7CC258, 0x31DB, 0x22E6, {0x9F, 0x22, 0x63, 0xB0, 0xB8, 0xEE, 0xD6, 0xB5}} - # GUIDs used by FdtDxe to serve a Device Tree at runtime. Not all of these need to apply - # to the current platform or match an actual FDF binary, but they need to be defined. - gRaspberryPi3ModelBFdtGuid = { 0xDF5DA223, 0x1D27, 0x47C3, { 0x8D, 0x1B, 0x9A, 0x41, 0xB5, 0x5A, 0x18, 0xBC } } - gRaspberryPi3ModelBPlusFdtGuid = { 0x3D523012, 0x73FE, 0x40E5, { 0x89, 0x2E, 0x1A, 0x4D, 0xF6, 0x0F, 0x3C, 0x0C } } - gRaspberryPi4ModelBFdtGuid = { 0x80AB6833, 0xCAE4, 0x4CEE, { 0xB5, 0x9D, 0xEB, 0x20, 0x39, 0xB0, 0x55, 0x51 } } - # Default Fdt to serve if the hardware model can't be detected. Should match one of the above. - gRaspberryPiDefaultFdtGuid = {0xDF5DA223, 0x1D27, 0x47C3, { 0x8D, 0x1B, 0x9A, 0x41, 0xB5, 0x5A, 0x18, 0xBC}} [PcdsFixedAtBuild.common] # -- 2.17.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [edk2-platforms][PATCH 1/2] RPi: rip out FdtDxe logic to use internal DTB 2020-04-30 21:16 ` [edk2-platforms][PATCH 1/2] RPi: rip out FdtDxe logic to use internal DTB Andrei Warkentin @ 2020-05-01 10:45 ` Pete Batard 2020-05-01 13:19 ` Ard Biesheuvel 1 sibling, 0 replies; 12+ messages in thread From: Pete Batard @ 2020-05-01 10:45 UTC (permalink / raw) To: Andrei Warkentin, devel; +Cc: ard.biesheuvel, leif, philmd Two non-blocking minor remarks inline: On 2020.04.30 22:16, Andrei Warkentin wrote: > Initially, FdtDxe used an internal (embedded in UEFI) FDT, > because it was neither understood how to consume the > one loaded by the VideoCore firmware, nor understood just > how important it is to use the DTB provided by config.txt. > > Embedding the DT was a bad idea, because: > - Permanently stale > - No overlays > > Also, on devices like the Pi 4 you _have_ to have a DT > around for the start4 VPU firmware to pick up, otherwise > the board is left in an inconsistent state. So we're being > proscriptive now about DT use with config.txt, which means > this internal DT logic is deadc code. > > Further FdtDxe cleanups are possible and will be handled > separately, specifically: > - probably no need to use a separate allocation for patched DT (optimize memory used) > - suspicious use of EfiBootServicesData (I filed https://github.com/ARM-software/ebbr/issues/45 to sort out the real requirements) > > Testing: Booted Ubuntu 18.04 on Pi 2B (1.2). > > Signed-off-by: Andrei Warkentin <andrey.warkentin@gmail.com> > --- > Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c | 206 ++++---------------- > Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf | 4 - > Platform/RaspberryPi/RPi3/RPi3.fdf | 11 -- > Platform/RaspberryPi/RPi4/RPi4.fdf | 8 - > Platform/RaspberryPi/RaspberryPi.dec | 7 - > 5 files changed, 38 insertions(+), 198 deletions(-) > > diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c > index e7143f57..187b9566 100644 > --- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c > +++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c > @@ -335,90 +335,6 @@ CleanSimpleFramebuffer ( > return EFI_SUCCESS; > } > > -#define MAX_CMDLINE_SIZE 512 > - > -STATIC > -EFI_STATUS > -UpdateBootArgs ( > - VOID > - ) > -{ > - INTN Node; > - INTN Retval; > - EFI_STATUS Status; > - CHAR8 *CommandLine; > - > - // > - // Locate the /chosen node > - // > - Node = fdt_path_offset (mFdtImage, "/chosen"); > - if (Node < 0) { > - DEBUG ((DEBUG_ERROR, "%a: failed to locate /chosen node\n", __FUNCTION__)); > - return EFI_NOT_FOUND; > - } > - > - // > - // If /chosen/bootargs already exists, we want to add a space character > - // before adding the firmware supplied arguments. However, the RpiFirmware > - // protocol expects a 32-bit aligned buffer. So let's allocate 4 bytes of > - // slack, and skip the first 3 when passing this buffer into libfdt. > - // > - CommandLine = AllocatePool (MAX_CMDLINE_SIZE) + 4; > - if (!CommandLine) { > - DEBUG ((DEBUG_ERROR, "%a: failed to allocate memory\n", __FUNCTION__)); > - return EFI_OUT_OF_RESOURCES; > - } > - > - // > - // Get the command line from the firmware > - // > - Status = mFwProtocol->GetCommandLine (MAX_CMDLINE_SIZE, CommandLine + 4); > - if (EFI_ERROR (Status)) { > - DEBUG ((DEBUG_ERROR, "%a: failed to retrieve command line\n", __FUNCTION__)); > - return Status; > - } > - > - if (AsciiStrLen (CommandLine + 4) == 0) { > - DEBUG ((DEBUG_INFO, "%a: empty command line received\n", __FUNCTION__)); > - return EFI_SUCCESS; > - } > - > - CommandLine[3] = ' '; > - > - Retval = fdt_appendprop_string (mFdtImage, Node, "bootargs", &CommandLine[3]); > - if (Retval != 0) { > - DEBUG ((DEBUG_ERROR, "%a: failed to set /chosen/bootargs property (%d)\n", > - __FUNCTION__, Retval)); > - } > - > - DEBUG_CODE_BEGIN (); > - CONST CHAR8 *Prop; > - INT32 Length; > - INT32 Index; > - > - Node = fdt_path_offset (mFdtImage, "/chosen"); > - ASSERT (Node >= 0); > - > - Prop = fdt_getprop (mFdtImage, Node, "bootargs", &Length); > - ASSERT (Prop != NULL); > - > - DEBUG ((DEBUG_INFO, "Command line set from firmware (length %d):\n'", Length)); > - > - for (Index = 0; Index < Length; Index++, Prop++) { > - if (*Prop == '\0') { > - continue; > - } > - DEBUG ((DEBUG_INFO, "%c", *Prop)); > - } > - > - DEBUG ((DEBUG_INFO, "'\n")); > - DEBUG_CODE_END (); > - > - FreePool (CommandLine - 4); > - return EFI_SUCCESS; > -} > - > - > /** > @param ImageHandle of the loaded driver > @param SystemTable Pointer to the System Table > @@ -435,13 +351,10 @@ FdtDxeInitialize ( > IN EFI_SYSTEM_TABLE *SystemTable > ) > { > + INT32 Retval; > EFI_STATUS Status; > - EFI_GUID *FdtGuid; > - VOID *FdtImage; > UINTN FdtSize; > - INT32 Retval; > - UINT32 BoardRevision; > - BOOLEAN Internal; > + VOID *FdtImage = NULL; I don't believe the NULL assignation is needed... > > if (PcdGet32 (PcdOptDeviceTree) == 0) { > DEBUG ((DEBUG_INFO, "Device Tree disabled per user configuration\n")); > @@ -450,77 +363,28 @@ FdtDxeInitialize ( > > Status = gBS->LocateProtocol (&gRaspberryPiFirmwareProtocolGuid, NULL, > (VOID**)&mFwProtocol); > - ASSERT_EFI_ERROR (Status); > + if (mFwProtocol == NULL) { > + /* > + * Impossible because of the depex... > + */ > + ASSERT_EFI_ERROR (Status); > + return EFI_NOT_FOUND; > + } > > - Internal = FALSE; > FdtImage = (VOID*)(UINTN)PcdGet32 (PcdFdtBaseAddress); ...since no code actually uses that variable before we assign it here. But I don't see a problem keeping it. > Retval = fdt_check_header (FdtImage); > - if (Retval == 0) { > - /* > - * Have FDT passed via config.txt. > - */ > - FdtSize = fdt_totalsize (FdtImage); > - DEBUG ((DEBUG_INFO, "Device Tree passed via config.txt (0x%lx bytes)\n", FdtSize)); > - Status = EFI_SUCCESS; > - } else { > - /* > - * Use one of the embedded FDT's. > - */ > - Internal = TRUE; > - DEBUG ((DEBUG_INFO, "No/Bad Device Tree found at address 0x%p (%a), " > - "looking up internal one...\n", FdtImage, fdt_strerror (Retval))); > + if (Retval != 0) { > /* > - * Query the board revision to differentiate between models. > + * Any one of: > + * - Invalid config.txt device_tree_address (not PcdFdtBaseAddress) > + * - Missing FDT for your Pi variant (if not overriding via device_tree=) > */ > - Status = mFwProtocol->GetModelRevision (&BoardRevision); > - if (EFI_ERROR (Status)) { > - DEBUG ((DEBUG_ERROR, "Failed to get board type: %r\n", Status)); > - DEBUG ((DEBUG_INFO, "Using default internal Device Tree\n")); > - FdtGuid = &gRaspberryPiDefaultFdtGuid; > - } else { > - // www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md > - switch ((BoardRevision >> 4) & 0xFF) { > - case 0x08: > - DEBUG ((DEBUG_INFO, "Using Raspberry Pi 3 Model B internal Device Tree\n")); > - FdtGuid = &gRaspberryPi3ModelBFdtGuid; > - break; > - case 0x0D: > - DEBUG ((DEBUG_INFO, "Using Raspberry Pi 3 Model B+ internal Device Tree\n")); > - FdtGuid = &gRaspberryPi3ModelBPlusFdtGuid; > - break; > - case 0x11: > - DEBUG ((DEBUG_INFO, "Using Raspberry Pi 4 Model B internal Device Tree\n")); > - FdtGuid = &gRaspberryPi4ModelBFdtGuid; > - break; > - default: > - DEBUG ((DEBUG_INFO, "Using default internal Device Tree\n")); > - FdtGuid = &gRaspberryPiDefaultFdtGuid; > - break; > - } > - } > - do { > - Status = GetSectionFromAnyFv (FdtGuid, EFI_SECTION_RAW, 0, &FdtImage, &FdtSize); > - if (Status == EFI_SUCCESS) { > - if (fdt_check_header (FdtImage) != 0) { > - Status = EFI_INCOMPATIBLE_VERSION; > - } > - } > - // No retry needed if we are successful or are dealing with the default Fdt. > - if ( (Status == EFI_SUCCESS) || > - (CompareGuid (FdtGuid, &gRaspberryPiDefaultFdtGuid)) ) > - break; > - // Otherwise, try one more time with the default Fdt. An example of this > - // is if we detected a non-default Fdt, that isn't included in the FDF. > - DEBUG ((DEBUG_INFO, "Internal Device Tree was not found for this platform, " > - "falling back to default...\n")); > - FdtGuid = &gRaspberryPiDefaultFdtGuid; > - } while (1); > + DEBUG ((DEBUG_ERROR, "No devicetree passed via config.txt\n")); > + return EFI_NOT_FOUND; > } > > - if (EFI_ERROR (Status)) { > - DEBUG ((DEBUG_ERROR, "Failed to locate Device Tree: %r\n", Status)); > - return Status; > - } > + FdtSize = fdt_totalsize (FdtImage); > + DEBUG ((DEBUG_INFO, "Devicetree passed via config.txt (0x%lx bytes)\n", FdtSize)); If we're going to modify that line, maybe we want to switch to using '%ld' instead of '0x%lx' as hex sizes aren't enormously helpful when dealing with user-handled files. But that's not something I want to respin this patch for, especially for debug output, so 0x%lx is fine too. > > /* > * Probably overkill. > @@ -529,12 +393,19 @@ FdtDxeInitialize ( > Status = gBS->AllocatePages (AllocateAnyPages, EfiBootServicesData, > EFI_SIZE_TO_PAGES (FdtSize), (EFI_PHYSICAL_ADDRESS*)&mFdtImage); > if (EFI_ERROR (Status)) { > - DEBUG ((DEBUG_ERROR, "Failed to allocate Device Tree: %r\n", Status)); > - return Status; > + DEBUG ((DEBUG_ERROR, "Failed to allocate devicetree: %r\n", Status)); > + goto out; > } > > Retval = fdt_open_into (FdtImage, mFdtImage, FdtSize); > - ASSERT (Retval == 0); > + if (Retval != 0) { > + DEBUG ((DEBUG_ERROR, "fdt_open_into failed: %d\n", Retval)); > + goto out; > + } > + > + /* > + * These are all best-effort. > + */ > > Status = SanitizePSCI (); > if (EFI_ERROR (Status)) { > @@ -566,19 +437,18 @@ FdtDxeInitialize ( > Print (L"Failed to update USB compatible properties: %r\n", Status); > } > > - if (Internal) { > - /* > - * A GPU-provided DTB already has the full command line. > - */ > - Status = UpdateBootArgs (); > - if (EFI_ERROR (Status)) { > - Print (L"Failed to update boot arguments: %r\n", Status); > - } > - } > - > - DEBUG ((DEBUG_INFO, "Installed Device Tree at address 0x%p\n", mFdtImage)); > + DEBUG ((DEBUG_INFO, "Installed devicetree at address %p\n", mFdtImage)); > Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mFdtImage); > - ASSERT_EFI_ERROR (Status); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "Couldn't register devicetree: %r\n", Status)); > + goto out; > + } > > +out: > + if (EFI_ERROR(Status)) { > + if (mFdtImage != NULL) { > + gBS->FreePages ((EFI_PHYSICAL_ADDRESS) mFdtImage, EFI_SIZE_TO_PAGES (FdtSize)); > + } > + } > return Status; > } > diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf > index fc37353f..49ba5ba1 100644 > --- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf > +++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf > @@ -35,10 +35,6 @@ > > [Guids] > gFdtTableGuid > - gRaspberryPi3ModelBFdtGuid > - gRaspberryPi3ModelBPlusFdtGuid > - gRaspberryPi4ModelBFdtGuid > - gRaspberryPiDefaultFdtGuid > > [Protocols] > gRaspberryPiFirmwareProtocolGuid ## CONSUMES > diff --git a/Platform/RaspberryPi/RPi3/RPi3.fdf b/Platform/RaspberryPi/RPi3/RPi3.fdf > index daedc443..e854cd21 100644 > --- a/Platform/RaspberryPi/RPi3/RPi3.fdf > +++ b/Platform/RaspberryPi/RPi3/RPi3.fdf > @@ -303,17 +303,6 @@ READ_LOCK_STATUS = TRUE > # > INF Platform/RaspberryPi/Drivers/LogoDxe/LogoDxe.inf > > - # > - # Device Tree support (used by FdtDxe) > - # GUIDs should match gRaspberryPi#####FdtGuid's from the .dec > - # > - FILE FREEFORM = DF5DA223-1D27-47C3-8D1B-9A41B55A18BC { > - SECTION RAW = Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2710-rpi-3-b.dtb > - } > - FILE FREEFORM = 3D523012-73FE-40E5-892E-1A4DF60F3C0C { > - SECTION RAW = Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2710-rpi-3-b-plus.dtb > - } > - > [FV.FVMAIN_COMPACT] > FvAlignment = 16 > ERASE_POLARITY = 1 > diff --git a/Platform/RaspberryPi/RPi4/RPi4.fdf b/Platform/RaspberryPi/RPi4/RPi4.fdf > index c3e9cfc4..b1f7aa23 100644 > --- a/Platform/RaspberryPi/RPi4/RPi4.fdf > +++ b/Platform/RaspberryPi/RPi4/RPi4.fdf > @@ -307,14 +307,6 @@ READ_LOCK_STATUS = TRUE > # > INF Platform/RaspberryPi/Drivers/LogoDxe/LogoDxe.inf > > - # > - # Device Tree support (used by FdtDxe) > - # GUIDs should match gRaspberryPi#####FdtGuid's from the .dec > - # > - FILE FREEFORM = 80AB6833-CAE4-4CEE-B59D-EB2039B05551 { > - SECTION RAW = Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2711-rpi-4-b.dtb > - } > - > [FV.FVMAIN_COMPACT] > FvAlignment = 16 > ERASE_POLARITY = 1 > diff --git a/Platform/RaspberryPi/RaspberryPi.dec b/Platform/RaspberryPi/RaspberryPi.dec > index b66322be..66ef6186 100644 > --- a/Platform/RaspberryPi/RaspberryPi.dec > +++ b/Platform/RaspberryPi/RaspberryPi.dec > @@ -26,13 +26,6 @@ > gRaspberryPiTokenSpaceGuid = {0xCD7CC258, 0x31DB, 0x11E6, {0x9F, 0xD3, 0x63, 0xB0, 0xB8, 0xEE, 0xD6, 0xB5}} > gRaspberryPiEventResetGuid = {0xCD7CC258, 0x31DB, 0x11E6, {0x9F, 0xD3, 0x63, 0xB4, 0xB4, 0xE4, 0xD4, 0xB4}} > gConfigDxeFormSetGuid = {0xCD7CC258, 0x31DB, 0x22E6, {0x9F, 0x22, 0x63, 0xB0, 0xB8, 0xEE, 0xD6, 0xB5}} > - # GUIDs used by FdtDxe to serve a Device Tree at runtime. Not all of these need to apply > - # to the current platform or match an actual FDF binary, but they need to be defined. > - gRaspberryPi3ModelBFdtGuid = { 0xDF5DA223, 0x1D27, 0x47C3, { 0x8D, 0x1B, 0x9A, 0x41, 0xB5, 0x5A, 0x18, 0xBC } } > - gRaspberryPi3ModelBPlusFdtGuid = { 0x3D523012, 0x73FE, 0x40E5, { 0x89, 0x2E, 0x1A, 0x4D, 0xF6, 0x0F, 0x3C, 0x0C } } > - gRaspberryPi4ModelBFdtGuid = { 0x80AB6833, 0xCAE4, 0x4CEE, { 0xB5, 0x9D, 0xEB, 0x20, 0x39, 0xB0, 0x55, 0x51 } } > - # Default Fdt to serve if the hardware model can't be detected. Should match one of the above. > - gRaspberryPiDefaultFdtGuid = {0xDF5DA223, 0x1D27, 0x47C3, { 0x8D, 0x1B, 0x9A, 0x41, 0xB5, 0x5A, 0x18, 0xBC}} > > [PcdsFixedAtBuild.common] > # > Reviewed-by: Pete Batard <pete@akeo.ie> Tested-on: Pi 3B, Pi 3B+, Pi 4B, with and without a Device Tree provided ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-platforms][PATCH 1/2] RPi: rip out FdtDxe logic to use internal DTB 2020-04-30 21:16 ` [edk2-platforms][PATCH 1/2] RPi: rip out FdtDxe logic to use internal DTB Andrei Warkentin 2020-05-01 10:45 ` Pete Batard @ 2020-05-01 13:19 ` Ard Biesheuvel 2020-05-01 15:11 ` Pete Batard 1 sibling, 1 reply; 12+ messages in thread From: Ard Biesheuvel @ 2020-05-01 13:19 UTC (permalink / raw) To: Andrei Warkentin, devel; +Cc: leif, pete, philmd On 4/30/20 11:16 PM, Andrei Warkentin wrote: > Initially, FdtDxe used an internal (embedded in UEFI) FDT, > because it was neither understood how to consume the > one loaded by the VideoCore firmware, nor understood just > how important it is to use the DTB provided by config.txt. > > Embedding the DT was a bad idea, because: > - Permanently stale > - No overlays > > Also, on devices like the Pi 4 you _have_ to have a DT > around for the start4 VPU firmware to pick up, otherwise > the board is left in an inconsistent state. So we're being > proscriptive now about DT use with config.txt, which means > this internal DT logic is deadc code. > > Further FdtDxe cleanups are possible and will be handled > separately, specifically: > - probably no need to use a separate allocation for patched DT (optimize memory used) > - suspicious use of EfiBootServicesData (I filed https://github.com/ARM-software/ebbr/issues/45 to sort out the real requirements) > > Testing: Booted Ubuntu 18.04 on Pi 2B (1.2). > > Signed-off-by: Andrei Warkentin <andrey.warkentin@gmail.com> > --- > Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c | 206 ++++---------------- > Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf | 4 - > Platform/RaspberryPi/RPi3/RPi3.fdf | 11 -- > Platform/RaspberryPi/RPi4/RPi4.fdf | 8 - > Platform/RaspberryPi/RaspberryPi.dec | 7 - > 5 files changed, 38 insertions(+), 198 deletions(-) > > diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c > index e7143f57..187b9566 100644 > --- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c > +++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c > @@ -335,90 +335,6 @@ CleanSimpleFramebuffer ( > return EFI_SUCCESS; > } > > -#define MAX_CMDLINE_SIZE 512 > - > -STATIC > -EFI_STATUS > -UpdateBootArgs ( > - VOID > - ) > -{ > - INTN Node; > - INTN Retval; > - EFI_STATUS Status; > - CHAR8 *CommandLine; > - > - // > - // Locate the /chosen node > - // > - Node = fdt_path_offset (mFdtImage, "/chosen"); > - if (Node < 0) { > - DEBUG ((DEBUG_ERROR, "%a: failed to locate /chosen node\n", __FUNCTION__)); > - return EFI_NOT_FOUND; > - } > - > - // > - // If /chosen/bootargs already exists, we want to add a space character > - // before adding the firmware supplied arguments. However, the RpiFirmware > - // protocol expects a 32-bit aligned buffer. So let's allocate 4 bytes of > - // slack, and skip the first 3 when passing this buffer into libfdt. > - // > - CommandLine = AllocatePool (MAX_CMDLINE_SIZE) + 4; > - if (!CommandLine) { > - DEBUG ((DEBUG_ERROR, "%a: failed to allocate memory\n", __FUNCTION__)); > - return EFI_OUT_OF_RESOURCES; > - } > - > - // > - // Get the command line from the firmware > - // > - Status = mFwProtocol->GetCommandLine (MAX_CMDLINE_SIZE, CommandLine + 4); > - if (EFI_ERROR (Status)) { > - DEBUG ((DEBUG_ERROR, "%a: failed to retrieve command line\n", __FUNCTION__)); > - return Status; > - } > - > - if (AsciiStrLen (CommandLine + 4) == 0) { > - DEBUG ((DEBUG_INFO, "%a: empty command line received\n", __FUNCTION__)); > - return EFI_SUCCESS; > - } > - > - CommandLine[3] = ' '; > - > - Retval = fdt_appendprop_string (mFdtImage, Node, "bootargs", &CommandLine[3]); > - if (Retval != 0) { > - DEBUG ((DEBUG_ERROR, "%a: failed to set /chosen/bootargs property (%d)\n", > - __FUNCTION__, Retval)); > - } > - > - DEBUG_CODE_BEGIN (); > - CONST CHAR8 *Prop; > - INT32 Length; > - INT32 Index; > - > - Node = fdt_path_offset (mFdtImage, "/chosen"); > - ASSERT (Node >= 0); > - > - Prop = fdt_getprop (mFdtImage, Node, "bootargs", &Length); > - ASSERT (Prop != NULL); > - > - DEBUG ((DEBUG_INFO, "Command line set from firmware (length %d):\n'", Length)); > - > - for (Index = 0; Index < Length; Index++, Prop++) { > - if (*Prop == '\0') { > - continue; > - } > - DEBUG ((DEBUG_INFO, "%c", *Prop)); > - } > - > - DEBUG ((DEBUG_INFO, "'\n")); > - DEBUG_CODE_END (); > - > - FreePool (CommandLine - 4); > - return EFI_SUCCESS; > -} > - > - > /** > @param ImageHandle of the loaded driver > @param SystemTable Pointer to the System Table > @@ -435,13 +351,10 @@ FdtDxeInitialize ( > IN EFI_SYSTEM_TABLE *SystemTable > ) > { > + INT32 Retval; > EFI_STATUS Status; > - EFI_GUID *FdtGuid; > - VOID *FdtImage; > UINTN FdtSize; > - INT32 Retval; > - UINT32 BoardRevision; > - BOOLEAN Internal; > + VOID *FdtImage = NULL; > > if (PcdGet32 (PcdOptDeviceTree) == 0) { > DEBUG ((DEBUG_INFO, "Device Tree disabled per user configuration\n")); > @@ -450,77 +363,28 @@ FdtDxeInitialize ( > > Status = gBS->LocateProtocol (&gRaspberryPiFirmwareProtocolGuid, NULL, > (VOID**)&mFwProtocol); > - ASSERT_EFI_ERROR (Status); > + if (mFwProtocol == NULL) { > + /* > + * Impossible because of the depex... > + */ > + ASSERT_EFI_ERROR (Status); > + return EFI_NOT_FOUND; > + } > Do we need this change? > - Internal = FALSE; > FdtImage = (VOID*)(UINTN)PcdGet32 (PcdFdtBaseAddress); > Retval = fdt_check_header (FdtImage); > - if (Retval == 0) { > - /* > - * Have FDT passed via config.txt. > - */ > - FdtSize = fdt_totalsize (FdtImage); > - DEBUG ((DEBUG_INFO, "Device Tree passed via config.txt (0x%lx bytes)\n", FdtSize)); > - Status = EFI_SUCCESS; > - } else { > - /* > - * Use one of the embedded FDT's. > - */ > - Internal = TRUE; > - DEBUG ((DEBUG_INFO, "No/Bad Device Tree found at address 0x%p (%a), " > - "looking up internal one...\n", FdtImage, fdt_strerror (Retval))); > + if (Retval != 0) { > /* > - * Query the board revision to differentiate between models. > + * Any one of: > + * - Invalid config.txt device_tree_address (not PcdFdtBaseAddress) > + * - Missing FDT for your Pi variant (if not overriding via device_tree=) > */ > - Status = mFwProtocol->GetModelRevision (&BoardRevision); > - if (EFI_ERROR (Status)) { > - DEBUG ((DEBUG_ERROR, "Failed to get board type: %r\n", Status)); > - DEBUG ((DEBUG_INFO, "Using default internal Device Tree\n")); > - FdtGuid = &gRaspberryPiDefaultFdtGuid; > - } else { > - // www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md > - switch ((BoardRevision >> 4) & 0xFF) { > - case 0x08: > - DEBUG ((DEBUG_INFO, "Using Raspberry Pi 3 Model B internal Device Tree\n")); > - FdtGuid = &gRaspberryPi3ModelBFdtGuid; > - break; > - case 0x0D: > - DEBUG ((DEBUG_INFO, "Using Raspberry Pi 3 Model B+ internal Device Tree\n")); > - FdtGuid = &gRaspberryPi3ModelBPlusFdtGuid; > - break; > - case 0x11: > - DEBUG ((DEBUG_INFO, "Using Raspberry Pi 4 Model B internal Device Tree\n")); > - FdtGuid = &gRaspberryPi4ModelBFdtGuid; > - break; > - default: > - DEBUG ((DEBUG_INFO, "Using default internal Device Tree\n")); > - FdtGuid = &gRaspberryPiDefaultFdtGuid; > - break; > - } > - } > - do { > - Status = GetSectionFromAnyFv (FdtGuid, EFI_SECTION_RAW, 0, &FdtImage, &FdtSize); > - if (Status == EFI_SUCCESS) { > - if (fdt_check_header (FdtImage) != 0) { > - Status = EFI_INCOMPATIBLE_VERSION; > - } > - } > - // No retry needed if we are successful or are dealing with the default Fdt. > - if ( (Status == EFI_SUCCESS) || > - (CompareGuid (FdtGuid, &gRaspberryPiDefaultFdtGuid)) ) > - break; > - // Otherwise, try one more time with the default Fdt. An example of this > - // is if we detected a non-default Fdt, that isn't included in the FDF. > - DEBUG ((DEBUG_INFO, "Internal Device Tree was not found for this platform, " > - "falling back to default...\n")); > - FdtGuid = &gRaspberryPiDefaultFdtGuid; > - } while (1); > + DEBUG ((DEBUG_ERROR, "No devicetree passed via config.txt\n")); > + return EFI_NOT_FOUND; > } > > - if (EFI_ERROR (Status)) { > - DEBUG ((DEBUG_ERROR, "Failed to locate Device Tree: %r\n", Status)); > - return Status; > - } > + FdtSize = fdt_totalsize (FdtImage); > + DEBUG ((DEBUG_INFO, "Devicetree passed via config.txt (0x%lx bytes)\n", FdtSize)); > > /* > * Probably overkill. > @@ -529,12 +393,19 @@ FdtDxeInitialize ( > Status = gBS->AllocatePages (AllocateAnyPages, EfiBootServicesData, > EFI_SIZE_TO_PAGES (FdtSize), (EFI_PHYSICAL_ADDRESS*)&mFdtImage); > if (EFI_ERROR (Status)) { > - DEBUG ((DEBUG_ERROR, "Failed to allocate Device Tree: %r\n", Status)); > - return Status; > + DEBUG ((DEBUG_ERROR, "Failed to allocate devicetree: %r\n", Status)); > + goto out; > } > > Retval = fdt_open_into (FdtImage, mFdtImage, FdtSize); > - ASSERT (Retval == 0); > + if (Retval != 0) { > + DEBUG ((DEBUG_ERROR, "fdt_open_into failed: %d\n", Retval)); > + goto out; > + } > + > + /* > + * These are all best-effort. > + */ > > Status = SanitizePSCI (); > if (EFI_ERROR (Status)) { > @@ -566,19 +437,18 @@ FdtDxeInitialize ( > Print (L"Failed to update USB compatible properties: %r\n", Status); > } > > - if (Internal) { > - /* > - * A GPU-provided DTB already has the full command line. > - */ > - Status = UpdateBootArgs (); > - if (EFI_ERROR (Status)) { > - Print (L"Failed to update boot arguments: %r\n", Status); > - } > - } > - > - DEBUG ((DEBUG_INFO, "Installed Device Tree at address 0x%p\n", mFdtImage)); > + DEBUG ((DEBUG_INFO, "Installed devicetree at address %p\n", mFdtImage)); > Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mFdtImage); > - ASSERT_EFI_ERROR (Status); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "Couldn't register devicetree: %r\n", Status)); > + goto out; > + } > > +out: > + if (EFI_ERROR(Status)) { > + if (mFdtImage != NULL) { > + gBS->FreePages ((EFI_PHYSICAL_ADDRESS) mFdtImage, EFI_SIZE_TO_PAGES (FdtSize)); > + } > + } > return Status; > } > diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf > index fc37353f..49ba5ba1 100644 > --- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf > +++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf > @@ -35,10 +35,6 @@ > > [Guids] > gFdtTableGuid > - gRaspberryPi3ModelBFdtGuid > - gRaspberryPi3ModelBPlusFdtGuid > - gRaspberryPi4ModelBFdtGuid > - gRaspberryPiDefaultFdtGuid > > [Protocols] > gRaspberryPiFirmwareProtocolGuid ## CONSUMES > diff --git a/Platform/RaspberryPi/RPi3/RPi3.fdf b/Platform/RaspberryPi/RPi3/RPi3.fdf > index daedc443..e854cd21 100644 > --- a/Platform/RaspberryPi/RPi3/RPi3.fdf > +++ b/Platform/RaspberryPi/RPi3/RPi3.fdf > @@ -303,17 +303,6 @@ READ_LOCK_STATUS = TRUE > # > INF Platform/RaspberryPi/Drivers/LogoDxe/LogoDxe.inf > > - # > - # Device Tree support (used by FdtDxe) > - # GUIDs should match gRaspberryPi#####FdtGuid's from the .dec > - # > - FILE FREEFORM = DF5DA223-1D27-47C3-8D1B-9A41B55A18BC { > - SECTION RAW = Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2710-rpi-3-b.dtb > - } > - FILE FREEFORM = 3D523012-73FE-40E5-892E-1A4DF60F3C0C { > - SECTION RAW = Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2710-rpi-3-b-plus.dtb > - } > - > [FV.FVMAIN_COMPACT] > FvAlignment = 16 > ERASE_POLARITY = 1 > diff --git a/Platform/RaspberryPi/RPi4/RPi4.fdf b/Platform/RaspberryPi/RPi4/RPi4.fdf > index c3e9cfc4..b1f7aa23 100644 > --- a/Platform/RaspberryPi/RPi4/RPi4.fdf > +++ b/Platform/RaspberryPi/RPi4/RPi4.fdf > @@ -307,14 +307,6 @@ READ_LOCK_STATUS = TRUE > # > INF Platform/RaspberryPi/Drivers/LogoDxe/LogoDxe.inf > > - # > - # Device Tree support (used by FdtDxe) > - # GUIDs should match gRaspberryPi#####FdtGuid's from the .dec > - # > - FILE FREEFORM = 80AB6833-CAE4-4CEE-B59D-EB2039B05551 { > - SECTION RAW = Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2711-rpi-4-b.dtb > - } > - > [FV.FVMAIN_COMPACT] > FvAlignment = 16 > ERASE_POLARITY = 1 > diff --git a/Platform/RaspberryPi/RaspberryPi.dec b/Platform/RaspberryPi/RaspberryPi.dec > index b66322be..66ef6186 100644 > --- a/Platform/RaspberryPi/RaspberryPi.dec > +++ b/Platform/RaspberryPi/RaspberryPi.dec > @@ -26,13 +26,6 @@ > gRaspberryPiTokenSpaceGuid = {0xCD7CC258, 0x31DB, 0x11E6, {0x9F, 0xD3, 0x63, 0xB0, 0xB8, 0xEE, 0xD6, 0xB5}} > gRaspberryPiEventResetGuid = {0xCD7CC258, 0x31DB, 0x11E6, {0x9F, 0xD3, 0x63, 0xB4, 0xB4, 0xE4, 0xD4, 0xB4}} > gConfigDxeFormSetGuid = {0xCD7CC258, 0x31DB, 0x22E6, {0x9F, 0x22, 0x63, 0xB0, 0xB8, 0xEE, 0xD6, 0xB5}} > - # GUIDs used by FdtDxe to serve a Device Tree at runtime. Not all of these need to apply > - # to the current platform or match an actual FDF binary, but they need to be defined. > - gRaspberryPi3ModelBFdtGuid = { 0xDF5DA223, 0x1D27, 0x47C3, { 0x8D, 0x1B, 0x9A, 0x41, 0xB5, 0x5A, 0x18, 0xBC } } > - gRaspberryPi3ModelBPlusFdtGuid = { 0x3D523012, 0x73FE, 0x40E5, { 0x89, 0x2E, 0x1A, 0x4D, 0xF6, 0x0F, 0x3C, 0x0C } } > - gRaspberryPi4ModelBFdtGuid = { 0x80AB6833, 0xCAE4, 0x4CEE, { 0xB5, 0x9D, 0xEB, 0x20, 0x39, 0xB0, 0x55, 0x51 } } > - # Default Fdt to serve if the hardware model can't be detected. Should match one of the above. > - gRaspberryPiDefaultFdtGuid = {0xDF5DA223, 0x1D27, 0x47C3, { 0x8D, 0x1B, 0x9A, 0x41, 0xB5, 0x5A, 0x18, 0xBC}} > > [PcdsFixedAtBuild.common] > # > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-platforms][PATCH 1/2] RPi: rip out FdtDxe logic to use internal DTB 2020-05-01 13:19 ` Ard Biesheuvel @ 2020-05-01 15:11 ` Pete Batard 2020-05-01 15:13 ` Ard Biesheuvel 0 siblings, 1 reply; 12+ messages in thread From: Pete Batard @ 2020-05-01 15:11 UTC (permalink / raw) To: Ard Biesheuvel, Andrei Warkentin, devel; +Cc: leif, philmd On 2020.05.01 14:19, Ard Biesheuvel wrote: > On 4/30/20 11:16 PM, Andrei Warkentin wrote: >> Initially, FdtDxe used an internal (embedded in UEFI) FDT, >> because it was neither understood how to consume the >> one loaded by the VideoCore firmware, nor understood just >> how important it is to use the DTB provided by config.txt. >> >> Embedding the DT was a bad idea, because: >> - Permanently stale >> - No overlays >> >> Also, on devices like the Pi 4 you _have_ to have a DT >> around for the start4 VPU firmware to pick up, otherwise >> the board is left in an inconsistent state. So we're being >> proscriptive now about DT use with config.txt, which means >> this internal DT logic is deadc code. >> >> Further FdtDxe cleanups are possible and will be handled >> separately, specifically: >> - probably no need to use a separate allocation for patched DT >> (optimize memory used) >> - suspicious use of EfiBootServicesData (I filed >> https://github.com/ARM-software/ebbr/issues/45 to sort out the real >> requirements) >> >> Testing: Booted Ubuntu 18.04 on Pi 2B (1.2). >> >> Signed-off-by: Andrei Warkentin <andrey.warkentin@gmail.com> >> --- >> Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c | 206 >> ++++---------------- >> Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf | 4 - >> Platform/RaspberryPi/RPi3/RPi3.fdf | 11 -- >> Platform/RaspberryPi/RPi4/RPi4.fdf | 8 - >> Platform/RaspberryPi/RaspberryPi.dec | 7 - >> 5 files changed, 38 insertions(+), 198 deletions(-) >> >> diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c >> b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c >> index e7143f57..187b9566 100644 >> --- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c >> +++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c >> @@ -335,90 +335,6 @@ CleanSimpleFramebuffer ( >> return EFI_SUCCESS; >> } >> -#define MAX_CMDLINE_SIZE 512 >> - >> -STATIC >> -EFI_STATUS >> -UpdateBootArgs ( >> - VOID >> - ) >> -{ >> - INTN Node; >> - INTN Retval; >> - EFI_STATUS Status; >> - CHAR8 *CommandLine; >> - >> - // >> - // Locate the /chosen node >> - // >> - Node = fdt_path_offset (mFdtImage, "/chosen"); >> - if (Node < 0) { >> - DEBUG ((DEBUG_ERROR, "%a: failed to locate /chosen node\n", >> __FUNCTION__)); >> - return EFI_NOT_FOUND; >> - } >> - >> - // >> - // If /chosen/bootargs already exists, we want to add a space >> character >> - // before adding the firmware supplied arguments. However, the >> RpiFirmware >> - // protocol expects a 32-bit aligned buffer. So let's allocate 4 >> bytes of >> - // slack, and skip the first 3 when passing this buffer into libfdt. >> - // >> - CommandLine = AllocatePool (MAX_CMDLINE_SIZE) + 4; >> - if (!CommandLine) { >> - DEBUG ((DEBUG_ERROR, "%a: failed to allocate memory\n", >> __FUNCTION__)); >> - return EFI_OUT_OF_RESOURCES; >> - } >> - >> - // >> - // Get the command line from the firmware >> - // >> - Status = mFwProtocol->GetCommandLine (MAX_CMDLINE_SIZE, CommandLine >> + 4); >> - if (EFI_ERROR (Status)) { >> - DEBUG ((DEBUG_ERROR, "%a: failed to retrieve command line\n", >> __FUNCTION__)); >> - return Status; >> - } >> - >> - if (AsciiStrLen (CommandLine + 4) == 0) { >> - DEBUG ((DEBUG_INFO, "%a: empty command line received\n", >> __FUNCTION__)); >> - return EFI_SUCCESS; >> - } >> - >> - CommandLine[3] = ' '; >> - >> - Retval = fdt_appendprop_string (mFdtImage, Node, "bootargs", >> &CommandLine[3]); >> - if (Retval != 0) { >> - DEBUG ((DEBUG_ERROR, "%a: failed to set /chosen/bootargs property >> (%d)\n", >> - __FUNCTION__, Retval)); >> - } >> - >> - DEBUG_CODE_BEGIN (); >> - CONST CHAR8 *Prop; >> - INT32 Length; >> - INT32 Index; >> - >> - Node = fdt_path_offset (mFdtImage, "/chosen"); >> - ASSERT (Node >= 0); >> - >> - Prop = fdt_getprop (mFdtImage, Node, "bootargs", &Length); >> - ASSERT (Prop != NULL); >> - >> - DEBUG ((DEBUG_INFO, "Command line set from firmware (length >> %d):\n'", Length)); >> - >> - for (Index = 0; Index < Length; Index++, Prop++) { >> - if (*Prop == '\0') { >> - continue; >> - } >> - DEBUG ((DEBUG_INFO, "%c", *Prop)); >> - } >> - >> - DEBUG ((DEBUG_INFO, "'\n")); >> - DEBUG_CODE_END (); >> - >> - FreePool (CommandLine - 4); >> - return EFI_SUCCESS; >> -} >> - >> - >> /** >> @param ImageHandle of the loaded driver >> @param SystemTable Pointer to the System Table >> @@ -435,13 +351,10 @@ FdtDxeInitialize ( >> IN EFI_SYSTEM_TABLE *SystemTable >> ) >> { >> + INT32 Retval; >> EFI_STATUS Status; >> - EFI_GUID *FdtGuid; >> - VOID *FdtImage; >> UINTN FdtSize; >> - INT32 Retval; >> - UINT32 BoardRevision; >> - BOOLEAN Internal; >> + VOID *FdtImage = NULL; >> if (PcdGet32 (PcdOptDeviceTree) == 0) { >> DEBUG ((DEBUG_INFO, "Device Tree disabled per user >> configuration\n")); >> @@ -450,77 +363,28 @@ FdtDxeInitialize ( >> Status = gBS->LocateProtocol (&gRaspberryPiFirmwareProtocolGuid, >> NULL, >> (VOID**)&mFwProtocol); >> - ASSERT_EFI_ERROR (Status); >> + if (mFwProtocol == NULL) { >> + /* >> + * Impossible because of the depex... >> + */ >> + ASSERT_EFI_ERROR (Status); >> + return EFI_NOT_FOUND; >> + } > > Do we need this change? Looking at what we are doing elsewhere, I thing we should go with: ASSERT_EFI_ERROR (Status); if (EFI_ERROR (Status)) { return Status; } The ASSERT_EFI_ERROR () on its own was not enough for RELEASE, so we definitely want to return an error code here if needed, and I guess that, technically, we could consider that LocateProtocol () may succeed and return a NULL value for mFwProtocol, but I doubt that's a valid consideration. Regards, /Pete > >> - Internal = FALSE; >> FdtImage = (VOID*)(UINTN)PcdGet32 (PcdFdtBaseAddress); >> Retval = fdt_check_header (FdtImage); >> - if (Retval == 0) { >> - /* >> - * Have FDT passed via config.txt. >> - */ >> - FdtSize = fdt_totalsize (FdtImage); >> - DEBUG ((DEBUG_INFO, "Device Tree passed via config.txt (0x%lx >> bytes)\n", FdtSize)); >> - Status = EFI_SUCCESS; >> - } else { >> - /* >> - * Use one of the embedded FDT's. >> - */ >> - Internal = TRUE; >> - DEBUG ((DEBUG_INFO, "No/Bad Device Tree found at address 0x%p >> (%a), " >> - "looking up internal one...\n", FdtImage, fdt_strerror (Retval))); >> + if (Retval != 0) { >> /* >> - * Query the board revision to differentiate between models. >> + * Any one of: >> + * - Invalid config.txt device_tree_address (not PcdFdtBaseAddress) >> + * - Missing FDT for your Pi variant (if not overriding via >> device_tree=) >> */ >> - Status = mFwProtocol->GetModelRevision (&BoardRevision); >> - if (EFI_ERROR (Status)) { >> - DEBUG ((DEBUG_ERROR, "Failed to get board type: %r\n", Status)); >> - DEBUG ((DEBUG_INFO, "Using default internal Device Tree\n")); >> - FdtGuid = &gRaspberryPiDefaultFdtGuid; >> - } else { >> - // >> www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md >> >> - switch ((BoardRevision >> 4) & 0xFF) { >> - case 0x08: >> - DEBUG ((DEBUG_INFO, "Using Raspberry Pi 3 Model B internal >> Device Tree\n")); >> - FdtGuid = &gRaspberryPi3ModelBFdtGuid; >> - break; >> - case 0x0D: >> - DEBUG ((DEBUG_INFO, "Using Raspberry Pi 3 Model B+ internal >> Device Tree\n")); >> - FdtGuid = &gRaspberryPi3ModelBPlusFdtGuid; >> - break; >> - case 0x11: >> - DEBUG ((DEBUG_INFO, "Using Raspberry Pi 4 Model B internal >> Device Tree\n")); >> - FdtGuid = &gRaspberryPi4ModelBFdtGuid; >> - break; >> - default: >> - DEBUG ((DEBUG_INFO, "Using default internal Device Tree\n")); >> - FdtGuid = &gRaspberryPiDefaultFdtGuid; >> - break; >> - } >> - } >> - do { >> - Status = GetSectionFromAnyFv (FdtGuid, EFI_SECTION_RAW, 0, >> &FdtImage, &FdtSize); >> - if (Status == EFI_SUCCESS) { >> - if (fdt_check_header (FdtImage) != 0) { >> - Status = EFI_INCOMPATIBLE_VERSION; >> - } >> - } >> - // No retry needed if we are successful or are dealing with the >> default Fdt. >> - if ( (Status == EFI_SUCCESS) || >> - (CompareGuid (FdtGuid, &gRaspberryPiDefaultFdtGuid)) ) >> - break; >> - // Otherwise, try one more time with the default Fdt. An >> example of this >> - // is if we detected a non-default Fdt, that isn't included in >> the FDF. >> - DEBUG ((DEBUG_INFO, "Internal Device Tree was not found for >> this platform, " >> - "falling back to default...\n")); >> - FdtGuid = &gRaspberryPiDefaultFdtGuid; >> - } while (1); >> + DEBUG ((DEBUG_ERROR, "No devicetree passed via config.txt\n")); >> + return EFI_NOT_FOUND; >> } >> - if (EFI_ERROR (Status)) { >> - DEBUG ((DEBUG_ERROR, "Failed to locate Device Tree: %r\n", Status)); >> - return Status; >> - } >> + FdtSize = fdt_totalsize (FdtImage); >> + DEBUG ((DEBUG_INFO, "Devicetree passed via config.txt (0x%lx >> bytes)\n", FdtSize)); >> /* >> * Probably overkill. >> @@ -529,12 +393,19 @@ FdtDxeInitialize ( >> Status = gBS->AllocatePages (AllocateAnyPages, EfiBootServicesData, >> EFI_SIZE_TO_PAGES (FdtSize), >> (EFI_PHYSICAL_ADDRESS*)&mFdtImage); >> if (EFI_ERROR (Status)) { >> - DEBUG ((DEBUG_ERROR, "Failed to allocate Device Tree: %r\n", >> Status)); >> - return Status; >> + DEBUG ((DEBUG_ERROR, "Failed to allocate devicetree: %r\n", >> Status)); >> + goto out; >> } >> Retval = fdt_open_into (FdtImage, mFdtImage, FdtSize); >> - ASSERT (Retval == 0); >> + if (Retval != 0) { >> + DEBUG ((DEBUG_ERROR, "fdt_open_into failed: %d\n", Retval)); >> + goto out; >> + } >> + >> + /* >> + * These are all best-effort. >> + */ >> Status = SanitizePSCI (); >> if (EFI_ERROR (Status)) { >> @@ -566,19 +437,18 @@ FdtDxeInitialize ( >> Print (L"Failed to update USB compatible properties: %r\n", >> Status); >> } >> - if (Internal) { >> - /* >> - * A GPU-provided DTB already has the full command line. >> - */ >> - Status = UpdateBootArgs (); >> - if (EFI_ERROR (Status)) { >> - Print (L"Failed to update boot arguments: %r\n", Status); >> - } >> - } >> - >> - DEBUG ((DEBUG_INFO, "Installed Device Tree at address 0x%p\n", >> mFdtImage)); >> + DEBUG ((DEBUG_INFO, "Installed devicetree at address %p\n", >> mFdtImage)); >> Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mFdtImage); >> - ASSERT_EFI_ERROR (Status); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_ERROR, "Couldn't register devicetree: %r\n", >> Status)); >> + goto out; >> + } >> +out: >> + if (EFI_ERROR(Status)) { >> + if (mFdtImage != NULL) { >> + gBS->FreePages ((EFI_PHYSICAL_ADDRESS) mFdtImage, >> EFI_SIZE_TO_PAGES (FdtSize)); >> + } >> + } >> return Status; >> } >> diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf >> b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf >> index fc37353f..49ba5ba1 100644 >> --- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf >> +++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf >> @@ -35,10 +35,6 @@ >> [Guids] >> gFdtTableGuid >> - gRaspberryPi3ModelBFdtGuid >> - gRaspberryPi3ModelBPlusFdtGuid >> - gRaspberryPi4ModelBFdtGuid >> - gRaspberryPiDefaultFdtGuid >> [Protocols] >> gRaspberryPiFirmwareProtocolGuid ## CONSUMES >> diff --git a/Platform/RaspberryPi/RPi3/RPi3.fdf >> b/Platform/RaspberryPi/RPi3/RPi3.fdf >> index daedc443..e854cd21 100644 >> --- a/Platform/RaspberryPi/RPi3/RPi3.fdf >> +++ b/Platform/RaspberryPi/RPi3/RPi3.fdf >> @@ -303,17 +303,6 @@ READ_LOCK_STATUS = TRUE >> # >> INF Platform/RaspberryPi/Drivers/LogoDxe/LogoDxe.inf >> - # >> - # Device Tree support (used by FdtDxe) >> - # GUIDs should match gRaspberryPi#####FdtGuid's from the .dec >> - # >> - FILE FREEFORM = DF5DA223-1D27-47C3-8D1B-9A41B55A18BC { >> - SECTION RAW = >> Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2710-rpi-3-b.dtb >> - } >> - FILE FREEFORM = 3D523012-73FE-40E5-892E-1A4DF60F3C0C { >> - SECTION RAW = >> Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2710-rpi-3-b-plus.dtb >> - } >> - >> [FV.FVMAIN_COMPACT] >> FvAlignment = 16 >> ERASE_POLARITY = 1 >> diff --git a/Platform/RaspberryPi/RPi4/RPi4.fdf >> b/Platform/RaspberryPi/RPi4/RPi4.fdf >> index c3e9cfc4..b1f7aa23 100644 >> --- a/Platform/RaspberryPi/RPi4/RPi4.fdf >> +++ b/Platform/RaspberryPi/RPi4/RPi4.fdf >> @@ -307,14 +307,6 @@ READ_LOCK_STATUS = TRUE >> # >> INF Platform/RaspberryPi/Drivers/LogoDxe/LogoDxe.inf >> - # >> - # Device Tree support (used by FdtDxe) >> - # GUIDs should match gRaspberryPi#####FdtGuid's from the .dec >> - # >> - FILE FREEFORM = 80AB6833-CAE4-4CEE-B59D-EB2039B05551 { >> - SECTION RAW = >> Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2711-rpi-4-b.dtb >> - } >> - >> [FV.FVMAIN_COMPACT] >> FvAlignment = 16 >> ERASE_POLARITY = 1 >> diff --git a/Platform/RaspberryPi/RaspberryPi.dec >> b/Platform/RaspberryPi/RaspberryPi.dec >> index b66322be..66ef6186 100644 >> --- a/Platform/RaspberryPi/RaspberryPi.dec >> +++ b/Platform/RaspberryPi/RaspberryPi.dec >> @@ -26,13 +26,6 @@ >> gRaspberryPiTokenSpaceGuid = {0xCD7CC258, 0x31DB, 0x11E6, {0x9F, >> 0xD3, 0x63, 0xB0, 0xB8, 0xEE, 0xD6, 0xB5}} >> gRaspberryPiEventResetGuid = {0xCD7CC258, 0x31DB, 0x11E6, {0x9F, >> 0xD3, 0x63, 0xB4, 0xB4, 0xE4, 0xD4, 0xB4}} >> gConfigDxeFormSetGuid = {0xCD7CC258, 0x31DB, 0x22E6, {0x9F, 0x22, >> 0x63, 0xB0, 0xB8, 0xEE, 0xD6, 0xB5}} >> - # GUIDs used by FdtDxe to serve a Device Tree at runtime. Not all >> of these need to apply >> - # to the current platform or match an actual FDF binary, but they >> need to be defined. >> - gRaspberryPi3ModelBFdtGuid = { 0xDF5DA223, 0x1D27, 0x47C3, { 0x8D, >> 0x1B, 0x9A, 0x41, 0xB5, 0x5A, 0x18, 0xBC } } >> - gRaspberryPi3ModelBPlusFdtGuid = { 0x3D523012, 0x73FE, 0x40E5, { >> 0x89, 0x2E, 0x1A, 0x4D, 0xF6, 0x0F, 0x3C, 0x0C } } >> - gRaspberryPi4ModelBFdtGuid = { 0x80AB6833, 0xCAE4, 0x4CEE, { 0xB5, >> 0x9D, 0xEB, 0x20, 0x39, 0xB0, 0x55, 0x51 } } >> - # Default Fdt to serve if the hardware model can't be detected. >> Should match one of the above. >> - gRaspberryPiDefaultFdtGuid = {0xDF5DA223, 0x1D27, 0x47C3, { 0x8D, >> 0x1B, 0x9A, 0x41, 0xB5, 0x5A, 0x18, 0xBC}} >> [PcdsFixedAtBuild.common] >> # >> > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-platforms][PATCH 1/2] RPi: rip out FdtDxe logic to use internal DTB 2020-05-01 15:11 ` Pete Batard @ 2020-05-01 15:13 ` Ard Biesheuvel 2020-05-01 15:16 ` Pete Batard 0 siblings, 1 reply; 12+ messages in thread From: Ard Biesheuvel @ 2020-05-01 15:13 UTC (permalink / raw) To: Pete Batard, Andrei Warkentin, devel; +Cc: leif, philmd On 5/1/20 5:11 PM, Pete Batard wrote: > On 2020.05.01 14:19, Ard Biesheuvel wrote: >> On 4/30/20 11:16 PM, Andrei Warkentin wrote: >>> Initially, FdtDxe used an internal (embedded in UEFI) FDT, >>> because it was neither understood how to consume the >>> one loaded by the VideoCore firmware, nor understood just >>> how important it is to use the DTB provided by config.txt. >>> >>> Embedding the DT was a bad idea, because: >>> - Permanently stale >>> - No overlays >>> >>> Also, on devices like the Pi 4 you _have_ to have a DT >>> around for the start4 VPU firmware to pick up, otherwise >>> the board is left in an inconsistent state. So we're being >>> proscriptive now about DT use with config.txt, which means >>> this internal DT logic is deadc code. >>> >>> Further FdtDxe cleanups are possible and will be handled >>> separately, specifically: >>> - probably no need to use a separate allocation for patched DT >>> (optimize memory used) >>> - suspicious use of EfiBootServicesData (I filed >>> https://github.com/ARM-software/ebbr/issues/45 to sort out the real >>> requirements) >>> >>> Testing: Booted Ubuntu 18.04 on Pi 2B (1.2). >>> >>> Signed-off-by: Andrei Warkentin <andrey.warkentin@gmail.com> >>> --- >>> Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c | 206 >>> ++++---------------- >>> Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf | 4 - >>> Platform/RaspberryPi/RPi3/RPi3.fdf | 11 -- >>> Platform/RaspberryPi/RPi4/RPi4.fdf | 8 - >>> Platform/RaspberryPi/RaspberryPi.dec | 7 - >>> 5 files changed, 38 insertions(+), 198 deletions(-) >>> >>> diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c >>> b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c >>> index e7143f57..187b9566 100644 >>> --- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c >>> +++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c >>> @@ -335,90 +335,6 @@ CleanSimpleFramebuffer ( >>> return EFI_SUCCESS; >>> } >>> -#define MAX_CMDLINE_SIZE 512 >>> - >>> -STATIC >>> -EFI_STATUS >>> -UpdateBootArgs ( >>> - VOID >>> - ) >>> -{ >>> - INTN Node; >>> - INTN Retval; >>> - EFI_STATUS Status; >>> - CHAR8 *CommandLine; >>> - >>> - // >>> - // Locate the /chosen node >>> - // >>> - Node = fdt_path_offset (mFdtImage, "/chosen"); >>> - if (Node < 0) { >>> - DEBUG ((DEBUG_ERROR, "%a: failed to locate /chosen node\n", >>> __FUNCTION__)); >>> - return EFI_NOT_FOUND; >>> - } >>> - >>> - // >>> - // If /chosen/bootargs already exists, we want to add a space >>> character >>> - // before adding the firmware supplied arguments. However, the >>> RpiFirmware >>> - // protocol expects a 32-bit aligned buffer. So let's allocate 4 >>> bytes of >>> - // slack, and skip the first 3 when passing this buffer into libfdt. >>> - // >>> - CommandLine = AllocatePool (MAX_CMDLINE_SIZE) + 4; >>> - if (!CommandLine) { >>> - DEBUG ((DEBUG_ERROR, "%a: failed to allocate memory\n", >>> __FUNCTION__)); >>> - return EFI_OUT_OF_RESOURCES; >>> - } >>> - >>> - // >>> - // Get the command line from the firmware >>> - // >>> - Status = mFwProtocol->GetCommandLine (MAX_CMDLINE_SIZE, >>> CommandLine + 4); >>> - if (EFI_ERROR (Status)) { >>> - DEBUG ((DEBUG_ERROR, "%a: failed to retrieve command line\n", >>> __FUNCTION__)); >>> - return Status; >>> - } >>> - >>> - if (AsciiStrLen (CommandLine + 4) == 0) { >>> - DEBUG ((DEBUG_INFO, "%a: empty command line received\n", >>> __FUNCTION__)); >>> - return EFI_SUCCESS; >>> - } >>> - >>> - CommandLine[3] = ' '; >>> - >>> - Retval = fdt_appendprop_string (mFdtImage, Node, "bootargs", >>> &CommandLine[3]); >>> - if (Retval != 0) { >>> - DEBUG ((DEBUG_ERROR, "%a: failed to set /chosen/bootargs >>> property (%d)\n", >>> - __FUNCTION__, Retval)); >>> - } >>> - >>> - DEBUG_CODE_BEGIN (); >>> - CONST CHAR8 *Prop; >>> - INT32 Length; >>> - INT32 Index; >>> - >>> - Node = fdt_path_offset (mFdtImage, "/chosen"); >>> - ASSERT (Node >= 0); >>> - >>> - Prop = fdt_getprop (mFdtImage, Node, "bootargs", &Length); >>> - ASSERT (Prop != NULL); >>> - >>> - DEBUG ((DEBUG_INFO, "Command line set from firmware (length >>> %d):\n'", Length)); >>> - >>> - for (Index = 0; Index < Length; Index++, Prop++) { >>> - if (*Prop == '\0') { >>> - continue; >>> - } >>> - DEBUG ((DEBUG_INFO, "%c", *Prop)); >>> - } >>> - >>> - DEBUG ((DEBUG_INFO, "'\n")); >>> - DEBUG_CODE_END (); >>> - >>> - FreePool (CommandLine - 4); >>> - return EFI_SUCCESS; >>> -} >>> - >>> - >>> /** >>> @param ImageHandle of the loaded driver >>> @param SystemTable Pointer to the System Table >>> @@ -435,13 +351,10 @@ FdtDxeInitialize ( >>> IN EFI_SYSTEM_TABLE *SystemTable >>> ) >>> { >>> + INT32 Retval; >>> EFI_STATUS Status; >>> - EFI_GUID *FdtGuid; >>> - VOID *FdtImage; >>> UINTN FdtSize; >>> - INT32 Retval; >>> - UINT32 BoardRevision; >>> - BOOLEAN Internal; >>> + VOID *FdtImage = NULL; >>> if (PcdGet32 (PcdOptDeviceTree) == 0) { >>> DEBUG ((DEBUG_INFO, "Device Tree disabled per user >>> configuration\n")); >>> @@ -450,77 +363,28 @@ FdtDxeInitialize ( >>> Status = gBS->LocateProtocol (&gRaspberryPiFirmwareProtocolGuid, >>> NULL, >>> (VOID**)&mFwProtocol); >>> - ASSERT_EFI_ERROR (Status); >>> + if (mFwProtocol == NULL) { >>> + /* >>> + * Impossible because of the depex... >>> + */ >>> + ASSERT_EFI_ERROR (Status); >>> + return EFI_NOT_FOUND; >>> + } >> >> Do we need this change? > > Looking at what we are doing elsewhere, I thing we should go with: > > ASSERT_EFI_ERROR (Status); > if (EFI_ERROR (Status)) { > return Status; > } > > The ASSERT_EFI_ERROR () on its own was not enough for RELEASE, so we > definitely want to return an error code here if needed, and I guess > that, technically, we could consider that LocateProtocol () may succeed > and return a NULL value for mFwProtocol, but I doubt that's a valid > consideration. > But the DEPEX guarantees that the module will only be dispatched at a time when LocateProtocol() will succeed. This is a common pattern, so I wonder why it is being silently modified here. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-platforms][PATCH 1/2] RPi: rip out FdtDxe logic to use internal DTB 2020-05-01 15:13 ` Ard Biesheuvel @ 2020-05-01 15:16 ` Pete Batard 2020-05-01 16:56 ` [edk2-devel] " Andrei Warkentin 0 siblings, 1 reply; 12+ messages in thread From: Pete Batard @ 2020-05-01 15:16 UTC (permalink / raw) To: Ard Biesheuvel, Andrei Warkentin, devel; +Cc: leif, philmd On 2020.05.01 16:13, Ard Biesheuvel wrote: > On 5/1/20 5:11 PM, Pete Batard wrote: >> On 2020.05.01 14:19, Ard Biesheuvel wrote: >>> On 4/30/20 11:16 PM, Andrei Warkentin wrote: >>>> Initially, FdtDxe used an internal (embedded in UEFI) FDT, >>>> because it was neither understood how to consume the >>>> one loaded by the VideoCore firmware, nor understood just >>>> how important it is to use the DTB provided by config.txt. >>>> >>>> Embedding the DT was a bad idea, because: >>>> - Permanently stale >>>> - No overlays >>>> >>>> Also, on devices like the Pi 4 you _have_ to have a DT >>>> around for the start4 VPU firmware to pick up, otherwise >>>> the board is left in an inconsistent state. So we're being >>>> proscriptive now about DT use with config.txt, which means >>>> this internal DT logic is deadc code. >>>> >>>> Further FdtDxe cleanups are possible and will be handled >>>> separately, specifically: >>>> - probably no need to use a separate allocation for patched DT >>>> (optimize memory used) >>>> - suspicious use of EfiBootServicesData (I filed >>>> https://github.com/ARM-software/ebbr/issues/45 to sort out the real >>>> requirements) >>>> >>>> Testing: Booted Ubuntu 18.04 on Pi 2B (1.2). >>>> >>>> Signed-off-by: Andrei Warkentin <andrey.warkentin@gmail.com> >>>> --- >>>> Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c | 206 >>>> ++++---------------- >>>> Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf | 4 - >>>> Platform/RaspberryPi/RPi3/RPi3.fdf | 11 -- >>>> Platform/RaspberryPi/RPi4/RPi4.fdf | 8 - >>>> Platform/RaspberryPi/RaspberryPi.dec | 7 - >>>> 5 files changed, 38 insertions(+), 198 deletions(-) >>>> >>>> diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c >>>> b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c >>>> index e7143f57..187b9566 100644 >>>> --- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c >>>> +++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c >>>> @@ -335,90 +335,6 @@ CleanSimpleFramebuffer ( >>>> return EFI_SUCCESS; >>>> } >>>> -#define MAX_CMDLINE_SIZE 512 >>>> - >>>> -STATIC >>>> -EFI_STATUS >>>> -UpdateBootArgs ( >>>> - VOID >>>> - ) >>>> -{ >>>> - INTN Node; >>>> - INTN Retval; >>>> - EFI_STATUS Status; >>>> - CHAR8 *CommandLine; >>>> - >>>> - // >>>> - // Locate the /chosen node >>>> - // >>>> - Node = fdt_path_offset (mFdtImage, "/chosen"); >>>> - if (Node < 0) { >>>> - DEBUG ((DEBUG_ERROR, "%a: failed to locate /chosen node\n", >>>> __FUNCTION__)); >>>> - return EFI_NOT_FOUND; >>>> - } >>>> - >>>> - // >>>> - // If /chosen/bootargs already exists, we want to add a space >>>> character >>>> - // before adding the firmware supplied arguments. However, the >>>> RpiFirmware >>>> - // protocol expects a 32-bit aligned buffer. So let's allocate 4 >>>> bytes of >>>> - // slack, and skip the first 3 when passing this buffer into libfdt. >>>> - // >>>> - CommandLine = AllocatePool (MAX_CMDLINE_SIZE) + 4; >>>> - if (!CommandLine) { >>>> - DEBUG ((DEBUG_ERROR, "%a: failed to allocate memory\n", >>>> __FUNCTION__)); >>>> - return EFI_OUT_OF_RESOURCES; >>>> - } >>>> - >>>> - // >>>> - // Get the command line from the firmware >>>> - // >>>> - Status = mFwProtocol->GetCommandLine (MAX_CMDLINE_SIZE, >>>> CommandLine + 4); >>>> - if (EFI_ERROR (Status)) { >>>> - DEBUG ((DEBUG_ERROR, "%a: failed to retrieve command line\n", >>>> __FUNCTION__)); >>>> - return Status; >>>> - } >>>> - >>>> - if (AsciiStrLen (CommandLine + 4) == 0) { >>>> - DEBUG ((DEBUG_INFO, "%a: empty command line received\n", >>>> __FUNCTION__)); >>>> - return EFI_SUCCESS; >>>> - } >>>> - >>>> - CommandLine[3] = ' '; >>>> - >>>> - Retval = fdt_appendprop_string (mFdtImage, Node, "bootargs", >>>> &CommandLine[3]); >>>> - if (Retval != 0) { >>>> - DEBUG ((DEBUG_ERROR, "%a: failed to set /chosen/bootargs >>>> property (%d)\n", >>>> - __FUNCTION__, Retval)); >>>> - } >>>> - >>>> - DEBUG_CODE_BEGIN (); >>>> - CONST CHAR8 *Prop; >>>> - INT32 Length; >>>> - INT32 Index; >>>> - >>>> - Node = fdt_path_offset (mFdtImage, "/chosen"); >>>> - ASSERT (Node >= 0); >>>> - >>>> - Prop = fdt_getprop (mFdtImage, Node, "bootargs", &Length); >>>> - ASSERT (Prop != NULL); >>>> - >>>> - DEBUG ((DEBUG_INFO, "Command line set from firmware (length >>>> %d):\n'", Length)); >>>> - >>>> - for (Index = 0; Index < Length; Index++, Prop++) { >>>> - if (*Prop == '\0') { >>>> - continue; >>>> - } >>>> - DEBUG ((DEBUG_INFO, "%c", *Prop)); >>>> - } >>>> - >>>> - DEBUG ((DEBUG_INFO, "'\n")); >>>> - DEBUG_CODE_END (); >>>> - >>>> - FreePool (CommandLine - 4); >>>> - return EFI_SUCCESS; >>>> -} >>>> - >>>> - >>>> /** >>>> @param ImageHandle of the loaded driver >>>> @param SystemTable Pointer to the System Table >>>> @@ -435,13 +351,10 @@ FdtDxeInitialize ( >>>> IN EFI_SYSTEM_TABLE *SystemTable >>>> ) >>>> { >>>> + INT32 Retval; >>>> EFI_STATUS Status; >>>> - EFI_GUID *FdtGuid; >>>> - VOID *FdtImage; >>>> UINTN FdtSize; >>>> - INT32 Retval; >>>> - UINT32 BoardRevision; >>>> - BOOLEAN Internal; >>>> + VOID *FdtImage = NULL; >>>> if (PcdGet32 (PcdOptDeviceTree) == 0) { >>>> DEBUG ((DEBUG_INFO, "Device Tree disabled per user >>>> configuration\n")); >>>> @@ -450,77 +363,28 @@ FdtDxeInitialize ( >>>> Status = gBS->LocateProtocol (&gRaspberryPiFirmwareProtocolGuid, >>>> NULL, >>>> (VOID**)&mFwProtocol); >>>> - ASSERT_EFI_ERROR (Status); >>>> + if (mFwProtocol == NULL) { >>>> + /* >>>> + * Impossible because of the depex... >>>> + */ >>>> + ASSERT_EFI_ERROR (Status); >>>> + return EFI_NOT_FOUND; >>>> + } >>> >>> Do we need this change? >> >> Looking at what we are doing elsewhere, I thing we should go with: >> >> ASSERT_EFI_ERROR (Status); >> if (EFI_ERROR (Status)) { >> return Status; >> } >> >> The ASSERT_EFI_ERROR () on its own was not enough for RELEASE, so we >> definitely want to return an error code here if needed, and I guess >> that, technically, we could consider that LocateProtocol () may >> succeed and return a NULL value for mFwProtocol, but I doubt that's a >> valid consideration. >> > > But the DEPEX guarantees that the module will only be dispatched at a > time when LocateProtocol() will succeed. This is a common pattern, so I > wonder why it is being silently modified here. > Fair enough. Let's wait for Andrei to reply on this. /Pete ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [edk2-platforms][PATCH 1/2] RPi: rip out FdtDxe logic to use internal DTB 2020-05-01 15:16 ` Pete Batard @ 2020-05-01 16:56 ` Andrei Warkentin 2020-05-01 17:04 ` Ard Biesheuvel 0 siblings, 1 reply; 12+ messages in thread From: Andrei Warkentin @ 2020-05-01 16:56 UTC (permalink / raw) To: Ard Biesheuvel, Andrei Warkentin, devel@edk2.groups.io, pete@akeo.ie Cc: leif@nuviainc.com, philmd@redhat.com [-- Attachment #1: Type: text/plain, Size: 7871 bytes --] Hi folks, I added that specific line more as a safeguard in case the depex ever changes, as I wasn't comfortable with code that relied on an external factor to have defined behavior. I don't have a strong position on it. If you want it to go, I'll send an update. A ________________________________ From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Pete Batard via groups.io <pete=akeo.ie@groups.io> Sent: Friday, May 1, 2020 10:16 AM To: Ard Biesheuvel <ard.biesheuvel@arm.com>; Andrei Warkentin <andrey.warkentin@gmail.com>; devel@edk2.groups.io <devel@edk2.groups.io> Cc: leif@nuviainc.com <leif@nuviainc.com>; philmd@redhat.com <philmd@redhat.com> Subject: Re: [edk2-devel] [edk2-platforms][PATCH 1/2] RPi: rip out FdtDxe logic to use internal DTB On 2020.05.01 16:13, Ard Biesheuvel wrote: > On 5/1/20 5:11 PM, Pete Batard wrote: >> On 2020.05.01 14:19, Ard Biesheuvel wrote: >>> On 4/30/20 11:16 PM, Andrei Warkentin wrote: >>>> Initially, FdtDxe used an internal (embedded in UEFI) FDT, >>>> because it was neither understood how to consume the >>>> one loaded by the VideoCore firmware, nor understood just >>>> how important it is to use the DTB provided by config.txt. >>>> >>>> Embedding the DT was a bad idea, because: >>>> - Permanently stale >>>> - No overlays >>>> >>>> Also, on devices like the Pi 4 you _have_ to have a DT >>>> around for the start4 VPU firmware to pick up, otherwise >>>> the board is left in an inconsistent state. So we're being >>>> proscriptive now about DT use with config.txt, which means >>>> this internal DT logic is deadc code. >>>> >>>> Further FdtDxe cleanups are possible and will be handled >>>> separately, specifically: >>>> - probably no need to use a separate allocation for patched DT >>>> (optimize memory used) >>>> - suspicious use of EfiBootServicesData (I filed >>>> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FARM-software%2Febbr%2Fissues%2F45&data=02%7C01%7Cawarkentin%40vmware.com%7Ccd65ce9968f54b569f8208d7ede2a025%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637239429916614640&sdata=pJ3Wu%2BGJhj%2FqfbBEFMQ9nQ%2B1Pgc%2Bo4Xw0fer2h9ZYvQ%3D&reserved=0 to sort out the real >>>> requirements) >>>> >>>> Testing: Booted Ubuntu 18.04 on Pi 2B (1.2). >>>> >>>> Signed-off-by: Andrei Warkentin <andrey.warkentin@gmail.com> >>>> --- >>>> Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c | 206 >>>> ++++---------------- >>>> Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf | 4 - >>>> Platform/RaspberryPi/RPi3/RPi3.fdf | 11 -- >>>> Platform/RaspberryPi/RPi4/RPi4.fdf | 8 - >>>> Platform/RaspberryPi/RaspberryPi.dec | 7 - >>>> 5 files changed, 38 insertions(+), 198 deletions(-) >>>> >>>> diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c >>>> b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c >>>> index e7143f57..187b9566 100644 >>>> --- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c >>>> +++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c >>>> @@ -335,90 +335,6 @@ CleanSimpleFramebuffer ( >>>> return EFI_SUCCESS; >>>> } >>>> -#define MAX_CMDLINE_SIZE 512 >>>> - >>>> -STATIC >>>> -EFI_STATUS >>>> -UpdateBootArgs ( >>>> - VOID >>>> - ) >>>> -{ >>>> - INTN Node; >>>> - INTN Retval; >>>> - EFI_STATUS Status; >>>> - CHAR8 *CommandLine; >>>> - >>>> - // >>>> - // Locate the /chosen node >>>> - // >>>> - Node = fdt_path_offset (mFdtImage, "/chosen"); >>>> - if (Node < 0) { >>>> - DEBUG ((DEBUG_ERROR, "%a: failed to locate /chosen node\n", >>>> __FUNCTION__)); >>>> - return EFI_NOT_FOUND; >>>> - } >>>> - >>>> - // >>>> - // If /chosen/bootargs already exists, we want to add a space >>>> character >>>> - // before adding the firmware supplied arguments. However, the >>>> RpiFirmware >>>> - // protocol expects a 32-bit aligned buffer. So let's allocate 4 >>>> bytes of >>>> - // slack, and skip the first 3 when passing this buffer into libfdt. >>>> - // >>>> - CommandLine = AllocatePool (MAX_CMDLINE_SIZE) + 4; >>>> - if (!CommandLine) { >>>> - DEBUG ((DEBUG_ERROR, "%a: failed to allocate memory\n", >>>> __FUNCTION__)); >>>> - return EFI_OUT_OF_RESOURCES; >>>> - } >>>> - >>>> - // >>>> - // Get the command line from the firmware >>>> - // >>>> - Status = mFwProtocol->GetCommandLine (MAX_CMDLINE_SIZE, >>>> CommandLine + 4); >>>> - if (EFI_ERROR (Status)) { >>>> - DEBUG ((DEBUG_ERROR, "%a: failed to retrieve command line\n", >>>> __FUNCTION__)); >>>> - return Status; >>>> - } >>>> - >>>> - if (AsciiStrLen (CommandLine + 4) == 0) { >>>> - DEBUG ((DEBUG_INFO, "%a: empty command line received\n", >>>> __FUNCTION__)); >>>> - return EFI_SUCCESS; >>>> - } >>>> - >>>> - CommandLine[3] = ' '; >>>> - >>>> - Retval = fdt_appendprop_string (mFdtImage, Node, "bootargs", >>>> &CommandLine[3]); >>>> - if (Retval != 0) { >>>> - DEBUG ((DEBUG_ERROR, "%a: failed to set /chosen/bootargs >>>> property (%d)\n", >>>> - __FUNCTION__, Retval)); >>>> - } >>>> - >>>> - DEBUG_CODE_BEGIN (); >>>> - CONST CHAR8 *Prop; >>>> - INT32 Length; >>>> - INT32 Index; >>>> - >>>> - Node = fdt_path_offset (mFdtImage, "/chosen"); >>>> - ASSERT (Node >= 0); >>>> - >>>> - Prop = fdt_getprop (mFdtImage, Node, "bootargs", &Length); >>>> - ASSERT (Prop != NULL); >>>> - >>>> - DEBUG ((DEBUG_INFO, "Command line set from firmware (length >>>> %d):\n'", Length)); >>>> - >>>> - for (Index = 0; Index < Length; Index++, Prop++) { >>>> - if (*Prop == '\0') { >>>> - continue; >>>> - } >>>> - DEBUG ((DEBUG_INFO, "%c", *Prop)); >>>> - } >>>> - >>>> - DEBUG ((DEBUG_INFO, "'\n")); >>>> - DEBUG_CODE_END (); >>>> - >>>> - FreePool (CommandLine - 4); >>>> - return EFI_SUCCESS; >>>> -} >>>> - >>>> - >>>> /** >>>> @param ImageHandle of the loaded driver >>>> @param SystemTable Pointer to the System Table >>>> @@ -435,13 +351,10 @@ FdtDxeInitialize ( >>>> IN EFI_SYSTEM_TABLE *SystemTable >>>> ) >>>> { >>>> + INT32 Retval; >>>> EFI_STATUS Status; >>>> - EFI_GUID *FdtGuid; >>>> - VOID *FdtImage; >>>> UINTN FdtSize; >>>> - INT32 Retval; >>>> - UINT32 BoardRevision; >>>> - BOOLEAN Internal; >>>> + VOID *FdtImage = NULL; >>>> if (PcdGet32 (PcdOptDeviceTree) == 0) { >>>> DEBUG ((DEBUG_INFO, "Device Tree disabled per user >>>> configuration\n")); >>>> @@ -450,77 +363,28 @@ FdtDxeInitialize ( >>>> Status = gBS->LocateProtocol (&gRaspberryPiFirmwareProtocolGuid, >>>> NULL, >>>> (VOID**)&mFwProtocol); >>>> - ASSERT_EFI_ERROR (Status); >>>> + if (mFwProtocol == NULL) { >>>> + /* >>>> + * Impossible because of the depex... >>>> + */ >>>> + ASSERT_EFI_ERROR (Status); >>>> + return EFI_NOT_FOUND; >>>> + } >>> >>> Do we need this change? >> >> Looking at what we are doing elsewhere, I thing we should go with: >> >> ASSERT_EFI_ERROR (Status); >> if (EFI_ERROR (Status)) { >> return Status; >> } >> >> The ASSERT_EFI_ERROR () on its own was not enough for RELEASE, so we >> definitely want to return an error code here if needed, and I guess >> that, technically, we could consider that LocateProtocol () may >> succeed and return a NULL value for mFwProtocol, but I doubt that's a >> valid consideration. >> > > But the DEPEX guarantees that the module will only be dispatched at a > time when LocateProtocol() will succeed. This is a common pattern, so I > wonder why it is being silently modified here. > Fair enough. Let's wait for Andrei to reply on this. /Pete [-- Attachment #2: Type: text/html, Size: 14679 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [edk2-platforms][PATCH 1/2] RPi: rip out FdtDxe logic to use internal DTB 2020-05-01 16:56 ` [edk2-devel] " Andrei Warkentin @ 2020-05-01 17:04 ` Ard Biesheuvel 0 siblings, 0 replies; 12+ messages in thread From: Ard Biesheuvel @ 2020-05-01 17:04 UTC (permalink / raw) To: Andrei Warkentin, Andrei Warkentin, devel@edk2.groups.io, pete@akeo.ie Cc: leif@nuviainc.com, philmd@redhat.com On 5/1/20 6:56 PM, Andrei Warkentin wrote: > Hi folks, > > I added that specific line more as a safeguard in case the depex ever > changes, as I wasn't comfortable with code that relied on an external > factor to have defined behavior. > > I don't have a strong position on it. If you want it to go, I'll send an > update. > No worries, I'll just drop it from the patch. > A > ------------------------------------------------------------------------ > *From:* devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Pete > Batard via groups.io <pete=akeo.ie@groups.io> > *Sent:* Friday, May 1, 2020 10:16 AM > *To:* Ard Biesheuvel <ard.biesheuvel@arm.com>; Andrei Warkentin > <andrey.warkentin@gmail.com>; devel@edk2.groups.io <devel@edk2.groups.io> > *Cc:* leif@nuviainc.com <leif@nuviainc.com>; philmd@redhat.com > <philmd@redhat.com> > *Subject:* Re: [edk2-devel] [edk2-platforms][PATCH 1/2] RPi: rip out > FdtDxe logic to use internal DTB > On 2020.05.01 16:13, Ard Biesheuvel wrote: >> On 5/1/20 5:11 PM, Pete Batard wrote: >>> On 2020.05.01 14:19, Ard Biesheuvel wrote: >>>> On 4/30/20 11:16 PM, Andrei Warkentin wrote: >>>>> Initially, FdtDxe used an internal (embedded in UEFI) FDT, >>>>> because it was neither understood how to consume the >>>>> one loaded by the VideoCore firmware, nor understood just >>>>> how important it is to use the DTB provided by config.txt. >>>>> >>>>> Embedding the DT was a bad idea, because: >>>>> - Permanently stale >>>>> - No overlays >>>>> >>>>> Also, on devices like the Pi 4 you _have_ to have a DT >>>>> around for the start4 VPU firmware to pick up, otherwise >>>>> the board is left in an inconsistent state. So we're being >>>>> proscriptive now about DT use with config.txt, which means >>>>> this internal DT logic is deadc code. >>>>> >>>>> Further FdtDxe cleanups are possible and will be handled >>>>> separately, specifically: >>>>> - probably no need to use a separate allocation for patched DT >>>>> (optimize memory used) >>>>> - suspicious use of EfiBootServicesData (I filed >>>>> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FARM-software%2Febbr%2Fissues%2F45&data=02%7C01%7Cawarkentin%40vmware.com%7Ccd65ce9968f54b569f8208d7ede2a025%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637239429916614640&sdata=pJ3Wu%2BGJhj%2FqfbBEFMQ9nQ%2B1Pgc%2Bo4Xw0fer2h9ZYvQ%3D&reserved=0 > to sort out the real >>>>> requirements) >>>>> >>>>> Testing: Booted Ubuntu 18.04 on Pi 2B (1.2). >>>>> >>>>> Signed-off-by: Andrei Warkentin <andrey.warkentin@gmail.com> >>>>> --- >>>>> Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c | 206 >>>>> ++++---------------- >>>>> Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf | 4 - >>>>> Platform/RaspberryPi/RPi3/RPi3.fdf | 11 -- >>>>> Platform/RaspberryPi/RPi4/RPi4.fdf | 8 - >>>>> Platform/RaspberryPi/RaspberryPi.dec | 7 - >>>>> 5 files changed, 38 insertions(+), 198 deletions(-) >>>>> >>>>> diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c >>>>> b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c >>>>> index e7143f57..187b9566 100644 >>>>> --- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c >>>>> +++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c >>>>> @@ -335,90 +335,6 @@ CleanSimpleFramebuffer ( >>>>> return EFI_SUCCESS; >>>>> } >>>>> -#define MAX_CMDLINE_SIZE 512 >>>>> - >>>>> -STATIC >>>>> -EFI_STATUS >>>>> -UpdateBootArgs ( >>>>> - VOID >>>>> - ) >>>>> -{ >>>>> - INTN Node; >>>>> - INTN Retval; >>>>> - EFI_STATUS Status; >>>>> - CHAR8 *CommandLine; >>>>> - >>>>> - // >>>>> - // Locate the /chosen node >>>>> - // >>>>> - Node = fdt_path_offset (mFdtImage, "/chosen"); >>>>> - if (Node < 0) { >>>>> - DEBUG ((DEBUG_ERROR, "%a: failed to locate /chosen node\n", >>>>> __FUNCTION__)); >>>>> - return EFI_NOT_FOUND; >>>>> - } >>>>> - >>>>> - // >>>>> - // If /chosen/bootargs already exists, we want to add a space >>>>> character >>>>> - // before adding the firmware supplied arguments. However, the >>>>> RpiFirmware >>>>> - // protocol expects a 32-bit aligned buffer. So let's allocate 4 >>>>> bytes of >>>>> - // slack, and skip the first 3 when passing this buffer into libfdt. >>>>> - // >>>>> - CommandLine = AllocatePool (MAX_CMDLINE_SIZE) + 4; >>>>> - if (!CommandLine) { >>>>> - DEBUG ((DEBUG_ERROR, "%a: failed to allocate memory\n", >>>>> __FUNCTION__)); >>>>> - return EFI_OUT_OF_RESOURCES; >>>>> - } >>>>> - >>>>> - // >>>>> - // Get the command line from the firmware >>>>> - // >>>>> - Status = mFwProtocol->GetCommandLine (MAX_CMDLINE_SIZE, >>>>> CommandLine + 4); >>>>> - if (EFI_ERROR (Status)) { >>>>> - DEBUG ((DEBUG_ERROR, "%a: failed to retrieve command line\n", >>>>> __FUNCTION__)); >>>>> - return Status; >>>>> - } >>>>> - >>>>> - if (AsciiStrLen (CommandLine + 4) == 0) { >>>>> - DEBUG ((DEBUG_INFO, "%a: empty command line received\n", >>>>> __FUNCTION__)); >>>>> - return EFI_SUCCESS; >>>>> - } >>>>> - >>>>> - CommandLine[3] = ' '; >>>>> - >>>>> - Retval = fdt_appendprop_string (mFdtImage, Node, "bootargs", >>>>> &CommandLine[3]); >>>>> - if (Retval != 0) { >>>>> - DEBUG ((DEBUG_ERROR, "%a: failed to set /chosen/bootargs >>>>> property (%d)\n", >>>>> - __FUNCTION__, Retval)); >>>>> - } >>>>> - >>>>> - DEBUG_CODE_BEGIN (); >>>>> - CONST CHAR8 *Prop; >>>>> - INT32 Length; >>>>> - INT32 Index; >>>>> - >>>>> - Node = fdt_path_offset (mFdtImage, "/chosen"); >>>>> - ASSERT (Node >= 0); >>>>> - >>>>> - Prop = fdt_getprop (mFdtImage, Node, "bootargs", &Length); >>>>> - ASSERT (Prop != NULL); >>>>> - >>>>> - DEBUG ((DEBUG_INFO, "Command line set from firmware (length >>>>> %d):\n'", Length)); >>>>> - >>>>> - for (Index = 0; Index < Length; Index++, Prop++) { >>>>> - if (*Prop == '\0') { >>>>> - continue; >>>>> - } >>>>> - DEBUG ((DEBUG_INFO, "%c", *Prop)); >>>>> - } >>>>> - >>>>> - DEBUG ((DEBUG_INFO, "'\n")); >>>>> - DEBUG_CODE_END (); >>>>> - >>>>> - FreePool (CommandLine - 4); >>>>> - return EFI_SUCCESS; >>>>> -} >>>>> - >>>>> - >>>>> /** >>>>> @param ImageHandle of the loaded driver >>>>> @param SystemTable Pointer to the System Table >>>>> @@ -435,13 +351,10 @@ FdtDxeInitialize ( >>>>> IN EFI_SYSTEM_TABLE *SystemTable >>>>> ) >>>>> { >>>>> + INT32 Retval; >>>>> EFI_STATUS Status; >>>>> - EFI_GUID *FdtGuid; >>>>> - VOID *FdtImage; >>>>> UINTN FdtSize; >>>>> - INT32 Retval; >>>>> - UINT32 BoardRevision; >>>>> - BOOLEAN Internal; >>>>> + VOID *FdtImage = NULL; >>>>> if (PcdGet32 (PcdOptDeviceTree) == 0) { >>>>> DEBUG ((DEBUG_INFO, "Device Tree disabled per user >>>>> configuration\n")); >>>>> @@ -450,77 +363,28 @@ FdtDxeInitialize ( >>>>> Status = gBS->LocateProtocol (&gRaspberryPiFirmwareProtocolGuid, >>>>> NULL, >>>>> (VOID**)&mFwProtocol); >>>>> - ASSERT_EFI_ERROR (Status); >>>>> + if (mFwProtocol == NULL) { >>>>> + /* >>>>> + * Impossible because of the depex... >>>>> + */ >>>>> + ASSERT_EFI_ERROR (Status); >>>>> + return EFI_NOT_FOUND; >>>>> + } >>>> >>>> Do we need this change? >>> >>> Looking at what we are doing elsewhere, I thing we should go with: >>> >>> ASSERT_EFI_ERROR (Status); >>> if (EFI_ERROR (Status)) { >>> return Status; >>> } >>> >>> The ASSERT_EFI_ERROR () on its own was not enough for RELEASE, so we >>> definitely want to return an error code here if needed, and I guess >>> that, technically, we could consider that LocateProtocol () may >>> succeed and return a NULL value for mFwProtocol, but I doubt that's a >>> valid consideration. >>> >> >> But the DEPEX guarantees that the module will only be dispatched at a >> time when LocateProtocol() will succeed. This is a common pattern, so I >> wonder why it is being silently modified here. >> > > Fair enough. Let's wait for Andrei to reply on this. > > /Pete > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [edk2-platforms][PATCH 2/2] RPi: remove any mention of an "internal DTB" 2020-04-30 21:16 [edk2-platforms][PATCH 0/2] Rip out "internal DTB" support in FdtDxe Andrei Warkentin 2020-04-30 21:16 ` [edk2-platforms][PATCH 1/2] RPi: rip out FdtDxe logic to use internal DTB Andrei Warkentin @ 2020-04-30 21:16 ` Andrei Warkentin 2020-05-01 10:47 ` Pete Batard 2020-05-01 17:08 ` [edk2-platforms][PATCH 0/2] Rip out "internal DTB" support in FdtDxe Ard Biesheuvel 2 siblings, 1 reply; 12+ messages in thread From: Andrei Warkentin @ 2020-04-30 21:16 UTC (permalink / raw) To: devel; +Cc: ard.biesheuvel, leif, pete, philmd For Pi 3 and Pi 4, since the relevant functionality is gone from FdtDxe. Signed-off-by: Andrei Warkentin <andrey.warkentin@gmail.com> --- Platform/RaspberryPi/RPi3/Readme.md | 3 +-- Platform/RaspberryPi/RPi4/Readme.md | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/Platform/RaspberryPi/RPi3/Readme.md b/Platform/RaspberryPi/RPi3/Readme.md index f19d59d8..66f52bee 100644 --- a/Platform/RaspberryPi/RPi3/Readme.md +++ b/Platform/RaspberryPi/RPi3/Readme.md @@ -125,8 +125,7 @@ Note: the address range **must** be `[0x1f0000:0x200000]`. `dtoverlay` and `dtpa This firmware will honor the command line passed by the GPU via `cmdline.txt`. Note, that the ultimate contents of `/chosen/bootargs` are a combination of several pieces: -- Original `/chosen/bootargs` if using the internal DTB. Seems to be completely discarded by GPU when booting with a custom device tree. -- GPU-passed hardware configuration. This one is always present. +- GPU-passed hardware configuration. - Additional boot options passed via `cmdline.txt`. # Limitations diff --git a/Platform/RaspberryPi/RPi4/Readme.md b/Platform/RaspberryPi/RPi4/Readme.md index 62a63c4c..03eb6c39 100644 --- a/Platform/RaspberryPi/RPi4/Readme.md +++ b/Platform/RaspberryPi/RPi4/Readme.md @@ -103,8 +103,7 @@ Note: the address range **must** be `[0x1f0000:0x200000]`. `dtoverlay` and `dtpa This firmware will honor the command line passed by the GPU via `cmdline.txt`. Note, that the ultimate contents of `/chosen/bootargs` are a combination of several pieces: -- Original `/chosen/bootargs` if using the internal DTB. Seems to be completely discarded by GPU when booting with a custom device tree. -- GPU-passed hardware configuration. This one is always present. +- GPU-passed hardware configuration. - Additional boot options passed via `cmdline.txt`. # Limitations -- 2.17.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [edk2-platforms][PATCH 2/2] RPi: remove any mention of an "internal DTB" 2020-04-30 21:16 ` [edk2-platforms][PATCH 2/2] RPi: remove any mention of an "internal DTB" Andrei Warkentin @ 2020-05-01 10:47 ` Pete Batard 0 siblings, 0 replies; 12+ messages in thread From: Pete Batard @ 2020-05-01 10:47 UTC (permalink / raw) To: Andrei Warkentin, devel; +Cc: ard.biesheuvel, leif, philmd On 2020.04.30 22:16, Andrei Warkentin wrote: > For Pi 3 and Pi 4, since the relevant functionality is gone from FdtDxe. > > Signed-off-by: Andrei Warkentin <andrey.warkentin@gmail.com> > --- > Platform/RaspberryPi/RPi3/Readme.md | 3 +-- > Platform/RaspberryPi/RPi4/Readme.md | 3 +-- > 2 files changed, 2 insertions(+), 4 deletions(-) > > diff --git a/Platform/RaspberryPi/RPi3/Readme.md b/Platform/RaspberryPi/RPi3/Readme.md > index f19d59d8..66f52bee 100644 > --- a/Platform/RaspberryPi/RPi3/Readme.md > +++ b/Platform/RaspberryPi/RPi3/Readme.md > @@ -125,8 +125,7 @@ Note: the address range **must** be `[0x1f0000:0x200000]`. `dtoverlay` and `dtpa > This firmware will honor the command line passed by the GPU via `cmdline.txt`. > > Note, that the ultimate contents of `/chosen/bootargs` are a combination of several pieces: > -- Original `/chosen/bootargs` if using the internal DTB. Seems to be completely discarded by GPU when booting with a custom device tree. > -- GPU-passed hardware configuration. This one is always present. > +- GPU-passed hardware configuration. > - Additional boot options passed via `cmdline.txt`. > > # Limitations > diff --git a/Platform/RaspberryPi/RPi4/Readme.md b/Platform/RaspberryPi/RPi4/Readme.md > index 62a63c4c..03eb6c39 100644 > --- a/Platform/RaspberryPi/RPi4/Readme.md > +++ b/Platform/RaspberryPi/RPi4/Readme.md > @@ -103,8 +103,7 @@ Note: the address range **must** be `[0x1f0000:0x200000]`. `dtoverlay` and `dtpa > This firmware will honor the command line passed by the GPU via `cmdline.txt`. > > Note, that the ultimate contents of `/chosen/bootargs` are a combination of several pieces: > -- Original `/chosen/bootargs` if using the internal DTB. Seems to be completely discarded by GPU when booting with a custom device tree. > -- GPU-passed hardware configuration. This one is always present. > +- GPU-passed hardware configuration. > - Additional boot options passed via `cmdline.txt`. > > # Limitations > Reviewed-by: Pete Batard <pete@akeo.ie> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-platforms][PATCH 0/2] Rip out "internal DTB" support in FdtDxe 2020-04-30 21:16 [edk2-platforms][PATCH 0/2] Rip out "internal DTB" support in FdtDxe Andrei Warkentin 2020-04-30 21:16 ` [edk2-platforms][PATCH 1/2] RPi: rip out FdtDxe logic to use internal DTB Andrei Warkentin 2020-04-30 21:16 ` [edk2-platforms][PATCH 2/2] RPi: remove any mention of an "internal DTB" Andrei Warkentin @ 2020-05-01 17:08 ` Ard Biesheuvel 2 siblings, 0 replies; 12+ messages in thread From: Ard Biesheuvel @ 2020-05-01 17:08 UTC (permalink / raw) To: Andrei Warkentin, devel; +Cc: leif, pete, philmd On 4/30/20 11:16 PM, Andrei Warkentin wrote: > Dear all, > > This patch set removes the internal (embedded) DTB support from Pi 3 > and Pi 4 platforms, since these platforms should *always* be using > the devicetree passed by the VideoCore firmware. > > Andrei Warkentin (2): > RPi: rip out FdtDxe logic to use internal DTB > RPi: remove any mention of an "internal DTB" > Pushed as defef0562115..9cf38493625a Thanks ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-05-01 17:08 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-04-30 21:16 [edk2-platforms][PATCH 0/2] Rip out "internal DTB" support in FdtDxe Andrei Warkentin 2020-04-30 21:16 ` [edk2-platforms][PATCH 1/2] RPi: rip out FdtDxe logic to use internal DTB Andrei Warkentin 2020-05-01 10:45 ` Pete Batard 2020-05-01 13:19 ` Ard Biesheuvel 2020-05-01 15:11 ` Pete Batard 2020-05-01 15:13 ` Ard Biesheuvel 2020-05-01 15:16 ` Pete Batard 2020-05-01 16:56 ` [edk2-devel] " Andrei Warkentin 2020-05-01 17:04 ` Ard Biesheuvel 2020-04-30 21:16 ` [edk2-platforms][PATCH 2/2] RPi: remove any mention of an "internal DTB" Andrei Warkentin 2020-05-01 10:47 ` Pete Batard 2020-05-01 17:08 ` [edk2-platforms][PATCH 0/2] Rip out "internal DTB" support in FdtDxe Ard Biesheuvel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox