diff mbox series

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

Message ID 1532351961-17377-1-git-send-email-sumit.garg@linaro.org
State New
Headers show
Series [edk2,edk2-platforms,v2,1/1] Silicon/SynQuacer: add optional OP-TEE DT node | expand

Commit Message

Sumit Garg July 23, 2018, 1:19 p.m. UTC
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(+)

-- 
2.7.4

Comments

Ard Biesheuvel July 25, 2018, 10:04 a.m. UTC | #1
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

>
Daniel Thompson July 26, 2018, 7:36 a.m. UTC | #2
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

> >
Ard Biesheuvel July 26, 2018, 7:39 a.m. UTC | #3
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

>> >
Daniel Thompson July 26, 2018, 7:50 a.m. UTC | #4
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.
Sumit Garg July 26, 2018, 8:42 a.m. UTC | #5
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.
Daniel Thompson July 27, 2018, 11:10 a.m. UTC | #6
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.
Sumit Garg July 27, 2018, 11:37 a.m. UTC | #7
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
Ard Biesheuvel July 27, 2018, 12:12 p.m. UTC | #8
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.
Mark Rutland July 27, 2018, 12:49 p.m. UTC | #9
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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Sumit Garg July 27, 2018, 2:29 p.m. UTC | #10
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.
diff mbox series

Patch

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"