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

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

[Konrad Dybcio]

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

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

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

[Will Deacon]

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

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

[Rob Clark]

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

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

[Will Deacon]

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

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

[Rob Clark]

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

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

[Konrad Dybcio]

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

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

[AngeloGioacchino Del Regno]

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

[PATCH 2/6] iommu/qcom: Write TCR before TTBRs to fix ASID access behavior

[Konrad Dybcio]

From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>

As also stated in the arm-smmu driver, we must write the TCR before
writing the TTBRs, since the TCR determines the access behavior of
some fields.

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>
---
 drivers/iommu/arm/arm-smmu/qcom_iommu.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 1728d4d7fe25..75f353866c40 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -273,18 +273,18 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
 			ctx->secure_init = true;
 		}
 
-		/* TTBRs */
-		iommu_writeq(ctx, ARM_SMMU_CB_TTBR0,
-				pgtbl_cfg.arm_lpae_s1_cfg.ttbr |
-				FIELD_PREP(ARM_SMMU_TTBRn_ASID, ctx->asid));
-		iommu_writeq(ctx, ARM_SMMU_CB_TTBR1, 0);
-
 		/* TCR */
 		iommu_writel(ctx, ARM_SMMU_CB_TCR2,
 				arm_smmu_lpae_tcr2(&pgtbl_cfg));
 		iommu_writel(ctx, ARM_SMMU_CB_TCR,
 			     arm_smmu_lpae_tcr(&pgtbl_cfg) | ARM_SMMU_TCR_EAE);
 
+		/* TTBRs */
+		iommu_writeq(ctx, ARM_SMMU_CB_TTBR0,
+				pgtbl_cfg.arm_lpae_s1_cfg.ttbr |
+				FIELD_PREP(ARM_SMMU_TTBRn_ASID, ctx->asid));
+		iommu_writeq(ctx, ARM_SMMU_CB_TTBR1, 0);
+
 		/* MAIRs (stage-1 only) */
 		iommu_writel(ctx, ARM_SMMU_CB_S1_MAIR0,
 				pgtbl_cfg.arm_lpae_s1_cfg.mair);
-- 
2.36.1

Re: [PATCH 2/6] iommu/qcom: Write TCR before TTBRs to fix ASID access behavior

[Will Deacon]

On Fri, May 27, 2022 at 11:28:57PM +0200, Konrad Dybcio wrote:
> From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> 
> As also stated in the arm-smmu driver, we must write the TCR before
> writing the TTBRs, since the TCR determines the access behavior of
> some fields.

Where is this stated in the arm-smmu driver?

> 
> 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>
> ---
>  drivers/iommu/arm/arm-smmu/qcom_iommu.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> index 1728d4d7fe25..75f353866c40 100644
> --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> @@ -273,18 +273,18 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
>  			ctx->secure_init = true;
>  		}
>  
> -		/* TTBRs */
> -		iommu_writeq(ctx, ARM_SMMU_CB_TTBR0,
> -				pgtbl_cfg.arm_lpae_s1_cfg.ttbr |
> -				FIELD_PREP(ARM_SMMU_TTBRn_ASID, ctx->asid));
> -		iommu_writeq(ctx, ARM_SMMU_CB_TTBR1, 0);
> -
>  		/* TCR */
>  		iommu_writel(ctx, ARM_SMMU_CB_TCR2,
>  				arm_smmu_lpae_tcr2(&pgtbl_cfg));
>  		iommu_writel(ctx, ARM_SMMU_CB_TCR,
>  			     arm_smmu_lpae_tcr(&pgtbl_cfg) | ARM_SMMU_TCR_EAE);
>  
> +		/* TTBRs */
> +		iommu_writeq(ctx, ARM_SMMU_CB_TTBR0,
> +				pgtbl_cfg.arm_lpae_s1_cfg.ttbr |
> +				FIELD_PREP(ARM_SMMU_TTBRn_ASID, ctx->asid));
> +		iommu_writeq(ctx, ARM_SMMU_CB_TTBR1, 0);

I'd have thought that SCTLR.M would be clear here, so it shouldn't matter
what order we write these in.

Will

Re: [PATCH 2/6] iommu/qcom: Write TCR before TTBRs to fix ASID access behavior

[Robin Murphy]

On 2022-05-31 16:55, Will Deacon wrote:
> On Fri, May 27, 2022 at 11:28:57PM +0200, Konrad Dybcio wrote:
>> From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
>>
>> As also stated in the arm-smmu driver, we must write the TCR before
>> writing the TTBRs, since the TCR determines the access behavior of
>> some fields.
> 
> Where is this stated in the arm-smmu driver?

In arm_smmu_write_context_bank() - IIRC it's mostly about the case where 
if you write a 16-bit ASID to TTBR before setting TCR2.AS you might end 
up losing the top 8 bits of it. However, in the context of a pantomime 
where we just have to pretend to program the "hardware" the way the 
firmware has already programmed it (on pain of getting randomly reset if 
we look at it wrong), I can't imagine it really matters.

Robin.

