From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f67.google.com (mail-wm1-f67.google.com [209.85.128.67]) by mx.groups.io with SMTP id smtpd.web12.12116.1588346188835759414 for ; Fri, 01 May 2020 08:16:29 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@akeo-ie.20150623.gappssmtp.com header.s=20150623 header.b=SN+uMgrD; spf=none, err=permanent DNS error (domain: akeo.ie, ip: 209.85.128.67, mailfrom: pete@akeo.ie) Received: by mail-wm1-f67.google.com with SMTP id x4so39550wmj.1 for ; Fri, 01 May 2020 08:16:28 -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=RprTpaIPCSPXp5esPHnQCaaYA02myGAh2zx2cOsXlTw=; b=SN+uMgrDobJEUmaHs03H2dSTIqvxx6QVUMb1fMapDaLINto1fwy+bKpJItpyzoU2Qt sMQQDD74hSn+CCi3xeNX3AbcSQIqNDy/wBlQaP1AX7RZdvcOawmmt0nDQ7A6z5WFcNLB z7MpzEAbrDBBkEoAPcUmaFGMqJnWR+V/EGLi2jhBSImjRZkloEo7M3AHLksHaeIdcPVH WMoefWbYYMKUi1YMuxsKsV28Y14gHIN/9XI0Nxp8XwDDw8w2z+4MobQc40rLQ1MPY0fl KsGeOnA1d5+TkHH2YPr5l5H3bikPysdGZN93Z71Zx8w6jZ8EtgG2VBw4atI8+spP+FKN cgEw== 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=RprTpaIPCSPXp5esPHnQCaaYA02myGAh2zx2cOsXlTw=; b=S1kthK9sueqQlKlRtHlMLKufPbHZBwC4wd+AnjBLRnvzY3YdX3IGasKqTKmj3Y2ura o/xJPaGxELa8jTt2A+a1GPwgOL6jvOTer3eZrOyXBXTj1lCkeJ3HtJbuox4zrk6VMpfH Dvy1vfrdPt7G/FUQ/YSo84kXQSGnNTvkbv3X56kMXulBQJjUQztx51vkqERfOAMC1D2s 7h628NnEgVYyotN19gBGdo7ueXwO9njMgqmPykaJlGEPjWE8ll+Srld92X8nxikboC+3 4341N6wMfzE+9vrk6brsAHcvOubqpcLsNmQxUEVI6KOQoK4PiohbU+z3Lh5m8Rv6/spG PeEw== X-Gm-Message-State: AGi0PuYBX3vB1bxkTQJeP0i5pt3Ka8o58GBzGqNk1wNozN5ozaCeYHTw iDsrEk94lESQor5V+JXePlexVw== X-Google-Smtp-Source: APiQypJVmUSVFGkKNswtSP+tSB3fJT96cuqAsDzMROzy211QGQox4ctWIifT+cIEi4YlvM2euscp9A== X-Received: by 2002:a1c:2846:: with SMTP id o67mr42297wmo.23.1588346187275; Fri, 01 May 2020 08:16:27 -0700 (PDT) Return-Path: Received: from [10.0.0.122] ([84.203.75.87]) by smtp.googlemail.com with ESMTPSA id h6sm19529wmf.31.2020.05.01.08.16.26 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 01 May 2020 08:16:26 -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> <4538b8ec-dc41-ec16-2ef5-011be3e90849@akeo.ie> <621362ba-ab8d-9acf-5a02-74b9e7ca80af@arm.com> From: "Pete Batard" Message-ID: Date: Fri, 1 May 2020 16:16:25 +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: <621362ba-ab8d-9acf-5a02-74b9e7ca80af@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 8bit 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 >>>> --- >>>>   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