[RFC PATCH] soc: Amlogic: Add secure monitor driver
Matthias Brugger
matthias.bgg at gmail.com
Mon May 23 04:09:51 PDT 2016
On 16/05/16 22:30, Carlo Caione wrote:
> On Mon, May 16, 2016 at 1:33 PM, Mark Rutland <mark.rutland at arm.com> wrote:
>> On Sun, May 15, 2016 at 08:10:45PM +0200, Carlo Caione wrote:
>>> From: Carlo Caione <carlo at endlessm.com>
>>>
>>> Introduce a driver to provide calls into secure monitor mode.
>>>
>>> In the Amlogic SoCs these calls are used for multiple reasons: access to
>>> NVMEM, reboot, set USB boot, enable JTAG, etc...
>>>
>>> Signed-off-by: Carlo Caione <carlo at endlessm.com>
>>> ---
>>> arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 4 +
>>> drivers/soc/Kconfig | 1 +
>>> drivers/soc/Makefile | 1 +
>>> drivers/soc/amlogic/Kconfig | 8 ++
>>> drivers/soc/amlogic/Makefile | 1 +
>>> drivers/soc/amlogic/meson_sm.c | 141 ++++++++++++++++++++++++++++
>>> include/linux/soc/amlogic/meson_sm.h | 57 +++++++++++
>>> 7 files changed, 213 insertions(+)
>>
>> The binding is missing documentation.
>
> Yes, not much to document for this RFC.
>
>> Is there any documentation of the actual interface?
>
> There isn't or at least I do not have it. This driver has been written
> by reverse engineering the Amlogic driver shipped in their SDK [1]
>
>> Are there any restrictions on how it's used? It looks like calls are
>> only expected to happen from the boot CPU, per the code below.
>
> Yes, that's what I could infer from the Amlogic code here [2] but then
> I checked this better and it seems that's not the case (see below).
>
>> There are no users in this patch. Are there any example uses (i.e.
>> specific code rather than the high-level examples above)?
>
> I use it already to read the NVMEM. It works fine.
> I'll submit the NVMEM driver as soon as I have this merged (if ever)
>
>>> create mode 100644 drivers/soc/amlogic/Kconfig
>>> create mode 100644 drivers/soc/amlogic/Makefile
>>> create mode 100644 drivers/soc/amlogic/meson_sm.c
>>> create mode 100644 include/linux/soc/amlogic/meson_sm.h
>>>
>>> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
>>> index 76b3b6d..e4017c3 100644
>>> --- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
>>> +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
>>> @@ -98,6 +98,10 @@
>>> method = "smc";
>>> };
>>>
>>> + sm {
>>> + compatible = "amlogic,meson-sm";
>>> + };
>>
>> We should have more specific strings, at least in addition to this.
>
> Agree, not sure what I can add though.
I would say someting linke "amlogic,gxbb-sm" to specify the SoC.
>
> [...]
>>> +struct meson_sm_data {
>>> + unsigned int cmd;
>>> + unsigned int arg0;
>>> + unsigned int arg1;
>>> + unsigned int arg2;
>>> + unsigned int arg3;
>>> + unsigned int arg4;
>>> + unsigned int ret;
>>> +};
>>
>> Please be explicit and use u32 rather than unsigned int.
>
> Ok
>
>> It looks like all the calls you care about are SMC32 SiP calls, per the
>> IDs in the header.
>
> Yes, all the SMC function identifiers I gathered from the Amlogic
> sources are SMC32 SiP calls
>
>>> +
>>> +static void __meson_sm_call(void *info)
>>> +{
>>> + struct meson_sm_data *data = info;
>>> + struct arm_smccc_res res;
>>> +
>>> + arm_smccc_smc(data->cmd,
>>> + data->arg0, data->arg1, data->arg2,
>>> + data->arg3, data->arg4, 0, 0, &res);
>>> + data->ret = res.a0;
>>> +}
>>> +
>>> +unsigned int meson_sm_call(unsigned int cmd, unsigned int arg0,
>>> + unsigned int arg1, unsigned int arg2,
>>> + unsigned int arg3, unsigned int arg4)
>>> +{
>>> + struct meson_sm_data data;
>>> +
>>> + data.cmd = cmd;
>>> + data.arg0 = arg0;
>>> + data.arg1 = arg1;
>>> + data.arg2 = arg2;
>>> + data.arg3 = arg3;
>>> + data.arg4 = arg4;
>>> + data.ret = 0;
>>> +
>>> + smp_call_function_single(0, __meson_sm_call, &data, 1);
>>
>> Why must this occur on a particular CPU?
>>
>> With kexec and so on logical CPU 0 is not necessarily the CPU that the
>> FW brought out of reset.
>
> You are right. I tried to move this to a different CPU and it keeps
> working fine. Still I wonder why Amlogic has something like [2]
>
>> Can the core with the secure OS be powered down, or is that prevented
>> with the usual PSCI machinery (e.g. MIGRATE, MIGRATE_INFO_TYPE,
>> MIGRATE_INFO_UP_CPU)
>>
>> I would recommend that any CPU restriction were described explicitly in
>> the DT, or detected dynamically based on the MIGRATE_INFO_UP_CPU value.
>
> MIGRATE_INFO_TYPE returns PSCI_0_2_TOS_MP. Does this confirm that the
> SMC calls can happen not only from the CPU 0?
>
>>> +
>>> + return data.ret;
>>> +}
>>> +
>>> +unsigned int meson_sm_call_read(void *buffer, unsigned int cmd,
>>> + unsigned int arg0, unsigned int arg1,
>>> + unsigned int arg2, unsigned int arg3,
>>> + unsigned int arg4)
>>> +{
>>> + unsigned int size;
>>> +
>>> + size = meson_sm_call(cmd, arg0, arg1, arg2, arg3, arg4);
>>> +
>>> + if (size != 0)
>>
>> Is the size guaranteed to never be > SM_MEM_SIZE? Can we sanity-check
>> that?
>
> Fix in v2
>
>> The call can't return an error code?
>
> 0 in R0 is considered error that we return back.
>
>>> + memcpy(buffer, sm_sharemem_out_base, size);
>>
>> Is the buffer completely opaque to this layer?
>>
>> Are there any endianness concerns, for example?
>
> The best answer I can give you here is: not that I know of.
> The driver works fine when reading from the NVMEM, so I assume there
> isn't any endiness issue.
>
>>> +
>>> + return size;
>>> +}
>>> +
>>> +unsigned int meson_sm_call_write(void *buffer, unsigned int size,
>>> + unsigned int cmd, unsigned int arg0,
>>> + unsigned int arg1, unsigned int arg2,
>>> + unsigned int arg3, unsigned int arg4)
>>> +{
>>> + memcpy(sm_sharemem_in_base, buffer, size);
>>> +
>>> + return meson_sm_call(cmd, arg0, arg1, arg2, arg3, arg4);
>>> +}
>>> +
>>> +static int meson_sm_probe(struct platform_device *pdev)
>>> +{
>>> + sm_phy_in_base = meson_sm_call(SM_GET_SHARE_MEM_INPUT_BASE, 0, 0, 0, 0, 0);
>>> +
>>> + sm_sharemem_in_base = ioremap_cache(sm_phy_in_base, SM_MEM_SIZE);
>>> + if (!sm_sharemem_in_base)
>>> + return -EINVAL;
>>
>> Are the attributes used for ioremap_cache guaranteed to match and not
>> cause a mismatched alias against the firmware's mapping?
>>
>> Which precise memory attributes does the secure firmware use for the
>> shared memory?
>
> Again I wish I had more information. Is there any way I can check this?
> The only reasonable answer I can give you right now is that this is
> how it is done in the original Amlogic driver in [1]
>
> [...]
>>> +#define SM_GET_SHARE_MEM_INPUT_BASE 0x82000020
>>> +#define SM_GET_SHARE_MEM_OUTPUT_BASE 0x82000021
>>> +#define SM_GET_REBOOT_REASON 0x82000022
>>> +#define SM_GET_SHARE_STORAGE_IN_BASE 0x82000023
>>> +#define SM_GET_SHARE_STORAGE_OUT_BASE 0x82000024
>>> +#define SM_GET_SHARE_STORAGE_BLOCK_BASE 0x82000025
>>> +#define SM_GET_SHARE_STORAGE_MESSAGE_BASE 0x82000026
>>> +#define SM_GET_SHARE_STORAGE_BLOCK_SIZE 0x82000027
>>> +#define SM_EFUSE_READ 0x82000030
>>> +#define SM_EFUSE_WRITE 0x82000031
>>> +#define SM_EFUSE_WRITE_PATTERN 0x82000032
>>> +#define SM_EFUSE_USER_MAX 0x82000033
>>> +#define SM_JTAG_ON 0x82000040
>>> +#define SM_JTAG_OFF 0x82000041
>>> +#define SM_SET_USB_BOOT_FUNC 0x82000043
>>> +#define SM_SECURITY_KEY_QUERY 0x82000060
>>> +#define SM_SECURITY_KEY_READ 0x82000061
>>> +#define SM_SECURITY_KEY_WRITE 0x82000062
>>> +#define SM_SECURITY_KEY_TELL 0x82000063
>>> +#define SM_SECURITY_KEY_VERIFY 0x82000064
>>> +#define SM_SECURITY_KEY_STATUS 0x82000065
>>> +#define SM_SECURITY_KEY_NOTIFY 0x82000066
>>> +#define SM_SECURITY_KEY_LIST 0x82000067
>>> +#define SM_SECURITY_KEY_REMOVE 0x82000068
>>> +#define SM_DEBUG_EFUSE_WRITE_PATTERN 0x820000F0
>>> +#define SM_DEBUG_EFUSE_READ_PATTERN 0x820000F1
>>> +#define SM_AML_DATA_PROCESS 0x820000FF
>>
>> Is there any documentation for these functions?
>
> Unfortunately there isn't.
> I could restrict the list to those functions I know how to use
> (*_EFUSE) maybe adding the remaining ones when we actually use them.
>
>>> +#define SM_PSCI_SYS_REBOOT 0x84000009
>>
>> The existing PCSI code should be the only caller of this. This should
>> not be used here.
>
> Ups, that slipped through when pasting the list.
>
> Thank you for reviewing this RFC, I'll submit a v2 soon.
>
> Cheers,
>
> [1] https://github.com/hardkernel/linux/blob/odroidc2-3.14.y/drivers/amlogic/secmon/secmon.c
> [2] https://github.com/hardkernel/linux/blob/odroidc2-3.14.y/drivers/amlogic/efuse/efuse_hw64.c#L94
>
More information about the linux-amlogic
mailing list