* Kirti Wankhede <kwankh...@nvidia.com> [2016-11-05 02:40:35 +0530]: Hi Kirti,
[...] > diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig > index da6e2ce77495..23eced02aaf6 100644 > --- a/drivers/vfio/Kconfig > +++ b/drivers/vfio/Kconfig > @@ -48,4 +48,5 @@ menuconfig VFIO_NOIOMMU > > source "drivers/vfio/pci/Kconfig" > source "drivers/vfio/platform/Kconfig" > +source "drivers/vfio/mdev/Kconfig" > source "virt/lib/Kconfig" > diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile > index 7b8a31f63fea..4a23c13b6be4 100644 > --- a/drivers/vfio/Makefile > +++ b/drivers/vfio/Makefile > @@ -7,3 +7,4 @@ obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o > obj-$(CONFIG_VFIO_SPAPR_EEH) += vfio_spapr_eeh.o > obj-$(CONFIG_VFIO_PCI) += pci/ > obj-$(CONFIG_VFIO_PLATFORM) += platform/ > +obj-$(CONFIG_VFIO_MDEV) += mdev/ > diff --git a/drivers/vfio/mdev/Kconfig b/drivers/vfio/mdev/Kconfig > new file mode 100644 > index 000000000000..303c14ce2847 > --- /dev/null > +++ b/drivers/vfio/mdev/Kconfig > @@ -0,0 +1,10 @@ > + > +config VFIO_MDEV > + tristate "Mediated device driver framework" > + depends on VFIO > + default n > + help > + Provides a framework to virtualize devices. > + See Documentation/vfio-mdev/vfio-mediated-device.txt for more details. We don't have this doc at this point of time. > + > + If you don't know what do here, say N. > diff --git a/drivers/vfio/mdev/Makefile b/drivers/vfio/mdev/Makefile > new file mode 100644 > index 000000000000..31bc04801d94 > --- /dev/null > +++ b/drivers/vfio/mdev/Makefile > @@ -0,0 +1,4 @@ > + > +mdev-y := mdev_core.o mdev_sysfs.o mdev_driver.o > + > +obj-$(CONFIG_VFIO_MDEV) += mdev.o > diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c > new file mode 100644 > index 000000000000..54c59f325336 > --- /dev/null > +++ b/drivers/vfio/mdev/mdev_core.c [...] > + > +/* > + * mdev_device_remove_ops gets called from sysfs's 'remove' and when parent > + * device is being unregistered from mdev device framework. > + * - 'force_remove' is set to 'false' when called from sysfs's 'remove' which > + * indicates that if the mdev device is active, used by VMM or userspace > + * application, vendor driver could return error then don't remove the > device. > + * - 'force_remove' is set to 'true' when called from > mdev_unregister_device() > + * which indicate that parent device is being removed from mdev device > + * framework so remove mdev device forcefully. > + */ > +static int mdev_device_remove_ops(struct mdev_device *mdev, bool > force_remove) ? s/force_remove/force/ > +{ > + struct parent_device *parent = mdev->parent; > + int ret; > + > + /* > + * Vendor driver can return error if VMM or userspace application is > + * using this mdev device. > + */ > + ret = parent->ops->remove(mdev); > + if (ret && !force_remove) > + return -EBUSY; > + > + sysfs_remove_groups(&mdev->dev.kobj, parent->ops->mdev_attr_groups); > + return 0; > +} > + > +static int mdev_device_remove_cb(struct device *dev, void *data) > +{ > + if (!dev_is_mdev(dev)) > + return 0; > + > + return mdev_device_remove(dev, data ? *(bool *)data : true); > +} > + > +/* > + * 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; > + 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); > + mutex_init(&parent->lock); > + > + parent->dev = dev; > + parent->ops = ops; > + > + if (!mdev_bus_compat_class) { > + mdev_bus_compat_class = class_compat_register("mdev_bus"); > + if (!mdev_bus_compat_class) { > + ret = -ENOMEM; > + goto add_dev_err; > + } > + } > + > + ret = parent_create_sysfs_files(parent); > + if (ret) > + goto add_dev_err; > + > + 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); > + if (parent) > + mdev_put_parent(parent); Why do this? I don't find the place that you call mdev_get_parent above. > + else > + put_device(dev); Shouldn't we always do this? > + return ret; > +} > +EXPORT_SYMBOL(mdev_register_device); > + > +/* > + * mdev_unregister_device : Unregister a parent device > + * @dev: device structure representing parent device. > + * > + * Remove device from list of registered parent devices. Give a chance to > free > + * existing mediated devices for given device. > + */ > + > +void mdev_unregister_device(struct device *dev) > +{ > + struct parent_device *parent; > + bool force_remove = true; > + > + mutex_lock(&parent_list_lock); > + parent = __find_parent_device(dev); > + > + if (!parent) { > + mutex_unlock(&parent_list_lock); > + return; > + } > + dev_info(dev, "MDEV: Unregistering\n"); > + > + list_del(&parent->next); > + class_compat_remove_link(mdev_bus_compat_class, dev, NULL); > + > + device_for_each_child(dev, (void *)&force_remove, > + mdev_device_remove_cb); > + > + parent_remove_sysfs_files(parent); > + > + mutex_unlock(&parent_list_lock); > + mdev_put_parent(parent); We also need to call put_device(dev) here since we have a get_device during registration, no? Or I must miss something... > +} > +EXPORT_SYMBOL(mdev_unregister_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; > + } > + > + /* > + * 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"); We don't have this module yet. > + > + return ret; > +} > + > +static void __exit mdev_exit(void) > +{ > + if (mdev_bus_compat_class) > + class_compat_unregister(mdev_bus_compat_class); > + > + mdev_bus_unregister(); > +} > + > +module_init(mdev_init) > +module_exit(mdev_exit) > + > +MODULE_VERSION(DRIVER_VERSION); > +MODULE_LICENSE("GPL"); > +MODULE_AUTHOR(DRIVER_AUTHOR); > +MODULE_DESCRIPTION(DRIVER_DESC); > diff --git a/drivers/vfio/mdev/mdev_driver.c b/drivers/vfio/mdev/mdev_driver.c > new file mode 100644 > index 000000000000..0b3250044a80 > --- /dev/null > +++ b/drivers/vfio/mdev/mdev_driver.c > @@ -0,0 +1,122 @@ > +/* > + * MDEV driver > + * > + * Copyright (c) 2016, NVIDIA CORPORATION. All rights reserved. > + * Author: Neo Jia <c...@nvidia.com> > + * Kirti Wankhede <kwankh...@nvidia.com> Don't know if you care much for this: There is a TAB before your name. :> > + * > + * 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/device.h> > +#include <linux/iommu.h> > +#include <linux/mdev.h> > + > +#include "mdev_private.h" > + > +static int mdev_attach_iommu(struct mdev_device *mdev) > +{ > + int ret; > + struct iommu_group *group; > + > + group = iommu_group_alloc(); > + if (IS_ERR(group)) > + return PTR_ERR(group); > + > + ret = iommu_group_add_device(group, &mdev->dev); > + if (ret) > + goto attach_fail; > + > + dev_info(&mdev->dev, "MDEV: group_id = %d\n", > + iommu_group_id(group)); nit: strange indentation. The above two lines could be safely merge into one line. > +attach_fail: > + iommu_group_put(group); > + return ret; > +} > + > +static void mdev_detach_iommu(struct mdev_device *mdev) > +{ > + iommu_group_remove_device(&mdev->dev); > + dev_info(&mdev->dev, "MDEV: detaching iommu\n"); > +} > + > +static int mdev_probe(struct device *dev) > +{ > + struct mdev_driver *drv = to_mdev_driver(dev->driver); > + struct mdev_device *mdev = to_mdev_device(dev); > + int ret; > + > + ret = mdev_attach_iommu(mdev); > + if (ret) > + return ret; > + > + if (drv && drv->probe) > + ret = drv->probe(dev); > + > + if (ret) > + mdev_detach_iommu(mdev); ? if (drv && drv->probe) { ret = drv->probe(dev); if (ret) mdev_detach_iommu(mdev); } > + > + return ret; > +} > + > +static int mdev_remove(struct device *dev) > +{ > + struct mdev_driver *drv = to_mdev_driver(dev->driver); > + struct mdev_device *mdev = to_mdev_device(dev); > + > + if (drv && drv->remove) > + drv->remove(dev); > + > + mdev_detach_iommu(mdev); > + > + return 0; > +} > + > +struct bus_type mdev_bus_type = { > + .name = "mdev", > + .probe = mdev_probe, > + .remove = mdev_remove, > +}; > +EXPORT_SYMBOL_GPL(mdev_bus_type); > + > +/* Is this a kernel-doc comment? It should be started with: /** > + * mdev_register_driver - register a new MDEV driver > + * @drv: the driver to register > + * @owner: module owner of driver to be registered > + * > + * Returns a negative value on error, otherwise 0. > + */ > +int mdev_register_driver(struct mdev_driver *drv, struct module *owner) > +{ > + /* initialize common driver fields */ > + drv->driver.name = drv->name; > + drv->driver.bus = &mdev_bus_type; > + drv->driver.owner = owner; > + > + /* register with core */ > + return driver_register(&drv->driver); > +} > +EXPORT_SYMBOL(mdev_register_driver); > + > +/* > + * mdev_unregister_driver - unregister MDEV driver > + * @drv: the driver to unregister > + * Empty line. > + */ > +void mdev_unregister_driver(struct mdev_driver *drv) > +{ > + driver_unregister(&drv->driver); > +} > +EXPORT_SYMBOL(mdev_unregister_driver); > + > +int mdev_bus_register(void) > +{ > + return bus_register(&mdev_bus_type); > +} > + > +void mdev_bus_unregister(void) > +{ > + bus_unregister(&mdev_bus_type); > +} [...] > diff --git a/include/linux/mdev.h b/include/linux/mdev.h > +/* Mediated device */ > +struct mdev_device { > + struct device dev; > + struct parent_device *parent; > + uuid_le uuid; > + void *driver_data; > + > + /* internal */ > + struct kref ref; > + struct list_head next; > + struct kobject *type_kobj; > +}; > + > + Empty line. > +/** > + * struct parent_ops - Structure to be registered for each parent device to > + * register the device to mdev module. > + * [...] > + * @mmap: mmap callback No need a piece of description for arguments of the mmap callback? > + * Parent device that support mediated device should be registered with mdev > + * module with parent_ops structure. > + **/ > + > +struct parent_ops { > + struct module *owner; > + const struct attribute_group **dev_attr_groups; > + const struct attribute_group **mdev_attr_groups; > + struct attribute_group **supported_type_groups; > + > + int (*create)(struct kobject *kobj, struct mdev_device *mdev); > + int (*remove)(struct mdev_device *mdev); > + int (*open)(struct mdev_device *mdev); > + void (*release)(struct mdev_device *mdev); > + ssize_t (*read)(struct mdev_device *mdev, char __user *buf, > + size_t count, loff_t *ppos); > + ssize_t (*write)(struct mdev_device *mdev, const char __user *buf, > + size_t count, loff_t *ppos); > + ssize_t (*ioctl)(struct mdev_device *mdev, unsigned int cmd, > + unsigned long arg); > + int (*mmap)(struct mdev_device *mdev, struct vm_area_struct *vma); > +}; > + [...] -- Dong Jia