[PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Jason Gunthorpe]

PCIe defines a 'no-snoop' bit in each the TLP which is usually implemented
by a platform as bypassing elements in the DMA coherent CPU cache
hierarchy. A driver can command a device to set this bit on some of its
transactions as a micro-optimization.

However, the driver is now responsible to synchronize the CPU cache with
the DMA that bypassed it. On x86 this may be done through the wbinvd
instruction, and the i915 GPU driver is the only Linux DMA driver that
calls it.

The problem comes that KVM on x86 will normally disable the wbinvd
instruction in the guest and render it a NOP. As the driver running in the
guest is not aware the wbinvd doesn't work it may still cause the device
to set the no-snoop bit and the platform will bypass the CPU cache.
Without a working wbinvd there is no way to re-synchronize the CPU cache
and the driver in the VM has data corruption.

Thus, we see a general direction on x86 that the IOMMU HW is able to block
the no-snoop bit in the TLP. This NOP's the optimization and allows KVM to
to NOP the wbinvd without causing any data corruption.

This control for Intel IOMMU was exposed by using IOMMU_CACHE and
IOMMU_CAP_CACHE_COHERENCY, however these two values now have multiple
meanings and usages beyond blocking no-snoop and the whole thing has
become confused. AMD IOMMU has the same feature and same IOPTE bits
however it unconditionally blocks no-snoop.

Change it so that:
 - IOMMU_CACHE is only about the DMA coherence of normal DMAs from a
   device. It is used by the DMA API/VFIO/etc when the user of the
   iommu_domain will not be doing manual cache coherency operations.

 - IOMMU_CAP_CACHE_COHERENCY indicates if IOMMU_CACHE can be used with the
   device.

 - The new optional domain op enforce_cache_coherency() will cause the
   entire domain to block no-snoop requests - ie there is no way for any
   device attached to the domain to opt out of the IOMMU_CACHE behavior.
   This is permanent on the domain and must apply to any future devices
   attached to it.

Ideally an iommu driver should implement enforce_cache_coherency() so that
by DMA API domains allow the no-snoop optimization. This leaves it
available to kernel drivers like i915. VFIO will call
enforce_cache_coherency() before establishing any mappings and the domain
should then permanently block no-snoop.

If enforce_cache_coherency() fails VFIO will communicate back through to
KVM into the arch code via kvm_arch_register_noncoherent_dma()
(only implemented by x86) which triggers a working wbinvd to be made
available to the VM.

While other iommu drivers are certainly welcome to implement
enforce_cache_coherency(), it is not clear there is any benefit in doing
so right now.

This is on github: https://github.com/jgunthorpe/linux/commits/intel_no_snoop

v2:
 - Abandon removing IOMMU_CAP_CACHE_COHERENCY - instead make it the cap
   flag that indicates IOMMU_CACHE is supported
 - Put the VFIO tests for IOMMU_CACHE at VFIO device registration
 - In the Intel driver remove the domain->iommu_snooping value, this is
   global not per-domain
v1: https://lore.kernel.org/r/0-v1-ef02c60ddb76+12ca2-intel_no_snoop_jgg@nvidia.com

Jason Gunthorpe (4):
  iommu: Introduce the domain op enforce_cache_coherency()
  vfio: Move the Intel no-snoop control off of IOMMU_CACHE
  iommu: Redefine IOMMU_CAP_CACHE_COHERENCY as the cap flag for
    IOMMU_CACHE
  vfio: Require that devices support DMA cache coherence

 drivers/iommu/amd/iommu.c       |  7 +++++++
 drivers/iommu/intel/iommu.c     | 17 +++++++++++++----
 drivers/vfio/vfio.c             |  7 +++++++
 drivers/vfio/vfio_iommu_type1.c | 30 +++++++++++++++++++-----------
 include/linux/intel-iommu.h     |  2 +-
 include/linux/iommu.h           |  7 +++++--
 6 files changed, 52 insertions(+), 18 deletions(-)


base-commit: 3123109284176b1532874591f7c81f3837bbdc17
-- 
2.35.1

[PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Jason Gunthorpe via iommu]

PCIe defines a 'no-snoop' bit in each the TLP which is usually implemented
by a platform as bypassing elements in the DMA coherent CPU cache
hierarchy. A driver can command a device to set this bit on some of its
transactions as a micro-optimization.

However, the driver is now responsible to synchronize the CPU cache with
the DMA that bypassed it. On x86 this may be done through the wbinvd
instruction, and the i915 GPU driver is the only Linux DMA driver that
calls it.

The problem comes that KVM on x86 will normally disable the wbinvd
instruction in the guest and render it a NOP. As the driver running in the
guest is not aware the wbinvd doesn't work it may still cause the device
to set the no-snoop bit and the platform will bypass the CPU cache.
Without a working wbinvd there is no way to re-synchronize the CPU cache
and the driver in the VM has data corruption.

Thus, we see a general direction on x86 that the IOMMU HW is able to block
the no-snoop bit in the TLP. This NOP's the optimization and allows KVM to
to NOP the wbinvd without causing any data corruption.

This control for Intel IOMMU was exposed by using IOMMU_CACHE and
IOMMU_CAP_CACHE_COHERENCY, however these two values now have multiple
meanings and usages beyond blocking no-snoop and the whole thing has
become confused. AMD IOMMU has the same feature and same IOPTE bits
however it unconditionally blocks no-snoop.

Change it so that:
 - IOMMU_CACHE is only about the DMA coherence of normal DMAs from a
   device. It is used by the DMA API/VFIO/etc when the user of the
   iommu_domain will not be doing manual cache coherency operations.

 - IOMMU_CAP_CACHE_COHERENCY indicates if IOMMU_CACHE can be used with the
   device.

 - The new optional domain op enforce_cache_coherency() will cause the
   entire domain to block no-snoop requests - ie there is no way for any
   device attached to the domain to opt out of the IOMMU_CACHE behavior.
   This is permanent on the domain and must apply to any future devices
   attached to it.

Ideally an iommu driver should implement enforce_cache_coherency() so that
by DMA API domains allow the no-snoop optimization. This leaves it
available to kernel drivers like i915. VFIO will call
enforce_cache_coherency() before establishing any mappings and the domain
should then permanently block no-snoop.

If enforce_cache_coherency() fails VFIO will communicate back through to
KVM into the arch code via kvm_arch_register_noncoherent_dma()
(only implemented by x86) which triggers a working wbinvd to be made
available to the VM.

While other iommu drivers are certainly welcome to implement
enforce_cache_coherency(), it is not clear there is any benefit in doing
so right now.

This is on github: https://github.com/jgunthorpe/linux/commits/intel_no_snoop

v2:
 - Abandon removing IOMMU_CAP_CACHE_COHERENCY - instead make it the cap
   flag that indicates IOMMU_CACHE is supported
 - Put the VFIO tests for IOMMU_CACHE at VFIO device registration
 - In the Intel driver remove the domain->iommu_snooping value, this is
   global not per-domain
v1: https://lore.kernel.org/r/0-v1-ef02c60ddb76+12ca2-intel_no_snoop_jgg@nvidia.com

Jason Gunthorpe (4):
  iommu: Introduce the domain op enforce_cache_coherency()
  vfio: Move the Intel no-snoop control off of IOMMU_CACHE
  iommu: Redefine IOMMU_CAP_CACHE_COHERENCY as the cap flag for
    IOMMU_CACHE
  vfio: Require that devices support DMA cache coherence

 drivers/iommu/amd/iommu.c       |  7 +++++++
 drivers/iommu/intel/iommu.c     | 17 +++++++++++++----
 drivers/vfio/vfio.c             |  7 +++++++
 drivers/vfio/vfio_iommu_type1.c | 30 +++++++++++++++++++-----------
 include/linux/intel-iommu.h     |  2 +-
 include/linux/iommu.h           |  7 +++++--
 6 files changed, 52 insertions(+), 18 deletions(-)


base-commit: 3123109284176b1532874591f7c81f3837bbdc17
-- 
2.35.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 1/4] iommu: Introduce the domain op enforce_cache_coherency()

[Jason Gunthorpe]

This new mechanism will replace using IOMMU_CAP_CACHE_COHERENCY and
IOMMU_CACHE to control the no-snoop blocking behavior of the IOMMU.

Currently only Intel and AMD IOMMUs are known to support this
feature. They both implement it as an IOPTE bit, that when set, will cause
PCIe TLPs to that IOVA with the no-snoop bit set to be treated as though
the no-snoop bit was clear.

The new API is triggered by calling enforce_cache_coherency() before
mapping any IOVA to the domain which globally switches on no-snoop
blocking. This allows other implementations that might block no-snoop
globally and outside the IOPTE - AMD also documents such a HW capability.

Leave AMD out of sync with Intel and have it block no-snoop even for
in-kernel users. This can be trivially resolved in a follow up patch.

Only VFIO will call this new API.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/amd/iommu.c   |  7 +++++++
 drivers/iommu/intel/iommu.c | 14 +++++++++++++-
 include/linux/intel-iommu.h |  1 +
 include/linux/iommu.h       |  4 ++++
 4 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index a1ada7bff44e61..e500b487eb3429 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2271,6 +2271,12 @@ static int amd_iommu_def_domain_type(struct device *dev)
 	return 0;
 }
 
