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

[Leif Lindholm <leif.lindholm@linaro.org>]

On Tue, Jan 22, 2019 at 08:27:10PM +0100, Marcin Wojtas wrote:
> > > > > 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.

Please go back and reread my first and my second email.
Then please point out where I have, other than as an alternative
solution, suggested growing the cutout size.

Then perhaps we can rewind this conversation and try again?

Best Regards,

Leif

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