[PATCH 0/3] soc: qcom: mdt_loader: General improvements

[PATCH 0/3] soc: qcom: mdt_loader: General improvements

[Siddharth Gupta]

This series of patches improves general functionality for the mdt loader.

Patch 1 adds the ability to dynamically detect hash segment location.
Patch 2 updates the logic used to identify whether the firmware is split or not.
Patch 3 updates the way the metadata is read and generated.

Siddharth Gupta (3):
  soc: qcom: mdt_loader: Allow hash at any phdr
  soc: qcom: mdt_loader: Handle split bins correctly
  soc: qcom: mdt_loader: Read hash from firmware blob

 drivers/remoteproc/qcom_q6v5_mss.c  |   4 +-
 drivers/soc/qcom/mdt_loader.c       | 110 ++++++++++++++++++++++++------------
 include/linux/soc/qcom/mdt_loader.h |   3 +-
 3 files changed, 79 insertions(+), 38 deletions(-)

-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 1/3] soc: qcom: mdt_loader: Allow hash at any phdr

[Siddharth Gupta]

The assumption that the elf program header will always have the hash
segment program header at index 1 may not hold true in all cases. This
change updates the read metadata function to find the hash program header
dynamically.

Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
---
 drivers/soc/qcom/mdt_loader.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
index 24cd193..813216d 100644
--- a/drivers/soc/qcom/mdt_loader.c
+++ b/drivers/soc/qcom/mdt_loader.c
@@ -4,7 +4,7 @@
  *
  * Copyright (C) 2016 Linaro Ltd
  * Copyright (C) 2015 Sony Mobile Communications Inc
- * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2012-2013, 2020 The Linux Foundation. All rights reserved.
  */
 
 #include <linux/device.h>
@@ -88,6 +88,7 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
 	const struct elf32_phdr *phdrs;
 	const struct elf32_hdr *ehdr;
 	size_t hash_offset;
+	size_t hash_index;
 	size_t hash_size;
 	size_t ehdr_size;
 	void *data;
@@ -98,14 +99,19 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
 	if (ehdr->e_phnum < 2)
 		return ERR_PTR(-EINVAL);
 
-	if (phdrs[0].p_type == PT_LOAD || phdrs[1].p_type == PT_LOAD)
+	if (phdrs[0].p_type == PT_LOAD)
 		return ERR_PTR(-EINVAL);
 
-	if ((phdrs[1].p_flags & QCOM_MDT_TYPE_MASK) != QCOM_MDT_TYPE_HASH)
+	for (hash_index = 1; hash_index < ehdr->e_phnum; hash_index++) {
+		if (phdrs[hash_index].p_type != PT_LOAD &&
+		   (phdrs[hash_index].p_flags & QCOM_MDT_TYPE_MASK) == QCOM_MDT_TYPE_HASH)
+			break;
+	}
+	if (hash_index >= ehdr->e_phnum)
 		return ERR_PTR(-EINVAL);
 
 	ehdr_size = phdrs[0].p_filesz;
-	hash_size = phdrs[1].p_filesz;
+	hash_size = phdrs[hash_index].p_filesz;
 
 	data = kmalloc(ehdr_size + hash_size, GFP_KERNEL);
 	if (!data)
@@ -115,7 +121,7 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
 	if (ehdr_size + hash_size == fw->size)
 		hash_offset = phdrs[0].p_filesz;
 	else
-		hash_offset = phdrs[1].p_offset;
+		hash_offset = phdrs[hash_index].p_offset;
 
 	memcpy(data, fw->data, ehdr_size);
 	memcpy(data + ehdr_size, fw->data + hash_offset, hash_size);
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/3] soc: qcom: mdt_loader: Handle split bins correctly

[Siddharth Gupta]

It may be that the offset of the first program header lies inside the mdt's
filesize, in this case the loader would incorrectly assume that the bins
were not split. The loading would then continue on to fail for split bins.
This change updates the logic used by the mdt loader to understand whether
the firmware images are split or not. It figures this out by checking if
each program header's segment lies within the file or not.

Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
---
 drivers/soc/qcom/mdt_loader.c | 60 +++++++++++++++++++++++++++----------------
 1 file changed, 38 insertions(+), 22 deletions(-)

diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
index 813216d..c9bbd8c 100644
--- a/drivers/soc/qcom/mdt_loader.c
+++ b/drivers/soc/qcom/mdt_loader.c
@@ -31,6 +31,26 @@ static bool mdt_phdr_valid(const struct elf32_phdr *phdr)
 	return true;
 }
 
+static bool qcom_mdt_bins_are_split(const struct firmware *fw)
+{
+	const struct elf32_phdr *phdrs;
+	const struct elf32_hdr *ehdr;
+	uint64_t seg_start, seg_end;
+	int i;
+
+	ehdr = (struct elf32_hdr *)fw->data;
+	phdrs = (struct elf32_phdr *)(ehdr + 1);
+
+	for (i = 0; i < ehdr->e_phnum; i++) {
+		seg_start = phdrs[i].p_offset;
+		seg_end = phdrs[i].p_offset + phdrs[i].p_filesz;
+		if (seg_start > fw->size || seg_end > fw->size)
+			return true;
+	}
+
+	return false;
+}
+
 /**
  * qcom_mdt_get_size() - acquire size of the memory region needed to load mdt
  * @fw:		firmware object for the mdt file
@@ -118,7 +138,7 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
 		return ERR_PTR(-ENOMEM);
 
 	/* Is the header and hash already packed */
-	if (ehdr_size + hash_size == fw->size)
+	if (qcom_mdt_bins_are_split(fw))
 		hash_offset = phdrs[0].p_filesz;
 	else
 		hash_offset = phdrs[hash_index].p_offset;
@@ -150,6 +170,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
 	void *metadata;
 	char *fw_name;
 	bool relocate = false;
+	bool is_split;
 	void *ptr;
 	int ret = 0;
 	int i;
@@ -157,6 +178,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
 	if (!fw || !mem_region || !mem_phys || !mem_size)
 		return -EINVAL;
 
+	is_split = qcom_mdt_bins_are_split(fw);
 	ehdr = (struct elf32_hdr *)fw->data;
 	phdrs = (struct elf32_phdr *)(ehdr + 1);
 
@@ -238,28 +260,22 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
 
 		ptr = mem_region + offset;
 
