[PATCH v2] remoteproc: qcom_q6v5_mss: map/unmap metadata region before/after use

[PATCH v2] remoteproc: qcom_q6v5_mss: map/unmap metadata region before/after use

[Sibi Sankar]

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(-)

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

Re: [PATCH v2] remoteproc: qcom_q6v5_mss: map/unmap metadata region before/after use

[Arnd Bergmann]

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

Re: [PATCH v2] remoteproc: qcom_q6v5_mss: map/unmap metadata region before/after use

[Sibi Sankar]

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
> 

Re: [PATCH v2] remoteproc: qcom_q6v5_mss: map/unmap metadata region before/after use

[Bjorn Andersson]

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
> 

Re: (subset) [PATCH v2] remoteproc: qcom_q6v5_mss: map/unmap metadata region before/after use

[Bjorn Andersson]

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,
-- 
Bjorn Andersson <bjorn.andersson@linaro.org>