[U-Boot] [PATCH][v4] powerpc/mpc85xx: SECURE BOOT- Add secure boot target for B4860QDS

[York Sun]

On 03/10/2014 02:14 AM, Bansal Aneesh-B39320 wrote:
>> -----Original Message-----
>> From: Sun York-R58495
>> Sent: Saturday, March 08, 2014 12:01 AM
>> To: Bansal Aneesh-B39320; u-boot at lists.denx.de
>> Cc: Wood Scott-B07421
>> Subject: Re: [PATCH][v4] powerpc/mpc85xx: SECURE BOOT- Add secure boot
>> target for B4860QDS
>>
>> On 03/07/2014 05:23 AM, Aneesh Bansal wrote:
>>> Changes:
>>> 1. L2 cache is being invalidated by Boot ROM code for e6500 core.
>>>    So removing the invalidation from start.S 2. Clear the LAW and
>>> corresponding configuration for CPC. Boot ROM
>>>    code uses it as hosekeeping area.
>>> 3. For Secure boot, CPC is configured as SRAM and used as house
>>>    keeping area. This configuration is to be disabled once in uboot.
>>>    Earlier this disabling of CPC as SRAM was happening in
>> cpu_init_r.
>>>    As a result cache invalidation function was getting skipped in
>>>    case CPC is configured as SRAM.This was causing random crashes.
>>>
>>> Signed-off-by: Aneesh Bansal <aneesh.bansal at freescale.com>
>>> ---
>>>  README                                     |  4 ++++
>>>  arch/powerpc/cpu/mpc85xx/cpu_init.c        | 27
>> ++++++++++++++++++++++-----
>>>  arch/powerpc/cpu/mpc85xx/start.S           |  3 ++-
>>>  arch/powerpc/include/asm/fsl_secure_boot.h |  6 ++++++
>>>  boards.cfg                                 |  1 +
>>>  5 files changed, 35 insertions(+), 6 deletions(-)
>>>
>>> Changes from v3:
>>> Renamed the macro to indicate CPC configured as SRAM at U-boot entry
>>> to CONFIG_SYS_CPC_SRAM
>>
>> Aneesh,
>>
>> I understand you are trying to address the comment about when CPC
>> needs to be disabled before initializing as normal CPC. But
>> introducing CONFIG_SYS_CPC_SRAM seems even more confusing. Let's take
>> one step back.
> In the previous patch we had defined it as CONFIG_SYS_SECURE_HKAREA_CPC. But as per the discussion on previous patch, we were advised to make it generic to have a macro stating that U-Boot is entered with the CPC configured as SRAM.

In your patch, you are using this new macro to disable SRAM. It would be more
clear if the macro is named CONFIG_SYS_CPC_REINIT_F.


>>
>> CPC as SRAM can be used for many reasons. There is only one reason we
>> should keep it as SRAM, i.e. u-boot is indeed using SRAM as its final
>> destination. For all other usages, we can safely reconfigure it as
>> normal CPC after u-boot relocates itself into DDR. If u-boot's final
>> destination is SRAM, it is a RAMBOOT. Can we use this condition to
>> deal with CPC?
> There is a separate condition for secure boot in which CPC is required as SRAM. For NOR secure boot, user needs to initialize a housekeeping area using PBI commands and provide pointer to this housekeeping area to boot ROM.  Currently we are using CPC configured as SRAM for this area. Since, this area was initialized as SRAM by PBI commands, there is no cleanup of this area by Boot ROM. Since out of ROM, this are is no longer required, in case of NOR secure boot we need to disable this setting. NOR secure boot is not a RAMBOOT.

That's not conflict with my suggestion. This is NOT a RAMBOOT, so CPC should be
reinitialized.

>  
>>
>> Do you see the need to disable CPC before relocation? You are doing so
>> in this patch. Does the temporary LAW or address conflict with u-boot?
> Earlier we were disabling cpc in cpu_init_r. However since cache invalidation function was getting skipped in the process, it was causing random crashes. Skipping invalidation works in RAMBOOT scenario, however since we don’t have valid data when CPC is configured as cache for the secure boot scenario which is not RAMBOOT, these crashes were occurring in few platforms. As a result we had to move this disable code early in the sequence, so that invalidation of cache doesn’t get skipped.
>>

As I suggested, if you have to do this before relocation, a macro
CONFIG_SYS_CPC_REINIT_F makes more sense.

York