Re: [PATCH v12 01/22] vfio: Mediated device Core driver

On 11/15/2016 2:00 PM, Dong Jia Shi wrote:

* Kirti Wankhede [off-list ref] [2016-11-14 21:12:15 +0530]:

Hi Kirti,

[...]

quoted

diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
new file mode 100644
index 000000000000..613e8a8a3b2a
--- /dev/null
+++ b/drivers/vfio/mdev/mdev_core.c

@@ -0,0 +1,374 @@
+/*
+ * Mediated device Core Driver
+ *
+ * Copyright (c) 2016, NVIDIA CORPORATION. All rights reserved.
+ *     Author: Neo Jia <cjia@nvidia.com>
+ *             Kirti Wankhede <kwankhede@nvidia.com>
+ *
+ * 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;
+
+static int _find_mdev_device(struct device *dev, void *data)

What the underscore prefix implies to me is that this should not be
called directly. While ...

This only called here, i.e. it is not called directly:

         dev = device_find_child(parent->dev, &uuid, _find_mdev_device);

quoted

+{
+	struct mdev_device *mdev;
+
+	if (!dev_is_mdev(dev))
+		return 0;
+
+	mdev = to_mdev_device(dev);
+
+	if (uuid_le_cmp(mdev->uuid, *(uuid_le *)data) == 0)
+		return 1;
+
+	return 0;
+}

[...]

quoted

+
+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);

*(bool *)data will always be true, correct?
If so, we chould get rid of it.

No, data can be true or false based in when it is called. This is passed
to mdev_device_remove_ops() where I had added comment.

/*
 * 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)

quoted

+}
+

[...]

quoted

diff --git a/drivers/vfio/mdev/mdev_driver.c b/drivers/vfio/mdev/mdev_driver.c
new file mode 100644
index 000000000000..6c19a2f6b5a2
--- /dev/null
+++ b/drivers/vfio/mdev/mdev_driver.c

@@ -0,0 +1,120 @@
+/*
+ * MDEV driver
+ *
+ * Copyright (c) 2016, NVIDIA CORPORATION. All rights reserved.
+ *     Author: Neo Jia <cjia@nvidia.com>
+ *             Kirti Wankhede <kwankhede@nvidia.com>
+ *
+ * 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));
+attach_fail:
+	iommu_group_put(group);
+	return ret;
+}

No need for a goto here. How about:
	ret = iommu_group_add_device(group, &mdev->dev);
	if (!ret)
		dev_info(&mdev->dev, "MDEV: group_id = %d\n",
			 iommu_group_id(group));
	iommu_group_put(group);
	return ret;

Ok.

Or just remove the dev_info stuff?

[...]

All findings from me are nitpickings. If you like you can have my r-b:
Reviewed-by: Dong Jia Shi <redacted>

Thanks,
Kirti