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

[Marcin Wojtas <mw@semihalf.com>]

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.

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