* [PATCH v3 1/5] iommu: Add DOMAIN_ATTR_SPLIT_TABLES
2019-12-16 16:37 [PATCH v3 0/5] iommu/arm-smmu: Split pagetable support for arm-smmu-v2 Jordan Crouse
@ 2019-12-16 16:37 ` Jordan Crouse
2020-01-09 14:36 ` Will Deacon
2019-12-16 16:37 ` [PATCH v3 2/5] iommu/arm-smmu: Add support for split pagetables Jordan Crouse
` (3 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Jordan Crouse @ 2019-12-16 16:37 UTC (permalink / raw)
To: iommu
Cc: robin.murphy, will, linux-arm-kernel, linux-arm-msm,
Joerg Roedel, linux-kernel
Add a new attribute to enable and query the state of split pagetables
for the domain.
Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
---
include/linux/iommu.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index f2223cb..18c861e 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -126,6 +126,7 @@ enum iommu_attr {
DOMAIN_ATTR_FSL_PAMUV1,
DOMAIN_ATTR_NESTING, /* two stages of translation */
DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE,
+ DOMAIN_ATTR_SPLIT_TABLES,
DOMAIN_ATTR_MAX,
};
--
2.7.4
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v3 1/5] iommu: Add DOMAIN_ATTR_SPLIT_TABLES
2019-12-16 16:37 ` [PATCH v3 1/5] iommu: Add DOMAIN_ATTR_SPLIT_TABLES Jordan Crouse
@ 2020-01-09 14:36 ` Will Deacon
0 siblings, 0 replies; 17+ messages in thread
From: Will Deacon @ 2020-01-09 14:36 UTC (permalink / raw)
To: Jordan Crouse
Cc: iommu, robin.murphy, linux-arm-kernel, linux-arm-msm,
Joerg Roedel, linux-kernel
On Mon, Dec 16, 2019 at 09:37:47AM -0700, Jordan Crouse wrote:
> Add a new attribute to enable and query the state of split pagetables
> for the domain.
>
> Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> ---
>
> include/linux/iommu.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index f2223cb..18c861e 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -126,6 +126,7 @@ enum iommu_attr {
> DOMAIN_ATTR_FSL_PAMUV1,
> DOMAIN_ATTR_NESTING, /* two stages of translation */
> DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE,
> + DOMAIN_ATTR_SPLIT_TABLES,
> DOMAIN_ATTR_MAX,
> };
Acked-by: Will Deacon <will@kernel.org>
Although a comment describing what this does wouldn't hurt.
Will
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 2/5] iommu/arm-smmu: Add support for split pagetables
2019-12-16 16:37 [PATCH v3 0/5] iommu/arm-smmu: Split pagetable support for arm-smmu-v2 Jordan Crouse
2019-12-16 16:37 ` [PATCH v3 1/5] iommu: Add DOMAIN_ATTR_SPLIT_TABLES Jordan Crouse
@ 2019-12-16 16:37 ` Jordan Crouse
2020-01-09 14:33 ` Will Deacon
` (2 more replies)
2019-12-16 16:37 ` [PATCH v3 3/5] drm/msm: Attach the IOMMU device during initialization Jordan Crouse
` (2 subsequent siblings)
4 siblings, 3 replies; 17+ messages in thread
From: Jordan Crouse @ 2019-12-16 16:37 UTC (permalink / raw)
To: iommu
Cc: robin.murphy, will, linux-arm-kernel, linux-arm-msm,
linux-kernel, Joerg Roedel
Add support to enable split pagetables (TTBR1) if the supporting driver
requests it via the DOMAIN_ATTR_SPLIT_TABLES flag. When enabled, the driver
will set up the TTBR0 and TTBR1 regions and program the default domain
pagetable on TTBR1.
After attaching the device, the value of he domain attribute can
be queried to see if the split pagetables were successfully programmed.
Furthermore the domain geometry will be updated so that the caller can
determine the active region for the pagetable that was programmed.
Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
---
drivers/iommu/arm-smmu.c | 40 +++++++++++++++++++++++++++++++++++-----
drivers/iommu/arm-smmu.h | 45 +++++++++++++++++++++++++++++++++++++++------
2 files changed, 74 insertions(+), 11 deletions(-)
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index c106406..7b59116 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -538,9 +538,17 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
cb->ttbr[0] = pgtbl_cfg->arm_v7s_cfg.ttbr;
cb->ttbr[1] = 0;
} else {
- cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
- cb->ttbr[0] |= FIELD_PREP(TTBRn_ASID, cfg->asid);
- cb->ttbr[1] = FIELD_PREP(TTBRn_ASID, cfg->asid);
+ if (pgtbl_cfg->quirks & IO_PGTABLE_QUIRK_ARM_TTBR1) {
+ cb->ttbr[0] = FIELD_PREP(TTBRn_ASID, cfg->asid);
+ cb->ttbr[1] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
+ cb->ttbr[1] |=
+ FIELD_PREP(TTBRn_ASID, cfg->asid);
+ } else {
+ cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
+ cb->ttbr[0] |=
+ FIELD_PREP(TTBRn_ASID, cfg->asid);
+ cb->ttbr[1] = FIELD_PREP(TTBRn_ASID, cfg->asid);
+ }
}
} else {
cb->ttbr[0] = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
@@ -651,6 +659,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
enum io_pgtable_fmt fmt;
struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
+ u32 quirks = 0;
mutex_lock(&smmu_domain->init_mutex);
if (smmu_domain->smmu)
@@ -719,6 +728,8 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
oas = smmu->ipa_size;
if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64) {
fmt = ARM_64_LPAE_S1;
+ if (smmu_domain->split_pagetables)
+ quirks |= IO_PGTABLE_QUIRK_ARM_TTBR1;
} else if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_L) {
fmt = ARM_32_LPAE_S1;
ias = min(ias, 32UL);
@@ -788,6 +799,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
.coherent_walk = smmu->features & ARM_SMMU_FEAT_COHERENT_WALK,
.tlb = smmu_domain->flush_ops,
.iommu_dev = smmu->dev,
+ .quirks = quirks,
};
if (smmu_domain->non_strict)
@@ -801,8 +813,15 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
/* Update the domain's page sizes to reflect the page table format */
domain->pgsize_bitmap = pgtbl_cfg.pgsize_bitmap;
- domain->geometry.aperture_end = (1UL << ias) - 1;
- domain->geometry.force_aperture = true;
+
+ if (pgtbl_cfg.quirks & IO_PGTABLE_QUIRK_ARM_TTBR1) {
+ domain->geometry.aperture_start = ~((1ULL << ias) - 1);
+ domain->geometry.aperture_end = ~0UL;
+ } else {
+ domain->geometry.aperture_end = (1UL << ias) - 1;
+ domain->geometry.force_aperture = true;
+ smmu_domain->split_pagetables = false;
+ }
/* Initialise the context bank with our page table cfg */
arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
@@ -1484,6 +1503,9 @@ static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
case DOMAIN_ATTR_NESTING:
*(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
return 0;
+ case DOMAIN_ATTR_SPLIT_TABLES:
+ *(int *)data = smmu_domain->split_pagetables;
+ return 0;
default:
return -ENODEV;
}
@@ -1524,6 +1546,14 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
else
smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
break;
+ case DOMAIN_ATTR_SPLIT_TABLES:
+ if (smmu_domain->smmu) {
+ ret = -EPERM;
+ goto out_unlock;
+ }
+ if (*(int *)data)
+ smmu_domain->split_pagetables = true;
+ break;
default:
ret = -ENODEV;
}
diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
index afab9de..68526cc 100644
--- a/drivers/iommu/arm-smmu.h
+++ b/drivers/iommu/arm-smmu.h
@@ -177,6 +177,16 @@ enum arm_smmu_cbar_type {
#define TCR_IRGN0 GENMASK(9, 8)
#define TCR_T0SZ GENMASK(5, 0)
+#define TCR_TG1 GENMASK(31, 30)
+
+#define TG0_4K 0
+#define TG0_64K 1
+#define TG0_16K 2
+
+#define TG1_16K 1
+#define TG1_4K 2
+#define TG1_64K 3
+
#define ARM_SMMU_CB_CONTEXTIDR 0x34
#define ARM_SMMU_CB_S1_MAIR0 0x38
#define ARM_SMMU_CB_S1_MAIR1 0x3c
@@ -329,16 +339,39 @@ struct arm_smmu_domain {
struct mutex init_mutex; /* Protects smmu pointer */
spinlock_t cb_lock; /* Serialises ATS1* ops and TLB syncs */
struct iommu_domain domain;
+ bool split_pagetables;
};
+static inline u32 arm_smmu_lpae_tcr_tg(struct io_pgtable_cfg *cfg)
+{
+ u32 val;
+
+ if (!(cfg->quirks & IO_PGTABLE_QUIRK_ARM_TTBR1))
+ return FIELD_PREP(TCR_TG0, cfg->arm_lpae_s1_cfg.tcr.tg);
+
+ val = FIELD_PREP(TCR_TG1, cfg->arm_lpae_s1_cfg.tcr.tg);
+
+ if (cfg->arm_lpae_s1_cfg.tcr.tg == TG1_4K)
+ val |= FIELD_PREP(TCR_TG0, TG0_4K);
+ else if (cfg->arm_lpae_s1_cfg.tcr.tg == TG1_16K)
+ val |= FIELD_PREP(TCR_TG0, TG0_16K);
+ else
+ val |= FIELD_PREP(TCR_TG0, TG0_64K);
+
+ return val;
+}
+
static inline u32 arm_smmu_lpae_tcr(struct io_pgtable_cfg *cfg)
{
- return TCR_EPD1 |
- FIELD_PREP(TCR_TG0, cfg->arm_lpae_s1_cfg.tcr.tg) |
- FIELD_PREP(TCR_SH0, cfg->arm_lpae_s1_cfg.tcr.sh) |
- FIELD_PREP(TCR_ORGN0, cfg->arm_lpae_s1_cfg.tcr.orgn) |
- FIELD_PREP(TCR_IRGN0, cfg->arm_lpae_s1_cfg.tcr.irgn) |
- FIELD_PREP(TCR_T0SZ, cfg->arm_lpae_s1_cfg.tcr.tsz);
+ u32 tcr = FIELD_PREP(TCR_SH0, cfg->arm_lpae_s1_cfg.tcr.sh) |
+ FIELD_PREP(TCR_ORGN0, cfg->arm_lpae_s1_cfg.tcr.orgn) |
+ FIELD_PREP(TCR_IRGN0, cfg->arm_lpae_s1_cfg.tcr.irgn) |
+ FIELD_PREP(TCR_T0SZ, cfg->arm_lpae_s1_cfg.tcr.tsz);
+
+ if (!(cfg->quirks & IO_PGTABLE_QUIRK_ARM_TTBR1))
+ return tcr | TCR_EPD1 | arm_smmu_lpae_tcr_tg(cfg);
+
+ return tcr | (tcr << 16) | arm_smmu_lpae_tcr_tg(cfg);
}
static inline u32 arm_smmu_lpae_tcr2(struct io_pgtable_cfg *cfg)
--
2.7.4
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v3 2/5] iommu/arm-smmu: Add support for split pagetables
2019-12-16 16:37 ` [PATCH v3 2/5] iommu/arm-smmu: Add support for split pagetables Jordan Crouse
@ 2020-01-09 14:33 ` Will Deacon
2020-01-09 19:25 ` Jordan Crouse
2020-01-21 14:36 ` Robin Murphy
2020-01-21 14:47 ` Robin Murphy
2 siblings, 1 reply; 17+ messages in thread
From: Will Deacon @ 2020-01-09 14:33 UTC (permalink / raw)
To: Jordan Crouse
Cc: iommu, robin.murphy, linux-arm-kernel, linux-arm-msm,
linux-kernel, Joerg Roedel
On Mon, Dec 16, 2019 at 09:37:48AM -0700, Jordan Crouse wrote:
> Add support to enable split pagetables (TTBR1) if the supporting driver
> requests it via the DOMAIN_ATTR_SPLIT_TABLES flag. When enabled, the driver
> will set up the TTBR0 and TTBR1 regions and program the default domain
> pagetable on TTBR1.
>
> After attaching the device, the value of he domain attribute can
> be queried to see if the split pagetables were successfully programmed.
> Furthermore the domain geometry will be updated so that the caller can
> determine the active region for the pagetable that was programmed.
>
> Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> ---
>
> drivers/iommu/arm-smmu.c | 40 +++++++++++++++++++++++++++++++++++-----
> drivers/iommu/arm-smmu.h | 45 +++++++++++++++++++++++++++++++++++++++------
> 2 files changed, 74 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index c106406..7b59116 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -538,9 +538,17 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
> cb->ttbr[0] = pgtbl_cfg->arm_v7s_cfg.ttbr;
> cb->ttbr[1] = 0;
> } else {
> - cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
> - cb->ttbr[0] |= FIELD_PREP(TTBRn_ASID, cfg->asid);
> - cb->ttbr[1] = FIELD_PREP(TTBRn_ASID, cfg->asid);
> + if (pgtbl_cfg->quirks & IO_PGTABLE_QUIRK_ARM_TTBR1) {
> + cb->ttbr[0] = FIELD_PREP(TTBRn_ASID, cfg->asid);
> + cb->ttbr[1] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
> + cb->ttbr[1] |=
> + FIELD_PREP(TTBRn_ASID, cfg->asid);
> + } else {
> + cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
> + cb->ttbr[0] |=
> + FIELD_PREP(TTBRn_ASID, cfg->asid);
> + cb->ttbr[1] = FIELD_PREP(TTBRn_ASID, cfg->asid);
> + }
I still don't understand why you have to set the ASID in both of the TTBRs.
Assuming TCR.A1 is clear, then we should only need to set the field in
TTBR0.
> }
> } else {
> cb->ttbr[0] = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
> @@ -651,6 +659,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> enum io_pgtable_fmt fmt;
> struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
> + u32 quirks = 0;
>
> mutex_lock(&smmu_domain->init_mutex);
> if (smmu_domain->smmu)
> @@ -719,6 +728,8 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> oas = smmu->ipa_size;
> if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64) {
> fmt = ARM_64_LPAE_S1;
> + if (smmu_domain->split_pagetables)
> + quirks |= IO_PGTABLE_QUIRK_ARM_TTBR1;
> } else if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_L) {
> fmt = ARM_32_LPAE_S1;
> ias = min(ias, 32UL);
> @@ -788,6 +799,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> .coherent_walk = smmu->features & ARM_SMMU_FEAT_COHERENT_WALK,
> .tlb = smmu_domain->flush_ops,
> .iommu_dev = smmu->dev,
> + .quirks = quirks,
> };
>
> if (smmu_domain->non_strict)
> @@ -801,8 +813,15 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>
> /* Update the domain's page sizes to reflect the page table format */
> domain->pgsize_bitmap = pgtbl_cfg.pgsize_bitmap;
> - domain->geometry.aperture_end = (1UL << ias) - 1;
> - domain->geometry.force_aperture = true;
> +
> + if (pgtbl_cfg.quirks & IO_PGTABLE_QUIRK_ARM_TTBR1) {
> + domain->geometry.aperture_start = ~((1ULL << ias) - 1);
> + domain->geometry.aperture_end = ~0UL;
> + } else {
> + domain->geometry.aperture_end = (1UL << ias) - 1;
> + domain->geometry.force_aperture = true;
> + smmu_domain->split_pagetables = false;
> + }
>
> /* Initialise the context bank with our page table cfg */
> arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
> @@ -1484,6 +1503,9 @@ static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
> case DOMAIN_ATTR_NESTING:
> *(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
> return 0;
> + case DOMAIN_ATTR_SPLIT_TABLES:
> + *(int *)data = smmu_domain->split_pagetables;
> + return 0;
> default:
> return -ENODEV;
> }
> @@ -1524,6 +1546,14 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
> else
> smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
> break;
> + case DOMAIN_ATTR_SPLIT_TABLES:
> + if (smmu_domain->smmu) {
> + ret = -EPERM;
> + goto out_unlock;
> + }
> + if (*(int *)data)
> + smmu_domain->split_pagetables = true;
> + break;
> default:
> ret = -ENODEV;
> }
> diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
> index afab9de..68526cc 100644
> --- a/drivers/iommu/arm-smmu.h
> +++ b/drivers/iommu/arm-smmu.h
> @@ -177,6 +177,16 @@ enum arm_smmu_cbar_type {
> #define TCR_IRGN0 GENMASK(9, 8)
> #define TCR_T0SZ GENMASK(5, 0)
>
> +#define TCR_TG1 GENMASK(31, 30)
> +
> +#define TG0_4K 0
> +#define TG0_64K 1
> +#define TG0_16K 2
> +
> +#define TG1_16K 1
> +#define TG1_4K 2
> +#define TG1_64K 3
> +
> #define ARM_SMMU_CB_CONTEXTIDR 0x34
> #define ARM_SMMU_CB_S1_MAIR0 0x38
> #define ARM_SMMU_CB_S1_MAIR1 0x3c
> @@ -329,16 +339,39 @@ struct arm_smmu_domain {
> struct mutex init_mutex; /* Protects smmu pointer */
> spinlock_t cb_lock; /* Serialises ATS1* ops and TLB syncs */
> struct iommu_domain domain;
> + bool split_pagetables;
> };
>
> +static inline u32 arm_smmu_lpae_tcr_tg(struct io_pgtable_cfg *cfg)
> +{
> + u32 val;
> +
> + if (!(cfg->quirks & IO_PGTABLE_QUIRK_ARM_TTBR1))
> + return FIELD_PREP(TCR_TG0, cfg->arm_lpae_s1_cfg.tcr.tg);
> +
> + val = FIELD_PREP(TCR_TG1, cfg->arm_lpae_s1_cfg.tcr.tg);
> +
> + if (cfg->arm_lpae_s1_cfg.tcr.tg == TG1_4K)
> + val |= FIELD_PREP(TCR_TG0, TG0_4K);
> + else if (cfg->arm_lpae_s1_cfg.tcr.tg == TG1_16K)
> + val |= FIELD_PREP(TCR_TG0, TG0_16K);
> + else
> + val |= FIELD_PREP(TCR_TG0, TG0_64K);
This looks like it's making assumptions about the order in which page-tables
are installed, which I'd really like to avoid. See below.
> static inline u32 arm_smmu_lpae_tcr(struct io_pgtable_cfg *cfg)
> {
> - return TCR_EPD1 |
> - FIELD_PREP(TCR_TG0, cfg->arm_lpae_s1_cfg.tcr.tg) |
> - FIELD_PREP(TCR_SH0, cfg->arm_lpae_s1_cfg.tcr.sh) |
> - FIELD_PREP(TCR_ORGN0, cfg->arm_lpae_s1_cfg.tcr.orgn) |
> - FIELD_PREP(TCR_IRGN0, cfg->arm_lpae_s1_cfg.tcr.irgn) |
> - FIELD_PREP(TCR_T0SZ, cfg->arm_lpae_s1_cfg.tcr.tsz);
> + u32 tcr = FIELD_PREP(TCR_SH0, cfg->arm_lpae_s1_cfg.tcr.sh) |
> + FIELD_PREP(TCR_ORGN0, cfg->arm_lpae_s1_cfg.tcr.orgn) |
> + FIELD_PREP(TCR_IRGN0, cfg->arm_lpae_s1_cfg.tcr.irgn) |
> + FIELD_PREP(TCR_T0SZ, cfg->arm_lpae_s1_cfg.tcr.tsz);
> +
> + if (!(cfg->quirks & IO_PGTABLE_QUIRK_ARM_TTBR1))
> + return tcr | TCR_EPD1 | arm_smmu_lpae_tcr_tg(cfg);
This is interesting. If the intention is to have both TTBR0 and TTBR1
used concurrently by different domains, then we probably need to be a bit
smarter about setting TCR_EPDx. Can we do something like start off with them
both set, and then just clear the one we want when installing a page-table?
Will
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v3 2/5] iommu/arm-smmu: Add support for split pagetables
2020-01-09 14:33 ` Will Deacon
@ 2020-01-09 19:25 ` Jordan Crouse
0 siblings, 0 replies; 17+ messages in thread
From: Jordan Crouse @ 2020-01-09 19:25 UTC (permalink / raw)
To: Will Deacon
Cc: iommu, robin.murphy, linux-arm-kernel, linux-arm-msm,
linux-kernel, Joerg Roedel
On Thu, Jan 09, 2020 at 02:33:34PM +0000, Will Deacon wrote:
> On Mon, Dec 16, 2019 at 09:37:48AM -0700, Jordan Crouse wrote:
> > Add support to enable split pagetables (TTBR1) if the supporting driver
> > requests it via the DOMAIN_ATTR_SPLIT_TABLES flag. When enabled, the driver
> > will set up the TTBR0 and TTBR1 regions and program the default domain
> > pagetable on TTBR1.
> >
> > After attaching the device, the value of he domain attribute can
> > be queried to see if the split pagetables were successfully programmed.
> > Furthermore the domain geometry will be updated so that the caller can
> > determine the active region for the pagetable that was programmed.
> >
> > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> > ---
> >
> > drivers/iommu/arm-smmu.c | 40 +++++++++++++++++++++++++++++++++++-----
> > drivers/iommu/arm-smmu.h | 45 +++++++++++++++++++++++++++++++++++++++------
> > 2 files changed, 74 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index c106406..7b59116 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -538,9 +538,17 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
> > cb->ttbr[0] = pgtbl_cfg->arm_v7s_cfg.ttbr;
> > cb->ttbr[1] = 0;
> > } else {
> > - cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
> > - cb->ttbr[0] |= FIELD_PREP(TTBRn_ASID, cfg->asid);
> > - cb->ttbr[1] = FIELD_PREP(TTBRn_ASID, cfg->asid);
> > + if (pgtbl_cfg->quirks & IO_PGTABLE_QUIRK_ARM_TTBR1) {
> > + cb->ttbr[0] = FIELD_PREP(TTBRn_ASID, cfg->asid);
> > + cb->ttbr[1] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
> > + cb->ttbr[1] |=
> > + FIELD_PREP(TTBRn_ASID, cfg->asid);
> > + } else {
> > + cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
> > + cb->ttbr[0] |=
> > + FIELD_PREP(TTBRn_ASID, cfg->asid);
> > + cb->ttbr[1] = FIELD_PREP(TTBRn_ASID, cfg->asid);
> > + }
>
> I still don't understand why you have to set the ASID in both of the TTBRs.
> Assuming TCR.A1 is clear, then we should only need to set the field in
> TTBR0.
This is mostly out of a sense of symmetry with the non-split configuration. I'll
clean it up.
>
> > }
> > } else {
> > cb->ttbr[0] = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
> > @@ -651,6 +659,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> > enum io_pgtable_fmt fmt;
> > struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> > struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
> > + u32 quirks = 0;
> >
> > mutex_lock(&smmu_domain->init_mutex);
> > if (smmu_domain->smmu)
> > @@ -719,6 +728,8 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> > oas = smmu->ipa_size;
> > if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64) {
> > fmt = ARM_64_LPAE_S1;
> > + if (smmu_domain->split_pagetables)
> > + quirks |= IO_PGTABLE_QUIRK_ARM_TTBR1;
> > } else if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_L) {
> > fmt = ARM_32_LPAE_S1;
> > ias = min(ias, 32UL);
> > @@ -788,6 +799,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> > .coherent_walk = smmu->features & ARM_SMMU_FEAT_COHERENT_WALK,
> > .tlb = smmu_domain->flush_ops,
> > .iommu_dev = smmu->dev,
> > + .quirks = quirks,
> > };
> >
> > if (smmu_domain->non_strict)
> > @@ -801,8 +813,15 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> >
> > /* Update the domain's page sizes to reflect the page table format */
> > domain->pgsize_bitmap = pgtbl_cfg.pgsize_bitmap;
> > - domain->geometry.aperture_end = (1UL << ias) - 1;
> > - domain->geometry.force_aperture = true;
> > +
> > + if (pgtbl_cfg.quirks & IO_PGTABLE_QUIRK_ARM_TTBR1) {
> > + domain->geometry.aperture_start = ~((1ULL << ias) - 1);
> > + domain->geometry.aperture_end = ~0UL;
> > + } else {
> > + domain->geometry.aperture_end = (1UL << ias) - 1;
> > + domain->geometry.force_aperture = true;
> > + smmu_domain->split_pagetables = false;
> > + }
> >
> > /* Initialise the context bank with our page table cfg */
> > arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
> > @@ -1484,6 +1503,9 @@ static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
> > case DOMAIN_ATTR_NESTING:
> > *(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
> > return 0;
> > + case DOMAIN_ATTR_SPLIT_TABLES:
> > + *(int *)data = smmu_domain->split_pagetables;
> > + return 0;
> > default:
> > return -ENODEV;
> > }
> > @@ -1524,6 +1546,14 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
> > else
> > smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
> > break;
> > + case DOMAIN_ATTR_SPLIT_TABLES:
> > + if (smmu_domain->smmu) {
> > + ret = -EPERM;
> > + goto out_unlock;
> > + }
> > + if (*(int *)data)
> > + smmu_domain->split_pagetables = true;
> > + break;
> > default:
> > ret = -ENODEV;
> > }
> > diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
> > index afab9de..68526cc 100644
> > --- a/drivers/iommu/arm-smmu.h
> > +++ b/drivers/iommu/arm-smmu.h
> > @@ -177,6 +177,16 @@ enum arm_smmu_cbar_type {
> > #define TCR_IRGN0 GENMASK(9, 8)
> > #define TCR_T0SZ GENMASK(5, 0)
> >
> > +#define TCR_TG1 GENMASK(31, 30)
> > +
> > +#define TG0_4K 0
> > +#define TG0_64K 1
> > +#define TG0_16K 2
> > +
> > +#define TG1_16K 1
> > +#define TG1_4K 2
> > +#define TG1_64K 3
> > +
> > #define ARM_SMMU_CB_CONTEXTIDR 0x34
> > #define ARM_SMMU_CB_S1_MAIR0 0x38
> > #define ARM_SMMU_CB_S1_MAIR1 0x3c
> > @@ -329,16 +339,39 @@ struct arm_smmu_domain {
> > struct mutex init_mutex; /* Protects smmu pointer */
> > spinlock_t cb_lock; /* Serialises ATS1* ops and TLB syncs */
> > struct iommu_domain domain;
> > + bool split_pagetables;
> > };
> >
> > +static inline u32 arm_smmu_lpae_tcr_tg(struct io_pgtable_cfg *cfg)
> > +{
> > + u32 val;
> > +
> > + if (!(cfg->quirks & IO_PGTABLE_QUIRK_ARM_TTBR1))
> > + return FIELD_PREP(TCR_TG0, cfg->arm_lpae_s1_cfg.tcr.tg);
> > +
> > + val = FIELD_PREP(TCR_TG1, cfg->arm_lpae_s1_cfg.tcr.tg);
> > +
> > + if (cfg->arm_lpae_s1_cfg.tcr.tg == TG1_4K)
> > + val |= FIELD_PREP(TCR_TG0, TG0_4K);
> > + else if (cfg->arm_lpae_s1_cfg.tcr.tg == TG1_16K)
> > + val |= FIELD_PREP(TCR_TG0, TG0_16K);
> > + else
> > + val |= FIELD_PREP(TCR_TG0, TG0_64K);
>
> This looks like it's making assumptions about the order in which page-tables
> are installed, which I'd really like to avoid. See below.
> > static inline u32 arm_smmu_lpae_tcr(struct io_pgtable_cfg *cfg)
> > {
> > - return TCR_EPD1 |
> > - FIELD_PREP(TCR_TG0, cfg->arm_lpae_s1_cfg.tcr.tg) |
> > - FIELD_PREP(TCR_SH0, cfg->arm_lpae_s1_cfg.tcr.sh) |
> > - FIELD_PREP(TCR_ORGN0, cfg->arm_lpae_s1_cfg.tcr.orgn) |
> > - FIELD_PREP(TCR_IRGN0, cfg->arm_lpae_s1_cfg.tcr.irgn) |
> > - FIELD_PREP(TCR_T0SZ, cfg->arm_lpae_s1_cfg.tcr.tsz);
> > + u32 tcr = FIELD_PREP(TCR_SH0, cfg->arm_lpae_s1_cfg.tcr.sh) |
> > + FIELD_PREP(TCR_ORGN0, cfg->arm_lpae_s1_cfg.tcr.orgn) |
> > + FIELD_PREP(TCR_IRGN0, cfg->arm_lpae_s1_cfg.tcr.irgn) |
> > + FIELD_PREP(TCR_T0SZ, cfg->arm_lpae_s1_cfg.tcr.tsz);
> > +
> > + if (!(cfg->quirks & IO_PGTABLE_QUIRK_ARM_TTBR1))
> > + return tcr | TCR_EPD1 | arm_smmu_lpae_tcr_tg(cfg);
>
> This is interesting. If the intention is to have both TTBR0 and TTBR1
> used concurrently by different domains, then we probably need to be a bit
> smarter about setting TCR_EPDx. Can we do something like start off with them
> both set, and then just clear the one we want when installing a page-table?
My intention is that there should only be one domain that programs the hardware
and installs the TTBR1 page-table.
Under the proposed design [1] we used an aux domain that was basically a wrapper
for a page-table and a domain attribute to return the physical address of the
page-table to the GPU hardware which can program the TTBR0 register at runtime
[2].
The object of this patch is that in split pagetable mode the "master" domain
programs both the TTBR0 and TTBR1 sides of the TCR register but leaves the
actual TTBR0 register empty for the GPU to manage.
The GPU isn't currently set up to program register configuration outside of
swapping the TTBR0 register and hitting the TIBALL bit which is part of an
built in opcode and its not practical to add TCR configuration to the mix.
I also feel pretty strongly that we should leave register configuration to the
arm-smmu driver as much as possible so that was my motivation for doing this
patch in this manner.
Jordan
[1] https://patchwork.freedesktop.org/patch/306117/
[2] https://patchwork.freedesktop.org/patch/307616/?series=57441&rev=3
> Will
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/5] iommu/arm-smmu: Add support for split pagetables
2019-12-16 16:37 ` [PATCH v3 2/5] iommu/arm-smmu: Add support for split pagetables Jordan Crouse
2020-01-09 14:33 ` Will Deacon
@ 2020-01-21 14:36 ` Robin Murphy
2020-01-21 17:11 ` Jordan Crouse
2020-01-21 14:47 ` Robin Murphy
2 siblings, 1 reply; 17+ messages in thread
From: Robin Murphy @ 2020-01-21 14:36 UTC (permalink / raw)
To: Jordan Crouse, iommu
Cc: will, linux-arm-kernel, linux-arm-msm, linux-kernel, Joerg Roedel
On 16/12/2019 4:37 pm, Jordan Crouse wrote:
> Add support to enable split pagetables (TTBR1) if the supporting driver
> requests it via the DOMAIN_ATTR_SPLIT_TABLES flag. When enabled, the driver
> will set up the TTBR0 and TTBR1 regions and program the default domain
> pagetable on TTBR1.
>
> After attaching the device, the value of he domain attribute can
> be queried to see if the split pagetables were successfully programmed.
> Furthermore the domain geometry will be updated so that the caller can
> determine the active region for the pagetable that was programmed.
>
> Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> ---
>
> drivers/iommu/arm-smmu.c | 40 +++++++++++++++++++++++++++++++++++-----
> drivers/iommu/arm-smmu.h | 45 +++++++++++++++++++++++++++++++++++++++------
> 2 files changed, 74 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index c106406..7b59116 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -538,9 +538,17 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
> cb->ttbr[0] = pgtbl_cfg->arm_v7s_cfg.ttbr;
> cb->ttbr[1] = 0;
> } else {
> - cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
> - cb->ttbr[0] |= FIELD_PREP(TTBRn_ASID, cfg->asid);
> - cb->ttbr[1] = FIELD_PREP(TTBRn_ASID, cfg->asid);
> + if (pgtbl_cfg->quirks & IO_PGTABLE_QUIRK_ARM_TTBR1) {
> + cb->ttbr[0] = FIELD_PREP(TTBRn_ASID, cfg->asid);
> + cb->ttbr[1] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
> + cb->ttbr[1] |=
> + FIELD_PREP(TTBRn_ASID, cfg->asid);
> + } else {
> + cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
> + cb->ttbr[0] |=
> + FIELD_PREP(TTBRn_ASID, cfg->asid);
> + cb->ttbr[1] = FIELD_PREP(TTBRn_ASID, cfg->asid);
> + }
> }
> } else {
> cb->ttbr[0] = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
> @@ -651,6 +659,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> enum io_pgtable_fmt fmt;
> struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
> + u32 quirks = 0;
>
> mutex_lock(&smmu_domain->init_mutex);
> if (smmu_domain->smmu)
> @@ -719,6 +728,8 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> oas = smmu->ipa_size;
> if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64) {
> fmt = ARM_64_LPAE_S1;
> + if (smmu_domain->split_pagetables)
> + quirks |= IO_PGTABLE_QUIRK_ARM_TTBR1;
> } else if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_L) {
> fmt = ARM_32_LPAE_S1;
> ias = min(ias, 32UL);
> @@ -788,6 +799,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> .coherent_walk = smmu->features & ARM_SMMU_FEAT_COHERENT_WALK,
> .tlb = smmu_domain->flush_ops,
> .iommu_dev = smmu->dev,
> + .quirks = quirks,
> };
>
> if (smmu_domain->non_strict)
> @@ -801,8 +813,15 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>
> /* Update the domain's page sizes to reflect the page table format */
> domain->pgsize_bitmap = pgtbl_cfg.pgsize_bitmap;
> - domain->geometry.aperture_end = (1UL << ias) - 1;
> - domain->geometry.force_aperture = true;
> +
> + if (pgtbl_cfg.quirks & IO_PGTABLE_QUIRK_ARM_TTBR1) {
> + domain->geometry.aperture_start = ~((1ULL << ias) - 1);
AKA "~0UL << ias", if I'm not mistaken ;)
> + domain->geometry.aperture_end = ~0UL;
> + } else {
> + domain->geometry.aperture_end = (1UL << ias) - 1;
> + domain->geometry.force_aperture = true;
> + smmu_domain->split_pagetables = false;
> + }
>
> /* Initialise the context bank with our page table cfg */
> arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
> @@ -1484,6 +1503,9 @@ static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
> case DOMAIN_ATTR_NESTING:
> *(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
> return 0;
> + case DOMAIN_ATTR_SPLIT_TABLES:
> + *(int *)data = smmu_domain->split_pagetables;
> + return 0;
> default:
> return -ENODEV;
> }
> @@ -1524,6 +1546,14 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
> else
> smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
> break;
> + case DOMAIN_ATTR_SPLIT_TABLES:
> + if (smmu_domain->smmu) {
> + ret = -EPERM;
> + goto out_unlock;
> + }
> + if (*(int *)data)
> + smmu_domain->split_pagetables = true;
I still like the idea of passing the actual split point here, but as it
is I think this sets the scene perfectly for coming back and doing that
later.
> + break;
> default:
> ret = -ENODEV;
> }
> diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
> index afab9de..68526cc 100644
> --- a/drivers/iommu/arm-smmu.h
> +++ b/drivers/iommu/arm-smmu.h
> @@ -177,6 +177,16 @@ enum arm_smmu_cbar_type {
> #define TCR_IRGN0 GENMASK(9, 8)
> #define TCR_T0SZ GENMASK(5, 0)
>
> +#define TCR_TG1 GENMASK(31, 30)
> +
> +#define TG0_4K 0
> +#define TG0_64K 1
> +#define TG0_16K 2
> +
> +#define TG1_16K 1
> +#define TG1_4K 2
> +#define TG1_64K 3
> +
> #define ARM_SMMU_CB_CONTEXTIDR 0x34
> #define ARM_SMMU_CB_S1_MAIR0 0x38
> #define ARM_SMMU_CB_S1_MAIR1 0x3c
> @@ -329,16 +339,39 @@ struct arm_smmu_domain {
> struct mutex init_mutex; /* Protects smmu pointer */
> spinlock_t cb_lock; /* Serialises ATS1* ops and TLB syncs */
> struct iommu_domain domain;
> + bool split_pagetables;
> };
>
> +static inline u32 arm_smmu_lpae_tcr_tg(struct io_pgtable_cfg *cfg)
> +{
> + u32 val;
> +
> + if (!(cfg->quirks & IO_PGTABLE_QUIRK_ARM_TTBR1))
> + return FIELD_PREP(TCR_TG0, cfg->arm_lpae_s1_cfg.tcr.tg);
> +
> + val = FIELD_PREP(TCR_TG1, cfg->arm_lpae_s1_cfg.tcr.tg);
> +
> + if (cfg->arm_lpae_s1_cfg.tcr.tg == TG1_4K)
> + val |= FIELD_PREP(TCR_TG0, TG0_4K);
> + else if (cfg->arm_lpae_s1_cfg.tcr.tg == TG1_16K)
> + val |= FIELD_PREP(TCR_TG0, TG0_16K);
> + else
> + val |= FIELD_PREP(TCR_TG0, TG0_64K);
> +
> + return val;
> +}
This is all a bit ugly - I'd really like to rely on the real values from
both io_pgtable instances if at all possible...
> +
> static inline u32 arm_smmu_lpae_tcr(struct io_pgtable_cfg *cfg)
> {
> - return TCR_EPD1 |
> - FIELD_PREP(TCR_TG0, cfg->arm_lpae_s1_cfg.tcr.tg) |
> - FIELD_PREP(TCR_SH0, cfg->arm_lpae_s1_cfg.tcr.sh) |
> - FIELD_PREP(TCR_ORGN0, cfg->arm_lpae_s1_cfg.tcr.orgn) |
> - FIELD_PREP(TCR_IRGN0, cfg->arm_lpae_s1_cfg.tcr.irgn) |
> - FIELD_PREP(TCR_T0SZ, cfg->arm_lpae_s1_cfg.tcr.tsz);
> + u32 tcr = FIELD_PREP(TCR_SH0, cfg->arm_lpae_s1_cfg.tcr.sh) |
> + FIELD_PREP(TCR_ORGN0, cfg->arm_lpae_s1_cfg.tcr.orgn) |
> + FIELD_PREP(TCR_IRGN0, cfg->arm_lpae_s1_cfg.tcr.irgn) |
> + FIELD_PREP(TCR_T0SZ, cfg->arm_lpae_s1_cfg.tcr.tsz);
> +
> + if (!(cfg->quirks & IO_PGTABLE_QUIRK_ARM_TTBR1))
> + return tcr | TCR_EPD1 | arm_smmu_lpae_tcr_tg(cfg);
> +
> + return tcr | (tcr << 16) | arm_smmu_lpae_tcr_tg(cfg);
...especially here - leaving TTBR0 enabled but pointing to
who-knows-what until someone fills it in at some arbitrary point in the
future seems rather scary.
I'm looking at iommu_aux_attach_device() and friends, and it appears
pretty achievable to hook that up in a workable manner, even if it's
just routed straight through to the impl to only work within
qcom-specific parameters to begin with. I figure the first
aux_attach_dev sanity-checks that the main domain is using TTBR1 with a
compatible split, sets TTBR0 and updates the merged TCR value at that
point. For subsequent calls it shouldn't need to do much more than
sanity-check that a new aux domain has the same parameters as the
existing one(s) (and again, such checks could potentially even start out
as just "this is OK by construction" comments). I guess we'd probably
want a count of the number of 'live' aux domains so we can simply
disable TTBR0 on the final aux_detach_dev without having to keep
detailed track of whatever the GPU has actually context switched in the
hardware. Can you see any holes in that idea?
I haven't thought it through in detail, but it also feels like between
aux_attach_dev and/or the TTBR1 quirk in attach_dev there ought to be
enough information to influence the context bank allocation or shuffle
any existing domains such that you can ensure that the right thing ends
up in magic context 0 when it needs to be. That could be a pretty neat
and robust way to finally put that to bed.
Robin.
> }
>
> static inline u32 arm_smmu_lpae_tcr2(struct io_pgtable_cfg *cfg)
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v3 2/5] iommu/arm-smmu: Add support for split pagetables
2020-01-21 14:36 ` Robin Murphy
@ 2020-01-21 17:11 ` Jordan Crouse
2020-01-21 18:54 ` Robin Murphy
0 siblings, 1 reply; 17+ messages in thread
From: Jordan Crouse @ 2020-01-21 17:11 UTC (permalink / raw)
To: Robin Murphy
Cc: iommu, will, linux-arm-kernel, linux-arm-msm, linux-kernel, Joerg Roedel
On Tue, Jan 21, 2020 at 02:36:19PM +0000, Robin Murphy wrote:
> On 16/12/2019 4:37 pm, Jordan Crouse wrote:
> >Add support to enable split pagetables (TTBR1) if the supporting driver
> >requests it via the DOMAIN_ATTR_SPLIT_TABLES flag. When enabled, the driver
> >will set up the TTBR0 and TTBR1 regions and program the default domain
> >pagetable on TTBR1.
> >
> >After attaching the device, the value of he domain attribute can
> >be queried to see if the split pagetables were successfully programmed.
> >Furthermore the domain geometry will be updated so that the caller can
> >determine the active region for the pagetable that was programmed.
> >
> >Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> >---
> >
> > drivers/iommu/arm-smmu.c | 40 +++++++++++++++++++++++++++++++++++-----
> > drivers/iommu/arm-smmu.h | 45 +++++++++++++++++++++++++++++++++++++++------
> > 2 files changed, 74 insertions(+), 11 deletions(-)
> >
> >diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> >index c106406..7b59116 100644
> >--- a/drivers/iommu/arm-smmu.c
> >+++ b/drivers/iommu/arm-smmu.c
> >@@ -538,9 +538,17 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
> > cb->ttbr[0] = pgtbl_cfg->arm_v7s_cfg.ttbr;
> > cb->ttbr[1] = 0;
> > } else {
> >- cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
> >- cb->ttbr[0] |= FIELD_PREP(TTBRn_ASID, cfg->asid);
> >- cb->ttbr[1] = FIELD_PREP(TTBRn_ASID, cfg->asid);
> >+ if (pgtbl_cfg->quirks & IO_PGTABLE_QUIRK_ARM_TTBR1) {
> >+ cb->ttbr[0] = FIELD_PREP(TTBRn_ASID, cfg->asid);
> >+ cb->ttbr[1] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
> >+ cb->ttbr[1] |=
> >+ FIELD_PREP(TTBRn_ASID, cfg->asid);
> >+ } else {
> >+ cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
> >+ cb->ttbr[0] |=
> >+ FIELD_PREP(TTBRn_ASID, cfg->asid);
> >+ cb->ttbr[1] = FIELD_PREP(TTBRn_ASID, cfg->asid);
> >+ }
> > }
> > } else {
> > cb->ttbr[0] = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
> >@@ -651,6 +659,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> > enum io_pgtable_fmt fmt;
> > struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> > struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
> >+ u32 quirks = 0;
> > mutex_lock(&smmu_domain->init_mutex);
> > if (smmu_domain->smmu)
> >@@ -719,6 +728,8 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> > oas = smmu->ipa_size;
> > if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64) {
> > fmt = ARM_64_LPAE_S1;
> >+ if (smmu_domain->split_pagetables)
> >+ quirks |= IO_PGTABLE_QUIRK_ARM_TTBR1;
> > } else if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_L) {
> > fmt = ARM_32_LPAE_S1;
> > ias = min(ias, 32UL);
> >@@ -788,6 +799,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> > .coherent_walk = smmu->features & ARM_SMMU_FEAT_COHERENT_WALK,
> > .tlb = smmu_domain->flush_ops,
> > .iommu_dev = smmu->dev,
> >+ .quirks = quirks,
> > };
> > if (smmu_domain->non_strict)
> >@@ -801,8 +813,15 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> > /* Update the domain's page sizes to reflect the page table format */
> > domain->pgsize_bitmap = pgtbl_cfg.pgsize_bitmap;
> >- domain->geometry.aperture_end = (1UL << ias) - 1;
> >- domain->geometry.force_aperture = true;
> >+
> >+ if (pgtbl_cfg.quirks & IO_PGTABLE_QUIRK_ARM_TTBR1) {
> >+ domain->geometry.aperture_start = ~((1ULL << ias) - 1);
>
> AKA "~0UL << ias", if I'm not mistaken ;)
>
> >+ domain->geometry.aperture_end = ~0UL;
> >+ } else {
> >+ domain->geometry.aperture_end = (1UL << ias) - 1;
> >+ domain->geometry.force_aperture = true;
> >+ smmu_domain->split_pagetables = false;
> >+ }
> > /* Initialise the context bank with our page table cfg */
> > arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
> >@@ -1484,6 +1503,9 @@ static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
> > case DOMAIN_ATTR_NESTING:
> > *(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
> > return 0;
> >+ case DOMAIN_ATTR_SPLIT_TABLES:
> >+ *(int *)data = smmu_domain->split_pagetables;
> >+ return 0;
> > default:
> > return -ENODEV;
> > }
> >@@ -1524,6 +1546,14 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
> > else
> > smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
> > break;
> >+ case DOMAIN_ATTR_SPLIT_TABLES:
> >+ if (smmu_domain->smmu) {
> >+ ret = -EPERM;
> >+ goto out_unlock;
> >+ }
> >+ if (*(int *)data)
> >+ smmu_domain->split_pagetables = true;
>
> I still like the idea of passing the actual split point here, but as it is I
> think this sets the scene perfectly for coming back and doing that later.
>
> >+ break;
> > default:
> > ret = -ENODEV;
> > }
> >diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
> >index afab9de..68526cc 100644
> >--- a/drivers/iommu/arm-smmu.h
> >+++ b/drivers/iommu/arm-smmu.h
> >@@ -177,6 +177,16 @@ enum arm_smmu_cbar_type {
> > #define TCR_IRGN0 GENMASK(9, 8)
> > #define TCR_T0SZ GENMASK(5, 0)
> >+#define TCR_TG1 GENMASK(31, 30)
> >+
> >+#define TG0_4K 0
> >+#define TG0_64K 1
> >+#define TG0_16K 2
> >+
> >+#define TG1_16K 1
> >+#define TG1_4K 2
> >+#define TG1_64K 3
> >+
> > #define ARM_SMMU_CB_CONTEXTIDR 0x34
> > #define ARM_SMMU_CB_S1_MAIR0 0x38
> > #define ARM_SMMU_CB_S1_MAIR1 0x3c
> >@@ -329,16 +339,39 @@ struct arm_smmu_domain {
> > struct mutex init_mutex; /* Protects smmu pointer */
> > spinlock_t cb_lock; /* Serialises ATS1* ops and TLB syncs */
> > struct iommu_domain domain;
> >+ bool split_pagetables;
> > };
> >+static inline u32 arm_smmu_lpae_tcr_tg(struct io_pgtable_cfg *cfg)
> >+{
> >+ u32 val;
> >+
> >+ if (!(cfg->quirks & IO_PGTABLE_QUIRK_ARM_TTBR1))
> >+ return FIELD_PREP(TCR_TG0, cfg->arm_lpae_s1_cfg.tcr.tg);
> >+
> >+ val = FIELD_PREP(TCR_TG1, cfg->arm_lpae_s1_cfg.tcr.tg);
> >+
> >+ if (cfg->arm_lpae_s1_cfg.tcr.tg == TG1_4K)
> >+ val |= FIELD_PREP(TCR_TG0, TG0_4K);
> >+ else if (cfg->arm_lpae_s1_cfg.tcr.tg == TG1_16K)
> >+ val |= FIELD_PREP(TCR_TG0, TG0_16K);
> >+ else
> >+ val |= FIELD_PREP(TCR_TG0, TG0_64K);
> >+
> >+ return val;
> >+}
>
> This is all a bit ugly - I'd really like to rely on the real values from
> both io_pgtable instances if at all possible...
>
> >+
> > static inline u32 arm_smmu_lpae_tcr(struct io_pgtable_cfg *cfg)
> > {
> >- return TCR_EPD1 |
> >- FIELD_PREP(TCR_TG0, cfg->arm_lpae_s1_cfg.tcr.tg) |
> >- FIELD_PREP(TCR_SH0, cfg->arm_lpae_s1_cfg.tcr.sh) |
> >- FIELD_PREP(TCR_ORGN0, cfg->arm_lpae_s1_cfg.tcr.orgn) |
> >- FIELD_PREP(TCR_IRGN0, cfg->arm_lpae_s1_cfg.tcr.irgn) |
> >- FIELD_PREP(TCR_T0SZ, cfg->arm_lpae_s1_cfg.tcr.tsz);
> >+ u32 tcr = FIELD_PREP(TCR_SH0, cfg->arm_lpae_s1_cfg.tcr.sh) |
> >+ FIELD_PREP(TCR_ORGN0, cfg->arm_lpae_s1_cfg.tcr.orgn) |
> >+ FIELD_PREP(TCR_IRGN0, cfg->arm_lpae_s1_cfg.tcr.irgn) |
> >+ FIELD_PREP(TCR_T0SZ, cfg->arm_lpae_s1_cfg.tcr.tsz);
> >+
> >+ if (!(cfg->quirks & IO_PGTABLE_QUIRK_ARM_TTBR1))
> >+ return tcr | TCR_EPD1 | arm_smmu_lpae_tcr_tg(cfg);
> >+
> >+ return tcr | (tcr << 16) | arm_smmu_lpae_tcr_tg(cfg);
>
> ...especially here - leaving TTBR0 enabled but pointing to who-knows-what
> until someone fills it in at some arbitrary point in the future seems rather
> scary.
>
> I'm looking at iommu_aux_attach_device() and friends, and it appears pretty
> achievable to hook that up in a workable manner, even if it's just routed
> straight through to the impl to only work within qcom-specific parameters to
> begin with. I figure the first aux_attach_dev sanity-checks that the main
> domain is using TTBR1 with a compatible split, sets TTBR0 and updates the
> merged TCR value at that point. For subsequent calls it shouldn't need to do
> much more than sanity-check that a new aux domain has the same parameters as
> the existing one(s) (and again, such checks could potentially even start out
> as just "this is OK by construction" comments). I guess we'd probably want a
> count of the number of 'live' aux domains so we can simply disable TTBR0 on
> the final aux_detach_dev without having to keep detailed track of whatever
> the GPU has actually context switched in the hardware. Can you see any holes
> in that idea?
Let me repeat this back just to be sure we're on the same page. When the quirk
is enabled on the primary domain, we'll set up TTBR1 and leave TTBR0 disabled.
Then, when the first aux domain is attached we will set up that io_ptgable
to enable TTBR0 and then let the GPU do what the GPU does until the last aux is
detached and we can switch off TTBR0 again.
I like this. I'll have to do a bit more exploration because the original aux
design assumed that we didn't need to touch the hardware and I'm not sure if
there are any resource contention issues between the primary domain and the aux
domain. Luckily, these should be solvable if they exist (and the original design
didn't take into account the TLB flush problem so this was likely something we
had to do anyway).
> I haven't thought it through in detail, but it also feels like between
> aux_attach_dev and/or the TTBR1 quirk in attach_dev there ought to be enough
> information to influence the context bank allocation or shuffle any existing
> domains such that you can ensure that the right thing ends up in magic
> context 0 when it needs to be. That could be a pretty neat and robust way to
> finally put that to bed.
I'll try to wrap my brain around this as well. Seems like we could do a magic
swizzle of the SID mappings but I'm not sure how we could safely pull that off
on an existing domain. Maybe I'm overthinking it.
I'll spin up a new copy of the TTBR1 quirk patch and revive the aux domain stuff
and then we can go from there.
Thanks,
Jordan
> Robin.
>
> > }
> > static inline u32 arm_smmu_lpae_tcr2(struct io_pgtable_cfg *cfg)
> >
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v3 2/5] iommu/arm-smmu: Add support for split pagetables
2020-01-21 17:11 ` Jordan Crouse
@ 2020-01-21 18:54 ` Robin Murphy
0 siblings, 0 replies; 17+ messages in thread
From: Robin Murphy @ 2020-01-21 18:54 UTC (permalink / raw)
To: iommu, will, linux-arm-kernel, linux-arm-msm, linux-kernel, Joerg Roedel
On 21/01/2020 5:11 pm, Jordan Crouse wrote:
[...]
>> I'm looking at iommu_aux_attach_device() and friends, and it appears pretty
>> achievable to hook that up in a workable manner, even if it's just routed
>> straight through to the impl to only work within qcom-specific parameters to
>> begin with. I figure the first aux_attach_dev sanity-checks that the main
>> domain is using TTBR1 with a compatible split, sets TTBR0 and updates the
>> merged TCR value at that point. For subsequent calls it shouldn't need to do
>> much more than sanity-check that a new aux domain has the same parameters as
>> the existing one(s) (and again, such checks could potentially even start out
>> as just "this is OK by construction" comments). I guess we'd probably want a
>> count of the number of 'live' aux domains so we can simply disable TTBR0 on
>> the final aux_detach_dev without having to keep detailed track of whatever
>> the GPU has actually context switched in the hardware. Can you see any holes
>> in that idea?
>
> Let me repeat this back just to be sure we're on the same page. When the quirk
> is enabled on the primary domain, we'll set up TTBR1 and leave TTBR0 disabled.
> Then, when the first aux domain is attached we will set up that io_ptgable
> to enable TTBR0 and then let the GPU do what the GPU does until the last aux is
> detached and we can switch off TTBR0 again.
>
> I like this. I'll have to do a bit more exploration because the original aux
> design assumed that we didn't need to touch the hardware and I'm not sure if
> there are any resource contention issues between the primary domain and the aux
> domain. Luckily, these should be solvable if they exist (and the original design
> didn't take into account the TLB flush problem so this was likely something we
> had to do anyway).
Yeah, sounds like you've got it (somehow I'd completely forgotten that
you'd already prototyped the aux domain part, and I only re-read the
cover letter after sending that review...). TBH it's not massively
different, just being a bit more honest about the intermediate hardware
state. As long as we can rely on all aux domains being equivalent and
the GPU never writing nonsense to TTBR0, then all arm-smmu really wants
to care about is whether there's *something* live or not at any given
time, so attach (with quirk) does:
TTBR1 = primary_domain->ttbr
TCR = primary_domain->tcr | EPD0
then attach_aux comes along and adds:
TTBR0 = aux_domain->ttbr
TCR = primary_doman->tcr | aux_domain->tcr
such that arm-smmu can be happy that TTBR0 is always pointing at *some*
valid pagetable from that point on regardless of what subsequently
happens underneath, and nobody need touch TCR until the party's
completely over.
>> I haven't thought it through in detail, but it also feels like between
>> aux_attach_dev and/or the TTBR1 quirk in attach_dev there ought to be enough
>> information to influence the context bank allocation or shuffle any existing
>> domains such that you can ensure that the right thing ends up in magic
>> context 0 when it needs to be. That could be a pretty neat and robust way to
>> finally put that to bed.
>
> I'll try to wrap my brain around this as well. Seems like we could do a magic
> swizzle of the SID mappings but I'm not sure how we could safely pull that off
> on an existing domain. Maybe I'm overthinking it.
What I'm imagining isn't all that far from how we do normal domain
attach, except instead of setting up the newly-allocated context for a
new domain you simply clone the existing context into it, and instead of
having a given device's set of Stream IDs to retarget you'd just scan
though the S2CRs checking cbndx and rewriting as appropriate. Then
finally rewrite domain->cfg.cbndx and the old context is all yours.
> I'll spin up a new copy of the TTBR1 quirk patch and revive the aux domain stuff
> and then we can go from there.
Sounds good, thanks!
Robin.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/5] iommu/arm-smmu: Add support for split pagetables
2019-12-16 16:37 ` [PATCH v3 2/5] iommu/arm-smmu: Add support for split pagetables Jordan Crouse
2020-01-09 14:33 ` Will Deacon
2020-01-21 14:36 ` Robin Murphy
@ 2020-01-21 14:47 ` Robin Murphy
2 siblings, 0 replies; 17+ messages in thread
From: Robin Murphy @ 2020-01-21 14:47 UTC (permalink / raw)
To: Jordan Crouse, iommu
Cc: will, linux-arm-kernel, linux-arm-msm, linux-kernel, Joerg Roedel
Oh, one more thing...
On 16/12/2019 4:37 pm, Jordan Crouse wrote:
[...]
> @@ -651,6 +659,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> enum io_pgtable_fmt fmt;
> struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
> + u32 quirks = 0;
>
> mutex_lock(&smmu_domain->init_mutex);
> if (smmu_domain->smmu)
> @@ -719,6 +728,8 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> oas = smmu->ipa_size;
> if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64) {
> fmt = ARM_64_LPAE_S1;
> + if (smmu_domain->split_pagetables)
> + quirks |= IO_PGTABLE_QUIRK_ARM_TTBR1;
To avoid me forgetting and questioning it again in future, I'd recommend
sticking a comment somewhere near here that we don't reduce cfg->ias in
this case because we're currently assuming SEP_UPSTREAM.
Robin.
> } else if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_L) {
> fmt = ARM_32_LPAE_S1;
> ias = min(ias, 32UL);
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 3/5] drm/msm: Attach the IOMMU device during initialization
2019-12-16 16:37 [PATCH v3 0/5] iommu/arm-smmu: Split pagetable support for arm-smmu-v2 Jordan Crouse
2019-12-16 16:37 ` [PATCH v3 1/5] iommu: Add DOMAIN_ATTR_SPLIT_TABLES Jordan Crouse
2019-12-16 16:37 ` [PATCH v3 2/5] iommu/arm-smmu: Add support for split pagetables Jordan Crouse
@ 2019-12-16 16:37 ` Jordan Crouse
2019-12-16 16:37 ` [PATCH v3 4/5] drm/msm: Refactor address space initialization Jordan Crouse
2019-12-16 16:37 ` [PATCH v3 5/5] drm/msm/a6xx: Support split pagetables Jordan Crouse
4 siblings, 0 replies; 17+ messages in thread
From: Jordan Crouse @ 2019-12-16 16:37 UTC (permalink / raw)
To: iommu
Cc: robin.murphy, will, linux-arm-kernel, linux-arm-msm,
David Airlie, Sean Paul, zhengbin, AngeloGioacchino Del Regno,
freedreno, Sam Ravnborg, dri-devel, Jeykumar Sankaran,
Fritz Koenig, Rob Clark, Thomas Gleixner, Drew Davenport,
Georgi Djakov, Daniel Vetter, Jeffrey Hugo, linux-kernel,
Greg Kroah-Hartman
Everywhere an IOMMU object is created by msm_gpu_create_address_space
the IOMMU device is attached immediately after. Instead of carrying around
the infrastructure to do the attach from the device specific code do it
directly in the msm_iommu_init() function. This gets it out of the way for
more aggressive cleanups that follow.
Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 8 --------
drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 4 ----
drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 7 -------
drivers/gpu/drm/msm/msm_gem_vma.c | 23 +++++++++++++++++++----
drivers/gpu/drm/msm/msm_gpu.c | 11 +----------
drivers/gpu/drm/msm/msm_gpummu.c | 6 ------
drivers/gpu/drm/msm/msm_iommu.c | 15 +++++++--------
drivers/gpu/drm/msm/msm_mmu.h | 1 -
8 files changed, 27 insertions(+), 48 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 6c92f0f..b082b23 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -704,7 +704,6 @@ static int _dpu_kms_mmu_init(struct dpu_kms *dpu_kms)
{
struct iommu_domain *domain;
struct msm_gem_address_space *aspace;
- int ret;
domain = iommu_domain_alloc(&platform_bus_type);
if (!domain)
@@ -720,13 +719,6 @@ static int _dpu_kms_mmu_init(struct dpu_kms *dpu_kms)
return PTR_ERR(aspace);
}
- ret = aspace->mmu->funcs->attach(aspace->mmu);
- if (ret) {
- DPU_ERROR("failed to attach iommu %d\n", ret);
- msm_gem_address_space_put(aspace);
- return ret;
- }
-
dpu_kms->base.aspace = aspace;
return 0;
}
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index dda0543..9dba37c 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -518,10 +518,6 @@ struct msm_kms *mdp4_kms_init(struct drm_device *dev)
}
kms->aspace = aspace;
-
- ret = aspace->mmu->funcs->attach(aspace->mmu);
- if (ret)
- goto fail;
} else {
DRM_DEV_INFO(dev->dev, "no iommu, fallback to phys "
"contig buffers for scanout\n");
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index e43ecd4..653dab2 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -736,13 +736,6 @@ struct msm_kms *mdp5_kms_init(struct drm_device *dev)
}
kms->aspace = aspace;
-
- ret = aspace->mmu->funcs->attach(aspace->mmu);
- if (ret) {
- DRM_DEV_ERROR(&pdev->dev, "failed to attach iommu: %d\n",
- ret);
- goto fail;
- }
} else {
DRM_DEV_INFO(&pdev->dev,
"no iommu, fallback to phys contig buffers for scanout\n");
diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
index 1af5354..91d993a 100644
--- a/drivers/gpu/drm/msm/msm_gem_vma.c
+++ b/drivers/gpu/drm/msm/msm_gem_vma.c
@@ -131,8 +131,8 @@ msm_gem_address_space_create(struct device *dev, struct iommu_domain *domain,
const char *name)
{
struct msm_gem_address_space *aspace;
- u64 size = domain->geometry.aperture_end -
- domain->geometry.aperture_start;
+ u64 start = domain->geometry.aperture_start;
+ u64 size = domain->geometry.aperture_end - start;
aspace = kzalloc(sizeof(*aspace), GFP_KERNEL);
if (!aspace)
@@ -141,9 +141,18 @@ msm_gem_address_space_create(struct device *dev, struct iommu_domain *domain,
spin_lock_init(&aspace->lock);
aspace->name = name;
aspace->mmu = msm_iommu_new(dev, domain);
+ if (IS_ERR(aspace->mmu)) {
+ int ret = PTR_ERR(aspace->mmu);
- drm_mm_init(&aspace->mm, (domain->geometry.aperture_start >> PAGE_SHIFT),
- size >> PAGE_SHIFT);
+ kfree(aspace);
+ return ERR_PTR(ret);
+ }
+
+ /*
+ * Attaching the IOMMU device changes the aperture values so use the
+ * cached values instead
+ */
+ drm_mm_init(&aspace->mm, start >> PAGE_SHIFT, size >> PAGE_SHIFT);
kref_init(&aspace->kref);
@@ -164,6 +173,12 @@ msm_gem_address_space_create_a2xx(struct device *dev, struct msm_gpu *gpu,
spin_lock_init(&aspace->lock);
aspace->name = name;
aspace->mmu = msm_gpummu_new(dev, gpu);
+ if (IS_ERR(aspace->mmu)) {
+ int ret = PTR_ERR(aspace->mmu);
+
+ kfree(aspace);
+ return ERR_PTR(ret);
+ }
drm_mm_init(&aspace->mm, (va_start >> PAGE_SHIFT),
size >> PAGE_SHIFT);
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 18f3a5c..f7bf80e 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -808,7 +808,6 @@ msm_gpu_create_address_space(struct msm_gpu *gpu, struct platform_device *pdev,
uint64_t va_start, uint64_t va_end)
{
struct msm_gem_address_space *aspace;
- int ret;
/*
* Setup IOMMU.. eventually we will (I think) do this once per context
@@ -833,17 +832,9 @@ msm_gpu_create_address_space(struct msm_gpu *gpu, struct platform_device *pdev,
va_start, va_end);
}
- if (IS_ERR(aspace)) {
+ if (IS_ERR(aspace))
DRM_DEV_ERROR(gpu->dev->dev, "failed to init mmu: %ld\n",
PTR_ERR(aspace));
- return ERR_CAST(aspace);
- }
-
- ret = aspace->mmu->funcs->attach(aspace->mmu);
- if (ret) {
- msm_gem_address_space_put(aspace);
- return ERR_PTR(ret);
- }
return aspace;
}
diff --git a/drivers/gpu/drm/msm/msm_gpummu.c b/drivers/gpu/drm/msm/msm_gpummu.c
index 34980d8..0ad0f84 100644
--- a/drivers/gpu/drm/msm/msm_gpummu.c
+++ b/drivers/gpu/drm/msm/msm_gpummu.c
@@ -21,11 +21,6 @@ struct msm_gpummu {
#define GPUMMU_PAGE_SIZE SZ_4K
#define TABLE_SIZE (sizeof(uint32_t) * GPUMMU_VA_RANGE / GPUMMU_PAGE_SIZE)
-static int msm_gpummu_attach(struct msm_mmu *mmu)
-{
- return 0;
-}
-
static void msm_gpummu_detach(struct msm_mmu *mmu)
{
}
@@ -85,7 +80,6 @@ static void msm_gpummu_destroy(struct msm_mmu *mmu)
}
static const struct msm_mmu_funcs funcs = {
- .attach = msm_gpummu_attach,
.detach = msm_gpummu_detach,
.map = msm_gpummu_map,
.unmap = msm_gpummu_unmap,
diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
index ad58cfe..544c519 100644
--- a/drivers/gpu/drm/msm/msm_iommu.c
+++ b/drivers/gpu/drm/msm/msm_iommu.c
@@ -23,13 +23,6 @@ static int msm_fault_handler(struct iommu_domain *domain, struct device *dev,
return 0;
}
-static int msm_iommu_attach(struct msm_mmu *mmu)
-{
- struct msm_iommu *iommu = to_msm_iommu(mmu);
-
- return iommu_attach_device(iommu->domain, mmu->dev);
-}
-
static void msm_iommu_detach(struct msm_mmu *mmu)
{
struct msm_iommu *iommu = to_msm_iommu(mmu);
@@ -66,7 +59,6 @@ static void msm_iommu_destroy(struct msm_mmu *mmu)
}
static const struct msm_mmu_funcs funcs = {
- .attach = msm_iommu_attach,
.detach = msm_iommu_detach,
.map = msm_iommu_map,
.unmap = msm_iommu_unmap,
@@ -76,6 +68,7 @@ static const struct msm_mmu_funcs funcs = {
struct msm_mmu *msm_iommu_new(struct device *dev, struct iommu_domain *domain)
{
struct msm_iommu *iommu;
+ int ret;
iommu = kzalloc(sizeof(*iommu), GFP_KERNEL);
if (!iommu)
@@ -85,5 +78,11 @@ struct msm_mmu *msm_iommu_new(struct device *dev, struct iommu_domain *domain)
msm_mmu_init(&iommu->base, dev, &funcs);
iommu_set_fault_handler(domain, msm_fault_handler, iommu);
+ ret = iommu_attach_device(iommu->domain, dev);
+ if (ret) {
+ kfree(iommu);
+ return ERR_PTR(ret);
+ }
+
return &iommu->base;
}
diff --git a/drivers/gpu/drm/msm/msm_mmu.h b/drivers/gpu/drm/msm/msm_mmu.h
index 67a623f..bae9e8e 100644
--- a/drivers/gpu/drm/msm/msm_mmu.h
+++ b/drivers/gpu/drm/msm/msm_mmu.h
@@ -10,7 +10,6 @@
#include <linux/iommu.h>
struct msm_mmu_funcs {
- int (*attach)(struct msm_mmu *mmu);
void (*detach)(struct msm_mmu *mmu);
int (*map)(struct msm_mmu *mmu, uint64_t iova, struct sg_table *sgt,
unsigned len, int prot);
--
2.7.4
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v3 4/5] drm/msm: Refactor address space initialization
2019-12-16 16:37 [PATCH v3 0/5] iommu/arm-smmu: Split pagetable support for arm-smmu-v2 Jordan Crouse
` (2 preceding siblings ...)
2019-12-16 16:37 ` [PATCH v3 3/5] drm/msm: Attach the IOMMU device during initialization Jordan Crouse
@ 2019-12-16 16:37 ` Jordan Crouse
2020-01-06 21:59 ` [Freedreno] " Jordan Crouse
2019-12-16 16:37 ` [PATCH v3 5/5] drm/msm/a6xx: Support split pagetables Jordan Crouse
4 siblings, 1 reply; 17+ messages in thread
From: Jordan Crouse @ 2019-12-16 16:37 UTC (permalink / raw)
To: iommu
Cc: robin.murphy, will, linux-arm-kernel, linux-arm-msm, Sean Paul,
dri-devel, linux-kernel, Fritz Koenig, David Airlie,
Allison Randal, AngeloGioacchino Del Regno, Thomas Gleixner,
Douglas Anderson, Rob Clark, Greg Kroah-Hartman, Jeffrey Hugo,
Wen Yang, Alexios Zavras, Jeykumar Sankaran, zhengbin,
Sam Ravnborg, Brian Masney, Drew Davenport, Georgi Djakov,
freedreno, Ben Dooks, Daniel Vetter
Refactor how address space initialization works. Instead of having the
address space function create the MMU object (and thus require separate but
equal functions for gpummu and iommu) use a single function and pass the
MMU struct. Make the generic code cleaner by using target specific
functions to create the address space so a2xx can do its own thing in its
own space. For all the other targets use a generic helper to initialize
IOMMU but leave the door open for newer targets to use customization
if they need it.
Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
---
drivers/gpu/drm/msm/adreno/a2xx_gpu.c | 16 ++++++++++
drivers/gpu/drm/msm/adreno/a3xx_gpu.c | 1 +
drivers/gpu/drm/msm/adreno/a4xx_gpu.c | 1 +
drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 1 +
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 1 +
drivers/gpu/drm/msm/adreno/adreno_gpu.c | 23 ++++++++++----
drivers/gpu/drm/msm/adreno/adreno_gpu.h | 8 +++++
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 10 +++---
drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 14 +++++----
drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c | 4 ---
drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 11 +++++--
drivers/gpu/drm/msm/msm_drv.h | 8 ++---
drivers/gpu/drm/msm/msm_gem_vma.c | 52 +++++---------------------------
drivers/gpu/drm/msm/msm_gpu.c | 40 ++----------------------
drivers/gpu/drm/msm/msm_gpu.h | 4 +--
drivers/gpu/drm/msm/msm_iommu.c | 3 ++
16 files changed, 83 insertions(+), 114 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
index 1f83bc1..60f6472 100644
--- a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
@@ -401,6 +401,21 @@ static struct msm_gpu_state *a2xx_gpu_state_get(struct msm_gpu *gpu)
return state;
}
+static struct msm_gem_address_space *
+a2xx_create_address_space(struct msm_gpu *gpu, struct platform_device *pdev)
+{
+ struct msm_mmu *mmu = msm_gpummu_new(&pdev->dev, gpu);
+ struct msm_gem_address_space *aspace;
+
+ aspace = msm_gem_address_space_create(mmu, "gpu", SZ_16M,
+ SZ_16M + 0xfff * SZ_64K);
+
+ if (IS_ERR(aspace) && !IS_ERR(mmu))
+ mmu->funcs->destroy(mmu);
+
+ return aspace;
+}
+
/* Register offset defines for A2XX - copy of A3XX */
static const unsigned int a2xx_register_offsets[REG_ADRENO_REGISTER_MAX] = {
REG_ADRENO_DEFINE(REG_ADRENO_CP_RB_BASE, REG_AXXX_CP_RB_BASE),
@@ -429,6 +444,7 @@ static const struct adreno_gpu_funcs funcs = {
#endif
.gpu_state_get = a2xx_gpu_state_get,
.gpu_state_put = adreno_gpu_state_put,
+ .create_address_space = a2xx_create_address_space,
},
};
diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
index 7ad1493..41e51e0 100644
--- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
@@ -441,6 +441,7 @@ static const struct adreno_gpu_funcs funcs = {
#endif
.gpu_state_get = a3xx_gpu_state_get,
.gpu_state_put = adreno_gpu_state_put,
+ .create_address_space = adreno_iommu_create_address_space,
},
};
diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
index b01388a..3655440 100644
--- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
@@ -532,6 +532,7 @@ static const struct adreno_gpu_funcs funcs = {
#endif
.gpu_state_get = a4xx_gpu_state_get,
.gpu_state_put = adreno_gpu_state_put,
+ .create_address_space = adreno_iommu_create_address_space,
},
.get_timestamp = a4xx_get_timestamp,
};
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index b02e204..0f5db72 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -1432,6 +1432,7 @@ static const struct adreno_gpu_funcs funcs = {
.gpu_busy = a5xx_gpu_busy,
.gpu_state_get = a5xx_gpu_state_get,
.gpu_state_put = a5xx_gpu_state_put,
+ .create_address_space = adreno_iommu_create_address_space,
},
.get_timestamp = a5xx_get_timestamp,
};
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index dc8ec2c..5dc0b2c 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -832,6 +832,7 @@ static const struct adreno_gpu_funcs funcs = {
#if defined(CONFIG_DRM_MSM_GPU_STATE)
.gpu_state_get = a6xx_gpu_state_get,
.gpu_state_put = a6xx_gpu_state_put,
+ .create_address_space = adreno_iommu_create_address_space,
#endif
},
.get_timestamp = a6xx_get_timestamp,
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 0783e4b..09c57891 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -157,6 +157,23 @@ int adreno_zap_shader_load(struct msm_gpu *gpu, u32 pasid)
return zap_shader_load_mdt(gpu, adreno_gpu->info->zapfw, pasid);
}
+struct msm_gem_address_space *
+adreno_iommu_create_address_space(struct msm_gpu *gpu,
+ struct platform_device *pdev)
+{
+ struct iommu_domain *iommu = iommu_domain_alloc(&platform_bus_type);
+ struct msm_mmu *mmu = msm_iommu_new(&pdev->dev, iommu);
+ struct msm_gem_address_space *aspace;
+
+ aspace = msm_gem_address_space_create(mmu, "gpu", SZ_16M,
+ 0xfffffff);
+
+ if (IS_ERR(aspace) && !IS_ERR(mmu))
+ mmu->funcs->destroy(mmu);
+
+ return aspace;
+}
+
int adreno_get_param(struct msm_gpu *gpu, uint32_t param, uint64_t *value)
{
struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
@@ -949,12 +966,6 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
adreno_gpu_config.ioname = "kgsl_3d0_reg_memory";
- adreno_gpu_config.va_start = SZ_16M;
- adreno_gpu_config.va_end = 0xffffffff;
- /* maximum range of a2xx mmu */
- if (adreno_is_a2xx(adreno_gpu))
- adreno_gpu_config.va_end = SZ_16M + 0xfff * SZ_64K;
-
adreno_gpu_config.nr_rings = nr_rings;
adreno_get_pwrlevels(&pdev->dev, gpu);
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index e71a757..5c1aa12 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -263,6 +263,14 @@ int adreno_gpu_state_get(struct msm_gpu *gpu, struct msm_gpu_state *state);
int adreno_gpu_state_put(struct msm_gpu_state *state);
/*
+ * Common helper function to initialize the default address space for arm-smmu
+ * attached targets
+ */
+struct msm_gem_address_space *
+adreno_iommu_create_address_space(struct msm_gpu *gpu,
+ struct platform_device *pdev);
+
+/*
* For a5xx and a6xx targets load the zap shader that is used to pull the GPU
* out of secure mode
*/
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index b082b23..4e6ebbd 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -704,18 +704,18 @@ static int _dpu_kms_mmu_init(struct dpu_kms *dpu_kms)
{
struct iommu_domain *domain;
struct msm_gem_address_space *aspace;
+ struct msm_mmu *mmu;
domain = iommu_domain_alloc(&platform_bus_type);
if (!domain)
return 0;
- domain->geometry.aperture_start = 0x1000;
- domain->geometry.aperture_end = 0xffffffff;
+ mmu = msm_iommu_new(dpu_kms->dev->dev, domain);
+ aspace = msm_gem_address_space_create(mmu, "dpu1",
+ 0x1000, 0xfffffff);
- aspace = msm_gem_address_space_create(dpu_kms->dev->dev,
- domain, "dpu1");
if (IS_ERR(aspace)) {
- iommu_domain_free(domain);
+ mmu->funcs->destroy(mmu);
return PTR_ERR(aspace);
}
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index 9dba37c..0889718 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -510,9 +510,15 @@ struct msm_kms *mdp4_kms_init(struct drm_device *dev)
mdelay(16);
if (config->iommu) {
- aspace = msm_gem_address_space_create(&pdev->dev,
- config->iommu, "mdp4");
+ struct msm_mmu *mmu = msm_iommu_new(&pdev->dev,
+ config->iommu);
+
+ aspace = msm_gem_address_space_create(mmu,
+ "mdp4", 0x1000, 0xffffffff);
+
if (IS_ERR(aspace)) {
+ if (!IS_ERR(mmu))
+ mmu->funcs->destroy(mmu);
ret = PTR_ERR(aspace);
goto fail;
}
@@ -565,10 +571,6 @@ static struct mdp4_platform_config *mdp4_get_config(struct platform_device *dev)
/* TODO: Chips that aren't apq8064 have a 200 Mhz max_clk */
config.max_clk = 266667000;
config.iommu = iommu_domain_alloc(&platform_bus_type);
- if (config.iommu) {
- config.iommu->geometry.aperture_start = 0x1000;
- config.iommu->geometry.aperture_end = 0xffffffff;
- }
return &config;
}
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
index 1f48f64..ebd651a 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
@@ -941,10 +941,6 @@ static struct mdp5_cfg_platform *mdp5_get_config(struct platform_device *dev)
static struct mdp5_cfg_platform config = {};
config.iommu = iommu_domain_alloc(&platform_bus_type);
- if (config.iommu) {
- config.iommu->geometry.aperture_start = 0x1000;
- config.iommu->geometry.aperture_end = 0xffffffff;
- }
return &config;
}
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 653dab2..20bdff9 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -724,13 +724,20 @@ struct msm_kms *mdp5_kms_init(struct drm_device *dev)
mdelay(16);
if (config->platform.iommu) {
+ struct msm_mmu *mmu;
+
iommu_dev = &pdev->dev;
if (!iommu_dev->iommu_fwspec)
iommu_dev = iommu_dev->parent;
- aspace = msm_gem_address_space_create(iommu_dev,
- config->platform.iommu, "mdp5");
+ mmu = msm_iommu_new(iommu_dev, config->platform.iommu);
+
+ aspace = msm_gem_address_space_create(mmu, "mdp5",
+ 0x1000, 0xffffffff);
+
if (IS_ERR(aspace)) {
+ if (!IS_ERR(mmu))
+ mmu->funcs->destroy(mmu);
ret = PTR_ERR(aspace);
goto fail;
}
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 71547e7..2203729 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -247,12 +247,8 @@ void msm_gem_close_vma(struct msm_gem_address_space *aspace,
void msm_gem_address_space_put(struct msm_gem_address_space *aspace);
struct msm_gem_address_space *
-msm_gem_address_space_create(struct device *dev, struct iommu_domain *domain,
- const char *name);
-
-struct msm_gem_address_space *
-msm_gem_address_space_create_a2xx(struct device *dev, struct msm_gpu *gpu,
- const char *name, uint64_t va_start, uint64_t va_end);
+msm_gem_address_space_create(struct msm_mmu *mmu, const char *name,
+ u64 va_start, u64 va_end);
int msm_register_mmu(struct drm_device *dev, struct msm_mmu *mmu);
void msm_unregister_mmu(struct drm_device *dev, struct msm_mmu *mmu);
diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
index 91d993a..075ce52 100644
--- a/drivers/gpu/drm/msm/msm_gem_vma.c
+++ b/drivers/gpu/drm/msm/msm_gem_vma.c
@@ -125,63 +125,25 @@ int msm_gem_init_vma(struct msm_gem_address_space *aspace,
return 0;
}
-
struct msm_gem_address_space *
-msm_gem_address_space_create(struct device *dev, struct iommu_domain *domain,
- const char *name)
-{
- struct msm_gem_address_space *aspace;
- u64 start = domain->geometry.aperture_start;
- u64 size = domain->geometry.aperture_end - start;
-
- aspace = kzalloc(sizeof(*aspace), GFP_KERNEL);
- if (!aspace)
- return ERR_PTR(-ENOMEM);
-
- spin_lock_init(&aspace->lock);
- aspace->name = name;
- aspace->mmu = msm_iommu_new(dev, domain);
- if (IS_ERR(aspace->mmu)) {
- int ret = PTR_ERR(aspace->mmu);
-
- kfree(aspace);
- return ERR_PTR(ret);
- }
-
- /*
- * Attaching the IOMMU device changes the aperture values so use the
- * cached values instead
- */
- drm_mm_init(&aspace->mm, start >> PAGE_SHIFT, size >> PAGE_SHIFT);
-
- kref_init(&aspace->kref);
-
- return aspace;
-}
-
-struct msm_gem_address_space *
-msm_gem_address_space_create_a2xx(struct device *dev, struct msm_gpu *gpu,
- const char *name, uint64_t va_start, uint64_t va_end)
+msm_gem_address_space_create(struct msm_mmu *mmu, const char *name,
+ u64 va_start, u64 va_end)
{
struct msm_gem_address_space *aspace;
u64 size = va_end - va_start;
+ if (IS_ERR(mmu))
+ return ERR_CAST(mmu);
+
aspace = kzalloc(sizeof(*aspace), GFP_KERNEL);
if (!aspace)
return ERR_PTR(-ENOMEM);
spin_lock_init(&aspace->lock);
aspace->name = name;
- aspace->mmu = msm_gpummu_new(dev, gpu);
- if (IS_ERR(aspace->mmu)) {
- int ret = PTR_ERR(aspace->mmu);
-
- kfree(aspace);
- return ERR_PTR(ret);
- }
+ aspace->mmu = mmu;
- drm_mm_init(&aspace->mm, (va_start >> PAGE_SHIFT),
- size >> PAGE_SHIFT);
+ drm_mm_init(&aspace->mm, va_start >> PAGE_SHIFT, size >> PAGE_SHIFT);
kref_init(&aspace->kref);
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index f7bf80e..f11df53 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -803,42 +803,6 @@ static int get_clocks(struct platform_device *pdev, struct msm_gpu *gpu)
return 0;
}
-static struct msm_gem_address_space *
-msm_gpu_create_address_space(struct msm_gpu *gpu, struct platform_device *pdev,
- uint64_t va_start, uint64_t va_end)
-{
- struct msm_gem_address_space *aspace;
-
- /*
- * Setup IOMMU.. eventually we will (I think) do this once per context
- * and have separate page tables per context. For now, to keep things
- * simple and to get something working, just use a single address space:
- */
- if (!adreno_is_a2xx(to_adreno_gpu(gpu))) {
- struct iommu_domain *iommu = iommu_domain_alloc(&platform_bus_type);
- if (!iommu)
- return NULL;
-
- iommu->geometry.aperture_start = va_start;
- iommu->geometry.aperture_end = va_end;
-
- DRM_DEV_INFO(gpu->dev->dev, "%s: using IOMMU\n", gpu->name);
-
- aspace = msm_gem_address_space_create(&pdev->dev, iommu, "gpu");
- if (IS_ERR(aspace))
- iommu_domain_free(iommu);
- } else {
- aspace = msm_gem_address_space_create_a2xx(&pdev->dev, gpu, "gpu",
- va_start, va_end);
- }
-
- if (IS_ERR(aspace))
- DRM_DEV_ERROR(gpu->dev->dev, "failed to init mmu: %ld\n",
- PTR_ERR(aspace));
-
- return aspace;
-}
-
int msm_gpu_init(struct drm_device *drm, struct platform_device *pdev,
struct msm_gpu *gpu, const struct msm_gpu_funcs *funcs,
const char *name, struct msm_gpu_config *config)
@@ -911,8 +875,8 @@ int msm_gpu_init(struct drm_device *drm, struct platform_device *pdev,
msm_devfreq_init(gpu);
- gpu->aspace = msm_gpu_create_address_space(gpu, pdev,
- config->va_start, config->va_end);
+
+ gpu->aspace = gpu->funcs->create_address_space(gpu, pdev);
if (gpu->aspace == NULL)
DRM_DEV_INFO(drm->dev, "%s: no IOMMU, fallback to VRAM carveout!\n", name);
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index ab8f0f9c..41d86c2 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -21,8 +21,6 @@ struct msm_gpu_state;
struct msm_gpu_config {
const char *ioname;
- uint64_t va_start;
- uint64_t va_end;
unsigned int nr_rings;
};
@@ -64,6 +62,8 @@ struct msm_gpu_funcs {
int (*gpu_state_put)(struct msm_gpu_state *state);
unsigned long (*gpu_get_freq)(struct msm_gpu *gpu);
void (*gpu_set_freq)(struct msm_gpu *gpu, unsigned long freq);
+ struct msm_gem_address_space *(*create_address_space)
+ (struct msm_gpu *gpu, struct platform_device *pdev);
};
struct msm_gpu {
diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
index 544c519..e773ef8 100644
--- a/drivers/gpu/drm/msm/msm_iommu.c
+++ b/drivers/gpu/drm/msm/msm_iommu.c
@@ -70,6 +70,9 @@ struct msm_mmu *msm_iommu_new(struct device *dev, struct iommu_domain *domain)
struct msm_iommu *iommu;
int ret;
+ if (!domain)
+ return ERR_PTR(-ENODEV);
+
iommu = kzalloc(sizeof(*iommu), GFP_KERNEL);
if (!iommu)
return ERR_PTR(-ENOMEM);
--
2.7.4
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Freedreno] [PATCH v3 4/5] drm/msm: Refactor address space initialization
2019-12-16 16:37 ` [PATCH v3 4/5] drm/msm: Refactor address space initialization Jordan Crouse
@ 2020-01-06 21:59 ` Jordan Crouse
0 siblings, 0 replies; 17+ messages in thread
From: Jordan Crouse @ 2020-01-06 21:59 UTC (permalink / raw)
To: iommu
Cc: Jeffrey Hugo, David Airlie, dri-devel, linux-kernel,
AngeloGioacchino Del Regno, Sam Ravnborg, Thomas Gleixner, will,
Wen Yang, Ben Dooks, linux-arm-kernel, Brian Masney, freedreno,
Fritz Koenig, linux-arm-msm, Alexios Zavras, Jeykumar Sankaran,
Sean Paul, Allison Randal, Greg Kroah-Hartman, Douglas Anderson,
zhengbin, Rob Clark, Daniel Vetter, Drew Davenport, robin.murphy,
Georgi Djakov
On Mon, Dec 16, 2019 at 09:37:50AM -0700, Jordan Crouse wrote:
> Refactor how address space initialization works. Instead of having the
> address space function create the MMU object (and thus require separate but
> equal functions for gpummu and iommu) use a single function and pass the
> MMU struct. Make the generic code cleaner by using target specific
> functions to create the address space so a2xx can do its own thing in its
> own space. For all the other targets use a generic helper to initialize
> IOMMU but leave the door open for newer targets to use customization
> if they need it.
>
> Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> ---
>
> drivers/gpu/drm/msm/adreno/a2xx_gpu.c | 16 ++++++++++
> drivers/gpu/drm/msm/adreno/a3xx_gpu.c | 1 +
> drivers/gpu/drm/msm/adreno/a4xx_gpu.c | 1 +
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 1 +
> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 1 +
> drivers/gpu/drm/msm/adreno/adreno_gpu.c | 23 ++++++++++----
> drivers/gpu/drm/msm/adreno/adreno_gpu.h | 8 +++++
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 10 +++---
> drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 14 +++++----
> drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c | 4 ---
> drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 11 +++++--
> drivers/gpu/drm/msm/msm_drv.h | 8 ++---
> drivers/gpu/drm/msm/msm_gem_vma.c | 52 +++++---------------------------
> drivers/gpu/drm/msm/msm_gpu.c | 40 ++----------------------
> drivers/gpu/drm/msm/msm_gpu.h | 4 +--
> drivers/gpu/drm/msm/msm_iommu.c | 3 ++
> 16 files changed, 83 insertions(+), 114 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
> index 1f83bc1..60f6472 100644
> --- a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
> @@ -401,6 +401,21 @@ static struct msm_gpu_state *a2xx_gpu_state_get(struct msm_gpu *gpu)
> return state;
> }
>
> +static struct msm_gem_address_space *
> +a2xx_create_address_space(struct msm_gpu *gpu, struct platform_device *pdev)
> +{
> + struct msm_mmu *mmu = msm_gpummu_new(&pdev->dev, gpu);
> + struct msm_gem_address_space *aspace;
> +
> + aspace = msm_gem_address_space_create(mmu, "gpu", SZ_16M,
> + SZ_16M + 0xfff * SZ_64K);
> +
> + if (IS_ERR(aspace) && !IS_ERR(mmu))
> + mmu->funcs->destroy(mmu);
> +
> + return aspace;
> +}
> +
> /* Register offset defines for A2XX - copy of A3XX */
> static const unsigned int a2xx_register_offsets[REG_ADRENO_REGISTER_MAX] = {
> REG_ADRENO_DEFINE(REG_ADRENO_CP_RB_BASE, REG_AXXX_CP_RB_BASE),
> @@ -429,6 +444,7 @@ static const struct adreno_gpu_funcs funcs = {
> #endif
> .gpu_state_get = a2xx_gpu_state_get,
> .gpu_state_put = adreno_gpu_state_put,
> + .create_address_space = a2xx_create_address_space,
> },
> };
>
> diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> index 7ad1493..41e51e0 100644
> --- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> @@ -441,6 +441,7 @@ static const struct adreno_gpu_funcs funcs = {
> #endif
> .gpu_state_get = a3xx_gpu_state_get,
> .gpu_state_put = adreno_gpu_state_put,
> + .create_address_space = adreno_iommu_create_address_space,
> },
> };
>
> diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> index b01388a..3655440 100644
> --- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> @@ -532,6 +532,7 @@ static const struct adreno_gpu_funcs funcs = {
> #endif
> .gpu_state_get = a4xx_gpu_state_get,
> .gpu_state_put = adreno_gpu_state_put,
> + .create_address_space = adreno_iommu_create_address_space,
> },
> .get_timestamp = a4xx_get_timestamp,
> };
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index b02e204..0f5db72 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -1432,6 +1432,7 @@ static const struct adreno_gpu_funcs funcs = {
> .gpu_busy = a5xx_gpu_busy,
> .gpu_state_get = a5xx_gpu_state_get,
> .gpu_state_put = a5xx_gpu_state_put,
> + .create_address_space = adreno_iommu_create_address_space,
> },
> .get_timestamp = a5xx_get_timestamp,
> };
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index dc8ec2c..5dc0b2c 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -832,6 +832,7 @@ static const struct adreno_gpu_funcs funcs = {
> #if defined(CONFIG_DRM_MSM_GPU_STATE)
> .gpu_state_get = a6xx_gpu_state_get,
> .gpu_state_put = a6xx_gpu_state_put,
> + .create_address_space = adreno_iommu_create_address_space,
> #endif
> },
> .get_timestamp = a6xx_get_timestamp,
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 0783e4b..09c57891 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -157,6 +157,23 @@ int adreno_zap_shader_load(struct msm_gpu *gpu, u32 pasid)
> return zap_shader_load_mdt(gpu, adreno_gpu->info->zapfw, pasid);
> }
>
> +struct msm_gem_address_space *
> +adreno_iommu_create_address_space(struct msm_gpu *gpu,
> + struct platform_device *pdev)
> +{
> + struct iommu_domain *iommu = iommu_domain_alloc(&platform_bus_type);
> + struct msm_mmu *mmu = msm_iommu_new(&pdev->dev, iommu);
> + struct msm_gem_address_space *aspace;
> +
> + aspace = msm_gem_address_space_create(mmu, "gpu", SZ_16M,
> + 0xfffffff);
> +
> + if (IS_ERR(aspace) && !IS_ERR(mmu))
> + mmu->funcs->destroy(mmu);
> +
> + return aspace;
> +}
> +
> int adreno_get_param(struct msm_gpu *gpu, uint32_t param, uint64_t *value)
> {
> struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> @@ -949,12 +966,6 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>
> adreno_gpu_config.ioname = "kgsl_3d0_reg_memory";
>
> - adreno_gpu_config.va_start = SZ_16M;
> - adreno_gpu_config.va_end = 0xffffffff;
> - /* maximum range of a2xx mmu */
> - if (adreno_is_a2xx(adreno_gpu))
> - adreno_gpu_config.va_end = SZ_16M + 0xfff * SZ_64K;
> -
> adreno_gpu_config.nr_rings = nr_rings;
>
> adreno_get_pwrlevels(&pdev->dev, gpu);
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> index e71a757..5c1aa12 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> @@ -263,6 +263,14 @@ int adreno_gpu_state_get(struct msm_gpu *gpu, struct msm_gpu_state *state);
> int adreno_gpu_state_put(struct msm_gpu_state *state);
>
> /*
> + * Common helper function to initialize the default address space for arm-smmu
> + * attached targets
> + */
> +struct msm_gem_address_space *
> +adreno_iommu_create_address_space(struct msm_gpu *gpu,
> + struct platform_device *pdev);
> +
> +/*
> * For a5xx and a6xx targets load the zap shader that is used to pull the GPU
> * out of secure mode
> */
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index b082b23..4e6ebbd 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -704,18 +704,18 @@ static int _dpu_kms_mmu_init(struct dpu_kms *dpu_kms)
> {
> struct iommu_domain *domain;
> struct msm_gem_address_space *aspace;
> + struct msm_mmu *mmu;
>
> domain = iommu_domain_alloc(&platform_bus_type);
> if (!domain)
> return 0;
>
> - domain->geometry.aperture_start = 0x1000;
> - domain->geometry.aperture_end = 0xffffffff;
> + mmu = msm_iommu_new(dpu_kms->dev->dev, domain);
> + aspace = msm_gem_address_space_create(mmu, "dpu1",
> + 0x1000, 0xfffffff);
>
> - aspace = msm_gem_address_space_create(dpu_kms->dev->dev,
> - domain, "dpu1");
> if (IS_ERR(aspace)) {
> - iommu_domain_free(domain);
> + mmu->funcs->destroy(mmu);
> return PTR_ERR(aspace);
> }
>
> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> index 9dba37c..0889718 100644
> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> @@ -510,9 +510,15 @@ struct msm_kms *mdp4_kms_init(struct drm_device *dev)
> mdelay(16);
>
> if (config->iommu) {
> - aspace = msm_gem_address_space_create(&pdev->dev,
> - config->iommu, "mdp4");
> + struct msm_mmu *mmu = msm_iommu_new(&pdev->dev,
> + config->iommu);
> +
> + aspace = msm_gem_address_space_create(mmu,
> + "mdp4", 0x1000, 0xffffffff);
> +
> if (IS_ERR(aspace)) {
> + if (!IS_ERR(mmu))
> + mmu->funcs->destroy(mmu);
> ret = PTR_ERR(aspace);
> goto fail;
> }
> @@ -565,10 +571,6 @@ static struct mdp4_platform_config *mdp4_get_config(struct platform_device *dev)
> /* TODO: Chips that aren't apq8064 have a 200 Mhz max_clk */
> config.max_clk = 266667000;
> config.iommu = iommu_domain_alloc(&platform_bus_type);
> - if (config.iommu) {
> - config.iommu->geometry.aperture_start = 0x1000;
> - config.iommu->geometry.aperture_end = 0xffffffff;
> - }
>
> return &config;
> }
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
> index 1f48f64..ebd651a 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
> @@ -941,10 +941,6 @@ static struct mdp5_cfg_platform *mdp5_get_config(struct platform_device *dev)
> static struct mdp5_cfg_platform config = {};
>
> config.iommu = iommu_domain_alloc(&platform_bus_type);
> - if (config.iommu) {
> - config.iommu->geometry.aperture_start = 0x1000;
> - config.iommu->geometry.aperture_end = 0xffffffff;
> - }
>
> return &config;
> }
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> index 653dab2..20bdff9 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> @@ -724,13 +724,20 @@ struct msm_kms *mdp5_kms_init(struct drm_device *dev)
> mdelay(16);
>
> if (config->platform.iommu) {
> + struct msm_mmu *mmu;
> +
> iommu_dev = &pdev->dev;
> if (!iommu_dev->iommu_fwspec)
> iommu_dev = iommu_dev->parent;
>
> - aspace = msm_gem_address_space_create(iommu_dev,
> - config->platform.iommu, "mdp5");
> + mmu = msm_iommu_new(iommu_dev, config->platform.iommu);
> +
> + aspace = msm_gem_address_space_create(mmu, "mdp5",
> + 0x1000, 0xffffffff);
> +
> if (IS_ERR(aspace)) {
> + if (!IS_ERR(mmu))
> + mmu->funcs->destroy(mmu);
> ret = PTR_ERR(aspace);
> goto fail;
> }
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 71547e7..2203729 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -247,12 +247,8 @@ void msm_gem_close_vma(struct msm_gem_address_space *aspace,
> void msm_gem_address_space_put(struct msm_gem_address_space *aspace);
>
> struct msm_gem_address_space *
> -msm_gem_address_space_create(struct device *dev, struct iommu_domain *domain,
> - const char *name);
> -
> -struct msm_gem_address_space *
> -msm_gem_address_space_create_a2xx(struct device *dev, struct msm_gpu *gpu,
> - const char *name, uint64_t va_start, uint64_t va_end);
> +msm_gem_address_space_create(struct msm_mmu *mmu, const char *name,
> + u64 va_start, u64 va_end);
>
> int msm_register_mmu(struct drm_device *dev, struct msm_mmu *mmu);
> void msm_unregister_mmu(struct drm_device *dev, struct msm_mmu *mmu);
> diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
> index 91d993a..075ce52 100644
> --- a/drivers/gpu/drm/msm/msm_gem_vma.c
> +++ b/drivers/gpu/drm/msm/msm_gem_vma.c
> @@ -125,63 +125,25 @@ int msm_gem_init_vma(struct msm_gem_address_space *aspace,
> return 0;
> }
>
> -
> struct msm_gem_address_space *
> -msm_gem_address_space_create(struct device *dev, struct iommu_domain *domain,
> - const char *name)
> -{
> - struct msm_gem_address_space *aspace;
> - u64 start = domain->geometry.aperture_start;
> - u64 size = domain->geometry.aperture_end - start;
> -
> - aspace = kzalloc(sizeof(*aspace), GFP_KERNEL);
> - if (!aspace)
> - return ERR_PTR(-ENOMEM);
> -
> - spin_lock_init(&aspace->lock);
> - aspace->name = name;
> - aspace->mmu = msm_iommu_new(dev, domain);
> - if (IS_ERR(aspace->mmu)) {
> - int ret = PTR_ERR(aspace->mmu);
> -
> - kfree(aspace);
> - return ERR_PTR(ret);
> - }
> -
> - /*
> - * Attaching the IOMMU device changes the aperture values so use the
> - * cached values instead
> - */
> - drm_mm_init(&aspace->mm, start >> PAGE_SHIFT, size >> PAGE_SHIFT);
> -
> - kref_init(&aspace->kref);
> -
> - return aspace;
> -}
> -
> -struct msm_gem_address_space *
> -msm_gem_address_space_create_a2xx(struct device *dev, struct msm_gpu *gpu,
> - const char *name, uint64_t va_start, uint64_t va_end)
> +msm_gem_address_space_create(struct msm_mmu *mmu, const char *name,
> + u64 va_start, u64 va_end)
The last parameter should be size.
> {
> struct msm_gem_address_space *aspace;
> u64 size = va_end - va_start;
And this line should go poof.
>
> + if (IS_ERR(mmu))
> + return ERR_CAST(mmu);
> +
> aspace = kzalloc(sizeof(*aspace), GFP_KERNEL);
> if (!aspace)
> return ERR_PTR(-ENOMEM);
>
> spin_lock_init(&aspace->lock);
> aspace->name = name;
> - aspace->mmu = msm_gpummu_new(dev, gpu);
> - if (IS_ERR(aspace->mmu)) {
> - int ret = PTR_ERR(aspace->mmu);
> -
> - kfree(aspace);
> - return ERR_PTR(ret);
> - }
> + aspace->mmu = mmu;
>
> - drm_mm_init(&aspace->mm, (va_start >> PAGE_SHIFT),
> - size >> PAGE_SHIFT);
> + drm_mm_init(&aspace->mm, va_start >> PAGE_SHIFT, size >> PAGE_SHIFT);
>
> kref_init(&aspace->kref);
Jordan
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 5/5] drm/msm/a6xx: Support split pagetables
2019-12-16 16:37 [PATCH v3 0/5] iommu/arm-smmu: Split pagetable support for arm-smmu-v2 Jordan Crouse
` (3 preceding siblings ...)
2019-12-16 16:37 ` [PATCH v3 4/5] drm/msm: Refactor address space initialization Jordan Crouse
@ 2019-12-16 16:37 ` Jordan Crouse
2019-12-16 17:43 ` Rob Clark
2019-12-24 2:57 ` smasetty
4 siblings, 2 replies; 17+ messages in thread
From: Jordan Crouse @ 2019-12-16 16:37 UTC (permalink / raw)
To: iommu
Cc: robin.murphy, will, linux-arm-kernel, linux-arm-msm, Sean Paul,
linux-kernel, dri-devel, Rob Clark, David Airlie, freedreno,
Daniel Vetter
Attempt to enable split pagetables if the arm-smmu driver supports it.
This will move the default address space from the default region to
the address range assigned to TTBR1. The behavior should be transparent
to the driver for now but it gets the default buffers out of the way
when we want to start swapping TTBR0 for context-specific pagetables.
Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
---
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 52 ++++++++++++++++++++++++++++++++++-
1 file changed, 51 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 5dc0b2c..1c6da93 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -811,6 +811,56 @@ static unsigned long a6xx_gpu_busy(struct msm_gpu *gpu)
return (unsigned long)busy_time;
}
+static struct msm_gem_address_space *
+a6xx_create_address_space(struct msm_gpu *gpu, struct platform_device *pdev)
+{
+ struct iommu_domain *iommu = iommu_domain_alloc(&platform_bus_type);
+ struct msm_gem_address_space *aspace;
+ struct msm_mmu *mmu;
+ u64 start, size;
+ u32 val = 1;
+ int ret;
+
+ if (!iommu)
+ return ERR_PTR(-ENOMEM);
+
+ /*
+ * Try to request split pagetables - the request has to be made before
+ * the domian is attached
+ */
+ iommu_domain_set_attr(iommu, DOMAIN_ATTR_SPLIT_TABLES, &val);
+
+ mmu = msm_iommu_new(&pdev->dev, iommu);
+ if (IS_ERR(mmu)) {
+ iommu_domain_free(iommu);
+ return ERR_CAST(mmu);
+ }
+
+ /*
+ * After the domain is attached, see if the split tables were actually
+ * successful.
+ */
+ ret = iommu_domain_get_attr(iommu, DOMAIN_ATTR_SPLIT_TABLES, &val);
+ if (!ret && val) {
+ /*
+ * The aperture start will be at the beginning of the TTBR1
+ * space so use that as a base
+ */
+ start = iommu->geometry.aperture_start;
+ size = 0xffffffff;
+ } else {
+ /* Otherwise use the legacy 32 bit region */
+ start = SZ_16M;
+ size = 0xffffffff - SZ_16M;
+ }
+
+ aspace = msm_gem_address_space_create(mmu, "gpu", start, size);
+ if (IS_ERR(aspace))
+ iommu_domain_free(iommu);
+
+ return aspace;
+}
+
static const struct adreno_gpu_funcs funcs = {
.base = {
.get_param = adreno_get_param,
@@ -832,7 +882,7 @@ static const struct adreno_gpu_funcs funcs = {
#if defined(CONFIG_DRM_MSM_GPU_STATE)
.gpu_state_get = a6xx_gpu_state_get,
.gpu_state_put = a6xx_gpu_state_put,
- .create_address_space = adreno_iommu_create_address_space,
+ .create_address_space = a6xx_create_address_space,
#endif
},
.get_timestamp = a6xx_get_timestamp,
--
2.7.4
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v3 5/5] drm/msm/a6xx: Support split pagetables
2019-12-16 16:37 ` [PATCH v3 5/5] drm/msm/a6xx: Support split pagetables Jordan Crouse
@ 2019-12-16 17:43 ` Rob Clark
2019-12-24 2:57 ` smasetty
1 sibling, 0 replies; 17+ messages in thread
From: Rob Clark @ 2019-12-16 17:43 UTC (permalink / raw)
To: Jordan Crouse
Cc: list@263.net:IOMMU DRIVERS
<iommu@lists.linux-foundation.org>,
Joerg Roedel <joro@8bytes.org>,,
Robin Murphy, Will Deacon,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-arm-msm, Sean Paul, Linux Kernel Mailing List, dri-devel,
David Airlie, freedreno, Daniel Vetter
On Mon, Dec 16, 2019 at 8:38 AM Jordan Crouse <jcrouse@codeaurora.org> wrote:
>
> Attempt to enable split pagetables if the arm-smmu driver supports it.
> This will move the default address space from the default region to
> the address range assigned to TTBR1. The behavior should be transparent
> to the driver for now but it gets the default buffers out of the way
> when we want to start swapping TTBR0 for context-specific pagetables.
>
> Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
Reviewed-by: Rob Clark <robdclark@gmail.com>
(my previous r-b's on the other patches from v2 carries over to v3)
> ---
>
> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 52 ++++++++++++++++++++++++++++++++++-
> 1 file changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 5dc0b2c..1c6da93 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -811,6 +811,56 @@ static unsigned long a6xx_gpu_busy(struct msm_gpu *gpu)
> return (unsigned long)busy_time;
> }
>
> +static struct msm_gem_address_space *
> +a6xx_create_address_space(struct msm_gpu *gpu, struct platform_device *pdev)
> +{
> + struct iommu_domain *iommu = iommu_domain_alloc(&platform_bus_type);
> + struct msm_gem_address_space *aspace;
> + struct msm_mmu *mmu;
> + u64 start, size;
> + u32 val = 1;
> + int ret;
> +
> + if (!iommu)
> + return ERR_PTR(-ENOMEM);
> +
> + /*
> + * Try to request split pagetables - the request has to be made before
> + * the domian is attached
> + */
> + iommu_domain_set_attr(iommu, DOMAIN_ATTR_SPLIT_TABLES, &val);
> +
> + mmu = msm_iommu_new(&pdev->dev, iommu);
> + if (IS_ERR(mmu)) {
> + iommu_domain_free(iommu);
> + return ERR_CAST(mmu);
> + }
> +
> + /*
> + * After the domain is attached, see if the split tables were actually
> + * successful.
> + */
> + ret = iommu_domain_get_attr(iommu, DOMAIN_ATTR_SPLIT_TABLES, &val);
> + if (!ret && val) {
> + /*
> + * The aperture start will be at the beginning of the TTBR1
> + * space so use that as a base
> + */
> + start = iommu->geometry.aperture_start;
> + size = 0xffffffff;
> + } else {
> + /* Otherwise use the legacy 32 bit region */
> + start = SZ_16M;
> + size = 0xffffffff - SZ_16M;
> + }
> +
> + aspace = msm_gem_address_space_create(mmu, "gpu", start, size);
> + if (IS_ERR(aspace))
> + iommu_domain_free(iommu);
> +
> + return aspace;
> +}
> +
> static const struct adreno_gpu_funcs funcs = {
> .base = {
> .get_param = adreno_get_param,
> @@ -832,7 +882,7 @@ static const struct adreno_gpu_funcs funcs = {
> #if defined(CONFIG_DRM_MSM_GPU_STATE)
> .gpu_state_get = a6xx_gpu_state_get,
> .gpu_state_put = a6xx_gpu_state_put,
> - .create_address_space = adreno_iommu_create_address_space,
> + .create_address_space = a6xx_create_address_space,
> #endif
> },
> .get_timestamp = a6xx_get_timestamp,
> --
> 2.7.4
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v3 5/5] drm/msm/a6xx: Support split pagetables
2019-12-16 16:37 ` [PATCH v3 5/5] drm/msm/a6xx: Support split pagetables Jordan Crouse
2019-12-16 17:43 ` Rob Clark
@ 2019-12-24 2:57 ` smasetty
2020-01-06 21:57 ` Jordan Crouse
1 sibling, 1 reply; 17+ messages in thread
From: smasetty @ 2019-12-24 2:57 UTC (permalink / raw)
To: Jordan Crouse
Cc: iommu, freedreno, David Airlie, will, robin.murphy, dri-devel,
linux-kernel, linux-arm-msm, Sean Paul, linux-arm-kernel
On 2019-12-16 22:07, Jordan Crouse wrote:
> Attempt to enable split pagetables if the arm-smmu driver supports it.
> This will move the default address space from the default region to
> the address range assigned to TTBR1. The behavior should be transparent
> to the driver for now but it gets the default buffers out of the way
> when we want to start swapping TTBR0 for context-specific pagetables.
>
> Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> ---
>
> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 52
> ++++++++++++++++++++++++++++++++++-
> 1 file changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 5dc0b2c..1c6da93 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -811,6 +811,56 @@ static unsigned long a6xx_gpu_busy(struct msm_gpu
> *gpu)
> return (unsigned long)busy_time;
> }
>
> +static struct msm_gem_address_space *
> +a6xx_create_address_space(struct msm_gpu *gpu, struct platform_device
> *pdev)
> +{
> + struct iommu_domain *iommu = iommu_domain_alloc(&platform_bus_type);
> + struct msm_gem_address_space *aspace;
> + struct msm_mmu *mmu;
> + u64 start, size;
> + u32 val = 1;
> + int ret;
> +
> + if (!iommu)
> + return ERR_PTR(-ENOMEM);
> +
> + /*
> + * Try to request split pagetables - the request has to be made
> before
> + * the domian is attached
> + */
> + iommu_domain_set_attr(iommu, DOMAIN_ATTR_SPLIT_TABLES, &val);
> +
> + mmu = msm_iommu_new(&pdev->dev, iommu);
> + if (IS_ERR(mmu)) {
> + iommu_domain_free(iommu);
> + return ERR_CAST(mmu);
> + }
> +
> + /*
> + * After the domain is attached, see if the split tables were
> actually
> + * successful.
> + */
> + ret = iommu_domain_get_attr(iommu, DOMAIN_ATTR_SPLIT_TABLES, &val);
> + if (!ret && val) {
> + /*
> + * The aperture start will be at the beginning of the TTBR1
> + * space so use that as a base
> + */
> + start = iommu->geometry.aperture_start;
> + size = 0xffffffff;
This should be the va_end and not the size
> + } else {
> + /* Otherwise use the legacy 32 bit region */
> + start = SZ_16M;
> + size = 0xffffffff - SZ_16M;
same as above
> + }
> +
> + aspace = msm_gem_address_space_create(mmu, "gpu", start, size);
> + if (IS_ERR(aspace))
> + iommu_domain_free(iommu);
> +
> + return aspace;
> +}
> +
> static const struct adreno_gpu_funcs funcs = {
> .base = {
> .get_param = adreno_get_param,
> @@ -832,7 +882,7 @@ static const struct adreno_gpu_funcs funcs = {
> #if defined(CONFIG_DRM_MSM_GPU_STATE)
> .gpu_state_get = a6xx_gpu_state_get,
> .gpu_state_put = a6xx_gpu_state_put,
> - .create_address_space = adreno_iommu_create_address_space,
> + .create_address_space = a6xx_create_address_space,
> #endif
> },
> .get_timestamp = a6xx_get_timestamp,
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v3 5/5] drm/msm/a6xx: Support split pagetables
2019-12-24 2:57 ` smasetty
@ 2020-01-06 21:57 ` Jordan Crouse
0 siblings, 0 replies; 17+ messages in thread
From: Jordan Crouse @ 2020-01-06 21:57 UTC (permalink / raw)
To: smasetty
Cc: iommu, freedreno, David Airlie, will, robin.murphy, dri-devel,
linux-kernel, linux-arm-msm, Sean Paul, linux-arm-kernel
On Tue, Dec 24, 2019 at 08:27:28AM +0530, smasetty@codeaurora.org wrote:
> On 2019-12-16 22:07, Jordan Crouse wrote:
> >Attempt to enable split pagetables if the arm-smmu driver supports it.
> >This will move the default address space from the default region to
> >the address range assigned to TTBR1. The behavior should be transparent
> >to the driver for now but it gets the default buffers out of the way
> >when we want to start swapping TTBR0 for context-specific pagetables.
> >
> >Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> >---
> >
> > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 52
> >++++++++++++++++++++++++++++++++++-
> > 1 file changed, 51 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> >b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> >index 5dc0b2c..1c6da93 100644
> >--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> >+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> >@@ -811,6 +811,56 @@ static unsigned long a6xx_gpu_busy(struct msm_gpu
> >*gpu)
> > return (unsigned long)busy_time;
> > }
> >
> >+static struct msm_gem_address_space *
> >+a6xx_create_address_space(struct msm_gpu *gpu, struct platform_device
> >*pdev)
> >+{
> >+ struct iommu_domain *iommu = iommu_domain_alloc(&platform_bus_type);
> >+ struct msm_gem_address_space *aspace;
> >+ struct msm_mmu *mmu;
> >+ u64 start, size;
> >+ u32 val = 1;
> >+ int ret;
> >+
> >+ if (!iommu)
> >+ return ERR_PTR(-ENOMEM);
> >+
> >+ /*
> >+ * Try to request split pagetables - the request has to be made before
> >+ * the domian is attached
> >+ */
> >+ iommu_domain_set_attr(iommu, DOMAIN_ATTR_SPLIT_TABLES, &val);
> >+
> >+ mmu = msm_iommu_new(&pdev->dev, iommu);
> >+ if (IS_ERR(mmu)) {
> >+ iommu_domain_free(iommu);
> >+ return ERR_CAST(mmu);
> >+ }
> >+
> >+ /*
> >+ * After the domain is attached, see if the split tables were actually
> >+ * successful.
> >+ */
> >+ ret = iommu_domain_get_attr(iommu, DOMAIN_ATTR_SPLIT_TABLES, &val);
> >+ if (!ret && val) {
> >+ /*
> >+ * The aperture start will be at the beginning of the TTBR1
> >+ * space so use that as a base
> >+ */
> >+ start = iommu->geometry.aperture_start;
> >+ size = 0xffffffff;
> This should be the va_end and not the size
This is a bug in msm_gem_address_space_create - I intended the parameter to be
the size.
Jordan
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 17+ messages in thread