| Message ID | 1652248625-990-1-git-send-email-quic_sibis@quicinc.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] remoteproc: qcom_q6v5_mss: map/unmap metadata region before/after use | expand |
On Wed, May 11, 2022 at 7:57 AM Sibi Sankar <quic_sibis@quicinc.com> wrote: > > The application processor accessing the dynamically assigned metadata > region after assigning it to the remote Q6 would lead to an XPU violation. > Fix this by un-mapping the metadata region post firmware header copy. The > metadata region is freed only after the modem Q6 is done with fw header > authentication. > > Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> Acked-by: Arnd Bergmann <arnd@arndb.de> Sorry for the late reply, this looks reasonable overall. Just two small comments: > > - memcpy(ptr, metadata, size); > + count = PAGE_ALIGN(size) >> PAGE_SHIFT; > + pages = kmalloc_array(count, sizeof(struct page *), GFP_KERNEL); > + if (!pages) { > + ret = -ENOMEM; > + goto free_dma_attrs; > + } If you know a fixed upper bound for the array size, it might be easier to put it on the stack. > + > + for (i = 0; i < count; i++) > + pages[i] = nth_page(page, i); > + > + vaddr = vmap(pages, count, flags, pgprot_dmacoherent(PAGE_KERNEL)); I was a bit unsure about this part, as I don't know how portable this is. If the CPU bypasses the cache with pgprot_dmacoherent(), then the other side should not use a cacheable access either, but that is a property of the hardware that is normally hidden from the driver interface. It's probably ok here, since the pages are not mapped anywhere else and should have no active cache lines. Arnd
Hey Arnd, Thanks for taking time to review the patch. On 5/30/22 9:41 PM, Arnd Bergmann wrote: > On Wed, May 11, 2022 at 7:57 AM Sibi Sankar <quic_sibis@quicinc.com> wrote: >> >> The application processor accessing the dynamically assigned metadata >> region after assigning it to the remote Q6 would lead to an XPU violation. >> Fix this by un-mapping the metadata region post firmware header copy. The >> metadata region is freed only after the modem Q6 is done with fw header >> authentication. >> >> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> > > Acked-by: Arnd Bergmann <arnd@arndb.de> > > Sorry for the late reply, this looks reasonable overall. Just two > small comments: > >> >> - memcpy(ptr, metadata, size); >> + count = PAGE_ALIGN(size) >> PAGE_SHIFT; >> + pages = kmalloc_array(count, sizeof(struct page *), GFP_KERNEL); >> + if (!pages) { >> + ret = -ENOMEM; >> + goto free_dma_attrs; >> + } > > If you know a fixed upper bound for the array size, it might be easier to > put it on the stack. The metadata consists of the 32bit elf header and SoC dependent variable number of program headers. Arriving at the upper bound from the spec seemed futile since the max program headers supported could be > 0xffff. The best I can do is get the max size of metadata of all the QC SoCs supported upstream for putting the pages on stack and leave "count" as the min between the dynamic calculation and upper bound. Would that be good enough? > >> + >> + for (i = 0; i < count; i++) >> + pages[i] = nth_page(page, i); >> + >> + vaddr = vmap(pages, count, flags, pgprot_dmacoherent(PAGE_KERNEL)); > > I was a bit unsure about this part, as I don't know how portable this is. > If the CPU bypasses the cache with pgprot_dmacoherent(), then the > other side should not use a cacheable access either, but that is a property > of the hardware that is normally hidden from the driver interface. > > It's probably ok here, since the pages are not mapped anywhere else > and should have no active cache lines. yup we make sure the other side can access the region only after no cache lines are active (that's the main problem that we are trying to solve through this patch). -Sibi > > Arnd >
On Wed 11 May 00:57 CDT 2022, Sibi Sankar wrote: > The application processor accessing the dynamically assigned metadata > region after assigning it to the remote Q6 would lead to an XPU violation. > Fix this by un-mapping the metadata region post firmware header copy. The > metadata region is freed only after the modem Q6 is done with fw header > authentication. > > Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > > V2: > * Fix error when MSS is built as a module [Kernel Test Bot] > * Fixup cleanup errors > > drivers/remoteproc/qcom_q6v5_mss.c | 38 ++++++++++++++++++++++++++++++++------ > 1 file changed, 32 insertions(+), 6 deletions(-) > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c > index af217de75e4d..4b37e11fbb03 100644 > --- a/drivers/remoteproc/qcom_q6v5_mss.c > +++ b/drivers/remoteproc/qcom_q6v5_mss.c > @@ -10,6 +10,7 @@ > #include <linux/clk.h> > #include <linux/delay.h> > #include <linux/devcoredump.h> > +#include <linux/dma-map-ops.h> > #include <linux/dma-mapping.h> > #include <linux/interrupt.h> > #include <linux/kernel.h> > @@ -932,27 +933,52 @@ static void q6v5proc_halt_axi_port(struct q6v5 *qproc, > static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw, > const char *fw_name) > { > - unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS; > + unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS | DMA_ATTR_NO_KERNEL_MAPPING; > + unsigned long flags = VM_DMA_COHERENT | VM_FLUSH_RESET_PERMS; > + struct page **pages; > + struct page *page; > dma_addr_t phys; > void *metadata; > int mdata_perm; > int xferop_ret; > size_t size; > - void *ptr; > + void *vaddr; > + int count; > int ret; > + int i; > > metadata = qcom_mdt_read_metadata(fw, &size, fw_name, qproc->dev); > if (IS_ERR(metadata)) > return PTR_ERR(metadata); > > - ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs); > - if (!ptr) { > + page = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs); > + if (!page) { > kfree(metadata); > dev_err(qproc->dev, "failed to allocate mdt buffer\n"); > return -ENOMEM; > } > > - memcpy(ptr, metadata, size); > + count = PAGE_ALIGN(size) >> PAGE_SHIFT; > + pages = kmalloc_array(count, sizeof(struct page *), GFP_KERNEL); > + if (!pages) { > + ret = -ENOMEM; > + goto free_dma_attrs; > + } > + > + for (i = 0; i < count; i++) > + pages[i] = nth_page(page, i); > + > + vaddr = vmap(pages, count, flags, pgprot_dmacoherent(PAGE_KERNEL)); > + kfree(pages); > + if (!vaddr) { > + dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n", &phys, size); > + ret = -EBUSY; > + goto free_dma_attrs; > + } > + > + memcpy(vaddr, metadata, size); > + > + vunmap(vaddr); > > /* Hypervisor mapping to access metadata by modem */ > mdata_perm = BIT(QCOM_SCM_VMID_HLOS); > @@ -982,7 +1008,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw, > "mdt buffer not reclaimed system may become unstable\n"); > > free_dma_attrs: > - dma_free_attrs(qproc->dev, size, ptr, phys, dma_attrs); > + dma_free_attrs(qproc->dev, size, page, phys, dma_attrs); > kfree(metadata); > > return ret < 0 ? ret : 0; > -- > 2.7.4 >
On Wed, 11 May 2022 11:27:05 +0530, Sibi Sankar wrote: > The application processor accessing the dynamically assigned metadata > region after assigning it to the remote Q6 would lead to an XPU violation. > Fix this by un-mapping the metadata region post firmware header copy. The > metadata region is freed only after the modem Q6 is done with fw header > authentication. > > > [...] Applied, thanks! [1/1] remoteproc: qcom_q6v5_mss: map/unmap metadata region before/after use commit: 8808fc4008e3bb70bfe682c41d8c0d8626d1ec0b Best regards,
diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c index af217de75e4d..4b37e11fbb03 100644 --- a/drivers/remoteproc/qcom_q6v5_mss.c +++ b/drivers/remoteproc/qcom_q6v5_mss.c @@ -10,6 +10,7 @@ #include <linux/clk.h> #include <linux/delay.h> #include <linux/devcoredump.h> +#include <linux/dma-map-ops.h> #include <linux/dma-mapping.h> #include <linux/interrupt.h> #include <linux/kernel.h> @@ -932,27 +933,52 @@ static void q6v5proc_halt_axi_port(struct q6v5 *qproc, static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw, const char *fw_name) { - unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS; + unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS | DMA_ATTR_NO_KERNEL_MAPPING; + unsigned long flags = VM_DMA_COHERENT | VM_FLUSH_RESET_PERMS; + struct page **pages; + struct page *page; dma_addr_t phys; void *metadata; int mdata_perm; int xferop_ret; size_t size; - void *ptr; + void *vaddr; + int count; int ret; + int i; metadata = qcom_mdt_read_metadata(fw, &size, fw_name, qproc->dev); if (IS_ERR(metadata)) return PTR_ERR(metadata); - ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs); - if (!ptr) { + page = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs); + if (!page) { kfree(metadata); dev_err(qproc->dev, "failed to allocate mdt buffer\n"); return -ENOMEM; } - memcpy(ptr, metadata, size); + count = PAGE_ALIGN(size) >> PAGE_SHIFT; + pages = kmalloc_array(count, sizeof(struct page *), GFP_KERNEL); + if (!pages) { + ret = -ENOMEM; + goto free_dma_attrs; + } + + for (i = 0; i < count; i++) + pages[i] = nth_page(page, i); + + vaddr = vmap(pages, count, flags, pgprot_dmacoherent(PAGE_KERNEL)); + kfree(pages); + if (!vaddr) { + dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n", &phys, size); + ret = -EBUSY; + goto free_dma_attrs; + } + + memcpy(vaddr, metadata, size); + + vunmap(vaddr); /* Hypervisor mapping to access metadata by modem */ mdata_perm = BIT(QCOM_SCM_VMID_HLOS); @@ -982,7 +1008,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw, "mdt buffer not reclaimed system may become unstable\n"); free_dma_attrs: - dma_free_attrs(qproc->dev, size, ptr, phys, dma_attrs); + dma_free_attrs(qproc->dev, size, page, phys, dma_attrs); kfree(metadata); return ret < 0 ? ret : 0;
The application processor accessing the dynamically assigned metadata region after assigning it to the remote Q6 would lead to an XPU violation. Fix this by un-mapping the metadata region post firmware header copy. The metadata region is freed only after the modem Q6 is done with fw header authentication. Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> --- V2: * Fix error when MSS is built as a module [Kernel Test Bot] * Fixup cleanup errors drivers/remoteproc/qcom_q6v5_mss.c | 38 ++++++++++++++++++++++++++++++++------ 1 file changed, 32 insertions(+), 6 deletions(-)