The format defined in OP-TEE, available here [1], allows concatenation of several firmware images into one signed binary image. For instance, for the stm32mp2 platform, we can run a TF-M and a Zephyr firmware on the Cortex-M33. [1] https://github.com/OP-TEE/optee_os/blob/master/ta/remoteproc/src/remoteproc_core.c#L18 > >> - decrypt images if encrypte > > This is done for some Qualcomm remoteprocs as well. Do you decrypt before authenticating? On the stm32mp1, due to memory constraints, we encrypt only the ELF segment to load. We compute the signature based on the encrypted version of the ELF. So, we authenticate before decrypting." > >> - ensure that the load is done in granted memories > > At the top of your reply you say that you're loading the firmware into > Linux memory, then you invoke TA_RPROC_FW_CMD_LOAD_FW to "load" it once > more - presumably copying into some other memory. Sorry if it was not clear. By 'loading the firmware into Linux memory' I mean calling request_firmware() to get the image from the filesystem and copy it into Linux-allocated memory. Then, the address of this memory is registered in OP-TEE memory space and provided as a parameter of the TA_RPROC_FW_CMD_LOAD_FW command. > > It sounds like this is an optimization to fail early in the case that > something is wrong? Not only that, it is to ensure that everything is valid and decrypted before enabling the parsing of the resource table (rproc_parse_fw()). > >> - manage the memory access rights to enure that the code and data memory >> is never accessible by the Linux. >> > > Right, after authentication Linux must be locked out. That's done in the > Qualcomm "authenticate and start" phase, at the very end of the loading > and setting things up. Make sense, but not possible with the resource table handling. > >>> >>> If we improve the definition of rproc_load_segments() to mean >>> "remoteproc (or remoteproc driver) is loading segments", then in your >>> case there's no "loading" operation in Linux. Instead you make that a >>> nop and invoke LOAD_FW and START_FW within your start callback, then you >>> can clean up the remnant state within your driver's start and stop >>> callbacks - without complicating the core framework. >> >> This would not work as I need to load the firmware before calling >> rproc_handle_resources(). Yes this is the blocking point. I suppose that you don't have this constraint in Qualcomm implementations? >> > > Ohh, now I see what you're saying. This is why you should follow > https://docs.kernel.org/process/submitting-patches.html#describe-your-changes > when you write a commit message. Then I would have understood your > problem in the very first paragraph of the patch. My apologize with the succession of versions and associated discussions. I forgot that a series version must be self-sufficient in terms of explanation. > > This very much sounds like what's intended to happen in > rproc_parse_fw(), per the comment right next to the call: > > /* Load resource table, core dump segment list etc from the firmware */ Yes, the main difference is that for ELF files, we get the resource table from the ELF image and put it in rproc->cached_table. Then, the cached_table is updated in rproc_handle_resources() and finally written to the destination memory in rproc_start(). For the TEE implementation, we directly provide the destination memory address, which is stored in rproc->cached_table. Notice that the rproc->cached_table is mandatory for the recovery. > > > But I'm guessing that would require loading the segments etc into the > destination memory, perform the authentication and then you can get hold > of the resource table? > > Which..per your patch you do before calling rproc_fw_boot(), so you're > already diverging completely from the expected flow. The call of rproc_load_fw() should be moved in rproc_fw_boot(), I guess What would you suggest as the next step for these commits? Should I just rename load_fw to release_fw? If so, do you have a naming suggestion? Or do we need to find another way to sequence the boot? Thanks, Arnaud > >> I can not use rproc_prepare_device() as it is not called on recovery >> > > The purpose of rproc_prepare_device() is to ensure that any clocks etc > for the memory where we're going to load the firmware is enabled, so > that doesn't sounds like the right place. > > Regards, > Bjorn > >> Thanks, >> Arnaud >> >>> >>> Regards, >>> Bjorn >>> >>>> + * @release_fw: optional function to release the firmware image from memories. >>>> + * This function is called after stopping the remote processor or in case of error >>>> */ >>>> struct rproc_ops { >>>> int (*prepare)(struct rproc *rproc); >>>> @@ -403,6 +407,8 @@ struct rproc_ops { >>>> u64 (*get_boot_addr)(struct rproc *rproc, const struct firmware *fw); >>>> unsigned long (*panic)(struct rproc *rproc); >>>> void (*coredump)(struct rproc *rproc); >>>> + int (*load_fw)(struct rproc *rproc, const struct firmware *fw); >>>> + void (*release_fw)(struct rproc *rproc); >>>> }; >>>> >>>> /** >>>> -- >>>> 2.25.1 >>>>
