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

["Pete Batard" <pete@akeo.ie>]

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