[PATCH 0/3] iommufd review updates

[PATCH 0/3] iommufd review updates

[Jason Gunthorpe]

A few patches from review comments that happened a bit too late to get
rebased into the commits.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Jason Gunthorpe (3):
  iommufd: Fix comment typos
  iommufd: Improve a few unclear bits of code
  iommufd: Change the order of MSI setup

 drivers/iommu/iommufd/device.c       | 54 ++++++++++++----------------
 drivers/iommu/iommufd/io_pagetable.c | 24 +++++++------
 drivers/iommu/iommufd/io_pagetable.h |  2 +-
 drivers/iommu/iommufd/pages.c        | 11 +++---
 4 files changed, 45 insertions(+), 46 deletions(-)


base-commit: 395f9d8975251ca906113c9f4408aa1592f38e07
-- 
2.38.1

[PATCH 1/3] iommufd: Fix comment typos

[Jason Gunthorpe]

Repair some typos in comments that were noticed late in the review
cycle.

Fixes: f394576eb11d ("iommufd: PFN handling for iopt_pages")
Reported-by: Binbin Wu <binbin.wu@linux.intel.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommufd/io_pagetable.h | 2 +-
 drivers/iommu/iommufd/pages.c        | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/iommufd/io_pagetable.h b/drivers/iommu/iommufd/io_pagetable.h
index 83e7c175f2a277..0ec3509b7e3392 100644
--- a/drivers/iommu/iommufd/io_pagetable.h
+++ b/drivers/iommu/iommufd/io_pagetable.h
@@ -33,7 +33,7 @@ struct iommu_domain;
  *
  * The io_pagetable::iova_rwsem protects node
  * The iopt_pages::mutex protects pages_node
- * iopt and immu_prot are immutable
+ * iopt and iommu_prot are immutable
  * The pages::mutex protects num_accesses
  */
 struct iopt_area {
diff --git a/drivers/iommu/iommufd/pages.c b/drivers/iommu/iommufd/pages.c
index 429fa3b0a239cd..fccdba782cb699 100644
--- a/drivers/iommu/iommufd/pages.c
+++ b/drivers/iommu/iommufd/pages.c
@@ -342,7 +342,7 @@ static void batch_destroy(struct pfn_batch *batch, void *backup)
 		kfree(batch->pfns);
 }
 
