Re: [PATCH v2 9/9] iommu/dart: Remove the force_bypass variable
Hej,
On Thu, Sep 28, 2023, at 01:47, Jason Gunthorpe wrote:
> This flag just caches if the IO page size is larger than the CPU
> PAGE_SIZE. This only needs to be checked in two places so remove the
> confusingly named cache.
>
> dart would like to not support paging domains at all if the IO page size
> is larger than the CPU page size. In this case we should ideally fail
> domain_alloc_paging(), as there is no point in creating a domain that can
> never be attached. Move the test into apple_dart_finalize_domain().
>
> The check in apple_dart_mod_streams() will prevent the domain from being
> attached to the wrong dart
>
> There is no HW limitation that prevents BLOCKED domains from working,
> remove that test.
>
> The check in apple_dart_of_xlate() is redundant since immediately after
> the pgsize is checked. Remove it.
>
> Remove the variable.
>
> Suggested-by: Janne Grunau
> Signed-off-by: Jason Gunthorpe
> ---
> drivers/iommu/apple-dart.c | 20 ++--
> 1 file changed, 6 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
> index 126da0d89f0dd4..821b4a3465dfb9 100644
> --- a/drivers/iommu/apple-dart.c
> +++ b/drivers/iommu/apple-dart.c
> @@ -196,7 +196,6 @@ struct apple_dart_hw {
> * @lock: lock for hardware operations involving this dart
> * @pgsize: pagesize supported by this DART
> * @supports_bypass: indicates if this DART supports bypass mode
> - * @force_bypass: force bypass mode due to pagesize mismatch?
> * @sid2group: maps stream ids to iommu_groups
> * @iommu: iommu core device
> */
> @@ -217,7 +216,6 @@ struct apple_dart {
> u32 pgsize;
> u32 num_streams;
> u32 supports_bypass : 1;
> - u32 force_bypass : 1;
>
> struct iommu_group *sid2group[DART_MAX_STREAMS];
> struct iommu_device iommu;
> @@ -576,6 +574,9 @@ static int apple_dart_finalize_domain(struct
> apple_dart_domain *dart_domain,
> int ret = 0;
> int i, j;
>
> + if (dart->pgsize > PAGE_SIZE)
> + return -EINVAL;
> +
> mutex_lock(_domain->init_lock);
>
> if (dart_domain->finalized)
> @@ -659,9 +660,6 @@ static int apple_dart_attach_dev_paging(struct
> iommu_domain *domain,
> struct apple_dart_master_cfg *cfg = dev_iommu_priv_get(dev);
> struct apple_dart_domain *dart_domain = to_dart_domain(domain);
>
> - if (cfg->stream_maps[0].dart->force_bypass)
> - return -EINVAL;
> -
> ret = apple_dart_finalize_domain(dart_domain, cfg);
> if (ret)
> return ret;
> @@ -706,9 +704,6 @@ static int apple_dart_attach_dev_blocked(struct
> iommu_domain *domain,
> struct apple_dart_stream_map *stream_map;
> int i;
>
> - if (cfg->stream_maps[0].dart->force_bypass)
> - return -EINVAL;
> -
> for_each_stream_map(i, cfg, stream_map)
> apple_dart_hw_disable_dma(stream_map);
> return 0;
> @@ -803,8 +798,6 @@ static int apple_dart_of_xlate(struct device *dev,
> struct of_phandle_args *args)
> if (cfg_dart) {
> if (cfg_dart->supports_bypass != dart->supports_bypass)
> return -EINVAL;
> - if (cfg_dart->force_bypass != dart->force_bypass)
> - return -EINVAL;
> if (cfg_dart->pgsize != dart->pgsize)
> return -EINVAL;
> }
> @@ -946,7 +939,7 @@ static int apple_dart_def_domain_type(struct device
> *dev)
> {
> struct apple_dart_master_cfg *cfg = dev_iommu_priv_get(dev);
>
> - if (cfg->stream_maps[0].dart->force_bypass)
> + if (cfg->stream_maps[0].dart->pgsize > PAGE_SIZE)
> return IOMMU_DOMAIN_IDENTITY;
> if (!cfg->stream_maps[0].dart->supports_bypass)
> return IOMMU_DOMAIN_DMA;
> @@ -1146,8 +1139,6 @@ static int apple_dart_probe(struct platform_device
> *pdev)
> goto err_clk_disable;
> }
>
> - dart->force_bypass = dart->pgsize > PAGE_SIZE;
> -
> ret = apple_dart_hw_reset(dart);
> if (ret)
> goto err_clk_disable;
> @@ -1171,7 +1162,8 @@ static int apple_dart_probe(struct
> platform_device *pdev)
> dev_info(
> >dev,
> "DART [pagesize %x, %d streams, bypass support: %d, bypass
> forced:
> %d] initialized\n",
> - dart->pgsize, dart->num_streams, dart->supports_bypass,
> dart->force_bypass);
> + dart->pgsize, dart->num_streams, dart->supports_bypass,
> + dart->pgsize > PAGE_SIZE);
> return 0;
>
> err_sysfs_remove:
> --
Reviewed-by: Janne Grunau
thanks,
Janne
Re: [PATCH 8/8] iommu/dart: Call apple_dart_finalize_domain() as part of alloc_paging()
On Fri, Sep 22, 2023 at 02:07:59PM -0300, Jason Gunthorpe wrote:
> In many cases the dev argument will now be !NULL so we should use it to
> finalize the domain at allocation.
>
> Make apple_dart_finalize_domain() accept the correct type.
>
> Signed-off-by: Jason Gunthorpe
> ---
> drivers/iommu/apple-dart.c | 26 ++
> 1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
> index 62efe0aa056f60..2c1832e357c7c6 100644
> --- a/drivers/iommu/apple-dart.c
> +++ b/drivers/iommu/apple-dart.c
> @@ -568,10 +568,9 @@ apple_dart_setup_translation(struct apple_dart_domain
> *domain,
> stream_map->dart->hw->invalidate_tlb(stream_map);
> }
>
> -static int apple_dart_finalize_domain(struct iommu_domain *domain,
> +static int apple_dart_finalize_domain(struct apple_dart_domain *dart_domain,
> struct apple_dart_master_cfg *cfg)
> {
> - struct apple_dart_domain *dart_domain = to_dart_domain(domain);
> struct apple_dart *dart = cfg->stream_maps[0].dart;
> struct io_pgtable_cfg pgtbl_cfg;
> int ret = 0;
> @@ -597,16 +596,17 @@ static int apple_dart_finalize_domain(struct
> iommu_domain *domain,
> .iommu_dev = dart->dev,
> };
>
> - dart_domain->pgtbl_ops =
> - alloc_io_pgtable_ops(dart->hw->fmt, _cfg, domain);
> + dart_domain->pgtbl_ops = alloc_io_pgtable_ops(dart->hw->fmt, _cfg,
> + _domain->domain);
> if (!dart_domain->pgtbl_ops) {
> ret = -ENOMEM;
> goto done;
> }
>
> - domain->pgsize_bitmap = pgtbl_cfg.pgsize_bitmap;
> - domain->geometry.aperture_start = 0;
> - domain->geometry.aperture_end = (dma_addr_t)DMA_BIT_MASK(dart->ias);
> + dart_domain->domain.pgsize_bitmap = pgtbl_cfg.pgsize_bitmap;
> + dart_domain->domain.geometry.aperture_start = 0;
> + dart_domain->domain.geometry.aperture_end =
> + (dma_addr_t)DMA_BIT_MASK(dart->ias);
>
> dart_domain->finalized = true;
>
> @@ -661,7 +661,7 @@ static int apple_dart_attach_dev_paging(struct
> iommu_domain *domain,
> if (cfg->stream_maps[0].dart->force_bypass)
> return -EINVAL;
>
> - ret = apple_dart_finalize_domain(domain, cfg);
> + ret = apple_dart_finalize_domain(dart_domain, cfg);
> if (ret)
> return ret;
>
> @@ -757,6 +757,16 @@ static struct iommu_domain
> *apple_dart_domain_alloc_paging(struct device *dev)
>
> mutex_init(_domain->init_lock);
>
> + if (dev) {
> + struct apple_dart_master_cfg *cfg = dev_iommu_priv_get(dev);
> + int ret;
> +
> + ret = apple_dart_finalize_domain(dart_domain, cfg);
> + if (ret) {
> + kfree(dart_domain);
> + return ERR_PTR(ret);
> + }
> + }
> return _domain->domain;
> }
>
> --
> 2.42.0
Reviewed-by: Janne Grunau
best regards
Janne
Re: [PATCH 7/8] iommu/dart: Convert to domain_alloc_paging()
On Fri, Sep 22, 2023 at 02:07:58PM -0300, Jason Gunthorpe wrote:
> Since the IDENTITY and BLOCKED behaviors were moved to global statics all
> that remains is the paging domain. Rename to
> apple_dart_attach_dev_paging() and remove the left over cruft.
>
> Signed-off-by: Jason Gunthorpe
> ---
> drivers/iommu/apple-dart.c | 33 +++--
> 1 file changed, 11 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
> index 376f4c5461e8f7..62efe0aa056f60 100644
> --- a/drivers/iommu/apple-dart.c
> +++ b/drivers/iommu/apple-dart.c
> @@ -650,8 +650,8 @@ static int apple_dart_domain_add_streams(struct
> apple_dart_domain *domain,
> true);
> }
>
> -static int apple_dart_attach_dev(struct iommu_domain *domain,
> - struct device *dev)
> +static int apple_dart_attach_dev_paging(struct iommu_domain *domain,
> + struct device *dev)
> {
> int ret, i;
> struct apple_dart_stream_map *stream_map;
> @@ -665,21 +665,13 @@ static int apple_dart_attach_dev(struct iommu_domain
> *domain,
> if (ret)
> return ret;
>
> - switch (domain->type) {
> - case IOMMU_DOMAIN_DMA:
> - case IOMMU_DOMAIN_UNMANAGED:
> - ret = apple_dart_domain_add_streams(dart_domain, cfg);
> - if (ret)
> - return ret;
> + ret = apple_dart_domain_add_streams(dart_domain, cfg);
> + if (ret)
> + return ret;
>
> - for_each_stream_map(i, cfg, stream_map)
> - apple_dart_setup_translation(dart_domain, stream_map);
> - break;
> - default:
> - return -EINVAL;
> - }
> -
> - return ret;
> + for_each_stream_map(i, cfg, stream_map)
> + apple_dart_setup_translation(dart_domain, stream_map);
> + return 0;
> }
>
> static int apple_dart_attach_dev_identity(struct iommu_domain *domain,
> @@ -755,13 +747,10 @@ static void apple_dart_release_device(struct device
> *dev)
> kfree(cfg);
> }
>
> -static struct iommu_domain *apple_dart_domain_alloc(unsigned int type)
> +static struct iommu_domain *apple_dart_domain_alloc_paging(struct device
> *dev)
> {
> struct apple_dart_domain *dart_domain;
>
> - if (type != IOMMU_DOMAIN_DMA && type != IOMMU_DOMAIN_UNMANAGED)
> - return NULL;
> -
> dart_domain = kzalloc(sizeof(*dart_domain), GFP_KERNEL);
> if (!dart_domain)
> return NULL;
> @@ -982,7 +971,7 @@ static void apple_dart_get_resv_regions(struct device
> *dev,
> static const struct iommu_ops apple_dart_iommu_ops = {
> .identity_domain = _dart_identity_domain,
> .blocked_domain = _dart_blocked_domain,
> - .domain_alloc = apple_dart_domain_alloc,
> + .domain_alloc_paging = apple_dart_domain_alloc_paging,
> .probe_device = apple_dart_probe_device,
> .release_device = apple_dart_release_device,
> .device_group = apple_dart_device_group,
> @@ -992,7 +981,7 @@ static const struct iommu_ops apple_dart_iommu_ops = {
> .pgsize_bitmap = -1UL, /* Restricted during dart probe */
> .owner = THIS_MODULE,
> .default_domain_ops = &(const struct iommu_domain_ops) {
> - .attach_dev = apple_dart_attach_dev,
> + .attach_dev = apple_dart_attach_dev_paging,
> .map_pages = apple_dart_map_pages,
> .unmap_pages= apple_dart_unmap_pages,
> .flush_iotlb_all = apple_dart_flush_iotlb_all,
> --
> 2.42.0
Reviewed-by: Janne Grunau
best regards
Janne
Re: [PATCH 6/8] iommu/dart: Move the blocked domain support to a global static
Hej,
On Fri, Sep 22, 2023 at 02:07:57PM -0300, Jason Gunthorpe wrote:
> Move to the new static global for blocked domains. Move the blocked
> specific code to apple_dart_attach_dev_blocked().
>
> Signed-off-by: Jason Gunthorpe
> ---
> drivers/iommu/apple-dart.c | 36 ++--
> 1 file changed, 26 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
> index 424f779ccc34df..376f4c5461e8f7 100644
> --- a/drivers/iommu/apple-dart.c
> +++ b/drivers/iommu/apple-dart.c
> @@ -675,10 +675,6 @@ static int apple_dart_attach_dev(struct iommu_domain
> *domain,
> for_each_stream_map(i, cfg, stream_map)
> apple_dart_setup_translation(dart_domain, stream_map);
> break;
> - case IOMMU_DOMAIN_BLOCKED:
> - for_each_stream_map(i, cfg, stream_map)
> - apple_dart_hw_disable_dma(stream_map);
> - break;
> default:
> return -EINVAL;
> }
> @@ -710,6 +706,30 @@ static struct iommu_domain apple_dart_identity_domain = {
> .ops = _dart_identity_ops,
> };
>
> +static int apple_dart_attach_dev_blocked(struct iommu_domain *domain,
> + struct device *dev)
> +{
> + struct apple_dart_master_cfg *cfg = dev_iommu_priv_get(dev);
> + struct apple_dart_stream_map *stream_map;
> + int i;
> +
> + if (cfg->stream_maps[0].dart->force_bypass)
> + return -EINVAL;
unrelated to this change as this keeps the current behavior but I think
force_bypass should not override IOMMU_DOMAIN_BLOCKED. It is set if the
CPU page size is smaller than dart's page size. Obviously dart can't
translate in that situation but it should be still possible to block it
completely.
How do we manage this? I can write a patch either to the current state
or based on this series.
> +
> + for_each_stream_map(i, cfg, stream_map)
> + apple_dart_hw_disable_dma(stream_map);
> + return 0;
> +}
> +
> +static const struct iommu_domain_ops apple_dart_blocked_ops = {
> + .attach_dev = apple_dart_attach_dev_blocked,
> +};
> +
> +static struct iommu_domain apple_dart_blocked_domain = {
> + .type = IOMMU_DOMAIN_BLOCKED,
> + .ops = _dart_blocked_ops,
> +};
> +
> static struct iommu_device *apple_dart_probe_device(struct device *dev)
> {
> struct apple_dart_master_cfg *cfg = dev_iommu_priv_get(dev);
> @@ -739,8 +759,7 @@ static struct iommu_domain
> *apple_dart_domain_alloc(unsigned int type)
> {
> struct apple_dart_domain *dart_domain;
>
> - if (type != IOMMU_DOMAIN_DMA && type != IOMMU_DOMAIN_UNMANAGED &&
> - type != IOMMU_DOMAIN_BLOCKED)
> + if (type != IOMMU_DOMAIN_DMA && type != IOMMU_DOMAIN_UNMANAGED)
> return NULL;
>
> dart_domain = kzalloc(sizeof(*dart_domain), GFP_KERNEL);
> @@ -749,10 +768,6 @@ static struct iommu_domain
> *apple_dart_domain_alloc(unsigned int type)
>
> mutex_init(_domain->init_lock);
>
> - /* no need to allocate pgtbl_ops or do any other finalization steps */
> - if (type == IOMMU_DOMAIN_BLOCKED)
> - dart_domain->finalized = true;
> -
> return _domain->domain;
> }
>
> @@ -966,6 +981,7 @@ static void apple_dart_get_resv_regions(struct device
> *dev,
>
> static const struct iommu_ops apple_dart_iommu_ops = {
> .identity_domain = _dart_identity_domain,
> + .blocked_domain = _dart_blocked_domain,
> .domain_alloc = apple_dart_domain_alloc,
> .probe_device = apple_dart_probe_device,
> .release_device = apple_dart_release_device,
> --
> 2.42.0
Reviewed-by: Janne Grunau
best regards
Janne
ps: I sent the reply to [Patch 4/8] accidentally with an incorrect from
address but the correct Reviewed-by:. I can resend if necessary.
