diff mbox series

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

Message ID 20220527212901.29268-2-konrad.dybcio@somainline.org
State Changes Requested, archived
Headers show
Series [1/6] iommu/qcom: Use the asid read from device-tree if specified | expand

Checks

Context Check Description
robh/checkpatch warning total: 0 errors, 1 warnings, 43 lines checked
robh/patch-applied success

Commit Message

Konrad Dybcio May 27, 2022, 9:28 p.m. UTC 
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>

As specified in this driver, the context banks are 0x1000 apart.
Problem is that sometimes the context number (our asid) does not
match this logic and we end up using the wrong one: this starts
being a problem in the case that we need to send TZ commands
to do anything on a specific context.

For this reason, read the ASID from the DT if the property
"qcom,ctx-num" is present on the IOMMU context node.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org>
---
 .../devicetree/bindings/iommu/qcom,iommu.txt   |  1 +
 drivers/iommu/arm/arm-smmu/qcom_iommu.c        | 18 +++++++++++++++---
 2 files changed, 16 insertions(+), 3 deletions(-)

Comments

Will Deacon May 31, 2022, 3:46 p.m. UTC | #1 
On Fri, May 27, 2022 at 11:28:56PM +0200, Konrad Dybcio wrote:
> From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> 
> As specified in this driver, the context banks are 0x1000 apart.
> Problem is that sometimes the context number (our asid) does not
> match this logic and we end up using the wrong one: this starts
> being a problem in the case that we need to send TZ commands
> to do anything on a specific context.

I don't understand this. The ASID is a software construct, so it shouldn't
matter what we use. If it does matter, then please can you explain why? The
fact that the context banks are 0x1000 apart seems unrelated.

Will
Rob Clark May 31, 2022, 4:15 p.m. UTC | #2 
On Tue, May 31, 2022 at 8:46 AM Will Deacon <will@kernel.org> wrote:
>
> On Fri, May 27, 2022 at 11:28:56PM +0200, Konrad Dybcio wrote:
> > From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> >
> > As specified in this driver, the context banks are 0x1000 apart.
> > Problem is that sometimes the context number (our asid) does not
> > match this logic and we end up using the wrong one: this starts
> > being a problem in the case that we need to send TZ commands
> > to do anything on a specific context.
>
> I don't understand this. The ASID is a software construct, so it shouldn't
> matter what we use. If it does matter, then please can you explain why? The
> fact that the context banks are 0x1000 apart seems unrelated.

I think the connection is that mapping from ctx bank to ASID is 1:1

BR,
-R
Will Deacon May 31, 2022, 4:19 p.m. UTC | #3 
On Tue, May 31, 2022 at 09:15:22AM -0700, Rob Clark wrote:
> On Tue, May 31, 2022 at 8:46 AM Will Deacon <will@kernel.org> wrote:
> >
> > On Fri, May 27, 2022 at 11:28:56PM +0200, Konrad Dybcio wrote:
> > > From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> > >
> > > As specified in this driver, the context banks are 0x1000 apart.
> > > Problem is that sometimes the context number (our asid) does not
> > > match this logic and we end up using the wrong one: this starts
> > > being a problem in the case that we need to send TZ commands
> > > to do anything on a specific context.
> >
> > I don't understand this. The ASID is a software construct, so it shouldn't
> > matter what we use. If it does matter, then please can you explain why? The
> > fact that the context banks are 0x1000 apart seems unrelated.
> 
> I think the connection is that mapping from ctx bank to ASID is 1:1

But in what sense? How is the ASID used beyond a tag in the TLB? The commit
message hints at "TZ commands" being a problem.

I'm not doubting that this is needed to make the thing work, I just don't
understand why.

Will
Rob Clark May 31, 2022, 8:57 p.m. UTC | #4 
On Tue, May 31, 2022 at 9:19 AM Will Deacon <will@kernel.org> wrote:
>
> On Tue, May 31, 2022 at 09:15:22AM -0700, Rob Clark wrote:
> > On Tue, May 31, 2022 at 8:46 AM Will Deacon <will@kernel.org> wrote:
> > >
> > > On Fri, May 27, 2022 at 11:28:56PM +0200, Konrad Dybcio wrote:
> > > > From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> > > >
> > > > As specified in this driver, the context banks are 0x1000 apart.
> > > > Problem is that sometimes the context number (our asid) does not
> > > > match this logic and we end up using the wrong one: this starts
> > > > being a problem in the case that we need to send TZ commands
> > > > to do anything on a specific context.
> > >
> > > I don't understand this. The ASID is a software construct, so it shouldn't
> > > matter what we use. If it does matter, then please can you explain why? The
> > > fact that the context banks are 0x1000 apart seems unrelated.
> >
> > I think the connection is that mapping from ctx bank to ASID is 1:1
>
> But in what sense? How is the ASID used beyond a tag in the TLB? The commit
> message hints at "TZ commands" being a problem.
>
> I'm not doubting that this is needed to make the thing work, I just don't
> understand why.

(disclaimer, it has been quite a while since I've looked at the smmu
setup with earlier tz, ie. things that use qcom_iommu, but from
memory...)

We cannot actually assign the context banks ourselves, so in the dt
bindings the "ASID" is actually the context bank index.  I don't
remember exactly if this was a limitation of the tz interface, or
result of not being able to program the smmu's global registers
ourselves.