>> 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>
>> ---
>>   drivers/iommu/arm/arm-smmu/qcom_iommu.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
>> index 1728d4d7fe25..75f353866c40 100644
>> --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
>> +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
>> @@ -273,18 +273,18 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
>>   			ctx->secure_init = true;
>>   		}
>>   
>> -		/* TTBRs */
>> -		iommu_writeq(ctx, ARM_SMMU_CB_TTBR0,
>> -				pgtbl_cfg.arm_lpae_s1_cfg.ttbr |
>> -				FIELD_PREP(ARM_SMMU_TTBRn_ASID, ctx->asid));
>> -		iommu_writeq(ctx, ARM_SMMU_CB_TTBR1, 0);
>> -
>>   		/* TCR */
>>   		iommu_writel(ctx, ARM_SMMU_CB_TCR2,
>>   				arm_smmu_lpae_tcr2(&pgtbl_cfg));
>>   		iommu_writel(ctx, ARM_SMMU_CB_TCR,
>>   			     arm_smmu_lpae_tcr(&pgtbl_cfg) | ARM_SMMU_TCR_EAE);
>>   
>> +		/* TTBRs */
>> +		iommu_writeq(ctx, ARM_SMMU_CB_TTBR0,
>> +				pgtbl_cfg.arm_lpae_s1_cfg.ttbr |
>> +				FIELD_PREP(ARM_SMMU_TTBRn_ASID, ctx->asid));
>> +		iommu_writeq(ctx, ARM_SMMU_CB_TTBR1, 0);
> 
> I'd have thought that SCTLR.M would be clear here, so it shouldn't matter
> what order we write these in.
> 
> Will
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 2/6] iommu/qcom: Write TCR before TTBRs to fix ASID access behavior

[Marijn Suijten]

On 2022-05-31 16:55:59, Will Deacon wrote:
> On Fri, May 27, 2022 at 11:28:57PM +0200, Konrad Dybcio wrote:
> > From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> > 
> > As also stated in the arm-smmu driver, we must write the TCR before
> > writing the TTBRs, since the TCR determines the access behavior of
> > some fields.
> 
> Where is this stated in the arm-smmu driver?
> 
> > 
> > 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>
> > ---
> >  drivers/iommu/arm/arm-smmu/qcom_iommu.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> > index 1728d4d7fe25..75f353866c40 100644
> > --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> > +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> > @@ -273,18 +273,18 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
> >  			ctx->secure_init = true;
> >  		}
> >  
> > -		/* TTBRs */
> > -		iommu_writeq(ctx, ARM_SMMU_CB_TTBR0,
> > -				pgtbl_cfg.arm_lpae_s1_cfg.ttbr |
> > -				FIELD_PREP(ARM_SMMU_TTBRn_ASID, ctx->asid));
> > -		iommu_writeq(ctx, ARM_SMMU_CB_TTBR1, 0);
> > -
> >  		/* TCR */
> >  		iommu_writel(ctx, ARM_SMMU_CB_TCR2,
> >  				arm_smmu_lpae_tcr2(&pgtbl_cfg));
> >  		iommu_writel(ctx, ARM_SMMU_CB_TCR,
> >  			     arm_smmu_lpae_tcr(&pgtbl_cfg) | ARM_SMMU_TCR_EAE);
> >  
> > +		/* TTBRs */
> > +		iommu_writeq(ctx, ARM_SMMU_CB_TTBR0,
> > +				pgtbl_cfg.arm_lpae_s1_cfg.ttbr |
> > +				FIELD_PREP(ARM_SMMU_TTBRn_ASID, ctx->asid));
> > +		iommu_writeq(ctx, ARM_SMMU_CB_TTBR1, 0);
> 
> I'd have thought that SCTLR.M would be clear here, so it shouldn't matter
> what order we write these in.

Having tested the series without this particular patch on 8976 (Sony
Loire Suzu), it doesn't seem to matter indeed.  I'll ask around if this
"access behaviour" was observed on a different board/platform.

- Marijn

Re: [PATCH 2/6] iommu/qcom: Write TCR before TTBRs to fix ASID access behavior

[AngeloGioacchino Del Regno]

Il 06/06/22 00:06, Marijn Suijten ha scritto:
> On 2022-05-31 16:55:59, Will Deacon wrote:
>> On Fri, May 27, 2022 at 11:28:57PM +0200, Konrad Dybcio wrote:
>>> From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
>>>
>>> As also stated in the arm-smmu driver, we must write the TCR before
>>> writing the TTBRs, since the TCR determines the access behavior of
>>> some fields.
>>
>> Where is this stated in the arm-smmu driver?
>>
>>>
>>> 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>
>>> ---
>>>   drivers/iommu/arm/arm-smmu/qcom_iommu.c | 12 ++++++------
>>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
>>> index 1728d4d7fe25..75f353866c40 100644
>>> --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
>>> +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
>>> @@ -273,18 +273,18 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
>>>   			ctx->secure_init = true;
>>>   		}
>>>   
>>> -		/* TTBRs */
>>> -		iommu_writeq(ctx, ARM_SMMU_CB_TTBR0,
>>> -				pgtbl_cfg.arm_lpae_s1_cfg.ttbr |
>>> -				FIELD_PREP(ARM_SMMU_TTBRn_ASID, ctx->asid));
>>> -		iommu_writeq(ctx, ARM_SMMU_CB_TTBR1, 0);
>>> -
>>>   		/* TCR */
>>>   		iommu_writel(ctx, ARM_SMMU_CB_TCR2,
>>>   				arm_smmu_lpae_tcr2(&pgtbl_cfg));
>>>   		iommu_writel(ctx, ARM_SMMU_CB_TCR,
>>>   			     arm_smmu_lpae_tcr(&pgtbl_cfg) | ARM_SMMU_TCR_EAE);
>>>   
>>> +		/* TTBRs */
>>> +		iommu_writeq(ctx, ARM_SMMU_CB_TTBR0,
>>> +				pgtbl_cfg.arm_lpae_s1_cfg.ttbr |
>>> +				FIELD_PREP(ARM_SMMU_TTBRn_ASID, ctx->asid));
>>> +		iommu_writeq(ctx, ARM_SMMU_CB_TTBR1, 0);
>>
>> I'd have thought that SCTLR.M would be clear here, so it shouldn't matter
>> what order we write these in.
> 
> Having tested the series without this particular patch on 8976 (Sony
> Loire Suzu), it doesn't seem to matter indeed.  I'll ask around if this
> "access behaviour" was observed on a different board/platform.
> 
> - Marijn