-/* true if the pfn could be added, false otherwise */
+/* true if the pfn was added, false otherwise */
 static bool batch_add_pfn(struct pfn_batch *batch, unsigned long pfn)
 {
 	const unsigned int MAX_NPFNS = type_max(typeof(*batch->npfns));
@@ -418,7 +418,7 @@ static struct page **raw_pages_from_domain(struct iommu_domain *domain,
 	return out_pages;
 }
 
-/* Continues reading a domain until we reach a discontiguity in the pfns. */
+/* Continues reading a domain until we reach a discontinuity in the pfns. */
 static void batch_from_domain_continue(struct pfn_batch *batch,
 				       struct iommu_domain *domain,
 				       struct iopt_area *area,
-- 
2.38.1

[PATCH 2/3] iommufd: Improve a few unclear bits of code

[Jason Gunthorpe]

Correct a few items noticed late in review:

 - We should assert that the math in batch_clear_carry() doesn't underflow

 - user->locked should be -1 not 0 sicne we just did mmput

 - npages should not have been recalculated, it already has that value

No functional change.

Fixes: 8d160cd4d506 ("iommufd: Algorithms for PFN storage")
Reported-by: Binbin Wu <binbin.wu@linux.intel.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommufd/pages.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/iommufd/pages.c b/drivers/iommu/iommufd/pages.c
index fccdba782cb699..c771772296485f 100644
--- a/drivers/iommu/iommufd/pages.c
+++ b/drivers/iommu/iommufd/pages.c
@@ -289,6 +289,10 @@ static void batch_clear_carry(struct pfn_batch *batch, unsigned int keep_pfns)
 	if (!keep_pfns)
 		return batch_clear(batch);
 
+	if (IS_ENABLED(CONFIG_IOMMUFD_TEST))
+		WARN_ON(!batch->end ||
+			batch->npfns[batch->end - 1] < keep_pfns);
+
 	batch->total_pfns = keep_pfns;
 	batch->npfns[0] = keep_pfns;
 	batch->pfns[0] = batch->pfns[batch->end - 1] +
@@ -723,7 +727,7 @@ static void pfn_reader_user_destroy(struct pfn_reader_user *user,
 			mmap_read_unlock(pages->source_mm);
 		if (pages->source_mm != current->mm)
 			mmput(pages->source_mm);
-		user->locked = 0;
+		user->locked = -1;
 	}
 
 	kfree(user->upages);
@@ -810,7 +814,6 @@ static int incr_user_locked_vm(struct iopt_pages *pages, unsigned long npages)
 
 	lock_limit = task_rlimit(pages->source_task, RLIMIT_MEMLOCK) >>
 		     PAGE_SHIFT;
-	npages = pages->npinned - pages->last_npinned;
 	do {
 		cur_pages = atomic_long_read(&pages->source_user->locked_vm);
 		new_pages = cur_pages + npages;
-- 
2.38.1

[PATCH 3/3] iommufd: Change the order of MSI setup

[Jason Gunthorpe]

Eric points out this is wrong for the rare case of someone using
allow_unsafe_interrupts on ARM. We always have to setup the MSI window in
the domain if the iommu driver asks for it.

Move the iommu_get_msi_cookie() setup to the top of the function and
always do it, regardless of the security mode. Add checks to
iommufd_device_setup_msi() to ensure the driver is not doing something
incomprehensible. No current driver will set both a HW and SW MSI window.

Fixes: e8d57210035b ("iommufd: Add kAPI toward external drivers for physical devices")
Reported-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommufd/device.c       | 54 ++++++++++++----------------
 drivers/iommu/iommufd/io_pagetable.c | 24 +++++++------
 2 files changed, 37 insertions(+), 41 deletions(-)

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index dd2a415b603e3b..29ff97f756bc42 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -139,19 +139,11 @@ static int iommufd_device_setup_msi(struct iommufd_device *idev,
 	int rc;
 
 	/*
-	 * IOMMU_CAP_INTR_REMAP means that the platform is isolating MSI, and it
-	 * creates the MSI window by default in the iommu domain. Nothing
-	 * further to do.
-	 */
-	if (device_iommu_capable(idev->dev, IOMMU_CAP_INTR_REMAP))
-		return 0;
-
-	/*
-	 * On ARM systems that set the global IRQ_DOMAIN_FLAG_MSI_REMAP every
-	 * allocated iommu_domain will block interrupts by default and this
-	 * special flow is needed to turn them back on. iommu_dma_prepare_msi()
-	 * will install pages into our domain after request_irq() to make this
-	 * work.
+	 * If the IOMMU driver gives a IOMMU_RESV_SW_MSI then it is asking us to
+	 * call iommu_get_msi_cookie() on its behalf. This is necessary to setup
+	 * the MSI window so iommu_dma_prepare_msi() can install pages into our
+	 * domain after request_irq(). If it is not done interrupts will not
+	 * work on this domain.
 	 *
 	 * FIXME: This is conceptually broken for iommufd since we want to allow
 	 * userspace to change the domains, eg switch from an identity IOAS to a
@@ -159,33 +151,33 @@ static int iommufd_device_setup_msi(struct iommufd_device *idev,
 	 * matches what the IRQ layer actually expects in a newly created
 	 * domain.
 	 */
-	if (irq_domain_check_msi_remap()) {
-		if (WARN_ON(!sw_msi_start))
-			return -EPERM;
+	if (sw_msi_start != PHYS_ADDR_MAX && !hwpt->msi_cookie) {
+		rc = iommu_get_msi_cookie(hwpt->domain, sw_msi_start);
+		if (rc)
+			return rc;
+
 		/*
 		 * iommu_get_msi_cookie() can only be called once per domain,
 		 * it returns -EBUSY on later calls.
 		 */
-		if (hwpt->msi_cookie)
-			return 0;
-		rc = iommu_get_msi_cookie(hwpt->domain, sw_msi_start);
-		if (rc)
-			return rc;
 		hwpt->msi_cookie = true;
-		return 0;
 	}
 
 	/*
-	 * Otherwise the platform has a MSI window that is not isolated. For
-	 * historical compat with VFIO allow a module parameter to ignore the
-	 * insecurity.
+	 * For historical compat with VFIO the insecure interrupt path is
+	 * allowed if the module parameter is set. Insecure means that a MemWr
+	 * operation from the device (eg a simple DMA) cannot trigger an
+	 * interrupt outside this iommufd context.
 	 */
-	if (!allow_unsafe_interrupts)
-		return -EPERM;
+	if (!device_iommu_capable(idev->dev, IOMMU_CAP_INTR_REMAP) &&
+	    !irq_domain_check_msi_remap()) {
+		if (!allow_unsafe_interrupts)
+			return -EPERM;
 
-	dev_warn(
-		idev->dev,
-		"MSI interrupt window cannot be isolated by the IOMMU, this platform is insecure. Use the \"allow_unsafe_interrupts\" module parameter to override\n");
+		dev_warn(
+			idev->dev,
+			"MSI interrupt window cannot be isolated by the IOMMU, this platform is insecure. Use the \"allow_unsafe_interrupts\" module parameter to override\n");
+	}
 	return 0;
 }
 
@@ -203,7 +195,7 @@ static bool iommufd_hw_pagetable_has_group(struct iommufd_hw_pagetable *hwpt,
 static int iommufd_device_do_attach(struct iommufd_device *idev,
 				    struct iommufd_hw_pagetable *hwpt)
 {
-	phys_addr_t sw_msi_start = 0;
+	phys_addr_t sw_msi_start = PHYS_ADDR_MAX;
 	int rc;
 
 	mutex_lock(&hwpt->devices_lock);
diff --git a/drivers/iommu/iommufd/io_pagetable.c b/drivers/iommu/iommufd/io_pagetable.c
index 3467cea795684c..e0ae72b9e67f86 100644
--- a/drivers/iommu/iommufd/io_pagetable.c
+++ b/drivers/iommu/iommufd/io_pagetable.c
@@ -1170,6 +1170,8 @@ int iopt_table_enforce_group_resv_regions(struct io_pagetable *iopt,
 	struct iommu_resv_region *resv;
 	struct iommu_resv_region *tmp;
 	LIST_HEAD(group_resv_regions);
+	unsigned int num_hw_msi = 0;
+	unsigned int num_sw_msi = 0;
 	int rc;
 
 	down_write(&iopt->iova_rwsem);
@@ -1181,23 +1183,25 @@ int iopt_table_enforce_group_resv_regions(struct io_pagetable *iopt,
 		if (resv->type == IOMMU_RESV_DIRECT_RELAXABLE)
 			continue;
 
-		/*
-		 * The presence of any 'real' MSI regions should take precedence
-		 * over the software-managed one if the IOMMU driver happens to
-		 * advertise both types.
-		 */
-		if (sw_msi_start && resv->type == IOMMU_RESV_MSI) {
-			*sw_msi_start = 0;
-			sw_msi_start = NULL;
-		}
-		if (sw_msi_start && resv->type == IOMMU_RESV_SW_MSI)
+		if (sw_msi_start && resv->type == IOMMU_RESV_MSI)
+			num_hw_msi++;
+		if (sw_msi_start && resv->type == IOMMU_RESV_SW_MSI) {
 			*sw_msi_start = resv->start;
+			num_sw_msi++;
+		}
 
 		rc = iopt_reserve_iova(iopt, resv->start,
 				       resv->length - 1 + resv->start, device);
 		if (rc)
 			goto out_reserved;
 	}
+
+	/* Drivers must offer sane combinations of regions */
+	if (WARN_ON(num_sw_msi && num_hw_msi) || WARN_ON(num_sw_msi > 1)) {
+		rc = -EINVAL;
+		goto out_reserved;
+	}
+
 	rc = 0;
 	goto out_free_resv;
 
-- 
2.38.1

RE: [PATCH 1/3] iommufd: Fix comment typos

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, December 8, 2022 4:45 AM
> 
> Repair some typos in comments that were noticed late in the review
> cycle.
> 
> Fixes: f394576eb11d ("iommufd: PFN handling for iopt_pages")
> Reported-by: Binbin Wu <binbin.wu@linux.intel.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

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

RE: [PATCH 2/3] iommufd: Improve a few unclear bits of code

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, December 8, 2022 4:45 AM
> 
> Correct a few items noticed late in review:
> 
>  - We should assert that the math in batch_clear_carry() doesn't underflow
> 
>  - user->locked should be -1 not 0 sicne we just did mmput
> 
>  - npages should not have been recalculated, it already has that value
> 
> No functional change.
> 
> Fixes: 8d160cd4d506 ("iommufd: Algorithms for PFN storage")
> Reported-by: Binbin Wu <binbin.wu@linux.intel.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

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

RE: [PATCH 3/3] iommufd: Change the order of MSI setup

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, December 8, 2022 4:45 AM
> 
> Eric points out this is wrong for the rare case of someone using
> allow_unsafe_interrupts on ARM. We always have to setup the MSI window
> in
> the domain if the iommu driver asks for it.
> 
> Move the iommu_get_msi_cookie() setup to the top of the function and
> always do it, regardless of the security mode. Add checks to
> iommufd_device_setup_msi() to ensure the driver is not doing something
> incomprehensible. No current driver will set both a HW and SW MSI window.

"and have more than one SW MSI window".

Should we also change vfio with same sanity check? 

> 
> Fixes: e8d57210035b ("iommufd: Add kAPI toward external drivers for
> physical devices")
> Reported-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

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

Re: [PATCH 3/3] iommufd: Change the order of MSI setup

[Jason Gunthorpe]

On Thu, Dec 08, 2022 at 06:06:33AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Thursday, December 8, 2022 4:45 AM
> > 
> > Eric points out this is wrong for the rare case of someone using
> > allow_unsafe_interrupts on ARM. We always have to setup the MSI window
> > in
> > the domain if the iommu driver asks for it.
> > 
> > Move the iommu_get_msi_cookie() setup to the top of the function and
> > always do it, regardless of the security mode. Add checks to
> > iommufd_device_setup_msi() to ensure the driver is not doing something
> > incomprehensible. No current driver will set both a HW and SW MSI window.
> 
> "and have more than one SW MSI window".
> 
> Should we also change vfio with same sanity check? 

Probably not, it is to detect driver bugs, if buggy drivers exist they
will get fixed once they are tested with iommufd

Jason

Re: [PATCH 2/3] iommufd: Improve a few unclear bits of code

[Eric Auger]

On 12/7/22 21:44, Jason Gunthorpe wrote:
> Correct a few items noticed late in review:
>
>  - We should assert that the math in batch_clear_carry() doesn't underflow
>
>  - user->locked should be -1 not 0 sicne we just did mmput
>
>  - npages should not have been recalculated, it already has that value
>
> No functional change.
>
> Fixes: 8d160cd4d506 ("iommufd: Algorithms for PFN storage")
> Reported-by: Binbin Wu <binbin.wu@linux.intel.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  drivers/iommu/iommufd/pages.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/iommufd/pages.c b/drivers/iommu/iommufd/pages.c
> index fccdba782cb699..c771772296485f 100644
> --- a/drivers/iommu/iommufd/pages.c
> +++ b/drivers/iommu/iommufd/pages.c
> @@ -289,6 +289,10 @@ static void batch_clear_carry(struct pfn_batch *batch, unsigned int keep_pfns)
>  	if (!keep_pfns)
>  		return batch_clear(batch);
>  
> +	if (IS_ENABLED(CONFIG_IOMMUFD_TEST))
> +		WARN_ON(!batch->end ||
> +			batch->npfns[batch->end - 1] < keep_pfns);
> +
>  	batch->total_pfns = keep_pfns;
>  	batch->npfns[0] = keep_pfns;
>  	batch->pfns[0] = batch->pfns[batch->end - 1] +
> @@ -723,7 +727,7 @@ static void pfn_reader_user_destroy(struct pfn_reader_user *user,
>  			mmap_read_unlock(pages->source_mm);
>  		if (pages->source_mm != current->mm)
>  			mmput(pages->source_mm);
> -		user->locked = 0;
> +		user->locked = -1;
>  	}
>  
>  	kfree(user->upages);
> @@ -810,7 +814,6 @@ static int incr_user_locked_vm(struct iopt_pages *pages, unsigned long npages)
>  
>  	lock_limit = task_rlimit(pages->source_task, RLIMIT_MEMLOCK) >>
>  		     PAGE_SHIFT;
> -	npages = pages->npinned - pages->last_npinned;
>  	do {
>  		cur_pages = atomic_long_read(&pages->source_user->locked_vm);
>  		new_pages = cur_pages + npages;

Re: [PATCH 1/3] iommufd: Fix comment typos

[Eric Auger]

On 12/7/22 21:44, Jason Gunthorpe wrote:
> Repair some typos in comments that were noticed late in the review
> cycle.
>
> Fixes: f394576eb11d ("iommufd: PFN handling for iopt_pages")
> Reported-by: Binbin Wu <binbin.wu@linux.intel.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Eric Auger <eric.auger@redhat.com>


Eric
> ---
>  drivers/iommu/iommufd/io_pagetable.h | 2 +-
>  drivers/iommu/iommufd/pages.c        | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iommu/iommufd/io_pagetable.h b/drivers/iommu/iommufd/io_pagetable.h
> index 83e7c175f2a277..0ec3509b7e3392 100644
> --- a/drivers/iommu/iommufd/io_pagetable.h
> +++ b/drivers/iommu/iommufd/io_pagetable.h
> @@ -33,7 +33,7 @@ struct iommu_domain;
>   *
>   * The io_pagetable::iova_rwsem protects node
>   * The iopt_pages::mutex protects pages_node
> - * iopt and immu_prot are immutable
> + * iopt and iommu_prot are immutable
>   * The pages::mutex protects num_accesses
>   */
>  struct iopt_area {
> diff --git a/drivers/iommu/iommufd/pages.c b/drivers/iommu/iommufd/pages.c
> index 429fa3b0a239cd..fccdba782cb699 100644
> --- a/drivers/iommu/iommufd/pages.c
> +++ b/drivers/iommu/iommufd/pages.c
> @@ -342,7 +342,7 @@ static void batch_destroy(struct pfn_batch *batch, void *backup)
>  		kfree(batch->pfns);
>  }
>  
> -/* true if the pfn could be added, false otherwise */
> +/* true if the pfn was added, false otherwise */
>  static bool batch_add_pfn(struct pfn_batch *batch, unsigned long pfn)
>  {
>  	const unsigned int MAX_NPFNS = type_max(typeof(*batch->npfns));
> @@ -418,7 +418,7 @@ static struct page **raw_pages_from_domain(struct iommu_domain *domain,
>  	return out_pages;
>  }
>  
> -/* Continues reading a domain until we reach a discontiguity in the pfns. */
> +/* Continues reading a domain until we reach a discontinuity in the pfns. */
>  static void batch_from_domain_continue(struct pfn_batch *batch,
>  				       struct iommu_domain *domain,
>  				       struct iopt_area *area,

Re: [PATCH 3/3] iommufd: Change the order of MSI setup

[Eric Auger]

Hi Jason,

On 12/7/22 21:44, Jason Gunthorpe wrote:
> Eric points out this is wrong for the rare case of someone using
> allow_unsafe_interrupts on ARM. We always have to setup the MSI window in
> the domain if the iommu driver asks for it.
>
> Move the iommu_get_msi_cookie() setup to the top of the function and
> always do it, regardless of the security mode. Add checks to
> iommufd_device_setup_msi() to ensure the driver is not doing something
> incomprehensible. No current driver will set both a HW and SW MSI window.
>
> Fixes: e8d57210035b ("iommufd: Add kAPI toward external drivers for physical devices")
> Reported-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/iommufd/device.c       | 54 ++++++++++++----------------
>  drivers/iommu/iommufd/io_pagetable.c | 24 +++++++------
>  2 files changed, 37 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index dd2a415b603e3b..29ff97f756bc42 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -139,19 +139,11 @@ static int iommufd_device_setup_msi(struct iommufd_device *idev,
>  	int rc;
>  
>  	/*
> -	 * IOMMU_CAP_INTR_REMAP means that the platform is isolating MSI, and it
> -	 * creates the MSI window by default in the iommu domain. Nothing
> -	 * further to do.
> -	 */
> -	if (device_iommu_capable(idev->dev, IOMMU_CAP_INTR_REMAP))
> -		return 0;
> -
> -	/*
> -	 * On ARM systems that set the global IRQ_DOMAIN_FLAG_MSI_REMAP every
> -	 * allocated iommu_domain will block interrupts by default and this
> -	 * special flow is needed to turn them back on. iommu_dma_prepare_msi()
> -	 * will install pages into our domain after request_irq() to make this
> -	 * work.
> +	 * If the IOMMU driver gives a IOMMU_RESV_SW_MSI then it is asking us to
> +	 * call iommu_get_msi_cookie() on its behalf. This is necessary to setup
> +	 * the MSI window so iommu_dma_prepare_msi() can install pages into our
> +	 * domain after request_irq(). If it is not done interrupts will not
> +	 * work on this domain.
>  	 *
>  	 * FIXME: This is conceptually broken for iommufd since we want to allow
>  	 * userspace to change the domains, eg switch from an identity IOAS to a
> @@ -159,33 +151,33 @@ static int iommufd_device_setup_msi(struct iommufd_device *idev,
>  	 * matches what the IRQ layer actually expects in a newly created
>  	 * domain.
>  	 */
> -	if (irq_domain_check_msi_remap()) {
> -		if (WARN_ON(!sw_msi_start))
> -			return -EPERM;
> +	if (sw_msi_start != PHYS_ADDR_MAX && !hwpt->msi_cookie) {
> +		rc = iommu_get_msi_cookie(hwpt->domain, sw_msi_start);
> +		if (rc)
> +			return rc;
> +
>  		/*
>  		 * iommu_get_msi_cookie() can only be called once per domain,
>  		 * it returns -EBUSY on later calls.
>  		 */
> -		if (hwpt->msi_cookie)
> -			return 0;
> -		rc = iommu_get_msi_cookie(hwpt->domain, sw_msi_start);
> -		if (rc)
> -			return rc;
>  		hwpt->msi_cookie = true;
> -		return 0;
>  	}
>  
>  	/*
> -	 * Otherwise the platform has a MSI window that is not isolated. For
> -	 * historical compat with VFIO allow a module parameter to ignore the
> -	 * insecurity.
> +	 * For historical compat with VFIO the insecure interrupt path is
> +	 * allowed if the module parameter is set. Insecure means that a MemWr
> +	 * operation from the device (eg a simple DMA) cannot trigger an
s/cannot/can
> +	 * interrupt outside this iommufd context.
>  	 */
> -	if (!allow_unsafe_interrupts)
> -		return -EPERM;
> +	if (!device_iommu_capable(idev->dev, IOMMU_CAP_INTR_REMAP) &&
> +	    !irq_domain_check_msi_remap()) {
> +		if (!allow_unsafe_interrupts)
> +			return -EPERM;
>  
> -	dev_warn(
> -		idev->dev,
> -		"MSI interrupt window cannot be isolated by the IOMMU, this platform is insecure. Use the \"allow_unsafe_interrupts\" module parameter to override\n");
> +		dev_warn(
> +			idev->dev,
> +			"MSI interrupt window cannot be isolated by the IOMMU, this platform is insecure. Use the \"allow_unsafe_interrupts\" module parameter to override\n");
I really prefer the legacy VFIO warning message: "No interrupt remapping
support.  Use ...". IRQ remapping is the feature that is actually
checked. MSI interrupt window sounds confusing: is it the MSI doorbell
or the range of addresses that correspond to MSI addresses.

Forgot to mention there is a minor typo in commit msg 2/3 "sicne"

Thanks

Eric
> +	}
>  	return 0;
>  }
>  
> @@ -203,7 +195,7 @@ static bool iommufd_hw_pagetable_has_group(struct iommufd_hw_pagetable *hwpt,
>  static int iommufd_device_do_attach(struct iommufd_device *idev,
>  				    struct iommufd_hw_pagetable *hwpt)
>  {
> -	phys_addr_t sw_msi_start = 0;
> +	phys_addr_t sw_msi_start = PHYS_ADDR_MAX;
>  	int rc;
>  
>  	mutex_lock(&hwpt->devices_lock);
> diff --git a/drivers/iommu/iommufd/io_pagetable.c b/drivers/iommu/iommufd/io_pagetable.c
> index 3467cea795684c..e0ae72b9e67f86 100644
> --- a/drivers/iommu/iommufd/io_pagetable.c
> +++ b/drivers/iommu/iommufd/io_pagetable.c
> @@ -1170,6 +1170,8 @@ int iopt_table_enforce_group_resv_regions(struct io_pagetable *iopt,
>  	struct iommu_resv_region *resv;
>  	struct iommu_resv_region *tmp;
>  	LIST_HEAD(group_resv_regions);
> +	unsigned int num_hw_msi = 0;
> +	unsigned int num_sw_msi = 0;
>  	int rc;
>  
>  	down_write(&iopt->iova_rwsem);
> @@ -1181,23 +1183,25 @@ int iopt_table_enforce_group_resv_regions(struct io_pagetable *iopt,
>  		if (resv->type == IOMMU_RESV_DIRECT_RELAXABLE)
>  			continue;
>  
> -		/*
> -		 * The presence of any 'real' MSI regions should take precedence
> -		 * over the software-managed one if the IOMMU driver happens to
> -		 * advertise both types.
> -		 */
> -		if (sw_msi_start && resv->type == IOMMU_RESV_MSI) {
> -			*sw_msi_start = 0;
> -			sw_msi_start = NULL;
> -		}
> -		if (sw_msi_start && resv->type == IOMMU_RESV_SW_MSI)
> +		if (sw_msi_start && resv->type == IOMMU_RESV_MSI)
> +			num_hw_msi++;
> +		if (sw_msi_start && resv->type == IOMMU_RESV_SW_MSI) {
>  			*sw_msi_start = resv->start;
> +			num_sw_msi++;
> +		}
>  
>  		rc = iopt_reserve_iova(iopt, resv->start,
>  				       resv->length - 1 + resv->start, device);
>  		if (rc)
>  			goto out_reserved;
>  	}
> +
> +	/* Drivers must offer sane combinations of regions */
> +	if (WARN_ON(num_sw_msi && num_hw_msi) || WARN_ON(num_sw_msi > 1)) {
> +		rc = -EINVAL;
> +		goto out_reserved;
> +	}
> +
>  	rc = 0;
>  	goto out_free_resv;
>  

Re: [PATCH 3/3] iommufd: Change the order of MSI setup

[Jason Gunthorpe]

On Thu, Dec 08, 2022 at 03:18:49PM +0100, Eric Auger wrote:
> > -	dev_warn(
> > -		idev->dev,
> > -		"MSI interrupt window cannot be isolated by the IOMMU, this platform is insecure. Use the \"allow_unsafe_interrupts\" module parameter to override\n");
> > +		dev_warn(
> > +			idev->dev,
> > +			"MSI interrupt window cannot be isolated by the IOMMU, this platform is insecure. Use the \"allow_unsafe_interrupts\" module parameter to override\n");

> I really prefer the legacy VFIO warning message: "No interrupt remapping
> support.  Use ...". IRQ remapping is the feature that is actually
> checked. MSI interrupt window sounds confusing: is it the MSI doorbell
> or the range of addresses that correspond to MSI addresses.

Well, that is true for x86 processors, but it is not what it is
called on ARM. I wanted something more neutral, but am not fussed
about it.

And I suppose "by the IOMMU" is not really acurate either.

How about

"MSI interrupts are not secure, they cannot be isolated by the
platform. Check that platform features like interrupt remapping are
enabled. Use the \"allow_unsafe_interrupts\" module parameter to override\n"

?

Thanks,
Jason

Re: [PATCH 3/3] iommufd: Change the order of MSI setup

[Eric Auger]

Hi,

On 12/8/22 15:35, Jason Gunthorpe wrote:
> On Thu, Dec 08, 2022 at 03:18:49PM +0100, Eric Auger wrote:
>>> -	dev_warn(
>>> -		idev->dev,
>>> -		"MSI interrupt window cannot be isolated by the IOMMU, this platform is insecure. Use the \"allow_unsafe_interrupts\" module parameter to override\n");
>>> +		dev_warn(
>>> +			idev->dev,
>>> +			"MSI interrupt window cannot be isolated by the IOMMU, this platform is insecure. Use the \"allow_unsafe_interrupts\" module parameter to override\n");
>> I really prefer the legacy VFIO warning message: "No interrupt remapping
>> support.  Use ...". IRQ remapping is the feature that is actually
>> checked. MSI interrupt window sounds confusing: is it the MSI doorbell
>> or the range of addresses that correspond to MSI addresses.
> Well, that is true for x86 processors, but it is not what it is
> called on ARM. I wanted something more neutral, but am not fussed
> about it.
>
> And I suppose "by the IOMMU" is not really acurate either.
>
> How about
>
> "MSI interrupts are not secure, they cannot be isolated by the
> platform. Check that platform features like interrupt remapping are
> enabled. Use the \"allow_unsafe_interrupts\" module parameter to override\n"

yep sounds OK

Thanks

Eric
>
> ?
>
> Thanks,
> Jason
>

Re: [PATCH 0/3] iommufd review updates

[Jason Gunthorpe]

On Wed, Dec 07, 2022 at 04:44:40PM -0400, Jason Gunthorpe wrote:
> A few patches from review comments that happened a bit too late to get
> rebased into the commits.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> 
> Jason Gunthorpe (3):
>   iommufd: Fix comment typos
>   iommufd: Improve a few unclear bits of code
>   iommufd: Change the order of MSI setup
> 
>  drivers/iommu/iommufd/device.c       | 54 ++++++++++++----------------
>  drivers/iommu/iommufd/io_pagetable.c | 24 +++++++------
>  drivers/iommu/iommufd/io_pagetable.h |  2 +-
>  drivers/iommu/iommufd/pages.c        | 11 +++---
>  4 files changed, 45 insertions(+), 46 deletions(-)

Applied to the iommufd tree

Jason