[PATCH v2 10/21] arm: socfpga: Add secure register access helper functions for SoC 64bits

[Marek Vasut]

On 2/21/20 7:01 PM, Ang, Chee Hong wrote:
>> On 2/20/20 6:54 PM, Ang, Chee Hong wrote:
>>>> On 2/20/20 3:02 AM, Ang, Chee Hong wrote:
>>>> [...]
>>>>>>> +#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF)
>>>>>>> +u32 socfpga_secure_reg_read32(phys_addr_t reg_addr); void
>>>>>>> +socfpga_secure_reg_write32(u32 val, phys_addr_t reg_addr); void
>>>>>>> +socfpga_secure_reg_update32(phys_addr_t reg_addr, u32 mask, u32
>>>>>>> +val); #else
>>>>>>> +#define socfpga_secure_reg_read32	readl
>>>>>>> +#define socfpga_secure_reg_write32	writel
>>>>>>> +#define socfpga_secure_reg_update32	clrsetbits_le32
>>>>>>> +#endif
>>>>>>
>>>>>> I think I don't understand how this is supposed to work. Would
>>>>>> every place in U- Boot have to be patched to call these functions now ?
>>>>>
>>>>> Not every register access need this. Only those accessing registers
>>>>> in secure zone such as 'System Manager' registers need to call this.
>>>>> It's basically determine whether the driver should issue SMC/PSCI
>>>>> call if it's running in EL2 (non-secure) or access the registers
>>>>> directly by simply using
>>>> readl/writel and etc if it's running in EL3 (secure).
>>>>> Accessing those registers in secure zone in non-secure mode (EL2)
>>>>> will cause
>>>> SError exception.
>>>>> So we can determine this behaviour in compile time:
>>>>> SPL always running in EL3. So it just simply fallback to use
>>>> readl/writel/clrsetbits_le32.
>>>>>
>>>>> For U-Boot proper (SSBL), there are 2 scenarios:
>>>>> 1) If CONFIG_SPL_ATF is defined, it means ATF is supported. It
>>>>> implies that U-Boot proper will be running in EL2 (non-secure), then
>>>>> it will use
>>>> SMC/PSCI calls to access the secure registers.
>>>>>
>>>>> 2) CONFIG_SPL_ATF is not defined, no ATF support. U-Boot proper will
>>>>> be running in EL3 which will fall back to simply using the direct
>>>>> access functions
>>>> (readl/writel and etc).
>>>>
>>>> I would expect the standard IO accessors would or should handle this stuff ?
>>> Standard IO accessors are just general memory read/write functions
>>> designed to be compatible with general hardware platforms. Not all
>>> platforms have secure/non-secure hardware zones. I don't think they should
>> handle this.
>>>
>>> If it's running in EL3 (secure mode) the standard I/O accessors will
>>> work just fine because
>>> EL3 can access to all secure/non-secure zones. In the header file, you
>>> can see the secure I/O accessors will be replaced by the standard I/O
>>> accessors if it's built for SPL and U-Boot proper without ATF. Because both are
>> running in EL3 (secure).
>>>
>>> If ATF is enabled, SPL will be still running in EL3 but U-Boot proper
>>> will be running in
>>> EL2 (non-secure). If any code accessing those secure zones without
>>> going through ATF (making SMC/PSCI calls to EL3), it will trigger 'SError'
>> exceptions and crash the U-Boot.
>>
>> Hmmm, if U-Boot is running in EL2 (non-secure), why would it ever access
>> secure zones in the first place ?
> SPL and U-Boot reuse the same drivers code. It runs without issue in SPL (EL3) but
> it crashes if running the same driver code in EL2 which access some secure zone registers.
> The System Manager (secure zone) contains some register which control the behaviours of
> EMAC/SDMMC and etc. Clock manager driver rely on those registers in System Manager as well
> for retrieving clock information. These clock/EMAC/SDMMC drivers access the System Manager
> (secure zone).

Maybe those registers should only be configured by the secure OS, so
maybe those drivers should be fixed ?

>> And if that's really necessary, should the ATF really provide secure-mode register
>> access primitives or should it provide some more high-level interface instead ?
> I see your point. I should have mentioned to you that these secure-mode register access provided by
> ATF actually do more stuffs than just primitive accessors.

So socfpga_secure_reg_read32 does not just read a register ? Then the
naming is misleading . And documentation is missing.

> ATF only allow certain secure registers required by the non-secure driver to be accessed.
> It will check the secure register address and block access if the register address is not allowed
> to be accessed by non-secure world (EL2).

Why don't you configure those secure registers in the secure mode then ?
It seems like that's the purpose of those registers being secure only.

> So these secure register access provided by ATF is not just simple accessor/delegate which
> simply allow access to any secure zone from non-secure world without any restrictions.
> I would say the secure register access provided by ATF is a 'middle-level' interface not just
> some primitive accessors.
> 
> Currently, we have like 20+ secure registers allowed access by drivers running in
> non-secure mode (U-Boot proper / Linux).
> I don't think we want to define and maintain those high level interfaces for each of those secure
> register accesses in ATF and U-Boot.

See above.