Re: [PATCH 11/14] vfio: clean up the check for mediated device in vfio_iommu_type1

[Kirti Wankhede <kwankhede@xxxxxxxxxx>]

On 9/13/2021 12:46 PM, Christoph Hellwig wrote:
> Pass the group flags to ->attach_group and remove the messy check for
> the bus type.

I like the way vfio_group_type is used in this patch, that removes messy 
way to call symbol_get(mdev_bus_type).

Any thoughts on how VFIO_IOMMU, i.e. IOMMU backed mdev, can be implemented?

For IOMMU backed mdev, mdev->dev->iommu_group should be same as 
mdev->type->parent->dev->iommu_group or in other words, parent device 
would be DMA alias for the mdev device with the restriction - single 
mdev device can be created for the physical device. Is it possible to 
link iommu_group of these two devices some way?

Thanks,
Kirti

> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> Reviewed-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> ---
>   drivers/vfio/vfio.c                 | 32 +++++------------------------
>   drivers/vfio/vfio.h                 | 27 +++++++++++++++++++++++-
>   drivers/vfio/vfio_iommu_spapr_tce.c |  2 +-
>   drivers/vfio/vfio_iommu_type1.c     | 19 ++---------------
>   4 files changed, 34 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 6589e296ef348c..08b27b64f0f935 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -68,30 +68,6 @@ struct vfio_unbound_dev {
>   	struct list_head		unbound_next;
>   };
>   
> -enum vfio_group_type {
> -	/*
> -	 * Physical device with IOMMU backing.
> -	 */
> -	VFIO_IOMMU,
> -
> -	/*
> -	 * Virtual device without IOMMU backing. The VFIO core fakes up an
> -	 * iommu_group as the iommu_group sysfs interface is part of the
> -	 * userspace ABI.  The user of these devices must not be able to
> -	 * directly trigger unmediated DMA.
> -	 */
> -	VFIO_EMULATED_IOMMU,
> -
> -	/*
> -	 * Physical device without IOMMU backing. The VFIO core fakes up an
> -	 * iommu_group as the iommu_group sysfs interface is part of the
> -	 * userspace ABI.  Users can trigger unmediated DMA by the device,
> -	 * usage is highly dangerous, requires an explicit opt-in and will
> -	 * taint the kernel.
> -	 */
> -	VFIO_NO_IOMMU,
> -};
> -
>   struct vfio_group {
>   	struct kref			kref;
>   	int				minor;
> @@ -219,7 +195,7 @@ static long vfio_noiommu_ioctl(void *iommu_data,
>   }
>   
>   static int vfio_noiommu_attach_group(void *iommu_data,
> -				     struct iommu_group *iommu_group)
> +		struct iommu_group *iommu_group, enum vfio_group_type type)
>   {
>   	return 0;
>   }
> @@ -1129,7 +1105,8 @@ static int __vfio_container_attach_groups(struct vfio_container *container,
>   	int ret = -ENODEV;
>   
>   	list_for_each_entry(group, &container->group_list, container_next) {
> -		ret = driver->ops->attach_group(data, group->iommu_group);
> +		ret = driver->ops->attach_group(data, group->iommu_group,
> +						group->type);
>   		if (ret)
>   			goto unwind;
>   	}
> @@ -1387,7 +1364,8 @@ static int vfio_group_set_container(struct vfio_group *group, int container_fd)
>   	driver = container->iommu_driver;
>   	if (driver) {
>   		ret = driver->ops->attach_group(container->iommu_data,
> -						group->iommu_group);
> +						group->iommu_group,
> +						group->type);
>   		if (ret)
>   			goto unlock_out;
>   	}
> diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
> index a78de649eb2f16..a6713022115155 100644
> --- a/drivers/vfio/vfio.h
> +++ b/drivers/vfio/vfio.h
> @@ -4,6 +4,30 @@
>    *     Author: Alex Williamson <alex.williamson@xxxxxxxxxx>
>    */
>   
> +enum vfio_group_type {
> +	/*
> +	 * Physical device with IOMMU backing.
> +	 */
> +	VFIO_IOMMU,
> +
> +	/*
> +	 * Virtual device without IOMMU backing. The VFIO core fakes up an
> +	 * iommu_group as the iommu_group sysfs interface is part of the
> +	 * userspace ABI.  The user of these devices must not be able to
> +	 * directly trigger unmediated DMA.
> +	 */
> +	VFIO_EMULATED_IOMMU,
> +
> +	/*
> +	 * Physical device without IOMMU backing. The VFIO core fakes up an
> +	 * iommu_group as the iommu_group sysfs interface is part of the
> +	 * userspace ABI.  Users can trigger unmediated DMA by the device,
> +	 * usage is highly dangerous, requires an explicit opt-in and will
> +	 * taint the kernel.
> +	 */
> +	VFIO_NO_IOMMU,
> +};
> +
>   /* events for the backend driver notify callback */
>   enum vfio_iommu_notify_type {
>   	VFIO_IOMMU_CONTAINER_CLOSE = 0,
> @@ -20,7 +44,8 @@ struct vfio_iommu_driver_ops {
>   	long		(*ioctl)(void *iommu_data, unsigned int cmd,
>   				 unsigned long arg);
>   	int		(*attach_group)(void *iommu_data,
> -					struct iommu_group *group);
> +					struct iommu_group *group,
> +					enum vfio_group_type);
>   	void		(*detach_group)(void *iommu_data,
>   					struct iommu_group *group);
>   	int		(*pin_pages)(void *iommu_data,
> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> index 3efd09faeca4a8..936a26b13c0b01 100644
> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> @@ -1239,7 +1239,7 @@ static long tce_iommu_take_ownership_ddw(struct tce_container *container,
>   }
>   
>   static int tce_iommu_attach_group(void *iommu_data,
> -		struct iommu_group *iommu_group)
> +		struct iommu_group *iommu_group, enum vfio_group_type type)
>   {
>   	int ret = 0;
>   	struct tce_container *container = iommu_data;
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 42a6be1fb7265e..a48e9f597cb213 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -36,7 +36,6 @@
>   #include <linux/uaccess.h>
>   #include <linux/vfio.h>
>   #include <linux/workqueue.h>
> -#include <linux/mdev.h>
>   #include <linux/notifier.h>
>   #include <linux/dma-iommu.h>
>   #include <linux/irqdomain.h>
> @@ -1934,20 +1933,6 @@ static bool vfio_iommu_has_sw_msi(struct list_head *group_resv_regions,
>   	return ret;
>   }
>   
> -static bool vfio_bus_is_mdev(struct bus_type *bus)
> -{
> -	struct bus_type *mdev_bus;
> -	bool ret = false;
> -
> -	mdev_bus = symbol_get(mdev_bus_type);
> -	if (mdev_bus) {
> -		ret = (bus == mdev_bus);
> -		symbol_put(mdev_bus_type);
> -	}
> -
> -	return ret;
> -}
> -
>   /*
>    * This is a helper function to insert an address range to iova list.
>    * The list is initially created with a single entry corresponding to
> @@ -2172,7 +2157,7 @@ static void vfio_iommu_iova_insert_copy(struct vfio_iommu *iommu,
>   }
>   
>   static int vfio_iommu_type1_attach_group(void *iommu_data,
> -					 struct iommu_group *iommu_group)
> +		struct iommu_group *iommu_group, enum vfio_group_type type)
>   {
>   	struct vfio_iommu *iommu = iommu_data;
>   	struct vfio_iommu_group *group;
> @@ -2207,7 +2192,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>   	if (ret)
>   		goto out_free;
>   
> -	if (vfio_bus_is_mdev(bus)) {
> +	if (type == VFIO_EMULATED_IOMMU) {
>   		if (!iommu->external_domain) {
>   			INIT_LIST_HEAD(&domain->group_list);
>   			iommu->external_domain = domain;