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 on behalf of Pete Batard via groups.io Sent: Friday, May 1, 2020 10:16 AM To: Ard Biesheuvel ; Andrei Warkentin ; devel@edk2.groups.io Cc: leif@nuviainc.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 >>>> --- >>>> 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