+static bool amd_iommu_enforce_cache_coherency(struct iommu_domain *domain)
+{
+	/* IOMMU_PTE_FC is always set */
+	return true;
+}
+
 const struct iommu_ops amd_iommu_ops = {
 	.capable = amd_iommu_capable,
 	.domain_alloc = amd_iommu_domain_alloc,
@@ -2293,6 +2299,7 @@ const struct iommu_ops amd_iommu_ops = {
 		.flush_iotlb_all = amd_iommu_flush_iotlb_all,
 		.iotlb_sync	= amd_iommu_iotlb_sync,
 		.free		= amd_iommu_domain_free,
+		.enforce_cache_coherency = amd_iommu_enforce_cache_coherency,
 	}
 };
 
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index df5c62ecf942b8..f08611a6cc4799 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4422,7 +4422,8 @@ static int intel_iommu_map(struct iommu_domain *domain,
 		prot |= DMA_PTE_READ;
 	if (iommu_prot & IOMMU_WRITE)
 		prot |= DMA_PTE_WRITE;
-	if ((iommu_prot & IOMMU_CACHE) && dmar_domain->iommu_snooping)
+	if (((iommu_prot & IOMMU_CACHE) && dmar_domain->iommu_snooping) ||
+	    dmar_domain->enforce_no_snoop)
 		prot |= DMA_PTE_SNP;
 
 	max_addr = iova + size;
@@ -4545,6 +4546,16 @@ static phys_addr_t intel_iommu_iova_to_phys(struct iommu_domain *domain,
 	return phys;
 }
 
+static bool intel_iommu_enforce_cache_coherency(struct iommu_domain *domain)
+{
+	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
+
+	if (!dmar_domain->iommu_snooping)
+		return false;
+	dmar_domain->enforce_no_snoop = true;
+	return true;
+}
+
 static bool intel_iommu_capable(enum iommu_cap cap)
 {
 	if (cap == IOMMU_CAP_CACHE_COHERENCY)
@@ -4898,6 +4909,7 @@ const struct iommu_ops intel_iommu_ops = {
 		.iotlb_sync		= intel_iommu_tlb_sync,
 		.iova_to_phys		= intel_iommu_iova_to_phys,
 		.free			= intel_iommu_domain_free,
+		.enforce_cache_coherency = intel_iommu_enforce_cache_coherency,
 	}
 };
 
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index 2f9891cb3d0014..1f930c0c225d94 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -540,6 +540,7 @@ struct dmar_domain {
 	u8 has_iotlb_device: 1;
 	u8 iommu_coherency: 1;		/* indicate coherency of iommu access */
 	u8 iommu_snooping: 1;		/* indicate snooping control feature */
+	u8 enforce_no_snoop : 1;        /* Create IOPTEs with snoop control */
 
 	struct list_head devices;	/* all devices' list */
 	struct iova_domain iovad;	/* iova's that belong to this domain */
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 9208eca4b0d1ac..fe4f24c469c373 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -272,6 +272,9 @@ struct iommu_ops {
  * @iotlb_sync: Flush all queued ranges from the hardware TLBs and empty flush
  *            queue
  * @iova_to_phys: translate iova to physical address
+ * @enforce_cache_coherency: Prevent any kind of DMA from bypassing IOMMU_CACHE,
+ *                           including no-snoop TLPs on PCIe or other platform
+ *                           specific mechanisms.
  * @enable_nesting: Enable nesting
  * @set_pgtable_quirks: Set io page table quirks (IO_PGTABLE_QUIRK_*)
  * @free: Release the domain after use.
@@ -300,6 +303,7 @@ struct iommu_domain_ops {
 	phys_addr_t (*iova_to_phys)(struct iommu_domain *domain,
 				    dma_addr_t iova);
 
+	bool (*enforce_cache_coherency)(struct iommu_domain *domain);
 	int (*enable_nesting)(struct iommu_domain *domain);
 	int (*set_pgtable_quirks)(struct iommu_domain *domain,
 				  unsigned long quirks);
-- 
2.35.1

[PATCH v2 1/4] iommu: Introduce the domain op enforce_cache_coherency()

[Jason Gunthorpe via iommu]

This new mechanism will replace using IOMMU_CAP_CACHE_COHERENCY and
IOMMU_CACHE to control the no-snoop blocking behavior of the IOMMU.

Currently only Intel and AMD IOMMUs are known to support this
feature. They both implement it as an IOPTE bit, that when set, will cause
PCIe TLPs to that IOVA with the no-snoop bit set to be treated as though
the no-snoop bit was clear.

The new API is triggered by calling enforce_cache_coherency() before
mapping any IOVA to the domain which globally switches on no-snoop
blocking. This allows other implementations that might block no-snoop
globally and outside the IOPTE - AMD also documents such a HW capability.

Leave AMD out of sync with Intel and have it block no-snoop even for
in-kernel users. This can be trivially resolved in a follow up patch.

Only VFIO will call this new API.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/amd/iommu.c   |  7 +++++++
 drivers/iommu/intel/iommu.c | 14 +++++++++++++-
 include/linux/intel-iommu.h |  1 +
 include/linux/iommu.h       |  4 ++++
 4 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index a1ada7bff44e61..e500b487eb3429 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2271,6 +2271,12 @@ static int amd_iommu_def_domain_type(struct device *dev)
 	return 0;
 }
 
+static bool amd_iommu_enforce_cache_coherency(struct iommu_domain *domain)
+{
+	/* IOMMU_PTE_FC is always set */
+	return true;
+}
+
 const struct iommu_ops amd_iommu_ops = {
 	.capable = amd_iommu_capable,
 	.domain_alloc = amd_iommu_domain_alloc,
@@ -2293,6 +2299,7 @@ const struct iommu_ops amd_iommu_ops = {
 		.flush_iotlb_all = amd_iommu_flush_iotlb_all,
 		.iotlb_sync	= amd_iommu_iotlb_sync,
 		.free		= amd_iommu_domain_free,
+		.enforce_cache_coherency = amd_iommu_enforce_cache_coherency,
 	}
 };
 
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index df5c62ecf942b8..f08611a6cc4799 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4422,7 +4422,8 @@ static int intel_iommu_map(struct iommu_domain *domain,
 		prot |= DMA_PTE_READ;
 	if (iommu_prot & IOMMU_WRITE)
 		prot |= DMA_PTE_WRITE;
-	if ((iommu_prot & IOMMU_CACHE) && dmar_domain->iommu_snooping)
+	if (((iommu_prot & IOMMU_CACHE) && dmar_domain->iommu_snooping) ||
+	    dmar_domain->enforce_no_snoop)
 		prot |= DMA_PTE_SNP;
 
 	max_addr = iova + size;
@@ -4545,6 +4546,16 @@ static phys_addr_t intel_iommu_iova_to_phys(struct iommu_domain *domain,
 	return phys;
 }
 
+static bool intel_iommu_enforce_cache_coherency(struct iommu_domain *domain)
+{
+	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
+
+	if (!dmar_domain->iommu_snooping)
+		return false;
+	dmar_domain->enforce_no_snoop = true;
+	return true;
+}
+
 static bool intel_iommu_capable(enum iommu_cap cap)
 {
 	if (cap == IOMMU_CAP_CACHE_COHERENCY)
@@ -4898,6 +4909,7 @@ const struct iommu_ops intel_iommu_ops = {
 		.iotlb_sync		= intel_iommu_tlb_sync,
 		.iova_to_phys		= intel_iommu_iova_to_phys,
 		.free			= intel_iommu_domain_free,
+		.enforce_cache_coherency = intel_iommu_enforce_cache_coherency,
 	}
 };
 
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index 2f9891cb3d0014..1f930c0c225d94 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -540,6 +540,7 @@ struct dmar_domain {
 	u8 has_iotlb_device: 1;
 	u8 iommu_coherency: 1;		/* indicate coherency of iommu access */
 	u8 iommu_snooping: 1;		/* indicate snooping control feature */
+	u8 enforce_no_snoop : 1;        /* Create IOPTEs with snoop control */
 
 	struct list_head devices;	/* all devices' list */
 	struct iova_domain iovad;	/* iova's that belong to this domain */
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 9208eca4b0d1ac..fe4f24c469c373 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -272,6 +272,9 @@ struct iommu_ops {
  * @iotlb_sync: Flush all queued ranges from the hardware TLBs and empty flush
  *            queue
  * @iova_to_phys: translate iova to physical address
+ * @enforce_cache_coherency: Prevent any kind of DMA from bypassing IOMMU_CACHE,
+ *                           including no-snoop TLPs on PCIe or other platform
+ *                           specific mechanisms.
  * @enable_nesting: Enable nesting
  * @set_pgtable_quirks: Set io page table quirks (IO_PGTABLE_QUIRK_*)
  * @free: Release the domain after use.
@@ -300,6 +303,7 @@ struct iommu_domain_ops {
 	phys_addr_t (*iova_to_phys)(struct iommu_domain *domain,
 				    dma_addr_t iova);
 
+	bool (*enforce_cache_coherency)(struct iommu_domain *domain);
 	int (*enable_nesting)(struct iommu_domain *domain);
 	int (*set_pgtable_quirks)(struct iommu_domain *domain,
 				  unsigned long quirks);
-- 
2.35.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 2/4] vfio: Move the Intel no-snoop control off of IOMMU_CACHE

[Jason Gunthorpe]

IOMMU_CACHE means "normal DMA to this iommu_domain's IOVA should be cache
coherent" and is used by the DMA API. The definition allows for special
non-coherent DMA to exist - ie processing of the no-snoop flag in PCIe
TLPs - so long as this behavior is opt-in by the device driver.

The flag is mainly used by the DMA API to synchronize the IOMMU setting
with the expected cache behavior of the DMA master. eg based on
dev_is_dma_coherent() in some case.

For Intel IOMMU IOMMU_CACHE was redefined to mean 'force all DMA to be
cache coherent' which has the practical effect of causing the IOMMU to
ignore the no-snoop bit in a PCIe TLP.

x86 platforms are always IOMMU_CACHE, so Intel should ignore this flag.

Instead use the new domain op enforce_cache_coherency() which causes every
IOPTE created in the domain to have the no-snoop blocking behavior.

Reconfigure VFIO to always use IOMMU_CACHE and call
enforce_cache_coherency() to operate the special Intel behavior.

Remove the IOMMU_CACHE test from Intel IOMMU.

Ultimately VFIO plumbs the result of enforce_cache_coherency() back into
the x86 platform code through kvm_arch_register_noncoherent_dma() which
controls if the WBINVD instruction is available in the guest. No other
arch implements kvm_arch_register_noncoherent_dma().

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/intel/iommu.c     |  7 ++-----
 drivers/vfio/vfio_iommu_type1.c | 30 +++++++++++++++++++-----------
 include/linux/intel-iommu.h     |  1 -
 3 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index f08611a6cc4799..8f3674e997df06 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -641,7 +641,6 @@ static unsigned long domain_super_pgsize_bitmap(struct dmar_domain *domain)
 static void domain_update_iommu_cap(struct dmar_domain *domain)
 {
 	domain_update_iommu_coherency(domain);
-	domain->iommu_snooping = domain_update_iommu_snooping(NULL);
 	domain->iommu_superpage = domain_update_iommu_superpage(domain, NULL);
 
 	/*
@@ -4283,7 +4282,6 @@ static int md_domain_init(struct dmar_domain *domain, int guest_width)
 	domain->agaw = width_to_agaw(adjust_width);
 
 	domain->iommu_coherency = false;
-	domain->iommu_snooping = false;
 	domain->iommu_superpage = 0;
 	domain->max_addr = 0;
 
@@ -4422,8 +4420,7 @@ static int intel_iommu_map(struct iommu_domain *domain,
 		prot |= DMA_PTE_READ;
 	if (iommu_prot & IOMMU_WRITE)
 		prot |= DMA_PTE_WRITE;
-	if (((iommu_prot & IOMMU_CACHE) && dmar_domain->iommu_snooping) ||
-	    dmar_domain->enforce_no_snoop)
+	if (dmar_domain->enforce_no_snoop)
 		prot |= DMA_PTE_SNP;
 
 	max_addr = iova + size;
@@ -4550,7 +4547,7 @@ static bool intel_iommu_enforce_cache_coherency(struct iommu_domain *domain)
 {
 	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
 
-	if (!dmar_domain->iommu_snooping)
+	if (!domain_update_iommu_snooping(NULL))
 		return false;
 	dmar_domain->enforce_no_snoop = true;
 	return true;
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 9394aa9444c10c..c13b9290e35759 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -84,8 +84,8 @@ struct vfio_domain {
 	struct iommu_domain	*domain;
 	struct list_head	next;
 	struct list_head	group_list;
-	int			prot;		/* IOMMU_CACHE */
-	bool			fgsp;		/* Fine-grained super pages */
+	bool			fgsp : 1;	/* Fine-grained super pages */
+	bool			enforce_cache_coherency : 1;
 };
 
 struct vfio_dma {
@@ -1461,7 +1461,7 @@ static int vfio_iommu_map(struct vfio_iommu *iommu, dma_addr_t iova,
 
 	list_for_each_entry(d, &iommu->domain_list, next) {
 		ret = iommu_map(d->domain, iova, (phys_addr_t)pfn << PAGE_SHIFT,
-				npage << PAGE_SHIFT, prot | d->prot);
+				npage << PAGE_SHIFT, prot | IOMMU_CACHE);
 		if (ret)
 			goto unwind;
 
@@ -1771,7 +1771,7 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
 			}
 
 			ret = iommu_map(domain->domain, iova, phys,
-					size, dma->prot | domain->prot);
+					size, dma->prot | IOMMU_CACHE);
 			if (ret) {
 				if (!dma->iommu_mapped) {
 					vfio_unpin_pages_remote(dma, iova,
@@ -1859,7 +1859,7 @@ static void vfio_test_domain_fgsp(struct vfio_domain *domain)
 		return;
 
 	ret = iommu_map(domain->domain, 0, page_to_phys(pages), PAGE_SIZE * 2,
-			IOMMU_READ | IOMMU_WRITE | domain->prot);
+			IOMMU_READ | IOMMU_WRITE | IOMMU_CACHE);
 	if (!ret) {
 		size_t unmapped = iommu_unmap(domain->domain, 0, PAGE_SIZE);
 
@@ -2267,8 +2267,15 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 		goto out_detach;
 	}
 
-	if (iommu_capable(bus, IOMMU_CAP_CACHE_COHERENCY))
-		domain->prot |= IOMMU_CACHE;
+	/*
+	 * If the IOMMU can block non-coherent operations (ie PCIe TLPs with
+	 * no-snoop set) then VFIO always turns this feature on because on Intel
+	 * platforms it optimizes KVM to disable wbinvd emulation.
+	 */
+	if (domain->domain->ops->enforce_cache_coherency)
+		domain->enforce_cache_coherency =
+			domain->domain->ops->enforce_cache_coherency(
+				domain->domain);
 
 	/*
 	 * Try to match an existing compatible domain.  We don't want to
@@ -2279,7 +2286,8 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	 */
 	list_for_each_entry(d, &iommu->domain_list, next) {
 		if (d->domain->ops == domain->domain->ops &&
-		    d->prot == domain->prot) {
+		    d->enforce_cache_coherency ==
+			    domain->enforce_cache_coherency) {
 			iommu_detach_group(domain->domain, group->iommu_group);
 			if (!iommu_attach_group(d->domain,
 						group->iommu_group)) {
@@ -2611,14 +2619,14 @@ static void vfio_iommu_type1_release(void *iommu_data)
 	kfree(iommu);
 }
 
-static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
+static int vfio_domains_have_enforce_cache_coherency(struct vfio_iommu *iommu)
 {
 	struct vfio_domain *domain;
 	int ret = 1;
 
 	mutex_lock(&iommu->lock);
 	list_for_each_entry(domain, &iommu->domain_list, next) {
-		if (!(domain->prot & IOMMU_CACHE)) {
+		if (!(domain->enforce_cache_coherency)) {
 			ret = 0;
 			break;
 		}
@@ -2641,7 +2649,7 @@ static int vfio_iommu_type1_check_extension(struct vfio_iommu *iommu,
 	case VFIO_DMA_CC_IOMMU:
 		if (!iommu)
 			return 0;
-		return vfio_domains_have_iommu_cache(iommu);
+		return vfio_domains_have_enforce_cache_coherency(iommu);
 	default:
 		return 0;
 	}
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index 1f930c0c225d94..bc39f633efdf03 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -539,7 +539,6 @@ struct dmar_domain {
 
 	u8 has_iotlb_device: 1;
 	u8 iommu_coherency: 1;		/* indicate coherency of iommu access */
-	u8 iommu_snooping: 1;		/* indicate snooping control feature */
 	u8 enforce_no_snoop : 1;        /* Create IOPTEs with snoop control */
 
 	struct list_head devices;	/* all devices' list */
-- 
2.35.1

[PATCH v2 2/4] vfio: Move the Intel no-snoop control off of IOMMU_CACHE

[Jason Gunthorpe via iommu]

IOMMU_CACHE means "normal DMA to this iommu_domain's IOVA should be cache
coherent" and is used by the DMA API. The definition allows for special
non-coherent DMA to exist - ie processing of the no-snoop flag in PCIe
TLPs - so long as this behavior is opt-in by the device driver.

The flag is mainly used by the DMA API to synchronize the IOMMU setting
with the expected cache behavior of the DMA master. eg based on
dev_is_dma_coherent() in some case.

For Intel IOMMU IOMMU_CACHE was redefined to mean 'force all DMA to be
cache coherent' which has the practical effect of causing the IOMMU to
ignore the no-snoop bit in a PCIe TLP.

x86 platforms are always IOMMU_CACHE, so Intel should ignore this flag.

Instead use the new domain op enforce_cache_coherency() which causes every
IOPTE created in the domain to have the no-snoop blocking behavior.

Reconfigure VFIO to always use IOMMU_CACHE and call
enforce_cache_coherency() to operate the special Intel behavior.

Remove the IOMMU_CACHE test from Intel IOMMU.

Ultimately VFIO plumbs the result of enforce_cache_coherency() back into
the x86 platform code through kvm_arch_register_noncoherent_dma() which
controls if the WBINVD instruction is available in the guest. No other
arch implements kvm_arch_register_noncoherent_dma().

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/intel/iommu.c     |  7 ++-----
 drivers/vfio/vfio_iommu_type1.c | 30 +++++++++++++++++++-----------
 include/linux/intel-iommu.h     |  1 -
 3 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index f08611a6cc4799..8f3674e997df06 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -641,7 +641,6 @@ static unsigned long domain_super_pgsize_bitmap(struct dmar_domain *domain)
 static void domain_update_iommu_cap(struct dmar_domain *domain)
 {
 	domain_update_iommu_coherency(domain);
-	domain->iommu_snooping = domain_update_iommu_snooping(NULL);
 	domain->iommu_superpage = domain_update_iommu_superpage(domain, NULL);
 
 	/*
@@ -4283,7 +4282,6 @@ static int md_domain_init(struct dmar_domain *domain, int guest_width)
 	domain->agaw = width_to_agaw(adjust_width);
 
 	domain->iommu_coherency = false;
-	domain->iommu_snooping = false;
 	domain->iommu_superpage = 0;
 	domain->max_addr = 0;
 
@@ -4422,8 +4420,7 @@ static int intel_iommu_map(struct iommu_domain *domain,
 		prot |= DMA_PTE_READ;
 	if (iommu_prot & IOMMU_WRITE)
 		prot |= DMA_PTE_WRITE;
-	if (((iommu_prot & IOMMU_CACHE) && dmar_domain->iommu_snooping) ||
-	    dmar_domain->enforce_no_snoop)
+	if (dmar_domain->enforce_no_snoop)
 		prot |= DMA_PTE_SNP;
 
 	max_addr = iova + size;
@@ -4550,7 +4547,7 @@ static bool intel_iommu_enforce_cache_coherency(struct iommu_domain *domain)
 {
 	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
 
-	if (!dmar_domain->iommu_snooping)
+	if (!domain_update_iommu_snooping(NULL))
 		return false;
 	dmar_domain->enforce_no_snoop = true;
 	return true;
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 9394aa9444c10c..c13b9290e35759 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -84,8 +84,8 @@ struct vfio_domain {
 	struct iommu_domain	*domain;
 	struct list_head	next;
 	struct list_head	group_list;
-	int			prot;		/* IOMMU_CACHE */
-	bool			fgsp;		/* Fine-grained super pages */
+	bool			fgsp : 1;	/* Fine-grained super pages */
+	bool			enforce_cache_coherency : 1;
 };
 
 struct vfio_dma {
@@ -1461,7 +1461,7 @@ static int vfio_iommu_map(struct vfio_iommu *iommu, dma_addr_t iova,
 
 	list_for_each_entry(d, &iommu->domain_list, next) {
 		ret = iommu_map(d->domain, iova, (phys_addr_t)pfn << PAGE_SHIFT,
-				npage << PAGE_SHIFT, prot | d->prot);
+				npage << PAGE_SHIFT, prot | IOMMU_CACHE);
 		if (ret)
 			goto unwind;
 
@@ -1771,7 +1771,7 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
 			}
 
 			ret = iommu_map(domain->domain, iova, phys,
-					size, dma->prot | domain->prot);
+					size, dma->prot | IOMMU_CACHE);
 			if (ret) {
 				if (!dma->iommu_mapped) {
 					vfio_unpin_pages_remote(dma, iova,
@@ -1859,7 +1859,7 @@ static void vfio_test_domain_fgsp(struct vfio_domain *domain)
 		return;
 
 	ret = iommu_map(domain->domain, 0, page_to_phys(pages), PAGE_SIZE * 2,
-			IOMMU_READ | IOMMU_WRITE | domain->prot);
+			IOMMU_READ | IOMMU_WRITE | IOMMU_CACHE);
 	if (!ret) {
 		size_t unmapped = iommu_unmap(domain->domain, 0, PAGE_SIZE);
 
@@ -2267,8 +2267,15 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 		goto out_detach;
 	}
 
-	if (iommu_capable(bus, IOMMU_CAP_CACHE_COHERENCY))
-		domain->prot |= IOMMU_CACHE;
+	/*
+	 * If the IOMMU can block non-coherent operations (ie PCIe TLPs with
+	 * no-snoop set) then VFIO always turns this feature on because on Intel
+	 * platforms it optimizes KVM to disable wbinvd emulation.
+	 */
+	if (domain->domain->ops->enforce_cache_coherency)
+		domain->enforce_cache_coherency =
+			domain->domain->ops->enforce_cache_coherency(
+				domain->domain);
 
 	/*
 	 * Try to match an existing compatible domain.  We don't want to
@@ -2279,7 +2286,8 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	 */
 	list_for_each_entry(d, &iommu->domain_list, next) {
 		if (d->domain->ops == domain->domain->ops &&
-		    d->prot == domain->prot) {
+		    d->enforce_cache_coherency ==
+			    domain->enforce_cache_coherency) {
 			iommu_detach_group(domain->domain, group->iommu_group);
 			if (!iommu_attach_group(d->domain,
 						group->iommu_group)) {
@@ -2611,14 +2619,14 @@ static void vfio_iommu_type1_release(void *iommu_data)
 	kfree(iommu);
 }
 
-static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
+static int vfio_domains_have_enforce_cache_coherency(struct vfio_iommu *iommu)
 {
 	struct vfio_domain *domain;
 	int ret = 1;
 
 	mutex_lock(&iommu->lock);
 	list_for_each_entry(domain, &iommu->domain_list, next) {
-		if (!(domain->prot & IOMMU_CACHE)) {
+		if (!(domain->enforce_cache_coherency)) {
 			ret = 0;
 			break;
 		}
@@ -2641,7 +2649,7 @@ static int vfio_iommu_type1_check_extension(struct vfio_iommu *iommu,
 	case VFIO_DMA_CC_IOMMU:
 		if (!iommu)
 			return 0;
-		return vfio_domains_have_iommu_cache(iommu);
+		return vfio_domains_have_enforce_cache_coherency(iommu);
 	default:
 		return 0;
 	}
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index 1f930c0c225d94..bc39f633efdf03 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -539,7 +539,6 @@ struct dmar_domain {
 
 	u8 has_iotlb_device: 1;
 	u8 iommu_coherency: 1;		/* indicate coherency of iommu access */
-	u8 iommu_snooping: 1;		/* indicate snooping control feature */
 	u8 enforce_no_snoop : 1;        /* Create IOPTEs with snoop control */
 
 	struct list_head devices;	/* all devices' list */
-- 
2.35.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 3/4] iommu: Redefine IOMMU_CAP_CACHE_COHERENCY as the cap flag for IOMMU_CACHE

[Jason Gunthorpe]

While the comment was correct that this flag was intended to convey the
block no-snoop support in the IOMMU, it has become widely implemented and
used to mean the IOMMU supports IOMMU_CACHE as a map flag. Only the Intel
driver was different.

Now that the Intel driver is using enforce_cache_coherency() update the
comment to make it clear that IOMMU_CAP_CACHE_COHERENCY is only about
IOMMU_CACHE.  Fix the Intel driver to return true since IOMMU_CACHE always
works.

The two places that test this flag, usnic and vdpa, are both assigning
userspace pages to a driver controlled iommu_domain and require
IOMMU_CACHE behavior as they offer no way for userspace to synchronize
caches.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/intel/iommu.c | 2 +-
 include/linux/iommu.h       | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 8f3674e997df06..14ba185175e9ec 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4556,7 +4556,7 @@ static bool intel_iommu_enforce_cache_coherency(struct iommu_domain *domain)
 static bool intel_iommu_capable(enum iommu_cap cap)
 {
 	if (cap == IOMMU_CAP_CACHE_COHERENCY)
-		return domain_update_iommu_snooping(NULL);
+		return true;
 	if (cap == IOMMU_CAP_INTR_REMAP)
 		return irq_remapping_enabled == 1;
 
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index fe4f24c469c373..fd58f7adc52796 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -103,8 +103,7 @@ static inline bool iommu_is_dma_domain(struct iommu_domain *domain)
 }
 
 enum iommu_cap {
-	IOMMU_CAP_CACHE_COHERENCY,	/* IOMMU can enforce cache coherent DMA
-					   transactions */
+	IOMMU_CAP_CACHE_COHERENCY,	/* IOMMU_CACHE is supported */
 	IOMMU_CAP_INTR_REMAP,		/* IOMMU supports interrupt isolation */
 	IOMMU_CAP_NOEXEC,		/* IOMMU_NOEXEC flag */
 };
-- 
2.35.1

[PATCH v2 3/4] iommu: Redefine IOMMU_CAP_CACHE_COHERENCY as the cap flag for IOMMU_CACHE

[Jason Gunthorpe via iommu]

While the comment was correct that this flag was intended to convey the
block no-snoop support in the IOMMU, it has become widely implemented and
used to mean the IOMMU supports IOMMU_CACHE as a map flag. Only the Intel
driver was different.

Now that the Intel driver is using enforce_cache_coherency() update the
comment to make it clear that IOMMU_CAP_CACHE_COHERENCY is only about
IOMMU_CACHE.  Fix the Intel driver to return true since IOMMU_CACHE always
works.

The two places that test this flag, usnic and vdpa, are both assigning
userspace pages to a driver controlled iommu_domain and require
IOMMU_CACHE behavior as they offer no way for userspace to synchronize
caches.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/intel/iommu.c | 2 +-
 include/linux/iommu.h       | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 8f3674e997df06..14ba185175e9ec 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4556,7 +4556,7 @@ static bool intel_iommu_enforce_cache_coherency(struct iommu_domain *domain)
 static bool intel_iommu_capable(enum iommu_cap cap)
 {
 	if (cap == IOMMU_CAP_CACHE_COHERENCY)
-		return domain_update_iommu_snooping(NULL);
+		return true;
 	if (cap == IOMMU_CAP_INTR_REMAP)
 		return irq_remapping_enabled == 1;
 
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index fe4f24c469c373..fd58f7adc52796 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -103,8 +103,7 @@ static inline bool iommu_is_dma_domain(struct iommu_domain *domain)
 }
 
 enum iommu_cap {
-	IOMMU_CAP_CACHE_COHERENCY,	/* IOMMU can enforce cache coherent DMA
-					   transactions */
+	IOMMU_CAP_CACHE_COHERENCY,	/* IOMMU_CACHE is supported */
 	IOMMU_CAP_INTR_REMAP,		/* IOMMU supports interrupt isolation */
 	IOMMU_CAP_NOEXEC,		/* IOMMU_NOEXEC flag */
 };
-- 
2.35.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 4/4] vfio: Require that devices support DMA cache coherence

[Jason Gunthorpe]

IOMMU_CACHE means that normal DMAs do not require any additional coherency
mechanism and is the basic uAPI that VFIO exposes to userspace. For
instance VFIO applications like DPDK will not work if additional coherency
operations are required.

Therefore check IOMMU_CAP_CACHE_COHERENCY like vdpa & usnic do before
allowing an IOMMU backed VFIO device to be created.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/vfio.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index a4555014bd1e72..9edad767cfdad3 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -815,6 +815,13 @@ static int __vfio_register_dev(struct vfio_device *device,
 
 int vfio_register_group_dev(struct vfio_device *device)
 {
+	/*
+	 * VFIO always sets IOMMU_CACHE because we offer no way for userspace to
+	 * restore cache coherency.
+	 */
+	if (!iommu_capable(device->dev->bus, IOMMU_CAP_CACHE_COHERENCY))
+		return -EINVAL;
+
 	return __vfio_register_dev(device,
 		vfio_group_find_or_alloc(device->dev));
 }
-- 
2.35.1

[PATCH v2 4/4] vfio: Require that devices support DMA cache coherence

[Jason Gunthorpe via iommu]

IOMMU_CACHE means that normal DMAs do not require any additional coherency
mechanism and is the basic uAPI that VFIO exposes to userspace. For
instance VFIO applications like DPDK will not work if additional coherency
operations are required.

Therefore check IOMMU_CAP_CACHE_COHERENCY like vdpa & usnic do before
allowing an IOMMU backed VFIO device to be created.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/vfio.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index a4555014bd1e72..9edad767cfdad3 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -815,6 +815,13 @@ static int __vfio_register_dev(struct vfio_device *device,
 
 int vfio_register_group_dev(struct vfio_device *device)
 {
+	/*
+	 * VFIO always sets IOMMU_CACHE because we offer no way for userspace to
+	 * restore cache coherency.
+	 */
+	if (!iommu_capable(device->dev->bus, IOMMU_CAP_CACHE_COHERENCY))
+		return -EINVAL;
+
 	return __vfio_register_dev(device,
 		vfio_group_find_or_alloc(device->dev));
 }
-- 
2.35.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Robin Murphy]

On 2022-04-07 16:23, Jason Gunthorpe wrote:
> PCIe defines a 'no-snoop' bit in each the TLP which is usually implemented
> by a platform as bypassing elements in the DMA coherent CPU cache
> hierarchy. A driver can command a device to set this bit on some of its
> transactions as a micro-optimization.
> 
> However, the driver is now responsible to synchronize the CPU cache with
> the DMA that bypassed it. On x86 this may be done through the wbinvd
> instruction, and the i915 GPU driver is the only Linux DMA driver that
> calls it.
> 
> The problem comes that KVM on x86 will normally disable the wbinvd
> instruction in the guest and render it a NOP. As the driver running in the
> guest is not aware the wbinvd doesn't work it may still cause the device
> to set the no-snoop bit and the platform will bypass the CPU cache.
> Without a working wbinvd there is no way to re-synchronize the CPU cache
> and the driver in the VM has data corruption.
> 
> Thus, we see a general direction on x86 that the IOMMU HW is able to block
> the no-snoop bit in the TLP. This NOP's the optimization and allows KVM to
> to NOP the wbinvd without causing any data corruption.
> 
> This control for Intel IOMMU was exposed by using IOMMU_CACHE and
> IOMMU_CAP_CACHE_COHERENCY, however these two values now have multiple
> meanings and usages beyond blocking no-snoop and the whole thing has
> become confused. AMD IOMMU has the same feature and same IOPTE bits
> however it unconditionally blocks no-snoop.
> 
> Change it so that:
>   - IOMMU_CACHE is only about the DMA coherence of normal DMAs from a
>     device. It is used by the DMA API/VFIO/etc when the user of the
>     iommu_domain will not be doing manual cache coherency operations.
> 
>   - IOMMU_CAP_CACHE_COHERENCY indicates if IOMMU_CACHE can be used with the
>     device.
> 
>   - The new optional domain op enforce_cache_coherency() will cause the
>     entire domain to block no-snoop requests - ie there is no way for any
>     device attached to the domain to opt out of the IOMMU_CACHE behavior.
>     This is permanent on the domain and must apply to any future devices
>     attached to it.
> 
> Ideally an iommu driver should implement enforce_cache_coherency() so that
> by DMA API domains allow the no-snoop optimization. This leaves it
> available to kernel drivers like i915. VFIO will call
> enforce_cache_coherency() before establishing any mappings and the domain
> should then permanently block no-snoop.
> 
> If enforce_cache_coherency() fails VFIO will communicate back through to
> KVM into the arch code via kvm_arch_register_noncoherent_dma()
> (only implemented by x86) which triggers a working wbinvd to be made
> available to the VM.
> 
> While other iommu drivers are certainly welcome to implement
> enforce_cache_coherency(), it is not clear there is any benefit in doing
> so right now.
> 
> This is on github: https://github.com/jgunthorpe/linux/commits/intel_no_snoop
> 
> v2:
>   - Abandon removing IOMMU_CAP_CACHE_COHERENCY - instead make it the cap
>     flag that indicates IOMMU_CACHE is supported
>   - Put the VFIO tests for IOMMU_CACHE at VFIO device registration
>   - In the Intel driver remove the domain->iommu_snooping value, this is
>     global not per-domain

At a glance, this all looks about the right shape to me now, thanks!

Ideally I'd hope patch #4 could go straight to device_iommu_capable() 
from my Thunderbolt series, but we can figure that out in a couple of 
weeks once Joerg starts queueing 5.19 material. I've got another VFIO 
patch waiting for the DMA ownership series to land anyway, so it's 
hardly the end of the world if I have to come back to follow up on this 
one too.

For the series,

Acked-by: Robin Murphy <robin.murphy@arm.com>

> v1: https://lore.kernel.org/r/0-v1-ef02c60ddb76+12ca2-intel_no_snoop_jgg@nvidia.com
> 
> Jason Gunthorpe (4):
>    iommu: Introduce the domain op enforce_cache_coherency()
>    vfio: Move the Intel no-snoop control off of IOMMU_CACHE
>    iommu: Redefine IOMMU_CAP_CACHE_COHERENCY as the cap flag for
>      IOMMU_CACHE
>    vfio: Require that devices support DMA cache coherence
> 
>   drivers/iommu/amd/iommu.c       |  7 +++++++
>   drivers/iommu/intel/iommu.c     | 17 +++++++++++++----
>   drivers/vfio/vfio.c             |  7 +++++++
>   drivers/vfio/vfio_iommu_type1.c | 30 +++++++++++++++++++-----------
>   include/linux/intel-iommu.h     |  2 +-
>   include/linux/iommu.h           |  7 +++++--
>   6 files changed, 52 insertions(+), 18 deletions(-)
> 
> 
> base-commit: 3123109284176b1532874591f7c81f3837bbdc17

Re: [PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Robin Murphy]

On 2022-04-07 16:23, Jason Gunthorpe wrote:
> PCIe defines a 'no-snoop' bit in each the TLP which is usually implemented
> by a platform as bypassing elements in the DMA coherent CPU cache
> hierarchy. A driver can command a device to set this bit on some of its
> transactions as a micro-optimization.
> 
> However, the driver is now responsible to synchronize the CPU cache with
> the DMA that bypassed it. On x86 this may be done through the wbinvd
> instruction, and the i915 GPU driver is the only Linux DMA driver that
> calls it.
> 
> The problem comes that KVM on x86 will normally disable the wbinvd
> instruction in the guest and render it a NOP. As the driver running in the
> guest is not aware the wbinvd doesn't work it may still cause the device
> to set the no-snoop bit and the platform will bypass the CPU cache.
> Without a working wbinvd there is no way to re-synchronize the CPU cache
> and the driver in the VM has data corruption.
> 
> Thus, we see a general direction on x86 that the IOMMU HW is able to block
> the no-snoop bit in the TLP. This NOP's the optimization and allows KVM to
> to NOP the wbinvd without causing any data corruption.
> 
> This control for Intel IOMMU was exposed by using IOMMU_CACHE and
> IOMMU_CAP_CACHE_COHERENCY, however these two values now have multiple
> meanings and usages beyond blocking no-snoop and the whole thing has
> become confused. AMD IOMMU has the same feature and same IOPTE bits
> however it unconditionally blocks no-snoop.
> 
> Change it so that:
>   - IOMMU_CACHE is only about the DMA coherence of normal DMAs from a
>     device. It is used by the DMA API/VFIO/etc when the user of the
>     iommu_domain will not be doing manual cache coherency operations.
> 
>   - IOMMU_CAP_CACHE_COHERENCY indicates if IOMMU_CACHE can be used with the
>     device.
> 
>   - The new optional domain op enforce_cache_coherency() will cause the
>     entire domain to block no-snoop requests - ie there is no way for any
>     device attached to the domain to opt out of the IOMMU_CACHE behavior.
>     This is permanent on the domain and must apply to any future devices
>     attached to it.
> 
> Ideally an iommu driver should implement enforce_cache_coherency() so that
> by DMA API domains allow the no-snoop optimization. This leaves it
> available to kernel drivers like i915. VFIO will call
> enforce_cache_coherency() before establishing any mappings and the domain
> should then permanently block no-snoop.
> 
> If enforce_cache_coherency() fails VFIO will communicate back through to
> KVM into the arch code via kvm_arch_register_noncoherent_dma()
> (only implemented by x86) which triggers a working wbinvd to be made
> available to the VM.
> 
> While other iommu drivers are certainly welcome to implement
> enforce_cache_coherency(), it is not clear there is any benefit in doing
> so right now.
> 
> This is on github: https://github.com/jgunthorpe/linux/commits/intel_no_snoop
> 
> v2:
>   - Abandon removing IOMMU_CAP_CACHE_COHERENCY - instead make it the cap
>     flag that indicates IOMMU_CACHE is supported
>   - Put the VFIO tests for IOMMU_CACHE at VFIO device registration
>   - In the Intel driver remove the domain->iommu_snooping value, this is
>     global not per-domain

At a glance, this all looks about the right shape to me now, thanks!

Ideally I'd hope patch #4 could go straight to device_iommu_capable() 
from my Thunderbolt series, but we can figure that out in a couple of 
weeks once Joerg starts queueing 5.19 material. I've got another VFIO 
patch waiting for the DMA ownership series to land anyway, so it's 
hardly the end of the world if I have to come back to follow up on this 
one too.

For the series,

Acked-by: Robin Murphy <robin.murphy@arm.com>

> v1: https://lore.kernel.org/r/0-v1-ef02c60ddb76+12ca2-intel_no_snoop_jgg@nvidia.com
> 
> Jason Gunthorpe (4):
>    iommu: Introduce the domain op enforce_cache_coherency()
>    vfio: Move the Intel no-snoop control off of IOMMU_CACHE
>    iommu: Redefine IOMMU_CAP_CACHE_COHERENCY as the cap flag for
>      IOMMU_CACHE
>    vfio: Require that devices support DMA cache coherence
> 
>   drivers/iommu/amd/iommu.c       |  7 +++++++
>   drivers/iommu/intel/iommu.c     | 17 +++++++++++++----
>   drivers/vfio/vfio.c             |  7 +++++++
>   drivers/vfio/vfio_iommu_type1.c | 30 +++++++++++++++++++-----------
>   include/linux/intel-iommu.h     |  2 +-
>   include/linux/iommu.h           |  7 +++++--
>   6 files changed, 52 insertions(+), 18 deletions(-)
> 
> 
> base-commit: 3123109284176b1532874591f7c81f3837bbdc17
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Jason Gunthorpe]

On Thu, Apr 07, 2022 at 06:03:37PM +0100, Robin Murphy wrote:
> At a glance, this all looks about the right shape to me now, thanks!

Thanks!

> Ideally I'd hope patch #4 could go straight to device_iommu_capable() from
> my Thunderbolt series, but we can figure that out in a couple of weeks once

Yes, this does helps that because now the only iommu_capable call is
in a context where a device is available :)

> Joerg starts queueing 5.19 material. I've got another VFIO patch waiting for
> the DMA ownership series to land anyway, so it's hardly the end of the world
> if I have to come back to follow up on this one too.

Hopefully Joerg will start soon, I also have patches written waiting
for the DMA ownership series.

Jason

Re: [PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Jason Gunthorpe via iommu]

On Thu, Apr 07, 2022 at 06:03:37PM +0100, Robin Murphy wrote:
> At a glance, this all looks about the right shape to me now, thanks!

Thanks!

> Ideally I'd hope patch #4 could go straight to device_iommu_capable() from
> my Thunderbolt series, but we can figure that out in a couple of weeks once

Yes, this does helps that because now the only iommu_capable call is
in a context where a device is available :)

> Joerg starts queueing 5.19 material. I've got another VFIO patch waiting for
> the DMA ownership series to land anyway, so it's hardly the end of the world
> if I have to come back to follow up on this one too.

Hopefully Joerg will start soon, I also have patches written waiting
for the DMA ownership series.

Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Robin Murphy]

On 2022-04-07 18:43, Jason Gunthorpe wrote:
> On Thu, Apr 07, 2022 at 06:03:37PM +0100, Robin Murphy wrote:
>> At a glance, this all looks about the right shape to me now, thanks!
> 
> Thanks!
> 
>> Ideally I'd hope patch #4 could go straight to device_iommu_capable() from
>> my Thunderbolt series, but we can figure that out in a couple of weeks once
> 
> Yes, this does helps that because now the only iommu_capable call is
> in a context where a device is available :)

Derp, of course I have *two* VFIO patches waiting, the other one 
touching the iommu_capable() calls (there's still IOMMU_CAP_INTR_REMAP, 
which, much as I hate it and would love to boot all that stuff over to 
drivers/irqchip, it's not in my way so I'm leaving it be for now). I'll 
have to rebase that anyway, so merging this as-is is absolutely fine!

Cheers,
Robin.

Re: [PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Robin Murphy]

On 2022-04-07 18:43, Jason Gunthorpe wrote:
> On Thu, Apr 07, 2022 at 06:03:37PM +0100, Robin Murphy wrote:
>> At a glance, this all looks about the right shape to me now, thanks!
> 
> Thanks!
> 
>> Ideally I'd hope patch #4 could go straight to device_iommu_capable() from
>> my Thunderbolt series, but we can figure that out in a couple of weeks once
> 
> Yes, this does helps that because now the only iommu_capable call is
> in a context where a device is available :)

Derp, of course I have *two* VFIO patches waiting, the other one 
touching the iommu_capable() calls (there's still IOMMU_CAP_INTR_REMAP, 
which, much as I hate it and would love to boot all that stuff over to 
drivers/irqchip, it's not in my way so I'm leaving it be for now). I'll 
have to rebase that anyway, so merging this as-is is absolutely fine!

Cheers,
Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Jason Gunthorpe]

On Thu, Apr 07, 2022 at 07:02:03PM +0100, Robin Murphy wrote:
> On 2022-04-07 18:43, Jason Gunthorpe wrote:
> > On Thu, Apr 07, 2022 at 06:03:37PM +0100, Robin Murphy wrote:
> > > At a glance, this all looks about the right shape to me now, thanks!
> > 
> > Thanks!
> > 
> > > Ideally I'd hope patch #4 could go straight to device_iommu_capable() from
> > > my Thunderbolt series, but we can figure that out in a couple of weeks once
> > 
> > Yes, this does helps that because now the only iommu_capable call is
> > in a context where a device is available :)
> 
> Derp, of course I have *two* VFIO patches waiting, the other one touching
> the iommu_capable() calls (there's still IOMMU_CAP_INTR_REMAP, which, much
> as I hate it and would love to boot all that stuff over to
> drivers/irqchip,

Oh me too...

> it's not in my way so I'm leaving it be for now). I'll have to rebase that
> anyway, so merging this as-is is absolutely fine!

This might help your effort - after this series and this below there
are no 'bus' users of iommu_capable left at all.

From 55d72be40bc0a031711e318c8dd1cb60673d9eca Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <jgg@nvidia.com>
Date: Thu, 7 Apr 2022 16:00:50 -0300
Subject: [PATCH] vfio: Move the IOMMU_CAP_INTR_REMAP to a context with a
 struct device

This check is done to ensure that the device we want to use is fully
isolated and the platform does not allow the device's MemWr TLPs to
trigger unauthorized MSIs.

Instead of doing it in the container context where we only have a group,
move the check to open_device where we actually know the device.

This is still security safe as userspace cannot trigger an MemWr TLPs
without obtaining a device fd.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/vfio.c             |  9 +++++++++
 drivers/vfio/vfio.h             |  1 +
 drivers/vfio/vfio_iommu_type1.c | 28 +++++++++++++++++-----------
 3 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 9edad767cfdad3..8db5cea1dc1d75 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -1355,6 +1355,15 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
 	if (IS_ERR(device))
 		return PTR_ERR(device);
 
+	/* Confirm this device is compatible with the container */
+	if (group->type == VFIO_IOMMU &&
+	    group->container->iommu_driver->ops->device_ok) {
+		ret = group->container->iommu_driver->ops->device_ok(
+			group->container->iommu_data, device->dev);
+		if (ret)
+			goto err_device_put;
+	}
+
 	if (!try_module_get(device->dev->driver->owner)) {
 		ret = -ENODEV;
 		goto err_device_put;
diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
index a6713022115155..3db60de71d42eb 100644
--- a/drivers/vfio/vfio.h
+++ b/drivers/vfio/vfio.h
@@ -66,6 +66,7 @@ struct vfio_iommu_driver_ops {
 						   struct iommu_group *group);
 	void		(*notify)(void *iommu_data,
 				  enum vfio_iommu_notify_type event);
+	int		(*device_ok)(void *iommu_data, struct device *device);
 };
 
 int vfio_register_iommu_driver(const struct vfio_iommu_driver_ops *ops);
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index c13b9290e35759..5e966fb0ab9202 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -2153,6 +2153,21 @@ static void vfio_iommu_iova_insert_copy(struct vfio_iommu *iommu,
 	list_splice_tail(iova_copy, iova);
 }
 
+static int vfio_iommu_device_ok(void *iommu_data, struct device *device)
+{
+	bool msi_remap;
+
+	msi_remap = irq_domain_check_msi_remap() ||
+		    iommu_capable(device->bus, IOMMU_CAP_INTR_REMAP);
+
+	if (!allow_unsafe_interrupts && !msi_remap) {
+		pr_warn("%s: No interrupt remapping support.  Use the module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this platform\n",
+			__func__);
+		return -EPERM;
+	}
+	return 0;
+}
+
 static int vfio_iommu_type1_attach_group(void *iommu_data,
 		struct iommu_group *iommu_group, enum vfio_group_type type)
 {
@@ -2160,7 +2175,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	struct vfio_iommu_group *group;
 	struct vfio_domain *domain, *d;
 	struct bus_type *bus = NULL;
-	bool resv_msi, msi_remap;
+	bool resv_msi;
 	phys_addr_t resv_msi_base = 0;
 	struct iommu_domain_geometry *geo;
 	LIST_HEAD(iova_copy);
@@ -2257,16 +2272,6 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	INIT_LIST_HEAD(&domain->group_list);
 	list_add(&group->next, &domain->group_list);
 
-	msi_remap = irq_domain_check_msi_remap() ||
-		    iommu_capable(bus, IOMMU_CAP_INTR_REMAP);
-
-	if (!allow_unsafe_interrupts && !msi_remap) {
-		pr_warn("%s: No interrupt remapping support.  Use the module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this platform\n",
-		       __func__);
-		ret = -EPERM;
-		goto out_detach;
-	}
-
 	/*
 	 * If the IOMMU can block non-coherent operations (ie PCIe TLPs with
 	 * no-snoop set) then VFIO always turns this feature on because on Intel
@@ -3159,6 +3164,7 @@ static const struct vfio_iommu_driver_ops vfio_iommu_driver_ops_type1 = {
 	.open			= vfio_iommu_type1_open,
 	.release		= vfio_iommu_type1_release,
 	.ioctl			= vfio_iommu_type1_ioctl,
+	.device_ok		= vfio_iommu_device_ok,
 	.attach_group		= vfio_iommu_type1_attach_group,
 	.detach_group		= vfio_iommu_type1_detach_group,
 	.pin_pages		= vfio_iommu_type1_pin_pages,
-- 
2.35.1

Re: [PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Jason Gunthorpe via iommu]

On Thu, Apr 07, 2022 at 07:02:03PM +0100, Robin Murphy wrote:
> On 2022-04-07 18:43, Jason Gunthorpe wrote:
> > On Thu, Apr 07, 2022 at 06:03:37PM +0100, Robin Murphy wrote:
> > > At a glance, this all looks about the right shape to me now, thanks!
> > 
> > Thanks!
> > 
> > > Ideally I'd hope patch #4 could go straight to device_iommu_capable() from
> > > my Thunderbolt series, but we can figure that out in a couple of weeks once
> > 
> > Yes, this does helps that because now the only iommu_capable call is
> > in a context where a device is available :)
> 
> Derp, of course I have *two* VFIO patches waiting, the other one touching
> the iommu_capable() calls (there's still IOMMU_CAP_INTR_REMAP, which, much
> as I hate it and would love to boot all that stuff over to
> drivers/irqchip,

Oh me too...

> it's not in my way so I'm leaving it be for now). I'll have to rebase that
> anyway, so merging this as-is is absolutely fine!

This might help your effort - after this series and this below there
are no 'bus' users of iommu_capable left at all.

From 55d72be40bc0a031711e318c8dd1cb60673d9eca Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <jgg@nvidia.com>
Date: Thu, 7 Apr 2022 16:00:50 -0300
Subject: [PATCH] vfio: Move the IOMMU_CAP_INTR_REMAP to a context with a
 struct device

This check is done to ensure that the device we want to use is fully
isolated and the platform does not allow the device's MemWr TLPs to
trigger unauthorized MSIs.

Instead of doing it in the container context where we only have a group,
move the check to open_device where we actually know the device.

This is still security safe as userspace cannot trigger an MemWr TLPs
without obtaining a device fd.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/vfio.c             |  9 +++++++++
 drivers/vfio/vfio.h             |  1 +
 drivers/vfio/vfio_iommu_type1.c | 28 +++++++++++++++++-----------
 3 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 9edad767cfdad3..8db5cea1dc1d75 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -1355,6 +1355,15 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
 	if (IS_ERR(device))
 		return PTR_ERR(device);
 
+	/* Confirm this device is compatible with the container */
+	if (group->type == VFIO_IOMMU &&
+	    group->container->iommu_driver->ops->device_ok) {
+		ret = group->container->iommu_driver->ops->device_ok(
+			group->container->iommu_data, device->dev);
+		if (ret)
+			goto err_device_put;
+	}
+
 	if (!try_module_get(device->dev->driver->owner)) {
 		ret = -ENODEV;
 		goto err_device_put;
diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
index a6713022115155..3db60de71d42eb 100644
--- a/drivers/vfio/vfio.h
+++ b/drivers/vfio/vfio.h
@@ -66,6 +66,7 @@ struct vfio_iommu_driver_ops {
 						   struct iommu_group *group);
 	void		(*notify)(void *iommu_data,
 				  enum vfio_iommu_notify_type event);
+	int		(*device_ok)(void *iommu_data, struct device *device);
 };
 
 int vfio_register_iommu_driver(const struct vfio_iommu_driver_ops *ops);
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index c13b9290e35759..5e966fb0ab9202 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -2153,6 +2153,21 @@ static void vfio_iommu_iova_insert_copy(struct vfio_iommu *iommu,
 	list_splice_tail(iova_copy, iova);
 }
 
+static int vfio_iommu_device_ok(void *iommu_data, struct device *device)
+{
+	bool msi_remap;
+
+	msi_remap = irq_domain_check_msi_remap() ||
+		    iommu_capable(device->bus, IOMMU_CAP_INTR_REMAP);
+
+	if (!allow_unsafe_interrupts && !msi_remap) {
+		pr_warn("%s: No interrupt remapping support.  Use the module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this platform\n",
+			__func__);
+		return -EPERM;
+	}
+	return 0;
+}
+
 static int vfio_iommu_type1_attach_group(void *iommu_data,
 		struct iommu_group *iommu_group, enum vfio_group_type type)
 {
@@ -2160,7 +2175,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	struct vfio_iommu_group *group;
 	struct vfio_domain *domain, *d;
 	struct bus_type *bus = NULL;
-	bool resv_msi, msi_remap;
+	bool resv_msi;
 	phys_addr_t resv_msi_base = 0;
 	struct iommu_domain_geometry *geo;
 	LIST_HEAD(iova_copy);
@@ -2257,16 +2272,6 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	INIT_LIST_HEAD(&domain->group_list);
 	list_add(&group->next, &domain->group_list);
 
-	msi_remap = irq_domain_check_msi_remap() ||
-		    iommu_capable(bus, IOMMU_CAP_INTR_REMAP);
-
-	if (!allow_unsafe_interrupts && !msi_remap) {
-		pr_warn("%s: No interrupt remapping support.  Use the module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this platform\n",
-		       __func__);
-		ret = -EPERM;
-		goto out_detach;
-	}
-
 	/*
 	 * If the IOMMU can block non-coherent operations (ie PCIe TLPs with
 	 * no-snoop set) then VFIO always turns this feature on because on Intel
@@ -3159,6 +3164,7 @@ static const struct vfio_iommu_driver_ops vfio_iommu_driver_ops_type1 = {
 	.open			= vfio_iommu_type1_open,
 	.release		= vfio_iommu_type1_release,
 	.ioctl			= vfio_iommu_type1_ioctl,
+	.device_ok		= vfio_iommu_device_ok,
 	.attach_group		= vfio_iommu_type1_attach_group,
 	.detach_group		= vfio_iommu_type1_detach_group,
 	.pin_pages		= vfio_iommu_type1_pin_pages,
-- 
2.35.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Robin Murphy]

On 2022-04-07 20:08, Jason Gunthorpe wrote:
> On Thu, Apr 07, 2022 at 07:02:03PM +0100, Robin Murphy wrote:
>> On 2022-04-07 18:43, Jason Gunthorpe wrote:
>>> On Thu, Apr 07, 2022 at 06:03:37PM +0100, Robin Murphy wrote:
>>>> At a glance, this all looks about the right shape to me now, thanks!
>>>
>>> Thanks!
>>>
>>>> Ideally I'd hope patch #4 could go straight to device_iommu_capable() from
>>>> my Thunderbolt series, but we can figure that out in a couple of weeks once
>>>
>>> Yes, this does helps that because now the only iommu_capable call is
>>> in a context where a device is available :)
>>
>> Derp, of course I have *two* VFIO patches waiting, the other one touching
>> the iommu_capable() calls (there's still IOMMU_CAP_INTR_REMAP, which, much
>> as I hate it and would love to boot all that stuff over to
>> drivers/irqchip,
> 
> Oh me too...
> 
>> it's not in my way so I'm leaving it be for now). I'll have to rebase that
>> anyway, so merging this as-is is absolutely fine!
> 
> This might help your effort - after this series and this below there
> are no 'bus' users of iommu_capable left at all.

Thanks, but I still need a device for the iommu_domain_alloc() as well, 
so at that point the interrupt check is OK to stay where it is. I 
figured out a locking strategy per my original idea that seems pretty 
clean, it just needs vfio_group_viable() to go away first:

https://gitlab.arm.com/linux-arm/linux-rm/-/commit/c6057da9f6b5f4b0fb67c6e647d2f8f76a6177fc

Cheers,
Robin.

Re: [PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Robin Murphy]

On 2022-04-07 20:08, Jason Gunthorpe wrote:
> On Thu, Apr 07, 2022 at 07:02:03PM +0100, Robin Murphy wrote:
>> On 2022-04-07 18:43, Jason Gunthorpe wrote:
>>> On Thu, Apr 07, 2022 at 06:03:37PM +0100, Robin Murphy wrote:
>>>> At a glance, this all looks about the right shape to me now, thanks!
>>>
>>> Thanks!
>>>
>>>> Ideally I'd hope patch #4 could go straight to device_iommu_capable() from
>>>> my Thunderbolt series, but we can figure that out in a couple of weeks once
>>>
>>> Yes, this does helps that because now the only iommu_capable call is
>>> in a context where a device is available :)
>>
>> Derp, of course I have *two* VFIO patches waiting, the other one touching
>> the iommu_capable() calls (there's still IOMMU_CAP_INTR_REMAP, which, much
>> as I hate it and would love to boot all that stuff over to
>> drivers/irqchip,
> 
> Oh me too...
> 
>> it's not in my way so I'm leaving it be for now). I'll have to rebase that
>> anyway, so merging this as-is is absolutely fine!
> 
> This might help your effort - after this series and this below there
> are no 'bus' users of iommu_capable left at all.

Thanks, but I still need a device for the iommu_domain_alloc() as well, 
so at that point the interrupt check is OK to stay where it is. I 
figured out a locking strategy per my original idea that seems pretty 
clean, it just needs vfio_group_viable() to go away first:

https://gitlab.arm.com/linux-arm/linux-rm/-/commit/c6057da9f6b5f4b0fb67c6e647d2f8f76a6177fc

Cheers,
Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 1/4] iommu: Introduce the domain op enforce_cache_coherency()

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, April 7, 2022 11:24 PM
> 
> This new mechanism will replace using IOMMU_CAP_CACHE_COHERENCY
> and
> IOMMU_CACHE to control the no-snoop blocking behavior of the IOMMU.
> 
> Currently only Intel and AMD IOMMUs are known to support this
> feature. They both implement it as an IOPTE bit, that when set, will cause
> PCIe TLPs to that IOVA with the no-snoop bit set to be treated as though
> the no-snoop bit was clear.
> 
> The new API is triggered by calling enforce_cache_coherency() before
> mapping any IOVA to the domain which globally switches on no-snoop
> blocking. This allows other implementations that might block no-snoop
> globally and outside the IOPTE - AMD also documents such a HW capability.
> 
> Leave AMD out of sync with Intel and have it block no-snoop even for
> in-kernel users. This can be trivially resolved in a follow up patch.
> 
> Only VFIO will call this new API.

I still didn't see the point of mandating a caller for a new API (and as
you pointed out iommufd will call it too).

[...]
> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
> index 2f9891cb3d0014..1f930c0c225d94 100644
> --- a/include/linux/intel-iommu.h
> +++ b/include/linux/intel-iommu.h
> @@ -540,6 +540,7 @@ struct dmar_domain {
>  	u8 has_iotlb_device: 1;
>  	u8 iommu_coherency: 1;		/* indicate coherency of
> iommu access */
>  	u8 iommu_snooping: 1;		/* indicate snooping control
> feature */
> +	u8 enforce_no_snoop : 1;        /* Create IOPTEs with snoop control */

it reads like no_snoop is the result of the enforcement... Probably
force_snooping better matches the intention here.

Thanks
Kevin

RE: [PATCH v2 1/4] iommu: Introduce the domain op enforce_cache_coherency()

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, April 7, 2022 11:24 PM
> 
> This new mechanism will replace using IOMMU_CAP_CACHE_COHERENCY
> and
> IOMMU_CACHE to control the no-snoop blocking behavior of the IOMMU.
> 
> Currently only Intel and AMD IOMMUs are known to support this
> feature. They both implement it as an IOPTE bit, that when set, will cause
> PCIe TLPs to that IOVA with the no-snoop bit set to be treated as though
> the no-snoop bit was clear.
> 
> The new API is triggered by calling enforce_cache_coherency() before
> mapping any IOVA to the domain which globally switches on no-snoop
> blocking. This allows other implementations that might block no-snoop
> globally and outside the IOPTE - AMD also documents such a HW capability.
> 
> Leave AMD out of sync with Intel and have it block no-snoop even for
> in-kernel users. This can be trivially resolved in a follow up patch.
> 
> Only VFIO will call this new API.

I still didn't see the point of mandating a caller for a new API (and as
you pointed out iommufd will call it too).

[...]
> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
> index 2f9891cb3d0014..1f930c0c225d94 100644
> --- a/include/linux/intel-iommu.h
> +++ b/include/linux/intel-iommu.h
> @@ -540,6 +540,7 @@ struct dmar_domain {
>  	u8 has_iotlb_device: 1;
>  	u8 iommu_coherency: 1;		/* indicate coherency of
> iommu access */
>  	u8 iommu_snooping: 1;		/* indicate snooping control
> feature */
> +	u8 enforce_no_snoop : 1;        /* Create IOPTEs with snoop control */

it reads like no_snoop is the result of the enforcement... Probably
force_snooping better matches the intention here.

Thanks
Kevin
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 2/4] vfio: Move the Intel no-snoop control off of IOMMU_CACHE

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, April 7, 2022 11:24 PM
> 
> IOMMU_CACHE means "normal DMA to this iommu_domain's IOVA should
> be cache
> coherent" and is used by the DMA API. The definition allows for special
> non-coherent DMA to exist - ie processing of the no-snoop flag in PCIe
> TLPs - so long as this behavior is opt-in by the device driver.
> 
> The flag is mainly used by the DMA API to synchronize the IOMMU setting
> with the expected cache behavior of the DMA master. eg based on
> dev_is_dma_coherent() in some case.
> 
> For Intel IOMMU IOMMU_CACHE was redefined to mean 'force all DMA to
> be
> cache coherent' which has the practical effect of causing the IOMMU to
> ignore the no-snoop bit in a PCIe TLP.
> 
> x86 platforms are always IOMMU_CACHE, so Intel should ignore this flag.
> 
> Instead use the new domain op enforce_cache_coherency() which causes
> every
> IOPTE created in the domain to have the no-snoop blocking behavior.
> 
> Reconfigure VFIO to always use IOMMU_CACHE and call
> enforce_cache_coherency() to operate the special Intel behavior.
> 
> Remove the IOMMU_CACHE test from Intel IOMMU.
> 
> Ultimately VFIO plumbs the result of enforce_cache_coherency() back into
> the x86 platform code through kvm_arch_register_noncoherent_dma()
> which
> controls if the WBINVD instruction is available in the guest. No other
> arch implements kvm_arch_register_noncoherent_dma().
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

btw as discussed in last version it is not necessarily to recalculate
snoop control globally with this new approach. Will follow up to
clean it up after this series is merged.

RE: [PATCH v2 2/4] vfio: Move the Intel no-snoop control off of IOMMU_CACHE

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, April 7, 2022 11:24 PM
> 
> IOMMU_CACHE means "normal DMA to this iommu_domain's IOVA should
> be cache
> coherent" and is used by the DMA API. The definition allows for special
> non-coherent DMA to exist - ie processing of the no-snoop flag in PCIe
> TLPs - so long as this behavior is opt-in by the device driver.
> 
> The flag is mainly used by the DMA API to synchronize the IOMMU setting
> with the expected cache behavior of the DMA master. eg based on
> dev_is_dma_coherent() in some case.
> 
> For Intel IOMMU IOMMU_CACHE was redefined to mean 'force all DMA to
> be
> cache coherent' which has the practical effect of causing the IOMMU to
> ignore the no-snoop bit in a PCIe TLP.
> 
> x86 platforms are always IOMMU_CACHE, so Intel should ignore this flag.
> 
> Instead use the new domain op enforce_cache_coherency() which causes
> every
> IOPTE created in the domain to have the no-snoop blocking behavior.
> 
> Reconfigure VFIO to always use IOMMU_CACHE and call
> enforce_cache_coherency() to operate the special Intel behavior.
> 
> Remove the IOMMU_CACHE test from Intel IOMMU.
> 
> Ultimately VFIO plumbs the result of enforce_cache_coherency() back into
> the x86 platform code through kvm_arch_register_noncoherent_dma()
> which
> controls if the WBINVD instruction is available in the guest. No other
> arch implements kvm_arch_register_noncoherent_dma().
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

btw as discussed in last version it is not necessarily to recalculate
snoop control globally with this new approach. Will follow up to
clean it up after this series is merged.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 3/4] iommu: Redefine IOMMU_CAP_CACHE_COHERENCY as the cap flag for IOMMU_CACHE

[Tian, Kevin]

(CC Jason Wang)

> From: Jason Gunthorpe
> Sent: Thursday, April 7, 2022 11:24 PM
> 
> While the comment was correct that this flag was intended to convey the
> block no-snoop support in the IOMMU, it has become widely implemented
> and
> used to mean the IOMMU supports IOMMU_CACHE as a map flag. Only the
> Intel
> driver was different.
> 
> Now that the Intel driver is using enforce_cache_coherency() update the
> comment to make it clear that IOMMU_CAP_CACHE_COHERENCY is only
> about
> IOMMU_CACHE.  Fix the Intel driver to return true since IOMMU_CACHE
> always
> works.
> 
> The two places that test this flag, usnic and vdpa, are both assigning
> userspace pages to a driver controlled iommu_domain and require
> IOMMU_CACHE behavior as they offer no way for userspace to synchronize
> caches.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

btw the comment about vsnic and vdpa matches my thought. But
a recent change in Qemu [1] possibly wants confirmation from
Jason Wang.

[1] https://lore.kernel.org/all/20220304133556.233983-20-mst@redhat.com/

Thanks
Kevin

RE: [PATCH v2 3/4] iommu: Redefine IOMMU_CAP_CACHE_COHERENCY as the cap flag for IOMMU_CACHE

[Tian, Kevin]

(CC Jason Wang)

> From: Jason Gunthorpe
> Sent: Thursday, April 7, 2022 11:24 PM
> 
> While the comment was correct that this flag was intended to convey the
> block no-snoop support in the IOMMU, it has become widely implemented
> and
> used to mean the IOMMU supports IOMMU_CACHE as a map flag. Only the
> Intel
> driver was different.
> 
> Now that the Intel driver is using enforce_cache_coherency() update the
> comment to make it clear that IOMMU_CAP_CACHE_COHERENCY is only
> about
> IOMMU_CACHE.  Fix the Intel driver to return true since IOMMU_CACHE
> always
> works.
> 
> The two places that test this flag, usnic and vdpa, are both assigning
> userspace pages to a driver controlled iommu_domain and require
> IOMMU_CACHE behavior as they offer no way for userspace to synchronize
> caches.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

btw the comment about vsnic and vdpa matches my thought. But
a recent change in Qemu [1] possibly wants confirmation from
Jason Wang.

[1] https://lore.kernel.org/all/20220304133556.233983-20-mst@redhat.com/

Thanks
Kevin
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 4/4] vfio: Require that devices support DMA cache coherence

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, April 7, 2022 11:24 PM
> 
> IOMMU_CACHE means that normal DMAs do not require any additional
> coherency
> mechanism and is the basic uAPI that VFIO exposes to userspace. For
> instance VFIO applications like DPDK will not work if additional coherency
> operations are required.
> 
> Therefore check IOMMU_CAP_CACHE_COHERENCY like vdpa & usnic do
> before
> allowing an IOMMU backed VFIO device to be created.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/vfio/vfio.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index a4555014bd1e72..9edad767cfdad3 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -815,6 +815,13 @@ static int __vfio_register_dev(struct vfio_device
> *device,
> 
>  int vfio_register_group_dev(struct vfio_device *device)
>  {
> +	/*
> +	 * VFIO always sets IOMMU_CACHE because we offer no way for
> userspace to
> +	 * restore cache coherency.
> +	 */
> +	if (!iommu_capable(device->dev->bus,
> IOMMU_CAP_CACHE_COHERENCY))
> +		return -EINVAL;
> +

One nit. Is it logistically more reasonable to put this patch before
changing VFIO to always set IOMMU_CACHE?

otherwise,

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

RE: [PATCH v2 4/4] vfio: Require that devices support DMA cache coherence

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, April 7, 2022 11:24 PM
> 
> IOMMU_CACHE means that normal DMAs do not require any additional
> coherency
> mechanism and is the basic uAPI that VFIO exposes to userspace. For
> instance VFIO applications like DPDK will not work if additional coherency
> operations are required.
> 
> Therefore check IOMMU_CAP_CACHE_COHERENCY like vdpa & usnic do
> before
> allowing an IOMMU backed VFIO device to be created.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/vfio/vfio.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index a4555014bd1e72..9edad767cfdad3 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -815,6 +815,13 @@ static int __vfio_register_dev(struct vfio_device
> *device,
> 
>  int vfio_register_group_dev(struct vfio_device *device)
>  {
> +	/*
> +	 * VFIO always sets IOMMU_CACHE because we offer no way for
> userspace to
> +	 * restore cache coherency.
> +	 */
> +	if (!iommu_capable(device->dev->bus,
> IOMMU_CAP_CACHE_COHERENCY))
> +		return -EINVAL;
> +

One nit. Is it logistically more reasonable to put this patch before
changing VFIO to always set IOMMU_CACHE?

otherwise,

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 1/4] iommu: Introduce the domain op enforce_cache_coherency()

[Tian, Kevin]

> From: Tian, Kevin
> Sent: Friday, April 8, 2022 4:06 PM
> 
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Thursday, April 7, 2022 11:24 PM
> >
> > This new mechanism will replace using IOMMU_CAP_CACHE_COHERENCY
> > and
> > IOMMU_CACHE to control the no-snoop blocking behavior of the IOMMU.
> >
> > Currently only Intel and AMD IOMMUs are known to support this
> > feature. They both implement it as an IOPTE bit, that when set, will cause
> > PCIe TLPs to that IOVA with the no-snoop bit set to be treated as though
> > the no-snoop bit was clear.
> >
> > The new API is triggered by calling enforce_cache_coherency() before
> > mapping any IOVA to the domain which globally switches on no-snoop
> > blocking. This allows other implementations that might block no-snoop
> > globally and outside the IOPTE - AMD also documents such a HW capability.
> >
> > Leave AMD out of sync with Intel and have it block no-snoop even for
> > in-kernel users. This can be trivially resolved in a follow up patch.
> >
> > Only VFIO will call this new API.
> 
> I still didn't see the point of mandating a caller for a new API (and as
> you pointed out iommufd will call it too).
> 
> [...]
> > diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
> > index 2f9891cb3d0014..1f930c0c225d94 100644
> > --- a/include/linux/intel-iommu.h
> > +++ b/include/linux/intel-iommu.h
> > @@ -540,6 +540,7 @@ struct dmar_domain {
> >  	u8 has_iotlb_device: 1;
> >  	u8 iommu_coherency: 1;		/* indicate coherency of
> > iommu access */
> >  	u8 iommu_snooping: 1;		/* indicate snooping control
> > feature */
> > +	u8 enforce_no_snoop : 1;        /* Create IOPTEs with snoop control */
> 
> it reads like no_snoop is the result of the enforcement... Probably
> force_snooping better matches the intention here.
> 

Above are just nits. Here is:

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

RE: [PATCH v2 1/4] iommu: Introduce the domain op enforce_cache_coherency()

[Tian, Kevin]

> From: Tian, Kevin
> Sent: Friday, April 8, 2022 4:06 PM
> 
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Thursday, April 7, 2022 11:24 PM
> >
> > This new mechanism will replace using IOMMU_CAP_CACHE_COHERENCY
> > and
> > IOMMU_CACHE to control the no-snoop blocking behavior of the IOMMU.
> >
> > Currently only Intel and AMD IOMMUs are known to support this
> > feature. They both implement it as an IOPTE bit, that when set, will cause
> > PCIe TLPs to that IOVA with the no-snoop bit set to be treated as though
> > the no-snoop bit was clear.
> >
> > The new API is triggered by calling enforce_cache_coherency() before
> > mapping any IOVA to the domain which globally switches on no-snoop
> > blocking. This allows other implementations that might block no-snoop
> > globally and outside the IOPTE - AMD also documents such a HW capability.
> >
> > Leave AMD out of sync with Intel and have it block no-snoop even for
> > in-kernel users. This can be trivially resolved in a follow up patch.
> >
> > Only VFIO will call this new API.
> 
> I still didn't see the point of mandating a caller for a new API (and as
> you pointed out iommufd will call it too).
> 
> [...]
> > diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
> > index 2f9891cb3d0014..1f930c0c225d94 100644
> > --- a/include/linux/intel-iommu.h
> > +++ b/include/linux/intel-iommu.h
> > @@ -540,6 +540,7 @@ struct dmar_domain {
> >  	u8 has_iotlb_device: 1;
> >  	u8 iommu_coherency: 1;		/* indicate coherency of
> > iommu access */
> >  	u8 iommu_snooping: 1;		/* indicate snooping control
> > feature */
> > +	u8 enforce_no_snoop : 1;        /* Create IOPTEs with snoop control */
> 
> it reads like no_snoop is the result of the enforcement... Probably
> force_snooping better matches the intention here.
> 

Above are just nits. Here is:

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Friday, April 8, 2022 3:08 AM
> On Thu, Apr 07, 2022 at 07:02:03PM +0100, Robin Murphy wrote:
> > On 2022-04-07 18:43, Jason Gunthorpe wrote:
> > > On Thu, Apr 07, 2022 at 06:03:37PM +0100, Robin Murphy wrote:
> > > > At a glance, this all looks about the right shape to me now, thanks!
> > >
> > > Thanks!
> > >
> > > > Ideally I'd hope patch #4 could go straight to device_iommu_capable()
> from
> > > > my Thunderbolt series, but we can figure that out in a couple of weeks
> once
> > >
> > > Yes, this does helps that because now the only iommu_capable call is
> > > in a context where a device is available :)
> >
> > Derp, of course I have *two* VFIO patches waiting, the other one touching
> > the iommu_capable() calls (there's still IOMMU_CAP_INTR_REMAP, which,
> much
> > as I hate it and would love to boot all that stuff over to
> > drivers/irqchip,
> 
> Oh me too...
> 
> > it's not in my way so I'm leaving it be for now). I'll have to rebase that
> > anyway, so merging this as-is is absolutely fine!
> 
> This might help your effort - after this series and this below there
> are no 'bus' users of iommu_capable left at all.
> 

Out of curiosity, while iommu_capable is being moved to a per-device
interface what about irq_domain_check_msi_remap() below (which
is also a global check)?

> +static int vfio_iommu_device_ok(void *iommu_data, struct device *device)
> +{
> +	bool msi_remap;
> +
> +	msi_remap = irq_domain_check_msi_remap() ||
> +		    iommu_capable(device->bus, IOMMU_CAP_INTR_REMAP);
> +

RE: [PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Friday, April 8, 2022 3:08 AM
> On Thu, Apr 07, 2022 at 07:02:03PM +0100, Robin Murphy wrote:
> > On 2022-04-07 18:43, Jason Gunthorpe wrote:
> > > On Thu, Apr 07, 2022 at 06:03:37PM +0100, Robin Murphy wrote:
> > > > At a glance, this all looks about the right shape to me now, thanks!
> > >
> > > Thanks!
> > >
> > > > Ideally I'd hope patch #4 could go straight to device_iommu_capable()
> from
> > > > my Thunderbolt series, but we can figure that out in a couple of weeks
> once
> > >
> > > Yes, this does helps that because now the only iommu_capable call is
> > > in a context where a device is available :)
> >
> > Derp, of course I have *two* VFIO patches waiting, the other one touching
> > the iommu_capable() calls (there's still IOMMU_CAP_INTR_REMAP, which,
> much
> > as I hate it and would love to boot all that stuff over to
> > drivers/irqchip,
> 
> Oh me too...
> 
> > it's not in my way so I'm leaving it be for now). I'll have to rebase that
> > anyway, so merging this as-is is absolutely fine!
> 
> This might help your effort - after this series and this below there
> are no 'bus' users of iommu_capable left at all.
> 

Out of curiosity, while iommu_capable is being moved to a per-device
interface what about irq_domain_check_msi_remap() below (which
is also a global check)?

> +static int vfio_iommu_device_ok(void *iommu_data, struct device *device)
> +{
> +	bool msi_remap;
> +
> +	msi_remap = irq_domain_check_msi_remap() ||
> +		    iommu_capable(device->bus, IOMMU_CAP_INTR_REMAP);
> +

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Robin Murphy]

On 2022-04-08 10:08, Tian, Kevin wrote:
>> From: Jason Gunthorpe <jgg@nvidia.com>
>> Sent: Friday, April 8, 2022 3:08 AM
>> On Thu, Apr 07, 2022 at 07:02:03PM +0100, Robin Murphy wrote:
>>> On 2022-04-07 18:43, Jason Gunthorpe wrote:
>>>> On Thu, Apr 07, 2022 at 06:03:37PM +0100, Robin Murphy wrote:
>>>>> At a glance, this all looks about the right shape to me now, thanks!
>>>>
>>>> Thanks!
>>>>
>>>>> Ideally I'd hope patch #4 could go straight to device_iommu_capable()
>> from
>>>>> my Thunderbolt series, but we can figure that out in a couple of weeks
>> once
>>>>
>>>> Yes, this does helps that because now the only iommu_capable call is
>>>> in a context where a device is available :)
>>>
>>> Derp, of course I have *two* VFIO patches waiting, the other one touching
>>> the iommu_capable() calls (there's still IOMMU_CAP_INTR_REMAP, which,
>> much
>>> as I hate it and would love to boot all that stuff over to
>>> drivers/irqchip,
>>
>> Oh me too...
>>
>>> it's not in my way so I'm leaving it be for now). I'll have to rebase that
>>> anyway, so merging this as-is is absolutely fine!
>>
>> This might help your effort - after this series and this below there
>> are no 'bus' users of iommu_capable left at all.
>>
> 
> Out of curiosity, while iommu_capable is being moved to a per-device
> interface what about irq_domain_check_msi_remap() below (which
> is also a global check)?

I suppose it could if anyone cared enough to make the effort - probably 
a case of resolving specific MSI domains for every device in the group, 
and potentially having to deal with hotplug later as well. 
Realistically, though, I wouldn't expect systems to have mixed 
capabilities in that regard (i.e. where the check would return false 
even though *some* domains support remapping), so there doesn't seem to 
be any pressing need to relax it.

Cheers,
Robin.

>> +static int vfio_iommu_device_ok(void *iommu_data, struct device *device)
>> +{
>> +	bool msi_remap;
>> +
>> +	msi_remap = irq_domain_check_msi_remap() ||
>> +		    iommu_capable(device->bus, IOMMU_CAP_INTR_REMAP);
>> +
> 

Re: [PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Robin Murphy]

On 2022-04-08 10:08, Tian, Kevin wrote:
>> From: Jason Gunthorpe <jgg@nvidia.com>
>> Sent: Friday, April 8, 2022 3:08 AM
>> On Thu, Apr 07, 2022 at 07:02:03PM +0100, Robin Murphy wrote:
>>> On 2022-04-07 18:43, Jason Gunthorpe wrote:
>>>> On Thu, Apr 07, 2022 at 06:03:37PM +0100, Robin Murphy wrote:
>>>>> At a glance, this all looks about the right shape to me now, thanks!
>>>>
>>>> Thanks!
>>>>
>>>>> Ideally I'd hope patch #4 could go straight to device_iommu_capable()
>> from
>>>>> my Thunderbolt series, but we can figure that out in a couple of weeks
>> once
>>>>
>>>> Yes, this does helps that because now the only iommu_capable call is
>>>> in a context where a device is available :)
>>>
>>> Derp, of course I have *two* VFIO patches waiting, the other one touching
>>> the iommu_capable() calls (there's still IOMMU_CAP_INTR_REMAP, which,
>> much
>>> as I hate it and would love to boot all that stuff over to
>>> drivers/irqchip,
>>
>> Oh me too...
>>
>>> it's not in my way so I'm leaving it be for now). I'll have to rebase that
>>> anyway, so merging this as-is is absolutely fine!
>>
>> This might help your effort - after this series and this below there
>> are no 'bus' users of iommu_capable left at all.
>>
> 
> Out of curiosity, while iommu_capable is being moved to a per-device
> interface what about irq_domain_check_msi_remap() below (which
> is also a global check)?

I suppose it could if anyone cared enough to make the effort - probably 
a case of resolving specific MSI domains for every device in the group, 
and potentially having to deal with hotplug later as well. 
Realistically, though, I wouldn't expect systems to have mixed 
capabilities in that regard (i.e. where the check would return false 
even though *some* domains support remapping), so there doesn't seem to 
be any pressing need to relax it.

Cheers,
Robin.

>> +static int vfio_iommu_device_ok(void *iommu_data, struct device *device)
>> +{
>> +	bool msi_remap;
>> +
>> +	msi_remap = irq_domain_check_msi_remap() ||
>> +		    iommu_capable(device->bus, IOMMU_CAP_INTR_REMAP);
>> +
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Jason Gunthorpe]

On Thu, Apr 07, 2022 at 08:27:05PM +0100, Robin Murphy wrote:
> On 2022-04-07 20:08, Jason Gunthorpe wrote:
> > On Thu, Apr 07, 2022 at 07:02:03PM +0100, Robin Murphy wrote:
> > > On 2022-04-07 18:43, Jason Gunthorpe wrote:
> > > > On Thu, Apr 07, 2022 at 06:03:37PM +0100, Robin Murphy wrote:
> > > > > At a glance, this all looks about the right shape to me now, thanks!
> > > > 
> > > > Thanks!
> > > > 
> > > > > Ideally I'd hope patch #4 could go straight to device_iommu_capable() from
> > > > > my Thunderbolt series, but we can figure that out in a couple of weeks once
> > > > 
> > > > Yes, this does helps that because now the only iommu_capable call is
> > > > in a context where a device is available :)
> > > 
> > > Derp, of course I have *two* VFIO patches waiting, the other one touching
> > > the iommu_capable() calls (there's still IOMMU_CAP_INTR_REMAP, which, much
> > > as I hate it and would love to boot all that stuff over to
> > > drivers/irqchip,
> > 
> > Oh me too...
> > 
> > > it's not in my way so I'm leaving it be for now). I'll have to rebase that
> > > anyway, so merging this as-is is absolutely fine!
> > 
> > This might help your effort - after this series and this below there
> > are no 'bus' users of iommu_capable left at all.
> 
> Thanks, but I still need a device for the iommu_domain_alloc() as well, so
> at that point the interrupt check is OK to stay where it is. 

It is a simple enough change that could avoid introducing the
device_iommu_capable() at all perhaps.

> I figured out a locking strategy per my original idea that seems
> pretty clean, it just needs vfio_group_viable() to go away first:

I think this should be more like:

  	        struct vfio_device *vdev;

		mutex_lock(&group->device_lock);
		vdev = list_first_entry(group->device_list, struct vfio_device, group_next);
		ret = driver->ops->attach_group(data, group->iommu_group,
						group->type,
						vdev->dev);
		mutex_unlock(&group->device_lock);

Then don't do the iommu_group_for_each_dev() at all.

The problem with iommu_group_for_each_dev() is that it may select a
struct device that does not have a vfio_device bound to it, so we
would be using a random struct device that is not protected by any
VFIO device_driver.

However, this creates an oddball situation where the vfio_device and
it's struct device could become unplugged from the system while the
domain that the struct device spawned continues to exist and remains
attached to other devices in the same group. ie the iommu driver has
to be careful not to retain the struct device input..

I suppose that is inevitable to have sharing of domains across
devices, so the iommu drivers will have to accommodate this.

Jason

Re: [PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Jason Gunthorpe via iommu]

On Thu, Apr 07, 2022 at 08:27:05PM +0100, Robin Murphy wrote:
> On 2022-04-07 20:08, Jason Gunthorpe wrote:
> > On Thu, Apr 07, 2022 at 07:02:03PM +0100, Robin Murphy wrote:
> > > On 2022-04-07 18:43, Jason Gunthorpe wrote:
> > > > On Thu, Apr 07, 2022 at 06:03:37PM +0100, Robin Murphy wrote:
> > > > > At a glance, this all looks about the right shape to me now, thanks!
> > > > 
> > > > Thanks!
> > > > 
> > > > > Ideally I'd hope patch #4 could go straight to device_iommu_capable() from
> > > > > my Thunderbolt series, but we can figure that out in a couple of weeks once
> > > > 
> > > > Yes, this does helps that because now the only iommu_capable call is
> > > > in a context where a device is available :)
> > > 
> > > Derp, of course I have *two* VFIO patches waiting, the other one touching
> > > the iommu_capable() calls (there's still IOMMU_CAP_INTR_REMAP, which, much
> > > as I hate it and would love to boot all that stuff over to
> > > drivers/irqchip,
> > 
> > Oh me too...
> > 
> > > it's not in my way so I'm leaving it be for now). I'll have to rebase that
> > > anyway, so merging this as-is is absolutely fine!
> > 
> > This might help your effort - after this series and this below there
> > are no 'bus' users of iommu_capable left at all.
> 
> Thanks, but I still need a device for the iommu_domain_alloc() as well, so
> at that point the interrupt check is OK to stay where it is. 

It is a simple enough change that could avoid introducing the
device_iommu_capable() at all perhaps.

> I figured out a locking strategy per my original idea that seems
> pretty clean, it just needs vfio_group_viable() to go away first:

I think this should be more like:

  	        struct vfio_device *vdev;

		mutex_lock(&group->device_lock);
		vdev = list_first_entry(group->device_list, struct vfio_device, group_next);
		ret = driver->ops->attach_group(data, group->iommu_group,
						group->type,
						vdev->dev);
		mutex_unlock(&group->device_lock);

Then don't do the iommu_group_for_each_dev() at all.

The problem with iommu_group_for_each_dev() is that it may select a
struct device that does not have a vfio_device bound to it, so we
would be using a random struct device that is not protected by any
VFIO device_driver.

However, this creates an oddball situation where the vfio_device and
it's struct device could become unplugged from the system while the
domain that the struct device spawned continues to exist and remains
attached to other devices in the same group. ie the iommu driver has
to be careful not to retain the struct device input..

I suppose that is inevitable to have sharing of domains across
devices, so the iommu drivers will have to accommodate this.

Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 3/4] iommu: Redefine IOMMU_CAP_CACHE_COHERENCY as the cap flag for IOMMU_CACHE

[Jason Gunthorpe]

On Fri, Apr 08, 2022 at 08:21:55AM +0000, Tian, Kevin wrote:
> (CC Jason Wang)
> 
> > From: Jason Gunthorpe
> > Sent: Thursday, April 7, 2022 11:24 PM
> > 
> > While the comment was correct that this flag was intended to convey the
> > block no-snoop support in the IOMMU, it has become widely implemented
> > and
> > used to mean the IOMMU supports IOMMU_CACHE as a map flag. Only the
> > Intel
> > driver was different.
> > 
> > Now that the Intel driver is using enforce_cache_coherency() update the
> > comment to make it clear that IOMMU_CAP_CACHE_COHERENCY is only
> > about
> > IOMMU_CACHE.  Fix the Intel driver to return true since IOMMU_CACHE
> > always
> > works.
> > 
> > The two places that test this flag, usnic and vdpa, are both assigning
> > userspace pages to a driver controlled iommu_domain and require
> > IOMMU_CACHE behavior as they offer no way for userspace to synchronize
> > caches.
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> 
> btw the comment about vsnic and vdpa matches my thought. But
> a recent change in Qemu [1] possibly wants confirmation from
> Jason Wang.
> 
> [1] https://lore.kernel.org/all/20220304133556.233983-20-mst@redhat.com/

That patch seems to have run into the confusion this series is
addressing.

VFIO_DMA_CC_IOMMU and snoop control is absolutely not needed by
VDPA. We expect the VDPA kernel driver to be well behaved and not
cause its device to generate no-snoop TLPs.

VDPA needs IOMMU_CACHE only.

Jason

Re: [PATCH v2 3/4] iommu: Redefine IOMMU_CAP_CACHE_COHERENCY as the cap flag for IOMMU_CACHE

[Jason Gunthorpe via iommu]

On Fri, Apr 08, 2022 at 08:21:55AM +0000, Tian, Kevin wrote:
> (CC Jason Wang)
> 
> > From: Jason Gunthorpe
> > Sent: Thursday, April 7, 2022 11:24 PM
> > 
> > While the comment was correct that this flag was intended to convey the
> > block no-snoop support in the IOMMU, it has become widely implemented
> > and
> > used to mean the IOMMU supports IOMMU_CACHE as a map flag. Only the
> > Intel
> > driver was different.
> > 
> > Now that the Intel driver is using enforce_cache_coherency() update the
> > comment to make it clear that IOMMU_CAP_CACHE_COHERENCY is only
> > about
> > IOMMU_CACHE.  Fix the Intel driver to return true since IOMMU_CACHE
> > always
> > works.
> > 
> > The two places that test this flag, usnic and vdpa, are both assigning
> > userspace pages to a driver controlled iommu_domain and require
> > IOMMU_CACHE behavior as they offer no way for userspace to synchronize
> > caches.
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> 
> btw the comment about vsnic and vdpa matches my thought. But
> a recent change in Qemu [1] possibly wants confirmation from
> Jason Wang.
> 
> [1] https://lore.kernel.org/all/20220304133556.233983-20-mst@redhat.com/

That patch seems to have run into the confusion this series is
addressing.

VFIO_DMA_CC_IOMMU and snoop control is absolutely not needed by
VDPA. We expect the VDPA kernel driver to be well behaved and not
cause its device to generate no-snoop TLPs.

VDPA needs IOMMU_CACHE only.

Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 4/4] vfio: Require that devices support DMA cache coherence

[Jason Gunthorpe via iommu]

On Fri, Apr 08, 2022 at 08:26:10AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Thursday, April 7, 2022 11:24 PM
> > 
> > IOMMU_CACHE means that normal DMAs do not require any additional
> > coherency
> > mechanism and is the basic uAPI that VFIO exposes to userspace. For
> > instance VFIO applications like DPDK will not work if additional coherency
> > operations are required.
> > 
> > Therefore check IOMMU_CAP_CACHE_COHERENCY like vdpa & usnic do
> > before
> > allowing an IOMMU backed VFIO device to be created.
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> >  drivers/vfio/vfio.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> > index a4555014bd1e72..9edad767cfdad3 100644
> > +++ b/drivers/vfio/vfio.c
> > @@ -815,6 +815,13 @@ static int __vfio_register_dev(struct vfio_device
> > *device,
> > 
> >  int vfio_register_group_dev(struct vfio_device *device)
> >  {
> > +	/*
> > +	 * VFIO always sets IOMMU_CACHE because we offer no way for
> > userspace to
> > +	 * restore cache coherency.
> > +	 */
> > +	if (!iommu_capable(device->dev->bus,
> > IOMMU_CAP_CACHE_COHERENCY))
> > +		return -EINVAL;
> > +
> 
> One nit. Is it logistically more reasonable to put this patch before
> changing VFIO to always set IOMMU_CACHE?

For bisectability it has to be after

    iommu: Redefine IOMMU_CAP_CACHE_COHERENCY as the cap flag for IOMMU_CACHE

Otherwise Intel iommu will stop working with VFIO

The ordering is OK as is because no IOMMU that works with VFIO cares
about IOMMU_CACHE.

Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 4/4] vfio: Require that devices support DMA cache coherence

[Jason Gunthorpe]

On Fri, Apr 08, 2022 at 08:26:10AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Thursday, April 7, 2022 11:24 PM
> > 
> > IOMMU_CACHE means that normal DMAs do not require any additional
> > coherency
> > mechanism and is the basic uAPI that VFIO exposes to userspace. For
> > instance VFIO applications like DPDK will not work if additional coherency
> > operations are required.
> > 
> > Therefore check IOMMU_CAP_CACHE_COHERENCY like vdpa & usnic do
> > before
> > allowing an IOMMU backed VFIO device to be created.
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> >  drivers/vfio/vfio.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> > index a4555014bd1e72..9edad767cfdad3 100644
> > +++ b/drivers/vfio/vfio.c
> > @@ -815,6 +815,13 @@ static int __vfio_register_dev(struct vfio_device
> > *device,
> > 
> >  int vfio_register_group_dev(struct vfio_device *device)
> >  {
> > +	/*
> > +	 * VFIO always sets IOMMU_CACHE because we offer no way for
> > userspace to
> > +	 * restore cache coherency.
> > +	 */
> > +	if (!iommu_capable(device->dev->bus,
> > IOMMU_CAP_CACHE_COHERENCY))
> > +		return -EINVAL;
> > +
> 
> One nit. Is it logistically more reasonable to put this patch before
> changing VFIO to always set IOMMU_CACHE?

For bisectability it has to be after

    iommu: Redefine IOMMU_CAP_CACHE_COHERENCY as the cap flag for IOMMU_CACHE

Otherwise Intel iommu will stop working with VFIO

The ordering is OK as is because no IOMMU that works with VFIO cares
about IOMMU_CACHE.

Jason

Re: [PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Robin Murphy]

On 2022-04-08 13:18, Jason Gunthorpe wrote:
> On Thu, Apr 07, 2022 at 08:27:05PM +0100, Robin Murphy wrote:
>> On 2022-04-07 20:08, Jason Gunthorpe wrote:
>>> On Thu, Apr 07, 2022 at 07:02:03PM +0100, Robin Murphy wrote:
>>>> On 2022-04-07 18:43, Jason Gunthorpe wrote:
>>>>> On Thu, Apr 07, 2022 at 06:03:37PM +0100, Robin Murphy wrote:
>>>>>> At a glance, this all looks about the right shape to me now, thanks!
>>>>>
>>>>> Thanks!
>>>>>
>>>>>> Ideally I'd hope patch #4 could go straight to device_iommu_capable() from
>>>>>> my Thunderbolt series, but we can figure that out in a couple of weeks once
>>>>>
>>>>> Yes, this does helps that because now the only iommu_capable call is
>>>>> in a context where a device is available :)
>>>>
>>>> Derp, of course I have *two* VFIO patches waiting, the other one touching
>>>> the iommu_capable() calls (there's still IOMMU_CAP_INTR_REMAP, which, much
>>>> as I hate it and would love to boot all that stuff over to
>>>> drivers/irqchip,
>>>
>>> Oh me too...
>>>
>>>> it's not in my way so I'm leaving it be for now). I'll have to rebase that
>>>> anyway, so merging this as-is is absolutely fine!
>>>
>>> This might help your effort - after this series and this below there
>>> are no 'bus' users of iommu_capable left at all.
>>
>> Thanks, but I still need a device for the iommu_domain_alloc() as well, so
>> at that point the interrupt check is OK to stay where it is.
> 
> It is a simple enough change that could avoid introducing the
> device_iommu_capable() at all perhaps.
> 
>> I figured out a locking strategy per my original idea that seems
>> pretty clean, it just needs vfio_group_viable() to go away first:
> 
> I think this should be more like:
> 
>    	        struct vfio_device *vdev;
> 
> 		mutex_lock(&group->device_lock);
> 		vdev = list_first_entry(group->device_list, struct vfio_device, group_next);
> 		ret = driver->ops->attach_group(data, group->iommu_group,
> 						group->type,
> 						vdev->dev);
> 		mutex_unlock(&group->device_lock);
> 
> Then don't do the iommu_group_for_each_dev() at all.
> 
> The problem with iommu_group_for_each_dev() is that it may select a
> struct device that does not have a vfio_device bound to it, so we
> would be using a random struct device that is not protected by any
> VFIO device_driver.

Yeah, I was just looking at the final puzzle piece of making sure to nab 
the actual VFIO device rather than some unbound device that's just along 
for the ride... If I can't come up with anything more self-contained 
I'll take this suggestion, thanks.

> However, this creates an oddball situation where the vfio_device and
> it's struct device could become unplugged from the system while the
> domain that the struct device spawned continues to exist and remains
> attached to other devices in the same group. ie the iommu driver has
> to be careful not to retain the struct device input..

Oh, I rather assumed that VFIO might automatically tear down the 
container/domain when the last real user disappears. I don't see there 
being an obvious problem if the domain does hang around with residual 
non-VFIO devices attached until userspace explicitly closes all the 
relevant handles, as long as we take care not to release DMA ownership 
until that point also. As you say, it just looks a bit funny.

> I suppose that is inevitable to have sharing of domains across
> devices, so the iommu drivers will have to accommodate this.

I think domain lifecycle management is already entirely up to the users 
and not something that IOMMU drivers need to worry about. Drivers should 
only need to look at per-device data in attach/detach (and, once I've 
finished, alloc) from the device argument which can be assumed to be 
valid at that point. Otherwise, all the relevant internal data for 
domain ops should belong to the domain already.

Cheers,
Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Robin Murphy]

On 2022-04-08 13:18, Jason Gunthorpe wrote:
> On Thu, Apr 07, 2022 at 08:27:05PM +0100, Robin Murphy wrote:
>> On 2022-04-07 20:08, Jason Gunthorpe wrote:
>>> On Thu, Apr 07, 2022 at 07:02:03PM +0100, Robin Murphy wrote:
>>>> On 2022-04-07 18:43, Jason Gunthorpe wrote:
>>>>> On Thu, Apr 07, 2022 at 06:03:37PM +0100, Robin Murphy wrote:
>>>>>> At a glance, this all looks about the right shape to me now, thanks!
>>>>>
>>>>> Thanks!
>>>>>
>>>>>> Ideally I'd hope patch #4 could go straight to device_iommu_capable() from
>>>>>> my Thunderbolt series, but we can figure that out in a couple of weeks once
>>>>>
>>>>> Yes, this does helps that because now the only iommu_capable call is
>>>>> in a context where a device is available :)
>>>>
>>>> Derp, of course I have *two* VFIO patches waiting, the other one touching
>>>> the iommu_capable() calls (there's still IOMMU_CAP_INTR_REMAP, which, much
>>>> as I hate it and would love to boot all that stuff over to
>>>> drivers/irqchip,
>>>
>>> Oh me too...
>>>
>>>> it's not in my way so I'm leaving it be for now). I'll have to rebase that
>>>> anyway, so merging this as-is is absolutely fine!
>>>
>>> This might help your effort - after this series and this below there
>>> are no 'bus' users of iommu_capable left at all.
>>
>> Thanks, but I still need a device for the iommu_domain_alloc() as well, so
>> at that point the interrupt check is OK to stay where it is.
> 
> It is a simple enough change that could avoid introducing the
> device_iommu_capable() at all perhaps.
> 
>> I figured out a locking strategy per my original idea that seems
>> pretty clean, it just needs vfio_group_viable() to go away first:
> 
> I think this should be more like:
> 
>    	        struct vfio_device *vdev;
> 
> 		mutex_lock(&group->device_lock);
> 		vdev = list_first_entry(group->device_list, struct vfio_device, group_next);
> 		ret = driver->ops->attach_group(data, group->iommu_group,
> 						group->type,
> 						vdev->dev);
> 		mutex_unlock(&group->device_lock);
> 
> Then don't do the iommu_group_for_each_dev() at all.
> 
> The problem with iommu_group_for_each_dev() is that it may select a
> struct device that does not have a vfio_device bound to it, so we
> would be using a random struct device that is not protected by any
> VFIO device_driver.

Yeah, I was just looking at the final puzzle piece of making sure to nab 
the actual VFIO device rather than some unbound device that's just along 
for the ride... If I can't come up with anything more self-contained 
I'll take this suggestion, thanks.

> However, this creates an oddball situation where the vfio_device and
> it's struct device could become unplugged from the system while the
> domain that the struct device spawned continues to exist and remains
> attached to other devices in the same group. ie the iommu driver has
> to be careful not to retain the struct device input..

Oh, I rather assumed that VFIO might automatically tear down the 
container/domain when the last real user disappears. I don't see there 
being an obvious problem if the domain does hang around with residual 
non-VFIO devices attached until userspace explicitly closes all the 
relevant handles, as long as we take care not to release DMA ownership 
until that point also. As you say, it just looks a bit funny.

> I suppose that is inevitable to have sharing of domains across
> devices, so the iommu drivers will have to accommodate this.

I think domain lifecycle management is already entirely up to the users 
and not something that IOMMU drivers need to worry about. Drivers should 
only need to look at per-device data in attach/detach (and, once I've 
finished, alloc) from the device argument which can be assumed to be 
valid at that point. Otherwise, all the relevant internal data for 
domain ops should belong to the domain already.

Cheers,
Robin.

Re: [PATCH v2 4/4] vfio: Require that devices support DMA cache coherence

[Robin Murphy]

On 2022-04-08 13:22, Jason Gunthorpe wrote:
> On Fri, Apr 08, 2022 at 08:26:10AM +0000, Tian, Kevin wrote:
>>> From: Jason Gunthorpe <jgg@nvidia.com>
>>> Sent: Thursday, April 7, 2022 11:24 PM
>>>
>>> IOMMU_CACHE means that normal DMAs do not require any additional
>>> coherency
>>> mechanism and is the basic uAPI that VFIO exposes to userspace. For
>>> instance VFIO applications like DPDK will not work if additional coherency
>>> operations are required.
>>>
>>> Therefore check IOMMU_CAP_CACHE_COHERENCY like vdpa & usnic do
>>> before
>>> allowing an IOMMU backed VFIO device to be created.
>>>
>>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>>>   drivers/vfio/vfio.c | 7 +++++++
>>>   1 file changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>>> index a4555014bd1e72..9edad767cfdad3 100644
>>> +++ b/drivers/vfio/vfio.c
>>> @@ -815,6 +815,13 @@ static int __vfio_register_dev(struct vfio_device
>>> *device,
>>>
>>>   int vfio_register_group_dev(struct vfio_device *device)
>>>   {
>>> +	/*
>>> +	 * VFIO always sets IOMMU_CACHE because we offer no way for
>>> userspace to
>>> +	 * restore cache coherency.
>>> +	 */
>>> +	if (!iommu_capable(device->dev->bus,
>>> IOMMU_CAP_CACHE_COHERENCY))
>>> +		return -EINVAL;
>>> +
>>
>> One nit. Is it logistically more reasonable to put this patch before
>> changing VFIO to always set IOMMU_CACHE?
> 
> For bisectability it has to be after
> 
>      iommu: Redefine IOMMU_CAP_CACHE_COHERENCY as the cap flag for IOMMU_CACHE
> 
> Otherwise Intel iommu will stop working with VFIO
> 
> The ordering is OK as is because no IOMMU that works with VFIO cares
> about IOMMU_CACHE.

The Arm SMMU drivers do (without it even coherent traffic would be 
downgraded to non-cacheable), but then they also handle 
IOMMU_CAP_CACHE_COHERENCY nonsensically, and it happens to work out 
since AFAIK there aren't (yet) any Arm-based systems where you can 
reasonably try to use VFIO that don't also have hardware-coherent PCI. 
Thus I don't think there's any risk of regression for us here.

Robin.

Re: [PATCH v2 4/4] vfio: Require that devices support DMA cache coherence

[Robin Murphy]

On 2022-04-08 13:22, Jason Gunthorpe wrote:
> On Fri, Apr 08, 2022 at 08:26:10AM +0000, Tian, Kevin wrote:
>>> From: Jason Gunthorpe <jgg@nvidia.com>
>>> Sent: Thursday, April 7, 2022 11:24 PM
>>>
>>> IOMMU_CACHE means that normal DMAs do not require any additional
>>> coherency
>>> mechanism and is the basic uAPI that VFIO exposes to userspace. For
>>> instance VFIO applications like DPDK will not work if additional coherency
>>> operations are required.
>>>
>>> Therefore check IOMMU_CAP_CACHE_COHERENCY like vdpa & usnic do
>>> before
>>> allowing an IOMMU backed VFIO device to be created.
>>>
>>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>>>   drivers/vfio/vfio.c | 7 +++++++
>>>   1 file changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>>> index a4555014bd1e72..9edad767cfdad3 100644
>>> +++ b/drivers/vfio/vfio.c
>>> @@ -815,6 +815,13 @@ static int __vfio_register_dev(struct vfio_device
>>> *device,
>>>
>>>   int vfio_register_group_dev(struct vfio_device *device)
>>>   {
>>> +	/*
>>> +	 * VFIO always sets IOMMU_CACHE because we offer no way for
>>> userspace to
>>> +	 * restore cache coherency.
>>> +	 */
>>> +	if (!iommu_capable(device->dev->bus,
>>> IOMMU_CAP_CACHE_COHERENCY))
>>> +		return -EINVAL;
>>> +
>>
>> One nit. Is it logistically more reasonable to put this patch before
>> changing VFIO to always set IOMMU_CACHE?
> 
> For bisectability it has to be after
> 
>      iommu: Redefine IOMMU_CAP_CACHE_COHERENCY as the cap flag for IOMMU_CACHE
> 
> Otherwise Intel iommu will stop working with VFIO
> 
> The ordering is OK as is because no IOMMU that works with VFIO cares
> about IOMMU_CACHE.

The Arm SMMU drivers do (without it even coherent traffic would be 
downgraded to non-cacheable), but then they also handle 
IOMMU_CAP_CACHE_COHERENCY nonsensically, and it happens to work out 
since AFAIK there aren't (yet) any Arm-based systems where you can 
reasonably try to use VFIO that don't also have hardware-coherent PCI. 
Thus I don't think there's any risk of regression for us here.

Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Jason Gunthorpe]

On Fri, Apr 08, 2022 at 02:11:10PM +0100, Robin Murphy wrote:

> > However, this creates an oddball situation where the vfio_device and
> > it's struct device could become unplugged from the system while the
> > domain that the struct device spawned continues to exist and remains
> > attached to other devices in the same group. ie the iommu driver has
> > to be careful not to retain the struct device input..
> 
> Oh, I rather assumed that VFIO might automatically tear down the
> container/domain when the last real user disappears. 

It does, that isn't quite what I mean..

Lets say a simple case with two groups and two devices.

Open a VFIO container FD

We open group A and SET_CONTAINER it. This results in an
   domain_A = iommu_domain_alloc(device_A)
   iommu_attach_group(domain_A, device_A->group)

We open group B and SET_CONTAINER it. Using the sharing logic we end
up doing
   iommu_attach_group(domain_A, device_B->group)

Now we close group A FD, detatch device_A->group from domain_A and the
driver core hot-unplugs device A completely from the system.

However, domain_A remains in the system used by group B's open FD.

It is a bit funny at least.. I think it is just something to document
and be aware of for iommu driver writers that they probably shouldn't
try to store the allocation device in their domain struct.

IHMO the only purpose of the allocation device is to crystalize the
configuration of the iommu_domain at allocation time.

> as long as we take care not to release DMA ownership until that point also.
> As you say, it just looks a bit funny.

The DMA ownership should be OK as we take ownership on each group FD
open
 
> > I suppose that is inevitable to have sharing of domains across
> > devices, so the iommu drivers will have to accommodate this.
> 
> I think domain lifecycle management is already entirely up to the users and
> not something that IOMMU drivers need to worry about. Drivers should only
> need to look at per-device data in attach/detach (and, once I've finished,
> alloc) from the device argument which can be assumed to be valid at that
> point. Otherwise, all the relevant internal data for domain ops should
> belong to the domain already.

Making attach/detach take a struct device would be nice - but I would
expect the attach/detatch to use a strictly paired struct device and I
don't think this trick of selecting an arbitary vfio_device will
achieve that.

So, I suppose VFIO would want to attach/detatch on every vfio_device
individually and it would iterate over the group instead of doing a
list_first_entry() like above. This would not be hard to do in VFIO.

Not sure what the iommu layer would have to do to accommodate this..

Jason

Re: [PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Jason Gunthorpe via iommu]

On Fri, Apr 08, 2022 at 02:11:10PM +0100, Robin Murphy wrote:

> > However, this creates an oddball situation where the vfio_device and
> > it's struct device could become unplugged from the system while the
> > domain that the struct device spawned continues to exist and remains
> > attached to other devices in the same group. ie the iommu driver has
> > to be careful not to retain the struct device input..
> 
> Oh, I rather assumed that VFIO might automatically tear down the
> container/domain when the last real user disappears. 

It does, that isn't quite what I mean..

Lets say a simple case with two groups and two devices.

Open a VFIO container FD

We open group A and SET_CONTAINER it. This results in an
   domain_A = iommu_domain_alloc(device_A)
   iommu_attach_group(domain_A, device_A->group)

We open group B and SET_CONTAINER it. Using the sharing logic we end
up doing
   iommu_attach_group(domain_A, device_B->group)

Now we close group A FD, detatch device_A->group from domain_A and the
driver core hot-unplugs device A completely from the system.

However, domain_A remains in the system used by group B's open FD.

It is a bit funny at least.. I think it is just something to document
and be aware of for iommu driver writers that they probably shouldn't
try to store the allocation device in their domain struct.

IHMO the only purpose of the allocation device is to crystalize the
configuration of the iommu_domain at allocation time.

> as long as we take care not to release DMA ownership until that point also.
> As you say, it just looks a bit funny.

The DMA ownership should be OK as we take ownership on each group FD
open
 
> > I suppose that is inevitable to have sharing of domains across
> > devices, so the iommu drivers will have to accommodate this.
> 
> I think domain lifecycle management is already entirely up to the users and
> not something that IOMMU drivers need to worry about. Drivers should only
> need to look at per-device data in attach/detach (and, once I've finished,
> alloc) from the device argument which can be assumed to be valid at that
> point. Otherwise, all the relevant internal data for domain ops should
> belong to the domain already.

Making attach/detach take a struct device would be nice - but I would
expect the attach/detatch to use a strictly paired struct device and I
don't think this trick of selecting an arbitary vfio_device will
achieve that.

So, I suppose VFIO would want to attach/detatch on every vfio_device
individually and it would iterate over the group instead of doing a
list_first_entry() like above. This would not be hard to do in VFIO.

Not sure what the iommu layer would have to do to accommodate this..

Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 4/4] vfio: Require that devices support DMA cache coherence

[Jason Gunthorpe]

On Fri, Apr 08, 2022 at 02:28:02PM +0100, Robin Murphy wrote:

> > > One nit. Is it logistically more reasonable to put this patch before
> > > changing VFIO to always set IOMMU_CACHE?
> > 
> > For bisectability it has to be after
> > 
> >      iommu: Redefine IOMMU_CAP_CACHE_COHERENCY as the cap flag for IOMMU_CACHE
> > 
> > Otherwise Intel iommu will stop working with VFIO
> > 
> > The ordering is OK as is because no IOMMU that works with VFIO cares
> > about IOMMU_CACHE.
> 
> The Arm SMMU drivers do (without it even coherent traffic would be
> downgraded to non-cacheable), but then they also handle
> IOMMU_CAP_CACHE_COHERENCY nonsensically, and it happens to work out since
> AFAIK there aren't (yet) any Arm-based systems where you can reasonably try
> to use VFIO that don't also have hardware-coherent PCI. Thus I don't think
> there's any risk of regression for us here.

Right, I was unclear, I meant 'requires IOMMU_CACHE to be unset to
work with VFIO'

Jason

Re: [PATCH v2 4/4] vfio: Require that devices support DMA cache coherence

[Jason Gunthorpe via iommu]

On Fri, Apr 08, 2022 at 02:28:02PM +0100, Robin Murphy wrote:

> > > One nit. Is it logistically more reasonable to put this patch before
> > > changing VFIO to always set IOMMU_CACHE?
> > 
> > For bisectability it has to be after
> > 
> >      iommu: Redefine IOMMU_CAP_CACHE_COHERENCY as the cap flag for IOMMU_CACHE
> > 
> > Otherwise Intel iommu will stop working with VFIO
> > 
> > The ordering is OK as is because no IOMMU that works with VFIO cares
> > about IOMMU_CACHE.
> 
> The Arm SMMU drivers do (without it even coherent traffic would be
> downgraded to non-cacheable), but then they also handle
> IOMMU_CAP_CACHE_COHERENCY nonsensically, and it happens to work out since
> AFAIK there aren't (yet) any Arm-based systems where you can reasonably try
> to use VFIO that don't also have hardware-coherent PCI. Thus I don't think
> there's any risk of regression for us here.

Right, I was unclear, I meant 'requires IOMMU_CACHE to be unset to
work with VFIO'

Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 2/4] vfio: Move the Intel no-snoop control off of IOMMU_CACHE

[Alex Williamson]

On Thu,  7 Apr 2022 12:23:45 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> IOMMU_CACHE means "normal DMA to this iommu_domain's IOVA should be cache
> coherent" and is used by the DMA API. The definition allows for special
> non-coherent DMA to exist - ie processing of the no-snoop flag in PCIe
> TLPs - so long as this behavior is opt-in by the device driver.
> 
> The flag is mainly used by the DMA API to synchronize the IOMMU setting
> with the expected cache behavior of the DMA master. eg based on
> dev_is_dma_coherent() in some case.
> 
> For Intel IOMMU IOMMU_CACHE was redefined to mean 'force all DMA to be
> cache coherent' which has the practical effect of causing the IOMMU to
> ignore the no-snoop bit in a PCIe TLP.
> 
> x86 platforms are always IOMMU_CACHE, so Intel should ignore this flag.
> 
> Instead use the new domain op enforce_cache_coherency() which causes every
> IOPTE created in the domain to have the no-snoop blocking behavior.
> 
> Reconfigure VFIO to always use IOMMU_CACHE and call
> enforce_cache_coherency() to operate the special Intel behavior.
> 
> Remove the IOMMU_CACHE test from Intel IOMMU.
> 
> Ultimately VFIO plumbs the result of enforce_cache_coherency() back into
> the x86 platform code through kvm_arch_register_noncoherent_dma() which
> controls if the WBINVD instruction is available in the guest. No other
> arch implements kvm_arch_register_noncoherent_dma().

I think this last sentence is alluding to it, but I wish the user
visible change to VFIO_DMA_CC_IOMMU on non-x86 were more explicit.
Perhaps for the last sentence:

  No other archs implement kvm_arch_register_noncoherent_dma() nor are
  there any other known consumers of VFIO_DMA_CC_IOMMU that might be
  affected by the user visible result change on non-x86 archs.

Otherwise,

Acked-by: Alex Williamson <alex.williamson@redhat.com>

> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/intel/iommu.c     |  7 ++-----
>  drivers/vfio/vfio_iommu_type1.c | 30 +++++++++++++++++++-----------
>  include/linux/intel-iommu.h     |  1 -
>  3 files changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index f08611a6cc4799..8f3674e997df06 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -641,7 +641,6 @@ static unsigned long domain_super_pgsize_bitmap(struct dmar_domain *domain)
>  static void domain_update_iommu_cap(struct dmar_domain *domain)
>  {
>  	domain_update_iommu_coherency(domain);
> -	domain->iommu_snooping = domain_update_iommu_snooping(NULL);
>  	domain->iommu_superpage = domain_update_iommu_superpage(domain, NULL);
>  
>  	/*
> @@ -4283,7 +4282,6 @@ static int md_domain_init(struct dmar_domain *domain, int guest_width)
>  	domain->agaw = width_to_agaw(adjust_width);
>  
>  	domain->iommu_coherency = false;
> -	domain->iommu_snooping = false;
>  	domain->iommu_superpage = 0;
>  	domain->max_addr = 0;
>  
> @@ -4422,8 +4420,7 @@ static int intel_iommu_map(struct iommu_domain *domain,
>  		prot |= DMA_PTE_READ;
>  	if (iommu_prot & IOMMU_WRITE)
>  		prot |= DMA_PTE_WRITE;
> -	if (((iommu_prot & IOMMU_CACHE) && dmar_domain->iommu_snooping) ||
> -	    dmar_domain->enforce_no_snoop)
> +	if (dmar_domain->enforce_no_snoop)
>  		prot |= DMA_PTE_SNP;
>  
>  	max_addr = iova + size;
> @@ -4550,7 +4547,7 @@ static bool intel_iommu_enforce_cache_coherency(struct iommu_domain *domain)
>  {
>  	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
>  
> -	if (!dmar_domain->iommu_snooping)
> +	if (!domain_update_iommu_snooping(NULL))
>  		return false;
>  	dmar_domain->enforce_no_snoop = true;
>  	return true;
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 9394aa9444c10c..c13b9290e35759 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -84,8 +84,8 @@ struct vfio_domain {
>  	struct iommu_domain	*domain;
>  	struct list_head	next;
>  	struct list_head	group_list;
> -	int			prot;		/* IOMMU_CACHE */
> -	bool			fgsp;		/* Fine-grained super pages */
> +	bool			fgsp : 1;	/* Fine-grained super pages */
> +	bool			enforce_cache_coherency : 1;
>  };
>  
>  struct vfio_dma {
> @@ -1461,7 +1461,7 @@ static int vfio_iommu_map(struct vfio_iommu *iommu, dma_addr_t iova,
>  
>  	list_for_each_entry(d, &iommu->domain_list, next) {
>  		ret = iommu_map(d->domain, iova, (phys_addr_t)pfn << PAGE_SHIFT,
> -				npage << PAGE_SHIFT, prot | d->prot);
> +				npage << PAGE_SHIFT, prot | IOMMU_CACHE);
>  		if (ret)
>  			goto unwind;
>  
> @@ -1771,7 +1771,7 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
>  			}
>  
>  			ret = iommu_map(domain->domain, iova, phys,
> -					size, dma->prot | domain->prot);
> +					size, dma->prot | IOMMU_CACHE);
>  			if (ret) {
>  				if (!dma->iommu_mapped) {
>  					vfio_unpin_pages_remote(dma, iova,
> @@ -1859,7 +1859,7 @@ static void vfio_test_domain_fgsp(struct vfio_domain *domain)
>  		return;
>  
>  	ret = iommu_map(domain->domain, 0, page_to_phys(pages), PAGE_SIZE * 2,
> -			IOMMU_READ | IOMMU_WRITE | domain->prot);
> +			IOMMU_READ | IOMMU_WRITE | IOMMU_CACHE);
>  	if (!ret) {
>  		size_t unmapped = iommu_unmap(domain->domain, 0, PAGE_SIZE);
>  
> @@ -2267,8 +2267,15 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  		goto out_detach;
>  	}
>  
> -	if (iommu_capable(bus, IOMMU_CAP_CACHE_COHERENCY))
> -		domain->prot |= IOMMU_CACHE;
> +	/*
> +	 * If the IOMMU can block non-coherent operations (ie PCIe TLPs with
> +	 * no-snoop set) then VFIO always turns this feature on because on Intel
> +	 * platforms it optimizes KVM to disable wbinvd emulation.
> +	 */
> +	if (domain->domain->ops->enforce_cache_coherency)
> +		domain->enforce_cache_coherency =
> +			domain->domain->ops->enforce_cache_coherency(
> +				domain->domain);
>  
>  	/*
>  	 * Try to match an existing compatible domain.  We don't want to
> @@ -2279,7 +2286,8 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  	 */
>  	list_for_each_entry(d, &iommu->domain_list, next) {
>  		if (d->domain->ops == domain->domain->ops &&
> -		    d->prot == domain->prot) {
> +		    d->enforce_cache_coherency ==
> +			    domain->enforce_cache_coherency) {
>  			iommu_detach_group(domain->domain, group->iommu_group);
>  			if (!iommu_attach_group(d->domain,
>  						group->iommu_group)) {
> @@ -2611,14 +2619,14 @@ static void vfio_iommu_type1_release(void *iommu_data)
>  	kfree(iommu);
>  }
>  
> -static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
> +static int vfio_domains_have_enforce_cache_coherency(struct vfio_iommu *iommu)
>  {
>  	struct vfio_domain *domain;
>  	int ret = 1;
>  
>  	mutex_lock(&iommu->lock);
>  	list_for_each_entry(domain, &iommu->domain_list, next) {
> -		if (!(domain->prot & IOMMU_CACHE)) {
> +		if (!(domain->enforce_cache_coherency)) {
>  			ret = 0;
>  			break;
>  		}
> @@ -2641,7 +2649,7 @@ static int vfio_iommu_type1_check_extension(struct vfio_iommu *iommu,
>  	case VFIO_DMA_CC_IOMMU:
>  		if (!iommu)
>  			return 0;
> -		return vfio_domains_have_iommu_cache(iommu);
> +		return vfio_domains_have_enforce_cache_coherency(iommu);
>  	default:
>  		return 0;
>  	}
> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
> index 1f930c0c225d94..bc39f633efdf03 100644
> --- a/include/linux/intel-iommu.h
> +++ b/include/linux/intel-iommu.h
> @@ -539,7 +539,6 @@ struct dmar_domain {
>  
>  	u8 has_iotlb_device: 1;
>  	u8 iommu_coherency: 1;		/* indicate coherency of iommu access */
> -	u8 iommu_snooping: 1;		/* indicate snooping control feature */
>  	u8 enforce_no_snoop : 1;        /* Create IOPTEs with snoop control */
>  
>  	struct list_head devices;	/* all devices' list */

Re: [PATCH v2 2/4] vfio: Move the Intel no-snoop control off of IOMMU_CACHE

[Alex Williamson]

On Thu,  7 Apr 2022 12:23:45 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> IOMMU_CACHE means "normal DMA to this iommu_domain's IOVA should be cache
> coherent" and is used by the DMA API. The definition allows for special
> non-coherent DMA to exist - ie processing of the no-snoop flag in PCIe
> TLPs - so long as this behavior is opt-in by the device driver.
> 
> The flag is mainly used by the DMA API to synchronize the IOMMU setting
> with the expected cache behavior of the DMA master. eg based on
> dev_is_dma_coherent() in some case.
> 
> For Intel IOMMU IOMMU_CACHE was redefined to mean 'force all DMA to be
> cache coherent' which has the practical effect of causing the IOMMU to
> ignore the no-snoop bit in a PCIe TLP.
> 
> x86 platforms are always IOMMU_CACHE, so Intel should ignore this flag.
> 
> Instead use the new domain op enforce_cache_coherency() which causes every
> IOPTE created in the domain to have the no-snoop blocking behavior.
> 
> Reconfigure VFIO to always use IOMMU_CACHE and call
> enforce_cache_coherency() to operate the special Intel behavior.
> 
> Remove the IOMMU_CACHE test from Intel IOMMU.
> 
> Ultimately VFIO plumbs the result of enforce_cache_coherency() back into
> the x86 platform code through kvm_arch_register_noncoherent_dma() which
> controls if the WBINVD instruction is available in the guest. No other
> arch implements kvm_arch_register_noncoherent_dma().

I think this last sentence is alluding to it, but I wish the user
visible change to VFIO_DMA_CC_IOMMU on non-x86 were more explicit.
Perhaps for the last sentence:

  No other archs implement kvm_arch_register_noncoherent_dma() nor are
  there any other known consumers of VFIO_DMA_CC_IOMMU that might be
  affected by the user visible result change on non-x86 archs.

Otherwise,

Acked-by: Alex Williamson <alex.williamson@redhat.com>

> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/intel/iommu.c     |  7 ++-----
>  drivers/vfio/vfio_iommu_type1.c | 30 +++++++++++++++++++-----------
>  include/linux/intel-iommu.h     |  1 -
>  3 files changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index f08611a6cc4799..8f3674e997df06 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -641,7 +641,6 @@ static unsigned long domain_super_pgsize_bitmap(struct dmar_domain *domain)
>  static void domain_update_iommu_cap(struct dmar_domain *domain)
>  {
>  	domain_update_iommu_coherency(domain);
> -	domain->iommu_snooping = domain_update_iommu_snooping(NULL);
>  	domain->iommu_superpage = domain_update_iommu_superpage(domain, NULL);
>  
>  	/*
> @@ -4283,7 +4282,6 @@ static int md_domain_init(struct dmar_domain *domain, int guest_width)
>  	domain->agaw = width_to_agaw(adjust_width);
>  
>  	domain->iommu_coherency = false;
> -	domain->iommu_snooping = false;
>  	domain->iommu_superpage = 0;
>  	domain->max_addr = 0;
>  
> @@ -4422,8 +4420,7 @@ static int intel_iommu_map(struct iommu_domain *domain,
>  		prot |= DMA_PTE_READ;
>  	if (iommu_prot & IOMMU_WRITE)
>  		prot |= DMA_PTE_WRITE;
> -	if (((iommu_prot & IOMMU_CACHE) && dmar_domain->iommu_snooping) ||
> -	    dmar_domain->enforce_no_snoop)
> +	if (dmar_domain->enforce_no_snoop)
>  		prot |= DMA_PTE_SNP;
>  
>  	max_addr = iova + size;
> @@ -4550,7 +4547,7 @@ static bool intel_iommu_enforce_cache_coherency(struct iommu_domain *domain)
>  {
>  	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
>  
> -	if (!dmar_domain->iommu_snooping)
> +	if (!domain_update_iommu_snooping(NULL))
>  		return false;
>  	dmar_domain->enforce_no_snoop = true;
>  	return true;
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 9394aa9444c10c..c13b9290e35759 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -84,8 +84,8 @@ struct vfio_domain {
>  	struct iommu_domain	*domain;
>  	struct list_head	next;
>  	struct list_head	group_list;
> -	int			prot;		/* IOMMU_CACHE */
> -	bool			fgsp;		/* Fine-grained super pages */
> +	bool			fgsp : 1;	/* Fine-grained super pages */
> +	bool			enforce_cache_coherency : 1;
>  };
>  
>  struct vfio_dma {
> @@ -1461,7 +1461,7 @@ static int vfio_iommu_map(struct vfio_iommu *iommu, dma_addr_t iova,
>  
>  	list_for_each_entry(d, &iommu->domain_list, next) {
>  		ret = iommu_map(d->domain, iova, (phys_addr_t)pfn << PAGE_SHIFT,
> -				npage << PAGE_SHIFT, prot | d->prot);
> +				npage << PAGE_SHIFT, prot | IOMMU_CACHE);
>  		if (ret)
>  			goto unwind;
>  
> @@ -1771,7 +1771,7 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
>  			}
>  
>  			ret = iommu_map(domain->domain, iova, phys,
> -					size, dma->prot | domain->prot);
> +					size, dma->prot | IOMMU_CACHE);
>  			if (ret) {
>  				if (!dma->iommu_mapped) {
>  					vfio_unpin_pages_remote(dma, iova,
> @@ -1859,7 +1859,7 @@ static void vfio_test_domain_fgsp(struct vfio_domain *domain)
>  		return;
>  
>  	ret = iommu_map(domain->domain, 0, page_to_phys(pages), PAGE_SIZE * 2,
> -			IOMMU_READ | IOMMU_WRITE | domain->prot);
> +			IOMMU_READ | IOMMU_WRITE | IOMMU_CACHE);
>  	if (!ret) {
>  		size_t unmapped = iommu_unmap(domain->domain, 0, PAGE_SIZE);
>  
> @@ -2267,8 +2267,15 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  		goto out_detach;
>  	}
>  
> -	if (iommu_capable(bus, IOMMU_CAP_CACHE_COHERENCY))
> -		domain->prot |= IOMMU_CACHE;
> +	/*
> +	 * If the IOMMU can block non-coherent operations (ie PCIe TLPs with
> +	 * no-snoop set) then VFIO always turns this feature on because on Intel
> +	 * platforms it optimizes KVM to disable wbinvd emulation.
> +	 */
> +	if (domain->domain->ops->enforce_cache_coherency)
> +		domain->enforce_cache_coherency =
> +			domain->domain->ops->enforce_cache_coherency(
> +				domain->domain);
>  
>  	/*
>  	 * Try to match an existing compatible domain.  We don't want to
> @@ -2279,7 +2286,8 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  	 */
>  	list_for_each_entry(d, &iommu->domain_list, next) {
>  		if (d->domain->ops == domain->domain->ops &&
> -		    d->prot == domain->prot) {
> +		    d->enforce_cache_coherency ==
> +			    domain->enforce_cache_coherency) {
>  			iommu_detach_group(domain->domain, group->iommu_group);
>  			if (!iommu_attach_group(d->domain,
>  						group->iommu_group)) {
> @@ -2611,14 +2619,14 @@ static void vfio_iommu_type1_release(void *iommu_data)
>  	kfree(iommu);
>  }
>  
> -static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
> +static int vfio_domains_have_enforce_cache_coherency(struct vfio_iommu *iommu)
>  {
>  	struct vfio_domain *domain;
>  	int ret = 1;
>  
>  	mutex_lock(&iommu->lock);
>  	list_for_each_entry(domain, &iommu->domain_list, next) {
> -		if (!(domain->prot & IOMMU_CACHE)) {
> +		if (!(domain->enforce_cache_coherency)) {
>  			ret = 0;
>  			break;
>  		}
> @@ -2641,7 +2649,7 @@ static int vfio_iommu_type1_check_extension(struct vfio_iommu *iommu,
>  	case VFIO_DMA_CC_IOMMU:
>  		if (!iommu)
>  			return 0;
> -		return vfio_domains_have_iommu_cache(iommu);
> +		return vfio_domains_have_enforce_cache_coherency(iommu);
>  	default:
>  		return 0;
>  	}
> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
> index 1f930c0c225d94..bc39f633efdf03 100644
> --- a/include/linux/intel-iommu.h
> +++ b/include/linux/intel-iommu.h
> @@ -539,7 +539,6 @@ struct dmar_domain {
>  
>  	u8 has_iotlb_device: 1;
>  	u8 iommu_coherency: 1;		/* indicate coherency of iommu access */
> -	u8 iommu_snooping: 1;		/* indicate snooping control feature */
>  	u8 enforce_no_snoop : 1;        /* Create IOPTEs with snoop control */
>  
>  	struct list_head devices;	/* all devices' list */

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 4/4] vfio: Require that devices support DMA cache coherence

[Alex Williamson]

On Thu,  7 Apr 2022 12:23:47 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> IOMMU_CACHE means that normal DMAs do not require any additional coherency
> mechanism and is the basic uAPI that VFIO exposes to userspace. For
> instance VFIO applications like DPDK will not work if additional coherency
> operations are required.
> 
> Therefore check IOMMU_CAP_CACHE_COHERENCY like vdpa & usnic do before
> allowing an IOMMU backed VFIO device to be created.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/vfio/vfio.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index a4555014bd1e72..9edad767cfdad3 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -815,6 +815,13 @@ static int __vfio_register_dev(struct vfio_device *device,
>  
>  int vfio_register_group_dev(struct vfio_device *device)
>  {
> +	/*
> +	 * VFIO always sets IOMMU_CACHE because we offer no way for userspace to
> +	 * restore cache coherency.
> +	 */
> +	if (!iommu_capable(device->dev->bus, IOMMU_CAP_CACHE_COHERENCY))
> +		return -EINVAL;
> +
>  	return __vfio_register_dev(device,
>  		vfio_group_find_or_alloc(device->dev));
>  }

Acked-by: Alex Williamson <alex.williamson@redhat.com>

Re: [PATCH v2 4/4] vfio: Require that devices support DMA cache coherence

[Alex Williamson]

On Thu,  7 Apr 2022 12:23:47 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> IOMMU_CACHE means that normal DMAs do not require any additional coherency
> mechanism and is the basic uAPI that VFIO exposes to userspace. For
> instance VFIO applications like DPDK will not work if additional coherency
> operations are required.
> 
> Therefore check IOMMU_CAP_CACHE_COHERENCY like vdpa & usnic do before
> allowing an IOMMU backed VFIO device to be created.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/vfio/vfio.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index a4555014bd1e72..9edad767cfdad3 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -815,6 +815,13 @@ static int __vfio_register_dev(struct vfio_device *device,
>  
>  int vfio_register_group_dev(struct vfio_device *device)
>  {
> +	/*
> +	 * VFIO always sets IOMMU_CACHE because we offer no way for userspace to
> +	 * restore cache coherency.
> +	 */
> +	if (!iommu_capable(device->dev->bus, IOMMU_CAP_CACHE_COHERENCY))
> +		return -EINVAL;
> +
>  	return __vfio_register_dev(device,
>  		vfio_group_find_or_alloc(device->dev));
>  }

Acked-by: Alex Williamson <alex.williamson@redhat.com>

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Robin Murphy]

On 2022-04-08 14:35, Jason Gunthorpe wrote:
> On Fri, Apr 08, 2022 at 02:11:10PM +0100, Robin Murphy wrote:
> 
>>> However, this creates an oddball situation where the vfio_device and
>>> it's struct device could become unplugged from the system while the
>>> domain that the struct device spawned continues to exist and remains
>>> attached to other devices in the same group. ie the iommu driver has
>>> to be careful not to retain the struct device input..
>>
>> Oh, I rather assumed that VFIO might automatically tear down the
>> container/domain when the last real user disappears.
> 
> It does, that isn't quite what I mean..
> 
> Lets say a simple case with two groups and two devices.
> 
> Open a VFIO container FD
> 
> We open group A and SET_CONTAINER it. This results in an
>     domain_A = iommu_domain_alloc(device_A)
>     iommu_attach_group(domain_A, device_A->group)
> 
> We open group B and SET_CONTAINER it. Using the sharing logic we end
> up doing
>     iommu_attach_group(domain_A, device_B->group)
> 
> Now we close group A FD, detatch device_A->group from domain_A and the
> driver core hot-unplugs device A completely from the system.
> 
> However, domain_A remains in the system used by group B's open FD.
> 
> It is a bit funny at least.. I think it is just something to document
> and be aware of for iommu driver writers that they probably shouldn't
> try to store the allocation device in their domain struct.
> 
> IHMO the only purpose of the allocation device is to crystalize the
> configuration of the iommu_domain at allocation time.

Oh, for sure. When I implement the API switch, I can certainly try to 
document it as clearly as possible that the device argument is only for 
resolving the correct IOMMU ops and target instance, and the resulting 
domain is still not in any way tied to that specific device.

I hadn't thought about how it might look to future developers who aren't 
already familiar with all the history here, so thanks for the nudge!

>> as long as we take care not to release DMA ownership until that point also.
>> As you say, it just looks a bit funny.
> 
> The DMA ownership should be OK as we take ownership on each group FD
> open
>   
>>> I suppose that is inevitable to have sharing of domains across
>>> devices, so the iommu drivers will have to accommodate this.
>>
>> I think domain lifecycle management is already entirely up to the users and
>> not something that IOMMU drivers need to worry about. Drivers should only
>> need to look at per-device data in attach/detach (and, once I've finished,
>> alloc) from the device argument which can be assumed to be valid at that
>> point. Otherwise, all the relevant internal data for domain ops should
>> belong to the domain already.
> 
> Making attach/detach take a struct device would be nice - but I would
> expect the attach/detatch to use a strictly paired struct device and I
> don't think this trick of selecting an arbitary vfio_device will
> achieve that.
> 
> So, I suppose VFIO would want to attach/detatch on every vfio_device
> individually and it would iterate over the group instead of doing a
> list_first_entry() like above. This would not be hard to do in VFIO.

It feels like we've already beaten that discussion to death in other 
threads; regardless of what exact argument the iommu_attach/detach 
operations end up taking, they have to operate on the whole (explicit or 
implicit) iommu_group at once, because doing anything else would defeat 
the point of isolation groups, and be impossible for alias groups.

> Not sure what the iommu layer would have to do to accommodate this..

If it's significantly easier for VFIO to just run through a whole list 
of devices and attach each one without having to keep track of whether 
they might share an iommu_group which has already been attached, then we 
can probably relax the API a little such that attaching to a domain 
which is already the current domain becomes a no-op instead of returning 
-EBUSY, but I'd rather not create an expectation that anyone *has* to do 
that. For any other callers that would be forcing *more* iommu_group 
implementation details onto them, when we all want less.

Cheers,
Robin.

Re: [PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Robin Murphy]

On 2022-04-08 14:35, Jason Gunthorpe wrote:
> On Fri, Apr 08, 2022 at 02:11:10PM +0100, Robin Murphy wrote:
> 
>>> However, this creates an oddball situation where the vfio_device and
>>> it's struct device could become unplugged from the system while the
>>> domain that the struct device spawned continues to exist and remains
>>> attached to other devices in the same group. ie the iommu driver has
>>> to be careful not to retain the struct device input..
>>
>> Oh, I rather assumed that VFIO might automatically tear down the
>> container/domain when the last real user disappears.
> 
> It does, that isn't quite what I mean..
> 
> Lets say a simple case with two groups and two devices.
> 
> Open a VFIO container FD
> 
> We open group A and SET_CONTAINER it. This results in an
>     domain_A = iommu_domain_alloc(device_A)
>     iommu_attach_group(domain_A, device_A->group)
> 
> We open group B and SET_CONTAINER it. Using the sharing logic we end
> up doing
>     iommu_attach_group(domain_A, device_B->group)
> 
> Now we close group A FD, detatch device_A->group from domain_A and the
> driver core hot-unplugs device A completely from the system.
> 
> However, domain_A remains in the system used by group B's open FD.
> 
> It is a bit funny at least.. I think it is just something to document
> and be aware of for iommu driver writers that they probably shouldn't
> try to store the allocation device in their domain struct.
> 
> IHMO the only purpose of the allocation device is to crystalize the
> configuration of the iommu_domain at allocation time.

Oh, for sure. When I implement the API switch, I can certainly try to 
document it as clearly as possible that the device argument is only for 
resolving the correct IOMMU ops and target instance, and the resulting 
domain is still not in any way tied to that specific device.

I hadn't thought about how it might look to future developers who aren't 
already familiar with all the history here, so thanks for the nudge!

>> as long as we take care not to release DMA ownership until that point also.
>> As you say, it just looks a bit funny.
> 
> The DMA ownership should be OK as we take ownership on each group FD
> open
>   
>>> I suppose that is inevitable to have sharing of domains across
>>> devices, so the iommu drivers will have to accommodate this.
>>
>> I think domain lifecycle management is already entirely up to the users and
>> not something that IOMMU drivers need to worry about. Drivers should only
>> need to look at per-device data in attach/detach (and, once I've finished,
>> alloc) from the device argument which can be assumed to be valid at that
>> point. Otherwise, all the relevant internal data for domain ops should
>> belong to the domain already.
> 
> Making attach/detach take a struct device would be nice - but I would
> expect the attach/detatch to use a strictly paired struct device and I
> don't think this trick of selecting an arbitary vfio_device will
> achieve that.
> 
> So, I suppose VFIO would want to attach/detatch on every vfio_device
> individually and it would iterate over the group instead of doing a
> list_first_entry() like above. This would not be hard to do in VFIO.

It feels like we've already beaten that discussion to death in other 
threads; regardless of what exact argument the iommu_attach/detach 
operations end up taking, they have to operate on the whole (explicit or 
implicit) iommu_group at once, because doing anything else would defeat 
the point of isolation groups, and be impossible for alias groups.

> Not sure what the iommu layer would have to do to accommodate this..

If it's significantly easier for VFIO to just run through a whole list 
of devices and attach each one without having to keep track of whether 
they might share an iommu_group which has already been attached, then we 
can probably relax the API a little such that attaching to a domain 
which is already the current domain becomes a no-op instead of returning 
-EBUSY, but I'd rather not create an expectation that anyone *has* to do 
that. For any other callers that would be forcing *more* iommu_group 
implementation details onto them, when we all want less.

Cheers,
Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/4] iommu: Introduce the domain op enforce_cache_coherency()

[Lu Baolu]

On 2022/4/8 16:05, Tian, Kevin wrote:
>> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
>> index 2f9891cb3d0014..1f930c0c225d94 100644
>> --- a/include/linux/intel-iommu.h
>> +++ b/include/linux/intel-iommu.h
>> @@ -540,6 +540,7 @@ struct dmar_domain {
>>   	u8 has_iotlb_device: 1;
>>   	u8 iommu_coherency: 1;		/* indicate coherency of
>> iommu access */
>>   	u8 iommu_snooping: 1;		/* indicate snooping control
>> feature */
>> +	u8 enforce_no_snoop : 1;        /* Create IOPTEs with snoop control */
> it reads like no_snoop is the result of the enforcement... Probably
> force_snooping better matches the intention here.

+1

Other changes in iommu/vt-d looks good to me.

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

Re: [PATCH v2 1/4] iommu: Introduce the domain op enforce_cache_coherency()

[Lu Baolu]

On 2022/4/8 16:05, Tian, Kevin wrote:
>> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
>> index 2f9891cb3d0014..1f930c0c225d94 100644
>> --- a/include/linux/intel-iommu.h
>> +++ b/include/linux/intel-iommu.h
>> @@ -540,6 +540,7 @@ struct dmar_domain {
>>   	u8 has_iotlb_device: 1;
>>   	u8 iommu_coherency: 1;		/* indicate coherency of
>> iommu access */
>>   	u8 iommu_snooping: 1;		/* indicate snooping control
>> feature */
>> +	u8 enforce_no_snoop : 1;        /* Create IOPTEs with snoop control */
> it reads like no_snoop is the result of the enforcement... Probably
> force_snooping better matches the intention here.

+1

Other changes in iommu/vt-d looks good to me.

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 2/4] vfio: Move the Intel no-snoop control off of IOMMU_CACHE

[Lu Baolu]

On 2022/4/8 16:16, Tian, Kevin wrote:
>> From: Jason Gunthorpe <jgg@nvidia.com>
>> Sent: Thursday, April 7, 2022 11:24 PM
>>
>> IOMMU_CACHE means "normal DMA to this iommu_domain's IOVA should
>> be cache
>> coherent" and is used by the DMA API. The definition allows for special
>> non-coherent DMA to exist - ie processing of the no-snoop flag in PCIe
>> TLPs - so long as this behavior is opt-in by the device driver.
>>
>> The flag is mainly used by the DMA API to synchronize the IOMMU setting
>> with the expected cache behavior of the DMA master. eg based on
>> dev_is_dma_coherent() in some case.
>>
>> For Intel IOMMU IOMMU_CACHE was redefined to mean 'force all DMA to
>> be
>> cache coherent' which has the practical effect of causing the IOMMU to
>> ignore the no-snoop bit in a PCIe TLP.
>>
>> x86 platforms are always IOMMU_CACHE, so Intel should ignore this flag.
>>
>> Instead use the new domain op enforce_cache_coherency() which causes
>> every
>> IOPTE created in the domain to have the no-snoop blocking behavior.
>>
>> Reconfigure VFIO to always use IOMMU_CACHE and call
>> enforce_cache_coherency() to operate the special Intel behavior.
>>
>> Remove the IOMMU_CACHE test from Intel IOMMU.
>>
>> Ultimately VFIO plumbs the result of enforce_cache_coherency() back into
>> the x86 platform code through kvm_arch_register_noncoherent_dma()
>> which
>> controls if the WBINVD instruction is available in the guest. No other
>> arch implements kvm_arch_register_noncoherent_dma().
>>
>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> 
> btw as discussed in last version it is not necessarily to recalculate
> snoop control globally with this new approach. Will follow up to
> clean it up after this series is merged.

Agreed. But it also requires the enforce_cache_coherency() to be called
only after domain being attached to a device just as VFIO is doing.

Anyway, for this change in iommu/vt-d:

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

Re: [PATCH v2 2/4] vfio: Move the Intel no-snoop control off of IOMMU_CACHE

[Lu Baolu]

On 2022/4/8 16:16, Tian, Kevin wrote:
>> From: Jason Gunthorpe <jgg@nvidia.com>
>> Sent: Thursday, April 7, 2022 11:24 PM
>>
>> IOMMU_CACHE means "normal DMA to this iommu_domain's IOVA should
>> be cache
>> coherent" and is used by the DMA API. The definition allows for special
>> non-coherent DMA to exist - ie processing of the no-snoop flag in PCIe
>> TLPs - so long as this behavior is opt-in by the device driver.
>>
>> The flag is mainly used by the DMA API to synchronize the IOMMU setting
>> with the expected cache behavior of the DMA master. eg based on
>> dev_is_dma_coherent() in some case.
>>
>> For Intel IOMMU IOMMU_CACHE was redefined to mean 'force all DMA to
>> be
>> cache coherent' which has the practical effect of causing the IOMMU to
>> ignore the no-snoop bit in a PCIe TLP.
>>
>> x86 platforms are always IOMMU_CACHE, so Intel should ignore this flag.
>>
>> Instead use the new domain op enforce_cache_coherency() which causes
>> every
>> IOPTE created in the domain to have the no-snoop blocking behavior.
>>
>> Reconfigure VFIO to always use IOMMU_CACHE and call
>> enforce_cache_coherency() to operate the special Intel behavior.
>>
>> Remove the IOMMU_CACHE test from Intel IOMMU.
>>
>> Ultimately VFIO plumbs the result of enforce_cache_coherency() back into
>> the x86 platform code through kvm_arch_register_noncoherent_dma()
>> which
>> controls if the WBINVD instruction is available in the guest. No other
>> arch implements kvm_arch_register_noncoherent_dma().
>>
>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> 
> btw as discussed in last version it is not necessarily to recalculate
> snoop control globally with this new approach. Will follow up to
> clean it up after this series is merged.

Agreed. But it also requires the enforce_cache_coherency() to be called
only after domain being attached to a device just as VFIO is doing.

Anyway, for this change in iommu/vt-d:

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 3/4] iommu: Redefine IOMMU_CAP_CACHE_COHERENCY as the cap flag for IOMMU_CACHE

[Lu Baolu]

On 2022/4/7 23:23, Jason Gunthorpe wrote:
> While the comment was correct that this flag was intended to convey the
> block no-snoop support in the IOMMU, it has become widely implemented and
> used to mean the IOMMU supports IOMMU_CACHE as a map flag. Only the Intel
> driver was different.
> 
> Now that the Intel driver is using enforce_cache_coherency() update the
> comment to make it clear that IOMMU_CAP_CACHE_COHERENCY is only about
> IOMMU_CACHE.  Fix the Intel driver to return true since IOMMU_CACHE always
> works.
> 
> The two places that test this flag, usnic and vdpa, are both assigning
> userspace pages to a driver controlled iommu_domain and require
> IOMMU_CACHE behavior as they offer no way for userspace to synchronize
> caches.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/iommu/intel/iommu.c | 2 +-
>   include/linux/iommu.h       | 3 +--
>   2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 8f3674e997df06..14ba185175e9ec 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4556,7 +4556,7 @@ static bool intel_iommu_enforce_cache_coherency(struct iommu_domain *domain)
>   static bool intel_iommu_capable(enum iommu_cap cap)
>   {
>   	if (cap == IOMMU_CAP_CACHE_COHERENCY)
> -		return domain_update_iommu_snooping(NULL);
> +		return true;
>   	if (cap == IOMMU_CAP_INTR_REMAP)
>   		return irq_remapping_enabled == 1;
>   
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index fe4f24c469c373..fd58f7adc52796 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -103,8 +103,7 @@ static inline bool iommu_is_dma_domain(struct iommu_domain *domain)
>   }
>   
>   enum iommu_cap {
> -	IOMMU_CAP_CACHE_COHERENCY,	/* IOMMU can enforce cache coherent DMA
> -					   transactions */
> +	IOMMU_CAP_CACHE_COHERENCY,	/* IOMMU_CACHE is supported */
>   	IOMMU_CAP_INTR_REMAP,		/* IOMMU supports interrupt isolation */
>   	IOMMU_CAP_NOEXEC,		/* IOMMU_NOEXEC flag */
>   };

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

Re: [PATCH v2 3/4] iommu: Redefine IOMMU_CAP_CACHE_COHERENCY as the cap flag for IOMMU_CACHE

[Lu Baolu]

On 2022/4/7 23:23, Jason Gunthorpe wrote:
> While the comment was correct that this flag was intended to convey the
> block no-snoop support in the IOMMU, it has become widely implemented and
> used to mean the IOMMU supports IOMMU_CACHE as a map flag. Only the Intel
> driver was different.
> 
> Now that the Intel driver is using enforce_cache_coherency() update the
> comment to make it clear that IOMMU_CAP_CACHE_COHERENCY is only about
> IOMMU_CACHE.  Fix the Intel driver to return true since IOMMU_CACHE always
> works.
> 
> The two places that test this flag, usnic and vdpa, are both assigning
> userspace pages to a driver controlled iommu_domain and require
> IOMMU_CACHE behavior as they offer no way for userspace to synchronize
> caches.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/iommu/intel/iommu.c | 2 +-
>   include/linux/iommu.h       | 3 +--
>   2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 8f3674e997df06..14ba185175e9ec 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4556,7 +4556,7 @@ static bool intel_iommu_enforce_cache_coherency(struct iommu_domain *domain)
>   static bool intel_iommu_capable(enum iommu_cap cap)
>   {
>   	if (cap == IOMMU_CAP_CACHE_COHERENCY)
> -		return domain_update_iommu_snooping(NULL);
> +		return true;
>   	if (cap == IOMMU_CAP_INTR_REMAP)
>   		return irq_remapping_enabled == 1;
>   
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index fe4f24c469c373..fd58f7adc52796 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -103,8 +103,7 @@ static inline bool iommu_is_dma_domain(struct iommu_domain *domain)
>   }
>   
>   enum iommu_cap {
> -	IOMMU_CAP_CACHE_COHERENCY,	/* IOMMU can enforce cache coherent DMA
> -					   transactions */
> +	IOMMU_CAP_CACHE_COHERENCY,	/* IOMMU_CACHE is supported */
>   	IOMMU_CAP_INTR_REMAP,		/* IOMMU supports interrupt isolation */
>   	IOMMU_CAP_NOEXEC,		/* IOMMU_NOEXEC flag */
>   };

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/4] iommu: Introduce the domain op enforce_cache_coherency()

[Jason Gunthorpe]

On Fri, Apr 08, 2022 at 08:05:38AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Thursday, April 7, 2022 11:24 PM
> > 
> > This new mechanism will replace using IOMMU_CAP_CACHE_COHERENCY
> > and
> > IOMMU_CACHE to control the no-snoop blocking behavior of the IOMMU.
> > 
> > Currently only Intel and AMD IOMMUs are known to support this
> > feature. They both implement it as an IOPTE bit, that when set, will cause
> > PCIe TLPs to that IOVA with the no-snoop bit set to be treated as though
> > the no-snoop bit was clear.
> > 
> > The new API is triggered by calling enforce_cache_coherency() before
> > mapping any IOVA to the domain which globally switches on no-snoop
> > blocking. This allows other implementations that might block no-snoop
> > globally and outside the IOPTE - AMD also documents such a HW capability.
> > 
> > Leave AMD out of sync with Intel and have it block no-snoop even for
> > in-kernel users. This can be trivially resolved in a follow up patch.
> > 
> > Only VFIO will call this new API.
> 
> I still didn't see the point of mandating a caller for a new API (and as
> you pointed out iommufd will call it too).

The language is not to mandate, but to explain why this hasn't come
with a core iommu wrapper function to call it.

> it reads like no_snoop is the result of the enforcement... Probably
> force_snooping better matches the intention here.

Done

Thanks,
Jason 

Re: [PATCH v2 1/4] iommu: Introduce the domain op enforce_cache_coherency()

[Jason Gunthorpe via iommu]

On Fri, Apr 08, 2022 at 08:05:38AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Thursday, April 7, 2022 11:24 PM
> > 
> > This new mechanism will replace using IOMMU_CAP_CACHE_COHERENCY
> > and
> > IOMMU_CACHE to control the no-snoop blocking behavior of the IOMMU.
> > 
> > Currently only Intel and AMD IOMMUs are known to support this
> > feature. They both implement it as an IOPTE bit, that when set, will cause
> > PCIe TLPs to that IOVA with the no-snoop bit set to be treated as though
> > the no-snoop bit was clear.
> > 
> > The new API is triggered by calling enforce_cache_coherency() before
> > mapping any IOVA to the domain which globally switches on no-snoop
> > blocking. This allows other implementations that might block no-snoop
> > globally and outside the IOPTE - AMD also documents such a HW capability.
> > 
> > Leave AMD out of sync with Intel and have it block no-snoop even for
> > in-kernel users. This can be trivially resolved in a follow up patch.
> > 
> > Only VFIO will call this new API.
> 
> I still didn't see the point of mandating a caller for a new API (and as
> you pointed out iommufd will call it too).

The language is not to mandate, but to explain why this hasn't come
with a core iommu wrapper function to call it.

> it reads like no_snoop is the result of the enforcement... Probably
> force_snooping better matches the intention here.

Done

Thanks,
Jason 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 2/4] vfio: Move the Intel no-snoop control off of IOMMU_CACHE

[Jason Gunthorpe]

On Fri, Apr 08, 2022 at 09:47:57AM -0600, Alex Williamson wrote:

> > Ultimately VFIO plumbs the result of enforce_cache_coherency() back into
> > the x86 platform code through kvm_arch_register_noncoherent_dma() which
> > controls if the WBINVD instruction is available in the guest. No other
> > arch implements kvm_arch_register_noncoherent_dma().
> 
> I think this last sentence is alluding to it, but I wish the user
> visible change to VFIO_DMA_CC_IOMMU on non-x86 were more explicit.
> Perhaps for the last sentence:
> 
>   No other archs implement kvm_arch_register_noncoherent_dma() nor are
>   there any other known consumers of VFIO_DMA_CC_IOMMU that might be
>   affected by the user visible result change on non-x86 archs.

Done

Thanks,
Jason

Re: [PATCH v2 2/4] vfio: Move the Intel no-snoop control off of IOMMU_CACHE

[Jason Gunthorpe via iommu]

On Fri, Apr 08, 2022 at 09:47:57AM -0600, Alex Williamson wrote:

> > Ultimately VFIO plumbs the result of enforce_cache_coherency() back into
> > the x86 platform code through kvm_arch_register_noncoherent_dma() which
> > controls if the WBINVD instruction is available in the guest. No other
> > arch implements kvm_arch_register_noncoherent_dma().
> 
> I think this last sentence is alluding to it, but I wish the user
> visible change to VFIO_DMA_CC_IOMMU on non-x86 were more explicit.
> Perhaps for the last sentence:
> 
>   No other archs implement kvm_arch_register_noncoherent_dma() nor are
>   there any other known consumers of VFIO_DMA_CC_IOMMU that might be
>   affected by the user visible result change on non-x86 archs.

Done

Thanks,
Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Tian, Kevin]

> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Friday, April 8, 2022 6:12 PM
> 
> On 2022-04-08 10:08, Tian, Kevin wrote:
> >> From: Jason Gunthorpe <jgg@nvidia.com>
> >> Sent: Friday, April 8, 2022 3:08 AM
> >> On Thu, Apr 07, 2022 at 07:02:03PM +0100, Robin Murphy wrote:
> >>> On 2022-04-07 18:43, Jason Gunthorpe wrote:
> >>>> On Thu, Apr 07, 2022 at 06:03:37PM +0100, Robin Murphy wrote:
> >>>>> At a glance, this all looks about the right shape to me now, thanks!
> >>>>
> >>>> Thanks!
> >>>>
> >>>>> Ideally I'd hope patch #4 could go straight to device_iommu_capable()
> >> from
> >>>>> my Thunderbolt series, but we can figure that out in a couple of weeks
> >> once
> >>>>
> >>>> Yes, this does helps that because now the only iommu_capable call is
> >>>> in a context where a device is available :)
> >>>
> >>> Derp, of course I have *two* VFIO patches waiting, the other one
> touching
> >>> the iommu_capable() calls (there's still IOMMU_CAP_INTR_REMAP,
> which,
> >> much
> >>> as I hate it and would love to boot all that stuff over to
> >>> drivers/irqchip,
> >>
> >> Oh me too...
> >>
> >>> it's not in my way so I'm leaving it be for now). I'll have to rebase that
> >>> anyway, so merging this as-is is absolutely fine!
> >>
> >> This might help your effort - after this series and this below there
> >> are no 'bus' users of iommu_capable left at all.
> >>
> >
> > Out of curiosity, while iommu_capable is being moved to a per-device
> > interface what about irq_domain_check_msi_remap() below (which
> > is also a global check)?
> 
> I suppose it could if anyone cared enough to make the effort - probably
> a case of resolving specific MSI domains for every device in the group,
> and potentially having to deal with hotplug later as well.
> Realistically, though, I wouldn't expect systems to have mixed
> capabilities in that regard (i.e. where the check would return false
> even though *some* domains support remapping), so there doesn't seem to
> be any pressing need to relax it.
> 

Yes, that makes sense if no such example so far.

RE: [PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Tian, Kevin]

> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Friday, April 8, 2022 6:12 PM
> 
> On 2022-04-08 10:08, Tian, Kevin wrote:
> >> From: Jason Gunthorpe <jgg@nvidia.com>
> >> Sent: Friday, April 8, 2022 3:08 AM
> >> On Thu, Apr 07, 2022 at 07:02:03PM +0100, Robin Murphy wrote:
> >>> On 2022-04-07 18:43, Jason Gunthorpe wrote:
> >>>> On Thu, Apr 07, 2022 at 06:03:37PM +0100, Robin Murphy wrote:
> >>>>> At a glance, this all looks about the right shape to me now, thanks!
> >>>>
> >>>> Thanks!
> >>>>
> >>>>> Ideally I'd hope patch #4 could go straight to device_iommu_capable()
> >> from
> >>>>> my Thunderbolt series, but we can figure that out in a couple of weeks
> >> once
> >>>>
> >>>> Yes, this does helps that because now the only iommu_capable call is
> >>>> in a context where a device is available :)
> >>>
> >>> Derp, of course I have *two* VFIO patches waiting, the other one
> touching
> >>> the iommu_capable() calls (there's still IOMMU_CAP_INTR_REMAP,
> which,
> >> much
> >>> as I hate it and would love to boot all that stuff over to
> >>> drivers/irqchip,
> >>
> >> Oh me too...
> >>
> >>> it's not in my way so I'm leaving it be for now). I'll have to rebase that
> >>> anyway, so merging this as-is is absolutely fine!
> >>
> >> This might help your effort - after this series and this below there
> >> are no 'bus' users of iommu_capable left at all.
> >>
> >
> > Out of curiosity, while iommu_capable is being moved to a per-device
> > interface what about irq_domain_check_msi_remap() below (which
> > is also a global check)?
> 
> I suppose it could if anyone cared enough to make the effort - probably
> a case of resolving specific MSI domains for every device in the group,
> and potentially having to deal with hotplug later as well.
> Realistically, though, I wouldn't expect systems to have mixed
> capabilities in that regard (i.e. where the check would return false
> even though *some* domains support remapping), so there doesn't seem to
> be any pressing need to relax it.
> 

Yes, that makes sense if no such example so far.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Tian, Kevin]

> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Saturday, April 9, 2022 1:45 AM
> 
> >
> > So, I suppose VFIO would want to attach/detatch on every vfio_device
> > individually and it would iterate over the group instead of doing a
> > list_first_entry() like above. This would not be hard to do in VFIO.
> 
> It feels like we've already beaten that discussion to death in other
> threads; regardless of what exact argument the iommu_attach/detach
> operations end up taking, they have to operate on the whole (explicit or
> implicit) iommu_group at once, because doing anything else would defeat
> the point of isolation groups, and be impossible for alias groups.
> 

Could you add a few words about what is 'alias group'?

RE: [PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Tian, Kevin]

> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Saturday, April 9, 2022 1:45 AM
> 
> >
> > So, I suppose VFIO would want to attach/detatch on every vfio_device
> > individually and it would iterate over the group instead of doing a
> > list_first_entry() like above. This would not be hard to do in VFIO.
> 
> It feels like we've already beaten that discussion to death in other
> threads; regardless of what exact argument the iommu_attach/detach
> operations end up taking, they have to operate on the whole (explicit or
> implicit) iommu_group at once, because doing anything else would defeat
> the point of isolation groups, and be impossible for alias groups.
> 

Could you add a few words about what is 'alias group'?
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 2/4] vfio: Move the Intel no-snoop control off of IOMMU_CACHE

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Saturday, April 9, 2022 8:51 PM
> 
> On 2022/4/8 16:16, Tian, Kevin wrote:
> >> From: Jason Gunthorpe <jgg@nvidia.com>
> >> Sent: Thursday, April 7, 2022 11:24 PM
> >>
> >> IOMMU_CACHE means "normal DMA to this iommu_domain's IOVA
> should
> >> be cache
> >> coherent" and is used by the DMA API. The definition allows for special
> >> non-coherent DMA to exist - ie processing of the no-snoop flag in PCIe
> >> TLPs - so long as this behavior is opt-in by the device driver.
> >>
> >> The flag is mainly used by the DMA API to synchronize the IOMMU setting
> >> with the expected cache behavior of the DMA master. eg based on
> >> dev_is_dma_coherent() in some case.
> >>
> >> For Intel IOMMU IOMMU_CACHE was redefined to mean 'force all DMA
> to
> >> be
> >> cache coherent' which has the practical effect of causing the IOMMU to
> >> ignore the no-snoop bit in a PCIe TLP.
> >>
> >> x86 platforms are always IOMMU_CACHE, so Intel should ignore this flag.
> >>
> >> Instead use the new domain op enforce_cache_coherency() which causes
> >> every
> >> IOPTE created in the domain to have the no-snoop blocking behavior.
> >>
> >> Reconfigure VFIO to always use IOMMU_CACHE and call
> >> enforce_cache_coherency() to operate the special Intel behavior.
> >>
> >> Remove the IOMMU_CACHE test from Intel IOMMU.
> >>
> >> Ultimately VFIO plumbs the result of enforce_cache_coherency() back into
> >> the x86 platform code through kvm_arch_register_noncoherent_dma()
> >> which
> >> controls if the WBINVD instruction is available in the guest. No other
> >> arch implements kvm_arch_register_noncoherent_dma().
> >>
> >> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> >
> > Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> >
> > btw as discussed in last version it is not necessarily to recalculate
> > snoop control globally with this new approach. Will follow up to
> > clean it up after this series is merged.
> 
> Agreed. But it also requires the enforce_cache_coherency() to be called
> only after domain being attached to a device just as VFIO is doing.

that actually makes sense, right? w/o device attached it's pointless to
call that interface on a domain...

> 
> Anyway, for this change in iommu/vt-d:
> 
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> 
> Best regards,
> baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 2/4] vfio: Move the Intel no-snoop control off of IOMMU_CACHE

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Saturday, April 9, 2022 8:51 PM
> 
> On 2022/4/8 16:16, Tian, Kevin wrote:
> >> From: Jason Gunthorpe <jgg@nvidia.com>
> >> Sent: Thursday, April 7, 2022 11:24 PM
> >>
> >> IOMMU_CACHE means "normal DMA to this iommu_domain's IOVA
> should
> >> be cache
> >> coherent" and is used by the DMA API. The definition allows for special
> >> non-coherent DMA to exist - ie processing of the no-snoop flag in PCIe
> >> TLPs - so long as this behavior is opt-in by the device driver.
> >>
> >> The flag is mainly used by the DMA API to synchronize the IOMMU setting
> >> with the expected cache behavior of the DMA master. eg based on
> >> dev_is_dma_coherent() in some case.
> >>
> >> For Intel IOMMU IOMMU_CACHE was redefined to mean 'force all DMA
> to
> >> be
> >> cache coherent' which has the practical effect of causing the IOMMU to
> >> ignore the no-snoop bit in a PCIe TLP.
> >>
> >> x86 platforms are always IOMMU_CACHE, so Intel should ignore this flag.
> >>
> >> Instead use the new domain op enforce_cache_coherency() which causes
> >> every
> >> IOPTE created in the domain to have the no-snoop blocking behavior.
> >>
> >> Reconfigure VFIO to always use IOMMU_CACHE and call
> >> enforce_cache_coherency() to operate the special Intel behavior.
> >>
> >> Remove the IOMMU_CACHE test from Intel IOMMU.
> >>
> >> Ultimately VFIO plumbs the result of enforce_cache_coherency() back into
> >> the x86 platform code through kvm_arch_register_noncoherent_dma()
> >> which
> >> controls if the WBINVD instruction is available in the guest. No other
> >> arch implements kvm_arch_register_noncoherent_dma().
> >>
> >> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> >
> > Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> >
> > btw as discussed in last version it is not necessarily to recalculate
> > snoop control globally with this new approach. Will follow up to
> > clean it up after this series is merged.
> 
> Agreed. But it also requires the enforce_cache_coherency() to be called
> only after domain being attached to a device just as VFIO is doing.

that actually makes sense, right? w/o device attached it's pointless to
call that interface on a domain...

> 
> Anyway, for this change in iommu/vt-d:
> 
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> 
> Best regards,
> baolu

Re: [PATCH v2 2/4] vfio: Move the Intel no-snoop control off of IOMMU_CACHE

[Lu Baolu]

On 2022/4/12 15:44, Tian, Kevin wrote:
>> From: Lu Baolu<baolu.lu@linux.intel.com>
>> Sent: Saturday, April 9, 2022 8:51 PM
>>
>> On 2022/4/8 16:16, Tian, Kevin wrote:
>>>> From: Jason Gunthorpe<jgg@nvidia.com>
>>>> Sent: Thursday, April 7, 2022 11:24 PM
>>>>
>>>> IOMMU_CACHE means "normal DMA to this iommu_domain's IOVA
>> should
>>>> be cache
>>>> coherent" and is used by the DMA API. The definition allows for special
>>>> non-coherent DMA to exist - ie processing of the no-snoop flag in PCIe
>>>> TLPs - so long as this behavior is opt-in by the device driver.
>>>>
>>>> The flag is mainly used by the DMA API to synchronize the IOMMU setting
>>>> with the expected cache behavior of the DMA master. eg based on
>>>> dev_is_dma_coherent() in some case.
>>>>
>>>> For Intel IOMMU IOMMU_CACHE was redefined to mean 'force all DMA
>> to
>>>> be
>>>> cache coherent' which has the practical effect of causing the IOMMU to
>>>> ignore the no-snoop bit in a PCIe TLP.
>>>>
>>>> x86 platforms are always IOMMU_CACHE, so Intel should ignore this flag.
>>>>
>>>> Instead use the new domain op enforce_cache_coherency() which causes
>>>> every
>>>> IOPTE created in the domain to have the no-snoop blocking behavior.
>>>>
>>>> Reconfigure VFIO to always use IOMMU_CACHE and call
>>>> enforce_cache_coherency() to operate the special Intel behavior.
>>>>
>>>> Remove the IOMMU_CACHE test from Intel IOMMU.
>>>>
>>>> Ultimately VFIO plumbs the result of enforce_cache_coherency() back into
>>>> the x86 platform code through kvm_arch_register_noncoherent_dma()
>>>> which
>>>> controls if the WBINVD instruction is available in the guest. No other
>>>> arch implements kvm_arch_register_noncoherent_dma().
>>>>
>>>> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>
>>> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
>>>
>>> btw as discussed in last version it is not necessarily to recalculate
>>> snoop control globally with this new approach. Will follow up to
>>> clean it up after this series is merged.
>> Agreed. But it also requires the enforce_cache_coherency() to be called
>> only after domain being attached to a device just as VFIO is doing.
> that actually makes sense, right? w/o device attached it's pointless to
> call that interface on a domain...

Agreed. Return -EOPNOTSUPP or -EINVAL to tell the caller that this
operation is invalid before any device attachment.

Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 2/4] vfio: Move the Intel no-snoop control off of IOMMU_CACHE

[Lu Baolu]

On 2022/4/12 15:44, Tian, Kevin wrote:
>> From: Lu Baolu<baolu.lu@linux.intel.com>
>> Sent: Saturday, April 9, 2022 8:51 PM
>>
>> On 2022/4/8 16:16, Tian, Kevin wrote:
>>>> From: Jason Gunthorpe<jgg@nvidia.com>
>>>> Sent: Thursday, April 7, 2022 11:24 PM
>>>>
>>>> IOMMU_CACHE means "normal DMA to this iommu_domain's IOVA
>> should
>>>> be cache
>>>> coherent" and is used by the DMA API. The definition allows for special
>>>> non-coherent DMA to exist - ie processing of the no-snoop flag in PCIe
>>>> TLPs - so long as this behavior is opt-in by the device driver.
>>>>
>>>> The flag is mainly used by the DMA API to synchronize the IOMMU setting
>>>> with the expected cache behavior of the DMA master. eg based on
>>>> dev_is_dma_coherent() in some case.
>>>>
>>>> For Intel IOMMU IOMMU_CACHE was redefined to mean 'force all DMA
>> to
>>>> be
>>>> cache coherent' which has the practical effect of causing the IOMMU to
>>>> ignore the no-snoop bit in a PCIe TLP.
>>>>
>>>> x86 platforms are always IOMMU_CACHE, so Intel should ignore this flag.
>>>>
>>>> Instead use the new domain op enforce_cache_coherency() which causes
>>>> every
>>>> IOPTE created in the domain to have the no-snoop blocking behavior.
>>>>
>>>> Reconfigure VFIO to always use IOMMU_CACHE and call
>>>> enforce_cache_coherency() to operate the special Intel behavior.
>>>>
>>>> Remove the IOMMU_CACHE test from Intel IOMMU.
>>>>
>>>> Ultimately VFIO plumbs the result of enforce_cache_coherency() back into
>>>> the x86 platform code through kvm_arch_register_noncoherent_dma()
>>>> which
>>>> controls if the WBINVD instruction is available in the guest. No other
>>>> arch implements kvm_arch_register_noncoherent_dma().
>>>>
>>>> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>
>>> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
>>>
>>> btw as discussed in last version it is not necessarily to recalculate
>>> snoop control globally with this new approach. Will follow up to
>>> clean it up after this series is merged.
>> Agreed. But it also requires the enforce_cache_coherency() to be called
>> only after domain being attached to a device just as VFIO is doing.
> that actually makes sense, right? w/o device attached it's pointless to
> call that interface on a domain...

Agreed. Return -EOPNOTSUPP or -EINVAL to tell the caller that this
operation is invalid before any device attachment.

Best regards,
baolu

Re: [PATCH v2 2/4] vfio: Move the Intel no-snoop control off of IOMMU_CACHE

[Jason Gunthorpe via iommu]

On Tue, Apr 12, 2022 at 09:13:27PM +0800, Lu Baolu wrote:

> > > > btw as discussed in last version it is not necessarily to recalculate
> > > > snoop control globally with this new approach. Will follow up to
> > > > clean it up after this series is merged.
> > > Agreed. But it also requires the enforce_cache_coherency() to be called
> > > only after domain being attached to a device just as VFIO is doing.
> > that actually makes sense, right? w/o device attached it's pointless to
> > call that interface on a domain...
> 
> Agreed. Return -EOPNOTSUPP or -EINVAL to tell the caller that this
> operation is invalid before any device attachment.

That is backwards. enforce_cache_coherency() succeeds on an empty
domain and attach of an incompatible device must fail.

Meaning you check the force_snoop flag in the domain when attaching
and refuse to attach the device if it cannot support it. This will
trigger vfio to create a new iommu_domain for that device and then
enforce_cache_coherency() will fail on that domain due to the
incompatible attached device.

Same scenario if it is the Nth device to be attached to a domain.

Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 2/4] vfio: Move the Intel no-snoop control off of IOMMU_CACHE

[Jason Gunthorpe]

On Tue, Apr 12, 2022 at 09:13:27PM +0800, Lu Baolu wrote:

> > > > btw as discussed in last version it is not necessarily to recalculate
> > > > snoop control globally with this new approach. Will follow up to
> > > > clean it up after this series is merged.
> > > Agreed. But it also requires the enforce_cache_coherency() to be called
> > > only after domain being attached to a device just as VFIO is doing.
> > that actually makes sense, right? w/o device attached it's pointless to
> > call that interface on a domain...
> 
> Agreed. Return -EOPNOTSUPP or -EINVAL to tell the caller that this
> operation is invalid before any device attachment.

That is backwards. enforce_cache_coherency() succeeds on an empty
domain and attach of an incompatible device must fail.

Meaning you check the force_snoop flag in the domain when attaching
and refuse to attach the device if it cannot support it. This will
trigger vfio to create a new iommu_domain for that device and then
enforce_cache_coherency() will fail on that domain due to the
incompatible attached device.

Same scenario if it is the Nth device to be attached to a domain.

Jason

RE: [PATCH v2 2/4] vfio: Move the Intel no-snoop control off of IOMMU_CACHE

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, April 12, 2022 9:21 PM
> 
> On Tue, Apr 12, 2022 at 09:13:27PM +0800, Lu Baolu wrote:
> 
> > > > > btw as discussed in last version it is not necessarily to recalculate
> > > > > snoop control globally with this new approach. Will follow up to
> > > > > clean it up after this series is merged.
> > > > Agreed. But it also requires the enforce_cache_coherency() to be called
> > > > only after domain being attached to a device just as VFIO is doing.
> > > that actually makes sense, right? w/o device attached it's pointless to
> > > call that interface on a domain...
> >
> > Agreed. Return -EOPNOTSUPP or -EINVAL to tell the caller that this
> > operation is invalid before any device attachment.
> 
> That is backwards. enforce_cache_coherency() succeeds on an empty
> domain and attach of an incompatible device must fail.

seems it's just a matter of the default policy on an empty domain. No
matter we by default allow enforce_cache_coherency succeed or not
the compatibility check against the default policy must be done anyway
when attaching a device.

given most IOMMUs supports force-snooping, allowing it succeed by
default certainly makes more sense.

Thanks
Kevin
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 2/4] vfio: Move the Intel no-snoop control off of IOMMU_CACHE

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, April 12, 2022 9:21 PM
> 
> On Tue, Apr 12, 2022 at 09:13:27PM +0800, Lu Baolu wrote:
> 
> > > > > btw as discussed in last version it is not necessarily to recalculate
> > > > > snoop control globally with this new approach. Will follow up to
> > > > > clean it up after this series is merged.
> > > > Agreed. But it also requires the enforce_cache_coherency() to be called
> > > > only after domain being attached to a device just as VFIO is doing.
> > > that actually makes sense, right? w/o device attached it's pointless to
> > > call that interface on a domain...
> >
> > Agreed. Return -EOPNOTSUPP or -EINVAL to tell the caller that this
> > operation is invalid before any device attachment.
> 
> That is backwards. enforce_cache_coherency() succeeds on an empty
> domain and attach of an incompatible device must fail.

seems it's just a matter of the default policy on an empty domain. No
matter we by default allow enforce_cache_coherency succeed or not
the compatibility check against the default policy must be done anyway
when attaching a device.

given most IOMMUs supports force-snooping, allowing it succeed by
default certainly makes more sense.

Thanks
Kevin

Re: [PATCH v2 2/4] vfio: Move the Intel no-snoop control off of IOMMU_CACHE

[Lu Baolu]

On 2022/4/13 7:04, Tian, Kevin wrote:
>> From: Jason Gunthorpe<jgg@nvidia.com>
>> Sent: Tuesday, April 12, 2022 9:21 PM
>>
>> On Tue, Apr 12, 2022 at 09:13:27PM +0800, Lu Baolu wrote:
>>
>>>>>> btw as discussed in last version it is not necessarily to recalculate
>>>>>> snoop control globally with this new approach. Will follow up to
>>>>>> clean it up after this series is merged.
>>>>> Agreed. But it also requires the enforce_cache_coherency() to be called
>>>>> only after domain being attached to a device just as VFIO is doing.
>>>> that actually makes sense, right? w/o device attached it's pointless to
>>>> call that interface on a domain...
>>> Agreed. Return -EOPNOTSUPP or -EINVAL to tell the caller that this
>>> operation is invalid before any device attachment.
>> That is backwards. enforce_cache_coherency() succeeds on an empty
>> domain and attach of an incompatible device must fail.
> seems it's just a matter of the default policy on an empty domain. No
> matter we by default allow enforce_cache_coherency succeed or not
> the compatibility check against the default policy must be done anyway
> when attaching a device.
> 
> given most IOMMUs supports force-snooping, allowing it succeed by
> default certainly makes more sense.

Make sense. I will come up with the patches after this series is merged.

Best regards,
baolu

Re: [PATCH v2 2/4] vfio: Move the Intel no-snoop control off of IOMMU_CACHE

[Lu Baolu]

On 2022/4/13 7:04, Tian, Kevin wrote:
>> From: Jason Gunthorpe<jgg@nvidia.com>
>> Sent: Tuesday, April 12, 2022 9:21 PM
>>
>> On Tue, Apr 12, 2022 at 09:13:27PM +0800, Lu Baolu wrote:
>>
>>>>>> btw as discussed in last version it is not necessarily to recalculate
>>>>>> snoop control globally with this new approach. Will follow up to
>>>>>> clean it up after this series is merged.
>>>>> Agreed. But it also requires the enforce_cache_coherency() to be called
>>>>> only after domain being attached to a device just as VFIO is doing.
>>>> that actually makes sense, right? w/o device attached it's pointless to
>>>> call that interface on a domain...
>>> Agreed. Return -EOPNOTSUPP or -EINVAL to tell the caller that this
>>> operation is invalid before any device attachment.
>> That is backwards. enforce_cache_coherency() succeeds on an empty
>> domain and attach of an incompatible device must fail.
> seems it's just a matter of the default policy on an empty domain. No
> matter we by default allow enforce_cache_coherency succeed or not
> the compatibility check against the default policy must be done anyway
> when attaching a device.
> 
> given most IOMMUs supports force-snooping, allowing it succeed by
> default certainly makes more sense.

Make sense. I will come up with the patches after this series is merged.

Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 4/4] vfio: Require that devices support DMA cache coherence

[Alexey Kardashevskiy]

On 4/8/22 01:23, Jason Gunthorpe via iommu wrote:
> IOMMU_CACHE means that normal DMAs do not require any additional coherency
> mechanism and is the basic uAPI that VFIO exposes to userspace. For
> instance VFIO applications like DPDK will not work if additional coherency
> operations are required.
> 
> Therefore check IOMMU_CAP_CACHE_COHERENCY like vdpa & usnic do before
> allowing an IOMMU backed VFIO device to be created.


This just broke VFIO on POWER which does not use iommu_ops.


> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/vfio/vfio.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index a4555014bd1e72..9edad767cfdad3 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -815,6 +815,13 @@ static int __vfio_register_dev(struct vfio_device *device,
>   
>   int vfio_register_group_dev(struct vfio_device *device)
>   {
> +	/*
> +	 * VFIO always sets IOMMU_CACHE because we offer no way for userspace to
> +	 * restore cache coherency.
> +	 */
> +	if (!iommu_capable(device->dev->bus, IOMMU_CAP_CACHE_COHERENCY))
> +		return -EINVAL;
> +
>   	return __vfio_register_dev(device,
>   		vfio_group_find_or_alloc(device->dev));
>   }

-- 
Alexey

Re: [PATCH v2 4/4] vfio: Require that devices support DMA cache coherence

[Alexey Kardashevskiy]

On 4/8/22 01:23, Jason Gunthorpe via iommu wrote:
> IOMMU_CACHE means that normal DMAs do not require any additional coherency
> mechanism and is the basic uAPI that VFIO exposes to userspace. For
> instance VFIO applications like DPDK will not work if additional coherency
> operations are required.
> 
> Therefore check IOMMU_CAP_CACHE_COHERENCY like vdpa & usnic do before
> allowing an IOMMU backed VFIO device to be created.


This just broke VFIO on POWER which does not use iommu_ops.


> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/vfio/vfio.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index a4555014bd1e72..9edad767cfdad3 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -815,6 +815,13 @@ static int __vfio_register_dev(struct vfio_device *device,
>   
>   int vfio_register_group_dev(struct vfio_device *device)
>   {
> +	/*
> +	 * VFIO always sets IOMMU_CACHE because we offer no way for userspace to
> +	 * restore cache coherency.
> +	 */
> +	if (!iommu_capable(device->dev->bus, IOMMU_CAP_CACHE_COHERENCY))
> +		return -EINVAL;
> +
>   	return __vfio_register_dev(device,
>   		vfio_group_find_or_alloc(device->dev));
>   }

-- 
Alexey
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 4/4] vfio: Require that devices support DMA cache coherence

[Tian, Kevin]

> From: Alexey Kardashevskiy <aik@ozlabs.ru>
> Sent: Friday, July 1, 2022 12:58 PM
> 
> On 4/8/22 01:23, Jason Gunthorpe via iommu wrote:
> > IOMMU_CACHE means that normal DMAs do not require any additional
> coherency
> > mechanism and is the basic uAPI that VFIO exposes to userspace. For
> > instance VFIO applications like DPDK will not work if additional coherency
> > operations are required.
> >
> > Therefore check IOMMU_CAP_CACHE_COHERENCY like vdpa & usnic do
> before
> > allowing an IOMMU backed VFIO device to be created.
> 
> 
> This just broke VFIO on POWER which does not use iommu_ops.

In this case below check is more reasonable to be put in type1
attach_group(). Do a iommu_group_for_each_dev() to verify
CACHE_COHERENCY similar to what Robin did for INTR_REMAP.

(sorry no access to my build machine now but I suppose Jason
can soon work out a fix once he sees this. 😊)

> 
> 
> >
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> >   drivers/vfio/vfio.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> > index a4555014bd1e72..9edad767cfdad3 100644
> > --- a/drivers/vfio/vfio.c
> > +++ b/drivers/vfio/vfio.c
> > @@ -815,6 +815,13 @@ static int __vfio_register_dev(struct vfio_device
> *device,
> >
> >   int vfio_register_group_dev(struct vfio_device *device)
> >   {
> > +	/*
> > +	 * VFIO always sets IOMMU_CACHE because we offer no way for
> userspace to
> > +	 * restore cache coherency.
> > +	 */
> > +	if (!iommu_capable(device->dev->bus,
> IOMMU_CAP_CACHE_COHERENCY))
> > +		return -EINVAL;
> > +
> >   	return __vfio_register_dev(device,
> >   		vfio_group_find_or_alloc(device->dev));
> >   }
> 
> --
> Alexey

RE: [PATCH v2 4/4] vfio: Require that devices support DMA cache coherence

[Tian, Kevin]

> From: Alexey Kardashevskiy <aik@ozlabs.ru>
> Sent: Friday, July 1, 2022 12:58 PM
> 
> On 4/8/22 01:23, Jason Gunthorpe via iommu wrote:
> > IOMMU_CACHE means that normal DMAs do not require any additional
> coherency
> > mechanism and is the basic uAPI that VFIO exposes to userspace. For
> > instance VFIO applications like DPDK will not work if additional coherency
> > operations are required.
> >
> > Therefore check IOMMU_CAP_CACHE_COHERENCY like vdpa & usnic do
> before
> > allowing an IOMMU backed VFIO device to be created.
> 
> 
> This just broke VFIO on POWER which does not use iommu_ops.

In this case below check is more reasonable to be put in type1
attach_group(). Do a iommu_group_for_each_dev() to verify
CACHE_COHERENCY similar to what Robin did for INTR_REMAP.

(sorry no access to my build machine now but I suppose Jason
can soon work out a fix once he sees this. 😊)

> 
> 
> >
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> >   drivers/vfio/vfio.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> > index a4555014bd1e72..9edad767cfdad3 100644
> > --- a/drivers/vfio/vfio.c
> > +++ b/drivers/vfio/vfio.c
> > @@ -815,6 +815,13 @@ static int __vfio_register_dev(struct vfio_device
> *device,
> >
> >   int vfio_register_group_dev(struct vfio_device *device)
> >   {
> > +	/*
> > +	 * VFIO always sets IOMMU_CACHE because we offer no way for
> userspace to
> > +	 * restore cache coherency.
> > +	 */
> > +	if (!iommu_capable(device->dev->bus,
> IOMMU_CAP_CACHE_COHERENCY))
> > +		return -EINVAL;
> > +
> >   	return __vfio_register_dev(device,
> >   		vfio_group_find_or_alloc(device->dev));
> >   }
> 
> --
> Alexey
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 4/4] vfio: Require that devices support DMA cache coherence

[Alexey Kardashevskiy]

On 7/1/22 16:07, Tian, Kevin wrote:
>> From: Alexey Kardashevskiy <aik@ozlabs.ru>
>> Sent: Friday, July 1, 2022 12:58 PM
>>
>> On 4/8/22 01:23, Jason Gunthorpe via iommu wrote:
>>> IOMMU_CACHE means that normal DMAs do not require any additional
>> coherency
>>> mechanism and is the basic uAPI that VFIO exposes to userspace. For
>>> instance VFIO applications like DPDK will not work if additional coherency
>>> operations are required.
>>>
>>> Therefore check IOMMU_CAP_CACHE_COHERENCY like vdpa & usnic do
>> before
>>> allowing an IOMMU backed VFIO device to be created.
>>
>>
>> This just broke VFIO on POWER which does not use iommu_ops.
> 
> In this case below check is more reasonable to be put in type1
> attach_group(). Do a iommu_group_for_each_dev() to verify
> CACHE_COHERENCY similar to what Robin did for INTR_REMAP.


ah makes sense. I've posted an RFC with another problem - "[RFC PATCH 
kernel] vfio: Skip checking for IOMMU_CAP_CACHE_COHERENCY on POWER and 
more", would be great if both addressed, or I try moving those next week 
:) Thanks,


> 
> (sorry no access to my build machine now but I suppose Jason
> can soon work out a fix once he sees this. 😊)
> 
>>
>>
>>>
>>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>>> ---
>>>    drivers/vfio/vfio.c | 7 +++++++
>>>    1 file changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>>> index a4555014bd1e72..9edad767cfdad3 100644
>>> --- a/drivers/vfio/vfio.c
>>> +++ b/drivers/vfio/vfio.c
>>> @@ -815,6 +815,13 @@ static int __vfio_register_dev(struct vfio_device
>> *device,
>>>
>>>    int vfio_register_group_dev(struct vfio_device *device)
>>>    {
>>> +	/*
>>> +	 * VFIO always sets IOMMU_CACHE because we offer no way for
>> userspace to
>>> +	 * restore cache coherency.
>>> +	 */
>>> +	if (!iommu_capable(device->dev->bus,
>> IOMMU_CAP_CACHE_COHERENCY))
>>> +		return -EINVAL;
>>> +
>>>    	return __vfio_register_dev(device,
>>>    		vfio_group_find_or_alloc(device->dev));
>>>    }
>>
>> --
>> Alexey

-- 
Alexey

Re: [PATCH v2 4/4] vfio: Require that devices support DMA cache coherence

[Alexey Kardashevskiy]

On 7/1/22 16:07, Tian, Kevin wrote:
>> From: Alexey Kardashevskiy <aik@ozlabs.ru>
>> Sent: Friday, July 1, 2022 12:58 PM
>>
>> On 4/8/22 01:23, Jason Gunthorpe via iommu wrote:
>>> IOMMU_CACHE means that normal DMAs do not require any additional
>> coherency
>>> mechanism and is the basic uAPI that VFIO exposes to userspace. For
>>> instance VFIO applications like DPDK will not work if additional coherency
>>> operations are required.
>>>
>>> Therefore check IOMMU_CAP_CACHE_COHERENCY like vdpa & usnic do
>> before
>>> allowing an IOMMU backed VFIO device to be created.
>>
>>
>> This just broke VFIO on POWER which does not use iommu_ops.
> 
> In this case below check is more reasonable to be put in type1
> attach_group(). Do a iommu_group_for_each_dev() to verify
> CACHE_COHERENCY similar to what Robin did for INTR_REMAP.


ah makes sense. I've posted an RFC with another problem - "[RFC PATCH 
kernel] vfio: Skip checking for IOMMU_CAP_CACHE_COHERENCY on POWER and 
more", would be great if both addressed, or I try moving those next week 
:) Thanks,


> 
> (sorry no access to my build machine now but I suppose Jason
> can soon work out a fix once he sees this. 😊)
> 
>>
>>
>>>
>>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>>> ---
>>>    drivers/vfio/vfio.c | 7 +++++++
>>>    1 file changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>>> index a4555014bd1e72..9edad767cfdad3 100644
>>> --- a/drivers/vfio/vfio.c
>>> +++ b/drivers/vfio/vfio.c
>>> @@ -815,6 +815,13 @@ static int __vfio_register_dev(struct vfio_device
>> *device,
>>>
>>>    int vfio_register_group_dev(struct vfio_device *device)
>>>    {
>>> +	/*
>>> +	 * VFIO always sets IOMMU_CACHE because we offer no way for
>> userspace to
>>> +	 * restore cache coherency.
>>> +	 */
>>> +	if (!iommu_capable(device->dev->bus,
>> IOMMU_CAP_CACHE_COHERENCY))
>>> +		return -EINVAL;
>>> +
>>>    	return __vfio_register_dev(device,
>>>    		vfio_group_find_or_alloc(device->dev));
>>>    }
>>
>> --
>> Alexey

-- 
Alexey
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu