[U-Boot] [U-Boot, 1/2, v4] powerpc/mpc85xx: SECURE BOOT- NAND secure boot target for P3041

[Scott Wood]

On Thu, 2015-02-26 at 23:46 -0600, Bansal Aneesh-B39320 wrote:
> 
> 
> Regards,
> Aneesh Bansal
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Friday, February 27, 2015 10:22 AM
> > To: Bansal Aneesh-B39320
> > Cc: u-boot at lists.denx.de; Sun York-R58495; Gupta Ruchika-R66431
> > Subject: Re: [U-Boot, 1/2, v4] powerpc/mpc85xx: SECURE BOOT- NAND
> > secure boot target for P3041
> > 
> > On Thu, 2015-02-26 at 22:35 -0600, Bansal Aneesh-B39320 wrote:
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Thursday, February 26, 2015 3:43 AM
> > > > To: Bansal Aneesh-B39320
> > > > Cc: u-boot at lists.denx.de; Sun York-R58495; Gupta Ruchika-R66431
> > > > Subject: Re: [U-Boot, 1/2, v4] powerpc/mpc85xx: SECURE BOOT- NAND
> > > > secure boot target for P3041
> > > >
> > > > [Reposting comment on v4 as York requested]
> > > >
> > > > On Wed, Feb 25, 2015 at 02:17:56PM +0530, Aneesh Bansal wrote:
> > > > > diff --git a/arch/powerpc/cpu/mpc85xx/cpu_init.c
> > > > > b/arch/powerpc/cpu/mpc85xx/cpu_init.c
> > > > > index 4cf8853..ef56cc0 100644
> > > > > --- a/arch/powerpc/cpu/mpc85xx/cpu_init.c
> > > > > +++ b/arch/powerpc/cpu/mpc85xx/cpu_init.c
> > > > > @@ -843,6 +843,23 @@ int cpu_init_r(void)
> > > > >  	setup_mp();
> > > > >  #endif
> > > > >
> > > > > +#if defined(CONFIG_SYS_RAMBOOT) &&
> > > > defined(CONFIG_SYS_INIT_L3_ADDR) && \
> > > > > +	defined(CONFIG_SECURE_BOOT)
> > > > > +	/* Disable the TLB Created for L3 and create the TLB required for
> > > > > +	 * PCIE (CONFIG_SYS_PCIE1_MEM_VIRT) which was not created
> > > > earlier.
> > > > > +	 */
> > > > > +	int tlb_index;
> > > > > +	tlb_index = find_tlb_idx((void *)CONFIG_BPTR_VIRT_ADDR, 1);
> > > > > +	if (tlb_index != -1) {
> > > > > +		disable_tlb(tlb_index);
> > > > > +
> > > > > +		set_tlb(1, CONFIG_SYS_PCIE1_MEM_VIRT,
> > > > > +			CONFIG_SYS_PCIE1_MEM_PHYS,
> > > > > +			MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
> > > > > +			0, tlb_index, BOOKE_PAGESZ_1G, 1);
> > > > > +	}
> > > > > +#endif
> > > >
> > > > Why are you assuming in generic 85xx code that the TLB for PCIE1
> > > > needs to be created?  e500mc should have enough TLB1 entries that
> > > > you don't need to share (or if it's due to address conflicts, a
> > > > board may have PCI at a different address), and PCI may not exist at all on
> > some boards.
> > > >
> > > > -Scott
> > >
> > > TLB's are created in freescale/common/p_corenet/tlb.c
> > 
> > Which doesn't apply to all 85xx boards (even custom corenet-based boards
> > might not use it -- or if that's not the case, it should be moved out of the
> > board directory).  It's also not obvious to anyone modifying that tlb.c file or
> > the address of PCIE1 that this would be affected.

Note that on b4860qds, it's SRIO2_MEM_VIRT that conflicts with
0xbff00000, not PCIE1_MEM_VIRT.

> > 
> > > In case of Secure Boot, L3 is used as 1M SRAM and the address of the
> > SRAM is at 0xbff00000.
> > 
> > Is this hardcoded into the silicon, or determined by PBI or something similar?
> > If it's not hardcoded, can we choose a less problematic address?
> > 
> It is not hardcoded but we have a restriction of choosing the address within 0 - 3.5G.
> 0xbff00000 seemed to be the least problematic at this point of time.

Where does the 3.5G limitation come from?  Even if the physical address
needs to be elsewhere due to bootrom constraints, we should be able to
map it wherever we want in the TLB once U-Boot takes control.

> > If it is hardcoded, and we don't want to change the PCIE1 virtual address,

Actually it's PCIE2 that conflicts -- U-Boot just happens to use one big
TLB entry to cover multiple PCIEs.

> > at
> > least create defines for the entry to be created once SRAM goes away,
> > rather than hardcoding PCIE1 here.
> > 
> Are you suggesting something like this in cpu_init_r()
> set_tlb(1, CONFIG_SECBOOT_TLB_VIRT_ADDR,
> 	CONFIG_SECBOOT_TLB_PHYS_ADDR,
> 	CONFIG_SECBOOT_TLB_PERM, CONFIG_SECBOOT_TLB_ATTR,
> 	0, tlb_index, CONFIG_SECBOOT_TLB_PAGESZ, 1);

If you can't remove the 3.5G limitation, yes.

> I plan to define these macros in tlb.c where we have added the code for these TLBS creation
> 
> #define CONFIG_SECBOOT_TLB_VIRT_ADDR	CONFIG_SYS_PCIE1_MEM_VIRT
> #define CONFIG_SECBOOT_TLB_PHYS_ADDR	CONFIG_SYS_PCIE1_MEM_PHYS
> #define CONFIG_SECBOOT_TLB_PERM 		MAS3_SW|MAS3_SR
> #define CONFIG_SECBOOT_TLB_ATTR		MAS2_I|MAS2_G
> #define CONFIG_SECBOOT_TLB_PAGESZ	BOOKE_PAGESZ_1G

No, they are board-specific and need to be defined in the same place
that the rest of the address map is defined (which I see you did in the
v5 patch).

-Scott