Re: [edk2-platforms][PATCH 1/2] RPi: rip out FdtDxe logic to use internal DTB

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

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.