Re: [Qemu-devel] [PATCH v9 01/12] vfio: Mediated device Core driver

[Jike Song]

On 10/21/2016 01:12 AM, Alex Williamson wrote:
> On Thu, 20 Oct 2016 15:23:53 +0800
> Jike Song <address@hidden> wrote:
> 
>> On 10/18/2016 05:22 AM, Kirti Wankhede wrote:
>>> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
>>> new file mode 100644
>>> index 000000000000..7db5ec164aeb
>>> --- /dev/null
>>> +++ b/drivers/vfio/mdev/mdev_core.c
>>> @@ -0,0 +1,372 @@
>>> +/*
>>> + * Mediated device Core Driver
>>> + *
>>> + * Copyright (c) 2016, NVIDIA CORPORATION. All rights reserved.
>>> + *     Author: Neo Jia <address@hidden>
>>> + *        Kirti Wankhede <address@hidden>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/device.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/uuid.h>
>>> +#include <linux/sysfs.h>
>>> +#include <linux/mdev.h>
>>> +
>>> +#include "mdev_private.h"
>>> +
>>> +#define DRIVER_VERSION             "0.1"
>>> +#define DRIVER_AUTHOR              "NVIDIA Corporation"
>>> +#define DRIVER_DESC                "Mediated device Core Driver"
>>> +
>>> +static LIST_HEAD(parent_list);
>>> +static DEFINE_MUTEX(parent_list_lock);
>>> +static struct class_compat *mdev_bus_compat_class;
>>> +  
>>
>>> +
>>> +/*
>>> + * mdev_register_device : Register a device
>>> + * @dev: device structure representing parent device.
>>> + * @ops: Parent device operation structure to be registered.
>>> + *
>>> + * Add device to list of registered parent devices.
>>> + * Returns a negative value on error, otherwise 0.
>>> + */
>>> +int mdev_register_device(struct device *dev, const struct parent_ops *ops)
>>> +{
>>> +   int ret = 0;
>>> +   struct parent_device *parent;
>>> +
>>> +   /* check for mandatory ops */
>>> +   if (!ops || !ops->create || !ops->remove || !ops->supported_type_groups)
>>> +           return -EINVAL;
>>> +
>>> +   dev = get_device(dev);
>>> +   if (!dev)
>>> +           return -EINVAL;
>>> +
>>> +   mutex_lock(&parent_list_lock);
>>> +
>>> +   /* Check for duplicate */
>>> +   parent = __find_parent_device(dev);
>>> +   if (parent) {
>>> +           ret = -EEXIST;
>>> +           goto add_dev_err;
>>> +   }
>>> +
>>> +   parent = kzalloc(sizeof(*parent), GFP_KERNEL);
>>> +   if (!parent) {
>>> +           ret = -ENOMEM;
>>> +           goto add_dev_err;
>>> +   }
>>> +
>>> +   kref_init(&parent->ref);
>>> +
>>> +   parent->dev = dev;
>>> +   parent->ops = ops;
>>> +
>>> +   ret = parent_create_sysfs_files(parent);
>>> +   if (ret) {
>>> +           mutex_unlock(&parent_list_lock);
>>> +           mdev_put_parent(parent);
>>> +           return ret;
>>> +   }
>>> +
>>> +   ret = class_compat_create_link(mdev_bus_compat_class, dev, NULL);
>>> +   if (ret)
>>> +           dev_warn(dev, "Failed to create compatibility class link\n");
>>> +
>>> +   list_add(&parent->next, &parent_list);
>>> +   mutex_unlock(&parent_list_lock);
>>> +
>>> +   dev_info(dev, "MDEV: Registered\n");
>>> +   return 0;
>>> +
>>> +add_dev_err:
>>> +   mutex_unlock(&parent_list_lock);
>>> +   put_device(dev);
>>> +   return ret;
>>> +}
>>> +EXPORT_SYMBOL(mdev_register_device);  
>>
>>> +static int __init mdev_init(void)
>>> +{
>>> +   int ret;
>>> +
>>> +   ret = mdev_bus_register();
>>> +   if (ret) {
>>> +           pr_err("Failed to register mdev bus\n");
>>> +           return ret;
>>> +   }
>>> +
>>> +   mdev_bus_compat_class = class_compat_register("mdev_bus");
>>> +   if (!mdev_bus_compat_class) {
>>> +           mdev_bus_unregister();
>>> +           return -ENOMEM;
>>> +   }
>>> +
>>> +   /*
>>> +    * Attempt to load known vfio_mdev.  This gives us a working environment
>>> +    * without the user needing to explicitly load vfio_mdev driver.
>>> +    */
>>> +   request_module_nowait("vfio_mdev");
>>> +
>>> +   return ret;
>>> +}
>>> +
>>> +static void __exit mdev_exit(void)
>>> +{
>>> +   class_compat_unregister(mdev_bus_compat_class);
>>> +   mdev_bus_unregister();
>>> +}
>>> +
>>> +module_init(mdev_init)
>>> +module_exit(mdev_exit)  
>>
>> Hi Kirti,
>>
>> There is a possible issue: mdev_bus_register is called from mdev_init,
>> a module_init, equal to device_initcall if builtin to vmlinux; however,
>> the vendor driver, say i915.ko for intel case, have to call
>> mdev_register_device from its module_init: at that time, mdev_init
>> is still not called.
>>
>> Not sure if this issue exists with nvidia.ko. Though in most cases we
>> are expecting users select mdev as a standalone module, we still won't
>> break builtin case.
>>
>>
>> Hi Alex, do you have any suggestion here?
> 
> To fully solve the problem of built-in drivers making use of the mdev
> infrastructure we'd need to make mdev itself builtin and possibly a
> subsystem that is initialized prior to device drivers.  Is that really
> necessary?  Even though i915.ko is often loaded as part of an
> initramfs, most systems still build it as a module.  I would expect
> that standard module dependencies will pull in the necessary mdev and
> vfio modules to make this work correctly.  I can't say that I'm
> prepared to make mdev be a subsystem as would be necessary for builtin
> drivers to make use of.  Perhaps if such a driver exists it could
> somehow do late binding with mdev.  i915 should certainly be tested as
> a builtin driver to make sure it doesn't fail with mdev support added.
> The kvm-vfio device (virt/kvm/vfio.c) makes use of symbol tricks to
> avoid hard dependencies between kvm and vfio, perhaps when builtin to
> the kernel, i915 could use something like that.  Thanks,

Fair enough, I'll use symbol_get to avoid the problem. Thanks!

--
Thanks,
Jike