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