On some platforms, the bootloader (and/or the hypervisor) is performing some
initialization of the IOMMU which, depending on the actual firmware version
that ran before booting Linux, may or may not leave SCTLR.M cleared.

Cheers,
Angelo

Re: [PATCH 2/6] iommu/qcom: Write TCR before TTBRs to fix ASID access behavior

[Robin Murphy]

On 2022-06-08 11:27, AngeloGioacchino Del Regno wrote:
> Il 06/06/22 00:06, Marijn Suijten ha scritto:
>> On 2022-05-31 16:55:59, Will Deacon wrote:
>>> On Fri, May 27, 2022 at 11:28:57PM +0200, Konrad Dybcio wrote:
>>>> From: AngeloGioacchino Del Regno 
>>>> <angelogioacchino.delregno@somainline.org>
>>>>
>>>> As also stated in the arm-smmu driver, we must write the TCR before
>>>> writing the TTBRs, since the TCR determines the access behavior of
>>>> some fields.
>>>
>>> Where is this stated in the arm-smmu driver?
>>>
>>>>
>>>> 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>
>>>> ---
>>>>   drivers/iommu/arm/arm-smmu/qcom_iommu.c | 12 ++++++------
>>>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c 
>>>> b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
>>>> index 1728d4d7fe25..75f353866c40 100644
>>>> --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
>>>> +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
>>>> @@ -273,18 +273,18 @@ static int qcom_iommu_init_domain(struct 
>>>> iommu_domain *domain,
>>>>               ctx->secure_init = true;
>>>>           }
>>>> -        /* TTBRs */
>>>> -        iommu_writeq(ctx, ARM_SMMU_CB_TTBR0,
>>>> -                pgtbl_cfg.arm_lpae_s1_cfg.ttbr |
>>>> -                FIELD_PREP(ARM_SMMU_TTBRn_ASID, ctx->asid));
>>>> -        iommu_writeq(ctx, ARM_SMMU_CB_TTBR1, 0);
>>>> -
>>>>           /* TCR */
>>>>           iommu_writel(ctx, ARM_SMMU_CB_TCR2,
>>>>                   arm_smmu_lpae_tcr2(&pgtbl_cfg));
>>>>           iommu_writel(ctx, ARM_SMMU_CB_TCR,
>>>>                    arm_smmu_lpae_tcr(&pgtbl_cfg) | ARM_SMMU_TCR_EAE);
>>>> +        /* TTBRs */
>>>> +        iommu_writeq(ctx, ARM_SMMU_CB_TTBR0,
>>>> +                pgtbl_cfg.arm_lpae_s1_cfg.ttbr |
>>>> +                FIELD_PREP(ARM_SMMU_TTBRn_ASID, ctx->asid));
>>>> +        iommu_writeq(ctx, ARM_SMMU_CB_TTBR1, 0);
>>>
>>> I'd have thought that SCTLR.M would be clear here, so it shouldn't 
>>> matter
>>> what order we write these in.
>>
>> Having tested the series without this particular patch on 8976 (Sony
>> Loire Suzu), it doesn't seem to matter indeed.  I'll ask around if this
>> "access behaviour" was observed on a different board/platform.
>>
>> - Marijn
> 
> On some platforms, the bootloader (and/or the hypervisor) is performing 
> some
> initialization of the IOMMU which, depending on the actual firmware version
> that ran before booting Linux, may or may not leave SCTLR.M cleared.

But does it actually matter even then? If we're only allowed to program 
the same ASID that was in use beforehand, then logically we can't be 
changing TCR2.AS in a way that makes any difference anyway.

I see no point in pretending to worry about theoretical architectural 
correctness in a driver tied to specific implementations that already 
violate the given architecture in many other ways. If there's a known 
firmware implementation that definitely requires this, that should be 
called out; otherwise, there doesn't seem much justification for the 
patch at all.

Thanks,
Robin.

Re: [PATCH 2/6] iommu/qcom: Write TCR before TTBRs to fix ASID access behavior

[AngeloGioacchino Del Regno]

