From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web11.15030.1588352711540752602 for ; Fri, 01 May 2020 10:05:11 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id E269030E; Fri, 1 May 2020 10:05:09 -0700 (PDT) Received: from [192.168.1.81] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 79EB33F305; Fri, 1 May 2020 10:05:08 -0700 (PDT) Subject: Re: [edk2-devel] [edk2-platforms][PATCH 1/2] RPi: rip out FdtDxe logic to use internal DTB To: Andrei Warkentin , Andrei Warkentin , "devel@edk2.groups.io" , "pete@akeo.ie" 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: "Ard Biesheuvel" Message-ID: <29929795-7336-b54f-2a89-e730fc738146@arm.com> Date: Fri, 1 May 2020 19:04:58 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 5/1/20 6:56 PM, Andrei Warkentin wrote: > Hi folks, >=20 > I added that specific line more as a safeguard in case the depex ever=20 > changes, as I wasn't comfortable with code that relied on an external=20 > factor to have defined behavior. >=20 > I don't have a strong position on it. If you want it to go, I'll send an= = =20 > update. >=20 No worries, I'll just drop it from the patch. > A > ------------------------------------------------------------------------ > *From:* devel@edk2.groups.io on behalf of Pete=20 > Batard via groups.io > *Sent:* Friday, May 1, 2020 10:16 AM > *To:* Ard Biesheuvel ; Andrei Warkentin=20 > ; devel@edk2.groups.io > *Cc:* leif@nuviainc.com ; philmd@redhat.com=20 > > *Subject:* Re: [edk2-devel] [edk2-platforms][PATCH 1/2] RPi: rip out=20 > 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=20 >>>>> (optimize memory used) >>>>> - suspicious use of EfiBootServicesData (I filed=20 >>>>> https://nam04.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2F= github.com%2FARM-software%2Febbr%2Fissues%2F45&data=3D02%7C01%7Cawarken= tin%40vmware.com%7Ccd65ce9968f54b569f8208d7ede2a025%7Cb39138ca3cee4b4aa4d6c= d83d9dd62f0%7C0%7C0%7C637239429916614640&sdata=3DpJ3Wu%2BGJhj%2FqfbBEFM= Q9nQ%2B1Pgc%2Bo4Xw0fer2h9ZYvQ%3D&reserved=3D0=20 > to sort out the real >>>>> requirements) >>>>> >>>>> Testing: Booted Ubuntu 18.04 on Pi 2B (1.2). >>>>> >>>>> Signed-off-by: Andrei Warkentin >>>>> --- >>>>> =A0 Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c=A0=A0 | 206=20 >>>>> ++++---------------- >>>>> =A0 Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf |=A0=A0 4 - >>>>> =A0 Platform/RaspberryPi/RPi3/RPi3.fdf=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= = =A0=A0 |=A0 11 -- >>>>> =A0 Platform/RaspberryPi/RPi4/RPi4.fdf=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= = =A0=A0 |=A0=A0 8 - >>>>> =A0 Platform/RaspberryPi/RaspberryPi.dec=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0 |=A0=A0 7 - >>>>> =A0 5 files changed, 38 insertions(+), 198 deletions(-) >>>>> >>>>> diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c=20 >>>>> 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 ( >>>>> =A0=A0=A0 return EFI_SUCCESS; >>>>> =A0 } >>>>> -#define MAX_CMDLINE_SIZE=A0=A0=A0 512 >>>>> - >>>>> -STATIC >>>>> -EFI_STATUS >>>>> -UpdateBootArgs ( >>>>> -=A0 VOID >>>>> -=A0 ) >>>>> -{ >>>>> -=A0 INTN=A0=A0=A0=A0=A0=A0=A0=A0=A0 Node; >>>>> -=A0 INTN=A0=A0=A0=A0=A0=A0=A0=A0=A0 Retval; >>>>> -=A0 EFI_STATUS=A0=A0=A0 Status; >>>>> -=A0 CHAR8=A0=A0=A0=A0=A0=A0=A0=A0 *CommandLine; >>>>> - >>>>> -=A0 // >>>>> -=A0 // Locate the /chosen node >>>>> -=A0 // >>>>> -=A0 Node =3D fdt_path_offset (mFdtImage, "/chosen"); >>>>> -=A0 if (Node < 0) { >>>>> -=A0=A0=A0 DEBUG ((DEBUG_ERROR, "%a: failed to locate /chosen node\n= ",=20 >>>>> __FUNCTION__)); >>>>> -=A0=A0=A0 return EFI_NOT_FOUND; >>>>> -=A0 } >>>>> - >>>>> -=A0 // >>>>> -=A0 // If /chosen/bootargs already exists, we want to add a space= =20 >>>>> character >>>>> -=A0 // before adding the firmware supplied arguments. However, the= =20 >>>>> RpiFirmware >>>>> -=A0 // protocol expects a 32-bit aligned buffer. So let's allocate = 4=20 >>>>> bytes of >>>>> -=A0 // slack, and skip the first 3 when passing this buffer into li= bfdt. >>>>> -=A0 // >>>>> -=A0 CommandLine =3D AllocatePool (MAX_CMDLINE_SIZE) + 4; >>>>> -=A0 if (!CommandLine) { >>>>> -=A0=A0=A0 DEBUG ((DEBUG_ERROR, "%a: failed to allocate memory\n",= =20 >>>>> __FUNCTION__)); >>>>> -=A0=A0=A0 return EFI_OUT_OF_RESOURCES; >>>>> -=A0 } >>>>> - >>>>> -=A0 // >>>>> -=A0 // Get the command line from the firmware >>>>> -=A0 // >>>>> -=A0 Status =3D mFwProtocol->GetCommandLine (MAX_CMDLINE_SIZE,=20 >>>>> CommandLine + 4); >>>>> -=A0 if (EFI_ERROR (Status)) { >>>>> -=A0=A0=A0 DEBUG ((DEBUG_ERROR, "%a: failed to retrieve command line= \n",=20 >>>>> __FUNCTION__)); >>>>> -=A0=A0=A0 return Status; >>>>> -=A0 } >>>>> - >>>>> -=A0 if (AsciiStrLen (CommandLine + 4) =3D=3D 0) { >>>>> -=A0=A0=A0 DEBUG ((DEBUG_INFO, "%a: empty command line received\n",= =20 >>>>> __FUNCTION__)); >>>>> -=A0=A0=A0 return EFI_SUCCESS; >>>>> -=A0 } >>>>> - >>>>> -=A0 CommandLine[3] =3D ' '; >>>>> - >>>>> -=A0 Retval =3D fdt_appendprop_string (mFdtImage, Node, "bootargs",= =20 >>>>> &CommandLine[3]); >>>>> -=A0 if (Retval !=3D 0) { >>>>> -=A0=A0=A0 DEBUG ((DEBUG_ERROR, "%a: failed to set /chosen/bootargs= =20 >>>>> property (%d)\n", >>>>> -=A0=A0=A0=A0=A0 __FUNCTION__, Retval)); >>>>> -=A0 } >>>>> - >>>>> -=A0 DEBUG_CODE_BEGIN (); >>>>> -=A0=A0=A0 CONST CHAR8=A0=A0=A0 *Prop; >>>>> -=A0=A0=A0 INT32=A0=A0=A0=A0=A0=A0=A0=A0 Length; >>>>> -=A0=A0=A0 INT32=A0=A0=A0=A0=A0=A0=A0=A0 Index; >>>>> - >>>>> -=A0=A0=A0 Node =3D fdt_path_offset (mFdtImage, "/chosen"); >>>>> -=A0=A0=A0 ASSERT (Node >=3D 0); >>>>> - >>>>> -=A0=A0=A0 Prop =3D fdt_getprop (mFdtImage, Node, "bootargs", &Lengt= h); >>>>> -=A0=A0=A0 ASSERT (Prop !=3D NULL); >>>>> - >>>>> -=A0=A0=A0 DEBUG ((DEBUG_INFO, "Command line set from firmware (leng= th=20 >>>>> %d):\n'", Length)); >>>>> - >>>>> -=A0=A0=A0 for (Index =3D 0; Index < Length; Index++, Prop++) { >>>>> -=A0=A0=A0=A0=A0 if (*Prop =3D=3D '\0') { >>>>> -=A0=A0=A0=A0=A0=A0=A0 continue; >>>>> -=A0=A0=A0=A0=A0 } >>>>> -=A0=A0=A0=A0=A0 DEBUG ((DEBUG_INFO, "%c", *Prop)); >>>>> -=A0=A0=A0 } >>>>> - >>>>> -=A0=A0=A0 DEBUG ((DEBUG_INFO, "'\n")); >>>>> -=A0 DEBUG_CODE_END (); >>>>> - >>>>> -=A0 FreePool (CommandLine - 4); >>>>> -=A0 return EFI_SUCCESS; >>>>> -} >>>>> - >>>>> - >>>>> =A0 /** >>>>> =A0=A0=A0 @param=A0 ImageHandle=A0=A0 of the loaded driver >>>>> =A0=A0=A0 @param=A0 SystemTable=A0=A0 Pointer to the System Table >>>>> @@ -435,13 +351,10 @@ FdtDxeInitialize ( >>>>> =A0=A0=A0 IN EFI_SYSTEM_TABLE=A0=A0 *SystemTable >>>>> =A0=A0=A0 ) >>>>> =A0 { >>>>> +=A0 INT32=A0=A0=A0=A0=A0 Retval; >>>>> =A0=A0=A0 EFI_STATUS Status; >>>>> -=A0 EFI_GUID=A0=A0 *FdtGuid; >>>>> -=A0 VOID=A0=A0=A0=A0=A0=A0 *FdtImage; >>>>> =A0=A0=A0 UINTN=A0=A0=A0=A0=A0 FdtSize; >>>>> -=A0 INT32=A0=A0=A0=A0=A0 Retval; >>>>> -=A0 UINT32=A0=A0=A0=A0 BoardRevision; >>>>> -=A0 BOOLEAN=A0=A0=A0 Internal; >>>>> +=A0 VOID=A0=A0=A0=A0=A0=A0 *FdtImage =3D NULL; >>>>> =A0=A0=A0 if (PcdGet32 (PcdOptDeviceTree) =3D=3D 0) { >>>>> =A0=A0=A0=A0=A0 DEBUG ((DEBUG_INFO, "Device Tree disabled per user= =20 >>>>> configuration\n")); >>>>> @@ -450,77 +363,28 @@ FdtDxeInitialize ( >>>>> =A0=A0=A0 Status =3D gBS->LocateProtocol (&gRaspberryPiFirmwareProto= colGuid,=20 >>>>> NULL, >>>>> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 (VOID**)&m= FwProtocol); >>>>> -=A0 ASSERT_EFI_ERROR (Status); >>>>> +=A0 if (mFwProtocol =3D=3D NULL) { >>>>> +=A0=A0=A0 /* >>>>> +=A0=A0=A0=A0 * Impossible because of the depex... >>>>> +=A0=A0=A0=A0 */ >>>>> +=A0=A0=A0 ASSERT_EFI_ERROR (Status); >>>>> +=A0=A0=A0 return EFI_NOT_FOUND; >>>>> +=A0 } >>>> >>>> Do we need this change? >>> >>> Looking at what we are doing elsewhere, I thing we should go with: >>> >>> =A0=A0 ASSERT_EFI_ERROR (Status); >>> =A0=A0 if (EFI_ERROR (Status)) { >>> =A0=A0=A0=A0 return Status; >>> =A0=A0 } >>> >>> The ASSERT_EFI_ERROR () on its own was not enough for RELEASE, so we= =20 >>> definitely want to return an error code here if needed, and I guess=20 >>> that, technically, we could consider that LocateProtocol () may=20 >>> succeed and return a NULL value for mFwProtocol, but I doubt that's a= =20 >>> valid consideration. >>> >>=20 >> But the DEPEX guarantees that the module will only be dispatched at a= =20 >> time when LocateProtocol() will succeed. This is a common pattern, so I= = =20 >> wonder why it is being silently modified here. >>=20 >=20 > Fair enough. Let's wait for Andrei to reply on this. >=20 > /Pete >=20 >=20 >=20