On 20/08/2019 11:19, Will Deacon wrote:
On Mon, Aug 19, 2019 at 07:19:29PM +0100, Robin Murphy wrote:
TTBR1 values have so far been redundant since no users implement any
support for split address spaces. Crucially, though, one of the main
reasons for wanting to do so is to be able to manage each half entirely
independently, e.g. context-switching one set of mappings without
disturbing the other. Thus it seems unlikely that tying two tables
together in a single io_pgtable_cfg would ever be particularly desirable
or useful.

Streamline the configs to just a single conceptual TTBR value
representing the allocated table. This paves the way for future users to
support split address spaces by simply allocating a table and dealing
with the detailed TTBRn logistics themselves.

Signed-off-by: Robin Murphy <[email protected]>
---
  drivers/iommu/arm-smmu-v3.c        |  2 +-
  drivers/iommu/arm-smmu.c           |  9 ++++-----
  drivers/iommu/io-pgtable-arm-v7s.c | 16 +++++++---------
  drivers/iommu/io-pgtable-arm.c     |  7 +++----
  drivers/iommu/ipmmu-vmsa.c         |  2 +-
  drivers/iommu/msm_iommu.c          |  4 ++--
  drivers/iommu/mtk_iommu.c          |  4 ++--
  drivers/iommu/qcom_iommu.c         |  3 +--
  include/linux/io-pgtable.h         |  4 ++--
  9 files changed, 23 insertions(+), 28 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 2a8db896d698..2e50cf49c3c4 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1722,7 +1722,7 @@ static int arm_smmu_domain_finalise_s1(struct 
arm_smmu_domain *smmu_domain,
        }
cfg->cd.asid = (u16)asid;
-       cfg->cd.ttbr = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
+       cfg->cd.ttbr = pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
        cfg->cd.tcr  = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
        cfg->cd.mair = pgtbl_cfg->arm_lpae_s1_cfg.mair;
        return 0;
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 184ca41e9de7..19030c4b5904 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -473,13 +473,12 @@ static void arm_smmu_init_context_bank(struct 
arm_smmu_domain *smmu_domain,
        /* TTBRs */
        if (stage1) {
                if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
-                       cb->ttbr[0] = pgtbl_cfg->arm_v7s_cfg.ttbr[0];
-                       cb->ttbr[1] = pgtbl_cfg->arm_v7s_cfg.ttbr[1];
+                       cb->ttbr[0] = pgtbl_cfg->arm_v7s_cfg.ttbr;
+                       cb->ttbr[1] = 0;
                } else {
-                       cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
+                       cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
                        cb->ttbr[0] |= FIELD_PREP(TTBRn_ASID, cfg->asid);
-                       cb->ttbr[1] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
-                       cb->ttbr[1] |= FIELD_PREP(TTBRn_ASID, cfg->asid);
+                       cb->ttbr[1] = FIELD_PREP(TTBRn_ASID, cfg->asid);

Why do you continue to put the ASID in here?

For the same reason we put it there before ;)

Although I guess if TCR.A1 were ever to get flipped accidentally then we're still cool.

diff --git a/drivers/iommu/qcom_iommu.c b/drivers/iommu/qcom_iommu.c
index 34bb357b3cfa..de55b6d82ef1 100644
--- a/drivers/iommu/qcom_iommu.c
+++ b/drivers/iommu/qcom_iommu.c
@@ -247,10 +247,9 @@ static int qcom_iommu_init_domain(struct iommu_domain 
*domain,
/* TTBRs */
                iommu_writeq(ctx, ARM_SMMU_CB_TTBR0,
-                               pgtbl_cfg.arm_lpae_s1_cfg.ttbr[0] |
+                               pgtbl_cfg.arm_lpae_s1_cfg.ttbr |
                                FIELD_PREP(TTBRn_ASID, ctx->asid));
                iommu_writeq(ctx, ARM_SMMU_CB_TTBR1,
-                               pgtbl_cfg.arm_lpae_s1_cfg.ttbr[1] |
                                FIELD_PREP(TTBRn_ASID, ctx->asid));

Same here.

diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
index a6c8aa204733..7a0905d7a006 100644
--- a/include/linux/io-pgtable.h
+++ b/include/linux/io-pgtable.h
@@ -90,7 +90,7 @@ struct io_pgtable_cfg {
        /* Low-level data specific to the table format */
        union {
                struct {
-                       u64     ttbr[2];
+                       u64     ttbr;
                        u64     tcr;
                        u64     mair;
                } arm_lpae_s1_cfg;
@@ -101,7 +101,7 @@ struct io_pgtable_cfg {
                } arm_lpae_s2_cfg;
struct {
-                       u32     ttbr[2];
+                       u32     ttbr;

We could probably do with a comment for these 'ttbr' field now saying that
they refer to ttbr0 (since the tcr will have EPD1 set).

Yeah, I did wonder whether this might want elaboration, or whether the commit messages plus the code consuming it made the idea sufficiently clear - I guess that's my answer...

Robin.
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Reply via email to