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.web10.12314.1588345993916924080
 for <devel@edk2.groups.io>;
 Fri, 01 May 2020 08:13:14 -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 8D45330E;
	Fri,  1 May 2020 08:13:12 -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 AE8F23F68F;
	Fri,  1 May 2020 08:13:11 -0700 (PDT)
Subject: Re: [edk2-platforms][PATCH 1/2] RPi: rip out FdtDxe logic to use internal DTB
To: Pete Batard <pete@akeo.ie>, Andrei Warkentin
 <andrey.warkentin@gmail.com>, 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>
From: "Ard Biesheuvel" <ard.biesheuvel@arm.com>
Message-ID: <621362ba-ab8d-9acf-5a02-74b9e7ca80af@arm.com>
Date: Fri, 1 May 2020 17:13:10 +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: <4538b8ec-dc41-ec16-2ef5-011be3e90849@akeo.ie>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: quoted-printable

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://github.com/ARM-software/ebbr/issues/45 to sort out the real=20
>>> requirements)
>>>
>>> Testing: Booted Ubuntu 18.04 on Pi 2B (1.2).
>>>
>>> Signed-off-by: Andrei Warkentin <andrey.warkentin@gmail.com>
>>> ---
>>> =C2=A0 Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c=C2=A0=C2=A0 | 206=
=20
>>> ++++----------------
>>> =C2=A0 Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf |=C2=A0=C2=A0 4=
 -
