Re: [Qemu-devel] [PATCH v8 2/6] vfio: VFIO based driver for Mediated devices

[Alex Williamson]

On Tue, 11 Oct 2016 01:58:33 +0530
Kirti Wankhede <address@hidden> wrote:

> vfio_mdev driver registers with mdev core driver.
> MDEV core driver creates mediated device and calls probe routine of
> vfio_mdev driver for each device.
> Probe routine of vfio_mdev driver adds mediated device to VFIO core module
> 
> This driver forms a shim layer that pass through VFIO devices operations
> to vendor driver for mediated devices.
> 
> Signed-off-by: Kirti Wankhede <address@hidden>
> Signed-off-by: Neo Jia <address@hidden>
> Change-Id: I583f4734752971d3d112324d69e2508c88f359ec
> ---
>  drivers/vfio/mdev/Kconfig           |   6 ++
>  drivers/vfio/mdev/Makefile          |   1 +
>  drivers/vfio/mdev/vfio_mdev.c       | 171 
> ++++++++++++++++++++++++++++++++++++
>  drivers/vfio/pci/vfio_pci_private.h |   6 +-
>  4 files changed, 181 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/vfio/mdev/vfio_mdev.c

Looking pretty good so far, a few preliminary comments below.  Thanks,

Alex

> 
> diff --git a/drivers/vfio/mdev/Kconfig b/drivers/vfio/mdev/Kconfig
> index 23d5b9d08a5c..e1b23697261d 100644
> --- a/drivers/vfio/mdev/Kconfig
> +++ b/drivers/vfio/mdev/Kconfig
> @@ -9,4 +9,10 @@ config VFIO_MDEV
>  
>          If you don't know what do here, say N.
>  
> +config VFIO_MDEV_DEVICE
> +    tristate "VFIO support for Mediated devices"
> +    depends on VFIO && VFIO_MDEV
> +    default n
> +    help
> +        VFIO based driver for mediated devices.
>  
> diff --git a/drivers/vfio/mdev/Makefile b/drivers/vfio/mdev/Makefile
> index 56a75e689582..e5087ed83a34 100644
> --- a/drivers/vfio/mdev/Makefile
> +++ b/drivers/vfio/mdev/Makefile
> @@ -2,4 +2,5 @@
>  mdev-y := mdev_core.o mdev_sysfs.o mdev_driver.o
>  
>  obj-$(CONFIG_VFIO_MDEV) += mdev.o
> +obj-$(CONFIG_VFIO_MDEV_DEVICE) += vfio_mdev.o
>  
> diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
> new file mode 100644
> index 000000000000..1efc3f309510
> --- /dev/null
> +++ b/drivers/vfio/mdev/vfio_mdev.c
> @@ -0,0 +1,171 @@
> +/*
> + * VFIO based driver for Mediated device
> + *
> + * 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/init.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/uuid.h>
> +#include <linux/vfio.h>
> +#include <linux/iommu.h>
> +#include <linux/mdev.h>
> +
> +#include "mdev_private.h"
> +
> +#define DRIVER_VERSION  "0.1"
> +#define DRIVER_AUTHOR   "NVIDIA Corporation"
> +#define DRIVER_DESC     "VFIO based driver for Mediated device"
> +
> +struct vfio_mdev {
> +     struct iommu_group *group;

a) this is never used, b) it's redundant, there are other ways to it.

> +     struct mdev_device *mdev;

Which leaves us with just *mdev, so do we even need a struct vfio_mdev?

> +};
> +
> +static int vfio_mdev_open(void *device_data)
> +{
> +     struct vfio_mdev *vmdev = device_data;
> +     struct parent_device *parent = vmdev->mdev->parent;
> +     int ret;
> +
> +     if (unlikely(!parent->ops->open))
> +             return -EINVAL;
> +
> +     if (!try_module_get(THIS_MODULE))
> +             return -ENODEV;
> +
> +     ret = parent->ops->open(vmdev->mdev);
> +     if (ret)
> +             module_put(THIS_MODULE);
> +
> +     return ret;
> +}
> +
> +static void vfio_mdev_release(void *device_data)
> +{
> +     struct vfio_mdev *vmdev = device_data;
> +     struct parent_device *parent = vmdev->mdev->parent;
> +
> +     if (parent->ops->release)
> +             parent->ops->release(vmdev->mdev);
> +
> +     module_put(THIS_MODULE);
> +}
> +
> +static long vfio_mdev_unlocked_ioctl(void *device_data,
> +                                  unsigned int cmd, unsigned long arg)
> +{
> +     struct vfio_mdev *vmdev = device_data;
> +     struct parent_device *parent = vmdev->mdev->parent;
> +
> +     if (unlikely(!parent->ops->ioctl))
> +             return -EINVAL;
> +
> +     return parent->ops->ioctl(vmdev->mdev, cmd, arg);
> +}
> +
> +static ssize_t vfio_mdev_read(void *device_data, char __user *buf,
> +                           size_t count, loff_t *ppos)
> +{
> +     struct vfio_mdev *vmdev = device_data;
> +     struct parent_device *parent = vmdev->mdev->parent;
> +
> +     if (unlikely(!parent->ops->read))
> +             return -EINVAL;
> +
> +     return parent->ops->read(vmdev->mdev, buf, count, *ppos);
> +}
> +
> +static ssize_t vfio_mdev_write(void *device_data, const char __user *buf,
> +                            size_t count, loff_t *ppos)
> +{
> +     struct vfio_mdev *vmdev = device_data;
> +     struct parent_device *parent = vmdev->mdev->parent;
> +
> +     if (unlikely(!parent->ops->write))
> +             return -EINVAL;
> +
> +     return parent->ops->write(vmdev->mdev, (char *)buf, count, *ppos);

We should not be losing the attributes on buf here, struct parent_ops
should be updated so the vendor driver needs to also use the correct
attributes.

> +}
> +
> +static int vfio_mdev_mmap(void *device_data, struct vm_area_struct *vma)
> +{
> +     struct vfio_mdev *vmdev = device_data;
> +     struct parent_device *parent = vmdev->mdev->parent;
> +
> +     if (unlikely(!parent->ops->mmap))
> +             return -EINVAL;
> +
> +     return parent->ops->mmap(vmdev->mdev, vma);
> +}
> +
> +static const struct vfio_device_ops vfio_mdev_dev_ops = {
> +     .name           = "vfio-mdev",
> +     .open           = vfio_mdev_open,
> +     .release        = vfio_mdev_release,
> +     .ioctl          = vfio_mdev_unlocked_ioctl,
> +     .read           = vfio_mdev_read,
> +     .write          = vfio_mdev_write,
> +     .mmap           = vfio_mdev_mmap,
> +};
> +
> +int vfio_mdev_probe(struct device *dev)
> +{
> +     struct vfio_mdev *vmdev;
> +     struct mdev_device *mdev = to_mdev_device(dev);
> +     int ret;
> +
> +     vmdev = kzalloc(sizeof(*vmdev), GFP_KERNEL);
> +     if (IS_ERR(vmdev))
> +             return PTR_ERR(vmdev);
> +
> +     vmdev->mdev = mdev;
> +     vmdev->group = mdev->group;
> +
> +     ret = vfio_add_group_dev(dev, &vfio_mdev_dev_ops, vmdev);
> +     if (ret)
> +             kfree(vmdev);
> +
> +     return ret;
> +}
> +
> +void vfio_mdev_remove(struct device *dev)
> +{
> +     struct vfio_mdev *vmdev;
> +
> +     vmdev = vfio_del_group_dev(dev);
> +     kfree(vmdev);
> +}
> +
> +struct mdev_driver vfio_mdev_driver = {
> +     .name   = "vfio_mdev",
> +     .probe  = vfio_mdev_probe,
> +     .remove = vfio_mdev_remove,
> +};
> +
> +static int __init vfio_mdev_init(void)
> +{
> +     return mdev_register_driver(&vfio_mdev_driver, THIS_MODULE);
> +}
> +
> +static void __exit vfio_mdev_exit(void)
> +{
> +     mdev_unregister_driver(&vfio_mdev_driver);
> +}
> +
> +module_init(vfio_mdev_init)
> +module_exit(vfio_mdev_exit)
> +
> +MODULE_VERSION(DRIVER_VERSION);
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR(DRIVER_AUTHOR);
> +MODULE_DESCRIPTION(DRIVER_DESC);
> diff --git a/drivers/vfio/pci/vfio_pci_private.h 
> b/drivers/vfio/pci/vfio_pci_private.h
> index 016c14a1b454..776cc2b063d4 100644
> --- a/drivers/vfio/pci/vfio_pci_private.h
> +++ b/drivers/vfio/pci/vfio_pci_private.h
> @@ -21,9 +21,9 @@
>  
>  #define VFIO_PCI_OFFSET_SHIFT   40
>  
> -#define VFIO_PCI_OFFSET_TO_INDEX(off)        (off >> VFIO_PCI_OFFSET_SHIFT)
> -#define VFIO_PCI_INDEX_TO_OFFSET(index)      ((u64)(index) << 
> VFIO_PCI_OFFSET_SHIFT)
> -#define VFIO_PCI_OFFSET_MASK (((u64)(1) << VFIO_PCI_OFFSET_SHIFT) - 1)
> +#define VFIO_PCI_OFFSET_TO_INDEX(off)   (off >> VFIO_PCI_OFFSET_SHIFT)
> +#define VFIO_PCI_INDEX_TO_OFFSET(index) ((u64)(index) << 
> VFIO_PCI_OFFSET_SHIFT)
> +#define VFIO_PCI_OFFSET_MASK    (((u64)(1) << VFIO_PCI_OFFSET_SHIFT) - 1)

Gratuitous white space changes, drop this chunk/file

>  
>  /* Special capability IDs predefined access */
>  #define PCI_CAP_ID_INVALID           0xFF    /* default raw access */