Re: [PATCH 6/8] iommu/dart: Move the blocked domain support to a global static
On Tue, Sep 26, 2023 at 09:05:08PM +0200, Janne Grunau wrote:
> > +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.
That would be great, dart is the only driver that can fail 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.
Wonderful, actually it probably shouldn't even support allocating a
paging domain if it can't support PAGE_SIZE mappings?
> How do we manage this? I can write a patch either to the current state
> or based on this series.
Let me just add a patch to this series to remove that test with your
explanation, that will avoid patch ordering troubles.
Thanks,
Jason
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.
Re: [PATCH 6/8] iommu/dart: Move the blocked domain support to a global static
On 2023-09-26 20:05, Janne Grunau wrote:
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.
The series is queued already, so best to send a patch based on
iommu/core (I guess just removing these lines?). It won't be
super-useful in practice since the blocking domain is normally only used
to transition to an unmanaged domain which in the force_bypass situation
can't be used anyway, but it's still nice on principle not to have
unnecessary reasons for attach to fail.
Thanks,
Robin.
+
+ 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.
Re: [PATCH 6/8] iommu/dart: Move the blocked domain support to a global static
On 2023-09-26 20:34, Robin Murphy wrote:
On 2023-09-26 20:05, Janne Grunau wrote:
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.
The series is queued already, so best to send a patch based on
iommu/core (I guess just removing these lines?).
Um, what? This isn't the domain_alloc_paging series itself, Robin you
fool. Clearly it's time to close the computer and try again tomorrow...
Cheers,
Robin.
It won't be
super-useful in practice since the blocking domain is normally only used
to transition to an unmanaged domain which in the force_bypass situation
can't be used anyway, but it's still nice on principle not to have
unnecessary reasons for attach to fail.
Thanks,
Robin.
+
+ 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.
[PATCH 6/8] iommu/dart: Move the blocked domain support to a global static
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;
+
+ 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