BR,
-R
Konrad Dybcio June 3, 2022, 6:03 p.m. UTC | #5 
On 31.05.2022 22:57, Rob Clark wrote:
> On Tue, May 31, 2022 at 9:19 AM Will Deacon <will@kernel.org> wrote:
>>
>> On Tue, May 31, 2022 at 09:15:22AM -0700, Rob Clark wrote:
>>> On Tue, May 31, 2022 at 8:46 AM Will Deacon <will@kernel.org> wrote:
>>>>
>>>> On Fri, May 27, 2022 at 11:28:56PM +0200, Konrad Dybcio wrote:
>>>>> From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
>>>>>
>>>>> As specified in this driver, the context banks are 0x1000 apart.
>>>>> Problem is that sometimes the context number (our asid) does not
>>>>> match this logic and we end up using the wrong one: this starts
>>>>> being a problem in the case that we need to send TZ commands
>>>>> to do anything on a specific context.
>>>>
>>>> I don't understand this. The ASID is a software construct, so it shouldn't
>>>> matter what we use. If it does matter, then please can you explain why? The
>>>> fact that the context banks are 0x1000 apart seems unrelated.
>>>
>>> I think the connection is that mapping from ctx bank to ASID is 1:1
>>
>> But in what sense? How is the ASID used beyond a tag in the TLB? The commit
>> message hints at "TZ commands" being a problem.
>>
>> I'm not doubting that this is needed to make the thing work, I just don't
>> understand why.
> 
> (disclaimer, it has been quite a while since I've looked at the smmu
> setup with earlier tz, ie. things that use qcom_iommu, but from
> memory...)
> 
> We cannot actually assign the context banks ourselves, so in the dt
> bindings the "ASID" is actually the context bank index.
I think so.

  I don't
> remember exactly if this was a limitation of the tz interface, or
> result of not being able to program the smmu's global registers
> ourselves.

As far as I understand, it's the latter, as changing the defaults is not allowed by the security policy on consumer devices.

Qualcomm arbitrarily chose some numbers that may or may have not aligned with their usual index-is-offset-divided-by-0x1000 and hardcoded them in the BSP, and now the secure side (if required, and well, it is..) expects precisely that configuration.


Konrad
AngeloGioacchino Del Regno June 8, 2022, 10:25 a.m. UTC | #6 
Il 03/06/22 20:03, Konrad Dybcio ha scritto:
> 
> 
> On 31.05.2022 22:57, Rob Clark wrote:
>> On Tue, May 31, 2022 at 9:19 AM Will Deacon <will@kernel.org> wrote:
>>>
>>> On Tue, May 31, 2022 at 09:15:22AM -0700, Rob Clark wrote:
>>>> On Tue, May 31, 2022 at 8:46 AM Will Deacon <will@kernel.org> wrote:
>>>>>
>>>>> On Fri, May 27, 2022 at 11:28:56PM +0200, Konrad Dybcio wrote:
>>>>>> From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
>>>>>>
>>>>>> As specified in this driver, the context banks are 0x1000 apart.
>>>>>> Problem is that sometimes the context number (our asid) does not
>>>>>> match this logic and we end up using the wrong one: this starts
>>>>>> being a problem in the case that we need to send TZ commands
>>>>>> to do anything on a specific context.
>>>>>
>>>>> I don't understand this. The ASID is a software construct, so it shouldn't
>>>>> matter what we use. If it does matter, then please can you explain why? The
>>>>> fact that the context banks are 0x1000 apart seems unrelated.
>>>>
>>>> I think the connection is that mapping from ctx bank to ASID is 1:1
>>>
>>> But in what sense? How is the ASID used beyond a tag in the TLB? The commit
>>> message hints at "TZ commands" being a problem.
>>>
>>> I'm not doubting that this is needed to make the thing work, I just don't
>>> understand why.
>>
>> (disclaimer, it has been quite a while since I've looked at the smmu
>> setup with earlier tz, ie. things that use qcom_iommu, but from
>> memory...)
>>
>> We cannot actually assign the context banks ourselves, so in the dt
>> bindings the "ASID" is actually the context bank index.
> I think so.
> 
>    I don't
>> remember exactly if this was a limitation of the tz interface, or
>> result of not being able to program the smmu's global registers
>> ourselves.
> 
> As far as I understand, it's the latter, as changing the defaults is not allowed by the security policy on consumer devices.
> 
> Qualcomm arbitrarily chose some numbers that may or may have not aligned with their usual index-is-offset-divided-by-0x1000 and hardcoded them in the BSP, and now the secure side (if required, and well, it is..) expects precisely that configuration.
> 
> 
> Konrad
> 

I can confirm that it's the latter, as described by Konrad.
The inability of programming the global registers from Linux is due to the
hypervisor disallowing that (in different ways depending on the SoC's firmware
but with the same outcome: AP reset by HYP).

Cheers,
Angelo
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/iommu/qcom,iommu.txt b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
index 059139abce35..ba0b77889f02 100644
--- a/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
+++ b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
@@ -46,6 +46,7 @@  to non-secure vs secure interrupt line.
                      for routing of context bank irq's to secure vs non-
                      secure lines.  (Ie. if the iommu contains secure
                      context banks)
+- qcom,ctx-num     : The number associated to the context bank
 
 
 ** Examples:
diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 4c077c38fbd6..1728d4d7fe25 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -566,7 +566,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;
 	}
@@ -654,7 +655,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:
@@ -662,7 +664,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)