mbox series

[V2,00/11] Fix XPU violation during modem metadata authentication

Message ID 20230109034843.23759-1-quic_sibis@quicinc.com
Headers show
Series Fix XPU violation during modem metadata authentication | expand

Message

Sibi Sankar Jan. 9, 2023, 3:48 a.m. UTC 
The memory region allocated using dma_alloc_attr with no kernel mapping
attribute set would still be a part of the linear kernel map. Any access
to this region by the application processor after assigning it to the
remote Q6 will result in a XPU violation. Fix this by replacing the
dynamically allocated memory region with a no-map carveout and unmap the
modem metadata memory region before passing control to the remote Q6.
The addition of the carveout and memunmap is required only on SoCs that
mandate memory protection before transferring control to Q6, hence the
driver falls back to dynamic memory allocation in the absence of the
modem metadata carveout.

V2:
 * Convert legacy bindings to yaml
 * Revert no_kernel_mapping [Mani/Robin]
 * Pad commit message to explain bindings break [Krzysztof]
 * Split dt/bindings per SoC  [Krzysztof] 

Sibi Sankar (11):
  dt-bindings: remoteproc: qcom,q6v5: Move MSM8996 to schema
  dt-bindings: remoteproc: qcom,msm8996-mss-pil: Update memory region
  dt-bindings: remoteproc: qcom,sc7180-mss-pil: Update memory-region
  dt-bindings: remoteproc: qcom,sc7280-mss-pil: Update memory-region
  remoteproc: qcom_q6v5_mss: revert "map/unmap metadata region
    before/after use"
  remoteproc: qcom_q6v5_mss: Use a carveout to authenticate modem
    headers
  arm64: dts: qcom: msm8996: Add a carveout for modem metadata
  arm64: dts: qcom: msm8998: Add a carveout for modem metadata
  arm64: dts: qcom: sdm845: Add a carveout for modem metadata
  arm64: dts: qcom: sc7180: Add a carveout for modem metadata
  arm64: dts: qcom: sc7280: Add a carveout for modem metadata

 .../remoteproc/qcom,msm8996-mss-pil.yaml      | 382 ++++++++++++++++++
 .../bindings/remoteproc/qcom,q6v5.txt         | 137 +------
 .../remoteproc/qcom,sc7180-mss-pil.yaml       |   3 +-
 .../remoteproc/qcom,sc7280-mss-pil.yaml       |   3 +-
 .../boot/dts/qcom/msm8996-xiaomi-common.dtsi  |   6 +
 arch/arm64/boot/dts/qcom/msm8996.dtsi         |   9 +
 arch/arm64/boot/dts/qcom/msm8998.dtsi         |   9 +
 arch/arm64/boot/dts/qcom/sc7180-idp.dts       |   7 +-
 arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi  |   7 +-
 .../dts/qcom/sc7280-herobrine-lte-sku.dtsi    |   7 +-
 arch/arm64/boot/dts/qcom/sdm845.dtsi          |   9 +
 drivers/remoteproc/qcom_q6v5_mss.c            |  78 ++--
 12 files changed, 486 insertions(+), 171 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,msm8996-mss-pil.yaml

Comments

Manivannan Sadhasivam Jan. 9, 2023, 8:18 a.m. UTC | #1 
+ Christoph

Hi Sibi,

On Mon, Jan 09, 2023 at 09:18:37AM +0530, Sibi Sankar wrote:
> This reverts commit fc156629b23a21181e473e60341e3a78af25a1d4.
> 
> The memory region allocated using dma_alloc_attr with no kernel mapping
> attribute set would still be a part of the linear kernel map. Hence as a
> precursor to using reserved memory for modem metadata region, revert back
> to the simpler way of dynamic memory allocation.
> 
> Suggested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>

Christoph already submitted a patch that reverts fc156629b23a:
https://lore.kernel.org/linux-arm-msm/20221223092703.61927-2-hch@lst.de/

Thanks,
Mani

