iommu/io-pgtable: Cleanup and prep for split tables

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

drivers/iommu/arm-smmu-v3.c +1 -1

index 2a8db896d698..2e50cf49c3c4 100644
@@ -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;

drivers/iommu/arm-smmu.c +0 -1

index 184ca41e9de7..19030c4b5904 100644
@@ -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];

drivers/iommu/qcom_iommu.c +0 -0

index 34bb357b3cfa..de55b6d82ef1 100644

include/linux/io-pgtable.h +1 -1

index a6c8aa204733..7a0905d7a006 100644
@@ -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;

Quoted 52 lines · Robin Murphy

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

drivers/iommu/arm-smmu-v3.c +1 -1

index 2a8db896d698..2e50cf49c3c4 100644
@@ -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;

drivers/iommu/arm-smmu.c +2 -2

index 184ca41e9de7..19030c4b5904 100644
@@ -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 {

drivers/iommu/arm-smmu.c static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain, · line 481

Quoted 6 lines

drivers/iommu/arm-smmu.c +2 -3

-			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?

Quoted 5 lines

drivers/iommu/qcom_iommu.c +0 -0

index 34bb357b3cfa..de55b6d82ef1 100644

drivers/iommu/qcom_iommu.c static int qcom_iommu_init_domain(struct iommu_domain *domain, · line 253

Quoted 9 lines

drivers/iommu/qcom_iommu.c +1 -2

@@ -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.

Quoted 16 lines

include/linux/io-pgtable.h +1 -1

index a6c8aa204733..7a0905d7a006 100644
@@ -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;
 

include/linux/io-pgtable.h struct io_pgtable_cfg { · line 104

Quoted 3 lines

include/linux/io-pgtable.h +1 -1

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

Quoted 52 lines · Will Deacon

Quoted 51 lines · Robin Murphy

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

drivers/iommu/arm-smmu-v3.c +1 -1

index 2a8db896d698..2e50cf49c3c4 100644
@@ -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;

drivers/iommu/arm-smmu.c +2 -2

index 184ca41e9de7..19030c4b5904 100644
@@ -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 {

Quoted 8 lines

drivers/iommu/arm-smmu.c static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain, · line 481

Quoted 6 lines

drivers/iommu/arm-smmu.c +2 -3

-			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.

Although it's conceptually nice for the io_pgtable_cfg to provide a standard VMSA TCR value, the reality is that no VMSA-compliant IOMMU looks exactly like an Arm CPU, and they all have various other TCR controls which io-pgtable can't be expected to understand. Thus since there is an expectation that drivers will have to add to the given TCR value anyway, let's strip it down to just the essentials that are directly relevant to io-pgatble's inner workings - namely the address sizes, walk attributes, and where appropriate, format selection.

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/arm-smmu-v3.c        | 7 +------
 drivers/iommu/arm-smmu.c           | 1 +
 drivers/iommu/arm-smmu.h           | 2 ++
 drivers/iommu/io-pgtable-arm-v7s.c | 6 ++----
 drivers/iommu/io-pgtable-arm.c     | 4 ----
 drivers/iommu/qcom_iommu.c         | 2 +-
 6 files changed, 7 insertions(+), 15 deletions(-)

Quoted 10 lines · Robin Murphy

Although it's conceptually nice for the io_pgtable_cfg to provide a standard VMSA TCR value, the reality is that no VMSA-compliant IOMMU looks exactly like an Arm CPU, and they all have various other TCR controls which io-pgtable can't be expected to understand. Thus since there is an expectation that drivers will have to add to the given TCR value anyway, let's strip it down to just the essentials that are directly relevant to io-pgatble's inner workings - namely the address sizes, walk attributes, and where appropriate, format selection.

Signed-off-by Robin Murphy <[email protected]>

Quoted 8 lines

---
  drivers/iommu/arm-smmu-v3.c        | 7 +------
  drivers/iommu/arm-smmu.c           | 1 +
  drivers/iommu/arm-smmu.h           | 2 ++
  drivers/iommu/io-pgtable-arm-v7s.c | 6 ++----
  drivers/iommu/io-pgtable-arm.c     | 4 ----
  drivers/iommu/qcom_iommu.c         | 2 +-
  6 files changed, 7 insertions(+), 15 deletions(-)

Hmm, so I'm a bit nervous about this one since I think we really should be providing a TCR with EPD1 set if we're only giving you TTBR0. Relying on the driver to do this worries me. See my comments on the next patch.

Quoted 20 lines · Will Deacon

Quoted 10 lines · Robin Murphy

Although it's conceptually nice for the io_pgtable_cfg to provide a standard VMSA TCR value, the reality is that no VMSA-compliant IOMMU looks exactly like an Arm CPU, and they all have various other TCR controls which io-pgtable can't be expected to understand. Thus since there is an expectation that drivers will have to add to the given TCR value anyway, let's strip it down to just the essentials that are directly relevant to io-pgatble's inner workings - namely the address sizes, walk attributes, and where appropriate, format selection.

Signed-off-by Robin Murphy <[email protected]>

Quoted 8 lines

---
  drivers/iommu/arm-smmu-v3.c        | 7 +------
  drivers/iommu/arm-smmu.c           | 1 +
  drivers/iommu/arm-smmu.h           | 2 ++
  drivers/iommu/io-pgtable-arm-v7s.c | 6 ++----
  drivers/iommu/io-pgtable-arm.c     | 4 ----
  drivers/iommu/qcom_iommu.c         | 2 +-
  6 files changed, 7 insertions(+), 15 deletions(-)

Quoted 3 lines

Hmm, so I'm a bit nervous about this one since I think we really should be providing a TCR with EPD1 set if we're only giving you TTBR0. Relying on the driver to do this worries me. See my comments on the next patch.

The whole idea is that we already know we can't provide a *complete* TCR value (not least because anything above bit 31 is the wild west), thus there's really no point in io-pgtable trying to provide anything other than the parts it definitely controls. It makes sense to provide this partial TCR value "as if" for TTBR0, since that's the most common case, but ultimately io-pgatble doesn't know (or need to) which TTBR the caller intends to actually use for this table. Even if the caller *is* allocating it for TTBR0, io-pgtable doesn't know that they haven't got something live in TTBR1 already, so it still wouldn't be in a position to make the EPD1 call either way.

Ultimately, it's the IOMMU drivers who decide what they put in which TTBR, so it's the IOMMU drivers which have to take responsibility for EPD*. Sure you can worry about it, but you can equally worry about them them misprogramming the ASID or anything else...

Quoted 25 lines · Robin Murphy

Quoted 20 lines · Will Deacon

Quoted 10 lines · Robin Murphy

Although it's conceptually nice for the io_pgtable_cfg to provide a standard VMSA TCR value, the reality is that no VMSA-compliant IOMMU looks exactly like an Arm CPU, and they all have various other TCR controls which io-pgtable can't be expected to understand. Thus since there is an expectation that drivers will have to add to the given TCR value anyway, let's strip it down to just the essentials that are directly relevant to io-pgatble's inner workings - namely the address sizes, walk attributes, and where appropriate, format selection.

Signed-off-by Robin Murphy <[email protected]>

Quoted 8 lines

---
   drivers/iommu/arm-smmu-v3.c        | 7 +------
   drivers/iommu/arm-smmu.c           | 1 +
   drivers/iommu/arm-smmu.h           | 2 ++
   drivers/iommu/io-pgtable-arm-v7s.c | 6 ++----
   drivers/iommu/io-pgtable-arm.c     | 4 ----
   drivers/iommu/qcom_iommu.c         | 2 +-
   6 files changed, 7 insertions(+), 15 deletions(-)

Quoted 3 lines

Hmm, so I'm a bit nervous about this one since I think we really should be providing a TCR with EPD1 set if we're only giving you TTBR0. Relying on the driver to do this worries me. See my comments on the next patch.

Quoted 10 lines

The whole idea is that we already know we can't provide a *complete* TCR value (not least because anything above bit 31 is the wild west), thus there's really no point in io-pgtable trying to provide anything other than the parts it definitely controls. It makes sense to provide this partial TCR value "as if" for TTBR0, since that's the most common case, but ultimately io-pgatble doesn't know (or need to) which TTBR the caller intends to actually use for this table. Even if the caller *is* allocating it for TTBR0, io-pgtable doesn't know that they haven't got something live in TTBR1 already, so it still wouldn't be in a position to make the EPD1 call either way.

Ok, but the driver can happily rewrite/ignore what it gets back. I suppose an alternative would be scrapped the 'u64 tcr' and instead having a bunch of named bitfields for the stuff we're actually providing, although I'd still like EPDx to be in there.

Quoted 4 lines

Ultimately, it's the IOMMU drivers who decide what they put in which TTBR, so it's the IOMMU drivers which have to take responsibility for EPD*. Sure you can worry about it, but you can equally worry about them them misprogramming the ASID or anything else...

I find the EPDx bits particularly dangerous because:

- They're easily overlooked
- Clobbering TTBR1 with 0x0 doesn't disable walks via TTBR1 as you might reasonably expect

- If you do the above without EPD, the breakage will be subtle

and given that I don't see any real downsides to us providing a default TCR value with EPD set appropriately, then I think we should do that. I'd be happy to revisit the decision later on if it's getting the way of a real use-case, but it feels like we're throwing the baby out with the bathwater at the moment and I'd rather do this incrementally based on actual need.

Quoted 37 lines · Will Deacon

Quoted 25 lines · Robin Murphy

Quoted 20 lines · Will Deacon

Quoted 10 lines · Robin Murphy

Although it's conceptually nice for the io_pgtable_cfg to provide a standard VMSA TCR value, the reality is that no VMSA-compliant IOMMU looks exactly like an Arm CPU, and they all have various other TCR controls which io-pgtable can't be expected to understand. Thus since there is an expectation that drivers will have to add to the given TCR value anyway, let's strip it down to just the essentials that are directly relevant to io-pgatble's inner workings - namely the address sizes, walk attributes, and where appropriate, format selection.

Signed-off-by Robin Murphy <[email protected]>

Quoted 8 lines

---
   drivers/iommu/arm-smmu-v3.c        | 7 +------
   drivers/iommu/arm-smmu.c           | 1 +
   drivers/iommu/arm-smmu.h           | 2 ++
   drivers/iommu/io-pgtable-arm-v7s.c | 6 ++----
   drivers/iommu/io-pgtable-arm.c     | 4 ----
   drivers/iommu/qcom_iommu.c         | 2 +-
   6 files changed, 7 insertions(+), 15 deletions(-)

Quoted 3 lines

Hmm, so I'm a bit nervous about this one since I think we really should be providing a TCR with EPD1 set if we're only giving you TTBR0. Relying on the driver to do this worries me. See my comments on the next patch.

Quoted 10 lines

The whole idea is that we already know we can't provide a *complete* TCR value (not least because anything above bit 31 is the wild west), thus there's really no point in io-pgtable trying to provide anything other than the parts it definitely controls. It makes sense to provide this partial TCR value "as if" for TTBR0, since that's the most common case, but ultimately io-pgatble doesn't know (or need to) which TTBR the caller intends to actually use for this table. Even if the caller *is* allocating it for TTBR0, io-pgtable doesn't know that they haven't got something live in TTBR1 already, so it still wouldn't be in a position to make the EPD1 call either way.

Quoted 4 lines

Ok, but the driver can happily rewrite/ignore what it gets back. I suppose an alternative would be scrapped the 'u64 tcr' and instead having a bunch of named bitfields for the stuff we're actually providing, although I'd still like EPDx to be in there.

I like the bitfield idea; it would certainly emphasise the "you have to do something more with this" angle that I'm pushing towards here, but still leave things framed in TCR terms without having to go to some more general abstraction. It really doesn't play into your EPD argument though - such a config would be providing TxSZ/TGx/IRGNx/ORGNx/SHx, but EPDy, for y = !x. For a driver to understand that and do the right thing with it is even more involved than for the driver to just set EPD1 by itself anyway.

If only LPAE had created these bits as enables rather than disables then things would be logical and we could all be happy, but here we are...

Quoted 43 lines · Robin Murphy

Quoted 37 lines · Will Deacon

Quoted 25 lines · Robin Murphy

Quoted 20 lines · Will Deacon

Quoted 10 lines · Robin Murphy

Although it's conceptually nice for the io_pgtable_cfg to provide a standard VMSA TCR value, the reality is that no VMSA-compliant IOMMU looks exactly like an Arm CPU, and they all have various other TCR controls which io-pgtable can't be expected to understand. Thus since there is an expectation that drivers will have to add to the given TCR value anyway, let's strip it down to just the essentials that are directly relevant to io-pgatble's inner workings - namely the address sizes, walk attributes, and where appropriate, format selection.

Signed-off-by Robin Murphy <[email protected]>

Quoted 8 lines

---
    drivers/iommu/arm-smmu-v3.c        | 7 +------
    drivers/iommu/arm-smmu.c           | 1 +
    drivers/iommu/arm-smmu.h           | 2 ++
    drivers/iommu/io-pgtable-arm-v7s.c | 6 ++----
    drivers/iommu/io-pgtable-arm.c     | 4 ----
    drivers/iommu/qcom_iommu.c         | 2 +-
    6 files changed, 7 insertions(+), 15 deletions(-)

Quoted 3 lines

Hmm, so I'm a bit nervous about this one since I think we really should be providing a TCR with EPD1 set if we're only giving you TTBR0. Relying on the driver to do this worries me. See my comments on the next patch.

Quoted 10 lines

The whole idea is that we already know we can't provide a *complete* TCR value (not least because anything above bit 31 is the wild west), thus there's really no point in io-pgtable trying to provide anything other than the parts it definitely controls. It makes sense to provide this partial TCR value "as if" for TTBR0, since that's the most common case, but ultimately io-pgatble doesn't know (or need to) which TTBR the caller intends to actually use for this table. Even if the caller *is* allocating it for TTBR0, io-pgtable doesn't know that they haven't got something live in TTBR1 already, so it still wouldn't be in a position to make the EPD1 call either way.

Quoted 4 lines

Ok, but the driver can happily rewrite/ignore what it gets back. I suppose an alternative would be scrapped the 'u64 tcr' and instead having a bunch of named bitfields for the stuff we're actually providing, although I'd still like EPDx to be in there.

Quoted 7 lines

I like the bitfield idea; it would certainly emphasise the "you have to do something more with this" angle that I'm pushing towards here, but still leave things framed in TCR terms without having to go to some more general abstraction. It really doesn't play into your EPD argument though - such a config would be providing TxSZ/TGx/IRGNx/ORGNx/SHx, but EPDy, for y = !x. For a driver to understand that and do the right thing with it is even more involved than for the driver to just set EPD1 by itself anyway.

Having considered the bitfield idea some more, I'm less attached to EPDx because we simply wouldn't be making a statement about them, rather than a (dangerous) zero value and expecting it to be ignored. So I think we're in agreement on that.

The only part I'm still stuck to is that I think io-pgtable should know whether it's targetting TTBR0 or TTBR1 so that it can sanitise input addresses correctly. Doing this in the driver code is possible, but I'd rather not start from that position, particularly as it would require things like sign-extension in the TLBI callbacks.

Quoted 2 lines

If only LPAE had created these bits as enables rather than disables then things would be logical and we could all be happy, but here we are...

I'm happy! :D:D:D

Quoted 52 lines · Will Deacon

Quoted 50 lines · Robin Murphy

Quoted 37 lines · Will Deacon

Quoted 25 lines · Robin Murphy

Quoted 20 lines · Will Deacon

Quoted 10 lines · Robin Murphy

Although it's conceptually nice for the io_pgtable_cfg to provide a standard VMSA TCR value, the reality is that no VMSA-compliant IOMMU looks exactly like an Arm CPU, and they all have various other TCR controls which io-pgtable can't be expected to understand. Thus since there is an expectation that drivers will have to add to the given TCR value anyway, let's strip it down to just the essentials that are directly relevant to io-pgatble's inner workings - namely the address sizes, walk attributes, and where appropriate, format selection.

Signed-off-by Robin Murphy <[email protected]>

Quoted 8 lines

---
   drivers/iommu/arm-smmu-v3.c        | 7 +------
   drivers/iommu/arm-smmu.c           | 1 +
   drivers/iommu/arm-smmu.h           | 2 ++
   drivers/iommu/io-pgtable-arm-v7s.c | 6 ++----
   drivers/iommu/io-pgtable-arm.c     | 4 ----
   drivers/iommu/qcom_iommu.c         | 2 +-
   6 files changed, 7 insertions(+), 15 deletions(-)

Quoted 3 lines

Hmm, so I'm a bit nervous about this one since I think we really should be providing a TCR with EPD1 set if we're only giving you TTBR0. Relying on the driver to do this worries me. See my comments on the next patch.

Quoted 10 lines

The whole idea is that we already know we can't provide a *complete* TCR value (not least because anything above bit 31 is the wild west), thus there's really no point in io-pgtable trying to provide anything other than the parts it definitely controls. It makes sense to provide this partial TCR value "as if" for TTBR0, since that's the most common case, but ultimately io-pgatble doesn't know (or need to) which TTBR the caller intends to actually use for this table. Even if the caller *is* allocating it for TTBR0, io-pgtable doesn't know that they haven't got something live in TTBR1 already, so it still wouldn't be in a position to make the EPD1 call either way.

Quoted 4 lines

Ok, but the driver can happily rewrite/ignore what it gets back. I suppose an alternative would be scrapped the 'u64 tcr' and instead having a bunch of named bitfields for the stuff we're actually providing, although I'd still like EPDx to be in there.

I like the bitfield idea; it would certainly emphasise the "you have to do something more with this" angle that I'm pushing towards here, but still leave things framed in TCR terms without having to go to some more general abstraction. It really doesn't play into your EPD argument though - such a config would be providing TxSZ/TGx/IRGNx/ORGNx/SHx, but EPDy, for y = !x. For a driver to understand that and do the right thing with it is even more involved than for the driver to just set EPD1 by itself anyway.

Quoted 4 lines

Having considered the bitfield idea some more, I'm less attached to EPDx because we simply wouldn't be making a statement about them, rather than a (dangerous) zero value and expecting it to be ignored. So I think we're in agreement on that.

Cool, I'll give bitfields a go for v2.

Quoted 5 lines

The only part I'm still stuck to is that I think io-pgtable should know whether it's targetting TTBR0 or TTBR1 so that it can sanitise input addresses correctly. Doing this in the driver code is possible, but I'd rather not start from that position, particularly as it would require things like sign-extension in the TLBI callbacks.

Quoted 53 lines · Robin Murphy

Quoted 52 lines · Will Deacon

Quoted 50 lines · Robin Murphy

Quoted 37 lines · Will Deacon

Quoted 25 lines · Robin Murphy

Quoted 20 lines · Will Deacon

Quoted 10 lines · Robin Murphy

Although it's conceptually nice for the io_pgtable_cfg to provide a standard VMSA TCR value, the reality is that no VMSA-compliant IOMMU looks exactly like an Arm CPU, and they all have various other TCR controls which io-pgtable can't be expected to understand. Thus since there is an expectation that drivers will have to add to the given TCR value anyway, let's strip it down to just the essentials that are directly relevant to io-pgatble's inner workings - namely the address sizes, walk attributes, and where appropriate, format selection.

Signed-off-by Robin Murphy <[email protected]>

Quoted 8 lines

---
   drivers/iommu/arm-smmu-v3.c        | 7 +------
   drivers/iommu/arm-smmu.c           | 1 +
   drivers/iommu/arm-smmu.h           | 2 ++
   drivers/iommu/io-pgtable-arm-v7s.c | 6 ++----
   drivers/iommu/io-pgtable-arm.c     | 4 ----
   drivers/iommu/qcom_iommu.c         | 2 +-
   6 files changed, 7 insertions(+), 15 deletions(-)

Quoted 3 lines

Hmm, so I'm a bit nervous about this one since I think we really should be providing a TCR with EPD1 set if we're only giving you TTBR0. Relying on the driver to do this worries me. See my comments on the next patch.

Quoted 10 lines

The whole idea is that we already know we can't provide a *complete* TCR value (not least because anything above bit 31 is the wild west), thus there's really no point in io-pgtable trying to provide anything other than the parts it definitely controls. It makes sense to provide this partial TCR value "as if" for TTBR0, since that's the most common case, but ultimately io-pgatble doesn't know (or need to) which TTBR the caller intends to actually use for this table. Even if the caller *is* allocating it for TTBR0, io-pgtable doesn't know that they haven't got something live in TTBR1 already, so it still wouldn't be in a position to make the EPD1 call either way.

Quoted 4 lines

Ok, but the driver can happily rewrite/ignore what it gets back. I suppose an alternative would be scrapped the 'u64 tcr' and instead having a bunch of named bitfields for the stuff we're actually providing, although I'd still like EPDx to be in there.

I like the bitfield idea; it would certainly emphasise the "you have to do something more with this" angle that I'm pushing towards here, but still leave things framed in TCR terms without having to go to some more general abstraction. It really doesn't play into your EPD argument though - such a config would be providing TxSZ/TGx/IRGNx/ORGNx/SHx, but EPDy, for y = !x. For a driver to understand that and do the right thing with it is even more involved than for the driver to just set EPD1 by itself anyway.

Quoted 12 lines

Quoted 4 lines

Having considered the bitfield idea some more, I'm less attached to EPDx because we simply wouldn't be making a statement about them, rather than a (dangerous) zero value and expecting it to be ignored. So I think we're in agreement on that.

Cool, I'll give bitfields a go for v2.

Quoted 5 lines

The only part I'm still stuck to is that I think io-pgtable should know whether it's targetting TTBR0 or TTBR1 so that it can sanitise input addresses correctly. Doing this in the driver code is possible, but I'd rather not start from that position, particularly as it would require things like sign-extension in the TLBI callbacks.

Bumping this as is our tradition in the -rc1 time frame before we get all distracted with other stuff. It sounds like the last agreement was for a TTBR1 hint for the EDP and the sign extension in the functions.

Quoted 67 lines · Jordan Crouse

Quoted 65 lines · Robin Murphy

Quoted 52 lines · Will Deacon

Quoted 43 lines · Robin Murphy

Quoted 37 lines · Will Deacon

Quoted 25 lines · Robin Murphy

Quoted 20 lines · Will Deacon

Quoted 10 lines · Robin Murphy

Although it's conceptually nice for the io_pgtable_cfg to provide a standard VMSA TCR value, the reality is that no VMSA-compliant IOMMU looks exactly like an Arm CPU, and they all have various other TCR controls which io-pgtable can't be expected to understand. Thus since there is an expectation that drivers will have to add to the given TCR value anyway, let's strip it down to just the essentials that are directly relevant to io-pgatble's inner workings - namely the address sizes, walk attributes, and where appropriate, format selection.

Signed-off-by Robin Murphy <[email protected]>

Quoted 8 lines

---
    drivers/iommu/arm-smmu-v3.c        | 7 +------
    drivers/iommu/arm-smmu.c           | 1 +
    drivers/iommu/arm-smmu.h           | 2 ++
    drivers/iommu/io-pgtable-arm-v7s.c | 6 ++----
    drivers/iommu/io-pgtable-arm.c     | 4 ----
    drivers/iommu/qcom_iommu.c         | 2 +-
    6 files changed, 7 insertions(+), 15 deletions(-)

Quoted 3 lines

Hmm, so I'm a bit nervous about this one since I think we really should be providing a TCR with EPD1 set if we're only giving you TTBR0. Relying on the driver to do this worries me. See my comments on the next patch.

Quoted 10 lines

The whole idea is that we already know we can't provide a *complete* TCR value (not least because anything above bit 31 is the wild west), thus there's really no point in io-pgtable trying to provide anything other than the parts it definitely controls. It makes sense to provide this partial TCR value "as if" for TTBR0, since that's the most common case, but ultimately io-pgatble doesn't know (or need to) which TTBR the caller intends to actually use for this table. Even if the caller *is* allocating it for TTBR0, io-pgtable doesn't know that they haven't got something live in TTBR1 already, so it still wouldn't be in a position to make the EPD1 call either way.

Quoted 4 lines

Ok, but the driver can happily rewrite/ignore what it gets back. I suppose an alternative would be scrapped the 'u64 tcr' and instead having a bunch of named bitfields for the stuff we're actually providing, although I'd still like EPDx to be in there.

Quoted 7 lines

I like the bitfield idea; it would certainly emphasise the "you have to do something more with this" angle that I'm pushing towards here, but still leave things framed in TCR terms without having to go to some more general abstraction. It really doesn't play into your EPD argument though - such a config would be providing TxSZ/TGx/IRGNx/ORGNx/SHx, but EPDy, for y = !x. For a driver to understand that and do the right thing with it is even more involved than for the driver to just set EPD1 by itself anyway.

Quoted 4 lines

Having considered the bitfield idea some more, I'm less attached to EPDx because we simply wouldn't be making a statement about them, rather than a (dangerous) zero value and expecting it to be ignored. So I think we're in agreement on that.

Cool, I'll give bitfields a go for v2.

Quoted 5 lines

The only part I'm still stuck to is that I think io-pgtable should know whether it's targetting TTBR0 or TTBR1 so that it can sanitise input addresses correctly. Doing this in the driver code is possible, but I'd rather not start from that position, particularly as it would require things like sign-extension in the TLBI callbacks.

Quoted 3 lines

Bumping this as is our tradition in the -rc1 time frame before we get all distracted with other stuff. It sounds like the last agreement was for a TTBR1 hint for the EDP and the sign extension in the functions.

If somebody respins this using bitfields and an explicit TTBR1 quirk then I'll merge it.

Quoted 68 lines · Will Deacon

Quoted 67 lines · Jordan Crouse

Quoted 65 lines · Robin Murphy

Quoted 52 lines · Will Deacon

Quoted 43 lines · Robin Murphy

Quoted 37 lines · Will Deacon

Quoted 25 lines · Robin Murphy

Quoted 20 lines · Will Deacon

Quoted 10 lines · Robin Murphy

Although it's conceptually nice for the io_pgtable_cfg to provide a standard VMSA TCR value, the reality is that no VMSA-compliant IOMMU looks exactly like an Arm CPU, and they all have various other TCR controls which io-pgtable can't be expected to understand. Thus since there is an expectation that drivers will have to add to the given TCR value anyway, let's strip it down to just the essentials that are directly relevant to io-pgatble's inner workings - namely the address sizes, walk attributes, and where appropriate, format selection.

Signed-off-by Robin Murphy <[email protected]>

Quoted 8 lines

---
    drivers/iommu/arm-smmu-v3.c        | 7 +------
    drivers/iommu/arm-smmu.c           | 1 +
    drivers/iommu/arm-smmu.h           | 2 ++
    drivers/iommu/io-pgtable-arm-v7s.c | 6 ++----
    drivers/iommu/io-pgtable-arm.c     | 4 ----
    drivers/iommu/qcom_iommu.c         | 2 +-
    6 files changed, 7 insertions(+), 15 deletions(-)

Quoted 3 lines

Hmm, so I'm a bit nervous about this one since I think we really should be providing a TCR with EPD1 set if we're only giving you TTBR0. Relying on the driver to do this worries me. See my comments on the next patch.

Quoted 10 lines

The whole idea is that we already know we can't provide a *complete* TCR value (not least because anything above bit 31 is the wild west), thus there's really no point in io-pgtable trying to provide anything other than the parts it definitely controls. It makes sense to provide this partial TCR value "as if" for TTBR0, since that's the most common case, but ultimately io-pgatble doesn't know (or need to) which TTBR the caller intends to actually use for this table. Even if the caller *is* allocating it for TTBR0, io-pgtable doesn't know that they haven't got something live in TTBR1 already, so it still wouldn't be in a position to make the EPD1 call either way.

Quoted 4 lines

Ok, but the driver can happily rewrite/ignore what it gets back. I suppose an alternative would be scrapped the 'u64 tcr' and instead having a bunch of named bitfields for the stuff we're actually providing, although I'd still like EPDx to be in there.

Quoted 7 lines

I like the bitfield idea; it would certainly emphasise the "you have to do something more with this" angle that I'm pushing towards here, but still leave things framed in TCR terms without having to go to some more general abstraction. It really doesn't play into your EPD argument though - such a config would be providing TxSZ/TGx/IRGNx/ORGNx/SHx, but EPDy, for y = !x. For a driver to understand that and do the right thing with it is even more involved than for the driver to just set EPD1 by itself anyway.

Quoted 4 lines

Having considered the bitfield idea some more, I'm less attached to EPDx because we simply wouldn't be making a statement about them, rather than a (dangerous) zero value and expecting it to be ignored. So I think we're in agreement on that.

Cool, I'll give bitfields a go for v2.

Quoted 5 lines

The only part I'm still stuck to is that I think io-pgtable should know whether it's targetting TTBR0 or TTBR1 so that it can sanitise input addresses correctly. Doing this in the driver code is possible, but I'd rather not start from that position, particularly as it would require things like sign-extension in the TLBI callbacks.

Quoted 6 lines

Quoted 3 lines

Bumping this as is our tradition in the -rc1 time frame before we get all distracted with other stuff. It sounds like the last agreement was for a TTBR1 hint for the EDP and the sign extension in the functions.

If somebody respins this using bitfields and an explicit TTBR1 quirk then I'll merge it.

Oops, the ping did register, I just didn't react outwardly ;)

I have been working on v2, and plan to have something ready next week - the holdup was that I started refactoring all the argument passing since the number of things we have to carry through from one end of map/unmap to the other is getting a bit silly, but I think I can still finish the TTBR1 quirk without that, so if I don't get it cracked imminently then I'll put it aside to revisit later.

Quoted 20 lines · Will Deacon

Quoted 10 lines · Robin Murphy

Although it's conceptually nice for the io_pgtable_cfg to provide a standard VMSA TCR value, the reality is that no VMSA-compliant IOMMU looks exactly like an Arm CPU, and they all have various other TCR controls which io-pgtable can't be expected to understand. Thus since there is an expectation that drivers will have to add to the given TCR value anyway, let's strip it down to just the essentials that are directly relevant to io-pgatble's inner workings - namely the address sizes, walk attributes, and where appropriate, format selection.

Signed-off-by Robin Murphy <[email protected]>

Quoted 8 lines

---
 drivers/iommu/arm-smmu-v3.c        | 7 +------
 drivers/iommu/arm-smmu.c           | 1 +
 drivers/iommu/arm-smmu.h           | 2 ++
 drivers/iommu/io-pgtable-arm-v7s.c | 6 ++----
 drivers/iommu/io-pgtable-arm.c     | 4 ----
 drivers/iommu/qcom_iommu.c         | 2 +-
 6 files changed, 7 insertions(+), 15 deletions(-)

Quoted 3 lines

Hmm, so I'm a bit nervous about this one since I think we really should be providing a TCR with EPD1 set if we're only giving you TTBR0. Relying on the driver to do this worries me. See my comments on the next patch.

The whole idea is that we already know we can't provide a *complete* TCR value (not least because anything above bit 31 is the wild west), thus there's really no point in io-pgtable trying to provide anything other than the parts it definitely controls. It makes sense to provide this partial TCR value "as if" for TTBR0, since that's the most common case, but ultimately io-pgatble doesn't know (or need to) which TTBR the caller intends to actually use for this table. Even if the caller *is* allocating it for TTBR0, io-pgtable doesn't know that they haven't got something live in TTBR1 already, so it still wouldn't be in a position to make the EPD1 call either way.

Ultimately, it's the IOMMU drivers who decide what they put in which TTBR, so it's the IOMMU drivers which have to take responsibility for EPD*. Sure you can worry about it, but you can equally worry about them them misprogramming the ASID or anything else...

Now that callers are free to use a given table for TTBR1 if they wish (all they need do is shift the provided attributes when constructing their final TCR value), the only remaining impediment is the address validation on map/unmap. The fact that the LPAE address space split is symmetric makes this easy to accommodate - by simplifying the current range checks into explicit tests that address bits above IAS are all zero, it then follows straightforwardly to add the inverse test to allow the all-ones case as well.

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/io-pgtable-arm.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/iommu/io-pgtable-arm.c +1 -0

index 09cb20671fbb..f39c50356351 100644
@@ -475,13 +475,13 @@ static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova,
 	arm_lpae_iopte *ptep = data->pgd;
 	int ret, lvl = ARM_LPAE_START_LVL(data);
 	arm_lpae_iopte prot;
+	long iaext = (long)iova >> data->iop.cfg.ias;
 

Quoted 33 lines · Robin Murphy

Now that callers are free to use a given table for TTBR1 if they wish (all they need do is shift the provided attributes when constructing their final TCR value), the only remaining impediment is the address validation on map/unmap. The fact that the LPAE address space split is symmetric makes this easy to accommodate - by simplifying the current range checks into explicit tests that address bits above IAS are all zero, it then follows straightforwardly to add the inverse test to allow the all-ones case as well.

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/iommu/io-pgtable-arm.c | 7 ++++---
  1 file changed, 4 insertions(+), 3 deletions(-)

drivers/iommu/io-pgtable-arm.c +2 -2

index 09cb20671fbb..f39c50356351 100644
@@ -475,13 +475,13 @@ static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova,
  	arm_lpae_iopte *ptep = data->pgd;
  	int ret, lvl = ARM_LPAE_START_LVL(data);
  	arm_lpae_iopte prot;
+	long iaext = (long)iova >> data->iop.cfg.ias;
 
  	/* If no access, then nothing to do */
  	if (!(iommu_prot & (IOMMU_READ | IOMMU_WRITE)))
  		return 0;
 
-	if (WARN_ON(iova >= (1ULL << data->iop.cfg.ias) ||
-		    paddr >= (1ULL << data->iop.cfg.oas)))
+	if (WARN_ON((iaext && ~iaext) || paddr >> data->iop.cfg.oas))
  		return -ERANGE;
 

drivers/iommu/io-pgtable-arm.c static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova, · line 487

Quoted 1 line

drivers/iommu/io-pgtable-arm.c +0 -0

  	prot = arm_lpae_prot_to_pte(data, iommu_prot);

We'll want to cast away the sign extended bits before mapping the iova, this might be a good patch for that too as long as we are calculating the iaext.

Quoted 11 lines

drivers/iommu/io-pgtable-arm.c +2 -1

@@ -647,8 +647,9 @@ static size_t arm_lpae_unmap(struct io_pgtable_ops *ops, unsigned long iova,
  	struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
  	arm_lpae_iopte *ptep = data->pgd;
  	int lvl = ARM_LPAE_START_LVL(data);
+	long iaext = (long)iova >> data->iop.cfg.ias;
 
-	if (WARN_ON(iova >= (1ULL << data->iop.cfg.ias)))
+	if (WARN_ON(iaext && ~iaext))
  		return 0;
 

drivers/iommu/io-pgtable-arm.c static size_t arm_lpae_unmap(struct io_pgtable_ops *ops, unsigned long iova, · line 655

Quoted 1 line

drivers/iommu/io-pgtable-arm.c +0 -0

  	return __arm_lpae_unmap(data, iova, size, lvl, ptep);

And here too.

Jordan

Now that callers are free to use a given table for TTBR1 if they wish (all they need do is shift the provided attributes when constructing their final TCR value), the only remaining impediment is the address validation on map/unmap. The fact that the LPAE address space split is symmetric makes this easy to accommodate - by simplifying the current range checks into explicit tests that address bits above IAS are all zero, it then follows straightforwardly to add the inverse test to allow the all-ones case as well.

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/io-pgtable-arm.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/iommu/io-pgtable-arm.c +0 -0

index 09cb20671fbb..f39c50356351 100644
@@ -475,13 +475,13 @@ static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova,
 	arm_lpae_iopte *ptep = data->pgd;
 	int ret, lvl = ARM_LPAE_START_LVL(data);

Quoted 21 lines · Robin Murphy

Now that callers are free to use a given table for TTBR1 if they wish (all they need do is shift the provided attributes when constructing their final TCR value), the only remaining impediment is the address validation on map/unmap. The fact that the LPAE address space split is symmetric makes this easy to accommodate - by simplifying the current range checks into explicit tests that address bits above IAS are all zero, it then follows straightforwardly to add the inverse test to allow the all-ones case as well.

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/iommu/io-pgtable-arm.c | 7 ++++---
  1 file changed, 4 insertions(+), 3 deletions(-)

drivers/iommu/io-pgtable-arm.c +0 -0

index 09cb20671fbb..f39c50356351 100644
@@ -475,13 +475,13 @@ static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova,
  	arm_lpae_iopte *ptep = data->pgd;
  	int ret, lvl = ARM_LPAE_START_LVL(data);

drivers/iommu/io-pgtable-arm.c static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova, · line 484

Quoted 10 lines

drivers/iommu/io-pgtable-arm.c +2 -2

  	arm_lpae_iopte prot;
+	long iaext = (long)iova >> data->iop.cfg.ias;
 
  	/* If no access, then nothing to do */
  	if (!(iommu_prot & (IOMMU_READ | IOMMU_WRITE)))
  		return 0;
 
-	if (WARN_ON(iova >= (1ULL << data->iop.cfg.ias) ||
-		    paddr >= (1ULL << data->iop.cfg.oas)))
+	if (WARN_ON((iaext && ~iaext) || paddr >> data->iop.cfg.oas))

I had to read that '&&' twice, but I see what you're doing now :)

drivers/iommu/io-pgtable-arm.c static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova, · line 485

Quoted 1 line

drivers/iommu/io-pgtable-arm.c +0 -0

  		return -ERANGE;

This doesn't seem sufficient to prevent a mixture of TTBR1 and TTBR0 addresses from being mapped in the same TTBR. Perhaps we need a quirk for TTBR1, which could then take care of setting EPDx appropriately?

Quoted 35 lines · Will Deacon

Quoted 19 lines · Robin Murphy

Now that callers are free to use a given table for TTBR1 if they wish (all they need do is shift the provided attributes when constructing their final TCR value), the only remaining impediment is the address validation on map/unmap. The fact that the LPAE address space split is symmetric makes this easy to accommodate - by simplifying the current range checks into explicit tests that address bits above IAS are all zero, it then follows straightforwardly to add the inverse test to allow the all-ones case as well.

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/iommu/io-pgtable-arm.c | 7 ++++---
  1 file changed, 4 insertions(+), 3 deletions(-)

drivers/iommu/io-pgtable-arm.c +0 -0

index 09cb20671fbb..f39c50356351 100644

drivers/iommu/io-pgtable-arm.c static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova, · line 482

Quoted 10 lines

drivers/iommu/io-pgtable-arm.c +2 -2

@@ -475,13 +475,13 @@ static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova,
  	arm_lpae_iopte *ptep = data->pgd;
  	int ret, lvl = ARM_LPAE_START_LVL(data);
  	arm_lpae_iopte prot;
+	long iaext = (long)iova >> data->iop.cfg.ias;
  	/* If no access, then nothing to do */
  	if (!(iommu_prot & (IOMMU_READ | IOMMU_WRITE)))
  		return 0;
-	if (WARN_ON(iova >= (1ULL << data->iop.cfg.ias) ||
-		    paddr >= (1ULL << data->iop.cfg.oas)))
+	if (WARN_ON((iaext && ~iaext) || paddr >> data->iop.cfg.oas))

I had to read that '&&' twice, but I see what you're doing now :)

Quoted 1 line

drivers/iommu/io-pgtable-arm.c +0 -0

  		return -ERANGE;

Quoted 3 lines

This doesn't seem sufficient to prevent a mixture of TTBR1 and TTBR0 addresses from being mapped in the same TTBR. Perhaps we need a quirk for TTBR1, which could then take care of setting EPDx appropriately?

Right, that's the one downside of going for the minimalist "io-pgtable doesn't even have to know" approach. On reflection, though, in that paradigm it should probably be the caller's responsibility to convert TTBR1 addresses to preserve the "as if TTBR0" illusion anyway :/

The advantage of not having a quirk is that it allows split address spaces to fit more closely with the aux_domain idea, i.e. we could allocate and initialise a domain without having to assume, or even care, whether it will end up attached as a primary or aux domain. It *might* even be potentially useful to have a domain attached to TTBR0 of one device's context and TTBR1 of another's at the same time, although that's pretty niche.