-		if (phdr->p_filesz && phdr->p_offset < fw->size) {
-			/* Firmware is large enough to be non-split */
-			if (phdr->p_offset + phdr->p_filesz > fw->size) {
-				dev_err(dev,
-					"failed to load segment %d from truncated file %s\n",
-					i, firmware);
-				ret = -EINVAL;
-				break;
+		if (phdr->p_filesz) {
+			if (!is_split) {
+				/* Firmware is large enough to be non-split */
+				memcpy(ptr, fw->data + phdr->p_offset, phdr->p_filesz);
+			} else {
+				/* Firmware not large enough, load split-out segments */
+				snprintf(fw_name + fw_name_len - 3, 4, "b%02d", i);
+				ret = request_firmware_into_buf(&seg_fw, fw_name, dev,
+								ptr, phdr->p_filesz);
+				if (ret) {
+					dev_err(dev, "failed to load %s\n", fw_name);
+					break;
+				}
+
+				release_firmware(seg_fw);
 			}
-
-			memcpy(ptr, fw->data + phdr->p_offset, phdr->p_filesz);
-		} else if (phdr->p_filesz) {
-			/* Firmware not large enough, load split-out segments */
-			sprintf(fw_name + fw_name_len - 3, "b%02d", i);
-			ret = request_firmware_into_buf(&seg_fw, fw_name, dev,
-							ptr, phdr->p_filesz);
-			if (ret) {
-				dev_err(dev, "failed to load %s\n", fw_name);
-				break;
-			}
-
-			release_firmware(seg_fw);
 		}
 
 		if (phdr->p_memsz > phdr->p_filesz)
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob

[Siddharth Gupta]

Since the split elf blobs will always contain the hash segment, we rely on
the blob file to get the hash rather than assume that it will be present in
the mdt file. This change uses the hash index to read the appropriate elf
blob to get the hash segment.

Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
---
 drivers/remoteproc/qcom_q6v5_mss.c  |  4 ++--
 drivers/soc/qcom/mdt_loader.c       | 38 +++++++++++++++++++++++++++----------
 include/linux/soc/qcom/mdt_loader.h |  3 ++-
 3 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index 66106ba..74c0229 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -4,7 +4,7 @@
  *
  * Copyright (C) 2016 Linaro Ltd.
  * Copyright (C) 2014 Sony Mobile Communications AB
- * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2012-2013, 2020 The Linux Foundation. All rights reserved.
  */
 
 #include <linux/clk.h>
@@ -828,7 +828,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
 	void *ptr;
 	int ret;
 
-	metadata = qcom_mdt_read_metadata(fw, &size);
+	metadata = qcom_mdt_read_metadata(qproc->dev, fw, qproc->hexagon_mdt_image, &size);
 	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 c9bbd8c..6876c0b 100644
--- a/drivers/soc/qcom/mdt_loader.c
+++ b/drivers/soc/qcom/mdt_loader.c
@@ -103,15 +103,18 @@ 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)
+void *qcom_mdt_read_metadata(struct device *dev, const struct firmware *fw, const char *firmware,
+			     size_t *data_len)
 {
 	const struct elf32_phdr *phdrs;
 	const struct elf32_hdr *ehdr;
-	size_t hash_offset;
+	const struct firmware *seg_fw;
 	size_t hash_index;
 	size_t hash_size;
 	size_t ehdr_size;
+	char *fw_name;
 	void *data;
+	int ret;
 
 	ehdr = (struct elf32_hdr *)fw->data;
 	phdrs = (struct elf32_phdr *)(ehdr + 1);
@@ -137,14 +140,29 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
 	if (!data)
 		return ERR_PTR(-ENOMEM);
 
-	/* Is the header and hash already packed */
-	if (qcom_mdt_bins_are_split(fw))
-		hash_offset = phdrs[0].p_filesz;
-	else
-		hash_offset = phdrs[hash_index].p_offset;
-
+	/* copy elf header */
 	memcpy(data, fw->data, ehdr_size);
-	memcpy(data + ehdr_size, fw->data + hash_offset, hash_size);
+
+	if (qcom_mdt_bins_are_split(fw)) {
+		fw_name = kstrdup(firmware, GFP_KERNEL);
+		if (!fw_name) {
+			kfree(data);
+			return ERR_PTR(-ENOMEM);
+		}
+		snprintf(fw_name + strlen(fw_name) - 3, 4, "b%02d", hash_index);
+
+		ret = request_firmware_into_buf(&seg_fw, fw_name, dev, data + ehdr_size, hash_size);
+		kfree(fw_name);
+
+		if (ret) {
+			kfree(data);
+			return ERR_PTR(ret);
+		}
+
+		release_firmware(seg_fw);
+	} else {
+		memcpy(data + ehdr_size, fw->data + phdrs[hash_index].p_offset, hash_size);
+	}
 
 	*data_len = ehdr_size + hash_size;
 
@@ -191,7 +209,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
 		return -ENOMEM;
 
 	if (pas_init) {
-		metadata = qcom_mdt_read_metadata(fw, &metadata_len);
+		metadata = qcom_mdt_read_metadata(dev, fw, firmware, &metadata_len);
 		if (IS_ERR(metadata)) {
 			ret = PTR_ERR(metadata);
 			goto out;
diff --git a/include/linux/soc/qcom/mdt_loader.h b/include/linux/soc/qcom/mdt_loader.h
index e600bae..04ba5e8 100644
--- a/include/linux/soc/qcom/mdt_loader.h
+++ b/include/linux/soc/qcom/mdt_loader.h
@@ -21,6 +21,7 @@ int qcom_mdt_load_no_init(struct device *dev, const struct firmware *fw,
 			  const char *fw_name, int pas_id, void *mem_region,
 			  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);
+void *qcom_mdt_read_metadata(struct device *dev, const struct firmware *fw, const char *firmware,
+			     size_t *data_len);
 
 #endif
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5160 bytes --]

Hi Siddharth,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.11-rc2 next-20210104]
[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]

url:    https://github.com/0day-ci/linux/commits/Siddharth-Gupta/soc-qcom-mdt_loader-General-improvements/20210107-052704
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git e71ba9452f0b5b2e8dc8aa5445198cd9214a6a62
config: arm64-allyesconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.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/0day-ci/linux/commit/ecd3796eaac75caf4abf7386c0c82546c104511a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Siddharth-Gupta/soc-qcom-mdt_loader-General-improvements/20210107-052704
        git checkout ecd3796eaac75caf4abf7386c0c82546c104511a
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/soc/qcom/mdt_loader.c: In function 'qcom_mdt_read_metadata':
>> drivers/soc/qcom/mdt_loader.c:152:51: warning: format '%d' expects argument of type 'int', but argument 4 has type 'size_t' {aka 'long unsigned int'} [-Wformat=]
     152 |   snprintf(fw_name + strlen(fw_name) - 3, 4, "b%02d", hash_index);
         |                                                ~~~^   ~~~~~~~~~~
         |                                                   |   |
         |                                                   int size_t {aka long unsigned int}
         |                                                %02ld


vim +152 drivers/soc/qcom/mdt_loader.c

    88	
    89	/**
    90	 * qcom_mdt_read_metadata() - read header and metadata from mdt or mbn
    91	 * @fw:		firmware of mdt header or mbn
    92	 * @data_len:	length of the read metadata blob
    93	 *
    94	 * The mechanism that performs the authentication of the loading firmware
    95	 * expects an ELF header directly followed by the segment of hashes, with no
    96	 * padding inbetween. This function allocates a chunk of memory for this pair
    97	 * and copy the two pieces into the buffer.
    98	 *
    99	 * In the case of split firmware the hash is found directly following the ELF
   100	 * header, rather than at p_offset described by the second program header.
   101	 *
   102	 * The caller is responsible to free (kfree()) the returned pointer.
   103	 *
   104	 * Return: pointer to data, or ERR_PTR()
   105	 */
   106	void *qcom_mdt_read_metadata(struct device *dev, const struct firmware *fw, const char *firmware,
   107				     size_t *data_len)
   108	{
   109		const struct elf32_phdr *phdrs;
   110		const struct elf32_hdr *ehdr;
   111		const struct firmware *seg_fw;
   112		size_t hash_index;
   113		size_t hash_size;
   114		size_t ehdr_size;
   115		char *fw_name;
   116		void *data;
   117		int ret;
   118	
   119		ehdr = (struct elf32_hdr *)fw->data;
   120		phdrs = (struct elf32_phdr *)(ehdr + 1);
   121	
   122		if (ehdr->e_phnum < 2)
   123			return ERR_PTR(-EINVAL);
   124	
   125		if (phdrs[0].p_type == PT_LOAD)
   126			return ERR_PTR(-EINVAL);
   127	
   128		for (hash_index = 1; hash_index < ehdr->e_phnum; hash_index++) {
   129			if (phdrs[hash_index].p_type != PT_LOAD &&
   130			   (phdrs[hash_index].p_flags & QCOM_MDT_TYPE_MASK) == QCOM_MDT_TYPE_HASH)
   131				break;
   132		}
   133		if (hash_index >= ehdr->e_phnum)
   134			return ERR_PTR(-EINVAL);
   135	
   136		ehdr_size = phdrs[0].p_filesz;
   137		hash_size = phdrs[hash_index].p_filesz;
   138	
   139		data = kmalloc(ehdr_size + hash_size, GFP_KERNEL);
   140		if (!data)
   141			return ERR_PTR(-ENOMEM);
   142	
   143		/* copy elf header */
   144		memcpy(data, fw->data, ehdr_size);
   145	
   146		if (qcom_mdt_bins_are_split(fw)) {
   147			fw_name = kstrdup(firmware, GFP_KERNEL);
   148			if (!fw_name) {
   149				kfree(data);
   150				return ERR_PTR(-ENOMEM);
   151			}
 > 152			snprintf(fw_name + strlen(fw_name) - 3, 4, "b%02d", hash_index);
   153	
   154			ret = request_firmware_into_buf(&seg_fw, fw_name, dev, data + ehdr_size, hash_size);
   155			kfree(fw_name);
   156	
   157			if (ret) {
   158				kfree(data);
   159				return ERR_PTR(ret);
   160			}
   161	
   162			release_firmware(seg_fw);
   163		} else {
   164			memcpy(data + ehdr_size, fw->data + phdrs[hash_index].p_offset, hash_size);
   165		}
   166	
   167		*data_len = ehdr_size + hash_size;
   168	
   169		return data;
   170	}
   171	EXPORT_SYMBOL_GPL(qcom_mdt_read_metadata);
   172	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 76477 bytes --]

Re: [PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5291 bytes --]

Hi Siddharth,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.11-rc2 next-20210104]
[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]