> ---
>  drivers/remoteproc/qcom_q6v5_mss.c | 38 +++++-------------------------
>  1 file changed, 6 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index 2f4027664a0e..e2f765f87ec9 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -10,7 +10,6 @@
>  #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>
> @@ -961,52 +960,27 @@ 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 | DMA_ATTR_NO_KERNEL_MAPPING;
> -	unsigned long flags = VM_DMA_COHERENT | VM_FLUSH_RESET_PERMS;
> -	struct page **pages;
> -	struct page *page;
> +	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
>  	dma_addr_t phys;
>  	void *metadata;
>  	int mdata_perm;
>  	int xferop_ret;
>  	size_t size;
> -	void *vaddr;
> -	int count;
> +	void *ptr;
>  	int ret;
> -	int i;
>  
>  	metadata = qcom_mdt_read_metadata(fw, &size, fw_name, qproc->dev);
>  	if (IS_ERR(metadata))
>  		return PTR_ERR(metadata);
>  
> -	page = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
> -	if (!page) {
> +	ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
> +	if (!ptr) {
>  		kfree(metadata);
>  		dev_err(qproc->dev, "failed to allocate mdt buffer\n");
>  		return -ENOMEM;
>  	}
>  
> -	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);
> +	memcpy(ptr, metadata, size);
>  
>  	/* Hypervisor mapping to access metadata by modem */
>  	mdata_perm = BIT(QCOM_SCM_VMID_HLOS);
> @@ -1036,7 +1010,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, page, phys, dma_attrs);
> +	dma_free_attrs(qproc->dev, size, ptr, phys, dma_attrs);
>  	kfree(metadata);
>  
>  	return ret < 0 ? ret : 0;
> -- 
> 2.17.1
>
Manivannan Sadhasivam Jan. 9, 2023, 8:32 a.m. UTC | #2 
On Mon, Jan 09, 2023 at 09:18:38AM +0530, Sibi Sankar wrote:
> Any access to the dynamically allocated metadata region by the application
> processor after assigning it to the remote Q6 will result in a XPU
> violation. Fix this by replacing the dynamically allocated memory region
> with a no-map carveout and unmap the modem metadata memory region before
> passing control to the remote Q6.
> 
> Reported-and-tested-by: Amit Pundir <amit.pundir@linaro.org>
> Fixes: 6c5a9dc2481b ("remoteproc: qcom: Make secure world call for mem ownership switch")
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---
> 
> v2:
>  * Revert no_kernel_mapping [Mani/Robin]
> 
>  drivers/remoteproc/qcom_q6v5_mss.c | 48 ++++++++++++++++++++++++++----
>  1 file changed, 42 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index e2f765f87ec9..b7a158751cef 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -215,6 +215,7 @@ struct q6v5 {
>  	size_t mba_size;
>  	size_t dp_size;
>  
> +	phys_addr_t mdata_phys;
>  	phys_addr_t mpss_phys;
>  	phys_addr_t mpss_reloc;
>  	size_t mpss_size;
> @@ -973,15 +974,29 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
>  	if (IS_ERR(metadata))
>  		return PTR_ERR(metadata);
>  
> -	ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
> -	if (!ptr) {
> -		kfree(metadata);
> -		dev_err(qproc->dev, "failed to allocate mdt buffer\n");
> -		return -ENOMEM;
> +	if (qproc->mdata_phys) {
> +		phys = qproc->mdata_phys;
> +		ptr = memremap(qproc->mdata_phys, size, MEMREMAP_WC);
> +		if (!ptr) {
> +			dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n",
> +				&qproc->mdata_phys, size);
> +			ret = -EBUSY;
> +			goto free_dma_attrs;

There is no memory to free at this point.

Thanks,
Mani

> +		}
> +	} else {
> +		ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
> +		if (!ptr) {
> +			kfree(metadata);
> +			dev_err(qproc->dev, "failed to allocate mdt buffer\n");
> +			return -ENOMEM;
> +		}
>  	}
>  
>  	memcpy(ptr, metadata, size);
>  
> +	if (qproc->mdata_phys)
> +		memunmap(ptr);
> +
>  	/* Hypervisor mapping to access metadata by modem */
>  	mdata_perm = BIT(QCOM_SCM_VMID_HLOS);
>  	ret = q6v5_xfer_mem_ownership(qproc, &mdata_perm, false, true,
> @@ -1010,7 +1025,8 @@ 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);
> +	if (!qproc->mdata_phys)
> +		dma_free_attrs(qproc->dev, size, ptr, phys, dma_attrs);
>  	kfree(metadata);
>  
>  	return ret < 0 ? ret : 0;
> @@ -1893,6 +1909,26 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc)
>  	qproc->mpss_phys = qproc->mpss_reloc = r.start;
>  	qproc->mpss_size = resource_size(&r);
>  
> +	if (!child) {
> +		node = of_parse_phandle(qproc->dev->of_node, "memory-region", 2);
> +	} else {
> +		child = of_get_child_by_name(qproc->dev->of_node, "metadata");
> +		node = of_parse_phandle(child, "memory-region", 0);
> +		of_node_put(child);
> +	}
> +
> +	if (!node)
> +		return 0;
> +
> +	ret = of_address_to_resource(node, 0, &r);
> +	of_node_put(node);
> +	if (ret) {
> +		dev_err(qproc->dev, "unable to resolve metadata region\n");
> +		return ret;
> +	}
> +
> +	qproc->mdata_phys = r.start;
> +
>  	return 0;
>  }
>  
> -- 
> 2.17.1
>
Sibi Sankar Jan. 9, 2023, 10 a.m. UTC | #3 
Hey Mani,
Thanks for taking time to review the series.

