diff mbox series

[v10,06/13] iommu/arm-smmu-qcom: Get and set the pagetable config for split pagetables

Message ID 20200720154047.3611092-7-jcrouse@codeaurora.org
State Superseded
Headers show
Series iommu/arm-smmu: Add Adreno SMMU specific implementation | expand

Commit Message

Jordan Crouse July 20, 2020, 3:40 p.m. UTC
The Adreno GPU has the capability to manage its own pagetables and switch
them dynamically from the hardware. To do this the GPU uses TTBR1 for
"global" GPU memory and creates local pagetables for each context and
switches them dynamically with the GPU.

Use DOMAIN_ATTR_PGTABLE_CFG to get the current configuration for the
TTBR1 pagetable from the smmu driver so the leaf driver can create
compatible pagetables for use with TTBR0.

Because TTBR0 is disabled by default when TTBR1 is enabled the GPU
driver can pass the configuration of one of the newly created pagetables
back through DOMAIN_ATTR_PGTABLE_CFG as a trigger to enable translation on
TTBR0.

Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
---

 drivers/iommu/arm-smmu-qcom.c | 47 +++++++++++++++++++++++++++++++++++
 drivers/iommu/arm-smmu.c      | 32 ++++++++++++++++++------
 drivers/iommu/arm-smmu.h      | 10 ++++++++
 3 files changed, 81 insertions(+), 8 deletions(-)

Comments

