Re: [platforms: PATCH v2 1/4] Marvell/Armada7k8k: Shift PEI stack base

[Marcin Wojtas <mw@semihalf.com>]

Hi Leif,

wt., 22 sty 2019 o 20:06 Leif Lindholm <leif.lindholm@linaro.org> napisał(a):
>
> On Tue, Jan 22, 2019 at 07:26:58PM +0100, Marcin Wojtas wrote:
> > Hi Leif,
> >
> > wt., 22 sty 2019 o 18:26 Leif Lindholm <leif.lindholm@linaro.org> napisał(a):
> > >
> > > On Tue, Jan 22, 2019 at 02:32:19AM +0100, Marcin Wojtas wrote:
> > > > Recent changes in the ARM-TF configure its runtime serices region
> > > > as protected, hence the hitherto PEI stack base address (0x41F0000)
> > > > violated it.
> > > >
> > > > In order to fix this, extend the region which is non-accessible
> > > > by the OS to cover both the ARM-TF (0x4000000 - 0x4200000) and OPTEE
> > > > (0x4400000 - 0x5400000) within a single area (0x4000000 - 0x5400000).
> > > > Set the PEI stack base address between both images (0x43F0000).
> > >
> > > OK, that is a much better description.
> > > But I'm getting slight cognitive dissonance from placing the PEI stack
> > > inside something we've just claimed belongs to Secure world...
> > >
> > > Could you instead break this out into two separate protected regions?
> > > PcdSecureOpteeBase/Size and PcdSecureTfBase/Size?
> > >
> > > Alternatively, nudge the stackbase to 0x5400000?
> >
> > As discussed some time ago with Ard, when the PEI stack base was
> > introduced, it is recommended that this stack is placed in the
> > location, which is not accessible by OS. Most preferred is to have it
> > in the SRAM (cannot do it on Armada7k8k) or in a reserved region - cut
> > out from the memory map passed to the OS.
> >
> > Currently we have a single region (a "hole") that covers:
> > 2MB for EL3 runtime services
> > 2MB of nothing
> > 16MB for OPTEE image
> >
> > The 2MB space between images IMO seems perfect for PEI stack to place.
> > If it was placed e.g. @0x5400000 and we kept the reserved regions
> > separate, the outcome would be:
> > 2MB for EL3 runtime services
> > 2MB of DRAM normal memory
> > 16MB + 64kB for Optee and PEI stack base.
> >
> > This is the reason, I'd like to keep original setting, proposed in the
> > patch. Please let know your opinion.
>
> I have no issue with the placement of the PEI stack between the ARM-TF
> region and the Op-TEE region. I _have_ an issue with the PEI stack
> being placed between PcdSecureRegionBase and (PcdSecureRegionBase +
> PcdSecureRegionSize). I.e. something that we describe as "the Secure
> region".
>
> I think I gave my suggestion for the resolution of this problem (with
> moving StackBase to 0x05400000 as the alternative) in my previous
> reply.
>

Yes, and I answered, presenting the alternative memory map with
additional 64kB "cut out" on top of 20MB "hole" of memory, which I'm
not fancy, given available space inside the 20MB chunk.

Because in fact this region is not entirely secure (EL3 runtime
services are exectued in NS context for example), how about I:
- rename the PCD's to be more generic (e.g.
gMarvellTokenSpaceGuid.PcdReservedRegionBase)
- add proper comment in Armada7k8k.dsc.inc for the default reserved
memory (+ maybe in Armada7k8kLib, where the PCD's are used)
?

Best regards,
Marcin

> >
> > Best regards,
> > Marcin
> >
> >
> > >
> > > /
> > >     Leif
> > >
> > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > > > ---
> > > >  Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> > > > index eafcd6e..c8c597f 100644
> > > > --- a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> > > > +++ b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> > > > @@ -376,12 +376,12 @@
> > > >
> > > >    gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|36
> > > >
> > > > -  gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x41F0000
> > > > +  gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x43F0000
> > > >    gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0x10000
> > > >
> > > >    # Secure region reservation
> > > >    gMarvellTokenSpaceGuid.PcdSecureRegionBase|0x4000000
> > > > -  gMarvellTokenSpaceGuid.PcdSecureRegionSize|0x0200000
> > > > +  gMarvellTokenSpaceGuid.PcdSecureRegionSize|0x1400000
> > > >
> > > >    # TRNG
> > > >    gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress|0xF2760000
> > > > --
> > > > 2.7.4
> > > >