[PATCH 0/6] iommu/s390: add support for IOMMU passthrough

[PATCH 0/6] iommu/s390: add support for IOMMU passthrough

[Matthew Rosato]

This series introduces the ability for certain devices on s390 to bypass
a layer of IOMMU via the iommu.passthrough=1 option.  In order to enable
this, the concept of an identity domain is added to s390-iommu.  On s390,
IOMMU passthrough is only allowed if indicated via a special bit in s390
CLP data for the associated device group, otherwise we must fall back to
dma-iommu.

Matthew Rosato (6):
  s390/pci: check for relaxed translation capability
  s390: enable ARCH_HAS_PHYS_TO_DMA
  iommu/s390: implement iommu passthrough via identity domain
  iommu: add routine to check strict setting
  iommu: document missing def_domain_type return
  iommu/s390: implement def_domain_type

 arch/s390/Kconfig                  |  1 +
 arch/s390/include/asm/device.h     | 12 ++++++
 arch/s390/include/asm/dma-direct.h | 14 +++++++
 arch/s390/include/asm/pci.h        |  2 +-
 arch/s390/include/asm/pci_clp.h    |  4 +-
 arch/s390/pci/pci.c                |  6 ++-
 arch/s390/pci/pci_bus.c            |  2 +
 arch/s390/pci/pci_clp.c            |  1 +
 drivers/iommu/iommu.c              |  5 +++
 drivers/iommu/s390-iommu.c         | 65 ++++++++++++++++++++++++++++--
 include/linux/iommu.h              |  2 +
 11 files changed, 107 insertions(+), 7 deletions(-)
 create mode 100644 arch/s390/include/asm/device.h
 create mode 100644 arch/s390/include/asm/dma-direct.h

-- 
2.47.0

[PATCH 1/6] s390/pci: check for relaxed translation capability

[Matthew Rosato]

