Re: [PATCH edk2-platforms v2 1/1] Silicon/SynQuacer: add optional OP-TEE DT node

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 26 July 2018 at 09:36, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> On Wed, Jul 25, 2018 at 12:04:58PM +0200, Ard Biesheuvel wrote:
>> On 23 July 2018 at 15:19, Sumit Garg <sumit.garg@linaro.org> 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 <ard.biesheuvel@linaro.org>
>> > Cc: Leif Lindholm <leif.lindholm@linaro.org>
>> > Contributed-under: TianoCore Contribution Agreement 1.1
>> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>> > ---
>> >
>>
>> 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?
>

Is there any way we can let OP-TEE supply a DT overlay?


>
>>
>> > 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
>> >