* [PATCH 0/3] soc: qcom: mdt_loader: General improvements
@ 2021-01-06 21:23 Siddharth Gupta
2021-01-06 21:23 ` [PATCH 1/3] soc: qcom: mdt_loader: Allow hash at any phdr Siddharth Gupta
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Siddharth Gupta @ 2021-01-06 21:23 UTC (permalink / raw)
To: bjorn.andersson, agross, ohad, linux-arm-msm, linux-remoteproc,
linux-kernel
Cc: Siddharth Gupta, psodagud, rishabhb
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
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/3] soc: qcom: mdt_loader: Allow hash at any phdr 2021-01-06 21:23 [PATCH 0/3] soc: qcom: mdt_loader: General improvements Siddharth Gupta @ 2021-01-06 21:23 ` Siddharth Gupta 2021-01-08 0:03 ` Bjorn Andersson 2021-01-06 21:23 ` [PATCH 2/3] soc: qcom: mdt_loader: Handle split bins correctly Siddharth Gupta 2021-01-06 21:23 ` [PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob Siddharth Gupta 2 siblings, 1 reply; 12+ messages in thread From: Siddharth Gupta @ 2021-01-06 21:23 UTC (permalink / raw) To: bjorn.andersson, agross, ohad, linux-arm-msm, linux-remoteproc, linux-kernel Cc: Siddharth Gupta, psodagud, rishabhb 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] soc: qcom: mdt_loader: Allow hash at any phdr 2021-01-06 21:23 ` [PATCH 1/3] soc: qcom: mdt_loader: Allow hash at any phdr Siddharth Gupta @ 2021-01-08 0:03 ` Bjorn Andersson 0 siblings, 0 replies; 12+ messages in thread From: Bjorn Andersson @ 2021-01-08 0:03 UTC (permalink / raw) To: Siddharth Gupta Cc: agross, ohad, linux-arm-msm, linux-remoteproc, linux-kernel, psodagud, rishabhb 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] soc: qcom: mdt_loader: Handle split bins correctly 2021-01-06 21:23 [PATCH 0/3] soc: qcom: mdt_loader: General improvements Siddharth Gupta 2021-01-06 21:23 ` [PATCH 1/3] soc: qcom: mdt_loader: Allow hash at any phdr Siddharth Gupta @ 2021-01-06 21:23 ` Siddharth Gupta 2021-01-08 0:07 ` Bjorn Andersson 2021-01-06 21:23 ` [PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob Siddharth Gupta 2 siblings, 1 reply; 12+ messages in thread From: Siddharth Gupta @ 2021-01-06 21:23 UTC (permalink / raw) To: bjorn.andersson, agross, ohad, linux-arm-msm, linux-remoteproc, linux-kernel Cc: Siddharth Gupta, psodagud, rishabhb 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] soc: qcom: mdt_loader: Handle split bins correctly 2021-01-06 21:23 ` [PATCH 2/3] soc: qcom: mdt_loader: Handle split bins correctly Siddharth Gupta @ 2021-01-08 0:07 ` Bjorn Andersson 0 siblings, 0 replies; 12+ messages in thread From: Bjorn Andersson @ 2021-01-08 0:07 UTC (permalink / raw) To: Siddharth Gupta Cc: agross, ohad, linux-arm-msm, linux-remoteproc, linux-kernel, psodagud, rishabhb 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob 2021-01-06 21:23 [PATCH 0/3] soc: qcom: mdt_loader: General improvements Siddharth Gupta 2021-01-06 21:23 ` [PATCH 1/3] soc: qcom: mdt_loader: Allow hash at any phdr Siddharth Gupta 2021-01-06 21:23 ` [PATCH 2/3] soc: qcom: mdt_loader: Handle split bins correctly Siddharth Gupta @ 2021-01-06 21:23 ` Siddharth Gupta 2021-01-06 23:41 ` kernel test robot 2021-01-08 0:21 ` Bjorn Andersson 2 siblings, 2 replies; 12+ messages in thread From: Siddharth Gupta @ 2021-01-06 21:23 UTC (permalink / raw) To: bjorn.andersson, agross, ohad, linux-arm-msm, linux-remoteproc, linux-kernel Cc: Siddharth Gupta, psodagud, rishabhb 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob 2021-01-06 21:23 ` [PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob Siddharth Gupta @ 2021-01-06 23:41 ` kernel test robot 2021-01-08 0:21 ` Bjorn Andersson 1 sibling, 0 replies; 12+ messages in thread From: kernel test robot @ 2021-01-06 23:41 UTC (permalink / raw) To: Siddharth Gupta, bjorn.andersson, agross, ohad, linux-arm-msm, linux-remoteproc, linux-kernel Cc: kbuild-all, Siddharth Gupta, psodagud, rishabhb [-- 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 --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob @ 2021-01-06 23:41 ` kernel test robot 0 siblings, 0 replies; 12+ messages in thread From: kernel test robot @ 2021-01-06 23:41 UTC (permalink / raw) To: kbuild-all [-- 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 --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob 2021-01-06 21:23 ` [PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob Siddharth Gupta 2021-01-06 23:41 ` kernel test robot @ 2021-01-08 0:21 ` Bjorn Andersson 2021-01-13 23:01 ` Siddharth Gupta 1 sibling, 1 reply; 12+ messages in thread From: Bjorn Andersson @ 2021-01-08 0:21 UTC (permalink / raw) To: Siddharth Gupta Cc: agross, ohad, linux-arm-msm, linux-remoteproc, linux-kernel, psodagud, rishabhb 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob 2021-01-08 0:21 ` Bjorn Andersson @ 2021-01-13 23:01 ` Siddharth Gupta 2021-01-14 17:46 ` Bjorn Andersson 0 siblings, 1 reply; 12+ messages in thread From: Siddharth Gupta @ 2021-01-13 23:01 UTC (permalink / raw) To: Bjorn Andersson Cc: agross, ohad, linux-arm-msm, linux-remoteproc, linux-kernel, psodagud, rishabhb 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 >> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob 2021-01-13 23:01 ` Siddharth Gupta @ 2021-01-14 17:46 ` Bjorn Andersson 2021-01-17 23:03 ` Siddharth Gupta 0 siblings, 1 reply; 12+ messages in thread From: Bjorn Andersson @ 2021-01-14 17:46 UTC (permalink / raw) To: Siddharth Gupta Cc: agross, ohad, linux-arm-msm, linux-remoteproc, linux-kernel, psodagud, rishabhb 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 > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob 2021-01-14 17:46 ` Bjorn Andersson @ 2021-01-17 23:03 ` Siddharth Gupta 0 siblings, 0 replies; 12+ messages in thread From: Siddharth Gupta @ 2021-01-17 23:03 UTC (permalink / raw) To: Bjorn Andersson Cc: agross, ohad, linux-arm-msm, linux-remoteproc, linux-kernel, psodagud, rishabhb 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 >>>> ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-01-17 23:04 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-06 21:23 [PATCH 0/3] soc: qcom: mdt_loader: General improvements Siddharth Gupta 2021-01-06 21:23 ` [PATCH 1/3] soc: qcom: mdt_loader: Allow hash at any phdr Siddharth Gupta 2021-01-08 0:03 ` Bjorn Andersson 2021-01-06 21:23 ` [PATCH 2/3] soc: qcom: mdt_loader: Handle split bins correctly Siddharth Gupta 2021-01-08 0:07 ` Bjorn Andersson 2021-01-06 21:23 ` [PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob Siddharth Gupta 2021-01-06 23:41 ` kernel test robot 2021-01-06 23:41 ` kernel test robot 2021-01-08 0:21 ` Bjorn Andersson 2021-01-13 23:01 ` Siddharth Gupta 2021-01-14 17:46 ` Bjorn Andersson 2021-01-17 23:03 ` Siddharth Gupta
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.