On Mon, Sep 12, 2022 at 11:41:32AM -0700, Gokul krishna Krishnakumar wrote: > By moving the memory allocation to mdt loader we can simplify the scm > call, by just packing arguments provided to it from the clients for > making secuer world calls. We can also simplify the memory allocation > for the qcom metadata, by just doing one memory allocation in the > mdt loader. > > Signed-off-by: Gokul krishna Krishnakumar <quic_gokukris@xxxxxxxxxxx> > --- > drivers/remoteproc/qcom_q6v5_mss.c | 2 +- > drivers/soc/qcom/mdt_loader.c | 41 ++++++++++++++++++++++++++++--------- > include/linux/soc/qcom/mdt_loader.h | 5 +++-- > 3 files changed, 35 insertions(+), 13 deletions(-) > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c > index fddb63c..1919bfc 100644 > --- a/drivers/remoteproc/qcom_q6v5_mss.c > +++ b/drivers/remoteproc/qcom_q6v5_mss.c > @@ -947,7 +947,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw, > int ret; > int i; > > - metadata = qcom_mdt_read_metadata(fw, &size, fw_name, qproc->dev); > + metadata = qcom_mdt_read_metadata(fw, &size, fw_name, qproc->dev, NULL); At the end of this function we invoke kfree(metadata), which would be bad if that comes from dma_alloc_coherent(). > if (IS_ERR(metadata)) > return PTR_ERR(metadata); > > diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c [..] > @@ -160,9 +164,18 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len, > ehdr_size = phdrs[0].p_filesz; > hash_size = phdrs[hash_segment].p_filesz; > > - data = kmalloc(ehdr_size + hash_size, GFP_KERNEL); > - if (!data) > - return ERR_PTR(-ENOMEM); > + /* > + * During the scm call memory protection will be enabled for the meta > + * data blob, so make sure it's physically contiguous, 4K aligned and > + * non-cachable to avoid XPU violations. > + */ > + scm_dev = qcom_get_scm_device(); As LKP points out, I don't seem to have this function. > + data = dma_alloc_coherent(scm_dev, ehdr_size + hash_size, mdata_phys, > + GFP_KERNEL); I am not thrilled about the idea of doing dma_alloc_coherent() in this file and dma_free_coherent() in the scm driver. Similarly, I consider these functions to operate in the context of the caller, so operating on the scm device's struct device isn't so nice. After trying various models I came to the conclusion that it was better to try to keep the MDT loader to just load MDT files, and move the SCM/PAS interaction out of that. Unfortunately we have a number of client drivers that would then need to (essentially) duplicate the content of qcom_mdt_pas_init() - so I left that in there. I still believe that keeping the MDT loader focused on loading MDTs is a good idea, but I'm open to any suggestions for improvements in the interaction between these different components. Regards, Bjorn