url:    https://github.com/0day-ci/linux/commits/Siddharth-Gupta/soc-qcom-mdt_loader-General-improvements/20210107-052704
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git e71ba9452f0b5b2e8dc8aa5445198cd9214a6a62
config: arm64-allyesconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.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/0day-ci/linux/commit/ecd3796eaac75caf4abf7386c0c82546c104511a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Siddharth-Gupta/soc-qcom-mdt_loader-General-improvements/20210107-052704
        git checkout ecd3796eaac75caf4abf7386c0c82546c104511a
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/soc/qcom/mdt_loader.c: In function 'qcom_mdt_read_metadata':
>> drivers/soc/qcom/mdt_loader.c:152:51: warning: format '%d' expects argument of type 'int', but argument 4 has type 'size_t' {aka 'long unsigned int'} [-Wformat=]
     152 |   snprintf(fw_name + strlen(fw_name) - 3, 4, "b%02d", hash_index);
         |                                                ~~~^   ~~~~~~~~~~
         |                                                   |   |
         |                                                   int size_t {aka long unsigned int}
         |                                                %02ld


vim +152 drivers/soc/qcom/mdt_loader.c

    88	
    89	/**
    90	 * qcom_mdt_read_metadata() - read header and metadata from mdt or mbn
    91	 * @fw:		firmware of mdt header or mbn
    92	 * @data_len:	length of the read metadata blob
    93	 *
    94	 * The mechanism that performs the authentication of the loading firmware
    95	 * expects an ELF header directly followed by the segment of hashes, with no
    96	 * padding inbetween. This function allocates a chunk of memory for this pair
    97	 * and copy the two pieces into the buffer.
    98	 *
    99	 * In the case of split firmware the hash is found directly following the ELF
   100	 * header, rather than at p_offset described by the second program header.
   101	 *
   102	 * The caller is responsible to free (kfree()) the returned pointer.
   103	 *
   104	 * Return: pointer to data, or ERR_PTR()
   105	 */
   106	void *qcom_mdt_read_metadata(struct device *dev, const struct firmware *fw, const char *firmware,
   107				     size_t *data_len)
   108	{
   109		const struct elf32_phdr *phdrs;
   110		const struct elf32_hdr *ehdr;
   111		const struct firmware *seg_fw;
   112		size_t hash_index;
   113		size_t hash_size;
   114		size_t ehdr_size;
   115		char *fw_name;
   116		void *data;
   117		int ret;
   118	
   119		ehdr = (struct elf32_hdr *)fw->data;
   120		phdrs = (struct elf32_phdr *)(ehdr + 1);
   121	
   122		if (ehdr->e_phnum < 2)
   123			return ERR_PTR(-EINVAL);
   124	
   125		if (phdrs[0].p_type == PT_LOAD)
   126			return ERR_PTR(-EINVAL);
   127	
   128		for (hash_index = 1; hash_index < ehdr->e_phnum; hash_index++) {
   129			if (phdrs[hash_index].p_type != PT_LOAD &&
   130			   (phdrs[hash_index].p_flags & QCOM_MDT_TYPE_MASK) == QCOM_MDT_TYPE_HASH)
   131				break;
   132		}
   133		if (hash_index >= ehdr->e_phnum)
   134			return ERR_PTR(-EINVAL);
   135	
   136		ehdr_size = phdrs[0].p_filesz;
   137		hash_size = phdrs[hash_index].p_filesz;
   138	
   139		data = kmalloc(ehdr_size + hash_size, GFP_KERNEL);
   140		if (!data)
   141			return ERR_PTR(-ENOMEM);
   142	
   143		/* copy elf header */
   144		memcpy(data, fw->data, ehdr_size);
   145	
   146		if (qcom_mdt_bins_are_split(fw)) {
   147			fw_name = kstrdup(firmware, GFP_KERNEL);
   148			if (!fw_name) {
   149				kfree(data);
   150				return ERR_PTR(-ENOMEM);
   151			}
 > 152			snprintf(fw_name + strlen(fw_name) - 3, 4, "b%02d", hash_index);
   153	
   154			ret = request_firmware_into_buf(&seg_fw, fw_name, dev, data + ehdr_size, hash_size);
   155			kfree(fw_name);
   156	
   157			if (ret) {
   158				kfree(data);
   159				return ERR_PTR(ret);
   160			}
   161	
   162			release_firmware(seg_fw);
   163		} else {
   164			memcpy(data + ehdr_size, fw->data + phdrs[hash_index].p_offset, hash_size);
   165		}
   166	
   167		*data_len = ehdr_size + hash_size;
   168	
   169		return data;
   170	}
   171	EXPORT_SYMBOL_GPL(qcom_mdt_read_metadata);
   172	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 76477 bytes --]

