[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v8 1/6] vfio: Mediated device Core driver
|
From: |
Alex Williamson |
|
Subject: |
Re: [Qemu-devel] [PATCH v8 1/6] vfio: Mediated device Core driver |
|
Date: |
Mon, 10 Oct 2016 21:51:49 -0600 |
On Tue, 11 Oct 2016 01:58:32 +0530
Kirti Wankhede <address@hidden> wrote:
> ---
> drivers/vfio/Kconfig | 1 +
> drivers/vfio/Makefile | 1 +
> drivers/vfio/mdev/Kconfig | 12 ++
> drivers/vfio/mdev/Makefile | 5 +
> drivers/vfio/mdev/mdev_core.c | 363
> +++++++++++++++++++++++++++++++++++++++
> drivers/vfio/mdev/mdev_driver.c | 131 ++++++++++++++
> drivers/vfio/mdev/mdev_private.h | 41 +++++
> drivers/vfio/mdev/mdev_sysfs.c | 295 +++++++++++++++++++++++++++++++
> include/linux/mdev.h | 178 +++++++++++++++++++
> 9 files changed, 1027 insertions(+)
> create mode 100644 drivers/vfio/mdev/Kconfig
> create mode 100644 drivers/vfio/mdev/Makefile
> create mode 100644 drivers/vfio/mdev/mdev_core.c
> create mode 100644 drivers/vfio/mdev/mdev_driver.c
> create mode 100644 drivers/vfio/mdev/mdev_private.h
> create mode 100644 drivers/vfio/mdev/mdev_sysfs.c
> create mode 100644 include/linux/mdev.h
Overall this is heading in a good direction. What kernel is this
series against? I could only apply it to v4.7, yet some of the
dependencies claimed in the cover letter are only in v4.8. linux-next
or v4.8 are both good baselines right now, as we move to v4.9-rc
releases, linux-next probably becomes a better target.
A few initial comments below, I'll likely have more as I wrap my head
around it. Thanks,
Alex
> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> new file mode 100644
> index 000000000000..019c196e62d5
> --- /dev/null
> +++ b/drivers/vfio/mdev/mdev_core.c
> @@ -0,0 +1,363 @@
> +/*
> + * 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/fs.h>
> +#include <linux/slab.h>
> +#include <linux/sched.h>
> +#include <linux/uuid.h>
> +#include <linux/vfio.h>
I don't see any vfio interfaces used here, is vfio.h necessary?
> +#include <linux/iommu.h>
> +#include <linux/sysfs.h>
> +#include <linux/mdev.h>
> +
> +#include "mdev_private.h"
> +
[snip]
> +int mdev_device_create(struct kobject *kobj, struct device *dev, uuid_le
> uuid)
> +{
> + int ret;
> + struct mdev_device *mdev;
> + struct parent_device *parent;
> + struct mdev_type *type = to_mdev_type(kobj);
> +
> + parent = mdev_get_parent(type->parent);
> + if (!parent)
> + return -EINVAL;
> +
> + /* Check for duplicate */
> + mdev = __find_mdev_device(parent, uuid);
> + if (mdev) {
> + ret = -EEXIST;
> + goto create_err;
> + }
> +
> + mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
> + if (!mdev) {
> + ret = -ENOMEM;
> + goto create_err;
> + }
> +
> + memcpy(&mdev->uuid, &uuid, sizeof(uuid_le));
> + mdev->parent = parent;
> + kref_init(&mdev->ref);
> +
> + mdev->dev.parent = dev;
> + mdev->dev.bus = &mdev_bus_type;
> + mdev->dev.release = mdev_device_release;
> + dev_set_name(&mdev->dev, "%pUl", uuid.b);
> +
> + ret = device_register(&mdev->dev);
> + if (ret) {
> + put_device(&mdev->dev);
> + goto create_err;
> + }
> +
> + ret = mdev_device_create_ops(kobj, mdev);
> + if (ret)
> + goto create_failed;
> +
> + ret = mdev_create_sysfs_files(&mdev->dev, type);
> + if (ret) {
> + mdev_device_remove_ops(mdev, true);
> + goto create_failed;
> + }
> +
> + mdev->type_kobj = kobj;
> + dev_dbg(&mdev->dev, "MDEV: created\n");
> +
> + return ret;
> +
> +create_failed:
> + device_unregister(&mdev->dev);
> +
> +create_err:
> + mdev_put_parent(parent);
> + return ret;
> +}
> +
> +int mdev_device_remove(struct device *dev, void *data)
I understand this void* is to be able to call this from
device_for_each_child(), but let's create a callback wrapper for that
path that converts data to bool, we really don't want to use void args
except where necessary. IOW,
static int mdev_device_remove_cb(struct device *dev, void *data)
{
mdev_device_remove(dev, data ? *(bool *)data : true);
}
int mdev_device_remove(struct device *dev, bool force_remove)
> +{
> + struct mdev_device *mdev;
> + struct parent_device *parent;
> + struct mdev_type *type;
> + bool force_remove = true;
> + int ret = 0;
> +
> + if (!dev_is_mdev(dev))
> + return 0;
> +
> + mdev = to_mdev_device(dev);
> + parent = mdev->parent;
> + type = to_mdev_type(mdev->type_kobj);
> +
> + if (data)
> + force_remove = *(bool *)data;
> +
> + ret = mdev_device_remove_ops(mdev, force_remove);
> + if (ret)
> + return ret;
> +
> + mdev_remove_sysfs_files(dev, type);
> + device_unregister(dev);
> + mdev_put_parent(parent);
> + return ret;
> +}
> +
[snip]
> diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
> new file mode 100644
> index 000000000000..228698f46234
> --- /dev/null
> +++ b/drivers/vfio/mdev/mdev_sysfs.c
[snip]
> +
> +static ssize_t create_store(struct kobject *kobj, struct device *dev,
> + const char *buf, size_t count)
> +{
> + char *str;
> + uuid_le uuid;
> + int ret;
> +
> + str = kstrndup(buf, count, GFP_KERNEL);
We can sanity test @count to buffer formats that uuid_le_to_bin() is
able to parse to make this safer.
> + if (!str)
> + return -ENOMEM;
> +
> + ret = uuid_le_to_bin(str, &uuid);
> + if (!ret) {
> +
> + ret = mdev_device_create(kobj, dev, uuid);
> + if (ret)
> + pr_err("mdev_create: Failed to create mdev device\n");
> + else
> + ret = count;
> + }
> +
> + kfree(str);
> + return ret;
> +}
> +
[snip]
> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> new file mode 100644
> index 000000000000..93c177609efe
> --- /dev/null
> +++ b/include/linux/mdev.h
> @@ -0,0 +1,178 @@
> +/*
> + * Mediated device definition
> + *
> + * 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.
> + */
> +
> +#ifndef MDEV_H
> +#define MDEV_H
> +
> +#include <uapi/linux/vfio.h>
> +
> +struct parent_device;
> +
> +/* Mediated device */
> +struct mdev_device {
> + struct device dev;
> + struct parent_device *parent;
> + struct iommu_group *group;
There's already an iommu_group pointer on struct device, isn't this a
duplicate?
> + uuid_le uuid;
> + void *driver_data;
> +
> + /* internal only */
> + struct kref ref;
> + struct list_head next;
> + struct kobject *type_kobj;
> +};
> +
> +
> +/**
> + * struct parent_ops - Structure to be registered for each parent device to
> + * register the device to mdev module.
> + *
> + * @owner: The module owner.
> + * @dev_attr_groups: Attributes of the parent device.
> + * @mdev_attr_groups: Attributes of the mediated device.
> + * @supported_type_groups: Attributes to define supported types. It is
> mandatory
> + * to provide supported types.
> + * @create: Called to allocate basic resources in parent device's
> + * driver for a particular mediated device. It is
> + * mandatory to provide create ops.
> + * @kobj: kobject of type for which 'create' is called.
> + * @mdev: mdev_device structure on of mediated device
> + * that is being created
> + * Returns integer: success (0) or error (< 0)
> + * @remove: Called to free resources in parent device's driver for a
> + * a mediated device. It is mandatory to provide 'remove'
> + * ops.
> + * @mdev: mdev_device device structure which is being
> + * destroyed
> + * Returns integer: success (0) or error (< 0)
> + * @open: Open mediated device.
> + * @mdev: mediated device.
> + * Returns integer: success (0) or error (< 0)
> + * @release: release mediated device
> + * @mdev: mediated device.
> + * @read: Read emulation callback
> + * @mdev: mediated device structure
> + * @buf: read buffer
> + * @count: number of bytes to read
> + * @pos: address.
> + * Retuns number on bytes read on success or error.
> + * @write: Write emulation callback
> + * @mdev: mediated device structure
> + * @buf: write buffer
> + * @count: number of bytes to be written
> + * @pos: address.
> + * Retuns number on bytes written on success or error.
> + * @ioctl: IOCTL callback
> + * @mdev: mediated device structure
> + * @cmd: mediated device structure
> + * @arg: mediated device structure
> + * @mmap: 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 *buf, size_t count,
> + loff_t pos);
char __user *buf
> + ssize_t (*write)(struct mdev_device *mdev, char *buf, size_t count,
> + loff_t pos);
const char __user *buf
> + ssize_t (*ioctl)(struct mdev_device *mdev, unsigned int cmd,
> + unsigned long arg);
> + int (*mmap)(struct mdev_device *mdev, struct vm_area_struct *vma);
> +};
> +
> +/* Parent Device */
> +struct parent_device {
> + struct device *dev;
> + const struct parent_ops *ops;
> +
> + /* internal */
> + struct kref ref;
> + struct list_head next;
> + struct kset *mdev_types_kset;
> + struct list_head type_list;
> +};
> +
> +/* interface for exporting mdev supported type attributes */
> +struct mdev_type_attribute {
> + struct attribute attr;
> + ssize_t (*show)(struct kobject *kobj, struct device *dev, char *buf);
> + ssize_t (*store)(struct kobject *kobj, struct device *dev,
> + const char *buf, size_t count);
> +};
> +
> +#define MDEV_TYPE_ATTR(_name, _mode, _show, _store) \
> +struct mdev_type_attribute mdev_type_attr_##_name = \
> + __ATTR(_name, _mode, _show, _store)
> +#define MDEV_TYPE_ATTR_RW(_name) \
> + struct mdev_type_attribute mdev_type_attr_##_name = __ATTR_RW(_name)
> +#define MDEV_TYPE_ATTR_RO(_name) \
> + struct mdev_type_attribute mdev_type_attr_##_name = __ATTR_RO(_name)
> +#define MDEV_TYPE_ATTR_WO(_name) \
> + struct mdev_type_attribute mdev_type_attr_##_name = __ATTR_WO(_name)
> +
> +/**
> + * struct mdev_driver - Mediated device driver
> + * @name: driver name
> + * @probe: called when new device created
> + * @remove: called when device removed
> + * @driver: device driver structure
> + *
> + **/
> +struct mdev_driver {
> + const char *name;
> + int (*probe)(struct device *dev);
> + void (*remove)(struct device *dev);
> + struct device_driver driver;
> +};
> +
> +static inline struct mdev_driver *to_mdev_driver(struct device_driver *drv)
> +{
> + return drv ? container_of(drv, struct mdev_driver, driver) : NULL;
> +}
> +
> +static inline struct mdev_device *to_mdev_device(struct device *dev)
> +{
> + return dev ? container_of(dev, struct mdev_device, dev) : NULL;
> +}
> +
> +static inline void *mdev_get_drvdata(struct mdev_device *mdev)
> +{
> + return mdev->driver_data;
> +}
> +
> +static inline void mdev_set_drvdata(struct mdev_device *mdev, void *data)
> +{
> + mdev->driver_data = data;
> +}
> +
> +extern struct bus_type mdev_bus_type;
> +
> +#define dev_is_mdev(d) ((d)->bus == &mdev_bus_type)
> +
> +extern int mdev_register_device(struct device *dev,
> + const struct parent_ops *ops);
> +extern void mdev_unregister_device(struct device *dev);
> +
> +extern int mdev_register_driver(struct mdev_driver *drv, struct module
> *owner);
> +extern void mdev_unregister_driver(struct mdev_driver *drv);
> +
> +#endif /* MDEV_H */