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

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

[Sumit Garg]

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

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

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

[Ard Biesheuvel]

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.

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

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

[Daniel Thompson]

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?


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

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

[Ard Biesheuvel]

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

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

[Daniel Thompson]

On Thu, Jul 26, 2018 at 09:39:37AM +0200, Ard Biesheuvel wrote:
> 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?

I guess it could implement a secure monitor call to provide it. In
fact I find it a rather pleasing approach. However I think it still loops
us round to pretty much the same question as before. Does TF-A "protec
" a normal world that makes an SMC to an OP-TEE that isn't there by
failing the call in a nice way?


Daniel.

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

[Sumit Garg]

On Thu, 26 Jul 2018 at 13:20, Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On Thu, Jul 26, 2018 at 09:39:37AM +0200, Ard Biesheuvel wrote:
> > 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?
> > >

If we pass a DT with OP-TEE and OP-TEE not present, Linux TEE generic
driver exits gracefully giving following message:

[    1.976021] optee: probing for conduit method from DT.
[    1.976033] optee: api uid mismatch

> >
> > Is there any way we can let OP-TEE supply a DT overlay?
>
> I guess it could implement a secure monitor call to provide it. In
> fact I find it a rather pleasing approach. However I think it still loops
> us round to pretty much the same question as before. Does TF-A "protec
> " a normal world that makes an SMC to an OP-TEE that isn't there by
> failing the call in a nice way?
>

TF-A returns SMC call for OP-TEE as unknown (error code: -1 in "x0"
register) if OP-TEE is not present.

-Sumit

>
> Daniel.

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

[Daniel Thompson]

On 26/07/18 09:42, Sumit Garg wrote:
> On Thu, 26 Jul 2018 at 13:20, Daniel Thompson
> <daniel.thompson@linaro.org> wrote:
>>
>> On Thu, Jul 26, 2018 at 09:39:37AM +0200, Ard Biesheuvel wrote:
>>> 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?
>>>>
> 
> If we pass a DT with OP-TEE and OP-TEE not present, Linux TEE generic
> driver exits gracefully giving following message:
> 
> [    1.976021] optee: probing for conduit method from DT.
> [    1.976033] optee: api uid mismatch

That certainly means we can be pretty relaxed about version skew of 
normal world components (since nothing bad happens if thinks get skewed).


>>> Is there any way we can let OP-TEE supply a DT overlay?
>>
>> I guess it could implement a secure monitor call to provide it. In
>> fact I find it a rather pleasing approach. However I think it still loops
>> us round to pretty much the same question as before. Does TF-A "protec
>> " a normal world that makes an SMC to an OP-TEE that isn't there by
>> failing the call in a nice way?
>>
> 
> TF-A returns SMC call for OP-TEE as unknown (error code: -1 in "x0"
> register) if OP-TEE is not present.

It is possible to experiment with getting EDK2 to detect OP-TEE using 
SMC? This would be fully generic and presumably be the first step in 
having an EFI OP-TEE driver.


Daniel.

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

[Sumit Garg]

On Fri, 27 Jul 2018 at 16:40, Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On 26/07/18 09:42, Sumit Garg wrote:
> > On Thu, 26 Jul 2018 at 13:20, Daniel Thompson
> > <daniel.thompson@linaro.org> wrote:
> >>
> >> On Thu, Jul 26, 2018 at 09:39:37AM +0200, Ard Biesheuvel wrote:
> >>> 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?
> >>>>
> >
> > If we pass a DT with OP-TEE and OP-TEE not present, Linux TEE generic
> > driver exits gracefully giving following message:
> >
> > [    1.976021] optee: probing for conduit method from DT.
> > [    1.976033] optee: api uid mismatch
>
> That certainly means we can be pretty relaxed about version skew of
> normal world components (since nothing bad happens if thinks get skewed).
>
>
> >>> Is there any way we can let OP-TEE supply a DT overlay?
> >>
> >> I guess it could implement a secure monitor call to provide it. In
> >> fact I find it a rather pleasing approach. However I think it still loops
> >> us round to pretty much the same question as before. Does TF-A "protec
> >> " a normal world that makes an SMC to an OP-TEE that isn't there by
> >> failing the call in a nice way?
> >>
> >
> > TF-A returns SMC call for OP-TEE as unknown (error code: -1 in "x0"
> > register) if OP-TEE is not present.
>
> It is possible to experiment with getting EDK2 to detect OP-TEE using
> SMC? This would be fully generic and presumably be the first step in
> having an EFI OP-TEE driver.
>

Agree. I will try to detect OP-TEE version via SMC call. If SMC
unknown is returned, then we say OP-TEE is not present and remove
corresponding DT node.

