public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Daniel Thompson <daniel.thompson@linaro.org>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Sumit Garg <sumit.garg@linaro.org>,
	"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
	Patch Tracking <patches@linaro.org>,
	Leif Lindholm <leif.lindholm@linaro.org>
Subject: Re: [PATCH edk2-platforms v2 1/1] Silicon/SynQuacer: add optional OP-TEE DT node
Date: Thu, 26 Jul 2018 08:36:16 +0100	[thread overview]
Message-ID: <20180726073616.ut62js3w6lxsvrvf@holly.lan> (raw)
In-Reply-To: <CAKv+Gu-ypTtBFffc-nN=hns+AXKunXS1Aa8oa2FkGuu3LE9H=g@mail.gmail.com>

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


  reply	other threads:[~2018-07-26  7:36 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-23 13:19 [PATCH edk2-platforms v2 1/1] Silicon/SynQuacer: add optional OP-TEE DT node Sumit Garg
2018-07-25 10:04 ` Ard Biesheuvel
2018-07-26  7:36   ` Daniel Thompson [this message]
2018-07-26  7:39     ` Ard Biesheuvel
2018-07-26  7:50       ` Daniel Thompson
2018-07-26  8:42         ` Sumit Garg
2018-07-27 11:10           ` Daniel Thompson
2018-07-27 11:37             ` Sumit Garg
2018-07-27 12:12               ` Ard Biesheuvel
2018-07-27 12:49           ` Mark Rutland
2018-07-27 14:29             ` Sumit Garg

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180726073616.ut62js3w6lxsvrvf@holly.lan \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox