Re: [PATCH 6/9] iommu: qcom: initialize secure page table

[Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

On 03/01/2017 09:42 AM, Rob Clark wrote:
> From: Stanimir Varbanov <stanimir.varbanov-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>
> This bassicaly get the secure page table size, allocate memory

s/bassicaly/basically/
s/get/gets/
s/allocate/allocates/

Heh, bass.

> and return back the physical address to the trusted zone.

s/return/returns/

>
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Rob Clark <robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/iommu/qcom_iommu.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
>
> diff --git a/drivers/iommu/qcom_iommu.c b/drivers/iommu/qcom_iommu.c
> index 5d3bb63..082ece7 100644
> --- a/drivers/iommu/qcom_iommu.c
> +++ b/drivers/iommu/qcom_iommu.c
> @@ -608,6 +608,51 @@ static void qcom_iommu_disable_clocks(struct qcom_iommu_dev *qcom_iommu)
>  	clk_disable_unprepare(qcom_iommu->iface_clk);
>  }
>  
> +static int qcom_iommu_sec_ptbl_init(struct device *dev)
> +{
> +	size_t psize = 0;
> +	unsigned int spare = 0;

Why not just pass 0 where it's used?

> +	void *cpu_addr;
> +	dma_addr_t paddr;
> +	unsigned long attrs;
> +	static bool allocated = false;
> +	int ret;
> +
> +	if (allocated)
> +		return 0;
> +
> +	ret = qcom_scm_iommu_secure_ptbl_size(spare, &psize);
> +	if (ret) {
> +		dev_err(dev, "failed to get iommu secure pgtable size (%d)\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	dev_info(dev, "iommu sec: pgtable size: %zu\n", psize);

Debug?

> +
> +	attrs = DMA_ATTR_NO_KERNEL_MAPPING;
> +
> +	cpu_addr = dma_alloc_attrs(dev, psize, &paddr, GFP_KERNEL, attrs);
> +	if (!cpu_addr) {
> +		dev_err(dev, "failed to allocate %zu bytes for pgtable\n",
> +			psize);
> +		return -ENOMEM;
> +	}
> +
> +	ret = qcom_scm_iommu_secure_ptbl_init(paddr, psize, spare);
> +	if (ret) {
> +		dev_err(dev, "failed to init iommu pgtable (%d)\n", ret);
> +		goto free_mem;
> +	}
> +
> +	allocated = true;
> +	return 0;
> +
> +free_mem:
> +	dma_free_attrs(dev, psize, cpu_addr, paddr, attrs);
> +	return ret;
> +}
> +
>  static int qcom_iommu_ctx_probe(struct platform_device *pdev)
>  {
>  	struct qcom_iommu_ctx *ctx;
> @@ -693,6 +738,17 @@ static struct platform_driver qcom_iommu_ctx_driver = {
>  };
>  module_platform_driver(qcom_iommu_ctx_driver);
>  
> +static bool qcom_iommu_has_secure_context(struct qcom_iommu_dev *qcom_iommu)
> +{
> +	struct device_node *child;
> +
> +	for_each_child_of_node(qcom_iommu->dev->of_node, child)
> +		if (of_device_is_compatible(child, "qcom,msm-iommu-v1-sec"))
> +			return true;

Missing an of_node_put(child) here?

> +
> +	return false;
> +}
> +
>  static int qcom_iommu_device_probe(struct platform_device *pdev)
>  {
>  	struct qcom_iommu_dev *qcom_iommu;
> @@ -731,6 +787,14 @@ static int qcom_iommu_device_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  
> +	if (qcom_iommu_has_secure_context(qcom_iommu)) {
> +		ret = qcom_iommu_sec_ptbl_init(dev);

Is dev the same dev that's passed to qcom_iommu_has_secure_context()?
Perhaps we can pass dev to has_secure_context instead to make things
clearer.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project