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

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

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.

Regards,

/Pete

> 
>> -  Internal = FALSE;
>>     FdtImage = (VOID*)(UINTN)PcdGet32 (PcdFdtBaseAddress);
>>     Retval = fdt_check_header (FdtImage);
>> -  if (Retval == 0) {
>> -    /*
>> -     * Have FDT passed via config.txt.
>> -     */
>> -    FdtSize = fdt_totalsize (FdtImage);
>> -    DEBUG ((DEBUG_INFO, "Device Tree passed via config.txt (0x%lx 
>> bytes)\n", FdtSize));
>> -    Status = EFI_SUCCESS;
>> -  } else {
>> -    /*
>> -     * Use one of the embedded FDT's.
>> -     */
>> -    Internal = TRUE;
>> -    DEBUG ((DEBUG_INFO, "No/Bad Device Tree found at address 0x%p 
>> (%a), "
>> -      "looking up internal one...\n", FdtImage, fdt_strerror (Retval)));
>> +  if (Retval != 0) {
>>       /*
>> -     * Query the board revision to differentiate between models.
>> +     * Any one of:
>> +     * - Invalid config.txt device_tree_address (not PcdFdtBaseAddress)
>> +     * - Missing FDT for your Pi variant (if not overriding via 
>> device_tree=)
>>        */
>> -    Status = mFwProtocol->GetModelRevision (&BoardRevision);
>> -    if (EFI_ERROR (Status)) {
>> -      DEBUG ((DEBUG_ERROR, "Failed to get board type: %r\n", Status));
>> -      DEBUG ((DEBUG_INFO, "Using default internal Device Tree\n"));
>> -      FdtGuid = &gRaspberryPiDefaultFdtGuid;
>> -    } else {
>> -      // 
>> www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md 
>>
>> -      switch ((BoardRevision >> 4) & 0xFF) {
>> -      case 0x08:
>> -        DEBUG ((DEBUG_INFO, "Using Raspberry Pi 3 Model B internal 
>> Device Tree\n"));
>> -        FdtGuid = &gRaspberryPi3ModelBFdtGuid;
>> -        break;
>> -      case 0x0D:
>> -        DEBUG ((DEBUG_INFO, "Using Raspberry Pi 3 Model B+ internal 
>> Device Tree\n"));
>> -        FdtGuid = &gRaspberryPi3ModelBPlusFdtGuid;
>> -        break;
>> -      case 0x11:
>> -        DEBUG ((DEBUG_INFO, "Using Raspberry Pi 4 Model B internal 
>> Device Tree\n"));
>> -        FdtGuid = &gRaspberryPi4ModelBFdtGuid;
>> -        break;
>> -      default:
>> -        DEBUG ((DEBUG_INFO, "Using default internal Device Tree\n"));
>> -        FdtGuid = &gRaspberryPiDefaultFdtGuid;
>> -        break;
>> -      }
>> -    }
>> -    do {
>> -      Status = GetSectionFromAnyFv (FdtGuid, EFI_SECTION_RAW, 0, 
>> &FdtImage, &FdtSize);
>> -      if (Status == EFI_SUCCESS) {
>> -        if (fdt_check_header (FdtImage) != 0) {
>> -          Status = EFI_INCOMPATIBLE_VERSION;
>> -        }
>> -      }
>> -      // No retry needed if we are successful or are dealing with the 
>> default Fdt.
>> -      if ( (Status == EFI_SUCCESS) ||
>> -           (CompareGuid (FdtGuid, &gRaspberryPiDefaultFdtGuid)) )
>> -        break;
>> -      // Otherwise, try one more time with the default Fdt. An 
>> example of this
>> -      // is if we detected a non-default Fdt, that isn't included in 
>> the FDF.
>> -      DEBUG ((DEBUG_INFO, "Internal Device Tree was not found for 
>> this platform, "
>> -        "falling back to default...\n"));
>> -      FdtGuid = &gRaspberryPiDefaultFdtGuid;
>> -    } while (1);
>> +    DEBUG ((DEBUG_ERROR, "No devicetree passed via config.txt\n"));
>> +    return EFI_NOT_FOUND;
>>     }
>> -  if (EFI_ERROR (Status)) {
>> -    DEBUG ((DEBUG_ERROR, "Failed to locate Device Tree: %r\n", Status));
>> -    return Status;
>> -  }
>> +  FdtSize = fdt_totalsize (FdtImage);
>> +  DEBUG ((DEBUG_INFO, "Devicetree passed via config.txt (0x%lx 
>> bytes)\n", FdtSize));
>>     /*
>>      * Probably overkill.
>> @@ -529,12 +393,19 @@ FdtDxeInitialize (
>>     Status = gBS->AllocatePages (AllocateAnyPages, EfiBootServicesData,
>>                     EFI_SIZE_TO_PAGES (FdtSize), 
>> (EFI_PHYSICAL_ADDRESS*)&mFdtImage);
>>     if (EFI_ERROR (Status)) {
>> -    DEBUG ((DEBUG_ERROR, "Failed to allocate Device Tree: %r\n", 
>> Status));
>> -    return Status;
>> +    DEBUG ((DEBUG_ERROR, "Failed to allocate devicetree: %r\n", 
>> Status));
>> +    goto out;
>>     }
>>     Retval = fdt_open_into (FdtImage, mFdtImage, FdtSize);
>> -  ASSERT (Retval == 0);
>> +  if (Retval != 0) {
>> +     DEBUG ((DEBUG_ERROR, "fdt_open_into failed: %d\n", Retval));
>> +     goto out;
>> +  }
>> +
>> +  /*
>> +   * These are all best-effort.
>> +   */
>>     Status = SanitizePSCI ();
>>     if (EFI_ERROR (Status)) {
>> @@ -566,19 +437,18 @@ FdtDxeInitialize (
>>       Print (L"Failed to update USB compatible properties: %r\n", 
>> Status);
>>     }
>> -  if (Internal) {
>> -    /*
>> -     * A GPU-provided DTB already has the full command line.
>> -     */
>> -    Status = UpdateBootArgs ();
>> -    if (EFI_ERROR (Status)) {
>> -      Print (L"Failed to update boot arguments: %r\n", Status);
>> -    }
>> -  }
>> -
>> -  DEBUG ((DEBUG_INFO, "Installed Device Tree at address 0x%p\n", 
>> mFdtImage));
>> +  DEBUG ((DEBUG_INFO, "Installed devicetree at address %p\n", 
>> mFdtImage));
>>     Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mFdtImage);
>> -  ASSERT_EFI_ERROR (Status);
>> +  if (EFI_ERROR (Status)) {
>> +     DEBUG ((DEBUG_ERROR, "Couldn't register devicetree: %r\n", 
>> Status));
>> +     goto out;
>> +  }
>> +out:
>> +  if (EFI_ERROR(Status)) {
>> +    if (mFdtImage != NULL) {
>> +      gBS->FreePages ((EFI_PHYSICAL_ADDRESS) mFdtImage, 
>> EFI_SIZE_TO_PAGES (FdtSize));
>> +    }
>> +  }
>>     return Status;
>>   }
>> diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf 
>> b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf
>> index fc37353f..49ba5ba1 100644
>> --- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf
>> +++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf
>> @@ -35,10 +35,6 @@
>>   [Guids]
>>     gFdtTableGuid
>> -  gRaspberryPi3ModelBFdtGuid
>> -  gRaspberryPi3ModelBPlusFdtGuid
>> -  gRaspberryPi4ModelBFdtGuid
>> -  gRaspberryPiDefaultFdtGuid
>>   [Protocols]
>>     gRaspberryPiFirmwareProtocolGuid              ## CONSUMES
>> diff --git a/Platform/RaspberryPi/RPi3/RPi3.fdf 
>> b/Platform/RaspberryPi/RPi3/RPi3.fdf
>> index daedc443..e854cd21 100644
>> --- a/Platform/RaspberryPi/RPi3/RPi3.fdf
>> +++ b/Platform/RaspberryPi/RPi3/RPi3.fdf
>> @@ -303,17 +303,6 @@ READ_LOCK_STATUS   = TRUE
>>     #
>>     INF Platform/RaspberryPi/Drivers/LogoDxe/LogoDxe.inf
>> -  #
>> -  # Device Tree support (used by FdtDxe)
>> -  # GUIDs should match gRaspberryPi#####FdtGuid's from the .dec
>> -  #
>> -  FILE FREEFORM = DF5DA223-1D27-47C3-8D1B-9A41B55A18BC {
>> -    SECTION RAW = 
>> Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2710-rpi-3-b.dtb
>> -  }
>> -  FILE FREEFORM = 3D523012-73FE-40E5-892E-1A4DF60F3C0C {
>> -    SECTION RAW = 
>> Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2710-rpi-3-b-plus.dtb
>> -  }
>> -
>>   [FV.FVMAIN_COMPACT]
>>   FvAlignment        = 16
>>   ERASE_POLARITY     = 1
>> diff --git a/Platform/RaspberryPi/RPi4/RPi4.fdf 
>> b/Platform/RaspberryPi/RPi4/RPi4.fdf
>> index c3e9cfc4..b1f7aa23 100644
>> --- a/Platform/RaspberryPi/RPi4/RPi4.fdf
>> +++ b/Platform/RaspberryPi/RPi4/RPi4.fdf
>> @@ -307,14 +307,6 @@ READ_LOCK_STATUS   = TRUE
>>     #
>>     INF Platform/RaspberryPi/Drivers/LogoDxe/LogoDxe.inf
>> -  #
>> -  # Device Tree support (used by FdtDxe)
>> -  # GUIDs should match gRaspberryPi#####FdtGuid's from the .dec
>> -  #
>> - FILE FREEFORM = 80AB6833-CAE4-4CEE-B59D-EB2039B05551 {
>> -    SECTION RAW = 
>> Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2711-rpi-4-b.dtb
>> -  }
>> -
>>   [FV.FVMAIN_COMPACT]
>>   FvAlignment        = 16
>>   ERASE_POLARITY     = 1
>> diff --git a/Platform/RaspberryPi/RaspberryPi.dec 
>> b/Platform/RaspberryPi/RaspberryPi.dec
>> index b66322be..66ef6186 100644
>> --- a/Platform/RaspberryPi/RaspberryPi.dec
>> +++ b/Platform/RaspberryPi/RaspberryPi.dec
>> @@ -26,13 +26,6 @@
>>     gRaspberryPiTokenSpaceGuid = {0xCD7CC258, 0x31DB, 0x11E6, {0x9F, 
>> 0xD3, 0x63, 0xB0, 0xB8, 0xEE, 0xD6, 0xB5}}
>>     gRaspberryPiEventResetGuid = {0xCD7CC258, 0x31DB, 0x11E6, {0x9F, 
>> 0xD3, 0x63, 0xB4, 0xB4, 0xE4, 0xD4, 0xB4}}
>>     gConfigDxeFormSetGuid = {0xCD7CC258, 0x31DB, 0x22E6, {0x9F, 0x22, 
>> 0x63, 0xB0, 0xB8, 0xEE, 0xD6, 0xB5}}
>> -  # GUIDs used by FdtDxe to serve a Device Tree at runtime. Not all 
>> of these need to apply
>> -  # to the current platform or match an actual FDF binary, but they 
>> need to be defined.
>> -  gRaspberryPi3ModelBFdtGuid = { 0xDF5DA223, 0x1D27, 0x47C3, { 0x8D, 
>> 0x1B, 0x9A, 0x41, 0xB5, 0x5A, 0x18, 0xBC } }
>> -  gRaspberryPi3ModelBPlusFdtGuid = { 0x3D523012, 0x73FE, 0x40E5, { 
>> 0x89, 0x2E, 0x1A, 0x4D, 0xF6, 0x0F, 0x3C, 0x0C } }
>> -  gRaspberryPi4ModelBFdtGuid = { 0x80AB6833, 0xCAE4, 0x4CEE, { 0xB5, 
>> 0x9D, 0xEB, 0x20, 0x39, 0xB0, 0x55, 0x51 } }
>> -  # Default Fdt to serve if the hardware model can't be detected. 
>> Should match one of the above.
>> -  gRaspberryPiDefaultFdtGuid = {0xDF5DA223, 0x1D27, 0x47C3, { 0x8D, 
>> 0x1B, 0x9A, 0x41, 0xB5, 0x5A, 0x18, 0xBC}}
>>   [PcdsFixedAtBuild.common]
>>     #
>>
>