Re: [PATCH 1/3] soc: qcom: mdt_loader: Allow hash at any phdr

[Bjorn Andersson]

On Wed 06 Jan 15:23 CST 2021, Siddharth Gupta wrote:

> The assumption that the elf program header will always have the hash
> segment program header at index 1 may not hold true in all cases. This
> change updates the read metadata function to find the hash program header
> dynamically.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> ---
>  drivers/soc/qcom/mdt_loader.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> index 24cd193..813216d 100644
> --- a/drivers/soc/qcom/mdt_loader.c
> +++ b/drivers/soc/qcom/mdt_loader.c
> @@ -4,7 +4,7 @@
>   *
>   * Copyright (C) 2016 Linaro Ltd
>   * Copyright (C) 2015 Sony Mobile Communications Inc
> - * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2012-2013, 2020 The Linux Foundation. All rights reserved.
>   */
>  
>  #include <linux/device.h>
> @@ -88,6 +88,7 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
>  	const struct elf32_phdr *phdrs;
>  	const struct elf32_hdr *ehdr;
>  	size_t hash_offset;
> +	size_t hash_index;
>  	size_t hash_size;
>  	size_t ehdr_size;
>  	void *data;
> @@ -98,14 +99,19 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
>  	if (ehdr->e_phnum < 2)
>  		return ERR_PTR(-EINVAL);
>  
> -	if (phdrs[0].p_type == PT_LOAD || phdrs[1].p_type == PT_LOAD)
> +	if (phdrs[0].p_type == PT_LOAD)
>  		return ERR_PTR(-EINVAL);
>  
> -	if ((phdrs[1].p_flags & QCOM_MDT_TYPE_MASK) != QCOM_MDT_TYPE_HASH)
> +	for (hash_index = 1; hash_index < ehdr->e_phnum; hash_index++) {
> +		if (phdrs[hash_index].p_type != PT_LOAD &&
> +		   (phdrs[hash_index].p_flags & QCOM_MDT_TYPE_MASK) == QCOM_MDT_TYPE_HASH)
> +			break;
> +	}
> +	if (hash_index >= ehdr->e_phnum)
>  		return ERR_PTR(-EINVAL);
>  
>  	ehdr_size = phdrs[0].p_filesz;
> -	hash_size = phdrs[1].p_filesz;
> +	hash_size = phdrs[hash_index].p_filesz;
>  
>  	data = kmalloc(ehdr_size + hash_size, GFP_KERNEL);
>  	if (!data)
> @@ -115,7 +121,7 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
>  	if (ehdr_size + hash_size == fw->size)
>  		hash_offset = phdrs[0].p_filesz;
>  	else
> -		hash_offset = phdrs[1].p_offset;
> +		hash_offset = phdrs[hash_index].p_offset;
>  
>  	memcpy(data, fw->data, ehdr_size);
>  	memcpy(data + ehdr_size, fw->data + hash_offset, hash_size);
> -- 
> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH 2/3] soc: qcom: mdt_loader: Handle split bins correctly

[Bjorn Andersson]

On Wed 06 Jan 15:23 CST 2021, Siddharth Gupta wrote:

