Re: [PATCH v2 11/11] iommufd: Allow iommufd to supply /dev/vfio/vfio

[Yi Liu <yi.l.liu@intel.com>]

On 2022/11/11 12:16, Yi Liu wrote:
> 
> 
> On 2022/11/8 08:52, Jason Gunthorpe wrote:
>> If the VFIO container is compiled out, give a kconfig option for iommufd
>> to provide the miscdev node with the same name and permissions as vfio
>> uses.
>>
>> The compatibility node supports the same ioctls as VFIO and automatically
>> enables the VFIO compatible pinned page accounting mode.
>>
>> Tested-by: Nicolin Chen <nicolinc@nvidia.com>
>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>> ---
>>   drivers/iommu/iommufd/Kconfig | 12 ++++++++++++
>>   drivers/iommu/iommufd/main.c  | 36 +++++++++++++++++++++++++++++++++++
>>   2 files changed, 48 insertions(+)
>>
>> diff --git a/drivers/iommu/iommufd/Kconfig b/drivers/iommu/iommufd/Kconfig
>> index 399a2edeaef6de..f387f803dc6f7f 100644
>> --- a/drivers/iommu/iommufd/Kconfig
>> +++ b/drivers/iommu/iommufd/Kconfig
>> @@ -12,6 +12,18 @@ config IOMMUFD
>>         If you don't know what to do here, say N.
>>   if IOMMUFD
>> +config IOMMUFD_VFIO_CONTAINER
>> +    bool "IOMMUFD provides the VFIO container /dev/vfio/vfio"
>> +    depends on VFIO && !VFIO_CONTAINER
>> +    default VFIO && !VFIO_CONTAINER
>> +    help
>> +      IOMMUFD will provide /dev/vfio/vfio instead of VFIO. This relies on
>> +      IOMMUFD providing compatibility emulation to give the same ioctls.
>> +      It provides an option to build a kernel with legacy VFIO components
>> +      removed.
>> +
>> +      Unless testing IOMMUFD say N here.
>> +
>>   config IOMMUFD_TEST
>>       bool "IOMMU Userspace API Test support"
>>       depends on RUNTIME_TESTING_MENU
>> diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c
>> index ab3fa05f38505d..1eeb326f74f005 100644
>> --- a/drivers/iommu/iommufd/main.c
>> +++ b/drivers/iommu/iommufd/main.c
>> @@ -18,6 +18,7 @@
>>   #include <uapi/linux/iommufd.h>
>>   #include <linux/iommufd.h>
>> +#include "io_pagetable.h"
>>   #include "iommufd_private.h"
>>   #include "iommufd_test.h"
>> @@ -25,6 +26,7 @@ struct iommufd_object_ops {
>>       void (*destroy)(struct iommufd_object *obj);
>>   };
>>   static const struct iommufd_object_ops iommufd_object_ops[];
>> +static struct miscdevice vfio_misc_dev;
>>   struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx,
>>                            size_t size,
>> @@ -170,6 +172,16 @@ static int iommufd_fops_open(struct inode *inode, 
>> struct file *filp)
>>       if (!ictx)
>>           return -ENOMEM;
>> +    /*
>> +     * For compatibility with VFIO when /dev/vfio/vfio is opened we default
>> +     * to the same rlimit accounting as vfio uses.
>> +     */
>> +    if (IS_ENABLED(CONFIG_IOMMUFD_VFIO_CONTAINER) &&
>> +        filp->private_data == &vfio_misc_dev) {
>> +        ictx->account_mode = IOPT_PAGES_ACCOUNT_MM;
>> +        pr_info_once("IOMMUFD is providing /dev/vfio/vfio, not VFIO.\n");
>> +    }
>> +
>>       xa_init_flags(&ictx->objects, XA_FLAGS_ALLOC1 | XA_FLAGS_ACCOUNT);
>>       ictx->file = filp;
>>       filp->private_data = ictx;
>> @@ -395,6 +407,15 @@ static struct miscdevice iommu_misc_dev = {
>>       .mode = 0660,
>>   };
>> +
>> +static struct miscdevice vfio_misc_dev = {
>> +    .minor = VFIO_MINOR,
>> +    .name = "vfio",
>> +    .fops = &iommufd_fops,
>> +    .nodename = "vfio/vfio",
>> +    .mode = 0666,
>> +};
>> +
>>   static int __init iommufd_init(void)
>>   {
>>       int ret;
>> @@ -402,18 +423,33 @@ static int __init iommufd_init(void)
>>       ret = misc_register(&iommu_misc_dev);
>>       if (ret)
>>           return ret;
>> +
>> +    if (IS_ENABLED(CONFIG_IOMMUFD_VFIO_CONTAINER)) {
>> +        ret = misc_register(&vfio_misc_dev);
>> +        if (ret)
>> +            goto err_misc;
>> +    }
>>       iommufd_test_init();
>>       return 0;
>> +err_misc:
>> +    misc_deregister(&iommu_misc_dev);
>> +    return ret;
>>   }
>>   static void __exit iommufd_exit(void)
>>   {
>>       iommufd_test_exit();
>> +    if (IS_ENABLED(CONFIG_IOMMUFD_VFIO_CONTAINER))
>> +        misc_deregister(&vfio_misc_dev);
>>       misc_deregister(&iommu_misc_dev);
>>   }
>>   module_init(iommufd_init);
>>   module_exit(iommufd_exit);
>> +#if IS_ENABLED(CONFIG_IOMMUFD_VFIO_CONTAINER)
>> +MODULE_ALIAS_MISCDEV(VFIO_MINOR);
>> +MODULE_ALIAS("devname:vfio/vfio");
> 
> will this line also result in systemd to create this devnodes at boot
> based on the module info even if the IOMMUFD_VFIO_CONTAINER is not
> configured?

stale comment. it's already under if IS_ENABLED(CONFIG_IOMMUFD_VFIO_CONTAINER).

looks good to me.

Reviewed-by: Yi Liu <yi.l.liu@intel.com>

>> +#endif
>>   MODULE_DESCRIPTION("I/O Address Space Management for passthrough 
>> devices");
>>   MODULE_LICENSE("GPL");
> 

-- 
Regards,
Yi Liu