qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Alex Williamson
Subject: Re: [Qemu-devel] [PATCH v9 01/12] vfio: Mediated device Core driver
Date: Thu, 20 Oct 2016 11:12:31 -0600

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,

Alex



reply via email to

[Prev in Thread] Current Thread [Next in Thread]