diff mbox series

[v2,04/25] iommu: Add IOMMU_DOMAIN_PLATFORM for S390

Message ID 4-v2-8d1dc464eac9+10f-iommu_all_defdom_jgg@nvidia.com (mailing list archive)
State Superseded
Headers show
Series iommu: Make default_domain's mandatory | expand

Commit Message

Jason Gunthorpe May 16, 2023, midnight UTC 
The PLATFORM domain will be set as the default domain and attached as
normal during probe. The driver will ignore the initial attach from a NULL
domain to the PLATFORM domain.

After this, the PLATFORM domain's attach_dev will be called whenever we
detach from an UNMANAGED domain (eg for VFIO). This is the same time the
original design would have called op->detach_dev().

This is temporary until the S390 dma-iommu.c conversion is merged.

Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Niklas Schnelle <schnelle@linux.ibm.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/s390-iommu.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

Comments

Robin Murphy June 1, 2023, 6:25 p.m. UTC | #1 
On 2023-05-16 01:00, Jason Gunthorpe wrote:
> The PLATFORM domain will be set as the default domain and attached as
> normal during probe. The driver will ignore the initial attach from a NULL
> domain to the PLATFORM domain.
> 
> After this, the PLATFORM domain's attach_dev will be called whenever we
> detach from an UNMANAGED domain (eg for VFIO). This is the same time the
> original design would have called op->detach_dev().
> 
> This is temporary until the S390 dma-iommu.c conversion is merged.

If we do need a stopgap here, can we please just call the current 
situation an identity domain? It's true enough in the sense that the 
IOMMU API is not offering any translation or guarantee of isolation, so 
the semantics of an identity domain - from the point of view of anything 
inside the IOMMU API that would be looking - are no weaker or less 
useful than a "platform" domain whose semantics are intentionally unknown.

Then similarly for patch #3 - since we already know s390 is temporary, 
it seems an anathema to introduce a whole domain type with its own weird 
ops->default_domain mechanism solely for POWER to not actually use 
domains with.

In terms of reasoning, I don't see that IOMMU_DOMAIN_PLATFORM is any 
more useful than a NULL default domain, it just renames the problem, and 
gives us more code to maintain for the privilege. As I say, though, we 
don't actually need to juggle the semantic of a "we don't know what's 
happening here" domain around any further, since it works out that a 
"we're not influencing anything here" domain actually suffices for what 
we want to reason about, and those are already well-defined. Sure, the 
platform DMA ops *might* be doing more, but that's beyond the scope of 
the IOMMU API either way. At that point, lo and behold, s390 and POWER 
now look just like ARM and the core code only needs a single special 
case for arch-specific default identity domains, lovely!

Thanks,
Robin.

> Tested-by: Heiko Stuebner <heiko@sntech.de>
> Tested-by: Niklas Schnelle <schnelle@linux.ibm.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/iommu/s390-iommu.c | 21 +++++++++++++++++++--
>   1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
> index fbf59a8db29b11..f0c867c57a5b9b 100644
> --- a/drivers/iommu/s390-iommu.c
> +++ b/drivers/iommu/s390-iommu.c
> @@ -142,14 +142,31 @@ static int s390_iommu_attach_device(struct iommu_domain *domain,
>   	return 0;
>   }
>   
> -static void s390_iommu_set_platform_dma(struct device *dev)
> +/*
> + * Switch control over the IOMMU to S390's internal dma_api ops
> + */
> +static int s390_iommu_platform_attach(struct iommu_domain *platform_domain,
> +				      struct device *dev)
>   {
>   	struct zpci_dev *zdev = to_zpci_dev(dev);
>   
> +	if (!zdev->s390_domain)
> +		return 0;
> +
>   	__s390_iommu_detach_device(zdev);
>   	zpci_dma_init_device(zdev);
> +	return 0;
>   }
>   
> +static struct iommu_domain_ops s390_iommu_platform_ops = {
> +	.attach_dev = s390_iommu_platform_attach,
> +};
> +
> +static struct iommu_domain s390_iommu_platform_domain = {
> +	.type = IOMMU_DOMAIN_PLATFORM,
> +	.ops = &s390_iommu_platform_ops,
> +};
> +
>   static void s390_iommu_get_resv_regions(struct device *dev,
>   					struct list_head *list)
>   {
> @@ -428,12 +445,12 @@ void zpci_destroy_iommu(struct zpci_dev *zdev)
>   }
>   
>   static const struct iommu_ops s390_iommu_ops = {
> +	.default_domain = &s390_iommu_platform_domain,
>   	.capable = s390_iommu_capable,
>   	.domain_alloc = s390_domain_alloc,
>   	.probe_device = s390_iommu_probe_device,
>   	.release_device = s390_iommu_release_device,
>   	.device_group = generic_device_group,
> -	.set_platform_dma_ops = s390_iommu_set_platform_dma,
>   	.pgsize_bitmap = SZ_4K,
>   	.get_resv_regions = s390_iommu_get_resv_regions,
>   	.default_domain_ops = &(const struct iommu_domain_ops) {
Jason Gunthorpe June 1, 2023, 7:58 p.m. UTC | #2 
On Thu, Jun 01, 2023 at 07:25:32PM +0100, Robin Murphy wrote:
> On 2023-05-16 01:00, Jason Gunthorpe wrote:
> > The PLATFORM domain will be set as the default domain and attached as
> > normal during probe. The driver will ignore the initial attach from a NULL
> > domain to the PLATFORM domain.
> > 
> > After this, the PLATFORM domain's attach_dev will be called whenever we
> > detach from an UNMANAGED domain (eg for VFIO). This is the same time the
> > original design would have called op->detach_dev().
> > 
> > This is temporary until the S390 dma-iommu.c conversion is merged.
> 
> If we do need a stopgap here, can we please just call the current situation
> an identity domain? 

I really rather wouldn't lie, especially since we need something for
PPC more permanently. I definately don't want to call PPC IDENTITY.

> Then similarly for patch #3 - since we already know s390 is temporary, it
> seems an anathema to introduce a whole domain type with its own weird
> ops->default_domain mechanism solely for POWER to not actually use domains
> with.

A main point of this entire series is to remove the NULL default
domain, so power's weirdness has to be accomodated somehow.
 
> In terms of reasoning, I don't see that IOMMU_DOMAIN_PLATFORM is any more
> useful than a NULL default domain, it just renames the problem

Yes! Renaming is the whole point.

We are giving actuall meaningful names to all the things that drivers
did with NULL. The ones that actually implemented IDENTITY correctly
gets to be called IDENTITY, everyone else needs to get labeled
something else.

Then we can tell what is correct and what needs fixing at a glance.

That is the *whole point*.

> and gives us more code to maintain for the privilege.

The PLATFORM bit is only 3 lines of core code. Let's not overstate the
cost of a label please.

> As I say, though, we don't actually need to juggle the semantic of a
> "we don't know what's happening here" domain around any further,
> since it works out that a "we're not influencing anything here"
> domain actually suffices for what we want to reason about, and those
> are already well-defined. Sure, the platform DMA ops *might* be
> doing more, but that's beyond the scope of the IOMMU API either
> way. At that point, lo and behold, s390 and POWER now look just like
> ARM and the core code only needs a single special case for
> arch-specific default identity domains, lovely!

I'm really against mis-labeling things.

That is totally the wrong direction for this series. The point is to
label things correctly. Labeling something that is not an IDENTITY
operation as IDENTITY is just repeating the whole mistake of an
ill-defined NULL all over again.

Labels need to be correct, labels are cheap to add, so lets not be
afraid to use proper labels to actually describe the underlying
behavior.

If you don't like PLATFORM we can choose PRIVATE, OPAQUE or some other
label, but not IDENTITY.

Jason
diff mbox series

Patch

diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
index fbf59a8db29b11..f0c867c57a5b9b 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -142,14 +142,31 @@  static int s390_iommu_attach_device(struct iommu_domain *domain,
 	return 0;
 }
 
-static void s390_iommu_set_platform_dma(struct device *dev)
+/*
+ * Switch control over the IOMMU to S390's internal dma_api ops
+ */
+static int s390_iommu_platform_attach(struct iommu_domain *platform_domain,
+				      struct device *dev)
 {
 	struct zpci_dev *zdev = to_zpci_dev(dev);
 
+	if (!zdev->s390_domain)
+		return 0;
+
 	__s390_iommu_detach_device(zdev);
 	zpci_dma_init_device(zdev);
+	return 0;
 }
 
+static struct iommu_domain_ops s390_iommu_platform_ops = {
+	.attach_dev = s390_iommu_platform_attach,
+};
+
+static struct iommu_domain s390_iommu_platform_domain = {
+	.type = IOMMU_DOMAIN_PLATFORM,
+	.ops = &s390_iommu_platform_ops,
+};
+
 static void s390_iommu_get_resv_regions(struct device *dev,
 					struct list_head *list)
 {
@@ -428,12 +445,12 @@  void zpci_destroy_iommu(struct zpci_dev *zdev)
 }
 
 static const struct iommu_ops s390_iommu_ops = {
+	.default_domain = &s390_iommu_platform_domain,
 	.capable = s390_iommu_capable,
 	.domain_alloc = s390_domain_alloc,
 	.probe_device = s390_iommu_probe_device,
 	.release_device = s390_iommu_release_device,
 	.device_group = generic_device_group,
-	.set_platform_dma_ops = s390_iommu_set_platform_dma,
 	.pgsize_bitmap = SZ_4K,
 	.get_resv_regions = s390_iommu_get_resv_regions,
 	.default_domain_ops = &(const struct iommu_domain_ops) {