From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bjorn Andersson Subject: Re: [PATCH 11/12] drm/msm: Add a quick and dirty PIL loader Date: Mon, 5 Dec 2016 11:57:43 -0800 Message-ID: <20161205195743.GM9322@tuxbot> References: <1480361317-9937-1-git-send-email-jcrouse@codeaurora.org> <1480361317-9937-12-git-send-email-jcrouse@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from mail-pf0-f174.google.com ([209.85.192.174]:34337 "EHLO mail-pf0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751201AbcLET5s (ORCPT ); Mon, 5 Dec 2016 14:57:48 -0500 Received: by mail-pf0-f174.google.com with SMTP id c4so65264131pfb.1 for ; Mon, 05 Dec 2016 11:57:47 -0800 (PST) Content-Disposition: inline In-Reply-To: <1480361317-9937-12-git-send-email-jcrouse@codeaurora.org> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Jordan Crouse Cc: freedreno@lists.freedesktop.org, linux-arm-msm , ML dri-devel On Mon 28 Nov 11:28 PST 2016, Jordan Crouse wrote: > In order to switch the GPU out of secure mode on most systems we > need to load a zap shader into memory and get it authenticated > and into the secure world. All the bits and pieces to do > the load are scattered throughout the kernel, but we need to > bring everything together. > > Add a semi-custom loader that will read a MDT file and get > it loaded and authenticated through SCM. > I've been trying to figure out a reasonable way to provide a scm/pas/mdt-loader, but haven't come up with anything sane yet. Perhaps it's better to provide helpers for the scm case and open code the MDT loading in the non-scm driver. I think this is an okay approach for now. But it's not a "PIL loader", that's just a side effect of reusing parts of the existing PIL load in the Qualcomm tree. I would suggest just making the subject "Add ZAP MDT loader". Some minor comments below. > Signed-off-by: Jordan Crouse > --- > drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 166 ++++++++++++++++++++++++++++++++++ > 1 file changed, 166 insertions(+) > > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > index 3824bc4..eefe197 100644 > --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > @@ -11,12 +11,178 @@ > * > */ > > +#include > +#include > +#include > +#include > +#include > +#include > #include "msm_gem.h" > #include "a5xx_gpu.h" > > extern bool hang_debug; > static void a5xx_dump(struct msm_gpu *gpu); > > +static inline bool _check_segment(const struct elf32_phdr *phdr) > +{ > + return ((phdr->p_type == PT_LOAD) && > + ((phdr->p_flags & (7 << 24)) != (2 << 24)) && > + phdr->p_memsz); > +} > + > +static int __pil_tz_load_image(struct platform_device *pdev, Rather than throwing more underscores at it I would have named this "zap_load_segments". > + const struct firmware *mdt, const char *fwname, > + void *fwptr, size_t fw_size, unsigned long fw_min_addr) > +{ > + char str[64] = { 0 }; Name this filename or similar and no need to initialize it. > + const struct elf32_hdr *ehdr = (struct elf32_hdr *) mdt->data; > + const struct elf32_phdr *phdrs = (struct elf32_phdr *) (ehdr + 1); > + const struct firmware *fw; > + int i, ret = 0; > + > + for (i = 0; i < ehdr->e_phnum; i++) { > + const struct elf32_phdr *phdr = &phdrs[i]; > + size_t offset; > + > + /* Make sure the segment is loadable */ > + if (!_check_segment(phdr)) > + continue; > + > + /* Get the offset of the segment within the region */ > + offset = (phdr->p_paddr - fw_min_addr); > + > + /* Request the file containing the segment */ > + snprintf(str, sizeof(str) - 1, "%s.b%02d", fwname, i); snprintf(, size, ) writes at most size bytes and includes null termination, so drop the "- 1". > + > + ret = request_firmware(&fw, str, &pdev->dev); > + if (ret) { > + dev_err(&pdev->dev, "Failed to load segment %s\n", str); > + break; > + } > + > + if (offset + fw->size > fw_size) { > + dev_err(&pdev->dev, "Segment %s is too big\n", str); > + ret = -EINVAL; > + release_firmware(fw); > + break; > + } > + > + /* Copy the segment into place */ > + memcpy(fwptr + offset, fw->data, fw->size); Just to be on the safe side, please add: if (phdr->p_memsz > phdr->p_filesz) memset(fwptr + fw->size, 0, phdr->p_memsz - phdr->p_filesz); > + release_firmware(fw); > + } > + > + return ret; > +} > + > +static int _pil_tz_load_image(struct platform_device *pdev) zap_load_mdt() ? > +{ > + char str[64] = { 0 }; > + const char *fwname; > + const struct elf32_hdr *ehdr; > + const struct elf32_phdr *phdrs; > + const struct firmware *mdt; > + phys_addr_t fw_min_addr, fw_max_addr; > + dma_addr_t fw_phys; > + size_t fw_size; > + u32 pas_id; > + void *ptr; > + int i, ret; > + > + if (pdev == NULL) > + return -ENODEV; This should not happen. > + > + if (!qcom_scm_is_available()) { > + dev_err(&pdev->dev, "SCM is not available\n"); > + return -EINVAL; We generally return -EPROBE_DEFER here. > + } > + > + ret = of_reserved_mem_device_init(&pdev->dev); > + Please drop the extra newline. > + if (ret) { > + dev_err(&pdev->dev, "Unable to set up the reserved memory\n"); > + return ret; > + } > + > + /* Get the firmware and PAS id from the device node */ > + if (of_property_read_string(pdev->dev.of_node, "qcom,firmware", > + &fwname)) { > + dev_err(&pdev->dev, "Could not read a firmware name\n"); > + return -EINVAL; > + } > + > + if (of_property_read_u32(pdev->dev.of_node, "qcom,pas-id", &pas_id)) { This is constant, so define it in the driver. > + dev_err(&pdev->dev, "Could not read the pas ID\n"); > + return -EINVAL; > + } > + > + snprintf(str, sizeof(str) - 1, "%s.mdt", fwname); As above, re "- 1", name of str and initialization. > + > + /* Request the MDT file for the firmware */ > + ret = request_firmware(&mdt, str, &pdev->dev); > + if (ret) { > + dev_err(&pdev->dev, "Unable to load %s\n", str); > + return ret; > + } > + > + ehdr = (struct elf32_hdr *) mdt->data; > + phdrs = (struct elf32_phdr *) (ehdr + 1); > + > + /* Get the extents of the firmware image */ > + > + fw_min_addr = (phys_addr_t) ULLONG_MAX; > + fw_max_addr = 0; > + > + for (i = 0; i < ehdr->e_phnum; i++) { > + const struct elf32_phdr *phdr = &phdrs[i]; > + > + if (!_check_segment(phdr)) > + continue; > + > + fw_min_addr = min_t(phys_addr_t, fw_min_addr, phdr->p_paddr); > + fw_max_addr = max_t(phys_addr_t, fw_max_addr, > + PAGE_ALIGN(phdr->p_paddr + phdr->p_memsz)); > + } > + > + if (fw_min_addr == (phys_addr_t) ULLONG_MAX && fw_max_addr == 0) > + goto out; > + > + fw_size = (size_t) (fw_max_addr - fw_min_addr); > + > + /* Verify the MDT header */ > + ret = qcom_scm_pas_init_image(pas_id, mdt->data, mdt->size); > + if (ret) { > + dev_err(&pdev->dev, "Invalid firmware metadata\n"); > + goto out; > + } > + > + /* allocate some memory */ > + ptr = dma_alloc_coherent(&pdev->dev, fw_size, &fw_phys, GFP_KERNEL); > + if (ptr == NULL) > + goto out; > + > + /* Set up the newly allocated memory region */ > + ret = qcom_scm_pas_mem_setup(pas_id, fw_phys, fw_size); > + if (ret) { > + dev_err(&pdev->dev, "Unable to set up firmware memory\n"); > + goto out; > + } > + > + ret = __pil_tz_load_image(pdev, mdt, fwname, ptr, fw_size, fw_min_addr); > + if (!ret) { Please don't mix style like this, goto out on error. > + ret = qcom_scm_pas_auth_and_reset(pas_id); > + if (ret) > + dev_err(&pdev->dev, "Unable to authorize the image\n"); > + } > + > +out: > + if (ret && ptr) > + dma_free_coherent(&pdev->dev, fw_size, ptr, fw_phys); > + > + release_firmware(mdt); > + return ret; > +} > + Regards, Bjorn