* [PATCH v2 00/14] Embed struct vfio_device in all sub-structures
@ 2021-03-13 0:55 Jason Gunthorpe
2021-03-13 0:55 ` [PATCH v2 03/14] vfio: Split creation of a vfio_device into init and register ops Jason Gunthorpe
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Jason Gunthorpe @ 2021-03-13 0:55 UTC (permalink / raw)
To: Alex Williamson, Cornelia Huck, Jonathan Corbet, Diana Craciun,
Eric Auger, kvm, Kirti Wankhede, linux-doc
Cc: Raj, Ashok, Bharat Bhushan, Christian Ehrhardt, Dan Williams,
Daniel Vetter, Christoph Hellwig, Kevin Tian, Leon Romanovsky,
Max Gurtovoy, Tarun Gupta, Liu Yi L
The main focus of this series is to make VFIO follow the normal kernel
convention of structure embedding for structure inheritance instead of
linking using a 'void *opaque'. Here we focus on moving the vfio_device to
be a member of every struct vfio_XX_device that is linked by a
vfio_add_group_dev().
In turn this allows 'struct vfio_device *' to be used everwhere, and the
public API out of vfio.c can be cleaned to remove places using 'struct
device *' and 'void *' as surrogates to refer to the device.
While this has the minor trade off of moving 'struct vfio_device' the
clarity of the design is worth it. I can speak directly to this idea, as
I've invested a fair amount of time carefully working backwards what all
the type-erased APIs are supposed to be and it is certainly not trivial or
intuitive.
When we get into mdev land things become even more inscrutable, and while
I now have a pretty clear picture, it was hard to obtain. I think this
agrees with the kernel style ideal of being explicit in typing and not
sacrificing clarity to create opaque structs.
After this series the general rules are:
- Any vfio_XX_device * can be obtained at no cost from a vfio_device *
using container_of(), and the reverse is possible by &XXdev->vdev
This is similar to how 'struct pci_device' and 'struct device' are
interrelated.
This allows 'device_data' to be completely removed from the vfio.c API.
- The drvdata for a struct device points at the vfio_XX_device that
belongs to the driver that was probed. drvdata is removed from the core
code, and only used as part of the implementation of the struct
device_driver.
- The lifetime of vfio_XX_device and vfio_device are identical, they are
the same memory.
This follows the existing model where vfio_del_group_dev() blocks until
all vfio_device_put()'s are completed. This in turn means the struct
device_driver remove() blocks, and thus under the driver_lock() a bound
driver must have a valid drvdata pointing at both vfio device
structs. A following series exploits this further.
Most vfio_XX_device structs have data that duplicates the 'struct
device *dev' member of vfio_device, a following series removes that
duplication too.
v2:
- Split the get/put changes out of "Simlpify the lifetime logic for
vfio_device"
- Add a patch to fix probe ordering in fsl-mc and remove FIXME
- Add a patch to re-org pci probe
- Add a patch to fix probe odering in pci and remove FIXME
- Remove the **pf_dev output from get_pf_vdev()
v1: https://lore.kernel.org/r/0-v1-7355d38b9344+17481-vfio1_jgg@nvidia.com
Thanks,
Jason
Jason Gunthorpe (14):
vfio: Remove extra put/gets around vfio_device->group
vfio: Simplify the lifetime logic for vfio_device
vfio: Split creation of a vfio_device into init and register ops
vfio/platform: Use vfio_init/register/unregister_group_dev
vfio/fsl-mc: Re-order vfio_fsl_mc_probe()
vfio/fsl-mc: Use vfio_init/register/unregister_group_dev
vfio/pci: Move VGA and VF initialization to functions
vfio/pci: Re-order vfio_pci_probe()
vfio/pci: Use vfio_init/register/unregister_group_dev
vfio/mdev: Use vfio_init/register/unregister_group_dev
vfio/mdev: Make to_mdev_device() into a static inline
vfio: Make vfio_device_ops pass a 'struct vfio_device *' instead of
'void *'
vfio/pci: Replace uses of vfio_device_data() with container_of
vfio: Remove device_data from the vfio bus driver API
Documentation/driver-api/vfio.rst | 48 ++--
drivers/vfio/fsl-mc/vfio_fsl_mc.c | 96 ++++---
drivers/vfio/fsl-mc/vfio_fsl_mc_private.h | 1 +
drivers/vfio/mdev/mdev_private.h | 5 +-
drivers/vfio/mdev/vfio_mdev.c | 57 ++--
drivers/vfio/pci/vfio_pci.c | 253 ++++++++++--------
drivers/vfio/pci/vfio_pci_private.h | 1 +
drivers/vfio/platform/vfio_amba.c | 8 +-
drivers/vfio/platform/vfio_platform.c | 21 +-
drivers/vfio/platform/vfio_platform_common.c | 56 ++--
drivers/vfio/platform/vfio_platform_private.h | 5 +-
drivers/vfio/vfio.c | 210 +++++----------
include/linux/vfio.h | 37 ++-
13 files changed, 397 insertions(+), 401 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 03/14] vfio: Split creation of a vfio_device into init and register ops
2021-03-13 0:55 [PATCH v2 00/14] Embed struct vfio_device in all sub-structures Jason Gunthorpe
@ 2021-03-13 0:55 ` Jason Gunthorpe
2021-03-16 7:55 ` Tian, Kevin
` (3 more replies)
2021-03-13 0:56 ` [PATCH v2 12/14] vfio: Make vfio_device_ops pass a 'struct vfio_device *' instead of 'void *' Jason Gunthorpe
2021-03-13 0:56 ` [PATCH v2 14/14] vfio: Remove device_data from the vfio bus driver API Jason Gunthorpe
2 siblings, 4 replies; 17+ messages in thread
From: Jason Gunthorpe @ 2021-03-13 0:55 UTC (permalink / raw)
To: Alex Williamson, Cornelia Huck, Jonathan Corbet, kvm, linux-doc
Cc: Raj, Ashok, Dan Williams, Daniel Vetter, Christoph Hellwig,
Leon Romanovsky, Max Gurtovoy, Tarun Gupta, Liu Yi L
This makes the struct vfio_pci_device part of the public interface so it
can be used with container_of and so forth, as is typical for a Linux
subystem.
This is the first step to bring some type-safety to the vfio interface by
allowing the replacement of 'void *' and 'struct device *' inputs with a
simple and clear 'struct vfio_pci_device *'
For now the self-allocating vfio_add_group_dev() interface is kept so each
user can be updated as a separate patch.
The expected usage pattern is
driver core probe() function:
my_device = kzalloc(sizeof(*mydevice));
vfio_init_group_dev(&my_device->vdev, dev, ops, mydevice);
/* other driver specific prep */
vfio_register_group_dev(&my_device->vdev);
dev_set_drvdata(my_device);
driver core remove() function:
my_device = dev_get_drvdata(dev);
vfio_unregister_group_dev(&my_device->vdev);
/* other driver specific tear down */
kfree(my_device);
Allowing the driver to be able to use the drvdata and vifo_device to go
to/from its own data.
The pattern also makes it clear that vfio_register_group_dev() must be
last in the sequence, as once it is called the core code can immediately
start calling ops. The init/register gap is provided to allow for the
driver to do setup before ops can be called and thus avoid races.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Liu Yi L <yi.l.liu@intel.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
Documentation/driver-api/vfio.rst | 31 ++++----
drivers/vfio/vfio.c | 123 ++++++++++++++++--------------
include/linux/vfio.h | 16 ++++
3 files changed, 98 insertions(+), 72 deletions(-)
diff --git a/Documentation/driver-api/vfio.rst b/Documentation/driver-api/vfio.rst
index f1a4d3c3ba0bb1..d3a02300913a7f 100644
--- a/Documentation/driver-api/vfio.rst
+++ b/Documentation/driver-api/vfio.rst
@@ -249,18 +249,23 @@ VFIO bus driver API
VFIO bus drivers, such as vfio-pci make use of only a few interfaces
into VFIO core. When devices are bound and unbound to the driver,
-the driver should call vfio_add_group_dev() and vfio_del_group_dev()
-respectively::
-
- extern int vfio_add_group_dev(struct device *dev,
- const struct vfio_device_ops *ops,
- void *device_data);
-
- extern void *vfio_del_group_dev(struct device *dev);
-
-vfio_add_group_dev() indicates to the core to begin tracking the
-iommu_group of the specified dev and register the dev as owned by
-a VFIO bus driver. The driver provides an ops structure for callbacks
+the driver should call vfio_register_group_dev() and
+vfio_unregister_group_dev() respectively::
+
+ void vfio_init_group_dev(struct vfio_device *device,
+ struct device *dev,
+ const struct vfio_device_ops *ops,
+ void *device_data);
+ int vfio_register_group_dev(struct vfio_device *device);
+ void vfio_unregister_group_dev(struct vfio_device *device);
+
+The driver should embed the vfio_device in its own structure and call
+vfio_init_group_dev() to pre-configure it before going to registration.
+vfio_register_group_dev() indicates to the core to begin tracking the
+iommu_group of the specified dev and register the dev as owned by a VFIO bus
+driver. Once vfio_register_group_dev() returns it is possible for userspace to
+start accessing the driver, thus the driver should ensure it is completely
+ready before calling it. The driver provides an ops structure for callbacks
similar to a file operations structure::
struct vfio_device_ops {
@@ -276,7 +281,7 @@ similar to a file operations structure::
};
Each function is passed the device_data that was originally registered
-in the vfio_add_group_dev() call above. This allows the bus driver
+in the vfio_register_group_dev() call above. This allows the bus driver
an easy place to store its opaque, private data. The open/release
callbacks are issued when a new file descriptor is created for a
device (via VFIO_GROUP_GET_DEVICE_FD). The ioctl interface provides
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 32660e8a69ae20..cfa06ae3b9018b 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -89,16 +89,6 @@ struct vfio_group {
struct blocking_notifier_head notifier;
};
-struct vfio_device {
- refcount_t refcount;
- struct completion comp;
- struct device *dev;
- const struct vfio_device_ops *ops;
- struct vfio_group *group;
- struct list_head group_next;
- void *device_data;
-};
-
#ifdef CONFIG_VFIO_NOIOMMU
static bool noiommu __read_mostly;
module_param_named(enable_unsafe_noiommu_mode,
@@ -532,35 +522,6 @@ static struct vfio_group *vfio_group_get_from_dev(struct device *dev)
/**
* Device objects - create, release, get, put, search
*/
-static
-struct vfio_device *vfio_group_create_device(struct vfio_group *group,
- struct device *dev,
- const struct vfio_device_ops *ops,
- void *device_data)
-{
- struct vfio_device *device;
-
- device = kzalloc(sizeof(*device), GFP_KERNEL);
- if (!device)
- return ERR_PTR(-ENOMEM);
-
- refcount_set(&device->refcount, 1);
- init_completion(&device->comp);
- device->dev = dev;
- /* Our reference on group is moved to the device */
- device->group = group;
- device->ops = ops;
- device->device_data = device_data;
- dev_set_drvdata(dev, device);
-
- mutex_lock(&group->device_lock);
- list_add(&device->group_next, &group->device_list);
- group->dev_counter++;
- mutex_unlock(&group->device_lock);
-
- return device;
-}
-
/* Device reference always implies a group reference */
void vfio_device_put(struct vfio_device *device)
{
@@ -779,14 +740,23 @@ static int vfio_iommu_group_notifier(struct notifier_block *nb,
/**
* VFIO driver API
*/
-int vfio_add_group_dev(struct device *dev,
- const struct vfio_device_ops *ops, void *device_data)
+void vfio_init_group_dev(struct vfio_device *device, struct device *dev,
+ const struct vfio_device_ops *ops, void *device_data)
+{
+ init_completion(&device->comp);
+ device->dev = dev;
+ device->ops = ops;
+ device->device_data = device_data;
+}
+EXPORT_SYMBOL_GPL(vfio_init_group_dev);
+
+int vfio_register_group_dev(struct vfio_device *device)
{
+ struct vfio_device *existing_device;
struct iommu_group *iommu_group;
struct vfio_group *group;
- struct vfio_device *device;
- iommu_group = iommu_group_get(dev);
+ iommu_group = iommu_group_get(device->dev);
if (!iommu_group)
return -EINVAL;
@@ -805,21 +775,50 @@ int vfio_add_group_dev(struct device *dev,
iommu_group_put(iommu_group);
}
- device = vfio_group_get_device(group, dev);
- if (device) {
- dev_WARN(dev, "Device already exists on group %d\n",
+ existing_device = vfio_group_get_device(group, device->dev);
+ if (existing_device) {
+ dev_WARN(device->dev, "Device already exists on group %d\n",
iommu_group_id(iommu_group));
- vfio_device_put(device);
+ vfio_device_put(existing_device);
vfio_group_put(group);
return -EBUSY;
}
- device = vfio_group_create_device(group, dev, ops, device_data);
- if (IS_ERR(device)) {
- vfio_group_put(group);
- return PTR_ERR(device);
- }
+ /* Our reference on group is moved to the device */
+ device->group = group;
+
+ /* Refcounting can't start until the driver calls register */
+ refcount_set(&device->refcount, 1);
+
+ mutex_lock(&group->device_lock);
+ list_add(&device->group_next, &group->device_list);
+ group->dev_counter++;
+ mutex_unlock(&group->device_lock);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(vfio_register_group_dev);
+
+int vfio_add_group_dev(struct device *dev, const struct vfio_device_ops *ops,
+ void *device_data)
+{
+ struct vfio_device *device;
+ int ret;
+
+ device = kzalloc(sizeof(*device), GFP_KERNEL);
+ if (!device)
+ return -ENOMEM;
+
+ vfio_init_group_dev(device, dev, ops, device_data);
+ ret = vfio_register_group_dev(device);
+ if (ret)
+ goto err_kfree;
+ dev_set_drvdata(dev, device);
return 0;
+
+err_kfree:
+ kfree(device);
+ return ret;
}
EXPORT_SYMBOL_GPL(vfio_add_group_dev);
@@ -887,11 +886,9 @@ EXPORT_SYMBOL_GPL(vfio_device_data);
/*
* Decrement the device reference count and wait for the device to be
* removed. Open file descriptors for the device... */
-void *vfio_del_group_dev(struct device *dev)
+void vfio_unregister_group_dev(struct vfio_device *device)
{
- struct vfio_device *device = dev_get_drvdata(dev);
struct vfio_group *group = device->group;
- void *device_data = device->device_data;
struct vfio_unbound_dev *unbound;
unsigned int i = 0;
bool interrupted = false;
@@ -908,7 +905,7 @@ void *vfio_del_group_dev(struct device *dev)
*/
unbound = kzalloc(sizeof(*unbound), GFP_KERNEL);
if (unbound) {
- unbound->dev = dev;
+ unbound->dev = device->dev;
mutex_lock(&group->unbound_lock);
list_add(&unbound->unbound_next, &group->unbound_list);
mutex_unlock(&group->unbound_lock);
@@ -919,7 +916,7 @@ void *vfio_del_group_dev(struct device *dev)
rc = try_wait_for_completion(&device->comp);
while (rc <= 0) {
if (device->ops->request)
- device->ops->request(device_data, i++);
+ device->ops->request(device->device_data, i++);
if (interrupted) {
rc = wait_for_completion_timeout(&device->comp,
@@ -929,7 +926,7 @@ void *vfio_del_group_dev(struct device *dev)
&device->comp, HZ * 10);
if (rc < 0) {
interrupted = true;
- dev_warn(dev,
+ dev_warn(device->dev,
"Device is currently in use, task"
" \"%s\" (%d) "
"blocked until device is released",
@@ -962,9 +959,17 @@ void *vfio_del_group_dev(struct device *dev)
/* Matches the get in vfio_group_create_device() */
vfio_group_put(group);
+}
+EXPORT_SYMBOL_GPL(vfio_unregister_group_dev);
+
+void *vfio_del_group_dev(struct device *dev)
+{
+ struct vfio_device *device = dev_get_drvdata(dev);
+ void *device_data = device->device_data;
+
+ vfio_unregister_group_dev(device);
dev_set_drvdata(dev, NULL);
kfree(device);
-
return device_data;
}
EXPORT_SYMBOL_GPL(vfio_del_group_dev);
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index b7e18bde5aa8b3..ad8b579d67d34a 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -15,6 +15,18 @@
#include <linux/poll.h>
#include <uapi/linux/vfio.h>
+struct vfio_device {
+ struct device *dev;
+ const struct vfio_device_ops *ops;
+ struct vfio_group *group;
+
+ /* Members below here are private, not for driver use */
+ refcount_t refcount;
+ struct completion comp;
+ struct list_head group_next;
+ void *device_data;
+};
+
/**
* struct vfio_device_ops - VFIO bus driver device callbacks
*
@@ -48,11 +60,15 @@ struct vfio_device_ops {
extern struct iommu_group *vfio_iommu_group_get(struct device *dev);
extern void vfio_iommu_group_put(struct iommu_group *group, struct device *dev);
+void vfio_init_group_dev(struct vfio_device *device, struct device *dev,
+ const struct vfio_device_ops *ops, void *device_data);
+int vfio_register_group_dev(struct vfio_device *device);
extern int vfio_add_group_dev(struct device *dev,
const struct vfio_device_ops *ops,
void *device_data);
extern void *vfio_del_group_dev(struct device *dev);
+void vfio_unregister_group_dev(struct vfio_device *device);
extern struct vfio_device *vfio_device_get_from_dev(struct device *dev);
extern void vfio_device_put(struct vfio_device *device);
extern void *vfio_device_data(struct vfio_device *device);
--
2.30.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 12/14] vfio: Make vfio_device_ops pass a 'struct vfio_device *' instead of 'void *'
2021-03-13 0:55 [PATCH v2 00/14] Embed struct vfio_device in all sub-structures Jason Gunthorpe
2021-03-13 0:55 ` [PATCH v2 03/14] vfio: Split creation of a vfio_device into init and register ops Jason Gunthorpe
@ 2021-03-13 0:56 ` Jason Gunthorpe
2021-03-15 8:58 ` Christoph Hellwig
2021-03-17 11:33 ` Cornelia Huck
2021-03-13 0:56 ` [PATCH v2 14/14] vfio: Remove device_data from the vfio bus driver API Jason Gunthorpe
2 siblings, 2 replies; 17+ messages in thread
From: Jason Gunthorpe @ 2021-03-13 0:56 UTC (permalink / raw)
To: Alex Williamson, Cornelia Huck, Jonathan Corbet, Diana Craciun,
Eric Auger, kvm, Kirti Wankhede, linux-doc
Cc: Raj, Ashok, Dan Williams, Daniel Vetter, Christoph Hellwig,
Leon Romanovsky, Max Gurtovoy, Tarun Gupta
This is the standard kernel pattern, the ops associated with a struct get
the struct pointer in for typesafety. The expected design is to use
container_of to cleanly go from the subsystem level type to the driver
level type without having any type erasure in a void *.
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
Documentation/driver-api/vfio.rst | 18 ++++----
drivers/vfio/fsl-mc/vfio_fsl_mc.c | 36 +++++++++------
drivers/vfio/mdev/vfio_mdev.c | 33 +++++++-------
drivers/vfio/pci/vfio_pci.c | 47 ++++++++++++--------
drivers/vfio/platform/vfio_platform_common.c | 33 ++++++++------
drivers/vfio/vfio.c | 20 ++++-----
include/linux/vfio.h | 16 +++----
7 files changed, 117 insertions(+), 86 deletions(-)
diff --git a/Documentation/driver-api/vfio.rst b/Documentation/driver-api/vfio.rst
index d3a02300913a7f..3337f337293a32 100644
--- a/Documentation/driver-api/vfio.rst
+++ b/Documentation/driver-api/vfio.rst
@@ -269,20 +269,22 @@ ready before calling it. The driver provides an ops structure for callbacks
similar to a file operations structure::
struct vfio_device_ops {
- int (*open)(void *device_data);
- void (*release)(void *device_data);
- ssize_t (*read)(void *device_data, char __user *buf,
+ int (*open)(struct vfio_device *vdev);
+ void (*release)(struct vfio_device *vdev);
+ ssize_t (*read)(struct vfio_device *vdev, char __user *buf,
size_t count, loff_t *ppos);
- ssize_t (*write)(void *device_data, const char __user *buf,
+ ssize_t (*write)(struct vfio_device *vdev,
+ const char __user *buf,
size_t size, loff_t *ppos);
- long (*ioctl)(void *device_data, unsigned int cmd,
+ long (*ioctl)(struct vfio_device *vdev, unsigned int cmd,
unsigned long arg);
- int (*mmap)(void *device_data, struct vm_area_struct *vma);
+ int (*mmap)(struct vfio_device *vdev,
+ struct vm_area_struct *vma);
};
-Each function is passed the device_data that was originally registered
+Each function is passed the vdev that was originally registered
in the vfio_register_group_dev() call above. This allows the bus driver
-an easy place to store its opaque, private data. The open/release
+to obtain its private data using container_of(). The open/release
callbacks are issued when a new file descriptor is created for a
device (via VFIO_GROUP_GET_DEVICE_FD). The ioctl interface provides
a direct pass through for VFIO_DEVICE_* ioctls. The read/write/mmap
diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
index 87ea8368aa510a..023b2222806424 100644
--- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
+++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
@@ -135,9 +135,10 @@ static void vfio_fsl_mc_regions_cleanup(struct vfio_fsl_mc_device *vdev)
kfree(vdev->regions);
}
-static int vfio_fsl_mc_open(void *device_data)
+static int vfio_fsl_mc_open(struct vfio_device *core_vdev)
{
- struct vfio_fsl_mc_device *vdev = device_data;
+ struct vfio_fsl_mc_device *vdev =
+ container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
int ret;
if (!try_module_get(THIS_MODULE))
@@ -161,9 +162,10 @@ static int vfio_fsl_mc_open(void *device_data)
return ret;
}
-static void vfio_fsl_mc_release(void *device_data)
+static void vfio_fsl_mc_release(struct vfio_device *core_vdev)
{
- struct vfio_fsl_mc_device *vdev = device_data;
+ struct vfio_fsl_mc_device *vdev =
+ container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
int ret;
mutex_lock(&vdev->reflck->lock);
@@ -197,11 +199,12 @@ static void vfio_fsl_mc_release(void *device_data)
module_put(THIS_MODULE);
}
-static long vfio_fsl_mc_ioctl(void *device_data, unsigned int cmd,
- unsigned long arg)
+static long vfio_fsl_mc_ioctl(struct vfio_device *core_vdev,
+ unsigned int cmd, unsigned long arg)
{
unsigned long minsz;
- struct vfio_fsl_mc_device *vdev = device_data;
+ struct vfio_fsl_mc_device *vdev =
+ container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
struct fsl_mc_device *mc_dev = vdev->mc_dev;
switch (cmd) {
@@ -327,10 +330,11 @@ static long vfio_fsl_mc_ioctl(void *device_data, unsigned int cmd,
}
}
-static ssize_t vfio_fsl_mc_read(void *device_data, char __user *buf,
+static ssize_t vfio_fsl_mc_read(struct vfio_device *core_vdev, char __user *buf,
size_t count, loff_t *ppos)
{
- struct vfio_fsl_mc_device *vdev = device_data;
+ struct vfio_fsl_mc_device *vdev =
+ container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
unsigned int index = VFIO_FSL_MC_OFFSET_TO_INDEX(*ppos);
loff_t off = *ppos & VFIO_FSL_MC_OFFSET_MASK;
struct fsl_mc_device *mc_dev = vdev->mc_dev;
@@ -404,10 +408,12 @@ static int vfio_fsl_mc_send_command(void __iomem *ioaddr, uint64_t *cmd_data)
return 0;
}
-static ssize_t vfio_fsl_mc_write(void *device_data, const char __user *buf,
- size_t count, loff_t *ppos)
+static ssize_t vfio_fsl_mc_write(struct vfio_device *core_vdev,
+ const char __user *buf, size_t count,
+ loff_t *ppos)
{
- struct vfio_fsl_mc_device *vdev = device_data;
+ struct vfio_fsl_mc_device *vdev =
+ container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
unsigned int index = VFIO_FSL_MC_OFFSET_TO_INDEX(*ppos);
loff_t off = *ppos & VFIO_FSL_MC_OFFSET_MASK;
struct fsl_mc_device *mc_dev = vdev->mc_dev;
@@ -468,9 +474,11 @@ static int vfio_fsl_mc_mmap_mmio(struct vfio_fsl_mc_region region,
size, vma->vm_page_prot);
}
-static int vfio_fsl_mc_mmap(void *device_data, struct vm_area_struct *vma)
+static int vfio_fsl_mc_mmap(struct vfio_device *core_vdev,
+ struct vm_area_struct *vma)
{
- struct vfio_fsl_mc_device *vdev = device_data;
+ struct vfio_fsl_mc_device *vdev =
+ container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
struct fsl_mc_device *mc_dev = vdev->mc_dev;
unsigned int index;
diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
index 4469aaf31b56cb..e7309caa99c71b 100644
--- a/drivers/vfio/mdev/vfio_mdev.c
+++ b/drivers/vfio/mdev/vfio_mdev.c
@@ -25,10 +25,11 @@ struct mdev_vfio_device {
struct vfio_device vdev;
};
-static int vfio_mdev_open(void *device_data)
+static int vfio_mdev_open(struct vfio_device *core_vdev)
{
- struct mdev_device *mdev = device_data;
+ struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
struct mdev_parent *parent = mdev->parent;
+
int ret;
if (unlikely(!parent->ops->open))
@@ -44,9 +45,9 @@ static int vfio_mdev_open(void *device_data)
return ret;
}
-static void vfio_mdev_release(void *device_data)
+static void vfio_mdev_release(struct vfio_device *core_vdev)
{
- struct mdev_device *mdev = device_data;
+ struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
struct mdev_parent *parent = mdev->parent;
if (likely(parent->ops->release))
@@ -55,10 +56,10 @@ static void vfio_mdev_release(void *device_data)
module_put(THIS_MODULE);
}
-static long vfio_mdev_unlocked_ioctl(void *device_data,
+static long vfio_mdev_unlocked_ioctl(struct vfio_device *core_vdev,
unsigned int cmd, unsigned long arg)
{
- struct mdev_device *mdev = device_data;
+ struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
struct mdev_parent *parent = mdev->parent;
if (unlikely(!parent->ops->ioctl))
@@ -67,10 +68,10 @@ static long vfio_mdev_unlocked_ioctl(void *device_data,
return parent->ops->ioctl(mdev, cmd, arg);
}
-static ssize_t vfio_mdev_read(void *device_data, char __user *buf,
+static ssize_t vfio_mdev_read(struct vfio_device *core_vdev, char __user *buf,
size_t count, loff_t *ppos)
{
- struct mdev_device *mdev = device_data;
+ struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
struct mdev_parent *parent = mdev->parent;
if (unlikely(!parent->ops->read))
@@ -79,10 +80,11 @@ static ssize_t vfio_mdev_read(void *device_data, char __user *buf,
return parent->ops->read(mdev, buf, count, ppos);
}
-static ssize_t vfio_mdev_write(void *device_data, const char __user *buf,
- size_t count, loff_t *ppos)
+static ssize_t vfio_mdev_write(struct vfio_device *core_vdev,
+ const char __user *buf, size_t count,
+ loff_t *ppos)
{
- struct mdev_device *mdev = device_data;
+ struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
struct mdev_parent *parent = mdev->parent;
if (unlikely(!parent->ops->write))
@@ -91,9 +93,10 @@ static ssize_t vfio_mdev_write(void *device_data, const char __user *buf,
return parent->ops->write(mdev, buf, count, ppos);
}
-static int vfio_mdev_mmap(void *device_data, struct vm_area_struct *vma)
+static int vfio_mdev_mmap(struct vfio_device *core_vdev,
+ struct vm_area_struct *vma)
{
- struct mdev_device *mdev = device_data;
+ struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
struct mdev_parent *parent = mdev->parent;
if (unlikely(!parent->ops->mmap))
@@ -102,9 +105,9 @@ static int vfio_mdev_mmap(void *device_data, struct vm_area_struct *vma)
return parent->ops->mmap(mdev, vma);
}
-static void vfio_mdev_request(void *device_data, unsigned int count)
+static void vfio_mdev_request(struct vfio_device *core_vdev, unsigned int count)
{
- struct mdev_device *mdev = device_data;
+ struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
struct mdev_parent *parent = mdev->parent;
if (parent->ops->request)
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index a0ac20a499cf6c..5f1a782d1c65ae 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -553,9 +553,10 @@ static void vfio_pci_vf_token_user_add(struct vfio_pci_device *vdev, int val)
vfio_device_put(pf_dev);
}
-static void vfio_pci_release(void *device_data)
+static void vfio_pci_release(struct vfio_device *core_vdev)
{
- struct vfio_pci_device *vdev = device_data;
+ struct vfio_pci_device *vdev =
+ container_of(core_vdev, struct vfio_pci_device, vdev);
mutex_lock(&vdev->reflck->lock);
@@ -581,9 +582,10 @@ static void vfio_pci_release(void *device_data)
module_put(THIS_MODULE);
}
-static int vfio_pci_open(void *device_data)
+static int vfio_pci_open(struct vfio_device *core_vdev)
{
- struct vfio_pci_device *vdev = device_data;
+ struct vfio_pci_device *vdev =
+ container_of(core_vdev, struct vfio_pci_device, vdev);
int ret = 0;
if (!try_module_get(THIS_MODULE))
@@ -797,10 +799,11 @@ struct vfio_devices {
int max_index;
};
-static long vfio_pci_ioctl(void *device_data,
+static long vfio_pci_ioctl(struct vfio_device *core_vdev,
unsigned int cmd, unsigned long arg)
{
- struct vfio_pci_device *vdev = device_data;
+ struct vfio_pci_device *vdev =
+ container_of(core_vdev, struct vfio_pci_device, vdev);
unsigned long minsz;
if (cmd == VFIO_DEVICE_GET_INFO) {
@@ -1402,11 +1405,10 @@ static long vfio_pci_ioctl(void *device_data,
return -ENOTTY;
}
-static ssize_t vfio_pci_rw(void *device_data, char __user *buf,
+static ssize_t vfio_pci_rw(struct vfio_pci_device *vdev, char __user *buf,
size_t count, loff_t *ppos, bool iswrite)
{
unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
- struct vfio_pci_device *vdev = device_data;
if (index >= VFIO_PCI_NUM_REGIONS + vdev->num_regions)
return -EINVAL;
@@ -1434,22 +1436,28 @@ static ssize_t vfio_pci_rw(void *device_data, char __user *buf,
return -EINVAL;
}
-static ssize_t vfio_pci_read(void *device_data, char __user *buf,
+static ssize_t vfio_pci_read(struct vfio_device *core_vdev, char __user *buf,
size_t count, loff_t *ppos)
{
+ struct vfio_pci_device *vdev =
+ container_of(core_vdev, struct vfio_pci_device, vdev);
+
if (!count)
return 0;
- return vfio_pci_rw(device_data, buf, count, ppos, false);
+ return vfio_pci_rw(vdev, buf, count, ppos, false);
}
-static ssize_t vfio_pci_write(void *device_data, const char __user *buf,
+static ssize_t vfio_pci_write(struct vfio_device *core_vdev, const char __user *buf,
size_t count, loff_t *ppos)
{
+ struct vfio_pci_device *vdev =
+ container_of(core_vdev, struct vfio_pci_device, vdev);
+
if (!count)
return 0;
- return vfio_pci_rw(device_data, (char __user *)buf, count, ppos, true);
+ return vfio_pci_rw(vdev, (char __user *)buf, count, ppos, true);
}
/* Return 1 on zap and vma_lock acquired, 0 on contention (only with @try) */
@@ -1646,9 +1654,10 @@ static const struct vm_operations_struct vfio_pci_mmap_ops = {
.fault = vfio_pci_mmap_fault,
};
-static int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma)
+static int vfio_pci_mmap(struct vfio_device *core_vdev, struct vm_area_struct *vma)
{
- struct vfio_pci_device *vdev = device_data;
+ struct vfio_pci_device *vdev =
+ container_of(core_vdev, struct vfio_pci_device, vdev);
struct pci_dev *pdev = vdev->pdev;
unsigned int index;
u64 phys_len, req_len, pgoff, req_start;
@@ -1714,9 +1723,10 @@ static int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma)
return 0;
}
-static void vfio_pci_request(void *device_data, unsigned int count)
+static void vfio_pci_request(struct vfio_device *core_vdev, unsigned int count)
{
- struct vfio_pci_device *vdev = device_data;
+ struct vfio_pci_device *vdev =
+ container_of(core_vdev, struct vfio_pci_device, vdev);
struct pci_dev *pdev = vdev->pdev;
mutex_lock(&vdev->igate);
@@ -1830,9 +1840,10 @@ static int vfio_pci_validate_vf_token(struct vfio_pci_device *vdev,
#define VF_TOKEN_ARG "vf_token="
-static int vfio_pci_match(void *device_data, char *buf)
+static int vfio_pci_match(struct vfio_device *core_vdev, char *buf)
{
- struct vfio_pci_device *vdev = device_data;
+ struct vfio_pci_device *vdev =
+ container_of(core_vdev, struct vfio_pci_device, vdev);
bool vf_token = false;
uuid_t uuid;
int ret;
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 6eb749250ee41c..f5f6b537084a67 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -218,9 +218,10 @@ static int vfio_platform_call_reset(struct vfio_platform_device *vdev,
return -EINVAL;
}
-static void vfio_platform_release(void *device_data)
+static void vfio_platform_release(struct vfio_device *core_vdev)
{
- struct vfio_platform_device *vdev = device_data;
+ struct vfio_platform_device *vdev =
+ container_of(core_vdev, struct vfio_platform_device, vdev);
mutex_lock(&driver_lock);
@@ -244,9 +245,10 @@ static void vfio_platform_release(void *device_data)
module_put(vdev->parent_module);
}
-static int vfio_platform_open(void *device_data)
+static int vfio_platform_open(struct vfio_device *core_vdev)
{
- struct vfio_platform_device *vdev = device_data;
+ struct vfio_platform_device *vdev =
+ container_of(core_vdev, struct vfio_platform_device, vdev);
int ret;
if (!try_module_get(vdev->parent_module))
@@ -293,10 +295,12 @@ static int vfio_platform_open(void *device_data)
return ret;
}
-static long vfio_platform_ioctl(void *device_data,
+static long vfio_platform_ioctl(struct vfio_device *core_vdev,
unsigned int cmd, unsigned long arg)
{
- struct vfio_platform_device *vdev = device_data;
+ struct vfio_platform_device *vdev =
+ container_of(core_vdev, struct vfio_platform_device, vdev);
+
unsigned long minsz;
if (cmd == VFIO_DEVICE_GET_INFO) {
@@ -455,10 +459,11 @@ static ssize_t vfio_platform_read_mmio(struct vfio_platform_region *reg,
return -EFAULT;
}
-static ssize_t vfio_platform_read(void *device_data, char __user *buf,
- size_t count, loff_t *ppos)
+static ssize_t vfio_platform_read(struct vfio_device *core_vdev,
+ char __user *buf, size_t count, loff_t *ppos)
{
- struct vfio_platform_device *vdev = device_data;
+ struct vfio_platform_device *vdev =
+ container_of(core_vdev, struct vfio_platform_device, vdev);
unsigned int index = VFIO_PLATFORM_OFFSET_TO_INDEX(*ppos);
loff_t off = *ppos & VFIO_PLATFORM_OFFSET_MASK;
@@ -531,10 +536,11 @@ static ssize_t vfio_platform_write_mmio(struct vfio_platform_region *reg,
return -EFAULT;
}
-static ssize_t vfio_platform_write(void *device_data, const char __user *buf,
+static ssize_t vfio_platform_write(struct vfio_device *core_vdev, const char __user *buf,
size_t count, loff_t *ppos)
{
- struct vfio_platform_device *vdev = device_data;
+ struct vfio_platform_device *vdev =
+ container_of(core_vdev, struct vfio_platform_device, vdev);
unsigned int index = VFIO_PLATFORM_OFFSET_TO_INDEX(*ppos);
loff_t off = *ppos & VFIO_PLATFORM_OFFSET_MASK;
@@ -573,9 +579,10 @@ static int vfio_platform_mmap_mmio(struct vfio_platform_region region,
req_len, vma->vm_page_prot);
}
-static int vfio_platform_mmap(void *device_data, struct vm_area_struct *vma)
+static int vfio_platform_mmap(struct vfio_device *core_vdev, struct vm_area_struct *vma)
{
- struct vfio_platform_device *vdev = device_data;
+ struct vfio_platform_device *vdev =
+ container_of(core_vdev, struct vfio_platform_device, vdev);
unsigned int index;
index = vma->vm_pgoff >> (VFIO_PLATFORM_OFFSET_SHIFT - PAGE_SHIFT);
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 2d6d7cc1d1ebf9..01de47d1810b6b 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -832,7 +832,7 @@ static struct vfio_device *vfio_device_get_from_name(struct vfio_group *group,
int ret;
if (it->ops->match) {
- ret = it->ops->match(it->device_data, buf);
+ ret = it->ops->match(it, buf);
if (ret < 0) {
device = ERR_PTR(ret);
break;
@@ -893,7 +893,7 @@ void vfio_unregister_group_dev(struct vfio_device *device)
rc = try_wait_for_completion(&device->comp);
while (rc <= 0) {
if (device->ops->request)
- device->ops->request(device->device_data, i++);
+ device->ops->request(device, i++);
if (interrupted) {
rc = wait_for_completion_timeout(&device->comp,
@@ -1379,7 +1379,7 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
if (IS_ERR(device))
return PTR_ERR(device);
- ret = device->ops->open(device->device_data);
+ ret = device->ops->open(device);
if (ret) {
vfio_device_put(device);
return ret;
@@ -1391,7 +1391,7 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
*/
ret = get_unused_fd_flags(O_CLOEXEC);
if (ret < 0) {
- device->ops->release(device->device_data);
+ device->ops->release(device);
vfio_device_put(device);
return ret;
}
@@ -1401,7 +1401,7 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
if (IS_ERR(filep)) {
put_unused_fd(ret);
ret = PTR_ERR(filep);
- device->ops->release(device->device_data);
+ device->ops->release(device);
vfio_device_put(device);
return ret;
}
@@ -1558,7 +1558,7 @@ static int vfio_device_fops_release(struct inode *inode, struct file *filep)
{
struct vfio_device *device = filep->private_data;
- device->ops->release(device->device_data);
+ device->ops->release(device);
vfio_group_try_dissolve_container(device->group);
@@ -1575,7 +1575,7 @@ static long vfio_device_fops_unl_ioctl(struct file *filep,
if (unlikely(!device->ops->ioctl))
return -EINVAL;
- return device->ops->ioctl(device->device_data, cmd, arg);
+ return device->ops->ioctl(device, cmd, arg);
}
static ssize_t vfio_device_fops_read(struct file *filep, char __user *buf,
@@ -1586,7 +1586,7 @@ static ssize_t vfio_device_fops_read(struct file *filep, char __user *buf,
if (unlikely(!device->ops->read))
return -EINVAL;
- return device->ops->read(device->device_data, buf, count, ppos);
+ return device->ops->read(device, buf, count, ppos);
}
static ssize_t vfio_device_fops_write(struct file *filep,
@@ -1598,7 +1598,7 @@ static ssize_t vfio_device_fops_write(struct file *filep,
if (unlikely(!device->ops->write))
return -EINVAL;
- return device->ops->write(device->device_data, buf, count, ppos);
+ return device->ops->write(device, buf, count, ppos);
}
static int vfio_device_fops_mmap(struct file *filep, struct vm_area_struct *vma)
@@ -1608,7 +1608,7 @@ static int vfio_device_fops_mmap(struct file *filep, struct vm_area_struct *vma)
if (unlikely(!device->ops->mmap))
return -EINVAL;
- return device->ops->mmap(device->device_data, vma);
+ return device->ops->mmap(device, vma);
}
static const struct file_operations vfio_device_fops = {
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 4995faf51efeae..784c34c0a28763 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -44,17 +44,17 @@ struct vfio_device {
*/
struct vfio_device_ops {
char *name;
- int (*open)(void *device_data);
- void (*release)(void *device_data);
- ssize_t (*read)(void *device_data, char __user *buf,
+ int (*open)(struct vfio_device *vdev);
+ void (*release)(struct vfio_device *vdev);
+ ssize_t (*read)(struct vfio_device *vdev, char __user *buf,
size_t count, loff_t *ppos);
- ssize_t (*write)(void *device_data, const char __user *buf,
+ ssize_t (*write)(struct vfio_device *vdev, const char __user *buf,
size_t count, loff_t *size);
- long (*ioctl)(void *device_data, unsigned int cmd,
+ long (*ioctl)(struct vfio_device *vdev, unsigned int cmd,
unsigned long arg);
- int (*mmap)(void *device_data, struct vm_area_struct *vma);
- void (*request)(void *device_data, unsigned int count);
- int (*match)(void *device_data, char *buf);
+ int (*mmap)(struct vfio_device *vdev, struct vm_area_struct *vma);
+ void (*request)(struct vfio_device *vdev, unsigned int count);
+ int (*match)(struct vfio_device *vdev, char *buf);
};
extern struct iommu_group *vfio_iommu_group_get(struct device *dev);
--
2.30.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 14/14] vfio: Remove device_data from the vfio bus driver API
2021-03-13 0:55 [PATCH v2 00/14] Embed struct vfio_device in all sub-structures Jason Gunthorpe
2021-03-13 0:55 ` [PATCH v2 03/14] vfio: Split creation of a vfio_device into init and register ops Jason Gunthorpe
2021-03-13 0:56 ` [PATCH v2 12/14] vfio: Make vfio_device_ops pass a 'struct vfio_device *' instead of 'void *' Jason Gunthorpe
@ 2021-03-13 0:56 ` Jason Gunthorpe
2021-03-16 8:22 ` Tian, Kevin
` (2 more replies)
2 siblings, 3 replies; 17+ messages in thread
From: Jason Gunthorpe @ 2021-03-13 0:56 UTC (permalink / raw)
To: Alex Williamson, Cornelia Huck, Jonathan Corbet, Diana Craciun,
Eric Auger, kvm, Kirti Wankhede, linux-doc
Cc: Raj, Ashok, Dan Williams, Daniel Vetter, Christoph Hellwig,
Leon Romanovsky, Max Gurtovoy, Tarun Gupta
There are no longer any users, so it can go away. Everything is using
container_of now.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
Documentation/driver-api/vfio.rst | 3 +--
drivers/vfio/fsl-mc/vfio_fsl_mc.c | 5 +++--
drivers/vfio/mdev/vfio_mdev.c | 2 +-
drivers/vfio/pci/vfio_pci.c | 2 +-
drivers/vfio/platform/vfio_platform_common.c | 2 +-
drivers/vfio/vfio.c | 12 +-----------
include/linux/vfio.h | 4 +---
7 files changed, 9 insertions(+), 21 deletions(-)
diff --git a/Documentation/driver-api/vfio.rst b/Documentation/driver-api/vfio.rst
index 3337f337293a32..decc68cb8114ac 100644
--- a/Documentation/driver-api/vfio.rst
+++ b/Documentation/driver-api/vfio.rst
@@ -254,8 +254,7 @@ vfio_unregister_group_dev() respectively::
void vfio_init_group_dev(struct vfio_device *device,
struct device *dev,
- const struct vfio_device_ops *ops,
- void *device_data);
+ const struct vfio_device_ops *ops);
int vfio_register_group_dev(struct vfio_device *device);
void vfio_unregister_group_dev(struct vfio_device *device);
diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
index 023b2222806424..3af3ca59478f94 100644
--- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
+++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
@@ -75,7 +75,8 @@ static int vfio_fsl_mc_reflck_attach(struct vfio_fsl_mc_device *vdev)
goto unlock;
}
- cont_vdev = vfio_device_data(device);
+ cont_vdev =
+ container_of(device, struct vfio_fsl_mc_device, vdev);
if (!cont_vdev || !cont_vdev->reflck) {
vfio_device_put(device);
ret = -ENODEV;
@@ -624,7 +625,7 @@ static int vfio_fsl_mc_probe(struct fsl_mc_device *mc_dev)
goto out_group_put;
}
- vfio_init_group_dev(&vdev->vdev, dev, &vfio_fsl_mc_ops, vdev);
+ vfio_init_group_dev(&vdev->vdev, dev, &vfio_fsl_mc_ops);
vdev->mc_dev = mc_dev;
mutex_init(&vdev->igate);
diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
index e7309caa99c71b..71bd28f976e5af 100644
--- a/drivers/vfio/mdev/vfio_mdev.c
+++ b/drivers/vfio/mdev/vfio_mdev.c
@@ -138,7 +138,7 @@ static int vfio_mdev_probe(struct device *dev)
if (!mvdev)
return -ENOMEM;
- vfio_init_group_dev(&mvdev->vdev, &mdev->dev, &vfio_mdev_dev_ops, mdev);
+ vfio_init_group_dev(&mvdev->vdev, &mdev->dev, &vfio_mdev_dev_ops);
ret = vfio_register_group_dev(&mvdev->vdev);
if (ret) {
kfree(mvdev);
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 1f70387c8afe37..55ef27a15d4d3f 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -2022,7 +2022,7 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
goto out_group_put;
}
- vfio_init_group_dev(&vdev->vdev, &pdev->dev, &vfio_pci_ops, vdev);
+ vfio_init_group_dev(&vdev->vdev, &pdev->dev, &vfio_pci_ops);
vdev->pdev = pdev;
vdev->irq_type = VFIO_PCI_NUM_IRQS;
mutex_init(&vdev->igate);
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index f5f6b537084a67..361e5b57e36932 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -666,7 +666,7 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
struct iommu_group *group;
int ret;
- vfio_init_group_dev(&vdev->vdev, dev, &vfio_platform_ops, vdev);
+ vfio_init_group_dev(&vdev->vdev, dev, &vfio_platform_ops);
ret = vfio_platform_acpi_probe(vdev, dev);
if (ret)
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 01de47d1810b6b..39ea77557ba0c4 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -741,12 +741,11 @@ static int vfio_iommu_group_notifier(struct notifier_block *nb,
* VFIO driver API
*/
void vfio_init_group_dev(struct vfio_device *device, struct device *dev,
- const struct vfio_device_ops *ops, void *device_data)
+ const struct vfio_device_ops *ops)
{
init_completion(&device->comp);
device->dev = dev;
device->ops = ops;
- device->device_data = device_data;
}
EXPORT_SYMBOL_GPL(vfio_init_group_dev);
@@ -851,15 +850,6 @@ static struct vfio_device *vfio_device_get_from_name(struct vfio_group *group,
return device;
}
-/*
- * Caller must hold a reference to the vfio_device
- */
-void *vfio_device_data(struct vfio_device *device)
-{
- return device->device_data;
-}
-EXPORT_SYMBOL_GPL(vfio_device_data);
-
/*
* Decrement the device reference count and wait for the device to be
* removed. Open file descriptors for the device... */
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 784c34c0a28763..a2c5b30e1763ba 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -24,7 +24,6 @@ struct vfio_device {
refcount_t refcount;
struct completion comp;
struct list_head group_next;
- void *device_data;
};
/**
@@ -61,12 +60,11 @@ extern struct iommu_group *vfio_iommu_group_get(struct device *dev);
extern void vfio_iommu_group_put(struct iommu_group *group, struct device *dev);
void vfio_init_group_dev(struct vfio_device *device, struct device *dev,
- const struct vfio_device_ops *ops, void *device_data);
+ const struct vfio_device_ops *ops);
int vfio_register_group_dev(struct vfio_device *device);
void vfio_unregister_group_dev(struct vfio_device *device);
extern struct vfio_device *vfio_device_get_from_dev(struct device *dev);
extern void vfio_device_put(struct vfio_device *device);
-extern void *vfio_device_data(struct vfio_device *device);
/* events for the backend driver notify callback */
enum vfio_iommu_notify_type {
--
2.30.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 12/14] vfio: Make vfio_device_ops pass a 'struct vfio_device *' instead of 'void *'
2021-03-13 0:56 ` [PATCH v2 12/14] vfio: Make vfio_device_ops pass a 'struct vfio_device *' instead of 'void *' Jason Gunthorpe
@ 2021-03-15 8:58 ` Christoph Hellwig
2021-03-17 11:33 ` Cornelia Huck
1 sibling, 0 replies; 17+ messages in thread
From: Christoph Hellwig @ 2021-03-15 8:58 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Alex Williamson, Cornelia Huck, Jonathan Corbet, Diana Craciun,
Eric Auger, kvm, Kirti Wankhede, linux-doc, Raj, Ashok,
Dan Williams, Daniel Vetter, Christoph Hellwig, Leon Romanovsky,
Max Gurtovoy, Tarun Gupta
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v2 03/14] vfio: Split creation of a vfio_device into init and register ops
2021-03-13 0:55 ` [PATCH v2 03/14] vfio: Split creation of a vfio_device into init and register ops Jason Gunthorpe
@ 2021-03-16 7:55 ` Tian, Kevin
2021-03-16 13:34 ` Jason Gunthorpe
2021-03-16 12:25 ` Cornelia Huck
` (2 subsequent siblings)
3 siblings, 1 reply; 17+ messages in thread
From: Tian, Kevin @ 2021-03-16 7:55 UTC (permalink / raw)
To: Jason Gunthorpe, Alex Williamson, Cornelia Huck, Jonathan Corbet,
kvm@vger.kernel.org, linux-doc@vger.kernel.org
Cc: Raj, Ashok, Williams, Dan J, Daniel Vetter, Christoph Hellwig,
Leon Romanovsky, Max Gurtovoy, Tarun Gupta, Liu, Yi L
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Saturday, March 13, 2021 8:56 AM
>
> This makes the struct vfio_pci_device part of the public interface so it
> can be used with container_of and so forth, as is typical for a Linux
> subystem.
>
> This is the first step to bring some type-safety to the vfio interface by
> allowing the replacement of 'void *' and 'struct device *' inputs with a
> simple and clear 'struct vfio_pci_device *'
>
> For now the self-allocating vfio_add_group_dev() interface is kept so each
> user can be updated as a separate patch.
>
> The expected usage pattern is
>
> driver core probe() function:
> my_device = kzalloc(sizeof(*mydevice));
> vfio_init_group_dev(&my_device->vdev, dev, ops, mydevice);
> /* other driver specific prep */
> vfio_register_group_dev(&my_device->vdev);
> dev_set_drvdata(my_device);
dev_set_drvdata(dev, my_device);
>
> driver core remove() function:
> my_device = dev_get_drvdata(dev);
> vfio_unregister_group_dev(&my_device->vdev);
> /* other driver specific tear down */
> kfree(my_device);
>
> Allowing the driver to be able to use the drvdata and vifo_device to go
> to/from its own data.
>
> The pattern also makes it clear that vfio_register_group_dev() must be
> last in the sequence, as once it is called the core code can immediately
> start calling ops. The init/register gap is provided to allow for the
> driver to do setup before ops can be called and thus avoid races.
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Liu Yi L <yi.l.liu@intel.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
> Documentation/driver-api/vfio.rst | 31 ++++----
> drivers/vfio/vfio.c | 123 ++++++++++++++++--------------
> include/linux/vfio.h | 16 ++++
> 3 files changed, 98 insertions(+), 72 deletions(-)
>
> diff --git a/Documentation/driver-api/vfio.rst b/Documentation/driver-
> api/vfio.rst
> index f1a4d3c3ba0bb1..d3a02300913a7f 100644
> --- a/Documentation/driver-api/vfio.rst
> +++ b/Documentation/driver-api/vfio.rst
> @@ -249,18 +249,23 @@ VFIO bus driver API
>
> VFIO bus drivers, such as vfio-pci make use of only a few interfaces
> into VFIO core. When devices are bound and unbound to the driver,
> -the driver should call vfio_add_group_dev() and vfio_del_group_dev()
> -respectively::
> -
> - extern int vfio_add_group_dev(struct device *dev,
> - const struct vfio_device_ops *ops,
> - void *device_data);
> -
> - extern void *vfio_del_group_dev(struct device *dev);
> -
> -vfio_add_group_dev() indicates to the core to begin tracking the
> -iommu_group of the specified dev and register the dev as owned by
> -a VFIO bus driver. The driver provides an ops structure for callbacks
> +the driver should call vfio_register_group_dev() and
> +vfio_unregister_group_dev() respectively::
> +
> + void vfio_init_group_dev(struct vfio_device *device,
> + struct device *dev,
> + const struct vfio_device_ops *ops,
> + void *device_data);
> + int vfio_register_group_dev(struct vfio_device *device);
> + void vfio_unregister_group_dev(struct vfio_device *device);
> +
> +The driver should embed the vfio_device in its own structure and call
> +vfio_init_group_dev() to pre-configure it before going to registration.
> +vfio_register_group_dev() indicates to the core to begin tracking the
> +iommu_group of the specified dev and register the dev as owned by a VFIO
> bus
> +driver. Once vfio_register_group_dev() returns it is possible for userspace
> to
> +start accessing the driver, thus the driver should ensure it is completely
> +ready before calling it. The driver provides an ops structure for callbacks
> similar to a file operations structure::
>
> struct vfio_device_ops {
> @@ -276,7 +281,7 @@ similar to a file operations structure::
> };
>
> Each function is passed the device_data that was originally registered
> -in the vfio_add_group_dev() call above. This allows the bus driver
> +in the vfio_register_group_dev() call above. This allows the bus driver
> an easy place to store its opaque, private data. The open/release
> callbacks are issued when a new file descriptor is created for a
> device (via VFIO_GROUP_GET_DEVICE_FD). The ioctl interface provides
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 32660e8a69ae20..cfa06ae3b9018b 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -89,16 +89,6 @@ struct vfio_group {
> struct blocking_notifier_head notifier;
> };
>
> -struct vfio_device {
> - refcount_t refcount;
> - struct completion comp;
> - struct device *dev;
> - const struct vfio_device_ops *ops;
> - struct vfio_group *group;
> - struct list_head group_next;
> - void *device_data;
> -};
> -
> #ifdef CONFIG_VFIO_NOIOMMU
> static bool noiommu __read_mostly;
> module_param_named(enable_unsafe_noiommu_mode,
> @@ -532,35 +522,6 @@ static struct vfio_group
> *vfio_group_get_from_dev(struct device *dev)
> /**
> * Device objects - create, release, get, put, search
> */
> -static
> -struct vfio_device *vfio_group_create_device(struct vfio_group *group,
> - struct device *dev,
> - const struct vfio_device_ops *ops,
> - void *device_data)
> -{
> - struct vfio_device *device;
> -
> - device = kzalloc(sizeof(*device), GFP_KERNEL);
> - if (!device)
> - return ERR_PTR(-ENOMEM);
> -
> - refcount_set(&device->refcount, 1);
> - init_completion(&device->comp);
> - device->dev = dev;
> - /* Our reference on group is moved to the device */
> - device->group = group;
> - device->ops = ops;
> - device->device_data = device_data;
> - dev_set_drvdata(dev, device);
> -
> - mutex_lock(&group->device_lock);
> - list_add(&device->group_next, &group->device_list);
> - group->dev_counter++;
> - mutex_unlock(&group->device_lock);
> -
> - return device;
> -}
> -
> /* Device reference always implies a group reference */
> void vfio_device_put(struct vfio_device *device)
> {
> @@ -779,14 +740,23 @@ static int vfio_iommu_group_notifier(struct
> notifier_block *nb,
> /**
> * VFIO driver API
> */
> -int vfio_add_group_dev(struct device *dev,
> - const struct vfio_device_ops *ops, void *device_data)
> +void vfio_init_group_dev(struct vfio_device *device, struct device *dev,
> + const struct vfio_device_ops *ops, void *device_data)
> +{
> + init_completion(&device->comp);
> + device->dev = dev;
> + device->ops = ops;
> + device->device_data = device_data;
> +}
> +EXPORT_SYMBOL_GPL(vfio_init_group_dev);
> +
> +int vfio_register_group_dev(struct vfio_device *device)
> {
> + struct vfio_device *existing_device;
> struct iommu_group *iommu_group;
> struct vfio_group *group;
> - struct vfio_device *device;
>
> - iommu_group = iommu_group_get(dev);
> + iommu_group = iommu_group_get(device->dev);
> if (!iommu_group)
> return -EINVAL;
>
> @@ -805,21 +775,50 @@ int vfio_add_group_dev(struct device *dev,
> iommu_group_put(iommu_group);
> }
>
> - device = vfio_group_get_device(group, dev);
> - if (device) {
> - dev_WARN(dev, "Device already exists on group %d\n",
> + existing_device = vfio_group_get_device(group, device->dev);
> + if (existing_device) {
> + dev_WARN(device->dev, "Device already exists on
> group %d\n",
> iommu_group_id(iommu_group));
> - vfio_device_put(device);
> + vfio_device_put(existing_device);
> vfio_group_put(group);
> return -EBUSY;
> }
>
> - device = vfio_group_create_device(group, dev, ops, device_data);
> - if (IS_ERR(device)) {
> - vfio_group_put(group);
> - return PTR_ERR(device);
> - }
> + /* Our reference on group is moved to the device */
> + device->group = group;
> +
> + /* Refcounting can't start until the driver calls register */
> + refcount_set(&device->refcount, 1);
> +
> + mutex_lock(&group->device_lock);
> + list_add(&device->group_next, &group->device_list);
> + group->dev_counter++;
> + mutex_unlock(&group->device_lock);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(vfio_register_group_dev);
> +
> +int vfio_add_group_dev(struct device *dev, const struct vfio_device_ops
> *ops,
> + void *device_data)
> +{
> + struct vfio_device *device;
> + int ret;
> +
> + device = kzalloc(sizeof(*device), GFP_KERNEL);
> + if (!device)
> + return -ENOMEM;
> +
> + vfio_init_group_dev(device, dev, ops, device_data);
> + ret = vfio_register_group_dev(device);
> + if (ret)
> + goto err_kfree;
> + dev_set_drvdata(dev, device);
> return 0;
> +
> +err_kfree:
> + kfree(device);
> + return ret;
> }
> EXPORT_SYMBOL_GPL(vfio_add_group_dev);
>
> @@ -887,11 +886,9 @@ EXPORT_SYMBOL_GPL(vfio_device_data);
> /*
> * Decrement the device reference count and wait for the device to be
> * removed. Open file descriptors for the device... */
> -void *vfio_del_group_dev(struct device *dev)
> +void vfio_unregister_group_dev(struct vfio_device *device)
> {
> - struct vfio_device *device = dev_get_drvdata(dev);
> struct vfio_group *group = device->group;
> - void *device_data = device->device_data;
> struct vfio_unbound_dev *unbound;
> unsigned int i = 0;
> bool interrupted = false;
> @@ -908,7 +905,7 @@ void *vfio_del_group_dev(struct device *dev)
> */
> unbound = kzalloc(sizeof(*unbound), GFP_KERNEL);
> if (unbound) {
> - unbound->dev = dev;
> + unbound->dev = device->dev;
> mutex_lock(&group->unbound_lock);
> list_add(&unbound->unbound_next, &group->unbound_list);
> mutex_unlock(&group->unbound_lock);
> @@ -919,7 +916,7 @@ void *vfio_del_group_dev(struct device *dev)
> rc = try_wait_for_completion(&device->comp);
> while (rc <= 0) {
> if (device->ops->request)
> - device->ops->request(device_data, i++);
> + device->ops->request(device->device_data, i++);
>
> if (interrupted) {
> rc = wait_for_completion_timeout(&device->comp,
> @@ -929,7 +926,7 @@ void *vfio_del_group_dev(struct device *dev)
> &device->comp, HZ * 10);
> if (rc < 0) {
> interrupted = true;
> - dev_warn(dev,
> + dev_warn(device->dev,
> "Device is currently in use, task"
> " \"%s\" (%d) "
> "blocked until device is released",
> @@ -962,9 +959,17 @@ void *vfio_del_group_dev(struct device *dev)
>
> /* Matches the get in vfio_group_create_device() */
> vfio_group_put(group);
> +}
> +EXPORT_SYMBOL_GPL(vfio_unregister_group_dev);
> +
> +void *vfio_del_group_dev(struct device *dev)
> +{
> + struct vfio_device *device = dev_get_drvdata(dev);
> + void *device_data = device->device_data;
> +
> + vfio_unregister_group_dev(device);
> dev_set_drvdata(dev, NULL);
Move to vfio_unregister_group_dev? In the cover letter you mentioned
that drvdata is managed by the driver but removed from the core. Looks
it's also the rule obeyed by the following patches.
Thanks
Kevin
> kfree(device);
> -
> return device_data;
> }
> EXPORT_SYMBOL_GPL(vfio_del_group_dev);
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index b7e18bde5aa8b3..ad8b579d67d34a 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -15,6 +15,18 @@
> #include <linux/poll.h>
> #include <uapi/linux/vfio.h>
>
> +struct vfio_device {
> + struct device *dev;
> + const struct vfio_device_ops *ops;
> + struct vfio_group *group;
> +
> + /* Members below here are private, not for driver use */
> + refcount_t refcount;
> + struct completion comp;
> + struct list_head group_next;
> + void *device_data;
> +};
> +
> /**
> * struct vfio_device_ops - VFIO bus driver device callbacks
> *
> @@ -48,11 +60,15 @@ struct vfio_device_ops {
> extern struct iommu_group *vfio_iommu_group_get(struct device *dev);
> extern void vfio_iommu_group_put(struct iommu_group *group, struct
> device *dev);
>
> +void vfio_init_group_dev(struct vfio_device *device, struct device *dev,
> + const struct vfio_device_ops *ops, void
> *device_data);
> +int vfio_register_group_dev(struct vfio_device *device);
> extern int vfio_add_group_dev(struct device *dev,
> const struct vfio_device_ops *ops,
> void *device_data);
>
> extern void *vfio_del_group_dev(struct device *dev);
> +void vfio_unregister_group_dev(struct vfio_device *device);
> extern struct vfio_device *vfio_device_get_from_dev(struct device *dev);
> extern void vfio_device_put(struct vfio_device *device);
> extern void *vfio_device_data(struct vfio_device *device);
> --
> 2.30.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v2 14/14] vfio: Remove device_data from the vfio bus driver API
2021-03-13 0:56 ` [PATCH v2 14/14] vfio: Remove device_data from the vfio bus driver API Jason Gunthorpe
@ 2021-03-16 8:22 ` Tian, Kevin
2021-03-17 12:08 ` Cornelia Huck
2021-03-17 23:24 ` Max Gurtovoy
2 siblings, 0 replies; 17+ messages in thread
From: Tian, Kevin @ 2021-03-16 8:22 UTC (permalink / raw)
To: Jason Gunthorpe, Alex Williamson, Cornelia Huck, Jonathan Corbet,
Diana Craciun, Eric Auger, kvm@vger.kernel.org, Kirti Wankhede,
linux-doc@vger.kernel.org
Cc: Raj, Ashok, Williams, Dan J, Daniel Vetter, Christoph Hellwig,
Leon Romanovsky, Max Gurtovoy, Tarun Gupta
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Saturday, March 13, 2021 8:56 AM
>
> There are no longer any users, so it can go away. Everything is using
> container_of now.
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> ---
> Documentation/driver-api/vfio.rst | 3 +--
> drivers/vfio/fsl-mc/vfio_fsl_mc.c | 5 +++--
> drivers/vfio/mdev/vfio_mdev.c | 2 +-
> drivers/vfio/pci/vfio_pci.c | 2 +-
> drivers/vfio/platform/vfio_platform_common.c | 2 +-
> drivers/vfio/vfio.c | 12 +-----------
> include/linux/vfio.h | 4 +---
> 7 files changed, 9 insertions(+), 21 deletions(-)
>
> diff --git a/Documentation/driver-api/vfio.rst b/Documentation/driver-
> api/vfio.rst
> index 3337f337293a32..decc68cb8114ac 100644
> --- a/Documentation/driver-api/vfio.rst
> +++ b/Documentation/driver-api/vfio.rst
> @@ -254,8 +254,7 @@ vfio_unregister_group_dev() respectively::
>
> void vfio_init_group_dev(struct vfio_device *device,
> struct device *dev,
> - const struct vfio_device_ops *ops,
> - void *device_data);
> + const struct vfio_device_ops *ops);
> int vfio_register_group_dev(struct vfio_device *device);
> void vfio_unregister_group_dev(struct vfio_device *device);
>
> diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c b/drivers/vfio/fsl-
> mc/vfio_fsl_mc.c
> index 023b2222806424..3af3ca59478f94 100644
> --- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
> +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
> @@ -75,7 +75,8 @@ static int vfio_fsl_mc_reflck_attach(struct
> vfio_fsl_mc_device *vdev)
> goto unlock;
> }
>
> - cont_vdev = vfio_device_data(device);
> + cont_vdev =
> + container_of(device, struct vfio_fsl_mc_device, vdev);
> if (!cont_vdev || !cont_vdev->reflck) {
> vfio_device_put(device);
> ret = -ENODEV;
> @@ -624,7 +625,7 @@ static int vfio_fsl_mc_probe(struct fsl_mc_device
> *mc_dev)
> goto out_group_put;
> }
>
> - vfio_init_group_dev(&vdev->vdev, dev, &vfio_fsl_mc_ops, vdev);
> + vfio_init_group_dev(&vdev->vdev, dev, &vfio_fsl_mc_ops);
> vdev->mc_dev = mc_dev;
> mutex_init(&vdev->igate);
>
> diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
> index e7309caa99c71b..71bd28f976e5af 100644
> --- a/drivers/vfio/mdev/vfio_mdev.c
> +++ b/drivers/vfio/mdev/vfio_mdev.c
> @@ -138,7 +138,7 @@ static int vfio_mdev_probe(struct device *dev)
> if (!mvdev)
> return -ENOMEM;
>
> - vfio_init_group_dev(&mvdev->vdev, &mdev->dev,
> &vfio_mdev_dev_ops, mdev);
> + vfio_init_group_dev(&mvdev->vdev, &mdev->dev,
> &vfio_mdev_dev_ops);
> ret = vfio_register_group_dev(&mvdev->vdev);
> if (ret) {
> kfree(mvdev);
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 1f70387c8afe37..55ef27a15d4d3f 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -2022,7 +2022,7 @@ static int vfio_pci_probe(struct pci_dev *pdev,
> const struct pci_device_id *id)
> goto out_group_put;
> }
>
> - vfio_init_group_dev(&vdev->vdev, &pdev->dev, &vfio_pci_ops, vdev);
> + vfio_init_group_dev(&vdev->vdev, &pdev->dev, &vfio_pci_ops);
> vdev->pdev = pdev;
> vdev->irq_type = VFIO_PCI_NUM_IRQS;
> mutex_init(&vdev->igate);
> diff --git a/drivers/vfio/platform/vfio_platform_common.c
> b/drivers/vfio/platform/vfio_platform_common.c
> index f5f6b537084a67..361e5b57e36932 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -666,7 +666,7 @@ int vfio_platform_probe_common(struct
> vfio_platform_device *vdev,
> struct iommu_group *group;
> int ret;
>
> - vfio_init_group_dev(&vdev->vdev, dev, &vfio_platform_ops, vdev);
> + vfio_init_group_dev(&vdev->vdev, dev, &vfio_platform_ops);
>
> ret = vfio_platform_acpi_probe(vdev, dev);
> if (ret)
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 01de47d1810b6b..39ea77557ba0c4 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -741,12 +741,11 @@ static int vfio_iommu_group_notifier(struct
> notifier_block *nb,
> * VFIO driver API
> */
> void vfio_init_group_dev(struct vfio_device *device, struct device *dev,
> - const struct vfio_device_ops *ops, void *device_data)
> + const struct vfio_device_ops *ops)
> {
> init_completion(&device->comp);
> device->dev = dev;
> device->ops = ops;
> - device->device_data = device_data;
> }
> EXPORT_SYMBOL_GPL(vfio_init_group_dev);
>
> @@ -851,15 +850,6 @@ static struct vfio_device
> *vfio_device_get_from_name(struct vfio_group *group,
> return device;
> }
>
> -/*
> - * Caller must hold a reference to the vfio_device
> - */
> -void *vfio_device_data(struct vfio_device *device)
> -{
> - return device->device_data;
> -}
> -EXPORT_SYMBOL_GPL(vfio_device_data);
> -
> /*
> * Decrement the device reference count and wait for the device to be
> * removed. Open file descriptors for the device... */
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index 784c34c0a28763..a2c5b30e1763ba 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -24,7 +24,6 @@ struct vfio_device {
> refcount_t refcount;
> struct completion comp;
> struct list_head group_next;
> - void *device_data;
> };
>
> /**
> @@ -61,12 +60,11 @@ extern struct iommu_group
> *vfio_iommu_group_get(struct device *dev);
> extern void vfio_iommu_group_put(struct iommu_group *group, struct
> device *dev);
>
> void vfio_init_group_dev(struct vfio_device *device, struct device *dev,
> - const struct vfio_device_ops *ops, void
> *device_data);
> + const struct vfio_device_ops *ops);
> int vfio_register_group_dev(struct vfio_device *device);
> void vfio_unregister_group_dev(struct vfio_device *device);
> extern struct vfio_device *vfio_device_get_from_dev(struct device *dev);
> extern void vfio_device_put(struct vfio_device *device);
> -extern void *vfio_device_data(struct vfio_device *device);
>
> /* events for the backend driver notify callback */
> enum vfio_iommu_notify_type {
> --
> 2.30.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 03/14] vfio: Split creation of a vfio_device into init and register ops
2021-03-13 0:55 ` [PATCH v2 03/14] vfio: Split creation of a vfio_device into init and register ops Jason Gunthorpe
2021-03-16 7:55 ` Tian, Kevin
@ 2021-03-16 12:25 ` Cornelia Huck
2021-03-16 21:13 ` Alex Williamson
2021-03-16 12:54 ` Max Gurtovoy
2021-03-18 13:18 ` Auger Eric
3 siblings, 1 reply; 17+ messages in thread
From: Cornelia Huck @ 2021-03-16 12:25 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Alex Williamson, Jonathan Corbet, kvm, linux-doc, Raj, Ashok,
Dan Williams, Daniel Vetter, Christoph Hellwig, Leon Romanovsky,
Max Gurtovoy, Tarun Gupta, Liu Yi L
On Fri, 12 Mar 2021 20:55:55 -0400
Jason Gunthorpe <jgg@nvidia.com> wrote:
> This makes the struct vfio_pci_device part of the public interface so it
> can be used with container_of and so forth, as is typical for a Linux
> subystem.
>
> This is the first step to bring some type-safety to the vfio interface by
> allowing the replacement of 'void *' and 'struct device *' inputs with a
> simple and clear 'struct vfio_pci_device *'
>
> For now the self-allocating vfio_add_group_dev() interface is kept so each
> user can be updated as a separate patch.
>
> The expected usage pattern is
>
> driver core probe() function:
> my_device = kzalloc(sizeof(*mydevice));
> vfio_init_group_dev(&my_device->vdev, dev, ops, mydevice);
> /* other driver specific prep */
> vfio_register_group_dev(&my_device->vdev);
> dev_set_drvdata(my_device);
>
> driver core remove() function:
> my_device = dev_get_drvdata(dev);
> vfio_unregister_group_dev(&my_device->vdev);
> /* other driver specific tear down */
> kfree(my_device);
>
> Allowing the driver to be able to use the drvdata and vifo_device to go
s/vifo_device/vfio_device/
> to/from its own data.
>
> The pattern also makes it clear that vfio_register_group_dev() must be
> last in the sequence, as once it is called the core code can immediately
> start calling ops. The init/register gap is provided to allow for the
> driver to do setup before ops can be called and thus avoid races.
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Liu Yi L <yi.l.liu@intel.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
> Documentation/driver-api/vfio.rst | 31 ++++----
> drivers/vfio/vfio.c | 123 ++++++++++++++++--------------
> include/linux/vfio.h | 16 ++++
> 3 files changed, 98 insertions(+), 72 deletions(-)
>
> diff --git a/Documentation/driver-api/vfio.rst b/Documentation/driver-api/vfio.rst
> index f1a4d3c3ba0bb1..d3a02300913a7f 100644
> --- a/Documentation/driver-api/vfio.rst
> +++ b/Documentation/driver-api/vfio.rst
> @@ -249,18 +249,23 @@ VFIO bus driver API
>
> VFIO bus drivers, such as vfio-pci make use of only a few interfaces
> into VFIO core. When devices are bound and unbound to the driver,
> -the driver should call vfio_add_group_dev() and vfio_del_group_dev()
> -respectively::
> -
> - extern int vfio_add_group_dev(struct device *dev,
> - const struct vfio_device_ops *ops,
> - void *device_data);
> -
> - extern void *vfio_del_group_dev(struct device *dev);
> -
> -vfio_add_group_dev() indicates to the core to begin tracking the
> -iommu_group of the specified dev and register the dev as owned by
> -a VFIO bus driver. The driver provides an ops structure for callbacks
> +the driver should call vfio_register_group_dev() and
> +vfio_unregister_group_dev() respectively::
> +
> + void vfio_init_group_dev(struct vfio_device *device,
> + struct device *dev,
> + const struct vfio_device_ops *ops,
> + void *device_data);
> + int vfio_register_group_dev(struct vfio_device *device);
> + void vfio_unregister_group_dev(struct vfio_device *device);
> +
> +The driver should embed the vfio_device in its own structure and call
> +vfio_init_group_dev() to pre-configure it before going to registration.
s/it/that structure/ (I guess?)
> +vfio_register_group_dev() indicates to the core to begin tracking the
> +iommu_group of the specified dev and register the dev as owned by a VFIO bus
> +driver. Once vfio_register_group_dev() returns it is possible for userspace to
> +start accessing the driver, thus the driver should ensure it is completely
> +ready before calling it. The driver provides an ops structure for callbacks
> similar to a file operations structure::
>
> struct vfio_device_ops {
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 03/14] vfio: Split creation of a vfio_device into init and register ops
2021-03-13 0:55 ` [PATCH v2 03/14] vfio: Split creation of a vfio_device into init and register ops Jason Gunthorpe
2021-03-16 7:55 ` Tian, Kevin
2021-03-16 12:25 ` Cornelia Huck
@ 2021-03-16 12:54 ` Max Gurtovoy
2021-03-18 13:18 ` Auger Eric
3 siblings, 0 replies; 17+ messages in thread
From: Max Gurtovoy @ 2021-03-16 12:54 UTC (permalink / raw)
To: Jason Gunthorpe, Alex Williamson, Cornelia Huck, Jonathan Corbet,
kvm, linux-doc
Cc: Raj, Ashok, Dan Williams, Daniel Vetter, Christoph Hellwig,
Leon Romanovsky, Tarun Gupta, Liu Yi L
On 3/13/2021 2:55 AM, Jason Gunthorpe wrote:
> This makes the struct vfio_pci_device part of the public interface so it
> can be used with container_of and so forth, as is typical for a Linux
> subystem.
>
> This is the first step to bring some type-safety to the vfio interface by
> allowing the replacement of 'void *' and 'struct device *' inputs with a
> simple and clear 'struct vfio_pci_device *'
>
> For now the self-allocating vfio_add_group_dev() interface is kept so each
> user can be updated as a separate patch.
>
> The expected usage pattern is
>
> driver core probe() function:
> my_device = kzalloc(sizeof(*mydevice));
> vfio_init_group_dev(&my_device->vdev, dev, ops, mydevice);
> /* other driver specific prep */
> vfio_register_group_dev(&my_device->vdev);
> dev_set_drvdata(my_device);
>
> driver core remove() function:
> my_device = dev_get_drvdata(dev);
> vfio_unregister_group_dev(&my_device->vdev);
> /* other driver specific tear down */
> kfree(my_device);
>
> Allowing the driver to be able to use the drvdata and vifo_device to go
> to/from its own data.
>
> The pattern also makes it clear that vfio_register_group_dev() must be
> last in the sequence, as once it is called the core code can immediately
> start calling ops. The init/register gap is provided to allow for the
> driver to do setup before ops can be called and thus avoid races.
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Liu Yi L <yi.l.liu@intel.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
> Documentation/driver-api/vfio.rst | 31 ++++----
> drivers/vfio/vfio.c | 123 ++++++++++++++++--------------
> include/linux/vfio.h | 16 ++++
> 3 files changed, 98 insertions(+), 72 deletions(-)
With comments from Cornelia and Kevin, looks good.
Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 03/14] vfio: Split creation of a vfio_device into init and register ops
2021-03-16 7:55 ` Tian, Kevin
@ 2021-03-16 13:34 ` Jason Gunthorpe
2021-03-17 0:55 ` Tian, Kevin
0 siblings, 1 reply; 17+ messages in thread
From: Jason Gunthorpe @ 2021-03-16 13:34 UTC (permalink / raw)
To: Tian, Kevin
Cc: Alex Williamson, Cornelia Huck, Jonathan Corbet,
kvm@vger.kernel.org, linux-doc@vger.kernel.org, Raj, Ashok,
Williams, Dan J, Daniel Vetter, Christoph Hellwig,
Leon Romanovsky, Max Gurtovoy, Tarun Gupta, Liu, Yi L
On Tue, Mar 16, 2021 at 07:55:11AM +0000, Tian, Kevin wrote:
> > +void *vfio_del_group_dev(struct device *dev)
> > +{
> > + struct vfio_device *device = dev_get_drvdata(dev);
> > + void *device_data = device->device_data;
> > +
> > + vfio_unregister_group_dev(device);
> > dev_set_drvdata(dev, NULL);
>
> Move to vfio_unregister_group_dev? In the cover letter you mentioned
> that drvdata is managed by the driver but removed from the core.
"removed from the core" means the core code doesn't touch drvdata at
all.
> Looks it's also the rule obeyed by the following patches.
The dev_set_drvdata(NULL) on remove is mostly cargo-cult nonsense. The
driver core sets it to null immediately after the remove function
returns, so to add another set needs a very strong reason.
It is only left here temporarily, the last patch deletes it.
Jason
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 03/14] vfio: Split creation of a vfio_device into init and register ops
2021-03-16 12:25 ` Cornelia Huck
@ 2021-03-16 21:13 ` Alex Williamson
2021-03-16 23:12 ` Jason Gunthorpe
0 siblings, 1 reply; 17+ messages in thread
From: Alex Williamson @ 2021-03-16 21:13 UTC (permalink / raw)
To: Cornelia Huck
Cc: Jason Gunthorpe, Jonathan Corbet, kvm, linux-doc, Raj, Ashok,
Dan Williams, Daniel Vetter, Christoph Hellwig, Leon Romanovsky,
Max Gurtovoy, Tarun Gupta, Liu Yi L
On Tue, 16 Mar 2021 13:25:59 +0100
Cornelia Huck <cohuck@redhat.com> wrote:
> On Fri, 12 Mar 2021 20:55:55 -0400
> Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> > This makes the struct vfio_pci_device part of the public interface so it
s/_pci//
> > can be used with container_of and so forth, as is typical for a Linux
> > subystem.
> >
> > This is the first step to bring some type-safety to the vfio interface by
> > allowing the replacement of 'void *' and 'struct device *' inputs with a
> > simple and clear 'struct vfio_pci_device *'
s/_pci//
> >
> > For now the self-allocating vfio_add_group_dev() interface is kept so each
> > user can be updated as a separate patch.
> >
> > The expected usage pattern is
> >
> > driver core probe() function:
> > my_device = kzalloc(sizeof(*mydevice));
> > vfio_init_group_dev(&my_device->vdev, dev, ops, mydevice);
> > /* other driver specific prep */
> > vfio_register_group_dev(&my_device->vdev);
> > dev_set_drvdata(my_device);
> >
> > driver core remove() function:
> > my_device = dev_get_drvdata(dev);
> > vfio_unregister_group_dev(&my_device->vdev);
> > /* other driver specific tear down */
> > kfree(my_device);
> >
> > Allowing the driver to be able to use the drvdata and vifo_device to go
>
> s/vifo_device/vfio_device/
>
> > to/from its own data.
> >
> > The pattern also makes it clear that vfio_register_group_dev() must be
> > last in the sequence, as once it is called the core code can immediately
> > start calling ops. The init/register gap is provided to allow for the
> > driver to do setup before ops can be called and thus avoid races.
> >
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > Reviewed-by: Liu Yi L <yi.l.liu@intel.com>
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> > Documentation/driver-api/vfio.rst | 31 ++++----
> > drivers/vfio/vfio.c | 123 ++++++++++++++++--------------
> > include/linux/vfio.h | 16 ++++
> > 3 files changed, 98 insertions(+), 72 deletions(-)
> >
> > diff --git a/Documentation/driver-api/vfio.rst b/Documentation/driver-api/vfio.rst
> > index f1a4d3c3ba0bb1..d3a02300913a7f 100644
> > --- a/Documentation/driver-api/vfio.rst
> > +++ b/Documentation/driver-api/vfio.rst
> > @@ -249,18 +249,23 @@ VFIO bus driver API
> >
> > VFIO bus drivers, such as vfio-pci make use of only a few interfaces
> > into VFIO core. When devices are bound and unbound to the driver,
> > -the driver should call vfio_add_group_dev() and vfio_del_group_dev()
> > -respectively::
> > -
> > - extern int vfio_add_group_dev(struct device *dev,
> > - const struct vfio_device_ops *ops,
> > - void *device_data);
> > -
> > - extern void *vfio_del_group_dev(struct device *dev);
> > -
> > -vfio_add_group_dev() indicates to the core to begin tracking the
> > -iommu_group of the specified dev and register the dev as owned by
> > -a VFIO bus driver. The driver provides an ops structure for callbacks
> > +the driver should call vfio_register_group_dev() and
> > +vfio_unregister_group_dev() respectively::
> > +
> > + void vfio_init_group_dev(struct vfio_device *device,
> > + struct device *dev,
> > + const struct vfio_device_ops *ops,
> > + void *device_data);
> > + int vfio_register_group_dev(struct vfio_device *device);
> > + void vfio_unregister_group_dev(struct vfio_device *device);
> > +
> > +The driver should embed the vfio_device in its own structure and call
> > +vfio_init_group_dev() to pre-configure it before going to registration.
>
> s/it/that structure/ (I guess?)
Seems less clear actually, is the object of "that structure" the
"vfio_device" or "its own structure". Phrasing somewhat suggests the
latter. s/it/the vfio_device structure/ seems excessively verbose. I
think "it" is probably sufficient here. Thanks,
Alex
> > +vfio_register_group_dev() indicates to the core to begin tracking the
> > +iommu_group of the specified dev and register the dev as owned by a VFIO bus
> > +driver. Once vfio_register_group_dev() returns it is possible for userspace to
> > +start accessing the driver, thus the driver should ensure it is completely
> > +ready before calling it. The driver provides an ops structure for callbacks
> > similar to a file operations structure::
> >
> > struct vfio_device_ops {
>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 03/14] vfio: Split creation of a vfio_device into init and register ops
2021-03-16 21:13 ` Alex Williamson
@ 2021-03-16 23:12 ` Jason Gunthorpe
0 siblings, 0 replies; 17+ messages in thread
From: Jason Gunthorpe @ 2021-03-16 23:12 UTC (permalink / raw)
To: Alex Williamson
Cc: Cornelia Huck, Jonathan Corbet, kvm, linux-doc, Raj, Ashok,
Dan Williams, Daniel Vetter, Christoph Hellwig, Leon Romanovsky,
Max Gurtovoy, Tarun Gupta, Liu Yi L
On Tue, Mar 16, 2021 at 03:13:06PM -0600, Alex Williamson wrote:
> > > + void vfio_init_group_dev(struct vfio_device *device,
> > > + struct device *dev,
> > > + const struct vfio_device_ops *ops,
> > > + void *device_data);
> > > + int vfio_register_group_dev(struct vfio_device *device);
> > > + void vfio_unregister_group_dev(struct vfio_device *device);
> > > +
> > > +The driver should embed the vfio_device in its own structure and call
> > > +vfio_init_group_dev() to pre-configure it before going to registration.
> >
> > s/it/that structure/ (I guess?)
>
> Seems less clear actually, is the object of "that structure" the
> "vfio_device" or "its own structure". Phrasing somewhat suggests the
> latter. s/it/the vfio_device structure/ seems excessively verbose. I
> think "it" is probably sufficient here. Thanks,
Right, it says directly above that vfio_init_group_dev() accepts a
vfio_device so I doubt anyone will be confused for long on what "it"
refers to.
I got the other language fixes thanks
Jason
^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v2 03/14] vfio: Split creation of a vfio_device into init and register ops
2021-03-16 13:34 ` Jason Gunthorpe
@ 2021-03-17 0:55 ` Tian, Kevin
0 siblings, 0 replies; 17+ messages in thread
From: Tian, Kevin @ 2021-03-17 0:55 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Alex Williamson, Cornelia Huck, Jonathan Corbet,
kvm@vger.kernel.org, linux-doc@vger.kernel.org, Raj, Ashok,
Williams, Dan J, Daniel Vetter, Christoph Hellwig,
Leon Romanovsky, Max Gurtovoy, Tarun Gupta, Liu, Yi L
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, March 16, 2021 9:34 PM
>
> On Tue, Mar 16, 2021 at 07:55:11AM +0000, Tian, Kevin wrote:
>
> > > +void *vfio_del_group_dev(struct device *dev)
> > > +{
> > > + struct vfio_device *device = dev_get_drvdata(dev);
> > > + void *device_data = device->device_data;
> > > +
> > > + vfio_unregister_group_dev(device);
> > > dev_set_drvdata(dev, NULL);
> >
> > Move to vfio_unregister_group_dev? In the cover letter you mentioned
> > that drvdata is managed by the driver but removed from the core.
>
> "removed from the core" means the core code doesn't touch drvdata at
> all.
>
> > Looks it's also the rule obeyed by the following patches.
>
> The dev_set_drvdata(NULL) on remove is mostly cargo-cult nonsense. The
> driver core sets it to null immediately after the remove function
> returns, so to add another set needs a very strong reason.
>
> It is only left here temporarily, the last patch deletes it.
>
Ah, I didn't realize dev_set_drvdata(NULL) is nonsense here. Just saw
no place clears it after this series.
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 12/14] vfio: Make vfio_device_ops pass a 'struct vfio_device *' instead of 'void *'
2021-03-13 0:56 ` [PATCH v2 12/14] vfio: Make vfio_device_ops pass a 'struct vfio_device *' instead of 'void *' Jason Gunthorpe
2021-03-15 8:58 ` Christoph Hellwig
@ 2021-03-17 11:33 ` Cornelia Huck
1 sibling, 0 replies; 17+ messages in thread
From: Cornelia Huck @ 2021-03-17 11:33 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Alex Williamson, Jonathan Corbet, Diana Craciun, Eric Auger, kvm,
Kirti Wankhede, linux-doc, Raj, Ashok, Dan Williams,
Daniel Vetter, Christoph Hellwig, Leon Romanovsky, Max Gurtovoy,
Tarun Gupta
On Fri, 12 Mar 2021 20:56:04 -0400
Jason Gunthorpe <jgg@nvidia.com> wrote:
> This is the standard kernel pattern, the ops associated with a struct get
> the struct pointer in for typesafety. The expected design is to use
> container_of to cleanly go from the subsystem level type to the driver
> level type without having any type erasure in a void *.
>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
> Documentation/driver-api/vfio.rst | 18 ++++----
> drivers/vfio/fsl-mc/vfio_fsl_mc.c | 36 +++++++++------
> drivers/vfio/mdev/vfio_mdev.c | 33 +++++++-------
> drivers/vfio/pci/vfio_pci.c | 47 ++++++++++++--------
> drivers/vfio/platform/vfio_platform_common.c | 33 ++++++++------
> drivers/vfio/vfio.c | 20 ++++-----
> include/linux/vfio.h | 16 +++----
> 7 files changed, 117 insertions(+), 86 deletions(-)
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 14/14] vfio: Remove device_data from the vfio bus driver API
2021-03-13 0:56 ` [PATCH v2 14/14] vfio: Remove device_data from the vfio bus driver API Jason Gunthorpe
2021-03-16 8:22 ` Tian, Kevin
@ 2021-03-17 12:08 ` Cornelia Huck
2021-03-17 23:24 ` Max Gurtovoy
2 siblings, 0 replies; 17+ messages in thread
From: Cornelia Huck @ 2021-03-17 12:08 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Alex Williamson, Jonathan Corbet, Diana Craciun, Eric Auger, kvm,
Kirti Wankhede, linux-doc, Raj, Ashok, Dan Williams,
Daniel Vetter, Christoph Hellwig, Leon Romanovsky, Max Gurtovoy,
Tarun Gupta
On Fri, 12 Mar 2021 20:56:06 -0400
Jason Gunthorpe <jgg@nvidia.com> wrote:
> There are no longer any users, so it can go away. Everything is using
> container_of now.
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
> Documentation/driver-api/vfio.rst | 3 +--
> drivers/vfio/fsl-mc/vfio_fsl_mc.c | 5 +++--
> drivers/vfio/mdev/vfio_mdev.c | 2 +-
> drivers/vfio/pci/vfio_pci.c | 2 +-
> drivers/vfio/platform/vfio_platform_common.c | 2 +-
> drivers/vfio/vfio.c | 12 +-----------
> include/linux/vfio.h | 4 +---
> 7 files changed, 9 insertions(+), 21 deletions(-)
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 14/14] vfio: Remove device_data from the vfio bus driver API
2021-03-13 0:56 ` [PATCH v2 14/14] vfio: Remove device_data from the vfio bus driver API Jason Gunthorpe
2021-03-16 8:22 ` Tian, Kevin
2021-03-17 12:08 ` Cornelia Huck
@ 2021-03-17 23:24 ` Max Gurtovoy
2 siblings, 0 replies; 17+ messages in thread
From: Max Gurtovoy @ 2021-03-17 23:24 UTC (permalink / raw)
To: Jason Gunthorpe, Alex Williamson, Cornelia Huck, Jonathan Corbet,
Diana Craciun, Eric Auger, kvm, Kirti Wankhede, linux-doc
Cc: Raj, Ashok, Dan Williams, Daniel Vetter, Christoph Hellwig,
Leon Romanovsky, Tarun Gupta
On 3/13/2021 2:56 AM, Jason Gunthorpe wrote:
> There are no longer any users, so it can go away. Everything is using
> container_of now.
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
> Documentation/driver-api/vfio.rst | 3 +--
> drivers/vfio/fsl-mc/vfio_fsl_mc.c | 5 +++--
> drivers/vfio/mdev/vfio_mdev.c | 2 +-
> drivers/vfio/pci/vfio_pci.c | 2 +-
> drivers/vfio/platform/vfio_platform_common.c | 2 +-
> drivers/vfio/vfio.c | 12 +-----------
> include/linux/vfio.h | 4 +---
> 7 files changed, 9 insertions(+), 21 deletions(-)
Looks good,
Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 03/14] vfio: Split creation of a vfio_device into init and register ops
2021-03-13 0:55 ` [PATCH v2 03/14] vfio: Split creation of a vfio_device into init and register ops Jason Gunthorpe
` (2 preceding siblings ...)
2021-03-16 12:54 ` Max Gurtovoy
@ 2021-03-18 13:18 ` Auger Eric
3 siblings, 0 replies; 17+ messages in thread
From: Auger Eric @ 2021-03-18 13:18 UTC (permalink / raw)
To: Jason Gunthorpe, Alex Williamson, Cornelia Huck, Jonathan Corbet,
kvm, linux-doc
Cc: Raj, Ashok, Dan Williams, Daniel Vetter, Christoph Hellwig,
Leon Romanovsky, Max Gurtovoy, Tarun Gupta, Liu Yi L
Hi,
On 3/13/21 1:55 AM, Jason Gunthorpe wrote:
> This makes the struct vfio_pci_device part of the public interface so it
> can be used with container_of and so forth, as is typical for a Linux
> subystem.
>
> This is the first step to bring some type-safety to the vfio interface by
> allowing the replacement of 'void *' and 'struct device *' inputs with a
> simple and clear 'struct vfio_pci_device *'
>
> For now the self-allocating vfio_add_group_dev() interface is kept so each
> user can be updated as a separate patch.
>
> The expected usage pattern is
>
> driver core probe() function:
> my_device = kzalloc(sizeof(*mydevice));
> vfio_init_group_dev(&my_device->vdev, dev, ops, mydevice);
> /* other driver specific prep */
> vfio_register_group_dev(&my_device->vdev);
> dev_set_drvdata(my_device);
>
> driver core remove() function:
> my_device = dev_get_drvdata(dev);
> vfio_unregister_group_dev(&my_device->vdev);
> /* other driver specific tear down */
> kfree(my_device);
>
> Allowing the driver to be able to use the drvdata and vifo_device to go
> to/from its own data.
>
> The pattern also makes it clear that vfio_register_group_dev() must be
> last in the sequence, as once it is called the core code can immediately
> start calling ops. The init/register gap is provided to allow for the
> driver to do setup before ops can be called and thus avoid races.
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Liu Yi L <yi.l.liu@intel.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
With previously commit msg and comment fixes,
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Thanks
Eric
> ---
> Documentation/driver-api/vfio.rst | 31 ++++----
> drivers/vfio/vfio.c | 123 ++++++++++++++++--------------
> include/linux/vfio.h | 16 ++++
> 3 files changed, 98 insertions(+), 72 deletions(-)
>
> diff --git a/Documentation/driver-api/vfio.rst b/Documentation/driver-api/vfio.rst
> index f1a4d3c3ba0bb1..d3a02300913a7f 100644
> --- a/Documentation/driver-api/vfio.rst
> +++ b/Documentation/driver-api/vfio.rst
> @@ -249,18 +249,23 @@ VFIO bus driver API
>
> VFIO bus drivers, such as vfio-pci make use of only a few interfaces
> into VFIO core. When devices are bound and unbound to the driver,
> -the driver should call vfio_add_group_dev() and vfio_del_group_dev()
> -respectively::
> -
> - extern int vfio_add_group_dev(struct device *dev,
> - const struct vfio_device_ops *ops,
> - void *device_data);
> -
> - extern void *vfio_del_group_dev(struct device *dev);
> -
> -vfio_add_group_dev() indicates to the core to begin tracking the
> -iommu_group of the specified dev and register the dev as owned by
> -a VFIO bus driver. The driver provides an ops structure for callbacks
> +the driver should call vfio_register_group_dev() and
> +vfio_unregister_group_dev() respectively::
> +
> + void vfio_init_group_dev(struct vfio_device *device,
> + struct device *dev,
> + const struct vfio_device_ops *ops,
> + void *device_data);
> + int vfio_register_group_dev(struct vfio_device *device);
> + void vfio_unregister_group_dev(struct vfio_device *device);
> +
> +The driver should embed the vfio_device in its own structure and call
> +vfio_init_group_dev() to pre-configure it before going to registration.
> +vfio_register_group_dev() indicates to the core to begin tracking the
> +iommu_group of the specified dev and register the dev as owned by a VFIO bus
> +driver. Once vfio_register_group_dev() returns it is possible for userspace to
> +start accessing the driver, thus the driver should ensure it is completely
> +ready before calling it. The driver provides an ops structure for callbacks
> similar to a file operations structure::
>
> struct vfio_device_ops {
> @@ -276,7 +281,7 @@ similar to a file operations structure::
> };
>
> Each function is passed the device_data that was originally registered
> -in the vfio_add_group_dev() call above. This allows the bus driver
> +in the vfio_register_group_dev() call above. This allows the bus driver
> an easy place to store its opaque, private data. The open/release
> callbacks are issued when a new file descriptor is created for a
> device (via VFIO_GROUP_GET_DEVICE_FD). The ioctl interface provides
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 32660e8a69ae20..cfa06ae3b9018b 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -89,16 +89,6 @@ struct vfio_group {
> struct blocking_notifier_head notifier;
> };
>
> -struct vfio_device {
> - refcount_t refcount;
> - struct completion comp;
> - struct device *dev;
> - const struct vfio_device_ops *ops;
> - struct vfio_group *group;
> - struct list_head group_next;
> - void *device_data;
> -};
> -
> #ifdef CONFIG_VFIO_NOIOMMU
> static bool noiommu __read_mostly;
> module_param_named(enable_unsafe_noiommu_mode,
> @@ -532,35 +522,6 @@ static struct vfio_group *vfio_group_get_from_dev(struct device *dev)
> /**
> * Device objects - create, release, get, put, search
> */
> -static
> -struct vfio_device *vfio_group_create_device(struct vfio_group *group,
> - struct device *dev,
> - const struct vfio_device_ops *ops,
> - void *device_data)
> -{
> - struct vfio_device *device;
> -
> - device = kzalloc(sizeof(*device), GFP_KERNEL);
> - if (!device)
> - return ERR_PTR(-ENOMEM);
> -
> - refcount_set(&device->refcount, 1);
> - init_completion(&device->comp);
> - device->dev = dev;
> - /* Our reference on group is moved to the device */
> - device->group = group;
> - device->ops = ops;
> - device->device_data = device_data;
> - dev_set_drvdata(dev, device);
> -
> - mutex_lock(&group->device_lock);
> - list_add(&device->group_next, &group->device_list);
> - group->dev_counter++;
> - mutex_unlock(&group->device_lock);
> -
> - return device;
> -}
> -
> /* Device reference always implies a group reference */
> void vfio_device_put(struct vfio_device *device)
> {
> @@ -779,14 +740,23 @@ static int vfio_iommu_group_notifier(struct notifier_block *nb,
> /**
> * VFIO driver API
> */
> -int vfio_add_group_dev(struct device *dev,
> - const struct vfio_device_ops *ops, void *device_data)
> +void vfio_init_group_dev(struct vfio_device *device, struct device *dev,
> + const struct vfio_device_ops *ops, void *device_data)
> +{
> + init_completion(&device->comp);
> + device->dev = dev;
> + device->ops = ops;
> + device->device_data = device_data;
> +}
> +EXPORT_SYMBOL_GPL(vfio_init_group_dev);
> +
> +int vfio_register_group_dev(struct vfio_device *device)
> {
> + struct vfio_device *existing_device;
> struct iommu_group *iommu_group;
> struct vfio_group *group;
> - struct vfio_device *device;
>
> - iommu_group = iommu_group_get(dev);
> + iommu_group = iommu_group_get(device->dev);
> if (!iommu_group)
> return -EINVAL;
>
> @@ -805,21 +775,50 @@ int vfio_add_group_dev(struct device *dev,
> iommu_group_put(iommu_group);
> }
>
> - device = vfio_group_get_device(group, dev);
> - if (device) {
> - dev_WARN(dev, "Device already exists on group %d\n",
> + existing_device = vfio_group_get_device(group, device->dev);
> + if (existing_device) {
> + dev_WARN(device->dev, "Device already exists on group %d\n",
> iommu_group_id(iommu_group));
> - vfio_device_put(device);
> + vfio_device_put(existing_device);
> vfio_group_put(group);
> return -EBUSY;
> }
>
> - device = vfio_group_create_device(group, dev, ops, device_data);
> - if (IS_ERR(device)) {
> - vfio_group_put(group);
> - return PTR_ERR(device);
> - }
> + /* Our reference on group is moved to the device */
> + device->group = group;
> +
> + /* Refcounting can't start until the driver calls register */
> + refcount_set(&device->refcount, 1);
> +
> + mutex_lock(&group->device_lock);
> + list_add(&device->group_next, &group->device_list);
> + group->dev_counter++;
> + mutex_unlock(&group->device_lock);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(vfio_register_group_dev);
> +
> +int vfio_add_group_dev(struct device *dev, const struct vfio_device_ops *ops,
> + void *device_data)
> +{
> + struct vfio_device *device;
> + int ret;
> +
> + device = kzalloc(sizeof(*device), GFP_KERNEL);
> + if (!device)
> + return -ENOMEM;
> +
> + vfio_init_group_dev(device, dev, ops, device_data);
> + ret = vfio_register_group_dev(device);
> + if (ret)
> + goto err_kfree;
> + dev_set_drvdata(dev, device);
> return 0;
> +
> +err_kfree:
> + kfree(device);
> + return ret;
> }
> EXPORT_SYMBOL_GPL(vfio_add_group_dev);
>
> @@ -887,11 +886,9 @@ EXPORT_SYMBOL_GPL(vfio_device_data);
> /*
> * Decrement the device reference count and wait for the device to be
> * removed. Open file descriptors for the device... */
> -void *vfio_del_group_dev(struct device *dev)
> +void vfio_unregister_group_dev(struct vfio_device *device)
> {
> - struct vfio_device *device = dev_get_drvdata(dev);
> struct vfio_group *group = device->group;
> - void *device_data = device->device_data;
> struct vfio_unbound_dev *unbound;
> unsigned int i = 0;
> bool interrupted = false;
> @@ -908,7 +905,7 @@ void *vfio_del_group_dev(struct device *dev)
> */
> unbound = kzalloc(sizeof(*unbound), GFP_KERNEL);
> if (unbound) {
> - unbound->dev = dev;
> + unbound->dev = device->dev;
> mutex_lock(&group->unbound_lock);
> list_add(&unbound->unbound_next, &group->unbound_list);
> mutex_unlock(&group->unbound_lock);
> @@ -919,7 +916,7 @@ void *vfio_del_group_dev(struct device *dev)
> rc = try_wait_for_completion(&device->comp);
> while (rc <= 0) {
> if (device->ops->request)
> - device->ops->request(device_data, i++);
> + device->ops->request(device->device_data, i++);
>
> if (interrupted) {
> rc = wait_for_completion_timeout(&device->comp,
> @@ -929,7 +926,7 @@ void *vfio_del_group_dev(struct device *dev)
> &device->comp, HZ * 10);
> if (rc < 0) {
> interrupted = true;
> - dev_warn(dev,
> + dev_warn(device->dev,
> "Device is currently in use, task"
> " \"%s\" (%d) "
> "blocked until device is released",
> @@ -962,9 +959,17 @@ void *vfio_del_group_dev(struct device *dev)
>
> /* Matches the get in vfio_group_create_device() */
> vfio_group_put(group);
> +}
> +EXPORT_SYMBOL_GPL(vfio_unregister_group_dev);
> +
> +void *vfio_del_group_dev(struct device *dev)
> +{
> + struct vfio_device *device = dev_get_drvdata(dev);
> + void *device_data = device->device_data;
> +
> + vfio_unregister_group_dev(device);
> dev_set_drvdata(dev, NULL);
> kfree(device);
> -
> return device_data;
> }
> EXPORT_SYMBOL_GPL(vfio_del_group_dev);
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index b7e18bde5aa8b3..ad8b579d67d34a 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -15,6 +15,18 @@
> #include <linux/poll.h>
> #include <uapi/linux/vfio.h>
>
> +struct vfio_device {
> + struct device *dev;
> + const struct vfio_device_ops *ops;
> + struct vfio_group *group;
> +
> + /* Members below here are private, not for driver use */
> + refcount_t refcount;
> + struct completion comp;
> + struct list_head group_next;
> + void *device_data;
> +};
> +
> /**
> * struct vfio_device_ops - VFIO bus driver device callbacks
> *
> @@ -48,11 +60,15 @@ struct vfio_device_ops {
> extern struct iommu_group *vfio_iommu_group_get(struct device *dev);
> extern void vfio_iommu_group_put(struct iommu_group *group, struct device *dev);
>
> +void vfio_init_group_dev(struct vfio_device *device, struct device *dev,
> + const struct vfio_device_ops *ops, void *device_data);
> +int vfio_register_group_dev(struct vfio_device *device);
> extern int vfio_add_group_dev(struct device *dev,
> const struct vfio_device_ops *ops,
> void *device_data);
>
> extern void *vfio_del_group_dev(struct device *dev);
> +void vfio_unregister_group_dev(struct vfio_device *device);
> extern struct vfio_device *vfio_device_get_from_dev(struct device *dev);
> extern void vfio_device_put(struct vfio_device *device);
> extern void *vfio_device_data(struct vfio_device *device);
>
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-03-18 13:19 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-03-13 0:55 [PATCH v2 00/14] Embed struct vfio_device in all sub-structures Jason Gunthorpe
2021-03-13 0:55 ` [PATCH v2 03/14] vfio: Split creation of a vfio_device into init and register ops Jason Gunthorpe
2021-03-16 7:55 ` Tian, Kevin
2021-03-16 13:34 ` Jason Gunthorpe
2021-03-17 0:55 ` Tian, Kevin
2021-03-16 12:25 ` Cornelia Huck
2021-03-16 21:13 ` Alex Williamson
2021-03-16 23:12 ` Jason Gunthorpe
2021-03-16 12:54 ` Max Gurtovoy
2021-03-18 13:18 ` Auger Eric
2021-03-13 0:56 ` [PATCH v2 12/14] vfio: Make vfio_device_ops pass a 'struct vfio_device *' instead of 'void *' Jason Gunthorpe
2021-03-15 8:58 ` Christoph Hellwig
2021-03-17 11:33 ` Cornelia Huck
2021-03-13 0:56 ` [PATCH v2 14/14] vfio: Remove device_data from the vfio bus driver API Jason Gunthorpe
2021-03-16 8:22 ` Tian, Kevin
2021-03-17 12:08 ` Cornelia Huck
2021-03-17 23:24 ` Max Gurtovoy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).