On 1/9/23 13:48, Manivannan Sadhasivam wrote:
> + Christoph
> 
> Hi Sibi,
> 
> On Mon, Jan 09, 2023 at 09:18:37AM +0530, Sibi Sankar wrote:
>> This reverts commit fc156629b23a21181e473e60341e3a78af25a1d4.
>>
>> The memory region allocated using dma_alloc_attr with no kernel mapping
>> attribute set would still be a part of the linear kernel map. Hence as a
>> precursor to using reserved memory for modem metadata region, revert back
>> to the simpler way of dynamic memory allocation.
>>
>> Suggested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> 
> Christoph already submitted a patch that reverts fc156629b23a:
> https://lore.kernel.org/linux-arm-msm/20221223092703.61927-2-hch@lst.de/

Having ^^ revert as part of the this series makes more sense. I'll
just replace my patch with ^^ in the next re-spin.

> 
> Thanks,
> Mani
> 
>> ---
>>   drivers/remoteproc/qcom_q6v5_mss.c | 38 +++++-------------------------
>>   1 file changed, 6 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
>> index 2f4027664a0e..e2f765f87ec9 100644
>> --- a/drivers/remoteproc/qcom_q6v5_mss.c
>> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
>> @@ -10,7 +10,6 @@
>>   #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>
>> @@ -961,52 +960,27 @@ 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 | DMA_ATTR_NO_KERNEL_MAPPING;
>> -	unsigned long flags = VM_DMA_COHERENT | VM_FLUSH_RESET_PERMS;
>> -	struct page **pages;
>> -	struct page *page;
>> +	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
>>   	dma_addr_t phys;
>>   	void *metadata;
>>   	int mdata_perm;
>>   	int xferop_ret;
>>   	size_t size;
>> -	void *vaddr;
>> -	int count;
>> +	void *ptr;
>>   	int ret;
>> -	int i;
>>   
>>   	metadata = qcom_mdt_read_metadata(fw, &size, fw_name, qproc->dev);
>>   	if (IS_ERR(metadata))
>>   		return PTR_ERR(metadata);
>>   
>> -	page = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
>> -	if (!page) {
>> +	ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
>> +	if (!ptr) {
>>   		kfree(metadata);
>>   		dev_err(qproc->dev, "failed to allocate mdt buffer\n");
>>   		return -ENOMEM;
>>   	}
>>   
>> -	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);
>> +	memcpy(ptr, metadata, size);
>>   
>>   	/* Hypervisor mapping to access metadata by modem */
>>   	mdata_perm = BIT(QCOM_SCM_VMID_HLOS);
>> @@ -1036,7 +1010,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, page, phys, dma_attrs);
>> +	dma_free_attrs(qproc->dev, size, ptr, phys, dma_attrs);
>>   	kfree(metadata);
>>   
>>   	return ret < 0 ? ret : 0;
>> -- 
>> 2.17.1
>>
>
Sibi Sankar Jan. 9, 2023, 10:05 a.m. UTC | #4 
Hey Mani,