> It may be that the offset of the first program header lies inside the mdt's
> filesize, in this case the loader would incorrectly assume that the bins
> were not split. The loading would then continue on to fail for split bins.
> This change updates the logic used by the mdt loader to understand whether
> the firmware images are split or not. It figures this out by checking if
> each program header's segment lies within the file or not.
> 
> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> ---
>  drivers/soc/qcom/mdt_loader.c | 60 +++++++++++++++++++++++++++----------------
>  1 file changed, 38 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> index 813216d..c9bbd8c 100644
> --- a/drivers/soc/qcom/mdt_loader.c
> +++ b/drivers/soc/qcom/mdt_loader.c
> @@ -31,6 +31,26 @@ static bool mdt_phdr_valid(const struct elf32_phdr *phdr)
>  	return true;
>  }
>  
> +static bool qcom_mdt_bins_are_split(const struct firmware *fw)
> +{
> +	const struct elf32_phdr *phdrs;
> +	const struct elf32_hdr *ehdr;
> +	uint64_t seg_start, seg_end;
> +	int i;
> +
> +	ehdr = (struct elf32_hdr *)fw->data;
> +	phdrs = (struct elf32_phdr *)(ehdr + 1);
> +
> +	for (i = 0; i < ehdr->e_phnum; i++) {
> +		seg_start = phdrs[i].p_offset;
> +		seg_end = phdrs[i].p_offset + phdrs[i].p_filesz;
> +		if (seg_start > fw->size || seg_end > fw->size)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>  /**
>   * qcom_mdt_get_size() - acquire size of the memory region needed to load mdt
>   * @fw:		firmware object for the mdt file
> @@ -118,7 +138,7 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
>  		return ERR_PTR(-ENOMEM);
>  
>  	/* Is the header and hash already packed */
> -	if (ehdr_size + hash_size == fw->size)
> +	if (qcom_mdt_bins_are_split(fw))
>  		hash_offset = phdrs[0].p_filesz;
>  	else
>  		hash_offset = phdrs[hash_index].p_offset;
> @@ -150,6 +170,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
>  	void *metadata;
>  	char *fw_name;
>  	bool relocate = false;
> +	bool is_split;
>  	void *ptr;
>  	int ret = 0;
>  	int i;
> @@ -157,6 +178,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
>  	if (!fw || !mem_region || !mem_phys || !mem_size)
>  		return -EINVAL;
>  
> +	is_split = qcom_mdt_bins_are_split(fw);
>  	ehdr = (struct elf32_hdr *)fw->data;
>  	phdrs = (struct elf32_phdr *)(ehdr + 1);
>  
> @@ -238,28 +260,22 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
>  
>  		ptr = mem_region + offset;
>  
> -		if (phdr->p_filesz && phdr->p_offset < fw->size) {
> -			/* Firmware is large enough to be non-split */
> -			if (phdr->p_offset + phdr->p_filesz > fw->size) {
> -				dev_err(dev,
> -					"failed to load segment %d from truncated file %s\n",
> -					i, firmware);
> -				ret = -EINVAL;
> -				break;
> +		if (phdr->p_filesz) {
> +			if (!is_split) {

In an effort to reduce the diff size and avoid adding another level of
indentation, how about making the conditionals:

		if (is_split && phdr->p_filesz) {
			memcpy();
		} else if (phdr->p_filesz) {
			...
		}

Apart from that I think this patch looks good!

Regards,
Bjorn

> +				/* Firmware is large enough to be non-split */
> +				memcpy(ptr, fw->data + phdr->p_offset, phdr->p_filesz);
> +			} else {
> +				/* Firmware not large enough, load split-out segments */
> +				snprintf(fw_name + fw_name_len - 3, 4, "b%02d", i);
> +				ret = request_firmware_into_buf(&seg_fw, fw_name, dev,
> +								ptr, phdr->p_filesz);
> +				if (ret) {
> +					dev_err(dev, "failed to load %s\n", fw_name);
> +					break;
> +				}
> +
> +				release_firmware(seg_fw);
>  			}
> -
> -			memcpy(ptr, fw->data + phdr->p_offset, phdr->p_filesz);
> -		} else if (phdr->p_filesz) {
> -			/* Firmware not large enough, load split-out segments */
> -			sprintf(fw_name + fw_name_len - 3, "b%02d", i);
> -			ret = request_firmware_into_buf(&seg_fw, fw_name, dev,
> -							ptr, phdr->p_filesz);
> -			if (ret) {
> -				dev_err(dev, "failed to load %s\n", fw_name);
> -				break;
> -			}
> -
> -			release_firmware(seg_fw);
>  		}
>  
>  		if (phdr->p_memsz > phdr->p_filesz)
> -- 
> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob

[Bjorn Andersson]

On Wed 06 Jan 15:23 CST 2021, Siddharth Gupta wrote:

> Since the split elf blobs will always contain the hash segment, we rely on

I think it will sounds better if we add "should" in "we should rely on..."

> the blob file to get the hash rather than assume that it will be present in
> the mdt file. This change uses the hash index to read the appropriate elf
> blob to get the hash segment.
> 
> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_mss.c  |  4 ++--
>  drivers/soc/qcom/mdt_loader.c       | 38 +++++++++++++++++++++++++++----------
>  include/linux/soc/qcom/mdt_loader.h |  3 ++-
>  3 files changed, 32 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index 66106ba..74c0229 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -4,7 +4,7 @@
>   *
>   * Copyright (C) 2016 Linaro Ltd.
>   * Copyright (C) 2014 Sony Mobile Communications AB
> - * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2012-2013, 2020 The Linux Foundation. All rights reserved.
>   */
>  
>  #include <linux/clk.h>
> @@ -828,7 +828,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
>  	void *ptr;
>  	int ret;
>  
> -	metadata = qcom_mdt_read_metadata(fw, &size);
> +	metadata = qcom_mdt_read_metadata(qproc->dev, fw, qproc->hexagon_mdt_image, &size);
>  	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 c9bbd8c..6876c0b 100644
> --- a/drivers/soc/qcom/mdt_loader.c
> +++ b/drivers/soc/qcom/mdt_loader.c
> @@ -103,15 +103,18 @@ 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)
> +void *qcom_mdt_read_metadata(struct device *dev, const struct firmware *fw, const char *firmware,
> +			     size_t *data_len)
>  {
>  	const struct elf32_phdr *phdrs;
>  	const struct elf32_hdr *ehdr;
> -	size_t hash_offset;
> +	const struct firmware *seg_fw;
>  	size_t hash_index;
>  	size_t hash_size;
>  	size_t ehdr_size;
> +	char *fw_name;
>  	void *data;
> +	int ret;
>  
>  	ehdr = (struct elf32_hdr *)fw->data;
>  	phdrs = (struct elf32_phdr *)(ehdr + 1);
> @@ -137,14 +140,29 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
>  	if (!data)
>  		return ERR_PTR(-ENOMEM);
>  
> -	/* Is the header and hash already packed */
> -	if (qcom_mdt_bins_are_split(fw))
> -		hash_offset = phdrs[0].p_filesz;
> -	else
> -		hash_offset = phdrs[hash_index].p_offset;
> -
> +	/* copy elf header */
>  	memcpy(data, fw->data, ehdr_size);
> -	memcpy(data + ehdr_size, fw->data + hash_offset, hash_size);
> +

This seems to duplicates parts of the loop in __qcom_mdt_load(), how
about breaking this out to a separate

static int mdt_load_segment(struct device *dev, const struct firmware *fw,
			    int idx, void *buf, size_t len, bool is_split)

Which either just memcpy from @fw or does the filename and loading
dance, based on @is_split?

Regards,
Bjorn

> +	if (qcom_mdt_bins_are_split(fw)) {
> +		fw_name = kstrdup(firmware, GFP_KERNEL);
> +		if (!fw_name) {
> +			kfree(data);
> +			return ERR_PTR(-ENOMEM);
> +		}
> +		snprintf(fw_name + strlen(fw_name) - 3, 4, "b%02d", hash_index);
> +
> +		ret = request_firmware_into_buf(&seg_fw, fw_name, dev, data + ehdr_size, hash_size);
> +		kfree(fw_name);
> +
> +		if (ret) {
> +			kfree(data);
> +			return ERR_PTR(ret);
> +		}
> +
> +		release_firmware(seg_fw);
> +	} else {
> +		memcpy(data + ehdr_size, fw->data + phdrs[hash_index].p_offset, hash_size);
> +	}
>  
>  	*data_len = ehdr_size + hash_size;
>  
> @@ -191,7 +209,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
>  		return -ENOMEM;
>  
>  	if (pas_init) {
> -		metadata = qcom_mdt_read_metadata(fw, &metadata_len);
> +		metadata = qcom_mdt_read_metadata(dev, fw, firmware, &metadata_len);
>  		if (IS_ERR(metadata)) {
>  			ret = PTR_ERR(metadata);
>  			goto out;
> diff --git a/include/linux/soc/qcom/mdt_loader.h b/include/linux/soc/qcom/mdt_loader.h
> index e600bae..04ba5e8 100644
> --- a/include/linux/soc/qcom/mdt_loader.h
> +++ b/include/linux/soc/qcom/mdt_loader.h
> @@ -21,6 +21,7 @@ int qcom_mdt_load_no_init(struct device *dev, const struct firmware *fw,
>  			  const char *fw_name, int pas_id, void *mem_region,
>  			  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);
> +void *qcom_mdt_read_metadata(struct device *dev, const struct firmware *fw, const char *firmware,
> +			     size_t *data_len);
>  
>  #endif
> -- 
> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob

[Siddharth Gupta]

On 1/7/2021 4:21 PM, Bjorn Andersson wrote:
> On Wed 06 Jan 15:23 CST 2021, Siddharth Gupta wrote:
>
>> Since the split elf blobs will always contain the hash segment, we rely on
> I think it will sounds better if we add "should" in "we should rely on..."
Sure
>
>> the blob file to get the hash rather than assume that it will be present in
>> the mdt file. This change uses the hash index to read the appropriate elf
>> blob to get the hash segment.
>>
>> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
>> ---
>>   drivers/remoteproc/qcom_q6v5_mss.c  |  4 ++--
>>   drivers/soc/qcom/mdt_loader.c       | 38 +++++++++++++++++++++++++++----------
>>   include/linux/soc/qcom/mdt_loader.h |  3 ++-
>>   3 files changed, 32 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
>> index 66106ba..74c0229 100644
>> --- a/drivers/remoteproc/qcom_q6v5_mss.c
>> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
>> @@ -4,7 +4,7 @@
>>    *
>>    * Copyright (C) 2016 Linaro Ltd.
>>    * Copyright (C) 2014 Sony Mobile Communications AB
>> - * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2012-2013, 2020 The Linux Foundation. All rights reserved.
>>    */
>>   
>>   #include <linux/clk.h>
>> @@ -828,7 +828,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
>>   	void *ptr;
>>   	int ret;
>>   
>> -	metadata = qcom_mdt_read_metadata(fw, &size);
>> +	metadata = qcom_mdt_read_metadata(qproc->dev, fw, qproc->hexagon_mdt_image, &size);
>>   	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 c9bbd8c..6876c0b 100644
>> --- a/drivers/soc/qcom/mdt_loader.c
>> +++ b/drivers/soc/qcom/mdt_loader.c
>> @@ -103,15 +103,18 @@ 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)
>> +void *qcom_mdt_read_metadata(struct device *dev, const struct firmware *fw, const char *firmware,
>> +			     size_t *data_len)
>>   {
>>   	const struct elf32_phdr *phdrs;
>>   	const struct elf32_hdr *ehdr;
>> -	size_t hash_offset;
>> +	const struct firmware *seg_fw;
>>   	size_t hash_index;
>>   	size_t hash_size;
>>   	size_t ehdr_size;
>> +	char *fw_name;
>>   	void *data;
>> +	int ret;
>>   
>>   	ehdr = (struct elf32_hdr *)fw->data;
>>   	phdrs = (struct elf32_phdr *)(ehdr + 1);
>> @@ -137,14 +140,29 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
>>   	if (!data)
>>   		return ERR_PTR(-ENOMEM);
>>   
>> -	/* Is the header and hash already packed */
>> -	if (qcom_mdt_bins_are_split(fw))
>> -		hash_offset = phdrs[0].p_filesz;
>> -	else
>> -		hash_offset = phdrs[hash_index].p_offset;
>> -
>> +	/* copy elf header */
>>   	memcpy(data, fw->data, ehdr_size);
>> -	memcpy(data + ehdr_size, fw->data + hash_offset, hash_size);
>> +
> This seems to duplicates parts of the loop in __qcom_mdt_load(), how
> about breaking this out to a separate
>
> static int mdt_load_segment(struct device *dev, const struct firmware *fw,
> 			    int idx, void *buf, size_t len, bool is_split)
>
> Which either just memcpy from @fw or does the filename and loading
> dance, based on @is_split?
Since mdt_load_segment won't know the name of the firmware without a 
global variable
(which in turn will make it non-reentrant), the idea of creating such a 
function and not passing
the actual name of the firmware seemed wrong.

If we want to pass the firmware name in this function the code size will 
be more or equal to
what we started with. If that is not a problem I can make the changes.

Thanks,
Sid
>
> Regards,
> Bjorn
>
>> +	if (qcom_mdt_bins_are_split(fw)) {
>> +		fw_name = kstrdup(firmware, GFP_KERNEL);
>> +		if (!fw_name) {
>> +			kfree(data);
>> +			return ERR_PTR(-ENOMEM);
>> +		}
>> +		snprintf(fw_name + strlen(fw_name) - 3, 4, "b%02d", hash_index);
>> +
>> +		ret = request_firmware_into_buf(&seg_fw, fw_name, dev, data + ehdr_size, hash_size);
>> +		kfree(fw_name);
>> +
>> +		if (ret) {
>> +			kfree(data);
>> +			return ERR_PTR(ret);
>> +		}
>> +
>> +		release_firmware(seg_fw);
>> +	} else {
>> +		memcpy(data + ehdr_size, fw->data + phdrs[hash_index].p_offset, hash_size);
>> +	}
>>   
>>   	*data_len = ehdr_size + hash_size;
>>   
>> @@ -191,7 +209,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
>>   		return -ENOMEM;
>>   
>>   	if (pas_init) {
>> -		metadata = qcom_mdt_read_metadata(fw, &metadata_len);
>> +		metadata = qcom_mdt_read_metadata(dev, fw, firmware, &metadata_len);
>>   		if (IS_ERR(metadata)) {
>>   			ret = PTR_ERR(metadata);
>>   			goto out;
>> diff --git a/include/linux/soc/qcom/mdt_loader.h b/include/linux/soc/qcom/mdt_loader.h
>> index e600bae..04ba5e8 100644
>> --- a/include/linux/soc/qcom/mdt_loader.h
>> +++ b/include/linux/soc/qcom/mdt_loader.h
>> @@ -21,6 +21,7 @@ int qcom_mdt_load_no_init(struct device *dev, const struct firmware *fw,
>>   			  const char *fw_name, int pas_id, void *mem_region,
>>   			  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);
>> +void *qcom_mdt_read_metadata(struct device *dev, const struct firmware *fw, const char *firmware,
>> +			     size_t *data_len);
>>   
>>   #endif
>> -- 
>> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>

Re: [PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob

[Bjorn Andersson]

On Wed 13 Jan 17:01 CST 2021, Siddharth Gupta wrote:

> 
> On 1/7/2021 4:21 PM, Bjorn Andersson wrote:
> > On Wed 06 Jan 15:23 CST 2021, Siddharth Gupta wrote:
> > 
> > > Since the split elf blobs will always contain the hash segment, we rely on
> > I think it will sounds better if we add "should" in "we should rely on..."
> Sure
> > 
> > > the blob file to get the hash rather than assume that it will be present in
> > > the mdt file. This change uses the hash index to read the appropriate elf
> > > blob to get the hash segment.
> > > 
> > > Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> > > ---
> > >   drivers/remoteproc/qcom_q6v5_mss.c  |  4 ++--
> > >   drivers/soc/qcom/mdt_loader.c       | 38 +++++++++++++++++++++++++++----------
> > >   include/linux/soc/qcom/mdt_loader.h |  3 ++-
> > >   3 files changed, 32 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> > > index 66106ba..74c0229 100644
> > > --- a/drivers/remoteproc/qcom_q6v5_mss.c
> > > +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> > > @@ -4,7 +4,7 @@
> > >    *
> > >    * Copyright (C) 2016 Linaro Ltd.
> > >    * Copyright (C) 2014 Sony Mobile Communications AB
> > > - * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
> > > + * Copyright (c) 2012-2013, 2020 The Linux Foundation. All rights reserved.
> > >    */
> > >   #include <linux/clk.h>
> > > @@ -828,7 +828,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
> > >   	void *ptr;
> > >   	int ret;
> > > -	metadata = qcom_mdt_read_metadata(fw, &size);
> > > +	metadata = qcom_mdt_read_metadata(qproc->dev, fw, qproc->hexagon_mdt_image, &size);
> > >   	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 c9bbd8c..6876c0b 100644
> > > --- a/drivers/soc/qcom/mdt_loader.c
> > > +++ b/drivers/soc/qcom/mdt_loader.c
> > > @@ -103,15 +103,18 @@ 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)
> > > +void *qcom_mdt_read_metadata(struct device *dev, const struct firmware *fw, const char *firmware,
> > > +			     size_t *data_len)
> > >   {
> > >   	const struct elf32_phdr *phdrs;
> > >   	const struct elf32_hdr *ehdr;
> > > -	size_t hash_offset;
> > > +	const struct firmware *seg_fw;
> > >   	size_t hash_index;
> > >   	size_t hash_size;
> > >   	size_t ehdr_size;
> > > +	char *fw_name;
> > >   	void *data;
> > > +	int ret;
> > >   	ehdr = (struct elf32_hdr *)fw->data;
> > >   	phdrs = (struct elf32_phdr *)(ehdr + 1);
> > > @@ -137,14 +140,29 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
> > >   	if (!data)
> > >   		return ERR_PTR(-ENOMEM);
> > > -	/* Is the header and hash already packed */
> > > -	if (qcom_mdt_bins_are_split(fw))
> > > -		hash_offset = phdrs[0].p_filesz;
> > > -	else
> > > -		hash_offset = phdrs[hash_index].p_offset;
> > > -
> > > +	/* copy elf header */
> > >   	memcpy(data, fw->data, ehdr_size);
> > > -	memcpy(data + ehdr_size, fw->data + hash_offset, hash_size);
> > > +
> > This seems to duplicates parts of the loop in __qcom_mdt_load(), how
> > about breaking this out to a separate
> > 
> > static int mdt_load_segment(struct device *dev, const struct firmware *fw,
> > 			    int idx, void *buf, size_t len, bool is_split)
> > 
> > Which either just memcpy from @fw or does the filename and loading
> > dance, based on @is_split?
> Since mdt_load_segment won't know the name of the firmware without a global
> variable
> (which in turn will make it non-reentrant), the idea of creating such a
> function and not passing
> the actual name of the firmware seemed wrong.
> 

Wouldn't you be able to pass "firmware" as an argument to the
load_segment function?

> If we want to pass the firmware name in this function the code size will be
> more or equal to
> what we started with. If that is not a problem I can make the changes.
> 

Perhaps I'm missing something here, I do expect that you would end with
code similar to the hunk you add here. But in doing so we should be able
to reuse that in the __qcom_mdt_load(). Or am I too optimistic?

(In particular I'm not fond of the fw_name dance and doing it twice is
worse)

Regards,
Bjorn

> Thanks,
> Sid
> > 
> > Regards,
> > Bjorn
> > 
> > > +	if (qcom_mdt_bins_are_split(fw)) {
> > > +		fw_name = kstrdup(firmware, GFP_KERNEL);
> > > +		if (!fw_name) {
> > > +			kfree(data);
> > > +			return ERR_PTR(-ENOMEM);
> > > +		}
> > > +		snprintf(fw_name + strlen(fw_name) - 3, 4, "b%02d", hash_index);
> > > +
> > > +		ret = request_firmware_into_buf(&seg_fw, fw_name, dev, data + ehdr_size, hash_size);
> > > +		kfree(fw_name);
> > > +
> > > +		if (ret) {
> > > +			kfree(data);
> > > +			return ERR_PTR(ret);
> > > +		}
> > > +
> > > +		release_firmware(seg_fw);
> > > +	} else {
> > > +		memcpy(data + ehdr_size, fw->data + phdrs[hash_index].p_offset, hash_size);
> > > +	}
> > >   	*data_len = ehdr_size + hash_size;
> > > @@ -191,7 +209,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
> > >   		return -ENOMEM;
> > >   	if (pas_init) {
> > > -		metadata = qcom_mdt_read_metadata(fw, &metadata_len);
> > > +		metadata = qcom_mdt_read_metadata(dev, fw, firmware, &metadata_len);
> > >   		if (IS_ERR(metadata)) {
> > >   			ret = PTR_ERR(metadata);
> > >   			goto out;
> > > diff --git a/include/linux/soc/qcom/mdt_loader.h b/include/linux/soc/qcom/mdt_loader.h
> > > index e600bae..04ba5e8 100644
> > > --- a/include/linux/soc/qcom/mdt_loader.h
> > > +++ b/include/linux/soc/qcom/mdt_loader.h
> > > @@ -21,6 +21,7 @@ int qcom_mdt_load_no_init(struct device *dev, const struct firmware *fw,
> > >   			  const char *fw_name, int pas_id, void *mem_region,
> > >   			  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);
> > > +void *qcom_mdt_read_metadata(struct device *dev, const struct firmware *fw, const char *firmware,
> > > +			     size_t *data_len);
> > >   #endif
> > > -- 
> > > Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> > > a Linux Foundation Collaborative Project
> > > 

Re: [PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob

[Siddharth Gupta]

On 1/14/2021 9:46 AM, Bjorn Andersson wrote:
> On Wed 13 Jan 17:01 CST 2021, Siddharth Gupta wrote:
>
>> On 1/7/2021 4:21 PM, Bjorn Andersson wrote:
>>> On Wed 06 Jan 15:23 CST 2021, Siddharth Gupta wrote:
>>>
>>>> Since the split elf blobs will always contain the hash segment, we rely on
>>> I think it will sounds better if we add "should" in "we should rely on..."
>> Sure
>>>> the blob file to get the hash rather than assume that it will be present in
>>>> the mdt file. This change uses the hash index to read the appropriate elf
>>>> blob to get the hash segment.
>>>>
>>>> Signed-off-by: Siddharth Gupta<sidgup@codeaurora.org>
>>>> ---
>>>>    drivers/remoteproc/qcom_q6v5_mss.c  |  4 ++--
>>>>    drivers/soc/qcom/mdt_loader.c       | 38 +++++++++++++++++++++++++++----------
>>>>    include/linux/soc/qcom/mdt_loader.h |  3 ++-
>>>>    3 files changed, 32 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
>>>> index 66106ba..74c0229 100644
>>>> --- a/drivers/remoteproc/qcom_q6v5_mss.c
>>>> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
>>>> @@ -4,7 +4,7 @@
>>>>     *
>>>>     * Copyright (C) 2016 Linaro Ltd.
>>>>     * Copyright (C) 2014 Sony Mobile Communications AB
>>>> - * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
>>>> + * Copyright (c) 2012-2013, 2020 The Linux Foundation. All rights reserved.
>>>>     */
>>>>    #include <linux/clk.h>
>>>> @@ -828,7 +828,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
>>>>    	void *ptr;
>>>>    	int ret;
>>>> -	metadata = qcom_mdt_read_metadata(fw, &size);
>>>> +	metadata = qcom_mdt_read_metadata(qproc->dev, fw, qproc->hexagon_mdt_image, &size);
>>>>    	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 c9bbd8c..6876c0b 100644
>>>> --- a/drivers/soc/qcom/mdt_loader.c
>>>> +++ b/drivers/soc/qcom/mdt_loader.c
>>>> @@ -103,15 +103,18 @@ 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)
>>>> +void *qcom_mdt_read_metadata(struct device *dev, const struct firmware *fw, const char *firmware,
>>>> +			     size_t *data_len)
>>>>    {
>>>>    	const struct elf32_phdr *phdrs;
>>>>    	const struct elf32_hdr *ehdr;
>>>> -	size_t hash_offset;
>>>> +	const struct firmware *seg_fw;
>>>>    	size_t hash_index;
>>>>    	size_t hash_size;
>>>>    	size_t ehdr_size;
>>>> +	char *fw_name;
>>>>    	void *data;
>>>> +	int ret;
>>>>    	ehdr = (struct elf32_hdr *)fw->data;
>>>>    	phdrs = (struct elf32_phdr *)(ehdr + 1);
>>>> @@ -137,14 +140,29 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
>>>>    	if (!data)
>>>>    		return ERR_PTR(-ENOMEM);
>>>> -	/* Is the header and hash already packed */
>>>> -	if (qcom_mdt_bins_are_split(fw))
>>>> -		hash_offset = phdrs[0].p_filesz;
>>>> -	else
>>>> -		hash_offset = phdrs[hash_index].p_offset;
>>>> -
>>>> +	/* copy elf header */
>>>>    	memcpy(data, fw->data, ehdr_size);
>>>> -	memcpy(data + ehdr_size, fw->data + hash_offset, hash_size);
>>>> +
>>> This seems to duplicates parts of the loop in __qcom_mdt_load(), how
>>> about breaking this out to a separate
>>>
>>> static int mdt_load_segment(struct device *dev, const struct firmware *fw,
>>> 			    int idx, void *buf, size_t len, bool is_split)
>>>
>>> Which either just memcpy from @fw or does the filename and loading
>>> dance, based on @is_split?
>> Since mdt_load_segment won't know the name of the firmware without a global
>> variable
>> (which in turn will make it non-reentrant), the idea of creating such a
>> function and not passing
>> the actual name of the firmware seemed wrong.
>>
> Wouldn't you be able to pass "firmware" as an argument to the
> load_segment function?
We can, which is what I guess you meant in the first place. What I thought
was that if we are creating something like mdt_load_segments passing
"firmware.mdt" with the index instead of "firmware.b<index>" seemed
kind of wrong, but I guess that is just a matter of the naming convention.
>> If we want to pass the firmware name in this function the code size will be
>> more or equal to
>> what we started with. If that is not a problem I can make the changes.
>>
> Perhaps I'm missing something here, I do expect that you would end with
> code similar to the hunk you add here. But in doing so we should be able
> to reuse that in the __qcom_mdt_load(). Or am I too optimistic?
>
> (In particular I'm not fond of the fw_name dance and doing it twice is
> worse)
If I am creating the firmware name inside this function then we should 
definitely
see a code reduction. I'll make the changes and push the new patchset soon.

Thanks,
Sid
>
> Regards,
> Bjorn
>
>> Thanks,
>> Sid
>>> Regards,
>>> Bjorn
>>>
>>>> +	if (qcom_mdt_bins_are_split(fw)) {
>>>> +		fw_name = kstrdup(firmware, GFP_KERNEL);
>>>> +		if (!fw_name) {
>>>> +			kfree(data);
>>>> +			return ERR_PTR(-ENOMEM);
>>>> +		}
>>>> +		snprintf(fw_name + strlen(fw_name) - 3, 4, "b%02d", hash_index);
>>>> +
>>>> +		ret = request_firmware_into_buf(&seg_fw, fw_name, dev, data + ehdr_size, hash_size);
>>>> +		kfree(fw_name);
>>>> +
>>>> +		if (ret) {
>>>> +			kfree(data);
>>>> +			return ERR_PTR(ret);
>>>> +		}
>>>> +
>>>> +		release_firmware(seg_fw);
>>>> +	} else {
>>>> +		memcpy(data + ehdr_size, fw->data + phdrs[hash_index].p_offset, hash_size);
>>>> +	}
>>>>    	*data_len = ehdr_size + hash_size;
>>>> @@ -191,7 +209,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
>>>>    		return -ENOMEM;
>>>>    	if (pas_init) {
>>>> -		metadata = qcom_mdt_read_metadata(fw, &metadata_len);
>>>> +		metadata = qcom_mdt_read_metadata(dev, fw, firmware, &metadata_len);
>>>>    		if (IS_ERR(metadata)) {
>>>>    			ret = PTR_ERR(metadata);
>>>>    			goto out;
>>>> diff --git a/include/linux/soc/qcom/mdt_loader.h b/include/linux/soc/qcom/mdt_loader.h
>>>> index e600bae..04ba5e8 100644
>>>> --- a/include/linux/soc/qcom/mdt_loader.h
>>>> +++ b/include/linux/soc/qcom/mdt_loader.h
>>>> @@ -21,6 +21,7 @@ int qcom_mdt_load_no_init(struct device *dev, const struct firmware *fw,
>>>>    			  const char *fw_name, int pas_id, void *mem_region,
>>>>    			  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);
>>>> +void *qcom_mdt_read_metadata(struct device *dev, const struct firmware *fw, const char *firmware,
>>>> +			     size_t *data_len);
>>>>    #endif
>>>> -- 
>>>> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>>>> a Linux Foundation Collaborative Project
>>>>