Rob Clark July 26, 2020, 5:03 p.m. UTC | #1
On Mon, Jul 20, 2020 at 8:41 AM Jordan Crouse <jcrouse@codeaurora.org> wrote:
>
> The Adreno GPU has the capability to manage its own pagetables and switch
> them dynamically from the hardware. To do this the GPU uses TTBR1 for
> "global" GPU memory and creates local pagetables for each context and
> switches them dynamically with the GPU.
>
> Use DOMAIN_ATTR_PGTABLE_CFG to get the current configuration for the
> TTBR1 pagetable from the smmu driver so the leaf driver can create
> compatible pagetables for use with TTBR0.
>
> Because TTBR0 is disabled by default when TTBR1 is enabled the GPU
> driver can pass the configuration of one of the newly created pagetables
> back through DOMAIN_ATTR_PGTABLE_CFG as a trigger to enable translation on
> TTBR0.
>
> Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> ---
>
>  drivers/iommu/arm-smmu-qcom.c | 47 +++++++++++++++++++++++++++++++++++
>  drivers/iommu/arm-smmu.c      | 32 ++++++++++++++++++------
>  drivers/iommu/arm-smmu.h      | 10 ++++++++
>  3 files changed, 81 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu-qcom.c b/drivers/iommu/arm-smmu-qcom.c
> index b9a5c5369e86..9a0c64ca9cb6 100644
> --- a/drivers/iommu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm-smmu-qcom.c
> @@ -34,6 +34,52 @@ static bool qcom_adreno_smmu_is_gpu_device(struct device *dev)
>         return false;
>  }
>
> +/*
> + * Local implementation to configure TTBR0 wil the specified pagetable config.
> + * The GPU driver will call this to enable TTBR0 when per-instance pagetables
> + * are active
> + */
> +static int qcom_adreno_smmu_set_pgtable_cfg(struct arm_smmu_domain *smmu_domain,
> +               struct io_pgtable_cfg *pgtbl_cfg)
> +{
> +       struct io_pgtable *pgtable = io_pgtable_ops_to_pgtable(smmu_domain->pgtbl_ops);
> +       struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
> +       struct arm_smmu_cb *cb = &smmu_domain->smmu->cbs[cfg->cbndx];
> +
> +       /* The domain must have split pagetables already enabled */
> +       if (cb->tcr[0] & ARM_SMMU_TCR_EPD1)
> +               return -EINVAL;
> +
> +       /* If the pagetable config is NULL, disable TTBR0 */
> +       if (!pgtbl_cfg) {
> +               /* Do nothing if it is already disabled */
> +               if ((cb->tcr[0] & ARM_SMMU_TCR_EPD0))
> +                       return -EINVAL;
> +
> +               /* Set TCR to the original configuration */
> +               cb->tcr[0] = arm_smmu_lpae_tcr(&pgtable->cfg);
> +               cb->ttbr[0] = FIELD_PREP(ARM_SMMU_TTBRn_ASID, cb->cfg->asid);
> +       } else {
> +               u32 tcr = cb->tcr[0];
> +
> +               /* FIXME: What sort of validation do we need to do here? */
> +
> +               /* Don't call this again if TTBR0 is already enabled */
> +               if (!(cb->tcr[0] & ARM_SMMU_TCR_EPD0))
> +                       return -EINVAL;
> +
> +               tcr |= arm_smmu_lpae_tcr(pgtbl_cfg);
> +               tcr &= ~(ARM_SMMU_TCR_EPD0 | ARM_SMMU_TCR_EPD1);
> +
> +               cb->tcr[0] = tcr;
> +               cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
> +               cb->ttbr[0] |= FIELD_PREP(ARM_SMMU_TTBRn_ASID, cb->cfg->asid);
> +       }
> +
> +       arm_smmu_write_context_bank(smmu_domain->smmu, cb->cfg->cbndx);
> +       return 0;
> +}
> +
>  static int qcom_adreno_smmu_alloc_context_bank(struct arm_smmu_domain *smmu_domain,
>                 struct device *dev, int start, int count)
>  {
> @@ -131,6 +177,7 @@ static const struct arm_smmu_impl qcom_adreno_smmu_impl = {
>         .def_domain_type = qcom_smmu_def_domain_type,
>         .reset = qcom_smmu500_reset,
>         .alloc_context_bank = qcom_adreno_smmu_alloc_context_bank,
> +       .set_pgtable_cfg = qcom_adreno_smmu_set_pgtable_cfg,
>  };
>
>  static struct arm_smmu_device *qcom_smmu_create(struct arm_smmu_device *smmu,
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index fff536a44faa..e1036ae54a8d 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -86,13 +86,6 @@ struct arm_smmu_smr {
>         bool                            valid;
>  };
>
> -struct arm_smmu_cb {
> -       u64                             ttbr[2];
> -       u32                             tcr[2];
> -       u32                             mair[2];
> -       struct arm_smmu_cfg             *cfg;
> -};
> -
>  static bool using_legacy_binding, using_generic_binding;
>
>  static inline int arm_smmu_rpm_get(struct arm_smmu_device *smmu)
> @@ -558,7 +551,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
>         }
>  }
>
> -static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
> +void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
>  {
>         u32 reg;
>         bool stage1;
> @@ -1515,6 +1508,18 @@ 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_PGTABLE_CFG: {
> +                       struct io_pgtable *pgtable;
> +                       struct io_pgtable_cfg *dest = data;
> +
> +                       if (!smmu_domain->pgtbl_ops)
> +                               return -ENODEV;
> +
> +                       pgtable = io_pgtable_ops_to_pgtable(smmu_domain->pgtbl_ops);
> +
> +                       memcpy(dest, &pgtable->cfg, sizeof(*dest));
> +                       return 0;
> +               }

hmm, maybe it would make sense to have impl hooks for get/set_attr, so
we could handle DOMAIN_ATTR_PGTABLE_CFG inside the adreno_smmu_impl?

Having impl specific domain attrs would be useful for what I have in
mind to enable stall/resume support, so we can hook in devcoredump to
iova faults (which would be a huge improvement for debugability, right
now iova faults are somewhat harder to debug than needed).  My rough
idea was to add DOMAIN_ATTR_RESUME, which could be used with
set_attr() to (1) enable STALL and let drm/msm know whether the iommu
supports it, and (2) resume translation from wq context after
devcoredump snapshot is collected.

BR,
-R

>                 default:
>                         return -ENODEV;
>                 }
> @@ -1555,6 +1560,17 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
>                         else
>                                 smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
>                         break;
> +               case DOMAIN_ATTR_PGTABLE_CFG: {
> +                       struct arm_smmu_device *smmu = smmu_domain->smmu;
> +
> +                       ret = -EPERM;
> +
> +                       if (smmu)
> +                               if (smmu->impl && smmu->impl->set_pgtable_cfg)
> +                                       ret = smmu->impl->set_pgtable_cfg(smmu_domain,
> +                                               data);
> +                       }
> +                       break;
>                 default:
>                         ret = -ENODEV;
>                 }
> diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
> index 9f81c1fffe1e..9325fc28d24a 100644
> --- a/drivers/iommu/arm-smmu.h
> +++ b/drivers/iommu/arm-smmu.h
> @@ -328,6 +328,13 @@ struct arm_smmu_cfg {
>  };
>  #define ARM_SMMU_INVALID_IRPTNDX       0xff
>
> +struct arm_smmu_cb {
> +       u64                             ttbr[2];
> +       u32                             tcr[2];
> +       u32                             mair[2];
> +       struct arm_smmu_cfg             *cfg;
> +};
> +
>  enum arm_smmu_domain_stage {
>         ARM_SMMU_DOMAIN_S1 = 0,
>         ARM_SMMU_DOMAIN_S2,
> @@ -408,6 +415,8 @@ struct arm_smmu_impl {
>         int (*def_domain_type)(struct device *dev);
>         int (*alloc_context_bank)(struct arm_smmu_domain *smmu_domain,
>                         struct device *dev, int start, int max);
> +       int (*set_pgtable_cfg)(struct arm_smmu_domain *smmu_domain,
> +                       struct io_pgtable_cfg *cfg);
>  };
>
>  #define INVALID_SMENDX                 -1
> @@ -493,6 +502,7 @@ struct arm_smmu_device *arm_smmu_impl_init(struct arm_smmu_device *smmu);
>  struct arm_smmu_device *qcom_smmu_impl_init(struct arm_smmu_device *smmu);
>  struct arm_smmu_device *qcom_adreno_smmu_impl_init(struct arm_smmu_device *smmu);
>
> +void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx);
>  int arm_mmu500_reset(struct arm_smmu_device *smmu);
>
>  #endif /* _ARM_SMMU_H */
> --
> 2.25.1
>
> _______________________________________________
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
Jordan Crouse July 27, 2020, 3:03 p.m. UTC | #2
On Sun, Jul 26, 2020 at 10:03:07AM -0700, Rob Clark wrote:
> On Mon, Jul 20, 2020 at 8:41 AM Jordan Crouse <jcrouse@codeaurora.org> wrote:
> >
> > The Adreno GPU has the capability to manage its own pagetables and switch
> > them dynamically from the hardware. To do this the GPU uses TTBR1 for
> > "global" GPU memory and creates local pagetables for each context and
> > switches them dynamically with the GPU.
> >
> > Use DOMAIN_ATTR_PGTABLE_CFG to get the current configuration for the
> > TTBR1 pagetable from the smmu driver so the leaf driver can create
> > compatible pagetables for use with TTBR0.
> >
> > Because TTBR0 is disabled by default when TTBR1 is enabled the GPU
> > driver can pass the configuration of one of the newly created pagetables
> > back through DOMAIN_ATTR_PGTABLE_CFG as a trigger to enable translation on
> > TTBR0.
> >
> > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> > ---
> >
> >  drivers/iommu/arm-smmu-qcom.c | 47 +++++++++++++++++++++++++++++++++++
> >  drivers/iommu/arm-smmu.c      | 32 ++++++++++++++++++------
> >  drivers/iommu/arm-smmu.h      | 10 ++++++++
> >  3 files changed, 81 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/iommu/arm-smmu-qcom.c b/drivers/iommu/arm-smmu-qcom.c
> > index b9a5c5369e86..9a0c64ca9cb6 100644
> > --- a/drivers/iommu/arm-smmu-qcom.c
> > +++ b/drivers/iommu/arm-smmu-qcom.c
> > @@ -34,6 +34,52 @@ static bool qcom_adreno_smmu_is_gpu_device(struct device *dev)
> >         return false;
> >  }
> >
> > +/*
> > + * Local implementation to configure TTBR0 wil the specified pagetable config.
> > + * The GPU driver will call this to enable TTBR0 when per-instance pagetables
> > + * are active
> > + */
> > +static int qcom_adreno_smmu_set_pgtable_cfg(struct arm_smmu_domain *smmu_domain,
> > +               struct io_pgtable_cfg *pgtbl_cfg)
> > +{
> > +       struct io_pgtable *pgtable = io_pgtable_ops_to_pgtable(smmu_domain->pgtbl_ops);
> > +       struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
> > +       struct arm_smmu_cb *cb = &smmu_domain->smmu->cbs[cfg->cbndx];
> > +
> > +       /* The domain must have split pagetables already enabled */
> > +       if (cb->tcr[0] & ARM_SMMU_TCR_EPD1)
> > +               return -EINVAL;
> > +
> > +       /* If the pagetable config is NULL, disable TTBR0 */
> > +       if (!pgtbl_cfg) {
> > +               /* Do nothing if it is already disabled */
> > +               if ((cb->tcr[0] & ARM_SMMU_TCR_EPD0))
> > +                       return -EINVAL;
> > +
> > +               /* Set TCR to the original configuration */
> > +               cb->tcr[0] = arm_smmu_lpae_tcr(&pgtable->cfg);
> > +               cb->ttbr[0] = FIELD_PREP(ARM_SMMU_TTBRn_ASID, cb->cfg->asid);
> > +       } else {
> > +               u32 tcr = cb->tcr[0];
> > +
> > +               /* FIXME: What sort of validation do we need to do here? */
> > +
> > +               /* Don't call this again if TTBR0 is already enabled */
> > +               if (!(cb->tcr[0] & ARM_SMMU_TCR_EPD0))
> > +                       return -EINVAL;
> > +
> > +               tcr |= arm_smmu_lpae_tcr(pgtbl_cfg);
> > +               tcr &= ~(ARM_SMMU_TCR_EPD0 | ARM_SMMU_TCR_EPD1);
> > +
> > +               cb->tcr[0] = tcr;
> > +               cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
> > +               cb->ttbr[0] |= FIELD_PREP(ARM_SMMU_TTBRn_ASID, cb->cfg->asid);
> > +       }
> > +
> > +       arm_smmu_write_context_bank(smmu_domain->smmu, cb->cfg->cbndx);
> > +       return 0;
> > +}
> > +
> >  static int qcom_adreno_smmu_alloc_context_bank(struct arm_smmu_domain *smmu_domain,
> >                 struct device *dev, int start, int count)
> >  {
> > @@ -131,6 +177,7 @@ static const struct arm_smmu_impl qcom_adreno_smmu_impl = {
> >         .def_domain_type = qcom_smmu_def_domain_type,
> >         .reset = qcom_smmu500_reset,
> >         .alloc_context_bank = qcom_adreno_smmu_alloc_context_bank,
> > +       .set_pgtable_cfg = qcom_adreno_smmu_set_pgtable_cfg,
> >  };
> >
> >  static struct arm_smmu_device *qcom_smmu_create(struct arm_smmu_device *smmu,
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index fff536a44faa..e1036ae54a8d 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -86,13 +86,6 @@ struct arm_smmu_smr {
> >         bool                            valid;
> >  };
> >
> > -struct arm_smmu_cb {
> > -       u64                             ttbr[2];
> > -       u32                             tcr[2];
> > -       u32                             mair[2];
> > -       struct arm_smmu_cfg             *cfg;
> > -};
> > -
> >  static bool using_legacy_binding, using_generic_binding;
> >
> >  static inline int arm_smmu_rpm_get(struct arm_smmu_device *smmu)
> > @@ -558,7 +551,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
> >         }
> >  }
> >
> > -static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
> > +void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
> >  {
> >         u32 reg;
> >         bool stage1;
> > @@ -1515,6 +1508,18 @@ 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_PGTABLE_CFG: {
> > +                       struct io_pgtable *pgtable;
> > +                       struct io_pgtable_cfg *dest = data;
> > +
> > +                       if (!smmu_domain->pgtbl_ops)
> > +                               return -ENODEV;
> > +
> > +                       pgtable = io_pgtable_ops_to_pgtable(smmu_domain->pgtbl_ops);
> > +
> > +                       memcpy(dest, &pgtable->cfg, sizeof(*dest));
> > +                       return 0;
> > +               }
> 
> hmm, maybe it would make sense to have impl hooks for get/set_attr, so
> we could handle DOMAIN_ATTR_PGTABLE_CFG inside the adreno_smmu_impl?
> 
> Having impl specific domain attrs would be useful for what I have in
> mind to enable stall/resume support, so we can hook in devcoredump to
> iova faults (which would be a huge improvement for debugability, right
> now iova faults are somewhat harder to debug than needed).  My rough
> idea was to add DOMAIN_ATTR_RESUME, which could be used with
> set_attr() to (1) enable STALL and let drm/msm know whether the iommu
> supports it, and (2) resume translation from wq context after
> devcoredump snapshot is collected.

Expanding on that, maybe a DOMAIN_ATTR_IMPL with struct { int subtype; void
*data } as the payload would let us add things without having to populate the
generic enum.  That would force us to export an arm-smmu header but at this
point it might be such a bad thing.

Jordan


> BR,
> -R
> 
> >                 default:
> >                         return -ENODEV;
> >                 }
> > @@ -1555,6 +1560,17 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
> >                         else
> >                                 smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
> >                         break;
> > +               case DOMAIN_ATTR_PGTABLE_CFG: {
> > +                       struct arm_smmu_device *smmu = smmu_domain->smmu;
> > +
> > +                       ret = -EPERM;
> > +
> > +                       if (smmu)
> > +                               if (smmu->impl && smmu->impl->set_pgtable_cfg)
> > +                                       ret = smmu->impl->set_pgtable_cfg(smmu_domain,
> > +                                               data);
> > +                       }
> > +                       break;
> >                 default:
> >                         ret = -ENODEV;
> >                 }
> > diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
> > index 9f81c1fffe1e..9325fc28d24a 100644
> > --- a/drivers/iommu/arm-smmu.h
> > +++ b/drivers/iommu/arm-smmu.h
> > @@ -328,6 +328,13 @@ struct arm_smmu_cfg {
> >  };
> >  #define ARM_SMMU_INVALID_IRPTNDX       0xff
> >
> > +struct arm_smmu_cb {
> > +       u64                             ttbr[2];
> > +       u32                             tcr[2];
> > +       u32                             mair[2];
> > +       struct arm_smmu_cfg             *cfg;
> > +};
> > +
> >  enum arm_smmu_domain_stage {
> >         ARM_SMMU_DOMAIN_S1 = 0,
> >         ARM_SMMU_DOMAIN_S2,
> > @@ -408,6 +415,8 @@ struct arm_smmu_impl {
> >         int (*def_domain_type)(struct device *dev);
> >         int (*alloc_context_bank)(struct arm_smmu_domain *smmu_domain,
> >                         struct device *dev, int start, int max);
> > +       int (*set_pgtable_cfg)(struct arm_smmu_domain *smmu_domain,
> > +                       struct io_pgtable_cfg *cfg);
> >  };
> >
> >  #define INVALID_SMENDX                 -1
> > @@ -493,6 +502,7 @@ struct arm_smmu_device *arm_smmu_impl_init(struct arm_smmu_device *smmu);
> >  struct arm_smmu_device *qcom_smmu_impl_init(struct arm_smmu_device *smmu);
> >  struct arm_smmu_device *qcom_adreno_smmu_impl_init(struct arm_smmu_device *smmu);
> >
> > +void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx);
> >  int arm_mmu500_reset(struct arm_smmu_device *smmu);
> >
> >  #endif /* _ARM_SMMU_H */
> > --
> > 2.25.1
> >
> > _______________________________________________
> > Freedreno mailing list
> > Freedreno@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/freedreno
Rob Clark July 27, 2020, 5:17 p.m. UTC | #3
On Mon, Jul 27, 2020 at 8:03 AM Jordan Crouse <jcrouse@codeaurora.org> wrote:
>
> On Sun, Jul 26, 2020 at 10:03:07AM -0700, Rob Clark wrote:
> > On Mon, Jul 20, 2020 at 8:41 AM Jordan Crouse <jcrouse@codeaurora.org> wrote:
> > >
> > > The Adreno GPU has the capability to manage its own pagetables and switch
> > > them dynamically from the hardware. To do this the GPU uses TTBR1 for
> > > "global" GPU memory and creates local pagetables for each context and
> > > switches them dynamically with the GPU.
> > >
> > > Use DOMAIN_ATTR_PGTABLE_CFG to get the current configuration for the
> > > TTBR1 pagetable from the smmu driver so the leaf driver can create
> > > compatible pagetables for use with TTBR0.
> > >
> > > Because TTBR0 is disabled by default when TTBR1 is enabled the GPU
> > > driver can pass the configuration of one of the newly created pagetables
> > > back through DOMAIN_ATTR_PGTABLE_CFG as a trigger to enable translation on
> > > TTBR0.
> > >
> > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> > > ---
> > >
> > >  drivers/iommu/arm-smmu-qcom.c | 47 +++++++++++++++++++++++++++++++++++
> > >  drivers/iommu/arm-smmu.c      | 32 ++++++++++++++++++------
> > >  drivers/iommu/arm-smmu.h      | 10 ++++++++
> > >  3 files changed, 81 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/iommu/arm-smmu-qcom.c b/drivers/iommu/arm-smmu-qcom.c
> > > index b9a5c5369e86..9a0c64ca9cb6 100644
> > > --- a/drivers/iommu/arm-smmu-qcom.c
> > > +++ b/drivers/iommu/arm-smmu-qcom.c
> > > @@ -34,6 +34,52 @@ static bool qcom_adreno_smmu_is_gpu_device(struct device *dev)
> > >         return false;
> > >  }
> > >
> > > +/*
> > > + * Local implementation to configure TTBR0 wil the specified pagetable config.
> > > + * The GPU driver will call this to enable TTBR0 when per-instance pagetables
> > > + * are active
> > > + */
> > > +static int qcom_adreno_smmu_set_pgtable_cfg(struct arm_smmu_domain *smmu_domain,
> > > +               struct io_pgtable_cfg *pgtbl_cfg)
> > > +{
> > > +       struct io_pgtable *pgtable = io_pgtable_ops_to_pgtable(smmu_domain->pgtbl_ops);
> > > +       struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
> > > +       struct arm_smmu_cb *cb = &smmu_domain->smmu->cbs[cfg->cbndx];
> > > +
> > > +       /* The domain must have split pagetables already enabled */
> > > +       if (cb->tcr[0] & ARM_SMMU_TCR_EPD1)
> > > +               return -EINVAL;
> > > +
> > > +       /* If the pagetable config is NULL, disable TTBR0 */
> > > +       if (!pgtbl_cfg) {
> > > +               /* Do nothing if it is already disabled */
> > > +               if ((cb->tcr[0] & ARM_SMMU_TCR_EPD0))
> > > +                       return -EINVAL;
> > > +
> > > +               /* Set TCR to the original configuration */
> > > +               cb->tcr[0] = arm_smmu_lpae_tcr(&pgtable->cfg);
> > > +               cb->ttbr[0] = FIELD_PREP(ARM_SMMU_TTBRn_ASID, cb->cfg->asid);
> > > +       } else {
> > > +               u32 tcr = cb->tcr[0];
> > > +
> > > +               /* FIXME: What sort of validation do we need to do here? */
> > > +
> > > +               /* Don't call this again if TTBR0 is already enabled */
> > > +               if (!(cb->tcr[0] & ARM_SMMU_TCR_EPD0))
> > > +                       return -EINVAL;
> > > +
> > > +               tcr |= arm_smmu_lpae_tcr(pgtbl_cfg);
> > > +               tcr &= ~(ARM_SMMU_TCR_EPD0 | ARM_SMMU_TCR_EPD1);
> > > +
> > > +               cb->tcr[0] = tcr;
> > > +               cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
> > > +               cb->ttbr[0] |= FIELD_PREP(ARM_SMMU_TTBRn_ASID, cb->cfg->asid);
> > > +       }
> > > +
> > > +       arm_smmu_write_context_bank(smmu_domain->smmu, cb->cfg->cbndx);
> > > +       return 0;
> > > +}
> > > +
> > >  static int qcom_adreno_smmu_alloc_context_bank(struct arm_smmu_domain *smmu_domain,
> > >                 struct device *dev, int start, int count)
> > >  {
> > > @@ -131,6 +177,7 @@ static const struct arm_smmu_impl qcom_adreno_smmu_impl = {
> > >         .def_domain_type = qcom_smmu_def_domain_type,
> > >         .reset = qcom_smmu500_reset,
> > >         .alloc_context_bank = qcom_adreno_smmu_alloc_context_bank,
> > > +       .set_pgtable_cfg = qcom_adreno_smmu_set_pgtable_cfg,
> > >  };
> > >
> > >  static struct arm_smmu_device *qcom_smmu_create(struct arm_smmu_device *smmu,
> > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > > index fff536a44faa..e1036ae54a8d 100644
> > > --- a/drivers/iommu/arm-smmu.c
> > > +++ b/drivers/iommu/arm-smmu.c
> > > @@ -86,13 +86,6 @@ struct arm_smmu_smr {
> > >         bool                            valid;
> > >  };
> > >
> > > -struct arm_smmu_cb {
> > > -       u64                             ttbr[2];
> > > -       u32                             tcr[2];
> > > -       u32                             mair[2];
> > > -       struct arm_smmu_cfg             *cfg;
> > > -};
> > > -
> > >  static bool using_legacy_binding, using_generic_binding;
> > >
> > >  static inline int arm_smmu_rpm_get(struct arm_smmu_device *smmu)
> > > @@ -558,7 +551,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
> > >         }
> > >  }
> > >
> > > -static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
> > > +void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
> > >  {
> > >         u32 reg;
> > >         bool stage1;
> > > @@ -1515,6 +1508,18 @@ 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_PGTABLE_CFG: {
> > > +                       struct io_pgtable *pgtable;
> > > +                       struct io_pgtable_cfg *dest = data;
> > > +
> > > +                       if (!smmu_domain->pgtbl_ops)
> > > +                               return -ENODEV;
> > > +
> > > +                       pgtable = io_pgtable_ops_to_pgtable(smmu_domain->pgtbl_ops);
> > > +
> > > +                       memcpy(dest, &pgtable->cfg, sizeof(*dest));
> > > +                       return 0;
> > > +               }
> >
> > hmm, maybe it would make sense to have impl hooks for get/set_attr, so
> > we could handle DOMAIN_ATTR_PGTABLE_CFG inside the adreno_smmu_impl?
> >
> > Having impl specific domain attrs would be useful for what I have in
> > mind to enable stall/resume support, so we can hook in devcoredump to
> > iova faults (which would be a huge improvement for debugability, right
> > now iova faults are somewhat harder to debug than needed).  My rough
> > idea was to add DOMAIN_ATTR_RESUME, which could be used with
> > set_attr() to (1) enable STALL and let drm/msm know whether the iommu
> > supports it, and (2) resume translation from wq context after
> > devcoredump snapshot is collected.
>
> Expanding on that, maybe a DOMAIN_ATTR_IMPL with struct { int subtype; void
> *data } as the payload would let us add things without having to populate the
> generic enum.  That would force us to export an arm-smmu header but at this
> point it might be such a bad thing.

That feels a bit like overkill to me, I don't expect there to be that
many custom things.  But I'll defer to the iommu folks as to which
they prefer.

BR,
-R

> Jordan
>
>
> > BR,
> > -R
> >
> > >                 default:
> > >                         return -ENODEV;
> > >                 }
> > > @@ -1555,6 +1560,17 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
> > >                         else
> > >                                 smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
> > >                         break;
> > > +               case DOMAIN_ATTR_PGTABLE_CFG: {
> > > +                       struct arm_smmu_device *smmu = smmu_domain->smmu;
> > > +
> > > +                       ret = -EPERM;
> > > +
> > > +                       if (smmu)
> > > +                               if (smmu->impl && smmu->impl->set_pgtable_cfg)
> > > +                                       ret = smmu->impl->set_pgtable_cfg(smmu_domain,
> > > +                                               data);
> > > +                       }
> > > +                       break;
> > >                 default:
> > >                         ret = -ENODEV;
> > >                 }
> > > diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
> > > index 9f81c1fffe1e..9325fc28d24a 100644
> > > --- a/drivers/iommu/arm-smmu.h
> > > +++ b/drivers/iommu/arm-smmu.h
> > > @@ -328,6 +328,13 @@ struct arm_smmu_cfg {
> > >  };
> > >  #define ARM_SMMU_INVALID_IRPTNDX       0xff
> > >
> > > +struct arm_smmu_cb {
> > > +       u64                             ttbr[2];
> > > +       u32                             tcr[2];
> > > +       u32                             mair[2];
> > > +       struct arm_smmu_cfg             *cfg;
> > > +};
> > > +
> > >  enum arm_smmu_domain_stage {
> > >         ARM_SMMU_DOMAIN_S1 = 0,
> > >         ARM_SMMU_DOMAIN_S2,
> > > @@ -408,6 +415,8 @@ struct arm_smmu_impl {
> > >         int (*def_domain_type)(struct device *dev);
> > >         int (*alloc_context_bank)(struct arm_smmu_domain *smmu_domain,
> > >                         struct device *dev, int start, int max);
> > > +       int (*set_pgtable_cfg)(struct arm_smmu_domain *smmu_domain,
> > > +                       struct io_pgtable_cfg *cfg);
> > >  };
> > >
> > >  #define INVALID_SMENDX                 -1
> > > @@ -493,6 +502,7 @@ struct arm_smmu_device *arm_smmu_impl_init(struct arm_smmu_device *smmu);
> > >  struct arm_smmu_device *qcom_smmu_impl_init(struct arm_smmu_device *smmu);
> > >  struct arm_smmu_device *qcom_adreno_smmu_impl_init(struct arm_smmu_device *smmu);
> > >
> > > +void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx);
> > >  int arm_mmu500_reset(struct arm_smmu_device *smmu);
> > >
> > >  #endif /* _ARM_SMMU_H */
> > > --
> > > 2.25.1
> > >
> > > _______________________________________________
> > > Freedreno mailing list
> > > Freedreno@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/freedreno
>
> --
> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
diff mbox series

Patch

diff --git a/drivers/iommu/arm-smmu-qcom.c b/drivers/iommu/arm-smmu-qcom.c
index b9a5c5369e86..9a0c64ca9cb6 100644
--- a/drivers/iommu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm-smmu-qcom.c
@@ -34,6 +34,52 @@  static bool qcom_adreno_smmu_is_gpu_device(struct device *dev)
 	return false;
 }
 
+/*
+ * Local implementation to configure TTBR0 wil the specified pagetable config.
+ * The GPU driver will call this to enable TTBR0 when per-instance pagetables
+ * are active
+ */
+static int qcom_adreno_smmu_set_pgtable_cfg(struct arm_smmu_domain *smmu_domain,
+		struct io_pgtable_cfg *pgtbl_cfg)
+{
+	struct io_pgtable *pgtable = io_pgtable_ops_to_pgtable(smmu_domain->pgtbl_ops);
+	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
+	struct arm_smmu_cb *cb = &smmu_domain->smmu->cbs[cfg->cbndx];
+
+	/* The domain must have split pagetables already enabled */
+	if (cb->tcr[0] & ARM_SMMU_TCR_EPD1)
+		return -EINVAL;
+
+	/* If the pagetable config is NULL, disable TTBR0 */
+	if (!pgtbl_cfg) {
+		/* Do nothing if it is already disabled */
+		if ((cb->tcr[0] & ARM_SMMU_TCR_EPD0))
+			return -EINVAL;
+
+		/* Set TCR to the original configuration */
+		cb->tcr[0] = arm_smmu_lpae_tcr(&pgtable->cfg);
+		cb->ttbr[0] = FIELD_PREP(ARM_SMMU_TTBRn_ASID, cb->cfg->asid);
+	} else {
+		u32 tcr = cb->tcr[0];
+
+		/* FIXME: What sort of validation do we need to do here? */
+
+		/* Don't call this again if TTBR0 is already enabled */
+		if (!(cb->tcr[0] & ARM_SMMU_TCR_EPD0))
+			return -EINVAL;
+
+		tcr |= arm_smmu_lpae_tcr(pgtbl_cfg);
+		tcr &= ~(ARM_SMMU_TCR_EPD0 | ARM_SMMU_TCR_EPD1);
+
+		cb->tcr[0] = tcr;
+		cb->ttbr[0] = pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
+		cb->ttbr[0] |= FIELD_PREP(ARM_SMMU_TTBRn_ASID, cb->cfg->asid);
+	}
+
+	arm_smmu_write_context_bank(smmu_domain->smmu, cb->cfg->cbndx);
+	return 0;
+}
+
 static int qcom_adreno_smmu_alloc_context_bank(struct arm_smmu_domain *smmu_domain,
 		struct device *dev, int start, int count)
 {
@@ -131,6 +177,7 @@  static const struct arm_smmu_impl qcom_adreno_smmu_impl = {
 	.def_domain_type = qcom_smmu_def_domain_type,
 	.reset = qcom_smmu500_reset,
 	.alloc_context_bank = qcom_adreno_smmu_alloc_context_bank,
+	.set_pgtable_cfg = qcom_adreno_smmu_set_pgtable_cfg,
 };
 
 static struct arm_smmu_device *qcom_smmu_create(struct arm_smmu_device *smmu,
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index fff536a44faa..e1036ae54a8d 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -86,13 +86,6 @@  struct arm_smmu_smr {
 	bool				valid;
 };
 
-struct arm_smmu_cb {
-	u64				ttbr[2];
-	u32				tcr[2];
-	u32				mair[2];
-	struct arm_smmu_cfg		*cfg;
-};
-
 static bool using_legacy_binding, using_generic_binding;
 
 static inline int arm_smmu_rpm_get(struct arm_smmu_device *smmu)
@@ -558,7 +551,7 @@  static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
 	}
 }
 
-static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
+void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
 {
 	u32 reg;
 	bool stage1;
@@ -1515,6 +1508,18 @@  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_PGTABLE_CFG: {
+			struct io_pgtable *pgtable;
+			struct io_pgtable_cfg *dest = data;
+
+			if (!smmu_domain->pgtbl_ops)
+				return -ENODEV;
+
+			pgtable = io_pgtable_ops_to_pgtable(smmu_domain->pgtbl_ops);
+
+			memcpy(dest, &pgtable->cfg, sizeof(*dest));
+			return 0;
+		}
 		default:
 			return -ENODEV;
 		}
@@ -1555,6 +1560,17 @@  static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
 			else
 				smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
 			break;
+		case DOMAIN_ATTR_PGTABLE_CFG: {
+			struct arm_smmu_device *smmu = smmu_domain->smmu;
+
+			ret = -EPERM;
+
+			if (smmu)
+				if (smmu->impl && smmu->impl->set_pgtable_cfg)
+					ret = smmu->impl->set_pgtable_cfg(smmu_domain,
+						data);
+			}
+			break;
 		default:
 			ret = -ENODEV;
 		}
diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
index 9f81c1fffe1e..9325fc28d24a 100644
--- a/drivers/iommu/arm-smmu.h
+++ b/drivers/iommu/arm-smmu.h
@@ -328,6 +328,13 @@  struct arm_smmu_cfg {
 };
 #define ARM_SMMU_INVALID_IRPTNDX	0xff
 