For each zdev, record whether or not CLP indicates relaxed translation
capability for the associated device group.

Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 arch/s390/include/asm/pci.h     | 2 +-
 arch/s390/include/asm/pci_clp.h | 4 +++-
 arch/s390/pci/pci_clp.c         | 1 +
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
index 474e1f8d1d3c..8fe4c7a72c0b 100644
--- a/arch/s390/include/asm/pci.h
+++ b/arch/s390/include/asm/pci.h
@@ -144,7 +144,7 @@ struct zpci_dev {
 	u8		util_str_avail	: 1;
 	u8		irqs_registered	: 1;
 	u8		tid_avail	: 1;
-	u8		reserved	: 1;
+	u8		rtr_avail	: 1; /* Relaxed translation allowed */
 	unsigned int	devfn;		/* DEVFN part of the RID*/
 
 	u8 pfip[CLP_PFIP_NR_SEGMENTS];	/* pci function internal path */
diff --git a/arch/s390/include/asm/pci_clp.h b/arch/s390/include/asm/pci_clp.h
index 3fff2f7095c8..7ebff39c84b3 100644
--- a/arch/s390/include/asm/pci_clp.h
+++ b/arch/s390/include/asm/pci_clp.h
@@ -156,7 +156,9 @@ struct clp_rsp_query_pci_grp {
 	u16			:  4;
 	u16 noi			: 12;	/* number of interrupts */
 	u8 version;
-	u8			:  6;
+	u8			:  2;
+	u8 rtr			:  1;	/* Relaxed translation requirement */
+	u8			:  3;
 	u8 frame		:  1;
 	u8 refresh		:  1;	/* TLB refresh mode */
 	u16			:  3;
diff --git a/arch/s390/pci/pci_clp.c b/arch/s390/pci/pci_clp.c
index 14bf7e8d06b7..27248686e588 100644
--- a/arch/s390/pci/pci_clp.c
+++ b/arch/s390/pci/pci_clp.c
@@ -112,6 +112,7 @@ static void clp_store_query_pci_fngrp(struct zpci_dev *zdev,
 	zdev->version = response->version;
 	zdev->maxstbl = response->maxstbl;
 	zdev->dtsm = response->dtsm;
+	zdev->rtr_avail = response->rtr;
 
 	switch (response->version) {
 	case 1:
-- 
2.47.0

[PATCH 2/6] s390: enable ARCH_HAS_PHYS_TO_DMA

[Matthew Rosato]

PCI devices on s390 have a DMA offset that is reported via CLP.  In
preparation for allowing identity domains, enable ARCH_HAS_PHYS_TO_DMA
for s390 and get the dma offset for all PCI devices from the reported
CLP value.

Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>
Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 arch/s390/Kconfig                  |  1 +
 arch/s390/include/asm/device.h     | 12 ++++++++++++
 arch/s390/include/asm/dma-direct.h | 14 ++++++++++++++
 arch/s390/pci/pci_bus.c            |  2 ++
 4 files changed, 29 insertions(+)
 create mode 100644 arch/s390/include/asm/device.h
 create mode 100644 arch/s390/include/asm/dma-direct.h

diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 0077969170e8..5746d8abc8a7 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -83,6 +83,7 @@ config S390
 	select ARCH_HAS_FORTIFY_SOURCE
 	select ARCH_HAS_GCOV_PROFILE_ALL
 	select ARCH_HAS_GIGANTIC_PAGE
+	select ARCH_HAS_PHYS_TO_DMA
 	select ARCH_HAS_KCOV
 	select ARCH_HAS_MEMBARRIER_SYNC_CORE
 	select ARCH_HAS_MEM_ENCRYPT
diff --git a/arch/s390/include/asm/device.h b/arch/s390/include/asm/device.h
new file mode 100644
index 000000000000..a6bf5e137a17
--- /dev/null
+++ b/arch/s390/include/asm/device.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_S390_DEVICE_H
+#define _ASM_S390_DEVICE_H
+
+struct dev_archdata {
+	dma_addr_t		dma_offset;
+};
+
+struct pdev_archdata {
+};
+
+#endif /* _ASM_S390_DEVICE_H */
diff --git a/arch/s390/include/asm/dma-direct.h b/arch/s390/include/asm/dma-direct.h
new file mode 100644
index 000000000000..9391820fca2c
--- /dev/null
+++ b/arch/s390/include/asm/dma-direct.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef ASM_S390_DMA_DIRECT_H
+#define ASM_S390_DMA_DIRECT_H 1
+
+static inline dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr)
+{
+	return paddr + dev->archdata.dma_offset;
+}
+
+static inline phys_addr_t dma_to_phys(struct device *dev, dma_addr_t daddr)
+{
+	return daddr - dev->archdata.dma_offset;
+}
+#endif /* ASM_S390_DMA_DIRECT_H */
diff --git a/arch/s390/pci/pci_bus.c b/arch/s390/pci/pci_bus.c
index d5ace00d10f0..ec874ba858c6 100644
--- a/arch/s390/pci/pci_bus.c
+++ b/arch/s390/pci/pci_bus.c
@@ -288,6 +288,8 @@ void pcibios_bus_add_device(struct pci_dev *pdev)
 {
 	struct zpci_dev *zdev = to_zpci(pdev);
 
+	pdev->dev.archdata.dma_offset = zdev->start_dma;
+
 	/*
 	 * With pdev->no_vf_scan the common PCI probing code does not
 	 * perform PF/VF linking.
-- 
2.47.0

Re: [PATCH 2/6] s390: enable ARCH_HAS_PHYS_TO_DMA

[Christoph Hellwig]

On Mon, Dec 09, 2024 at 02:23:59PM -0500, Matthew Rosato wrote:
> PCI devices on s390 have a DMA offset that is reported via CLP.  In
> preparation for allowing identity domains, enable ARCH_HAS_PHYS_TO_DMA
> for s390 and get the dma offset for all PCI devices from the reported
> CLP value.

Nothing new should select ARCH_HAS_PHYS_TO_DMA, please fill out the
bus_dma_region attached to the device instead.

Re: [PATCH 2/6] s390: enable ARCH_HAS_PHYS_TO_DMA

[Matthew Rosato]

On 12/9/24 11:33 PM, Christoph Hellwig wrote:
> On Mon, Dec 09, 2024 at 02:23:59PM -0500, Matthew Rosato wrote:
>> PCI devices on s390 have a DMA offset that is reported via CLP.  In
>> preparation for allowing identity domains, enable ARCH_HAS_PHYS_TO_DMA
>> for s390 and get the dma offset for all PCI devices from the reported
>> CLP value.
> 
> Nothing new should select ARCH_HAS_PHYS_TO_DMA, please fill out the
> bus_dma_region attached to the device instead.
> 

OK, thanks for the pointer.  I think I've got it converted, will test some more and include this change in next version.

[PATCH 3/6] iommu/s390: implement iommu passthrough via identity domain

[Matthew Rosato]

Enabled via the kernel command-line 'iommu.passthrough=1' option.

Introduce the concept of identity domains to s390-iommu, which relies on
phys_to_dma and dma_to_phys to offset identity mappings to the start of
the DMA aperture advertized by CLP.

Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 arch/s390/pci/pci.c        |  6 ++++--
 drivers/iommu/s390-iommu.c | 42 +++++++++++++++++++++++++++++++++++---
 2 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
index 88f72745fa59..758b23331754 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c
@@ -124,14 +124,16 @@ int zpci_register_ioat(struct zpci_dev *zdev, u8 dmaas,
 	struct zpci_fib fib = {0};
 	u8 cc;
 
-	WARN_ON_ONCE(iota & 0x3fff);
 	fib.pba = base;
 	/* Work around off by one in ISM virt device */
 	if (zdev->pft == PCI_FUNC_TYPE_ISM && limit > base)
 		fib.pal = limit + (1 << 12);
 	else
 		fib.pal = limit;
-	fib.iota = iota | ZPCI_IOTA_RTTO_FLAG;
+	if (iota == 0)
+		fib.iota = iota;
+	else
+		fib.iota = iota | ZPCI_IOTA_RTTO_FLAG;
 	fib.gd = zdev->gisa;
 	cc = zpci_mod_fc(req, &fib, status);
 	if (cc)
diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
index fbdeded3d48b..18979d8109b2 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -392,9 +392,11 @@ static int blocking_domain_attach_device(struct iommu_domain *domain,
 		return 0;
 
 	s390_domain = to_s390_domain(zdev->s390_domain);
-	spin_lock_irqsave(&s390_domain->list_lock, flags);
-	list_del_rcu(&zdev->iommu_list);
-	spin_unlock_irqrestore(&s390_domain->list_lock, flags);
+	if (zdev->dma_table) {
+		spin_lock_irqsave(&s390_domain->list_lock, flags);
+		list_del_rcu(&zdev->iommu_list);
+		spin_unlock_irqrestore(&s390_domain->list_lock, flags);
+	}
 
 	zpci_unregister_ioat(zdev, 0);
 	zdev->dma_table = NULL;
@@ -787,6 +789,39 @@ static int __init s390_iommu_init(void)
 }
 subsys_initcall(s390_iommu_init);
 
+static int s390_attach_dev_identity(struct iommu_domain *domain,
+				    struct device *dev)
+{
+	struct zpci_dev *zdev = to_zpci_dev(dev);
+	u8 status;
+	int cc;
+
+	blocking_domain_attach_device(&blocking_domain, dev);
+
+	/* If we fail now DMA remains blocked via blocking domain */
+	cc = zpci_register_ioat(zdev, 0, zdev->start_dma, zdev->end_dma,
+				0, &status);
+	/*
+	 * If the device is undergoing error recovery the reset code
+	 * will re-establish the new domain.
+	 */
+	if (cc && status != ZPCI_PCI_ST_FUNC_NOT_AVAIL)
+		return -EIO;
+
+	zdev_s390_domain_update(zdev, domain);
+
+	return 0;
+}
+
+static const struct iommu_domain_ops s390_identity_ops = {
+	.attach_dev = s390_attach_dev_identity,
+};
+
+static struct iommu_domain s390_identity_domain = {
+	.type = IOMMU_DOMAIN_IDENTITY,
+	.ops = &s390_identity_ops,
+};
+
 static struct iommu_domain blocking_domain = {
 	.type = IOMMU_DOMAIN_BLOCKED,
 	.ops = &(const struct iommu_domain_ops) {
@@ -797,6 +832,7 @@ static struct iommu_domain blocking_domain = {
 static const struct iommu_ops s390_iommu_ops = {
 	.blocked_domain		= &blocking_domain,
 	.release_domain		= &blocking_domain,
+	.identity_domain	= &s390_identity_domain,
 	.capable = s390_iommu_capable,
 	.domain_alloc_paging = s390_domain_alloc_paging,
 	.probe_device = s390_iommu_probe_device,
-- 
2.47.0

[PATCH 4/6] iommu: add routine to check strict setting

[Matthew Rosato]

Add a simple routine to return whether or not strict TLB invalidation
is in effect for the iommu.  For drivers that implement the
def_domain_type op, this can be used to determine whether or not flush
queueing is allowed.

Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>
Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 drivers/iommu/iommu.c | 5 +++++
 include/linux/iommu.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 599030e1e890..6bdede4177ff 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -715,6 +715,11 @@ void iommu_set_dma_strict(void)
 		iommu_def_domain_type = IOMMU_DOMAIN_DMA;
 }
 
+bool iommu_dma_is_strict(void)
+{
+	return iommu_dma_strict;
+}
+
 static ssize_t iommu_group_attr_show(struct kobject *kobj,
 				     struct attribute *__attr, char *buf)
 {
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 318d27841130..05279109c732 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -911,6 +911,7 @@ int iommu_set_pgtable_quirks(struct iommu_domain *domain,
 		unsigned long quirks);
 
 void iommu_set_dma_strict(void);
+extern bool iommu_dma_is_strict(void);
 
 extern int report_iommu_fault(struct iommu_domain *domain, struct device *dev,
 			      unsigned long iova, int flags);
-- 
2.47.0

[PATCH 5/6] iommu: document missing def_domain_type return

[Matthew Rosato]

In addition to IOMMU_DOMAIN_DMA, def_domain_type can also return
IOMMU_DOMAIN_DMA_FQ when applicable, else flush queues will never be
used.

Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 include/linux/iommu.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 05279109c732..d0da1918d2de 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -585,6 +585,7 @@ iommu_copy_struct_from_full_user_array(void *kdst, size_t kdst_entry_size,
  * @def_domain_type: device default domain type, return value:
  *		- IOMMU_DOMAIN_IDENTITY: must use an identity domain
  *		- IOMMU_DOMAIN_DMA: must use a dma domain
+ *              - IOMMU_DOMAIN_DMA_FQ: dma domain with batch invalidation
  *		- 0: use the default setting
  * @default_domain_ops: the default ops for domains
  * @remove_dev_pasid: Remove any translation configurations of a specific
-- 
2.47.0

Re: [PATCH 5/6] iommu: document missing def_domain_type return

[Baolu Lu]

On 12/10/24 03:24, Matthew Rosato wrote:
> In addition to IOMMU_DOMAIN_DMA, def_domain_type can also return
> IOMMU_DOMAIN_DMA_FQ when applicable, else flush queues will never be
> used.
> 
> Signed-off-by: Matthew Rosato<mjrosato@linux.ibm.com>
> ---
>   include/linux/iommu.h | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 05279109c732..d0da1918d2de 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -585,6 +585,7 @@ iommu_copy_struct_from_full_user_array(void *kdst, size_t kdst_entry_size,
>    * @def_domain_type: device default domain type, return value:
>    *		- IOMMU_DOMAIN_IDENTITY: must use an identity domain
>    *		- IOMMU_DOMAIN_DMA: must use a dma domain
> + *              - IOMMU_DOMAIN_DMA_FQ: dma domain with batch invalidation

In which case must an iommu driver return IOMMU_DOMAIN_DMA_FQ?

The flush queue is a policy of "when and how to synchronize the IOTLB"
in dma-iommu.c. The iommu driver actually has no need to understand such
policy.

By the way, "dma domain" in this comment is a bit confusing, it reads
better if we make it like:

- IOMMU_DOMAIN_IDENTITY: must use an identity domain
- IOMMU_DOMAIN_DMA: must use a paging domain

Thanks,
baolu

Re: [PATCH 5/6] iommu: document missing def_domain_type return

[Matthew Rosato]

On 12/9/24 9:57 PM, Baolu Lu wrote:
> On 12/10/24 03:24, Matthew Rosato wrote:
>> In addition to IOMMU_DOMAIN_DMA, def_domain_type can also return
>> IOMMU_DOMAIN_DMA_FQ when applicable, else flush queues will never be
>> used.
>>
>> Signed-off-by: Matthew Rosato<mjrosato@linux.ibm.com>
>> ---
>>   include/linux/iommu.h | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 05279109c732..d0da1918d2de 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -585,6 +585,7 @@ iommu_copy_struct_from_full_user_array(void *kdst, size_t kdst_entry_size,
>>    * @def_domain_type: device default domain type, return value:
>>    *        - IOMMU_DOMAIN_IDENTITY: must use an identity domain
>>    *        - IOMMU_DOMAIN_DMA: must use a dma domain
>> + *              - IOMMU_DOMAIN_DMA_FQ: dma domain with batch invalidation
> 
> In which case must an iommu driver return IOMMU_DOMAIN_DMA_FQ?
> 
> The flush queue is a policy of "when and how to synchronize the IOTLB"
> in dma-iommu.c. The iommu driver actually has no need to understand such
> policy.

If you look ahead to the next patch where I implement def_domain_type for s390, I found that if I only ever return IOMMU_DOMAIN_DMA from ops->def_domain_type then when go through iommu_dma_init_domain() we will never call iommu_dma_init_fq() regardless of IOMMU_CAP_DEFERRED_FLUSH because of the if (domain->type == IOMMU_DOMAIN_DMA_FQ) check.  So something isn't right here.

It looks to me like the following is happening:

We first have the iommu_def_domain_type set in iommu_subsys_init or via one of the set_default routines, e.g.:
	if (!iommu_default_passthrough() && !iommu_dma_strict)
		iommu_def_domain_type = IOMMU_DOMAIN_DMA_FQ;

But when we arrive at iommu_group_alloc_default_domain()...

if we have no ops->def_domain_type() defined we will call __iommu_group_alloc_default_domain using what is in iommu_def_domain_type, which could be IOMMU_DOMAIN_DMA, IOMMU_DOMAIN_DMA_FQ or IOMMU_DOMAIN_IDENTITY based on strict/passthrough settings.  Testing an s390 scenario today without this series applied, we will call __iommu_group_alloc_default_domain with IOMMU_DOMAIN_DMA_FQ, as long as iommu.strict/passthrough is not specified, so then later in dma-iommu:iommu_dma_init_domain() we can use FQ based on IOMMU_CAP_DEFERRED_FLUSH.

but once we add ops->def_domain_type() then we end up calling iommu_group_alloc_default_domain() with a req_type == the return value from ops->def_domain_type(), which by the current definition can only be IOMMU_DOMAIN_DMA or IOMMU_DOMAIN_IDENTITY.  We will then call __iommu_group_alloc_default_domain with that req_type; so without this patch + the DMA_FQ path in patch 6 we would always end up allocating IOMMU_DOMAIN_DMA instead of IOMMU_DOMAIN_DMA_FQ by default, so when we arrive at dma:iommu_dma_init_domain() we won't check for IOMMU_CAP_DEFERRED_FLUSH because of the type.

So unless I'm missing something I think either we have to
1) be more flexible in what ops->default_domain_type() is allowed to return as this patch does 
or 
2) iommu core needs to look at the return from ops->default_domain_type() and decide whether it's OK to convert IOMMU_DOMAIN_DMA->IOMMU_DOMAIN_DMA_FQ based on strict setting.  This removes the decision from the individual drivers and dma-iommu can later decide whether or not to use it or not based on IOMMU_CAP_DEFERRED_FLUSH?  But would also affect other users of def_domain_type() today that perhaps did not want DMA_FQ?  Unsure.  What I mean is something like (untested):

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 6bdede4177ff..275daa7f819d 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1744,9 +1744,11 @@ static int iommu_get_def_domain_type(struct iommu_group *group,
                 */
                type = ops->default_domain->type;
        } else {
-               if (ops->def_domain_type)
+               if (ops->def_domain_type) {
                        type = ops->def_domain_type(dev);
-               else
+                       if (type == IOMMU_DOMAIN_DMA && !iommu_dma_strict)
+                               type = IOMMU_DOMAIN_DMA_FQ;
+               } else
                        return cur_type;
        }
        if (!type || cur_type == type)

Re: [PATCH 5/6] iommu: document missing def_domain_type return

[Robin Murphy]

On 10/12/2024 4:26 pm, Matthew Rosato wrote:
> On 12/9/24 9:57 PM, Baolu Lu wrote:
>> On 12/10/24 03:24, Matthew Rosato wrote:
>>> In addition to IOMMU_DOMAIN_DMA, def_domain_type can also return
>>> IOMMU_DOMAIN_DMA_FQ when applicable, else flush queues will never be
>>> used.
>>>
>>> Signed-off-by: Matthew Rosato<mjrosato@linux.ibm.com>
>>> ---
>>>    include/linux/iommu.h | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>>> index 05279109c732..d0da1918d2de 100644
>>> --- a/include/linux/iommu.h
>>> +++ b/include/linux/iommu.h
>>> @@ -585,6 +585,7 @@ iommu_copy_struct_from_full_user_array(void *kdst, size_t kdst_entry_size,
>>>     * @def_domain_type: device default domain type, return value:
>>>     *        - IOMMU_DOMAIN_IDENTITY: must use an identity domain
>>>     *        - IOMMU_DOMAIN_DMA: must use a dma domain
>>> + *              - IOMMU_DOMAIN_DMA_FQ: dma domain with batch invalidation
>>
>> In which case must an iommu driver return IOMMU_DOMAIN_DMA_FQ?
>>
>> The flush queue is a policy of "when and how to synchronize the IOTLB"
>> in dma-iommu.c. The iommu driver actually has no need to understand such
>> policy.
> 
> If you look ahead to the next patch where I implement def_domain_type for s390, I found that if I only ever return IOMMU_DOMAIN_DMA from ops->def_domain_type then when go through iommu_dma_init_domain() we will never call iommu_dma_init_fq() regardless of IOMMU_CAP_DEFERRED_FLUSH because of the if (domain->type == IOMMU_DOMAIN_DMA_FQ) check.  So something isn't right here.

Conceptually I don't think it ever makes sense for a driver to *require* 
a device to use deferred invalidation. Furthermore it's been the whole 
design for a while now that drivers should never see nor have to 
acknowledge IOMMU_DOMAIN_DMA_FQ, it's now just an internal type which 
exists largely for the sake of making the sysfs interface work really 
neatly. Also beware that a major reason for overriding 
iommu_def_domain_type with a paging domain is for untrusted devices, so 
massaging the result based on iommu_dma_strict is still not necessarily 
appropriate anyway.

It appears the real underlying issue is that, like everyone else in the 
same situation, you're doing def_domain_type wrong. If and when you 
can't support IOMMU_DOMAIN_IDENTITY, the expectation is that you make 
__iommu_alloc_identity_domain() fail, such that if iommu_def_domain_type 
is then ever set to passthrough, iommu_group_alloc_default_domain() 
falls back to IOMMU_DOMAIN_DMA by itself, and the user gets told they 
did a silly thing.

What you see apple-dart doing is a hack around the old bus-based 
iommu_domain_alloc() API where there wasn't enough information at the 
right point to necessarily do the right thing.

Thanks,
Robin.

> It looks to me like the following is happening:
> 
> We first have the iommu_def_domain_type set in iommu_subsys_init or via one of the set_default routines, e.g.:
> 	if (!iommu_default_passthrough() && !iommu_dma_strict)
> 		iommu_def_domain_type = IOMMU_DOMAIN_DMA_FQ;
> 
> But when we arrive at iommu_group_alloc_default_domain()...
> 
> if we have no ops->def_domain_type() defined we will call __iommu_group_alloc_default_domain using what is in iommu_def_domain_type, which could be IOMMU_DOMAIN_DMA, IOMMU_DOMAIN_DMA_FQ or IOMMU_DOMAIN_IDENTITY based on strict/passthrough settings.  Testing an s390 scenario today without this series applied, we will call __iommu_group_alloc_default_domain with IOMMU_DOMAIN_DMA_FQ, as long as iommu.strict/passthrough is not specified, so then later in dma-iommu:iommu_dma_init_domain() we can use FQ based on IOMMU_CAP_DEFERRED_FLUSH.
> 
> but once we add ops->def_domain_type() then we end up calling iommu_group_alloc_default_domain() with a req_type == the return value from ops->def_domain_type(), which by the current definition can only be IOMMU_DOMAIN_DMA or IOMMU_DOMAIN_IDENTITY.  We will then call __iommu_group_alloc_default_domain with that req_type; so without this patch + the DMA_FQ path in patch 6 we would always end up allocating IOMMU_DOMAIN_DMA instead of IOMMU_DOMAIN_DMA_FQ by default, so when we arrive at dma:iommu_dma_init_domain() we won't check for IOMMU_CAP_DEFERRED_FLUSH because of the type.
> 
> So unless I'm missing something I think either we have to
> 1) be more flexible in what ops->default_domain_type() is allowed to return as this patch does
> or
> 2) iommu core needs to look at the return from ops->default_domain_type() and decide whether it's OK to convert IOMMU_DOMAIN_DMA->IOMMU_DOMAIN_DMA_FQ based on strict setting.  This removes the decision from the individual drivers and dma-iommu can later decide whether or not to use it or not based on IOMMU_CAP_DEFERRED_FLUSH?  But would also affect other users of def_domain_type() today that perhaps did not want DMA_FQ?  Unsure.  What I mean is something like (untested):
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 6bdede4177ff..275daa7f819d 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1744,9 +1744,11 @@ static int iommu_get_def_domain_type(struct iommu_group *group,
>                   */
>                  type = ops->default_domain->type;
>          } else {
> -               if (ops->def_domain_type)
> +               if (ops->def_domain_type) {
>                          type = ops->def_domain_type(dev);
> -               else
> +                       if (type == IOMMU_DOMAIN_DMA && !iommu_dma_strict)
> +                               type = IOMMU_DOMAIN_DMA_FQ;
> +               } else
>                          return cur_type;
>          }
>          if (!type || cur_type == type)
> 
> 

Re: [PATCH 5/6] iommu: document missing def_domain_type return

[Matthew Rosato]

On 12/10/24 1:42 PM, Robin Murphy wrote:
> On 10/12/2024 4:26 pm, Matthew Rosato wrote:
>> On 12/9/24 9:57 PM, Baolu Lu wrote:
>>> On 12/10/24 03:24, Matthew Rosato wrote:
>>>> In addition to IOMMU_DOMAIN_DMA, def_domain_type can also return
>>>> IOMMU_DOMAIN_DMA_FQ when applicable, else flush queues will never be
>>>> used.
>>>>
>>>> Signed-off-by: Matthew Rosato<mjrosato@linux.ibm.com>
>>>> ---
>>>>    include/linux/iommu.h | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>>>> index 05279109c732..d0da1918d2de 100644
>>>> --- a/include/linux/iommu.h
>>>> +++ b/include/linux/iommu.h
>>>> @@ -585,6 +585,7 @@ iommu_copy_struct_from_full_user_array(void *kdst, size_t kdst_entry_size,
>>>>     * @def_domain_type: device default domain type, return value:
>>>>     *        - IOMMU_DOMAIN_IDENTITY: must use an identity domain
>>>>     *        - IOMMU_DOMAIN_DMA: must use a dma domain
>>>> + *              - IOMMU_DOMAIN_DMA_FQ: dma domain with batch invalidation
>>>
>>> In which case must an iommu driver return IOMMU_DOMAIN_DMA_FQ?
>>>
>>> The flush queue is a policy of "when and how to synchronize the IOTLB"
>>> in dma-iommu.c. The iommu driver actually has no need to understand such
>>> policy.
>>
>> If you look ahead to the next patch where I implement def_domain_type for s390, I found that if I only ever return IOMMU_DOMAIN_DMA from ops->def_domain_type then when go through iommu_dma_init_domain() we will never call iommu_dma_init_fq() regardless of IOMMU_CAP_DEFERRED_FLUSH because of the if (domain->type == IOMMU_DOMAIN_DMA_FQ) check.  So something isn't right here.
> 
> Conceptually I don't think it ever makes sense for a driver to *require* a device to use deferred invalidation. Furthermore it's been the whole design for a while now that drivers should never see nor have to acknowledge IOMMU_DOMAIN_DMA_FQ, it's now just an internal type which exists largely for the sake of making the sysfs interface work really neatly. Also beware that a major reason for overriding iommu_def_domain_type with a paging domain is for untrusted devices, so massaging the result based on iommu_dma_strict is still not necessarily appropriate anyway.
> 
> It appears the real underlying issue is that, like everyone else in the same situation, you're doing def_domain_type wrong. If and when you can't support IOMMU_DOMAIN_IDENTITY, the expectation is that you make __iommu_alloc_identity_domain() fail, such that if iommu_def_domain_type is then ever set to passthrough, iommu_group_alloc_default_domain() falls back to IOMMU_DOMAIN_DMA by itself, and the user gets told they did a silly thing.

OK, I almost see where this all fits to throw out def_domain_type for this series...  but looking at __iommu_alloc_identity_domain, the preferred approach is using a static identity domain which turns __iommu_alloc_identity_domain into a nofail case once you define the identity_domain:
 
if (ops->identity_domain)
	return ops->identity_domain;

So it seems to me to be an all-or-nothing thing, whereas what I'm trying to achieve is a device-based decision on whether the group is allowed to use that identity domain.  Which reminds me that this is ultimately why I ended up looking into def_domain_type in the first place.

If I need __iommu_alloc_identity_domain to fail, I guess what I'm looking to do boils down to something like...

if (ops->identity_domain) { 
	if (!ops->allow_identity || ops->allow_identity(dev))
		return ops->identity_domain;
	else
		return ERR_PTR(-EOPNOTSUPP);
}

Re: [PATCH 5/6] iommu: document missing def_domain_type return

[Robin Murphy]

On 10/12/2024 10:06 pm, Matthew Rosato wrote:
> On 12/10/24 1:42 PM, Robin Murphy wrote:
>> On 10/12/2024 4:26 pm, Matthew Rosato wrote:
>>> On 12/9/24 9:57 PM, Baolu Lu wrote:
>>>> On 12/10/24 03:24, Matthew Rosato wrote:
>>>>> In addition to IOMMU_DOMAIN_DMA, def_domain_type can also return
>>>>> IOMMU_DOMAIN_DMA_FQ when applicable, else flush queues will never be
>>>>> used.
>>>>>
>>>>> Signed-off-by: Matthew Rosato<mjrosato@linux.ibm.com>
>>>>> ---
>>>>>     include/linux/iommu.h | 1 +
>>>>>     1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>>>>> index 05279109c732..d0da1918d2de 100644
>>>>> --- a/include/linux/iommu.h
>>>>> +++ b/include/linux/iommu.h
>>>>> @@ -585,6 +585,7 @@ iommu_copy_struct_from_full_user_array(void *kdst, size_t kdst_entry_size,
>>>>>      * @def_domain_type: device default domain type, return value:
>>>>>      *        - IOMMU_DOMAIN_IDENTITY: must use an identity domain
>>>>>      *        - IOMMU_DOMAIN_DMA: must use a dma domain
>>>>> + *              - IOMMU_DOMAIN_DMA_FQ: dma domain with batch invalidation
>>>>
>>>> In which case must an iommu driver return IOMMU_DOMAIN_DMA_FQ?
>>>>
>>>> The flush queue is a policy of "when and how to synchronize the IOTLB"
>>>> in dma-iommu.c. The iommu driver actually has no need to understand such
>>>> policy.
>>>
>>> If you look ahead to the next patch where I implement def_domain_type for s390, I found that if I only ever return IOMMU_DOMAIN_DMA from ops->def_domain_type then when go through iommu_dma_init_domain() we will never call iommu_dma_init_fq() regardless of IOMMU_CAP_DEFERRED_FLUSH because of the if (domain->type == IOMMU_DOMAIN_DMA_FQ) check.  So something isn't right here.
>>
>> Conceptually I don't think it ever makes sense for a driver to *require* a device to use deferred invalidation. Furthermore it's been the whole design for a while now that drivers should never see nor have to acknowledge IOMMU_DOMAIN_DMA_FQ, it's now just an internal type which exists largely for the sake of making the sysfs interface work really neatly. Also beware that a major reason for overriding iommu_def_domain_type with a paging domain is for untrusted devices, so massaging the result based on iommu_dma_strict is still not necessarily appropriate anyway.
>>
>> It appears the real underlying issue is that, like everyone else in the same situation, you're doing def_domain_type wrong. If and when you can't support IOMMU_DOMAIN_IDENTITY, the expectation is that you make __iommu_alloc_identity_domain() fail, such that if iommu_def_domain_type is then ever set to passthrough, iommu_group_alloc_default_domain() falls back to IOMMU_DOMAIN_DMA by itself, and the user gets told they did a silly thing.
> 
> OK, I almost see where this all fits to throw out def_domain_type for this series...  but looking at __iommu_alloc_identity_domain, the preferred approach is using a static identity domain which turns __iommu_alloc_identity_domain into a nofail case once you define the identity_domain:
>   
> if (ops->identity_domain)
> 	return ops->identity_domain;
> 
> So it seems to me to be an all-or-nothing thing, whereas what I'm trying to achieve is a device-based decision on whether the group is allowed to use that identity domain.  Which reminds me that this is ultimately why I ended up looking into def_domain_type in the first place.

Well, yeah, the static domain is very much designed for the typical case 
where it *is* unconditionally available. Simple options right now would 
be to have two sets of ops and select them per-instance, or if it's a 
system-wide property perhaps have non-const ops and populate/clear 
identity_domain as appropriate, or possibly even just stick with the 
ops->domain_alloc(IOMMU_DOMAIN_IDENTITY) path (perhaps eventually 
evolving into a specialised domain_alloc_identity op to also become 
per-device?). What we should not have is mechanisms like the 
def_domain_type hack where something claims to be available, but then 
some other thing tries to say "oh but you mustn't touch that".

Thanks,
Robin.

> If I need __iommu_alloc_identity_domain to fail, I guess what I'm looking to do boils down to something like...
> 
> if (ops->identity_domain) {
> 	if (!ops->allow_identity || ops->allow_identity(dev))
> 		return ops->identity_domain;
> 	else
> 		return ERR_PTR(-EOPNOTSUPP);
> }
> 
> 
> 
> 
> 
> 

[PATCH 6/6] iommu/s390: implement def_domain_type

[Matthew Rosato]

On s390, identity domains are only supported under certain virtualization
scenarios.  If iommu passthrough is requested but unavailable for a
device, ignore the request and instead fall back to dma-iommu with a
message.

Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 drivers/iommu/s390-iommu.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
index 18979d8109b2..6906e1b5b1b0 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -462,6 +462,28 @@ static void s390_iommu_get_resv_regions(struct device *dev,
 	}
 }
 
+static int s390_iommu_def_domain_type(struct device *dev)
+{
+	struct zpci_dev *zdev = to_zpci_dev(dev);
+
+	/* If strict DMA is requested this takes priority */
+	if (iommu_dma_is_strict())
+		return IOMMU_DOMAIN_DMA;
+
+	/* DMA_FQ is the default unless passthrough is specified */
+	if (!iommu_default_passthrough())
+		return IOMMU_DOMAIN_DMA_FQ;
+
+	/* If passthrough specified, CLP must indicate it is allowed. */
+	if (zdev->rtr_avail)
+		return IOMMU_DOMAIN_IDENTITY;
+
+	/* If not allowed, fall back to using translation via DMA_FQ */
+	pr_info("Passthrough ignored, translation required for %s.\n",
+		pci_name(to_pci_dev(dev)));
+	return IOMMU_DOMAIN_DMA_FQ;
+}
+
 static struct iommu_device *s390_iommu_probe_device(struct device *dev)
 {
 	struct zpci_dev *zdev;
@@ -839,6 +861,7 @@ static const struct iommu_ops s390_iommu_ops = {
 	.device_group = generic_device_group,
 	.pgsize_bitmap = SZ_4K,
 	.get_resv_regions = s390_iommu_get_resv_regions,
+	.def_domain_type = s390_iommu_def_domain_type,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= s390_iommu_attach_device,
 		.map_pages	= s390_iommu_map_pages,
-- 
2.47.0