On 1/9/23 14:02, Manivannan Sadhasivam wrote:
> On Mon, Jan 09, 2023 at 09:18:38AM +0530, Sibi Sankar wrote:
>> Any access to the dynamically allocated metadata region by the application
>> processor after assigning it to the remote Q6 will result in a XPU
>> violation. Fix this by replacing the dynamically allocated memory region
>> with a no-map carveout and unmap the modem metadata memory region before
>> passing control to the remote Q6.
>>
>> Reported-and-tested-by: Amit Pundir <amit.pundir@linaro.org>
>> Fixes: 6c5a9dc2481b ("remoteproc: qcom: Make secure world call for mem ownership switch")
>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>> ---
>>
>> v2:
>>   * Revert no_kernel_mapping [Mani/Robin]
>>
>>   drivers/remoteproc/qcom_q6v5_mss.c | 48 ++++++++++++++++++++++++++----
>>   1 file changed, 42 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
>> index e2f765f87ec9..b7a158751cef 100644
>> --- a/drivers/remoteproc/qcom_q6v5_mss.c
>> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
>> @@ -215,6 +215,7 @@ struct q6v5 {
>>   	size_t mba_size;
>>   	size_t dp_size;
>>   
>> +	phys_addr_t mdata_phys;
>>   	phys_addr_t mpss_phys;
>>   	phys_addr_t mpss_reloc;
>>   	size_t mpss_size;
>> @@ -973,15 +974,29 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
>>   	if (IS_ERR(metadata))
>>   		return PTR_ERR(metadata);
>>   
>> -	ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
>> -	if (!ptr) {
>> -		kfree(metadata);
>> -		dev_err(qproc->dev, "failed to allocate mdt buffer\n");
>> -		return -ENOMEM;
>> +	if (qproc->mdata_phys) {
>> +		phys = qproc->mdata_phys;
>> +		ptr = memremap(qproc->mdata_phys, size, MEMREMAP_WC);
>> +		if (!ptr) {
>> +			dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n",
>> +				&qproc->mdata_phys, size);
>> +			ret = -EBUSY;
>> +			goto free_dma_attrs;
> 
> There is no memory to free at this point.

we would just free the metadata in the no-map carveout scenario since
mdata_phys wouldn't be NULL. I can do a kfree(metadata) directly from
this branch and return as well if you think it makes things more
readable.

> 
> Thanks,
> Mani
> 
>> +		}
>> +	} else {
>> +		ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
>> +		if (!ptr) {
>> +			kfree(metadata);
>> +			dev_err(qproc->dev, "failed to allocate mdt buffer\n");
>> +			return -ENOMEM;
>> +		}
>>   	}
>>   
>>   	memcpy(ptr, metadata, size);
>>   
>> +	if (qproc->mdata_phys)
>> +		memunmap(ptr);
>> +
>>   	/* Hypervisor mapping to access metadata by modem */
>>   	mdata_perm = BIT(QCOM_SCM_VMID_HLOS);
>>   	ret = q6v5_xfer_mem_ownership(qproc, &mdata_perm, false, true,
>> @@ -1010,7 +1025,8 @@ 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);
>> +	if (!qproc->mdata_phys)
>> +		dma_free_attrs(qproc->dev, size, ptr, phys, dma_attrs);
>>   	kfree(metadata);
>>   
>>   	return ret < 0 ? ret : 0;
>> @@ -1893,6 +1909,26 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc)
>>   	qproc->mpss_phys = qproc->mpss_reloc = r.start;
>>   	qproc->mpss_size = resource_size(&r);
>>   
>> +	if (!child) {
>> +		node = of_parse_phandle(qproc->dev->of_node, "memory-region", 2);
>> +	} else {
>> +		child = of_get_child_by_name(qproc->dev->of_node, "metadata");
>> +		node = of_parse_phandle(child, "memory-region", 0);
>> +		of_node_put(child);
>> +	}
>> +
>> +	if (!node)
>> +		return 0;
>> +
>> +	ret = of_address_to_resource(node, 0, &r);
>> +	of_node_put(node);
>> +	if (ret) {
>> +		dev_err(qproc->dev, "unable to resolve metadata region\n");
>> +		return ret;
>> +	}
>> +
>> +	qproc->mdata_phys = r.start;
>> +
>>   	return 0;
>>   }
>>   
>> -- 
>> 2.17.1
>>
>
Manivannan Sadhasivam Jan. 11, 2023, 11:23 a.m. UTC | #5 
On Mon, Jan 09, 2023 at 03:35:31PM +0530, Sibi Sankar wrote:
> Hey Mani,
> 
> On 1/9/23 14:02, Manivannan Sadhasivam wrote:
> > On Mon, Jan 09, 2023 at 09:18:38AM +0530, Sibi Sankar wrote:
> > > Any access to the dynamically allocated metadata region by the application
> > > processor after assigning it to the remote Q6 will result in a XPU
> > > violation. Fix this by replacing the dynamically allocated memory region
> > > with a no-map carveout and unmap the modem metadata memory region before
> > > passing control to the remote Q6.
> > > 
> > > Reported-and-tested-by: Amit Pundir <amit.pundir@linaro.org>
> > > Fixes: 6c5a9dc2481b ("remoteproc: qcom: Make secure world call for mem ownership switch")
> > > Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> > > ---
> > > 
> > > v2:
> > >   * Revert no_kernel_mapping [Mani/Robin]
> > > 
> > >   drivers/remoteproc/qcom_q6v5_mss.c | 48 ++++++++++++++++++++++++++----
> > >   1 file changed, 42 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> > > index e2f765f87ec9..b7a158751cef 100644
> > > --- a/drivers/remoteproc/qcom_q6v5_mss.c
> > > +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> > > @@ -215,6 +215,7 @@ struct q6v5 {
> > >   	size_t mba_size;
> > >   	size_t dp_size;
> > > +	phys_addr_t mdata_phys;
> > >   	phys_addr_t mpss_phys;
> > >   	phys_addr_t mpss_reloc;
> > >   	size_t mpss_size;
> > > @@ -973,15 +974,29 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
> > >   	if (IS_ERR(metadata))
> > >   		return PTR_ERR(metadata);
> > > -	ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
> > > -	if (!ptr) {
> > > -		kfree(metadata);
> > > -		dev_err(qproc->dev, "failed to allocate mdt buffer\n");
> > > -		return -ENOMEM;
> > > +	if (qproc->mdata_phys) {
> > > +		phys = qproc->mdata_phys;
> > > +		ptr = memremap(qproc->mdata_phys, size, MEMREMAP_WC);
> > > +		if (!ptr) {
> > > +			dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n",
> > > +				&qproc->mdata_phys, size);
> > > +			ret = -EBUSY;
> > > +			goto free_dma_attrs;
> > 
> > There is no memory to free at this point.
> 
> we would just free the metadata in the no-map carveout scenario since
> mdata_phys wouldn't be NULL. I can do a kfree(metadata) directly from
> this branch and return as well if you think it makes things more
> readable.
> 

Oops, I missed that. But yeah it is confusing too with the current way of
freeing metadata. I'd suggest using a separate label instead.

Thanks,
Mani

> > 
> > Thanks,
> > Mani
> > 
> > > +		}
> > > +	} else {
> > > +		ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
> > > +		if (!ptr) {
> > > +			kfree(metadata);
> > > +			dev_err(qproc->dev, "failed to allocate mdt buffer\n");
> > > +			return -ENOMEM;
> > > +		}
> > >   	}
> > >   	memcpy(ptr, metadata, size);
> > > +	if (qproc->mdata_phys)
> > > +		memunmap(ptr);
> > > +
> > >   	/* Hypervisor mapping to access metadata by modem */
> > >   	mdata_perm = BIT(QCOM_SCM_VMID_HLOS);
> > >   	ret = q6v5_xfer_mem_ownership(qproc, &mdata_perm, false, true,
> > > @@ -1010,7 +1025,8 @@ 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);
> > > +	if (!qproc->mdata_phys)
> > > +		dma_free_attrs(qproc->dev, size, ptr, phys, dma_attrs);
> > >   	kfree(metadata);
> > >   	return ret < 0 ? ret : 0;
> > > @@ -1893,6 +1909,26 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc)
> > >   	qproc->mpss_phys = qproc->mpss_reloc = r.start;
> > >   	qproc->mpss_size = resource_size(&r);
> > > +	if (!child) {
> > > +		node = of_parse_phandle(qproc->dev->of_node, "memory-region", 2);
> > > +	} else {
> > > +		child = of_get_child_by_name(qproc->dev->of_node, "metadata");
> > > +		node = of_parse_phandle(child, "memory-region", 0);
> > > +		of_node_put(child);
> > > +	}
> > > +
> > > +	if (!node)
> > > +		return 0;
> > > +
> > > +	ret = of_address_to_resource(node, 0, &r);
> > > +	of_node_put(node);
> > > +	if (ret) {
> > > +		dev_err(qproc->dev, "unable to resolve metadata region\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	qproc->mdata_phys = r.start;
> > > +
> > >   	return 0;
> > >   }
> > > -- 
> > > 2.17.1
> > > 
> >
Manivannan Sadhasivam Jan. 11, 2023, 11:24 a.m. UTC | #6 
On Mon, Jan 09, 2023 at 03:30:22PM +0530, Sibi Sankar wrote:
> Hey Mani,
> Thanks for taking time to review the series.
> 
> On 1/9/23 13:48, Manivannan Sadhasivam wrote:
> > + Christoph
> > 
> > Hi Sibi,
> > 
> > On Mon, Jan 09, 2023 at 09:18:37AM +0530, Sibi Sankar wrote:
> > > This reverts commit fc156629b23a21181e473e60341e3a78af25a1d4.
> > > 
> > > The memory region allocated using dma_alloc_attr with no kernel mapping
> > > attribute set would still be a part of the linear kernel map. Hence as a
> > > precursor to using reserved memory for modem metadata region, revert back
> > > to the simpler way of dynamic memory allocation.
> > > 
> > > Suggested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> > 
> > Christoph already submitted a patch that reverts fc156629b23a:
> > https://lore.kernel.org/linux-arm-msm/20221223092703.61927-2-hch@lst.de/
> 
> Having ^^ revert as part of the this series makes more sense. I'll
> just replace my patch with ^^ in the next re-spin.
> 

That makes sense to me.

Thanks,
Mani

> > 
> > Thanks,
> > Mani
> > 
> > > ---
> > >   drivers/remoteproc/qcom_q6v5_mss.c | 38 +++++-------------------------
> > >   1 file changed, 6 insertions(+), 32 deletions(-)
> > > 
> > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> > > index 2f4027664a0e..e2f765f87ec9 100644
> > > --- a/drivers/remoteproc/qcom_q6v5_mss.c
> > > +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> > > @@ -10,7 +10,6 @@
> > >   #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>
> > > @@ -961,52 +960,27 @@ 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 | DMA_ATTR_NO_KERNEL_MAPPING;
> > > -	unsigned long flags = VM_DMA_COHERENT | VM_FLUSH_RESET_PERMS;
> > > -	struct page **pages;
> > > -	struct page *page;
> > > +	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
> > >   	dma_addr_t phys;
> > >   	void *metadata;
> > >   	int mdata_perm;
> > >   	int xferop_ret;
> > >   	size_t size;
> > > -	void *vaddr;
> > > -	int count;
> > > +	void *ptr;
> > >   	int ret;
> > > -	int i;
> > >   	metadata = qcom_mdt_read_metadata(fw, &size, fw_name, qproc->dev);
> > >   	if (IS_ERR(metadata))
> > >   		return PTR_ERR(metadata);
> > > -	page = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
> > > -	if (!page) {
> > > +	ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
> > > +	if (!ptr) {
> > >   		kfree(metadata);
> > >   		dev_err(qproc->dev, "failed to allocate mdt buffer\n");
> > >   		return -ENOMEM;
> > >   	}
> > > -	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);
> > > +	memcpy(ptr, metadata, size);
> > >   	/* Hypervisor mapping to access metadata by modem */
> > >   	mdata_perm = BIT(QCOM_SCM_VMID_HLOS);
> > > @@ -1036,7 +1010,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, page, phys, dma_attrs);
> > > +	dma_free_attrs(qproc->dev, size, ptr, phys, dma_attrs);
> > >   	kfree(metadata);
> > >   	return ret < 0 ? ret : 0;
> > > -- 
> > > 2.17.1
> > > 
> >