Re: [Qemu-devel] [PATCH v8 1/6] vfio: Mediated device Core driver

[Tian, Kevin]

> From: Kirti Wankhede
> Sent: Tuesday, October 11, 2016 4:29 AM
> 
[...]

> +
> +/*
> + * 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(_list_lock);
> + parent = __find_parent_device(dev);
> +
> + if (!parent) {
> + mutex_unlock(_list_lock);
> + return;
> + }
> + dev_info(dev, "MDEV: Unregistering\n");
> +
> + /*
> +  * Remove parent from the list and remove "mdev_supported_types"
> +  * sysfs files so that no new mediated device could be
> +  * created for this parent
> +  */
> + list_del(>next);
> + parent_remove_sysfs_files(parent);

this could be moved out of mutex.

> +
> + mutex_unlock(_list_lock);
> +
> + device_for_each_child(dev, (void *)_remove, mdev_device_remove);
> + mdev_put_parent(parent);
> +}
> +EXPORT_SYMBOL(mdev_unregister_device);
> +

Thanks
Kevin

Re: [Qemu-devel] [PATCH v8 1/6] vfio: Mediated device Core driver

[Kirti Wankhede]

On 10/11/2016 9:21 AM, Alex Williamson wrote:
> On Tue, 11 Oct 2016 01:58:32 +0530
> Kirti Wankhede  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.
> 

Thanks Alex.

Yes, this series is against kernel v4.7. Patch 1 - 5 gets applied to
linux-next cleanly, patch 6/6 shows conflicts against linux-next.

I'm preparing next version of this patch set against linux-next.

Thanks,
Kirti.


> A few initial comments below, I'll likely have more as I wrap my head
> around it.  Thanks,
> 
> Alex
> 

Re: [Qemu-devel] [PATCH v8 1/6] vfio: Mediated device Core driver

[Alex Williamson]

On Tue, 11 Oct 2016 01:58:32 +0530
Kirti Wankhede  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 ..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 
> + *  Kirti Wankhede 
> + *
> + * 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 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

I don't see any vfio interfaces used here, is vfio.h necessary?

> +#include 
> +#include 
> +#include 
> +
> +#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(>uuid, , sizeof(uuid_le));
> + mdev->parent = parent;
> + kref_init(>ref);
> +
> + mdev->dev.parent  = dev;
> + mdev->dev.bus = _bus_type;
> + mdev->dev.release = mdev_device_release;
> + dev_set_name(>dev, "%pUl", uuid.b);
> +
> + ret = device_register(>dev);
> + if (ret) {
> + put_device(>dev);
> + goto create_err;
> + }
> +
> + ret = mdev_device_create_ops(kobj, mdev);
> + if (ret)
> + goto create_failed;
> +
> + ret = mdev_create_sysfs_files(>dev, type);
> + if (ret) {
> + mdev_device_remove_ops(mdev, true);
> + goto create_failed;
> + }
> +
> + mdev->type_kobj = kobj;
> + dev_dbg(>dev, "MDEV: created\n");
> +
> + return ret;
> +
> +create_failed:
> + device_unregister(>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;

Re: [Qemu-devel] [PATCH v8 1/6] vfio: Mediated device Core driver

[Eric Blake]

On 10/10/2016 03:28 PM, Kirti Wankhede wrote:
> Design for Mediated Device Driver:
> Main purpose of this driver is to provide a common interface for mediated
> device management that can be used by different drivers of different
> devices.
> 

> 
> Signed-off-by: Kirti Wankhede 
> Signed-off-by: Neo Jia 
> Change-Id: I73a5084574270b14541c529461ea2f03c292d510
> ---

> +++ b/drivers/vfio/mdev/Kconfig
> @@ -0,0 +1,12 @@
> +
> +config VFIO_MDEV
> +tristate "Mediated device driver framework"
> +depends on VFIO
> +default n
> +help
> +Provides a framework to virtualize device.

Feels like a missing word or two here, maybe 'virtualize a _ device' ?

> + See Documentation/vfio-mdev/vfio-mediated-device.txt for more details.
> +
> +If you don't know what do here, say N.
> +

> +++ 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 
> + *  Kirti Wankhede 
> + *
> + * 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.

Umm - "All rights reserved" is incompatible with GPLv2.  Either you
reserved the rights (and it is therefore not GPL), or it is GPL (and you
are specifically granting rights, and therefore not all rights are
reserved).

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature