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

["Ard Biesheuvel" <ard.biesheuvel@arm.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&amp;data=02%7C01%7Cawarkentin%40vmware.com%7Ccd65ce9968f54b569f8208d7ede2a025%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637239429916614640&amp;sdata=pJ3Wu%2BGJhj%2FqfbBEFMQ9nQ%2B1Pgc%2Bo4Xw0fer2h9ZYvQ%3D&amp;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
> 
> 
>