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

["Andrei Warkentin" <awarkentin@vmware.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&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




[-- Attachment #2: Type: text/html, Size: 14679 bytes --]