[PATCH v1 1/2] drivers: firmware: introduce Meson Secure Monitor driver

[neil.armstrong at linaro.org]

Hi,

On 16/07/2023 01:40, Simon Glass wrote:
> Hi,
> 
> On Thu, 13 Jul 2023 at 23:30, AKASHI Takahiro
> <takahiro.akashi at linaro.org> wrote:
>>
>> On Tue, Jul 11, 2023 at 01:13:29PM -0600, Simon Glass wrote:
>>> +AKASHI Takahiro
>>
>> Me?
> 
> Yes, I'm asking for your help to try to clean this stuff up.

The thread is long and hard to answer directly, but as AKASHI
said there's no point to add a SMC class since it's only the message
passing instruction, and there's no point using remoteproc since the
firmware runs on a separate secure state of the same CPU.

And I don't see how we can actually define a finite set of ops because
none of the secure firmware interfaces has even similar functions.

So a new UCLASS for each firmware interface should be added, not sure
this is scalable or required since those firmwares are mainly SoC or
vendor specific, except the PSCI or other ARM specific interfaces of course.

So I think UCLASS_FIRMWARE is good enough since it avoids using UCLASS_MISC,
but it should be probably documented somewhere that the ops are implementation
defined.

Neil

> 
>>
>>> Hi Alexey,
>>>
>>> On Tue, 11 Jul 2023 at 04:25, Alexey Romanov <AVRomanov at sberdevices.ru> wrote:
>>>>
>>>> Hi Simon,
>>>>
>>>> On Mon, Jul 10, 2023 at 01:45:53PM -0600, Simon Glass wrote:
>>>>> Hi Alexey,
>>>>>
>>>>> On Mon, 10 Jul 2023 at 02:34, Alexey Romanov <AVRomanov at sberdevices.ru> wrote:
>>>>>>
>>>>>>
>>>>>> Hello!
>>>>>>
>>>>>> On Fri, Jul 07, 2023 at 11:35:47AM -0600, Simon Glass wrote:
>>>>>>> Hi Alexey,
>>>>>>>
>>>>>>> On Fri, 7 Jul 2023 at 09:43, Alexey Romanov <AVRomanov at sberdevices.ru> wrote:
>>>>>>>>
>>>>>>>> Hello, Simon!
>>>>>>>>
>>>>>>>> On Thu, Jul 06, 2023 at 09:58:02AM -0600, Simon Glass wrote:
>>>>>>>>> Hi Alexey,
>>>>>>>>>
>>>>>>>>> On Thu, 6 Jul 2023 at 14:16, Alexey Romanov <avromanov at sberdevices.ru> wrote:
>>>>>>>>>>
>>>>>>>>>> At the moment, only smc API is a set of functions in
>>>>>>>>>> arch/arm/mach-meson/sm.c. This approach is hard to configure
>>>>>>>>>> and also doesni't contain any generic API for calling smc.
>>>>>>>>>>
>>>>>>>>>> This patch add Meson SM driver with generic API (struct meson_sm_ops):
>>>>>>>>>>
>>>>>>>>>> - sm_call()
>>>>>>>>>> - sm_call_write()
>>>>>>>>>> - sm_call_read()
>>>>>>>>>>
>>>>>>>>>> A typical driver usage example is shown here:
>>>>>>>>>>
>>>>>>>>>> 1. uclass_get_device_by_driver(UCLASS_FIRMWARE, "secure-monitor", &dev);
>>>>>>>>>> 2. handle = meson_sm_get_handle(dev);
>>>>>>>>>> 3. handle->ops.sm_call(dev, cmd, ...);
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Alexey Romanov <avromanov at sberdevices.ru>
>>>>>>>>>> ---
>>>>>>>>>>   arch/arm/mach-meson/Kconfig       |   1 +
>>>>>>>>>>   drivers/firmware/Kconfig          |  10 ++
>>>>>>>>>>   drivers/firmware/Makefile         |   1 +
>>>>>>>>>>   drivers/firmware/meson/Kconfig    |   6 +
>>>>>>>>>>   drivers/firmware/meson/Makefile   |   3 +
>>>>>>>>>>   drivers/firmware/meson/meson_sm.c | 217 ++++++++++++++++++++++++++++++
>>>>>>>>>>   include/meson/sm_handle.h         |  38 ++++++
>>>>>>>>>>   7 files changed, 276 insertions(+)
>>>>>>>>>>   create mode 100644 drivers/firmware/meson/Kconfig
>>>>>>>>>>   create mode 100644 drivers/firmware/meson/Makefile
>>>>>>>>>>   create mode 100644 drivers/firmware/meson/meson_sm.c
>>>>>>>>>>   create mode 100644 include/meson/sm_handle.h
>>>>>>>>>
>>>>>>>>> Please can you use the remoteproc uclass for this and add a proper driver?
>>>>>>>>>
>>>>>>>>
>>>>>>>> I don't see it architecturally well. Can you explain please?
>>>>>>>>
>>>>>>>> This driver is just ARM SMC fw interface. There seems to be nothing to
>>>>>>>> do here for remoteproc uclass.
>>>>>>>
>>>>>>> Well you seem to be implementing a remote CPU interface, which is what
>>>>>>> remoteproc is for. How does Linux do this?
>>>>>>
>>>>>> The main idea of the patchset is to abstract the smc calls (which run on
>>>>>> the same CPU) and make a request to the firmware that runs on a higher
>>>>>> EL. UCLASS_REMOTEPROC may give the impression that we are
>>>>>> interacting with another CPU, although this is not the case.
>>>>>>
>>>>>> Also, UCLASS_REMOTEPROC requires two mandatory interfaces: load() and
>>>>>> start(), and I don't even know how to apply my current changes to them.
>>>>>
>>>>> You can return -ENOSYS if not implemented.
>>>>>
>>>>>>
>>>>>> My implementation is very close to the Linux implementation, they
>>>>>> also use the firmware driver without remoteproc:
>>>>>>
>>>>>> https://elixir.bootlin.com/linux/latest/source/drivers/firmware/meson/meson_sm.c
>>>>>
>>>>> Yes that seems like it doesn't use any common infra. I don't think it
>>>>> is a great model for U-Boot though.
>>>>>
>>>>>>
>>>>>> I spoke to Neil on IRC and he said UCLASS_FIRMWARE for such driver is
>>>>>> OK.
>>>>>>
>>>>>>>
>>>>>>> Also there is a pending series on FFA - is that related? It uses smc
>>>>>>> from what I can tell.
>>>>>>
>>>>>> Not entirely clear, my changes don't seem to be related to this
>>>>>> patchset.
>>>>>
>>>>> So perhaps this needs a new UCLASS_SMC? I see various other SMC calls
>>>>> in U-Boot but no one has taken the initiative to think about this in
>>>>> terms of driver model.
>>>>
>>>> What will be the feature of this uclass? If it's just us adding a new
>>>> uclass with a different name... Don't know if that makes amy sense?
>>>> Then the difference between UCLASS_SMC and UCLASS_FIRMWARE will be only
>>>> in the name.
>>>
>>> Honestly I'm not sure, but I question why you are putting all this on
>>> the reviewer. Can't you look around the code base and figure out a
>>> sensible approach and defend it in your patch?
>>>
>>> I've just reviewed the SCMI stuff which use UCLASS_MISC in one case.
>>
>> Do you mean a "sandbox_scmi_devices" in drivers/firmware/scmi/sandbox-scmi_devices.c?
>> If so, it has nothing to do with the discussion here.
>>
>> It is a kind of pseudo device for dm ut, which is subject to manage
>> with the faked scmi server in sandbox (sandbox-scmi_agent.c).
>> It is connected, for instance, to clock lines or vol regulators.
>> (see sandbox's test.dts.)
>>
>> I believe that similar usages can be seen across drivers/, for instance,
>> drivers/clk/clk_sandbox_test.c.
>>
>> Do you see anything wrong with it?
> 
> At this point I'm just confused. Perhaps MISC and FIRMWARE should not
> be used as much? It seems that FIRMWARE doesn't really mean anything
> at present?
> 
>>
>>> I'm not sure if this is related to firmware or not, or whether what
>>> you are doing relates.
>>>
>>>>
>>>>>
>>>>> It might just need one operation, to make a call, passing a regs
>>>>> struct, perhaps? The uclass would presumably be ARM-specific.
>>>>>
>>>>> I really don't think this is UCLASS_FIRMWARE. That seems to be for
>>>>> loading firmware. It doesn't even have a firmware.h header.
>>>>
>>>> I see what drivers/firmware/psci.c is UCLASS_FIRMWARE and also use
>>>> smc_call() function. Or firmware-zynqmo.c. In this case, we then have to
>>>> convert them to UCLASS_SMC in the same way?
>>
>> SMCCC stands for SMC Calling Convention, where SMC (or Secure Monitor Call)
>> instruction escalates the cpu's EL (Execution Level) to EL3 (secure).
>> This interface is clearly defined by Arm, and many high level services are
>> defined and implemented on top of that.
>> PSCI and FF-A are ones among those users, and even vendor specific functions
>> may be allowed.
>>
>>  From the viewpoint of implementation, arm_smccc_smc() (and smc_call()) is
>> just a C helper function around one SMC assembly. I don't think we need to
>> have UCLASS_SMC as it is basically a communication method.
>>
>> Nevertheless, I agree with Simon in the point that the SMC related code
>> can be a bit reworked since there are duplicated code.
>> Especially, the followings are essentially the same:
>>    -- arch/arm/cpu/armv8/fwcall.c with smc_call()
>>         psci_system_reset()
>>         psci_system_off()
>>    -- drivers/firmware/psci.c with arm_smccc_smc()
>>         psci_sys_reset()
>>         psci_sys_poweroff()
> 
> OK, what that is a start.
> 
>>
>>
>>>> It seemed to me that UCLASS_FIRMWARE is a SoC(arch)-specific driver
>>>> that can work with any interface.
>>
>> IMO, UCLASS_FIRMWARE is quite misleading as all the drivers belonging to
>> this class do not have any common operations.
> 
> Well misc.h does actually have operations, but yes, people misuse it.
> Perhaps we should invent something else for these people?
> 
> For FIRMWARE, what should it mean? It needs to have a firmware.h file
> with some information.
> 
>> (And UCLASS_MISC as well.)
>>
>> -Takahiro Akashi
>>
>>> OK, well please take a look at all this and see what you think is
>>> best. If UCLASS_FIRMWARE is correct, then please create a proper
>>> firmware.h file and define what the uclass operations are. You can't
>>> just randomly create your own private operations and ignore the rest
>>> of the code base.
>>>
>>> Driver model is intended to bring order to the chaos that used to
>>> exist with drivers in U-Boot. Please see [1] for an introduction.
>>>
>>> Please try to dig in and understand how things should be done. It is
>>> important for the health of the project.
>>>
>>>>
>>>>>
>>>>> Also instead of:
>>>>>
>>>>> 1. uclass_get_device_by_driver(UCLASS_FIRMWARE, "secure-monitor", &dev);
>>>>>
>>>>> use:
>>>>>
>>>>> ret = uclass_first_device_err(UCLASS_SMC, &dev)
>>>>>
>>>>> using device tree to find the device.
>>>>>
>>>>> Regards,
>>>>> Simon
>>>>
>>>> --
>>>> Thank you,
>>>> Alexey
>>>
>>> [1] https://elinux.org/Boot_Loaders#Order_at_Last:_The_New_U-Boot_Driver_Model_Architecture_.5BELCE_2015.5D
> 
> Regards,
> Simon