diff mbox series

[v3,2/6] iommu/qcom: Use the asid read from device-tree if specified

Message ID 20221115101122.155440-3-angelogioacchino.delregno@collabora.com (mailing list archive)
State Not Applicable
Headers show
Series Add support for Qualcomm's legacy IOMMU v2 | expand

Commit Message

AngeloGioacchino Del Regno Nov. 15, 2022, 10:11 a.m. UTC 
As specified in this driver, the context banks are 0x1000 apart but
on some SoCs the context number does not necessarily match this
logic, hence we end up using the wrong ASID: keeping in mind that
this IOMMU implementation relies heavily on SCM (TZ) calls, it is
mandatory that we communicate the right context number.

Since this is all about how context banks are mapped in firmware,
which may be board dependent (as a different firmware version may
eventually change the expected context bank numbers), introduce a
new property "qcom,ctx-num": when found, the ASID will be forced
as read from the devicetree.

When "qcom,ctx-num" is not found, this driver retains the previous
behavior as to avoid breaking older devicetrees or systems that do
not require forcing ASID numbers.

Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
[Marijn: Rebased over next-20221111]
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/iommu/arm/arm-smmu/qcom_iommu.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

Comments

Dmitry Baryshkov March 7, 2023, 4:44 p.m. UTC | #1 
On 15/11/2022 12:11, AngeloGioacchino Del Regno wrote:
> As specified in this driver, the context banks are 0x1000 apart but
> on some SoCs the context number does not necessarily match this
> logic, hence we end up using the wrong ASID: keeping in mind that
> this IOMMU implementation relies heavily on SCM (TZ) calls, it is
> mandatory that we communicate the right context number.
> 
> Since this is all about how context banks are mapped in firmware,
> which may be board dependent (as a different firmware version may
> eventually change the expected context bank numbers), introduce a
> new property "qcom,ctx-num": when found, the ASID will be forced
> as read from the devicetree.
> 
> When "qcom,ctx-num" is not found, this driver retains the previous
> behavior as to avoid breaking older devicetrees or systems that do
> not require forcing ASID numbers.
> 
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> [Marijn: Rebased over next-20221111]
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>   drivers/iommu/arm/arm-smmu/qcom_iommu.c | 18 +++++++++++++++---
>   1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> index bfd7b51eb5db..491a8093f3d6 100644
> --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> @@ -551,7 +551,8 @@ static int qcom_iommu_of_xlate(struct device *dev, struct of_phandle_args *args)
>   	 * index into qcom_iommu->ctxs:
>   	 */
>   	if (WARN_ON(asid < 1) ||
> -	    WARN_ON(asid > qcom_iommu->num_ctxs)) {
> +	    WARN_ON(asid > qcom_iommu->num_ctxs) ||
> +	    WARN_ON(qcom_iommu->ctxs[asid - 1] == NULL)) {

Separate change in my opinion. Please split it to a separate patch with 
proper Fixes: tag.

>   		put_device(&iommu_pdev->dev);
>   		return -EINVAL;
>   	}
> @@ -638,7 +639,8 @@ static int qcom_iommu_sec_ptbl_init(struct device *dev)
>   
>   static int get_asid(const struct device_node *np)
>   {
> -	u32 reg;
> +	u32 reg, val;
> +	int asid;
>   
>   	/* read the "reg" property directly to get the relative address
>   	 * of the context bank, and calculate the asid from that:
> @@ -646,7 +648,17 @@ static int get_asid(const struct device_node *np)
>   	if (of_property_read_u32_index(np, "reg", 0, &reg))
>   		return -ENODEV;
>   
> -	return reg / 0x1000;      /* context banks are 0x1000 apart */
> +	/*
> +	 * Context banks are 0x1000 apart but, in some cases, the ASID
> +	 * number doesn't match to this logic and needs to be passed
> +	 * from the DT configuration explicitly.
> +	 */
> +	if (of_property_read_u32(np, "qcom,ctx-num", &val))
> +		asid = reg / 0x1000;
> +	else
> +		asid = val;

As a matter of preference (and logic) I'd have written that as:

if (!of_property_read(np, "qcom,ctx-num", &val))
     asid = val;
else
     asid = reg / 0x1000;

LGTM otherwise

> +
> +	return asid;
>   }
>   
>   static int qcom_iommu_ctx_probe(struct platform_device *pdev)
Konrad Dybcio March 7, 2023, 4:47 p.m. UTC | #2 
On 7.03.2023 17:44, Dmitry Baryshkov wrote:
> On 15/11/2022 12:11, AngeloGioacchino Del Regno wrote:
>> As specified in this driver, the context banks are 0x1000 apart but
>> on some SoCs the context number does not necessarily match this
>> logic, hence we end up using the wrong ASID: keeping in mind that
>> this IOMMU implementation relies heavily on SCM (TZ) calls, it is
>> mandatory that we communicate the right context number.
>>
>> Since this is all about how context banks are mapped in firmware,
>> which may be board dependent (as a different firmware version may
>> eventually change the expected context bank numbers), introduce a
>> new property "qcom,ctx-num": when found, the ASID will be forced
>> as read from the devicetree.
>>
>> When "qcom,ctx-num" is not found, this driver retains the previous
>> behavior as to avoid breaking older devicetrees or systems that do
>> not require forcing ASID numbers.
>>
>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
>> [Marijn: Rebased over next-20221111]
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/iommu/arm/arm-smmu/qcom_iommu.c | 18 +++++++++++++++---
>>   1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
>> index bfd7b51eb5db..491a8093f3d6 100644
>> --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
>> +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
>> @@ -551,7 +551,8 @@ static int qcom_iommu_of_xlate(struct device *dev, struct of_phandle_args *args)
>>        * index into qcom_iommu->ctxs:
>>        */
>>       if (WARN_ON(asid < 1) ||
>> -        WARN_ON(asid > qcom_iommu->num_ctxs)) {
>> +        WARN_ON(asid > qcom_iommu->num_ctxs) ||
>> +        WARN_ON(qcom_iommu->ctxs[asid - 1] == NULL)) {
> 
> Separate change in my opinion. Please split it to a separate patch with proper Fixes: tag.
> 
>>           put_device(&iommu_pdev->dev);
>>           return -EINVAL;
>>       }
>> @@ -638,7 +639,8 @@ static int qcom_iommu_sec_ptbl_init(struct device *dev)
>>     static int get_asid(const struct device_node *np)
>>   {
>> -    u32 reg;
>> +    u32 reg, val;
>> +    int asid;
>>         /* read the "reg" property directly to get the relative address
>>        * of the context bank, and calculate the asid from that:
>> @@ -646,7 +648,17 @@ static int get_asid(const struct device_node *np)
>>       if (of_property_read_u32_index(np, "reg", 0, &reg))
Use platform_get_resource(pdev, IORESOURCE_MEM, 0); reg = res->start
or something like this, the current approach relies on #address-cells = <1>

Konrad
>>           return -ENODEV;
>>   -    return reg / 0x1000;      /* context banks are 0x1000 apart */
>> +    /*
>> +     * Context banks are 0x1000 apart but, in some cases, the ASID
>> +     * number doesn't match to this logic and needs to be passed
>> +     * from the DT configuration explicitly.
>> +     */
>> +    if (of_property_read_u32(np, "qcom,ctx-num", &val))
>> +        asid = reg / 0x1000;
>> +    else
>> +        asid = val;
> 
> As a matter of preference (and logic) I'd have written that as:
> 
> if (!of_property_read(np, "qcom,ctx-num", &val))
>     asid = val;
> else
>     asid = reg / 0x1000;
> 
> LGTM otherwise
> 
>> +
>> +    return asid;
>>   }
>>     static int qcom_iommu_ctx_probe(struct platform_device *pdev)
>
AngeloGioacchino Del Regno June 20, 2023, 9:43 a.m. UTC | #3 
Il 07/03/23 17:44, Dmitry Baryshkov ha scritto:
> On 15/11/2022 12:11, AngeloGioacchino Del Regno wrote:
>> As specified in this driver, the context banks are 0x1000 apart but
>> on some SoCs the context number does not necessarily match this
>> logic, hence we end up using the wrong ASID: keeping in mind that
>> this IOMMU implementation relies heavily on SCM (TZ) calls, it is
>> mandatory that we communicate the right context number.
>>
>> Since this is all about how context banks are mapped in firmware,
>> which may be board dependent (as a different firmware version may
>> eventually change the expected context bank numbers), introduce a
>> new property "qcom,ctx-num": when found, the ASID will be forced
>> as read from the devicetree.
>>
>> When "qcom,ctx-num" is not found, this driver retains the previous
>> behavior as to avoid breaking older devicetrees or systems that do
>> not require forcing ASID numbers.
>>
>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
>> [Marijn: Rebased over next-20221111]
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/iommu/arm/arm-smmu/qcom_iommu.c | 18 +++++++++++++++---
>>   1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c 
>> b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
>> index bfd7b51eb5db..491a8093f3d6 100644
>> --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
>> +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
>> @@ -551,7 +551,8 @@ static int qcom_iommu_of_xlate(struct device *dev, struct 
>> of_phandle_args *args)
>>        * index into qcom_iommu->ctxs:
>>        */
>>       if (WARN_ON(asid < 1) ||
>> -        WARN_ON(asid > qcom_iommu->num_ctxs)) {
>> +        WARN_ON(asid > qcom_iommu->num_ctxs) ||
>> +        WARN_ON(qcom_iommu->ctxs[asid - 1] == NULL)) {
> 
> Separate change in my opinion. Please split it to a separate patch with proper 
> Fixes: tag.
> 

This is of_xlate: the array entry at [asid - 1] is always initialized before
the introduction of `qcom,ctx-num`, so this is not a separate change and it
does not require a Fixes tag, as it is not fixing a previous behavior, but
accounting for a new one.

>>           put_device(&iommu_pdev->dev);
>>           return -EINVAL;
>>       }
>> @@ -638,7 +639,8 @@ static int qcom_iommu_sec_ptbl_init(struct device *dev)
>>   static int get_asid(const struct device_node *np)
>>   {
>> -    u32 reg;
>> +    u32 reg, val;
>> +    int asid;
>>       /* read the "reg" property directly to get the relative address
>>        * of the context bank, and calculate the asid from that:
>> @@ -646,7 +648,17 @@ static int get_asid(const struct device_node *np)
>>       if (of_property_read_u32_index(np, "reg", 0, &reg))
>>           return -ENODEV;
>> -    return reg / 0x1000;      /* context banks are 0x1000 apart */
>> +    /*
>> +     * Context banks are 0x1000 apart but, in some cases, the ASID
>> +     * number doesn't match to this logic and needs to be passed
>> +     * from the DT configuration explicitly.
>> +     */
>> +    if (of_property_read_u32(np, "qcom,ctx-num", &val))
>> +        asid = reg / 0x1000;
>> +    else
>> +        asid = val;
> 
> As a matter of preference (and logic) I'd have written that as:
> 
> if (!of_property_read(np, "qcom,ctx-num", &val))
>      asid = val;
> else
>      asid = reg / 0x1000;
> 
> LGTM otherwise
> 

Will do!

Thanks,
Angelo

P.S.: Sorry for the very late reply.

>> +
>> +    return asid;
>>   }
>>   static int qcom_iommu_ctx_probe(struct platform_device *pdev)
>
diff mbox series