+struct arm_smmu_cb {
+	u64				ttbr[2];
+	u32				tcr[2];
+	u32				mair[2];
+	struct arm_smmu_cfg		*cfg;
+};
+
 enum arm_smmu_domain_stage {
 	ARM_SMMU_DOMAIN_S1 = 0,
 	ARM_SMMU_DOMAIN_S2,
@@ -408,6 +415,8 @@  struct arm_smmu_impl {
 	int (*def_domain_type)(struct device *dev);
 	int (*alloc_context_bank)(struct arm_smmu_domain *smmu_domain,
 			struct device *dev, int start, int max);
+	int (*set_pgtable_cfg)(struct arm_smmu_domain *smmu_domain,
+			struct io_pgtable_cfg *cfg);
 };
 
 #define INVALID_SMENDX			-1
@@ -493,6 +502,7 @@  struct arm_smmu_device *arm_smmu_impl_init(struct arm_smmu_device *smmu);
 struct arm_smmu_device *qcom_smmu_impl_init(struct arm_smmu_device *smmu);
 struct arm_smmu_device *qcom_adreno_smmu_impl_init(struct arm_smmu_device *smmu);
 
+void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx);
 int arm_mmu500_reset(struct arm_smmu_device *smmu);
 
 #endif /* _ARM_SMMU_H */