From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by mx.groups.io with SMTP id smtpd.web12.11989.1588345892328156935 for ; Fri, 01 May 2020 08:11:32 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@akeo-ie.20150623.gappssmtp.com header.s=20150623 header.b=zwAHAr+N; spf=none, err=permanent DNS error (domain: akeo.ie, ip: 209.85.221.66, mailfrom: pete@akeo.ie) Received: by mail-wr1-f66.google.com with SMTP id g13so11830617wrb.8 for ; Fri, 01 May 2020 08:11:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=akeo-ie.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=Jc2W6r44mZ+JKcY0Vm9ChlESwo/mdkufCprTIUwJz/E=; b=zwAHAr+Ny+fvGvn6czqik/9dDmF5G+YXy4J4UpG6I9BtAi8l+2w4n/q9Ox5dpfGaL3 HwfR3f/MnrXQXrnn+L45tOLomZX1oXddK/r3TEEYMtnZNKbTkmedLUtafDWdSSl1DePv DfsXUcWAq8hIgBnOD+fsdoNwe32wmvD3tKCOpdOGUyjY18VWZRsDEtJBOpYar1OYHzGp 84HVQiLHGHUnJiA6QP5KR6+uBaixt6ZcYh/BKPK7xRdf5go9JrZ6Js3Jj5WhxSH0Wgrc O9Zli3oQWxFZj4Ro/V7CetH6TVtiiaa7tQ2u3K3HSDBwUpTVDhkyNnfZg5g6pGFFJQ4Q DGaw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=Jc2W6r44mZ+JKcY0Vm9ChlESwo/mdkufCprTIUwJz/E=; b=dwxfb0c7ke4cKWsyi3wvblLVPnrUP4fEHjI6Kc4So7GdwGZN0LUO1rVtORl5sTDtbM gqz9cPik/cmcG/TpOvbqK9l8r2AJfWdnx/pr6nPioFYJPgU6mZdKrydDj7EvZB2Tntak XRcSZCx70wgpQxrJfK8BG7TKKfzBo1yhnsSc2gw8LlgD5+BwfDlmPU9bYBKpNFoNvfot Dx8xkUhJRWFtEhI47jHlnJNnjqKmKvNXwLAND3v23C8rtWwJ5WO14tDfP1+OPJmTjHjE 9mxT97Cpy2aGfiTCat0Co1dOm4HYJrnd8tTeeIHPHLZDM7JFTiK1Ucg5/ZXlIXLQQa1N 3IDg== X-Gm-Message-State: AGi0PuZg5imK2+yyYPjg7X9DI7VX9L5U5C0xii1umDgCkjRxgG+U/LdI +W69NyPfZOIYolHZANF3vc5pYw== X-Google-Smtp-Source: APiQypKF5RP6ntfupU4Tx8MZ5cocA5CLrYzNiA257WSJEth5D8oV05IX+3EUN0sfuoH4lfc6VUuafg== X-Received: by 2002:adf:f48a:: with SMTP id l10mr4423950wro.231.1588345890725; Fri, 01 May 2020 08:11:30 -0700 (PDT) Return-Path: Received: from [10.0.0.122] ([84.203.75.87]) by smtp.googlemail.com with ESMTPSA id j13sm4620250wro.51.2020.05.01.08.11.29 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 01 May 2020 08:11:30 -0700 (PDT) Subject: Re: [edk2-platforms][PATCH 1/2] RPi: rip out FdtDxe logic to use internal DTB To: Ard Biesheuvel , Andrei Warkentin , devel@edk2.groups.io Cc: leif@nuviainc.com, philmd@redhat.com References: <20200430211617.120926-1-andrey.warkentin@gmail.com> <20200430211617.120926-2-andrey.warkentin@gmail.com> <77f961d0-55a2-4e85-f8e6-478c6499cd21@arm.com> From: "Pete Batard" Message-ID: <4538b8ec-dc41-ec16-2ef5-011be3e90849@akeo.ie> Date: Fri, 1 May 2020 16:11:28 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <77f961d0-55a2-4e85-f8e6-478c6499cd21@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 8bit 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 >> --- >>   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] >>     # >> >