>>> =C2=A0 Platform/RaspberryPi/RPi3/RPi3.fdf=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0 11 --
>>> =C2=A0 Platform/RaspberryPi/RPi4/RPi4.fdf=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0 8 -
>>> =C2=A0 Platform/RaspberryPi/RaspberryPi.dec=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0 7 -
>>> =C2=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 (
>>> =C2=A0=C2=A0=C2=A0 return EFI_SUCCESS;
>>> =C2=A0 }
>>> -#define MAX_CMDLINE_SIZE=C2=A0=C2=A0=C2=A0 512
>>> -
>>> -STATIC
>>> -EFI_STATUS
>>> -UpdateBootArgs (
>>> -=C2=A0 VOID
>>> -=C2=A0 )
>>> -{
>>> -=C2=A0 INTN=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 No=
de;
>>> -=C2=A0 INTN=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Re=
tval;
>>> -=C2=A0 EFI_STATUS=C2=A0=C2=A0=C2=A0 Status;
>>> -=C2=A0 CHAR8=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 *Comman=
dLine;
>>> -
>>> -=C2=A0 //
>>> -=C2=A0 // Locate the /chosen node
>>> -=C2=A0 //
>>> -=C2=A0 Node =3D fdt_path_offset (mFdtImage, "/chosen");
>>> -=C2=A0 if (Node < 0) {
>>> -=C2=A0=C2=A0=C2=A0 DEBUG ((DEBUG_ERROR, "%a: failed to locate /chose=
n node\n",=20
>>> __FUNCTION__));
>>> -=C2=A0=C2=A0=C2=A0 return EFI_NOT_FOUND;
>>> -=C2=A0 }
>>> -
>>> -=C2=A0 //
>>> -=C2=A0 // If /chosen/bootargs already exists, we want to add a space=
=20
>>> character
>>> -=C2=A0 // before adding the firmware supplied arguments. However, th=
e=20
>>> RpiFirmware
>>> -=C2=A0 // protocol expects a 32-bit aligned buffer. So let's allocat=
e 4=20
>>> bytes of
>>> -=C2=A0 // slack, and skip the first 3 when passing this buffer into =
libfdt.
>>> -=C2=A0 //
>>> -=C2=A0 CommandLine =3D AllocatePool (MAX_CMDLINE_SIZE) + 4;
>>> -=C2=A0 if (!CommandLine) {
>>> -=C2=A0=C2=A0=C2=A0 DEBUG ((DEBUG_ERROR, "%a: failed to allocate memo=
ry\n",=20
>>> __FUNCTION__));
>>> -=C2=A0=C2=A0=C2=A0 return EFI_OUT_OF_RESOURCES;
>>> -=C2=A0 }
>>> -
>>> -=C2=A0 //
>>> -=C2=A0 // Get the command line from the firmware
>>> -=C2=A0 //
>>> -=C2=A0 Status =3D mFwProtocol->GetCommandLine (MAX_CMDLINE_SIZE,=20
>>> CommandLine + 4);
>>> -=C2=A0 if (EFI_ERROR (Status)) {
>>> -=C2=A0=C2=A0=C2=A0 DEBUG ((DEBUG_ERROR, "%a: failed to retrieve comm=
and line\n",=20
>>> __FUNCTION__));
>>> -=C2=A0=C2=A0=C2=A0 return Status;
>>> -=C2=A0 }
>>> -
>>> -=C2=A0 if (AsciiStrLen (CommandLine + 4) =3D=3D 0) {
>>> -=C2=A0=C2=A0=C2=A0 DEBUG ((DEBUG_INFO, "%a: empty command line recei=
ved\n",=20
>>> __FUNCTION__));
>>> -=C2=A0=C2=A0=C2=A0 return EFI_SUCCESS;
>>> -=C2=A0 }
>>> -
>>> -=C2=A0 CommandLine[3] =3D ' ';
>>> -
>>> -=C2=A0 Retval =3D fdt_appendprop_string (mFdtImage, Node, "bootargs"=
,=20
>>> &CommandLine[3]);
>>> -=C2=A0 if (Retval !=3D 0) {
>>> -=C2=A0=C2=A0=C2=A0 DEBUG ((DEBUG_ERROR, "%a: failed to set /chosen/b=
ootargs=20
>>> property (%d)\n",
>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 __FUNCTION__, Retval));
>>> -=C2=A0 }
>>> -
>>> -=C2=A0 DEBUG_CODE_BEGIN ();
>>> -=C2=A0=C2=A0=C2=A0 CONST CHAR8=C2=A0=C2=A0=C2=A0 *Prop;
>>> -=C2=A0=C2=A0=C2=A0 INT32=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0 Length;
>>> -=C2=A0=C2=A0=C2=A0 INT32=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0 Index;
>>> -
>>> -=C2=A0=C2=A0=C2=A0 Node =3D fdt_path_offset (mFdtImage, "/chosen");
>>> -=C2=A0=C2=A0=C2=A0 ASSERT (Node >=3D 0);
>>> -
>>> -=C2=A0=C2=A0=C2=A0 Prop =3D fdt_getprop (mFdtImage, Node, "bootargs"=
, &Length);
>>> -=C2=A0=C2=A0=C2=A0 ASSERT (Prop !=3D NULL);
>>> -
>>> -=C2=A0=C2=A0=C2=A0 DEBUG ((DEBUG_INFO, "Command line set from firmwa=
re (length=20
>>> %d):\n'", Length));
>>> -
>>> -=C2=A0=C2=A0=C2=A0 for (Index =3D 0; Index < Length; Index++, Prop++=
) {
>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (*Prop =3D=3D '\0') {
>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 continue;
>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }
>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 DEBUG ((DEBUG_INFO, "%c", *Prop));
>>> -=C2=A0=C2=A0=C2=A0 }
>>> -
>>> -=C2=A0=C2=A0=C2=A0 DEBUG ((DEBUG_INFO, "'\n"));
>>> -=C2=A0 DEBUG_CODE_END ();
>>> -
>>> -=C2=A0 FreePool (CommandLine - 4);
>>> -=C2=A0 return EFI_SUCCESS;
>>> -}
>>> -
>>> -
>>> =C2=A0 /**
>>> =C2=A0=C2=A0=C2=A0 @param=C2=A0 ImageHandle=C2=A0=C2=A0 of the loaded=
 driver
>>> =C2=A0=C2=A0=C2=A0 @param=C2=A0 SystemTable=C2=A0=C2=A0 Pointer to th=
e System Table
>>> @@ -435,13 +351,10 @@ FdtDxeInitialize (
>>> =C2=A0=C2=A0=C2=A0 IN EFI_SYSTEM_TABLE=C2=A0=C2=A0 *SystemTable
>>> =C2=A0=C2=A0=C2=A0 )
>>> =C2=A0 {
>>> +=C2=A0 INT32=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Retval;
>>> =C2=A0=C2=A0=C2=A0 EFI_STATUS Status;
>>> -=C2=A0 EFI_GUID=C2=A0=C2=A0 *FdtGuid;
>>> -=C2=A0 VOID=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 *FdtImage;
>>> =C2=A0=C2=A0=C2=A0 UINTN=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 FdtSize;
>>> -=C2=A0 INT32=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Retval;
>>> -=C2=A0 UINT32=C2=A0=C2=A0=C2=A0=C2=A0 BoardRevision;
>>> -=C2=A0 BOOLEAN=C2=A0=C2=A0=C2=A0 Internal;
>>> +=C2=A0 VOID=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 *FdtImage =3D NULL;
>>> =C2=A0=C2=A0=C2=A0 if (PcdGet32 (PcdOptDeviceTree) =3D=3D 0) {
>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 DEBUG ((DEBUG_INFO, "Device Tree disab=
led per user=20
>>> configuration\n"));
>>> @@ -450,77 +363,28 @@ FdtDxeInitialize (
>>> =C2=A0=C2=A0=C2=A0 Status =3D gBS->LocateProtocol (&gRaspberryPiFirmw=
areProtocolGuid,=20
>>> NULL,
>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (VOID**)&mFwProtocol);
>>> -=C2=A0 ASSERT_EFI_ERROR (Status);
>>> +=C2=A0 if (mFwProtocol =3D=3D NULL) {
>>> +=C2=A0=C2=A0=C2=A0 /*
>>> +=C2=A0=C2=A0=C2=A0=C2=A0 * Impossible because of the depex...
>>> +=C2=A0=C2=A0=C2=A0=C2=A0 */
>>> +=C2=A0=C2=A0=C2=A0 ASSERT_EFI_ERROR (Status);
>>> +=C2=A0=C2=A0=C2=A0 return EFI_NOT_FOUND;
>>> +=C2=A0 }
>>
>> Do we need this change?
>=20
> Looking at what we are doing elsewhere, I thing we should go with:
>=20
>  =C2=A0 ASSERT_EFI_ERROR (Status);
>  =C2=A0 if (EFI_ERROR (Status)) {
>  =C2=A0=C2=A0=C2=A0 return Status;
>  =C2=A0 }
>=20
> 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 succeed=
=20
> and return a NULL value for mFwProtocol, but I doubt that's a valid=20
> 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.