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 <[email protected]>
---
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);
if (IS_ERR(metadata))
return PTR_ERR(metadata);
diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
index 8d06125..e730413 100644
--- a/drivers/soc/qcom/mdt_loader.c
+++ b/drivers/soc/qcom/mdt_loader.c
@@ -16,6 +16,7 @@
#include <linux/sizes.h>
#include <linux/slab.h>
#include <linux/soc/qcom/mdt_loader.h>
+#include <linux/dma-mapping.h>
static bool mdt_phdr_valid(const struct elf32_phdr *phdr)
{
@@ -110,6 +111,7 @@ EXPORT_SYMBOL_GPL(qcom_mdt_get_size);
* @data_len: length of the read metadata blob
* @fw_name: name of the firmware, for construction of segment file names
* @dev: device handle to associate resources with
+ * @mdata_phys: phys address for the assigned metadata buffer
*
* The mechanism that performs the authentication of the loading firmware
* expects an ELF header directly followed by the segment of hashes, with no
@@ -124,11 +126,13 @@ EXPORT_SYMBOL_GPL(qcom_mdt_get_size);
* Return: pointer to data, or ERR_PTR()
*/
void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len,
- const char *fw_name, struct device *dev)
+ const char *fw_name, struct device *dev,
+ dma_addr_t *mdata_phys)
{
const struct elf32_phdr *phdrs;
const struct elf32_hdr *ehdr;
unsigned int hash_segment = 0;
+ struct device *scm_dev = NULL;
size_t hash_offset;
size_t hash_size;
size_t ehdr_size;
@@ -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();
+ data = dma_alloc_coherent(scm_dev, ehdr_size + hash_size, mdata_phys,
+ GFP_KERNEL);
+ if (!data) {
+ dev_err(dev, "Allocation of metadata buffer failed.\n");
+ return NULL;
+ }
/* Copy ELF header */
memcpy(data, fw->data, ehdr_size);
@@ -179,7 +192,7 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len,
/* Hash is in its own segment, beyond the loaded file */
ret = mdt_load_split_segment(data + ehdr_size, phdrs, hash_segment, fw_name, dev);
if (ret) {
- kfree(data);
+ dma_free_coherent(scm_dev, ehdr_size + hash_size, data, mdata_phys);
return ERR_PTR(ret);
}
}
@@ -209,10 +222,11 @@ int qcom_mdt_pas_init(struct device *dev, const struct firmware *fw,
const struct elf32_phdr *phdr;
const struct elf32_hdr *ehdr;
phys_addr_t min_addr = PHYS_ADDR_MAX;
+ struct device *scm_dev = NULL;
phys_addr_t max_addr = 0;
dma_addr_t mdata_phys;
size_t metadata_len;
- void *metadata;
+ void *mdata_buf;
int ret;
int i;
@@ -232,15 +246,22 @@ int qcom_mdt_pas_init(struct device *dev, const struct firmware *fw,
max_addr = ALIGN(phdr->p_paddr + phdr->p_memsz, SZ_4K);
}
- metadata = qcom_mdt_read_metadata(fw, &metadata_len, fw_name, dev);
- if (IS_ERR(metadata)) {
- ret = PTR_ERR(metadata);
+ mdata_buf = qcom_mdt_read_metadata(fw, &metadata_len, fw_name, dev, &mdata_phys);
+ if (IS_ERR(mdata_buf)) {
+ ret = PTR_ERR(mdata_buf);
dev_err(dev, "error %d reading firmware %s metadata\n", ret, fw_name);
goto out;
}
ret = qcom_scm_pas_init_image(pas_id, mdata_phys);
- kfree(metadata);
+ if (ret || !ctx) {
+ dma_free_coherent(scm_dev, metadata_len, mdata_buf, mdata_phys);
+ } else if (ctx) {
+ ctx->ptr = mdata_buf;
+ ctx->phys = mdata_phys;
+ ctx->size = metadata_len;
+ }
+
if (ret) {
/* Invalid firmware metadata */
dev_err(dev, "error %d initializing firmware %s\n", ret, fw_name);
diff --git a/include/linux/soc/qcom/mdt_loader.h b/include/linux/soc/qcom/mdt_loader.h
index 9e8e604..d438442 100644
--- a/include/linux/soc/qcom/mdt_loader.h
+++ b/include/linux/soc/qcom/mdt_loader.h
@@ -28,7 +28,8 @@ int qcom_mdt_load_no_init(struct device *dev, const struct firmware *fw,
phys_addr_t mem_phys, size_t mem_size,
phys_addr_t *reloc_base);
void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len,
- const char *fw_name, struct device *dev);
+ const char *fw_name, struct device *dev,
+ dma_addr_t *mdata_phys);
#else /* !IS_ENABLED(CONFIG_QCOM_MDT_LOADER) */
@@ -64,7 +65,7 @@ static inline int qcom_mdt_load_no_init(struct device *dev,
static inline void *qcom_mdt_read_metadata(const struct firmware *fw,
size_t *data_len, const char *fw_name,
- struct device *dev)
+ struct device *dev, dma_addr_t *mdata_phys)
{
return ERR_PTR(-ENODEV);
}
--
2.7.4
Hi Gokul,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on remoteproc/rproc-next]
[also build test ERROR on linus/master v6.0-rc5 next-20220912]
[cannot apply to pza/reset/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Gokul-krishna-Krishnakumar/Memory-allocation-change-in-scm-mdt_loader/20220913-024309
base: git://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git rproc-next
config: arm64-randconfig-r003-20220911
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 1546df49f5a6d09df78f569e4137ddb365a3e827)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/93615f3d80d56bd470c334e2055f86bea3e53c25
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Gokul-krishna-Krishnakumar/Memory-allocation-change-in-scm-mdt_loader/20220913-024309
git checkout 93615f3d80d56bd470c334e2055f86bea3e53c25
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/soc/qcom/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
>> drivers/soc/qcom/mdt_loader.c:172:12: error: call to undeclared function 'qcom_get_scm_device'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
scm_dev = qcom_get_scm_device();
^
>> drivers/soc/qcom/mdt_loader.c:172:10: error: incompatible integer to pointer conversion assigning to 'struct device *' from 'int' [-Wint-conversion]
scm_dev = qcom_get_scm_device();
^ ~~~~~~~~~~~~~~~~~~~~~
>> drivers/soc/qcom/mdt_loader.c:195:60: error: incompatible pointer to integer conversion passing 'dma_addr_t *' (aka 'unsigned long long *') to parameter of type 'dma_addr_t' (aka 'unsigned long long'); dereference with * [-Wint-conversion]
dma_free_coherent(scm_dev, ehdr_size + hash_size, data, mdata_phys);
^~~~~~~~~~
*
include/linux/dma-mapping.h:433:30: note: passing argument to parameter 'dma_handle' here
void *cpu_addr, dma_addr_t dma_handle)
^
3 errors generated.
vim +/qcom_get_scm_device +172 drivers/soc/qcom/mdt_loader.c
107
108 /**
109 * qcom_mdt_read_metadata() - read header and metadata from mdt or mbn
110 * @fw: firmware of mdt header or mbn
111 * @data_len: length of the read metadata blob
112 * @fw_name: name of the firmware, for construction of segment file names
113 * @dev: device handle to associate resources with
114 * @mdata_phys: phys address for the assigned metadata buffer
115 *
116 * The mechanism that performs the authentication of the loading firmware
117 * expects an ELF header directly followed by the segment of hashes, with no
118 * padding inbetween. This function allocates a chunk of memory for this pair
119 * and copy the two pieces into the buffer.
120 *
121 * In the case of split firmware the hash is found directly following the ELF
122 * header, rather than at p_offset described by the second program header.
123 *
124 * The caller is responsible to free (kfree()) the returned pointer.
125 *
126 * Return: pointer to data, or ERR_PTR()
127 */
128 void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len,
129 const char *fw_name, struct device *dev,
130 dma_addr_t *mdata_phys)
131 {
132 const struct elf32_phdr *phdrs;
133 const struct elf32_hdr *ehdr;
134 unsigned int hash_segment = 0;
135 struct device *scm_dev = NULL;
136 size_t hash_offset;
137 size_t hash_size;
138 size_t ehdr_size;
139 unsigned int i;
140 ssize_t ret;
141 void *data;
142
143 ehdr = (struct elf32_hdr *)fw->data;
144 phdrs = (struct elf32_phdr *)(ehdr + 1);
145
146 if (ehdr->e_phnum < 2)
147 return ERR_PTR(-EINVAL);
148
149 if (phdrs[0].p_type == PT_LOAD)
150 return ERR_PTR(-EINVAL);
151
152 for (i = 1; i < ehdr->e_phnum; i++) {
153 if ((phdrs[i].p_flags & QCOM_MDT_TYPE_MASK) == QCOM_MDT_TYPE_HASH) {
154 hash_segment = i;
155 break;
156 }
157 }
158
159 if (!hash_segment) {
160 dev_err(dev, "no hash segment found in %s\n", fw_name);
161 return ERR_PTR(-EINVAL);
162 }
163
164 ehdr_size = phdrs[0].p_filesz;
165 hash_size = phdrs[hash_segment].p_filesz;
166
167 /*
168 * During the scm call memory protection will be enabled for the meta
169 * data blob, so make sure it's physically contiguous, 4K aligned and
170 * non-cachable to avoid XPU violations.
171 */
> 172 scm_dev = qcom_get_scm_device();
173 data = dma_alloc_coherent(scm_dev, ehdr_size + hash_size, mdata_phys,
174 GFP_KERNEL);
175 if (!data) {
176 dev_err(dev, "Allocation of metadata buffer failed.\n");
177 return NULL;
178 }
179
180 /* Copy ELF header */
181 memcpy(data, fw->data, ehdr_size);
182
183 if (ehdr_size + hash_size == fw->size) {
184 /* Firmware is split and hash is packed following the ELF header */
185 hash_offset = phdrs[0].p_filesz;
186 memcpy(data + ehdr_size, fw->data + hash_offset, hash_size);
187 } else if (phdrs[hash_segment].p_offset + hash_size <= fw->size) {
188 /* Hash is in its own segment, but within the loaded file */
189 hash_offset = phdrs[hash_segment].p_offset;
190 memcpy(data + ehdr_size, fw->data + hash_offset, hash_size);
191 } else {
192 /* Hash is in its own segment, beyond the loaded file */
193 ret = mdt_load_split_segment(data + ehdr_size, phdrs, hash_segment, fw_name, dev);
194 if (ret) {
> 195 dma_free_coherent(scm_dev, ehdr_size + hash_size, data, mdata_phys);
196 return ERR_PTR(ret);
197 }
198 }
199
200 *data_len = ehdr_size + hash_size;
201
202 return data;
203 }
204 EXPORT_SYMBOL_GPL(qcom_mdt_read_metadata);
205
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi Gokul,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on remoteproc/rproc-next]
[also build test ERROR on linus/master v6.0-rc5 next-20220913]
[cannot apply to pza/reset/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Gokul-krishna-Krishnakumar/Memory-allocation-change-in-scm-mdt_loader/20220913-024309
base: git://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git rproc-next
config: arm-defconfig
compiler: arm-linux-gnueabi-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/93615f3d80d56bd470c334e2055f86bea3e53c25
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Gokul-krishna-Krishnakumar/Memory-allocation-change-in-scm-mdt_loader/20220913-024309
git checkout 93615f3d80d56bd470c334e2055f86bea3e53c25
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/soc/qcom/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All error/warnings (new ones prefixed by >>):
drivers/soc/qcom/mdt_loader.c: In function 'qcom_mdt_read_metadata':
>> drivers/soc/qcom/mdt_loader.c:172:19: error: implicit declaration of function 'qcom_get_scm_device' [-Werror=implicit-function-declaration]
172 | scm_dev = qcom_get_scm_device();
| ^~~~~~~~~~~~~~~~~~~
>> drivers/soc/qcom/mdt_loader.c:172:17: warning: assignment to 'struct device *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
172 | scm_dev = qcom_get_scm_device();
| ^
>> drivers/soc/qcom/mdt_loader.c:195:81: warning: passing argument 4 of 'dma_free_coherent' makes integer from pointer without a cast [-Wint-conversion]
195 | dma_free_coherent(scm_dev, ehdr_size + hash_size, data, mdata_phys);
| ^~~~~~~~~~
| |
| dma_addr_t * {aka unsigned int *}
In file included from drivers/soc/qcom/mdt_loader.c:19:
include/linux/dma-mapping.h:433:44: note: expected 'dma_addr_t' {aka 'unsigned int'} but argument is of type 'dma_addr_t *' {aka 'unsigned int *'}
433 | void *cpu_addr, dma_addr_t dma_handle)
| ~~~~~~~~~~~^~~~~~~~~~
cc1: some warnings being treated as errors
vim +/qcom_get_scm_device +172 drivers/soc/qcom/mdt_loader.c
107
108 /**
109 * qcom_mdt_read_metadata() - read header and metadata from mdt or mbn
110 * @fw: firmware of mdt header or mbn
111 * @data_len: length of the read metadata blob
112 * @fw_name: name of the firmware, for construction of segment file names
113 * @dev: device handle to associate resources with
114 * @mdata_phys: phys address for the assigned metadata buffer
115 *
116 * The mechanism that performs the authentication of the loading firmware
117 * expects an ELF header directly followed by the segment of hashes, with no
118 * padding inbetween. This function allocates a chunk of memory for this pair
119 * and copy the two pieces into the buffer.
120 *
121 * In the case of split firmware the hash is found directly following the ELF
122 * header, rather than at p_offset described by the second program header.
123 *
124 * The caller is responsible to free (kfree()) the returned pointer.
125 *
126 * Return: pointer to data, or ERR_PTR()
127 */
128 void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len,
129 const char *fw_name, struct device *dev,
130 dma_addr_t *mdata_phys)
131 {
132 const struct elf32_phdr *phdrs;
133 const struct elf32_hdr *ehdr;
134 unsigned int hash_segment = 0;
135 struct device *scm_dev = NULL;
136 size_t hash_offset;
137 size_t hash_size;
138 size_t ehdr_size;
139 unsigned int i;
140 ssize_t ret;
141 void *data;
142
143 ehdr = (struct elf32_hdr *)fw->data;
144 phdrs = (struct elf32_phdr *)(ehdr + 1);
145
146 if (ehdr->e_phnum < 2)
147 return ERR_PTR(-EINVAL);
148
149 if (phdrs[0].p_type == PT_LOAD)
150 return ERR_PTR(-EINVAL);
151
152 for (i = 1; i < ehdr->e_phnum; i++) {
153 if ((phdrs[i].p_flags & QCOM_MDT_TYPE_MASK) == QCOM_MDT_TYPE_HASH) {
154 hash_segment = i;
155 break;
156 }
157 }
158
159 if (!hash_segment) {
160 dev_err(dev, "no hash segment found in %s\n", fw_name);
161 return ERR_PTR(-EINVAL);
162 }
163
164 ehdr_size = phdrs[0].p_filesz;
165 hash_size = phdrs[hash_segment].p_filesz;
166
167 /*
168 * During the scm call memory protection will be enabled for the meta
169 * data blob, so make sure it's physically contiguous, 4K aligned and
170 * non-cachable to avoid XPU violations.
171 */
> 172 scm_dev = qcom_get_scm_device();
173 data = dma_alloc_coherent(scm_dev, ehdr_size + hash_size, mdata_phys,
174 GFP_KERNEL);
175 if (!data) {
176 dev_err(dev, "Allocation of metadata buffer failed.\n");
177 return NULL;
178 }
179
180 /* Copy ELF header */
181 memcpy(data, fw->data, ehdr_size);
182
183 if (ehdr_size + hash_size == fw->size) {
184 /* Firmware is split and hash is packed following the ELF header */
185 hash_offset = phdrs[0].p_filesz;
186 memcpy(data + ehdr_size, fw->data + hash_offset, hash_size);
187 } else if (phdrs[hash_segment].p_offset + hash_size <= fw->size) {
188 /* Hash is in its own segment, but within the loaded file */
189 hash_offset = phdrs[hash_segment].p_offset;
190 memcpy(data + ehdr_size, fw->data + hash_offset, hash_size);
191 } else {
192 /* Hash is in its own segment, beyond the loaded file */
193 ret = mdt_load_split_segment(data + ehdr_size, phdrs, hash_segment, fw_name, dev);
194 if (ret) {
> 195 dma_free_coherent(scm_dev, ehdr_size + hash_size, data, mdata_phys);
196 return ERR_PTR(ret);
197 }
198 }
199
200 *data_len = ehdr_size + hash_size;
201
202 return data;
203 }
204 EXPORT_SYMBOL_GPL(qcom_mdt_read_metadata);
205
--
0-DAY CI Kernel Test Service
https://01.org/lkp
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 <[email protected]>
> ---
> 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
>At the end of this function we invoke kfree(metadata), which would be bad if that comes from dma_alloc_coherent().
+ if (mdata_phys) {
+ data = dma_alloc_coherent(dev, ehdr_size + hash_size, mdata_phys,
+ GFP_KERNEL);
+ } else {
+ data = kmalloc(ehdr_size + hash_size, GFP_KERNEL);
Adding dma_alloc_coherent without affecting the mss driver.
> As LKP points out, I don't seem to have this function.
Removing the qcom_get_scm_device() and calling dma_alloc_coherent from device context.
+ data = dma_alloc_coherent(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.
With this patch we moving all the dma_alloc_coherent() and dma_free_coherent() to the MDT loader.
So now the MDT loader has the functionality of loading and allocating memory
and the SCM driver packs the arguments and makes a call to the secure world.
-----Original Message-----
From: Bjorn Andersson <[email protected]>
Sent: Tuesday, September 13, 2022 4:11 PM
To: Gokul krishna Krishnakumar (QUIC) <[email protected]>
Cc: Andy Gross <[email protected]>; Konrad Dybcio <[email protected]>; Philipp Zabel <[email protected]>; [email protected]; [email protected]; Trilok Soni (QUIC) <[email protected]>; Satya Durga Srinivasu Prabhala (QUIC) <[email protected]>; Rajendra Nayak (QUIC) <[email protected]>; Elliot Berman (QUIC) <[email protected]>; Guru Das Srinagesh (QUIC) <[email protected]>
Subject: Re: [PATCH v1 2/2] soc: qcom: mdt_loader: Move the memory allocation into mdt loader
WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.
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 <[email protected]>
> ---
> 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
Hi Bjorn,
With this patch we have moved the dma_alloc_coherent/dma_free_coherent
is called from the mdt loader and is operating in the context of the
caller, the scm device's struct device is not used in this patch. For
the clients which do not pass the metadata physical argument to the
qcom_mdt_read_metadata() - the memory is allocated using kmalloc- so the
clients like qcom_q6v5_mss.c, where kfree is called will not be broken
with this change.
Thanks,
Gokul
On 9/21/2022 12:39 PM, Gokul krishna Krishnakumar (QUIC) wrote:
>> At the end of this function we invoke kfree(metadata), which would be bad if that comes from dma_alloc_coherent().
> + if (mdata_phys) {
> + data = dma_alloc_coherent(dev, ehdr_size + hash_size, mdata_phys,
> + GFP_KERNEL);
> + } else {
> + data = kmalloc(ehdr_size + hash_size, GFP_KERNEL);
> Adding dma_alloc_coherent without affecting the mss driver.
>
>
>> As LKP points out, I don't seem to have this function.
> Removing the qcom_get_scm_device() and calling dma_alloc_coherent from device context.
> + data = dma_alloc_coherent(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.
>
> With this patch we moving all the dma_alloc_coherent() and dma_free_coherent() to the MDT loader.
> So now the MDT loader has the functionality of loading and allocating memory
> and the SCM driver packs the arguments and makes a call to the secure world.
>
> -----Original Message-----
> From: Bjorn Andersson <[email protected]>
> Sent: Tuesday, September 13, 2022 4:11 PM
> To: Gokul krishna Krishnakumar (QUIC) <[email protected]>
> Cc: Andy Gross <[email protected]>; Konrad Dybcio <[email protected]>; Philipp Zabel <[email protected]>; [email protected]; [email protected]; Trilok Soni (QUIC) <[email protected]>; Satya Durga Srinivasu Prabhala (QUIC) <[email protected]>; Rajendra Nayak (QUIC) <[email protected]>; Elliot Berman (QUIC) <[email protected]>; Guru Das Srinagesh (QUIC) <[email protected]>
> Subject: Re: [PATCH v1 2/2] soc: qcom: mdt_loader: Move the memory allocation into mdt loader
>
> WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.
>
> 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 <[email protected]>
>> ---
>> 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