| Message ID | 20220527212901.29268-3-konrad.dybcio@somainline.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Fix and extend Qualcomm IOMMU support | expand |
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
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
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
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
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.
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.
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);