[RFC PATCH] soc: Amlogic: Add secure monitor driver
Carlo Caione
carlo at caione.org
Mon May 16 13:30:50 PDT 2016
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.
[...]
>> +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
--
Carlo Caione
More information about the linux-arm-kernel
mailing list