Re: [PATCH u-boot-mvebu 2/3] arm: mvebu: a37xx: Map CCI-400 and AP BootROM address space

[Stefan Roese <sr@denx.de>]

On 2/15/22 14:04, Pali Rohár wrote:
> On Tuesday 15 February 2022 13:11:25 Marek Behún wrote:
>> On Tue, 15 Feb 2022 00:28:34 +0100
>> Pali Rohár <pali@kernel.org> wrote:
>>
>>> In function build_mem_map() prepares also mapping for CCI-400 and
>>                              * prepare
>>> AP BootROM address space.
>>>
>>> A53 AP BootROM by default starts at address 0xfff00000 and is 16 kB long.
>>
>> RVBAR_EL3 register has value 0xffff0000. The BootROM is 16 KiB long but
>> the window is 1 MiB long, so the content repeats every 4 KiB.
>>
>>> CCI-400 in new TF-A version starts at address 0xfe000000 and is 64 kB long.
>>>
>>> Physical addresses are read directly from mvebu registers, so if TF-A
>>> remaps it in future then it would not cause any issue.
>>
>> As we talked about in private conversation, I still don't think we
>> should do this unless it is needed.
>>
>> CCI may be needed to be mapped if ever there is some driver that needs
>> to interact with it.
>>
>> BootROM is never needed by the U-Boot code.
>>
>> I really don't think that we should map these in production U-Boot
>> binaries for everyone, when the intention is "for debugging
>> purposes only". In the last 4 years there were 2 people (me, and you
>> :)) who were interested in BootROM. In the next 10 years there will be
>> maybe 2 more. So I really don't think the windows should be mapped for
>> everyone.
> 
> I looked at this stuff because other people asked me about possibility
> to dump BootROM. So it is not "only me".
> 
> Anyway, the main issue with all this stuff is that there is no public
> documentation for it and things which are missing in U-Boot would be
> missing forever (because there are only few people with access to
> documentation; which is even more incomplete, inconsistent and
> incorrect). So adding this stuff may help other people from community
> who would like to study this platform or code.
> 
> Note that, by default U-Boot has already enabled 'md', 'mw', 'pci' and
> so other commands which are intended for debug purposes only, so I do
> not see reason why _in this version_ cannot be mapped also BootROM code.
> 
> In _production version_ where is no debug capability and no access to
> any memory (just ability to boot) is is probably not needed, but none of
> A3720 board is building this kind of version (by default). And in case
> BootROM is mapped also in these versions, is there any issue with it?
> BootROM is read-only, same in all A3720 SoCs, so by this definition it
> is public and everybody can inspect it...
> 
> Stefan, what do you think about it?

Frankly, my first thought was similar to what Marek expressed. Do we
really need this? But I also see your point and from the "security"
point of view, I don't see that the system get's more insecure by
enabling access to these areas. And at least I myself have been happy
to have all kind of debugging possibilities enabled in U-Boot per
default.

So to sum it up, I'm currently in favor to accepting these changes
right now.

Thanks,
Stefan