diff mbox series

[v2,01/17] iommufd: Move isolated msi enforcement to iommufd_device_bind()

Message ID 1-v2-51b9896e7862+8a8c-iommufd_alloc_jgg@nvidia.com (mailing list archive)
State New, archived
Headers show
Series Add iommufd physical device operations for replace and alloc hwpt | expand

Commit Message

Jason Gunthorpe March 8, 2023, 12:35 a.m. UTC
With the recent rework this no longer needs to be done at domain
attachment time, we know if the device is usable by iommufd when we bind
it.

The value of msi_device_has_isolated_msi() is not allowed to change while
a driver is bound.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommufd/device.c | 38 ++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 18 deletions(-)

Comments

Baolu Lu March 8, 2023, 2:55 a.m. UTC | #1
On 3/8/23 8:35 AM, Jason Gunthorpe wrote:
> With the recent rework this no longer needs to be done at domain
> attachment time, we know if the device is usable by iommufd when we bind
> it.
> 
> The value of msi_device_has_isolated_msi() is not allowed to change while
> a driver is bound.
> 
> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>
> ---
>   drivers/iommu/iommufd/device.c | 38 ++++++++++++++++++----------------
>   1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index c6f4852a8a0c08..63b65cdfe97f29 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -60,6 +60,26 @@ struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx,
>   	if (!group)
>   		return ERR_PTR(-ENODEV);
>   
> +	/*
> +	 * 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

Nit:

"... cannot trigger an ..." or "... can trigger an ..."?

> +	 * interrupt outside this iommufd context.
> +	 */
> +	if (!iommufd_selftest_is_mock_dev(dev) &&
> +	    !iommu_group_has_isolated_msi(group)) {
> +		if (!allow_unsafe_interrupts) {
> +			rc = -EPERM;
> +			goto out_group_put;
> +		}
> +
> +		dev_warn(
> +			dev,
> +			"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");
> +	}

Anyway,

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

Best regards,
baolu
Jason Gunthorpe March 9, 2023, 12:53 p.m. UTC | #2
On Wed, Mar 08, 2023 at 10:55:31AM +0800, Baolu Lu wrote:
> On 3/8/23 8:35 AM, Jason Gunthorpe wrote:
> > With the recent rework this no longer needs to be done at domain
> > attachment time, we know if the device is usable by iommufd when we bind
> > it.
> > 
> > The value of msi_device_has_isolated_msi() is not allowed to change while
> > a driver is bound.
> > 
> > Reviewed-by: Kevin Tian<kevin.tian@intel.com>
> > Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>
> > ---
> >   drivers/iommu/iommufd/device.c | 38 ++++++++++++++++++----------------
> >   1 file changed, 20 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> > index c6f4852a8a0c08..63b65cdfe97f29 100644
> > --- a/drivers/iommu/iommufd/device.c
> > +++ b/drivers/iommu/iommufd/device.c
> > @@ -60,6 +60,26 @@ struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx,
> >   	if (!group)
> >   		return ERR_PTR(-ENODEV);
> > +	/*
> > +	 * 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
> 
> Nit:
> 
> "... cannot trigger an ..." or "... can trigger an ..."?

Oh, yes that got flipped at some point

Thanks,
Jason
diff mbox series

Patch

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index c6f4852a8a0c08..63b65cdfe97f29 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -60,6 +60,26 @@  struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx,
 	if (!group)
 		return ERR_PTR(-ENODEV);
 
+	/*
+	 * 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 (!iommufd_selftest_is_mock_dev(dev) &&
+	    !iommu_group_has_isolated_msi(group)) {
+		if (!allow_unsafe_interrupts) {
+			rc = -EPERM;
+			goto out_group_put;
+		}
+
+		dev_warn(
+			dev,
+			"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");
+	}
+
 	rc = iommu_device_claim_dma_owner(dev, ictx);
 	if (rc)
 		goto out_group_put;
@@ -146,24 +166,6 @@  static int iommufd_device_setup_msi(struct iommufd_device *idev,
 		 */
 		hwpt->msi_cookie = true;
 	}
-
-	/*
-	 * 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 (!iommufd_selftest_is_mock_dev(idev->dev) &&
-	    !iommu_group_has_isolated_msi(idev->group)) {
-		if (!allow_unsafe_interrupts)
-			return -EPERM;
-
-		dev_warn(
-			idev->dev,
-			"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");
-	}
 	return 0;
 }