Il 08/06/22 12:54, Robin Murphy ha scritto:
> On 2022-06-08 11:27, AngeloGioacchino Del Regno wrote:
>> Il 06/06/22 00:06, Marijn Suijten ha scritto:
>>> On 2022-05-31 16:55:59, Will Deacon wrote:
>>>> On Fri, May 27, 2022 at 11:28:57PM +0200, Konrad Dybcio wrote:
>>>>> From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
>>>>>
>>>>> As also stated in the arm-smmu driver, we must write the TCR before
>>>>> writing the TTBRs, since the TCR determines the access behavior of
>>>>> some fields.
>>>>
>>>> Where is this stated in the arm-smmu driver?
>>>>
>>>>>
>>>>> 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>
>>>>> ---
>>>>>   drivers/iommu/arm/arm-smmu/qcom_iommu.c | 12 ++++++------
>>>>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c 
>>>>> b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
>>>>> index 1728d4d7fe25..75f353866c40 100644
>>>>> --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
>>>>> +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
>>>>> @@ -273,18 +273,18 @@ static int qcom_iommu_init_domain(struct iommu_domain 
>>>>> *domain,
>>>>>               ctx->secure_init = true;
>>>>>           }
>>>>> -        /* TTBRs */
>>>>> -        iommu_writeq(ctx, ARM_SMMU_CB_TTBR0,
>>>>> -                pgtbl_cfg.arm_lpae_s1_cfg.ttbr |
>>>>> -                FIELD_PREP(ARM_SMMU_TTBRn_ASID, ctx->asid));
>>>>> -        iommu_writeq(ctx, ARM_SMMU_CB_TTBR1, 0);
>>>>> -
>>>>>           /* TCR */
>>>>>           iommu_writel(ctx, ARM_SMMU_CB_TCR2,
>>>>>                   arm_smmu_lpae_tcr2(&pgtbl_cfg));
>>>>>           iommu_writel(ctx, ARM_SMMU_CB_TCR,
>>>>>                    arm_smmu_lpae_tcr(&pgtbl_cfg) | ARM_SMMU_TCR_EAE);
>>>>> +        /* TTBRs */
>>>>> +        iommu_writeq(ctx, ARM_SMMU_CB_TTBR0,
>>>>> +                pgtbl_cfg.arm_lpae_s1_cfg.ttbr |
>>>>> +                FIELD_PREP(ARM_SMMU_TTBRn_ASID, ctx->asid));
>>>>> +        iommu_writeq(ctx, ARM_SMMU_CB_TTBR1, 0);
>>>>
>>>> I'd have thought that SCTLR.M would be clear here, so it shouldn't matter
>>>> what order we write these in.
>>>
>>> Having tested the series without this particular patch on 8976 (Sony
>>> Loire Suzu), it doesn't seem to matter indeed.  I'll ask around if this
>>> "access behaviour" was observed on a different board/platform.
>>>
>>> - Marijn
>>
>> On some platforms, the bootloader (and/or the hypervisor) is performing some
>> initialization of the IOMMU which, depending on the actual firmware version
>> that ran before booting Linux, may or may not leave SCTLR.M cleared.
> 
> But does it actually matter even then? If we're only allowed to program the same 
> ASID that was in use beforehand, then logically we can't be changing TCR2.AS in a 
> way that makes any difference anyway.
> 
> I see no point in pretending to worry about theoretical architectural correctness 
> in a driver tied to specific implementations that already violate the given 
> architecture in many other ways. If there's a known firmware implementation that 
> definitely requires this, that should be called out; otherwise, there doesn't seem 
> much justification for the patch at all.
> 

This is something I wrote more than one year ago, hence I don't remember clearly,
but if my memories aren't failing me, this was necessary to enable support for
the AArch64 pagetables.
If that doesn't make sense to you, I guess that Marijn or Konrad can help testing
switching to AA64 PT with the incorrect programming sequence.

Aside from that, as a strictly personal opinion (and nothing else), I think that
ensuring architectural correctness *where possible* can only be good: I don't see
why we should intentionally keep a wrong programming sequence in principle.

Regards,
Angelo

> Thanks,
> Robin.

[PATCH 3/6] iommu/qcom: Properly reset the IOMMU context

[Konrad Dybcio]

From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>

To avoid context faults reset the context entirely on detach and
to ensure a fresh clean start also do a complete reset before
programming the context for domain initialization.

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>
---
 drivers/iommu/arm/arm-smmu/qcom_iommu.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 75f353866c40..129e322f56a6 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -223,6 +223,23 @@ static irqreturn_t qcom_iommu_fault(int irq, void *dev)
 	return IRQ_HANDLED;
 }
 
+static void qcom_iommu_reset_ctx(struct qcom_iommu_ctx *ctx)
+{
+	iommu_writel(ctx, ARM_SMMU_CB_FAR, 0);
+	iommu_writel(ctx, ARM_SMMU_CB_FSR, 0);
+	iommu_writel(ctx, ARM_SMMU_CB_S1_MAIR1, 0);
+	iommu_writel(ctx, ARM_SMMU_CB_PAR, 0);
+	iommu_writel(ctx, ARM_SMMU_CB_S1_MAIR0, 0);
+	iommu_writel(ctx, ARM_SMMU_CB_SCTLR, 0);
+	iommu_writel(ctx, ARM_SMMU_CB_TCR2, 0);
+	iommu_writel(ctx, ARM_SMMU_CB_TCR, 0);
+	iommu_writeq(ctx, ARM_SMMU_CB_TTBR0, 0);
+	iommu_writeq(ctx, ARM_SMMU_CB_TTBR1, 0);
+
+	/* Should we issue a TLBSYNC there instead? */
+	wmb();
+}
+
 static int qcom_iommu_init_domain(struct iommu_domain *domain,
 				  struct qcom_iommu_dev *qcom_iommu,
 				  struct device *dev)
@@ -273,6 +290,8 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
 			ctx->secure_init = true;
 		}
 
+		qcom_iommu_reset_ctx(ctx);
+
 		/* TCR */
 		iommu_writel(ctx, ARM_SMMU_CB_TCR2,
 				arm_smmu_lpae_tcr2(&pgtbl_cfg));
@@ -406,8 +425,8 @@ static void qcom_iommu_detach_dev(struct iommu_domain *domain, struct device *de
 	for (i = 0; i < fwspec->num_ids; i++) {
 		struct qcom_iommu_ctx *ctx = to_ctx(qcom_domain, fwspec->ids[i]);
 
-		/* Disable the context bank: */
-		iommu_writel(ctx, ARM_SMMU_CB_SCTLR, 0);
+		/* Disable and reset the context bank */
+		qcom_iommu_reset_ctx(ctx);
 
 		ctx->domain = NULL;
 	}
-- 
2.36.1

[PATCH 4/6] iommu/qcom: Add support for AArch64 IOMMU pagetables

[Konrad Dybcio]

From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>

Some IOMMUs associated with some TZ firmwares may support switching
to the AArch64 pagetable format by sending a "set pagetable format"
scm command indicating the IOMMU secure ID and the context number
to switch.

Add a DT property "qcom,use-aarch64-pagetables" for this driver to
send this command to the secure world and to switch the pagetable
format to benefit of the ARM64 IOMMU pagetables, where possible.

Note that, even though the command should be valid to switch each
context, the property is made global because:
1. It doesn't make too much sense to switch only one or two
   context(s) to AA64 instead of just the entire thing
2. Some IOMMUs will go crazy and produce spectacular results when
   trying to mix up the pagetables on a per-context basis.

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  |  2 +
 drivers/iommu/arm/arm-smmu/qcom_iommu.c       | 54 +++++++++++++++----
 2 files changed, 47 insertions(+), 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/iommu/qcom,iommu.txt b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
index ba0b77889f02..72ae0595efff 100644
--- a/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
+++ b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
@@ -47,6 +47,8 @@ to non-secure vs secure interrupt line.
                      secure lines.  (Ie. if the iommu contains secure
                      context banks)
 - qcom,ctx-num     : The number associated to the context bank
+- qcom,use-aarch64-pagetables : Switch to AArch64 pagetable format on all
+                                contexts declared in this IOMMU
 
 
 ** Examples:
diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 129e322f56a6..530aa92bf6a1 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -52,6 +52,7 @@ struct qcom_iommu_dev {
 	void __iomem		*local_base;
 	u32			 sec_id;
 	u8			 num_ctxs;
+	bool			 use_aarch64_pt;
 	struct qcom_iommu_ctx	*ctxs[];   /* indexed by asid-1 */
 };
 
@@ -164,11 +165,17 @@ static void qcom_iommu_tlb_inv_range_nosync(unsigned long iova, size_t size,
 	reg = leaf ? ARM_SMMU_CB_S1_TLBIVAL : ARM_SMMU_CB_S1_TLBIVA;
 
 	for (i = 0; i < fwspec->num_ids; i++) {
+		struct qcom_iommu_dev *qcom_iommu = qcom_domain->iommu;
 		struct qcom_iommu_ctx *ctx = to_ctx(qcom_domain, fwspec->ids[i]);
 		size_t s = size;
 
-		iova = (iova >> 12) << 12;
-		iova |= ctx->asid;
+		if (qcom_iommu->use_aarch64_pt) {
+			iova >>= 12;
+			iova |= (unsigned long)ctx->asid << 48;
+		} else {
+			iova &= (1UL << 12) - 1UL;
+			iova |= ctx->asid;
+		}
 		do {
 			iommu_writel(ctx, reg, iova);
 			iova += granule;
@@ -248,6 +255,8 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
 	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
 	struct io_pgtable_ops *pgtbl_ops;
 	struct io_pgtable_cfg pgtbl_cfg;
+	enum io_pgtable_fmt pgtbl_fmt;
+	unsigned long ias, oas;
 	int i, ret = 0;
 	u32 reg;
 
@@ -255,10 +264,19 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
 	if (qcom_domain->iommu)
 		goto out_unlock;
 
+	if (qcom_iommu->use_aarch64_pt) {
+		pgtbl_fmt = ARM_64_LPAE_S1;
+		ias = oas = 48;
+	} else {
+		pgtbl_fmt = ARM_32_LPAE_S1;
+		ias = 32;
+		oas = 40;
+	}
+
 	pgtbl_cfg = (struct io_pgtable_cfg) {
 		.pgsize_bitmap	= qcom_iommu_ops.pgsize_bitmap,
-		.ias		= 32,
-		.oas		= 40,
+		.ias		= ias,
+		.oas		= oas,
 		.tlb		= &qcom_flush_ops,
 		.iommu_dev	= qcom_iommu->dev,
 	};
@@ -266,7 +284,7 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
 	qcom_domain->iommu = qcom_iommu;
 	qcom_domain->fwspec = fwspec;
 
-	pgtbl_ops = alloc_io_pgtable_ops(ARM_32_LPAE_S1, &pgtbl_cfg, qcom_domain);
+	pgtbl_ops = alloc_io_pgtable_ops(pgtbl_fmt, &pgtbl_cfg, qcom_domain);
 	if (!pgtbl_ops) {
 		dev_err(qcom_iommu->dev, "failed to allocate pagetable ops\n");
 		ret = -ENOMEM;
@@ -280,6 +298,7 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
 
 	for (i = 0; i < fwspec->num_ids; i++) {
 		struct qcom_iommu_ctx *ctx = to_ctx(qcom_domain, fwspec->ids[i]);
+		u32 tcr[2];
 
 		if (!ctx->secure_init) {
 			ret = qcom_scm_restore_sec_cfg(qcom_iommu->sec_id, ctx->asid);
@@ -292,11 +311,25 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
 
 		qcom_iommu_reset_ctx(ctx);
 
+
+		tcr[0] = arm_smmu_lpae_tcr(&pgtbl_cfg);
+		tcr[1] = arm_smmu_lpae_tcr2(&pgtbl_cfg);
+
+		if (!qcom_iommu->use_aarch64_pt) {
+			tcr[0] |= ARM_SMMU_TCR_EAE;
+		} else {
+			/* This shall not fail, or spectacular things happen! */
+			if (qcom_scm_iommu_set_pt_format(qcom_iommu->sec_id, ctx->asid, 1)) {
+				dev_warn(qcom_iommu->dev, "Cannot set AArch64 pt format\n");
+				goto out_clear_iommu;
+			}
+
+			tcr[1] |= ARM_SMMU_TCR2_AS;
+		}
+
 		/* TCR */
-		iommu_writel(ctx, ARM_SMMU_CB_TCR2,
-				arm_smmu_lpae_tcr2(&pgtbl_cfg));
-		iommu_writel(ctx, ARM_SMMU_CB_TCR,
-			     arm_smmu_lpae_tcr(&pgtbl_cfg) | ARM_SMMU_TCR_EAE);
+		iommu_writel(ctx, ARM_SMMU_CB_TCR2, tcr[1]);
+		iommu_writel(ctx, ARM_SMMU_CB_TCR, tcr[0]);
 
 		/* TTBRs */
 		iommu_writeq(ctx, ARM_SMMU_CB_TTBR0,
@@ -844,6 +877,9 @@ static int qcom_iommu_device_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	if (of_property_read_bool(dev->of_node, "qcom,use-aarch64-pagetables"))
+		qcom_iommu->use_aarch64_pt = true;
+
 	if (qcom_iommu_has_secure_context(qcom_iommu)) {
 		ret = qcom_iommu_sec_ptbl_init(dev);
 		if (ret) {
-- 
2.36.1

Re: [PATCH 4/6] iommu/qcom: Add support for AArch64 IOMMU pagetables

[kernel test robot]

Hi Konrad,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on joro-iommu/next]
[also build test WARNING on v5.18 next-20220527]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Konrad-Dybcio/iommu-qcom-Use-the-asid-read-from-device-tree-if-specified/20220528-062952
base:   https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20220528/202205280904.vsNCfpph-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/0744f7d6ebfff8d6854a24d0f95f8e58885b5212
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Konrad-Dybcio/iommu-qcom-Use-the-asid-read-from-device-tree-if-specified/20220528-062952
        git checkout 0744f7d6ebfff8d6854a24d0f95f8e58885b5212
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash drivers/iommu/arm/arm-smmu/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/iommu/arm/arm-smmu/qcom_iommu.c: In function 'qcom_iommu_tlb_inv_range_nosync':
>> drivers/iommu/arm/arm-smmu/qcom_iommu.c:174:58: warning: left shift count >= width of type [-Wshift-count-overflow]
     174 |                         iova |= (unsigned long)ctx->asid << 48;
         |                                                          ^~


vim +174 drivers/iommu/arm/arm-smmu/qcom_iommu.c

   157	
   158	static void qcom_iommu_tlb_inv_range_nosync(unsigned long iova, size_t size,
   159						    size_t granule, bool leaf, void *cookie)
   160	{
   161		struct qcom_iommu_domain *qcom_domain = cookie;
   162		struct iommu_fwspec *fwspec = qcom_domain->fwspec;
   163		unsigned i, reg;
   164	
   165		reg = leaf ? ARM_SMMU_CB_S1_TLBIVAL : ARM_SMMU_CB_S1_TLBIVA;
   166	
   167		for (i = 0; i < fwspec->num_ids; i++) {
   168			struct qcom_iommu_dev *qcom_iommu = qcom_domain->iommu;
   169			struct qcom_iommu_ctx *ctx = to_ctx(qcom_domain, fwspec->ids[i]);
   170			size_t s = size;
   171	
   172			if (qcom_iommu->use_aarch64_pt) {
   173				iova >>= 12;
 > 174				iova |= (unsigned long)ctx->asid << 48;
   175			} else {
   176				iova &= (1UL << 12) - 1UL;
   177				iova |= ctx->asid;
   178			}
   179			do {
   180				iommu_writel(ctx, reg, iova);
   181				iova += granule;
   182			} while (s -= granule);
   183		}
   184	}
   185	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 4/6] iommu/qcom: Add support for AArch64 IOMMU pagetables

[Rob Herring]

On Fri, May 27, 2022 at 11:28:59PM +0200, Konrad Dybcio wrote:
> From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> 
> Some IOMMUs associated with some TZ firmwares may support switching
> to the AArch64 pagetable format by sending a "set pagetable format"
> scm command indicating the IOMMU secure ID and the context number
> to switch.
> 
> Add a DT property "qcom,use-aarch64-pagetables" for this driver to
> send this command to the secure world and to switch the pagetable
> format to benefit of the ARM64 IOMMU pagetables, where possible.
> 
> Note that, even though the command should be valid to switch each
> context, the property is made global because:
> 1. It doesn't make too much sense to switch only one or two
>    context(s) to AA64 instead of just the entire thing
> 2. Some IOMMUs will go crazy and produce spectacular results when
>    trying to mix up the pagetables on a per-context basis.
> 
> 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  |  2 +

Bindings should be separate patch.

As you are making multiple changes, please convert this to DT schema 
first.

>  drivers/iommu/arm/arm-smmu/qcom_iommu.c       | 54 +++++++++++++++----
>  2 files changed, 47 insertions(+), 9 deletions(-)

[PATCH 5/6] iommu/qcom: Index contexts by asid number to allow asid 0

[Konrad Dybcio]

From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>

This driver was indexing the contexts by asid-1, which is probably
done under the assumption that the first ASID is always 1.

Unfortunately this is not entirely true: at least in the MSM8956
and MSM8976 GPU IOMMU, the gpu_user context's ASID number is zero.
To allow using an asid number of zero, stop indexing the contexts
by asid-1 and rather index them by asid.

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>
---
 drivers/iommu/arm/arm-smmu/qcom_iommu.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 530aa92bf6a1..4fefbab15b71 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -53,7 +53,7 @@ struct qcom_iommu_dev {
 	u32			 sec_id;
 	u8			 num_ctxs;
 	bool			 use_aarch64_pt;
-	struct qcom_iommu_ctx	*ctxs[];   /* indexed by asid-1 */
+	struct qcom_iommu_ctx	*ctxs[];   /* indexed by asid */
 };
 
 struct qcom_iommu_ctx {
@@ -95,7 +95,7 @@ static struct qcom_iommu_ctx * to_ctx(struct qcom_iommu_domain *d, unsigned asid
 	struct qcom_iommu_dev *qcom_iommu = d->iommu;
 	if (!qcom_iommu)
 		return NULL;
-	return qcom_iommu->ctxs[asid - 1];
+	return qcom_iommu->ctxs[asid];
 }
 
 static inline void
@@ -614,12 +614,10 @@ static int qcom_iommu_of_xlate(struct device *dev, struct of_phandle_args *args)
 	qcom_iommu = platform_get_drvdata(iommu_pdev);
 
 	/* make sure the asid specified in dt is valid, so we don't have
-	 * to sanity check this elsewhere, since 'asid - 1' is used to
-	 * index into qcom_iommu->ctxs:
+	 * to sanity check this elsewhere:
 	 */
-	if (WARN_ON(asid < 1) ||
-	    WARN_ON(asid > qcom_iommu->num_ctxs) ||
-	    WARN_ON(qcom_iommu->ctxs[asid - 1] == NULL)) {
+	if (WARN_ON(asid >= qcom_iommu->num_ctxs) ||
+	    WARN_ON(qcom_iommu->ctxs[asid] == NULL)) {
 		put_device(&iommu_pdev->dev);
 		return -EINVAL;
 	}
@@ -778,7 +776,7 @@ static int qcom_iommu_ctx_probe(struct platform_device *pdev)
 
 	dev_dbg(dev, "found asid %u\n", ctx->asid);
 
-	qcom_iommu->ctxs[ctx->asid - 1] = ctx;
+	qcom_iommu->ctxs[ctx->asid] = ctx;
 
 	return 0;
 }
@@ -790,7 +788,7 @@ static int qcom_iommu_ctx_remove(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, NULL);
 
-	qcom_iommu->ctxs[ctx->asid - 1] = NULL;
+	qcom_iommu->ctxs[ctx->asid] = NULL;
 
 	return 0;
 }
@@ -828,7 +826,7 @@ static int qcom_iommu_device_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct resource *res;
 	struct clk *clk;
-	int ret, max_asid = 0;
+	int ret, num_ctxs, max_asid = 0;
 
 	/* find the max asid (which is 1:1 to ctx bank idx), so we know how
 	 * many child ctx devices we have:
@@ -836,11 +834,13 @@ static int qcom_iommu_device_probe(struct platform_device *pdev)
 	for_each_child_of_node(dev->of_node, child)
 		max_asid = max(max_asid, get_asid(child));
 
-	qcom_iommu = devm_kzalloc(dev, struct_size(qcom_iommu, ctxs, max_asid),
+	num_ctxs = max_asid + 1;
+
+	qcom_iommu = devm_kzalloc(dev, struct_size(qcom_iommu, ctxs, num_ctxs),
 				  GFP_KERNEL);
 	if (!qcom_iommu)
 		return -ENOMEM;
-	qcom_iommu->num_ctxs = max_asid;
+	qcom_iommu->num_ctxs = num_ctxs;
 	qcom_iommu->dev = dev;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-- 
2.36.1

Re: [PATCH 5/6] iommu/qcom: Index contexts by asid number to allow asid 0

[Brian Masney]

On Fri, May 27, 2022 at 11:29:00PM +0200, Konrad Dybcio wrote:
> From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> 
> This driver was indexing the contexts by asid-1, which is probably
> done under the assumption that the first ASID is always 1.
> 
> Unfortunately this is not entirely true: at least in the MSM8956
> and MSM8976 GPU IOMMU, the gpu_user context's ASID number is zero.
> To allow using an asid number of zero, stop indexing the contexts
> by asid-1 and rather index them by asid.
> 
> 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>

msm8974 will need this as well.

Reviewed-by: Brian Masney <bmasney@redhat.com>

[PATCH 6/6] iommu/qcom: Add support for QCIOMMUv2 and QCIOMMU-500 secured contexts

[Konrad Dybcio]

From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>

This IOMMU is yet another Qualcomm variant of known IOMMUs, found in
Family-B SoCs, such as MSM8956, MSM8976, MSM8953, MSM8917 and others,
and that firmware perfectly adheres to this driver logic.
This time, though, the catch is that the secure contexts are also
secured, meaning that these are programmed by the bootloader or TZ
and their "interesting" registers are locked out, so the hypervisor
disallows touching them from the non-secure world: in this case
the OS is supposed to blindly trust the secure configuration of
these contexts and just use them "as they are".

For this reason, it is necessary to distinguish between the v1 and
500/v2 secure contexts in this driver in order to adhere to this
specification. To do this, add a new DT compatible, named
"qcom,msm-iommu-v2-sec" that will trigger the new behavior.

For the sake of completeness, also add a "qcom,msm-iommu-v2-ns" so
that the human eye gets pleased with it when reading the contexts
in the final SoC DT. Of course, the latter is just cosmetic.

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   |  2 ++
 drivers/iommu/arm/arm-smmu/qcom_iommu.c        | 18 ++++++++++++++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/iommu/qcom,iommu.txt b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
index 72ae0595efff..861c0cd9c512 100644
--- a/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
+++ b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
@@ -36,6 +36,8 @@ to non-secure vs secure interrupt line.
   - compatible     : Should be one of:
         - "qcom,msm-iommu-v1-ns"  : non-secure context bank
         - "qcom,msm-iommu-v1-sec" : secure context bank
+        - "qcom,msm-iommu-v2-ns"  : non-secure QSMMUv2/QSMMU500 context bank
+        - "qcom,msm-iommu-v2-sec" : secure QSMMUv2/QSMMU500 context bank
   - reg            : Base address and size of context bank within the iommu
   - interrupts     : The context fault irq.
 
diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 4fefbab15b71..aa7359ae34a9 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -60,6 +60,7 @@ struct qcom_iommu_ctx {
 	struct device		*dev;
 	void __iomem		*base;
 	bool			 secure_init;
+	bool			 secured_ctx;
 	u8			 asid;      /* asid and ctx bank # are 1:1 */
 	struct iommu_domain	*domain;
 };
@@ -309,6 +310,12 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
 			ctx->secure_init = true;
 		}
 
+		/* Secured QSMMU-500/QSMMU-v2 contexts cannot be programmed */
+		if (ctx->secured_ctx) {
+			ctx->domain = domain;
+			break;
+		}
+
 		qcom_iommu_reset_ctx(ctx);
 
 
@@ -751,10 +758,14 @@ static int qcom_iommu_ctx_probe(struct platform_device *pdev)
 	if (irq < 0)
 		return -ENODEV;
 
+	if (of_device_is_compatible(dev->of_node, "qcom,msm-iommu-v2-sec"))
+		ctx->secured_ctx = true;
+
 	/* clear IRQs before registering fault handler, just in case the
 	 * boot-loader left us a surprise:
 	 */
-	iommu_writel(ctx, ARM_SMMU_CB_FSR, iommu_readl(ctx, ARM_SMMU_CB_FSR));
+	if (!ctx->secured_ctx)
+		iommu_writel(ctx, ARM_SMMU_CB_FSR, iommu_readl(ctx, ARM_SMMU_CB_FSR));
 
 	ret = devm_request_irq(dev, irq,
 			       qcom_iommu_fault,
@@ -796,6 +807,8 @@ static int qcom_iommu_ctx_remove(struct platform_device *pdev)
 static const struct of_device_id ctx_of_match[] = {
 	{ .compatible = "qcom,msm-iommu-v1-ns" },
 	{ .compatible = "qcom,msm-iommu-v1-sec" },
+	{ .compatible = "qcom,msm-iommu-v2-ns" },
+	{ .compatible = "qcom,msm-iommu-v2-sec" },
 	{ /* sentinel */ }
 };
 
@@ -813,7 +826,8 @@ 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"))
+		if (of_device_is_compatible(child, "qcom,msm-iommu-v1-sec") ||
+		    of_device_is_compatible(child, "qcom,msm-iommu-v2-sec"))
 			return true;
 
 	return false;
-- 
2.36.1