So I think this EFI OP-TEE driver makes more sense in edk2 rather than
edk2-platforms.

-Sumit

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

[Ard Biesheuvel]

On 27 July 2018 at 13:37, Sumit Garg <sumit.garg@linaro.org> wrote:
> On Fri, 27 Jul 2018 at 16:40, Daniel Thompson
> <daniel.thompson@linaro.org> wrote:
>>
>> On 26/07/18 09:42, Sumit Garg wrote:
>> > On Thu, 26 Jul 2018 at 13:20, Daniel Thompson
>> > <daniel.thompson@linaro.org> wrote:
>> >>
>> >> On Thu, Jul 26, 2018 at 09:39:37AM +0200, Ard Biesheuvel wrote:
>> >>> 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?
>> >>>>
>> >
>> > If we pass a DT with OP-TEE and OP-TEE not present, Linux TEE generic
>> > driver exits gracefully giving following message:
>> >
>> > [    1.976021] optee: probing for conduit method from DT.
>> > [    1.976033] optee: api uid mismatch
>>
>> That certainly means we can be pretty relaxed about version skew of
>> normal world components (since nothing bad happens if thinks get skewed).
>>
>>
>> >>> Is there any way we can let OP-TEE supply a DT overlay?
>> >>
>> >> I guess it could implement a secure monitor call to provide it. In
>> >> fact I find it a rather pleasing approach. However I think it still loops
>> >> us round to pretty much the same question as before. Does TF-A "protec
>> >> " a normal world that makes an SMC to an OP-TEE that isn't there by
>> >> failing the call in a nice way?
>> >>
>> >
>> > TF-A returns SMC call for OP-TEE as unknown (error code: -1 in "x0"
>> > register) if OP-TEE is not present.
>>
>> It is possible to experiment with getting EDK2 to detect OP-TEE using
>> SMC? This would be fully generic and presumably be the first step in
>> having an EFI OP-TEE driver.
>>
>
> Agree. I will try to detect OP-TEE version via SMC call. If SMC
> unknown is returned, then we say OP-TEE is not present and remove
> corresponding DT node.
>
> So I think this EFI OP-TEE driver makes more sense in edk2 rather than
> edk2-platforms.
>

Indeed.

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

[Mark Rutland]

On Thu, Jul 26, 2018 at 02:12:04PM +0530, Sumit Garg wrote:
> On Thu, 26 Jul 2018 at 13:20, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> > I guess it could implement a secure monitor call to provide it. In
> > fact I find it a rather pleasing approach. However I think it still loops
> > us round to pretty much the same question as before. Does TF-A "protec
> > " a normal world that makes an SMC to an OP-TEE that isn't there by
> > failing the call in a nice way?
> 
> TF-A returns SMC call for OP-TEE as unknown (error code: -1 in "x0"
> register) if OP-TEE is not present.

Be careful here; you can't use an arbitrary SMC since that could be
implemented by another trusted OS (with a completely different meaning).

Assuming you know the system provides SMCCC, you can use the "Call UID
Query" in the trusted OS range, and check that returned value matches
OP-TEE's UID.

i.e

  uid = smccc_uid_query(OPTEE_RANGE);
  if (uid == OPTEEE_SMCCC_UID) {
          [ OP-TEE present ]
  } else {
          [ unknown/no trusted OS present ]
  }

Thanks,
Mark.

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

[Sumit Garg]

On Fri, 27 Jul 2018 at 18:19, Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Thu, Jul 26, 2018 at 02:12:04PM +0530, Sumit Garg wrote:
> > On Thu, 26 Jul 2018 at 13:20, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> > > I guess it could implement a secure monitor call to provide it. In
> > > fact I find it a rather pleasing approach. However I think it still loops
> > > us round to pretty much the same question as before. Does TF-A "protec
> > > " a normal world that makes an SMC to an OP-TEE that isn't there by
> > > failing the call in a nice way?
> >
> > TF-A returns SMC call for OP-TEE as unknown (error code: -1 in "x0"
> > register) if OP-TEE is not present.
>
> Be careful here; you can't use an arbitrary SMC since that could be
> implemented by another trusted OS (with a completely different meaning).
>
> Assuming you know the system provides SMCCC, you can use the "Call UID
> Query" in the trusted OS range, and check that returned value matches
> OP-TEE's UID.
>
> i.e
>
>   uid = smccc_uid_query(OPTEE_RANGE);
>   if (uid == OPTEEE_SMCCC_UID) {
>           [ OP-TEE present ]
>   } else {
>           [ unknown/no trusted OS present ]
>   }
>

Thanks Mark for this useful suggestion. Will try to use it.

-Sumit

> Thanks,
> Mark.