On Wed, Jul 25, 2018 at 12:04:58PM +0200, Ard Biesheuvel wrote:
> On 23 July 2018 at 15:19, Sumit Garg <[email protected]> wrote:
> > OP-TEE is optional on Developerbox controlled via SCP firmware. To check
> > if we need to delete OP-TEE DT node, we use DRAM1 region info as SCP
> > firmware conditionally carves out Secure memory from DRAM1 region.
> >
> > Cc: Ard Biesheuvel <[email protected]>
> > Cc: Leif Lindholm <[email protected]>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Sumit Garg <[email protected]>
> > ---
> >
>
> As discussed on IRC, i am not a fan of inferring the presence of
> OP-TEE from the base/size values of the first DRAM region.
>
> Please refer to the existing PCIe code how to read a GPIO in PEI and
> set a dynamic PCD accordingly, so you can use its value in
> PlatformDxe.
For Trusted Firmware I asked Sumit to look for the OP-TEE memory carve
out rather than looking at the switches. This was based on concerns
about version skew (new C-A53 firmware, old SCP firmware[1]), in particular
if TF-A jumps to an OP-TEE that isn't actually loaded the system will
fail in a not very transparent way (especially if the user hasn't found
the debug UART behind the back panel yet).
What is the consequence of passing a DT with OP-TEE present if one is
not actually present? Do we at least get as far as bringing up the
framebuffer before things explode?
Daniel.
>
> > Changes since v1:
> > - Add support for optional OP-TEE DT node addition
> >
> >
> > Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf
> > | 3 ++
> >
> > Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c
> > | 33 ++++++++++++++++++++
> > Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi
> > | 7 +++++
> > 3 files changed, 43 insertions(+)
> >
> > diff --git
> > a/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf
> >
> > b/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf
> > index 548d62fd5c0a..46cd3f85e509 100644
> > ---
> > a/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf
> > +++
> > b/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf
> > @@ -35,6 +35,9 @@ [LibraryClasses]
> > FdtLib
> > MemoryAllocationLib
> >
> > +[FixedPcd]
> > + gSynQuacerTokenSpaceGuid.PcdDramInfoBase
> > +
> > [Pcd]
> > gSynQuacerTokenSpaceGuid.PcdPcieEnableMask
> > gSynQuacerTokenSpaceGuid.PcdPlatformSettings
> > diff --git
> > a/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c
> >
> > b/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c
> > index 897d06743708..da1209b4a613 100644
> > ---
> > a/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c
> > +++
> > b/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c
> > @@ -19,10 +19,13 @@
> > #include <Library/DebugLib.h>
> > #include <Library/DxeServicesLib.h>
> > #include <Library/MemoryAllocationLib.h>
> > +#include <Platform/DramInfo.h>
> > #include <Platform/VarStore.h>
> >
> > // add enough space for three instances of 'status = "disabled"'
> > #define DTB_PADDING 64
> > +// base address for OP-TEE used to determine its presence
> > +#define OPTEE_BASE_ADDR 0xFC000000
> >
> > STATIC
> > VOID
> > @@ -47,6 +50,29 @@ DisableDtNode (
> > }
> > }
> >
> > +STATIC
> > +VOID
> > +DeleteDtNode (
> > + IN VOID *Dtb,
> > + IN CONST CHAR8 *NodePath
> > + )
> > +{
> > + INT32 Node;
> > + INT32 Rc;
> > +
> > + Node = fdt_path_offset (Dtb, NodePath);
> > + if (Node < 0) {
> > + DEBUG ((DEBUG_ERROR, "%a: failed to locate DT path '%a': %a\n",
> > + __FUNCTION__, NodePath, fdt_strerror (Node)));
> > + return;
> > + }
> > + Rc = fdt_del_node (Dtb, Node);
> > + if (Rc < 0) {
> > + DEBUG ((DEBUG_ERROR, "%a: failed to delete node on '%a': %a\n",
> > + __FUNCTION__, NodePath, fdt_strerror (Rc)));
> > + }
> > +}
> > +
> > /**
> > Return a pool allocated copy of the DTB image that is appropriate for
> > booting the current platform via DT.
> > @@ -73,6 +99,7 @@ DtPlatformLoadDtb (
> > UINTN CopyDtbSize;
> > INT32 Rc;
> > UINT64 SettingsVal;
> > + DRAM_INFO *DramInfo;
> > SYNQUACER_PLATFORM_VARSTORE_DATA *Settings;
> >
> > Status = GetSectionFromAnyFv (&gDtPlatformDefaultDtbFileGuid,
> > @@ -107,6 +134,12 @@ DtPlatformLoadDtb (
> > DisableDtNode (CopyDtb, "/sdhci@52300000");
> > }
> >
> > + DramInfo = (VOID *)(UINTN)FixedPcdGet64 (PcdDramInfoBase);
> > +
> > + if ((DramInfo->Entry[0].Base + DramInfo->Entry[0].Size) >
> > OPTEE_BASE_ADDR) {
> > + DeleteDtNode (CopyDtb, "/firmware/optee");
> > + }
> > +
> > *Dtb = CopyDtb;
> > *DtbSize = CopyDtbSize;
> >
> > diff --git a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi
> > b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi
> > index 37d642e4b237..d109a5742793 100644
> > --- a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi
> > +++ b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi
> > @@ -574,6 +574,13 @@
> > #address-cells = <1>;
> > #size-cells = <0>;
> > };
> > +
> > + firmware {
> > + optee {
> > + compatible = "linaro,optee-tz";
> > + method = "smc";
> > + };
> > + };
> > };
> >
> > #include "SynQuacerCaches.dtsi"
> > --
> > 2.7.4
> >
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