Patch

diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index bfd7b51eb5db..491a8093f3d6 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -551,7 +551,8 @@  static int qcom_iommu_of_xlate(struct device *dev, struct of_phandle_args *args)
 	 * index into qcom_iommu->ctxs:
 	 */
 	if (WARN_ON(asid < 1) ||
-	    WARN_ON(asid > qcom_iommu->num_ctxs)) {
+	    WARN_ON(asid > qcom_iommu->num_ctxs) ||
+	    WARN_ON(qcom_iommu->ctxs[asid - 1] == NULL)) {
 		put_device(&iommu_pdev->dev);
 		return -EINVAL;
 	}
@@ -638,7 +639,8 @@  static int qcom_iommu_sec_ptbl_init(struct device *dev)
 
 static int get_asid(const struct device_node *np)
 {
-	u32 reg;
+	u32 reg, val;
+	int asid;
 
 	/* read the "reg" property directly to get the relative address
 	 * of the context bank, and calculate the asid from that:
@@ -646,7 +648,17 @@  static int get_asid(const struct device_node *np)
 	if (of_property_read_u32_index(np, "reg", 0, &reg))
 		return -ENODEV;
 
-	return reg / 0x1000;      /* context banks are 0x1000 apart */
+	/*
+	 * Context banks are 0x1000 apart but, in some cases, the ASID
+	 * number doesn't match to this logic and needs to be passed
+	 * from the DT configuration explicitly.
+	 */
+	if (of_property_read_u32(np, "qcom,ctx-num", &val))
+		asid = reg / 0x1000;
+	else
+		asid = val;
+
+	return asid;
 }
 
 static int qcom_iommu_ctx_probe(struct platform_device *pdev)