Based on previous attempts and discussions this is the latest attempt at
inheriting stream mappings set up by the bootloader, for e.g. boot splash or
efifb.
Per Will's request this builds on the work by Jordan and Rob for the Adreno
SMMU support. It applies cleanly ontop of v16 of their series, which can be
found at
https://lore.kernel.org/linux-arm-msm/[email protected]/
Bjorn Andersson (8):
iommu/arm-smmu: Refactor context bank allocation
iommu/arm-smmu: Delay modifying domain during init
iommu/arm-smmu: Consult context bank allocator for identify domains
iommu/arm-smmu-qcom: Emulate bypass by using context banks
iommu/arm-smmu-qcom: Consistently initialize stream mappings
iommu/arm-smmu: Add impl hook for inherit boot mappings
iommu/arm-smmu: Provide helper for allocating identity domain
iommu/arm-smmu-qcom: Setup identity domain for boot mappings
drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 111 ++++++++++++++++++-
drivers/iommu/arm/arm-smmu/arm-smmu.c | 122 ++++++++++++++-------
drivers/iommu/arm/arm-smmu/arm-smmu.h | 14 ++-
3 files changed, 205 insertions(+), 42 deletions(-)
--
2.28.0
Add a new operation to allow platform implementations to inherit any
stream mappings from the boot loader.
Signed-off-by: Bjorn Andersson <[email protected]>
---
Changes since v2:
- New patch/interface
drivers/iommu/arm/arm-smmu/arm-smmu.c | 11 ++++++-----
drivers/iommu/arm/arm-smmu/arm-smmu.h | 6 ++++++
2 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index eb5c6ca5c138..4c4d302cd747 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -85,11 +85,6 @@ static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu)
pm_runtime_put_autosuspend(smmu->dev);
}
-static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
-{
- return container_of(dom, struct arm_smmu_domain, domain);
-}
-
static struct platform_driver arm_smmu_driver;
static struct iommu_ops arm_smmu_ops;
@@ -2188,6 +2183,12 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
if (err)
return err;
+ if (smmu->impl->inherit_mappings) {
+ err = smmu->impl->inherit_mappings(smmu);
+ if (err)
+ return err;
+ }
+
if (smmu->version == ARM_SMMU_V2) {
if (smmu->num_context_banks > smmu->num_context_irqs) {
dev_err(dev,
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h b/drivers/iommu/arm/arm-smmu/arm-smmu.h
index 235d9a3a6ab6..f58164976e74 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
@@ -378,6 +378,11 @@ struct arm_smmu_domain {
struct iommu_domain domain;
};
+static inline struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
+{
+ return container_of(dom, struct arm_smmu_domain, domain);
+}
+
struct arm_smmu_master_cfg {
struct arm_smmu_device *smmu;
s16 smendx[];
@@ -442,6 +447,7 @@ struct arm_smmu_impl {
int (*alloc_context_bank)(struct arm_smmu_domain *smmu_domain,
struct arm_smmu_device *smmu,
struct device *dev, int start);
+ int (*inherit_mappings)(struct arm_smmu_device *smmu);
};
#define INVALID_SMENDX -1
--
2.28.0
Some platform implementations needs to be able to allocate a domain for
emulating identity mappings using a context bank without translation.
Provide a helper function to allocate such a domain.
Signed-off-by: Bjorn Andersson <[email protected]>
---
Changes since v2:
- Extracted from previous arm_smmu_setup_identity() implementation
drivers/iommu/arm/arm-smmu/arm-smmu.c | 25 +++++++++++++++++++++++++
drivers/iommu/arm/arm-smmu/arm-smmu.h | 2 ++
2 files changed, 27 insertions(+)
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 4c4d302cd747..3c06146dfdb9 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -1924,6 +1924,31 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
return 0;
}
+struct iommu_domain *arm_smmu_alloc_identity_domain(struct arm_smmu_device *smmu)
+{
+ struct iommu_domain *identity;
+ int ret;
+
+ /* Create a IDENTITY domain to use for all inherited streams */
+ identity = arm_smmu_domain_alloc(IOMMU_DOMAIN_IDENTITY);
+ if (!identity) {
+ dev_err(smmu->dev, "failed to create identity domain\n");
+ return ERR_PTR(-ENOMEM);
+ }
+
+ identity->pgsize_bitmap = smmu->pgsize_bitmap;
+ identity->type = IOMMU_DOMAIN_IDENTITY;
+ identity->ops = &arm_smmu_ops;
+
+ ret = arm_smmu_init_domain_context(identity, smmu, NULL);
+ if (ret < 0) {
+ dev_err(smmu->dev, "failed to initialize identity domain: %d\n", ret);
+ return ERR_PTR(ret);
+ }
+
+ return identity;
+}
+
struct arm_smmu_match_data {
enum arm_smmu_arch_version version;
enum arm_smmu_implementation model;
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h b/drivers/iommu/arm/arm-smmu/arm-smmu.h
index f58164976e74..fbdf3d7ca70d 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
@@ -537,4 +537,6 @@ struct arm_smmu_device *qcom_adreno_smmu_impl_init(struct arm_smmu_device *smmu)
void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx);
int arm_mmu500_reset(struct arm_smmu_device *smmu);
+struct iommu_domain *arm_smmu_alloc_identity_domain(struct arm_smmu_device *smmu);
+
#endif /* _ARM_SMMU_H */
--
2.28.0
For implementations of the ARM SMMU where stream mappings of bypass type
are prohibited identity domains can be implemented by using context
banks with translation disabled.
Postpone the decision to skip allocating a context bank until the
implementation specific context bank allocator has been consulted and if
it decides to use a context bank for the identity map, don't enable
translation (i.e. omit ARM_SMMU_SCTLR_M).
Signed-off-by: Bjorn Andersson <[email protected]>
---
Changes since v2:
- Tie this to alloc_context_bank rather than carrying a Qualcomm specific quirk
in the generic code.
drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 4 ++++
drivers/iommu/arm/arm-smmu/arm-smmu.c | 23 +++++++++++++++-------
drivers/iommu/arm/arm-smmu/arm-smmu.h | 3 +++
3 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index 0663d7d26908..229fc8ff8cea 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -94,8 +94,12 @@ static int qcom_adreno_smmu_alloc_context_bank(struct arm_smmu_domain *smmu_doma
struct arm_smmu_device *smmu,
struct device *dev, int start)
{
+ struct iommu_domain *domain = &smmu_domain->domain;
int count;
+ if (domain->type == IOMMU_DOMAIN_IDENTITY)
+ return ARM_SMMU_CBNDX_BYPASS;
+
/*
* Assign context bank 0 to the GPU device so the GPU hardware can
* switch pagetables
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index add2e1807e21..eb5c6ca5c138 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -611,7 +611,9 @@ void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
/* SCTLR */
reg = ARM_SMMU_SCTLR_CFIE | ARM_SMMU_SCTLR_CFRE | ARM_SMMU_SCTLR_AFE |
- ARM_SMMU_SCTLR_TRE | ARM_SMMU_SCTLR_M;
+ ARM_SMMU_SCTLR_TRE;
+ if (cfg->m)
+ reg |= ARM_SMMU_SCTLR_M;
if (stage1)
reg |= ARM_SMMU_SCTLR_S1_ASIDPNE;
if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
@@ -627,9 +629,14 @@ static int arm_smmu_alloc_context_bank(struct arm_smmu_domain *smmu_domain,
struct arm_smmu_device *smmu,
struct device *dev, unsigned int start)
{
+ struct iommu_domain *domain = &smmu_domain->domain;
+
if (smmu->impl && smmu->impl->alloc_context_bank)
return smmu->impl->alloc_context_bank(smmu_domain, smmu, dev, start);
+ if (domain->type == IOMMU_DOMAIN_IDENTITY)
+ return ARM_SMMU_CBNDX_BYPASS;
+
return __arm_smmu_alloc_bitmap(smmu->context_map, start, smmu->num_context_banks);
}
@@ -653,12 +660,6 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
if (smmu_domain->smmu)
goto out_unlock;
- if (domain->type == IOMMU_DOMAIN_IDENTITY) {
- smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
- smmu_domain->smmu = smmu;
- goto out_unlock;
- }
-
/*
* Mapping the requested stage onto what we support is surprisingly
* complicated, mainly because the spec allows S1+S2 SMMUs without
@@ -757,6 +758,10 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
ret = arm_smmu_alloc_context_bank(smmu_domain, smmu, dev, start);
if (ret < 0) {
goto out_unlock;
+ } else if (ret == ARM_SMMU_CBNDX_BYPASS) {
+ smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
+ smmu_domain->smmu = smmu;
+ goto out_unlock;
}
smmu_domain->smmu = smmu;
@@ -813,6 +818,10 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
domain->geometry.force_aperture = true;
+ /* Enable translation for non-identity context banks */
+ if (domain->type != IOMMU_DOMAIN_IDENTITY)
+ cfg->m = true;
+
/* Initialise the context bank with our page table cfg */
arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
arm_smmu_write_context_bank(smmu, cfg->cbndx);
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h b/drivers/iommu/arm/arm-smmu/arm-smmu.h
index ddf2ca4c923d..235d9a3a6ab6 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
@@ -243,6 +243,8 @@ enum arm_smmu_cbar_type {
#define TLB_LOOP_TIMEOUT 1000000 /* 1s! */
#define TLB_SPIN_COUNT 10
+#define ARM_SMMU_CBNDX_BYPASS 0xffff
+
/* Shared driver definitions */
enum arm_smmu_arch_version {
ARM_SMMU_V1,
@@ -346,6 +348,7 @@ struct arm_smmu_cfg {
u32 sctlr_clr; /* bits to mask in SCTLR */
enum arm_smmu_cbar_type cbar;
enum arm_smmu_context_fmt fmt;
+ bool m;
};
#define ARM_SMMU_INVALID_IRPTNDX 0xff
--
2.28.0
Delay modifications to the domain during arm_smmu_init_domain_context()
until we've allocated a context bank. This will allow us to postpone the
special handling of identity domains until the platform specific context
bank allocator has been executed, in a later patch.
Signed-off-by: Bjorn Andersson <[email protected]>
---
Changes since v2:
- New patch to allow us to rely on the impl specific alloc_context_bank().
drivers/iommu/arm/arm-smmu/arm-smmu.c | 40 +++++++++++++++------------
1 file changed, 23 insertions(+), 17 deletions(-)
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index e19d7bdc7674..add2e1807e21 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -645,6 +645,9 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
irqreturn_t (*context_fault)(int irq, void *dev);
+ struct arm_smmu_cfg new_cfg = *cfg;
+ enum arm_smmu_domain_stage new_stage = smmu_domain->stage;
+ const struct iommu_flush_ops *flush_ops;
mutex_lock(&smmu_domain->init_mutex);
if (smmu_domain->smmu)
@@ -675,9 +678,9 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
* Note that you can't actually request stage-2 mappings.
*/
if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S1))
- smmu_domain->stage = ARM_SMMU_DOMAIN_S2;
+ new_stage = ARM_SMMU_DOMAIN_S2;
if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S2))
- smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
+ new_stage = ARM_SMMU_DOMAIN_S1;
/*
* Choosing a suitable context format is even more fiddly. Until we
@@ -688,32 +691,32 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
* support to be a superset of AArch32 support...
*/
if (smmu->features & ARM_SMMU_FEAT_FMT_AARCH32_L)
- cfg->fmt = ARM_SMMU_CTX_FMT_AARCH32_L;
+ new_cfg.fmt = ARM_SMMU_CTX_FMT_AARCH32_L;
if (IS_ENABLED(CONFIG_IOMMU_IO_PGTABLE_ARMV7S) &&
!IS_ENABLED(CONFIG_64BIT) && !IS_ENABLED(CONFIG_ARM_LPAE) &&
(smmu->features & ARM_SMMU_FEAT_FMT_AARCH32_S) &&
- (smmu_domain->stage == ARM_SMMU_DOMAIN_S1))
- cfg->fmt = ARM_SMMU_CTX_FMT_AARCH32_S;
- if ((IS_ENABLED(CONFIG_64BIT) || cfg->fmt == ARM_SMMU_CTX_FMT_NONE) &&
+ (new_stage == ARM_SMMU_DOMAIN_S1))
+ new_cfg.fmt = ARM_SMMU_CTX_FMT_AARCH32_S;
+ if ((IS_ENABLED(CONFIG_64BIT) || new_cfg.fmt == ARM_SMMU_CTX_FMT_NONE) &&
(smmu->features & (ARM_SMMU_FEAT_FMT_AARCH64_64K |
ARM_SMMU_FEAT_FMT_AARCH64_16K |
ARM_SMMU_FEAT_FMT_AARCH64_4K)))
- cfg->fmt = ARM_SMMU_CTX_FMT_AARCH64;
+ new_cfg.fmt = ARM_SMMU_CTX_FMT_AARCH64;
- if (cfg->fmt == ARM_SMMU_CTX_FMT_NONE) {
+ if (new_cfg.fmt == ARM_SMMU_CTX_FMT_NONE) {
ret = -EINVAL;
goto out_unlock;
}
- switch (smmu_domain->stage) {
+ switch (new_stage) {
case ARM_SMMU_DOMAIN_S1:
- cfg->cbar = CBAR_TYPE_S1_TRANS_S2_BYPASS;
+ new_cfg.cbar = CBAR_TYPE_S1_TRANS_S2_BYPASS;
start = smmu->num_s2_context_banks;
ias = smmu->va_size;
oas = smmu->ipa_size;
- if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64) {
+ if (new_cfg.fmt == ARM_SMMU_CTX_FMT_AARCH64) {
fmt = ARM_64_LPAE_S1;
- } else if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_L) {
+ } else if (new_cfg.fmt == ARM_SMMU_CTX_FMT_AARCH32_L) {
fmt = ARM_32_LPAE_S1;
ias = min(ias, 32UL);
oas = min(oas, 40UL);
@@ -722,7 +725,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
ias = min(ias, 32UL);
oas = min(oas, 32UL);
}
- smmu_domain->flush_ops = &arm_smmu_s1_tlb_ops;
+ flush_ops = &arm_smmu_s1_tlb_ops;
break;
case ARM_SMMU_DOMAIN_NESTED:
/*
@@ -730,11 +733,11 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
* involved.
*/
case ARM_SMMU_DOMAIN_S2:
- cfg->cbar = CBAR_TYPE_S2_TRANS;
+ new_cfg.cbar = CBAR_TYPE_S2_TRANS;
start = 0;
ias = smmu->ipa_size;
oas = smmu->pa_size;
- if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64) {
+ if (new_cfg.fmt == ARM_SMMU_CTX_FMT_AARCH64) {
fmt = ARM_64_LPAE_S2;
} else {
fmt = ARM_32_LPAE_S2;
@@ -742,9 +745,9 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
oas = min(oas, 40UL);
}
if (smmu->version == ARM_SMMU_V2)
- smmu_domain->flush_ops = &arm_smmu_s2_tlb_ops_v2;
+ flush_ops = &arm_smmu_s2_tlb_ops_v2;
else
- smmu_domain->flush_ops = &arm_smmu_s2_tlb_ops_v1;
+ flush_ops = &arm_smmu_s2_tlb_ops_v1;
break;
default:
ret = -EINVAL;
@@ -757,6 +760,9 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
}
smmu_domain->smmu = smmu;
+ smmu_domain->cfg = new_cfg;
+ smmu_domain->stage = new_stage;
+ smmu_domain->flush_ops = flush_ops;
cfg->cbndx = ret;
if (smmu->version < ARM_SMMU_V2) {
--
2.28.0
On Fri, Sep 4, 2020 at 8:55 AM Bjorn Andersson
<[email protected]> wrote:
>
> Based on previous attempts and discussions this is the latest attempt at
> inheriting stream mappings set up by the bootloader, for e.g. boot splash or
> efifb.
>
> Per Will's request this builds on the work by Jordan and Rob for the Adreno
> SMMU support. It applies cleanly ontop of v16 of their series, which can be
> found at
> https://lore.kernel.org/linux-arm-msm/[email protected]/
>
> Bjorn Andersson (8):
> iommu/arm-smmu: Refactor context bank allocation
> iommu/arm-smmu: Delay modifying domain during init
> iommu/arm-smmu: Consult context bank allocator for identify domains
> iommu/arm-smmu-qcom: Emulate bypass by using context banks
> iommu/arm-smmu-qcom: Consistently initialize stream mappings
> iommu/arm-smmu: Add impl hook for inherit boot mappings
> iommu/arm-smmu: Provide helper for allocating identity domain
> iommu/arm-smmu-qcom: Setup identity domain for boot mappings
I have squashed 1/8 into v17 of the adreno-smmu series as suggested by
Bjorn, the remainder are:
Reviewed-by: Rob Clark <[email protected]>
and on the lenovo c630,
Tested-by: Rob Clark <[email protected]>
> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 111 ++++++++++++++++++-
> drivers/iommu/arm/arm-smmu/arm-smmu.c | 122 ++++++++++++++-------
> drivers/iommu/arm/arm-smmu/arm-smmu.h | 14 ++-
> 3 files changed, 205 insertions(+), 42 deletions(-)
>
> --
> 2.28.0
>
> _______________________________________________
> iommu mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
On 2020-09-04 16:55, Bjorn Andersson wrote:
> Based on previous attempts and discussions this is the latest attempt at
> inheriting stream mappings set up by the bootloader, for e.g. boot splash or
> efifb.
>
> Per Will's request this builds on the work by Jordan and Rob for the Adreno
> SMMU support. It applies cleanly ontop of v16 of their series, which can be
> found at
> https://lore.kernel.org/linux-arm-msm/[email protected]/
>
> Bjorn Andersson (8):
> iommu/arm-smmu: Refactor context bank allocation
> iommu/arm-smmu: Delay modifying domain during init
> iommu/arm-smmu: Consult context bank allocator for identify domains
> iommu/arm-smmu-qcom: Emulate bypass by using context banks
> iommu/arm-smmu-qcom: Consistently initialize stream mappings
> iommu/arm-smmu: Add impl hook for inherit boot mappings
> iommu/arm-smmu: Provide helper for allocating identity domain
> iommu/arm-smmu-qcom: Setup identity domain for boot mappings
>
> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 111 ++++++++++++++++++-
> drivers/iommu/arm/arm-smmu/arm-smmu.c | 122 ++++++++++++++-------
> drivers/iommu/arm/arm-smmu/arm-smmu.h | 14 ++-
> 3 files changed, 205 insertions(+), 42 deletions(-)
>
Tested on the OnePlus 6 (SDM845), allows booting with display enabled.
Hi Bjorn,
On 9/4/2020 6:55 PM, Bjorn Andersson wrote:
> Based on previous attempts and discussions this is the latest attempt at
> inheriting stream mappings set up by the bootloader, for e.g. boot splash or
> efifb.
>
> Per Will's request this builds on the work by Jordan and Rob for the Adreno
> SMMU support. It applies cleanly ontop of v16 of their series, which can be
> found at
> https://lore.kernel.org/linux-arm-msm/[email protected]/
Is there a git repo available with all the patches put together?
---
Thanks & Best Regards, Laurentiu
On Fri, Sep 4, 2020 at 8:56 AM Bjorn Andersson
<[email protected]> wrote:
>
> Based on previous attempts and discussions this is the latest attempt at
> inheriting stream mappings set up by the bootloader, for e.g. boot splash or
> efifb.
>
> Per Will's request this builds on the work by Jordan and Rob for the Adreno
> SMMU support. It applies cleanly ontop of v16 of their series, which can be
> found at
> https://lore.kernel.org/linux-arm-msm/[email protected]/
>
Apologies, I just found this today. I've pulled your patches and Rob's
into my own tree here:
https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/db845c-mainline-WIP
And they all work fine on the db845c.
So for your whole series:
Tested-by: John Stultz <[email protected]>
thanks
-john
Hi Bjorn,
On 2020-09-04 21:25, Bjorn Andersson wrote:
> Based on previous attempts and discussions this is the latest attempt
> at
> inheriting stream mappings set up by the bootloader, for e.g. boot
> splash or
> efifb.
>
> Per Will's request this builds on the work by Jordan and Rob for the
> Adreno
> SMMU support. It applies cleanly ontop of v16 of their series, which
> can be
> found at
> https://lore.kernel.org/linux-arm-msm/[email protected]/
>
Thanks for working on this, I have tested this on qcom platforms
where firmware does these shenanigans(most android) and this series
works well and where firmware doesn't do all this (chrome) and no
regressions there. Review and test tags given on individual patches.
Thanks,
Sai
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member
of Code Aurora Forum, hosted by The Linux Foundation
On 2020-09-04 21:25, Bjorn Andersson wrote:
> Delay modifications to the domain during arm_smmu_init_domain_context()
> until we've allocated a context bank. This will allow us to postpone
> the
> special handling of identity domains until the platform specific
> context
> bank allocator has been executed, in a later patch.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
Reviewed-by: Sai Prakash Ranjan <[email protected]>
Tested-by: Sai Prakash Ranjan <[email protected]>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member
of Code Aurora Forum, hosted by The Linux Foundation
On 2020-09-04 21:25, Bjorn Andersson wrote:
> For implementations of the ARM SMMU where stream mappings of bypass
> type
> are prohibited identity domains can be implemented by using context
> banks with translation disabled.
>
> Postpone the decision to skip allocating a context bank until the
> implementation specific context bank allocator has been consulted and
> if
> it decides to use a context bank for the identity map, don't enable
> translation (i.e. omit ARM_SMMU_SCTLR_M).
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
>
Minor nit in the subject: identify -> identity
Reviewed-by: Sai Prakash Ranjan <[email protected]>
Tested-by: Sai Prakash Ranjan <[email protected]>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member
of Code Aurora Forum, hosted by The Linux Foundation
On 2020-09-04 21:25, Bjorn Andersson wrote:
> For implementations of the ARM SMMU where stream mappings of bypass
> type
> are prohibited identity domains can be implemented by using context
> banks with translation disabled.
>
> Postpone the decision to skip allocating a context bank until the
> implementation specific context bank allocator has been consulted and
> if
> it decides to use a context bank for the identity map, don't enable
> translation (i.e. omit ARM_SMMU_SCTLR_M).
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
>
<snip>...
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h
> b/drivers/iommu/arm/arm-smmu/arm-smmu.h
> index ddf2ca4c923d..235d9a3a6ab6 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
> @@ -243,6 +243,8 @@ enum arm_smmu_cbar_type {
> #define TLB_LOOP_TIMEOUT 1000000 /* 1s! */
> #define TLB_SPIN_COUNT 10
>
> +#define ARM_SMMU_CBNDX_BYPASS 0xffff
> +
> /* Shared driver definitions */
> enum arm_smmu_arch_version {
> ARM_SMMU_V1,
> @@ -346,6 +348,7 @@ struct arm_smmu_cfg {
> u32 sctlr_clr; /* bits to mask in SCTLR */
> enum arm_smmu_cbar_type cbar;
> enum arm_smmu_context_fmt fmt;
> + bool m;
Can we use mmu_enable instead of m here to be more descriptive?
Thanks,
Sai
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member
of Code Aurora Forum, hosted by The Linux Foundation
On 2020-09-04 21:25, Bjorn Andersson wrote:
> Add a new operation to allow platform implementations to inherit any
> stream mappings from the boot loader.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
>
Reviewed-by: Sai Prakash Ranjan <[email protected]>
Tested-by: Sai Prakash Ranjan <[email protected]>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member
of Code Aurora Forum, hosted by The Linux Foundation
On 2020-09-04 21:25, Bjorn Andersson wrote:
> Some platform implementations needs to be able to allocate a domain for
> emulating identity mappings using a context bank without translation.
> Provide a helper function to allocate such a domain.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
>
Reviewed-by: Sai Prakash Ranjan <[email protected]>
Tested-by: Sai Prakash Ranjan <[email protected]>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member
of Code Aurora Forum, hosted by The Linux Foundation
On Fri, 4 Sep 2020 at 21:25, Bjorn Andersson <[email protected]> wrote:
>
> Based on previous attempts and discussions this is the latest attempt at
> inheriting stream mappings set up by the bootloader, for e.g. boot splash or
> efifb.
>
> Per Will's request this builds on the work by Jordan and Rob for the Adreno
> SMMU support. It applies cleanly ontop of v16 of their series, which can be
> found at
> https://lore.kernel.org/linux-arm-msm/[email protected]/
>
Boot tested the series on Xiaomi Poco F1 phone (sdm845)
Tested-by: Amit Pundir <[email protected]>
> Bjorn Andersson (8):
> iommu/arm-smmu: Refactor context bank allocation
> iommu/arm-smmu: Delay modifying domain during init
> iommu/arm-smmu: Consult context bank allocator for identify domains
> iommu/arm-smmu-qcom: Emulate bypass by using context banks
> iommu/arm-smmu-qcom: Consistently initialize stream mappings
> iommu/arm-smmu: Add impl hook for inherit boot mappings
> iommu/arm-smmu: Provide helper for allocating identity domain
> iommu/arm-smmu-qcom: Setup identity domain for boot mappings
>
> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 111 ++++++++++++++++++-
> drivers/iommu/arm/arm-smmu/arm-smmu.c | 122 ++++++++++++++-------
> drivers/iommu/arm/arm-smmu/arm-smmu.h | 14 ++-
> 3 files changed, 205 insertions(+), 42 deletions(-)
>
> --
> 2.28.0
>
On 2020-09-04 16:55, Bjorn Andersson wrote:
> Add a new operation to allow platform implementations to inherit any
> stream mappings from the boot loader.
Is there a reason we need an explicit step for this? The aim of the
cfg_probe hook is that the SMMU software state should all be set up by
then, and you can mess about with it however you like before
arm_smmu_reset() actually commits anything to hardware. I would have
thought you could permanently steal a context bank, configure it as your
bypass hole, read out the previous SME configuration and tweak
smmu->smrs and smmu->s2crs appropriately all together "invisibly" at
that point. If that can't work, I'm very curious as to what I've overlooked.
Robin.
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
>
> Changes since v2:
> - New patch/interface
>
> drivers/iommu/arm/arm-smmu/arm-smmu.c | 11 ++++++-----
> drivers/iommu/arm/arm-smmu/arm-smmu.h | 6 ++++++
> 2 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index eb5c6ca5c138..4c4d302cd747 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -85,11 +85,6 @@ static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu)
> pm_runtime_put_autosuspend(smmu->dev);
> }
>
> -static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
> -{
> - return container_of(dom, struct arm_smmu_domain, domain);
> -}
> -
> static struct platform_driver arm_smmu_driver;
> static struct iommu_ops arm_smmu_ops;
>
> @@ -2188,6 +2183,12 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
> if (err)
> return err;
>
> + if (smmu->impl->inherit_mappings) {
> + err = smmu->impl->inherit_mappings(smmu);
> + if (err)
> + return err;
> + }
> +
> if (smmu->version == ARM_SMMU_V2) {
> if (smmu->num_context_banks > smmu->num_context_irqs) {
> dev_err(dev,
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h b/drivers/iommu/arm/arm-smmu/arm-smmu.h
> index 235d9a3a6ab6..f58164976e74 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
> @@ -378,6 +378,11 @@ struct arm_smmu_domain {
> struct iommu_domain domain;
> };
>
> +static inline struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
> +{
> + return container_of(dom, struct arm_smmu_domain, domain);
> +}
> +
> struct arm_smmu_master_cfg {
> struct arm_smmu_device *smmu;
> s16 smendx[];
> @@ -442,6 +447,7 @@ struct arm_smmu_impl {
> int (*alloc_context_bank)(struct arm_smmu_domain *smmu_domain,
> struct arm_smmu_device *smmu,
> struct device *dev, int start);
> + int (*inherit_mappings)(struct arm_smmu_device *smmu);
> };
>
> #define INVALID_SMENDX -1
>
On Fri 11 Sep 12:13 CDT 2020, Robin Murphy wrote:
> On 2020-09-04 16:55, Bjorn Andersson wrote:
> > Add a new operation to allow platform implementations to inherit any
> > stream mappings from the boot loader.
>
> Is there a reason we need an explicit step for this? The aim of the
> cfg_probe hook is that the SMMU software state should all be set up by then,
> and you can mess about with it however you like before arm_smmu_reset()
> actually commits anything to hardware. I would have thought you could
> permanently steal a context bank, configure it as your bypass hole, read out
> the previous SME configuration and tweak smmu->smrs and smmu->s2crs
> appropriately all together "invisibly" at that point.
I did this because as of 6a79a5a3842b ("iommu/arm-smmu: Call
configuration impl hook before consuming features") we no longer have
setup pgsize_bitmap as we hit cfg_probe, which means that I need to
replicate this logic to set up the iommu_domain.
If I avoid setting up an iommu_domain for the identity context, as you
request in patch 8, this shouldn't be needed anymore.
> If that can't work, I'm very curious as to what I've overlooked.
>
I believe that will work, I will rework the patches and try it out.
Thanks,
Bjorn
> Robin.
>
> > Signed-off-by: Bjorn Andersson <[email protected]>
> > ---
> >
> > Changes since v2:
> > - New patch/interface
> >
> > drivers/iommu/arm/arm-smmu/arm-smmu.c | 11 ++++++-----
> > drivers/iommu/arm/arm-smmu/arm-smmu.h | 6 ++++++
> > 2 files changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > index eb5c6ca5c138..4c4d302cd747 100644
> > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > @@ -85,11 +85,6 @@ static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu)
> > pm_runtime_put_autosuspend(smmu->dev);
> > }
> > -static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
> > -{
> > - return container_of(dom, struct arm_smmu_domain, domain);
> > -}
> > -
> > static struct platform_driver arm_smmu_driver;
> > static struct iommu_ops arm_smmu_ops;
> > @@ -2188,6 +2183,12 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
> > if (err)
> > return err;
> > + if (smmu->impl->inherit_mappings) {
> > + err = smmu->impl->inherit_mappings(smmu);
> > + if (err)
> > + return err;
> > + }
> > +
> > if (smmu->version == ARM_SMMU_V2) {
> > if (smmu->num_context_banks > smmu->num_context_irqs) {
> > dev_err(dev,
> > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h b/drivers/iommu/arm/arm-smmu/arm-smmu.h
> > index 235d9a3a6ab6..f58164976e74 100644
> > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
> > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
> > @@ -378,6 +378,11 @@ struct arm_smmu_domain {
> > struct iommu_domain domain;
> > };
> > +static inline struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
> > +{
> > + return container_of(dom, struct arm_smmu_domain, domain);
> > +}
> > +
> > struct arm_smmu_master_cfg {
> > struct arm_smmu_device *smmu;
> > s16 smendx[];
> > @@ -442,6 +447,7 @@ struct arm_smmu_impl {
> > int (*alloc_context_bank)(struct arm_smmu_domain *smmu_domain,
> > struct arm_smmu_device *smmu,
> > struct device *dev, int start);
> > + int (*inherit_mappings)(struct arm_smmu_device *smmu);
> > };
> > #define INVALID_SMENDX -1
> >
On 9/4/2020 6:55 PM, Bjorn Andersson wrote:
> Based on previous attempts and discussions this is the latest attempt at
> inheriting stream mappings set up by the bootloader, for e.g. boot splash or
> efifb.
>
> Per Will's request this builds on the work by Jordan and Rob for the Adreno
> SMMU support. It applies cleanly ontop of v16 of their series, which can be
> found at
> https://lore.kernel.org/linux-arm-msm/[email protected]/
>
> Bjorn Andersson (8):
> iommu/arm-smmu: Refactor context bank allocation
> iommu/arm-smmu: Delay modifying domain during init
> iommu/arm-smmu: Consult context bank allocator for identify domains
> iommu/arm-smmu-qcom: Emulate bypass by using context banks
> iommu/arm-smmu-qcom: Consistently initialize stream mappings
> iommu/arm-smmu: Add impl hook for inherit boot mappings
> iommu/arm-smmu: Provide helper for allocating identity domain
> iommu/arm-smmu-qcom: Setup identity domain for boot mappings
>
> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 111 ++++++++++++++++++-
> drivers/iommu/arm/arm-smmu/arm-smmu.c | 122 ++++++++++++++-------
> drivers/iommu/arm/arm-smmu/arm-smmu.h | 14 ++-
> 3 files changed, 205 insertions(+), 42 deletions(-)
>
Tested on a NXP LX2160A with John's tree [1] and below diff [2], so for
the whole series:
Tested-by: Laurentiu Tudor <[email protected]>
[1]
https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/db845c-mainline-WIP
[2]
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-impl.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-impl.c
@@ -190,11 +190,43 @@ static const struct arm_smmu_impl mrvl_mmu500_impl = {
.reset = arm_mmu500_reset,
};
+static int nxp_smmu_inherit_mappings(struct arm_smmu_device *smmu)
+{
+ u32 smr;
+ int i, cnt = 0;
+
+ for (i = 0; i < smmu->num_mapping_groups; i++) {
+ smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
+
+ if (FIELD_GET(ARM_SMMU_SMR_VALID, smr)) {
+ smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
+ smmu->smrs[i].mask =
FIELD_GET(ARM_SMMU_SMR_MASK, smr);
+ smmu->smrs[i].valid = true;
+
+ smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
+ smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
+ smmu->s2crs[i].count++;
+
+ cnt++;
+ }
+ }
+
+ dev_notice(smmu->dev, "\tpreserved %d boot mapping%s\n", cnt,
+ cnt == 1 ? "" : "s");
+
+ return 0;
+}
+
+static const struct arm_smmu_impl nxp_impl = {
+ .inherit_mappings = nxp_smmu_inherit_mappings,
+};
struct arm_smmu_device *arm_smmu_impl_init(struct arm_smmu_device *smmu)
{
const struct device_node *np = smmu->dev->of_node;
/*
* Set the impl for model-specific implementation quirks first,
* such that platform integration quirks can pick it up and
@@ -229,5 +261,12 @@ struct arm_smmu_device *arm_smmu_impl_init(struct
arm_smmu_device *smmu)
if (of_device_is_compatible(np, "marvell,ap806-smmu-500"))
smmu->impl = &mrvl_mmu500_impl;
+ if (of_property_read_bool(np, "nxp,keep-bypass-mappings"))
+ smmu->impl = &nxp_impl;
---
Best Regards, Laurentiu
On Sat, Sep 12, 2020 at 10:25:59PM -0500, Bjorn Andersson wrote:
> On Fri 11 Sep 12:13 CDT 2020, Robin Murphy wrote:
> > On 2020-09-04 16:55, Bjorn Andersson wrote:
> > > Add a new operation to allow platform implementations to inherit any
> > > stream mappings from the boot loader.
> >
> > Is there a reason we need an explicit step for this? The aim of the
> > cfg_probe hook is that the SMMU software state should all be set up by then,
> > and you can mess about with it however you like before arm_smmu_reset()
> > actually commits anything to hardware. I would have thought you could
> > permanently steal a context bank, configure it as your bypass hole, read out
> > the previous SME configuration and tweak smmu->smrs and smmu->s2crs
> > appropriately all together "invisibly" at that point.
>
> I did this because as of 6a79a5a3842b ("iommu/arm-smmu: Call
> configuration impl hook before consuming features") we no longer have
> setup pgsize_bitmap as we hit cfg_probe, which means that I need to
> replicate this logic to set up the iommu_domain.
>
> If I avoid setting up an iommu_domain for the identity context, as you
> request in patch 8, this shouldn't be needed anymore.
>
> > If that can't work, I'm very curious as to what I've overlooked.
> >
>
> I believe that will work, I will rework the patches and try it out.
Did you get a chance to rework this?
Will
On Mon 21 Sep 16:08 CDT 2020, Will Deacon wrote:
> On Sat, Sep 12, 2020 at 10:25:59PM -0500, Bjorn Andersson wrote:
> > On Fri 11 Sep 12:13 CDT 2020, Robin Murphy wrote:
> > > On 2020-09-04 16:55, Bjorn Andersson wrote:
> > > > Add a new operation to allow platform implementations to inherit any
> > > > stream mappings from the boot loader.
> > >
> > > Is there a reason we need an explicit step for this? The aim of the
> > > cfg_probe hook is that the SMMU software state should all be set up by then,
> > > and you can mess about with it however you like before arm_smmu_reset()
> > > actually commits anything to hardware. I would have thought you could
> > > permanently steal a context bank, configure it as your bypass hole, read out
> > > the previous SME configuration and tweak smmu->smrs and smmu->s2crs
> > > appropriately all together "invisibly" at that point.
> >
> > I did this because as of 6a79a5a3842b ("iommu/arm-smmu: Call
> > configuration impl hook before consuming features") we no longer have
> > setup pgsize_bitmap as we hit cfg_probe, which means that I need to
> > replicate this logic to set up the iommu_domain.
> >
> > If I avoid setting up an iommu_domain for the identity context, as you
> > request in patch 8, this shouldn't be needed anymore.
> >
> > > If that can't work, I'm very curious as to what I've overlooked.
> > >
> >
> > I believe that will work, I will rework the patches and try it out.
>
> Did you get a chance to rework this?
>
Unfortunately not, I hope to get to this shortly.
Thanks,
Bjorn
On Mon 21 Sep 23:08 CEST 2020, Will Deacon wrote:
> On Sat, Sep 12, 2020 at 10:25:59PM -0500, Bjorn Andersson wrote:
> > On Fri 11 Sep 12:13 CDT 2020, Robin Murphy wrote:
> > > On 2020-09-04 16:55, Bjorn Andersson wrote:
> > > > Add a new operation to allow platform implementations to inherit any
> > > > stream mappings from the boot loader.
> > >
> > > Is there a reason we need an explicit step for this? The aim of the
> > > cfg_probe hook is that the SMMU software state should all be set up by then,
> > > and you can mess about with it however you like before arm_smmu_reset()
> > > actually commits anything to hardware. I would have thought you could
> > > permanently steal a context bank, configure it as your bypass hole, read out
> > > the previous SME configuration and tweak smmu->smrs and smmu->s2crs
> > > appropriately all together "invisibly" at that point.
> >
> > I did this because as of 6a79a5a3842b ("iommu/arm-smmu: Call
> > configuration impl hook before consuming features") we no longer have
> > setup pgsize_bitmap as we hit cfg_probe, which means that I need to
> > replicate this logic to set up the iommu_domain.
> >
> > If I avoid setting up an iommu_domain for the identity context, as you
> > request in patch 8, this shouldn't be needed anymore.
> >
> > > If that can't work, I'm very curious as to what I've overlooked.
> > >
> >
> > I believe that will work, I will rework the patches and try it out.
>
> Did you get a chance to rework this?
>
Finally got a chance to dig through this properly.
Initial results where positive and with an implementation of cfg_probe
in qcom_smmu_impl I'm able to probe the arm-smmu driver just fine - and
display (e.g. efifb) stays alive.
Unfortunately as the display driver (drivers/gpu/drm/msm) is about to
probe a new iommu domain is created, which due to its match against
qcom_smmu_client_of_match[] becomes of type IOMMU_DOMAIN_IDENTITY.
This results in a S2CR of BYPASS type, which the firmware intercepts and
turns the stream into a type FAULT.
So while the cfg_probe looks very reasonable we're still in need of a
mechanism to use the fake identity context for the iommu domain
associated with the display controller.
The workings of the display driver is that it gets the iommu domain
setup for byass and then after that creates a translation context for
this same stream where it maps the framebuffer.
For testing purposes I made def_domain_type always return 0 in the qcom
impl and the result is that we get a few page faults while probing the
display driver, but these are handled somewhat gracefully and the
initialization did proceed and the system comes up nicely (but in the
case that the display driver would probe defer this leads to an storm of
faults as the screen continues to be refreshed).
TL;DR I think we still need to have a way to get the arm-smmu driver to
allow the qcom implementation to configure identity domains to use
translation - but we can make the setup of the identity context a detail
of the qcom driver.
Regards,
Bjorn
On 2020-10-12 08:31, Bjorn Andersson wrote:
> On Mon 21 Sep 23:08 CEST 2020, Will Deacon wrote:
>
>> On Sat, Sep 12, 2020 at 10:25:59PM -0500, Bjorn Andersson wrote:
>>> On Fri 11 Sep 12:13 CDT 2020, Robin Murphy wrote:
>>>> On 2020-09-04 16:55, Bjorn Andersson wrote:
>>>>> Add a new operation to allow platform implementations to inherit any
>>>>> stream mappings from the boot loader.
>>>>
>>>> Is there a reason we need an explicit step for this? The aim of the
>>>> cfg_probe hook is that the SMMU software state should all be set up by then,
>>>> and you can mess about with it however you like before arm_smmu_reset()
>>>> actually commits anything to hardware. I would have thought you could
>>>> permanently steal a context bank, configure it as your bypass hole, read out
>>>> the previous SME configuration and tweak smmu->smrs and smmu->s2crs
>>>> appropriately all together "invisibly" at that point.
>>>
>>> I did this because as of 6a79a5a3842b ("iommu/arm-smmu: Call
>>> configuration impl hook before consuming features") we no longer have
>>> setup pgsize_bitmap as we hit cfg_probe, which means that I need to
>>> replicate this logic to set up the iommu_domain.
>>>
>>> If I avoid setting up an iommu_domain for the identity context, as you
>>> request in patch 8, this shouldn't be needed anymore.
>>>
>>>> If that can't work, I'm very curious as to what I've overlooked.
>>>>
>>>
>>> I believe that will work, I will rework the patches and try it out.
>>
>> Did you get a chance to rework this?
>>
>
> Finally got a chance to dig through this properly.
>
> Initial results where positive and with an implementation of cfg_probe
> in qcom_smmu_impl I'm able to probe the arm-smmu driver just fine - and
> display (e.g. efifb) stays alive.
>
> Unfortunately as the display driver (drivers/gpu/drm/msm) is about to
> probe a new iommu domain is created, which due to its match against
> qcom_smmu_client_of_match[] becomes of type IOMMU_DOMAIN_IDENTITY.
> This results in a S2CR of BYPASS type, which the firmware intercepts and
> turns the stream into a type FAULT.
>
> So while the cfg_probe looks very reasonable we're still in need of a
> mechanism to use the fake identity context for the iommu domain
> associated with the display controller.
Yes, we'll still need some kind of hook somewhere to make identity
domains work at all - my point about cfg_probe was to keep the
reservation and configuration of the special identity context, plus the
handling of the initial SME state, simple and entirely internal to the
impl. In terms of where said hook should be, TBH it might actually work
out pretty clean to simply hook GR0 register accesses so you can rewrite
between S2CR bypass entries and translation entries targeting your
reserved context on-the-fly. Failing that, something to massage "type"
and "cbndx" in arm_smmu_domain_add_master() would be the next best
option, I think.
Robin.
> The workings of the display driver is that it gets the iommu domain
> setup for byass and then after that creates a translation context for
> this same stream where it maps the framebuffer.
>
> For testing purposes I made def_domain_type always return 0 in the qcom
> impl and the result is that we get a few page faults while probing the
> display driver, but these are handled somewhat gracefully and the
> initialization did proceed and the system comes up nicely (but in the
> case that the display driver would probe defer this leads to an storm of
> faults as the screen continues to be refreshed).
>
> TL;DR I think we still need to have a way to get the arm-smmu driver to
> allow the qcom implementation to configure identity domains to use
> translation - but we can make the setup of the identity context a detail
> of the qcom driver.
>
> Regards,
> Bjorn
>